Date:   Fri, 29 Oct 2021 09:43:35 -0400
From:   Sean Paul <sean@...rly.run>
To:     Mark Yacoub <markyacoub@...omium.org>
Cc:     Sean Paul <sean@...rly.run>, seanpaul@...omium.org,
        pmenzel@...gen.mpg.de, Mark Yacoub <markyacoub@...gle.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        intel-gfx@...ts.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v3 1/3] drm: Rename lut check functions to
 lut channel checks

On Thu, Oct 28, 2021 at 11:03:54PM -0400, Mark Yacoub wrote:
> On Thu, Oct 28, 2021 at 8:42 PM Sean Paul <sean@...rly.run> wrote:
> >
> > On Tue, Oct 26, 2021 at 03:21:00PM -0400, Mark Yacoub wrote:
> > > From: Mark Yacoub <markyacoub@...gle.com>
> > >
> > > [Why]
> > > This function and enum do not do generic checking on the luts but they
> > > test color channels in the LUTs.
> >
> > I'm not sure there's anything inherently specific to channels, it seems like
> > one could add a new test to reflect a HW limitation and it would fit pretty well
> > in the lut check function. I wonder if it would be better to expose the types of
> > tests required by the crtc such that the atomic_check could also do the test?
> >
> So the tests of the color are pretty unique to intel devices, no other
> device is using it so I didn't think it adds a lot of benefit adding
> it to the lut check. However, it's still in DRM because technically it
> can be supported by any driver. But once it is, the driver will have
> to expose the tests it wants so we can check it in atomic_check. but
> given that no one does expose any test but intel, i just left it only
> used by them.
> 

Yeah, understood. Regardless of that, the spirit of the function is not specific
to the color channels in the LUT, so renaming as channels_check is probably not
the correct fix. I'd probably lean towards stuffing this in i915 as a
driver-specific check instead of renaming it.

Sean

> > Sean
> >
> > > Keeping the name explicit as more generic LUT checks will follow.
> > >
> > > Tested on Eldrid ChromeOS (TGL).
> > >
> > > Signed-off-by: Mark Yacoub <markyacoub@...omium.org>
> > > ---
> > >  drivers/gpu/drm/drm_color_mgmt.c           | 12 ++++++------
> > >  drivers/gpu/drm/i915/display/intel_color.c | 10 +++++-----
> > >  include/drm/drm_color_mgmt.h               |  7 ++++---
> > >  3 files changed, 15 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > index bb14f488c8f6c..6f4e04746d90f 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -585,17 +585,17 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> > >  EXPORT_SYMBOL(drm_plane_create_color_properties);
> > >
> > >  /**
> > > - * drm_color_lut_check - check validity of lookup table
> > > + * drm_color_lut_channels_check - check validity of the channels in the lookup table
> > >   * @lut: property blob containing LUT to check
> > >   * @tests: bitmask of tests to run
> > >   *
> > > - * Helper to check whether a userspace-provided lookup table is valid and
> > > - * satisfies hardware requirements.  Drivers pass a bitmask indicating which of
> > > - * the tests in &drm_color_lut_tests should be performed.
> > > + * Helper to check whether each color channel of userspace-provided lookup table is valid and
> > > + * satisfies hardware requirements. Drivers pass a bitmask indicating which of in
> > > + * &drm_color_lut_channels_tests should be performed.
> > >   *
> > >   * Returns 0 on success, -EINVAL on failure.
> > >   */
> > > -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> > > +int drm_color_lut_channels_check(const struct drm_property_blob *lut, u32 tests)
> > >  {
> > >       const struct drm_color_lut *entry;
> > >       int i;
> > > @@ -625,4 +625,4 @@ int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
> > >
> > >       return 0;
> > >  }
> > > -EXPORT_SYMBOL(drm_color_lut_check);
> > > +EXPORT_SYMBOL(drm_color_lut_channels_check);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> > > index dab892d2251ba..4bb1bc76c4de9 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > > @@ -1285,7 +1285,7 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> > >       const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut;
> > >       const struct drm_property_blob *degamma_lut = crtc_state->hw.degamma_lut;
> > >       int gamma_length, degamma_length;
> > > -     u32 gamma_tests, degamma_tests;
> > > +     u32 gamma_channels_tests, degamma_channels_tests;
> > >
> > >       /* Always allow legacy gamma LUT with no further checking. */
> > >       if (crtc_state_is_legacy_gamma(crtc_state))
> > > @@ -1300,15 +1300,15 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
> > >
> > >       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;
> > > +     degamma_channels_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > > +     gamma_channels_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))
> > > +     if (drm_color_lut_channels_check(degamma_lut, degamma_channels_tests) ||
> > > +         drm_color_lut_channels_check(gamma_lut, gamma_channels_tests))
> > >               return -EINVAL;
> > >
> > >       return 0;
> > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > index 81c298488b0c8..cb1bf361ad3e3 100644
> > > --- a/include/drm/drm_color_mgmt.h
> > > +++ b/include/drm/drm_color_mgmt.h
> > > @@ -94,12 +94,12 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
> > >                                     enum drm_color_range default_range);
> > >
> > >  /**
> > > - * enum drm_color_lut_tests - hw-specific LUT tests to perform
> > > + * enum drm_color_lut_channels_tests - hw-specific LUT tests to perform
> > >   *
> > >   * The drm_color_lut_check() function takes a bitmask of the values here to
> > >   * determine which tests to apply to a userspace-provided LUT.
> > >   */
> > > -enum drm_color_lut_tests {
> > > +enum drm_color_lut_channels_tests {
> > >       /**
> > >        * @DRM_COLOR_LUT_EQUAL_CHANNELS:
> > >        *
> > > @@ -119,5 +119,6 @@ enum drm_color_lut_tests {
> > >       DRM_COLOR_LUT_NON_DECREASING = BIT(1),
> > >  };
> > >
> > > -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests);
> > > +int drm_color_lut_channels_check(const struct drm_property_blob *lut,
> > > +                              u32 tests);
> > >  #endif
> > > --
> > > 2.33.0.1079.g6e70778dc9-goog
> > >
> >
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Powered by blists - more mailing lists