Re: [Intel-gfx] [RFC 1/2] drm/connector: add connector list iteration with filtering

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 15 Dec 2022, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Oct 05, 2022 at 01:51:43PM +0300, Jani Nikula wrote:
>> Add new function drm_connector_list_iter_filter_begin() to initialize
>> connector list iterator with a filter function. Subsequent iteration on
>> the list will only return connectors on which the filter function
>> returns true.
>> 
>> Cc: Arun R Murthy <arun.r.murthy@intel.com>
>> Cc: Suraj Kandpal <suraj.kandpal@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_connector.c | 57 ++++++++++++++++++++++++++-------
>>  include/drm/drm_connector.h     |  9 ++++++
>>  2 files changed, 54 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index e3142c8142b3..d54b4b54cecb 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -762,6 +762,29 @@ static struct lockdep_map connector_list_iter_dep_map = {
>>  };
>>  #endif
>>  
>> +/**
>> + * drm_connector_list_iter_filter_begin - initialize a connector_list iterator with filter
>> + * @dev: DRM device
>> + * @iter: connector_list iterator
>> + * @filter: connector filter function
>> + * @filter_context: context to be passed to the filter function
>> + *
>> + * Same as drm_connector_list_iter_begin(), but sets up the iterator to only
>> + * return connectors where filter(connector) returns true.
>> + */
>> +void drm_connector_list_iter_filter_begin(struct drm_device *dev,
>> +					  struct drm_connector_list_iter *iter,
>> +					  drm_connector_list_iter_filter_fn filter,
>> +					  void *filter_context)
>> +{
>> +	iter->dev = dev;
>> +	iter->conn = NULL;
>> +	iter->filter = filter;
>> +	iter->filter_context = filter_context;
>> +	lock_acquire_shared_recursive(&connector_list_iter_dep_map, 0, 1, NULL, _RET_IP_);
>> +}
>> +EXPORT_SYMBOL(drm_connector_list_iter_filter_begin);
>
> So maybe I'm missing the obvious, but can't we just put a for_each_fi
> right after the for_each_connector_iter?
>
> And then maybe provide a default filter to kick out connectors and maybe a
> macro that does the filtered iteration? The iter_begin/next/end is already
> fairly tricky pattern compared to plain for_each macro, I think we should
> try hard to not complicate it further with lots of variants if that's not
> absolutely necessary.

Sorry, dropped the ball here, and lost sight of it.

You mean not have any of this in drm core, and just add the
for_each_if() in local wrappers everywhere?

BR,
Jani.


> -Daniel
>
>
>> +
>>  /**
>>   * drm_connector_list_iter_begin - initialize a connector_list iterator
>>   * @dev: DRM device
>> @@ -775,9 +798,7 @@ static struct lockdep_map connector_list_iter_dep_map = {
>>  void drm_connector_list_iter_begin(struct drm_device *dev,
>>  				   struct drm_connector_list_iter *iter)
>>  {
>> -	iter->dev = dev;
>> -	iter->conn = NULL;
>> -	lock_acquire_shared_recursive(&connector_list_iter_dep_map, 0, 1, NULL, _RET_IP_);
>> +	drm_connector_list_iter_filter_begin(dev, iter, NULL, NULL);
>>  }
>>  EXPORT_SYMBOL(drm_connector_list_iter_begin);
>>  
>> @@ -800,15 +821,8 @@ __drm_connector_put_safe(struct drm_connector *conn)
>>  	schedule_work(&config->connector_free_work);
>>  }
>>  
>> -/**
>> - * drm_connector_list_iter_next - return next connector
>> - * @iter: connector_list iterator
>> - *
>> - * Returns: the next connector for @iter, or NULL when the list walk has
>> - * completed.
>> - */
>> -struct drm_connector *
>> -drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
>> +static struct drm_connector *
>> +__drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
>>  {
>>  	struct drm_connector *old_conn = iter->conn;
>>  	struct drm_mode_config *config = &iter->dev->mode_config;
>> @@ -836,6 +850,25 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
>>  
>>  	return iter->conn;
>>  }
>> +
>> +/**
>> + * drm_connector_list_iter_next - return next connector
>> + * @iter: connector_list iterator
>> + *
>> + * Returns: the next connector for @iter, or NULL when the list walk has
>> + * completed.
>> + */
>> +struct drm_connector *
>> +drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
>> +{
>> +	struct drm_connector *connector;
>> +
>> +	while ((connector = __drm_connector_list_iter_next(iter)) &&
>> +	       iter->filter && !iter->filter(connector, iter->filter_context))
>> +		;
>> +
>> +	return connector;
>> +}
>>  EXPORT_SYMBOL(drm_connector_list_iter_next);
>>  
>>  /**
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 56aee949c6fa..497b98197d3a 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -1868,6 +1868,9 @@ struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev,
>>  void drm_mode_put_tile_group(struct drm_device *dev,
>>  			     struct drm_tile_group *tg);
>>  
>> +typedef bool (*drm_connector_list_iter_filter_fn)(const struct drm_connector *connector,
>> +						  void *filter_context);
>> +
>>  /**
>>   * struct drm_connector_list_iter - connector_list iterator
>>   *
>> @@ -1886,10 +1889,16 @@ struct drm_connector_list_iter {
>>  /* private: */
>>  	struct drm_device *dev;
>>  	struct drm_connector *conn;
>> +	drm_connector_list_iter_filter_fn filter;
>> +	void *filter_context;
>>  };
>>  
>>  void drm_connector_list_iter_begin(struct drm_device *dev,
>>  				   struct drm_connector_list_iter *iter);
>> +void drm_connector_list_iter_filter_begin(struct drm_device *dev,
>> +					  struct drm_connector_list_iter *iter,
>> +					  drm_connector_list_iter_filter_fn filter,
>> +					  void *filter_context);
>>  struct drm_connector *
>>  drm_connector_list_iter_next(struct drm_connector_list_iter *iter);
>>  void drm_connector_list_iter_end(struct drm_connector_list_iter *iter);
>> -- 
>> 2.34.1
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center