Re: [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK

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

On Thu, Jun 15, 2017 at 06:58:45PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 6/15/2017 6:12 PM, Ville Syrjälä wrote:
> > On Thu, Jun 15, 2017 at 05:57:21PM +0530, Sharma, Shashank wrote:
> >> Regards
> >> Shashank
> >> On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> On GLK the plane CSC controls moved into the COLOR_CTL register.
> >>> Update the code to progam the YCbCr->RGB CSC mode correctly when
> >>> faced with an YCbCr framebuffer.
> >>>
> >>> The spec is rather confusing as it calls the mode "YUV601 to RGB709".
> >>> I'm going to assume that just means it's going to use the YCbCr->RGB
> >>> matrix as specified in BT.601 and doesn't actually change the gamut.
> >>>
> >>> Cc: Jyri Sarha <jsarha@ti.com>
> >>> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_reg.h      |  5 +++++
> >>>    drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++++++++---
> >>>    drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >>>    drivers/gpu/drm/i915/intel_sprite.c  | 13 +++++--------
> >>>    4 files changed, 28 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index ce7c0dc79cf7..f4f51e7a3e83 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -5924,6 +5924,11 @@ enum {
> >>>    #define _PLANE_COLOR_CTL_3_A			0x703CC /* GLK+ */
> >>>    #define   PLANE_COLOR_PIPE_GAMMA_ENABLE		(1 << 30)
> >>>    #define   PLANE_COLOR_PIPE_CSC_ENABLE		(1 << 23)
> >>> +#define   PLANE_COLOR_CSC_MODE_BYPASS			(0 << 17)
> >>> +#define   PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709		(1 << 17)
> >>> +#define   PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709		(2 << 17)
> >>> +#define   PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020	(3 << 17)
> >>> +#define   PLANE_COLOR_CSC_MODE_RGB709_TO_RGB2020	(4 << 17)
> >>>    #define   PLANE_COLOR_PLANE_GAMMA_DISABLE	(1 << 13)
> >>>    #define _PLANE_BUF_CFG_1_A			0x7027c
> >>>    #define _PLANE_BUF_CFG_2_A			0x7037c
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 25390dd8e08e..e2aaaf60a969 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -3335,6 +3335,21 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >>>    	return plane_ctl;
> >>>    }
> >>>    
> >>> +u32 glk_color_ctl(const struct intel_plane_state *plane_state)
> >>> +{
> >>> +	const struct drm_framebuffer *fb = plane_state->base.fb;
> >>> +	u32 color_ctl;
> >>> +
> >>> +	color_ctl = PLANE_COLOR_PIPE_GAMMA_ENABLE |
> >>> +		PLANE_COLOR_PIPE_CSC_ENABLE |
> >>> +		PLANE_COLOR_PLANE_GAMMA_DISABLE;
> >>> +
> >>> +	if (intel_format_is_yuv(fb->format->format))
> >>> +		color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> >> why not YUV709_TO_RGB709 ?
> > To match the other platforms.
> I am slightly confused. Dint we just upgrade our CSC matrix from stone 
> age, to REC_709, in patch 4 ?
> Shouldn't we be consistent here ? or am I missing something basic :-) ?

This is patch 2, which comes before patch 4. So it's consistent with
the state of the other platforms at the time. Patch 4 will then change
all platforms to BT.709 in one fell swoop.

> 
> - Shashank
> >>> +
> >>> +	return color_ctl;
> >>> +}
> >>> +
> >> You might want to have a look on this series from Dhanya too:
> >>
> >> https://patchwork.freedesktop.org/series/17259/
> > I think we have everything we need from there.
> >
> >> - Shashank
> >>
> >>>    static void skylake_update_primary_plane(struct intel_plane *plane,
> >>>    					 const struct intel_crtc_state *crtc_state,
> >>>    					 const struct intel_plane_state *plane_state)
> >>> @@ -3374,9 +3389,7 @@ static void skylake_update_primary_plane(struct intel_plane *plane,
> >>>    
> >>>    	if (IS_GEMINILAKE(dev_priv)) {
> >>>    		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> >>> -			      PLANE_COLOR_PIPE_GAMMA_ENABLE |
> >>> -			      PLANE_COLOR_PIPE_CSC_ENABLE |
> >>> -			      PLANE_COLOR_PLANE_GAMMA_DISABLE);
> >>> +			      glk_color_ctl(plane_state));
> >>>    	}
> >>>    
> >>>    	I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>> index 83dd40905821..e13e714b1176 100644
> >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>> @@ -1484,6 +1484,7 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
> >>>    
> >>>    u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >>>    		  const struct intel_plane_state *plane_state);
> >>> +u32 glk_color_ctl(const struct intel_plane_state *plane_state);
> >>>    u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
> >>>    		     unsigned int rotation);
> >>>    int skl_check_plane_surface(struct intel_plane_state *plane_state);
> >>> @@ -1888,6 +1889,7 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
> >>>    
> >>>    
> >>>    /* intel_sprite.c */
> >>> +bool intel_format_is_yuv(u32 format);
> >>>    int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
> >>>    			     int usecs);
> >>>    struct intel_plane *intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>> index 2ce3b3c6ffa8..cc07157f2eb6 100644
> >>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>> @@ -40,8 +40,7 @@
> >>>    #include <drm/i915_drm.h>
> >>>    #include "i915_drv.h"
> >>>    
> >>> -static bool
> >>> -format_is_yuv(uint32_t format)
> >>> +bool intel_format_is_yuv(u32 format)
> >>>    {
> >>>    	switch (format) {
> >>>    	case DRM_FORMAT_YUYV:
> >>> @@ -264,9 +263,7 @@ skl_update_plane(struct intel_plane *plane,
> >>>    
> >>>    	if (IS_GEMINILAKE(dev_priv)) {
> >>>    		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> >>> -			      PLANE_COLOR_PIPE_GAMMA_ENABLE |
> >>> -			      PLANE_COLOR_PIPE_CSC_ENABLE |
> >>> -			      PLANE_COLOR_PLANE_GAMMA_DISABLE);
> >>> +			      glk_color_ctl(plane_state));
> >>>    	}
> >>>    
> >>>    	if (key->flags) {
> >>> @@ -333,7 +330,7 @@ chv_update_csc(const struct intel_plane_state *plane_state)
> >>>    	enum plane_id plane_id = plane->id;
> >>>    
> >>>    	/* Seems RGB data bypasses the CSC always */
> >>> -	if (!format_is_yuv(fb->format->format))
> >>> +	if (!intel_format_is_yuv(fb->format->format))
> >>>    		return;
> >>>    
> >>>    	/*
> >>> @@ -375,7 +372,7 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
> >>>    	enum plane_id plane_id = plane->id;
> >>>    	int con, bri, sh_sin, sh_cos;
> >>>    
> >>> -	if (format_is_yuv(fb->format->format)) {
> >>> +	if (intel_format_is_yuv(fb->format->format)) {
> >>>    		/*
> >>>    		 * expand limited range to full range.
> >>>    		 * contrast is applied first, then brightness
> >>> @@ -933,7 +930,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
> >>>    		src_y = src->y1 >> 16;
> >>>    		src_h = drm_rect_height(src) >> 16;
> >>>    
> >>> -		if (format_is_yuv(fb->format->format)) {
> >>> +		if (intel_format_is_yuv(fb->format->format)) {
> >>>    			src_x &= ~1;
> >>>    			src_w &= ~1;
> >>>    

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