Re: [PATCH 3/7] drm/gma500: Make use of the drm connector iterator

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 22, 2022 at 02:17:38PM +0100, Patrik Jakobsson wrote:
> This makes sure we're using proper locking when iterating the list of
> connectors.
> 
> Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@xxxxxxxxx>

Note that this is only needed if your driver deals with hotpluggable
connectors. Since gma500 doesn't, not really a need to convert this over,
but it also doesn't hurt.

If the kerneldoc doesn't explain this, maybe we should add it? Care for a
patch.

Either way on the entire series:

Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

I'll leave it up to you whether you want to push this one too or not.
-Daniel

> ---
>  drivers/gpu/drm/gma500/cdv_device.c        | 10 ++++++--
>  drivers/gpu/drm/gma500/cdv_intel_display.c |  9 +++++--
>  drivers/gpu/drm/gma500/framebuffer.c       |  6 +++--
>  drivers/gpu/drm/gma500/gma_display.c       | 16 ++++++++-----
>  drivers/gpu/drm/gma500/oaktrail_crtc.c     | 17 ++++++++-----
>  drivers/gpu/drm/gma500/oaktrail_lvds.c     | 15 ++++++------
>  drivers/gpu/drm/gma500/psb_device.c        | 28 +++++++++++++++-------
>  drivers/gpu/drm/gma500/psb_drv.c           | 10 ++++----
>  drivers/gpu/drm/gma500/psb_intel_display.c | 15 ++++++++----
>  9 files changed, 84 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gma500/cdv_device.c b/drivers/gpu/drm/gma500/cdv_device.c
> index f854f58bcbb3..dd32b484dd82 100644
> --- a/drivers/gpu/drm/gma500/cdv_device.c
> +++ b/drivers/gpu/drm/gma500/cdv_device.c
> @@ -262,6 +262,7 @@ static int cdv_save_display_registers(struct drm_device *dev)
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
>  	struct psb_save_area *regs = &dev_priv->regs;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *connector;
>  
>  	dev_dbg(dev->dev, "Saving GPU registers.\n");
> @@ -298,8 +299,10 @@ static int cdv_save_display_registers(struct drm_device *dev)
>  	regs->cdv.saveIER = REG_READ(PSB_INT_ENABLE_R);
>  	regs->cdv.saveIMR = REG_READ(PSB_INT_MASK_R);
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter)
>  		connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	return 0;
>  }
> @@ -317,6 +320,7 @@ static int cdv_restore_display_registers(struct drm_device *dev)
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
>  	struct psb_save_area *regs = &dev_priv->regs;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *connector;
>  	u32 temp;
>  
> @@ -373,8 +377,10 @@ static int cdv_restore_display_registers(struct drm_device *dev)
>  
>  	drm_mode_config_reset(dev);
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter)
>  		connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	/* Resume the modeset for every activated CRTC */
>  	drm_helper_resume_force_mode(dev);
> diff --git a/drivers/gpu/drm/gma500/cdv_intel_display.c b/drivers/gpu/drm/gma500/cdv_intel_display.c
> index 94ebc48a4349..0c3ddcdc29dc 100644
> --- a/drivers/gpu/drm/gma500/cdv_intel_display.c
> +++ b/drivers/gpu/drm/gma500/cdv_intel_display.c
> @@ -584,13 +584,14 @@ static int cdv_intel_crtc_mode_set(struct drm_crtc *crtc,
>  	bool ok;
>  	bool is_lvds = false;
>  	bool is_dp = false;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *connector;
>  	const struct gma_limit_t *limit;
>  	u32 ddi_select = 0;
>  	bool is_edp = false;
>  
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		struct gma_encoder *gma_encoder =
>  					gma_attached_encoder(connector);
>  
> @@ -613,10 +614,14 @@ static int cdv_intel_crtc_mode_set(struct drm_crtc *crtc,
>  			is_edp = true;
>  			break;
>  		default:
> +			drm_connector_list_iter_end(&conn_iter);
>  			DRM_ERROR("invalid output type.\n");
>  			return 0;
>  		}
> +
> +		break;
>  	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	if (dev_priv->dplla_96mhz)
>  		/* low-end sku, 96/100 mhz */
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 2b99c996fdc2..0ac6ea5fd3a1 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -451,6 +451,7 @@ static const struct drm_mode_config_funcs psb_mode_funcs = {
>  static void psb_setup_outputs(struct drm_device *dev)
>  {
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *connector;
>  
>  	drm_mode_create_scaling_mode_property(dev);
> @@ -461,8 +462,8 @@ static void psb_setup_outputs(struct drm_device *dev)
>  							"backlight", 0, 100);
>  	dev_priv->ops->output_init(dev);
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list,
> -			    head) {
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>  		struct drm_encoder *encoder = &gma_encoder->base;
>  		int crtc_mask = 0, clone_mask = 0;
> @@ -505,6 +506,7 @@ static void psb_setup_outputs(struct drm_device *dev)
>  		encoder->possible_clones =
>  		    gma_connector_clones(dev, clone_mask);
>  	}
> +	drm_connector_list_iter_end(&conn_iter);
>  }
>  
>  void psb_modeset_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index 1d7964c339f4..e8157464d9eb 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -27,17 +27,21 @@
>  bool gma_pipe_has_type(struct drm_crtc *crtc, int type)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
> -	struct drm_connector *l_entry;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
>  
> -	list_for_each_entry(l_entry, &mode_config->connector_list, head) {
> -		if (l_entry->encoder && l_entry->encoder->crtc == crtc) {
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		if (connector->encoder && connector->encoder->crtc == crtc) {
>  			struct gma_encoder *gma_encoder =
> -						gma_attached_encoder(l_entry);
> -			if (gma_encoder->type == type)
> +						gma_attached_encoder(connector);
> +			if (gma_encoder->type == type) {
> +				drm_connector_list_iter_end(&conn_iter);
>  				return true;
> +			}
>  		}
>  	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	return false;
>  }
> diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> index 36c7c2686c90..873c17cf8fb4 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> @@ -372,9 +372,9 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
>  	bool ok, is_sdvo = false;
>  	bool is_lvds = false;
>  	bool is_mipi = false;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct gma_encoder *gma_encoder = NULL;
>  	uint64_t scalingType = DRM_MODE_SCALE_FULLSCREEN;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *connector;
>  	int i;
>  	int need_aux = gma_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ? 1 : 0;
> @@ -392,7 +392,8 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
>  		adjusted_mode,
>  		sizeof(struct drm_display_mode));
>  
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (!connector->encoder || connector->encoder->crtc != crtc)
>  			continue;
>  
> @@ -409,8 +410,16 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
>  			is_mipi = true;
>  			break;
>  		}
> +
> +		break;
>  	}
>  
> +	if (gma_encoder)
> +		drm_object_property_get_value(&connector->base,
> +			dev->mode_config.scaling_mode_property, &scalingType);
> +
> +	drm_connector_list_iter_end(&conn_iter);
> +
>  	/* Disable the VGA plane that we never use */
>  	for (i = 0; i <= need_aux; i++)
>  		REG_WRITE_WITH_AUX(VGACNTRL, VGA_DISP_DISABLE, i);
> @@ -424,10 +433,6 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
>  					     (mode->crtc_vdisplay - 1), i);
>  	}
>  
> -	if (gma_encoder)
> -		drm_object_property_get_value(&connector->base,
> -			dev->mode_config.scaling_mode_property, &scalingType);
> -
>  	if (scalingType == DRM_MODE_SCALE_NO_SCALE) {
>  		/* Moorestown doesn't have register support for centering so
>  		 * we need to mess with the h/vblank and h/vsync start and
> diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c b/drivers/gpu/drm/gma500/oaktrail_lvds.c
> index 28b995ef2844..04852dbc7fb3 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_lvds.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c
> @@ -85,7 +85,7 @@ static void oaktrail_lvds_mode_set(struct drm_encoder *encoder,
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  	struct psb_intel_mode_device *mode_dev = &dev_priv->mode_dev;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *connector = NULL;
>  	struct drm_crtc *crtc = encoder->crtc;
>  	u32 lvds_port;
> @@ -112,21 +112,22 @@ static void oaktrail_lvds_mode_set(struct drm_encoder *encoder,
>  	REG_WRITE(LVDS, lvds_port);
>  
>  	/* Find the connector we're trying to set up */
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		if (connector->encoder && connector->encoder->crtc == crtc)
>  			break;
>  	}
>  
> -	if (list_entry_is_head(connector, &mode_config->connector_list, head)) {
> +	if (!connector) {
> +		drm_connector_list_iter_end(&conn_iter);
>  		DRM_ERROR("Couldn't find connector when setting mode");
>  		gma_power_end(dev);
>  		return;
>  	}
>  
> -	drm_object_property_get_value(
> -		&connector->base,
> -		dev->mode_config.scaling_mode_property,
> -		&v);
> +	drm_object_property_get_value( &connector->base,
> +		dev->mode_config.scaling_mode_property, &v);
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	if (v == DRM_MODE_SCALE_NO_SCALE)
>  		REG_WRITE(PFIT_CONTROL, 0);
> diff --git a/drivers/gpu/drm/gma500/psb_device.c b/drivers/gpu/drm/gma500/psb_device.c
> index 59f325165667..71534f4ca834 100644
> --- a/drivers/gpu/drm/gma500/psb_device.c
> +++ b/drivers/gpu/drm/gma500/psb_device.c
> @@ -168,8 +168,10 @@ static void psb_init_pm(struct drm_device *dev)
>  static int psb_save_display_registers(struct drm_device *dev)
>  {
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +	struct gma_connector *gma_connector;
>  	struct drm_crtc *crtc;
> -	struct gma_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
>  	struct psb_state *regs = &dev_priv->regs.psb;
>  
>  	/* Display arbitration control + watermarks */
> @@ -189,9 +191,13 @@ static int psb_save_display_registers(struct drm_device *dev)
>  			dev_priv->ops->save_crtc(crtc);
>  	}
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> -		if (connector->save)
> -			connector->save(&connector->base);
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		gma_connector = to_gma_connector(connector);
> +		if (gma_connector->save)
> +			gma_connector->save(connector);
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	drm_modeset_unlock_all(dev);
>  	return 0;
> @@ -206,8 +212,10 @@ static int psb_save_display_registers(struct drm_device *dev)
>  static int psb_restore_display_registers(struct drm_device *dev)
>  {
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +	struct gma_connector *gma_connector;
>  	struct drm_crtc *crtc;
> -	struct gma_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
>  	struct psb_state *regs = &dev_priv->regs.psb;
>  
>  	/* Display arbitration + watermarks */
> @@ -228,9 +236,13 @@ static int psb_restore_display_registers(struct drm_device *dev)
>  		if (drm_helper_crtc_in_use(crtc))
>  			dev_priv->ops->restore_crtc(crtc);
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> -		if (connector->restore)
> -			connector->restore(&connector->base);
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		gma_connector = to_gma_connector(connector);
> +		if (gma_connector->restore)
> +			gma_connector->restore(connector);
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	drm_modeset_unlock_all(dev);
>  	return 0;
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> index b231fddb8817..bb0e3288e35b 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -236,10 +236,11 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  	unsigned long resource_start, resource_len;
>  	unsigned long irqflags;
> -	int ret = -ENOMEM;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *connector;
>  	struct gma_encoder *gma_encoder;
>  	struct psb_gtt *pg;
> +	int ret = -ENOMEM;
>  
>  	/* initializing driver private data */
>  
> @@ -390,9 +391,9 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
>  	psb_fbdev_init(dev);
>  	drm_kms_helper_poll_init(dev);
>  
> -	/* Only add backlight support if we have LVDS output */
> -	list_for_each_entry(connector, &dev->mode_config.connector_list,
> -			    head) {
> +	/* Only add backlight support if we have LVDS or MIPI output */
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		gma_encoder = gma_attached_encoder(connector);
>  
>  		switch (gma_encoder->type) {
> @@ -402,6 +403,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
>  			break;
>  		}
>  	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
> index a99859b5b13a..fb8234f4d128 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_display.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_display.c
> @@ -106,7 +106,7 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc,
>  	u32 dpll = 0, fp = 0, dspcntr, pipeconf;
>  	bool ok, is_sdvo = false;
>  	bool is_lvds = false, is_tv = false;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *connector;
>  	const struct gma_limit_t *limit;
>  
> @@ -116,7 +116,8 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc,
>  		return 0;
>  	}
>  
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>  
>  		if (!connector->encoder
> @@ -135,6 +136,7 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc,
>  			break;
>  		}
>  	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
>  	refclk = 96000;
>  
> @@ -534,16 +536,19 @@ struct drm_crtc *psb_intel_get_crtc_from_pipe(struct drm_device *dev, int pipe)
>  
>  int gma_connector_clones(struct drm_device *dev, int type_mask)
>  {
> -	int index_mask = 0;
> +	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *connector;
> +	int index_mask = 0;
>  	int entry = 0;
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list,
> -			    head) {
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
>  		struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>  		if (type_mask & (1 << gma_encoder->type))
>  			index_mask |= (1 << entry);
>  		entry++;
>  	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
>  	return index_mask;
>  }
> -- 
> 2.35.1
> 

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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux