by Noralf Trønnes

[permalink] [raw]

Subject: Re: [PATCH v2 00/41] drm: Analog TV Improvements

Den 01.09.2022 21.35, skrev Noralf Trønnes:
>
>
> I have finally found a workaround for my kernel hangs.
>
> Dom had a look at my kernel and found that the VideoCore was fine, and
> he said this:
>
>> That suggests cause of lockup was on arm side rather than VC side.
>>
>> But it's hard to diagnose further. Once you've had a peripheral not
>> respond, the AXI bus locks up and no further operations are possible.
>> Usual causes of this are required clocks being stopped or domains
>> disabled and then trying to access the hardware.
>>
>
> So when I got this on my 64-bit build:
>
> [ 166.702171] SError Interrupt on CPU1, code 0x00000000bf000002 -- SError
> [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: G W
> 5.19.0-rc6-00096-gba7973977976-dirty #1
> [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT)
> [ 166.702206] Workqueue: events_freezable_power_ thermal_zone_device_check
> [ 166.702231] pstate: 200000c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28
> [ 166.702261] lr : regmap_mmio_read+0x44/0x70
> ...
> [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal]
>
> I wondered if that reg read was stalled due to a clock being stopped.
>
> Lo and behold, disabling runtime pm and keeping the vec clock running
> all the time fixed it[1].
>
> I don't know what the problem is, but at least I can now test this patchset.
>
> [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065
>

It turns out I didn't have to disable runtime pm:
https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2

Noralf.

2022-09-05 14:36:07

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option

On Fri, Sep 02, 2022 at 12:46:29AM +0200, Mateusz Kwiatkowski wrote:
> > @@ -2212,20 +2239,22 @@ struct drm_named_mode {
> > ??? ?unsigned int xres;
> > ??? ?unsigned int yres;
> > ??? ?unsigned int flags;
> > +?? ?unsigned int tv_mode;
> >? };
>
> Are _all_ named modes supposed to be about analog TV?
>
> If so, then probably this structure should be renamed drm_named_analog_tv_mode
> or something.

I don't think they need to, but it's the only use case we've had so far.
We could also imagine using UHD for 3840x2160 for example, so I wouldn't
say it's limited to analog tv.

> If not, then including tv_mode in all of them sounds almost dangrous. 0 is a
> valid value for enum drm_connector_tv_mode, corresponding to
> DRM_MODE_TV_MODE_NTSC_443. This is a very weird default (maybe it shouldn't be
> the one that has a numeric value of 0?) and if there ever is a named mode that
> is not related to analog TV, it looks that it will refer to NTSC-443.
>
> Not sure where could that actually propagate, and maybe what I'm saying can't
> happen, but I'm imagining weird scenarios where a GPU that has both a
> VGA/HDMI/whatever output, and a composite output, switches to NTSC-443 on the
> composite output by default because a named mode for the modern output is
> selected.

So, named modes are per-connector so the fact that there's another
output doesn't really matter. Then, the answer is quite simple actually,
the HDMI driver wouldn't register and use the TV mode property at all,
so it would completely ignore it, no matter what value it has.

So it's not really a concern.

> Maybe something like DRM_MODE_TV_MODE_NONE = 0 would make sense?

But I guess we can add it still.

Maxime

Attachments:

(No filename) (1.70 kB)
signature.asc (235.00 B)
Download all attachments

2022-09-05 15:16:19

2022-09-24 16:07:03

by Noralf Trønnes

[permalink] [raw]

Subject: Re: [PATCH v2 00/41] drm: Analog TV Improvements

Den 21.09.2022 16.03, skrev Maxime Ripard:
> On Wed, Sep 07, 2022 at 06:44:53PM +0200, Noralf Trønnes wrote:
>>
>>
>> Den 07.09.2022 12.36, skrev Stefan Wahren:
>>> Hi Maxime,
>>>
>>> Am 05.09.22 um 16:57 schrieb Maxime Ripard:
>>>> On Fri, Sep 02, 2022 at 01:28:16PM +0200, Noralf Trønnes wrote:
>>>>>
>>>>> Den 01.09.2022 21.35, skrev Noralf Trønnes:
>>>>>>
>>>>>> I have finally found a workaround for my kernel hangs.
>>>>>>
>>>>>> Dom had a look at my kernel and found that the VideoCore was fine, and
>>>>>> he said this:
>>>>>>
>>>>>>> That suggests cause of lockup was on arm side rather than VC side.
>>>>>>>
>>>>>>> But it's hard to diagnose further. Once you've had a peripheral not
>>>>>>> respond, the AXI bus locks up and no further operations are possible.
>>>>>>> Usual causes of this are required clocks being stopped or domains
>>>>>>> disabled and then trying to access the hardware.
>>>>>>>
>>>>>> So when I got this on my 64-bit build:
>>>>>>
>>>>>> [ 166.702171] SError Interrupt on CPU1, code 0x00000000bf000002 --
>>>>>> SError
>>>>>> [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: G W
>>>>>> 5.19.0-rc6-00096-gba7973977976-dirty #1
>>>>>> [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT)
>>>>>> [ 166.702206] Workqueue: events_freezable_power_
>>>>>> thermal_zone_device_check
>>>>>> [ 166.702231] pstate: 200000c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS
>>>>>> BTYPE=--)
>>>>>> [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28
>>>>>> [ 166.702261] lr : regmap_mmio_read+0x44/0x70
>>>>>> ...
>>>>>> [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal]
>>>>>>
>>>>>> I wondered if that reg read was stalled due to a clock being stopped.
>>>>>>
>>>>>> Lo and behold, disabling runtime pm and keeping the vec clock running
>>>>>> all the time fixed it[1].
>>>>>>
>>>>>> I don't know what the problem is, but at least I can now test this
>>>>>> patchset.
>>>>>>
>>>>>> [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065
>>>>>>
>>>>> It turns out I didn't have to disable runtime pm:
>>>>> https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2
>>>> If the bcm2711_thermal IP needs that clock to be enabled, it should grab
>>>> a reference itself, but it looks like even the device tree binding
>>>> doesn't ask for one.
>>> The missing clock in the device tree binding is expected, because
>>> despite of the code there is not much information about the BCM2711
>>> clock tree. But i'm skeptical that the AVS IP actually needs the VEC
>>> clock, i think it's more likely that the VEC clock parent is changed and
>>> that cause this issue. I could take care of the bcm2711 binding & driver
>>> if i know which clock is really necessary.
>>
>> Seems you're right, keeping the parent always enabled is enough:
>>
>> clk_prepare_enable(clk_get_parent(vec->clock)); // pllc_per
>>
>> I tried enabling just the grandparent clock as well, but that didn't help.
>
> Yeah, adding tracing to the clock framework shows that it indeed
> disables PLLC_PER. So there's probably some other device that depends on
> it but doesn't take a reference to it.
>
> I had a look, but it's not really obvious what that might be.
>
> This patch makes sure that the PLL*_PER are never disabled, could you
> test it? It seems to work for me.
>
>
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 48a1eb9f2d55..3839261ee419 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1675,7 +1675,7 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
> .load_mask = CM_PLLA_LOADPER,
> .hold_mask = CM_PLLA_HOLDPER,
> .fixed_divider = 1,
> - .flags = CLK_SET_RATE_PARENT),
> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
> [BCM2835_PLLA_DSI0] = REGISTER_PLL_DIV(
> SOC_ALL,
> .name = "plla_dsi0",
> @@ -1784,7 +1784,7 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
> .load_mask = CM_PLLC_LOADPER,
> .hold_mask = CM_PLLC_HOLDPER,
> .fixed_divider = 1,
> - .flags = CLK_SET_RATE_PARENT),
> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
>

Yes, this worked, but it's enough to mark pllc_per as critical.

Noralf.

> /*
> * PLLD is the display PLL, used to drive DSI display panels.
> @@ -1891,7 +1891,7 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
> .load_mask = CM_PLLH_LOADAUX,
> .hold_mask = 0,
> .fixed_divider = 1,
> - .flags = CLK_SET_RATE_PARENT),
> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
> [BCM2835_PLLH_PIX] = REGISTER_PLL_DIV(
> SOC_BCM2835,
> .name = "pllh_pix",
>
>
> Maxime