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

[Daniel Vetter]

On Wed, Sep 23, 2015 at 06:29:31PM +0530, Sharma, Shashank wrote:
> 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 at 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 at intel.com>
> >>>>Signed-off-by: Kausal Malladi <kausalmalladi at 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.

I'm confused, since this is exactly what I'm suggesting. My only
observation is that we don't need a separate num_samples field in the blob
structure itself since userspace already needs to tell the kernel the size
of the blob property separately. And we can derive num_samples from the
size of the blob easily (which means it'll make the necessary overflow
checks simpler).

You _must_ check drm_property_blob->length anyway (atm that seems to be
missing, but I didn't check with a full search), so might as well use that
to compute num_samples instead of just making sure they match.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch