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

[sonika <sonika.jindal@intel.com>]

On Wednesday 11 March 2015 02:57 PM, Ville Syrjälä wrote:
> 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.
>
But we can use that from state right?
>>> +	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));
>>>    	}
>>>    }
>>>    

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx