On Wed, 2023-11-01 at 09:11 +0100, Thomas Zimmermann wrote:
> 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?
It will mainly share display code with i915.
>
> > 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.
Client name doesn't affect name used in fbinfo. For i915 it is
i915drmfb as earlier and for xe its xedrmfb. Also this client name is
completely new added by your patches. I'm not sure where it will
visible. I was originally thinking using driver->name as done in
drm_fb_helper_fill_info, but I think common intel-fbdev is just fine.
BR,
Jouni Högander
>
> 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)
