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

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

On Tue, Mar 10, 2015 at 01:57:09PM -0700, Matt Roper wrote:
> On Tue, Mar 10, 2015 at 07:59:15PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 10, 2015 at 10:10:40AM -0700, Matt Roper wrote:
> > > On Tue, Mar 10, 2015 at 01:15:25PM +0200, 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().
> > > 
> > > Personally I feel like it would make more sense to just completely kill
> > > off .update_primary_plane() now rather than trying to evolve it.  We
> > > already have an intel_plane->update_plane() function pointer which is
> > > never set or called for non-sprites at the moment.  We could unify the
> > > handling of low-level plane programming by just using that function
> > > pointer for primary planes as well.
> > 
> > I want to kill it off as well, but that means either killing off
> > set_base_atomic() or making it use the plane commit hook. I suppose we
> > could hand craft a suitable plane state for it and just commit that
> > without any checks or anything?
> 
> I'm not sure I follow your concern about set_base_atomic().  After your
> patch series, it'll be calling
> 
>    dev_priv->display.update_primary_plane(crtc, fb, x, y,                                                                                                                                                   
>                                           0, 0,                                                                                                                                                             
>                                           intel_crtc->config->pipe_src_w,                                                                                                                                   
>                                           intel_crtc->config->pipe_src_h);                                                                                                                                  
> 
> which basically directly programs the hardware for the primary plane and
> doesn't do anything state-related.
> 
> It seems to me that just making that low-level call be:
> 
>    intel_plane = to_intel_plane(crtc->primary);
>    intel_state = to_intel_plane_state(crtc->primary->state);
> 
>    intel_plane->update_plane(crtc->primary, crtc, fb, intel_fb_obj(fb),
>                              0, 0,
>                              intel_crtc->config->pipe_src_w,
>                              intel_crtc->config->pipe_src_h,
>                              x, y,
>                              drm_rect_width(intel_state->src),
>                              drm_rect_height(intel_state->src));

We can't really use the current plane state here. It might not match what
we want. Although as I indicated with one FIXME in these patches,
set_base_atomic() will do something bad anyway if the fbdev framebuffer
is too small for the current pipe source size. I suppose we could try
to enable the panel fitter to avoid that, but then we run into problems
with managing the panel fitter which is a scarce resource (and there
is only one on gmch platforms and potentially with extra restrictions
on which pipes can use it).

Maybe we should just sanity check that the fbdev framebuffer is suitably
large for the current mode/pfit settings, and do nothing if it's not?
Or we could try to use a plane that can be resized (or potentially even
scaled) to preset the fbdev framebuffer. The other option would be to
throw out set_base_atomic() entirely. I have no idea if it works at all
currently.

And, as you've mentioned .update_plane(), I'm actually thinking we'll be
wanting to pass just a plane state there, and throw out most of the
function arguments as all that data should be in the plane state
already. And then we'd probably want to hand craft the plane state we
pass into .update_plane() for set_base_atomic() (assuming we'll keep it
at all) so that we don't leak fb references and whatnot. But all of
that is material for another set of patches.

> 
> shouldn't make a difference; the implementation of
> intel_plane->update_plane() would still be the same low-level register
> programming without any state manipulation, the only difference being
> that we get an extra pair of parameters containing source rect
> width/height.
> 
> This would also allow us to point both primary and sprite planes at the
> same function on SKL, which has two nearly-identical functions at the
> moment for the lowest-level plane hardware programming.

As I've noted in my reply to Sonika, the primary_enabled flag is the
main complication here. We really need to sort that out before can
fully unify this stuff.

> 
> 
> Matt
> 
> > 
> > > 
> > > I wouldn't mind also seeing the name of that low-level entrypoint
> > > renamed to something like 'update_hw_plane' to avoid confusion with the
> > > drm_plane entrypoint...
> > > 
> > > 
> > > Matt
> > > 
> > > > 
> > > > 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));
> > > > +	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));
> > > >  	}
> > > >  }
> > > >  
> > > > -- 
> > > > 2.0.5
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > IoTG Platform Enabling & Development
> > > Intel Corporation
> > > (916) 356-2795
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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