| Message ID | 20180222214232.6064-4-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>-----Original Message----- >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com] >Sent: Friday, February 23, 2018 3:13 AM >To: intel-gfx@lists.freedesktop.org >Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma ><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com> >Subject: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >On pre-HSW we have dedicated hardware for the RGB limited range handling, and >so we don't want to compress with the CSC matrix. > >Toss in a FIXME about gamma LUT vs. limited range using the CSC. > >Cc: Johnson Lin <johnson.lin@intel.com> >Cc: Uma Shankar <uma.shankar@intel.com> >Cc: Shashank Sharma <shashank.sharma@intel.com> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >--- > drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > >diff --git a/drivers/gpu/drm/i915/intel_color.c >b/drivers/gpu/drm/i915/intel_color.c >index af1e61d3bacd..89ab0f70aa22 100644 >--- a/drivers/gpu/drm/i915/intel_color.c >+++ b/drivers/gpu/drm/i915/intel_color.c >@@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct drm_crtc_state >*crtc_state) > int i, pipe = intel_crtc->pipe; > uint16_t coeffs[9] = { 0, }; > struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state); >+ bool limited_color_range = false; >+ >+ /* >+ * FIXME if there's a gamma LUT after the CSC, we should >+ * do the range compression using the gamma LUT instead. >+ */ >+ if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) >+ limited_color_range = intel_crtc_state->limited_color_range; > Hi Ville, For VLV or similar platforms (having dedicated color range h/w) for limited_range this matrix would have been getting used. Though they have a dedicated h/w but I don't think it's getting programmed currently. Not sure, with removing this CSC scaling logic, it may break the limited_color scenarios on those platforms. I believe using the dedicated h/w or this scaled_down CSC should be giving similar results making the things work currently. Not sure if we are using any limited_range combinations on those platforms though :) Regards, Uma Shankar > if (intel_crtc_state->ycbcr420) { > ilk_load_ycbcr_conversion_matrix(intel_crtc); >@@ -150,7 +158,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state >*crtc_state) > const u64 *input; > u64 temp[9]; > >- if (intel_crtc_state->limited_color_range) >+ if (limited_color_range) > input = ctm_mult_by_limited(temp, ctm->matrix); > else > input = ctm->matrix; >@@ -200,7 +208,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state >*crtc_state) > * into consideration. > */ > for (i = 0; i < 3; i++) { >- if (intel_crtc_state->limited_color_range) >+ if (limited_color_range) > coeffs[i * 3 + i] = > ILK_CSC_COEFF_LIMITED_RANGE; > else >@@ -224,7 +232,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state >*crtc_state) > if (INTEL_GEN(dev_priv) > 6) { > uint16_t postoff = 0; > >- if (intel_crtc_state->limited_color_range) >+ if (limited_color_range) > postoff = (16 * (1 << 12) / 255) & 0x1fff; > > I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff); @@ -235,7 >+243,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state) > } else { > uint32_t mode = CSC_MODE_YUV_TO_RGB; > >- if (intel_crtc_state->limited_color_range) >+ if (limited_color_range) > mode |= CSC_BLACK_SCREEN_OFFSET; > > I915_WRITE(PIPE_CSC_MODE(pipe), mode); >-- >2.16.1
On Fri, Feb 23, 2018 at 01:33:42PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com] > >Sent: Friday, February 23, 2018 3:13 AM > >To: intel-gfx@lists.freedesktop.org > >Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma > ><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com> > >Subject: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW > > > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >On pre-HSW we have dedicated hardware for the RGB limited range handling, and > >so we don't want to compress with the CSC matrix. > > > >Toss in a FIXME about gamma LUT vs. limited range using the CSC. > > > >Cc: Johnson Lin <johnson.lin@intel.com> > >Cc: Uma Shankar <uma.shankar@intel.com> > >Cc: Shashank Sharma <shashank.sharma@intel.com> > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >--- > > drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_color.c > >b/drivers/gpu/drm/i915/intel_color.c > >index af1e61d3bacd..89ab0f70aa22 100644 > >--- a/drivers/gpu/drm/i915/intel_color.c > >+++ b/drivers/gpu/drm/i915/intel_color.c > >@@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct drm_crtc_state > >*crtc_state) > > int i, pipe = intel_crtc->pipe; > > uint16_t coeffs[9] = { 0, }; > > struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state); > >+ bool limited_color_range = false; > >+ > >+ /* > >+ * FIXME if there's a gamma LUT after the CSC, we should > >+ * do the range compression using the gamma LUT instead. > >+ */ > >+ if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) > >+ limited_color_range = intel_crtc_state->limited_color_range; > > > > Hi Ville, > For VLV or similar platforms (having dedicated color range h/w) for limited_range this matrix > would have been getting used. Though they have a dedicated h/w but I don't think it's getting > programmed currently. Not sure, with removing this CSC scaling logic, it may break the > limited_color scenarios on those platforms. I believe using the dedicated h/w or this scaled_down > CSC should be giving similar results making the things work currently. Not sure if we are using > any limited_range combinations on those platforms though :) All pre-HSW platforms that have the pipe CSC (ILK,SNB,IVB) are using the dedicated hardware for the limited range RGB output. We don't use the CSC on those platforms for anything at the moment so it doesn't actually matter what we program into it. But we want to start using the CSC for ctm and ycbcr444 output which means we have to start setting it up correctly. > > Regards, > Uma Shankar > > > if (intel_crtc_state->ycbcr420) { > > ilk_load_ycbcr_conversion_matrix(intel_crtc); > >@@ -150,7 +158,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state > >*crtc_state) > > const u64 *input; > > u64 temp[9]; > > > >- if (intel_crtc_state->limited_color_range) > >+ if (limited_color_range) > > input = ctm_mult_by_limited(temp, ctm->matrix); > > else > > input = ctm->matrix; > >@@ -200,7 +208,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state > >*crtc_state) > > * into consideration. > > */ > > for (i = 0; i < 3; i++) { > >- if (intel_crtc_state->limited_color_range) > >+ if (limited_color_range) > > coeffs[i * 3 + i] = > > ILK_CSC_COEFF_LIMITED_RANGE; > > else > >@@ -224,7 +232,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state > >*crtc_state) > > if (INTEL_GEN(dev_priv) > 6) { > > uint16_t postoff = 0; > > > >- if (intel_crtc_state->limited_color_range) > >+ if (limited_color_range) > > postoff = (16 * (1 << 12) / 255) & 0x1fff; > > > > I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff); @@ -235,7 > >+243,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state) > > } else { > > uint32_t mode = CSC_MODE_YUV_TO_RGB; > > > >- if (intel_crtc_state->limited_color_range) > >+ if (limited_color_range) > > mode |= CSC_BLACK_SCREEN_OFFSET; > > > > I915_WRITE(PIPE_CSC_MODE(pipe), mode); > >-- > >2.16.1 >
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Friday, February 23, 2018 7:23 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson <johnson.lin@intel.com>; >Sharma, Shashank <shashank.sharma@intel.com> >Subject: Re: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW > >On Fri, Feb 23, 2018 at 01:33:42PM +0000, Shankar, Uma wrote: >> >> >> >-----Original Message----- >> >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com] >> >Sent: Friday, February 23, 2018 3:13 AM >> >To: intel-gfx@lists.freedesktop.org >> >Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma >> ><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com> >> >Subject: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW >> > >> >From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> >On pre-HSW we have dedicated hardware for the RGB limited range >> >handling, and so we don't want to compress with the CSC matrix. >> > >> >Toss in a FIXME about gamma LUT vs. limited range using the CSC. >> > >> >Cc: Johnson Lin <johnson.lin@intel.com> >> >Cc: Uma Shankar <uma.shankar@intel.com> >> >Cc: Shashank Sharma <shashank.sharma@intel.com> >> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >--- >> > drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++---- >> > 1 file changed, 12 insertions(+), 4 deletions(-) >> > >> >diff --git a/drivers/gpu/drm/i915/intel_color.c >> >b/drivers/gpu/drm/i915/intel_color.c >> >index af1e61d3bacd..89ab0f70aa22 100644 >> >--- a/drivers/gpu/drm/i915/intel_color.c >> >+++ b/drivers/gpu/drm/i915/intel_color.c >> >@@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct >> >drm_crtc_state >> >*crtc_state) >> > int i, pipe = intel_crtc->pipe; >> > uint16_t coeffs[9] = { 0, }; >> > struct intel_crtc_state *intel_crtc_state = >> >to_intel_crtc_state(crtc_state); >> >+ bool limited_color_range = false; >> >+ >> >+ /* >> >+ * FIXME if there's a gamma LUT after the CSC, we should >> >+ * do the range compression using the gamma LUT instead. >> >+ */ >> >+ if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) >> >+ limited_color_range = intel_crtc_state->limited_color_range; >> > >> >> Hi Ville, >> For VLV or similar platforms (having dedicated color range h/w) for >> limited_range this matrix would have been getting used. Though they >> have a dedicated h/w but I don't think it's getting programmed >> currently. Not sure, with removing this CSC scaling logic, it may >> break the limited_color scenarios on those platforms. I believe using >> the dedicated h/w or this scaled_down CSC should be giving similar >> results making the things work currently. Not sure if we are using any >> limited_range combinations on those platforms though :) > >All pre-HSW platforms that have the pipe CSC (ILK,SNB,IVB) are using the >dedicated hardware for the limited range RGB output. We don't use the CSC on >those platforms for anything at the moment so it doesn't actually matter what >we program into it. But we want to start using the CSC for ctm and ycbcr444 >output which means we have to start setting it up correctly. > Sounds good. Reviewed-by: Uma Shankar <uma.shankar@intel.com> >> >> Regards, >> Uma Shankar >> >> > if (intel_crtc_state->ycbcr420) { >> > ilk_load_ycbcr_conversion_matrix(intel_crtc); >> >@@ -150,7 +158,7 @@ static void ilk_load_csc_matrix(struct >> >drm_crtc_state >> >*crtc_state) >> > const u64 *input; >> > u64 temp[9]; >> > >> >- if (intel_crtc_state->limited_color_range) >> >+ if (limited_color_range) >> > input = ctm_mult_by_limited(temp, ctm->matrix); >> > else >> > input = ctm->matrix; >> >@@ -200,7 +208,7 @@ static void ilk_load_csc_matrix(struct >> >drm_crtc_state >> >*crtc_state) >> > * into consideration. >> > */ >> > for (i = 0; i < 3; i++) { >> >- if (intel_crtc_state->limited_color_range) >> >+ if (limited_color_range) >> > coeffs[i * 3 + i] = >> > ILK_CSC_COEFF_LIMITED_RANGE; >> > else >> >@@ -224,7 +232,7 @@ static void ilk_load_csc_matrix(struct >> >drm_crtc_state >> >*crtc_state) >> > if (INTEL_GEN(dev_priv) > 6) { >> > uint16_t postoff = 0; >> > >> >- if (intel_crtc_state->limited_color_range) >> >+ if (limited_color_range) >> > postoff = (16 * (1 << 12) / 255) & 0x1fff; >> > >> > I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff); @@ -235,7 >> >+243,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state >> >+*crtc_state) >> > } else { >> > uint32_t mode = CSC_MODE_YUV_TO_RGB; >> > >> >- if (intel_crtc_state->limited_color_range) >> >+ if (limited_color_range) >> > mode |= CSC_BLACK_SCREEN_OFFSET; >> > >> > I915_WRITE(PIPE_CSC_MODE(pipe), mode); >> >-- >> >2.16.1 >> > >-- >Ville Syrjälä >Intel OTC
On Wed, Feb 28, 2018 at 01:11:12PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >Sent: Friday, February 23, 2018 7:23 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson <johnson.lin@intel.com>; > >Sharma, Shashank <shashank.sharma@intel.com> > >Subject: Re: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW > > > >On Fri, Feb 23, 2018 at 01:33:42PM +0000, Shankar, Uma wrote: > >> > >> > >> >-----Original Message----- > >> >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com] > >> >Sent: Friday, February 23, 2018 3:13 AM > >> >To: intel-gfx@lists.freedesktop.org > >> >Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma > >> ><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com> > >> >Subject: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW > >> > > >> >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > > >> >On pre-HSW we have dedicated hardware for the RGB limited range > >> >handling, and so we don't want to compress with the CSC matrix. > >> > > >> >Toss in a FIXME about gamma LUT vs. limited range using the CSC. > >> > > >> >Cc: Johnson Lin <johnson.lin@intel.com> > >> >Cc: Uma Shankar <uma.shankar@intel.com> > >> >Cc: Shashank Sharma <shashank.sharma@intel.com> > >> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> >--- > >> > drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++---- > >> > 1 file changed, 12 insertions(+), 4 deletions(-) > >> > > >> >diff --git a/drivers/gpu/drm/i915/intel_color.c > >> >b/drivers/gpu/drm/i915/intel_color.c > >> >index af1e61d3bacd..89ab0f70aa22 100644 > >> >--- a/drivers/gpu/drm/i915/intel_color.c > >> >+++ b/drivers/gpu/drm/i915/intel_color.c > >> >@@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct > >> >drm_crtc_state > >> >*crtc_state) > >> > int i, pipe = intel_crtc->pipe; > >> > uint16_t coeffs[9] = { 0, }; > >> > struct intel_crtc_state *intel_crtc_state = > >> >to_intel_crtc_state(crtc_state); > >> >+ bool limited_color_range = false; > >> >+ > >> >+ /* > >> >+ * FIXME if there's a gamma LUT after the CSC, we should > >> >+ * do the range compression using the gamma LUT instead. > >> >+ */ > >> >+ if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) > >> >+ limited_color_range = intel_crtc_state->limited_color_range; > >> > > >> > >> Hi Ville, > >> For VLV or similar platforms (having dedicated color range h/w) for > >> limited_range this matrix would have been getting used. Though they > >> have a dedicated h/w but I don't think it's getting programmed > >> currently. Not sure, with removing this CSC scaling logic, it may > >> break the limited_color scenarios on those platforms. I believe using > >> the dedicated h/w or this scaled_down CSC should be giving similar > >> results making the things work currently. Not sure if we are using any > >> limited_range combinations on those platforms though :) > > > >All pre-HSW platforms that have the pipe CSC (ILK,SNB,IVB) are using the > >dedicated hardware for the limited range RGB output. We don't use the CSC on > >those platforms for anything at the moment so it doesn't actually matter what > >we program into it. But we want to start using the CSC for ctm and ycbcr444 > >output which means we have to start setting it up correctly. > > > > Sounds good. > Reviewed-by: Uma Shankar <uma.shankar@intel.com> Thanks. Pushed to dinq.
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index af1e61d3bacd..89ab0f70aa22 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state) int i, pipe = intel_crtc->pipe; uint16_t coeffs[9] = { 0, }; struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state); + bool limited_color_range = false; + + /* + * FIXME if there's a gamma LUT after the CSC, we should + * do the range compression using the gamma LUT instead. + */ + if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) + limited_color_range = intel_crtc_state->limited_color_range; if (intel_crtc_state->ycbcr420) { ilk_load_ycbcr_conversion_matrix(intel_crtc); @@ -150,7 +158,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state) const u64 *input; u64 temp[9]; - if (intel_crtc_state->limited_color_range) + if (limited_color_range) input = ctm_mult_by_limited(temp, ctm->matrix); else input = ctm->matrix; @@ -200,7 +208,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state) * into consideration. */ for (i = 0; i < 3; i++) { - if (intel_crtc_state->limited_color_range) + if (limited_color_range) coeffs[i * 3 + i] = ILK_CSC_COEFF_LIMITED_RANGE; else @@ -224,7 +232,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state) if (INTEL_GEN(dev_priv) > 6) { uint16_t postoff = 0; - if (intel_crtc_state->limited_color_range) + if (limited_color_range) postoff = (16 * (1 << 12) / 255) & 0x1fff; I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff); @@ -235,7 +243,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state) } else { uint32_t mode = CSC_MODE_YUV_TO_RGB; - if (intel_crtc_state->limited_color_range) + if (limited_color_range) mode |= CSC_BLACK_SCREEN_OFFSET; I915_WRITE(PIPE_CSC_MODE(pipe), mode);