RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
> -Original Message-
> From: Alex Hung
> Sent: Thursday, March 27, 2025 5:17 AM
> To: dri-de...@lists.freedesktop.org; amd-...@lists.freedesktop.org
> Cc: wayland-devel@lists.freedesktop.org; harry.wentl...@amd.com;
> alex.h...@amd.com; leo@amd.com; ville.syrj...@linux.intel.com;
> pekka.paala...@collabora.com; cont...@emersion.fr; m...@igalia.com;
> jad...@redhat.com; sebastian.w...@redhat.com; shashank.sha...@amd.com;
> ago...@nvidia.com; jos...@froggi.es; mdaen...@redhat.com;
> aleix...@kde.org; xaver.h...@gmail.com; victo...@system76.com;
> dan...@ffwll.ch; Shankar, Uma ;
> quic_nas...@quicinc.com; quic_cbr...@quicinc.com;
> quic_abhin...@quicinc.com; mar...@marcan.st; liviu.du...@arm.com;
> sashamcint...@google.com; Borah, Chaitanya Kumar
> ; louis.chau...@bootlin.com
> Subject: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
>
> We've previously introduced DRM_COLOROP_1D_CURVE for pre-defined 1D
> curves. But we also have HW that supports custom curves and userspace needs
> the ability to pass custom curves, aka LUTs.
>
> This patch introduces a new colorop type, called DRM_COLOROP_1D_LUT that
> provides a SIZE property which is used by a driver to advertise the supported
> SIZE
> of the LUT, as well as a DATA property which userspace uses to set the LUT.
>
> DATA and size function in the same way as current drm_crtc GAMMA and
> DEGAMMA LUTs.
Thanks Alex and Harry for driving this forward.
We want to have just one change in the way we expose the hardware capabilities
else
all looks good in general.
To expose the capabilities of 1D Lut (of any kind), we can use a generic
implementation which covers the distribution of LUT samples along with size and
precision in a common color op. Something like below:
struct drm_color_lut_range {
__u32 flags;
__u16 count;
__s32 start, end;
__u32 norm_factor;
struct {
__u16 intp;
__u16 fracp;
} precision;
};
The idea is to expose the lut distribution covering the entire color range from
0 to 1.0.
The underlying implementation in hardware can have just 1 segment covering the
full range
OR a complex LUT distribution in Hardware like PWL, multi segmented etc.
To explain the usage with some real life example
1. Conventional 1D LUT with just one segment
|---|---||
0 1 2 1024
- Hardware Description: A color block with a LUT linearly interpolating
and
covering range from 0 to 1.0
- Number of segments - 1
- Number of samples in LUT 1024
- Precision of LUT samples in HW 0.10
- Normalization Factor - Max value to
represent 1.0
in terms of smallest step size
which is 1024.
In this case, it will be represented by the following structure.
struct drm_color_lut_range lut_1024[] = {
.start = 0 .end = (1 << 10);
.normalization_factor = 1024;
.count = 1024;
.precision {
.int_comp = 0;
.fractional_comp = 10;
}
}
2. Piece Wise Linear 1D LUT
|---|---||
0 1 232
| \
| \
| \
|\
0 \
|---|---|--...---|
0 1 28
- Hardware Description: A color block with a LUT linearly interpolating
and
covering range from 0 to 1.0
- Number of segments 2
- Number of samples
- segment 1 - 9 (covers range
from 0 to 1/32)
- segment 2 - 30 (covers range
from 2/32 to 1.0)
- Precision of LUT samples in HW 0.24
- Normalization Factor - Max value to
represent 1.0
in terms of smallest step size
which is 8*32.
struct drm_color_lut_range lut_pwl[] = {
/* segment 1 */
{
.count = 9,
.start = 0, .end = 8,
.norm_factor = 8*32,
Re: Pipeline vs. no pipeline (Re: [PATCH V8 06/43] drm/colorop: Add 1D Curve subtype)
On 2025-04-10 03:53, Pekka Paalanen wrote: > On Tue, 8 Apr 2025 13:30:46 -0400 > Harry Wentland wrote: > >> On 2025-04-08 12:40, Daniel Stone wrote: >>> Hi there, >>> >>> On Tue, 1 Apr 2025 at 20:53, Simon Ser wrote: On Tuesday, April 1st, 2025 at 17:14, Daniel Stone wrote: > > ... > > 1. Is it guaranteed that, if any plane on a device supports the > COLOR_PIPELINE property, all planes will support COLOR_PIPELINE? > (Given the amdgpu cursor-plane discussion, it looks like no, which is > unfortunate but oh well.) I don't think so. (They could all expose a COLOR_PIPELINE with the only choice as the zero bypass pipeline, but that sounds silly.) >>> >>> Works for me: so any planes could not have colour pipelines, and the >>> result would be undefined (well, less defined) colour. >> >> Yes, basically it would be what we have now (without color pipelines). > > Hi, > > I see Alex wrote: > >> In order to support YUV we'll need to add COLOR_ENCODING and COLOR_RANGE >> support to the color pipeline. I have sketched these out already but >> don't have it all hooked up yet. This should not hinder adoption of this >> API for gaming use-cases. > > Was it considered to be able to lift the full-range RGB restriction > from the color pipelines, eventually leading to the possibility of > scanning out limited-range YCbCr bit-identical, giving userspace access > to the sub-black and super-white ranges for e.g. BT.814 purposes? > For AMD HW design and validation assumes that the pipeline is dealing with our internal floating point format and RGB values. Anything beyond that is somewhat undefined. Things might work as one expects but the product was definitely not designed and validated for that usage. I assume other HW design makes similar assumptions. > These questions are pointing in the direction of a bypass > COLOR_PIPELINE being different from no COLOR_PIPELINE. I assume a > bypass pipeline needs to shovel values through unchanged, while > "without color pipelines" would need the old COLOR_ENCODING and > COLOR_RANGE properties. > What I take it to mean is that this iteration of COLOR_PIPELINE only allows for RGB pipelines as YCbCr ones are underspecified without COLOR_RANGE and COLOR_ENCODING. For RGB a bypass pipeline should be the same as no COLOR_PIPELINE. > That reminds me of yet another question: if the framebuffer is limited > range, and it's not converted to full-range at the start of a color > pipeline, how will the sub-black and super-white ranges be represented? > Will they be negative and greater than 1.0 values, respectively? This > would be meaningful for the colorops being defined now, as I assume > people might implicitly limit their thinking to the [0.0, 1.0] range, > or at least exclude negative values. > Without COLOR_RANGE there is no way to know whether the input is limited or full. Is your question about when we have COLOR_RANGE specified? If that is set to LIMITED then I expect the values to get expanded at the beginning of the pipeline. In that case it's probably a HW or driver implementation detail whether the sub-blacks or super-whites will still be represented (as negative or >1.0 values) or clipped. > The 3x4 CTM colorop is not yet explicit on whether it clamps its inputs > or outputs. Should all colorops be explicit about it? > Do we expect all HW/drivers to be able to support the same behavior? Is this critical to using the colorop? Harry > > Thanks, > pq
Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
On 2025-04-15 02:40, Shankar, Uma wrote: > > >> -Original Message- >> From: Simon Ser >> Sent: Tuesday, April 15, 2025 11:47 AM >> To: Shankar, Uma >> Cc: Alex Hung ; dri-de...@lists.freedesktop.org; amd- >> g...@lists.freedesktop.org; intel-...@lists.freedesktop.org; wayland- >> de...@lists.freedesktop.org; harry.wentl...@amd.com; leo@amd.com; >> ville.syrj...@linux.intel.com; pekka.paala...@collabora.com; >> m...@igalia.com; jad...@redhat.com; sebastian.w...@redhat.com; >> shashank.sha...@amd.com; ago...@nvidia.com; jos...@froggi.es; >> mdaen...@redhat.com; aleix...@kde.org; xaver.h...@gmail.com; >> victo...@system76.com; dan...@ffwll.ch; quic_nas...@quicinc.com; >> quic_cbr...@quicinc.com; quic_abhin...@quicinc.com; mar...@marcan.st; >> liviu.du...@arm.com; sashamcint...@google.com; Borah, Chaitanya Kumar >> ; louis.chau...@bootlin.com >> Subject: RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type >> >> On Tuesday, April 15th, 2025 at 08:09, Shankar, Uma >> wrote: >> >>> We want to have just one change in the way we expose the hardware >>> capabilities else all looks good in general. >> >> I would really recommend leaving this as a follow-up extension. It's a >> complicated >> addition that requires more discussion. > > Hi Simon, > We have tried to solve the complex part and made it simple to understand and > implement > along with a reference implementation [1] (can also help add the same for AMD > case as well). > Without this we will end up with up 2 interfaces for 1dL Lut which is not > nice where the one above > will be able to cover the current one. Let us know the problems with the > proposed interface and we can > work to fix the same. But having a common and single interface is good and > the current one will not fit > Intel's color pipeline distribution so the generic one anyways will be > needed, and it will benefit userspace > to know the underlying LUT distribution to compute the LUT samples. > > [1] https://patchwork.freedesktop.org/series/129812/ > I think there is a lot of value in giving userspace a simple LUT to work with. There are many compositors and many compositor maintainers. When someone new jumps into color management usually same thing happens. It starts with "it's not too complicated", and then over a period of time progresses to "this is very much non-trivial" as understanding one bit usually opens ten more questions. Forcing people to deal with another level of complexity will discourage implementations and be counterproductive to furthering adoption of color operations for HW acceleration, IMO. I'm am not opposed to a complex LUT definition but I don't think it should replace a simple and well-understood definition. Harry > Regards, > Uma Shankar >
Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
On Tuesday, April 15th, 2025 at 17:05, Harry Wentland wrote: > > > > We want to have just one change in the way we expose the hardware > > > > capabilities else all looks good in general. > > > > > > I would really recommend leaving this as a follow-up extension. It's a > > > complicated > > > addition that requires more discussion. > > > > Hi Simon, > > We have tried to solve the complex part and made it simple to understand > > and implement > > along with a reference implementation [1] (can also help add the same for > > AMD case as well). > > Without this we will end up with up 2 interfaces for 1dL Lut which is not > > nice where the one above > > will be able to cover the current one. Let us know the problems with the > > proposed interface and we can > > work to fix the same. But having a common and single interface is good and > > the current one will not fit > > Intel's color pipeline distribution so the generic one anyways will be > > needed, and it will benefit userspace > > to know the underlying LUT distribution to compute the LUT samples. > > > > [1] https://patchwork.freedesktop.org/series/129812/ > > I think there is a lot of value in giving userspace a simple LUT > to work with. There are many compositors and many compositor > maintainers. When someone new jumps into color management usually > same thing happens. It starts with "it's not too complicated", > and then over a period of time progresses to "this is very much > non-trivial" as understanding one bit usually opens ten more > questions. > > Forcing people to deal with another level of complexity will > discourage implementations and be counterproductive to furthering > adoption of color operations for HW acceleration, IMO. > > I'm am not opposed to a complex LUT definition but I don't think > it should replace a simple and well-understood definition. Agreed. To add on this, I think shipping many additional features from day one significantly increases the work load (more code to write, review, test at once) and we'd also need to go through supplementary rounds to validate the API design and ensure it's not too Intel-specific. Also adding this feature as a second step will prove that the API is as extensible as we desire. I don't really understand why it's important to have this feature in the first version. Intel has been converting simple LUTs into the fancy distribution for the existing GAMMA_LUT and DEGAMMA_LUT for a while, so can do it for colorop as well. The upsides of the fancy distribution is more precise and smaller LUTs, but that doesn't seem critical? Simon
RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
> -Original Message- > From: Simon Ser > Sent: Tuesday, April 15, 2025 11:47 AM > To: Shankar, Uma > Cc: Alex Hung ; dri-de...@lists.freedesktop.org; amd- > g...@lists.freedesktop.org; intel-...@lists.freedesktop.org; wayland- > de...@lists.freedesktop.org; harry.wentl...@amd.com; leo@amd.com; > ville.syrj...@linux.intel.com; pekka.paala...@collabora.com; > m...@igalia.com; jad...@redhat.com; sebastian.w...@redhat.com; > shashank.sha...@amd.com; ago...@nvidia.com; jos...@froggi.es; > mdaen...@redhat.com; aleix...@kde.org; xaver.h...@gmail.com; > victo...@system76.com; dan...@ffwll.ch; quic_nas...@quicinc.com; > quic_cbr...@quicinc.com; quic_abhin...@quicinc.com; mar...@marcan.st; > liviu.du...@arm.com; sashamcint...@google.com; Borah, Chaitanya Kumar > ; louis.chau...@bootlin.com > Subject: RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type > > On Tuesday, April 15th, 2025 at 08:09, Shankar, Uma > wrote: > > > We want to have just one change in the way we expose the hardware > > capabilities else all looks good in general. > > I would really recommend leaving this as a follow-up extension. It's a > complicated > addition that requires more discussion. Hi Simon, We have tried to solve the complex part and made it simple to understand and implement along with a reference implementation [1] (can also help add the same for AMD case as well). Without this we will end up with up 2 interfaces for 1dL Lut which is not nice where the one above will be able to cover the current one. Let us know the problems with the proposed interface and we can work to fix the same. But having a common and single interface is good and the current one will not fit Intel's color pipeline distribution so the generic one anyways will be needed, and it will benefit userspace to know the underlying LUT distribution to compute the LUT samples. [1] https://patchwork.freedesktop.org/series/129812/ Regards, Uma Shankar
