Hello,
This patch series adds cubic (3D) look up table support to the CMM and DU drivers, and extend the KMS userspace API to expose the cubic LUT to userspace.
The code is fairly straightforward. Patch 1/4 refactors the CMM (color management module, the Renesas R-Car IP core that handles 1D and 3D lookup tables for the display) driver, which currently supports the 1D (a.k.a. gamma) table only, to prepare for 3D LUT support (including a modification to the API between the CMM and DU drivers). The CMM driver is then extended in patch 2/4 to support the 3D LUT.
Patch 3/4 adds support for the 3D LUT in the KMS core and the KMS userspace API, in the form of two new properties. I expect this to be the most controversial part of the series, not so much for the feature itself, but for when it is inserted in the color management pipeline.
Finally, patch 4/4 wires the KMS extension to the DU driver.
The R-Car CMM applies the 3D LUT at the output of the display, where data is transmitted in RGB space (when outputting YUV data to the display the CMM can't be used according to the documentation, but I wouldn't be entirely surprised if this limitation could be worked around), before the 1D LUT. I've located the 3D LUT between the CTM and the gamma LUT, but it could equally be placed before the degamma LUT or between the degamma LUT and the CTM in my case, as the R-Car color management pipeline has no CTM and has a single 1D LUT on the output side (there's provision for 1D LUT on the input side for some of the planes, but that's a separate topic).
I however don't expect this to necessarily match all hardware though, and this feature may require us to give up on a fixed, one size fits them all, color management pipeline exposed to userspace. Whether this would mean device-specific APIs (not necessarily in the form of device-specific properties, but in how they map to hardware features, as I think helpers to handle a 3D LUT property in the KMS core can save code duplication in drivers), or the need for a new property to expose the order in which color management operations are implemented, I don't know.
I started having a look at userspace to see how this could be handled, searching for color management support in weston, kwin and wlroots/sway. All three support setting the gamma table when using the DRM/KMS backend, weston and kwin through the legacy API, and wlroots through the atomic API. Weston reads an ICC profile using Little CMS and applies the gamma table. kwin is a bit more elaborate, it also uses Little CMS to read an ICC profile, but additionally supports setting the brightness and color temperature. It however only sets a gamma table in the end. Wlroots seems to have an API to set the gamma table, but I haven't seen where sway uses it (I may have missed that though). In any case, there's limited support there for color management.
Inputs would be appreciated on this, for instance with feedback on how X.org and Android handle color management, on how 3D LUTs are implemented on other platforms, or in general on how we would like to use them. I don't mind doing some work in userspace to prototype this, but I won't have the bandwidth to design a completely new framework.
Kieran Bingham (3): drm: rcar-du: cmm: Provide 3D-CLU support drm: Extend color correction to support 3D-CLU drm: rcar-du: kms: Configure the CLU
Laurent Pinchart (1): drm: rcar-du: cmm: Refactor LUT configuration
drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_state_helper.c | 3 + drivers/gpu/drm/drm_atomic_uapi.c | 10 ++ drivers/gpu/drm/drm_color_mgmt.c | 41 ++++++-- drivers/gpu/drm/drm_mode_config.c | 14 +++ drivers/gpu/drm/rcar-du/rcar_cmm.c | 110 +++++++++++++++------- drivers/gpu/drm/rcar-du/rcar_cmm.h | 22 +++-- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 45 ++++++--- include/drm/drm_crtc.h | 9 ++ include/drm/drm_mode_config.h | 11 +++ 10 files changed, 207 insertions(+), 59 deletions(-)
To prepare for CLU support, expend the CMM API exposed to the DU driver to separate the LUT table pointer from the LUT update decision. This will be required, as we will need to update the LUT and CLU independently.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/rcar-du/rcar_cmm.c | 60 ++++++++++++-------------- drivers/gpu/drm/rcar-du/rcar_cmm.h | 19 +++++--- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 22 +++++++--- 3 files changed, 55 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c index 382d53f8a22e..ccc8c8b03bac 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.c +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -42,23 +42,33 @@ static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) iowrite32(data, rcmm->base + reg); }
-/* - * rcar_cmm_lut_write() - Scale the DRM LUT table entries to hardware precision - * and write to the CMM registers - * @rcmm: Pointer to the CMM device - * @drm_lut: Pointer to the DRM LUT table - */ -static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, - const struct drm_color_lut *drm_lut) +static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm, + const struct drm_color_lut *table) { - unsigned int i; + bool enable = !!table;
- for (i = 0; i < CM2_LUT_SIZE; ++i) { - u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16 - | drm_color_lut_extract(drm_lut[i].green, 8) << 8 - | drm_color_lut_extract(drm_lut[i].blue, 8); + if (rcmm->lut.enabled != enable) { + rcar_cmm_write(rcmm, CM2_LUT_CTRL, + enable ? CM2_LUT_CTRL_LUT_EN : 0); + rcmm->lut.enabled = enable; + }
- rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry); + if (table) { + unsigned int i; + + /* + * Scale the DRM LUT table entries to the hardware precision + * and program it. + */ + for (i = 0; i < CM2_LUT_SIZE; ++i) { + const struct drm_color_lut *lut = &table[i]; + + u32 entry = drm_color_lut_extract(lut->red, 8) << 16 + | drm_color_lut_extract(lut->green, 8) << 8 + | drm_color_lut_extract(lut->blue, 8); + + rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry); + } } }
@@ -83,23 +93,8 @@ int rcar_cmm_setup(struct platform_device *pdev, { struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- /* Disable LUT if no table is provided. */ - if (!config->lut.table) { - if (rcmm->lut.enabled) { - rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0); - rcmm->lut.enabled = false; - } - - return 0; - } - - /* Enable LUT and program the new gamma table values. */ - if (!rcmm->lut.enabled) { - rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN); - rcmm->lut.enabled = true; - } - - rcar_cmm_lut_write(rcmm, config->lut.table); + if (config->lut.update) + rcar_cmm_lut_configure(rcmm, config->lut.table);
return 0; } @@ -144,8 +139,7 @@ void rcar_cmm_disable(struct platform_device *pdev) { struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0); - rcmm->lut.enabled = false; + rcar_cmm_lut_configure(rcmm, NULL);
pm_runtime_put(&pdev->dev); } diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h index b5f7ec6db04a..f4b16535ec16 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.h +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -13,16 +13,23 @@ struct drm_color_lut; struct platform_device;
+/** + * struct rcar_cmm_table_config - CMM LUT configuration + * @update: When true, update the LUT configuration. + * @table: Table data. The LUT is enabled if non-NULL, and disabled + * otherwise. The value is ignored if @update is false. + */ +struct rcar_cmm_table_config { + bool update; + struct drm_color_lut *table; +}; + /** * struct rcar_cmm_config - CMM configuration - * - * @lut: 1D-LUT configuration - * @lut.table: 1D-LUT table entries. Disable LUT operations when NULL + * @lut: 1D-LUT configuration */ struct rcar_cmm_config { - struct { - struct drm_color_lut *table; - } lut; + struct rcar_cmm_table_config lut; };
#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 9a099c0fe1d4..426b1870b3cb 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -500,17 +500,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc, return 0; }
-static void rcar_du_cmm_setup(struct drm_crtc *crtc) +static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc, + const struct drm_crtc_state *old_state, + const struct drm_crtc_state *new_state) { - struct drm_property_blob *drm_lut = crtc->state->gamma_lut; - struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); struct rcar_cmm_config cmm_config = {};
if (!rcrtc->cmm) return;
- if (drm_lut) - cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data; + if (!old_state || + !old_state->gamma_lut != !new_state->gamma_lut || + (old_state->gamma_lut && new_state->gamma_lut && + old_state->gamma_lut->base.id != new_state->gamma_lut->base.id)) { + cmm_config.lut.update = true; + cmm_config.lut.table = new_state->gamma_lut + ? new_state->gamma_lut->data : NULL; + }
rcar_cmm_setup(rcrtc->cmm, &cmm_config); } @@ -744,7 +750,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, * after the DU channel has been activated. Investigate the impact * of this restriction on the first displayed frame. */ - rcar_du_cmm_setup(crtc); + rcar_du_cmm_setup(rcrtc, NULL, crtc->state); }
static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, @@ -781,6 +787,8 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state) { + struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state, + crtc); struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
WARN_ON(!crtc->state->enable); @@ -801,7 +809,7 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
/* If the active state changed, we let .atomic_enable handle CMM. */ if (crtc->state->color_mgmt_changed && !crtc->state->active_changed) - rcar_du_cmm_setup(crtc); + rcar_du_cmm_setup(rcrtc, old_state, crtc->state);
if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_atomic_begin(rcrtc);
Hi Laurent,
On 21/12/2020 01:57, Laurent Pinchart wrote:
To prepare for CLU support, expend the CMM API exposed to the DU driver
s/expend/extend/ ...?
to separate the LUT table pointer from the LUT update decision. This will be required, as we will need to update the LUT and CLU independently.
Aha, I see this has changed a little since I originally looked at this, but that's probably a good thing.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/rcar-du/rcar_cmm.c | 60 ++++++++++++-------------- drivers/gpu/drm/rcar-du/rcar_cmm.h | 19 +++++--- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 22 +++++++--- 3 files changed, 55 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c index 382d53f8a22e..ccc8c8b03bac 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.c +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -42,23 +42,33 @@ static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) iowrite32(data, rcmm->base + reg); }
-/*
- rcar_cmm_lut_write() - Scale the DRM LUT table entries to hardware precision
and write to the CMM registers
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
-static void rcar_cmm_lut_write(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm,
const struct drm_color_lut *table){
- unsigned int i;
- bool enable = !!table;
Ahh good, handling both enable and disable in here makes more sense. I like it.
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16| drm_color_lut_extract(drm_lut[i].green, 8) << 8| drm_color_lut_extract(drm_lut[i].blue, 8);
- if (rcmm->lut.enabled != enable) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL,enable ? CM2_LUT_CTRL_LUT_EN : 0);rcmm->lut.enabled = enable;- }
rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);
- if (table) {
unsigned int i;/** Scale the DRM LUT table entries to the hardware precision* and program it.*/for (i = 0; i < CM2_LUT_SIZE; ++i) {const struct drm_color_lut *lut = &table[i];u32 entry = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);}}
@@ -83,23 +93,8 @@ int rcar_cmm_setup(struct platform_device *pdev, { struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- /* Disable LUT if no table is provided. */
- if (!config->lut.table) {
if (rcmm->lut.enabled) {rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);rcmm->lut.enabled = false;}return 0;- }
- /* Enable LUT and program the new gamma table values. */
- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_write(rcmm, config->lut.table);
- if (config->lut.update)
rcar_cmm_lut_configure(rcmm, config->lut.table);
Does something need to reset config->lut.update to false? Or is this structure reset / cleaned on each call?
Never mind, looks like this is always used from a fresh initialised structure in rcar_du_cmm_setup().
return 0; } @@ -144,8 +139,7 @@ void rcar_cmm_disable(struct platform_device *pdev) { struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- rcmm->lut.enabled = false;
rcar_cmm_lut_configure(rcmm, NULL);
pm_runtime_put(&pdev->dev);
} diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h index b5f7ec6db04a..f4b16535ec16 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.h +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -13,16 +13,23 @@ struct drm_color_lut; struct platform_device;
+/**
- struct rcar_cmm_table_config - CMM LUT configuration
- @update: When true, update the LUT configuration.
- @table: Table data. The LUT is enabled if non-NULL, and disabled
- otherwise. The value is ignored if @update is false.
- */
+struct rcar_cmm_table_config {
- bool update;
- struct drm_color_lut *table;
+};
/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut.table: 1D-LUT table entries. Disable LUT operations when NULL
*/
- @lut: 1D-LUT configuration
struct rcar_cmm_config {
- struct {
struct drm_color_lut *table;- } lut;
- struct rcar_cmm_table_config lut;
};
#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 9a099c0fe1d4..426b1870b3cb 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -500,17 +500,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc, return 0; }
-static void rcar_du_cmm_setup(struct drm_crtc *crtc) +static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc,
const struct drm_crtc_state *old_state,const struct drm_crtc_state *new_state){
struct drm_property_blob *drm_lut = crtc->state->gamma_lut;
struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); struct rcar_cmm_config cmm_config = {};
if (!rcrtc->cmm) return;
if (drm_lut)
cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;
- if (!old_state ||
!old_state->gamma_lut != !new_state->gamma_lut ||(old_state->gamma_lut && new_state->gamma_lut &&old_state->gamma_lut->base.id != new_state->gamma_lut->base.id)) {
The conditional looks a bit terse, but it looks like it does the expected.
Everything else looks good to me.
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
cmm_config.lut.update = true;cmm_config.lut.table = new_state->gamma_lut? new_state->gamma_lut->data : NULL;}
rcar_cmm_setup(rcrtc->cmm, &cmm_config);
} @@ -744,7 +750,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, * after the DU channel has been activated. Investigate the impact * of this restriction on the first displayed frame. */
- rcar_du_cmm_setup(crtc);
- rcar_du_cmm_setup(rcrtc, NULL, crtc->state);
}
static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, @@ -781,6 +787,8 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state) {
struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state,
crtc);struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
WARN_ON(!crtc->state->enable);
@@ -801,7 +809,7 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
/* If the active state changed, we let .atomic_enable handle CMM. */ if (crtc->state->color_mgmt_changed && !crtc->state->active_changed)
rcar_du_cmm_setup(crtc);
rcar_du_cmm_setup(rcrtc, old_state, crtc->state);if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_atomic_begin(rcrtc);
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
The CMM module provides a three-dimensional cubic look up table that converts three-color-component data into desired three color components by use of a lookup table.
While the 1D-LUT can only control each of three color components separately, the 3D-CLU can be used for specific color adjustment.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/rcar-du/rcar_cmm.c | 52 ++++++++++++++++++++++++++++-- drivers/gpu/drm/rcar-du/rcar_cmm.h | 11 ++++--- 2 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c index ccc8c8b03bac..9a20728a3534 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.c +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -17,9 +17,18 @@
#define CM2_LUT_CTRL 0x0000 #define CM2_LUT_CTRL_LUT_EN BIT(0) + +#define CM2_CLU_CTRL 0x0100 +#define CM2_CLU_CTRL_CLU_EN BIT(1) +#define CM2_CLU_CTRL_MVS BIT(24) +#define CM2_CLU_CTRL_AAI BIT(28) + #define CM2_LUT_TBL_BASE 0x0600 #define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+#define CM2_CLU_ADDR 0x0a00 +#define CM2_CLU_DATA 0x0a04 + struct rcar_cmm { void __iomem *base;
@@ -30,6 +39,10 @@ struct rcar_cmm { struct { bool enabled; } lut; + + struct { + bool enabled; + } clu; };
static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) @@ -72,13 +85,44 @@ static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm, } }
+static void rcar_cmm_clu_configure(struct rcar_cmm *rcmm, + const struct drm_color_lut *table) +{ + static const u32 cfg = CM2_CLU_CTRL_AAI + | CM2_CLU_CTRL_MVS + | CM2_CLU_CTRL_CLU_EN; + bool enable = !!table; + + if (rcmm->clu.enabled != enable) { + rcar_cmm_write(rcmm, CM2_CLU_CTRL, enable ? cfg : 0); + rcmm->clu.enabled = enable; + } + + if (table) { + unsigned int i; + + /* Utilise CM2_CLU_CTRL_AAI (auto-increment). */ + rcar_cmm_write(rcmm, CM2_CLU_ADDR, 0); + + for (i = 0; i < CM2_CLU_SIZE; ++i) { + const struct drm_color_lut *lut = &table[i]; + + u32 entry = drm_color_lut_extract(lut->red, 8) << 16 + | drm_color_lut_extract(lut->green, 8) << 8 + | drm_color_lut_extract(lut->blue, 8); + + rcar_cmm_write(rcmm, CM2_CLU_DATA, entry); + } + } +} + /* * rcar_cmm_setup() - Configure the CMM unit * @pdev: The platform device associated with the CMM instance * @config: The CMM unit configuration * - * Configure the CMM unit with the given configuration. Currently enabling, - * disabling and programming of the 1-D LUT unit is supported. + * Configure the CMM unit with the given configuration, handling both the + * 1-D LUT and the 3-D CLU. * * As rcar_cmm_setup() accesses the CMM registers the unit should be powered * and its functional clock enabled. To guarantee this, before any call to @@ -96,6 +140,9 @@ int rcar_cmm_setup(struct platform_device *pdev, if (config->lut.update) rcar_cmm_lut_configure(rcmm, config->lut.table);
+ if (config->clu.update) + rcar_cmm_clu_configure(rcmm, config->clu.table); + return 0; } EXPORT_SYMBOL_GPL(rcar_cmm_setup); @@ -140,6 +187,7 @@ void rcar_cmm_disable(struct platform_device *pdev) struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
rcar_cmm_lut_configure(rcmm, NULL); + rcar_cmm_clu_configure(rcmm, NULL);
pm_runtime_put(&pdev->dev); } diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h index f4b16535ec16..35f901158cec 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.h +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -9,14 +9,15 @@ #define __RCAR_CMM_H__
#define CM2_LUT_SIZE 256 +#define CM2_CLU_SIZE (17 * 17 * 17)
struct drm_color_lut; struct platform_device;
/** - * struct rcar_cmm_table_config - CMM LUT configuration - * @update: When true, update the LUT configuration. - * @table: Table data. The LUT is enabled if non-NULL, and disabled + * struct rcar_cmm_table_config - CMM LUT and CLU configuration + * @update: When true, update the LUT or CLU configuration. + * @table: Table data. The LUT or CLU is enabled if non-NULL, and disabled * otherwise. The value is ignored if @update is false. */ struct rcar_cmm_table_config { @@ -26,10 +27,12 @@ struct rcar_cmm_table_config {
/** * struct rcar_cmm_config - CMM configuration - * @lut: 1D-LUT configuration + * @lut: 1D LUT configuration + * @clu: 3D (cubic) LUT configuration */ struct rcar_cmm_config { struct rcar_cmm_table_config lut; + struct rcar_cmm_table_config clu; };
#if IS_ENABLED(CONFIG_DRM_RCAR_CMM)
Hi Laurent,
On 21/12/2020 01:57, Laurent Pinchart wrote:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
The CMM module provides a three-dimensional cubic look up table that converts three-color-component data into desired three color components by use of a lookup table.
While the 1D-LUT can only control each of three color components separately, the 3D-CLU can be used for specific color adjustment.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
For the updates since I wrote the patch:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
drivers/gpu/drm/rcar-du/rcar_cmm.c | 52 ++++++++++++++++++++++++++++-- drivers/gpu/drm/rcar-du/rcar_cmm.h | 11 ++++--- 2 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c index ccc8c8b03bac..9a20728a3534 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.c +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -17,9 +17,18 @@
#define CM2_LUT_CTRL 0x0000 #define CM2_LUT_CTRL_LUT_EN BIT(0)
+#define CM2_CLU_CTRL 0x0100 +#define CM2_CLU_CTRL_CLU_EN BIT(1) +#define CM2_CLU_CTRL_MVS BIT(24) +#define CM2_CLU_CTRL_AAI BIT(28)
#define CM2_LUT_TBL_BASE 0x0600 #define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+#define CM2_CLU_ADDR 0x0a00 +#define CM2_CLU_DATA 0x0a04
struct rcar_cmm { void __iomem *base;
@@ -30,6 +39,10 @@ struct rcar_cmm { struct { bool enabled; } lut;
- struct {
bool enabled;- } clu;
};
static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) @@ -72,13 +85,44 @@ static void rcar_cmm_lut_configure(struct rcar_cmm *rcmm, } }
+static void rcar_cmm_clu_configure(struct rcar_cmm *rcmm,
const struct drm_color_lut *table)+{
- static const u32 cfg = CM2_CLU_CTRL_AAI
| CM2_CLU_CTRL_MVS| CM2_CLU_CTRL_CLU_EN;- bool enable = !!table;
Good, not sure why I was calculating this outside of the call.
- if (rcmm->clu.enabled != enable) {
rcar_cmm_write(rcmm, CM2_CLU_CTRL, enable ? cfg : 0);rcmm->clu.enabled = enable;- }
- if (table) {
unsigned int i;/* Utilise CM2_CLU_CTRL_AAI (auto-increment). */rcar_cmm_write(rcmm, CM2_CLU_ADDR, 0);for (i = 0; i < CM2_CLU_SIZE; ++i) {const struct drm_color_lut *lut = &table[i];u32 entry = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);rcar_cmm_write(rcmm, CM2_CLU_DATA, entry);}- }
+}
/*
- rcar_cmm_setup() - Configure the CMM unit
- @pdev: The platform device associated with the CMM instance
- @config: The CMM unit configuration
- Configure the CMM unit with the given configuration. Currently enabling,
- disabling and programming of the 1-D LUT unit is supported.
- Configure the CMM unit with the given configuration, handling both the
- 1-D LUT and the 3-D CLU.
- As rcar_cmm_setup() accesses the CMM registers the unit should be powered
- and its functional clock enabled. To guarantee this, before any call to
@@ -96,6 +140,9 @@ int rcar_cmm_setup(struct platform_device *pdev, if (config->lut.update) rcar_cmm_lut_configure(rcmm, config->lut.table);
- if (config->clu.update)
rcar_cmm_clu_configure(rcmm, config->clu.table);- return 0;
} EXPORT_SYMBOL_GPL(rcar_cmm_setup); @@ -140,6 +187,7 @@ void rcar_cmm_disable(struct platform_device *pdev) struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
rcar_cmm_lut_configure(rcmm, NULL);
rcar_cmm_clu_configure(rcmm, NULL);
pm_runtime_put(&pdev->dev);
} diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h index f4b16535ec16..35f901158cec 100644 --- a/drivers/gpu/drm/rcar-du/rcar_cmm.h +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -9,14 +9,15 @@ #define __RCAR_CMM_H__
#define CM2_LUT_SIZE 256 +#define CM2_CLU_SIZE (17 * 17 * 17)
struct drm_color_lut; struct platform_device;
/**
- struct rcar_cmm_table_config - CMM LUT configuration
- @update: When true, update the LUT configuration.
- @table: Table data. The LUT is enabled if non-NULL, and disabled
- struct rcar_cmm_table_config - CMM LUT and CLU configuration
- @update: When true, update the LUT or CLU configuration.
*/
- @table: Table data. The LUT or CLU is enabled if non-NULL, and disabled
- otherwise. The value is ignored if @update is false.
struct rcar_cmm_table_config { @@ -26,10 +27,12 @@ struct rcar_cmm_table_config {
/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut: 1D LUT configuration
*/
- @clu: 3D (cubic) LUT configuration
struct rcar_cmm_config { struct rcar_cmm_table_config lut;
- struct rcar_cmm_table_config clu;
};
#if IS_ENABLED(CONFIG_DRM_RCAR_CMM)
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Extend the existing color management properties to support provision of a 3D cubic look up table, allowing for color specific adjustments.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Co-developed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_state_helper.c | 3 ++ drivers/gpu/drm/drm_atomic_uapi.c | 10 ++++++ drivers/gpu/drm/drm_color_mgmt.c | 41 +++++++++++++++++++---- drivers/gpu/drm/drm_mode_config.c | 14 ++++++++ include/drm/drm_crtc.h | 9 +++++ include/drm/drm_mode_config.h | 11 ++++++ 7 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..0f54897d3c8d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob); + replaced |= drm_property_replace_blob(&crtc_state->cubic_lut, NULL); crtc_state->color_mgmt_changed |= replaced;
ret = drm_atomic_commit(state); diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index ddcf5c2c8e6a..61c685b50677 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, drm_property_blob_get(state->ctm); if (state->gamma_lut) drm_property_blob_get(state->gamma_lut); + if (state->cubic_lut) + drm_property_blob_get(state->cubic_lut); state->mode_changed = false; state->active_changed = false; state->planes_changed = false; @@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) drm_property_blob_put(state->degamma_lut); drm_property_blob_put(state->ctm); drm_property_blob_put(state->gamma_lut); + drm_property_blob_put(state->cubic_lut); } EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 268bb69c2e2f..07229acab71c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret; + } else if (property == config->cubic_lut_property) { + ret = drm_atomic_replace_property_blob_from_id(dev, + &state->cubic_lut, + val, + -1, sizeof(struct drm_color_lut), + &replaced); + state->color_mgmt_changed |= replaced; + return ret; } else if (property == config->prop_out_fence_ptr) { s32 __user *fence_ptr = u64_to_user_ptr(val);
@@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0; + else if (property == config->cubic_lut_property) + *val = (state->cubic_lut) ? state->cubic_lut->base.id : 0; else if (property == config->prop_out_fence_ptr) *val = 0; else if (property == crtc->scaling_filter_property) diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..85bbbc8ce8e5 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -33,7 +33,7 @@ /** * DOC: overview * - * Color management or color space adjustments is supported through a set of 5 + * Color management or color space adjustments is supported through a set of 7 * properties on the &drm_crtc object. They are set up by calling * drm_crtc_enable_color_mgmt(). * @@ -60,7 +60,7 @@ * “CTM”: * Blob property to set the current transformation matrix (CTM) apply to * pixel data after the lookup through the degamma LUT and before the - * lookup through the gamma LUT. The data is interpreted as a struct + * lookup through the cubic LUT. The data is interpreted as a struct * &drm_color_ctm. * * Setting this to NULL (blob property value set to 0) means a @@ -68,13 +68,40 @@ * boot-up state too. Drivers can access the blob for the color conversion * matrix through &drm_crtc_state.ctm. * + * ”CUBIC_LUT”: + * Blob property to set the cubic (3D) lookup table performing color + * mapping after the transformation matrix and before the lookup through + * the gamma LUT. Unlike the degamma and gamma LUTs that map color + * components independently, the 3D LUT converts an input color to an + * output color by indexing into the 3D table using the color components + * as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision + * would require too much storage space in the hardware, so the precision + * of the color components is reduced before the look up, and the low + * order bits may be used to interpolate between the nearest points in 3D + * space. + * + * The data is interpreted as an array of &struct drm_color_lut elements. + * Hardware might choose not to use the full precision of the LUT + * elements. + * + * Setting this to NULL (blob property value set to 0) means the output + * color is identical to the input color. This is generally the driver + * boot-up state too. Drivers can access this blob through + * &drm_crtc_state.cubic_lut. + * + * ”CUBIC_LUT_SIZE”: + * Unsigned range property to give the size of the lookup table to be set + * on the CUBIC_LUT property (the size depends on the underlying hardware). + * If drivers support multiple LUT sizes then they should publish the + * largest size, and sub-sample smaller sized LUTs appropriately. + * * “GAMMA_LUT”: * Blob property to set the gamma lookup table (LUT) mapping pixel data - * after the transformation matrix to data sent to the connector. The - * data is interpreted as an array of &struct drm_color_lut elements. - * Hardware might choose not to use the full precision of the LUT elements - * nor use all the elements of the LUT (for example the hardware might - * choose to interpolate between LUT[0] and LUT[4]). + * after the cubic LUT to data sent to the connector. The data is + * interpreted as an array of &struct drm_color_lut elements. Hardware + * might choose not to use the full precision of the LUT elements nor use + * all the elements of the LUT (for example the hardware might choose to + * interpolate between LUT[0] and LUT[4]). * * Setting this to NULL (blob property value set to 0) means a * linear/pass-thru gamma table should be used. This is generally the diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..6c3324f60e7d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.gamma_lut_size_property = prop;
+ prop = drm_property_create(dev, + DRM_MODE_PROP_BLOB, + "CUBIC_LUT", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.cubic_lut_property = prop; + + prop = drm_property_create_range(dev, + DRM_MODE_PROP_IMMUTABLE, + "CUBIC_LUT_SIZE", 0, UINT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.cubic_lut_size_property = prop; + prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, "IN_FORMATS", 0); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5f43d64d2a07..df5cc2239adb 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -288,6 +288,15 @@ struct drm_crtc_state { */ struct drm_property_blob *gamma_lut;
+ /** + * @cubic_lut: + * + * Cubic Lookup table for converting pixel data. See + * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array + * of &struct drm_color_lut. + */ + struct drm_property_blob *cubic_lut; + /** * @target_vblank: * diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index ab424ddd7665..8edb0094e5a7 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -800,6 +800,17 @@ struct drm_mode_config { */ struct drm_property *gamma_lut_size_property;
+ /** + * @cubic_lut_property: Optional CRTC property to set the 3D LUT used to + * convert color spaces. + */ + struct drm_property *cubic_lut_property; + /** + * @cubic_lut_size_property: Optional CRTC property for the size of the + * 3D LUT as supported by the driver (read-only). + */ + struct drm_property *cubic_lut_size_property; + /** * @suggested_x_property: Optional connector property with a hint for * the position of the output on the host's screen.
Hi Laurent,
On 21/12/2020 01:57, Laurent Pinchart wrote:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Extend the existing color management properties to support provision of a 3D cubic look up table, allowing for color specific adjustments.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Co-developed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Again, for the updates since I created the patch:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
A bit of a question on clarifying the added documentation but I don't think it's major.
drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_state_helper.c | 3 ++ drivers/gpu/drm/drm_atomic_uapi.c | 10 ++++++ drivers/gpu/drm/drm_color_mgmt.c | 41 +++++++++++++++++++---- drivers/gpu/drm/drm_mode_config.c | 14 ++++++++ include/drm/drm_crtc.h | 9 +++++ include/drm/drm_mode_config.h | 11 ++++++ 7 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..0f54897d3c8d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
replaced |= drm_property_replace_blob(&crtc_state->cubic_lut, NULL); crtc_state->color_mgmt_changed |= replaced;
ret = drm_atomic_commit(state);
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index ddcf5c2c8e6a..61c685b50677 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, drm_property_blob_get(state->ctm); if (state->gamma_lut) drm_property_blob_get(state->gamma_lut);
- if (state->cubic_lut)
drm_property_blob_get(state->cubic_lut);@@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) drm_property_blob_put(state->degamma_lut); drm_property_blob_put(state->ctm); drm_property_blob_put(state->gamma_lut);
- drm_property_blob_put(state->cubic_lut);
} EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 268bb69c2e2f..07229acab71c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
- } else if (property == config->cubic_lut_property) {
ret = drm_atomic_replace_property_blob_from_id(dev,&state->cubic_lut,val,-1, sizeof(struct drm_color_lut),&replaced);state->color_mgmt_changed |= replaced;return ret;@@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
- else if (property == config->cubic_lut_property)
*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..85bbbc8ce8e5 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -33,7 +33,7 @@ /**
- DOC: overview
- Color management or color space adjustments is supported through a set of 5
- Color management or color space adjustments is supported through a set of 7
- properties on the &drm_crtc object. They are set up by calling
- drm_crtc_enable_color_mgmt().
@@ -60,7 +60,7 @@
- “CTM”:
- Blob property to set the current transformation matrix (CTM) apply to
- pixel data after the lookup through the degamma LUT and before the
- lookup through the gamma LUT. The data is interpreted as a struct
- lookup through the cubic LUT. The data is interpreted as a struct
- &drm_color_ctm.
- Setting this to NULL (blob property value set to 0) means a
@@ -68,13 +68,40 @@
- boot-up state too. Drivers can access the blob for the color conversion
- matrix through &drm_crtc_state.ctm.
- ”CUBIC_LUT”:
- Blob property to set the cubic (3D) lookup table performing color
- mapping after the transformation matrix and before the lookup through
- the gamma LUT. Unlike the degamma and gamma LUTs that map color
- components independently, the 3D LUT converts an input color to an
- output color by indexing into the 3D table using the color components
- as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
- would require too much storage space in the hardware, so the precision
This sentence is a bit confusing. What bit depth is actually used, is it mandated to a specific subsampling? Or restricted to 8-bit....
- of the color components is reduced before the look up, and the low
- order bits may be used to interpolate between the nearest points in 3D
- space.
- The data is interpreted as an array of &struct drm_color_lut elements.
- Hardware might choose not to use the full precision of the LUT
- elements.
Is the table already reduced precision though ?
- Setting this to NULL (blob property value set to 0) means the output
- color is identical to the input color. This is generally the driver
- boot-up state too. Drivers can access this blob through
- &drm_crtc_state.cubic_lut.
- ”CUBIC_LUT_SIZE”:
- Unsigned range property to give the size of the lookup table to be set
- on the CUBIC_LUT property (the size depends on the underlying hardware).
- If drivers support multiple LUT sizes then they should publish the
- largest size, and sub-sample smaller sized LUTs appropriately.
- “GAMMA_LUT”:
- Blob property to set the gamma lookup table (LUT) mapping pixel data
- after the transformation matrix to data sent to the connector. The
- data is interpreted as an array of &struct drm_color_lut elements.
- Hardware might choose not to use the full precision of the LUT elements
- nor use all the elements of the LUT (for example the hardware might
- choose to interpolate between LUT[0] and LUT[4]).
- after the cubic LUT to data sent to the connector. The data is
- interpreted as an array of &struct drm_color_lut elements. Hardware
- might choose not to use the full precision of the LUT elements nor use
- all the elements of the LUT (for example the hardware might choose to
- interpolate between LUT[0] and LUT[4]).
- Setting this to NULL (blob property value set to 0) means a
- linear/pass-thru gamma table should be used. This is generally the
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..6c3324f60e7d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.gamma_lut_size_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB,"CUBIC_LUT", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cubic_lut_property = prop;
- prop = drm_property_create_range(dev,
DRM_MODE_PROP_IMMUTABLE,"CUBIC_LUT_SIZE", 0, UINT_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.cubic_lut_size_property = prop;
- prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, "IN_FORMATS", 0);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5f43d64d2a07..df5cc2239adb 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -288,6 +288,15 @@ struct drm_crtc_state { */ struct drm_property_blob *gamma_lut;
- /**
* @cubic_lut:** Cubic Lookup table for converting pixel data. See* drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array* of &struct drm_color_lut.*/- struct drm_property_blob *cubic_lut;
- /**
- @target_vblank:
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index ab424ddd7665..8edb0094e5a7 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -800,6 +800,17 @@ struct drm_mode_config { */ struct drm_property *gamma_lut_size_property;
- /**
* @cubic_lut_property: Optional CRTC property to set the 3D LUT used to* convert color spaces.*/- struct drm_property *cubic_lut_property;
- /**
* @cubic_lut_size_property: Optional CRTC property for the size of the* 3D LUT as supported by the driver (read-only).*/- struct drm_property *cubic_lut_size_property;
- /**
- @suggested_x_property: Optional connector property with a hint for
- the position of the output on the host's screen.
Hi Kieran,
On Mon, Dec 21, 2020 at 02:09:09PM +0000, Kieran Bingham wrote:
On 21/12/2020 01:57, Laurent Pinchart wrote:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Extend the existing color management properties to support provision of a 3D cubic look up table, allowing for color specific adjustments.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Co-developed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Again, for the updates since I created the patch:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
A bit of a question on clarifying the added documentation but I don't think it's major.
drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_state_helper.c | 3 ++ drivers/gpu/drm/drm_atomic_uapi.c | 10 ++++++ drivers/gpu/drm/drm_color_mgmt.c | 41 +++++++++++++++++++---- drivers/gpu/drm/drm_mode_config.c | 14 ++++++++ include/drm/drm_crtc.h | 9 +++++ include/drm/drm_mode_config.h | 11 ++++++ 7 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..0f54897d3c8d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
replaced |= drm_property_replace_blob(&crtc_state->cubic_lut, NULL); crtc_state->color_mgmt_changed |= replaced;
ret = drm_atomic_commit(state);
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index ddcf5c2c8e6a..61c685b50677 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, drm_property_blob_get(state->ctm); if (state->gamma_lut) drm_property_blob_get(state->gamma_lut);
- if (state->cubic_lut)
drm_property_blob_get(state->cubic_lut);@@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) drm_property_blob_put(state->degamma_lut); drm_property_blob_put(state->ctm); drm_property_blob_put(state->gamma_lut);
- drm_property_blob_put(state->cubic_lut);
} EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 268bb69c2e2f..07229acab71c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
- } else if (property == config->cubic_lut_property) {
ret = drm_atomic_replace_property_blob_from_id(dev,&state->cubic_lut,val,-1, sizeof(struct drm_color_lut),&replaced);state->color_mgmt_changed |= replaced;return ret;@@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
- else if (property == config->cubic_lut_property)
*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..85bbbc8ce8e5 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -33,7 +33,7 @@ /**
- DOC: overview
- Color management or color space adjustments is supported through a set of 5
- Color management or color space adjustments is supported through a set of 7
- properties on the &drm_crtc object. They are set up by calling
- drm_crtc_enable_color_mgmt().
@@ -60,7 +60,7 @@
- “CTM”:
- Blob property to set the current transformation matrix (CTM) apply to
- pixel data after the lookup through the degamma LUT and before the
- lookup through the gamma LUT. The data is interpreted as a struct
- lookup through the cubic LUT. The data is interpreted as a struct
- &drm_color_ctm.
- Setting this to NULL (blob property value set to 0) means a
@@ -68,13 +68,40 @@
- boot-up state too. Drivers can access the blob for the color conversion
- matrix through &drm_crtc_state.ctm.
- ”CUBIC_LUT”:
- Blob property to set the cubic (3D) lookup table performing color
- mapping after the transformation matrix and before the lookup through
- the gamma LUT. Unlike the degamma and gamma LUTs that map color
- components independently, the 3D LUT converts an input color to an
- output color by indexing into the 3D table using the color components
- as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
- would require too much storage space in the hardware, so the precision
This sentence is a bit confusing. What bit depth is actually used, is it mandated to a specific subsampling? Or restricted to 8-bit....
We're both confused then, because I don't think I understand the question :-)
- of the color components is reduced before the look up, and the low
- order bits may be used to interpolate between the nearest points in 3D
- space.
- The data is interpreted as an array of &struct drm_color_lut elements.
- Hardware might choose not to use the full precision of the LUT
- elements.
Is the table already reduced precision though ?
drm_color_lut has 16-bit precision. The precision of the table on the hardware side varies.
- Setting this to NULL (blob property value set to 0) means the output
- color is identical to the input color. This is generally the driver
- boot-up state too. Drivers can access this blob through
- &drm_crtc_state.cubic_lut.
- ”CUBIC_LUT_SIZE”:
- Unsigned range property to give the size of the lookup table to be set
- on the CUBIC_LUT property (the size depends on the underlying hardware).
- If drivers support multiple LUT sizes then they should publish the
- largest size, and sub-sample smaller sized LUTs appropriately.
- “GAMMA_LUT”:
- Blob property to set the gamma lookup table (LUT) mapping pixel data
- after the transformation matrix to data sent to the connector. The
- data is interpreted as an array of &struct drm_color_lut elements.
- Hardware might choose not to use the full precision of the LUT elements
- nor use all the elements of the LUT (for example the hardware might
- choose to interpolate between LUT[0] and LUT[4]).
- after the cubic LUT to data sent to the connector. The data is
- interpreted as an array of &struct drm_color_lut elements. Hardware
- might choose not to use the full precision of the LUT elements nor use
- all the elements of the LUT (for example the hardware might choose to
- interpolate between LUT[0] and LUT[4]).
- Setting this to NULL (blob property value set to 0) means a
- linear/pass-thru gamma table should be used. This is generally the
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..6c3324f60e7d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.gamma_lut_size_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB,"CUBIC_LUT", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cubic_lut_property = prop;
- prop = drm_property_create_range(dev,
DRM_MODE_PROP_IMMUTABLE,"CUBIC_LUT_SIZE", 0, UINT_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.cubic_lut_size_property = prop;
- prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, "IN_FORMATS", 0);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5f43d64d2a07..df5cc2239adb 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -288,6 +288,15 @@ struct drm_crtc_state { */ struct drm_property_blob *gamma_lut;
- /**
* @cubic_lut:** Cubic Lookup table for converting pixel data. See* drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array* of &struct drm_color_lut.*/- struct drm_property_blob *cubic_lut;
- /**
- @target_vblank:
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index ab424ddd7665..8edb0094e5a7 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -800,6 +800,17 @@ struct drm_mode_config { */ struct drm_property *gamma_lut_size_property;
- /**
* @cubic_lut_property: Optional CRTC property to set the 3D LUT used to* convert color spaces.*/- struct drm_property *cubic_lut_property;
- /**
* @cubic_lut_size_property: Optional CRTC property for the size of the* 3D LUT as supported by the driver (read-only).*/- struct drm_property *cubic_lut_size_property;
- /**
- @suggested_x_property: Optional connector property with a hint for
- the position of the output on the host's screen.
On Mon, Dec 21, 2020 at 2:57 AM Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Extend the existing color management properties to support provision of a 3D cubic look up table, allowing for color specific adjustments.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Co-developed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Assuming this is meant for merging to upstream: Needs igt + open userspace in a compositor that cares enough. -Daniel
drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_state_helper.c | 3 ++ drivers/gpu/drm/drm_atomic_uapi.c | 10 ++++++ drivers/gpu/drm/drm_color_mgmt.c | 41 +++++++++++++++++++---- drivers/gpu/drm/drm_mode_config.c | 14 ++++++++ include/drm/drm_crtc.h | 9 +++++ include/drm/drm_mode_config.h | 11 ++++++ 7 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..0f54897d3c8d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
replaced |= drm_property_replace_blob(&crtc_state->cubic_lut, NULL); crtc_state->color_mgmt_changed |= replaced; ret = drm_atomic_commit(state);diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index ddcf5c2c8e6a..61c685b50677 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, drm_property_blob_get(state->ctm); if (state->gamma_lut) drm_property_blob_get(state->gamma_lut);
if (state->cubic_lut)drm_property_blob_get(state->cubic_lut); state->mode_changed = false; state->active_changed = false; state->planes_changed = false;@@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) drm_property_blob_put(state->degamma_lut); drm_property_blob_put(state->ctm); drm_property_blob_put(state->gamma_lut);
drm_property_blob_put(state->cubic_lut);} EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 268bb69c2e2f..07229acab71c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
} else if (property == config->cubic_lut_property) {ret = drm_atomic_replace_property_blob_from_id(dev,&state->cubic_lut,val,-1, sizeof(struct drm_color_lut),&replaced);state->color_mgmt_changed |= replaced;return ret; } else if (property == config->prop_out_fence_ptr) { s32 __user *fence_ptr = u64_to_user_ptr(val);@@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
else if (property == config->cubic_lut_property)*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0; else if (property == config->prop_out_fence_ptr) *val = 0; else if (property == crtc->scaling_filter_property)diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..85bbbc8ce8e5 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -33,7 +33,7 @@ /**
- DOC: overview
- Color management or color space adjustments is supported through a set of 5
- Color management or color space adjustments is supported through a set of 7
- properties on the &drm_crtc object. They are set up by calling
- drm_crtc_enable_color_mgmt().
@@ -60,7 +60,7 @@
- “CTM”:
Blob property to set the current transformation matrix (CTM) apply topixel data after the lookup through the degamma LUT and before the
lookup through the gamma LUT. The data is interpreted as a struct
lookup through the cubic LUT. The data is interpreted as a struct&drm_color_ctm.Setting this to NULL (blob property value set to 0) means a@@ -68,13 +68,40 @@
boot-up state too. Drivers can access the blob for the color conversionmatrix through &drm_crtc_state.ctm.
- ”CUBIC_LUT”:
Blob property to set the cubic (3D) lookup table performing color
mapping after the transformation matrix and before the lookup through
the gamma LUT. Unlike the degamma and gamma LUTs that map color
components independently, the 3D LUT converts an input color to an
output color by indexing into the 3D table using the color components
as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
would require too much storage space in the hardware, so the precision
of the color components is reduced before the look up, and the low
order bits may be used to interpolate between the nearest points in 3D
space.
The data is interpreted as an array of &struct drm_color_lut elements.
Hardware might choose not to use the full precision of the LUT
elements.
Setting this to NULL (blob property value set to 0) means the output
color is identical to the input color. This is generally the driver
boot-up state too. Drivers can access this blob through
&drm_crtc_state.cubic_lut.
- ”CUBIC_LUT_SIZE”:
Unsigned range property to give the size of the lookup table to be set
on the CUBIC_LUT property (the size depends on the underlying hardware).
If drivers support multiple LUT sizes then they should publish the
largest size, and sub-sample smaller sized LUTs appropriately.
- “GAMMA_LUT”:
Blob property to set the gamma lookup table (LUT) mapping pixel data
after the transformation matrix to data sent to the connector. The
data is interpreted as an array of &struct drm_color_lut elements.
Hardware might choose not to use the full precision of the LUT elements
nor use all the elements of the LUT (for example the hardware might
choose to interpolate between LUT[0] and LUT[4]).
after the cubic LUT to data sent to the connector. The data is
interpreted as an array of &struct drm_color_lut elements. Hardware
might choose not to use the full precision of the LUT elements nor use
all the elements of the LUT (for example the hardware might choose to
interpolate between LUT[0] and LUT[4]).Setting this to NULL (blob property value set to 0) means alinear/pass-thru gamma table should be used. This is generally thediff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..6c3324f60e7d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.gamma_lut_size_property = prop;
prop = drm_property_create(dev,DRM_MODE_PROP_BLOB,"CUBIC_LUT", 0);if (!prop)return -ENOMEM;dev->mode_config.cubic_lut_property = prop;prop = drm_property_create_range(dev,DRM_MODE_PROP_IMMUTABLE,"CUBIC_LUT_SIZE", 0, UINT_MAX);if (!prop)return -ENOMEM;dev->mode_config.cubic_lut_size_property = prop;prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, "IN_FORMATS", 0);diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5f43d64d2a07..df5cc2239adb 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -288,6 +288,15 @@ struct drm_crtc_state { */ struct drm_property_blob *gamma_lut;
/*** @cubic_lut:** Cubic Lookup table for converting pixel data. See* drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array* of &struct drm_color_lut.*/struct drm_property_blob *cubic_lut;/** * @target_vblank: *diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index ab424ddd7665..8edb0094e5a7 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -800,6 +800,17 @@ struct drm_mode_config { */ struct drm_property *gamma_lut_size_property;
/*** @cubic_lut_property: Optional CRTC property to set the 3D LUT used to* convert color spaces.*/struct drm_property *cubic_lut_property;/*** @cubic_lut_size_property: Optional CRTC property for the size of the* 3D LUT as supported by the driver (read-only).*/struct drm_property *cubic_lut_size_property;/** * @suggested_x_property: Optional connector property with a hint for * the position of the output on the host's screen.-- Regards,
Laurent Pinchart
Hi Daniel,
On Mon, Dec 21, 2020 at 07:36:22PM +0100, Daniel Vetter wrote:
On Mon, Dec 21, 2020 at 2:57 AM Laurent Pinchart wrote:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Extend the existing color management properties to support provision of a 3D cubic look up table, allowing for color specific adjustments.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Co-developed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Assuming this is meant for merging to upstream: Needs igt + open userspace in a compositor that cares enough.
Please see the cover letter :-) Feedback on what an appropriate userspace would be would be appreciated.
drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_state_helper.c | 3 ++ drivers/gpu/drm/drm_atomic_uapi.c | 10 ++++++ drivers/gpu/drm/drm_color_mgmt.c | 41 +++++++++++++++++++---- drivers/gpu/drm/drm_mode_config.c | 14 ++++++++ include/drm/drm_crtc.h | 9 +++++ include/drm/drm_mode_config.h | 11 ++++++ 7 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..0f54897d3c8d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
replaced |= drm_property_replace_blob(&crtc_state->cubic_lut, NULL); crtc_state->color_mgmt_changed |= replaced; ret = drm_atomic_commit(state);diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index ddcf5c2c8e6a..61c685b50677 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, drm_property_blob_get(state->ctm); if (state->gamma_lut) drm_property_blob_get(state->gamma_lut);
if (state->cubic_lut)drm_property_blob_get(state->cubic_lut); state->mode_changed = false; state->active_changed = false; state->planes_changed = false;@@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) drm_property_blob_put(state->degamma_lut); drm_property_blob_put(state->ctm); drm_property_blob_put(state->gamma_lut);
drm_property_blob_put(state->cubic_lut);} EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 268bb69c2e2f..07229acab71c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
} else if (property == config->cubic_lut_property) {ret = drm_atomic_replace_property_blob_from_id(dev,&state->cubic_lut,val,-1, sizeof(struct drm_color_lut),&replaced);state->color_mgmt_changed |= replaced;return ret; } else if (property == config->prop_out_fence_ptr) { s32 __user *fence_ptr = u64_to_user_ptr(val);@@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
else if (property == config->cubic_lut_property)*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0; else if (property == config->prop_out_fence_ptr) *val = 0; else if (property == crtc->scaling_filter_property)diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..85bbbc8ce8e5 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -33,7 +33,7 @@ /**
- DOC: overview
- Color management or color space adjustments is supported through a set of 5
- Color management or color space adjustments is supported through a set of 7
- properties on the &drm_crtc object. They are set up by calling
- drm_crtc_enable_color_mgmt().
@@ -60,7 +60,7 @@
- “CTM”:
Blob property to set the current transformation matrix (CTM) apply topixel data after the lookup through the degamma LUT and before the
lookup through the gamma LUT. The data is interpreted as a struct
lookup through the cubic LUT. The data is interpreted as a struct&drm_color_ctm.Setting this to NULL (blob property value set to 0) means a@@ -68,13 +68,40 @@
boot-up state too. Drivers can access the blob for the color conversionmatrix through &drm_crtc_state.ctm.
- ”CUBIC_LUT”:
Blob property to set the cubic (3D) lookup table performing color
mapping after the transformation matrix and before the lookup through
the gamma LUT. Unlike the degamma and gamma LUTs that map color
components independently, the 3D LUT converts an input color to an
output color by indexing into the 3D table using the color components
as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
would require too much storage space in the hardware, so the precision
of the color components is reduced before the look up, and the low
order bits may be used to interpolate between the nearest points in 3D
space.
The data is interpreted as an array of &struct drm_color_lut elements.
Hardware might choose not to use the full precision of the LUT
elements.
Setting this to NULL (blob property value set to 0) means the output
color is identical to the input color. This is generally the driver
boot-up state too. Drivers can access this blob through
&drm_crtc_state.cubic_lut.
- ”CUBIC_LUT_SIZE”:
Unsigned range property to give the size of the lookup table to be set
on the CUBIC_LUT property (the size depends on the underlying hardware).
If drivers support multiple LUT sizes then they should publish the
largest size, and sub-sample smaller sized LUTs appropriately.
- “GAMMA_LUT”:
Blob property to set the gamma lookup table (LUT) mapping pixel data
after the transformation matrix to data sent to the connector. The
data is interpreted as an array of &struct drm_color_lut elements.
Hardware might choose not to use the full precision of the LUT elements
nor use all the elements of the LUT (for example the hardware might
choose to interpolate between LUT[0] and LUT[4]).
after the cubic LUT to data sent to the connector. The data is
interpreted as an array of &struct drm_color_lut elements. Hardware
might choose not to use the full precision of the LUT elements nor use
all the elements of the LUT (for example the hardware might choose to
interpolate between LUT[0] and LUT[4]).Setting this to NULL (blob property value set to 0) means alinear/pass-thru gamma table should be used. This is generally thediff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..6c3324f60e7d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.gamma_lut_size_property = prop;
prop = drm_property_create(dev,DRM_MODE_PROP_BLOB,"CUBIC_LUT", 0);if (!prop)return -ENOMEM;dev->mode_config.cubic_lut_property = prop;prop = drm_property_create_range(dev,DRM_MODE_PROP_IMMUTABLE,"CUBIC_LUT_SIZE", 0, UINT_MAX);if (!prop)return -ENOMEM;dev->mode_config.cubic_lut_size_property = prop;prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, "IN_FORMATS", 0);diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5f43d64d2a07..df5cc2239adb 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -288,6 +288,15 @@ struct drm_crtc_state { */ struct drm_property_blob *gamma_lut;
/*** @cubic_lut:** Cubic Lookup table for converting pixel data. See* drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array* of &struct drm_color_lut.*/struct drm_property_blob *cubic_lut;/** * @target_vblank: *diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index ab424ddd7665..8edb0094e5a7 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -800,6 +800,17 @@ struct drm_mode_config { */ struct drm_property *gamma_lut_size_property;
/*** @cubic_lut_property: Optional CRTC property to set the 3D LUT used to* convert color spaces.*/struct drm_property *cubic_lut_property;/*** @cubic_lut_size_property: Optional CRTC property for the size of the* 3D LUT as supported by the driver (read-only).*/struct drm_property *cubic_lut_size_property;/** * @suggested_x_property: Optional connector property with a hint for * the position of the output on the host's screen.
On Mon, Dec 21, 2020 at 08:38:44PM +0200, Laurent Pinchart wrote:
Hi Daniel,
On Mon, Dec 21, 2020 at 07:36:22PM +0100, Daniel Vetter wrote:
On Mon, Dec 21, 2020 at 2:57 AM Laurent Pinchart wrote:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Extend the existing color management properties to support provision of a 3D cubic look up table, allowing for color specific adjustments.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Co-developed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Assuming this is meant for merging to upstream: Needs igt + open userspace in a compositor that cares enough.
Please see the cover letter :-) Feedback on what an appropriate userspace would be would be appreciated.
Oops sorry.
Wrt userspace CrOS was the one originally used to merge this, they do the full ICC color correction for their panels with degamm + ctm + lut. So if you somewhat care about that (or can make google care about 3d/cube luts) then that might be simplest.
The Kodi people also care quite a lot about color correction stuff, and iirc some of the per-plane color management is being prototyped with code (so that the movie and the UI both have their correct colors).
Otherwise I guess weston is the closest with real color management, but nothing merged yet. -Daniel
drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_state_helper.c | 3 ++ drivers/gpu/drm/drm_atomic_uapi.c | 10 ++++++ drivers/gpu/drm/drm_color_mgmt.c | 41 +++++++++++++++++++---- drivers/gpu/drm/drm_mode_config.c | 14 ++++++++ include/drm/drm_crtc.h | 9 +++++ include/drm/drm_mode_config.h | 11 ++++++ 7 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..0f54897d3c8d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
replaced |= drm_property_replace_blob(&crtc_state->cubic_lut, NULL); crtc_state->color_mgmt_changed |= replaced; ret = drm_atomic_commit(state);diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index ddcf5c2c8e6a..61c685b50677 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, drm_property_blob_get(state->ctm); if (state->gamma_lut) drm_property_blob_get(state->gamma_lut);
if (state->cubic_lut)drm_property_blob_get(state->cubic_lut); state->mode_changed = false; state->active_changed = false; state->planes_changed = false;@@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) drm_property_blob_put(state->degamma_lut); drm_property_blob_put(state->ctm); drm_property_blob_put(state->gamma_lut);
drm_property_blob_put(state->cubic_lut);} EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 268bb69c2e2f..07229acab71c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
} else if (property == config->cubic_lut_property) {ret = drm_atomic_replace_property_blob_from_id(dev,&state->cubic_lut,val,-1, sizeof(struct drm_color_lut),&replaced);state->color_mgmt_changed |= replaced;return ret; } else if (property == config->prop_out_fence_ptr) { s32 __user *fence_ptr = u64_to_user_ptr(val);@@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
else if (property == config->cubic_lut_property)*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0; else if (property == config->prop_out_fence_ptr) *val = 0; else if (property == crtc->scaling_filter_property)diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..85bbbc8ce8e5 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -33,7 +33,7 @@ /**
- DOC: overview
- Color management or color space adjustments is supported through a set of 5
- Color management or color space adjustments is supported through a set of 7
- properties on the &drm_crtc object. They are set up by calling
- drm_crtc_enable_color_mgmt().
@@ -60,7 +60,7 @@
- “CTM”:
Blob property to set the current transformation matrix (CTM) apply topixel data after the lookup through the degamma LUT and before the
lookup through the gamma LUT. The data is interpreted as a struct
lookup through the cubic LUT. The data is interpreted as a struct&drm_color_ctm.Setting this to NULL (blob property value set to 0) means a@@ -68,13 +68,40 @@
boot-up state too. Drivers can access the blob for the color conversionmatrix through &drm_crtc_state.ctm.
- ”CUBIC_LUT”:
Blob property to set the cubic (3D) lookup table performing color
mapping after the transformation matrix and before the lookup through
the gamma LUT. Unlike the degamma and gamma LUTs that map color
components independently, the 3D LUT converts an input color to an
output color by indexing into the 3D table using the color components
as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
would require too much storage space in the hardware, so the precision
of the color components is reduced before the look up, and the low
order bits may be used to interpolate between the nearest points in 3D
space.
The data is interpreted as an array of &struct drm_color_lut elements.
Hardware might choose not to use the full precision of the LUT
elements.
Setting this to NULL (blob property value set to 0) means the output
color is identical to the input color. This is generally the driver
boot-up state too. Drivers can access this blob through
&drm_crtc_state.cubic_lut.
- ”CUBIC_LUT_SIZE”:
Unsigned range property to give the size of the lookup table to be set
on the CUBIC_LUT property (the size depends on the underlying hardware).
If drivers support multiple LUT sizes then they should publish the
largest size, and sub-sample smaller sized LUTs appropriately.
- “GAMMA_LUT”:
Blob property to set the gamma lookup table (LUT) mapping pixel data
after the transformation matrix to data sent to the connector. The
data is interpreted as an array of &struct drm_color_lut elements.
Hardware might choose not to use the full precision of the LUT elements
nor use all the elements of the LUT (for example the hardware might
choose to interpolate between LUT[0] and LUT[4]).
after the cubic LUT to data sent to the connector. The data is
interpreted as an array of &struct drm_color_lut elements. Hardware
might choose not to use the full precision of the LUT elements nor use
all the elements of the LUT (for example the hardware might choose to
interpolate between LUT[0] and LUT[4]).Setting this to NULL (blob property value set to 0) means alinear/pass-thru gamma table should be used. This is generally thediff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..6c3324f60e7d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.gamma_lut_size_property = prop;
prop = drm_property_create(dev,DRM_MODE_PROP_BLOB,"CUBIC_LUT", 0);if (!prop)return -ENOMEM;dev->mode_config.cubic_lut_property = prop;prop = drm_property_create_range(dev,DRM_MODE_PROP_IMMUTABLE,"CUBIC_LUT_SIZE", 0, UINT_MAX);if (!prop)return -ENOMEM;dev->mode_config.cubic_lut_size_property = prop;prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, "IN_FORMATS", 0);diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5f43d64d2a07..df5cc2239adb 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -288,6 +288,15 @@ struct drm_crtc_state { */ struct drm_property_blob *gamma_lut;
/*** @cubic_lut:** Cubic Lookup table for converting pixel data. See* drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array* of &struct drm_color_lut.*/struct drm_property_blob *cubic_lut;/** * @target_vblank: *diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index ab424ddd7665..8edb0094e5a7 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -800,6 +800,17 @@ struct drm_mode_config { */ struct drm_property *gamma_lut_size_property;
/*** @cubic_lut_property: Optional CRTC property to set the 3D LUT used to* convert color spaces.*/struct drm_property *cubic_lut_property;/*** @cubic_lut_size_property: Optional CRTC property for the size of the* 3D LUT as supported by the driver (read-only).*/struct drm_property *cubic_lut_size_property;/** * @suggested_x_property: Optional connector property with a hint for * the position of the output on the host's screen.-- Regards,
Laurent Pinchart
On Mon, Dec 21, 2020 at 03:57:29AM +0200, Laurent Pinchart wrote:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Extend the existing color management properties to support provision of a 3D cubic look up table, allowing for color specific adjustments.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Co-developed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
FYI I've got a WIP 3D LUT implementation for i915 here: git://github.com/vsyrjala/linux.git 3dlut
I named the prop GAMMA_LUT_3D to indicate its position in the pipeline (on our hw it's postioned after the normal gamma LUT). Alas no userspace for it yet, so can't really do anything more with it for the time being.
Rudimentary tests also available here: git://github.com/vsyrjala/intel-gpu-tools.git 3dlut
Hi Laurent and Kieran,
Do you forget to attach cubic_lut_property with crtc?
drm_object_attach_property(&crtc->base, config->cubic_lut_property, cubic_lut_size);
在 2020/12/21 9:57, Laurent Pinchart 写道:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Extend the existing color management properties to support provision of a 3D cubic look up table, allowing for color specific adjustments.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Co-developed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_state_helper.c | 3 ++ drivers/gpu/drm/drm_atomic_uapi.c | 10 ++++++ drivers/gpu/drm/drm_color_mgmt.c | 41 +++++++++++++++++++---- drivers/gpu/drm/drm_mode_config.c | 14 ++++++++ include/drm/drm_crtc.h | 9 +++++ include/drm/drm_mode_config.h | 11 ++++++ 7 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..0f54897d3c8d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
replaced |= drm_property_replace_blob(&crtc_state->cubic_lut, NULL); crtc_state->color_mgmt_changed |= replaced;
ret = drm_atomic_commit(state);
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index ddcf5c2c8e6a..61c685b50677 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, drm_property_blob_get(state->ctm); if (state->gamma_lut) drm_property_blob_get(state->gamma_lut);
- if (state->cubic_lut)
drm_property_blob_get(state->cubic_lut);@@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) drm_property_blob_put(state->degamma_lut); drm_property_blob_put(state->ctm); drm_property_blob_put(state->gamma_lut);
- drm_property_blob_put(state->cubic_lut); } EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 268bb69c2e2f..07229acab71c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
- } else if (property == config->cubic_lut_property) {
ret = drm_atomic_replace_property_blob_from_id(dev,&state->cubic_lut,val,-1, sizeof(struct drm_color_lut),&replaced);state->color_mgmt_changed |= replaced;return ret;@@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
- else if (property == config->cubic_lut_property)
*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..85bbbc8ce8e5 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -33,7 +33,7 @@ /**
- DOC: overview
- Color management or color space adjustments is supported through a set of 5
- Color management or color space adjustments is supported through a set of 7
- properties on the &drm_crtc object. They are set up by calling
- drm_crtc_enable_color_mgmt().
@@ -60,7 +60,7 @@
- “CTM”:
- Blob property to set the current transformation matrix (CTM) apply to
- pixel data after the lookup through the degamma LUT and before the
- lookup through the gamma LUT. The data is interpreted as a struct
- lookup through the cubic LUT. The data is interpreted as a struct
- &drm_color_ctm.
- Setting this to NULL (blob property value set to 0) means a
@@ -68,13 +68,40 @@
- boot-up state too. Drivers can access the blob for the color conversion
- matrix through &drm_crtc_state.ctm.
- ”CUBIC_LUT”:
- Blob property to set the cubic (3D) lookup table performing color
- mapping after the transformation matrix and before the lookup through
- the gamma LUT. Unlike the degamma and gamma LUTs that map color
- components independently, the 3D LUT converts an input color to an
- output color by indexing into the 3D table using the color components
- as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
- would require too much storage space in the hardware, so the precision
- of the color components is reduced before the look up, and the low
- order bits may be used to interpolate between the nearest points in 3D
- space.
- The data is interpreted as an array of &struct drm_color_lut elements.
- Hardware might choose not to use the full precision of the LUT
- elements.
- Setting this to NULL (blob property value set to 0) means the output
- color is identical to the input color. This is generally the driver
- boot-up state too. Drivers can access this blob through
- &drm_crtc_state.cubic_lut.
- ”CUBIC_LUT_SIZE”:
- Unsigned range property to give the size of the lookup table to be set
- on the CUBIC_LUT property (the size depends on the underlying hardware).
- If drivers support multiple LUT sizes then they should publish the
- largest size, and sub-sample smaller sized LUTs appropriately.
- “GAMMA_LUT”:
- Blob property to set the gamma lookup table (LUT) mapping pixel data
- after the transformation matrix to data sent to the connector. The
- data is interpreted as an array of &struct drm_color_lut elements.
- Hardware might choose not to use the full precision of the LUT elements
- nor use all the elements of the LUT (for example the hardware might
- choose to interpolate between LUT[0] and LUT[4]).
- after the cubic LUT to data sent to the connector. The data is
- interpreted as an array of &struct drm_color_lut elements. Hardware
- might choose not to use the full precision of the LUT elements nor use
- all the elements of the LUT (for example the hardware might choose to
- interpolate between LUT[0] and LUT[4]).
- Setting this to NULL (blob property value set to 0) means a
- linear/pass-thru gamma table should be used. This is generally the
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..6c3324f60e7d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.gamma_lut_size_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB,"CUBIC_LUT", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cubic_lut_property = prop;
- prop = drm_property_create_range(dev,
DRM_MODE_PROP_IMMUTABLE,"CUBIC_LUT_SIZE", 0, UINT_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.cubic_lut_size_property = prop;
- prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, "IN_FORMATS", 0);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5f43d64d2a07..df5cc2239adb 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -288,6 +288,15 @@ struct drm_crtc_state { */ struct drm_property_blob *gamma_lut;
- /**
* @cubic_lut:** Cubic Lookup table for converting pixel data. See* drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array* of &struct drm_color_lut.*/- struct drm_property_blob *cubic_lut;
- /**
- @target_vblank:
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index ab424ddd7665..8edb0094e5a7 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -800,6 +800,17 @@ struct drm_mode_config { */ struct drm_property *gamma_lut_size_property;
- /**
* @cubic_lut_property: Optional CRTC property to set the 3D LUT used to* convert color spaces.*/- struct drm_property *cubic_lut_property;
- /**
* @cubic_lut_size_property: Optional CRTC property for the size of the* 3D LUT as supported by the driver (read-only).*/- struct drm_property *cubic_lut_size_property;
- /**
- @suggested_x_property: Optional connector property with a hint for
- the position of the output on the host's screen.
Hi Laurent and Kieran,
Do you forget to attach cubic_lut_property with crtc?
drm_object_attach_property(&crtc->base, config->cubic_lut_property, cubic_lut_size);
在 2020/12/21 9:57, Laurent Pinchart 写道:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Extend the existing color management properties to support provision of a 3D cubic look up table, allowing for color specific adjustments.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Co-developed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_state_helper.c | 3 ++ drivers/gpu/drm/drm_atomic_uapi.c | 10 ++++++ drivers/gpu/drm/drm_color_mgmt.c | 41 +++++++++++++++++++---- drivers/gpu/drm/drm_mode_config.c | 14 ++++++++ include/drm/drm_crtc.h | 9 +++++ include/drm/drm_mode_config.h | 11 ++++++ 7 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..0f54897d3c8d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3558,6 +3558,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
replaced |= drm_property_replace_blob(&crtc_state->cubic_lut, NULL); crtc_state->color_mgmt_changed |= replaced;
ret = drm_atomic_commit(state);
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index ddcf5c2c8e6a..61c685b50677 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -141,6 +141,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, drm_property_blob_get(state->ctm); if (state->gamma_lut) drm_property_blob_get(state->gamma_lut);
- if (state->cubic_lut)
drm_property_blob_get(state->cubic_lut);@@ -213,6 +215,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) drm_property_blob_put(state->degamma_lut); drm_property_blob_put(state->ctm); drm_property_blob_put(state->gamma_lut);
- drm_property_blob_put(state->cubic_lut); } EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 268bb69c2e2f..07229acab71c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -471,6 +471,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
- } else if (property == config->cubic_lut_property) {
ret = drm_atomic_replace_property_blob_from_id(dev,&state->cubic_lut,val,-1, sizeof(struct drm_color_lut),&replaced);state->color_mgmt_changed |= replaced;return ret;@@ -516,6 +524,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
- else if (property == config->cubic_lut_property)
*val = (state->cubic_lut) ? state->cubic_lut->base.id : 0;diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..85bbbc8ce8e5 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -33,7 +33,7 @@ /**
- DOC: overview
- Color management or color space adjustments is supported through a set of 5
- Color management or color space adjustments is supported through a set of 7
- properties on the &drm_crtc object. They are set up by calling
- drm_crtc_enable_color_mgmt().
@@ -60,7 +60,7 @@
- “CTM”:
- Blob property to set the current transformation matrix (CTM) apply to
- pixel data after the lookup through the degamma LUT and before the
- lookup through the gamma LUT. The data is interpreted as a struct
- lookup through the cubic LUT. The data is interpreted as a struct
- &drm_color_ctm.
- Setting this to NULL (blob property value set to 0) means a
@@ -68,13 +68,40 @@
- boot-up state too. Drivers can access the blob for the color conversion
- matrix through &drm_crtc_state.ctm.
- ”CUBIC_LUT”:
- Blob property to set the cubic (3D) lookup table performing color
- mapping after the transformation matrix and before the lookup through
- the gamma LUT. Unlike the degamma and gamma LUTs that map color
- components independently, the 3D LUT converts an input color to an
- output color by indexing into the 3D table using the color components
- as a 3D coordinate. The LUT is subsampled as 8-bit (or more) precision
- would require too much storage space in the hardware, so the precision
- of the color components is reduced before the look up, and the low
- order bits may be used to interpolate between the nearest points in 3D
- space.
- The data is interpreted as an array of &struct drm_color_lut elements.
- Hardware might choose not to use the full precision of the LUT
- elements.
- Setting this to NULL (blob property value set to 0) means the output
- color is identical to the input color. This is generally the driver
- boot-up state too. Drivers can access this blob through
- &drm_crtc_state.cubic_lut.
- ”CUBIC_LUT_SIZE”:
- Unsigned range property to give the size of the lookup table to be set
- on the CUBIC_LUT property (the size depends on the underlying hardware).
- If drivers support multiple LUT sizes then they should publish the
- largest size, and sub-sample smaller sized LUTs appropriately.
- “GAMMA_LUT”:
- Blob property to set the gamma lookup table (LUT) mapping pixel data
- after the transformation matrix to data sent to the connector. The
- data is interpreted as an array of &struct drm_color_lut elements.
- Hardware might choose not to use the full precision of the LUT elements
- nor use all the elements of the LUT (for example the hardware might
- choose to interpolate between LUT[0] and LUT[4]).
- after the cubic LUT to data sent to the connector. The data is
- interpreted as an array of &struct drm_color_lut elements. Hardware
- might choose not to use the full precision of the LUT elements nor use
- all the elements of the LUT (for example the hardware might choose to
- interpolate between LUT[0] and LUT[4]).
- Setting this to NULL (blob property value set to 0) means a
- linear/pass-thru gamma table should be used. This is generally the
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..6c3324f60e7d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -364,6 +364,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.gamma_lut_size_property = prop;
- prop = drm_property_create(dev,
DRM_MODE_PROP_BLOB,"CUBIC_LUT", 0);- if (!prop)
return -ENOMEM;- dev->mode_config.cubic_lut_property = prop;
- prop = drm_property_create_range(dev,
DRM_MODE_PROP_IMMUTABLE,"CUBIC_LUT_SIZE", 0, UINT_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.cubic_lut_size_property = prop;
- prop = drm_property_create(dev, DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, "IN_FORMATS", 0);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5f43d64d2a07..df5cc2239adb 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -288,6 +288,15 @@ struct drm_crtc_state { */ struct drm_property_blob *gamma_lut;
- /**
* @cubic_lut:** Cubic Lookup table for converting pixel data. See* drm_crtc_enable_color_mgmt(). The blob (if not NULL) is a 3D array* of &struct drm_color_lut.*/- struct drm_property_blob *cubic_lut;
- /**
- @target_vblank:
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index ab424ddd7665..8edb0094e5a7 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -800,6 +800,17 @@ struct drm_mode_config { */ struct drm_property *gamma_lut_size_property;
- /**
* @cubic_lut_property: Optional CRTC property to set the 3D LUT used to* convert color spaces.*/- struct drm_property *cubic_lut_property;
- /**
* @cubic_lut_size_property: Optional CRTC property for the size of the* 3D LUT as supported by the driver (read-only).*/- struct drm_property *cubic_lut_size_property;
- /**
- @suggested_x_property: Optional connector property with a hint for
- the position of the output on the host's screen.
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Link the DRM 3D-CLU configuration to the CMM setup configuration.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 426b1870b3cb..5c77017084ed 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -484,19 +484,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { struct drm_property_blob *drm_lut = state->gamma_lut; + struct drm_property_blob *drm_clu = state->cubic_lut; struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); struct device *dev = rcrtc->dev->dev;
- if (!drm_lut) - return 0; - - /* We only accept fully populated LUT tables. */ - if (drm_color_lut_size(drm_lut) != CM2_LUT_SIZE) { + /* We only accept fully populated LUTs. */ + if (drm_lut && drm_color_lut_size(drm_lut) != CM2_LUT_SIZE) { dev_err(dev, "invalid gamma lut size: %zu bytes\n", drm_lut->length); return -EINVAL; }
+ if (drm_clu && drm_color_lut_size(drm_clu) != CM2_CLU_SIZE) { + dev_err(dev, "invalid cubic lut size: %zu bytes\n", + drm_clu->length); + return -EINVAL; + } + return 0; }
@@ -518,6 +522,15 @@ static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc, ? new_state->gamma_lut->data : NULL; }
+ if (!old_state || + !old_state->cubic_lut != !new_state->cubic_lut || + (old_state->cubic_lut && new_state->cubic_lut && + old_state->cubic_lut->base.id != new_state->cubic_lut->base.id)) { + cmm_config.clu.update = true; + cmm_config.clu.table = new_state->cubic_lut + ? new_state->cubic_lut->data : NULL; + } + rcar_cmm_setup(rcrtc->cmm, &cmm_config); }
Hi Laurent,
On 21/12/2020 01:57, Laurent Pinchart wrote:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Link the DRM 3D-CLU configuration to the CMM setup configuration.
Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
And for the updates from my original patch (variable rename, and property blob rework, and that ... obvious bug ... from my patch that's now obviously gone :-D ...)
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 426b1870b3cb..5c77017084ed 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -484,19 +484,23 @@ static int rcar_du_cmm_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { struct drm_property_blob *drm_lut = state->gamma_lut;
- struct drm_property_blob *drm_clu = state->cubic_lut; struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); struct device *dev = rcrtc->dev->dev;
- if (!drm_lut)
return 0;- /* We only accept fully populated LUT tables. */
- if (drm_color_lut_size(drm_lut) != CM2_LUT_SIZE) {
/* We only accept fully populated LUTs. */
if (drm_lut && drm_color_lut_size(drm_lut) != CM2_LUT_SIZE) { dev_err(dev, "invalid gamma lut size: %zu bytes\n", drm_lut->length); return -EINVAL; }
if (drm_clu && drm_color_lut_size(drm_clu) != CM2_CLU_SIZE) {
dev_err(dev, "invalid cubic lut size: %zu bytes\n",drm_clu->length);return -EINVAL;}
return 0;
}
@@ -518,6 +522,15 @@ static void rcar_du_cmm_setup(struct rcar_du_crtc *rcrtc, ? new_state->gamma_lut->data : NULL; }
- if (!old_state ||
!old_state->cubic_lut != !new_state->cubic_lut ||(old_state->cubic_lut && new_state->cubic_lut &&old_state->cubic_lut->base.id != new_state->cubic_lut->base.id)) {cmm_config.clu.update = true;cmm_config.clu.table = new_state->cubic_lut? new_state->cubic_lut->data : NULL;- }
- rcar_cmm_setup(rcrtc->cmm, &cmm_config);
}
Hi,
On Monday, December 21st, 2020 at 2:57 AM, Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:
I started having a look at userspace to see how this could be handled, searching for color management support in weston, kwin and wlroots/sway. All three support setting the gamma table when using the DRM/KMS backend, weston and kwin through the legacy API, and wlroots through the atomic API. Weston reads an ICC profile using Little CMS and applies the gamma table. kwin is a bit more elaborate, it also uses Little CMS to read an ICC profile, but additionally supports setting the brightness and color temperature. It however only sets a gamma table in the end. Wlroots seems to have an API to set the gamma table, but I haven't seen where sway uses it (I may have missed that though)
wlroots delegates setting the gamma table to a privileged client, to allow users to set it to whatever they want. Use-cases include setting the color temperature and setting the brightness. wlroots doesn't support ICC profiles (and I don't know of a client setting the gamma LUT from an ICC profile).
In any case, there's limited support there for color management.
That's correct.
Inputs would be appreciated on this, for instance with feedback on how X.org and Android handle color management, on how 3D LUTs are implemented on other platforms, or in general on how we would like to use them. I don't mind doing some work in userspace to prototype this, but I won't have the bandwidth to design a completely new framework.
Weston is working on improving color management support [1] [2]. I think it's still a little early, but maybe see with Pekka if something can be worked out?
Other than that, maybe some media players have support for some color management and would need to blend in multiple buffers and standardize protocols. Maybe look into Kodi or mpv?
Simon
[1]: https://www.collabora.com/news-and-blog/blog/2020/11/19/developing-wayland-c... [2]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/14
On Mon, 21 Dec 2020 03:57:26 +0200 Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:
Hello,
This patch series adds cubic (3D) look up table support to the CMM and DU drivers, and extend the KMS userspace API to expose the cubic LUT to userspace.
Hi,
when you say "cubic" I immediately think "polynomial of third degree", and got really curious how that works. But it seems that is not at all what you have here, instead you have a 3D LUT with probably trilinear(?) interpolation.
I would suggest to stop using the misleading term "cubic" (e.g. cubic interpolation is a thing).
Where does the abbreviation CLU come from? If that refers to cubic as well, it would be best to change that too to avoid misleading.
Unless your hardware actually does cubic interpolation in the 3D LUT?
The code is fairly straightforward. Patch 1/4 refactors the CMM (color management module, the Renesas R-Car IP core that handles 1D and 3D lookup tables for the display) driver, which currently supports the 1D (a.k.a. gamma) table only, to prepare for 3D LUT support (including a modification to the API between the CMM and DU drivers). The CMM driver is then extended in patch 2/4 to support the 3D LUT.
Patch 3/4 adds support for the 3D LUT in the KMS core and the KMS userspace API, in the form of two new properties. I expect this to be the most controversial part of the series, not so much for the feature itself, but for when it is inserted in the color management pipeline.
Finally, patch 4/4 wires the KMS extension to the DU driver.
The R-Car CMM applies the 3D LUT at the output of the display, where data is transmitted in RGB space (when outputting YUV data to the display the CMM can't be used according to the documentation, but I wouldn't be entirely surprised if this limitation could be worked around), before the 1D LUT. I've located the 3D LUT between the CTM and the gamma LUT, but it could equally be placed before the degamma LUT or between the degamma LUT and the CTM in my case, as the R-Car color management pipeline has no CTM and has a single 1D LUT on the output side (there's provision for 1D LUT on the input side for some of the planes, but that's a separate topic).
I however don't expect this to necessarily match all hardware though, and this feature may require us to give up on a fixed, one size fits them all, color management pipeline exposed to userspace. Whether this would mean device-specific APIs (not necessarily in the form of device-specific properties, but in how they map to hardware features, as I think helpers to handle a 3D LUT property in the KMS core can save code duplication in drivers), or the need for a new property to expose the order in which color management operations are implemented, I don't know.
That is a difficult problem indeed. Userspace must know everything what happens to the pixel values exactly, beyond that I have no suggestions there.
I started having a look at userspace to see how this could be handled, searching for color management support in weston, kwin and wlroots/sway. All three support setting the gamma table when using the DRM/KMS backend, weston and kwin through the legacy API, and wlroots through the atomic API. Weston reads an ICC profile using Little CMS and applies the gamma table. kwin is a bit more elaborate, it also uses Little CMS to read an ICC profile, but additionally supports setting the brightness and color temperature. It however only sets a gamma table in the end. Wlroots seems to have an API to set the gamma table, but I haven't seen where sway uses it (I may have missed that though). In any case, there's limited support there for color management.
Inputs would be appreciated on this, for instance with feedback on how X.org and Android handle color management, on how 3D LUTs are implemented on other platforms, or in general on how we would like to use them. I don't mind doing some work in userspace to prototype this, but I won't have the bandwidth to design a completely new framework.
The idea for Weston (and Wayland in general) is that the display server uses a CMM to compute a transformation it needs to apply, based on the display and content color properties (e.g. ICC profiles) and more. What that transformation exactly is depends on the CMM, and it may further depend on what kind of ICC profiles are being used (an ICC file may contain different kinds of transformation definitions, from parameterised standard formulas to 1D and 3D LUTs and chains of those). So a display server gets a more or less opaque transformation object from a CMM and needs to implement what it describes somehow.
Implementing the transformation depends on what kind of API the CMM offers. The good thing with a 3D LUT is that, AFAIU, no matter what the actual transformation is, it can always be represented as a 3D LUT. The only open question then is the size and precision of the 3D LUT, and how it is interpolated. If the precision is sufficient, a display server may choose to use the hardware 3D LUT.
All this I believe should be internal to a Wayland display server.
Mind, that everything I talk about in the above is done in *addition* to "the LUT" (VCGT tag in ICC files). In the X11 model of color management, "the LUT" is considered calibration and is 3 x 1D. If some LUT was in place when profiling a monitor, the same LUT must also be in place when using that monitor color profile. Confusingly, calibration values like "the LUT" are not considered as part of the color profile, even though technically "the LUT" can be saved in an ICC file. For more information, see: https://ninedegreesbelow.com/photography/monitor-profile-calibrate-confuse.h... https://gitlab.freedesktop.org/swick/wayland-protocols/-/blob/color/unstable...
The code you saw in Weston loading a LUT from an ICC profile does the VCGT tag part and nothing else. IOW, it loads a calibration, not a monitor color profile. A 3D LUT could be regarded as calibration as it is in the old-school model, or it could be actively computed and used like the Wayland model being designed.
Loading a LUT from the VCGT tag is not really color management. It's just calibration, like tuning the monitor brightness knob.
Simon already pointed you to https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/14 and Weston work is underway as well, but it may be quite some time before Weston could actually take advantage of a KMS 3D LUT. In Weston we consider hardware features like the 3D LUT as optional optimisations that can be used if the circumstances are right. This means that we want the "software path", that is, GL shaders, to work first. Shaders we can run and test everywhere, but the 3D LUT is not always available and requires a writeback connector to test in an automated test suite..
As a summary, I could say that X.org does not do color management. Period. X.org does allow loading "the LUT" (VCGT), but it does nothing with color profiles. Applying color profiles is left for each X11 app to do on their own to their own pixels. If X11 protocol extensions do not already support setting a 3D LUT, then you have to add that to the protocol.
Wayland is being designed differently: the display server is responsible for color management, unless Wayland applications explicitly ask to do it themselves. Wayland protocol is intended to not allow free-to-all access to hardware LUTs like X11 does.
Therefore, I would say that while adding X11 protocol to set a 3D LUT in hardware does mechanically exercise the UAPI, it does not in my opinion prove anything about the usability of the UAPI, because it lacks an actual use case. You would need something to meaningfully and purposefully to set the 3D LUT to some particular values to reach some specific effect or goal for a real world use case. This may be an unpopular opinion, but IMO X11 RandR is little more than a library ABI exposing KMS UAPI as is, similar to libdrm: an interface, not a full application alone.
CrOS and Kodi mentioned by danvet sound like much better proving vehicles than X.org, Weston I'm afraid might be quite some months away still.
A consideration from KMS UAPI point of view: for the 3D LUT to be usable for Wayland compositors, setting the 3D LUT must be either glitch-free, or if it can cause a hickup, glitch, stall, or anything else not observed during a normal pageflip, it must require ALLOW_MODESET.
If setting any KMS property is possible without ALLOW_MODESET but it may cause a visible glitch, pageflip timings included, then that KMS property is much less useful to a Wayland display server as it can only be used as if it always required ALLOW_MODESET, even with drivers where it is glitch-free.
Thanks, pq
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Kieran Bingham -
Laurent Pinchart -
Laurent Pinchart -
Pekka Paalanen -
Sandy Huang -
Simon Ser -
Ville Syrjälä