Re: [PATCH v3 06/19] drm/i915: Split skl_update_scaler, v3.

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

 



On Mon, Jun 15, 2015 at 12:33:43PM +0200, Maarten Lankhorst wrote:
> It's easier to read separate functions for crtc and plane scaler state.
> 
> Changes since v1:
>  - Update documentation.
> Changes since v2:
>  - Get rid of parameters to skl_update_scaler only used for traces.
>    This avoids needing to document the other parameters.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 211 +++++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_dp.c      |   2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  12 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |   3 +-
>  4 files changed, 121 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0f7652a31c95..26d610acb61f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4303,62 +4303,16 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>  	}
>  }
>  
> -/**
> - * skl_update_scaler_users - Stages update to crtc's scaler state
> - * @intel_crtc: crtc
> - * @crtc_state: crtc_state
> - * @plane: plane (NULL indicates crtc is requesting update)
> - * @plane_state: plane's state
> - * @force_detach: request unconditional detachment of scaler
> - *
> - * This function updates scaler state for requested plane or crtc.
> - * To request scaler usage update for a plane, caller shall pass plane pointer.
> - * To request scaler usage update for crtc, caller shall pass plane pointer
> - * as NULL.
> - *
> - * Return
> - *     0 - scaler_usage updated successfully
> - *    error - requested scaling cannot be supported or other error condition
> - */
> -int
> -skl_update_scaler_users(
> -	struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state,
> -	struct intel_plane *intel_plane, struct intel_plane_state *plane_state,
> -	int force_detach)
> +static int
> +skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
> +		  unsigned scaler_idx, int *scaler_id, unsigned int rotation,
                           ^^^^^^^^^^
This parameter isn't actually the scaler index is it (that's what
scaler_id winds up being once assigned here)?  I think this one is the
plane index that we're assigning a scaler for (or the special value of
SKL_CRTC_INDEX if we're assigning for the CRTC instead of a plane).

Maybe 'scaler_target' or 'scaler_user' would be better?


Matt

