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

[Thomas Zimmermann]

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 at 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)
> 

-- 
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20231101/657a9ac6/attachment.sig>