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 at 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 at intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi at 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
>>
>
