Re: [PATCH 04/23] drm: Add drm structures for palette color property

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards
Shashank

On 9/23/2015 6:19 PM, Daniel Vetter wrote:
> On Wed, Sep 23, 2015 at 01:45:16PM +0530, Sharma, Shashank wrote:
>> Regards
>> Shashank
>>
>> On 9/22/2015 6:38 PM, Daniel Vetter wrote:
>>> On Wed, Sep 16, 2015 at 11:07:01PM +0530, Shashank Sharma wrote:
>>>> From: Kausal Malladi <kausalmalladi@gmail.com>
>>>>
>>>> This patch adds new structures in DRM layer for Palette color
>>>> correction.These structures will be used by user space agents
>>>> to configure appropriate number of samples and Palette LUT for
>>>> a platform.
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>>>> ---
>>>>   include/uapi/drm/drm.h | 27 +++++++++++++++++++++++++++
>>>>   1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>>> index e3c642f..f72b916 100644
>>>> --- a/include/uapi/drm/drm.h
>>>> +++ b/include/uapi/drm/drm.h
>>>> @@ -840,6 +840,33 @@ struct drm_palette_caps {
>>>>   	__u32 num_samples_after_ctm;
>>>>   };
>>>>
>>>> +struct drm_r32g32b32 {
>>>> +	/*
>>>> +	 * Data is in U8.24 fixed point format.
>>>> +	 * All platforms support values within [0, 1.0] range,
>>>> +	 * for Red, Green and Blue colors.
>>>> +	 */
>>>> +	__u32 r32;
>>>> +	__u32 g32;
>>>> +	__u32 b32;
>>>
>>> It's not strictly required, but adding a __u32 reserved here to align the
>>> struct to 64 bits seems good imo. Slight overhead but meh about that.
>> Humm, ok, we can check this out.
>>>
>>>> +};
>>>> +
>>>> +struct drm_palette {
>>>> +	/* Structure version. Should be 1 currently */
>>>> +	__u32 version;
>>>
>>> Definitely great practice to take compat into account and definitely
>>> needed for the first design using ioctls but I don't think we need this
>>> here. Properties are already extinsible themselves: We can just greate a
>>> "ctm-v2", "ctm-v3" if the layout changes, and since the actual ctm matrix
>>> is stored in the drm_crtc_state any compat code on the kernel will be
>>> shared.
>>>
>>> Aside: For an ioctl the recommended way to handle backwards compat and
>>> extensions in drm is with a flags bitfield. That's more flexible than a
>>> linear version field, and extending the ioctl struct at the end is already
>>> handled by the drm core in a transparent fashion (it 0-fills either kernel
>>> or userspace side).
>>>
>> Agree, we will drop this. Do you think we should add a flags field, or is it
>> ok without it ?
>
> No need for a flag field since this is not an ioctl struct. That "Aside:"
> was really meant as a comment aside and not relevant for properties.
>
>>>> +	/*
>>>> +	 * This has to be a supported value during get call.
>>>> +	 * Feature will be disabled if this is 0 while set
>>>> +	 */
>>>> +	__u32 num_samples;
>>>
>>> blob properties already have a size, storing it again in the blob is
>>> redundnant. Instead I think a small helper to get the number of samples
>>> for a given gamma table blob would be needed.
>>>
>>> Cheers, Daniel
>> Please note that they are different. One is the size of blob and other one
>> is the num_samples supported by the property, in the current correction
>> mode. If you check the design doc, num_sample serves the purpose of deciding
>> which correction mode to be applied also. fox ex, for gamma, num_samples=0
>> indicates disable gamma, whereas num_samples=512 indicates split gamma mode.
>
> num_samples = blob_size/(sizeof(drm_r32g32b32));
>
> I just think that this information is redundant and if userspace supplies
> a gamma table with the wrong size we should just reject it. There's really
> no reason for userspace to create a blob property where the size doesn't
> exactly match the gamma table.
>
> I guess again that this was needed for the ioctl where there's no sideband
> for the size. But properties _are_ sized.
Again, this is what we decided in the design discussion. The driver will 
showcase the best option for property, but that doesn't stop a user 
space with more knowledge of HW to send other supported options. for 
example, in case of gamma, the driver supports all 3 possible modes:
- 8 bit legacy gamma (256 coeff)
- 10 bit split gamma (1024 coeff (512 + 512))
- 12 bit interpolated gamma (coeff 513)
So here, we have used the no of coeff to define which type of gamma we 
want to apply. So in the core gamma function you will find 4 cases:
switch(no_coeff)
case 0: disable gamma;
case 256: enable legacy gamma;
case 512: enable 10 bit split gamma;
case 513: enable 12 bit interpolated gamma;

This is the simplest implementation, and there is no need for any 
additional variable.
>
> Also, you need to make sure that the property size is aligned and reject
> the gamma table property if that's not the case, i.e.
>
> 	if (blob_size % sizeof(drm_r32g32b32))
> 		return -EINVAL;
>
>
> Plus then driver-specific code to reject anything that's not one of the
> supported sizes too.
>
> Of course that also needs igt test coverage.
> -Daniel
>
Shashank
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel