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
