Re: [PATCH 09/20] drm/i915: Add helper function to update scaler_users in crtc_state

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

On Wed, Apr 01, 2015 at 07:59:38PM -0700, Chandra Konduru wrote:
> This helper function stages a scaler request for a plane/crtc into
> crtc_state->scaler_users (which is a bit field). It also performs
> required checks before staging any change into scaler_state.
> 
> v2:
> -updates to use single copy of scaler limits (Matt)
> -added force detach parameter for pfit disable purpose (me)
> 
> v3:
> -updated function header to kerneldoc format (Matt)
> -dropped need_scaling checks (Matt)
> 
> v4:
> -move clearing of scaler id from commit path to check path (Matt)
> -updated colorkey checks based on recent updates (me)
> -squashed scaler check while enabling colorkey to here (me)
> -use values in plane_state->src as regular integers (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

As noted in the previous review cycle, this should at least be squashed
with patch 17, which is when you start calling this.

> ---
>  drivers/gpu/drm/i915/intel_display.c |  144 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    3 +
>  drivers/gpu/drm/i915/intel_sprite.c  |    9 +++
>  3 files changed, 156 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8b2eff4..603a2dc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12594,6 +12594,150 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	}
>  }
>  
> +/**
> + * 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)
> +{
> +	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;
> +
> +	if (!intel_crtc || !crtc_state ||
> +		(intel_plane && intel_plane->base.type == DRM_PLANE_TYPE_CURSOR))

It doesn't look possible to get here with a cursor plane to me.  Maybe
wrap that plane type test in a WARN_ON() so that we notice if we screw
up?

> +		return 0;
> +
> +	scaler_state = &crtc_state->scaler_state;
> +
> +	if (!scaler_state->num_scalers) {
> +		DRM_DEBUG_KMS("crtc_state = %p, num_scalers = %d\n", crtc_state,
> +			scaler_state->num_scalers);
> +		return 0;
> +	}
> +
> +	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);
> +		src_h = drm_rect_height(&plane_state->src);
> +		dst_w = drm_rect_width(&plane_state->dst);
> +		dst_h = drm_rect_height(&plane_state->dst);
> +		scaler_id = &plane_state->scaler_id;
> +	} 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;
> +	}
> +	need_scaling = (src_w != dst_w || src_h != dst_h);
> +
> +	/*
> +	 * if plane is being disabled or scaler is no more required or force detach
> +	 *  - free scaler binded to this plane/crtc
> +	 *  - in order to do this, update crtc->scaler_usage
> +	 *
> +	 * Here scaler state in crtc_state is set free so that
> +	 * scaler can be assigned to other user. Actual register
> +	 * 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 &&

I guess this force_detach approach is a bit of a necessary evil because
we haven't converted to atomic for full modesets yet (or have real
crtc_state tracking yet).  I'm a little bit worried that it will make
the eventual atomic conversion a bit trickier, but I guess there isn't
much we can do about that if we're planning to merge this series on
today's driver rather than waiting to finish atomic conversion.

Ander is Cc'd on this, and it intersects mainly with his ongoing work,
so if he's okay with how the scaler assignment state is managed with
this, then I am too.

> +		(!fb || !plane_state->visible))) {
> +		if (*scaler_id >= 0) {
> +			scaler_state->scaler_users &= ~(1 << idx);
> +			scaler_state->scalers[*scaler_id].in_use = 0;
> +			*scaler_id = -1;
> +
> +			DRM_DEBUG_KMS("Staged freeing scaler id %u.%u 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,
> +				scaler_state->scaler_users);
> +		}
> +		return 0;
> +	}
> +
> +	/* range checks */
> +	if (src_w < scaler_state->min_src_w || src_h < scaler_state->min_src_h ||
> +		dst_w < scaler_state->min_dst_w || dst_h < scaler_state->min_dst_h ||
> +
> +		src_w > scaler_state->max_src_w || src_h > scaler_state->max_src_h ||
> +		dst_w > scaler_state->max_dst_w || dst_h > scaler_state->max_dst_h) {
> +		DRM_DEBUG_KMS("%s:%d 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);
> +		return -EINVAL;
> +	}
> +
> +	/* check colorkey */
> +	if (intel_plane && 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_ARGB2101010:
> +		case DRM_FORMAT_XBGR2101010:
> +		case DRM_FORMAT_ABGR2101010:
> +		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;
> +}
> +
>  static int
>  intel_check_primary_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e8140c7..e20ddd5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1143,6 +1143,9 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_state *pipe_config);
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
> +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);
>  
>  unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>  				     struct drm_i915_gem_object *obj);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ac4aa68..c8feff7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1119,6 +1119,15 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  	}
>  
>  	intel_plane = to_intel_plane(plane);
> +
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		/* plane scaling and colorkey are mutually exclusive */
> +		if (to_intel_plane_state(plane->state)->scaler_id >= 0) {
> +			DRM_ERROR("colorkey not allowed with scaler\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	intel_plane->ckey = *set;
>  
>  	/*
> -- 
> 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