On Fri, Apr 12, 2019 at 03:50:57PM +0530, Uma Shankar wrote: > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > Add a gamma mode property to enable various kind of > gamma modes supported by platforms like: Interpolated, Split, > Multi Segmented etc. Userspace can get this property and > should be able to get the platform capabilties wrt various > gamma modes possible and the possible ranges. > > It can select one of the modes exposed as blob_id as an > enum and set the respective mode. > > It can then create the LUT and send it to driver using > already available GAMMA_LUT property as blob. > > v2: Addressed Sam Ravnborg's review comments. Implemented > gamma mode with just one property and renamed the current > one to GAMMA_MODE property as recommended by Ville. > > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com> > Signed-off-by: Uma Shankar <uma.shan...@intel.com>
Please also extend the CTM property docs, see https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#color-management-properties And especially how GAMMA_MODE interacts with everything else we have already. I think the current comments don't really explain well how this is supposed to be used. Also, since this is quite a complicated data structure, can't we do at least some basic validation in the core code? -Daniel > --- > drivers/gpu/drm/drm_atomic_uapi.c | 5 +++ > drivers/gpu/drm/drm_color_mgmt.c | 77 > +++++++++++++++++++++++++++++++++++++++ > include/drm/drm_color_mgmt.h | 8 ++++ > include/drm/drm_crtc.h | 7 ++++ > include/drm/drm_mode_config.h | 6 +++ > include/uapi/drm/drm_mode.h | 38 +++++++++++++++++++ > 6 files changed, 141 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > b/drivers/gpu/drm/drm_atomic_uapi.c > index ea797d4..d85e0c9 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -459,6 +459,9 @@ static int drm_atomic_crtc_set_property(struct drm_crtc > *crtc, > &replaced); > state->color_mgmt_changed |= replaced; > return ret; > + } else if (property == config->gamma_mode_property) { > + state->gamma_mode = val; > + state->color_mgmt_changed |= replaced; > } else if (property == config->prop_out_fence_ptr) { > s32 __user *fence_ptr = u64_to_user_ptr(val); > > @@ -495,6 +498,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc > *crtc, > *val = (state->mode_blob) ? state->mode_blob->base.id : 0; > else if (property == config->prop_vrr_enabled) > *val = state->vrr_enabled; > + else if (property == config->gamma_mode_property) > + *val = state->gamma_mode; > else if (property == config->degamma_lut_property) > *val = (state->degamma_lut) ? state->degamma_lut->base.id : 0; > else if (property == config->ctm_property) > diff --git a/drivers/gpu/drm/drm_color_mgmt.c > b/drivers/gpu/drm/drm_color_mgmt.c > index d5d34d0..4d6792d 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -176,6 +176,83 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > } > EXPORT_SYMBOL(drm_crtc_enable_color_mgmt); > > +void drm_crtc_attach_gamma_mode_property(struct drm_crtc *crtc) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_mode_config *config = &dev->mode_config; > + > + if (!config->gamma_mode_property) > + return; > + > + drm_object_attach_property(&crtc->base, > + config->gamma_mode_property, 0); > +} > +EXPORT_SYMBOL(drm_crtc_attach_gamma_mode_property); > + > +int drm_color_create_gamma_mode_property(struct drm_device *dev, > + int num_values) > +{ > + struct drm_mode_config *config = &dev->mode_config; > + struct drm_property *prop; > + > + prop = drm_property_create(dev, > + DRM_MODE_PROP_ENUM, > + "GAMMA_MODE", num_values); > + if (!prop) > + return -ENOMEM; > + > + config->gamma_mode_property = prop; > + > + return 0; > +} > +EXPORT_SYMBOL(drm_color_create_gamma_mode_property); > + > +int drm_color_add_gamma_mode_range(struct drm_device *dev, > + const char *name, > + const struct drm_color_lut_range *ranges, > + size_t length) > +{ > + struct drm_mode_config *config = &dev->mode_config; > + struct drm_property_blob *blob; > + struct drm_property *prop; > + int num_ranges = length / sizeof(ranges[0]); > + int i, ret, num_types_0; > + > + if (WARN_ON(length == 0 || length % sizeof(ranges[0]) != 0)) > + return -EINVAL; > + > + num_types_0 = hweight8(ranges[0].flags & (DRM_MODE_LUT_GAMMA | > + DRM_MODE_LUT_DEGAMMA)); > + if (num_types_0 == 0) > + return -EINVAL; > + > + for (i = 1; i < num_ranges; i++) { > + int num_types = hweight8(ranges[i].flags & (DRM_MODE_LUT_GAMMA | > + > DRM_MODE_LUT_DEGAMMA)); > + > + /* either all ranges have DEGAMMA|GAMMA or none have it */ > + if (num_types_0 != num_types) > + return -EINVAL; > + } > + > + prop = config->gamma_mode_property; > + if (!prop) > + return -EINVAL; > + > + blob = drm_property_create_blob(dev, length, ranges); > + if (IS_ERR(blob)) > + return PTR_ERR(blob); > + > + ret = drm_property_add_enum(prop, blob->base.id, name); > + if (ret) { > + drm_property_blob_put(blob); > + return ret; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_color_add_gamma_mode_range); > + > /** > * drm_mode_crtc_set_gamma_size - set the gamma table size > * @crtc: CRTC to set the gamma table size for > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h > index d1c662d..f18e9b8 100644 > --- a/include/drm/drm_color_mgmt.h > +++ b/include/drm/drm_color_mgmt.h > @@ -51,6 +51,14 @@ static inline int drm_color_lut_size(const struct > drm_property_blob *blob) > return blob->length / sizeof(struct drm_color_lut); > } > > +int drm_color_create_gamma_mode_property(struct drm_device *dev, > + int num_values); > +void drm_crtc_attach_gamma_mode_property(struct drm_crtc *crtc); > +int drm_color_add_gamma_mode_range(struct drm_device *dev, > + const char *name, > + const struct drm_color_lut_range *ranges, > + size_t length); > + > enum drm_color_encoding { > DRM_COLOR_YCBCR_BT601, > DRM_COLOR_YCBCR_BT709, > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 58ad983..f2e60bd 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -249,6 +249,13 @@ struct drm_crtc_state { > struct drm_property_blob *mode_blob; > > /** > + * @gamma_mode: This is a blob_id and exposes the platform capabilties > + * wrt to various gamma modes and the respective lut ranges. This also > + * helps user select a gamma mode amongst the supported ones. > + */ > + u32 gamma_mode; > + > + /** > * @degamma_lut: > * > * Lookup table for converting framebuffer pixel data before apply the > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 7f60e8e..8f961c5b 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -761,6 +761,12 @@ struct drm_mode_config { > */ > struct drm_property *content_type_property; > /** > + * @gamma_mode_property: Optional CRTC property to enumerate and > + * select the mode of the crtc gamma/degmama LUTs. This also exposes > + * the lut ranges of the various supported gamma modes to userspace. > + */ > + struct drm_property *gamma_mode_property; > + /** > * @degamma_lut_property: Optional CRTC property to set the LUT used to > * convert the framebuffer's colors to linear gamma. > */ > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 83cd163..e70b7f8 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -630,6 +630,44 @@ struct drm_color_lut { > __u16 reserved; > }; > > +/* > + * DRM_MODE_LUT_GAMMA|DRM_MODE_LUT_DEGAMMA is legal and means the LUT > + * can be used for either purpose, but not simultaneously. To expose > + * modes that support gamma and degamma simultaneously the gamma mode > + * must declare distinct DRM_MODE_LUT_GAMMA and DRM_MODE_LUT_DEGAMMA > + * ranges. > + */ > +/* LUT is for gamma (after CTM) */ > +#define DRM_MODE_LUT_GAMMA BIT(0) > +/* LUT is for degamma (before CTM) */ > +#define DRM_MODE_LUT_DEGAMMA BIT(1) > +/* linearly interpolate between the points */ > +#define DRM_MODE_LUT_INTERPOLATE BIT(2) > +/* > + * the last value of the previous range is the > + * first value of the current range. > + */ > +#define DRM_MODE_LUT_REUSE_LAST BIT(3) > +/* the curve must be non-decreasing */ > +#define DRM_MODE_LUT_NON_DECREASING BIT(4) > +/* the curve is reflected across origin for negative inputs */ > +#define DRM_MODE_LUT_REFLECT_NEGATIVE BIT(5) > +/* the same curve (red) is used for blue and green channels as well */ > +#define DRM_MODE_LUT_SINGLE_CHANNEL BIT(6) > + > +struct drm_color_lut_range { > + /* DRM_MODE_LUT_* */ > + __u32 flags; > + /* number of points on the curve */ > + __u16 count; > + /* input/output bits per component */ > + __u8 input_bpc, output_bpc; > + /* input start/end values */ > + __s32 start, end; > + /* output min/max values */ > + __s32 min, max; > +}; > + > #define DRM_MODE_PAGE_FLIP_EVENT 0x01 > #define DRM_MODE_PAGE_FLIP_ASYNC 0x02 > #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4 > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
