| Message ID | 20230927102808.18650-6-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Convert fbdev to DRM client | expand |
Hi Thomas, One minor comment inline below. 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@suse.de> > --- > 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 might actually make sense to use intel-fbdev here rather than i915- fbdev. 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)
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@suse.de> >> --- >> 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) >
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@suse.de> > > > --- > > > 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)
Hi Am 01.11.23 um 10:10 schrieb Hogander, Jouni: [...] >>> 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. Right, I was confusing the names. The client name is the name of the framebuffer in various places. https://elixir.bootlin.com/linux/v6.6/source/drivers/gpu/drm/drm_client.c#L421 https://elixir.bootlin.com/linux/v6.6/source/drivers/gpu/drm/drm_framebuffer.c#L1182 But it appears to be overwritten by fbdev later during initialization: https://elixir.bootlin.com/linux/v6.6/source/drivers/gpu/drm/drm_fb_helper.c#L1672 I'll change the name then, but I'm not sure if it'll make a difference in the end. Best regards Thomas > > 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) >
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", + &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)
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@suse.de> --- drivers/gpu/drm/i915/display/intel_fbdev.c | 43 ++++++++++++++++++++-- 1 file changed, 39 insertions(+), 4 deletions(-)