On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
From: Kausal Malladi Kausal.Malladi@intel.com
The DRM color management framework is targeting various hardware platforms and drivers. Different platforms can have different color correction and enhancement capabilities.
A commom user space application can query these capabilities using the DRM property interface. Each driver can fill this property with its platform's color capabilities.
This patch adds new structures in DRM layer for querying color capabilities. These structures will be used by all user space agents to configure appropriate color configurations.
As I indicated before, I don't think we should go for a full fledged query API, because, I don't believe we can ever make it good enough to cover future hardware (and probably not today's hardware across vendors).
These kinds of query APIs have been frown upon in the past for that exact reason.
- Accept configurations that are mostly likely to be working across vendors (256 enties for 8 bits) That should be enough for basic functionality.
- To support things that are really hw specific: make sure the kernel API can accept those, put the hw specific knowledge into a user-space HAL where APIs can evolve. What you're trying to do here with queries about per-platform details can go into userspace and still have a generic compositor code using those limits. Let's just not put the limits into the kernel API, we won't be able to get it right.
Now maybe there's a middle ground and we can expose basic limits. In this case, maybe a list of supported LUT sizes, but the sampling details don't belong to a kernel interface IMHO. I'm even hesitant putting the hw precision at that level.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi Kausal.Malladi@intel.com
include/uapi/drm/drm.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3801584..d9562a2 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -829,6 +829,40 @@ struct drm_event_vblank { __u32 reserved; };
+struct drm_palette_sampling_details {
- __u32 sample_fract_precision;
- __u32 last_sample_int_max;
- __u32 remaining_sample_int_max;
- __u32 num_samples;
+};
If we're are indeed going with this, it will need documentation in the code. Why do we need the precision for instance? It's not clear to me.
Also, we don't describe what the values beyond 1.0 represent, how the interpolation is done, etc... (to support my first point to no try describing too much).
+struct drm_palette_caps {
- __u32 version;
- __u32 num_supported_types;
- struct drm_palette_sampling_details palette_sampling_types[4];
+};
+struct drm_ctm_caps {
- __u32 version;
- __u32 ctm_coeff_fract_precision;
- __u32 ctm_coeff_int_max;
- __s32 ctm_coeff_int_min;
+};
Same story for those precision, int_min and int_max fields. Why do we need those? if we get 256 enties, we can take the values in the interface form (whatever it is, say 15.16 + sign bit) and clip the values to what the hardware supports.
+struct drm_cge_caps {
- __u32 version;
- __u32 cge_max_weight;
+};
This doesn't beling to DRM
+struct drm_color_caps {
- __u32 version;
- __u32 reserved;
Why do we need reserved here? isn't version enough to make sure we can grow the structure?
- struct drm_palette_caps palette_caps_after_ctm;
- struct drm_palette_caps palette_caps_before_ctm;
- struct drm_ctm_caps ctm_caps;
- struct drm_cge_caps cge_caps;
No CGE in the DRM core, other hw won't have it.
+};
/* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t; -- 2.4.5
On Thu, Jul 02, 2015 at 05:20:45PM +0100, Damien Lespiau wrote:
On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
From: Kausal Malladi Kausal.Malladi@intel.com
The DRM color management framework is targeting various hardware platforms and drivers. Different platforms can have different color correction and enhancement capabilities.
A commom user space application can query these capabilities using the DRM property interface. Each driver can fill this property with its platform's color capabilities.
This patch adds new structures in DRM layer for querying color capabilities. These structures will be used by all user space agents to configure appropriate color configurations.
As I indicated before, I don't think we should go for a full fledged query API, because, I don't believe we can ever make it good enough to cover future hardware (and probably not today's hardware across vendors).
These kinds of query APIs have been frown upon in the past for that exact reason.
- Accept configurations that are mostly likely to be working across vendors
(256 enties for 8 bits) That should be enough for basic functionality.
- To support things that are really hw specific: make sure the kernel API can
accept those, put the hw specific knowledge into a user-space HAL where APIs can evolve. What you're trying to do here with queries about per-platform details can go into userspace and still have a generic compositor code using those limits. Let's just not put the limits into the kernel API, we won't be able to get it right.
Now maybe there's a middle ground and we can expose basic limits. In this case, maybe a list of supported LUT sizes, but the sampling details don't belong to a kernel interface IMHO. I'm even hesitant putting the hw precision at that level.
To re-iterate the point with actual examples, the proposed query API doesn't seem to handle things we'd want to know today:
- What are the extra values are coding for. eg. for 8 bits, the values after index 255 are special and we have no description for those. (and it can get fiddly to describe them, you may want to add the type of interpolation for instance). - How to you represent capabilities across hardware unit. Eg. split gamma lowering the number of LUT entries.
That somewhat demonstrates my point I think. We won't be able to get the query API right.
On Thu, Jul 02, 2015 at 05:45:32PM +0100, Damien Lespiau wrote:
On Thu, Jul 02, 2015 at 05:20:45PM +0100, Damien Lespiau wrote:
On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
From: Kausal Malladi Kausal.Malladi@intel.com
The DRM color management framework is targeting various hardware platforms and drivers. Different platforms can have different color correction and enhancement capabilities.
A commom user space application can query these capabilities using the DRM property interface. Each driver can fill this property with its platform's color capabilities.
This patch adds new structures in DRM layer for querying color capabilities. These structures will be used by all user space agents to configure appropriate color configurations.
As I indicated before, I don't think we should go for a full fledged query API, because, I don't believe we can ever make it good enough to cover future hardware (and probably not today's hardware across vendors).
These kinds of query APIs have been frown upon in the past for that exact reason.
- Accept configurations that are mostly likely to be working across vendors
(256 enties for 8 bits) That should be enough for basic functionality.
- To support things that are really hw specific: make sure the kernel API can
accept those, put the hw specific knowledge into a user-space HAL where APIs can evolve. What you're trying to do here with queries about per-platform details can go into userspace and still have a generic compositor code using those limits. Let's just not put the limits into the kernel API, we won't be able to get it right.
Now maybe there's a middle ground and we can expose basic limits. In this case, maybe a list of supported LUT sizes, but the sampling details don't belong to a kernel interface IMHO. I'm even hesitant putting the hw precision at that level.
To re-iterate the point with actual examples, the proposed query API doesn't seem to handle things we'd want to know today:
- What are the extra values are coding for. eg. for 8 bits, the values after index 255 are special and we have no description for those. (and it can get fiddly to describe them, you may want to add the type of interpolation for instance).
- How to you represent capabilities across hardware unit. Eg. split gamma lowering the number of LUT entries.
That somewhat demonstrates my point I think. We won't be able to get the query API right.
Yeah. My first idea for the gamma stuff was that we'd simply have the blob property for the data, and then a separate enum property which decides how we interpret the blob contents. The default for the enum would be the 8bpc/256 entries thing always, but the other values could be potentially hardware specific. Obviously this would limit the use of the fancier modes to the atomic API since you'd need to configure both the blob and enum at the same time. But I don't see why we shouldn't be allowed to rely on atomic from now on.
Well, I guess we could allow the user to change just the enum if the expected blob size will match what was already provided, although thinking up a valid use case for this is a bit hard :)
On Thu, Jul 02, 2015 at 08:04:40PM +0300, Ville Syrjälä wrote:
On Thu, Jul 02, 2015 at 05:45:32PM +0100, Damien Lespiau wrote:
On Thu, Jul 02, 2015 at 05:20:45PM +0100, Damien Lespiau wrote:
On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
From: Kausal Malladi Kausal.Malladi@intel.com
The DRM color management framework is targeting various hardware platforms and drivers. Different platforms can have different color correction and enhancement capabilities.
A commom user space application can query these capabilities using the DRM property interface. Each driver can fill this property with its platform's color capabilities.
This patch adds new structures in DRM layer for querying color capabilities. These structures will be used by all user space agents to configure appropriate color configurations.
As I indicated before, I don't think we should go for a full fledged query API, because, I don't believe we can ever make it good enough to cover future hardware (and probably not today's hardware across vendors).
These kinds of query APIs have been frown upon in the past for that exact reason.
- Accept configurations that are mostly likely to be working across vendors
(256 enties for 8 bits) That should be enough for basic functionality.
- To support things that are really hw specific: make sure the kernel API can
accept those, put the hw specific knowledge into a user-space HAL where APIs can evolve. What you're trying to do here with queries about per-platform details can go into userspace and still have a generic compositor code using those limits. Let's just not put the limits into the kernel API, we won't be able to get it right.
Now maybe there's a middle ground and we can expose basic limits. In this case, maybe a list of supported LUT sizes, but the sampling details don't belong to a kernel interface IMHO. I'm even hesitant putting the hw precision at that level.
To re-iterate the point with actual examples, the proposed query API doesn't seem to handle things we'd want to know today:
- What are the extra values are coding for. eg. for 8 bits, the values after index 255 are special and we have no description for those. (and it can get fiddly to describe them, you may want to add the type of interpolation for instance).
- How to you represent capabilities across hardware unit. Eg. split gamma lowering the number of LUT entries.
That somewhat demonstrates my point I think. We won't be able to get the query API right.
Yeah. My first idea for the gamma stuff was that we'd simply have the blob property for the data, and then a separate enum property which decides how we interpret the blob contents. The default for the enum would be the 8bpc/256 entries thing always, but the other values could be potentially hardware specific. Obviously this would limit the use of the fancier modes to the atomic API since you'd need to configure both the blob and enum at the same time. But I don't see why we shouldn't be allowed to rely on atomic from now on.
Yeah, enum+blob is what I like too. We can do one gamma table format enum in drm core. And drivers can then create it with just the enum values they actually support, similar to how we have rotation/mirror defines for everything, but the driver doesn't necessarily support it all. And the enum would fully encode everything there is to know about a layout, i.e. including excat precision and stuff like the 1025th/513th additional entry we have in some intel gamma tables. -Daniel
On Fri, Jul 03, 2015 at 08:41:31AM +0200, Daniel Vetter wrote:
Yeah. My first idea for the gamma stuff was that we'd simply have the blob property for the data, and then a separate enum property which decides how we interpret the blob contents. The default for the enum would be the 8bpc/256 entries thing always, but the other values could be potentially hardware specific. Obviously this would limit the use of the fancier modes to the atomic API since you'd need to configure both the blob and enum at the same time. But I don't see why we shouldn't be allowed to rely on atomic from now on.
Yeah, enum+blob is what I like too. We can do one gamma table format enum in drm core. And drivers can then create it with just the enum values they actually support, similar to how we have rotation/mirror defines for everything, but the driver doesn't necessarily support it all. And the enum would fully encode everything there is to know about a layout, i.e. including excat precision and stuff like the 1025th/513th additional entry we have in some intel gamma tables.
I'm assuming you're still talking about 3 separate properties though? pre-csc lut, csc, post-csc lut?
On Fri, Jul 03, 2015 at 11:28:41AM +0100, Damien Lespiau wrote:
On Fri, Jul 03, 2015 at 08:41:31AM +0200, Daniel Vetter wrote:
Yeah. My first idea for the gamma stuff was that we'd simply have the blob property for the data, and then a separate enum property which decides how we interpret the blob contents. The default for the enum would be the 8bpc/256 entries thing always, but the other values could be potentially hardware specific. Obviously this would limit the use of the fancier modes to the atomic API since you'd need to configure both the blob and enum at the same time. But I don't see why we shouldn't be allowed to rely on atomic from now on.
Yeah, enum+blob is what I like too. We can do one gamma table format enum in drm core. And drivers can then create it with just the enum values they actually support, similar to how we have rotation/mirror defines for everything, but the driver doesn't necessarily support it all. And the enum would fully encode everything there is to know about a layout, i.e. including excat precision and stuff like the 1025th/513th additional entry we have in some intel gamma tables.
I'm assuming you're still talking about 3 separate properties though? pre-csc lut, csc, post-csc lut?
For CHV that would seem to fit since the pre and post gamma have different size and precision. Although I suppose we could stuff it in one blob if needed. Hmm. Actually I think you can even enable both GGM and the old gamma unit at the same time, but maybe we should just decice that it's going too far and not provide the option for that.
And we'd need to consider the CGM CSC vs. VLV wide gamut CSC too. The pipe looks something like this: "cgm degamma -> cgm csc -> cgm gamma -> wide gamut CSC -> pipe gamma" So just pre and post csc gammas aren't going to cut it if we want to expoe it all. But if we decice to not expose the wide gamut csc, then we should be able to use either the cgm gamma or the pipe gamma as the post-csc gamma.
Another thing which is unclear at this point is whether the plane control register gamma bit can be used to bypass the CGM gamma/degamma on a per plane basis, or if the bit only affects the pipe gamma.
VLV is easier since it just has the wide gamut csc and pipe gamma after it, and older gmch platforms have no csc unit.
For ILK+ we can choose to position the gamma before or after the csc, or we can do the split gamma on IVB+. So from hardware POV it's just a single table, but I suppose we could expose it as two blobs to be a bit more compatible with CHV if we use two blobs there.
I tried to enumerate all the different gamma modes we have and came up with the following list: - 0.8 x 256 legacy gamma - 0.10 and 6bit slope x 128 gen2/3 10bit interpolated gamma - 0.16 x 128 + 1.16 x 1 gen4/g4x/vlv/chv 10bit interpolated gamma - 0.14 x 65 chv interpolated gcm degamma - 0.10 x 257 chv interpolated gcm gamma - 0.10 x 1024 ilk/snb 10bit gamma - 0.16 x 512 + 1.16 x 1 ilk/snb 12bit interpolated gamma - 0.10 x 1024 + 3.16 x 1 ivb+ 10bit gamma - 0.10 x 512 + 3.16 x 1 ivb+ split gamma before csc - 0.10 x 512 ivb+ split gamma after csc - 0.16 x 512 + 1.16 x 1 + 3.16 x 1 ivb+ 12bit interpolated gamma
I hope I didn't forget anyting. Also looks like in the future we'll get yet another extra entry for the higher precision modes.
So if we have two blobs, would we also want two enums? In that case we'd also need some kind of "bypass" or "disabled" enum value. But with two enums coming up with supported combinations might become a bit hairy. If we'd instead go with a single enum I suppose we'd need to have pre-csc and post-csc variants of all the non-split modes for ilk+.
dri-devel@lists.freedesktop.org
-
Damien Lespiau -
Daniel Vetter -
Ville Syrjälä