| Message ID | 0816cb2f539f2a8c7cd0d2e1252e6902f687e1ae.1464251087.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jyri, Daniel, On 26/05/16 11:35, Jyri Sarha wrote: > Add drm_crtc_enable_color_mgmt(), remove drm_helper_crtc_enable_color_mgmt() > and update drm/i915-driver (the only user of the old function). > > The new function is more flexible. It allows driver to enable only the > features it has without forcing to enable all three color management > properties: degamma lut, csc matrix (ctm), and gamma lut. > > Suggested-by: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Jyri Sarha <jsarha@ti.com> > +void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > + uint degamma_lut_size, > + bool has_ctm, > + uint gamma_lut_size) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_mode_config *config = &dev->mode_config; > + > + if (degamma_lut_size) { > + drm_object_attach_property(&crtc->base, > + config->degamma_lut_property, 0); > + drm_object_attach_property(&crtc->base, > + config->degamma_lut_size_property, > + degamma_lut_size); > + } > + > + if (has_ctm) > + drm_object_attach_property(&crtc->base, > + config->ctm_property, 0); > + > + if (gamma_lut_size) { > + drm_object_attach_property(&crtc->base, > + config->gamma_lut_property, 0); > + drm_object_attach_property(&crtc->base, > + config->gamma_lut_size_property, > + gamma_lut_size); > + } > +} As I mentioned, I think it would make sense to call drm_mode_crtc_set_gamma_size() here. At least from omapdrm perspective. I had a look at i915, and that one looks a bit odd. It always sets drm_mode_crtc_set_gamma_size(256), but then only calls drm_helper_crtc_enable_color_mgmt() if INTEL_INFO(dev)->color.gamma_lut_size > 0, and gives gamma_lut_size as the size. Is there some legacy stuff at play here? drm_mode_crtc_set_gamma_size() should always be 256 (as X expects that), but the GAMMA_LUT property can give the real gamma lut size? Tomi
On 05/26/16 12:05, Tomi Valkeinen wrote: > Hi Jyri, Daniel, > > On 26/05/16 11:35, Jyri Sarha wrote: >> Add drm_crtc_enable_color_mgmt(), remove drm_helper_crtc_enable_color_mgmt() >> and update drm/i915-driver (the only user of the old function). >> >> The new function is more flexible. It allows driver to enable only the >> features it has without forcing to enable all three color management >> properties: degamma lut, csc matrix (ctm), and gamma lut. >> >> Suggested-by: Daniel Vetter <daniel@ffwll.ch> >> Signed-off-by: Jyri Sarha <jsarha@ti.com> > >> +void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, >> + uint degamma_lut_size, >> + bool has_ctm, >> + uint gamma_lut_size) >> +{ >> + struct drm_device *dev = crtc->dev; >> + struct drm_mode_config *config = &dev->mode_config; >> + >> + if (degamma_lut_size) { >> + drm_object_attach_property(&crtc->base, >> + config->degamma_lut_property, 0); >> + drm_object_attach_property(&crtc->base, >> + config->degamma_lut_size_property, >> + degamma_lut_size); >> + } >> + >> + if (has_ctm) >> + drm_object_attach_property(&crtc->base, >> + config->ctm_property, 0); >> + >> + if (gamma_lut_size) { >> + drm_object_attach_property(&crtc->base, >> + config->gamma_lut_property, 0); >> + drm_object_attach_property(&crtc->base, >> + config->gamma_lut_size_property, >> + gamma_lut_size); >> + } >> +} > > As I mentioned, I think it would make sense to call > drm_mode_crtc_set_gamma_size() here. At least from omapdrm perspective. > > I had a look at i915, and that one looks a bit odd. It always sets > drm_mode_crtc_set_gamma_size(256), but then only calls > drm_helper_crtc_enable_color_mgmt() if > INTEL_INFO(dev)->color.gamma_lut_size > 0, and gives gamma_lut_size as > the size. > > Is there some legacy stuff at play here? drm_mode_crtc_set_gamma_size() > should always be 256 (as X expects that), but the GAMMA_LUT property can > give the real gamma lut size? > This indeed seems to be the case. If call drm_mode_crtc_set_gamma_size, with 256, but set the gamma_lut_size_property to 1024, the X still works but trough atomic API I can use the full length gamma table. I wonder if should do yet one more patch-round? BR, Jyri
On Thu, May 26, 2016 at 12:59:09PM +0300, Jyri Sarha wrote: > On 05/26/16 12:05, Tomi Valkeinen wrote: > > Hi Jyri, Daniel, > > > > On 26/05/16 11:35, Jyri Sarha wrote: > >> Add drm_crtc_enable_color_mgmt(), remove drm_helper_crtc_enable_color_mgmt() > >> and update drm/i915-driver (the only user of the old function). > >> > >> The new function is more flexible. It allows driver to enable only the > >> features it has without forcing to enable all three color management > >> properties: degamma lut, csc matrix (ctm), and gamma lut. > >> > >> Suggested-by: Daniel Vetter <daniel@ffwll.ch> > >> Signed-off-by: Jyri Sarha <jsarha@ti.com> > > > >> +void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > >> + uint degamma_lut_size, > >> + bool has_ctm, > >> + uint gamma_lut_size) > >> +{ > >> + struct drm_device *dev = crtc->dev; > >> + struct drm_mode_config *config = &dev->mode_config; > >> + > >> + if (degamma_lut_size) { > >> + drm_object_attach_property(&crtc->base, > >> + config->degamma_lut_property, 0); > >> + drm_object_attach_property(&crtc->base, > >> + config->degamma_lut_size_property, > >> + degamma_lut_size); > >> + } > >> + > >> + if (has_ctm) > >> + drm_object_attach_property(&crtc->base, > >> + config->ctm_property, 0); > >> + > >> + if (gamma_lut_size) { > >> + drm_object_attach_property(&crtc->base, > >> + config->gamma_lut_property, 0); > >> + drm_object_attach_property(&crtc->base, > >> + config->gamma_lut_size_property, > >> + gamma_lut_size); > >> + } > >> +} > > > > As I mentioned, I think it would make sense to call > > drm_mode_crtc_set_gamma_size() here. At least from omapdrm perspective. > > > > I had a look at i915, and that one looks a bit odd. It always sets > > drm_mode_crtc_set_gamma_size(256), but then only calls > > drm_helper_crtc_enable_color_mgmt() if > > INTEL_INFO(dev)->color.gamma_lut_size > 0, and gives gamma_lut_size as > > the size. > > > > Is there some legacy stuff at play here? drm_mode_crtc_set_gamma_size() > > should always be 256 (as X expects that), but the GAMMA_LUT property can > > give the real gamma lut size? > > > > This indeed seems to be the case. If call drm_mode_crtc_set_gamma_size, > with 256, but set the gamma_lut_size_property to 1024, the X still works > but trough atomic API I can use the full length gamma table. > > I wonder if should do yet one more patch-round? The interaction between legacy gamma table and new atomic is a bit ill-defined. But yeah I think the only valid value for legacy gamma table is 256 afaik ... -Daniel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d2a6d95..b097226 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -6065,3 +6065,48 @@ struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, return tg; } EXPORT_SYMBOL(drm_mode_create_tile_group); + +/** + * drm_crtc_enable_color_mgmt - enable color management properties + * @crtc: DRM CRTC + * @degamma_lut_size: the size of the degamma lut (before CSC) + * @has_ctm: whether to attach ctm_property for CSC matrix + * @gamma_lut_size: the size of the gamma lut (after CSC) + * + * This function lets the driver enable the color correction + * properties on a CRTC. This includes 3 degamma, csc and gamma + * properties that userspace can set and 2 size properties to inform + * the userspace of the lut sizes. Each of the properties are + * optional. The gamma and degamma properties are only attached if + * their size is not 0 and ctm_property is only attached if has_ctm is + * true. + */ +void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, + uint degamma_lut_size, + bool has_ctm, + uint gamma_lut_size) +{ + struct drm_device *dev = crtc->dev; + struct drm_mode_config *config = &dev->mode_config; + + if (degamma_lut_size) { + drm_object_attach_property(&crtc->base, + config->degamma_lut_property, 0); + drm_object_attach_property(&crtc->base, + config->degamma_lut_size_property, + degamma_lut_size); + } + + if (has_ctm) + drm_object_attach_property(&crtc->base, + config->ctm_property, 0); + + if (gamma_lut_size) { + drm_object_attach_property(&crtc->base, + config->gamma_lut_property, 0); + drm_object_attach_property(&crtc->base, + config->gamma_lut_size_property, + gamma_lut_size); + } +} +EXPORT_SYMBOL(drm_crtc_enable_color_mgmt); diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index a6e4243..bf10d70 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -1121,36 +1121,3 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, return drm_plane_helper_commit(plane, plane_state, old_fb); } EXPORT_SYMBOL(drm_helper_crtc_mode_set_base); - -/** - * drm_helper_crtc_enable_color_mgmt - enable color management properties - * @crtc: DRM CRTC - * @degamma_lut_size: the size of the degamma lut (before CSC) - * @gamma_lut_size: the size of the gamma lut (after CSC) - * - * This function lets the driver enable the color correction properties on a - * CRTC. This includes 3 degamma, csc and gamma properties that userspace can - * set and 2 size properties to inform the userspace of the lut sizes. - */ -void drm_helper_crtc_enable_color_mgmt(struct drm_crtc *crtc, - int degamma_lut_size, - int gamma_lut_size) -{ - struct drm_device *dev = crtc->dev; - struct drm_mode_config *config = &dev->mode_config; - - drm_object_attach_property(&crtc->base, - config->degamma_lut_property, 0); - drm_object_attach_property(&crtc->base, - config->ctm_property, 0); - drm_object_attach_property(&crtc->base, - config->gamma_lut_property, 0); - - drm_object_attach_property(&crtc->base, - config->degamma_lut_size_property, - degamma_lut_size); - drm_object_attach_property(&crtc->base, - config->gamma_lut_size_property, - gamma_lut_size); -} -EXPORT_SYMBOL(drm_helper_crtc_enable_color_mgmt); diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index 1b3f974..522f5a2 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -547,7 +547,8 @@ void intel_color_init(struct drm_crtc *crtc) /* Enable color management support when we have degamma & gamma LUTs. */ if (INTEL_INFO(dev)->color.degamma_lut_size != 0 && INTEL_INFO(dev)->color.gamma_lut_size != 0) - drm_helper_crtc_enable_color_mgmt(crtc, + drm_crtc_enable_color_mgmt(crtc, INTEL_INFO(dev)->color.degamma_lut_size, + true, INTEL_INFO(dev)->color.gamma_lut_size); } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d1559cd..36d3bbf 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -2553,7 +2553,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device unsigned int supported_rotations); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations); - +extern void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, + uint degamma_lut_size, + bool has_ctm, + uint gamma_lut_size); /* Helpers */ static inline struct drm_plane *drm_plane_find(struct drm_device *dev, diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 97fa894..4b37afa 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -48,9 +48,6 @@ extern bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode, int x, int y, struct drm_framebuffer *old_fb); -extern void drm_helper_crtc_enable_color_mgmt(struct drm_crtc *crtc, - int degamma_lut_size, - int gamma_lut_size); extern bool drm_helper_crtc_in_use(struct drm_crtc *crtc); extern bool drm_helper_encoder_in_use(struct drm_encoder *encoder);
Add drm_crtc_enable_color_mgmt(), remove drm_helper_crtc_enable_color_mgmt() and update drm/i915-driver (the only user of the old function). The new function is more flexible. It allows driver to enable only the features it has without forcing to enable all three color management properties: degamma lut, csc matrix (ctm), and gamma lut. Suggested-by: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Jyri Sarha <jsarha@ti.com> --- drivers/gpu/drm/drm_crtc.c | 45 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc_helper.c | 33 ---------------------------- drivers/gpu/drm/i915/intel_color.c | 3 ++- include/drm/drm_crtc.h | 5 ++++- include/drm/drm_crtc_helper.h | 3 --- 5 files changed, 51 insertions(+), 38 deletions(-)