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

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch