[Intel-gfx] [PATCH] drm/i915: Fix skl srckey mask bits

[Ville Syrjälä]

On Wed, Jan 30, 2019 at 10:25:22AM -0800, Matt Roper wrote:
> On Tue, Jan 29, 2019 at 04:49:19PM +0200, Ville Syrjälä wrote:
> > On Tue, Jan 29, 2019 at 04:33:15PM +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 25, 2019 at 01:54:47PM -0800, Matt Roper wrote:
> > > > On Fri, Jan 25, 2019 at 08:38:46PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > 
> > > > > We're incorrectly masking off the R/V channel enable bit from
> > > > > KEYMSK. Fix it up.
> > > > > 
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > > Fixes: b20815255693 ("drm/i915: Add plane alpha blending support, v2.")
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > 
> > > > The "srckey" terminology in the headline is a bit confusing since the
> > > > channel mask applies to both "destination colorkey" and "source
> > > > colorkey" behavior.
> > > 
> > > Does it? IIRC it used to be only for source keying. I guess I'll have to
> > > try it for reals.
> > 
> > Still seems to affect only the source key.
> 
> Hmm, strange.  I don't see any mention of this only applying to source
> color key in the bspec (neither PLANE_KEYVAL or PLANE_KEYMSK
> descriptions mention it).  I also notice that SNA explicitly sets the
> channel mask (including the 0x7 << 24 bits), even though it only uses
> destination color keying.
> 
> Is this documented somewhere that we're overlooking, or should we file a
> bspec defect to get it added?

It used to be documented. Eg. g4x docs say:

"26 V/R Channel Source Key Enable: Specifies the source color key enable for the V/Red channel.
 25 Y/G Channel Source Key Enable: Specifies the source color key enable for the Y/Green channel.
 24 U/B Channel Source Key Enable: Specifies the source color key enable for the U/Blue channel
 23:16 R mask Dest Key Value: Specifies the destination color key mask for the sprite R channel.
 15:8 G mask Dest Key Value: Specifies the destination color key mask for the sprite G channel.
 7:0 B mask Dest Key Value: Specifies the destination color key mask for the sprite B channel."

Curiously it also says:
"Note that source key and destination key are mutually exclusive modes of
 operation, they can not be used simultaneously. For the function that is
 not enabled, the associated bits in this register should be programmed to
 zeroes."
which we are clearly not following. But I don't know of any issue that
has caused.

Prior to g4x keying was way more confusing as there were a lot more ways
the planes could interact. I think skl sort of went back to the pre-g4x
style by moving the destination keying from the "sprite" to the
"primary" and so it's more like the "src key window" mode of old.
Curiously skl also has that mode, not quite sure what the difference is
between the two tbh. Also vlv/chv are totally in the pre-g4x camp, and
thus implementing destination keying on those is a bit more complicated
(https://patchwork.freedesktop.org/series/43902/).

> 
> 
> Matt
> 
> > 
> > > 
> > > > Otherwise,
> > > > 
> > > > Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> > > > 
> > > > Out of curiousity, has there ever been a userspace consumer of source
> > > > colorkey?  I know SNA uses destination colorkey (for Xv), but not source
> > > > colorkey.  And afaik, UXA never did any colorkeying through the sprite
> > > > interface at all.
> > > 
> > > No, I don't think we have any userspace for this.
> > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_sprite.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > index b02d3d9809e3..cd42e81f8a90 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > @@ -493,7 +493,7 @@ skl_program_plane(struct intel_plane *plane,
> > > > >  
> > > > >  	keymax = (key->max_value & 0xffffff) | PLANE_KEYMAX_ALPHA(alpha);
> > > > >  
> > > > > -	keymsk = key->channel_mask & 0x3ffffff;
> > > > > +	keymsk = key->channel_mask & 0x7ffffff;
> > > > >  	if (alpha < 0xff)
> > > > >  		keymsk |= PLANE_KEYMSK_ALPHA_ENABLE;
> > > > >  
> > > > > -- 
> > > > > 2.19.2
> > > > > 
> > > > 
> > > > -- 
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > IoTG Platform Enabling & Development
> > > > Intel Corporation
> > > > (916) 356-2795
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel