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@xxxxxxxxxxxxxxx>
> > > > >
> > > > > In prepartion for per-platform color_check() functions extract the
> > > > > common code into a separate function.
> > > > >
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxx>
>
> >
> >
> > 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
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
