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.
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