[Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

[Matheson, Annie J]

Thank you Shashank! 

Annie Matheson

> On Sep 28, 2015, at 9:29 PM, Sharma, Shashank <shashank.sharma at intel.com> wrote:
> 
> Ok, anyways it was sounding like a good idea. 
> Will do the changes accordingly. 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Roper, Matthew D 
> Sent: Tuesday, September 29, 2015 3:13 AM
> To: Sharma, Shashank
> Cc: Daniel Vetter; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; Matheson, Annie J; kausalmalladi at gmail.com; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
> 
> Yep, Daniel's right; for properties that are not specific to the driver, the core core should take care of stuffing the values provided by userspace into the state structures so that every driver that wants to use these doesn't need to replicate that logic.  My bad for not catching this in my earlier reviews; sorry about that.  Basically the change required is to add another else clause to drm_atomic_crtc_{get,set}_property(), before the core tries to call into the driver-specific handler.  You'll notice that what you're doing with color management blobs here is actually very similar to what's already done for the existing mode blob, so you can take a look at that case for an example.
> 
> Also, the functions like intel_color_manager_set_pipe_gamma() will get moved into the DRM core and the "intel_" prefix will switch to "drm_"
> since there's really nothing Intel-specific about what they're doing.
> 
> Sorry again for not noticing this and bringing it up on the earlier reviews.  Fortunately the changes required should be pretty small.
> 
> 
> Matt
> 
>> On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote:
>> Matt, your opinion about this ? 
>> 
>> Regards
>> Shashank
>> -----Original Message-----
>> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of 
>> Daniel Vetter
>> Sent: Monday, September 28, 2015 12:14 PM
>> To: Sharma, Shashank
>> Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; 
>> Smith, Gary K; dri-devel at lists.freedesktop.org; 
>> intel-gfx at lists.freedesktop.org; Matheson, Annie J; 
>> kausalmalladi at gmail.com; Vetter, Daniel
>> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction 
>> handlers
>> 
>>> On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
>>> On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
>>>>> Since color manager properties are meant as a new standardize KMS 
>>>>> extension (we put them into the core drm_crtc_state) the get/set 
>>>>> support should also be in the core. See e.g. how the rotation 
>>>>> property is handled in drm_atomic_plane_get/set_property. So all 
>>>>> this code should be added to drm_atomic_crtc_get/set_property.
>>>> Thanks, sounds like a good one. Will move this.
>>> Actually, while implementing this, I realized that this change is 
>>> not required.
>>> What we want to do in drm_atomic_crtc_get/set code is:
>>> if (prop == config->cm_palette_after_ctm_property || prop ==    
>>>    config->cm_palette_before_ctm_property) {
>>>    crtc->funcs->atomic_get_property();
>>> }
>>> 
>>> Which is already being done in the current code:
>>> else if (crtc->funcs->atomic_get_property)
>>>    return crtc->funcs->atomic_get_property(crtc, state, property, 
>>> val);
>> 
>> This code is to pass any property unknown to the drm core into the driver.
>> But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
>> 
>> Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795