| Message ID | 0f96c44b10dcd1444ad43e6027fd5c6aff34e54d.1655211704.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/arm/hdlcd: Take over EFI framebuffer properly | expand |
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>
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.
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); >
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.
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.
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.
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. >
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.
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 >
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);
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(+)