Re: [v5][PATCH 01/11] drm/i915: Introduce vfunc read_luts() to create hw lut

From: Jani Nikula <jani.nikula@intel.com>
To: "Sharma, Swati2" <swati2.sharma@intel.com>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
	ankit.k.nautiyal@intel.com
Subject: Re: [v5][PATCH 01/11] drm/i915: Introduce vfunc read_luts() to create hw lut
Date: Tue, 07 May 2019 09:48:02 +0300	[thread overview]
Message-ID: <87d0kun4kd.fsf@intel.com> (raw)
In-Reply-To: <1eda6a72-a84b-e17e-7653-84be392d522c@intel.com>

On Mon, 06 May 2019, "Sharma, Swati2" <swati2.sharma@intel.com> wrote:
> On 06-May-19 7:11 PM, Jani Nikula wrote:
>> On Mon, 06 May 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>> On Mon, May 06, 2019 at 04:21:07PM +0300, Jani Nikula wrote:
>>>> On Sat, 04 May 2019, Swati Sharma <swati2.sharma@intel.com> wrote:
>>>>> In this patch, a vfunc read_luts() is introduced to create a hw lut
>>>>> i.e. lut having values read from gamma/degamma registers which will
>>>>> later be used to compare with sw lut to validate gamma/degamma lut values.
>>>>>
>>>>> v3: -Rebase
>>>>> v4: -Renamed intel_get_color_config to intel_color_get_config [Jani]
>>>>>      -Wrapped get_color_config() [Jani]
>>>>> v5: -Renamed intel_color_get_config() to intel_color_read_luts()
>>>> Ville, did you really want this chage? I sure didn't.
>>> I want the low level funcs to be called something like
>>> ilk_read_lut_10(), chv_read_cgm_gamma_lut(), etc.
>>> The name of the vfunc is more of a meh.
>> And I don't care about the low level functions so much, I care about the
>> interface which I should emphasize the "get config" stage. :)
>
> Sure, will do that.But my major concerns are in patch 11 :/ Please review and guide.
> Also, on CI tests are getting aborted..don't know why. Locally it's OK. Debugging.

Please move patch 11 right after current patch 2. This was the point in
making the vfuncs optional; you can then add platforms one by one and
each change is meaningful. Now you add everything but only really enable
the checks in one big lump at the end.

BR,
Jani.

>
>>
>> BR,
>> Jani.
>>
>>>> BR,
>>>> Jani.
>>>>
>>>>
>>>>>      -Renamed get_color_config to read_luts
>>>>>
>>>>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_drv.h    | 1 +
>>>>>   drivers/gpu/drm/i915/intel_color.c | 8 ++++++++
>>>>>   drivers/gpu/drm/i915/intel_color.h | 1 +
>>>>>   3 files changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index 1cea98f..1b6d891 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -342,6 +342,7 @@ struct drm_i915_display_funcs {
>>>>>   	 * involved with the same commit.
>>>>>   	 */
>>>>>   	void (*load_luts)(const struct intel_crtc_state *crtc_state);
>>>>> +	void (*read_luts)(struct intel_crtc_state *crtc_state);
>>>>>   };
>>>>>   
>>>>>   #define CSR_VERSION(major, minor)	((major) << 16 | (minor))
>>>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>>>>> index 962db12..0048d8a 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_color.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_color.c
>>>>> @@ -879,6 +879,14 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>>>>>   	return dev_priv->display.color_check(crtc_state);
>>>>>   }
>>>>>   
>>>>> +void intel_color_read_luts(struct intel_crtc_state *crtc_state)
>>>>> +{
>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>>>>> +
>>>>> +	if (dev_priv->display.read_luts)
>>>>> +		dev_priv->display.read_luts(crtc_state);
>>>>> +}
>>>>> +
>>>>>   static bool need_plane_update(struct intel_plane *plane,
>>>>>   			      const struct intel_crtc_state *crtc_state)
>>>>>   {
>>>>> diff --git a/drivers/gpu/drm/i915/intel_color.h b/drivers/gpu/drm/i915/intel_color.h
>>>>> index b8a3ce6..fc53de9 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_color.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_color.h
>>>>> @@ -13,5 +13,6 @@
>>>>>   int intel_color_check(struct intel_crtc_state *crtc_state);
>>>>>   void intel_color_commit(const struct intel_crtc_state *crtc_state);
>>>>>   void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
>>>>> +void intel_color_read_luts(struct intel_crtc_state *crtc_state);
>>>>>   
>>>>>   #endif /* __INTEL_COLOR_H__ */
>>>> -- 
>>>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx