| Message ID | 57a68c52732f1b463b036168d2d53737821d1087.1550862776.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: hdlcd: Stop failing atomic disable check | expand |
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. 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 >
[ +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. 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 >> >
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? 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 > > > > >
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; }
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
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@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. 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 ;) 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 Robin.
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@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. > > 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.
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 */
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> --- 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... Robin. drivers/gpu/drm/arm/hdlcd_crtc.c | 3 +++ 1 file changed, 3 insertions(+)