Re: [PATCH 04/14] drm/i915: Polish the skl+ plane keyval/msk/max register setup

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

On Wed, Nov 07, 2018 at 11:55:27AM -0800, Rodrigo Vivi wrote:
> On Thu, Nov 01, 2018 at 05:05:55PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Due to the constant alpha we're going to have to program two of
> > the the tree keying registers anyway, so might as well always
> > program all three.
> > 
> > And parametrize the plane constant alpha define while at it.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I'm not 100% confident we can just ignore the key->flags,
> but I couldn't convince me that we need that either.

The colorkey registers should be nops unless we enable the relevant bits
in the control register (skl_plane_ctl()). If not we'd already have a
bug because we were leaving stale values in those registers when
colorkeying was not enabled.

> 
> And clean-up by itself looks good, so:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> 
> 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h     |  2 +-
> >  drivers/gpu/drm/i915/intel_sprite.c | 22 +++++++++-------------
> >  2 files changed, 10 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8d089ef848b2..b6ee863b5df2 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6545,7 +6545,7 @@ enum {
> >  #define  PLANE_KEYMSK_ALPHA_ENABLE		(1 << 31)
> >  #define _PLANE_KEYMAX_1_A			0x701a0
> >  #define _PLANE_KEYMAX_2_A			0x702a0
> > -#define  PLANE_KEYMAX_ALPHA_SHIFT		24
> > +#define  PLANE_KEYMAX_ALPHA(a)			((a) << 24)
> >  #define _PLANE_AUX_DIST_1_A			0x701c0
> >  #define _PLANE_AUX_DIST_2_A			0x702c0
> >  #define _PLANE_AUX_OFFSET_1_A			0x701c4
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 399d44c57a7d..b36238282b4e 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -382,31 +382,27 @@ skl_program_plane(struct intel_plane *plane,
> >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> >  	struct intel_plane *linked = plane_state->linked_plane;
> > +	u8 alpha = plane_state->base.alpha >> 8;
> >  	unsigned long irqflags;
> > -	u32 keymsk = 0, keymax = 0;
> > +	u32 keymsk, keymax;
> >  
> >  	/* Sizes are 0 based */
> >  	src_w--;
> >  	src_h--;
> >  
> > +	keymax = (key->max_value & 0xffffff) | PLANE_KEYMAX_ALPHA(alpha);
> > +
> > +	keymsk = key->channel_mask & 0x3ffffff;
> > +	if (alpha < 0xff)
> > +		keymsk |= PLANE_KEYMSK_ALPHA_ENABLE;
> > +
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >  
> >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >  		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> >  			      plane_state->color_ctl);
> >  
> > -	if (key->flags) {
> > -		I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> > -
> > -		keymax |= key->max_value & 0xffffff;
> > -		keymsk |= key->channel_mask & 0x3ffffff;
> > -	}
> > -
> > -	keymax |= (plane_state->base.alpha >> 8) << PLANE_KEYMAX_ALPHA_SHIFT;
> > -
> > -	if (plane_state->base.alpha < 0xff00)
> > -		keymsk |= PLANE_KEYMSK_ALPHA_ENABLE;
> > -
> > +	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> >  	I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
> >  	I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
> >  
> > -- 
> > 2.18.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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