[82/86] drm/i915: Move custom hotplug code into separate callback

[Thomas Zimmermann]

Hi

Am 19.08.24 um 10:52 schrieb Sui Jingfeng:
> Hi, Thomas
>
>
> I love your patch, yet ...
>
>
> On 2024/8/16 20:23, Thomas Zimmermann wrote:
>> i915's fbdev contains additional code for hotplugging a display that
>> cannot be ported to the common fbdev client. Introduce the callback
>> struct drm_fb_helper.fb_hotplug and implement it for i915. The fbdev
>> helpers invoke the callback before handing the hotplug event.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Tvrtko Ursulin <tursulin at ursulin.net>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Cc: "Thomas Hellström" <thomas.hellstrom at linux.intel.com>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c            |  6 +++
>>   drivers/gpu/drm/i915/display/intel_fbdev.c | 43 ++++++++++++----------
>>   include/drm/drm_fb_helper.h                | 13 +++++++
>>   3 files changed, 42 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c 
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index d9e539b0fd1a..92926cb02dfb 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -1938,6 +1938,12 @@ int drm_fb_helper_hotplug_event(struct 
>> drm_fb_helper *fb_helper)
>>       if (!drm_fbdev_emulation || !fb_helper)
>>           return 0;
>>   +    if (fb_helper->funcs->fb_hotplug) {
>
> We seems need to check the existence on the 'fb_helper->funcs' here,
>
> For example:
>
>
> if (fb_helper->funcs && fb_helper->funcs->fb_hotplug) {
>
> Otherwise, it will de-reference NULL pointer.
> Can be observed on a trivial driver though,
> with no monitor(display) connected.

Indeed. That needs to be fixed. Thank you for noting.

To give some context:  I was hoping to remove drm_fb_helper_funcs at 
some point. fb_probe is now gone with these patches and fb_dirty can 
certainly be replaced as well. (I once had prototype patches to do 
that). This leaves the new callbacks for 915, for which I don't have a 
good alternative solution. So it seems that drm_fb_helper_funcs will 
only be used by i915/xe in the long term.

Best regards
Thomas

>
>
>> +        err = fb_helper->funcs->fb_hotplug(fb_helper);
>> +        if (err)
>> +            return err;
>> +    }
>> +
>>       mutex_lock(&fb_helper->lock);
>>       if (fb_helper->deferred_setup) {
>>           err = __drm_fb_helper_initial_config_and_unlock(fb_helper);
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c 
>> b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index c03fb00002a4..abe77ef0bd84 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> @@ -305,10 +305,32 @@ static void intelfb_restore(struct 
>> drm_fb_helper *fb_helper)
>>       intel_fbdev_invalidate(ifbdev);
>>   }
>>   +static int intelfb_hotplug(struct drm_fb_helper *fb_helper)
>> +{
>> +    struct drm_device *dev = fb_helper->client.dev;
>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>> +    struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
>> +    bool send_hpd;
>> +
>> +    if (!ifbdev)
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&ifbdev->hpd_lock);
>> +    send_hpd = !ifbdev->hpd_suspended;
>> +    ifbdev->hpd_waiting = true;
>> +    mutex_unlock(&ifbdev->hpd_lock);
>> +
>> +    if (!send_hpd || !(ifbdev->vma || dev->fb_helper->deferred_setup))
>> +        return -EAGAIN;
>> +
>> +    return 0;
>> +}
>> +
>>   static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>>       .fb_probe = intelfb_create,
>>       .fb_dirty = intelfb_dirty,
>>       .fb_restore = intelfb_restore,
>> +    .fb_hotplug = intelfb_hotplug,
>>   };
>>     /*
>> @@ -557,25 +579,6 @@ void intel_fbdev_set_suspend(struct drm_device 
>> *dev, int state, bool synchronous
>>       intel_fbdev_hpd_set_suspend(dev_priv, state);
>>   }
>>   -static int intel_fbdev_output_poll_changed(struct drm_device *dev)
>> -{
>> -    struct intel_fbdev *ifbdev = to_i915(dev)->display.fbdev.fbdev;
>> -    bool send_hpd;
>> -
>> -    if (!ifbdev)
>> -        return -EINVAL;
>> -
>> -    mutex_lock(&ifbdev->hpd_lock);
>> -    send_hpd = !ifbdev->hpd_suspended;
>> -    ifbdev->hpd_waiting = true;
>> -    mutex_unlock(&ifbdev->hpd_lock);
>> -
>> -    if (send_hpd && (ifbdev->vma || dev->fb_helper->deferred_setup))
>> -        drm_fb_helper_hotplug_event(dev->fb_helper);
>> -
>> -    return 0;
>> -}
>> -
>>   static int intel_fbdev_restore_mode(struct drm_i915_private *dev_priv)
>>   {
>>       struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
>> @@ -637,7 +640,7 @@ static int intel_fbdev_client_hotplug(struct 
>> drm_client_dev *client)
>>       int ret;
>>         if (dev->fb_helper)
>> -        return intel_fbdev_output_poll_changed(dev);
>> +        return drm_fb_helper_hotplug_event(fb_helper);
>>         ret = drm_fb_helper_init(dev, fb_helper);
>>       if (ret)
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 34eb77c18a13..3dcb9a60e408 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -112,6 +112,19 @@ struct drm_fb_helper_funcs {
>>        * TODO: Fix i915 to not require this callback.
>>        */
>>       void (*fb_restore)(struct drm_fb_helper *helper);
>> +
>> +    /**
>> +     * @fb_hotplug:
>> +     *
>> +     * Driver callback to prepare hotplug event. If set, fbdev
>> +     * emulation will invoke this callback before sending a hotplug
>> +     * event.
>> +     *
>> +     * Only for i915. Do not use in new code.
>> +     *
>> +     * TODO: Fix i915 to not require this callback.
>> +     */
>> +    int (*fb_hotplug)(struct drm_fb_helper *helper);
>>   };
>>     /**
>

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