| Message ID | 20190318161317.30918-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Clean up intel_color_check() | expand |
On Mon, 18 Mar 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > In prepartion for per-platform color_check() functions extract the > common code into a separate function. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------ > 1 file changed, 40 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index 467fd1a1630c..b6a992a5b14f 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) > return 0; > } > > +static int check_luts(const struct intel_crtc_state *crtc_state) luts_check() to be more in line with other _commit and _check functions? BR, Jani. > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > + const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > + const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > + int gamma_length, degamma_length; > + u32 gamma_tests, degamma_tests; > + > + /* Always allow legacy gamma LUT with no further checking. */ > + if (crtc_state_is_legacy_gamma(crtc_state)) > + return 0; > + > + /* C8 needs the legacy LUT all to itself */ > + if (crtc_state->c8_planes) > + return -EINVAL; > + > + degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > + gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > + degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > + gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > + > + if (check_lut_size(degamma_lut, degamma_length) || > + check_lut_size(gamma_lut, gamma_length)) > + return -EINVAL; > + > + if (drm_color_lut_check(degamma_lut, degamma_tests) || > + drm_color_lut_check(gamma_lut, gamma_tests)) > + return -EINVAL; > + > + return 0; > +} > + > static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) > { > u32 cgm_mode = 0; > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > bool limited_color_range = false; > - int gamma_length, degamma_length; > - u32 gamma_tests, degamma_tests; > int ret; > > - degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > - gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > - > - /* C8 needs the legacy LUT all to itself */ > - if (crtc_state->c8_planes && > - !crtc_state_is_legacy_gamma(crtc_state)) > - return -EINVAL; > + ret = check_luts(crtc_state); > + if (ret) > + return ret; > > crtc_state->gamma_enable = (gamma_lut || degamma_lut) && > !crtc_state->c8_planes; > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > if (IS_CHERRYVIEW(dev_priv)) > crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > > - /* Always allow legacy gamma LUT with no further checking. */ > if (!crtc_state->gamma_enable || > - crtc_state_is_legacy_gamma(crtc_state)) { > + crtc_state_is_legacy_gamma(crtc_state)) > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > - if (INTEL_GEN(dev_priv) >= 11 && > - crtc_state->gamma_enable) > - crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > - return 0; > - } > - > - if (check_lut_size(degamma_lut, degamma_length) || > - check_lut_size(gamma_lut, gamma_length)) > - return -EINVAL; > - > - if (drm_color_lut_check(degamma_lut, degamma_tests) || > - drm_color_lut_check(gamma_lut, gamma_tests)) > - return -EINVAL; > - > - if (INTEL_GEN(dev_priv) >= 11) > + else if (INTEL_GEN(dev_priv) >= 11) > crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT | > PRE_CSC_GAMMA_ENABLE | > POST_CSC_GAMMA_ENABLE; > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > if (INTEL_GEN(dev_priv) >= 11) { > + if (crtc_state->gamma_enable) > + crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > + > if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || > crtc_state->limited_color_range) > crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > In prepartion for per-platform color_check() functions extract the > common code into a separate function. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------ > 1 file changed, 40 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index 467fd1a1630c..b6a992a5b14f 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) > return 0; > } > > +static int check_luts(const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > + const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > + const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > + int gamma_length, degamma_length; > + u32 gamma_tests, degamma_tests; > + > + /* Always allow legacy gamma LUT with no further checking. */ > + if (crtc_state_is_legacy_gamma(crtc_state)) > + return 0; > + > + /* C8 needs the legacy LUT all to itself */ > + if (crtc_state->c8_planes) > + return -EINVAL; Do we need to test for non-NULL gamma_lut here? The logic is the same as before your patch, so either we already had a pre-existing bug or, more likely, I'm overlooking something obvious. > + > + degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > + gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > + degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > + gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > + > + if (check_lut_size(degamma_lut, degamma_length) || > + check_lut_size(gamma_lut, gamma_length)) > + return -EINVAL; > + > + if (drm_color_lut_check(degamma_lut, degamma_tests) || > + drm_color_lut_check(gamma_lut, gamma_tests)) > + return -EINVAL; > + > + return 0; > +} > + > static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) > { > u32 cgm_mode = 0; > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > bool limited_color_range = false; > - int gamma_length, degamma_length; > - u32 gamma_tests, degamma_tests; > int ret; > > - degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > - gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > - > - /* C8 needs the legacy LUT all to itself */ > - if (crtc_state->c8_planes && > - !crtc_state_is_legacy_gamma(crtc_state)) > - return -EINVAL; > + ret = check_luts(crtc_state); > + if (ret) > + return ret; > > crtc_state->gamma_enable = (gamma_lut || degamma_lut) && > !crtc_state->c8_planes; > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > if (IS_CHERRYVIEW(dev_priv)) > crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > > - /* Always allow legacy gamma LUT with no further checking. */ > if (!crtc_state->gamma_enable || > - crtc_state_is_legacy_gamma(crtc_state)) { > + crtc_state_is_legacy_gamma(crtc_state)) > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > - if (INTEL_GEN(dev_priv) >= 11 && > - crtc_state->gamma_enable) > - crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > - return 0; > - } > - > - if (check_lut_size(degamma_lut, degamma_length) || > - check_lut_size(gamma_lut, gamma_length)) > - return -EINVAL; > - > - if (drm_color_lut_check(degamma_lut, degamma_tests) || > - drm_color_lut_check(gamma_lut, gamma_tests)) > - return -EINVAL; > - > - if (INTEL_GEN(dev_priv) >= 11) > + else if (INTEL_GEN(dev_priv) >= 11) > crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT | > PRE_CSC_GAMMA_ENABLE | > POST_CSC_GAMMA_ENABLE; Should we drop the GAMMA_ENABLE here and just rely on the new check you're adding below to handle it? Matt > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > if (INTEL_GEN(dev_priv) >= 11) { > + if (crtc_state->gamma_enable) > + crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > + > if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || > crtc_state->limited_color_range) > crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE; > -- > 2.19.2 >
On Tue, Mar 19, 2019 at 02:59:38PM -0700, Matt Roper wrote: > On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > In prepartion for per-platform color_check() functions extract the > > common code into a separate function. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------ > > 1 file changed, 40 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > > index 467fd1a1630c..b6a992a5b14f 100644 > > --- a/drivers/gpu/drm/i915/intel_color.c > > +++ b/drivers/gpu/drm/i915/intel_color.c > > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) > > return 0; > > } > > > > +static int check_luts(const struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > + const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > + const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > + int gamma_length, degamma_length; > > + u32 gamma_tests, degamma_tests; > > + > > + /* Always allow legacy gamma LUT with no further checking. */ > > + if (crtc_state_is_legacy_gamma(crtc_state)) > > + return 0; > > + > > + /* C8 needs the legacy LUT all to itself */ > > + if (crtc_state->c8_planes) > > + return -EINVAL; > > Do we need to test for non-NULL gamma_lut here? For c8? We already checked crtc_state_is_legacy_gamma() above. > > The logic is the same as before your patch, so either we already had a > pre-existing bug or, more likely, I'm overlooking something obvious. > > > + > > + degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > + gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > + degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > + gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > + > > + if (check_lut_size(degamma_lut, degamma_length) || > > + check_lut_size(gamma_lut, gamma_length)) > > + return -EINVAL; > > + > > + if (drm_color_lut_check(degamma_lut, degamma_tests) || > > + drm_color_lut_check(gamma_lut, gamma_tests)) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) > > { > > u32 cgm_mode = 0; > > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > bool limited_color_range = false; > > - int gamma_length, degamma_length; > > - u32 gamma_tests, degamma_tests; > > int ret; > > > > - degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > - gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > - > > - /* C8 needs the legacy LUT all to itself */ > > - if (crtc_state->c8_planes && > > - !crtc_state_is_legacy_gamma(crtc_state)) > > - return -EINVAL; > > + ret = check_luts(crtc_state); > > + if (ret) > > + return ret; > > > > crtc_state->gamma_enable = (gamma_lut || degamma_lut) && > > !crtc_state->c8_planes; > > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > if (IS_CHERRYVIEW(dev_priv)) > > crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > > > > - /* Always allow legacy gamma LUT with no further checking. */ > > if (!crtc_state->gamma_enable || > > - crtc_state_is_legacy_gamma(crtc_state)) { > > + crtc_state_is_legacy_gamma(crtc_state)) > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > - if (INTEL_GEN(dev_priv) >= 11 && > > - crtc_state->gamma_enable) > > - crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > - return 0; > > - } > > - > > - if (check_lut_size(degamma_lut, degamma_length) || > > - check_lut_size(gamma_lut, gamma_length)) > > - return -EINVAL; > > - > > - if (drm_color_lut_check(degamma_lut, degamma_tests) || > > - drm_color_lut_check(gamma_lut, gamma_tests)) > > - return -EINVAL; > > - > > - if (INTEL_GEN(dev_priv) >= 11) > > + else if (INTEL_GEN(dev_priv) >= 11) > > crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT | > > PRE_CSC_GAMMA_ENABLE | > > POST_CSC_GAMMA_ENABLE; > > Should we drop the GAMMA_ENABLE here and just rely on the new check > you're adding below to handle it? This will get rewritten entirely in following patches, so I figured I'd just keep this patch minimal. > > > Matt > > > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > > > if (INTEL_GEN(dev_priv) >= 11) { > > + if (crtc_state->gamma_enable) > > + crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > + > > if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || > > crtc_state->limited_color_range) > > crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE; > > -- > > 2.19.2 > > > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
On Tue, Mar 19, 2019 at 06:24:47PM +0200, Jani Nikula wrote: > On Mon, 18 Mar 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > In prepartion for per-platform color_check() functions extract the > > common code into a separate function. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------ > > 1 file changed, 40 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > > index 467fd1a1630c..b6a992a5b14f 100644 > > --- a/drivers/gpu/drm/i915/intel_color.c > > +++ b/drivers/gpu/drm/i915/intel_color.c > > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) > > return 0; > > } > > > > +static int check_luts(const struct intel_crtc_state *crtc_state) > > luts_check() to be more in line with other _commit and _check functions? Shrug. For some reason check_luts() feels more natural to me. > > BR, > Jani. > > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > + const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > + const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > + int gamma_length, degamma_length; > > + u32 gamma_tests, degamma_tests; > > + > > + /* Always allow legacy gamma LUT with no further checking. */ > > + if (crtc_state_is_legacy_gamma(crtc_state)) > > + return 0; > > + > > + /* C8 needs the legacy LUT all to itself */ > > + if (crtc_state->c8_planes) > > + return -EINVAL; > > + > > + degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > + gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > + degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > + gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > + > > + if (check_lut_size(degamma_lut, degamma_length) || > > + check_lut_size(gamma_lut, gamma_length)) > > + return -EINVAL; > > + > > + if (drm_color_lut_check(degamma_lut, degamma_tests) || > > + drm_color_lut_check(gamma_lut, gamma_tests)) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) > > { > > u32 cgm_mode = 0; > > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > bool limited_color_range = false; > > - int gamma_length, degamma_length; > > - u32 gamma_tests, degamma_tests; > > int ret; > > > > - degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > - gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > - > > - /* C8 needs the legacy LUT all to itself */ > > - if (crtc_state->c8_planes && > > - !crtc_state_is_legacy_gamma(crtc_state)) > > - return -EINVAL; > > + ret = check_luts(crtc_state); > > + if (ret) > > + return ret; > > > > crtc_state->gamma_enable = (gamma_lut || degamma_lut) && > > !crtc_state->c8_planes; > > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > if (IS_CHERRYVIEW(dev_priv)) > > crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > > > > - /* Always allow legacy gamma LUT with no further checking. */ > > if (!crtc_state->gamma_enable || > > - crtc_state_is_legacy_gamma(crtc_state)) { > > + crtc_state_is_legacy_gamma(crtc_state)) > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > - if (INTEL_GEN(dev_priv) >= 11 && > > - crtc_state->gamma_enable) > > - crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > - return 0; > > - } > > - > > - if (check_lut_size(degamma_lut, degamma_length) || > > - check_lut_size(gamma_lut, gamma_length)) > > - return -EINVAL; > > - > > - if (drm_color_lut_check(degamma_lut, degamma_tests) || > > - drm_color_lut_check(gamma_lut, gamma_tests)) > > - return -EINVAL; > > - > > - if (INTEL_GEN(dev_priv) >= 11) > > + else if (INTEL_GEN(dev_priv) >= 11) > > crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT | > > PRE_CSC_GAMMA_ENABLE | > > POST_CSC_GAMMA_ENABLE; > > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > > > if (INTEL_GEN(dev_priv) >= 11) { > > + if (crtc_state->gamma_enable) > > + crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > + > > if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || > > crtc_state->limited_color_range) > > crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE; > > -- > Jani Nikula, Intel Open Source Graphics Center
On Wed, Mar 20, 2019 at 04:41:31PM +0200, Ville Syrjälä wrote: > On Tue, Mar 19, 2019 at 02:59:38PM -0700, Matt Roper wrote: > > On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > In prepartion for per-platform color_check() functions extract the > > > common code into a separate function. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------ > > > 1 file changed, 40 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > > > index 467fd1a1630c..b6a992a5b14f 100644 > > > --- a/drivers/gpu/drm/i915/intel_color.c > > > +++ b/drivers/gpu/drm/i915/intel_color.c > > > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) > > > return 0; > > > } > > > > > > +static int check_luts(const struct intel_crtc_state *crtc_state) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > > + const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > > + const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > > + int gamma_length, degamma_length; > > > + u32 gamma_tests, degamma_tests; > > > + > > > + /* Always allow legacy gamma LUT with no further checking. */ > > > + if (crtc_state_is_legacy_gamma(crtc_state)) > > > + return 0; > > > + > > > + /* C8 needs the legacy LUT all to itself */ > > > + if (crtc_state->c8_planes) > > > + return -EINVAL; > > > > Do we need to test for non-NULL gamma_lut here? > > For c8? We already checked crtc_state_is_legacy_gamma() above. > If gamma_lut == NULL, crtc_state_is_legacy_gamma() will return false, so we'll wind up rejecting any setup involving c8 planes with -EINVAL, which I don't think is what we wanted. Matt > > > > The logic is the same as before your patch, so either we already had a > > pre-existing bug or, more likely, I'm overlooking something obvious. > > > > > + > > > + degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > > + gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > > + degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > > + gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > > + > > > + if (check_lut_size(degamma_lut, degamma_length) || > > > + check_lut_size(gamma_lut, gamma_length)) > > > + return -EINVAL; > > > + > > > + if (drm_color_lut_check(degamma_lut, degamma_tests) || > > > + drm_color_lut_check(gamma_lut, gamma_tests)) > > > + return -EINVAL; > > > + > > > + return 0; > > > +} > > > + > > > static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) > > > { > > > u32 cgm_mode = 0; > > > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > > const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > > const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > > bool limited_color_range = false; > > > - int gamma_length, degamma_length; > > > - u32 gamma_tests, degamma_tests; > > > int ret; > > > > > > - degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > > - gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > > - > > > - /* C8 needs the legacy LUT all to itself */ > > > - if (crtc_state->c8_planes && > > > - !crtc_state_is_legacy_gamma(crtc_state)) > > > - return -EINVAL; > > > + ret = check_luts(crtc_state); > > > + if (ret) > > > + return ret; > > > > > > crtc_state->gamma_enable = (gamma_lut || degamma_lut) && > > > !crtc_state->c8_planes; > > > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > > if (IS_CHERRYVIEW(dev_priv)) > > > crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > > > > > > - /* Always allow legacy gamma LUT with no further checking. */ > > > if (!crtc_state->gamma_enable || > > > - crtc_state_is_legacy_gamma(crtc_state)) { > > > + crtc_state_is_legacy_gamma(crtc_state)) > > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > > - if (INTEL_GEN(dev_priv) >= 11 && > > > - crtc_state->gamma_enable) > > > - crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > > - return 0; > > > - } > > > - > > > - if (check_lut_size(degamma_lut, degamma_length) || > > > - check_lut_size(gamma_lut, gamma_length)) > > > - return -EINVAL; > > > - > > > - if (drm_color_lut_check(degamma_lut, degamma_tests) || > > > - drm_color_lut_check(gamma_lut, gamma_tests)) > > > - return -EINVAL; > > > - > > > - if (INTEL_GEN(dev_priv) >= 11) > > > + else if (INTEL_GEN(dev_priv) >= 11) > > > crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT | > > > PRE_CSC_GAMMA_ENABLE | > > > POST_CSC_GAMMA_ENABLE; > > > > Should we drop the GAMMA_ENABLE here and just rely on the new check > > you're adding below to handle it? > > This will get rewritten entirely in following patches, so I figured I'd > just keep this patch minimal. > > > > > > > Matt > > > > > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > > > > > if (INTEL_GEN(dev_priv) >= 11) { > > > + if (crtc_state->gamma_enable) > > > + crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > > + > > > if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || > > > crtc_state->limited_color_range) > > > crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE; > > > -- > > > 2.19.2 > > > > > > > -- > > Matt Roper > > Graphics Software Engineer > > IoTG Platform Enabling & Development > > Intel Corporation > > (916) 356-2795 > > -- > Ville Syrjälä > Intel
On Wed, Mar 20, 2019 at 08:14:56AM -0700, Matt Roper wrote: > On Wed, Mar 20, 2019 at 04:41:31PM +0200, Ville Syrjälä wrote: > > On Tue, Mar 19, 2019 at 02:59:38PM -0700, Matt Roper wrote: > > > On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > In prepartion for per-platform color_check() functions extract the > > > > common code into a separate function. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------ > > > > 1 file changed, 40 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > > > > index 467fd1a1630c..b6a992a5b14f 100644 > > > > --- a/drivers/gpu/drm/i915/intel_color.c > > > > +++ b/drivers/gpu/drm/i915/intel_color.c > > > > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) > > > > return 0; > > > > } > > > > > > > > +static int check_luts(const struct intel_crtc_state *crtc_state) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > > > + const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > > > + const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > > > + int gamma_length, degamma_length; > > > > + u32 gamma_tests, degamma_tests; > > > > + > > > > + /* Always allow legacy gamma LUT with no further checking. */ > > > > + if (crtc_state_is_legacy_gamma(crtc_state)) > > > > + return 0; > > > > + > > > > + /* C8 needs the legacy LUT all to itself */ > > > > + if (crtc_state->c8_planes) > > > > + return -EINVAL; > > > > > > Do we need to test for non-NULL gamma_lut here? > > > > For c8? We already checked crtc_state_is_legacy_gamma() above. > > > > If gamma_lut == NULL, crtc_state_is_legacy_gamma() will return false, so > we'll wind up rejecting any setup involving c8 planes with -EINVAL, > which I don't think is what we wanted. I think it is. c8 scanout always uses the legacy LUT, so it had better be populated with the correct data or else we'll risk producing garbage on the display. > > > Matt > > > > > > > > The logic is the same as before your patch, so either we already had a > > > pre-existing bug or, more likely, I'm overlooking something obvious. > > > > > > > + > > > > + degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > > > + gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > > > + degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > > > + gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > > > + > > > > + if (check_lut_size(degamma_lut, degamma_length) || > > > > + check_lut_size(gamma_lut, gamma_length)) > > > > + return -EINVAL; > > > > + > > > > + if (drm_color_lut_check(degamma_lut, degamma_tests) || > > > > + drm_color_lut_check(gamma_lut, gamma_tests)) > > > > + return -EINVAL; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) > > > > { > > > > u32 cgm_mode = 0; > > > > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > > > const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > > > const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > > > bool limited_color_range = false; > > > > - int gamma_length, degamma_length; > > > > - u32 gamma_tests, degamma_tests; > > > > int ret; > > > > > > > > - degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > > > - gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > > > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > > > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > > > - > > > > - /* C8 needs the legacy LUT all to itself */ > > > > - if (crtc_state->c8_planes && > > > > - !crtc_state_is_legacy_gamma(crtc_state)) > > > > - return -EINVAL; > > > > + ret = check_luts(crtc_state); > > > > + if (ret) > > > > + return ret; > > > > > > > > crtc_state->gamma_enable = (gamma_lut || degamma_lut) && > > > > !crtc_state->c8_planes; > > > > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > > > if (IS_CHERRYVIEW(dev_priv)) > > > > crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > > > > > > > > - /* Always allow legacy gamma LUT with no further checking. */ > > > > if (!crtc_state->gamma_enable || > > > > - crtc_state_is_legacy_gamma(crtc_state)) { > > > > + crtc_state_is_legacy_gamma(crtc_state)) > > > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > > > - if (INTEL_GEN(dev_priv) >= 11 && > > > > - crtc_state->gamma_enable) > > > > - crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > > > - return 0; > > > > - } > > > > - > > > > - if (check_lut_size(degamma_lut, degamma_length) || > > > > - check_lut_size(gamma_lut, gamma_length)) > > > > - return -EINVAL; > > > > - > > > > - if (drm_color_lut_check(degamma_lut, degamma_tests) || > > > > - drm_color_lut_check(gamma_lut, gamma_tests)) > > > > - return -EINVAL; > > > > - > > > > - if (INTEL_GEN(dev_priv) >= 11) > > > > + else if (INTEL_GEN(dev_priv) >= 11) > > > > crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT | > > > > PRE_CSC_GAMMA_ENABLE | > > > > POST_CSC_GAMMA_ENABLE; > > > > > > Should we drop the GAMMA_ENABLE here and just rely on the new check > > > you're adding below to handle it? > > > > This will get rewritten entirely in following patches, so I figured I'd > > just keep this patch minimal. > > > > > > > > > > > Matt > > > > > > > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > > > > > > > if (INTEL_GEN(dev_priv) >= 11) { > > > > + if (crtc_state->gamma_enable) > > > > + crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > > > + > > > > if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || > > > > crtc_state->limited_color_range) > > > > crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE; > > > > -- > > > > 2.19.2 > > > > > > > > > > -- > > > Matt Roper > > > Graphics Software Engineer > > > IoTG Platform Enabling & Development > > > Intel Corporation > > > (916) 356-2795 > > > > -- > > Ville Syrjälä > > Intel > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
On Wed, Mar 20, 2019 at 05:31:07PM +0200, Ville Syrjälä wrote: > On Wed, Mar 20, 2019 at 08:14:56AM -0700, Matt Roper wrote: > > On Wed, Mar 20, 2019 at 04:41:31PM +0200, Ville Syrjälä wrote: > > > On Tue, Mar 19, 2019 at 02:59:38PM -0700, Matt Roper wrote: > > > > On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > In prepartion for per-platform color_check() functions extract the > > > > > common code into a separate function. > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------ > > > > > 1 file changed, 40 insertions(+), 28 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > > > > > index 467fd1a1630c..b6a992a5b14f 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_color.c > > > > > +++ b/drivers/gpu/drm/i915/intel_color.c > > > > > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) > > > > > return 0; > > > > > } > > > > > > > > > > +static int check_luts(const struct intel_crtc_state *crtc_state) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); > > > > > + const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > > > > + const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > > > > + int gamma_length, degamma_length; > > > > > + u32 gamma_tests, degamma_tests; > > > > > + > > > > > + /* Always allow legacy gamma LUT with no further checking. */ > > > > > + if (crtc_state_is_legacy_gamma(crtc_state)) > > > > > + return 0; > > > > > + > > > > > + /* C8 needs the legacy LUT all to itself */ > > > > > + if (crtc_state->c8_planes) > > > > > + return -EINVAL; > > > > > > > > Do we need to test for non-NULL gamma_lut here? > > > > > > For c8? We already checked crtc_state_is_legacy_gamma() above. > > > > > > > If gamma_lut == NULL, crtc_state_is_legacy_gamma() will return false, so > > we'll wind up rejecting any setup involving c8 planes with -EINVAL, > > which I don't think is what we wanted. > > I think it is. c8 scanout always uses the legacy LUT, so it had > better be populated with the correct data or else we'll risk > producing garbage on the display. Ah, makes sense now. I think I misinterpreted the "all to itself" part of the comment. Maybe making this condition an 'else if' and rewording the comment to something like "C8 relies on its palette being stored in the legacy LUT" would make it more obvious. Either way, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > > > Matt > > > > > > > > > > > > The logic is the same as before your patch, so either we already had a > > > > pre-existing bug or, more likely, I'm overlooking something obvious. > > > > > > > > > + > > > > > + degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > > > > + gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > > > > + degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > > > > + gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > > > > + > > > > > + if (check_lut_size(degamma_lut, degamma_length) || > > > > > + check_lut_size(gamma_lut, gamma_length)) > > > > > + return -EINVAL; > > > > > + > > > > > + if (drm_color_lut_check(degamma_lut, degamma_tests) || > > > > > + drm_color_lut_check(gamma_lut, gamma_tests)) > > > > > + return -EINVAL; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) > > > > > { > > > > > u32 cgm_mode = 0; > > > > > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > > > > const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; > > > > > const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; > > > > > bool limited_color_range = false; > > > > > - int gamma_length, degamma_length; > > > > > - u32 gamma_tests, degamma_tests; > > > > > int ret; > > > > > > > > > > - degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; > > > > > - gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; > > > > > - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; > > > > > - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; > > > > > - > > > > > - /* C8 needs the legacy LUT all to itself */ > > > > > - if (crtc_state->c8_planes && > > > > > - !crtc_state_is_legacy_gamma(crtc_state)) > > > > > - return -EINVAL; > > > > > + ret = check_luts(crtc_state); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > > > crtc_state->gamma_enable = (gamma_lut || degamma_lut) && > > > > > !crtc_state->c8_planes; > > > > > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > > > > if (IS_CHERRYVIEW(dev_priv)) > > > > > crtc_state->cgm_mode = chv_cgm_mode(crtc_state); > > > > > > > > > > - /* Always allow legacy gamma LUT with no further checking. */ > > > > > if (!crtc_state->gamma_enable || > > > > > - crtc_state_is_legacy_gamma(crtc_state)) { > > > > > + crtc_state_is_legacy_gamma(crtc_state)) > > > > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > > > > - if (INTEL_GEN(dev_priv) >= 11 && > > > > > - crtc_state->gamma_enable) > > > > > - crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > > > > - return 0; > > > > > - } > > > > > - > > > > > - if (check_lut_size(degamma_lut, degamma_length) || > > > > > - check_lut_size(gamma_lut, gamma_length)) > > > > > - return -EINVAL; > > > > > - > > > > > - if (drm_color_lut_check(degamma_lut, degamma_tests) || > > > > > - drm_color_lut_check(gamma_lut, gamma_tests)) > > > > > - return -EINVAL; > > > > > - > > > > > - if (INTEL_GEN(dev_priv) >= 11) > > > > > + else if (INTEL_GEN(dev_priv) >= 11) > > > > > crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT | > > > > > PRE_CSC_GAMMA_ENABLE | > > > > > POST_CSC_GAMMA_ENABLE; > > > > > > > > Should we drop the GAMMA_ENABLE here and just rely on the new check > > > > you're adding below to handle it? > > > > > > This will get rewritten entirely in following patches, so I figured I'd > > > just keep this patch minimal. > > > > > > > > > > > > > > > Matt > > > > > > > > > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state) > > > > > crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; > > > > > > > > > > if (INTEL_GEN(dev_priv) >= 11) { > > > > > + if (crtc_state->gamma_enable) > > > > > + crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; > > > > > + > > > > > if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || > > > > > crtc_state->limited_color_range) > > > > > crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE; > > > > > -- > > > > > 2.19.2 > > > > > > > > > > > > > -- > > > > Matt Roper > > > > Graphics Software Engineer > > > > IoTG Platform Enabling & Development > > > > Intel Corporation > > > > (916) 356-2795 > > > > > > -- > > > Ville Syrjälä > > > Intel > > > > -- > > Matt Roper > > Graphics Software Engineer > > IoTG Platform Enabling & Development > > Intel Corporation > > (916) 356-2795 > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index 467fd1a1630c..b6a992a5b14f 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) return 0; } +static int check_luts(const struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); + const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; + const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; + int gamma_length, degamma_length; + u32 gamma_tests, degamma_tests; + + /* Always allow legacy gamma LUT with no further checking. */ + if (crtc_state_is_legacy_gamma(crtc_state)) + return 0; + + /* C8 needs the legacy LUT all to itself */ + if (crtc_state->c8_planes) + return -EINVAL; + + degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; + gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; + degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; + gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; + + if (check_lut_size(degamma_lut, degamma_length) || + check_lut_size(gamma_lut, gamma_length)) + return -EINVAL; + + if (drm_color_lut_check(degamma_lut, degamma_tests) || + drm_color_lut_check(gamma_lut, gamma_tests)) + return -EINVAL; + + return 0; +} + static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state) { u32 cgm_mode = 0; @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state) const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut; const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut; bool limited_color_range = false; - int gamma_length, degamma_length; - u32 gamma_tests, degamma_tests; int ret; - degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; - gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; - degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests; - gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests; - - /* C8 needs the legacy LUT all to itself */ - if (crtc_state->c8_planes && - !crtc_state_is_legacy_gamma(crtc_state)) - return -EINVAL; + ret = check_luts(crtc_state); + if (ret) + return ret; crtc_state->gamma_enable = (gamma_lut || degamma_lut) && !crtc_state->c8_planes; @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state) if (IS_CHERRYVIEW(dev_priv)) crtc_state->cgm_mode = chv_cgm_mode(crtc_state); - /* Always allow legacy gamma LUT with no further checking. */ if (!crtc_state->gamma_enable || - crtc_state_is_legacy_gamma(crtc_state)) { + crtc_state_is_legacy_gamma(crtc_state)) crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; - if (INTEL_GEN(dev_priv) >= 11 && - crtc_state->gamma_enable) - crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; - return 0; - } - - if (check_lut_size(degamma_lut, degamma_length) || - check_lut_size(gamma_lut, gamma_length)) - return -EINVAL; - - if (drm_color_lut_check(degamma_lut, degamma_tests) || - drm_color_lut_check(gamma_lut, gamma_tests)) - return -EINVAL; - - if (INTEL_GEN(dev_priv) >= 11) + else if (INTEL_GEN(dev_priv) >= 11) crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT | PRE_CSC_GAMMA_ENABLE | POST_CSC_GAMMA_ENABLE; @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state) crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT; if (INTEL_GEN(dev_priv) >= 11) { + if (crtc_state->gamma_enable) + crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE; + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB || crtc_state->limited_color_range) crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;