> +		  int src_w, int src_h, int dst_w, int dst_h)
>  {
> +	struct intel_crtc_scaler_state *scaler_state =
> +		&crtc_state->scaler_state;
> +	struct intel_crtc *intel_crtc =
> +		to_intel_crtc(crtc_state->base.crtc);
>  	int need_scaling;
> -	int idx;
> -	int src_w, src_h, dst_w, dst_h;
> -	int *scaler_id;
> -	struct drm_framebuffer *fb;
> -	struct intel_crtc_scaler_state *scaler_state;
> -	unsigned int rotation;
> -
> -	if (!intel_crtc || !crtc_state)
> -		return 0;
> -
> -	scaler_state = &crtc_state->scaler_state;
> -
> -	idx = intel_plane ? drm_plane_index(&intel_plane->base) : SKL_CRTC_INDEX;
> -	fb = intel_plane ? plane_state->base.fb : NULL;
> -
> -	if (intel_plane) {
> -		src_w = drm_rect_width(&plane_state->src) >> 16;
> -		src_h = drm_rect_height(&plane_state->src) >> 16;
> -		dst_w = drm_rect_width(&plane_state->dst);
> -		dst_h = drm_rect_height(&plane_state->dst);
> -		scaler_id = &plane_state->scaler_id;
> -		rotation = plane_state->base.rotation;
> -	} else {
> -		struct drm_display_mode *adjusted_mode =
> -			&crtc_state->base.adjusted_mode;
> -		src_w = crtc_state->pipe_src_w;
> -		src_h = crtc_state->pipe_src_h;
> -		dst_w = adjusted_mode->hdisplay;
> -		dst_h = adjusted_mode->vdisplay;
> -		scaler_id = &scaler_state->scaler_id;
> -		rotation = DRM_ROTATE_0;
> -	}
>  
>  	need_scaling = intel_rotation_90_or_270(rotation) ?
>  		(src_h != dst_w || src_w != dst_h):
> @@ -4374,17 +4328,14 @@ skl_update_scaler_users(
>  	 * update to free the scaler is done in plane/panel-fit programming.
>  	 * For this purpose crtc/plane_state->scaler_id isn't reset here.
>  	 */
> -	if (force_detach || !need_scaling || (intel_plane &&
> -		(!fb || !plane_state->visible))) {
> +	if (force_detach || !need_scaling) {
>  		if (*scaler_id >= 0) {
> -			scaler_state->scaler_users &= ~(1 << idx);
> +			scaler_state->scaler_users &= ~(1 << scaler_idx);
>  			scaler_state->scalers[*scaler_id].in_use = 0;
>  
> -			DRM_DEBUG_KMS("Staged freeing scaler id %d.%d from %s:%d "
> -				"crtc_state = %p scaler_users = 0x%x\n",
> -				intel_crtc->pipe, *scaler_id, intel_plane ? "PLANE" : "CRTC",
> -				intel_plane ? intel_plane->base.base.id :
> -				intel_crtc->base.base.id, crtc_state,
> +			DRM_DEBUG_KMS("scaler_user index %u.%u: "
> +				"Staged freeing scaler id %d.%d scaler_users = 0x%x\n",
> +				intel_crtc->pipe, scaler_idx, *scaler_id, scaler_idx,
>  				scaler_state->scaler_users);
>  			*scaler_id = -1;
>  		}
> @@ -4397,51 +4348,112 @@ skl_update_scaler_users(
>  
>  		src_w > SKL_MAX_SRC_W || src_h > SKL_MAX_SRC_H ||
>  		dst_w > SKL_MAX_DST_W || dst_h > SKL_MAX_DST_H) {
> -		DRM_DEBUG_KMS("%s:%d scaler_user index %u.%u: src %ux%u dst %ux%u "
> +		DRM_DEBUG_KMS("scaler_user index %u.%u: src %ux%u dst %ux%u "
>  			"size is out of scaler range\n",
> -			intel_plane ? "PLANE" : "CRTC",
> -			intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
> -			intel_crtc->pipe, idx, src_w, src_h, dst_w, dst_h);
> +			intel_crtc->pipe, scaler_idx, src_w, src_h, dst_w, dst_h);
>  		return -EINVAL;
>  	}
>  
> +	/* mark this plane as a scaler user in crtc_state */
> +	scaler_state->scaler_users |= (1 << scaler_idx);
> +	DRM_DEBUG_KMS("scaler_user index %u.%u: "
> +		"staged scaling request for %ux%u->%ux%u scaler_users = 0x%x\n",
> +		intel_crtc->pipe, scaler_idx, src_w, src_h, dst_w, dst_h,
> +		scaler_state->scaler_users);
> +
> +	return 0;
> +}
> +
> +/**
> + * skl_update_scaler_crtc - Stages update to scaler state for a given crtc.
> + *
> + * @state: crtc's scaler state
> + * @force_detach: whether to forcibly disable scaler
> + *
> + * Return
> + *     0 - scaler_usage updated successfully
> + *    error - requested scaling cannot be supported or other error condition
> + */
> +int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> +	struct drm_display_mode *adjusted_mode =
> +		&state->base.adjusted_mode;
> +
> +	DRM_DEBUG_KMS("Updating scaler for [CRTC:%i] scaler_user index %u.%u\n",
> +		      intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX);
> +
> +	return skl_update_scaler(state, force_detach, SKL_CRTC_INDEX,
> +		&state->scaler_state.scaler_id, DRM_ROTATE_0,
> +		state->pipe_src_w, state->pipe_src_h,
> +		adjusted_mode->hdisplay, adjusted_mode->hdisplay);
> +}
> +
> +/**
> + * skl_update_scaler_plane - Stages update to scaler state for a given plane.
> + *
> + * @state: crtc's scaler state
> + * @intel_plane: affected plane
> + * @plane_state: atomic plane state to update
> + *
> + * Return
> + *     0 - scaler_usage updated successfully
> + *    error - requested scaling cannot be supported or other error condition
> + */
> +int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
> +			    struct intel_plane *intel_plane,
> +			    struct intel_plane_state *plane_state)
> +{
> +
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_framebuffer *fb = plane_state->base.fb;
> +	int ret;
> +
> +	bool force_detach = !fb || !plane_state->visible;
> +
> +	DRM_DEBUG_KMS("Updating scaler for [PLANE:%d] scaler_user index %u.%u\n",
> +		      intel_plane->base.base.id, intel_crtc->pipe,
> +		      drm_plane_index(&intel_plane->base));
> +
> +	ret = skl_update_scaler(crtc_state, force_detach,
> +				drm_plane_index(&intel_plane->base),
> +				&plane_state->scaler_id,
> +				plane_state->base.rotation,
> +				drm_rect_width(&plane_state->src) >> 16,
> +				drm_rect_height(&plane_state->src) >> 16,
> +				drm_rect_width(&plane_state->dst),
> +				drm_rect_height(&plane_state->dst));
> +
> +	if (ret || plane_state->scaler_id < 0)
> +		return ret;
> +
>  	/* check colorkey */
> -	if (WARN_ON(intel_plane &&
> -		intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) {
> -		DRM_DEBUG_KMS("PLANE:%d scaling %ux%u->%ux%u not allowed with colorkey",
> -			intel_plane->base.base.id, src_w, src_h, dst_w, dst_h);
> +	if (WARN_ON(intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) {
> +		DRM_DEBUG_KMS("[PLANE:%d] scaling with color key not allowed",
> +			intel_plane->base.base.id);
>  		return -EINVAL;
>  	}
>  
>  	/* Check src format */
> -	if (intel_plane) {
> -		switch (fb->pixel_format) {
> -		case DRM_FORMAT_RGB565:
> -		case DRM_FORMAT_XBGR8888:
> -		case DRM_FORMAT_XRGB8888:
> -		case DRM_FORMAT_ABGR8888:
> -		case DRM_FORMAT_ARGB8888:
> -		case DRM_FORMAT_XRGB2101010:
> -		case DRM_FORMAT_XBGR2101010:
> -		case DRM_FORMAT_YUYV:
> -		case DRM_FORMAT_YVYU:
> -		case DRM_FORMAT_UYVY:
> -		case DRM_FORMAT_VYUY:
> -			break;
> -		default:
> -			DRM_DEBUG_KMS("PLANE:%d FB:%d unsupported scaling format 0x%x\n",
> -				intel_plane->base.base.id, fb->base.id, fb->pixel_format);
> -			return -EINVAL;
> -		}
> +	switch (fb->pixel_format) {
> +	case DRM_FORMAT_RGB565:
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ABGR8888:
> +	case DRM_FORMAT_ARGB8888:
> +	case DRM_FORMAT_XRGB2101010:
> +	case DRM_FORMAT_XBGR2101010:
> +	case DRM_FORMAT_YUYV:
> +	case DRM_FORMAT_YVYU:
> +	case DRM_FORMAT_UYVY:
> +	case DRM_FORMAT_VYUY:
> +		break;
> +	default:
> +		DRM_DEBUG_KMS("[PLANE:%d] FB:%d unsupported scaling format 0x%x\n",
> +			intel_plane->base.base.id, fb->base.id, fb->pixel_format);
> +		return -EINVAL;
>  	}
>  
> -	/* mark this plane as a scaler user in crtc_state */
> -	scaler_state->scaler_users |= (1 << idx);
> -	DRM_DEBUG_KMS("%s:%d staged scaling request for %ux%u->%ux%u "
> -		"crtc_state = %p scaler_users = 0x%x\n",
> -		intel_plane ? "PLANE" : "CRTC",
> -		intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
> -		src_w, src_h, dst_w, dst_h, crtc_state, scaler_state->scaler_users);
>  	return 0;
>  }
>  
> @@ -4456,7 +4468,7 @@ static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
>  	DRM_DEBUG_KMS("for crtc_state = %p\n", crtc->config);
>  
>  	/* To update pfit, first update scaler state */
> -	skl_update_scaler_users(crtc, crtc->config, NULL, NULL, !enable);
> +	skl_update_scaler_crtc(crtc->config, !enable);
>  	intel_atomic_setup_scalers(crtc->base.dev, crtc, crtc->config);
>  	skl_detach_scalers(crtc);
>  	if (!enable)
> @@ -13647,8 +13659,9 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> -		ret = skl_update_scaler_users(intel_crtc, crtc_state,
> -			to_intel_plane(plane), state, 0);
> +		ret = skl_update_scaler_plane(crtc_state,
> +					      to_intel_plane(plane),
> +					      state);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fb3d7f15c04b..f5ce8fdc8bf9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1377,7 +1377,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  
>  		if (INTEL_INFO(dev)->gen >= 9) {
>  			int ret;
> -			ret = skl_update_scaler_users(intel_crtc, pipe_config, NULL, NULL, 0);
> +			ret = skl_update_scaler_crtc(pipe_config, 0);
>  			if (ret)
>  				return ret;
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 29d6031b19d8..b78fa457a780 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -263,7 +263,7 @@ struct intel_plane_state {
>  	 * plane requiring a scaler:
>  	 *   - During check_plane, its bit is set in
>  	 *     crtc_state->scaler_state.scaler_users by calling helper function
> -	 *     update_scaler_users.
> +	 *     update_scaler_plane.
>  	 *   - scaler_id indicates the scaler it got assigned.
>  	 *
>  	 * plane doesn't require a scaler:
> @@ -271,7 +271,7 @@ struct intel_plane_state {
>  	 *     got disabled.
>  	 *   - During check_plane, corresponding bit is reset in
>  	 *     crtc_state->scaler_state.scaler_users by calling helper function
> -	 *     update_scaler_users.
> +	 *     update_scaler_plane.
>  	 */
>  	int scaler_id;
>  };
> @@ -1147,9 +1147,11 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  void skl_detach_scalers(struct intel_crtc *intel_crtc);
> -int skl_update_scaler_users(struct intel_crtc *intel_crtc,
> -	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
> -	struct intel_plane_state *plane_state, int force_detach);
> +int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
> +			    struct intel_plane *intel_plane,
> +			    struct intel_plane_state *plane_state);
> +
> +int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state, int force_detach);
>  int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
>  
>  unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8193a35388d7..fe95f25f019a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -941,8 +941,7 @@ finish:
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> -		ret = skl_update_scaler_users(intel_crtc, crtc_state, intel_plane,
> -			state, 0);
> +		ret = skl_update_scaler_plane(crtc_state, intel_plane, state);
>  		if (ret)
>  			return ret;
>  	}
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux