Re: [PATCH 5/5] drm: Add client-lib module

[Jocelyn Falempe <jfalempe@redhat.com>]

On 30/09/2024 13:55, Jocelyn Falempe wrote:
> On 27/09/2024 16:37, Thomas Zimmermann wrote:
>> Add drm_client_lib.ko to contain most of the client code. Move the
>> existing client for fbdev emulation into the new module. Protect the
>> new module behind CONFIG_DRM_CLIENT.
>>
>> The Kconfig rules separate the DRM drivers from the DRM clients. A
>> driver can opt into the default clients, but the user configures
>> each client individually. To do so, DRM drivers still select
>> DRM_CLIENT_SELECTION. The option is now a tristate that further
>> selects all dependencies of the enabled DRM clients. There's
>> a menu option for each client. Enabling at least one client also
>> selects DRM_CLIENT_SETUP, so that drivers call drm_client_setup().
>> New DRM clients should depend on DRM_CLIENT_SELECTION.
>>
>> The KMS-helper module now exports handful of symbols needed by the
>> DRM client library.
>>
>> There are existing kernel options in drm_fb_helper.o, so leave this
>> file in the KMS-helper module for now.
>>
>> Amdgpu has an internal DRM client, so it has to select DRM_CLIENT by
>> itself unconditionally.
> 
> Thanks, I found a typo in the Makefile (see below), other than that it 
> looks good to me.

Sorry, this time with the typo below.
> 
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/Kconfig            | 31 ++++++++++++++++++++++++------
>>   drivers/gpu/drm/Makefile           | 19 +++++++++++++-----
>>   drivers/gpu/drm/amd/amdgpu/Kconfig |  1 +
>>   drivers/gpu/drm/drm_client.c       |  3 +++
>>   drivers/gpu/drm/drm_dumb_buffers.c |  2 ++
>>   drivers/gpu/drm/drm_file.c         |  2 ++
>>   drivers/gpu/drm/drm_framebuffer.c  |  2 ++
>>   drivers/gpu/drm/drm_gem.c          |  2 ++
>>   8 files changed, 51 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 88b2ba55fe16..379fccf3a6ea 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -9,8 +9,6 @@ menuconfig DRM
>>       tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI 
>> support)"
>>       depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
>>       select DRM_PANEL_ORIENTATION_QUIRKS
>> -    select DRM_KMS_HELPER if DRM_FBDEV_EMULATION
>> -    select FB_CORE if DRM_FBDEV_EMULATION
>>       select HDMI
>>       select I2C
>>       select DMA_SHARED_BUFFER
>> @@ -209,21 +207,40 @@ config DRM_DEBUG_MODESET_LOCK
>>         If in doubt, say "N".
>> +config DRM_CLIENT
>> +    tristate
>> +    depends on DRM
>> +    help
>> +      Enables the DRM client-lib module. DRM drivers that need
>> +      struct drm_client_dev and its interfaces should select this
>> +      option. Drivers that support the default clients should
>> +      select DRM_CLIENT_SELECTION instead.
>> +
>>   config DRM_CLIENT_SELECTION
>> -    bool
>> +    tristate
>>       depends on DRM
>> -    select DRM_CLIENT_SETUP if DRM_FBDEV_EMULATION
>> +    select DRM_CLIENT if DRM_FBDEV_EMULATION
>> +    select DRM_KMS_HELPER if DRM_FBDEV_EMULATION
>> +    select FB_CORE if DRM_FBDEV_EMULATION
>>       help
>>         Drivers that support in-kernel DRM clients have to select this
>> -      option.
>> +      option. It selects all modules and components according to the
>> +      enabled clients.
>>   config DRM_CLIENT_SETUP
>>       bool
>>       depends on DRM_CLIENT_SELECTION
>> +    help
>> +      Enables the DRM client selection. DRM drivers that support the
>> +      default clients should select DRM_CLIENT_SELECTION instead.
>> +
>> +menu "Supported DRM clients"
>> +    depends on DRM_CLIENT_SELECTION
>>   config DRM_FBDEV_EMULATION
>>       bool "Enable legacy fbdev support for your modesetting driver"
>> -    depends on DRM
>> +    depends on DRM_CLIENT_SELECTION
>> +    select DRM_CLIENT_SETUP
>>       select FRAMEBUFFER_CONSOLE_DETECT_PRIMARY if FRAMEBUFFER_CONSOLE
>>       default FB
>>       help
>> @@ -262,6 +279,8 @@ config DRM_FBDEV_LEAK_PHYS_SMEM
>>         If in doubt, say "N" or spread the word to your closed source
>>         library vendor.
>> +endmenu
>> +
>>   config DRM_LOAD_EDID_FIRMWARE
>>       bool "Allow to specify an EDID data set instead of probing for it"
>>       depends on DRM
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index c50443756457..419208b97f57 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -41,9 +41,7 @@ drm-y := \
>>       drm_blend.o \
>>       drm_bridge.o \
>>       drm_cache.o \
>> -    drm_client.o \
>>       drm_client_dev.o \
>> -    drm_client_modeset.o \
>>       drm_color_mgmt.o \
>>       drm_connector.o \
>>       drm_crtc.o \
>> @@ -144,14 +142,25 @@ drm_kms_helper-y := \
>>       drm_probe_helper.o \
>>       drm_self_refresh_helper.o \
>>       drm_simple_kms_helper.o
>> -drm_kms_helper-$(CONFIG_DRM_CLIENT_SETUP) += \
>> -    drm_client_setup.o
>>   drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
>>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += \
>> -    drm_fbdev_client.o \
>>       drm_fb_helper.o
>> +drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o

