dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Pekka Paalanen <ppaalanen@gmail.com>
To: "Shankar, Uma" <uma.shankar@intel.com>,
	Harry Wentland <harry.wentland@amd.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	Vitaly Prosyak <vitaly.prosyak@amd.com>,
	Sebastian Wick <sebastian@sebastianwick.net>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>
Subject: Re: [PATCH 02/21] drm: Add Plane Degamma Mode property
Date: Tue, 8 Jun 2021 11:34:35 +0300	[thread overview]
Message-ID: <20210608113435.6f621fb1@eldfell> (raw)
In-Reply-To: <152c3ac0e84e4bcdae832d8819179fce@intel.com>

[-- Attachment #1: Type: text/plain, Size: 9881 bytes --]

On Mon, 7 Jun 2021 17:34:06 +0000
"Shankar, Uma" <uma.shankar@intel.com> wrote:

> > -----Original Message-----
> > From: Harry Wentland <harry.wentland@amd.com>
> > Sent: Friday, June 4, 2021 11:54 PM
> > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org
> > Cc: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>; Cyr, Aric
> > <Aric.Cyr@amd.com>
> > Subject: Re: [PATCH 02/21] drm: Add Plane Degamma Mode property
> > 
> > On 2021-06-01 6:51 a.m., Uma Shankar wrote:  
> > > Add Plane Degamma Mode as an enum property. Create a helper function
> > > for all plane color management features.
> > >
> > > This is an enum property with values as blob_id's and exposes the
> > > various gamma modes supported and the lut ranges. Getting the blob id
> > > in userspace, user can get the mode supported and also the range of
> > > gamma mode supported with number of lut coefficients. It can then set
> > > one of the modes using this enum property.
> > >
> > > Lut values will be sent through separate GAMMA_LUT blob property.
> > >
> > > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > ---
> > >  Documentation/gpu/drm-kms.rst             | 90 ++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_atomic.c              |  1 +
> > >  drivers/gpu/drm/drm_atomic_state_helper.c |  2 +
> > >  drivers/gpu/drm/drm_atomic_uapi.c         |  4 +
> > >  drivers/gpu/drm/drm_color_mgmt.c          | 93 ++++++++++++++++++++++-
> > >  include/drm/drm_mode_object.h             |  2 +-
> > >  include/drm/drm_plane.h                   | 23 ++++++
> > >  7 files changed, 212 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/gpu/drm-kms.rst
> > > b/Documentation/gpu/drm-kms.rst index 87e5023e3f55..752be545e7d7
> > > 100644
> > > --- a/Documentation/gpu/drm-kms.rst
> > > +++ b/Documentation/gpu/drm-kms.rst
> > > @@ -514,9 +514,99 @@ Damage Tracking Properties  Color Management
> > > Properties
> > >  ---------------------------
> > >
> > > +Below is how a typical hardware pipeline for color will look like:
> > > +
> > > +.. kernel-render:: DOT
> > > +   :alt: Display Color Pipeline
> > > +   :caption: Display Color Pipeline Overview
> > > +
> > > +   digraph "KMS" {
> > > +      node [shape=box]
> > > +
> > > +      subgraph cluster_static {
> > > +          style=dashed
> > > +          label="Display Color Hardware Blocks"
> > > +
> > > +          node [bgcolor=grey style=filled]
> > > +          "Plane Degamma A" -> "Plane CSC/CTM A"
> > > +          "Plane CSC/CTM A" -> "Plane Gamma A"
> > > +          "Pipe Blender" [color=lightblue,style=filled, width=5.25, height=0.75];
> > > +          "Plane Gamma A" -> "Pipe Blender"
> > > +	  "Pipe Blender" -> "Pipe DeGamma"
> > > +          "Pipe DeGamma" -> "Pipe CSC/CTM"
> > > +          "Pipe CSC/CTM" -> "Pipe Gamma"
> > > +          "Pipe Gamma" -> "Pipe Output"
> > > +      }
> > > +  
> > 
> > It might be worthwhile to also highlight the YCbCr coefficient matrix in the pipeline,
> > between the FB and Plane degamma, i.e.
> >   YCbCr coefficients > plane degamma > csc > ...
> > 
> > One problem with this view is that not all HW will support all (or any) of these CM
> > blocks on all planes. For example, on AMD HW cursors are very different from other
> > planes and don't really have full CM support.
> >   
> > > +      subgraph cluster_static {
> > > +          style=dashed
> > > +
> > > +          node [shape=box]
> > > +          "Plane Degamma B" -> "Plane CSC/CTM B"
> > > +          "Plane CSC/CTM B" -> "Plane Gamma B"
> > > +          "Plane Gamma B" -> "Pipe Blender"
> > > +      }
> > > +
> > > +      subgraph cluster_static {
> > > +          style=dashed
> > > +
> > > +          node [shape=box]
> > > +          "Plane Degamma C" -> "Plane CSC/CTM C"
> > > +          "Plane CSC/CTM C" -> "Plane Gamma C"
> > > +          "Plane Gamma C" -> "Pipe Blender"
> > > +      }
> > > +
> > > +      subgraph cluster_fb {
> > > +          style=dashed
> > > +          label="RAM"
> > > +
> > > +          node [shape=box width=1.7 height=0.2]
> > > +
> > > +          "FB 1" -> "Plane Degamma A"
> > > +          "FB 2" -> "Plane Degamma B"
> > > +          "FB 3" -> "Plane Degamma C"
> > > +      }
> > > +   }
> > > +
> > > +In real world usecases,
> > > +
> > > +1. Plane Degamma can be used to linearize a non linear gamma encoded
> > > +framebuffer. This is needed to do any linear math like color space
> > > +conversion. For ex, linearize frames encoded in SRGB or by HDR curve.
> > > +
> > > +2. Later Plane CTM block can convert the content to some different
> > > +colorspace. For ex, SRGB to BT2020 etc.
> > > +
> > > +3. Plane Gamma block can be used later to re-apply the non-linear
> > > +curve. This can also be used to apply Tone Mapping for HDR usecases.
> > > +  
> > 
> > This would mean you're blending in gamma space which is likely not what most
> > compositors expect. There are numerous articles that describe why blending in
> > gamma space is problematic, such as [1]
> > 
> > [1] https://ninedegreesbelow.com/photography/linear-gamma-blur-normal-
> > blend.html
> > 
> > To blend in linear space this should be configured to do
> > 
> >   Plane Degamma > Plane CTM > CRTC Gamma
> > 
> > I think it would also be good if we moved away from calling this gamma. It's really
> > only gamma for legacy SDR scenarios. For HDR cases I would never expect these to
> > use gamma and even though the sRGB transfer function is based on gamma
> > functions it's complicated [2].
> > 
> > [2] https://en.wikipedia.org/wiki/SRGB
> > 
> > A better way to describe these would be as "transfer function" and "inverse transfer
> > function." The space at various stages could then be described as linear or non-
> > linear, specifically PQ, HLG, sRGB, BT709, or using another transfer function.  
> 
> Yeah I just kept the naming based on what we had for crtc to avoid ambiguity. But yes, we
> can rename these to have them named generically, as gamma lut actually can be used for
> Tone mapping as well and its not just meant for applying a fixed non liner curve only. However
> naming based on transfer function is also not optimum as Tone mapping is not related to transfer
> function. May be we can name them as:
> linear_lut and nl_tone_mapper_lut.
> 
> Other ideas for names are welcome.

I would suggest to avoid any kind of naming that explicitly refers to
gamma, EOTF, EOTF^-1, tone map, or anything else that has a specific
meaning. Likewise, the diagram should not claim that the pixel data at
certain stages of the pipeline is in certain form, e.g. linear,
non-linear, or in certain color space. Doing that would semantically
restrict what userspace can do. After all, kernel UAPI design is
mechanism, not policy, right?

However, to make the pipeline understandable, the documentation could
show example configurations, where you *can* say that this LUT
implements EOTF, pixels are light-linear at that point, the color space
at another point is XXX, and so on.

What exactly the LUT is, are pixel values linear or non-linear, and
what the color space is at any step are something that userspace will
define through providing the parameters for each processing step in the
pipeline.

Unless, your hardware has been designed for a certain policy and
deviating from that policy should be discouraged?

So what to name these:

That's the question we are struggling with at Weston too. You can seek
inspiration from CMM data structures (e.g. Little CMS 2) or from ICC
specifications. Just need to be careful to not pick a term that has
more meaning than kernel UAPI wants.

I personally like the terms "color curve" for 1D LUT -like things, and
"color mapping" for matrix or 3D LUT -like things. I'm using the term
"color transformation" for going from one encoding, color space, gamut,
and dynamic range to another with a specific render intent, which may
require a sequence of several color curves and color mappings to
achieve. But that's just me.

Why bother with all this indirection:

Use cases that want blending in non-linear spaces are one thing, but
probably less useful with HDR. Instead, I would imagine that doing
numerical optimization on a complete pipeline to achieve the best
precision given hardware limitations or minimizing memory consumption
or other, may result in different curves and mapping tables than simply
plugging in the EOTFs and CSCs into their intended places.

I have actually started to think that when Weston's DRM-backend is
trying to fit a window on a KMS plane, it would need to get the color
pipeline capabilities of that specific plane and ask the color manager
component to optimize the color transformation against the exact KMS
capabilities, evaluating whether a sufficient precision can be
achieved. Then the color manager gives back a description of how the
KMS plane would need to be configured, and the DRM-backend continues
with an atomic TEST_ONLY commit to see if it would work. The color
manager would quite likely delegate that optimization to a CMM, like
Little CMS 2 if possible. But this is all still so much hand-waving and
the KMS UAPI is not really there yet, that I will mostly just ignore
this idea for now and do something much simpler.

Or maybe all this optimization has already been done when designing the
new hardware, and we really do need to simply plug in the EOTFs and CSCs
without thinking?

Then again, if hardware changes every generation, that implies the
optimization is not complete, and there might be something to gain from
software optimization still. Old hardware always exists, too.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2021-06-08  8:34 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-01 10:51 [PATCH 00/21] Add Support for Plane Color Lut and CSC features Uma Shankar
2021-06-01 10:51 ` [PATCH 01/21] drm: Add Enhanced Gamma and color lut range attributes Uma Shankar
2021-06-02  9:33   ` Pekka Paalanen
2021-06-02 20:26     ` Shankar, Uma
2021-06-04 15:23       ` Harry Wentland
2021-06-07 17:19         ` Shankar, Uma
2021-06-01 10:51 ` [PATCH 02/21] drm: Add Plane Degamma Mode property Uma Shankar
2021-06-04 18:24   ` Harry Wentland
2021-06-07 11:00     ` Pekka Paalanen
2021-06-07 17:34     ` Shankar, Uma
2021-06-08  8:34       ` Pekka Paalanen [this message]
2021-06-01 10:52 ` [PATCH 03/21] drm: Add Plane Degamma Lut property Uma Shankar
2021-06-01 10:52 ` [PATCH 04/21] drm/i915/xelpd: Define Degamma Lut range struct for HDR planes Uma Shankar
2021-06-28 15:14   ` Harry Wentland
2021-06-30 11:36     ` Shankar, Uma
2021-06-01 10:52 ` [PATCH 05/21] drm/i915/xelpd: Add register definitions for Plane Degamma Uma Shankar
2021-06-01 10:52 ` [PATCH 06/21] drm/i915/xelpd: Enable plane color features Uma Shankar
2021-06-01 10:52 ` [PATCH 07/21] drm/i915/xelpd: Add color capabilities of SDR planes Uma Shankar
2021-06-01 10:52 ` [PATCH 08/21] drm/i915/xelpd: Program Plane Degamma Registers Uma Shankar
2021-06-01 10:52 ` [PATCH 09/21] drm/i915/xelpd: Add plane color check to glk_plane_color_ctl Uma Shankar
2021-06-01 10:52 ` [PATCH 10/21] drm/i915/xelpd: Initialize plane color features Uma Shankar
2021-06-01 10:52 ` [PATCH 11/21] drm/i915/xelpd: Load plane color luts from atomic flip Uma Shankar
2021-06-01 10:52 ` [PATCH 12/21] drm: Add Plane CTM property Uma Shankar
2021-06-01 10:52 ` [PATCH 13/21] drm: Add helper to attach Plane ctm property Uma Shankar
2021-06-01 10:52 ` [PATCH 14/21] drm/i915/xelpd: Define Plane CSC Registers Uma Shankar
2021-06-01 10:52 ` [PATCH 15/21] drm/i915/xelpd: Enable Plane CSC Uma Shankar
2021-06-01 10:52 ` [PATCH 16/21] drm: Add Plane Gamma Mode property Uma Shankar
2021-06-01 10:52 ` [PATCH 17/21] drm: Add Plane Gamma Lut property Uma Shankar
2021-06-01 10:52 ` [PATCH 18/21] drm/i915/xelpd: Define and Initialize Plane Gamma Lut range Uma Shankar
2021-06-01 10:52 ` [PATCH 19/21] drm/i915/xelpd: Add register definitions for Plane Gamma Uma Shankar
2021-06-01 10:52 ` [PATCH 20/21] drm/i915/xelpd: Program Plane Gamma Registers Uma Shankar
2021-06-01 10:52 ` [PATCH 21/21] drm/i915/xelpd: Enable plane gamma Uma Shankar
2021-06-02  9:28 ` [PATCH 00/21] Add Support for Plane Color Lut and CSC features Pekka Paalanen
2021-06-02 20:22   ` Shankar, Uma
2021-06-02 23:42     ` Harry Wentland
2021-06-03  8:47       ` Pekka Paalanen
2021-06-03 12:30         ` Sebastian Wick
2021-06-03 12:58           ` Pekka Paalanen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210608113435.6f621fb1@eldfell \
    --to=ppaalanen@gmail.com \
    --cc=bhanuprakash.modem@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=harry.wentland@amd.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=sebastian@sebastianwick.net \
    --cc=uma.shankar@intel.com \
    --cc=vitaly.prosyak@amd.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).