diff mbox

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

Message ID 1438879107-22819-16-git-send-email-shashank.sharma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sharma, Shashank Aug. 6, 2015, 4:38 p.m. UTC 
From: Kausal Malladi <kausalmalladi@gmail.com>

This patch initializes gamma color correction proeprty
for Gen8 and higher platforms.

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

Comments

Matt Roper Aug. 21, 2015, 10:41 p.m. UTC | #1 
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
> for Gen8 and higher platforms.

I'd specifically say 'BDW and Gen9' here since we already have some gen8
support (CHV).

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

> +{
> +	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?

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


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
>
Sharma, Shashank Aug. 22, 2015, 6:31 a.m. UTC | #2 
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
>>
>
diff mbox

Patch

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)
+{
+	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);
+
+	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)) {
 		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