Re: [Intel-gfx] [PATCH 09/23] drm/i915: Register pipe color capabilities

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 23, 2015 at 02:05:25PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 9/22/2015 6:54 PM, Daniel Vetter wrote:
> >On Wed, Sep 16, 2015 at 11:07:06PM +0530, Shashank Sharma wrote:
> >>DRM color manager contains these color properties:
> >>
> >>1. "crtc_palette_capabilities_property": to allow a
> >>core driver to load and showcase its color correction
> >>capabilities to user space.
> >>2. "ctm": Color transformation matrix property, where a
> >>color transformation matrix of 9 correction values gets
> >>applied as correction.
> >>3. "palette_before_ctm": for corrections which get applied
> >>beore color transformation matrix correction.
> >>4. "palette_after_ctm": for corrections which get applied
> >>after color transformation matrix correction.
> >>
> >>Intel color manager registers:
> >>1. Gamma correction property as "palette_after_ctm" property
> >>2. Degamma correction capability as "palette_bafore_ctm" property
> >>capability as "palette_after_ctm" DRM color property hook.
> >>3. CSC as "ctm" property.
> >>
> >>This patch does the following:
> >>1. Add a function which loads the platform's color correction
> >>capabilities in the cm_crtc_palette_capabilities_property structure.
> >>2. Attaches the cm_crtc_palette_capabilities_property to every CRTC
> >>getting initiaized.
> >>3. Adds two new parameters "num_samples_after_ctm" and
> >>"num_samples_before_ctm" in intel_device_info as gamma and
> >>degamma coefficients vary per platform basis.
> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h            |  2 ++
> >>  drivers/gpu/drm/i915/intel_color_manager.c | 45 ++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_color_manager.h |  2 ++
> >>  3 files changed, 49 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 3bf8a9b..22de2cb 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -798,6 +798,8 @@ struct intel_device_info {
> >>  	u8 num_sprites[I915_MAX_PIPES];
> >>  	u8 gen;
> >>  	u8 ring_mask; /* Rings supported by the HW */
> >>+	u16 num_samples_after_ctm;
> >>+	u16 num_samples_before_ctm;
> >>  	DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON);
> >>  	/* Register offsets for the various display pipes and transcoders */
> >>  	int pipe_offsets[I915_MAX_TRANSCODERS];
> >>diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> >>index 7357d99..77f58f2 100644
> >>--- a/drivers/gpu/drm/i915/intel_color_manager.c
> >>+++ b/drivers/gpu/drm/i915/intel_color_manager.c
> >>@@ -27,7 +27,52 @@
> >>
> >>  #include "intel_color_manager.h"
> >>
> >>+int get_pipe_capabilities(struct drm_device *dev,
> >>+		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
> >>+{
> >>+	struct drm_property_blob *blob;
> >>+
> >>+	/*
> >>+	 * This function loads best capability for gamma correction
> >>+	 * For example:
> >>+	 * CHV best Gamma correction (CGM unit, 10 bit)
> >>+	 * has 257 entries, best degamma is 65 entries
> >>+	 */
> >>+	palette_caps->version = COLOR_STRUCT_VERSION;
> >>+	palette_caps->num_samples_after_ctm =
> >>+				INTEL_INFO(dev)->num_samples_after_ctm;
> >>+	palette_caps->num_samples_before_ctm =
> >>+				INTEL_INFO(dev)->num_samples_before_ctm;
> >>+	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
> >>+			(const void *) palette_caps);
> >>+	if (IS_ERR_OR_NULL(blob)) {
> >>+		DRM_ERROR("Create blob for capabilities failed\n");
> >>+		return PTR_ERR(blob);
> >>+	}
> >>+
> >>+	return blob->base.id;
> >>+}
> >>+
> >>  void intel_attach_color_properties_to_crtc(struct drm_device *dev,
> >>  		struct drm_mode_object *mode_obj)
> >>  {
> >>+	struct drm_mode_config *config = &dev->mode_config;
> >>+	struct drm_palette_caps *palette_caps;
> >>+	struct drm_crtc *crtc;
> >>+	int capabilities_blob_id;
> >>+
> >>+	crtc = obj_to_crtc(mode_obj);
> >>+	palette_caps = kzalloc(sizeof(struct drm_palette_caps),
> >>+			GFP_KERNEL);
> >>+	capabilities_blob_id = get_pipe_capabilities(dev,
> >>+						palette_caps, crtc);
> >>+
> >>+	if (config->cm_crtc_palette_capabilities_property) {
> >>+		drm_object_attach_property(mode_obj,
> >>+			config->cm_crtc_palette_capabilities_property,
> >>+			capabilities_blob_id);
> >
> >I didn't find any code to attach the before/after_ctm gamma properties and
> >the ctm property. Also I think a small helper would be really nice here in
> >the drm core:
> >
> Patch 15,16,17 adds those for gamma, degamma and CSC. This is just for the
> capabilities property.

Indeed, my mail search failed me.

> >drm_crtc_attach_cc_properties(struct drm_crtc *crtc,
> >			      int gamma_before_ctm,
> >			      bool ctm,
> >			      int gamm_after_ctm)
> >
> >And then that directly constructs the palette caps from the given values
> >(if they're non-0) and also attaches the properties to the crtc (if
> >they're non-0 for gamma, treu for the ctm).
> >
> >If that attach_property code really isn't there I wonder a bit how this
> >all works, we /should/ be filtering out properties which aren't attached
> >to the object.
> As I mentioned in the cover-letter, here is the sequence:
> First 5 patches add color management support in DRM layer
> Next 7 patches add Intel color management framework in I915 driver (This is
> still the framework part)
> Next 5 patches add color correction for CHV (gamma, degamma and CSC)
> (Here comes the core property and enabling part)
> Next 2 patches enable color management, by attaching the properties to
>  CRTC(Matt's comment on enable property code only when the job is done)
> Next 4 patches add color correction for BDW (gamma, degamma)

Yeah I just missed this, no idea why the search in my mail client didn't
turn up patches 15-17. I still think drm_crtc_attach_cc_properties in the
drm core would be good, instead of splitting part of the color manager
core support around the properties between the driver and core like you do
here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel