Re: [PATCH 08/10] drm/i915: Split cursor check_plane into i845 and i9xx variants

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

On Tue, Mar 07, 2017 at 10:24:21PM +0000, Chris Wilson wrote:
> On Tue, Mar 07, 2017 at 05:27:07PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The 845/865 and 830/855/9xx+ style cursor don't have that
> > much in common with each other, so let's just split the
> > .check_plane() hook into two variants as well.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 232 ++++++++++++++++++++++-------------
> >  1 file changed, 145 insertions(+), 87 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 222f54ffd113..41cbaee66f1b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9269,6 +9269,74 @@ static void i845_disable_cursor(struct intel_plane *plane,
> >  	i845_update_cursor(plane, NULL, NULL);
> >  }
> >  
> > +static bool i845_cursor_size_ok(const struct intel_plane_state *plane_state)
> > +{
> > +	struct drm_i915_private *dev_priv =
> > +		to_i915(plane_state->base.plane->dev);
> > +	int width = plane_state->base.crtc_w;
> > +	int height = plane_state->base.crtc_h;
> > +
> > +	if (width == 0 || height == 0)
> > +		return false;
> > +
> > +	/*
> > +	 * 845g/865g are only limited by the width of their cursors,
> > +	 * the height is arbitrary up to the precision of the register.
> > +	 */
> > +	if ((width & 63) != 0)
> 
> if (!IS_ALIGNED(width, 64)) ?

Why not.

> 
> > +		return false;
> > +
> > +	if (width > (IS_I845G(dev_priv) ? 64 : 512))
> > +		return false;
> > +
> > +	if (height > 1023)
> > +		return false;
> > +
> > +	return true;
> > +}
> 
> >  static void i9xx_update_cursor(struct intel_plane *plane,
> >  			       const struct intel_crtc_state *crtc_state,
> >  			       const struct intel_plane_state *plane_state)
> > @@ -9328,41 +9396,92 @@ static void i9xx_disable_cursor(struct intel_plane *plane,
> >  	i9xx_update_cursor(plane, NULL, NULL);
> >  }
> >  
> > -static bool cursor_size_ok(struct drm_i915_private *dev_priv,
> > -			   uint32_t width, uint32_t height)
> > +static bool i9xx_cursor_size_ok(const struct intel_plane_state *plane_state)
> >  {
> > +	struct drm_i915_private *dev_priv =
> > +		to_i915(plane_state->base.plane->dev);
> > +	int width = plane_state->base.crtc_w;
> > +	int height = plane_state->base.crtc_h;
> > +
> >  	if (width == 0 || height == 0)
> >  		return false;
> >  
> >  	/*
> > -	 * 845g/865g are special in that they are only limited by
> > -	 * the width of their cursors, the height is arbitrary up to
> > -	 * the precision of the register. Everything else requires
> > -	 * square cursors, limited to a few power-of-two sizes.
> > +	 * Cursors are limited to a few power-of-two
> > +	 * sizes, and they must be square.
> >  	 */
> > -	if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) {
> > -		if ((width & 63) != 0)
> > +	switch (width | height) {
> > +	case 256:
> > +	case 128:
> > +		if (IS_GEN2(dev_priv))
> >  			return false;
> > +	case 64:
> > +		break;
> > +	default:
> > +		return false;
> 
> Checks out ok.
> 
> > +	}
> 
> There's still quite a bit of boilerplate duplication between the two
> check_plane functions. No chance for some sharing? I presume their is
> also an ulterior motive for the split in later patches. That would be
> worth mentioning in the changelog to quell some of the doubts over
> duplication.

I think in the end we're left with the drm_plane_helper_check_state()
call and the modifier check. Those could be done back to back and moved
to a common place. Although I'm sort of hoping the modifier check can
be made to vanish once we get the getplane2 stuff and the driver provides
the core with an explicit list of acceptable modifiers for each plane.
So we might be left with just the check_state() call.

OTOH we're actually missing any and all checks for the src coordinates.
The cursor plane can't pan around freely, so we should add some checks
for that, and putting those into a common place migth be the right
thing. I'm not sure if we should just say you can't pan at all, or
that the panning must happen in whatever chunks CURBASE allows.
Obviously on most platforms that would still only allow vertical
panning in units of several lines. Should be pretty trivial to achieve
that with a call to the compute_tile_offset() thing and a check to make
sure the leftover x/y coordinates are 0.

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