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

[Liviu Dudau]

On Wed, Jun 15, 2022 at 05:09:15PM +0100, 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.
> 
> We'll do this after binding the HDMI encoder, since that's the most
> likely thing to fail, and the EFI console is still better than nothing
> when that happens. However, the two HDLCD controllers on Juno are
> independent, and many users will still be using older firmware without
> any display support, so we'll only bother if we find that the HDLCD
> we're probing is already enabled. And if it is, then we'll also stop it,
> since otherwise the display can end up shifted if it's still scanning
> out while the rest of the registers are subsequently reconfigured.

Agree on trying to figure out if the controller is enabled before taking over the
framebuffer, but we're also making the big asssumption (currently true) that EKD2
will only initialise one controller. If that is ever false, we should be looking into
HDLCD_REG_FB_BASE to figure out the start of the firmware framebuffer and request
that via drm_aperture_remove_conflicting_framebuffers(). Not a big deal at the moment
and I think the patch can be merged as is.

What I'm interested to know more is this

> since otherwise the display can end up shifted if it's still scanning
> out while the rest of the registers are subsequently reconfigured.

Now I know that probe time is not atomic and it can have some weird behaviour, but I
was not expecting pixels to shift. Maybe it is because of clock reprogramming?

> 
> Signed-off-by: Robin Murphy <robin.murphy at arm.com>

Acked-by: Liviu Dudau <liviu.dudau at arm.com>

Will merge this into drm-misc-next if/when there are no more comments.

Best regards,
Liviu

> ---
> 
> Since I ended up adding (relatively) a lot here, I didn't want to
> second-guess Javier's opinion so left off the R-b tag from v1.
> 
>  drivers/gpu/drm/arm/hdlcd_drv.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index e89ae0ec60eb..1f1171f2f16a 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -21,6 +21,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  
> +#include <drm/drm_aperture.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_debugfs.h>
> @@ -314,6 +315,12 @@ static int hdlcd_drm_bind(struct device *dev)
>  		goto err_vblank;
>  	}
>  
> +	/* If EFI left us running, take over from efifb/sysfb */
> +	if (hdlcd_read(hdlcd, HDLCD_REG_COMMAND)) {
> +		hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
> +		drm_aperture_remove_framebuffers(false, &hdlcd_driver);
> +	}
> +
>  	drm_mode_config_reset(drm);
>  	drm_kms_helper_poll_init(drm);
>  
> -- 
> 2.36.1.dirty
> 

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