Re: [PATCH v5 08/11] drm/fb-helper: Remove drm_fb_helper_connector

[Sam Ravnborg <sam@ravnborg.org>]

Hi Noralf.

See few comments in the following.

	Sam

On Mon, May 06, 2019 at 08:01:36PM +0200, Noralf Trønnes wrote:
> All drivers add all their connectors so there's no need to keep around an
> array of available connectors.
> 
> Rename functions which signature is changed since they will be moved to
> drm_client in a later patch.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> @@ -1645,8 +1461,9 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  	struct drm_client_dev *client = &fb_helper->client;
>  	int ret = 0;
>  	int crtc_count = 0;
> -	int i;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_fb_helper_surface_size sizes;
> +	struct drm_connector *connector;
>  	struct drm_mode_set *mode_set;
>  	int best_depth = 0;
>  
> @@ -1663,11 +1480,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  	if (preferred_bpp != sizes.surface_bpp)
>  		sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
>  
> -	drm_fb_helper_for_each_connector(fb_helper, i) {
> -		struct drm_fb_helper_connector *fb_helper_conn = fb_helper->connector_info[i];
> +	drm_connector_list_iter_begin(fb_helper->dev, &conn_iter);
> +	drm_client_for_each_connector_iter(connector, &conn_iter) {
>  		struct drm_cmdline_mode *cmdline_mode;

I fail to see when to use drm_client_for_each_connector_iter() and when
to use a simple for loop.
In this patch drm_fb_helper_for_each_connector() is here replaced by the
iterator variant and in many other placed a simple for loop.

The old code, in this place, would go through all connectors, but this
code will skip 	DRM_MODE_CONNECTOR_WRITEBACK conectors.
So it does not like identical code.

>  
> -		cmdline_mode = &fb_helper_conn->connector->cmdline_mode;
> +		cmdline_mode = &connector->cmdline_mode;
>  
>  		if (cmdline_mode->bpp_specified) {
>  			switch (cmdline_mode->bpp) {
> @@ -1692,6 +1509,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  			break;
>  		}
>  	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	/*
>  	 * If we run into a situation where, for example, the primary plane
> @@ -1876,26 +1694,12 @@ void drm_fb_helper_fill_info(struct fb_info *info,
>  }
>  EXPORT_SYMBOL(drm_fb_helper_fill_info);
>  
> +static void drm_client_connectors_enabled(struct drm_connector **connectors,
> +					  unsigned int connector_count,
> +					  bool *enabled)
>  {
>  	bool any_enabled = false;
>  	struct drm_connector *connector;
>  	int i = 0;
>  
> -	drm_fb_helper_for_each_connector(fb_helper, i) {
> -		connector = fb_helper->connector_info[i]->connector;
> +	for (i = 0; i < connector_count; i++) {
> +		connector = connectors[i];
This is for example a place where the drm_fb_helper_for_each_connector()
is replaced by a simple for loop.
The simple for loop is nice and readable - just a comment replated to
the above comment.

>  		struct drm_display_mode *mode = modes[i];
>  		struct drm_crtc *crtc = crtcs[i];
>  		struct drm_fb_offset *offset = &offsets[i];
>  
>  		if (mode && crtc) {
>  			struct drm_mode_set *modeset = drm_client_find_modeset(client, crtc);
> -			struct drm_connector *connector =
> -				fb_helper->connector_info[i]->connector;
> +			struct drm_connector *connector = connectors[i];
>  
>  			DRM_DEBUG_KMS("desired mode %s set on crtc %d (%d,%d)\n",
>  				      mode->name, crtc->base.id, offset->x, offset->y);
> @@ -2539,6 +2354,10 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  	kfree(modes);
>  	kfree(offsets);
>  	kfree(enabled);
> +free_connectors:
> +	for (i = 0; i < connector_count; i++)
> +		drm_connector_put(connectors[i]);
This may call drm_connector_put(NULL) as not all connectors are init.

> +	kfree(connectors);
>  }
>  
>  /*
> @@ -2551,10 +2370,11 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>  {
>  	struct drm_client_dev *client = &fb_helper->client;
> +	struct drm_connector_list_iter conn_iter;
>  	struct fb_info *info = fb_helper->fbdev;
>  	unsigned int rotation, sw_rotations = 0;
> +	struct drm_connector *connector;
>  	struct drm_mode_set *modeset;
> -	int i;
>  
>  	mutex_lock(&client->modeset_mutex);
>  	drm_client_for_each_modeset(modeset, client) {
> @@ -2571,10 +2391,8 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>  	}
>  	mutex_unlock(&client->modeset_mutex);
>  
> -	mutex_lock(&fb_helper->dev->mode_config.mutex);
Why is it OK to drop this lock?
In general I fail to see how the connector data is protected (locking
rules?) This is likely just me missing the bigger picture here..

> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	switch (sw_rotations) {
>  	case DRM_MODE_ROTATE_0:

> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 6cf48419f77f..8d94880bbe25 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -7,6 +7,7 @@
>  #include <linux/mutex.h>
>  #include <linux/types.h>
>  
> +#include <drm/drm_connector.h>
>  #include <drm/drm_crtc.h>
>  
>  struct drm_client_dev;
> @@ -169,6 +170,20 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode);
>  	for (({ lockdep_assert_held(&(client)->modeset_mutex); }), \
>  	     modeset = (client)->modesets; modeset->crtc; modeset++)
>  
> +/**
> + * drm_client_for_each_connector_iter - connector_list iterator macro
> + * @connector: &struct drm_connector pointer used as cursor
> + * @iter: &struct drm_connector_list_iter
> + *
> + * This iterates the connectors that are useable for internal clients (excludes
> + * writeback connectors).
> + *
> + * For more info see drm_for_each_connector_iter().
> + */
> +#define drm_client_for_each_connector_iter(connector, iter) \
> +	drm_for_each_connector_iter(connector, iter) \
> +		if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> +
>  int drm_client_debugfs_init(struct drm_minor *minor);
>  
>  #endif
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 6b334f4d8a22..e14d52ef9638 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -97,16 +97,10 @@ struct drm_fb_helper_funcs {
>  			struct drm_fb_helper_surface_size *sizes);
>  };
>  
> -struct drm_fb_helper_connector {
> -	struct drm_connector *connector;
> -};
> -
>  /**
>   * struct drm_fb_helper - main structure to emulate fbdev on top of KMS
>   * @fb: Scanout framebuffer object
>   * @dev: DRM device
> - * @connector_count: number of connected connectors
> - * @connector_info_alloc_count: size of connector_info
>   * @funcs: driver callbacks for fb helper
>   * @fbdev: emulated fbdev device info struct
>   * @pseudo_palette: fake palette of 16 colors
> @@ -138,15 +132,6 @@ struct drm_fb_helper {
>  
>  	struct drm_framebuffer *fb;
>  	struct drm_device *dev;
> -	int connector_count;
> -	int connector_info_alloc_count;
> -	/**
> -	 * @connector_info:
> -	 *
> -	 * Array of per-connector information. Do not iterate directly, but use
> -	 * drm_fb_helper_for_each_connector.
> -	 */
> -	struct drm_fb_helper_connector **connector_info;

As we are removing connector_info the comment for the @lock member
should be adjusted not to mention connector_info.


>  	const struct drm_fb_helper_funcs *funcs;
>  	struct fb_info *fbdev;
>  	u32 pseudo_palette[17];
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx