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

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

[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@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);
+
 	drm_mode_config_reset(drm);
 	drm_kms_helper_poll_init(drm);
 
-- 
2.36.1.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Javier Martinez Canillas]

Hello Robin,

On 6/14/22 15:04, Robin Murphy wrote:
> 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@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);
> +

Seems you are using an older base, since this function doesn't exist anymore
after commit 603dc7ed917f ("drm/aperture: Inline fbdev conflict helpers into
aperture helpers").

Instead, you should use the drm_aperture_remove_framebuffers() function, i.e:

 +	drm_aperture_remove_framebuffers(false, &hdlcd_driver);

If you do that and re-spin the patch, feel free to add:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robin Murphy]

On 2022-06-14 14:26, Javier Martinez Canillas wrote:
> Hello Robin,
> 
> On 6/14/22 15:04, Robin Murphy wrote:
>> 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@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);
>> +
> 
> Seems you are using an older base, since this function doesn't exist anymore
> after commit 603dc7ed917f ("drm/aperture: Inline fbdev conflict helpers into
> aperture helpers").

Ah, you got me! I'm having to work with a 5.10 kernel at the moment, but 
the randomly-disappearing console had finally sufficiently annoyed me 
into figuring out and fixing it.

> Instead, you should use the drm_aperture_remove_framebuffers() function, i.e:
> 
>   +	drm_aperture_remove_framebuffers(false, &hdlcd_driver);
> 
> If you do that and re-spin the patch, feel free to add:
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks for the advice and review - I'll send a v2 later once I've had 
time to build and boot test 5.19-rc.

Cheers,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1530 bytes --]

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@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().

Best regards
Thomas

>   	drm_mode_config_reset(drm);
>   	drm_kms_helper_poll_init(drm);
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

Cheers,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3000 bytes --]

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@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. :/

Best regards
Thomas

> 
> Cheers,
> Robin.

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1164 bytes --]



Am 15.06.22 um 09:50 schrieb Javier Martinez Canillas:
[...]
>> 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.

Shh! Don't tell anyone.

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Javier Martinez Canillas]

On 6/15/22 09:53, Thomas Zimmermann wrote:
> 
> 
> Am 15.06.22 um 09:50 schrieb Javier Martinez Canillas:
> [...]
>>> 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.
> 
> Shh! Don't tell anyone.
>

:)

What I tried to say is that deciding where to kick out the firmware-provided
framebuffer isn't trivial and would just land the patch as is. At some point
we should probably agree on the best place and audit all the drivers to make
sure that are doing it properly. 

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Liviu Dudau]

On Wed, Jun 15, 2022 at 10:00:52AM +0200, Javier Martinez Canillas wrote:
> On 6/15/22 09:53, Thomas Zimmermann wrote:
> > 
> > 
> > Am 15.06.22 um 09:50 schrieb Javier Martinez Canillas:
> > [...]
> >>> 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.
> > 
> > Shh! Don't tell anyone.
> >
> 
> :)
> 
> What I tried to say is that deciding where to kick out the firmware-provided
> framebuffer isn't trivial and would just land the patch as is. At some point
> we should probably agree on the best place and audit all the drivers to make
> sure that are doing it properly. 

I agree, we should review v2 with the updated API and land the patch if it is
reasonable. Due to my "cleverness" HDLCD and mali-dp are probably the only drivers
that also use the component framework that adds extra complications in terms of
silently not having all dependencies met (you forgot to compile the I2C driver or you
didn't load it as a module), so taking over the efifb framebuffer late is a good
idea.

Best regards,
Liviu

> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel