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.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?
> > >
> > > 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! /
---------------
¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
