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@xxxxxxx>
> >> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> >> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >> ---
> >>
> >> (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.
--
Regards,
Laurent Pinchart
