[PATCH] drm: hdlcd: Stop failing atomic disable check
Liviu Dudau
Liviu.Dudau at arm.com
Wed Apr 3 09:29:28 UTC 2019
On Tue, Apr 02, 2019 at 06:40:56PM +0100, Robin Murphy wrote:
> On 01/04/2019 17:06, Liviu Dudau wrote:
> > On Fri, Mar 29, 2019 at 06:46:21PM +0000, Robin Murphy wrote:
> > > On 19/03/2019 14:49, Liviu Dudau wrote:
> > > > 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?
> > >
> > > It doesn't make any noticeable difference, no. FWIW the local diff I have on
> > > top of this patch is now as below.
> > >
> > > Robin.
> > >
> > > ----->8-----
> > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > index f020a4416eb5..1e92c3186708 100644
> > > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > @@ -197,10 +197,12 @@ static int hdlcd_crtc_atomic_check(struct drm_crtc
> > > *crtc,
> > > return 0;
> > >
> > > rate = clk_round_rate(hdlcd->clk, clk_rate);
> > > - if (rate != clk_rate) {
> > > + // 1% seems close enough for my monitor
> > > + if (abs(rate - clk_rate) * 100 > clk_rate) {
> > > /* clock required by mode not supported by hardware */
> > > return -EINVAL;
> > > }
> > > + mode->clock = rate / 1000;
> > >
> > > return 0;
> > > }
> >
> > If you make the comment a bit more generic to explain that SCMI clock
> > driver might return values that are usable within 1% of the mode
> > requested, I would be happy to take the patch.
>
> TBH I still feel it's a bit too hacky to be comfortable with - I did
> briefly try to find some sort of HDMI spec for clock tolerance, but nothing
> jumped out. I just can't help remembering the early Juno days when we were
> collecting different monitors around the office to find models which
> actually worked OK ;)
Yeah, I remember those days as well. However, going for an HDMI spec is
going too far, :) as we don't encode the signals for output, but give the
datastream to the HDMI encoder, so we need to make sure that what we
output is within the encoder's capabilities of sync-ing up to the signal.
I have some fuzzy memories about the way we connected the HDLCD to the
TDA19988 chip and I am under the impression that the encoder needs to
self-generate some sync signals based on the VBLANK and HBLANK lines.
However, TDA19988 seems to be a very forgiving encoder, at some moment during
early bringup of U-Boot Mali DP driver I've managed to get an HD signal
out of the Juno boards without having to program one register in the encoder,
so it probably has quite wide margins for clock rates.
>
> That said, in writing this reply I followed up on another hunch around that
> 130.89MHz, found the datasheet for the PLL and, long story short, my SCP is
> now giving me a precise 131MHz clock when asked, and I have another mail to
> write to the firmware folks about a rounding error :D
Let me know when they fix it as I'm interested in getting the updated SCP firmware
as well.
Best regards,
Liviu
>
> Robin.
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
More information about the dri-devel
mailing list