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

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

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@intel.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.

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.

> +#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. 'plane_color' seems a little pointless.
Or are we expecting to OR bits in conditionally?

> +	} 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

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