[PATCH] drm: hdlcd: Stop failing atomic disable check

Tue Apr 2 17:40:56 UTC 2019

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 ;)

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.