Re: [PATCH 00/23] Color Management for DRM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?
==========================
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

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).

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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux