> Subject: [v5 05/24] drm: Define helper to initialize segmented 1D LUT
> 
> This adds helper functions to create 1D multi-segmented Lut color block

Use imperative style of commit messages

> capabilities. It exposes the hardware block as segments which are converted
> to blob and passed in the property.
> This also adds helper to initialize 1D segmented LUT.
> 
> v2: Squashed the 1d lut helpers (Dmitry)
> v3: Change name space to drm_plane_*
> 
> Signed-off-by: Chaitanya Kumar Borah <[email protected]>
> Signed-off-by: Uma Shankar <[email protected]>
> ---
>  drivers/gpu/drm/drm_colorop.c | 68
> +++++++++++++++++++++++++++++++++++
>  include/drm/drm_colorop.h     |  4 +++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index 97e9acbb0f2c..6e17f51c24be 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -90,6 +90,48 @@ static const struct drm_prop_enum_list
> drm_colorop_lut3d_interpolation_list[] =  };
> 
>  /* Init Helpers */
> +static int drm_create_colorop_capability_prop(struct drm_device *dev,
> +                                           struct drm_colorop *colorop,
> +                                           struct drm_property_blob *blob) {

I don't really see the benefit of having this as a separate function as of now.

> +     struct drm_property *prop = NULL;
> +
> +     prop = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> +                                DRM_MODE_PROP_ATOMIC |
> +                                DRM_MODE_PROP_IMMUTABLE,
> +                                "HW_CAPS", 1);
> +     if (!prop)
> +             return -ENOMEM;
> +
> +     colorop->hw_caps_property = prop;
> +     drm_object_attach_property(&colorop->base,
> +                                colorop->hw_caps_property,
> +                                blob->base.id);
> +
> +     return 0;
> +}
> +
> +static int drm_plane_colorop_lutcaps_init(struct drm_colorop *colorop,
> +                                       struct drm_plane *plane,
> +                                       const struct drm_color_lut_range
> *ranges,
> +                                       size_t length)

So you call it lutcaps here in the name and hw_caps is the property which I 
think is confusing.
First we need to make the naming for the field and function name consistent.
Personally I would like the property to be renamed to LUT_CAP since that is 
what it is to represent
According to the documentation provided in the previous patches.

Regards,
Suraj Kandpal

> +{
> +     struct drm_device *dev = plane->dev;
> +     struct drm_property_blob *blob;
> +
> +     /* Create Color Caps property for multi-segmented 1D LUT */
> +     if (colorop->type != DRM_COLOROP_1D_LUT_MULTSEG)
> +             return -EINVAL;
> +
> +     if (WARN_ON(length == 0 || length % sizeof(ranges[0]) != 0))
> +             return -EINVAL;
> +
> +     blob = drm_property_create_blob(plane->dev, length, ranges);
> +     if (IS_ERR(blob))
> +             return PTR_ERR(blob);
> +
> +     return drm_create_colorop_capability_prop(dev, colorop, blob); }
> 
>  static int drm_plane_colorop_init(struct drm_device *dev, struct drm_colorop
> *colorop,
>                           struct drm_plane *plane, enum drm_colorop_type
> type, uint32_t flags) @@ -337,6 +379,32 @@ int
> drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct
> drm_color  }  EXPORT_SYMBOL(drm_plane_colorop_curve_1d_lut_init);
> 
> +int drm_plane_colorop_curve_1d_lut_multseg_init(struct drm_device *dev,
> struct drm_colorop *colorop,
> +                                             struct drm_plane *plane,
> +                                             const struct
> drm_color_lut_range *ranges,
> +                                             size_t length, uint32_t flags)
> +{
> +     int ret;
> +
> +     ret = drm_plane_colorop_init(dev, colorop, plane,
> DRM_COLOROP_1D_LUT_MULTSEG, flags);
> +     if (ret)
> +             return ret;
> +
> +     ret = drm_plane_colorop_lutcaps_init(colorop, plane, ranges, length);
> +     if (ret)
> +             return ret;
> +
> +     /* data */
> +     ret = drm_colorop_create_data_prop(dev, colorop);
> +     if (ret)
> +             return ret;
> +
> +     drm_colorop_reset(colorop);
> +
> +     return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_colorop_curve_1d_lut_multseg_init);
> +
>  int drm_plane_colorop_ctm_3x4_init(struct drm_device *dev, struct
> drm_colorop *colorop,
>                                  struct drm_plane *plane, uint32_t flags)  {
> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h index
> 46099e81bbfa..44b4dfd69db7 100644
> --- a/include/drm/drm_colorop.h
> +++ b/include/drm/drm_colorop.h
> @@ -393,6 +393,10 @@ int drm_plane_colorop_curve_1d_lut_init(struct
> drm_device *dev, struct drm_color
>                                       struct drm_plane *plane, uint32_t
> lut_size,
>                                       enum
> drm_colorop_lut1d_interpolation_type lut1d_interpolation,
>                                       uint32_t flags);
> +int drm_plane_colorop_curve_1d_lut_multseg_init(struct drm_device *dev,
> struct drm_colorop *colorop,
> +                                             struct drm_plane *plane,
> +                                             const struct
> drm_color_lut_range *ranges,
> +                                             size_t length, uint32_t flags);
>  int drm_plane_colorop_ctm_3x4_init(struct drm_device *dev, struct
> drm_colorop *colorop,
>                                  struct drm_plane *plane, uint32_t flags);
> int drm_plane_colorop_mult_init(struct drm_device *dev, struct drm_colorop
> *colorop,
> --
> 2.42.0

Reply via email to