Re: [PATCH 09/14] drm/i915: setup scalers for crtc_compute_config

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Apr 07, 2015 at 03:28:42PM -0700, Chandra Konduru wrote:
> Added intel_atomic_setup_scalers to setup scalers based on
> staged scaling requests from a crtc and its planes. If staged
> requests are supportable, this function assigns scalers to
> requested planes and crtc. Note that the scaler assignement
> itself is staged into crtc_state and respective plane_states
> for later commit after all checks have been done.
> 
> overall high level flow:
>  - scaler requests are staged into crtc_state by planes/crtc
>  - check whether staged scaling requests can be supported
>  - add planes using scalers that aren't in current transaction
>  - assign scalers to requested users
>  - as part of plane commit, scalers will be committed
>    (i.e., either attached or detached) to respective planes in hw
>  - as part of crtc_commit, scaler will be either attached or detached
>    to crtc in hw
> 
> crtc_compute_config calls intel_atomic_setup_scalers() to start
> scaler assignments as per scaler state in crtc config. This call
> should be moved to atomic crtc once it is available.
> 
> v2:
> -removed a log message (me)
> -changed input parameter to crtc_state (me)
> 
> v3:
> -remove assigning plane_state returned by drm_atomic_get_plane_state (Matt)
> -fail if there is an error from drm_atomic_get_plane_state (Matt)
> 
> v4:
> -changes to align with updated scaler structure (Matt, me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  137 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   10 ++-
>  drivers/gpu/drm/i915/intel_drv.h     |    3 +
>  3 files changed, 149 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 3903b90..ac317b4 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -241,3 +241,140 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
>  {
>  	drm_atomic_helper_crtc_destroy_state(crtc, state);
>  }
> +
> +/**
> + * intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
> + * @dev: DRM device
> + * @crtc: intel crtc
> + * @crtc_state: incoming crtc_state to validate and setup scalers
> + *
> + * This function sets up scalers based on staged scaling requests for
> + * a @crtc and its planes. It is called from crtc level check path. If request
> + * is a supportable request, it attaches scalers to requested planes and crtc.
> + *
> + * This function takes into account the current scaler(s) in use by any planes
> + * not being part of this atomic state
> + *
> + *  Returns:
> + *         0 - scalers were setup succesfully
> + *         error code - otherwise
> + */
> +int intel_atomic_setup_scalers(struct drm_device *dev,
> +	struct intel_crtc *intel_crtc,
> +	struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_plane *plane = NULL;
> +	struct intel_plane *intel_plane;
> +	struct intel_plane_state *plane_state = NULL;
> +	struct intel_crtc_scaler_state *scaler_state;
> +	struct drm_atomic_state *drm_state;
> +	int num_scalers_need;
> +	int i, j;
> +
> +	if (INTEL_INFO(dev)->gen < 9 || !intel_crtc || !crtc_state)
> +		return 0;
> +
> +	scaler_state = &crtc_state->scaler_state;
> +	drm_state = crtc_state->base.state;
> +
> +	num_scalers_need = hweight32(scaler_state->scaler_users);
> +	DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users = 0x%x\n",
> +		crtc_state, num_scalers_need, intel_crtc->num_scalers,
> +		scaler_state->scaler_users);
> +
> +	/*
> +	 * High level flow:
> +	 * - staged scaler requests are already in scaler_state->scaler_users
> +	 * - check whether staged scaling requests can be supported
> +	 * - add planes using scalers that aren't in current transaction
> +	 * - assign scalers to requested users
> +	 * - as part of plane commit, scalers will be committed
> +	 *   (i.e., either attached or detached) to respective planes in hw
> +	 * - as part of crtc_commit, scaler will be either attached or detached
> +	 *   to crtc in hw
> +	 */
> +
> +	/* fail if required scalers > available scalers */
> +	if (num_scalers_need > intel_crtc->num_scalers){
> +		DRM_DEBUG_KMS("Too many scaling requests %d > %d\n",
> +			num_scalers_need, intel_crtc->num_scalers);
> +		return -EINVAL;
> +	}
> +
> +	/* walkthrough scaler_users bits and start assigning scalers */
> +	for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
> +		int *scaler_id;
> +
> +		/* skip if scaler not required */
> +		if (!(scaler_state->scaler_users & (1 << i)))
> +			continue;
> +
> +		if (i == SKL_CRTC_INDEX) {
> +			/* panel fitter case: assign as a crtc scaler */
> +			scaler_id = &scaler_state->scaler_id;
> +		} else {
> +			if (!drm_state)
> +				continue;
> +
> +			/* plane scaler case: assign as a plane scaler */
> +			/* find the plane that set the bit as scaler_user */
> +			plane = drm_state->planes[i];
> +
> +			/*
> +			 * to enable/disable hq mode, add planes that are using scaler
> +			 * into this transaction
> +			 */

The code here looks fine, but the reference to "hq mode" kind of comes
out of the blue.  I realize you're setting HQ or DYN down at the bottom
of this function, but there's never really any indication of what those
are or what they mean (or how having the other plane states in the
top-level transaction relates to HQ mode).  I'm assuming what your
comment here is trying to say is that the mode for an already setup and
unchanged plane+scaler might have to change due to changes to the
total number of scalers overall, right?

So maybe just add a paragraph to the commit message explaining the HQ
stuff a little bit?  I think the code itself should be fine so you can
consider it

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

after updating the commit message.


Matt

> +			if (!plane) {
> +				struct drm_plane_state *state;
> +				plane = drm_plane_from_index(dev, i);
> +				state = drm_atomic_get_plane_state(drm_state, plane);
> +				if (IS_ERR(state)) {
> +					DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state\n",
> +						plane->base.id);
> +					return PTR_ERR(state);
> +				}
> +			}
> +
> +			intel_plane = to_intel_plane(plane);
> +
> +			/* plane on different crtc cannot be a scaler user of this crtc */
> +			if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
> +				continue;
> +			}
> +
> +			plane_state = to_intel_plane_state(drm_state->plane_states[i]);
> +			scaler_id = &plane_state->scaler_id;
> +		}
> +
> +		if (*scaler_id < 0) {
> +			/* find a free scaler */
> +			for (j = 0; j < intel_crtc->num_scalers; j++) {
> +				if (!scaler_state->scalers[j].in_use) {
> +					scaler_state->scalers[j].in_use = 1;
> +					*scaler_id = scaler_state->scalers[j].id;
> +					DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
> +						intel_crtc->pipe,
> +						i == SKL_CRTC_INDEX ? scaler_state->scaler_id :
> +							plane_state->scaler_id,
> +						i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
> +						i == SKL_CRTC_INDEX ?  intel_crtc->base.base.id :
> +						plane->base.id);
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (WARN_ON(*scaler_id < 0)) {
> +			DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n",
> +				i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
> +				i == SKL_CRTC_INDEX ? intel_crtc->base.base.id:plane->base.id);
> +			continue;
> +		}
> +
> +		/* set scaler mode */
> +		scaler_state->scalers[*scaler_id].mode = (num_scalers_need == 1) ?
> +			PS_SCALER_MODE_HQ : PS_SCALER_MODE_DYN;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f1051f0..9691768 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5809,6 +5809,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> +	int ret;
>  
>  	/* FIXME should check pixel clock limits on all platforms */
>  	if (INTEL_INFO(dev)->gen < 4) {
> @@ -5863,7 +5864,14 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  	if (pipe_config->has_pch_encoder)
>  		return ironlake_fdi_compute_config(crtc, pipe_config);
>  
> -	return 0;
> +	/* FIXME: remove below call once atomic mode set is place and all crtc
> +	 * related checks called from atomic_crtc_check function */
> +	ret = 0;
> +	DRM_DEBUG_KMS("intel_crtc = %p drm_state (pipe_config->base.state) = %p\n",
> +		crtc, pipe_config->base.state);
> +	ret = intel_atomic_setup_scalers(dev, crtc, pipe_config);
> +
> +	return ret;
>  }
>  
>  static int skylake_get_display_clock_speed(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index adca692..f9614fb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1382,6 +1382,9 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
>  
>  	return to_intel_crtc_state(crtc_state);
>  }
> +int intel_atomic_setup_scalers(struct drm_device *dev,
> +	struct intel_crtc *intel_crtc,
> +	struct intel_crtc_state *crtc_state);
>  
>  /* intel_atomic_plane.c */
>  struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx