[PATCH] drm/arm/hdlcd: Take over EFI framebuffer properly

[Javier Martinez Canillas]

On 6/15/22 09:39, Thomas Zimmermann wrote:
> Hi
> 
> Am 14.06.22 um 23:06 schrieb Robin Murphy:
>> On 2022-06-14 14:48, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 14.06.22 um 15:04 schrieb Robin Murphy:
>>>> The Arm Juno board EDK2 port has provided an EFI GOP display via HDLCD0
>>>> for some time now, which works nicely as an early framebuffer. However,
>>>> once the HDLCD driver probes and takes over the hardware, it should
>>>> take over the logical framebuffer as well, otherwise the now-defunct GOP
>>>> device hangs about and virtual console output inevitably disappears into
>>>> the wrong place most of the time.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy at arm.com>
>>>> ---
>>>>   drivers/gpu/drm/arm/hdlcd_drv.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c 
>>>> b/drivers/gpu/drm/arm/hdlcd_drv.c
>>>> index af59077a5481..a5d04884658b 100644
>>>> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
>>>> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
>>>> @@ -331,6 +331,8 @@ static int hdlcd_drm_bind(struct device *dev)
>>>>           goto err_vblank;
>>>>       }
>>>> +    drm_fb_helper_remove_conflicting_framebuffers(NULL, "hdlcd", 
>>>> false);
>>>> +
>>>
>>> In addition to what Javier said, it appears to be too late to call 
>>> this function. If anything her etouches hardware, you might 
>>> accidentally interfere with the EFI-related driver. Rather call it at 
>>> the top of ldlcd_drm_bind().
>>
>> OK, thanks for the info. I mostly just copied the pattern from the 
>> simplest-looking other users (sun4i, tegra, vc4) who all seemed to call 
>> it fairly late, and indeed naively it seemed logical not to do it *too* 
>> early when there's more chance we might fail to bind and leave the user 
>> with no framebuffer at all. In particular, waiting until we've bound the 
>> HDMI encoder seems like a good idea in the case of the Juno board (which 
>> is the only real HDLCD user), as the I2C bus often gets stuck if the 
>> System Control Processor is having a bad day. I also don't believe 
>> there's anything here that would affect efifb more than the fact that 
>> once the DRM CRTC is alive we simply stop scanning out from the region 
>> of memory that efifb is managing, but if it's considered good practice 
>> to do this early then I can certainly make that change too.
> We've been struggling with this a bit. If it works reliably, you're 
> welcome to leave it where it is.
> 
> Historically, most drivers call this function very early. But for error 
> recovery it would be better to do it as late as possible.  Ideally, 
> drivers would first initialize their DRM software state, then kickout 
> the generic driver, and finally take over hardware. But that would 
> require a careful review of each driver. :/
>

We got bug reports on Fedora about regressions caused by the fact that some
programs made the (wrong) assumption that /dev/dri/card0 would be the "main"
display and just hard-coded that path.

But removing the conflicting framebuffers after calling devm_drm_dev_alloc()
breaks this assumption, since the registered device will be /dev/dri/card1.

All this is to say that doing it too late, even if nothing is touching the HW
yet, could still have unexpected consequences across your graphics stack.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat