drm: hdlcd: Stop failing atomic disable check
diff mbox series

Message ID 57a68c52732f1b463b036168d2d53737821d1087.1550862776.git.robin.murphy@arm.com
State New
Headers show
Series
  • drm: hdlcd: Stop failing atomic disable check
Related show

Commit Message

Robin Murphy Feb. 25, 2019, 2:39 p.m. UTC
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(+)

Comments

Liviu Dudau Feb. 27, 2019, 9:40 a.m. UTC | #1
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
>
Robin Murphy March 19, 2019, 1:14 p.m. UTC | #2
[ +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
>>
>
Liviu Dudau March 19, 2019, 2:49 p.m. UTC | #3
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
> > > 
> >
Robin Murphy March 29, 2019, 6:46 p.m. UTC | #4
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;
  }
Liviu Dudau April 1, 2019, 4:06 p.m. UTC | #5
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
Robin Murphy April 2, 2019, 5:40 p.m. UTC | #6
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.
Liviu Dudau April 3, 2019, 9:29 a.m. UTC | #7
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.

Patch
diff mbox series

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 */