Re: [PATCH 15/18] drm/i915: Initialize Gen8 pipe gamma correction

From: "Sharma, Shashank" <shashank.sharma@intel.com>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: annie.j.matheson@intel.com, robert.bradford@intel.com,
	kausalmalladi@gmail.com, avinash.reddy.palleti@intel.com,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	vijay.a.purushothaman@intel.com, jesse.barnes@intel.com,
	gary.k.smith@intel.com, jim.bish@intel.com,
	daniel.vetter@intel.com, kiran.s.kumar@intel.com,
	susanta.bhattacharjee@intel.com
Subject: Re: [PATCH 15/18] drm/i915: Initialize Gen8 pipe gamma correction
Date: Sat, 22 Aug 2015 12:01:32 +0530	[thread overview]
Message-ID: <55D81744.8010406@intel.com> (raw)
In-Reply-To: <20150821224128.GK13406@intel.com>

Regards
Shashank

On 8/22/2015 4:11 AM, Matt Roper wrote:
> On Thu, Aug 06, 2015 at 10:08:24PM +0530, Shashank Sharma wrote:
>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>
>> This patch initializes gamma color correction proeprty
>                                                  ^^^^^^^^
>                                                  typo
Oops !
>> for Gen8 and higher platforms.
>
> I'd specifically say 'BDW and Gen9' here since we already have some gen8
> support (CHV).
>
Agree. Will change it.
>>
>> It does the following :
>> 1. Load pipe Gamma color correction capabilities for BDW/SKL/BXT
>> 2. Attach the color properties to CRTC
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color_manager.c | 30 +++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/i915/intel_color_manager.h |  3 +++
>>   2 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 5fa575b..bc77ab5 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -475,11 +475,39 @@ int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
>>   	return 0;
>>   }
>>
>> +int get_gen9_pipe_gamma_capabilities(struct drm_device *dev,
>> +		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>
> Calling this 'gen9' seems a little confusing to me given that it's also
> used for BDW, which is a gen8 platform.  The general pattern is that
> functions get named after the first platform that works a specific way,
> so I'd expect this to be called "get_bdw_pipe_gamma_capabilities."
>
Yes, its a mistake. I will fix this and will be more specific.
>> +{
>> +	struct drm_property_blob *blob = NULL;
>> +
>> +	/*
>> +	 * This function exposes best capability for DeGamma and Gamma
>> +	 * For BXT, the DeGamma LUT has 512 entries
>> +	 * and the best Gamma capability has 512 entries
>> +	 */
>> +	palette_caps->version = GEN9_PALETTE_STRUCT_VERSION;
>> +	palette_caps->num_samples_before_ctm =
>> +		GEN9_SPLITGAMMA_MAX_VALS;
>> +	palette_caps->num_samples_after_ctm =
>> +		GEN9_SPLITGAMMA_MAX_VALS;
>> +
>> +	blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
>> +			(const void *) palette_caps);
>
> We're pretty much doing the same thing we did for CHV, but just filling
> in different values.  Could we just stick the number of samples in
> INTEL_INFO(dev)->num_gamma_samples_{before/after}_ctm instead and then
> have a single function that fills out your capability blob (or at least
> the part of it that we have today) across all platforms?  Or is this
> something that we think might actually start to vary across the
> different pipes of a single platform in the future?
>
Thanks, that's pretty good suggestion. Will do that.
>> +
>> +	if (blob)
>> +		return blob->base.id;
>> +
>> +	return 0;
>> +}
>> +
>>   int get_pipe_gamma_capabilities(struct drm_device *dev,
>>   		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>>   {
>>   	if (IS_CHERRYVIEW(dev))
>>   		return get_chv_pipe_gamma_capabilities(dev, palette_caps, crtc);
>> +	if (IS_BROADWELL(dev) || IS_GEN9(dev))
>> +		return get_gen9_pipe_gamma_capabilities(dev,
>> +				palette_caps, crtc);
>>   	return -EINVAL;
>>   }
>>
>> @@ -491,7 +519,7 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev,
>>   	struct drm_crtc *crtc;
>>   	int capabilities_blob_id;
>>
>> -	if (IS_CHERRYVIEW(dev)) {
>> +	if (IS_CHERRYVIEW(dev) || IS_BROADWELL(dev) || IS_GEN9(dev)) {
>
> 'IS_CHERRYVIEW(dev) || IS_BROADWELL(dev)' could be simplified to just
> 'IS_GEN8(dev)' right?
>
yep, will do it.
>
> Matt
>
>
>>   		crtc = obj_to_crtc(mode_obj);
>>
>>   		palette_caps = kzalloc(sizeof(struct drm_palette_caps),
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
>> index b2ee847..78de1a2 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -35,6 +35,9 @@
>>   #define CHV_DEGAMMA_MAX_VALS			65
>>   #define CHV_10BIT_GAMMA_MAX_VALS		257
>>
>> +#define GEN9_PALETTE_STRUCT_VERSION		1
>> +#define GEN9_SPLITGAMMA_MAX_VALS		512
>> +
>>   /* Gamma correction */
>>   #define CHV_GAMMA_DATA_STRUCT_VERSION		1
>>   #define CHV_10BIT_GAMMA_MAX_VALS		257
>> --
>> 1.9.1
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel