Re: [PATCH v2] drm/i915/glk: Refactor handling of PLANE_COLOR_CTL for GLK+

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

On Fri, Nov 03, 2017 at 10:45:30AM -0700, James Ausmus wrote:
> On Fri, Nov 03, 2017 at 07:31:44PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 03, 2017 at 09:58:48AM -0700, James Ausmus wrote:
> > > Since GLK, some plane configuration settings have moved to the
> > > PLANE_COLOR_CTL register. Refactor handling of the register to work like
> > > PLANE_CTL. This also allows us to fix the set/read of the plane Alpha
> > > Mode for GLK+.
> > > 
> > > v2: Adjust ordering of platform checks to be newest->oldest, drop
> > > redundant comment about alpha blending. (Ville)
> > > 
> > > Signed-off-by: James Ausmus <james.ausmus@intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h      | 12 +++++---
> > >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++----
> > >  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 14 +++++----
> > >  4 files changed, 71 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index f0f8f6059652..ecd6b236e005 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6263,7 +6263,7 @@ enum {
> > >  #define _PLANE_CTL_2_A				0x70280
> > >  #define _PLANE_CTL_3_A				0x70380
> > >  #define   PLANE_CTL_ENABLE			(1 << 31)
> > > -#define   PLANE_CTL_PIPE_GAMMA_ENABLE		(1 << 30)
> > > +#define   PLANE_CTL_PIPE_GAMMA_ENABLE		(1 << 30)   /* Pre-GLK */
> > >  #define   PLANE_CTL_FORMAT_MASK			(0xf << 24)
> > >  #define   PLANE_CTL_FORMAT_YUV422		(  0 << 24)
> > >  #define   PLANE_CTL_FORMAT_NV12			(  1 << 24)
> > > @@ -6273,7 +6273,7 @@ enum {
> > >  #define   PLANE_CTL_FORMAT_AYUV			(  8 << 24)
> > >  #define   PLANE_CTL_FORMAT_INDEXED		( 12 << 24)
> > >  #define   PLANE_CTL_FORMAT_RGB_565		( 14 << 24)
> > > -#define   PLANE_CTL_PIPE_CSC_ENABLE		(1 << 23)
> > > +#define   PLANE_CTL_PIPE_CSC_ENABLE		(1 << 23) /* Pre-GLK */
> > >  #define   PLANE_CTL_KEY_ENABLE_MASK		(0x3 << 21)
> > >  #define   PLANE_CTL_KEY_ENABLE_SOURCE		(  1 << 21)
> > >  #define   PLANE_CTL_KEY_ENABLE_DESTINATION	(  2 << 21)
> > > @@ -6286,13 +6286,13 @@ enum {
> > >  #define   PLANE_CTL_YUV422_VYUY			(  3 << 16)
> > >  #define   PLANE_CTL_DECOMPRESSION_ENABLE	(1 << 15)
> > >  #define   PLANE_CTL_TRICKLE_FEED_DISABLE	(1 << 14)
> > > -#define   PLANE_CTL_PLANE_GAMMA_DISABLE		(1 << 13)
> > > +#define   PLANE_CTL_PLANE_GAMMA_DISABLE		(1 << 13) /* Pre-GLK */
> > >  #define   PLANE_CTL_TILED_MASK			(0x7 << 10)
> > >  #define   PLANE_CTL_TILED_LINEAR		(  0 << 10)
> > >  #define   PLANE_CTL_TILED_X			(  1 << 10)
> > >  #define   PLANE_CTL_TILED_Y			(  4 << 10)
> > >  #define   PLANE_CTL_TILED_YF			(  5 << 10)
> > > -#define   PLANE_CTL_ALPHA_MASK			(0x3 << 4)
> > > +#define   PLANE_CTL_ALPHA_MASK			(0x3 << 4) /* Pre-GLK */
> > >  #define   PLANE_CTL_ALPHA_DISABLE		(  0 << 4)
> > >  #define   PLANE_CTL_ALPHA_SW_PREMULTIPLY	(  2 << 4)
> > >  #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
> > > @@ -6332,6 +6332,10 @@ enum {
> > >  #define   PLANE_COLOR_PIPE_GAMMA_ENABLE		(1 << 30)
> > >  #define   PLANE_COLOR_PIPE_CSC_ENABLE		(1 << 23)
> > >  #define   PLANE_COLOR_PLANE_GAMMA_DISABLE	(1 << 13)
> > > +#define   PLANE_COLOR_ALPHA_MASK		(0x3 << 4)
> > > +#define   PLANE_COLOR_ALPHA_DISABLE		(0 << 4)
> > > +#define   PLANE_COLOR_ALPHA_SW_PREMULTIPLY	(2 << 4)
> > > +#define   PLANE_COLOR_ALPHA_HW_PREMULTIPLY	(3 << 4)
> > >  #define _PLANE_BUF_CFG_1_A			0x7027c
> > >  #define _PLANE_BUF_CFG_2_A			0x7037c
> > >  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 737de251d0f8..39453276dad1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3466,6 +3466,23 @@ static u32 skl_plane_ctl_format(uint32_t pixel_format)
> > >  	return 0;
> > >  }
> > >  
> > > +static u32 glk_plane_ctl_format(uint32_t pixel_format)
> > > +{
> > > +	/* GLK+ moves the alpha mask to a different register */
> > > +	return skl_plane_ctl_format(pixel_format) & ~PLANE_CTL_ALPHA_MASK;
> > 
> > I think it would be less confusing to extract the alpha stuff
> > from skl_plane_ctl_format() into skl_plane_ctl_alpha().
> > 
> > I guess with that we wouldn't even need glk_plane_ctl_format()?
> 
> Yeah, I like that better. As a matter of fact, the mask and definitions
> are the same between PLANE_CTL and PLANE_COLOR_CTL - would it be cleaner
> to just rename the defines to PLANE_ALPHA_*, name the function
> skl_plane_alpha, and then be able to use the single function for both
> PLANE_CTL and PLANE_COLOR_CTL usages?

Probably not. At least I was scratching my head again recently with
the DDI_PORT_WIDTH() defines which get used with two otherwise totally
different registers. I cooked up a patch to duplicate the defines to
make sure I won't have to be confused again. Didn't send those out yet
though.

> 
> > 
> > > +}
> > > +
> > > +static u32 glk_plane_color_ctl_alpha(uint32_t pixel_format)
> > > +{
> > > +	switch (pixel_format) {
> > > +	case DRM_FORMAT_ABGR8888:
> > > +	case DRM_FORMAT_ARGB8888:
> > > +		return PLANE_COLOR_ALPHA_SW_PREMULTIPLY;
> > > +	default:
> > > +		return PLANE_COLOR_ALPHA_DISABLE;
> > > +	}
> > > +}
> > > +
> > >  static u32 skl_plane_ctl_tiling(uint64_t fb_modifier)
> > >  {
> > >  	switch (fb_modifier) {
> > > @@ -3522,14 +3539,16 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > >  
> > >  	plane_ctl = PLANE_CTL_ENABLE;
> > >  
> > > -	if (!IS_GEMINILAKE(dev_priv) && !IS_CANNONLAKE(dev_priv)) {
> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> > > +		plane_ctl |= glk_plane_ctl_format(fb->format->format);
> > > +	} else {
> > >  		plane_ctl |=
> > >  			PLANE_CTL_PIPE_GAMMA_ENABLE |
> > >  			PLANE_CTL_PIPE_CSC_ENABLE |
> > >  			PLANE_CTL_PLANE_GAMMA_DISABLE;
> > > +		plane_ctl |= skl_plane_ctl_format(fb->format->format);
> > >  	}
> > >  
> > > -	plane_ctl |= skl_plane_ctl_format(fb->format->format);
> > >  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> > >  	plane_ctl |= skl_plane_ctl_rotation(rotation);
> > >  
> > > @@ -3541,6 +3560,20 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > >  	return plane_ctl;
> > >  }
> > >  
> > > +u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> > > +			const struct intel_plane_state *plane_state)
> > > +{
> > > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> > > +	u32 plane_color_ctl = 0;
> > > +
> > > +	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> > > +	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> > > +	plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > > +	plane_color_ctl |= glk_plane_color_ctl_alpha(fb->format->format);
> > > +
> > > +	return plane_color_ctl;
> > > +}
> > > +
> > >  static int
> > >  __intel_display_resume(struct drm_device *dev,
> > >  		       struct drm_atomic_state *state,
> > > @@ -8426,7 +8459,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> > >  {
> > >  	struct drm_device *dev = crtc->base.dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > -	u32 val, base, offset, stride_mult, tiling;
> > > +	u32 val, base, offset, stride_mult, tiling, alpha;
> > >  	int pipe = crtc->pipe;
> > >  	int fourcc, pixel_format;
> > >  	unsigned int aligned_height;
> > > @@ -8448,9 +8481,16 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> > >  		goto error;
> > >  
> > >  	pixel_format = val & PLANE_CTL_FORMAT_MASK;
> > > +
> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> > > +		alpha = I915_READ(PLANE_COLOR_CTL(pipe, 0));
> > > +		alpha &= PLANE_COLOR_ALPHA_MASK;
> > > +	} else {
> > > +		alpha = val & PLANE_CTL_ALPHA_MASK;
> > > +	}
> > > +
> > >  	fourcc = skl_format_to_fourcc(pixel_format,
> > > -				      val & PLANE_CTL_ORDER_RGBX,
> > > -				      val & PLANE_CTL_ALPHA_MASK);
> > > +				      val & PLANE_CTL_ORDER_RGBX, alpha);
> > >  	fb->format = drm_format_info(fourcc);
> > >  
> > >  	tiling = val & PLANE_CTL_TILED_MASK;
> > > @@ -12831,6 +12871,11 @@ intel_check_primary_plane(struct intel_plane *plane,
> > >  		state->ctl = i9xx_plane_ctl(crtc_state, state);
> > >  	}
> > >  
> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > > +		state->color_ctl = glk_plane_color_ctl(crtc_state, state);
> > > +	else
> > > +		state->color_ctl = 0;
> > 
> > The else branch doesn't seem necessary since we never use color_ctl
> > on other platforms.
> 
> OK - wasn't sure if it was better to leave it uninitialized since it's
> not used, or uselessly initialize it. :)

We zero initialize most things we allocate, and depend on that fact
elsewhere as well.

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