[PATCH] drm: hdlcd: Stop failing atomic disable check

[Liviu Dudau]

On Tue, Mar 19, 2019 at 01:14:54PM +0000, Robin Murphy wrote:
> [ +Sudeep - just FYI ]
> 
> Hi Liviu,
> 
> On 27/02/2019 09:40, Liviu Dudau wrote:
> > Hi Robin,
> > 
> > Sorry for the delay in reviewing this patch, I am drowning a bit this
> > week in meetings :)
> > 
> > On Mon, Feb 25, 2019 at 02:39:13PM +0000, Robin Murphy wrote:
> > > When __drm_atomic_helper_disable_all() tries to commit the disabled
> > > state, we end up in hdlcd_crtc_atomic_check() with a mode clock rate
> > > of 0. If the input clock has a nonzero minimum rate, this fails the
> > > clk_round_rate() check and prevents the CRTC being torn down cleanly.
> > > 
> > > If we're disabling the output, though, then the clock rate should be
> > > pretty much irrelevant, so skip it in that case. The kerneldoc seems
> > > to imply that we probably shouldn't be looking at the rest of the
> > > state anyway if enable=false.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy at arm.com>
> > 
> > Acked-by: Liviu Dudau <liviu.dudau at arm.com>
> > 
> > 
> > I'll pull your patch into my tree but it will be after v5.1-rc1 that
> > I'll send fixes to airlied.
> > 
> > > ---
> > > 
> > > I'm still occasionally trying to get to the bottom of why the HDLCD on
> > > Juno doesn't work properly with recent upstream EDK2 (the Linux driver
> > > thinks it's initialised and taken over, but the hardware stays stuck
> > > displaying the last contents of the EFI framebuffer). I was hoping that
> > > just unbinding and reprobing the HDLCD/TDA998x drivers might help reset
> > > things hard enough to start working again, but sadly no...
> > 
> > I think both HDLCD and Mali DP drivers misbehave when the bootloader
> > enables the device before the Linux driver probes. I'm interested in
> > sorting this one out and it involves talking to the SMMU as well, so
> > I'll get in touch with you outside this thread to see how I can
> > reproduce your EDK2 environment.
> 
> Well, I've had another quick play and to my great surprise this time I
> actually made things work :)
> 
> After making sense of the finer points of the DRM debug infrastructure, it
> seems that what I was seeing was the HDLCD failing to initialise the CRTC
> but then continuing on anyway with the client in some kind of
> half-configured headless state. And the reason for the CRTC failing is in
> fact this same clk_rate check again - turns out it's got nothing to do with
> EFI per se, but in order to use the EFI display I had to update from SCPI to
> SCMI, and therein lies a critical difference between the respective clock
> drivers. When HDLCD asks for 131MHz, scpi_clk_round_rate() was just saying
> "yeah, whatever" (which is presumably also why we hadn't spotted the disable
> problem before either), whereas scmi_clk_round_rate() is coming back with
> 130.89MHz and thus failing the test.
> 
> I assume that SCMI is telling the truth about the real rate here, so I'm not
> sure what the most appropriate solution is - for now I've just hacked it in
> my tree and will keep that alongside the rest of Sudeep's Juno SCMI patches
> that I'm using lcoally.

Hmm, clk_round_rate() is so confusing! It returns a clock rate "rounded"
to the *exact* value that the hardware supports :)

I'm not sure how much SCMI was lying before vs the amount of hidden tuning
that went into the implementation side in SCP in order to match a lot of
common refresh rates, but I can see that we can probably update the
state->adjusted_mode->clock to the value returned by clk_round_rate()
and not fail. Or accept some small delta vs the requested rate instead
of failing.

If you update state->adjusted_mode->clock to the value returned from
clk_round_rate(), do you see any artefacts in the display?

Best regards,
Liviu

> 
> Robin.
> 
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > 
> > > Robin.
> > > 
> > >   drivers/gpu/drm/arm/hdlcd_crtc.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > index e4d67b70244d..30a0d9570b57 100644
> > > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > @@ -193,6 +193,9 @@ static int hdlcd_crtc_atomic_check(struct drm_crtc *crtc,
> > >   	struct drm_display_mode *mode = &state->adjusted_mode;
> > >   	long rate, clk_rate = mode->clock * 1000;
> > > +	if (!state->enable)
> > > +		return 0;
> > > +
> > >   	rate = clk_round_rate(hdlcd->clk, clk_rate);
> > >   	if (rate != clk_rate) {
> > >   		/* clock required by mode not supported by hardware */
> > > -- 
> > > 2.20.1.dirty
> > > 
> > 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