Re: [PATCH v5 5/7] drm/i915: Initialize fbdev DRM client with callback functions

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 25.10.23 um 10:36 schrieb Hogander, Jouni:
> Hi Thomas, One minor comment inline below.

Thank you so much for taking the time to review these patches.

> On Wed, 2023-09-27 at 12:26 +0200, Thomas Zimmermann wrote:
> > Initialize i915's fbdev client by giving an instance of struct
> > drm_client_funcs to drm_client_init(). Also clean up with
> > drm_client_release().
> >
> > Doing this in i915 prevents fbdev helpers from initializing and
> > releasing the client internally (see drm_fb_helper_init()). No
> > functional change yet; the client callbacks will be filled later.
> >
> > v2:
> >          * call drm_fb_helper_unprepare() in error handling (Jani)
> >          * fix typo in commit message (Sam)
> >
> > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/display/intel_fbdev.c | 43
> > ++++++++++++++++++++--
> >   1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > index 2695c65b55ddc..d9e69471a782a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > @@ -378,6 +378,7 @@ static void intel_fbdev_destroy(struct
> > intel_fbdev *ifbdev)
> >          if (ifbdev->fb)
> >                  drm_framebuffer_remove(&ifbdev->fb->base);
> >   
> > +       drm_client_release(&ifbdev->helper.client);
> >          drm_fb_helper_unprepare(&ifbdev->helper);
> >          kfree(ifbdev);
> >   }
> > @@ -671,6 +672,30 @@ void intel_fbdev_restore_mode(struct
> > drm_i915_private *dev_priv)
> >                  intel_fbdev_invalidate(ifbdev);
> >   }
> >   
> > +/*
> > + * Fbdev client and struct drm_client_funcs
> > + */
> > +
> > +static void intel_fbdev_client_unregister(struct drm_client_dev
> > *client)
> > +{ }
> > +
> > +static int intel_fbdev_client_restore(struct drm_client_dev *client)
> > +{
> > +       return 0;
> > +}
> > +
> > +static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
> > +{
> > +       return 0;
> > +}
> > +
> > +static const struct drm_client_funcs intel_fbdev_client_funcs = {
> > +       .owner          = THIS_MODULE,
> > +       .unregister     = intel_fbdev_client_unregister,
> > +       .restore        = intel_fbdev_client_restore,
> > +       .hotplug        = intel_fbdev_client_hotplug,
> > +};
> > +
> >   int intel_fbdev_init(struct drm_device *dev)
> >   {
> >          struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -692,16 +717,26 @@ int intel_fbdev_init(struct drm_device *dev)
> >          else
> >                  ifbdev->preferred_bpp = ifbdev->helper.preferred_bpp;
> >   
> > +       ret = drm_client_init(dev, &ifbdev->helper.client, "i915-
> > fbdev",
>
> We are currently working on new driver named as Xe. Due to this it

I've always thought that it's an entirely new driver. But I'm not really 
up-to-date. So the Xe driver is located under i915/ and also shares code 
with the existing i915 driver?

> might actually make sense to use intel-fbdev here rather than i915-
> fbdev.

That change could break user-space programs. See the comment at [1] and 
the commit 842470c4e211 ("Revert "drm/fb-helper: improve DRM fbdev 
emulation device names"").  I'd rather leave the string as it is.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.6/source/drivers/gpu/drm/drm_fb_helper.c#L1755

> BR,
>
> Jouni Högander
>
> > +                             &intel_fbdev_client_funcs);
> > +       if (ret)
> > +               goto err_drm_fb_helper_unprepare;
> > +
> >          ret = drm_fb_helper_init(dev, &ifbdev->helper);
> > -       if (ret) {
> > -               kfree(ifbdev);
> > -               return ret;
> > -       }
> > +       if (ret)
> > +               goto err_drm_client_release;
> >   
> >          dev_priv->display.fbdev.fbdev = ifbdev;
> >          INIT_WORK(&dev_priv->display.fbdev.suspend_work,
> > intel_fbdev_suspend_worker);
> >   
> >          return 0;
> > +
> > +err_drm_client_release:
> > +       drm_client_release(&ifbdev->helper.client);
> > +err_drm_fb_helper_unprepare:
> > +       drm_fb_helper_unprepare(&ifbdev->helper);
> > +       kfree(ifbdev);
> > +       return ret;
> >   }
> >   
> >   static void intel_fbdev_initial_config(void *data, async_cookie_t
> > cookie)

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature