Re: [PATCH v2 5/9] drm/i915: Pass primary plane size to .update_primary_plane()

["Jindal, Sonika" <sonika.jindal@intel.com>]

On 3/19/2015 7:58 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> In preparation to movable/resizeable primary planes pass the clipped
> plane size to .update_primary_plane().
>
> v2: Pass src size too and use it appropriately (Sonika)
>
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |  4 +-
>   drivers/gpu/drm/i915/intel_display.c | 81 ++++++++++++++++++++++++------------
>   2 files changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ec23290..054be42 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -581,7 +581,9 @@ struct drm_i915_display_funcs {
>   			  uint32_t flags);
>   	void (*update_primary_plane)(struct drm_crtc *crtc,
>   				     struct drm_framebuffer *fb,
> -				     int x, int y);
> +				     int x, int y,
> +				     int src_w, int src_h,
> +				     int crtc_w, int crtc_h);
>   	void (*hpd_irq_setup)(struct drm_device *dev);
>   	/* clock updates for mode set */
>   	/* cursor updates */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5499c8e..2e9fdaa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2482,7 +2482,9 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
>
>   static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>   				      struct drm_framebuffer *fb,
> -				      int x, int y)
> +				      int x, int y,
> +				      int src_w, int src_h,
> +				      int crtc_w, int crtc_h)
>   {
>   	struct drm_device *dev = crtc->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2514,23 +2516,22 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>
>   	dspcntr |= DISPLAY_PLANE_ENABLE;
>
> +	WARN_ONCE(src_w != crtc_w || src_h != crtc_h,
> +		  "primary plane doesn't support scaling\n");
> +
>   	if (INTEL_INFO(dev)->gen < 4) {
>   		if (intel_crtc->pipe == PIPE_B)
>   			dspcntr |= DISPPLANE_SEL_PIPE_B;
> -
> -		/* pipesrc and dspsize control the size that is scaled from,
> -		 * which should always be the user's requested size.
> -		 */
> -		I915_WRITE(DSPSIZE(plane),
> -			   ((intel_crtc->config->pipe_src_h - 1) << 16) |
> -			   (intel_crtc->config->pipe_src_w - 1));
> +		I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) << 16) | (crtc_w - 1));
Shouldn't this be src_* here, like you are doing for skl? Again it 
doesn't matter because no scaling. But still it will look better if we 
use same params across.
>   		I915_WRITE(DSPPOS(plane), 0);
>   	} else if (IS_CHERRYVIEW(dev) && plane == PLANE_B) {
> -		I915_WRITE(PRIMSIZE(plane),
> -			   ((intel_crtc->config->pipe_src_h - 1) << 16) |
> -			   (intel_crtc->config->pipe_src_w - 1));
> +		I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) << 16) | (crtc_w - 1));
Same as above.
>   		I915_WRITE(PRIMPOS(plane), 0);
>   		I915_WRITE(PRIMCNSTALPHA(plane), 0);
> +	} else {
> +		WARN_ONCE(crtc_w != intel_crtc->config->pipe_src_w ||
> +			  crtc_h != intel_crtc->config->pipe_src_h,
> +			  "primary plane size doesn't match pipe size\n");
>   	}
>
>   	switch (fb->pixel_format) {
> @@ -2586,14 +2587,14 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>   	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) {
>   		dspcntr |= DISPPLANE_ROTATE_180;
>
> -		x += (intel_crtc->config->pipe_src_w - 1);
> -		y += (intel_crtc->config->pipe_src_h - 1);
> +		x += (src_w - 1);
> +		y += (src_h - 1);
>
>   		/* Finding the last pixel of the last line of the display
>   		data and adding to linear_offset*/
>   		linear_offset +=
> -			(intel_crtc->config->pipe_src_h - 1) * fb->pitches[0] +
> -			(intel_crtc->config->pipe_src_w - 1) * pixel_size;
> +			(src_h - 1) * fb->pitches[0] +
> +			(src_w - 1) * pixel_size;
>   	}
>
>   	I915_WRITE(reg, dspcntr);
> @@ -2611,7 +2612,9 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>
>   static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>   					  struct drm_framebuffer *fb,
> -					  int x, int y)
> +					  int x, int y,
> +					  int src_w, int src_h,
> +					  int crtc_w, int crtc_h)
>   {
>   	struct drm_device *dev = crtc->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2643,6 +2646,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>   	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>   		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
>
> +	WARN_ONCE(src_w != crtc_w || src_h != crtc_h,
> +		  "primary plane doesn't support scaling\n");
> +
> +	WARN_ONCE(crtc_w != intel_crtc->config->pipe_src_w ||
> +		  crtc_h != intel_crtc->config->pipe_src_h,
> +		  "primary plane size doesn't match pipe size\n");
> +
I feel this should be part of the check_plane functions where we already 
perform many size related checks.
>   	switch (fb->pixel_format) {
>   	case DRM_FORMAT_C8:
>   		dspcntr |= DISPPLANE_8BPP;
> @@ -2686,14 +2696,14 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>   		dspcntr |= DISPPLANE_ROTATE_180;
>
>   		if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> -			x += (intel_crtc->config->pipe_src_w - 1);
> -			y += (intel_crtc->config->pipe_src_h - 1);
> +			x += (src_w - 1);
> +			y += (src_h - 1);
>
>   			/* Finding the last pixel of the last line of the display
>   			data and adding to linear_offset*/
>   			linear_offset +=
> -				(intel_crtc->config->pipe_src_h - 1) * fb->pitches[0] +
> -				(intel_crtc->config->pipe_src_w - 1) * pixel_size;
> +				(src_h - 1) * fb->pitches[0] +
> +				(src_w - 1) * pixel_size;
>   		}
>   	}
>
> @@ -2747,7 +2757,9 @@ u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
>
>   static void skylake_update_primary_plane(struct drm_crtc *crtc,
>   					 struct drm_framebuffer *fb,
> -					 int x, int y)
> +					 int x, int y,
> +					 int src_w, int src_h,
> +					 int crtc_w, int crtc_h)
>   {
>   	struct drm_device *dev = crtc->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2767,6 +2779,9 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>   		    PLANE_CTL_PIPE_GAMMA_ENABLE |
>   		    PLANE_CTL_PIPE_CSC_ENABLE;
>
> +	WARN_ONCE(src_w != crtc_w || src_h != crtc_h,
> +		  "primary plane doesn't support scaling\n");
> +
>   	switch (fb->pixel_format) {
>   	case DRM_FORMAT_RGB565:
>   		plane_ctl |= PLANE_CTL_FORMAT_RGB_565;
> @@ -2826,9 +2841,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>
>   	I915_WRITE(PLANE_POS(pipe, 0), 0);
>   	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> -	I915_WRITE(PLANE_SIZE(pipe, 0),
> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
> -		   (intel_crtc->config->pipe_src_w - 1));
> +	I915_WRITE(PLANE_SIZE(pipe, 0), ((src_h - 1) << 16) | (src_w - 1));
>   	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
>   	I915_WRITE(PLANE_SURF(pipe, 0), i915_gem_obj_ggtt_offset(obj));
>
> @@ -2845,12 +2858,18 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>   {
>   	struct drm_device *dev = crtc->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
>   	if (dev_priv->display.disable_fbc)
>   		dev_priv->display.disable_fbc(dev);
>
> +	/* FIXME: this will go badly if the fb isn't big enough */
>   	to_intel_crtc(crtc)->primary_enabled = true;
> -	dev_priv->display.update_primary_plane(crtc, fb, x, y);
> +	dev_priv->display.update_primary_plane(crtc, fb, x, y,
> +					       intel_crtc->config->pipe_src_w,
> +					       intel_crtc->config->pipe_src_h,
> +					       intel_crtc->config->pipe_src_w,
> +					       intel_crtc->config->pipe_src_h);
>
>   	return 0;
>   }
> @@ -2885,7 +2904,11 @@ static void intel_update_primary_planes(struct drm_device *dev)
>   			dev_priv->display.update_primary_plane(crtc,
>   							       state->base.fb,
>   							       state->src.x1 >> 16,
> -							       state->src.y1 >> 16);
> +							       state->src.y1 >> 16,
> +							       drm_rect_width(&state->src) >> 16,
> +							       drm_rect_height(&state->src) >> 16,
> +							       drm_rect_width(&state->dst),
> +							       drm_rect_height(&state->dst));
The order in intel_sprite.c is dst x, y, dest w, h, then src x, y, src 
w, h. But we can sort it later when we are actually merging both the 
update functions for primary and sprites.

Also, I think it will be better to pass crtc_x, crtc_y also to be used 
to set plane position.

-Sonika
>   		}
>
>   		drm_modeset_unlock(&crtc->mutex);
> @@ -12070,7 +12093,11 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   		dev_priv->display.update_primary_plane(crtc,
>   						       state->base.fb,
>   						       state->src.x1 >> 16,
> -						       state->src.y1 >> 16);
> +						       state->src.y1 >> 16,
> +						       drm_rect_width(&state->src) >> 16,
> +						       drm_rect_height(&state->src) >> 16,
> +						       drm_rect_width(&state->dst),
> +						       drm_rect_height(&state->dst));
>   	}
>   }
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx