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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Mar 11, 2015 at 10:39:31AM +0530, sonika wrote:
> 
> On Tuesday 10 March 2015 04:45 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > In preparation to movable/resizable primary planes pass the clipped
> > plane size to .update_primary_plane().
> >
> > 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      |  3 +-
> >   drivers/gpu/drm/i915/intel_display.c | 63 ++++++++++++++++++++----------------
> >   2 files changed, 38 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index b16c0a7..e99eef0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -578,7 +578,8 @@ 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 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 fdc83f1..1a789f0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2482,7 +2482,8 @@ 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 crtc_w, int crtc_h)
> >   {
> >   	struct drm_device *dev = crtc->dev;
> >   	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -2517,20 +2518,16 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> >   	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));
> >   		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));
> >   		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 +2583,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 += (crtc_w - 1);
> > +		y += (crtc_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;
> > +			(crtc_h - 1) * fb->pitches[0] +
> > +			(crtc_w - 1) * pixel_size;
> >   	}
> >   
> >   	I915_WRITE(reg, dspcntr);
> > @@ -2611,7 +2608,8 @@ 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 crtc_w, int crtc_h)
> >   {
> >   	struct drm_device *dev = crtc->dev;
> >   	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -2643,6 +2641,10 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >   	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >   		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> >   
> > +	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) {
> >   	case DRM_FORMAT_C8:
> >   		dspcntr |= DISPPLANE_8BPP;
> > @@ -2686,14 +2688,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 += (crtc_w - 1);
> > +			y += (crtc_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;
> > +				(crtc_h - 1) * fb->pitches[0] +
> > +				(crtc_w - 1) * pixel_size;
> >   		}
> >   	}
> >   
> > @@ -2747,7 +2749,8 @@ 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 crtc_w, int crtc_h)
> >   {
> >   	struct drm_device *dev = crtc->dev;
> >   	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -2826,9 +2829,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));
> Shouldn't this be x, y instead of crtc_h and crtc_w?
> Because as you explained earlier (on one of my patches) plane_size 
> represents the size of scanned out area from the fb.

Let me dig up the spec again... You're right, PLANE_SIZE is the input
size. But in this case it doesn't actually matter as there's no scaling
so input and output size are the same.

But yeah, it would be more correct to pass also the src size here and
use that where appropriate. That will make the parameter list look
almost like the sprite .update_plane() function, which is actually a
good thing. We should try to remvoe the primary plane specific functions
and just have one plane update function for each plane "variant". Right
now on SKL we're duplicating basically the same code between
skl_update_primary_plane() and skl_update_plane().

One extra complication is the primary_enabled flag. That needs to be
sorted out somehow before we can unify the primary/sprite code in this
way.

In any case, I'll respin this and pass the src size in there as well.

> > +	I915_WRITE(PLANE_SIZE(pipe, 0), ((crtc_h - 1) << 16) | (crtc_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 +2846,16 @@ 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);
> >   
> >   	return 0;
> >   }
> > @@ -2885,7 +2890,9 @@ 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->dst),
> > +							       drm_rect_height(&state->dst));
> >   		}
> >   
> >   		drm_modeset_unlock(&crtc->mutex);
> > @@ -12019,7 +12026,9 @@ 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->dst),
> > +						       drm_rect_height(&state->dst));
> >   	}
> >   }
> >   

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx