[Intel-gfx] [PATCH 16/22] drm/i915: Commit color correction to CRTC

[Emil Velikov]

On 10 October 2015 at 06:20, Sharma, Shashank <shashank.sharma at intel.com> wrote:
> On 10/10/2015 4:54 AM, Emil Velikov wrote:
>>
>> Hi Shashank,
>>
>> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma at intel.com>
>> wrote:
>>>
>>> The color correction blob values are loaded during set_property
>>> calls. This patch adds a function to find the blob and apply the
>>> correction values to the display registers, during the atomic
>>> commit call.
>>>
>>> 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 | 44
>>> ++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_display.c       |  2 ++
>>>   drivers/gpu/drm/i915/intel_drv.h           |  3 ++
>>>   3 files changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
>>> b/drivers/gpu/drm/i915/intel_color_manager.c
>>> index 433e50a..d5315b2 100644
>>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>>> @@ -307,6 +307,50 @@ static int chv_set_gamma(struct drm_device *dev,
>>> struct drm_property_blob *blob,
>>>          return ret;
>>>   }
>>>
>>> +void intel_color_manager_crtc_commit(struct drm_device *dev,
>>> +               struct drm_crtc_state *crtc_state)
>>> +{
>>> +       struct drm_property_blob *blob;
>>> +       struct drm_crtc *crtc = crtc_state->crtc;
>>> +       int ret = -EINVAL;
>>
>> Most places/people advise against pre-emptively initializing ret.
>>
> Just in this case, if makes sense to keep one, coz there might be multiple
> blobs which we are applying in the commit action, and every blob can return
> error, from the core function.
> Assume that there is a situation where both palette_after_ctm(gamma) and CTM
> is in place, then we will be going through multiple if() cases and have to
> check the return values.
When you have an exception of any rule, this implies that you are
using a suboptimal way of doing things.

>>>
>>> +
>>> +       blob = crtc_state->palette_after_ctm_blob;
>>> +       if (blob) {
>>> +               /* Gamma correction is platform specific */
>>> +               if (IS_CHERRYVIEW(dev))
>>> +                       ret = chv_set_gamma(dev, blob, crtc);
>>> +
>>> +               if (ret)
>>> +                       DRM_ERROR("set Gamma correction failed\n");
>>
>> Do we really want to spam dmesg on for each non Cherryview device ?
>
> If you see the complete implementation, as IS_GEN8()/IS_SKL is coming up
> here after IS_CHV(patch 19,20,21) which will cover BDW/SKL/BXT. Anything
> else which comes here, deserves a DRM_ERROR() as this platform will not be
> supported.
>
Your patches should be independent changes. You cannot say "I'm adding
something iffy here, but it will be fixed with another patch".
Otherwise you'll get developer/user X bisecting the kernel, which will
end up with broken system (flooded dmesg in this case) half way
through the bisect.

Regards,
Emil