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

[Sui Jingfeng <sui.jingfeng@linux.dev>]

Hi,


On 2024/8/20 15:39, Thomas Zimmermann wrote:
> 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@suse.de>
>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Cc: "Thomas Hellström" <thomas.hellstrom@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.
>

Thanks for you efforts then.


> To give some context:  I was hoping to remove drm_fb_helper_funcs at 
> some point. 


Yeah, too many helper functions may make peoples daze.


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


Well, the grammar of "ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);" looks strange, 
It's lengthy and I observed you have cleaned it up at the last patch. 
Which also eliminates one pair "if and else" clause, the codes looks 
more fluent now.


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

Well, since it is a DRM client now, maybe we could try to drop it into struct drm_driver.
Just like the '.fbdev_probe' callback, this may help to achieve a 100% DRM-based console/logger IMO.

Besides, a lot of DRM driver instances has the DMA/2D acceleration hardware, promote it
into drm_driver structure may has the potential to utilize hardware acceleration. Drivers
will more easily to have custom implementation. I'm not 100% sure if it will only be used
by i915 in the future.

Best regards,
Sui