| Message ID | 20220429084253.1085911-3-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Fix some race between sysfb device registration and drivers probe | expand |
Hi Am 29.04.22 um 10:42 schrieb Javier Martinez Canillas: > The DRIVER_FIRMWARE flag denotes that a DRM driver uses a framebuffer > that was initialized and provided by the system firmware for scanout. > > Indicate to the fbdev subsystem that the registered framebuffer is a > FBINFO_MISC_FIRMWARE, so that it can handle accordingly. For example, > wold hot-unplug the associated device if asked to remove conflicting > framebuffers. > > Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > (no changes since v1) > > drivers/gpu/drm/drm_fb_helper.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index d265a73313c9..76dd11888621 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper, > /* don't leak any physical addresses to userspace */ > info->flags |= FBINFO_HIDE_SMEM_START; > > + /* Indicate that the framebuffer is provided by the firmware */ > + if (drm_core_check_feature(dev, DRIVER_FIRMWARE)) > + info->flags |= FBINFO_MISC_FIRMWARE; > + Patches 1 to 3 should be squashed into one before landing. We can do this with DRIVER_FIRMWARE. Alternatively, I'd suggest to we could also used the existing final parameter of drm_fbdev_generic_setup() to pass a flag that designates a firmware device. > /* Need to drop locks to avoid recursive deadlock in > * register_framebuffer. This is ok because the only thing left to do is > * register the fbdev emulation instance in kernel_fb_helper_list. */
Hello Thomas, On 4/29/22 11:14, Thomas Zimmermann wrote: > Hi > > Am 29.04.22 um 10:42 schrieb Javier Martinez Canillas: >> The DRIVER_FIRMWARE flag denotes that a DRM driver uses a framebuffer >> that was initialized and provided by the system firmware for scanout. >> >> Indicate to the fbdev subsystem that the registered framebuffer is a >> FBINFO_MISC_FIRMWARE, so that it can handle accordingly. For example, >> wold hot-unplug the associated device if asked to remove conflicting >> framebuffers. >> >> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> --- >> >> (no changes since v1) >> >> drivers/gpu/drm/drm_fb_helper.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index d265a73313c9..76dd11888621 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper, >> /* don't leak any physical addresses to userspace */ >> info->flags |= FBINFO_HIDE_SMEM_START; >> >> + /* Indicate that the framebuffer is provided by the firmware */ >> + if (drm_core_check_feature(dev, DRIVER_FIRMWARE)) >> + info->flags |= FBINFO_MISC_FIRMWARE; >> + > > Patches 1 to 3 should be squashed into one before landing. > I actually considered this but then decided to go with the each change goes into its own patch approach. But I'll squash it in next revisions. > We can do this with DRIVER_FIRMWARE. Alternatively, I'd suggest to we > could also used the existing final parameter of > drm_fbdev_generic_setup() to pass a flag that designates a firmware device. > By existing final parameter you mean @preferred_bpp ? That doesn't seem correct. I also like that by using DRIVER_FIRMWARE it is completely data driven and transparent to the DRM driver. Or do you envision a case where a driver would be DRIVER_FIRMWARE but we wouldn't want the emulated fbdev to also be FBINFO_MISC_FIRMWARE ?
Hi Javier Am 29.04.22 um 11:23 schrieb Javier Martinez Canillas: > Hello Thomas, > > On 4/29/22 11:14, Thomas Zimmermann wrote: >> Hi >> >> Am 29.04.22 um 10:42 schrieb Javier Martinez Canillas: >>> The DRIVER_FIRMWARE flag denotes that a DRM driver uses a framebuffer >>> that was initialized and provided by the system firmware for scanout. >>> >>> Indicate to the fbdev subsystem that the registered framebuffer is a >>> FBINFO_MISC_FIRMWARE, so that it can handle accordingly. For example, >>> wold hot-unplug the associated device if asked to remove conflicting >>> framebuffers. >>> >>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >>> --- >>> >>> (no changes since v1) >>> >>> drivers/gpu/drm/drm_fb_helper.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >>> index d265a73313c9..76dd11888621 100644 >>> --- a/drivers/gpu/drm/drm_fb_helper.c >>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>> @@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper, >>> /* don't leak any physical addresses to userspace */ >>> info->flags |= FBINFO_HIDE_SMEM_START; >>> >>> + /* Indicate that the framebuffer is provided by the firmware */ >>> + if (drm_core_check_feature(dev, DRIVER_FIRMWARE)) >>> + info->flags |= FBINFO_MISC_FIRMWARE; >>> + >> >> Patches 1 to 3 should be squashed into one before landing. >> > > I actually considered this but then decided to go with the each change > goes into its own patch approach. But I'll squash it in next revisions. > >> We can do this with DRIVER_FIRMWARE. Alternatively, I'd suggest to we >> could also used the existing final parameter of >> drm_fbdev_generic_setup() to pass a flag that designates a firmware device. >> > > By existing final parameter you mean @preferred_bpp ? That doesn't seem > correct. I also like that by using DRIVER_FIRMWARE it is completely data > driven and transparent to the DRM driver. DRIVER_FIRMWARE is an indirection and only used here. (Just like FBINFO_MISC_FIRMWARE is a bad interface for marking framebuffers that can be unplugged.) If a driver supports hot-unplugging, it should simply register itself with aperture helpers, regardless of whether it's a firmware framebuffer or not. Of preferred_bpp, we really only use the lowest byte. All other bits are up for grabbing. The argument is a workaround for handling mode_config.prefered_depth correctly. Eventually, preferred_depth should be replaced by something like 'preferred_format', which will hold the driver's preferred format in 4CC. We won't need preferred_bpp then. So we could turn preferred_bpp into a flags argument. Best regards Thomas > > Or do you envision a case where a driver would be DRIVER_FIRMWARE but we > wouldn't want the emulated fbdev to also be FBINFO_MISC_FIRMWARE ? >
On 4/29/22 12:20, Thomas Zimmermann wrote: > Hi Javier [snip] >> >>> We can do this with DRIVER_FIRMWARE. Alternatively, I'd suggest to we >>> could also used the existing final parameter of >>> drm_fbdev_generic_setup() to pass a flag that designates a firmware device. >>> >> >> By existing final parameter you mean @preferred_bpp ? That doesn't seem >> correct. I also like that by using DRIVER_FIRMWARE it is completely data >> driven and transparent to the DRM driver. > > DRIVER_FIRMWARE is an indirection and only used here. (Just like > FBINFO_MISC_FIRMWARE is a bad interface for marking framebuffers that > can be unplugged.) If a driver supports hot-unplugging, it should simply > register itself with aperture helpers, regardless of whether it's a > firmware framebuffer or not. > That's fair, and if in practice will only be used by one driver (simpledrm) then that would also allow us to drop patches 1 and 2 from this series. IOW, we wouldn't really need a DRIVER_FIRMWARE capability flag. > Of preferred_bpp, we really only use the lowest byte. All other bits are > up for grabbing. The argument is a workaround for handling > mode_config.prefered_depth correctly. > Yeah, but I didn't want to abuse that argument or package data in an int. > Eventually, preferred_depth should be replaced by something like > 'preferred_format', which will hold the driver's preferred format in > 4CC. We won't need preferred_bpp then. So we could turn preferred_bpp > into a flags argument. > That's a good point, maybe we could start from there and do this cleanup as a preparatory change of this series ? Then the patches would only be 1) renaming preferred_bpp (that would be unused at this point) to flags and 2) make simpledrm to set FBDEV_FIRMWARE flag or something like that. Another option is to add a third flags param to drm_fbdev_generic_setup() and make all drivers to set 0 besides simpledrm. That way marking the fb as FBINFO_MISC_FIRMWARE won't be blocked by the preferred_depth cleanup.
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index d265a73313c9..76dd11888621 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper, /* don't leak any physical addresses to userspace */ info->flags |= FBINFO_HIDE_SMEM_START; + /* Indicate that the framebuffer is provided by the firmware */ + if (drm_core_check_feature(dev, DRIVER_FIRMWARE)) + info->flags |= FBINFO_MISC_FIRMWARE; + /* Need to drop locks to avoid recursive deadlock in * register_framebuffer. This is ok because the only thing left to do is * register the fbdev emulation instance in kernel_fb_helper_list. */
The DRIVER_FIRMWARE flag denotes that a DRM driver uses a framebuffer that was initialized and provided by the system firmware for scanout. Indicate to the fbdev subsystem that the registered framebuffer is a FBINFO_MISC_FIRMWARE, so that it can handle accordingly. For example, wold hot-unplug the associated device if asked to remove conflicting framebuffers. Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- (no changes since v1) drivers/gpu/drm/drm_fb_helper.c | 4 ++++ 1 file changed, 4 insertions(+)