Typo: the above line is duplicated.

>>   obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>> +#
>> +# DRM clients
>> +#
>> +
>> +drm_client_lib-y := \
>> +    drm_client.o \
>> +    drm_client_modeset.o
>> +drm_client_lib-$(CONFIG_DRM_CLIENT_SETUP) += \
>> +    drm_client_setup.o
>> +drm_client_lib-$(CONFIG_DRM_FBDEV_EMULATION) += \
>> +    drm_fbdev_client.o
>> +obj-$(CONFIG_DRM_CLIENT) += drm_client_lib.o
>> +
>>   #
>>   # Drivers and the rest
>>   #
>> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/ 
>> amdgpu/Kconfig
>> index 680a94c361ba..41fa3377d9cf 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
>> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
>> @@ -5,6 +5,7 @@ config DRM_AMDGPU
>>       depends on DRM && PCI && MMU
>>       depends on !UML
>>       select FW_LOADER
>> +    select DRM_CLIENT
>>       select DRM_CLIENT_SELECTION
>>       select DRM_DISPLAY_DP_HELPER
>>       select DRM_DISPLAY_DSC_HELPER
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index 549b28a5918c..864ee96e48cf 100644
>> --- a/drivers/gpu/drm/drm_client.c
>> +++ b/drivers/gpu/drm/drm_client.c
>> @@ -490,3 +490,6 @@ int drm_client_framebuffer_flush(struct 
>> drm_client_buffer *buffer, struct drm_re
>>                       0, 0, NULL, 0);
>>   }
>>   EXPORT_SYMBOL(drm_client_framebuffer_flush);
>> +
>> +MODULE_DESCRIPTION("In-kernel DRM clients");
>> +MODULE_LICENSE("GPL and additional rights");
>> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/ 
>> drm_dumb_buffers.c
>> index 70032bba1c97..7ed8f05a8d5c 100644
>> --- a/drivers/gpu/drm/drm_dumb_buffers.c
>> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
>> @@ -95,6 +95,7 @@ int drm_mode_create_dumb(struct drm_device *dev,
>>       return dev->driver->dumb_create(file_priv, dev, args);
>>   }
>> +EXPORT_SYMBOL(drm_mode_create_dumb);
>>   int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>>                      void *data, struct drm_file *file_priv)
>> @@ -141,6 +142,7 @@ int drm_mode_destroy_dumb(struct drm_device *dev, 
>> u32 handle,
>>       return drm_gem_handle_delete(file_priv, handle);
>>   }
>> +EXPORT_SYMBOL(drm_mode_destroy_dumb);
>>   int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
>>                   void *data, struct drm_file *file_priv)
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 01fde94fe2a9..dd847b574457 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -186,6 +186,7 @@ struct drm_file *drm_file_alloc(struct drm_minor 
>> *minor)
>>       return ERR_PTR(ret);
>>   }
>> +EXPORT_SYMBOL(drm_file_alloc);
>>   static void drm_events_release(struct drm_file *file_priv)
>>   {
>> @@ -261,6 +262,7 @@ void drm_file_free(struct drm_file *file)
>>       put_pid(rcu_access_pointer(file->pid));
>>       kfree(file);
>>   }
>> +EXPORT_SYMBOL(drm_file_free);
>>   static void drm_close_helper(struct file *filp)
>>   {
>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/ 
>> drm_framebuffer.c
>> index 47e6e8577b62..761262529416 100644
>> --- a/drivers/gpu/drm/drm_framebuffer.c
>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>> @@ -349,6 +349,7 @@ int drm_mode_addfb2(struct drm_device *dev,
>>       return 0;
>>   }
>> +EXPORT_SYMBOL(drm_mode_addfb2);
>>   int drm_mode_addfb2_ioctl(struct drm_device *dev,
>>                 void *data, struct drm_file *file_priv)
>> @@ -473,6 +474,7 @@ int drm_mode_rmfb(struct drm_device *dev, u32 fb_id,
>>       return 0;
>>   }
>> +EXPORT_SYMBOL(drm_mode_rmfb);
>>   int drm_mode_rmfb_ioctl(struct drm_device *dev,
>>               void *data, struct drm_file *file_priv)
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index ee811764c3df..07ae82e35517 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -1191,12 +1191,14 @@ int drm_gem_pin_locked(struct drm_gem_object 
>> *obj)
>>       return 0;
>>   }
>> +EXPORT_SYMBOL(drm_gem_pin_locked);
>>   void drm_gem_unpin_locked(struct drm_gem_object *obj)
>>   {
>>       if (obj->funcs->unpin)
>>           obj->funcs->unpin(obj);
>>   }
>> +EXPORT_SYMBOL(drm_gem_unpin_locked);
>>   int drm_gem_pin(struct drm_gem_object *obj)
>>   {
>