| Message ID | 31acd57f4aa8a4d02877026fa3a8c8d035e15a0d.1655309004.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/arm/hdlcd: Take over EFI framebuffer properly | expand |
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@arm.com> Acked-by: Liviu Dudau <liviu.dudau@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 >
Hello Robin, On 6/15/22 18:09, 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. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > > 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. > Yes, v2 looks good to me so feel free to keep my R-b: Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> [snip] > > + /* If EFI left us running, take over from efifb/sysfb */ There are other drivers such as simplefb and simpledrm that also use a simple platform-provided framebuffer. So instead of mentioning all drivers maybe you could just have something like the following ? /* If EFI left us running, take over from simple framebuffer drivers */ And maybe you can also add some words about why you are checking the HDLCD_REG_COMMAND register ? Liviu gave more context in this thread, I believe some of this info could be in the comment.
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);
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. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- 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(+)