On Thu, 12 Jan 2023, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Wed, Jan 11, 2023 at 02:37:38PM -0300, Maíra Canal wrote:
>> Introduce the ability to add DRM debugfs files to a list managed by the
>> connector and, during drm_connector_register(), all added files will be
>> created at once.
>>
>> Moreover, introduce some typesafety as struct drm_debugfs_connector_entry
>> holds a drm_connector instead of a drm_device. So, the drivers can get
>> a connector object directly from the struct drm_debugfs_connector_entry
>> in the show() callback.
>>
>> Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_connector.c | 5 +++++
>> drivers/gpu/drm/drm_debugfs.c | 35 +++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/drm_internal.h | 5 +++++
>> include/drm/drm_connector.h | 15 ++++++++++++++
>> include/drm/drm_debugfs.h | 26 ++++++++++++++++++++++++
>> 5 files changed, 86 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 8d92777e57dd..c93655bb0edf 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -273,8 +273,10 @@ static int __drm_connector_init(struct drm_device *dev,
>> INIT_LIST_HEAD(&connector->global_connector_list_entry);
>> INIT_LIST_HEAD(&connector->probed_modes);
>> INIT_LIST_HEAD(&connector->modes);
>> + INIT_LIST_HEAD(&connector->debugfs_list);
>> mutex_init(&connector->mutex);
>> mutex_init(&connector->edid_override_mutex);
>> + mutex_init(&connector->debugfs_mutex);
>> connector->edid_blob_ptr = NULL;
>> connector->epoch_counter = 0;
>> connector->tile_blob_ptr = NULL;
>> @@ -581,6 +583,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>> connector->state);
>>
>> mutex_destroy(&connector->mutex);
>> + mutex_destroy(&connector->debugfs_mutex);
>>
>> memset(connector, 0, sizeof(*connector));
>>
>> @@ -627,6 +630,8 @@ int drm_connector_register(struct drm_connector *connector)
>> goto err_debugfs;
>> }
>>
>> + drm_debugfs_connector_init(connector);
>> +
>> drm_mode_object_register(connector->dev, &connector->base);
>>
>> connector->registration_state = DRM_CONNECTOR_REGISTERED;
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 23f6ed7b5d68..d9ec1ed5a7ec 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -260,6 +260,17 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>> return 0;
>> }
>>
>> +void drm_debugfs_connector_init(struct drm_connector *connector)
>> +{
>> + struct drm_minor *minor = connector->dev->primary;
>> + struct drm_debugfs_connector_entry *entry, *tmp;
>> +
>> + if (!minor)
>> + return;
>> +
>> + drm_create_file_from_list(connector);
>> +}
>> +
>> void drm_debugfs_late_register(struct drm_device *dev)
>> {
>> struct drm_minor *minor = dev->primary;
>> @@ -369,6 +380,30 @@ void drm_debugfs_add_files(struct drm_device *dev, const struct drm_debugfs_info
>> }
>> EXPORT_SYMBOL(drm_debugfs_add_files);
>>
>> +/**
>> + * drm_debugfs_connector_add_file - Add a given file to the DRM connector debugfs file list
>> + * @connector: DRM connector object
>> + * @name: debugfs file name
>> + * @show: show callback
>> + * @data: driver-private data, should not be device-specific
>> + *
>> + * Add a given file entry to the DRM connector debugfs file list to be created on
>> + * drm_debugfs_connector_init().
>> + */
>> +void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
>> + int (*show)(struct seq_file*, void*), void *data)
>> +{
>> + struct drm_debugfs_connector_entry *entry = drmm_kzalloc(connector->dev,
>> + sizeof(*entry),
>> + GFP_KERNEL);
>> +
>> + if (!entry)
>> + return;
>> +
>> + drm_debugfs_add_file_to_list(connector);
>> +}
>> +EXPORT_SYMBOL(drm_debugfs_connector_add_file);
>> +
>> static int connector_show(struct seq_file *m, void *data)
>> {
>> struct drm_connector *connector = m->private;
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index ed2103ee272c..dd9d7b8b45bd 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -185,6 +185,7 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
>> #if defined(CONFIG_DEBUG_FS)
>> int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>> struct dentry *root);
>> +void drm_debugfs_connector_init(struct drm_connector *connector);
>> void drm_debugfs_cleanup(struct drm_minor *minor);
>> void drm_debugfs_late_register(struct drm_device *dev);
>> void drm_debugfs_connector_add(struct drm_connector *connector);
>> @@ -199,6 +200,10 @@ static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>> return 0;
>> }
>>
>> +static inline void drm_debugfs_connector_init(struct drm_connector *connector)
>> +{
>> +}
>> +
>> static inline void drm_debugfs_cleanup(struct drm_minor *minor)
>> {
>> }
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 9037f1317aee..51340f3162ed 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -1720,6 +1720,21 @@ struct drm_connector {
>> /** @debugfs_entry: debugfs directory for this connector */
>> struct dentry *debugfs_entry;
>>
>> + /**
>> + * @debugfs_mutex:
>> + *
>> + * Protects &debugfs_list access.
>> + */
>> + struct mutex debugfs_mutex;
>> +
>> + /**
>> + * @debugfs_list:
>> + *
>> + * List of debugfs files to be created by the DRM connector. The files
>> + * must be added during drm_connector_register().
>> + */
>> + struct list_head debugfs_list;
>> +
>> /**
>> * @state:
>> *
>> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
>> index 7616f457ce70..c09c82274622 100644
>> --- a/include/drm/drm_debugfs.h
>> +++ b/include/drm/drm_debugfs.h
>> @@ -122,6 +122,23 @@ struct drm_debugfs_entry {
>> struct list_head list;
>> };
>>
>> +/**
>> + * struct drm_debugfs_connector_entry - Per-connector debugfs node structure
>> + *
>> + * This structure represents a debugfs file, as an instantiation of a &struct
>> + * drm_debugfs_info on a &struct drm_connector.
>> + */
>> +struct drm_debugfs_connector_entry {
>> + /** @connector: &struct drm_connector for this node. */
>> + struct drm_connector *connector;
>> +
>> + /** @file: Template for this node. */
>> + struct drm_debugfs_info file;
>> +
>> + /** @list: Linked list of all connector nodes. */
>> + struct list_head list;
>> +};
>
> I missed it in the main api, but I'm not a big fan of exposing this struct
> to driver. And I don't see the need ... if we just directly put the
> drm_connector into seq_file->private (or an explicit parameter for our
> show function, with some glue provided) then drivers don't need to be able
> to see this? This really should be just an implementation detail I think.
> -Daniel
See also https://lore.kernel.org/r/878ri8glee.fsf@xxxxxxxxx
>
>> +
>> #if defined(CONFIG_DEBUG_FS)
>> void drm_debugfs_create_files(const struct drm_info_list *files,
>> int count, struct dentry *root,
>> @@ -134,6 +151,9 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
>>
>> void drm_debugfs_add_files(struct drm_device *dev,
>> const struct drm_debugfs_info *files, int count);
>> +
>> +void drm_debugfs_connector_add_file(struct drm_connector *connector, const char *name,
>> + int (*show)(struct seq_file*, void*), void *data);
>> #else
>> static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>> int count, struct dentry *root,
>> @@ -155,6 +175,12 @@ static inline void drm_debugfs_add_files(struct drm_device *dev,
>> const struct drm_debugfs_info *files,
>> int count)
>> {}
>> +
>> +static inline void drm_debugfs_connector_add_file(struct drm_connector *connector,
>> + const char *name,
>> + int (*show)(struct seq_file*, void*),
>> + void *data)
>> +{}
>> #endif
>>
>> #endif /* _DRM_DEBUGFS_H_ */
>> --
>> 2.39.0
>>
--
Jani Nikula, Intel Open Source Graphics Center
