| Message ID | 1442425040-32185-5-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > +}; > + > +struct drm_palette { > + /* Structure version. Should be 1 currently */ > + __u32 version; I don't think we want to version the blobs. For one, we don't have a way for userspace to ask for a specific version, so the kernel wouldn't know which version it should return to each client. If we ever need to come up with new version of a specific blob, I think we just have to define another property for it. Either that or we'd some kind of client cap stuff to negotiate the used version between the kernel and a specific client. Well, I suppose we migth be able to borrow the "flags+extend at the end" trick we sometimes use for ioctl parameters to work for blobs too. But I have a feeling we don't want to go there. So yeah, I think we should go with the "blob layout is fixed for each property" approach. Versioning then happens by introducing new versions of the same property if needed. > + /* > + * This has to be a supported value during get call. > + * Feature will be disabled if this is 0 while set > + */ > + __u32 num_samples; > + /* > + * 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Regards Shashank On 9/21/2015 10:16 PM, Ville Syrjälä 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; >> +}; >> + >> +struct drm_palette { >> + /* Structure version. Should be 1 currently */ >> + __u32 version; > > I don't think we want to version the blobs. For one, we don't have a way > for userspace to ask for a specific version, so the kernel wouldn't know > which version it should return to each client. > > If we ever need to come up with new version of a specific blob, I think we > just have to define another property for it. Either that or we'd some kind > of client cap stuff to negotiate the used version between the kernel and > a specific client. > > Well, I suppose we migth be able to borrow the "flags+extend at the end" > trick we sometimes use for ioctl parameters to work for blobs too. But I > have a feeling we don't want to go there. > > So yeah, I think we should go with the "blob layout is fixed for each > property" approach. Versioning then happens by introducing new versions > of the same property if needed. > Well, reason behind adding this version was, as this framework was a new development, we wanted to keep scope for further changes on request from other drivers/modules. But yes, I agree, its kind of overhead, so we can also remove it. Will do that in next patch set. >> + /* >> + * This has to be a supported value during get call. >> + * Feature will be disabled if this is 0 while set >> + */ >> + __u32 num_samples; >> + /* >> + * 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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel >
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. > +}; > + > +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). > + /* > + * 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 > + /* > + * 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 22 September 2015 at 14:08, Daniel Vetter <daniel@ffwll.ch> 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 [snip] >> +struct drm_palette { [snip] > ... extending the ioctl struct at the end ... Is this really going to work, considering that we currently have a zero sized drm_r32g32b32 array at the end ? Iirc someone mentioned that using a pointer to the data (over an array) has drawbacks, although for the sake of me I cannot think of any. Can anyone care to enlighten me, please ? Thanks Emil
On Tue, Sep 22, 2015 at 02:53:54PM +0100, Emil Velikov wrote: > On 22 September 2015 at 14:08, Daniel Vetter <daniel@ffwll.ch> 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 > [snip] > >> +struct drm_palette { > [snip] > > ... extending the ioctl struct at the end ... > Is this really going to work, considering that we currently have a > zero sized drm_r32g32b32 array at the end ? Well in this particular case it would be ugly. Sure, it can be done, but we couldn't actually define the struct for it using C. Would be a neat feature though. Someone should propose it for the next C standard :) In any case, for blobs I think we shouldn't extend them like ioctls. In this case the blob is just an array of something, so the blob size can of course vary depending how many elements are required for the particual platform. > Iirc someone mentioned that using a pointer to the data (over an > array) has drawbacks, although for the sake of me I cannot think of > any. Can anyone care to enlighten me, please ? I guess an extensible array at the end of the struct is simply a nice fit sometimes. Also makes it more obvious how much junk gets copied over I suppose.
Hi Ville, On 22 September 2015 at 16:00, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Sep 22, 2015 at 02:53:54PM +0100, Emil Velikov wrote: >> On 22 September 2015 at 14:08, Daniel Vetter <daniel@ffwll.ch> 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 >> [snip] >> >> +struct drm_palette { >> [snip] >> > ... extending the ioctl struct at the end ... >> Is this really going to work, considering that we currently have a >> zero sized drm_r32g32b32 array at the end ? > > Well in this particular case it would be ugly. Sure, it can be done, > but we couldn't actually define the struct for it using C. Would be > a neat feature though. Someone should propose it for the next C > standard :) > > In any case, for blobs I think we shouldn't extend them like ioctls. > In this case the blob is just an array of something, so the blob size > can of course vary depending how many elements are required for the > particual platform. > Sure it makes sense on its own. I'm curious how likely it will be for people to get confused/carried away and miss that part. Just a food for thought. >> Iirc someone mentioned that using a pointer to the data (over an >> array) has drawbacks, although for the sake of me I cannot think of >> any. Can anyone care to enlighten me, please ? > > I guess an extensible array at the end of the struct is simply a nice > fit sometimes. Also makes it more obvious how much junk gets copied over > I suppose. > So no serious 'issues' there, but it's mostly done for convenience. Thanks for clarifying. Regards, Emil
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 ? >> + /* >> + * 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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel >
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
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
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@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. 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
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; +}; + +struct drm_palette { + /* Structure version. Should be 1 currently */ + __u32 version; + /* + * This has to be a supported value during get call. + * Feature will be disabled if this is 0 while set + */ + __u32 num_samples; + /* + * 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;