Re: [PATCH 2/4] drm/i915/glk: Plane color correction register changes

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Tue, 2017-01-24 at 18:16 +0200, Ville Syrjälä wrote:
> On Tue, Jan 24, 2017 at 03:35:53PM +0200, Ander Conselvan de Oliveira wrote:
> > 
> > From: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>
> > 
> > In Geminilake, the bits for enabling pipe csc, pipe gamma and plane
> > gamma moved to a new register. So update the plane update functions
> > to set the right bits.
> > 
> > Pipe CSC is kept disabled though, since enabling that also enables the
> > dedicated degamma table, and that is not properly programmed yet,
> > leading to a black screen.
> > 
> > Signed-off-by: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@inte
> > l.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      | 19 ++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
> >  drivers/gpu/drm/i915/intel_sprite.c  | 19 ++++++++++++++-----
> >  3 files changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 8861683..7c240e4 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5874,6 +5874,13 @@ enum {
> >  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
> >  #define _PLANE_NV12_BUF_CFG_2_A		0x70378
> >  
> > +#define _PLANE_COLOR_CTL_1_A			0x701CC
> > +#define _PLANE_COLOR_CTL_2_A			0x702CC
> > +#define _PLANE_COLOR_CTL_3_A			0x703CC
> I'd prefer that we try to keep the register defines in order based
> on the offset.

Do you mean stuffing this into the list above? I found that less legible, but
ok.

> Have you've tested this stuff on real hw? Bspec seems to have become
> rather illegible as of late, so I had to jump hoops to even find the
> register offsets for these, and even then it seems to be telling me
> that these registers do and do not exist at the same time.

Yep, I tested this with kms_pipe_color, and it did the right thing for the CTM
tests, although I had to workaround it wanting a programmable degamma LUT. I
should probably clean that patch up and submit.

> > 
> > +#define   PLANE_COLOR_PIPE_GAMMA_ENABLE		(1 << 30)
> > +#define   PLANE_COLOR_PIPE_CSC_ENABLE		(1 << 23)
> > +#define   PLANE_COLOR_PLANE_GAMMA_DISABLE	(1 << 13)
> Offsets and bits seem to match what I eventually managed to dig up
> from the spec.
> 
> > 
> > +
> >  #define _PLANE_CTL_1_B				0x71180
> >  #define _PLANE_CTL_2_B				0x71280
> >  #define _PLANE_CTL_3_B				0x71380
> > @@ -5968,7 +5975,17 @@ enum {
> >  #define PLANE_NV12_BUF_CFG(pipe, plane)	\
> >  	_MMIO_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe),
> > _PLANE_NV12_BUF_CFG_2(pipe))
> >  
> > -/* SKL new cursor registers */
> > +#define _PLANE_COLOR_CTL_1_B			0x711CC
> > +#define _PLANE_COLOR_CTL_2_B			0x712CC
> > +#define _PLANE_COLOR_CTL_3_B			0x713CC
> > +#define _PLANE_COLOR_CTL_1(pipe)	\
> > +	_PIPE(pipe, _PLANE_COLOR_CTL_1_A, _PLANE_COLOR_CTL_1_B)
> > +#define _PLANE_COLOR_CTL_2(pipe)	\
> > +	_PIPE(pipe, _PLANE_COLOR_CTL_2_A, _PLANE_COLOR_CTL_2_B)
> > +#define PLANE_COLOR_CTL(pipe, plane)	\
> > +	_MMIO_PLANE(plane, _PLANE_COLOR_CTL_1(pipe),
> > _PLANE_COLOR_CTL_2(pipe))
> > +
> > +#/* SKL new cursor registers */
> >  #define _CUR_BUF_CFG_A				0x7017c
> >  #define _CUR_BUF_CFG_B				0x7117c
> >  #define CUR_BUF_CFG(pipe)	_MMIO_PIPE(pipe, _CUR_BUF_CFG_A,
> > _CUR_BUF_CFG_B)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 0bf8e1b..5313c1f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3386,9 +3386,20 @@ static void skylake_update_primary_plane(struct
> > drm_plane *plane,
> >  	int dst_w = drm_rect_width(&plane_state->base.dst);
> >  	int dst_h = drm_rect_height(&plane_state->base.dst);
> >  
> > -	plane_ctl = PLANE_CTL_ENABLE |
> > -		    PLANE_CTL_PIPE_GAMMA_ENABLE |
> > -		    PLANE_CTL_PIPE_CSC_ENABLE;
> > +	plane_ctl = PLANE_CTL_ENABLE;
> > +
> > +	if (IS_GEMINILAKE(dev_priv)) {
> > +		u32 plane_color =
> > +			PLANE_COLOR_PIPE_GAMMA_ENABLE |
> > +			PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > +
> > +		I915_WRITE(PLANE_COLOR_CTL(pipe, 0), plane_color);
> Use plane_id please.

Will do.

>  'plane_color' seems a little pointless.
> Or are we expecting to OR bits in conditionally?

I found that a bit more readable. Will change.

Ander

> 
> > 
> > +	} 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_tiling(fb->modifier);
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index c05545f..25f49a0 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -219,14 +219,23 @@ skl_update_plane(struct drm_plane *drm_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;
> >  
> > -	plane_ctl = PLANE_CTL_ENABLE |
> > -		PLANE_CTL_PIPE_GAMMA_ENABLE |
> > -		PLANE_CTL_PIPE_CSC_ENABLE;
> > +	plane_ctl = PLANE_CTL_ENABLE;
> > +
> > +	if (IS_GEMINILAKE(dev_priv)) {
> > +		u32 plane_color =
> > +			PLANE_COLOR_PIPE_GAMMA_ENABLE |
> > +			PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > +
> > +		I915_WRITE(PLANE_COLOR_CTL(pipe, plane_id), plane_color);
> > +	} 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_tiling(fb->modifier);
> > -
> > -	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> >  	plane_ctl |= skl_plane_ctl_rotation(rotation);
> >  
> >  	if (key->flags) {
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx