From: Liviu Dudau <Liviu.Dudau@arm.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: "linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
Sudeep Holla <Sudeep.Holla@arm.com>
Subject: Re: [PATCH] drm: hdlcd: Stop failing atomic disable check
Date: Mon, 1 Apr 2019 17:06:39 +0100 [thread overview]
Message-ID: <20190401160639.GV21747@e110455-lin.cambridge.arm.com> (raw)
In-Reply-To: <6f65759d-fbe3-397c-7e42-c74163fd2241@arm.com>
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@arm.com>
> > > >
> > > > Acked-by: Liviu Dudau <liviu.dudau@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.
Best regards,
Liviu
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
====================
| 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
next prev parent reply other threads:[~2019-04-01 16:06 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-25 14:39 [PATCH] drm: hdlcd: Stop failing atomic disable check Robin Murphy
2019-02-27 9:40 ` Liviu Dudau
2019-03-19 13:14 ` Robin Murphy
2019-03-19 14:49 ` Liviu Dudau
2019-03-29 18:46 ` Robin Murphy
2019-04-01 16:06 ` Liviu Dudau [this message]
2019-04-02 17:40 ` Robin Murphy
2019-04-03 9:29 ` Liviu Dudau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190401160639.GV21747@e110455-lin.cambridge.arm.com \
--to=liviu.dudau@arm.com \
--cc=Sudeep.Holla@arm.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=robin.murphy@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).