Re: [PATCH 1/2] drm: provide a helper for the encoder possible_crtcs mask

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

On Mon, 13 Jan 2014, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Russell King <rmk+kernel@arm.linux.org.uk>
>
> The encoder possible_crtcs mask identifies which CRTCs can be bound to
> a particular encoder.  Each bit from bit 0 defines an index in the list
> of CRTCs held in the DRM mode_config crtc_list.  Rather than having
> drivers trying to track the position of their CRTCs in the list, expose
> the code which already exists for calculating the appropriate mask bit
> for a CRTC.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> [treding@nvidia.com: add drm_crtc_index(), move to core]
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - further extract a new drm_crtc_index() function
> - move to DRM core (drm_crtc.c)
>
>  drivers/gpu/drm/drm_crtc.c        | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_helper.c | 16 +++-------------
>  include/drm/drm_crtc.h            |  2 ++
>  3 files changed, 43 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 266a01d7f635..fdfa5a0e51d6 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -675,6 +675,44 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
>  EXPORT_SYMBOL(drm_crtc_cleanup);
>  
>  /**
> + * drm_crtc_index - find the index of a registered CRTC
> + * @crtc: CRTC to find index for
> + *
> + * Given a registered CRTC, return the index of that CRTC within a DRM
> + * device's list of CRTCs. If the CRTC isn't registered, return -1.
> + */
> +int drm_crtc_index(struct drm_crtc *crtc)
> +{
> +	struct drm_crtc *tmp;
> +	int index = 0;
> +
> +	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
> +		if (tmp == crtc)
> +			return index;
> +
> +		index++;
> +	}
> +
> +	return -1;

I share David's opinion of using BUG() here.

> +}
> +EXPORT_SYMBOL(drm_crtc_index);
> +
> +/**
> + * drm_crtc_mask - find the mask of a registered CRTC
> + * @crtc: CRTC to find mask for
> + *
> + * Given a registered CRTC, return the mask bit of that CRTC for an
> + * encoder's possible_crtcs field.
> + */
> +uint32_t drm_crtc_mask(struct drm_crtc *crtc)
> +{
> +	int i = drm_crtc_index(crtc);
> +
> +	return i < 0 ? 0 : 1 << i;
> +}
> +EXPORT_SYMBOL(drm_crtc_mask);
> +
> +/**
>   * drm_mode_probed_add - add a mode to a connector's probed mode list
>   * @connector: connector the new mode
>   * @mode: mode data
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 01361aba033b..95b32098badf 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -334,23 +334,13 @@ EXPORT_SYMBOL(drm_helper_disable_unused_functions);
>  static bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
>  				struct drm_crtc *crtc)
>  {
> -	struct drm_device *dev;
> -	struct drm_crtc *tmp;
> -	int crtc_mask = 1;
> +	uint32_t crtc_mask;
>  
>  	WARN(!crtc, "checking null crtc?\n");

You could drop this while at it, the code will neatly oops a moment
later anyway if crtc == NULL.

Otherwise, both patches are

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  
> -	dev = crtc->dev;
> -
> -	list_for_each_entry(tmp, &dev->mode_config.crtc_list, head) {
> -		if (tmp == crtc)
> -			break;
> -		crtc_mask <<= 1;
> -	}
> +	crtc_mask = drm_crtc_mask(crtc);
>  
> -	if (encoder->possible_crtcs & crtc_mask)
> -		return true;
> -	return false;
> +	return !!(encoder->possible_crtcs & crtc_mask);
>  }
>  
>  /*
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f32c5cd51f41..a61fb73fdcb5 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -929,6 +929,8 @@ extern int drm_crtc_init(struct drm_device *dev,
>  			 struct drm_crtc *crtc,
>  			 const struct drm_crtc_funcs *funcs);
>  extern void drm_crtc_cleanup(struct drm_crtc *crtc);
> +extern int drm_crtc_index(struct drm_crtc *crtc);
> +extern uint32_t drm_crtc_mask(struct drm_crtc *crtc);
>  
>  extern void drm_connector_ida_init(void);
>  extern void drm_connector_ida_destroy(void);
> -- 
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center