This patch set adds Color Manager implementation in DRM layer. Color Manager is an extension in DRM framework to support color correction/enhancement.
Various Hardware platforms can support several color correction capabilities. Color Manager provides abstraction of these capabilities and allows a user space UI agent to correct/enhance the display using the DRM property interface.
How is this going to work? ========================== 1. This patch series adds a few new properties in DRM framework. These properties are: a. color_capabilities property (type blob) b. Color Transformation Matrix property for corrections like CSC (called CTM, type blob) c. Palette correction properties for corrections like gamma fixup (called palette_correction, type blob) 2. Also, this patch series adds few structures to indicate specifications of a property like size, no_of_samples for correction etc. 3. These properties are present in mode_config. 4. When the platform's display driver loads, it fills up the values of color_capabilities property using the standard structures (added in step 2). For example, Intel's I915 driver adds following color correction capabilities: a. gamma correction capability as palette correction property, with 257 correction coefficients and a max/min value b. csc correction capability as CTM correction property, with 3x3 transformation matrix values and max/min values 5. Now when userspace comes up, it queries the platform's color capabilities by doing a get_property() on color_capabilities DRM property 6. Reading the blob, the userspace understands the color capabilities of the platform. For example, userspace will understand it can support: a. palette_correction with 257 coefficients b. CSC correction with 3x3 = 9 values 7. To set color correction values, userspace: a. creates a blob using the create_blob_ioctl in standard palette_correction structure format, with the correction values b. calls the set_property_ioctl with the blob_id as value for the property 8. Driver refers to the blob, gets the correction values and applies the correction in HW. 9. To get currently applied color correction values, userspace: a. calls a get_property_ioctl on that color property b. gets the blob_id for the currently applied correction from DRM infrastructure c. gets the blob using get_blob_ioctl and hence the currently applied values
That's all! :)
About the patch series: ======================= The patch series first adds the color management support in DRM layer. Then it adds the color management framework in I915 layer. After that, it implements platform specific core color correction functios.
Intel color manager registers color correction with DRM color manager in this way: - CSC transformation is registered as CTM DRM property - Gamma correction is registered as palette_after_ctm DRM property - Degamma correction is registered as palette_before_ctm DRM property
Our thanks to all the reviewers who have given valuable comments in terms of design and implementation to our previous sets of patches. Special mention of thanks should go to Matt Roper for all his inputs/suggestions in implementation of this module, using DRM atomic CRTC commit path.
V2: Worked on review comments from Matt, Jim, Thierry, Rob. V3: Worked on review comments from Matt, Jim, Rob: - Jim, Rob: ======== Re-arranged the whole patch series in the sequence of features, currently: First 5 patches add color management support in DRM layer Next 7 patches add Intel color management framework in I915 driver Next 5 patches add color correction for CHV (gamma, degamma and CSC) Next 2 patches enable color management, by attaching the properties to CRTC(Matt) Next 4 patches add color correction for BDW (gamma, degamma) - Matt: ===== Patch 3: Added refernce/unreference for blob Patch 7: return -EINVAL and added debug message Patch 8: check for valid blob, from create blob moved call to intel_crtc_attach_color_prop in the end of full implementation (CHV) Patch 9: DRM_ERROR->DRM_DEBUG for NULL blob case Patch 13: Added static for internal functions Patch 20-24: renamed gen9_* functions to bdw_* Added new variables in device_info structure num_samples_after_ctm and num_samples_before_ctm Added new function in patch 8 to load capabilities based on device_info across all platforms
Shashank Sharma (23): drm: Create Color Management DRM properties drm: Add structure for querying palette color capabilities drm: Add color correction blobs in CRTC state drm: Add drm structures for palette color property drm: Add structure to set/get a CTM color property drm/i915: Add atomic set property interface for CRTC drm/i915: Add atomic get property interface for CRTC drm/i915: Create color management files drm/i915: Register pipe color capabilities drm/i915: Add gamma correction handlers drm/i915: Add pipe deGamma correction handlers drm/i915: Add pipe CSC correction handlers drm/i915: CHV: Load gamma color correction values drm/i915: CHV: Pipe level Gamma correction drm/i915: CHV: Load degamma color correction values drm/i915: CHV: Pipe level degamma correction drm/i915: CHV: Pipe level CSC correction drm/i915: Commit color changes to CRTC drm/i915: Attach color properties to CRTC drm/i915: BDW: Load gamma correction values drm/i915: BDW: Pipe level Gamma correction drm/i915: BDW: Load degamma correction values drm/i915: BDW: Pipe level degamma correction
drivers/gpu/drm/drm_atomic_helper.c | 12 + drivers/gpu/drm/drm_crtc.c | 26 + drivers/gpu/drm/i915/Makefile | 3 +- drivers/gpu/drm/i915/i915_drv.c | 17 + drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_reg.h | 41 +- drivers/gpu/drm/i915/intel_atomic.c | 56 ++ drivers/gpu/drm/i915/intel_color_manager.c | 852 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 99 ++++ drivers/gpu/drm/i915/intel_display.c | 6 + drivers/gpu/drm/i915/intel_drv.h | 22 + include/drm/drm_crtc.h | 11 + include/uapi/drm/drm.h | 50 ++ 13 files changed, 1195 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
From: Kausal Malladi kausalmalladi@gmail.com
Color Management is an extension to Kernel display framework. It allows abstraction of hardware color correction and enhancement capabilities by virtue of DRM properties.
This patch initializes color management framework by : 1. Introducing new pointers in DRM mode_config structure to carry CTM and Palette color correction properties. 2. Creating these DRM properties in DRM standard properties creation sequence.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 2 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9b9c4b4..d809c67 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1472,6 +1472,32 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_mode_id = prop;
+ /* Color Management properties */ + prop = drm_property_create(dev, + DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE, + "CRTC_PALETTE_CAPABILITIES", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cm_crtc_palette_capabilities_property = prop; + + prop = drm_property_create(dev, + DRM_MODE_PROP_BLOB, "PALETTE_AFTER_CTM", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cm_palette_after_ctm_property = prop; + + prop = drm_property_create(dev, + DRM_MODE_PROP_BLOB, "PALETTE_BEFORE_CTM", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cm_palette_before_ctm_property = prop; + + prop = drm_property_create(dev, + DRM_MODE_PROP_BLOB, "CTM", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cm_ctm_property = prop; + return 0; }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c0366e9..c35531e 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1153,6 +1153,12 @@ struct drm_mode_config { struct drm_property *suggested_x_property; struct drm_property *suggested_y_property;
+ /* Color Management Properties */ + struct drm_property *cm_crtc_palette_capabilities_property; + struct drm_property *cm_palette_before_ctm_property; + struct drm_property *cm_palette_after_ctm_property; + struct drm_property *cm_ctm_property; + /* dumb ioctl parameters */ uint32_t preferred_depth, prefer_shadow;
On Wed, Sep 16, 2015 at 11:06:58PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.com
Color Management is an extension to Kernel display framework. It allows abstraction of hardware color correction and enhancement capabilities by virtue of DRM properties.
This patch initializes color management framework by :
- Introducing new pointers in DRM mode_config structure to carry CTM and Palette color correction properties.
- Creating these DRM properties in DRM standard properties creation sequence.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
We should probably update the property section of Documentation/DocBook/drm.tmpl with this patch as well to include these new properties in the table (that docbook ultimately generates documentation that looks like https://kernel.org/doc/htmldocs/drm/drm-kms-properties.html ).
One minor note: people not involved in color management probably won't immediately figure out what "CTM" stands for, so you might want to just add a comment somewhere that spells out the full "color transformation matrix" term.
Matt
drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 2 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9b9c4b4..d809c67 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1472,6 +1472,32 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_mode_id = prop;
- /* Color Management properties */
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,"CRTC_PALETTE_CAPABILITIES", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cm_crtc_palette_capabilities_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB, "PALETTE_AFTER_CTM", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cm_palette_after_ctm_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB, "PALETTE_BEFORE_CTM", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cm_palette_before_ctm_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB, "CTM", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cm_ctm_property = prop;
- return 0;
}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c0366e9..c35531e 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1153,6 +1153,12 @@ struct drm_mode_config { struct drm_property *suggested_x_property; struct drm_property *suggested_y_property;
- /* Color Management Properties */
- struct drm_property *cm_crtc_palette_capabilities_property;
- struct drm_property *cm_palette_before_ctm_property;
- struct drm_property *cm_palette_after_ctm_property;
- struct drm_property *cm_ctm_property;
- /* dumb ioctl parameters */ uint32_t preferred_depth, prefer_shadow;
-- 1.9.1
Sure Matt, We are planning to add documentation for color properties, as suggested by you and Daniel.
Regards Shashank -----Original Message----- From: Roper, Matthew D Sent: Wednesday, September 16, 2015 11:21 PM To: Sharma, Shashank Cc: Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com Subject: Re: [PATCH 01/23] drm: Create Color Management DRM properties
On Wed, Sep 16, 2015 at 11:06:58PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.com
Color Management is an extension to Kernel display framework. It allows abstraction of hardware color correction and enhancement capabilities by virtue of DRM properties.
This patch initializes color management framework by :
- Introducing new pointers in DRM mode_config structure to carry CTM and Palette color correction properties.
- Creating these DRM properties in DRM standard properties creation sequence.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
We should probably update the property section of Documentation/DocBook/drm.tmpl with this patch as well to include these new properties in the table (that docbook ultimately generates documentation that looks like https://kernel.org/doc/htmldocs/drm/drm-kms-properties.html ).
One minor note: people not involved in color management probably won't immediately figure out what "CTM" stands for, so you might want to just add a comment somewhere that spells out the full "color transformation matrix" term.
Matt
drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 2 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9b9c4b4..d809c67 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1472,6 +1472,32 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_mode_id = prop;
- /* Color Management properties */
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,"CRTC_PALETTE_CAPABILITIES", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cm_crtc_palette_capabilities_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB, "PALETTE_AFTER_CTM", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cm_palette_after_ctm_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB, "PALETTE_BEFORE_CTM", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cm_palette_before_ctm_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB, "CTM", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cm_ctm_property = prop;
- return 0;
}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c0366e9..c35531e 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1153,6 +1153,12 @@ struct drm_mode_config { struct drm_property *suggested_x_property; struct drm_property *suggested_y_property;
- /* Color Management Properties */
- struct drm_property *cm_crtc_palette_capabilities_property;
- struct drm_property *cm_palette_before_ctm_property;
- struct drm_property *cm_palette_after_ctm_property;
- struct drm_property *cm_ctm_property;
- /* dumb ioctl parameters */ uint32_t preferred_depth, prefer_shadow;
-- 1.9.1
From: Kausal Malladi kausalmalladi@gmail.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 palette color capabilities.
This patch adds new structure in DRM layer for querying palette color capabilities. This structure will be used by all user space agents to configure appropriate color configurations.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- include/uapi/drm/drm.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3801584..e3c642f 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -829,6 +829,17 @@ struct drm_event_vblank { __u32 reserved; };
+struct drm_palette_caps { + /* Structure version. Should be 1 currently */ + __u32 version; + /* For padding and future use */ + __u32 reserved; + /* This may be 0 if not supported. e.g. plane palette or VLV pipe */ + __u32 num_samples_before_ctm; + /* This will be non-zero for pipe. May be zero for planes on some HW */ + __u32 num_samples_after_ctm; +}; + /* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t;
On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.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 palette color capabilities.
This patch adds new structure in DRM layer for querying palette color capabilities. This structure will be used by all user space agents to configure appropriate color configurations.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
I think you provided an explanation on a previous code review cycle, but I forget the details now...what's the benefit to using a blob for caps rather than having these be individual properties? Individual properties seems more natural to me, but I think you had a justification for blobbing them together; that reasoning would be good to include in the commit message.
Matt
include/uapi/drm/drm.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3801584..e3c642f 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -829,6 +829,17 @@ struct drm_event_vblank { __u32 reserved; };
+struct drm_palette_caps {
- /* Structure version. Should be 1 currently */
- __u32 version;
- /* For padding and future use */
- __u32 reserved;
- /* This may be 0 if not supported. e.g. plane palette or VLV pipe */
- __u32 num_samples_before_ctm;
- /* This will be non-zero for pipe. May be zero for planes on some HW */
- __u32 num_samples_after_ctm;
+};
/* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t; -- 1.9.1
On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.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 palette color capabilities.
This patch adds new structure in DRM layer for querying palette color capabilities. This structure will be used by all user space agents to configure appropriate color configurations.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
I think you provided an explanation on a previous code review cycle, but I forget the details now...what's the benefit to using a blob for caps rather than having these be individual properties? Individual properties seems more natural to me, but I think you had a justification for blobbing them together; that reasoning would be good to include in the commit message.
Yeah I'm leaning slightly towards individual props too, that would give us a bit more freedom with placing them (e.g. if someone comes up with funky hw where before_ctm and ctm are per-plane and after_ctm is on the crtc, with only some planes support the before_ctm gamma table).
Also if you do per-prop properties instead of the blob you can drop the version/reserved fields, since properties are inheritedly designed to be extendible. So no need to revision them again (it only leads to more code that might break). -Daniel
Matt
include/uapi/drm/drm.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3801584..e3c642f 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -829,6 +829,17 @@ struct drm_event_vblank { __u32 reserved; };
+struct drm_palette_caps {
- /* Structure version. Should be 1 currently */
- __u32 version;
- /* For padding and future use */
- __u32 reserved;
- /* This may be 0 if not supported. e.g. plane palette or VLV pipe */
- __u32 num_samples_before_ctm;
- /* This will be non-zero for pipe. May be zero for planes on some HW */
- __u32 num_samples_after_ctm;
+};
/* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t; -- 1.9.1
-- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Matt, Daniel Addressing the review comments from both of you here.
Regards Shashank
On 9/22/2015 6:32 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.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 palette color capabilities.
This patch adds new structure in DRM layer for querying palette color capabilities. This structure will be used by all user space agents to configure appropriate color configurations.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
I think you provided an explanation on a previous code review cycle, but I forget the details now...what's the benefit to using a blob for caps rather than having these be individual properties? Individual properties seems more natural to me, but I think you had a justification for blobbing them together; that reasoning would be good to include in the commit message.
Yeah I'm leaning slightly towards individual props too, that would give us a bit more freedom with placing them (e.g. if someone comes up with funky hw where before_ctm and ctm are per-plane and after_ctm is on the crtc, with only some planes support the before_ctm gamma table).
This was the part where we spent most of the time during the design review, and the reason we came up for this was: - This is a read only property, which userspace would like to read only once, and cache the information. It was also Gary's opinion to keep this as single blob for all. - Making individual property needs more information to be provided to user space. - This is a blob only for pipe level capabilities, the plane level blob will be separate from this. - We can handle this HW also, by loading proper plane and pipe level capability blob. This is more convenient to have all the capabilities together at the same place, than keep on querying the same.
Also if you do per-prop properties instead of the blob you can drop the version/reserved fields, since properties are inheritedly designed to be extendible. So no need to revision them again (it only leads to more code that might break). -Daniel
We are anyways planning to drop the version, as per Ville's comment. - Shashank
Matt
include/uapi/drm/drm.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3801584..e3c642f 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -829,6 +829,17 @@ struct drm_event_vblank { __u32 reserved; };
+struct drm_palette_caps {
- /* Structure version. Should be 1 currently */
- __u32 version;
- /* For padding and future use */
- __u32 reserved;
- /* This may be 0 if not supported. e.g. plane palette or VLV pipe */
- __u32 num_samples_before_ctm;
- /* This will be non-zero for pipe. May be zero for planes on some HW */
- __u32 num_samples_after_ctm;
+};
- /* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t;
-- 1.9.1
-- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Given that its only one word of info per LUT, I'm OK with it being two separate properties. I believe it was much more complex previously with a lot more info per LUT, which is probably why I preferred a blob.
Thanks Gary
-----Original Message----- From: Sharma, Shashank Sent: Wednesday, September 23, 2015 9:10 AM To: Daniel Vetter; Roper, Matthew D Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; Smith, Gary K; kausalmalladi@gmail.com; Vetter, Daniel Subject: Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities
Hi Matt, Daniel Addressing the review comments from both of you here.
Regards Shashank
On 9/22/2015 6:32 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.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 palette color capabilities.
This patch adds new structure in DRM layer for querying palette color capabilities. This structure will be used by all user space agents to configure appropriate color configurations.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
I think you provided an explanation on a previous code review cycle, but I forget the details now...what's the benefit to using a blob for caps rather than having these be individual properties? Individual properties seems more natural to me, but I think you had a justification for blobbing them together; that reasoning would be good to include in the commit message.
Yeah I'm leaning slightly towards individual props too, that would give us a bit more freedom with placing them (e.g. if someone comes up with funky hw where before_ctm and ctm are per-plane and after_ctm is on the crtc, with only some planes support the before_ctm gamma table).
This was the part where we spent most of the time during the design review, and the reason we came up for this was: - This is a read only property, which userspace would like to read only once, and cache the information. It was also Gary's opinion to keep this as single blob for all. - Making individual property needs more information to be provided to user space. - This is a blob only for pipe level capabilities, the plane level blob will be separate from this. - We can handle this HW also, by loading proper plane and pipe level capability blob. This is more convenient to have all the capabilities together at the same place, than keep on querying the same.
Also if you do per-prop properties instead of the blob you can drop the version/reserved fields, since properties are inheritedly designed to be extendible. So no need to revision them again (it only leads to more code that might break). -Daniel
We are anyways planning to drop the version, as per Ville's comment. - Shashank
Matt
include/uapi/drm/drm.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3801584..e3c642f 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -829,6 +829,17 @@ struct drm_event_vblank { __u32 reserved; };
+struct drm_palette_caps {
- /* Structure version. Should be 1 currently */
- __u32 version;
- /* For padding and future use */
- __u32 reserved;
- /* This may be 0 if not supported. e.g. plane palette or VLV pipe */
- __u32 num_samples_before_ctm;
- /* This will be non-zero for pipe. May be zero for planes on some HW */
- __u32 num_samples_after_ctm;
+};
- /* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t;
-- 1.9.1
-- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
This would be an interface/design change, to change from one blob of correction property, to split into multiple query properties for palette_before_blob and palette_after_blob. Please let me know if this is really required ?
Regards Shashank -----Original Message----- From: Smith, Gary K Sent: Wednesday, September 23, 2015 3:17 PM To: Sharma, Shashank; Daniel Vetter; Roper, Matthew D Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; kausalmalladi@gmail.com; Vetter, Daniel Subject: RE: [PATCH 02/23] drm: Add structure for querying palette color capabilities
Given that its only one word of info per LUT, I'm OK with it being two separate properties. I believe it was much more complex previously with a lot more info per LUT, which is probably why I preferred a blob.
Thanks Gary
-----Original Message----- From: Sharma, Shashank Sent: Wednesday, September 23, 2015 9:10 AM To: Daniel Vetter; Roper, Matthew D Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; Smith, Gary K; kausalmalladi@gmail.com; Vetter, Daniel Subject: Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities
Hi Matt, Daniel Addressing the review comments from both of you here.
Regards Shashank
On 9/22/2015 6:32 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.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 palette color capabilities.
This patch adds new structure in DRM layer for querying palette color capabilities. This structure will be used by all user space agents to configure appropriate color configurations.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
I think you provided an explanation on a previous code review cycle, but I forget the details now...what's the benefit to using a blob for caps rather than having these be individual properties? Individual properties seems more natural to me, but I think you had a justification for blobbing them together; that reasoning would be good to include in the commit message.
Yeah I'm leaning slightly towards individual props too, that would give us a bit more freedom with placing them (e.g. if someone comes up with funky hw where before_ctm and ctm are per-plane and after_ctm is on the crtc, with only some planes support the before_ctm gamma table).
This was the part where we spent most of the time during the design review, and the reason we came up for this was: - This is a read only property, which userspace would like to read only once, and cache the information. It was also Gary's opinion to keep this as single blob for all. - Making individual property needs more information to be provided to user space. - This is a blob only for pipe level capabilities, the plane level blob will be separate from this. - We can handle this HW also, by loading proper plane and pipe level capability blob. This is more convenient to have all the capabilities together at the same place, than keep on querying the same.
Also if you do per-prop properties instead of the blob you can drop the version/reserved fields, since properties are inheritedly designed to be extendible. So no need to revision them again (it only leads to more code that might break). -Daniel
We are anyways planning to drop the version, as per Ville's comment. - Shashank
Matt
include/uapi/drm/drm.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3801584..e3c642f 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -829,6 +829,17 @@ struct drm_event_vblank { __u32 reserved; };
+struct drm_palette_caps {
- /* Structure version. Should be 1 currently */
- __u32 version;
- /* For padding and future use */
- __u32 reserved;
- /* This may be 0 if not supported. e.g. plane palette or VLV pipe */
- __u32 num_samples_before_ctm;
- /* This will be non-zero for pipe. May be zero for planes on some HW */
- __u32 num_samples_after_ctm;
+};
- /* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t;
-- 1.9.1
-- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 23, 2015 at 11:57:58AM +0000, Sharma, Shashank wrote:
This would be an interface/design change, to change from one blob of correction property, to split into multiple query properties for palette_before_blob and palette_after_blob. Please let me know if this is really required ?
Yes I think splitting this up into one property per value is the right approach. That's also why we split the before/after gamma tables and the ctm each into it's own property.
And I don't think this is a design change of the interface - we still exchange the exact same values with the exact semantics between kernel and userspace, there's just a small difference in the actual transport protocol. Those kinds of minor changes happen all the time when upstreaming features. And that's the reason why we absolutely _must_ have a close collaboration between the kernel and userspace side. Which unfortunately on this feature here took a few months to get going, but hopefully with shared git trees and talking on irc now that should be easier. -Daniel
Regards Shashank -----Original Message----- From: Smith, Gary K Sent: Wednesday, September 23, 2015 3:17 PM To: Sharma, Shashank; Daniel Vetter; Roper, Matthew D Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; kausalmalladi@gmail.com; Vetter, Daniel Subject: RE: [PATCH 02/23] drm: Add structure for querying palette color capabilities
Given that its only one word of info per LUT, I'm OK with it being two separate properties. I believe it was much more complex previously with a lot more info per LUT, which is probably why I preferred a blob.
Thanks Gary
-----Original Message----- From: Sharma, Shashank Sent: Wednesday, September 23, 2015 9:10 AM To: Daniel Vetter; Roper, Matthew D Cc: Matheson, Annie J; Bradford, Robert; Palleti, Avinash Reddy; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Mukherjee, Indranil; Bish, Jim; Smith, Gary K; kausalmalladi@gmail.com; Vetter, Daniel Subject: Re: [PATCH 02/23] drm: Add structure for querying palette color capabilities
Hi Matt, Daniel Addressing the review comments from both of you here.
Regards Shashank
On 9/22/2015 6:32 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 10:51:31AM -0700, Matt Roper wrote:
On Wed, Sep 16, 2015 at 11:06:59PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.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 palette color capabilities.
This patch adds new structure in DRM layer for querying palette color capabilities. This structure will be used by all user space agents to configure appropriate color configurations.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
I think you provided an explanation on a previous code review cycle, but I forget the details now...what's the benefit to using a blob for caps rather than having these be individual properties? Individual properties seems more natural to me, but I think you had a justification for blobbing them together; that reasoning would be good to include in the commit message.
Yeah I'm leaning slightly towards individual props too, that would give us a bit more freedom with placing them (e.g. if someone comes up with funky hw where before_ctm and ctm are per-plane and after_ctm is on the crtc, with only some planes support the before_ctm gamma table).
This was the part where we spent most of the time during the design review, and the reason we came up for this was:
- This is a read only property, which userspace would like to read only once, and cache the information. It was also Gary's opinion to keep this as single blob for all.
- Making individual property needs more information to be provided to user space.
- This is a blob only for pipe level capabilities, the plane level blob will be separate from this.
- We can handle this HW also, by loading proper plane and pipe level capability blob. This is more convenient to have all the capabilities together at the same place, than keep on querying the same.
Also if you do per-prop properties instead of the blob you can drop the version/reserved fields, since properties are inheritedly designed to be extendible. So no need to revision them again (it only leads to more code that might break). -Daniel
We are anyways planning to drop the version, as per Ville's comment.
- Shashank
Matt
include/uapi/drm/drm.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3801584..e3c642f 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -829,6 +829,17 @@ struct drm_event_vblank { __u32 reserved; };
+struct drm_palette_caps {
- /* Structure version. Should be 1 currently */
- __u32 version;
- /* For padding and future use */
- __u32 reserved;
- /* This may be 0 if not supported. e.g. plane palette or VLV pipe */
- __u32 num_samples_before_ctm;
- /* This will be non-zero for pipe. May be zero for planes on some HW */
- __u32 num_samples_after_ctm;
+};
- /* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t;
-- 1.9.1
-- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Kausal Malladi kausalmalladi@gmail.com
This patch adds new variables in CRTC state, to hold respective color correction blobs. These blobs will be required during the atomic commit for writing the color correction values in correction registers.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++++++++ include/drm/drm_crtc.h | 5 +++++ 2 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 94d6c8e..1fe91ee 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2168,6 +2168,12 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
if (state->mode_blob) drm_property_reference_blob(state->mode_blob); + if (state->ctm_blob) + drm_property_reference_blob(state->ctm_blob); + if (state->palette_after_ctm_blob) + drm_property_reference_blob(state->palette_after_ctm_blob); + if (state->palette_before_ctm_blob) + drm_property_reference_blob(state->palette_before_ctm_blob); state->mode_changed = false; state->active_changed = false; state->planes_changed = false; @@ -2213,6 +2219,12 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, { if (state->mode_blob) drm_property_unreference_blob(state->mode_blob); + if (state->ctm_blob) + drm_property_unreference_blob(state->ctm_blob); + if (state->palette_after_ctm_blob) + drm_property_unreference_blob(state->palette_after_ctm_blob); + if (state->palette_before_ctm_blob) + drm_property_unreference_blob(state->palette_before_ctm_blob); } EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c35531e..fcbe682 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -302,6 +302,11 @@ struct drm_crtc_state { /* blob property to expose current mode to atomic userspace */ struct drm_property_blob *mode_blob;
+ /* blob properties to hold the color properties' blobs */ + struct drm_property_blob *palette_before_ctm_blob; + struct drm_property_blob *palette_after_ctm_blob; + struct drm_property_blob *ctm_blob; + struct drm_pending_vblank_event *event;
struct drm_atomic_state *state;
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; + /* + * 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;
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
From: Kausal Malladi kausalmalladi@gmail.com
Color Manager framework defines a color correction property for color space transformation and Gamut mapping. This property is called CTM (Color Transformation Matrix).
This patch adds a new structure in DRM layer for CTM. This structure can be used by all user space agents to configure CTM coefficients for color correction.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- include/uapi/drm/drm.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index f72b916..9580772 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -867,6 +867,18 @@ struct drm_palette { struct drm_r32g32b32 lut[0]; };
+struct drm_ctm { + /* Structure version. Should be 1 currently */ + __u32 version; + /* + * Each value is in S31.32 format. + * This is 3x3 matrix in row major format. + * Integer part will be clipped to nearest + * max/min boundary as supported by the HW platform. + */ + __s64 ctm_coeff[9]; +}; + /* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t;
On Wed, Sep 16, 2015 at 11:07:02PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.com
Color Manager framework defines a color correction property for color space transformation and Gamut mapping. This property is called CTM (Color Transformation Matrix).
This patch adds a new structure in DRM layer for CTM. This structure can be used by all user space agents to configure CTM coefficients for color correction.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
include/uapi/drm/drm.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index f72b916..9580772 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -867,6 +867,18 @@ struct drm_palette { struct drm_r32g32b32 lut[0]; };
+struct drm_ctm {
- /* Structure version. Should be 1 currently */
- __u32 version;
Same thing here, no version needed for properties.
- /*
* Each value is in S31.32 format.* This is 3x3 matrix in row major format.* Integer part will be clipped to nearest* max/min boundary as supported by the HW platform.*/- __s64 ctm_coeff[9];
+};
/* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t; -- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Regards Shashank
On 9/22/2015 6:38 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 11:07:02PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.com
Color Manager framework defines a color correction property for color space transformation and Gamut mapping. This property is called CTM (Color Transformation Matrix).
This patch adds a new structure in DRM layer for CTM. This structure can be used by all user space agents to configure CTM coefficients for color correction.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
include/uapi/drm/drm.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index f72b916..9580772 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -867,6 +867,18 @@ struct drm_palette { struct drm_r32g32b32 lut[0]; };
+struct drm_ctm {
- /* Structure version. Should be 1 currently */
- __u32 version;
Same thing here, no version needed for properties.
Agree. -Shashank
- /*
* Each value is in S31.32 format.* This is 3x3 matrix in row major format.* Integer part will be clipped to nearest* max/min boundary as supported by the HW platform.*/- __s64 ctm_coeff[9];
+};
- /* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t;
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Sep 16, 2015 at 11:07:02PM +0530, Shashank Sharma wrote:
From: Kausal Malladi kausalmalladi@gmail.com
Color Manager framework defines a color correction property for color space transformation and Gamut mapping. This property is called CTM (Color Transformation Matrix).
This patch adds a new structure in DRM layer for CTM. This structure can be used by all user space agents to configure CTM coefficients for color correction.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
include/uapi/drm/drm.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index f72b916..9580772 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -867,6 +867,18 @@ struct drm_palette { struct drm_r32g32b32 lut[0]; };
+struct drm_ctm {
- /* Structure version. Should be 1 currently */
- __u32 version;
- /*
* Each value is in S31.32 format.* This is 3x3 matrix in row major format.
I'm not sure that exlains things. Maybe sketch the math equation here (something like what I put in i915_reg.h for the CHV sprite CSC). Which reminds me: what are we going to do about the pre/post offset stuff? I was sort of expecting to see it here, but maybe you put those into separate properties?
* Integer part will be clipped to nearest* max/min boundary as supported by the HW platform.*/- __s64 ctm_coeff[9];
ctm_ prefix seems superfluous. Also could maybe use some comment explaining what ctm is?
+};
/* 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
From: Kausal Malladi kausalmalladi@gmail.com
This patch adds atomic set property interface for Intel CRTC. This interface will be used for set operation on any DRM properties.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/intel_atomic.c | 9 +++++++++ drivers/gpu/drm/i915/intel_display.c | 2 ++ drivers/gpu/drm/i915/intel_drv.h | 4 ++++ 3 files changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index f1975f2..d5b7eb1 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -309,3 +309,12 @@ void intel_atomic_state_clear(struct drm_atomic_state *s) drm_atomic_state_default_clear(&state->base); state->dpll_set = false; } + +int intel_crtc_atomic_set_property(struct drm_crtc *crtc, + struct drm_crtc_state *state, + struct drm_property *property, + uint64_t val) +{ + DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); + return -EINVAL; +} diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fc00867..444ea30 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13192,6 +13192,8 @@ static const struct drm_crtc_funcs intel_crtc_funcs = { .set_config = drm_atomic_helper_set_config, .destroy = intel_crtc_destroy, .page_flip = intel_crtc_page_flip, + .set_property = drm_atomic_helper_crtc_set_property, + .atomic_set_property = intel_crtc_atomic_set_property, .atomic_duplicate_state = intel_crtc_duplicate_state, .atomic_destroy_state = intel_crtc_destroy_state, }; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 02a755a..510f559 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1450,6 +1450,10 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state, int intel_atomic_setup_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state); +int intel_crtc_atomic_set_property(struct drm_crtc *plane, + struct drm_crtc_state *state, + struct drm_property *property, + uint64_t val);
/* intel_atomic_plane.c */ struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
From: Kausal Malladi kausalmalladi@gmail.com
This patch adds atomic get property interface for Intel CRTC. This interface will be used for get operation on any non-core DRM properties.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/intel_atomic.c | 9 +++++++++ drivers/gpu/drm/i915/intel_display.c | 1 + drivers/gpu/drm/i915/intel_drv.h | 4 ++++ 3 files changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index d5b7eb1..500d2998 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -318,3 +318,12 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc, DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL; } + +int intel_crtc_atomic_get_property(struct drm_crtc *crtc, + const struct drm_crtc_state *state, + struct drm_property *property, + uint64_t *val) +{ + DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); + return -EINVAL; +} diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 444ea30..010acca 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13194,6 +13194,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = { .page_flip = intel_crtc_page_flip, .set_property = drm_atomic_helper_crtc_set_property, .atomic_set_property = intel_crtc_atomic_set_property, + .atomic_get_property = intel_crtc_atomic_get_property, .atomic_duplicate_state = intel_crtc_duplicate_state, .atomic_destroy_state = intel_crtc_destroy_state, }; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 510f559..e27e754 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1454,6 +1454,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc *plane, struct drm_crtc_state *state, struct drm_property *property, uint64_t val); +int intel_crtc_atomic_get_property(struct drm_crtc *plane, + const struct drm_crtc_state *state, + struct drm_property *property, + uint64_t *val);
/* intel_atomic_plane.c */ struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
From: Kausal Malladi kausalmalladi@gmail.com
This patch create new files intel_color_manager.c which will contain the core color correction code for I915 driver and its header intel_color_manager.h
The per color property patches coming up in this patch series will fill the appropriate functions in this file.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/Makefile | 3 ++- drivers/gpu/drm/i915/intel_color_manager.c | 33 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 29 ++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 44d290a..56caf9e 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -64,7 +64,8 @@ i915-y += intel_audio.o \ intel_overlay.o \ intel_psr.o \ intel_sideband.o \ - intel_sprite.o + intel_sprite.o \ + intel_color_manager.o i915-$(CONFIG_ACPI) += intel_acpi.o intel_opregion.o i915-$(CONFIG_DRM_FBDEV_EMULATION) += intel_fbdev.o
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c new file mode 100644 index 0000000..7357d99 --- /dev/null +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -0,0 +1,33 @@ +/* + * Copyright © 2015 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + * Shashank Sharma shashank.sharma@intel.com + * Kausal Malladi Kausal.Malladi@intel.com + */ + +#include "intel_color_manager.h" + +void intel_attach_color_properties_to_crtc(struct drm_device *dev, + struct drm_mode_object *mode_obj) +{ +} diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h new file mode 100644 index 0000000..04c921d --- /dev/null +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -0,0 +1,29 @@ +/* + * Copyright © 2015 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + * Shashank Sharma shashank.sharma@intel.com + * Kausal Malladi Kausal.Malladi@intel.com + */ +#include <drm/drmP.h> +#include <drm/drm_crtc_helper.h> +#include "i915_drv.h"
DRM color manager contains these color properties:
1. "crtc_palette_capabilities_property": to allow a core driver to load and showcase its color correction capabilities to user space. 2. "ctm": Color transformation matrix property, where a color transformation matrix of 9 correction values gets applied as correction. 3. "palette_before_ctm": for corrections which get applied beore color transformation matrix correction. 4. "palette_after_ctm": for corrections which get applied after color transformation matrix correction.
Intel color manager registers: 1. Gamma correction property as "palette_after_ctm" property 2. Degamma correction capability as "palette_bafore_ctm" property capability as "palette_after_ctm" DRM color property hook. 3. CSC as "ctm" property.
This patch does the following: 1. Add a function which loads the platform's color correction capabilities in the cm_crtc_palette_capabilities_property structure. 2. Attaches the cm_crtc_palette_capabilities_property to every CRTC getting initiaized. 3. Adds two new parameters "num_samples_after_ctm" and "num_samples_before_ctm" in intel_device_info as gamma and degamma coefficients vary per platform basis.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_color_manager.c | 45 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 2 ++ 3 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3bf8a9b..22de2cb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -798,6 +798,8 @@ struct intel_device_info { u8 num_sprites[I915_MAX_PIPES]; u8 gen; u8 ring_mask; /* Rings supported by the HW */ + u16 num_samples_after_ctm; + u16 num_samples_before_ctm; DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON); /* Register offsets for the various display pipes and transcoders */ int pipe_offsets[I915_MAX_TRANSCODERS]; diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 7357d99..77f58f2 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,7 +27,52 @@
#include "intel_color_manager.h"
+int get_pipe_capabilities(struct drm_device *dev, + struct drm_palette_caps *palette_caps, struct drm_crtc *crtc) +{ + struct drm_property_blob *blob; + + /* + * This function loads best capability for gamma correction + * For example: + * CHV best Gamma correction (CGM unit, 10 bit) + * has 257 entries, best degamma is 65 entries + */ + palette_caps->version = COLOR_STRUCT_VERSION; + palette_caps->num_samples_after_ctm = + INTEL_INFO(dev)->num_samples_after_ctm; + palette_caps->num_samples_before_ctm = + INTEL_INFO(dev)->num_samples_before_ctm; + blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps), + (const void *) palette_caps); + if (IS_ERR_OR_NULL(blob)) { + DRM_ERROR("Create blob for capabilities failed\n"); + return PTR_ERR(blob); + } + + return blob->base.id; +} + void intel_attach_color_properties_to_crtc(struct drm_device *dev, struct drm_mode_object *mode_obj) { + struct drm_mode_config *config = &dev->mode_config; + struct drm_palette_caps *palette_caps; + struct drm_crtc *crtc; + int capabilities_blob_id; + + crtc = obj_to_crtc(mode_obj); + palette_caps = kzalloc(sizeof(struct drm_palette_caps), + GFP_KERNEL); + capabilities_blob_id = get_pipe_capabilities(dev, + palette_caps, crtc); + + if (config->cm_crtc_palette_capabilities_property) { + drm_object_attach_property(mode_obj, + config->cm_crtc_palette_capabilities_property, + capabilities_blob_id); + DRM_DEBUG_DRIVER("Capabilities attached to CRTC\n"); + } + + kfree(palette_caps); } diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 04c921d..1a42244 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -27,3 +27,5 @@ #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include "i915_drv.h" + +#define COLOR_STRUCT_VERSION 1
On Wed, Sep 16, 2015 at 11:07:06PM +0530, Shashank Sharma wrote:
DRM color manager contains these color properties:
- "crtc_palette_capabilities_property": to allow a
core driver to load and showcase its color correction capabilities to user space. 2. "ctm": Color transformation matrix property, where a color transformation matrix of 9 correction values gets applied as correction. 3. "palette_before_ctm": for corrections which get applied beore color transformation matrix correction. 4. "palette_after_ctm": for corrections which get applied after color transformation matrix correction.
Intel color manager registers:
- Gamma correction property as "palette_after_ctm" property
- Degamma correction capability as "palette_bafore_ctm" property
capability as "palette_after_ctm" DRM color property hook. 3. CSC as "ctm" property.
This patch does the following:
- Add a function which loads the platform's color correction
capabilities in the cm_crtc_palette_capabilities_property structure. 2. Attaches the cm_crtc_palette_capabilities_property to every CRTC getting initiaized. 3. Adds two new parameters "num_samples_after_ctm" and "num_samples_before_ctm" in intel_device_info as gamma and degamma coefficients vary per platform basis.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com
drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_color_manager.c | 45 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 2 ++ 3 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3bf8a9b..22de2cb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -798,6 +798,8 @@ struct intel_device_info { u8 num_sprites[I915_MAX_PIPES]; u8 gen; u8 ring_mask; /* Rings supported by the HW */
- u16 num_samples_after_ctm;
- u16 num_samples_before_ctm; DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON); /* Register offsets for the various display pipes and transcoders */ int pipe_offsets[I915_MAX_TRANSCODERS];
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 7357d99..77f58f2 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,7 +27,52 @@
#include "intel_color_manager.h"
+int get_pipe_capabilities(struct drm_device *dev,
struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)+{
- struct drm_property_blob *blob;
- /*
* This function loads best capability for gamma correction* For example:* CHV best Gamma correction (CGM unit, 10 bit)* has 257 entries, best degamma is 65 entries*/- palette_caps->version = COLOR_STRUCT_VERSION;
- palette_caps->num_samples_after_ctm =
INTEL_INFO(dev)->num_samples_after_ctm;- palette_caps->num_samples_before_ctm =
INTEL_INFO(dev)->num_samples_before_ctm;- blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
(const void *) palette_caps);- if (IS_ERR_OR_NULL(blob)) {
DRM_ERROR("Create blob for capabilities failed\n");return PTR_ERR(blob);- }
- return blob->base.id;
+}
void intel_attach_color_properties_to_crtc(struct drm_device *dev, struct drm_mode_object *mode_obj) {
- struct drm_mode_config *config = &dev->mode_config;
- struct drm_palette_caps *palette_caps;
- struct drm_crtc *crtc;
- int capabilities_blob_id;
- crtc = obj_to_crtc(mode_obj);
- palette_caps = kzalloc(sizeof(struct drm_palette_caps),
GFP_KERNEL);- capabilities_blob_id = get_pipe_capabilities(dev,
palette_caps, crtc);- if (config->cm_crtc_palette_capabilities_property) {
drm_object_attach_property(mode_obj,config->cm_crtc_palette_capabilities_property,capabilities_blob_id);
I didn't find any code to attach the before/after_ctm gamma properties and the ctm property. Also I think a small helper would be really nice here in the drm core:
drm_crtc_attach_cc_properties(struct drm_crtc *crtc, int gamma_before_ctm, bool ctm, int gamm_after_ctm)
And then that directly constructs the palette caps from the given values (if they're non-0) and also attaches the properties to the crtc (if they're non-0 for gamma, treu for the ctm).
If that attach_property code really isn't there I wonder a bit how this all works, we /should/ be filtering out properties which aren't attached to the object. -Daniel
DRM_DEBUG_DRIVER("Capabilities attached to CRTC\n");- }
- kfree(palette_caps);
} diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 04c921d..1a42244 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -27,3 +27,5 @@ #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include "i915_drv.h"
+#define COLOR_STRUCT_VERSION 1
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Regards Shashank
On 9/22/2015 6:54 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 11:07:06PM +0530, Shashank Sharma wrote:
DRM color manager contains these color properties:
- "crtc_palette_capabilities_property": to allow a
core driver to load and showcase its color correction capabilities to user space. 2. "ctm": Color transformation matrix property, where a color transformation matrix of 9 correction values gets applied as correction. 3. "palette_before_ctm": for corrections which get applied beore color transformation matrix correction. 4. "palette_after_ctm": for corrections which get applied after color transformation matrix correction.
Intel color manager registers:
- Gamma correction property as "palette_after_ctm" property
- Degamma correction capability as "palette_bafore_ctm" property
capability as "palette_after_ctm" DRM color property hook. 3. CSC as "ctm" property.
This patch does the following:
- Add a function which loads the platform's color correction
capabilities in the cm_crtc_palette_capabilities_property structure. 2. Attaches the cm_crtc_palette_capabilities_property to every CRTC getting initiaized. 3. Adds two new parameters "num_samples_after_ctm" and "num_samples_before_ctm" in intel_device_info as gamma and degamma coefficients vary per platform basis.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com
drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_color_manager.c | 45 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 2 ++ 3 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3bf8a9b..22de2cb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -798,6 +798,8 @@ struct intel_device_info { u8 num_sprites[I915_MAX_PIPES]; u8 gen; u8 ring_mask; /* Rings supported by the HW */
- u16 num_samples_after_ctm;
- u16 num_samples_before_ctm; DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON); /* Register offsets for the various display pipes and transcoders */ int pipe_offsets[I915_MAX_TRANSCODERS];
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 7357d99..77f58f2 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,7 +27,52 @@
#include "intel_color_manager.h"
+int get_pipe_capabilities(struct drm_device *dev,
struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)+{
- struct drm_property_blob *blob;
- /*
* This function loads best capability for gamma correction* For example:* CHV best Gamma correction (CGM unit, 10 bit)* has 257 entries, best degamma is 65 entries*/- palette_caps->version = COLOR_STRUCT_VERSION;
- palette_caps->num_samples_after_ctm =
INTEL_INFO(dev)->num_samples_after_ctm;- palette_caps->num_samples_before_ctm =
INTEL_INFO(dev)->num_samples_before_ctm;- blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
(const void *) palette_caps);- if (IS_ERR_OR_NULL(blob)) {
DRM_ERROR("Create blob for capabilities failed\n");return PTR_ERR(blob);- }
- return blob->base.id;
+}
- void intel_attach_color_properties_to_crtc(struct drm_device *dev, struct drm_mode_object *mode_obj) {
- struct drm_mode_config *config = &dev->mode_config;
- struct drm_palette_caps *palette_caps;
- struct drm_crtc *crtc;
- int capabilities_blob_id;
- crtc = obj_to_crtc(mode_obj);
- palette_caps = kzalloc(sizeof(struct drm_palette_caps),
GFP_KERNEL);- capabilities_blob_id = get_pipe_capabilities(dev,
palette_caps, crtc);- if (config->cm_crtc_palette_capabilities_property) {
drm_object_attach_property(mode_obj,config->cm_crtc_palette_capabilities_property,capabilities_blob_id);I didn't find any code to attach the before/after_ctm gamma properties and the ctm property. Also I think a small helper would be really nice here in the drm core:
Patch 15,16,17 adds those for gamma, degamma and CSC. This is just for the capabilities property.
drm_crtc_attach_cc_properties(struct drm_crtc *crtc, int gamma_before_ctm, bool ctm, int gamm_after_ctm)
And then that directly constructs the palette caps from the given values (if they're non-0) and also attaches the properties to the crtc (if they're non-0 for gamma, treu for the ctm).
If that attach_property code really isn't there I wonder a bit how this all works, we /should/ be filtering out properties which aren't attached to the object.
As I mentioned in the cover-letter, here is the sequence: First 5 patches add color management support in DRM layer Next 7 patches add Intel color management framework in I915 driver (This is still the framework part) Next 5 patches add color correction for CHV (gamma, degamma and CSC) (Here comes the core property and enabling part) Next 2 patches enable color management, by attaching the properties to CRTC(Matt's comment on enable property code only when the job is done) Next 4 patches add color correction for BDW (gamma, degamma)
-Daniel
DRM_DEBUG_DRIVER("Capabilities attached to CRTC\n");- }
- kfree(palette_caps); }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 04c921d..1a42244 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -27,3 +27,5 @@ #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include "i915_drv.h"
+#define COLOR_STRUCT_VERSION 1
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Sep 23, 2015 at 02:05:25PM +0530, Sharma, Shashank wrote:
Regards Shashank
On 9/22/2015 6:54 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 11:07:06PM +0530, Shashank Sharma wrote:
DRM color manager contains these color properties:
- "crtc_palette_capabilities_property": to allow a
core driver to load and showcase its color correction capabilities to user space. 2. "ctm": Color transformation matrix property, where a color transformation matrix of 9 correction values gets applied as correction. 3. "palette_before_ctm": for corrections which get applied beore color transformation matrix correction. 4. "palette_after_ctm": for corrections which get applied after color transformation matrix correction.
Intel color manager registers:
- Gamma correction property as "palette_after_ctm" property
- Degamma correction capability as "palette_bafore_ctm" property
capability as "palette_after_ctm" DRM color property hook. 3. CSC as "ctm" property.
This patch does the following:
- Add a function which loads the platform's color correction
capabilities in the cm_crtc_palette_capabilities_property structure. 2. Attaches the cm_crtc_palette_capabilities_property to every CRTC getting initiaized. 3. Adds two new parameters "num_samples_after_ctm" and "num_samples_before_ctm" in intel_device_info as gamma and degamma coefficients vary per platform basis.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com
drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_color_manager.c | 45 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 2 ++ 3 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3bf8a9b..22de2cb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -798,6 +798,8 @@ struct intel_device_info { u8 num_sprites[I915_MAX_PIPES]; u8 gen; u8 ring_mask; /* Rings supported by the HW */
- u16 num_samples_after_ctm;
- u16 num_samples_before_ctm; DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON); /* Register offsets for the various display pipes and transcoders */ int pipe_offsets[I915_MAX_TRANSCODERS];
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 7357d99..77f58f2 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,7 +27,52 @@
#include "intel_color_manager.h"
+int get_pipe_capabilities(struct drm_device *dev,
struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)+{
- struct drm_property_blob *blob;
- /*
* This function loads best capability for gamma correction* For example:* CHV best Gamma correction (CGM unit, 10 bit)* has 257 entries, best degamma is 65 entries*/- palette_caps->version = COLOR_STRUCT_VERSION;
- palette_caps->num_samples_after_ctm =
INTEL_INFO(dev)->num_samples_after_ctm;- palette_caps->num_samples_before_ctm =
INTEL_INFO(dev)->num_samples_before_ctm;- blob = drm_property_create_blob(dev, sizeof(struct drm_palette_caps),
(const void *) palette_caps);- if (IS_ERR_OR_NULL(blob)) {
DRM_ERROR("Create blob for capabilities failed\n");return PTR_ERR(blob);- }
- return blob->base.id;
+}
void intel_attach_color_properties_to_crtc(struct drm_device *dev, struct drm_mode_object *mode_obj) {
- struct drm_mode_config *config = &dev->mode_config;
- struct drm_palette_caps *palette_caps;
- struct drm_crtc *crtc;
- int capabilities_blob_id;
- crtc = obj_to_crtc(mode_obj);
- palette_caps = kzalloc(sizeof(struct drm_palette_caps),
GFP_KERNEL);- capabilities_blob_id = get_pipe_capabilities(dev,
palette_caps, crtc);- if (config->cm_crtc_palette_capabilities_property) {
drm_object_attach_property(mode_obj,config->cm_crtc_palette_capabilities_property,capabilities_blob_id);I didn't find any code to attach the before/after_ctm gamma properties and the ctm property. Also I think a small helper would be really nice here in the drm core:
Patch 15,16,17 adds those for gamma, degamma and CSC. This is just for the capabilities property.
Indeed, my mail search failed me.
drm_crtc_attach_cc_properties(struct drm_crtc *crtc, int gamma_before_ctm, bool ctm, int gamm_after_ctm)
And then that directly constructs the palette caps from the given values (if they're non-0) and also attaches the properties to the crtc (if they're non-0 for gamma, treu for the ctm).
If that attach_property code really isn't there I wonder a bit how this all works, we /should/ be filtering out properties which aren't attached to the object.
As I mentioned in the cover-letter, here is the sequence: First 5 patches add color management support in DRM layer Next 7 patches add Intel color management framework in I915 driver (This is still the framework part) Next 5 patches add color correction for CHV (gamma, degamma and CSC) (Here comes the core property and enabling part) Next 2 patches enable color management, by attaching the properties to CRTC(Matt's comment on enable property code only when the job is done) Next 4 patches add color correction for BDW (gamma, degamma)
Yeah I just missed this, no idea why the search in my mail client didn't turn up patches 15-17. I still think drm_crtc_attach_cc_properties in the drm core would be good, instead of splitting part of the color manager core support around the properties between the driver and core like you do here. -Daniel
I915 driver registers gamma correction as palette correction property with DRM layer. This patch adds set_property() and get_property() handlers for pipe level gamma correction.
The set function attaches the Gamma correction blob to CRTC state, these values will be committed during atomic commit.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/intel_atomic.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 5 +++++ 3 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 500d2998..0b61fef 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t val) { + struct drm_device *dev = crtc->dev; + struct drm_mode_config *config = &dev->mode_config; + + if (property == config->cm_palette_after_ctm_property) + return intel_color_manager_set_pipe_gamma(dev, state, + &crtc->base, val); + DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL; } @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t *val) { + struct drm_device *dev = crtc->dev; + struct drm_mode_config *config = &dev->mode_config; + + if (property == config->cm_palette_after_ctm_property) { + *val = (state->palette_after_ctm_blob) ? + state->palette_after_ctm_blob->base.id : 0; + goto found; + } + DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL; + +found: + DRM_DEBUG_KMS("Found property %s\n", property->name); + return 0; } diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 77f58f2..9421bb6 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,6 +27,27 @@
#include "intel_color_manager.h"
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev, + struct drm_crtc_state *crtc_state, + struct drm_mode_object *obj, uint32_t blob_id) +{ + struct drm_property_blob *blob; + + blob = drm_property_lookup_blob(dev, blob_id); + if (!blob) { + DRM_DEBUG_KMS("Invalid Blob ID\n"); + return -EINVAL; + } + + if (crtc_state->palette_after_ctm_blob) + drm_property_unreference_blob( + crtc_state->palette_after_ctm_blob); + + /* Attach the blob to be committed in state */ + crtc_state->palette_after_ctm_blob = blob; + return 0; +} + int get_pipe_capabilities(struct drm_device *dev, struct drm_palette_caps *palette_caps, struct drm_crtc *crtc) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e27e754..d0193e2 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+/* intel_color_manager.c */ +int intel_color_manager_set_pipe_gamma(struct drm_device *dev, + struct drm_crtc_state *crtc_state, + struct drm_mode_object *obj, uint32_t blob_id); + #endif /* __INTEL_DRV_H__ */
On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
I915 driver registers gamma correction as palette correction property with DRM layer. This patch adds set_property() and get_property() handlers for pipe level gamma correction.
The set function attaches the Gamma correction blob to CRTC state, these values will be committed during atomic commit.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
drivers/gpu/drm/i915/intel_atomic.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 5 +++++ 3 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 500d2998..0b61fef 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t val) {
- struct drm_device *dev = crtc->dev;
- struct drm_mode_config *config = &dev->mode_config;
- if (property == config->cm_palette_after_ctm_property)
return intel_color_manager_set_pipe_gamma(dev, state,&crtc->base, val);- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL;
} @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t *val) {
- struct drm_device *dev = crtc->dev;
- struct drm_mode_config *config = &dev->mode_config;
- if (property == config->cm_palette_after_ctm_property) {
*val = (state->palette_after_ctm_blob) ?state->palette_after_ctm_blob->base.id : 0;goto found;- }
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL;
+found:
- DRM_DEBUG_KMS("Found property %s\n", property->name);
- return 0;
} diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 77f58f2..9421bb6 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,6 +27,27 @@
#include "intel_color_manager.h"
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
struct drm_crtc_state *crtc_state,struct drm_mode_object *obj, uint32_t blob_id)+{
- struct drm_property_blob *blob;
- blob = drm_property_lookup_blob(dev, blob_id);
- if (!blob) {
DRM_DEBUG_KMS("Invalid Blob ID\n");return -EINVAL;- }
- if (crtc_state->palette_after_ctm_blob)
drm_property_unreference_blob(crtc_state->palette_after_ctm_blob);- /* Attach the blob to be committed in state */
- crtc_state->palette_after_ctm_blob = blob;
- return 0;
+}
What is this used for? It looks a bit like legacy property code, and we have a generic helper to make that happen (drm_atomic_helper_crtc_set_property).
Generally please don't add functions/structs without also adding a user, it means that reviewers have to constantly jump around in your patch series to figure out how something is used. Instead if you want to split things up really fine add a stub function frist (but including relevant callers) and then fill out the bits separately. -Daniel
int get_pipe_capabilities(struct drm_device *dev, struct drm_palette_caps *palette_caps, struct drm_crtc *crtc) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e27e754..d0193e2 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+/* intel_color_manager.c */ +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
struct drm_crtc_state *crtc_state,struct drm_mode_object *obj, uint32_t blob_id);#endif /* __INTEL_DRV_H__ */
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Sep 22, 2015 at 03:15:11PM +0200, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
I915 driver registers gamma correction as palette correction property with DRM layer. This patch adds set_property() and get_property() handlers for pipe level gamma correction.
The set function attaches the Gamma correction blob to CRTC state, these values will be committed during atomic commit.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
drivers/gpu/drm/i915/intel_atomic.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 5 +++++ 3 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 500d2998..0b61fef 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t val) {
- struct drm_device *dev = crtc->dev;
- struct drm_mode_config *config = &dev->mode_config;
- if (property == config->cm_palette_after_ctm_property)
return intel_color_manager_set_pipe_gamma(dev, state,&crtc->base, val);- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL;
} @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t *val) {
- struct drm_device *dev = crtc->dev;
- struct drm_mode_config *config = &dev->mode_config;
- if (property == config->cm_palette_after_ctm_property) {
*val = (state->palette_after_ctm_blob) ?state->palette_after_ctm_blob->base.id : 0;goto found;- }
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
I've forgotten to explain how drivers can then opt-in KMS extensions if we have the decode support unconditionally there and also register the props unconditionally: That's done by only attaching these standardized props to a crtc/plane if the corresponding object supports it. -Daniel
- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL;
+found:
- DRM_DEBUG_KMS("Found property %s\n", property->name);
- return 0;
} diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 77f58f2..9421bb6 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,6 +27,27 @@
#include "intel_color_manager.h"
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
struct drm_crtc_state *crtc_state,struct drm_mode_object *obj, uint32_t blob_id)+{
- struct drm_property_blob *blob;
- blob = drm_property_lookup_blob(dev, blob_id);
- if (!blob) {
DRM_DEBUG_KMS("Invalid Blob ID\n");return -EINVAL;- }
- if (crtc_state->palette_after_ctm_blob)
drm_property_unreference_blob(crtc_state->palette_after_ctm_blob);- /* Attach the blob to be committed in state */
- crtc_state->palette_after_ctm_blob = blob;
- return 0;
+}
What is this used for? It looks a bit like legacy property code, and we have a generic helper to make that happen (drm_atomic_helper_crtc_set_property).
Generally please don't add functions/structs without also adding a user, it means that reviewers have to constantly jump around in your patch series to figure out how something is used. Instead if you want to split things up really fine add a stub function frist (but including relevant callers) and then fill out the bits separately. -Daniel
int get_pipe_capabilities(struct drm_device *dev, struct drm_palette_caps *palette_caps, struct drm_crtc *crtc) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e27e754..d0193e2 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+/* intel_color_manager.c */ +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
struct drm_crtc_state *crtc_state,struct drm_mode_object *obj, uint32_t blob_id);#endif /* __INTEL_DRV_H__ */
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Regards Shashank
On 9/22/2015 6:45 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
I915 driver registers gamma correction as palette correction property with DRM layer. This patch adds set_property() and get_property() handlers for pipe level gamma correction.
The set function attaches the Gamma correction blob to CRTC state, these values will be committed during atomic commit.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
drivers/gpu/drm/i915/intel_atomic.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 5 +++++ 3 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 500d2998..0b61fef 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t val) {
- struct drm_device *dev = crtc->dev;
- struct drm_mode_config *config = &dev->mode_config;
- if (property == config->cm_palette_after_ctm_property)
return intel_color_manager_set_pipe_gamma(dev, state,&crtc->base, val);- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL; }
@@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t *val) {
- struct drm_device *dev = crtc->dev;
- struct drm_mode_config *config = &dev->mode_config;
- if (property == config->cm_palette_after_ctm_property) {
*val = (state->palette_after_ctm_blob) ?state->palette_after_ctm_blob->base.id : 0;goto found;- }
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL;
+found:
- DRM_DEBUG_KMS("Found property %s\n", property->name);
- return 0; }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 77f58f2..9421bb6 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,6 +27,27 @@
#include "intel_color_manager.h"
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
struct drm_crtc_state *crtc_state,struct drm_mode_object *obj, uint32_t blob_id)+{
- struct drm_property_blob *blob;
- blob = drm_property_lookup_blob(dev, blob_id);
- if (!blob) {
DRM_DEBUG_KMS("Invalid Blob ID\n");return -EINVAL;- }
- if (crtc_state->palette_after_ctm_blob)
drm_property_unreference_blob(crtc_state->palette_after_ctm_blob);- /* Attach the blob to be committed in state */
- crtc_state->palette_after_ctm_blob = blob;
- return 0;
+}
What is this used for? It looks a bit like legacy property code, and we have a generic helper to make that happen (drm_atomic_helper_crtc_set_property).
No Daniel, its not the legacy part. Please check atomic_begin() part in the next patches. This is like the top half of atomic set_property for CRTC and will be applicable for any property. As you already know, to set a property: - userspace creates a blob, and sends the blob id via set_property() interface - set_property() saves the blob_id in the corresponding pointer space in the crtc_state() - From the atomic_commit_begin() we will extract the values from blob and start committing to the registers. Its kind of bottom_half() of atomic set_property for CRTC.
Generally please don't add functions/structs without also adding a user, it means that reviewers have to constantly jump around in your patch series to figure out how something is used. Instead if you want to split things up really fine add a stub function frist (but including relevant callers) and then fill out the bits separately.
I think the above explanation should make it clear.
-Daniel
- int get_pipe_capabilities(struct drm_device *dev, struct drm_palette_caps *palette_caps, struct drm_crtc *crtc) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e27e754..d0193e2 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+/* intel_color_manager.c */ +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
struct drm_crtc_state *crtc_state,struct drm_mode_object *obj, uint32_t blob_id);- #endif /* __INTEL_DRV_H__ */
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Sep 23, 2015 at 01:52:21PM +0530, Sharma, Shashank wrote:
Regards Shashank
On 9/22/2015 6:45 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
I915 driver registers gamma correction as palette correction property with DRM layer. This patch adds set_property() and get_property() handlers for pipe level gamma correction.
The set function attaches the Gamma correction blob to CRTC state, these values will be committed during atomic commit.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
drivers/gpu/drm/i915/intel_atomic.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 5 +++++ 3 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 500d2998..0b61fef 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t val) {
- struct drm_device *dev = crtc->dev;
- struct drm_mode_config *config = &dev->mode_config;
- if (property == config->cm_palette_after_ctm_property)
return intel_color_manager_set_pipe_gamma(dev, state,&crtc->base, val);- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL;
} @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t *val) {
- struct drm_device *dev = crtc->dev;
- struct drm_mode_config *config = &dev->mode_config;
- if (property == config->cm_palette_after_ctm_property) {
*val = (state->palette_after_ctm_blob) ?state->palette_after_ctm_blob->base.id : 0;goto found;- }
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
When moving this please don't forget to add the blob->length sanity checks mentioned in another thread in this discussion.
- DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL;
+found:
- DRM_DEBUG_KMS("Found property %s\n", property->name);
- return 0;
} diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 77f58f2..9421bb6 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,6 +27,27 @@
#include "intel_color_manager.h"
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
struct drm_crtc_state *crtc_state,struct drm_mode_object *obj, uint32_t blob_id)+{
- struct drm_property_blob *blob;
- blob = drm_property_lookup_blob(dev, blob_id);
- if (!blob) {
DRM_DEBUG_KMS("Invalid Blob ID\n");return -EINVAL;- }
- if (crtc_state->palette_after_ctm_blob)
drm_property_unreference_blob(crtc_state->palette_after_ctm_blob);- /* Attach the blob to be committed in state */
- crtc_state->palette_after_ctm_blob = blob;
- return 0;
+}
What is this used for? It looks a bit like legacy property code, and we have a generic helper to make that happen (drm_atomic_helper_crtc_set_property).
No Daniel, its not the legacy part. Please check atomic_begin() part in the next patches. This is like the top half of atomic set_property for CRTC and will be applicable for any property. As you already know, to set a property:
- userspace creates a blob, and sends the blob id via set_property()
interface
- set_property() saves the blob_id in the corresponding pointer space in the
crtc_state()
- From the atomic_commit_begin() we will extract the values from blob and
start committing to the registers. Its kind of bottom_half() of atomic set_property for CRTC.
Generally please don't add functions/structs without also adding a user, it means that reviewers have to constantly jump around in your patch series to figure out how something is used. Instead if you want to split things up really fine add a stub function frist (but including relevant callers) and then fill out the bits separately.
I think the above explanation should make it clear.
In my defence I'd like to say that I was still jetlagged ;-) Reading things now again it all looks good. -Daniel
On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
Actually, while implementing this, I realized that this change is not required. What we want to do in drm_atomic_crtc_get/set code is: if (prop == config->cm_palette_after_ctm_property || prop == config->cm_palette_before_ctm_property) { crtc->funcs->atomic_get_property(); }
Which is already being done in the current code: else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val);
so I dont really think we need this change.
Regards Shashank
On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
Actually, while implementing this, I realized that this change is not required. What we want to do in drm_atomic_crtc_get/set code is: if (prop == config->cm_palette_after_ctm_property || prop == config->cm_palette_before_ctm_property) { crtc->funcs->atomic_get_property(); }
Which is already being done in the current code: else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val);
This code is to pass any property unknown to the drm core into the driver. But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property. -Daniel
Matt, your opinion about this ?
Regards Shashank -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, September 28, 2015 12:14 PM To: Sharma, Shashank Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
Actually, while implementing this, I realized that this change is not required. What we want to do in drm_atomic_crtc_get/set code is: if (prop == config->cm_palette_after_ctm_property || prop == config->cm_palette_before_ctm_property) { crtc->funcs->atomic_get_property(); }
Which is already being done in the current code: else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val);
This code is to pass any property unknown to the drm core into the driver. But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Yep, Daniel's right; for properties that are not specific to the driver, the core core should take care of stuffing the values provided by userspace into the state structures so that every driver that wants to use these doesn't need to replicate that logic. My bad for not catching this in my earlier reviews; sorry about that. Basically the change required is to add another else clause to drm_atomic_crtc_{get,set}_property(), before the core tries to call into the driver-specific handler. You'll notice that what you're doing with color management blobs here is actually very similar to what's already done for the existing mode blob, so you can take a look at that case for an example.
Also, the functions like intel_color_manager_set_pipe_gamma() will get moved into the DRM core and the "intel_" prefix will switch to "drm_" since there's really nothing Intel-specific about what they're doing.
Sorry again for not noticing this and bringing it up on the earlier reviews. Fortunately the changes required should be pretty small.
Matt
On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote:
Matt, your opinion about this ?
Regards Shashank -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, September 28, 2015 12:14 PM To: Sharma, Shashank Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
Actually, while implementing this, I realized that this change is not required. What we want to do in drm_atomic_crtc_get/set code is: if (prop == config->cm_palette_after_ctm_property || prop == config->cm_palette_before_ctm_property) { crtc->funcs->atomic_get_property(); }
Which is already being done in the current code: else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val);
This code is to pass any property unknown to the drm core into the driver. But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Ok, anyways it was sounding like a good idea. Will do the changes accordingly.
Regards Shashank -----Original Message----- From: Roper, Matthew D Sent: Tuesday, September 29, 2015 3:13 AM To: Sharma, Shashank Cc: Daniel Vetter; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
Yep, Daniel's right; for properties that are not specific to the driver, the core core should take care of stuffing the values provided by userspace into the state structures so that every driver that wants to use these doesn't need to replicate that logic. My bad for not catching this in my earlier reviews; sorry about that. Basically the change required is to add another else clause to drm_atomic_crtc_{get,set}_property(), before the core tries to call into the driver-specific handler. You'll notice that what you're doing with color management blobs here is actually very similar to what's already done for the existing mode blob, so you can take a look at that case for an example.
Also, the functions like intel_color_manager_set_pipe_gamma() will get moved into the DRM core and the "intel_" prefix will switch to "drm_" since there's really nothing Intel-specific about what they're doing.
Sorry again for not noticing this and bringing it up on the earlier reviews. Fortunately the changes required should be pretty small.
Matt
On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote:
Matt, your opinion about this ?
Regards Shashank -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, September 28, 2015 12:14 PM To: Sharma, Shashank Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
Actually, while implementing this, I realized that this change is not required. What we want to do in drm_atomic_crtc_get/set code is: if (prop == config->cm_palette_after_ctm_property || prop == config->cm_palette_before_ctm_property) { crtc->funcs->atomic_get_property(); }
Which is already being done in the current code: else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val);
This code is to pass any property unknown to the drm core into the driver. But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795
Thank you Shashank!
Annie Matheson
On Sep 28, 2015, at 9:29 PM, Sharma, Shashank shashank.sharma@intel.com wrote:
Ok, anyways it was sounding like a good idea. Will do the changes accordingly.
Regards Shashank -----Original Message----- From: Roper, Matthew D Sent: Tuesday, September 29, 2015 3:13 AM To: Sharma, Shashank Cc: Daniel Vetter; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
Yep, Daniel's right; for properties that are not specific to the driver, the core core should take care of stuffing the values provided by userspace into the state structures so that every driver that wants to use these doesn't need to replicate that logic. My bad for not catching this in my earlier reviews; sorry about that. Basically the change required is to add another else clause to drm_atomic_crtc_{get,set}_property(), before the core tries to call into the driver-specific handler. You'll notice that what you're doing with color management blobs here is actually very similar to what's already done for the existing mode blob, so you can take a look at that case for an example.
Also, the functions like intel_color_manager_set_pipe_gamma() will get moved into the DRM core and the "intel_" prefix will switch to "drm_" since there's really nothing Intel-specific about what they're doing.
Sorry again for not noticing this and bringing it up on the earlier reviews. Fortunately the changes required should be pretty small.
Matt
On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote: Matt, your opinion about this ?
Regards Shashank -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, September 28, 2015 12:14 PM To: Sharma, Shashank Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Matheson, Annie J; kausalmalladi@gmail.com; Vetter, Daniel Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote: On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
Since color manager properties are meant as a new standardize KMS extension (we put them into the core drm_crtc_state) the get/set support should also be in the core. See e.g. how the rotation property is handled in drm_atomic_plane_get/set_property. So all this code should be added to drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
Actually, while implementing this, I realized that this change is not required. What we want to do in drm_atomic_crtc_get/set code is: if (prop == config->cm_palette_after_ctm_property || prop == config->cm_palette_before_ctm_property) { crtc->funcs->atomic_get_property(); }
Which is already being done in the current code: else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val);
This code is to pass any property unknown to the drm core into the driver. But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795
This patch adds set_property and get_property handlers for deGamma color correction capability at Pipe level.
Set function just attaches the deGamma correction blob to CRTC state, which will be later committed in the atomic commit path.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/intel_atomic.c | 10 ++++++++++ drivers/gpu/drm/i915/intel_color_manager.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 4 +++- 3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 0b61fef..b504b4f 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -322,6 +322,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc, return intel_color_manager_set_pipe_gamma(dev, state, &crtc->base, val);
+ if (property == config->cm_palette_before_ctm_property) + return intel_color_manager_set_pipe_degamma(dev, state, + &crtc->base, val); + DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL; } @@ -340,6 +344,12 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc, goto found; }
+ if (property == config->cm_palette_before_ctm_property) { + *val = (state->palette_before_ctm_blob) ? + state->palette_before_ctm_blob->base.id : 0; + goto found; + } + DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL;
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 9421bb6..8890b09 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,6 +27,26 @@
#include "intel_color_manager.h"
+int intel_color_manager_set_pipe_degamma(struct drm_device *dev, + struct drm_crtc_state *crtc_state, + struct drm_mode_object *obj, uint32_t blob_id) +{ + struct drm_property_blob *blob; + + blob = drm_property_lookup_blob(dev, blob_id); + if (!blob) { + DRM_ERROR("Invalid Blob ID\n"); + return -EINVAL; + } + + if (crtc_state->palette_before_ctm_blob) + drm_property_unreference_blob( + crtc_state->palette_before_ctm_blob); + + crtc_state->palette_before_ctm_blob = blob; + return 0; +} + int intel_color_manager_set_pipe_gamma(struct drm_device *dev, struct drm_crtc_state *crtc_state, struct drm_mode_object *obj, uint32_t blob_id) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d0193e2..0298dd0 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1470,5 +1470,7 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; int intel_color_manager_set_pipe_gamma(struct drm_device *dev, struct drm_crtc_state *crtc_state, struct drm_mode_object *obj, uint32_t blob_id); - +int intel_color_manager_set_pipe_degamma(struct drm_device *dev, + struct drm_crtc_state *crtc_state, + struct drm_mode_object *obj, uint32_t blob_id); #endif /* __INTEL_DRV_H__ */
This patch adds set_property and get_property handlers for pipe level CSC color correction for CHV/BSW platform. The set function just attaches the CSC blob to CRTC state, that later gets committed using atomic path.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/intel_atomic.c | 8 ++++++++ drivers/gpu/drm/i915/intel_color_manager.c | 19 +++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 3 +++ 3 files changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index b504b4f..57eafca 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -325,6 +325,9 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc, if (property == config->cm_palette_before_ctm_property) return intel_color_manager_set_pipe_degamma(dev, state, &crtc->base, val); + if (property == config->cm_ctm_property) + return intel_color_manager_set_pipe_csc(dev, state, + &crtc->base, val);
DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL; @@ -350,6 +353,11 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc, goto found; }
+ if (property == config->cm_ctm_property) { + *val = (state->ctm_blob) ? state->ctm_blob->base.id : 0; + goto found; + } + DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); return -EINVAL;
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 8890b09..2ba3fd7 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -27,6 +27,25 @@
#include "intel_color_manager.h"
+int intel_color_manager_set_pipe_csc(struct drm_device *dev, + struct drm_crtc_state *crtc_state, + struct drm_mode_object *obj, uint32_t blob_id) +{ + struct drm_property_blob *blob; + + blob = drm_property_lookup_blob(dev, blob_id); + if (!blob) { + DRM_ERROR("Invalid Blob ID\n"); + return -EINVAL; + } + + if (crtc_state->ctm_blob) + drm_property_unreference_blob(crtc_state->ctm_blob); + + crtc_state->ctm_blob = blob; + return 0; +} + int intel_color_manager_set_pipe_degamma(struct drm_device *dev, struct drm_crtc_state *crtc_state, struct drm_mode_object *obj, uint32_t blob_id) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0298dd0..4d571a2f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1473,4 +1473,7 @@ int intel_color_manager_set_pipe_gamma(struct drm_device *dev, int intel_color_manager_set_pipe_degamma(struct drm_device *dev, struct drm_crtc_state *crtc_state, struct drm_mode_object *obj, uint32_t blob_id); +int intel_color_manager_set_pipe_csc(struct drm_device *dev, + struct drm_crtc_state *crtc_state, + struct drm_mode_object *obj, uint32_t blob_id); #endif /* __INTEL_DRV_H__ */
DRM color manager allows the driver to showcase its best color correction capabilities using the cm_crtc_palette_capabilities_property. Driver loads the no. of coefficients for various color correction as per the platform, during the init time.
This patch adds no of coefficitents for best gamma color correction modes possible in CHV, in device info structure, which is: Gamma(10 bit, CGM HW unit):- 257 coeff These values will be loaded in cm_crtc_palette_capabilities_property during the CRTC init section, by color manager's attach function.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com --- drivers/gpu/drm/i915/i915_drv.c | 2 ++ drivers/gpu/drm/i915/intel_color_manager.h | 1 + 2 files changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index bdec64c..04a8e45 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -34,6 +34,7 @@ #include "i915_drv.h" #include "i915_trace.h" #include "intel_drv.h" +#include "intel_color_manager.h"
#include <linux/console.h> #include <linux/module.h> @@ -349,6 +350,7 @@ static const struct intel_device_info intel_cherryview_info = { .gen = 8, .num_pipes = 3, .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, + .num_samples_after_ctm = CHV_10BIT_GAMMA_MAX_VALS, .is_valleyview = 1, .display_mmio_offset = VLV_DISPLAY_BASE, GEN_CHV_PIPEOFFSETS, diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 1a42244..99ef646 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -29,3 +29,4 @@ #include "i915_drv.h"
#define COLOR_STRUCT_VERSION 1 +#define CHV_10BIT_GAMMA_MAX_VALS 257
DRM color manager allows the driver to showcase its best color correction capabilities using the cm_crtc_palette_capabilities_property.
This patch adds no of coefficitents for degamma color correction modes possible in CHV, in device info structure, which is: CGM Degamma(10 bit, CGM HW unit):- 65 coeff
These values will be loaded in cm_crtc_palette_capabilities_property during the CRTC init section, by color manager's attach function.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com --- drivers/gpu/drm/i915/i915_drv.c | 1 + drivers/gpu/drm/i915/intel_color_manager.h | 1 + 2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 04a8e45..03db841 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -351,6 +351,7 @@ static const struct intel_device_info intel_cherryview_info = { .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, .num_samples_after_ctm = CHV_10BIT_GAMMA_MAX_VALS, + .num_samples_before_ctm = CHV_DEGAMMA_MAX_VALS, .is_valleyview = 1, .display_mmio_offset = VLV_DISPLAY_BASE, GEN_CHV_PIPEOFFSETS, diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 99ef646..402b825 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -30,3 +30,4 @@
#define COLOR_STRUCT_VERSION 1 #define CHV_10BIT_GAMMA_MAX_VALS 257 +#define CHV_DEGAMMA_MAX_VALS 65
From: Kausal Malladi kausalmalladi@gmail.com
CHV/BSW platform supports two different pipe level gamma correction modes, which are: 1. Legacy 8-bit mode 2. 10-bit CGM (Color Gamut Mapping) mode
This patch does the following: 1. Attaches Gamma property to CRTC 3. Adds the core Gamma correction function for CHV/BSW 4. Adds Gamma correction macros
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/i915_reg.h | 12 +++ drivers/gpu/drm/i915/intel_color_manager.c | 127 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 21 +++++ 3 files changed, 160 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 67bf205..93aa1f9 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7977,4 +7977,16 @@ enum skl_disp_power_wells { #define GEN9_VEBOX_MOCS_0 0xcb00 /* Video MOCS base register*/ #define GEN9_BLT_MOCS_0 0xcc00 /* Blitter MOCS base register*/
+/* Color Management */ +#define PIPEA_CGM_CONTROL (VLV_DISPLAY_BASE + 0x67A00) +#define PIPEB_CGM_CONTROL (VLV_DISPLAY_BASE + 0x69A00) +#define PIPEC_CGM_CONTROL (VLV_DISPLAY_BASE + 0x6BA00) +#define PIPEA_CGM_GAMMA (VLV_DISPLAY_BASE + 0x67000) +#define PIPEB_CGM_GAMMA (VLV_DISPLAY_BASE + 0x69000) +#define PIPEC_CGM_GAMMA (VLV_DISPLAY_BASE + 0x6B000) +#define _PIPE_CGM_CONTROL(pipe) \ + (_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL)) +#define _PIPE_GAMMA_BASE(pipe) \ + (_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA)) + #endif /* _I915_REG_H_ */ diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 2ba3fd7..222924d 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -66,6 +66,127 @@ int intel_color_manager_set_pipe_degamma(struct drm_device *dev, return 0; }
+static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, + struct drm_crtc *crtc) +{ + bool flag = false; + enum pipe pipe; + u8 red_int, blue_int, green_int; + u16 red_fract, green_fract, blue_fract; + u32 red, green, blue; + u32 cgm_control_reg = 0; + u32 cgm_gamma_reg = 0; + u32 count = 0, num_samples, word; + int ret = 0, length; + struct drm_r32g32b32 *correction_values = NULL; + struct drm_palette *gamma_data; + struct drm_i915_private *dev_priv = dev->dev_private; + + if (WARN_ON(!blob)) + return -EINVAL; + + gamma_data = (struct drm_palette *)blob->data; + + if (gamma_data->version != GAMMA_DATA_STRUCT_VERSION) { + DRM_DEBUG_KMS("Invalid Gamma Data struct version\n"); + return -EINVAL; + } + + pipe = to_intel_crtc(crtc)->pipe; + num_samples = gamma_data->num_samples; + length = num_samples * sizeof(struct drm_r32g32b32); + + switch (num_samples) { + case 0: + + /* Disable Gamma functionality on Pipe - CGM Block */ + cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe)); + cgm_control_reg &= ~CGM_GAMMA_EN; + I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg); + + DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n", + pipe_name(pipe)); + ret = 0; + break; + + case CHV_8BIT_GAMMA_MAX_VALS: + case CHV_10BIT_GAMMA_MAX_VALS: + + count = 0; + cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe); + correction_values = (struct drm_r32g32b32 *)&gamma_data->lut; + + while (count < num_samples) { + blue = correction_values[count].b32; + green = correction_values[count].g32; + red = correction_values[count].r32; + + blue_int = _GAMMA_INT_PART(blue); + if (blue_int > GAMMA_INT_MAX) + blue = CHV_MAX_GAMMA; + + green_int = _GAMMA_INT_PART(green); + if (green_int > GAMMA_INT_MAX) + green = CHV_MAX_GAMMA; + + red_int = _GAMMA_INT_PART(red); + if (red_int > GAMMA_INT_MAX) + red = CHV_MAX_GAMMA; + + blue_fract = _GAMMA_FRACT_PART(blue); + green_fract = _GAMMA_FRACT_PART(green); + red_fract = _GAMMA_FRACT_PART(red); + + blue_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT; + green_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT; + red_fract >>= CHV_10BIT_GAMMA_MSB_SHIFT; + + /* Green (25:16) and Blue (9:0) to be written */ + word = green_fract; + word <<= CHV_GAMMA_SHIFT_GREEN; + word |= blue_fract; + I915_WRITE(cgm_gamma_reg, word); + cgm_gamma_reg += 4; + + /* Red (9:0) to be written */ + word = red_fract; + I915_WRITE(cgm_gamma_reg, word); + + cgm_gamma_reg += 4; + count++; + + /* + * On CHV, the best supported Gamma capability is + * CGM block, that takes 257 samples. + * If user gives 256 samples (legacy mode), then + * duplicate the 256th value to 257th. + * "flag" is used to make sure it happens only once + */ + if (num_samples == CHV_8BIT_GAMMA_MAX_VALS + && count == CHV_8BIT_GAMMA_MAX_VALS + && !flag) { + count--; + flag = true; + } + } + + /* Enable (CGM) Gamma on Pipe */ + I915_WRITE(_PIPE_CGM_CONTROL(pipe), + I915_READ(_PIPE_CGM_CONTROL(pipe)) + | CGM_GAMMA_EN); + DRM_DEBUG_DRIVER("CGM Gamma enabled on Pipe %c\n", + pipe_name(pipe)); + ret = 0; + break; + + default: + DRM_ERROR("Invalid number of samples for Gamma LUT\n"); + ret = -EINVAL; + } + + return ret; +} + int intel_color_manager_set_pipe_gamma(struct drm_device *dev, struct drm_crtc_state *crtc_state, struct drm_mode_object *obj, uint32_t blob_id) @@ -134,5 +255,11 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev, DRM_DEBUG_DRIVER("Capabilities attached to CRTC\n"); }
+ if (config->cm_palette_after_ctm_property) { + drm_object_attach_property(mode_obj, + config->cm_palette_after_ctm_property, 0); + DRM_DEBUG_DRIVER("palette after CTM attached to CRTC\n"); + } + kfree(palette_caps); } diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 402b825..5f800c9 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -28,6 +28,27 @@ #include <drm/drm_crtc_helper.h> #include "i915_drv.h"
+#define _GAMMA_INT_PART(value) (value >> GAMMA_INT_SHIFT) +#define _GAMMA_FRACT_PART(value) (value >> GAMMA_FRACT_SHIFT) + #define COLOR_STRUCT_VERSION 1 #define CHV_10BIT_GAMMA_MAX_VALS 257 #define CHV_DEGAMMA_MAX_VALS 65 + +/* Gamma correction */ +#define GAMMA_DATA_STRUCT_VERSION 1 +#define CHV_10BIT_GAMMA_MAX_VALS 257 +#define CHV_8BIT_GAMMA_MAX_VALS 256 +#define CHV_10BIT_GAMMA_MSB_SHIFT 6 +#define CHV_GAMMA_SHIFT_GREEN 16 + +/* Gamma values are u8.24 format */ +#define GAMMA_INT_SHIFT 24 +#define GAMMA_FRACT_SHIFT 8 +/* Max int value for Gamma is 1 */ +#define GAMMA_INT_MAX 1 +/* Max value for Gamma on CHV */ +#define CHV_MAX_GAMMA 0x10000 + +/* CHV CGM Block */ +#define CGM_GAMMA_EN (1 << 2)
CHV/BSW supports DeGamma color correction, which linearizes all the non-linear color values. This will be applied before Color Transformation.
This patch does the following: 1. Attach deGamma property to CRTC 2. Add the core function to program DeGamma correction values for CHV/BSW platform 2. Add DeGamma correction macros/defines
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/i915_reg.h | 6 ++ drivers/gpu/drm/i915/intel_color_manager.c | 111 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 6 ++ 3 files changed, 123 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 93aa1f9..3dde6a8 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7989,4 +7989,10 @@ enum skl_disp_power_wells { #define _PIPE_GAMMA_BASE(pipe) \ (_PIPE3(pipe, PIPEA_CGM_GAMMA, PIPEB_CGM_GAMMA, PIPEC_CGM_GAMMA))
+#define PIPEA_CGM_DEGAMMA (VLV_DISPLAY_BASE + 0x66000) +#define PIPEB_CGM_DEGAMMA (VLV_DISPLAY_BASE + 0x68000) +#define PIPEC_CGM_DEGAMMA (VLV_DISPLAY_BASE + 0x6A000) +#define _PIPE_DEGAMMA_BASE(pipe) \ + (_PIPE3(pipe, PIPEA_CGM_DEGAMMA, PIPEB_CGM_DEGAMMA, PIPEC_CGM_DEGAMMA)) + #endif /* _I915_REG_H_ */ diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 222924d..e2ffbdc 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -66,6 +66,111 @@ int intel_color_manager_set_pipe_degamma(struct drm_device *dev, return 0; }
+static int chv_set_degamma(struct drm_device *dev, + struct drm_property_blob *blob, struct drm_crtc *crtc) +{ + struct drm_palette *degamma_data; + struct drm_i915_private *dev_priv = dev->dev_private; + u32 cgm_control_reg = 0; + u32 cgm_degamma_reg = 0; + enum pipe pipe; + u32 red, green, blue; + u8 red_int, green_int, blue_int; + u16 red_fract, green_fract, blue_fract; + u32 count = 0; + struct drm_r32g32b32 *correction_values = NULL; + u32 num_samples; + u32 word; + int ret = 0, length; + + if (!blob) { + DRM_ERROR("NULL Blob\n"); + return -EINVAL; + } + + degamma_data = (struct drm_palette *)blob->data; + + if (degamma_data->version != DEGAMMA_DATA_STRUCT_VERSION) { + DRM_ERROR("Invalid DeGamma Data struct version\n"); + return -EINVAL; + } + + pipe = to_intel_crtc(crtc)->pipe; + num_samples = degamma_data->num_samples; + length = num_samples * sizeof(struct drm_r32g32b32); + + if (num_samples == 0) { + + /* Disable DeGamma functionality on Pipe - CGM Block */ + cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe)); + cgm_control_reg &= ~CGM_DEGAMMA_EN; + I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg); + + DRM_DEBUG_DRIVER("DeGamma disabled on Pipe %c\n", + pipe_name(pipe)); + ret = 0; + } else if (num_samples == CHV_DEGAMMA_MAX_VALS) { + cgm_degamma_reg = _PIPE_DEGAMMA_BASE(pipe); + + count = 0; + correction_values = (struct drm_r32g32b32 *)°amma_data->lut; + while (count < CHV_DEGAMMA_MAX_VALS) { + blue = correction_values[count].b32; + green = correction_values[count].g32; + red = correction_values[count].r32; + + blue_int = _GAMMA_INT_PART(blue); + if (blue_int > GAMMA_INT_MAX) + blue = CHV_MAX_GAMMA; + green_int = _GAMMA_INT_PART(green); + if (green_int > GAMMA_INT_MAX) + green = CHV_MAX_GAMMA; + red_int = _GAMMA_INT_PART(red); + if (red_int > GAMMA_INT_MAX) + red = CHV_MAX_GAMMA; + + blue_fract = _GAMMA_FRACT_PART(blue); + green_fract = _GAMMA_FRACT_PART(green); + red_fract = _GAMMA_FRACT_PART(red); + + blue_fract >>= CHV_DEGAMMA_MSB_SHIFT; + green_fract >>= CHV_DEGAMMA_MSB_SHIFT; + red_fract >>= CHV_DEGAMMA_MSB_SHIFT; + + /* Green (29:16) and Blue (13:0) in DWORD1 */ + word = green_fract; + word <<= CHV_DEGAMMA_GREEN_SHIFT; + word = word | blue; + I915_WRITE(cgm_degamma_reg, word); + + cgm_degamma_reg += 4; + + /* Red (13:0) to be written to DWORD2 */ + word = red_fract; + I915_WRITE(cgm_degamma_reg, word); + + cgm_degamma_reg += 4; + count++; + } + + DRM_DEBUG_DRIVER("DeGamma LUT loaded for Pipe %c\n", + pipe_name(pipe)); + + /* Enable DeGamma on Pipe */ + I915_WRITE(_PIPE_CGM_CONTROL(pipe), + I915_READ(_PIPE_CGM_CONTROL(pipe)) | CGM_DEGAMMA_EN); + + DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n", + pipe_name(pipe)); + ret = 0; + } else { + DRM_ERROR("Invalid number of samples for DeGamma LUT\n"); + return -EINVAL; + } + + return ret; +} + static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, struct drm_crtc *crtc) { @@ -261,5 +366,11 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev, DRM_DEBUG_DRIVER("palette after CTM attached to CRTC\n"); }
+ if (config->cm_palette_before_ctm_property) { + drm_object_attach_property(mode_obj, + config->cm_palette_before_ctm_property, 0); + DRM_DEBUG_DRIVER("palette before CTM attached to CRTC\n"); + } + kfree(palette_caps); } diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 5f800c9..5eaa0f3 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -50,5 +50,11 @@ /* Max value for Gamma on CHV */ #define CHV_MAX_GAMMA 0x10000
+/* DeGamma correction */ +#define DEGAMMA_DATA_STRUCT_VERSION 1 +#define CHV_DEGAMMA_MSB_SHIFT 2 +#define CHV_DEGAMMA_GREEN_SHIFT 16 + /* CHV CGM Block */ #define CGM_GAMMA_EN (1 << 2) +#define CGM_DEGAMMA_EN (1 << 0)
CHV/BSW supports Color Space Conversion (CSC) using a 3x3 matrix that needs to be programmed into CGM (Color Gamut Mapping) registers.
This patch does the following: 1. Attaches CSC property to CRTC 2. Adds the core function to program CSC correction values 3. Adds CSC correction macros
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/i915_reg.h | 8 +++ drivers/gpu/drm/i915/intel_color_manager.c | 99 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 20 ++++++ 3 files changed, 127 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 3dde6a8..5c7759d 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7995,4 +7995,12 @@ enum skl_disp_power_wells { #define _PIPE_DEGAMMA_BASE(pipe) \ (_PIPE3(pipe, PIPEA_CGM_DEGAMMA, PIPEB_CGM_DEGAMMA, PIPEC_CGM_DEGAMMA))
+#define PIPEA_CGM_CSC (VLV_DISPLAY_BASE + 0x67900) +#define PIPEB_CGM_CSC (VLV_DISPLAY_BASE + 0x69900) +#define PIPEC_CGM_CSC (VLV_DISPLAY_BASE + 0x6B900) +#define _PIPE_CSC_BASE(pipe) \ + (_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC)) + + + #endif /* _I915_REG_H_ */ diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index e2ffbdc..22b708f 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -66,6 +66,99 @@ int intel_color_manager_set_pipe_degamma(struct drm_device *dev, return 0; }
+static s16 get_csc_s3_12_format(s64 csc_value) +{ + s32 csc_int_value; + u32 csc_fract_value; + s16 csc_s3_12_format; + + if (csc_value >= 0) { + csc_value += CHV_CSC_FRACT_ROUNDOFF; + if (csc_value > CHV_CSC_COEFF_MAX) + csc_value = CHV_CSC_COEFF_MAX; + } else { + csc_value = -csc_value; + csc_value += CHV_CSC_FRACT_ROUNDOFF; + if (csc_value > CHV_CSC_COEFF_MAX + 1) + csc_value = CHV_CSC_COEFF_MAX + 1; + csc_value = -csc_value; + } + + csc_int_value = csc_value >> CHV_CSC_COEFF_SHIFT; + csc_int_value <<= CHV_CSC_COEFF_INT_SHIFT; + if (csc_value < 0) + csc_int_value |= CSC_COEFF_SIGN; + csc_fract_value = csc_value; + csc_fract_value >>= CHV_CSC_COEFF_FRACT_SHIFT; + csc_s3_12_format = csc_int_value | csc_fract_value; + + return csc_s3_12_format; +} + +static int chv_set_csc(struct drm_device *dev, struct drm_property_blob *blob, + struct drm_crtc *crtc) +{ + struct drm_ctm *csc_data; + struct drm_i915_private *dev_priv = dev->dev_private; + u32 reg; + enum pipe pipe; + s32 word, temp; + int count = 0; + + if (!blob) { + DRM_ERROR("NULL Blob\n"); + return -EINVAL; + } + + if (blob->length != sizeof(struct drm_ctm)) { + DRM_ERROR("Invalid length of data received\n"); + return -EINVAL; + } + + csc_data = (struct drm_ctm *)blob->data; + if (csc_data->version != CSC_DATA_STRUCT_VERSION) { + DRM_ERROR("Invalid CSC Data struct version\n"); + return -EINVAL; + } + + pipe = to_intel_crtc(crtc)->pipe; + + /* Disable CSC functionality */ + reg = _PIPE_CGM_CONTROL(pipe); + I915_WRITE(reg, I915_READ(reg) & (~CGM_CSC_EN)); + + DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n", + pipe_name(pipe)); + + reg = _PIPE_CSC_BASE(pipe); + while (count < CSC_MAX_VALS) { + word = get_csc_s3_12_format(csc_data->ctm_coeff[count]); + + /* + * Last value to be written in 1 register. + * Otherwise, each pair of CSC values go + * into 1 register + */ + if (count != (CSC_MAX_VALS - 1)) { + count++; + temp = get_csc_s3_12_format(csc_data->ctm_coeff[count]); + word |= temp; + } + I915_WRITE(reg, word); + reg += 4; + count++; + } + + DRM_DEBUG_DRIVER("All CSC values written to registers\n"); + + /* Enable CSC functionality */ + reg = _PIPE_CGM_CONTROL(pipe); + I915_WRITE(reg, I915_READ(reg) | CGM_CSC_EN); + DRM_DEBUG_DRIVER("CSC enabled on Pipe %c\n", pipe_name(pipe)); + + return 0; +} + static int chv_set_degamma(struct drm_device *dev, struct drm_property_blob *blob, struct drm_crtc *crtc) { @@ -372,5 +465,11 @@ void intel_attach_color_properties_to_crtc(struct drm_device *dev, DRM_DEBUG_DRIVER("palette before CTM attached to CRTC\n"); }
+ if (config->cm_ctm_property) { + drm_object_attach_property(mode_obj, + config->cm_ctm_property, 0); + DRM_DEBUG_DRIVER("CTM property attached to CRTC\n"); + } + kfree(palette_caps); } diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 5eaa0f3..6e5158e 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -55,6 +55,26 @@ #define CHV_DEGAMMA_MSB_SHIFT 2 #define CHV_DEGAMMA_GREEN_SHIFT 16
+/* CSC correction */ +#define CSC_DATA_STRUCT_VERSION 1 +/* + * Fractional part is 32 bit, and we need only 12 MSBs for programming + * into registers. ROUNDOFF is required to minimize loss of precision. + */ +#define CHV_CSC_FRACT_ROUNDOFF (1 << 19) +/* + * CSC values are 64-bit values. For CHV, the maximum CSC value that + * user can program is 7.99999..., which can be represented in fixed point + * S31.32 format like this, with all fractional bits as 1 + */ +#define CHV_CSC_COEFF_MAX 0x00000007FFFFFFFF +#define CHV_CSC_COEFF_SHIFT 32 +#define CHV_CSC_COEFF_INT_SHIFT 28 +#define CSC_COEFF_SIGN (1 << 31) +#define CHV_CSC_COEFF_FRACT_SHIFT 20 +#define CSC_MAX_VALS 9 + /* CHV CGM Block */ #define CGM_GAMMA_EN (1 << 2) +#define CGM_CSC_EN (1 << 1) #define CGM_DEGAMMA_EN (1 << 0)
The color correction blob values are loaded during set_property calls. This patch adds a function to find the blob and apply the correction values to the display registers, during the atomic commit call.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/intel_color_manager.c | 46 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 2 ++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ 3 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index 22b708f..d33a2be 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -406,6 +406,52 @@ int intel_color_manager_set_pipe_gamma(struct drm_device *dev, return 0; }
+void intel_color_manager_crtc_commit(struct drm_device *dev, + struct drm_crtc_state *crtc_state) +{ + struct drm_property_blob *blob; + struct drm_crtc *crtc = crtc_state->crtc; + int ret = -EINVAL; + + blob = crtc_state->palette_after_ctm_blob; + if (blob) { + /* Gamma correction is platform specific */ + if (IS_CHERRYVIEW(dev)) + ret = chv_set_gamma(dev, blob, crtc); + + if (ret) + DRM_ERROR("set Gamma correction failed\n"); + else + DRM_DEBUG_DRIVER("Gamma correction success\n"); + } + + blob = crtc_state->palette_before_ctm_blob; + if (blob) { + /* degamma correction */ + if (IS_CHERRYVIEW(dev)) + ret = chv_set_degamma(dev, blob, crtc); + + if (ret) + DRM_ERROR("set degamma correction failed\n"); + else + DRM_DEBUG_DRIVER("degamma correction success\n"); + } + + blob = crtc_state->ctm_blob; + if (blob) { + /* CSC correction */ + if (IS_CHERRYVIEW(dev)) + ret = chv_set_csc(dev, blob, crtc); + + if (ret) + DRM_ERROR("set CSC correction failed\n"); + else + DRM_DEBUG_DRIVER("CSC correction success\n"); + } + +} + + int get_pipe_capabilities(struct drm_device *dev, struct drm_palette_caps *palette_caps, struct drm_crtc *crtc) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 010acca..9b9d267 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13487,6 +13487,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, intel_update_pipe_config(intel_crtc, old_intel_state); else if (INTEL_INFO(dev)->gen >= 9) skl_detach_scalers(intel_crtc); + + intel_color_manager_crtc_commit(dev, crtc->state); }
static void intel_finish_crtc_commit(struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4d571a2f..f87a99d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1476,4 +1476,6 @@ int intel_color_manager_set_pipe_degamma(struct drm_device *dev, int intel_color_manager_set_pipe_csc(struct drm_device *dev, struct drm_crtc_state *crtc_state, struct drm_mode_object *obj, uint32_t blob_id); +void intel_color_manager_crtc_commit(struct drm_device *dev, + struct drm_crtc_state *crtc_state); #endif /* __INTEL_DRV_H__ */
Function intel_attach_color_properties_to_crtc attaches a color property to its CRTC object. This patch calls this function from crtc initialization sequence.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/intel_display.c | 1 + drivers/gpu/drm/i915/intel_drv.h | 2 ++ 2 files changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9b9d267..8b5f266 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13818,6 +13818,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) intel_crtc->cursor_size = ~0;
intel_crtc->wm.cxsr_allowed = true; + intel_attach_color_properties_to_crtc(dev, &intel_crtc->base.base);
BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) || dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f87a99d..f992345 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1478,4 +1478,6 @@ int intel_color_manager_set_pipe_csc(struct drm_device *dev, struct drm_mode_object *obj, uint32_t blob_id); void intel_color_manager_crtc_commit(struct drm_device *dev, struct drm_crtc_state *crtc_state); +void intel_attach_color_properties_to_crtc(struct drm_device *dev, + struct drm_mode_object *mode_obj); #endif /* __INTEL_DRV_H__ */
I915 color manager registers pipe gamma correction as palette correction after CTM property.
For BDW and higher platforms, split gamma correction is the best gamma correction. This patch adds the no of coefficients(512) for split gamma correction as "num_samples_after_ctm" parameter in device info structures, for all of those.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com --- drivers/gpu/drm/i915/i915_drv.c | 7 +++++++ drivers/gpu/drm/i915/intel_color_manager.h | 2 ++ 2 files changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 03db841..69afed3 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -302,6 +302,7 @@ static const struct intel_device_info intel_broadwell_d_info = { .gen = 8, .num_pipes = 3, .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, + .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -314,6 +315,7 @@ static const struct intel_device_info intel_broadwell_m_info = { .gen = 8, .is_mobile = 1, .num_pipes = 3, .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, + .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -326,6 +328,7 @@ static const struct intel_device_info intel_broadwell_gt3d_info = { .gen = 8, .num_pipes = 3, .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING, + .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -338,6 +341,7 @@ static const struct intel_device_info intel_broadwell_gt3m_info = { .gen = 8, .is_mobile = 1, .num_pipes = 3, .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING, + .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -363,6 +367,7 @@ static const struct intel_device_info intel_skylake_info = { .gen = 9, .num_pipes = 3, .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, + .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -376,6 +381,7 @@ static const struct intel_device_info intel_skylake_gt3_info = { .gen = 9, .num_pipes = 3, .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING, + .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -389,6 +395,7 @@ static const struct intel_device_info intel_broxton_info = { .gen = 9, .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, + .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, .num_pipes = 3, .has_ddi = 1, .has_fpga_dbg = 1, diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 6e5158e..3687989 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -42,6 +42,8 @@ #define CHV_10BIT_GAMMA_MSB_SHIFT 6 #define CHV_GAMMA_SHIFT_GREEN 16
+#define BDW_SPLITGAMMA_MAX_VALS 512 + /* Gamma values are u8.24 format */ #define GAMMA_INT_SHIFT 24 #define GAMMA_FRACT_SHIFT 8
BDW/SKL/BXT platforms support various Gamma correction modes which are: 1. Legacy 8-bit mode 2. 10-bit mode 3. 10-bit Split Gamma mode 4. 12-bit mode
This patch does the following: 1. Adds the core function to program Gamma correction values for BDW/SKL/BXT platforms 2. Adds Gamma correction macros/defines
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/i915_reg.h | 17 +- drivers/gpu/drm/i915/intel_color_manager.c | 270 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 15 ++ 3 files changed, 300 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 5c7759d..88f4e41 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5634,7 +5634,9 @@ enum skl_disp_power_wells {
#define _GAMMA_MODE_A 0x4a480 #define _GAMMA_MODE_B 0x4ac80 -#define GAMMA_MODE(pipe) _PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B) +#define _GAMMA_MODE_C 0x4b480 +#define GAMMA_MODE(pipe) \ + _PIPE3(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B, _GAMMA_MODE_C) #define GAMMA_MODE_MODE_MASK (3 << 0) #define GAMMA_MODE_MODE_8BIT (0 << 0) #define GAMMA_MODE_MODE_10BIT (1 << 0) @@ -8001,6 +8003,17 @@ enum skl_disp_power_wells { #define _PIPE_CSC_BASE(pipe) \ (_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC))
- +/* BDW gamma correction */ +#define PAL_PREC_INDEX_A 0x4A400 +#define PAL_PREC_INDEX_B 0x4AC00 +#define PAL_PREC_INDEX_C 0x4B400 +#define PAL_PREC_DATA_A 0x4A404 +#define PAL_PREC_DATA_B 0x4AC04 +#define PAL_PREC_DATA_C 0x4B404 + +#define _PREC_PAL_INDEX(pipe) \ + (_PIPE3(pipe, PAL_PREC_INDEX_A, PAL_PREC_INDEX_B, PAL_PREC_INDEX_C)) +#define _PREC_PAL_DATA(pipe) \ + (_PIPE3(pipe, PAL_PREC_DATA_A, PAL_PREC_DATA_B, PAL_PREC_DATA_C))
#endif /* _I915_REG_H_ */ diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index d33a2be..d935fd8 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -264,6 +264,274 @@ static int chv_set_degamma(struct drm_device *dev, return ret; }
+static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32 blue) +{ + u32 word; + u8 blue_int, green_int, red_int; + u16 blue_fract, green_fract, red_fract; + + blue_int = _GAMMA_INT_PART(blue); + if (blue_int > GAMMA_INT_MAX) + blue = BDW_MAX_GAMMA; + + green_int = _GAMMA_INT_PART(green); + if (green_int > GAMMA_INT_MAX) + green = BDW_MAX_GAMMA; + + red_int = _GAMMA_INT_PART(red); + if (red_int > GAMMA_INT_MAX) + red = BDW_MAX_GAMMA; + + blue_fract = _GAMMA_FRACT_PART(blue); + green_fract = _GAMMA_FRACT_PART(green); + red_fract = _GAMMA_FRACT_PART(red); + + blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT; + green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT; + red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT; + + /* Red (29:20) Green (19:10) and Blue (9:0) */ + word = red_fract; + word <<= BDW_GAMMA_SHIFT; + word = word | green_fract; + word <<= BDW_GAMMA_SHIFT; + word = word | blue_fract; + + return word; +} + +/* Apply unity gamma for gamma reset */ +static void bdw_reset_gamma(struct drm_i915_private *dev_priv, + enum pipe pipe) +{ + u16 count = 0; + u32 val; + u32 pal_prec_data = LGC_PALETTE(pipe); + + DRM_DEBUG_DRIVER("\n"); + + /* Reset the palette for unit gamma */ + while (count < BDW_8BIT_GAMMA_MAX_VALS) { + /* Red (23:16) Green (15:8) and Blue (7:0) */ + val = (count << 16) | (count << 8) | count; + I915_WRITE(pal_prec_data, val); + pal_prec_data += 4; + count++; + } +} + +static int bdw_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, + struct drm_crtc *crtc) +{ + u8 blue_int, green_int, red_int; + u16 blue_fract, green_fract, red_fract; + u16 blue_odd, green_odd, red_odd; + u16 blue_even, green_even, red_even; + + enum pipe pipe; + int count, num_samples; + u32 blue, green, red; + u32 mode, pal_prec_index, pal_prec_data; + u32 index, word; + struct drm_palette *gamma_data; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_r32g32b32 *correction_values = NULL; + + if (!blob) { + DRM_ERROR("Null Blob\n"); + return -EINVAL; + } + + gamma_data = (struct drm_palette *)blob->data; + + if (gamma_data->version != GAMMA_DATA_STRUCT_VERSION) { + DRM_DEBUG_KMS("Invalid Gamma Data struct version\n"); + return -EINVAL; + } + + pipe = to_intel_crtc(crtc)->pipe; + num_samples = gamma_data->num_samples; + + pal_prec_index = _PREC_PAL_INDEX(pipe); + pal_prec_data = _PREC_PAL_DATA(pipe); + + correction_values = (struct drm_r32g32b32 *)&gamma_data->lut; + index = I915_READ(pal_prec_index); + switch (num_samples) { + + case 0: + + /* Disable Gamma functionality on Pipe */ + DRM_DEBUG_DRIVER("Disabling gamma on Pipe %c\n", + pipe_name(pipe)); + bdw_reset_gamma(dev_priv, pipe); + word = GAMMA_MODE_MODE_8BIT; + break; + + case BDW_8BIT_GAMMA_MAX_VALS: + + /* Legacy palette */ + pal_prec_data = LGC_PALETTE(pipe); + + count = 0; + while (count < BDW_8BIT_GAMMA_MAX_VALS) { + blue = correction_values[count].b32; + green = correction_values[count].g32; + red = correction_values[count].r32; + + blue_int = _GAMMA_INT_PART(blue); + if (blue_int > GAMMA_INT_MAX) + blue = BDW_MAX_GAMMA; + green_int = _GAMMA_INT_PART(green); + if (green_int > GAMMA_INT_MAX) + green = BDW_MAX_GAMMA; + red_int = _GAMMA_INT_PART(red); + if (red_int > GAMMA_INT_MAX) + red = BDW_MAX_GAMMA; + + blue_fract = _GAMMA_FRACT_PART(blue); + green_fract = _GAMMA_FRACT_PART(green); + red_fract = _GAMMA_FRACT_PART(red); + + blue_fract >>= BDW_8BIT_GAMMA_MSB_SHIFT; + green_fract >>= BDW_8BIT_GAMMA_MSB_SHIFT; + red_fract >>= BDW_8BIT_GAMMA_MSB_SHIFT; + + /* Red (23:16) Green (15:8) and Blue (7:0) */ + word = red_fract; + word <<= BDW_8BIT_GAMMA_SHIFT; + word = word | green_fract; + word <<= BDW_8BIT_GAMMA_SHIFT; + word = word | blue_fract; + + I915_WRITE(pal_prec_data, word); + pal_prec_data += 4; + + count++; + } + + word = GAMMA_MODE_MODE_8BIT; + break; + + case BDW_SPLITGAMMA_MAX_VALS: + + index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE; + I915_WRITE(pal_prec_index, index); + + count = 0; + while (count < BDW_SPLITGAMMA_MAX_VALS) { + blue = correction_values[count].b32; + green = correction_values[count].g32; + red = correction_values[count].r32; + + word = bdw_write_10bit_gamma_precision(red, + green, blue); + I915_WRITE(pal_prec_data, word); + count++; + } + + word = GAMMA_MODE_MODE_SPLIT; + break; + + case BDW_12BIT_GAMMA_MAX_VALS: + + index |= BDW_INDEX_AUTO_INCREMENT; + index &= ~BDW_INDEX_SPLIT_MODE; + I915_WRITE(pal_prec_index, index); + + count = 0; + while (count < BDW_12BIT_GAMMA_MAX_VALS) { + blue = correction_values[count].b32; + green = correction_values[count].g32; + red = correction_values[count].r32; + + blue_int = _GAMMA_INT_PART(blue); + if (blue_int > GAMMA_INT_MAX) + blue = BDW_MAX_GAMMA; + green_int = _GAMMA_INT_PART(green); + if (green_int > GAMMA_INT_MAX) + green = BDW_MAX_GAMMA; + red_int = _GAMMA_INT_PART(red); + if (red_int > GAMMA_INT_MAX) + red = BDW_MAX_GAMMA; + + blue_fract = _GAMMA_FRACT_PART(blue); + green_fract = _GAMMA_FRACT_PART(green); + red_fract = _GAMMA_FRACT_PART(red); + + /* Odd index */ + if (count % 2 == 0) { + blue_odd = blue_fract >> + BDW_12BIT_GAMMA_ODD_SHIFT; + green_odd = green_fract >> + BDW_12BIT_GAMMA_ODD_SHIFT; + red_odd = red_fract >> + BDW_12BIT_GAMMA_ODD_SHIFT; + + word = red_odd; + word = word << BDW_GAMMA_SHIFT; + word = word | green_odd; + word = word << BDW_GAMMA_SHIFT; + word = word | blue_odd; + } else { + blue_even = blue_fract << + BDW_12BIT_GAMMA_EVEN_SHIFT1 >> + BDW_12BIT_GAMMA_EVEN_SHIFT2; + green_even = green_fract << + BDW_12BIT_GAMMA_EVEN_SHIFT1 >> + BDW_12BIT_GAMMA_EVEN_SHIFT2; + red_even = red_fract << + BDW_12BIT_GAMMA_EVEN_SHIFT1 >> + BDW_12BIT_GAMMA_EVEN_SHIFT2; + + word = red_even; + word = word << BDW_GAMMA_SHIFT; + word = word | green_even; + word = word << BDW_GAMMA_SHIFT; + word = word | blue_even; + } + I915_WRITE(pal_prec_data, word); + count++; + } + + word = GAMMA_MODE_MODE_12BIT; + break; + + case BDW_10BIT_GAMMA_MAX_VALS: + + index |= BDW_INDEX_AUTO_INCREMENT; + index &= ~BDW_INDEX_SPLIT_MODE; + I915_WRITE(pal_prec_index, index); + + count = 0; + while (count < BDW_10BIT_GAMMA_MAX_VALS) { + blue = correction_values[count].b32; + green = correction_values[count].g32; + red = correction_values[count].r32; + + word = bdw_write_10bit_gamma_precision(red, + green, blue); + I915_WRITE(pal_prec_data, word); + count++; + } + + word = GAMMA_MODE_MODE_10BIT; + break; + + default: + DRM_ERROR("Invalid number of samples\n"); + return -EINVAL; + } + + /* Set gamma mode on pipe control reg */ + mode = I915_READ(GAMMA_MODE(pipe)); + mode &= ~GAMMA_MODE_MODE_MASK; + I915_WRITE(GAMMA_MODE(pipe), mode | word); + DRM_DEBUG_DRIVER("Gamma applied on pipe %c\n", + pipe_name(pipe)); + return 0; +} + static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, struct drm_crtc *crtc) { @@ -418,6 +686,8 @@ void intel_color_manager_crtc_commit(struct drm_device *dev, /* Gamma correction is platform specific */ if (IS_CHERRYVIEW(dev)) ret = chv_set_gamma(dev, blob, crtc); + else if (IS_BROADWELL(dev) || IS_GEN9(dev)) + ret = bdw_set_gamma(dev, blob, crtc);
if (ret) DRM_ERROR("set Gamma correction failed\n"); diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 3687989..17fcf3d 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -43,6 +43,9 @@ #define CHV_GAMMA_SHIFT_GREEN 16
#define BDW_SPLITGAMMA_MAX_VALS 512 +#define BDW_8BIT_GAMMA_MAX_VALS 256 +#define BDW_10BIT_GAMMA_MAX_VALS 1024 +#define BDW_12BIT_GAMMA_MAX_VALS 513
/* Gamma values are u8.24 format */ #define GAMMA_INT_SHIFT 24 @@ -52,6 +55,18 @@ /* Max value for Gamma on CHV */ #define CHV_MAX_GAMMA 0x10000
+/* Gen 9 */ +#define BDW_MAX_GAMMA 0x10000 +#define BDW_10BIT_GAMMA_MSB_SHIFT 6 +#define BDW_GAMMA_SHIFT 10 +#define BDW_INDEX_AUTO_INCREMENT (1 << 15) +#define BDW_INDEX_SPLIT_MODE (1 << 31) +#define BDW_8BIT_GAMMA_MSB_SHIFT 8 +#define BDW_8BIT_GAMMA_SHIFT 8 +#define BDW_12BIT_GAMMA_ODD_SHIFT 6 +#define BDW_12BIT_GAMMA_EVEN_SHIFT1 10 +#define BDW_12BIT_GAMMA_EVEN_SHIFT2 6 + /* DeGamma correction */ #define DEGAMMA_DATA_STRUCT_VERSION 1 #define CHV_DEGAMMA_MSB_SHIFT 2
Hi Shashank, some feedback below that you would be great to get addressed before your next version.
On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
BDW/SKL/BXT platforms support various Gamma correction modes which are:
- Legacy 8-bit mode
- 10-bit mode
- 10-bit Split Gamma mode
- 12-bit mode
This patch does the following:
- Adds the core function to program Gamma correction values for BDW/SKL/BXT platforms
- Adds Gamma correction macros/defines
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
drivers/gpu/drm/i915/i915_reg.h | 17 +- drivers/gpu/drm/i915/intel_color_manager.c | 270 +++++++++++++++++++++++++++++
*snip*
+static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32 blue) +{
- u32 word;
- u8 blue_int, green_int, red_int;
- u16 blue_fract, green_fract, red_fract;
- blue_int = _GAMMA_INT_PART(blue);
- if (blue_int > GAMMA_INT_MAX)
blue = BDW_MAX_GAMMA;- green_int = _GAMMA_INT_PART(green);
- if (green_int > GAMMA_INT_MAX)
green = BDW_MAX_GAMMA;- red_int = _GAMMA_INT_PART(red);
- if (red_int > GAMMA_INT_MAX)
red = BDW_MAX_GAMMA;- blue_fract = _GAMMA_FRACT_PART(blue);
- green_fract = _GAMMA_FRACT_PART(green);
- red_fract = _GAMMA_FRACT_PART(red);
- blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- /* Red (29:20) Green (19:10) and Blue (9:0) */
- word = red_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | green_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | blue_fract;
- return word;
+}
I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.
In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.
I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.
In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.
BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:
" 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ? 13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil 13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as all white with a linear one "
You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff
*snip*
+/* Gen 9 */ +#define BDW_MAX_GAMMA 0x10000
*snip*
I look forward to testing against your next version.
Rob
Hey Rob,
Thanks for the feedback, and the testing efforts. I will check into your suggestions and get back.
Regards Shashank -----Original Message----- From: Bradford, Robert Sent: Wednesday, September 30, 2015 8:02 PM To: Sharma, Shashank; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction
Hi Shashank, some feedback below that you would be great to get addressed before your next version.
On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
BDW/SKL/BXT platforms support various Gamma correction modes which are:
- Legacy 8-bit mode
- 10-bit mode
- 10-bit Split Gamma mode
- 12-bit mode
This patch does the following:
- Adds the core function to program Gamma correction values for BDW/SKL/BXT platforms
- Adds Gamma correction macros/defines
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
drivers/gpu/drm/i915/i915_reg.h | 17 +- drivers/gpu/drm/i915/intel_color_manager.c | 270 +++++++++++++++++++++++++++++
*snip*
+static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32 blue) +{
- u32 word;
- u8 blue_int, green_int, red_int;
- u16 blue_fract, green_fract, red_fract;
- blue_int = _GAMMA_INT_PART(blue);
- if (blue_int > GAMMA_INT_MAX)
blue = BDW_MAX_GAMMA;- green_int = _GAMMA_INT_PART(green);
- if (green_int > GAMMA_INT_MAX)
green = BDW_MAX_GAMMA;- red_int = _GAMMA_INT_PART(red);
- if (red_int > GAMMA_INT_MAX)
red = BDW_MAX_GAMMA;- blue_fract = _GAMMA_FRACT_PART(blue);
- green_fract = _GAMMA_FRACT_PART(green);
- red_fract = _GAMMA_FRACT_PART(red);
- blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- /* Red (29:20) Green (19:10) and Blue (9:0) */
- word = red_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | green_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | blue_fract;
- return word;
+}
I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.
In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.
I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.
In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.
BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:
" 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ? 13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil 13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as all white with a linear one "
You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff
*snip*
+/* Gen 9 */ +#define BDW_MAX_GAMMA 0x10000
*snip*
I look forward to testing against your next version.
Rob
Shashank,
Please let us know when the next patch series would be ready assuming you incorporate the feedback from Rob.
I would like to hope we're done with feedback at this point.
Thanks.
Annie Matheson
On Sep 30, 2015, at 9:25 AM, Sharma, Shashank shashank.sharma@intel.com wrote:
Hey Rob,
Thanks for the feedback, and the testing efforts. I will check into your suggestions and get back.
Regards Shashank -----Original Message----- From: Bradford, Robert Sent: Wednesday, September 30, 2015 8:02 PM To: Sharma, Shashank; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction
Hi Shashank, some feedback below that you would be great to get addressed before your next version.
On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote: BDW/SKL/BXT platforms support various Gamma correction modes which are:
- Legacy 8-bit mode
- 10-bit mode
- 10-bit Split Gamma mode
- 12-bit mode
This patch does the following:
- Adds the core function to program Gamma correction values
for BDW/SKL/BXT platforms 2. Adds Gamma correction macros/defines
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
drivers/gpu/drm/i915/i915_reg.h | 17 +- drivers/gpu/drm/i915/intel_color_manager.c | 270 +++++++++++++++++++++++++++++
*snip*
+static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32 blue) +{
- u32 word;
- u8 blue_int, green_int, red_int;
- u16 blue_fract, green_fract, red_fract;
- blue_int = _GAMMA_INT_PART(blue);
- if (blue_int > GAMMA_INT_MAX)
blue = BDW_MAX_GAMMA;- green_int = _GAMMA_INT_PART(green);
- if (green_int > GAMMA_INT_MAX)
green = BDW_MAX_GAMMA;- red_int = _GAMMA_INT_PART(red);
- if (red_int > GAMMA_INT_MAX)
red = BDW_MAX_GAMMA;- blue_fract = _GAMMA_FRACT_PART(blue);
- green_fract = _GAMMA_FRACT_PART(green);
- red_fract = _GAMMA_FRACT_PART(red);
- blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- /* Red (29:20) Green (19:10) and Blue (9:0) */
- word = red_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | green_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | blue_fract;
- return word;
+}
I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.
In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.
I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.
In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.
BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:
" 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ? 13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil 13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as all white with a linear one "
You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff
*snip*
+/* Gen 9 */ +#define BDW_MAX_GAMMA 0x10000
*snip*
I look forward to testing against your next version.
Rob
Hi Annie, This might add a day or two, but considering this is a long weekend in India, you can expect the patches by mid next week.
Regards Shashank -----Original Message----- From: Matheson, Annie J Sent: Wednesday, September 30, 2015 10:01 PM To: Sharma, Shashank Cc: Bradford, Robert; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction
Shashank,
Please let us know when the next patch series would be ready assuming you incorporate the feedback from Rob.
I would like to hope we're done with feedback at this point.
Thanks.
Annie Matheson
On Sep 30, 2015, at 9:25 AM, Sharma, Shashank shashank.sharma@intel.com wrote:
Hey Rob,
Thanks for the feedback, and the testing efforts. I will check into your suggestions and get back.
Regards Shashank -----Original Message----- From: Bradford, Robert Sent: Wednesday, September 30, 2015 8:02 PM To: Sharma, Shashank; Roper, Matthew D; Bish, Jim; Smith, Gary K; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Cc: Vetter, Daniel; Matheson, Annie J; Mukherjee, Indranil; Palleti, Avinash Reddy; kausalmalladi@gmail.com Subject: Re: [PATCH 21/23] drm/i915: BDW: Pipe level Gamma correction
Hi Shashank, some feedback below that you would be great to get addressed before your next version.
On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote: BDW/SKL/BXT platforms support various Gamma correction modes which are:
- Legacy 8-bit mode
- 10-bit mode
- 10-bit Split Gamma mode
- 12-bit mode
This patch does the following:
- Adds the core function to program Gamma correction values
for BDW/SKL/BXT platforms 2. Adds Gamma correction macros/defines
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
drivers/gpu/drm/i915/i915_reg.h | 17 +- drivers/gpu/drm/i915/intel_color_manager.c | 270 +++++++++++++++++++++++++++++
*snip*
+static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32 blue) +{
- u32 word;
- u8 blue_int, green_int, red_int;
- u16 blue_fract, green_fract, red_fract;
- blue_int = _GAMMA_INT_PART(blue);
- if (blue_int > GAMMA_INT_MAX)
blue = BDW_MAX_GAMMA;- green_int = _GAMMA_INT_PART(green);
- if (green_int > GAMMA_INT_MAX)
green = BDW_MAX_GAMMA;- red_int = _GAMMA_INT_PART(red);
- if (red_int > GAMMA_INT_MAX)
red = BDW_MAX_GAMMA;- blue_fract = _GAMMA_FRACT_PART(blue);
- green_fract = _GAMMA_FRACT_PART(green);
- red_fract = _GAMMA_FRACT_PART(red);
- blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- /* Red (29:20) Green (19:10) and Blue (9:0) */
- word = red_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | green_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | blue_fract;
- return word;
+}
I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.
In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.
I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.
In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.
BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:
" 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ? 13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil 13:56 < danvet> also this probably needs an igt - check that all black with an all 1.0 gamma table is the same as all white with a linear one "
You won't see this with your test program (color-correction.c) as the largest value you program in appears to be 0.ff
*snip*
+/* Gen 9 */ +#define BDW_MAX_GAMMA 0x10000
*snip*
I look forward to testing against your next version.
Rob
On Wed, Sep 30, 2015 at 03:31:34PM +0100, Rob Bradford wrote:
Hi Shashank, some feedback below that you would be great to get addressed before your next version.
On Wed, 2015-09-16 at 23:07 +0530, Shashank Sharma wrote:
BDW/SKL/BXT platforms support various Gamma correction modes which are:
- Legacy 8-bit mode
- 10-bit mode
- 10-bit Split Gamma mode
- 12-bit mode
This patch does the following:
- Adds the core function to program Gamma correction values for BDW/SKL/BXT platforms
- Adds Gamma correction macros/defines
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com
drivers/gpu/drm/i915/i915_reg.h | 17 +- drivers/gpu/drm/i915/intel_color_manager.c | 270 +++++++++++++++++++++++++++++
*snip*
+static u32 bdw_write_10bit_gamma_precision(u32 red, u32 green, u32 blue) +{
- u32 word;
- u8 blue_int, green_int, red_int;
- u16 blue_fract, green_fract, red_fract;
- blue_int = _GAMMA_INT_PART(blue);
- if (blue_int > GAMMA_INT_MAX)
blue = BDW_MAX_GAMMA;- green_int = _GAMMA_INT_PART(green);
- if (green_int > GAMMA_INT_MAX)
green = BDW_MAX_GAMMA;- red_int = _GAMMA_INT_PART(red);
- if (red_int > GAMMA_INT_MAX)
red = BDW_MAX_GAMMA;- blue_fract = _GAMMA_FRACT_PART(blue);
- green_fract = _GAMMA_FRACT_PART(green);
- red_fract = _GAMMA_FRACT_PART(red);
- blue_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- green_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- red_fract >>= BDW_10BIT_GAMMA_MSB_SHIFT;
- /* Red (29:20) Green (19:10) and Blue (9:0) */
- word = red_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | green_fract;
- word <<= BDW_GAMMA_SHIFT;
- word = word | blue_fract;
- return word;
+}
I think the above function, and perhaps others in this series have the same flaw with respect to maximum colour value.
In our discussions we agreed that we would follow the "GL style" where maximum colour (i.e. 255 in 8-bit) would be represented by 1.0f. 1.0f when converted to fixed point 8.24 notation is 1 << 24.
I observed that with my test code that a linear ramp (where the last entry is set to 1.0) gives me black for white. I tracked it down to this function.
In order to map 1.0 to the maximum value for the table entry in the hardware this function needs to be changed. One way to achieve this would be change the test "blue_int > GAMMA_INT_MAX" to be "blue_int >= GAMMA_INT_MAX" but since GAMMA_INT_MAX = 1 then it might be clearer to say blue_int > 0.
BDW_MAX_GAMMA also looks wrong. I think it should be (1 << 24) - 1 not the 0x10000 (see below). As well as correct clamping it is also necessary to scale as Daniel suggested on IRC:
" 13:54 < danvet> robster, so we'd need to rescale in the kernel from 1.0 to 0.1111111111b ? 13:55 < danvet> well substract 0.0000000001b for all values > 0.5 probably since float math in the kernel is evil
Or just clamp to '(1<<bits)-1'. I think the end result is the same actually.
I915 color manager registers pipe degamma correction as palette correction before CTM, DRM property.
This patch adds the no of coefficients(65) for degamma correction as "num_samples_before_ctm" parameter in device info structures, for BDW and higher platforms.
Signed-off-by: Shashank Sharma shashank.sharma@intel.com --- drivers/gpu/drm/i915/i915_drv.c | 7 +++++++ drivers/gpu/drm/i915/intel_color_manager.h | 2 ++ 2 files changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 69afed3..23ad2cc 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -303,6 +303,7 @@ static const struct intel_device_info intel_broadwell_d_info = { .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, + .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -316,6 +317,7 @@ static const struct intel_device_info intel_broadwell_m_info = { .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, + .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -329,6 +331,7 @@ static const struct intel_device_info intel_broadwell_gt3d_info = { .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING, .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, + .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -342,6 +345,7 @@ static const struct intel_device_info intel_broadwell_gt3m_info = { .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING, .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, + .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -368,6 +372,7 @@ static const struct intel_device_info intel_skylake_info = { .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, + .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -382,6 +387,7 @@ static const struct intel_device_info intel_skylake_gt3_info = { .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING, .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, + .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS, .has_llc = 1, .has_ddi = 1, .has_fpga_dbg = 1, @@ -396,6 +402,7 @@ static const struct intel_device_info intel_broxton_info = { .need_gfx_hws = 1, .has_hotplug = 1, .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, .num_samples_after_ctm = BDW_SPLITGAMMA_MAX_VALS, + .num_samples_before_ctm = BDW_DEGAMMA_MAX_VALS, .num_pipes = 3, .has_ddi = 1, .has_fpga_dbg = 1, diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h index 17fcf3d..a428825 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.h +++ b/drivers/gpu/drm/i915/intel_color_manager.h @@ -72,6 +72,8 @@ #define CHV_DEGAMMA_MSB_SHIFT 2 #define CHV_DEGAMMA_GREEN_SHIFT 16
+#define BDW_DEGAMMA_MAX_VALS 512 + /* CSC correction */ #define CSC_DATA_STRUCT_VERSION 1 /*
BDW/SKL/BXT supports Degamma color correction feature, which linearizes the non-linearity due to gamma encoded color values. This will be applied before Color Transformation.
This patch does the following: 1. Adds the core function to program DeGamma correction values for BDW/SKL/BXT platform 2. Adds DeGamma correction macros/defines
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi kausalmalladi@gmail.com --- drivers/gpu/drm/i915/intel_color_manager.c | 61 ++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c index d935fd8..46f880d 100644 --- a/drivers/gpu/drm/i915/intel_color_manager.c +++ b/drivers/gpu/drm/i915/intel_color_manager.c @@ -532,6 +532,65 @@ static int bdw_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, return 0; }
+static int bdw_set_degamma(struct drm_device *dev, + struct drm_property_blob *blob, struct drm_crtc *crtc) +{ + enum pipe pipe; + int num_samples, length; + int count = 0; + u32 index, word; + u32 blue, green, red; + u32 mode, pal_prec_index, pal_prec_data; + struct drm_palette *degamma_data; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_r32g32b32 *correction_values = NULL; + + if (WARN_ON(!blob)) + return -EINVAL; + + degamma_data = (struct drm_palette *)blob->data; + if (degamma_data->version != DEGAMMA_DATA_STRUCT_VERSION) { + DRM_ERROR("Invalid DeGamma Data struct version\n"); + return -EINVAL; + } + + pipe = to_intel_crtc(crtc)->pipe; + num_samples = degamma_data->num_samples; + if (num_samples != BDW_SPLITGAMMA_MAX_VALS) { + DRM_ERROR("Invalid number of samples\n"); + return -EINVAL; + } + + length = num_samples * sizeof(struct drm_r32g32b32); + mode = I915_READ(GAMMA_MODE(pipe)); + pal_prec_index = _PREC_PAL_INDEX(pipe); + pal_prec_data = _PREC_PAL_DATA(pipe); + + correction_values = (struct drm_r32g32b32 *)°amma_data->lut; + index = I915_READ(pal_prec_index); + index |= BDW_INDEX_AUTO_INCREMENT | BDW_INDEX_SPLIT_MODE; + I915_WRITE(pal_prec_index, index); + + while (count < num_samples) { + blue = correction_values[count].b32; + green = correction_values[count].g32; + red = correction_values[count].r32; + + word = bdw_write_10bit_gamma_precision(red, green, blue); + I915_WRITE(pal_prec_data, word); + count++; + } + + /* Enable DeGamma on Pipe */ + mode &= ~GAMMA_MODE_MODE_MASK; + I915_WRITE(GAMMA_MODE(pipe), mode | GAMMA_MODE_MODE_SPLIT); + + DRM_DEBUG_DRIVER("DeGamma correction enabled on Pipe %c\n", + pipe_name(pipe)); + + return 0; +} + static int chv_set_gamma(struct drm_device *dev, struct drm_property_blob *blob, struct drm_crtc *crtc) { @@ -700,6 +759,8 @@ void intel_color_manager_crtc_commit(struct drm_device *dev, /* degamma correction */ if (IS_CHERRYVIEW(dev)) ret = chv_set_degamma(dev, blob, crtc); + else if (IS_BROADWELL(dev) || IS_GEN9(dev)) + ret = bdw_set_degamma(dev, blob, crtc);
if (ret) DRM_ERROR("set degamma correction failed\n");
On Wed, Sep 16, 2015 at 11:06:57PM +0530, Shashank Sharma wrote:
This patch set adds Color Manager implementation in DRM layer. Color Manager is an extension in DRM framework to support color correction/enhancement.
Various Hardware platforms can support several color correction capabilities. Color Manager provides abstraction of these capabilities and allows a user space UI agent to correct/enhance the display using the DRM property interface.
How is this going to work?
- This patch series adds a few new properties in DRM framework. These properties are: a. color_capabilities property (type blob) b. Color Transformation Matrix property for corrections like CSC (called CTM, type blob) c. Palette correction properties for corrections like gamma fixup (called palette_correction, type blob)
- Also, this patch series adds few structures to indicate specifications of a property like size, no_of_samples for correction etc.
- These properties are present in mode_config.
- When the platform's display driver loads, it fills up the values of color_capabilities property using the standard structures (added in step 2). For example, Intel's I915 driver adds following color correction capabilities: a. gamma correction capability as palette correction property, with 257 correction coefficients and a max/min value b. csc correction capability as CTM correction property, with 3x3 transformation matrix values and max/min values
- Now when userspace comes up, it queries the platform's color capabilities by doing a get_property() on color_capabilities DRM property
- Reading the blob, the userspace understands the color capabilities of the platform. For example, userspace will understand it can support: a. palette_correction with 257 coefficients b. CSC correction with 3x3 = 9 values
- To set color correction values, userspace: a. creates a blob using the create_blob_ioctl in standard palette_correction structure format, with the correction values b. calls the set_property_ioctl with the blob_id as value for the property
- Driver refers to the blob, gets the correction values and applies the correction in HW.
- To get currently applied color correction values, userspace: a. calls a get_property_ioctl on that color property b. gets the blob_id for the currently applied correction from DRM infrastructure c. gets the blob using get_blob_ioctl and hence the currently applied values
That's all! :)
About the patch series:
The patch series first adds the color management support in DRM layer. Then it adds the color management framework in I915 layer. After that, it implements platform specific core color correction functios.
Intel color manager registers color correction with DRM color manager in this way:
- CSC transformation is registered as CTM DRM property
- Gamma correction is registered as palette_after_ctm DRM property
- Degamma correction is registered as palette_before_ctm DRM property
Our thanks to all the reviewers who have given valuable comments in terms of design and implementation to our previous sets of patches. Special mention of thanks should go to Matt Roper for all his inputs/suggestions in implementation of this module, using DRM atomic CRTC commit path.
V2: Worked on review comments from Matt, Jim, Thierry, Rob. V3: Worked on review comments from Matt, Jim, Rob:
Jim, Rob:
Re-arranged the whole patch series in the sequence of features, currently: First 5 patches add color management support in DRM layer Next 7 patches add Intel color management framework in I915 driver Next 5 patches add color correction for CHV (gamma, degamma and CSC) Next 2 patches enable color management, by attaching the properties to CRTC(Matt) Next 4 patches add color correction for BDW (gamma, degamma)Matt:
Patch 3: Added refernce/unreference for blob Patch 7: return -EINVAL and added debug message Patch 8: check for valid blob, from create blob moved call to intel_crtc_attach_color_prop in the end of full implementation (CHV) Patch 9: DRM_ERROR->DRM_DEBUG for NULL blob case Patch 13: Added static for internal functions Patch 20-24: renamed gen9_* functions to bdw_* Added new variables in device_info structure num_samples_after_ctm and num_samples_before_ctm Added new function in patch 8 to load capabilities based on device_info across all platforms
Ok I finally got around to look at the kernel/userspace and drm core parts in detail. There's a few minor bits to polish but nothing serious I think, looks good overall (but I didn't really look at the i915 side).
One thing though is the kerneldoc part. You have some great detailed comments in the structs about how this is all supposed to be used, but afaict they're not pulled into the kerneldoc anywhere. Also we don't have an overview section, but if you add the drm_crtc_attach_cc_properties function like I suggested we could add that there (with links to all the various structures).
There's also the giant drm props table but that one is a mess. My suggestion would be to start a new DOC: section just for color correction, with an integrated markdown table. Then add that as a subsection to KMS property documentation. That way we'll have solid docs for this KMS extension (something we should have for all the other stuff like rotation or zpos too).
Thanks, Daniel
Shashank Sharma (23): drm: Create Color Management DRM properties drm: Add structure for querying palette color capabilities drm: Add color correction blobs in CRTC state drm: Add drm structures for palette color property drm: Add structure to set/get a CTM color property drm/i915: Add atomic set property interface for CRTC drm/i915: Add atomic get property interface for CRTC drm/i915: Create color management files drm/i915: Register pipe color capabilities drm/i915: Add gamma correction handlers drm/i915: Add pipe deGamma correction handlers drm/i915: Add pipe CSC correction handlers drm/i915: CHV: Load gamma color correction values drm/i915: CHV: Pipe level Gamma correction drm/i915: CHV: Load degamma color correction values drm/i915: CHV: Pipe level degamma correction drm/i915: CHV: Pipe level CSC correction drm/i915: Commit color changes to CRTC drm/i915: Attach color properties to CRTC drm/i915: BDW: Load gamma correction values drm/i915: BDW: Pipe level Gamma correction drm/i915: BDW: Load degamma correction values drm/i915: BDW: Pipe level degamma correction
drivers/gpu/drm/drm_atomic_helper.c | 12 + drivers/gpu/drm/drm_crtc.c | 26 + drivers/gpu/drm/i915/Makefile | 3 +- drivers/gpu/drm/i915/i915_drv.c | 17 + drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_reg.h | 41 +- drivers/gpu/drm/i915/intel_atomic.c | 56 ++ drivers/gpu/drm/i915/intel_color_manager.c | 852 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 99 ++++ drivers/gpu/drm/i915/intel_display.c | 6 + drivers/gpu/drm/i915/intel_drv.h | 22 + include/drm/drm_crtc.h | 11 + include/uapi/drm/drm.h | 50 ++ 13 files changed, 1195 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Regards Shashank
On 9/22/2015 6:57 PM, Daniel Vetter wrote:
On Wed, Sep 16, 2015 at 11:06:57PM +0530, Shashank Sharma wrote:
This patch set adds Color Manager implementation in DRM layer. Color Manager is an extension in DRM framework to support color correction/enhancement.
Various Hardware platforms can support several color correction capabilities. Color Manager provides abstraction of these capabilities and allows a user space UI agent to correct/enhance the display using the DRM property interface.
How is this going to work?
- This patch series adds a few new properties in DRM framework. These properties are: a. color_capabilities property (type blob) b. Color Transformation Matrix property for corrections like CSC (called CTM, type blob) c. Palette correction properties for corrections like gamma fixup (called palette_correction, type blob)
- Also, this patch series adds few structures to indicate specifications of a property like size, no_of_samples for correction etc.
- These properties are present in mode_config.
- When the platform's display driver loads, it fills up the values of color_capabilities property using the standard structures (added in step 2). For example, Intel's I915 driver adds following color correction capabilities: a. gamma correction capability as palette correction property, with 257 correction coefficients and a max/min value b. csc correction capability as CTM correction property, with 3x3 transformation matrix values and max/min values
- Now when userspace comes up, it queries the platform's color capabilities by doing a get_property() on color_capabilities DRM property
- Reading the blob, the userspace understands the color capabilities of the platform. For example, userspace will understand it can support: a. palette_correction with 257 coefficients b. CSC correction with 3x3 = 9 values
- To set color correction values, userspace: a. creates a blob using the create_blob_ioctl in standard palette_correction structure format, with the correction values b. calls the set_property_ioctl with the blob_id as value for the property
- Driver refers to the blob, gets the correction values and applies the correction in HW.
- To get currently applied color correction values, userspace: a. calls a get_property_ioctl on that color property b. gets the blob_id for the currently applied correction from DRM infrastructure c. gets the blob using get_blob_ioctl and hence the currently applied values
That's all! :)
About the patch series:
The patch series first adds the color management support in DRM layer. Then it adds the color management framework in I915 layer. After that, it implements platform specific core color correction functios.
Intel color manager registers color correction with DRM color manager in this way:
- CSC transformation is registered as CTM DRM property
- Gamma correction is registered as palette_after_ctm DRM property
- Degamma correction is registered as palette_before_ctm DRM property
Our thanks to all the reviewers who have given valuable comments in terms of design and implementation to our previous sets of patches. Special mention of thanks should go to Matt Roper for all his inputs/suggestions in implementation of this module, using DRM atomic CRTC commit path.
V2: Worked on review comments from Matt, Jim, Thierry, Rob. V3: Worked on review comments from Matt, Jim, Rob:
Jim, Rob:
Re-arranged the whole patch series in the sequence of features, currently: First 5 patches add color management support in DRM layer Next 7 patches add Intel color management framework in I915 driver Next 5 patches add color correction for CHV (gamma, degamma and CSC) Next 2 patches enable color management, by attaching the properties to CRTC(Matt) Next 4 patches add color correction for BDW (gamma, degamma)Matt:
Patch 3: Added refernce/unreference for blob Patch 7: return -EINVAL and added debug message Patch 8: check for valid blob, from create blob moved call to intel_crtc_attach_color_prop in the end of full implementation (CHV) Patch 9: DRM_ERROR->DRM_DEBUG for NULL blob case Patch 13: Added static for internal functions Patch 20-24: renamed gen9_* functions to bdw_* Added new variables in device_info structure num_samples_after_ctm and num_samples_before_ctm Added new function in patch 8 to load capabilities based on device_info across all platformsOk I finally got around to look at the kernel/userspace and drm core parts in detail. There's a few minor bits to polish but nothing serious I think, looks good overall (but I didn't really look at the i915 side).
One thing though is the kerneldoc part. You have some great detailed comments in the structs about how this is all supposed to be used, but afaict they're not pulled into the kerneldoc anywhere. Also we don't have an overview section, but if you add the drm_crtc_attach_cc_properties function like I suggested we could add that there (with links to all the various structures).
Now, after the explanation in the corresponding patch, do you still find a need for the function ?
There's also the giant drm props table but that one is a mess. My suggestion would be to start a new DOC: section just for color correction, with an integrated markdown table. Then add that as a subsection to KMS property documentation. That way we'll have solid docs for this KMS extension (something we should have for all the other stuff like rotation or zpos too).
Yes, sure, I will do this.
Thanks, Daniel
Shashank Sharma (23): drm: Create Color Management DRM properties drm: Add structure for querying palette color capabilities drm: Add color correction blobs in CRTC state drm: Add drm structures for palette color property drm: Add structure to set/get a CTM color property drm/i915: Add atomic set property interface for CRTC drm/i915: Add atomic get property interface for CRTC drm/i915: Create color management files drm/i915: Register pipe color capabilities drm/i915: Add gamma correction handlers drm/i915: Add pipe deGamma correction handlers drm/i915: Add pipe CSC correction handlers drm/i915: CHV: Load gamma color correction values drm/i915: CHV: Pipe level Gamma correction drm/i915: CHV: Load degamma color correction values drm/i915: CHV: Pipe level degamma correction drm/i915: CHV: Pipe level CSC correction drm/i915: Commit color changes to CRTC drm/i915: Attach color properties to CRTC drm/i915: BDW: Load gamma correction values drm/i915: BDW: Pipe level Gamma correction drm/i915: BDW: Load degamma correction values drm/i915: BDW: Pipe level degamma correction
drivers/gpu/drm/drm_atomic_helper.c | 12 + drivers/gpu/drm/drm_crtc.c | 26 + drivers/gpu/drm/i915/Makefile | 3 +- drivers/gpu/drm/i915/i915_drv.c | 17 + drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_reg.h | 41 +- drivers/gpu/drm/i915/intel_atomic.c | 56 ++ drivers/gpu/drm/i915/intel_color_manager.c | 852 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_color_manager.h | 99 ++++ drivers/gpu/drm/i915/intel_display.c | 6 + drivers/gpu/drm/i915/intel_drv.h | 22 + include/drm/drm_crtc.h | 11 + include/uapi/drm/drm.h | 50 ++ 13 files changed, 1195 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
Matheson, Annie J -
Matt Roper -
Rob Bradford -
Sharma, Shashank -
Shashank Sharma
-
Smith, Gary K -
Ville Syrjälä