On 10 October 2015 at 06:20, Sharma, Shashank <shashank.sharma@xxxxxxxxx> wrote:
> On 10/10/2015 4:54 AM, Emil Velikov wrote:
>>
>> Hi Shashank,
>>
>> On 9 October 2015 at 20:29, Shashank Sharma <shashank.sharma@xxxxxxxxx>
>> 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@xxxxxxxxx>
>>> Signed-off-by: Kausal Malladi <kausalmalladi@xxxxxxxxx>
>>> ---
>>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
