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

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

On Fri, Mar 20, 2015 at 04:23:51PM +0530, Jindal, Sonika wrote:
> 
> 
> On 3/20/2015 3:34 PM, Ville Syrjälä wrote:
> > On Fri, Mar 20, 2015 at 03:19:53PM +0530, Jindal, Sonika wrote:
> >>
> >>
> >> 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.
> >
> > Nope. On CHV there is a scaler on one of the planes and there we'll need
> > to write the dst size into PRIMSIZE, and the src size to another register.
> > So just using the dst size for PRIMSIZE seems like the right choice.
> >
> > Also with ILK-IVB sprites the dst size is also in the plane size register,
> > and the source size is in the plane scale register (eg. DVSSIZE and
> > DVSSSCALE), so this way seems more consistent with most of the platforms.
> >
> Then it would be good to have some comment here?

Dunno. To me it's perfectly clear. SKL is the oddball really, so such
a comment would be better placed there. Although I don't feel a need
to have it there either as the code explains it. And if you don't
believe the code you can always check the spec :)

> > Although if we want to expose the pixel/line doubling as fixed 2x
> > scaling we might need to rethink this again. But I'm going to pretend
> > that there's no such thing for now.
> >
> >>>    		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.
> >
> > This is just an assertion to catch anyone passing bogus values here,
> > and a reminder to people that if they want to extend the primary plane
> > capabilities there's more work to be done.
> >
> Hmm, but still this can be part of the check_plane. No problem, you call.

We still call the .update_primary_plane() directly from some places
without check_plane, so better have it here for now.

> >>>    	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.
> >
> > Oh. Failed to notice that. But yeah easy enough to fix when we unify.
> >
> >>
> >> Also, I think it will be better to pass crtc_x, crtc_y also to be used
> >> to set plane position.
> >
> > That's in the next patch.
> >
> Oops, yes :)
> >>
> >> -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));
> >>>    	}
> >>>    }
> >>>
> >>>
> >

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