Re: [PATCH v5 1/4] drm: drm_helper_crtc_enable_color_mgmt() => drm_crtc_enable_color_mgmt()

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
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