Re: [PATCH v2 04/10] drm: Add Gamma correction structure

From: Matt Roper <matthew.d.roper@intel.com>
To: "Sharma, Shashank" <shashank.sharma@intel.com>
Cc: annie.j.matheson@intel.com, dhanya.p.r@intel.com,
	avinash.reddy.palleti@intel.com, dri-devel@lists.freedesktop.org,
	vijay.a.purushothaman@intel.com, indranil.mukherjee@intel.com,
	Kausal Malladi <Kausal.Malladi@intel.com>,
	jesse.barnes@intel.com, daniel.vetter@intel.com,
	sunil.kamath@intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 04/10] drm: Add Gamma correction structure
Date: Mon, 8 Jun 2015 17:48:23 -0700	[thread overview]
Message-ID: <20150609004823.GA7449@intel.com> (raw)
In-Reply-To: <5572DECD.3040500@intel.com>

On Sat, Jun 06, 2015 at 05:21:41PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 6/6/2015 6:30 AM, Matt Roper wrote:
> >On Thu, Jun 04, 2015 at 07:12:35PM +0530, Kausal Malladi wrote:
> >>From: Kausal Malladi <Kausal.Malladi@intel.com>
> >>
> >>This patch adds a new structure in DRM layer for Gamma color correction.
> >>This structure will be used by all user space agents to configure
> >>appropriate Gamma precision and Gamma level.
> >>
> >>struct drm_intel_gamma {
> >>        __u32 gamma_level;
> >>	(The gamma_level variable indicates if the Gamma correction is to be
> >>	applied on Pipe/plane)
> >
> >I'm not sure I understand the need for this one...you're getting the
> >set_property call against a specific DRM object, so I don't think there
> >should be any confusion at that point about what the values apply to?
> >
> Actually that is for the get_property path. If you have a look at
> the design document, there is a provision to query the current
> applied gamma which can be pipe/plane level. Hence keeping this.

But the get_property path should work the same way...get_property is
issued against a specific DRM object, so the DRM core will call the
appropriate handler to look up the value depending on whether the object
ID passed references a plane or a CRTC.  The crtc get_property handler
will look in crtc->state to get the value to return while the plane
get_property handler will look in plane->state to get the value.  I
don't see anything in the design document that indicates this would be a
problem.  Maybe I'm not understanding your concern?

> >
> >>        __u32 gamma_precision;
> >>	(The Gamma precision indicates the Gamma mode to be applied)
> >>
> >>	Supported precisions are -
> >>	#define I915_GAMMA_PRECISION_UNKNOWN	0
> >>	#define I915_GAMMA_PRECISION_CURRENT	0xFFFFFFFF
> >>	#define I915_GAMMA_PRECISION_LEGACY	(1 << 0)
> >>	#define I915_GAMMA_PRECISION_10BIT	(1 << 1)
> >>	#define I915_GAMMA_PRECISION_12BIT	(1 << 2)
> >>	#define I915_GAMMA_PRECISION_14BIT	(1 << 3)
> >>	#define I915_GAMMA_PRECISION_16BIT	(1 << 4)
> >
> >I feel like the precision would work better as a separate enum property
> >rather than being part of your blob; I think it would be cleaner if your
> >blob only held the actual values if possible.
> >
> Again, this would be required for the get_property path. If we
> create a separate property for this, the design might be very
> complex.
> 
> This is the current implementation:
> 1. Pack the correction values and configurations in blob()
> 2. call a create_blob() and get the blob_id
> 3. do a set_porperty() with the blob_id
> 4. set_property will find this blob, with the blob_id and apply
> corrections.

When you call set_property, I'd expect your new blob to replace the old
blob (see drm_atomic_set_mode_for_crtc() for an example of how this
works with mode blobs).  My understanding is that the 'precision' value
here is a distinct piece of information that would be valuable to set
and query separately from the actual gamma values, so there's really no
benefit to smashing them together into a single blob as far as I can
see.  As two separate properties, you'd just push both properties into
the property set submitted to the atomic ioctl by userspace.  Ultimately
everything you set winds up in the DRM object's state pointer and that
state pointer is what is used later to update the hardware programming.

Granted, if your userspace isn't using the atomic ioctl yet and is just
calling individual drmModeSetProperty() calls, then it's a bit uglier
since you need to make multiple ioctl calls.  But atomic is the way
forward, so we're not really trying to design around the legacy
interfaces anymore.

Let me know if I'm overlooking something in your design.

> Adding a separate property for gamma_level will add one more state
> here, which will make it further complex. Do you agree ?

There's only ever one state object for a DRM object (plane->state,
crtc->state, etc.).  That state object (and it's i915-specific subclass)
have multiple fields to store different kinds of information though.  Is
that what you're referring to?

Matt

> >>
> >>	__u32 num_samples;
> >>	(The num_samples indicates the number of Gamma correction
> >>	coefficients)
> >>	__u32 reserved;
> >>	__u16 values[0];
> >>	(An array of size 0, to accommodate the "num_samples" number of
> >>	R16G16B16 pixels, dynamically)
> >>};
> >>
> >>v2: Addressing Daniel Stone's comment, added a variable sized array to
> >>carry Gamma correction values as blob property.
> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> >>---
> >>  include/drm/drm_crtc.h |  3 +++
> >>  include/uapi/drm/drm.h | 10 ++++++++++
> >>  2 files changed, 13 insertions(+)
> >>
> >>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >>index 2a75d7d..bc44f27 100644
> >>--- a/include/drm/drm_crtc.h
> >>+++ b/include/drm/drm_crtc.h
> >>@@ -483,6 +483,9 @@ struct drm_crtc {
> >>  	 * acquire context.
> >>  	 */
> >>  	struct drm_modeset_acquire_ctx *acquire_ctx;
> >>+
> >>+	/* Color Management Blob IDs */
> >>+	u32 gamma_blob_id;
> >>  };
> >>
> >>  /**
> >>diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> >>index 3801584..fc2661c 100644
> >>--- a/include/uapi/drm/drm.h
> >>+++ b/include/uapi/drm/drm.h
> >>@@ -829,6 +829,16 @@ struct drm_event_vblank {
> >>  	__u32 reserved;
> >>  };
> >>
> >>+/* Color Management structure for Gamma */
> >>+struct drm_gamma {
> >>+	__u32 flags;
> >
> >The flags aren't described in your commit message...what are they used
> >for?  I guess it will become more clear as I read farther through your
> >patch series.
> >
> >
> >Matt
> We are currently using this to define gamma/degamma as such, but yes
> I agree that we have added this for future / undefined last minute
> usages, and can be removed.
> >
> >
> >>+	__u32 gamma_level;
> >>+	__u32 gamma_precision;
> >>+	__u32 num_samples;
> >>+	__u32 reserved;
> >>+	__u16 values[0];
> >>+};
> >>+
> >>  /* typedef area */
> >>  #ifndef __KERNEL__
> >>  typedef struct drm_clip_rect drm_clip_rect_t;
> >>--
> >>2.4.2
> >>
> >

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel