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

[Robin Murphy]

[ +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.

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