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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Kausal Malladi <kausalmalladi@xxxxxxxxx>
---
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
