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.mur...@arm.com> > > > > Acked-by: Liviu Dudau <liviu.du...@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! / --------------- ¯\_(ツ)_/¯ _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel