[82/86] drm/i915: Move custom hotplug code into separate callback
Sui Jingfeng
sui.jingfeng at linux.dev
Tue Aug 20 10:39:29 UTC 2024
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 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.
>
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
More information about the amd-gfx
mailing list