Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

From: Damien Lespiau <damien.lespiau@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, intel-gfx@lists.freedesktop.org,
	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
Subject: Re: [PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW
Date: Tue, 9 Jun 2015 12:23:09 +0100	[thread overview]
Message-ID: <20150609112309.GE1671@strange.amr.corp.intel.com> (raw)
In-Reply-To: <5572E2F3.70005@intel.com>

On Sat, Jun 06, 2015 at 05:39:23PM +0530, Sharma, Shashank wrote:
> >>+    if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> >Switch/case instead?
> Yep, got it.
> >
> >Also, is UNKNOWN for disabling? Why not rename it to DISABLE then?
> Actually unknown is valid in case of get_property() when we want to query
> about the capabilities, just want to reuse the same, to avoid need for
> another one. Else we have to handle one extra case in each get_prop
> (disable) and set_prop(unknown)

Is it? the code isn't telling that story, at least in the current form.

get_property() should primarily be for retrieving the current value of
that property, so I'd suggest having a precision field of 0 means
what is CURRENT today (that'd be the default expected behaviour of
get_property()).

Then, 2 other "needs":

 - Query interface: how useful is the query interface in this present
   form? a query for the precision only isn't that useful as we need
   per-platform knowledge beyond that anyway (say we can't encode things
   like split gamma configurations sharing the same LUT storage)

 - Some other value to disable the function (set_property() with
   precision = 0 looks fine to me.

The get_property() path is missing from this patch set AFAICT.

-- 
Damien
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel