Re: [PATCH 5/7] drm/i915/gen11: Program the chroma upsampler for HDR planes.

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

On Mon, Sep 24, 2018 at 10:38:17AM +0200, Maarten Lankhorst wrote:
> Op 21-09-18 om 20:53 schreef Ville Syrjälä:
> > On Fri, Sep 21, 2018 at 07:39:43PM +0200, Maarten Lankhorst wrote:
> >> We configure the chroma upsampler with the same chroma siting as
> >> used by the scaler for consistency, the chroma upsampler is used
> >> instead of the scaler for YUV 4:2:0 on ICL's HDR planes.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_reg.h     | 22 ++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_sprite.c | 22 ++++++++++++++++++++++
> >>  2 files changed, 44 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index 1b59d15aaf59..b614a06b66c4 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -6560,6 +6560,19 @@ enum {
> >>  #define _PLANE_AUX_DIST_2_A			0x702c0
> >>  #define _PLANE_AUX_OFFSET_1_A			0x701c4
> >>  #define _PLANE_AUX_OFFSET_2_A			0x702c4
> >> +#define _PLANE_CUS_CTL_1_A			0x701c8
> >> +#define _PLANE_CUS_CTL_2_A			0x702c8
> >> +#define  PLANE_CUS_ENABLE			(1 << 31)
> >> +#define  PLANE_CUS_PLANE_6			(0 << 30)
> >> +#define  PLANE_CUS_PLANE_7			(1 << 30)
> >> +#define  PLANE_CUS_HPHASE_SIGN_NEGATIVE		(1 << 19)
> >> +#define  PLANE_CUS_HPHASE_0			(0 << 16)
> >> +#define  PLANE_CUS_HPHASE_0_25			(1 << 16)
> >> +#define  PLANE_CUS_HPHASE_0_5			(2 << 16)
> >> +#define  PLANE_CUS_VPHASE_SIGN_NEGATIVE		(1 << 15)
> >> +#define  PLANE_CUS_VPHASE_0			(0 << 12)
> >> +#define  PLANE_CUS_VPHASE_0_25			(1 << 12)
> >> +#define  PLANE_CUS_VPHASE_0_5			(2 << 12)
> >>  #define _PLANE_COLOR_CTL_1_A			0x701CC /* GLK+ */
> >>  #define _PLANE_COLOR_CTL_2_A			0x702CC /* GLK+ */
> >>  #define _PLANE_COLOR_CTL_3_A			0x703CC /* GLK+ */
> >> @@ -6697,6 +6710,15 @@ enum {
> >>  #define PLANE_AUX_OFFSET(pipe, plane)   \
> >>  	_MMIO_PLANE(plane, _PLANE_AUX_OFFSET_1(pipe), _PLANE_AUX_OFFSET_2(pipe))
> >>  
> >> +#define _PLANE_CUS_CTL_1_B		0x711c8
> >> +#define _PLANE_CUS_CTL_2_B		0x712c8
> >> +#define _PLANE_CUS_CTL_1(pipe)       \
> >> +		_PIPE(pipe, _PLANE_CUS_CTL_1_A, _PLANE_CUS_CTL_1_B)
> >> +#define _PLANE_CUS_CTL_2(pipe)       \
> >> +		_PIPE(pipe, _PLANE_CUS_CTL_2_A, _PLANE_CUS_CTL_2_B)
> >> +#define PLANE_CUS_CTL(pipe, plane)   \
> >> +	_MMIO_PLANE(plane, _PLANE_CUS_CTL_1(pipe), _PLANE_CUS_CTL_2(pipe))
> >> +
> >>  #define _PLANE_COLOR_CTL_1_B			0x711CC
> >>  #define _PLANE_COLOR_CTL_2_B			0x712CC
> >>  #define _PLANE_COLOR_CTL_3_B			0x713CC
> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >> index 111d72a5d5a0..c4e05b0b60bf 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -358,6 +358,7 @@ skl_update_plane(struct intel_plane *plane,
> >>  	uint32_t y = plane_state->color_plane[0].y;
> >>  	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;
> >>  	unsigned long irqflags;
> >>  
> >>  	/* Sizes are 0 based */
> >> @@ -385,6 +386,27 @@ skl_update_plane(struct intel_plane *plane,
> >>  		      (plane_state->color_plane[1].y << 16) |
> >>  		      plane_state->color_plane[1].x);
> >>  
> >> +	if (icl_is_hdr_plane(plane)) {
> >> +		u32 cus_ctl = 0;
> >> +
> >> +		if (linked) {
> >> +			/* Enable and use MPEG-2 chroma siting */
> >> +			cus_ctl = PLANE_CUS_ENABLE |
> >> +				PLANE_CUS_HPHASE_0 |
> >> +				PLANE_CUS_VPHASE_SIGN_NEGATIVE |
> >> +				PLANE_CUS_VPHASE_0_25;
> > Or vphase=-0.5 and hphase=-0.25 maybe? That would be consistent
> > with the phase we program into the normal scaler. But maybe the
> > cus considers works differently?
> 
> Just following what the documentation says in PLANE_CUS_CTL..

I wouldn't trust the docs. They were nonsese for the normal
plane scaler too. Better verify visually.

IIRC I used a hacked version of this for the skl+ stuff:
git://github.com/vsyrjala/intel-gpu-tools.git plane_scaling_basic

> 
> YUV 420 Chroma Siting
> Left (MPEG-2): Hphase: 0, Vphase: -0.25
> 
> >> +
> >> +			if (linked->id == PLANE_SPRITE5)
> >> +				cus_ctl |= PLANE_CUS_PLANE_7;
> >> +			else if (linked->id == PLANE_SPRITE4)
> >> +				cus_ctl |= PLANE_CUS_PLANE_6;
> > PLANE_CUS_SOMETHING(plane) ?
> Considered it, but the field is a bool. So it doesn't work like that.

Oh. I guess the plane_6/7 thing is the best we can do with that then.

> 
> I rather explicitly add the MISSING_CASE() and not hide the fact you can't use
> an arbitrary plane in there.
> >> +			else
> >> +				MISSING_CASE(linked->id);
> >> +		}
> >> +
> >> +		I915_WRITE_FW(PLANE_CUS_CTL(pipe, plane_id), cus_ctl);
> >> +	}
> >> +
> >>  	/* program plane scaler */
> >>  	if (plane_state->scaler_id >= 0) {
> >>  		skl_program_scaler(dev_priv, plane, crtc_state, plane_state);
> >> -- 
> >> 2.18.0
> >>
> >> _______________________________________________
> >> 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