Message-ID: <8d82bde2-8861-49e4-9796-3bd74b89194b@ideasonboard.com>
Date:   Thu, 2 Nov 2023 09:33:15 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Aradhya Bhatia <a-bhatia1@...com>,
        Devarsh Thakkar <devarsht@...com>,
        Jyri Sarha <jyri.sarha@....fi>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/10] drm/tidss: Fix dss reset

On 01/11/2023 16:30, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote:
>> The probe function calls dispc_softreset() before runtime PM is enabled
>> and without enabling any of the DSS clocks. This happens to work by
>> luck, and we need to make sure the DSS HW is active and the fclk is
>> enabled.
>>
>> To fix the above, add a new function, dispc_init_hw(), which does:
>>
>> - pm_runtime_set_active()
>> - clk_prepare_enable(fclk)
>> - dispc_softreset().
>>
>> This ensures that the reset can be successfully accomplished.
>>
>> Note that we use pm_runtime_set_active(), not the normal
>> pm_runtime_get(). The reason for this is that at this point we haven't
>> enabled the runtime PM yet and also we don't want the normal resume
>> callback to be called: the dispc resume callback does some initial HW
>> setup, and it expects that the HW was off (no video ports are
>> streaming). If the bootloader has enabled the DSS and has set up a
>> boot time splash-screen, the DSS would be enabled and streaming which
>> might lead to issues with the normal resume callback.
> 
> I think the right way to do this would be, in probe(), to
> 
> - power on the device
> - enable runtime PM, masking the device as active
> - at end of probe, calling pm_runtime_put_autosuspend()

Can you explain what that would accomplish, or why the code in this 
patch is wrong?

If I understand it right, you're suggesting a more "normal" power up at 
the probe time, and then leaving the DSS enabled, but with autosuspend. 
That would require powering up, doing a reset, and calling 
dispc_runtime_resume. Which can be done, but I'm not sure why it's 
better, as we're not interested in "normal" power up at probe time.

But I can see that my approach looks perhaps a bit odd just by looking 
at these patches. This work was related to keeping the bootloader's 
splash screen on the screen for a longer time, i.e. delaying reset.

For that, I wanted an early function (dispc_init_hw) which would, 
instead of always resetting the DSS as it does in this version, peek at 
the DSS hardware, and see if the DSS is already streaming. If no, do a 
reset and proceed normally. If yes, skip the reset, leave the clocks 
enabled, and keep DSS PM active.

Later, when we'd be doing the first modeset, the driver would do the 
initial reset.

So, that's why I wanted an independent function for the HW probing/init, 
which is called before runtime PM is enabled, and I did not want normal 
runtime resume to be called as dispc_runtime_resume() would break the 
display.

I think a better solution would be to set up the fb of tidss's fbdev to 
use the reserved memory, used for the boot splash screen. But I didn't 
figure out a way to do this. But even there we'd like to delay the reset 
until the first modeset (when the fbdev display is getting enabled).

  Tomi

Powered by blists - more mailing lists