| Message ID | 20220502153900.408522-4-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | drm: Allow simpledrm to setup its emulated FB as firmware provided | expand |
Hi Javier, Thank you for the patch. On Mon, May 02, 2022 at 05:39:00PM +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> > --- > > (no changes since v1) > > drivers/gpu/drm/drm_fb_helper.c | 9 +++++++++ > drivers/gpu/drm/tiny/simpledrm.c | 2 +- > include/drm/drm_fb_helper.h | 10 ++++++++++ > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index fd0084ad77c3..775e47c5de1f 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. > @@ -2538,6 +2544,7 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options) > { > struct drm_fb_helper *fb_helper; > unsigned int preferred_bpp = DRM_FB_GET_OPTION(DRM_FB_BPP, options); > + bool firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options); > int ret; > > drm_WARN(dev, !dev->registered, "Device has not been registered.\n"); > @@ -2570,6 +2577,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options) > preferred_bpp = 32; > fb_helper->preferred_bpp = preferred_bpp; > > + fb_helper->firmware = firmware; I'd get rid of the local variable and write fb_helper->firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options); Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > 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..5dcc21ea6180 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_SET_OPTION(DRM_FB_FW, 1)); > > return 0; > } > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > index 740f87560102..77a72d55308d 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 *
Hello Laurent, On 5/2/22 18:17, Laurent Pinchart wrote: > Hi Javier, > > Thank you for the patch. > > On Mon, May 02, 2022 at 05:39:00PM +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> >> --- >> >> (no changes since v1) >> >> drivers/gpu/drm/drm_fb_helper.c | 9 +++++++++ >> drivers/gpu/drm/tiny/simpledrm.c | 2 +- >> include/drm/drm_fb_helper.h | 10 ++++++++++ >> 3 files changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index fd0084ad77c3..775e47c5de1f 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. >> @@ -2538,6 +2544,7 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options) >> { >> struct drm_fb_helper *fb_helper; >> unsigned int preferred_bpp = DRM_FB_GET_OPTION(DRM_FB_BPP, options); >> + bool firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options); >> int ret; >> >> drm_WARN(dev, !dev->registered, "Device has not been registered.\n"); >> @@ -2570,6 +2577,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options) >> preferred_bpp = 32; >> fb_helper->preferred_bpp = preferred_bpp; >> >> + fb_helper->firmware = firmware; > > I'd get rid of the local variable and write > I actually considered that but then decided to add a local variable to have both options set in the same place, since I thought that would be easier to read and also consistent with how preferred_bpp is handled. Maybe I could go the other way around and rework patch 2/3 to also not require a preferred_bpp local variable ? That patch won't be as small as it's now though. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Hi Javier, On Mon, May 02, 2022 at 07:09:17PM +0200, Javier Martinez Canillas wrote: > On 5/2/22 18:17, Laurent Pinchart wrote: > > On Mon, May 02, 2022 at 05:39:00PM +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> > >> --- > >> > >> (no changes since v1) > >> > >> drivers/gpu/drm/drm_fb_helper.c | 9 +++++++++ > >> drivers/gpu/drm/tiny/simpledrm.c | 2 +- > >> include/drm/drm_fb_helper.h | 10 ++++++++++ > >> 3 files changed, 20 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > >> index fd0084ad77c3..775e47c5de1f 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. > >> @@ -2538,6 +2544,7 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options) > >> { > >> struct drm_fb_helper *fb_helper; > >> unsigned int preferred_bpp = DRM_FB_GET_OPTION(DRM_FB_BPP, options); > >> + bool firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options); > >> int ret; > >> > >> drm_WARN(dev, !dev->registered, "Device has not been registered.\n"); > >> @@ -2570,6 +2577,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options) > >> preferred_bpp = 32; > >> fb_helper->preferred_bpp = preferred_bpp; > >> > >> + fb_helper->firmware = firmware; > > > > I'd get rid of the local variable and write > > > > I actually considered that but then decided to add a local variable to > have both options set in the same place, since I thought that would be > easier to read and also consistent with how preferred_bpp is handled. > > Maybe I could go the other way around and rework patch 2/3 to also not > require a preferred_bpp local variable ? That patch won't be as small > as it's now though. -- Up to you, or you could ignore the comment, it's minor. If you want to keep the variable, you could also make it const, it's a good practice to show it isn't intended to be modified.
Hello Laurent, On 5/2/22 20:01, Laurent Pinchart wrote: > Hi Javier, [snip] >>>> + fb_helper->firmware = firmware; >>> >>> I'd get rid of the local variable and write >>> >> >> I actually considered that but then decided to add a local variable to >> have both options set in the same place, since I thought that would be >> easier to read and also consistent with how preferred_bpp is handled. >> >> Maybe I could go the other way around and rework patch 2/3 to also not >> require a preferred_bpp local variable ? That patch won't be as small >> as it's now though. -- > > Up to you, or you could ignore the comment, it's minor. If you want to > keep the variable, you could also make it const, it's a good practice to > show it isn't intended to be modified. > Right. I'll also do the same for the preferred_bpp variable in patch 2/3 if I choose to keep them in v3. Thanks again for your feedback and comments!
Hi Javier,
I love your patch! Yet something to improve:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on shawnguo/for-next linus/master linux/master v5.18-rc5 next-20220502]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/drm-Allow-simpledrm-to-setup-its-emulated-FB-as-firmware-provided/20220502-234145
base: git://anongit.freedesktop.org/drm/drm drm-next
config: hexagon-randconfig-r045-20220501 (https://download.01.org/0day-ci/archive/20220503/202205030522.Y2Nq4tz7-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 09325d36061e42b495d1f4c7e933e260eac260ed)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/28ef46724e385165777a21d9f661188fa2577a1e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Javier-Martinez-Canillas/drm-Allow-simpledrm-to-setup-its-emulated-FB-as-firmware-provided/20220502-234145
git checkout 28ef46724e385165777a21d9f661188fa2577a1e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/gpu/drm/tiny/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/gpu/drm/tiny/simpledrm.c:904:31: error: call to undeclared function 'DRM_FB_SET_OPTION'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
drm_fbdev_generic_setup(dev, DRM_FB_SET_OPTION(DRM_FB_FW, 1));
^
>> drivers/gpu/drm/tiny/simpledrm.c:904:49: error: use of undeclared identifier 'DRM_FB_FW'
drm_fbdev_generic_setup(dev, DRM_FB_SET_OPTION(DRM_FB_FW, 1));
^
2 errors generated.
vim +/DRM_FB_SET_OPTION +904 drivers/gpu/drm/tiny/simpledrm.c
884
885 /*
886 * Platform driver
887 */
888
889 static int simpledrm_probe(struct platform_device *pdev)
890 {
891 struct simpledrm_device *sdev;
892 struct drm_device *dev;
893 int ret;
894
895 sdev = simpledrm_device_create(&simpledrm_driver, pdev);
896 if (IS_ERR(sdev))
897 return PTR_ERR(sdev);
898 dev = &sdev->dev;
899
900 ret = drm_dev_register(dev, 0);
901 if (ret)
902 return ret;
903
> 904 drm_fbdev_generic_setup(dev, DRM_FB_SET_OPTION(DRM_FB_FW, 1));
905
906 return 0;
907 }
908
Hi Javier,
I love your patch! Yet something to improve:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on shawnguo/for-next linus/master linux/master v5.18-rc5 next-20220502]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/drm-Allow-simpledrm-to-setup-its-emulated-FB-as-firmware-provided/20220502-234145
base: git://anongit.freedesktop.org/drm/drm drm-next
config: x86_64-randconfig-a011 (https://download.01.org/0day-ci/archive/20220503/202205030810.VwAEOAqj-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/28ef46724e385165777a21d9f661188fa2577a1e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Javier-Martinez-Canillas/drm-Allow-simpledrm-to-setup-its-emulated-FB-as-firmware-provided/20220502-234145
git checkout 28ef46724e385165777a21d9f661188fa2577a1e
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/gpu/drm/tiny/simpledrm.c: In function 'simpledrm_probe':
>> drivers/gpu/drm/tiny/simpledrm.c:904:38: error: implicit declaration of function 'DRM_FB_SET_OPTION'; did you mean 'DRM_FB_GET_OPTION'? [-Werror=implicit-function-declaration]
904 | drm_fbdev_generic_setup(dev, DRM_FB_SET_OPTION(DRM_FB_FW, 1));
| ^~~~~~~~~~~~~~~~~
| DRM_FB_GET_OPTION
>> drivers/gpu/drm/tiny/simpledrm.c:904:56: error: 'DRM_FB_FW' undeclared (first use in this function)
904 | drm_fbdev_generic_setup(dev, DRM_FB_SET_OPTION(DRM_FB_FW, 1));
| ^~~~~~~~~
drivers/gpu/drm/tiny/simpledrm.c:904:56: note: each undeclared identifier is reported only once for each function it appears in
cc1: some warnings being treated as errors
vim +904 drivers/gpu/drm/tiny/simpledrm.c
884
885 /*
886 * Platform driver
887 */
888
889 static int simpledrm_probe(struct platform_device *pdev)
890 {
891 struct simpledrm_device *sdev;
892 struct drm_device *dev;
893 int ret;
894
895 sdev = simpledrm_device_create(&simpledrm_driver, pdev);
896 if (IS_ERR(sdev))
897 return PTR_ERR(sdev);
898 dev = &sdev->dev;
899
900 ret = drm_dev_register(dev, 0);
901 if (ret)
902 return ret;
903
> 904 drm_fbdev_generic_setup(dev, DRM_FB_SET_OPTION(DRM_FB_FW, 1));
905
906 return 0;
907 }
908
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index fd0084ad77c3..775e47c5de1f 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. @@ -2538,6 +2544,7 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options) { struct drm_fb_helper *fb_helper; unsigned int preferred_bpp = DRM_FB_GET_OPTION(DRM_FB_BPP, options); + bool firmware = DRM_FB_GET_OPTION(DRM_FB_FW, options); int ret; drm_WARN(dev, !dev->registered, "Device has not been registered.\n"); @@ -2570,6 +2577,8 @@ void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int options) preferred_bpp = 32; fb_helper->preferred_bpp = preferred_bpp; + fb_helper->firmware = firmware; + 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..5dcc21ea6180 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_SET_OPTION(DRM_FB_FW, 1)); return 0; } diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 740f87560102..77a72d55308d 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 *