| Message ID | 20190502094942.21115-1-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/GLK: Properly handle plane CSC for BT2020 framebuffers | expand |
On Thu, May 02, 2019 at 03:19:42PM +0530, Shashank Sharma wrote: > Framebuffer formats P01x are supported by GLK, but the function which > handles CSC on plane color control register, still expectes the input > buffer to be REC709. This can cause inaccurate output for direct P01x > flips. > > This patch checks if the color_encoding property is set to YCBCR_2020, > and enables the corresponding color conversion mode on plane CSC. > > PS: renamed variable plane_color_ctl to color_ctl for 80 char stuff. > > Cc: Ville Syrjala <ville.syrjala@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@intel.com> > Cc: Uma Shankar <uma.shankar@intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index dd65d7c521c1..2d4d3128bf1f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3868,24 +3868,30 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state, > to_i915(plane_state->base.plane->dev); > const struct drm_framebuffer *fb = plane_state->base.fb; > struct intel_plane *plane = to_intel_plane(plane_state->base.plane); > - u32 plane_color_ctl = 0; > + u32 color_ctl = 0; > > - plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; > - plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state); > + color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; > + color_ctl |= glk_plane_color_ctl_alpha(plane_state); > > if (fb->format->is_yuv && !icl_is_hdr_plane(dev_priv, plane->id)) { > - if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) > - plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; > - else > - plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; > + switch (plane_state->base.color_encoding) { > + case DRM_COLOR_YCBCR_BT709: > + color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; > + break; > + case DRM_COLOR_YCBCR_BT2020: > + color_ctl |= PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020; > + break; > + default: > + color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; > + } This isn't going to do anything without adjusting the property supported encodings as well. > > if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE) > - plane_color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; > + color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; > } else if (fb->format->is_yuv) { > - plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; > + color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; > } > > - return plane_color_ctl; > + return color_ctl; > } > > static int > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Thursday, May 2, 2019 3:45 PM > To: Sharma, Shashank <shashank.sharma@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, > Maarten <maarten.lankhorst@intel.com> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/GLK: Properly handle plane CSC for > BT2020 framebuffers > > On Thu, May 02, 2019 at 03:19:42PM +0530, Shashank Sharma wrote: > > Framebuffer formats P01x are supported by GLK, but the function which > > handles CSC on plane color control register, still expectes the input > > buffer to be REC709. This can cause inaccurate output for direct P01x > > flips. > > > > This patch checks if the color_encoding property is set to YCBCR_2020, > > and enables the corresponding color conversion mode on plane CSC. > > > > PS: renamed variable plane_color_ctl to color_ctl for 80 char stuff. > > > > Cc: Ville Syrjala <ville.syrjala@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@intel.com> > > Cc: Uma Shankar <uma.shankar@intel.com> > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++---------- > > 1 file changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index dd65d7c521c1..2d4d3128bf1f 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -3868,24 +3868,30 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state > *crtc_state, > > to_i915(plane_state->base.plane->dev); > > const struct drm_framebuffer *fb = plane_state->base.fb; > > struct intel_plane *plane = to_intel_plane(plane_state->base.plane); > > - u32 plane_color_ctl = 0; > > + u32 color_ctl = 0; > > > > - plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; > > - plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state); > > + color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; > > + color_ctl |= glk_plane_color_ctl_alpha(plane_state); > > > > if (fb->format->is_yuv && !icl_is_hdr_plane(dev_priv, plane->id)) { > > - if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) > > - plane_color_ctl |= > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; > > - else > > - plane_color_ctl |= > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; > > + switch (plane_state->base.color_encoding) { > > + case DRM_COLOR_YCBCR_BT709: > > + color_ctl |= > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; > > + break; > > + case DRM_COLOR_YCBCR_BT2020: > > + color_ctl |= > PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020; > > + break; > > + default: > > + color_ctl |= > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; > > + } > > This isn't going to do anything without adjusting the property supported encodings as > well. > I might have not understood this comment properly, but, AFAIK, if userspace sets this property well, and we set this color_ctl bit properly, driver is setting PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020 bits in GLK color control register. As GLK has a fix function plane CSC, HW will apply a different matrix internally to convert input buffer to RGB_2020 from YCBCR_2020 (earlier this would have been YCBCR_709). So I think we should see visible changes in output. Do you think otherwise ? - Shashank > > > > if (plane_state->base.color_range == > DRM_COLOR_YCBCR_FULL_RANGE) > > - plane_color_ctl |= > PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; > > + color_ctl |= > PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; > > } else if (fb->format->is_yuv) { > > - plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; > > + color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; > > } > > > > - return plane_color_ctl; > > + return color_ctl; > > } > > > > static int > > -- > > 2.17.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Thu, May 02, 2019 at 10:40:39AM +0000, Sharma, Shashank wrote: > > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Thursday, May 2, 2019 3:45 PM > > To: Sharma, Shashank <shashank.sharma@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, > > Maarten <maarten.lankhorst@intel.com> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/GLK: Properly handle plane CSC for > > BT2020 framebuffers > > > > On Thu, May 02, 2019 at 03:19:42PM +0530, Shashank Sharma wrote: > > > Framebuffer formats P01x are supported by GLK, but the function which > > > handles CSC on plane color control register, still expectes the input > > > buffer to be REC709. This can cause inaccurate output for direct P01x > > > flips. > > > > > > This patch checks if the color_encoding property is set to YCBCR_2020, > > > and enables the corresponding color conversion mode on plane CSC. > > > > > > PS: renamed variable plane_color_ctl to color_ctl for 80 char stuff. > > > > > > Cc: Ville Syrjala <ville.syrjala@intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@intel.com> > > > Cc: Uma Shankar <uma.shankar@intel.com> > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++---------- > > > 1 file changed, 16 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index dd65d7c521c1..2d4d3128bf1f 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -3868,24 +3868,30 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state > > *crtc_state, > > > to_i915(plane_state->base.plane->dev); > > > const struct drm_framebuffer *fb = plane_state->base.fb; > > > struct intel_plane *plane = to_intel_plane(plane_state->base.plane); > > > - u32 plane_color_ctl = 0; > > > + u32 color_ctl = 0; > > > > > > - plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; > > > - plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state); > > > + color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; > > > + color_ctl |= glk_plane_color_ctl_alpha(plane_state); > > > > > > if (fb->format->is_yuv && !icl_is_hdr_plane(dev_priv, plane->id)) { > > > - if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) > > > - plane_color_ctl |= > > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; > > > - else > > > - plane_color_ctl |= > > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; > > > + switch (plane_state->base.color_encoding) { > > > + case DRM_COLOR_YCBCR_BT709: > > > + color_ctl |= > > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; > > > + break; > > > + case DRM_COLOR_YCBCR_BT2020: > > > + color_ctl |= > > PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020; > > > + break; > > > + default: > > > + color_ctl |= > > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; > > > + } > > > > This isn't going to do anything without adjusting the property supported encodings as > > well. > > > I might have not understood this comment properly, but, AFAIK, if userspace sets this property well, and we set this color_ctl bit properly, driver is setting PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020 bits in GLK color control register. As GLK has a fix function plane CSC, HW will apply a different matrix internally to convert input buffer to RGB_2020 from YCBCR_2020 (earlier this would have been YCBCR_709). So I think we should see visible changes in output. Do you think otherwise ? The property won't accept the BT2020 value. Or if it does we have a bug somewhere. I guess tests would be nice. Maybe we should extend the kms_plane pixel format tests to check different YCbCr encodings as well? Or maybe Maarten's kms_yuv already tests this?
tor 2019-05-02 klockan 14:51 +0300 skrev Ville Syrjälä: > On Thu, May 02, 2019 at 10:40:39AM +0000, Sharma, Shashank wrote: > > > > > > > -----Original Message----- > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > > Sent: Thursday, May 2, 2019 3:45 PM > > > To: Sharma, Shashank <shashank.sharma@intel.com> > > > Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjal > > > a@intel.com>; Lankhorst, > > > Maarten <maarten.lankhorst@intel.com> > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/GLK: Properly handle > > > plane CSC for > > > BT2020 framebuffers > > > > > > On Thu, May 02, 2019 at 03:19:42PM +0530, Shashank Sharma wrote: > > > > Framebuffer formats P01x are supported by GLK, but the function > > > > which > > > > handles CSC on plane color control register, still expectes the > > > > input > > > > buffer to be REC709. This can cause inaccurate output for > > > > direct P01x > > > > flips. > > > > > > > > This patch checks if the color_encoding property is set to > > > > YCBCR_2020, > > > > and enables the corresponding color conversion mode on plane > > > > CSC. > > > > > > > > PS: renamed variable plane_color_ctl to color_ctl for 80 char > > > > stuff. > > > > > > > > Cc: Ville Syrjala <ville.syrjala@intel.com> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@intel.com> > > > > Cc: Uma Shankar <uma.shankar@intel.com> > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++-- > > > > -------- > > > > 1 file changed, 16 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index dd65d7c521c1..2d4d3128bf1f 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -3868,24 +3868,30 @@ u32 glk_plane_color_ctl(const struct > > > > intel_crtc_state > > > > > > *crtc_state, > > > > to_i915(plane_state->base.plane->dev); > > > > const struct drm_framebuffer *fb = plane_state- > > > > >base.fb; > > > > struct intel_plane *plane = > > > > to_intel_plane(plane_state->base.plane); > > > > - u32 plane_color_ctl = 0; > > > > + u32 color_ctl = 0; > > > > > > > > - plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; > > > > - plane_color_ctl |= > > > > glk_plane_color_ctl_alpha(plane_state); > > > > + color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; > > > > + color_ctl |= glk_plane_color_ctl_alpha(plane_state); > > > > > > > > if (fb->format->is_yuv && !icl_is_hdr_plane(dev_priv, > > > > plane->id)) { > > > > - if (plane_state->base.color_encoding == > > > > DRM_COLOR_YCBCR_BT709) > > > > - plane_color_ctl |= > > > > > > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; > > > > - else > > > > - plane_color_ctl |= > > > > > > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; > > > > + switch (plane_state->base.color_encoding) { > > > > + case DRM_COLOR_YCBCR_BT709: > > > > + color_ctl |= > > > > > > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; > > > > + break; > > > > + case DRM_COLOR_YCBCR_BT2020: > > > > + color_ctl |= > > > > > > PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020; > > > > + break; > > > > + default: > > > > + color_ctl |= > > > > > > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; > > > > + } > > > > > > This isn't going to do anything without adjusting the property > > > supported encodings as > > > well. > > > > > > > I might have not understood this comment properly, but, AFAIK, if > > userspace sets this property well, and we set this color_ctl bit > > properly, driver is setting PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020 > > bits in GLK color control register. As GLK has a fix function plane > > CSC, HW will apply a different matrix internally to convert input > > buffer to RGB_2020 from YCBCR_2020 (earlier this would have been > > YCBCR_709). So I think we should see visible changes in output. Do > > you think otherwise ? > > The property won't accept the BT2020 value. Or if it does we have a > bug > somewhere. > > I guess tests would be nice. Maybe we should extend the kms_plane > pixel > format tests to check different YCbCr encodings as well? Or maybe > Maarten's kms_yuv already tests this? Not yet, unfortunately we have no way to set CSC in igt yet. :( Best way to do so would be to add a igt_create_fb_yuv() which would a igt_create_fb that accepts igt color encoding and range as arguments. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dd65d7c521c1..2d4d3128bf1f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3868,24 +3868,30 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state, to_i915(plane_state->base.plane->dev); const struct drm_framebuffer *fb = plane_state->base.fb; struct intel_plane *plane = to_intel_plane(plane_state->base.plane); - u32 plane_color_ctl = 0; + u32 color_ctl = 0; - plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; - plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state); + color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE; + color_ctl |= glk_plane_color_ctl_alpha(plane_state); if (fb->format->is_yuv && !icl_is_hdr_plane(dev_priv, plane->id)) { - if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) - plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; - else - plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; + switch (plane_state->base.color_encoding) { + case DRM_COLOR_YCBCR_BT709: + color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; + break; + case DRM_COLOR_YCBCR_BT2020: + color_ctl |= PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020; + break; + default: + color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; + } if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE) - plane_color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; + color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; } else if (fb->format->is_yuv) { - plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; + color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; } - return plane_color_ctl; + return color_ctl; } static int
Framebuffer formats P01x are supported by GLK, but the function which handles CSC on plane color control register, still expectes the input buffer to be REC709. This can cause inaccurate output for direct P01x flips. This patch checks if the color_encoding property is set to YCBCR_2020, and enables the corresponding color conversion mode on plane CSC. PS: renamed variable plane_color_ctl to color_ctl for 80 char stuff. Cc: Ville Syrjala <ville.syrjala@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@intel.com> Cc: Uma Shankar <uma.shankar@intel.com> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)