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 ? >> + /* >> + * 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.
Shashank > >> + /* >> + * Starting of palette LUT in R32G32B32 format. >> + * Each of RGB value is in U8.24 fixed point format. >> + * Actual number of samples will depend upon num_samples >> + */ >> + struct drm_r32g32b32 lut[0]; >> +}; >> + >> /* typedef area */ >> #ifndef __KERNEL__ >> typedef struct drm_clip_rect drm_clip_rect_t; >> -- >> 1.9.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel >