| Message ID | 20220503071540.471667-4-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | drm: Allow simpledrm to setup its emulated FB as firmware provided | expand |
On Tue, May 03, 2022 at 09:15:40AM +0200, Javier Martinez Canillas wrote: > Indicate to fbdev subsystem that the registered framebuffer is provided by > the system firmware, so that it can handle accordingly. For example, would > unregister the FB devices if asked to remove the conflicting framebuffers. > > Add a new DRM_FB_FW field to drm_fbdev_generic_setup() options parameter. > Drivers can use this to indicate the FB helper initialization that the FB > registered is provided by the firmware, so it can be configured as such. > > Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > Changes in v3: > - Drop the firmware local variable (Laurent Pinchart). > - Use DRM_FB_OPTION() since DRM_FB_SET_OPTION() got renamed (kernel test robot). Just for the record what I brought up on irc already: FBINFO_MISC_FIRMWARE is purely an internal flag with no uapi impact, and it's only to control whether we nuke this from remove_conflicting_framebuffer or not. Since simpledrm only ever binds against sysfb I think it'd be cleaner to only rely on that, and relegate that entire FBINFO_MISC_FIRMWARE misc hack to the fbdev dungeons and let it quietly wither away there. Also I'm not a huge fan of these midlayer flags in general :-) -Daniel > > drivers/gpu/drm/drm_fb_helper.c | 8 ++++++++ > drivers/gpu/drm/tiny/simpledrm.c | 2 +- > include/drm/drm_fb_helper.h | 10 ++++++++++ > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 52042ba1e4cf..28b21858b726 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 (fb_helper->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. */ > @@ -2512,6 +2516,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { > * > * * DRM_FB_BPP: bits per pixel for the device. If the field is not set, > * @dev->mode_config.preferred_depth is used instead. > + * * DRM_FB_FW: if the framebuffer for the device is provided by the > + * system firmware. > * > * This function sets up generic fbdev emulation for drivers that supports > * dumb buffers with a virtual address and that can be mmap'ed. > @@ -2569,6 +2575,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev, const unsigned int options) > if (!fb_helper->preferred_bpp) > fb_helper->preferred_bpp = 32; > > + fb_helper->firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options); > + > ret = drm_fbdev_client_hotplug(&fb_helper->client); > if (ret) > drm_dbg_kms(dev, "client hotplug ret=%d\n", ret); > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c > index f5b8e864a5cd..f6f1c5e108b2 100644 > --- a/drivers/gpu/drm/tiny/simpledrm.c > +++ b/drivers/gpu/drm/tiny/simpledrm.c > @@ -901,7 +901,7 @@ static int simpledrm_probe(struct platform_device *pdev) > if (ret) > return ret; > > - drm_fbdev_generic_setup(dev, 0); > + drm_fbdev_generic_setup(dev, DRM_FB_OPTION(DRM_FB_FW, 1)); > > return 0; > } > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > index 5fc41cf0c987..5a17af423944 100644 > --- a/include/drm/drm_fb_helper.h > +++ b/include/drm/drm_fb_helper.h > @@ -44,6 +44,7 @@ enum mode_set_atomic { > }; > > #define DRM_FB_BPP_MASK GENMASK(7, 0) > +#define DRM_FB_FW_MASK GENMASK(8, 8) > > /* Using the GNU statement expression extension */ > #define DRM_FB_OPTION(option, value) \ > @@ -197,6 +198,15 @@ struct drm_fb_helper { > * See also: @deferred_setup > */ > int preferred_bpp; > + > + /** > + * @firmware: > + * > + * Set if the driver indicates to the FB helper initialization that the > + * framebuffer for the device being registered is provided by firmware, > + * so that it can pass this on when registering the framebuffer device. > + */ > + bool firmware; > }; > > static inline struct drm_fb_helper * > -- > 2.35.1 >
On 5/4/22 12:58, Daniel Vetter wrote: > On Tue, May 03, 2022 at 09:15:40AM +0200, Javier Martinez Canillas wrote: >> Indicate to fbdev subsystem that the registered framebuffer is provided by >> the system firmware, so that it can handle accordingly. For example, would >> unregister the FB devices if asked to remove the conflicting framebuffers. >> >> Add a new DRM_FB_FW field to drm_fbdev_generic_setup() options parameter. >> Drivers can use this to indicate the FB helper initialization that the FB >> registered is provided by the firmware, so it can be configured as such. >> >> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> >> Changes in v3: >> - Drop the firmware local variable (Laurent Pinchart). >> - Use DRM_FB_OPTION() since DRM_FB_SET_OPTION() got renamed (kernel test robot). > > Just for the record what I brought up on irc already: > > FBINFO_MISC_FIRMWARE is purely an internal flag with no uapi impact, and > it's only to control whether we nuke this from > remove_conflicting_framebuffer or not. Since simpledrm only ever binds > against sysfb I think it'd be cleaner to only rely on that, and relegate That's not actually true. The OF subsystem also registers "simple-framebuffer" devices when there are Device Tree nodes that contain a "simple-framebuffer" compatible string. In that case these pdev will also bind against simpledrm. > that entire FBINFO_MISC_FIRMWARE misc hack to the fbdev dungeons and let > it quietly wither away there. > > Also I'm not a huge fan of these midlayer flags in general :-) And while I agree with you that these midlayer flags are horrible, that is what any other fbdev that makes use of a firmware-provided framebuffer set, so simpledrm emulated fbdev shouldn't be the exception IMO.
On Wed, May 04, 2022 at 01:32:37PM +0200, Javier Martinez Canillas wrote: > On 5/4/22 12:58, Daniel Vetter wrote: > > On Tue, May 03, 2022 at 09:15:40AM +0200, Javier Martinez Canillas wrote: > >> Indicate to fbdev subsystem that the registered framebuffer is provided by > >> the system firmware, so that it can handle accordingly. For example, would > >> unregister the FB devices if asked to remove the conflicting framebuffers. > >> > >> Add a new DRM_FB_FW field to drm_fbdev_generic_setup() options parameter. > >> Drivers can use this to indicate the FB helper initialization that the FB > >> registered is provided by the firmware, so it can be configured as such. > >> > >> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> > >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> > >> Changes in v3: > >> - Drop the firmware local variable (Laurent Pinchart). > >> - Use DRM_FB_OPTION() since DRM_FB_SET_OPTION() got renamed (kernel test robot). > > > > Just for the record what I brought up on irc already: > > > > FBINFO_MISC_FIRMWARE is purely an internal flag with no uapi impact, and > > it's only to control whether we nuke this from > > remove_conflicting_framebuffer or not. Since simpledrm only ever binds > > against sysfb I think it'd be cleaner to only rely on that, and relegate > > That's not actually true. The OF subsystem also registers "simple-framebuffer" > devices when there are Device Tree nodes that contain a "simple-framebuffer" > compatible string. In that case these pdev will also bind against simpledrm. TIL. > > that entire FBINFO_MISC_FIRMWARE misc hack to the fbdev dungeons and let > > it quietly wither away there. > > > > Also I'm not a huge fan of these midlayer flags in general :-) > > And while I agree with you that these midlayer flags are horrible, that is > what any other fbdev that makes use of a firmware-provided framebuffer set, > so simpledrm emulated fbdev shouldn't be the exception IMO. So we discussed this a pile more on irc, and at least my take is that people who run simpledrm but want to combine that with fbdev drivers and expect it to all work nicely we can probably ignore. At least until all this sysfb stuff is nicely unified, and at that point we shouldn't need special flags anymore. -Daniel
Hello Daniel, On 5/5/22 14:11, Daniel Vetter wrote: [snip] >> >> And while I agree with you that these midlayer flags are horrible, that is >> what any other fbdev that makes use of a firmware-provided framebuffer set, >> so simpledrm emulated fbdev shouldn't be the exception IMO. > > So we discussed this a pile more on irc, and at least my take is that > people who run simpledrm but want to combine that with fbdev drivers and > expect it to all work nicely we can probably ignore. At least until all > this sysfb stuff is nicely unified, and at that point we shouldn't need > special flags anymore. I'm OK with this take and happy to just drop this patch-set then. My worry was just that someone could complain that we broke their uncommon setup [0]. [0]: https://xkcd.com/1172/.
On Thu, May 05, 2022 at 02:45:27PM +0200, Javier Martinez Canillas wrote: > Hello Daniel, > > On 5/5/22 14:11, Daniel Vetter wrote: > > [snip] > > >> > >> And while I agree with you that these midlayer flags are horrible, that is > >> what any other fbdev that makes use of a firmware-provided framebuffer set, > >> so simpledrm emulated fbdev shouldn't be the exception IMO. > > > > So we discussed this a pile more on irc, and at least my take is that > > people who run simpledrm but want to combine that with fbdev drivers and > > expect it to all work nicely we can probably ignore. At least until all > > this sysfb stuff is nicely unified, and at that point we shouldn't need > > special flags anymore. > > I'm OK with this take and happy to just drop this patch-set then. My worry > was just that someone could complain that we broke their uncommon setup [0]. > > [0]: https://xkcd.com/1172/. Since you have to explicitly enable simpledrm and that combo never worked, we did not actually break anything. Me, who has over a decade experience legalesing around the "no regression" rule :-) -Daniel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 52042ba1e4cf..28b21858b726 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 (fb_helper->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. */ @@ -2512,6 +2516,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = { * * * DRM_FB_BPP: bits per pixel for the device. If the field is not set, * @dev->mode_config.preferred_depth is used instead. + * * DRM_FB_FW: if the framebuffer for the device is provided by the + * system firmware. * * This function sets up generic fbdev emulation for drivers that supports * dumb buffers with a virtual address and that can be mmap'ed. @@ -2569,6 +2575,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev, const unsigned int options) if (!fb_helper->preferred_bpp) fb_helper->preferred_bpp = 32; + fb_helper->firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options); + ret = drm_fbdev_client_hotplug(&fb_helper->client); if (ret) drm_dbg_kms(dev, "client hotplug ret=%d\n", ret); diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index f5b8e864a5cd..f6f1c5e108b2 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -901,7 +901,7 @@ static int simpledrm_probe(struct platform_device *pdev) if (ret) return ret; - drm_fbdev_generic_setup(dev, 0); + drm_fbdev_generic_setup(dev, DRM_FB_OPTION(DRM_FB_FW, 1)); return 0; } diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 5fc41cf0c987..5a17af423944 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -44,6 +44,7 @@ enum mode_set_atomic { }; #define DRM_FB_BPP_MASK GENMASK(7, 0) +#define DRM_FB_FW_MASK GENMASK(8, 8) /* Using the GNU statement expression extension */ #define DRM_FB_OPTION(option, value) \ @@ -197,6 +198,15 @@ struct drm_fb_helper { * See also: @deferred_setup */ int preferred_bpp; + + /** + * @firmware: + * + * Set if the driver indicates to the FB helper initialization that the + * framebuffer for the device being registered is provided by firmware, + * so that it can pass this on when registering the framebuffer device. + */ + bool firmware; }; static inline struct drm_fb_helper *