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

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx