[PATCH v4 0/3] RK3288 Gamma LUT

[PATCH v4 0/3] RK3288 Gamma LUT

[Ezequiel Garcia]

Following Sean's comments, here's a new version.

On this new iteration, we modify the GAMMA LUT
on .atomic_enable and .atomic_begin.

With this change, the GAMMA settings are effectively
re-applied after resuming the machine, so the previous
patch "RFC: drm/atomic-helper: Reapply color transformation after
resume" is now dropped.

Also, I dropped Reviewed-bys tags on patch 2, given
the implementation is a bit different now.

Thanks!

Ezequiel Garcia (3):
  dt-bindings: display: rockchip: document VOP gamma LUT address
  drm/rockchip: Add optional support for CRTC gamma LUT
  ARM: dts: rockchip: Add RK3288 VOP gamma LUT address

 .../display/rockchip/rockchip-vop.txt         |   6 +-
 arch/arm/boot/dts/rk3288.dtsi                 |   4 +-
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |   1 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 125 ++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h   |   5 +
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c   |   2 +
 6 files changed, 140 insertions(+), 3 deletions(-)

-- 
2.22.0

[PATCH v4 1/3] dt-bindings: display: rockchip: document VOP gamma LUT address

[Ezequiel Garcia]

Add the register specifier description for an
optional gamma LUT address.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes from v3:
* None.
Changes from v2:
* None.
Changes from v1:
* Drop reg-names, suggested by Doug.
---
 .../devicetree/bindings/display/rockchip/rockchip-vop.txt   | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
index 4f58c5a2d195..8b3a5f514205 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
@@ -20,6 +20,10 @@ Required properties:
 		"rockchip,rk3228-vop";
 		"rockchip,rk3328-vop";
 
+- reg: Must contain one entry corresponding to the base address and length
+	of the register space. Can optionally contain a second entry
+	corresponding to the CRTC gamma LUT address.
+
 - interrupts: should contain a list of all VOP IP block interrupts in the
 		 order: VSYNC, LCD_SYSTEM. The interrupt specifier
 		 format depends on the interrupt controller used.
@@ -48,7 +52,7 @@ Example:
 SoC specific DT entry:
 	vopb: vopb@ff930000 {
 		compatible = "rockchip,rk3288-vop";
-		reg = <0xff930000 0x19c>;
+		reg = <0x0 0xff930000 0x0 0x19c>, <0x0 0xff931000 0x0 0x1000>;
 		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru ACLK_VOP0>, <&cru DCLK_VOP0>, <&cru HCLK_VOP0>;
 		clock-names = "aclk_vop", "dclk_vop", "hclk_vop";
-- 
2.22.0

[PATCH v4 2/3] drm/rockchip: Add optional support for CRTC gamma LUT

[Ezequiel Garcia]

Add an optional CRTC gamma LUT support, and enable it on RK3288.
This is currently enabled via a separate address resource,
which needs to be specified in the devicetree.

The address resource is required because on some SoCs, such as
RK3288, the LUT address is after the MMU address, and the latter
is supported by a different driver. This prevents the DRM driver
from requesting an entire register space.

The current implementation works for RGB 10-bit tables, as that
is what seems to work on RK3288.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
Changes from v3:
* Move to atomic_enable and atomic_begin,
  as discussed with Sean Paul.
* Dropped the Reviewed-bys.
Changes from v2:
* None.
Changes from v1:
* drop explicit linear LUT after finding a proper
  way to disable gamma correction.
* avoid setting gamma is the CRTC is not active.
* s/int/unsigned int as suggested by Jacopo.
* only enable color management and set gamma size
  if gamma LUT is supported, suggested by Doug.
* drop the reg-names usage, and instead just use indexed reg
  specifiers, suggested by Doug.
Changes from RFC:
* Request (an optional) address resource for the LUT.
* Drop support for RK3399, which doesn't seem to work
  out of the box and needs more research.
* Support pass-thru setting when GAMMA_LUT is NULL.
* Add a check for the gamma size, as suggested by Ilia.
* Move gamma setting to atomic_commit_tail, as pointed
  out by Jacopo/Laurent, is the correct way.
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |   1 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 125 ++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   5 +
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   2 +
 4 files changed, 133 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index ca01234c037c..697ee04b85cf 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -17,6 +17,7 @@
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_fb.h"
 #include "rockchip_drm_gem.h"
+#include "rockchip_drm_vop.h"
 
 static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
 	.destroy       = drm_gem_fb_destroy,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 613404f86668..85c1269a1218 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -139,6 +139,7 @@ struct vop {
 
 	uint32_t *regsbak;
 	void __iomem *regs;
+	void __iomem *lut_regs;
 
 	/* physical map length of vop register */
 	uint32_t len;
@@ -1048,6 +1049,84 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
+static bool vop_dsp_lut_is_enable(struct vop *vop)
+{
+	return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en);
+}
+
+static void vop_crtc_write_gamma_lut(struct vop *vop, struct drm_crtc *crtc)
+{
+	struct drm_color_lut *lut = crtc->state->gamma_lut->data;
+	unsigned int i;
+
+	for (i = 0; i < crtc->gamma_size; i++) {
+		u32 word;
+
+		word = (drm_color_lut_extract(lut[i].red, 10) << 20) |
+		       (drm_color_lut_extract(lut[i].green, 10) << 10) |
+			drm_color_lut_extract(lut[i].blue, 10);
+		writel(word, vop->lut_regs + i * 4);
+	}
+}
+
+static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
+			       struct drm_crtc_state *old_crtc_state)
+{
+	unsigned int idle;
+	int ret;
+
+	/*
+	 * In order to write the LUT to the internal memory,
+	 * we need to first make sure the dsp_lut_en bit is cleared.
+	 */
+	spin_lock(&vop->reg_lock);
+	VOP_REG_SET(vop, common, dsp_lut_en, 0);
+	vop_cfg_done(vop);
+	spin_unlock(&vop->reg_lock);
+
+	/*
+	 * If the CRTC is not active, dsp_lut_en will not get cleared.
+	 * Apparently we still need to do the above step to for
+	 * gamma correction to be disabled.
+	 */
+	if (!crtc->state->active)
+		return;
+
+	ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
+				 idle, !idle, 5, 30 * 1000);
+	if (ret) {
+		DRM_DEV_ERROR(vop->dev, "display LUT RAM enable timeout!\n");
+		return;
+	}
+
+	if (crtc->state->gamma_lut &&
+	   (!old_crtc_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
+					old_crtc_state->gamma_lut->base.id))) {
+
+		spin_lock(&vop->reg_lock);
+
+		vop_crtc_write_gamma_lut(vop, crtc);
+		VOP_REG_SET(vop, common, dsp_lut_en, 1);
+		vop_cfg_done(vop);
+
+		spin_unlock(&vop->reg_lock);
+	}
+}
+
+static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
+				   struct drm_crtc_state *old_crtc_state)
+{
+	struct vop *vop = to_vop(crtc);
+
+	/*
+	 * Only update GAMMA if the 'active' flag is not changed,
+	 * otherwise it's updated by .atomic_enable.
+	 */
+	if (vop->lut_regs && crtc->state->color_mgmt_changed &&
+	    !crtc->state->active_changed)
+		vop_crtc_gamma_set(vop, crtc, old_crtc_state);
+}
+
 static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_state)
 {
@@ -1075,6 +1154,14 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
 		return;
 	}
 
+	/*
+	 * If we have a GAMMA LUT in the state, then let's make sure
+	 * it's updated. We might be coming out of suspend,
+	 * which means the LUT internal memory needs to be re-written.
+	 */
+	if (vop->lut_regs && crtc->state->gamma_lut)
+		vop_crtc_gamma_set(vop, crtc, old_state);
+
 	mutex_lock(&vop->vop_lock);
 
 	WARN_ON(vop->event);
@@ -1191,6 +1278,26 @@ static void vop_wait_for_irq_handler(struct vop *vop)
 	synchronize_irq(vop->irq);
 }
 
+static int vop_crtc_atomic_check(struct drm_crtc *crtc,
+				 struct drm_crtc_state *crtc_state)
+{
+	struct vop *vop = to_vop(crtc);
+
+	if (vop->lut_regs && crtc_state->color_mgmt_changed &&
+	    crtc_state->gamma_lut) {
+		unsigned int len;
+
+		len = drm_color_lut_size(crtc_state->gamma_lut);
+		if (len != crtc->gamma_size) {
+			DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
+				      len, crtc->gamma_size);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 				  struct drm_crtc_state *old_crtc_state)
 {
@@ -1243,6 +1350,8 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 
 static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
 	.mode_fixup = vop_crtc_mode_fixup,
+	.atomic_check = vop_crtc_atomic_check,
+	.atomic_begin = vop_crtc_atomic_begin,
 	.atomic_flush = vop_crtc_atomic_flush,
 	.atomic_enable = vop_crtc_atomic_enable,
 	.atomic_disable = vop_crtc_atomic_disable,
@@ -1361,6 +1470,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.disable_vblank = vop_crtc_disable_vblank,
 	.set_crc_source = vop_crtc_set_crc_source,
 	.verify_crc_source = vop_crtc_verify_crc_source,
+	.gamma_set = drm_atomic_helper_legacy_gamma_set,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
@@ -1518,6 +1628,10 @@ static int vop_create_crtc(struct vop *vop)
 		goto err_cleanup_planes;
 
 	drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs);
+	if (vop->lut_regs) {
+		drm_mode_crtc_set_gamma_size(crtc, vop_data->lut_size);
+		drm_crtc_enable_color_mgmt(crtc, 0, false, vop_data->lut_size);
+	}
 
 	/*
 	 * Create drm_planes for overlay windows with possible_crtcs restricted
@@ -1822,6 +1936,17 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	if (IS_ERR(vop->regs))
 		return PTR_ERR(vop->regs);
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res) {
+		if (!vop_data->lut_size) {
+			DRM_DEV_ERROR(dev, "no gamma LUT size defined\n");
+			return -EINVAL;
+		}
+		vop->lut_regs = devm_ioremap_resource(dev, res);
+		if (IS_ERR(vop->lut_regs))
+			return PTR_ERR(vop->lut_regs);
+	}
+
 	vop->regsbak = devm_kzalloc(dev, vop->len, GFP_KERNEL);
 	if (!vop->regsbak)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 2149a889c29d..8192c90d48c4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -7,6 +7,8 @@
 #ifndef _ROCKCHIP_DRM_VOP_H
 #define _ROCKCHIP_DRM_VOP_H
 
+#include <drm/drm_atomic.h>
+
 /*
  * major: IP major version, used for IP structure
  * minor: big feature change under same structure
@@ -67,6 +69,7 @@ struct vop_common {
 	struct vop_reg dither_down_mode;
 	struct vop_reg dither_down_en;
 	struct vop_reg dither_up;
+	struct vop_reg dsp_lut_en;
 	struct vop_reg gate_en;
 	struct vop_reg mmu_en;
 	struct vop_reg out_mode;
@@ -170,6 +173,7 @@ struct vop_data {
 	const struct vop_win_yuv2yuv_data *win_yuv2yuv;
 	const struct vop_win_data *win;
 	unsigned int win_size;
+	unsigned int lut_size;
 
 #define VOP_FEATURE_OUTPUT_RGB10	BIT(0)
 #define VOP_FEATURE_INTERNAL_RGB	BIT(1)
@@ -373,4 +377,5 @@ static inline int scl_vop_cal_lb_mode(int width, bool is_yuv)
 }
 
 extern const struct component_ops vop_component_ops;
+
 #endif /* _ROCKCHIP_DRM_VOP_H */
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
index d1494be14471..42ddcb698c82 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -598,6 +598,7 @@ static const struct vop_common rk3288_common = {
 	.dither_down_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 2),
 	.pre_dither_down = VOP_REG(RK3288_DSP_CTRL1, 0x1, 1),
 	.dither_up = VOP_REG(RK3288_DSP_CTRL1, 0x1, 6),
+	.dsp_lut_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 0),
 	.data_blank = VOP_REG(RK3288_DSP_CTRL0, 0x1, 19),
 	.dsp_blank = VOP_REG(RK3288_DSP_CTRL0, 0x3, 18),
 	.out_mode = VOP_REG(RK3288_DSP_CTRL0, 0xf, 0),
@@ -646,6 +647,7 @@ static const struct vop_data rk3288_vop = {
 	.output = &rk3288_output,
 	.win = rk3288_vop_win_data,
 	.win_size = ARRAY_SIZE(rk3288_vop_win_data),
+	.lut_size = 1024,
 };
 
 static const int rk3368_vop_intrs[] = {
-- 
2.22.0

[PATCH v4 3/3] ARM: dts: rockchip: Add RK3288 VOP gamma LUT address

[Ezequiel Garcia]

RK3288 SoC VOPs have optional support Gamma LUT setting,
which requires specifying the Gamma LUT address in the devicetree.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes from v3:
* None.
Changes from v2:
* None.
Changes from v1:
* Drop reg-names, as suggested by Doug.
---
 arch/arm/boot/dts/rk3288.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index cc893e154fe5..c6fc633ace80 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -1023,7 +1023,7 @@
 
 	vopb: vop@ff930000 {
 		compatible = "rockchip,rk3288-vop";
-		reg = <0x0 0xff930000 0x0 0x19c>;
+		reg = <0x0 0xff930000 0x0 0x19c>, <0x0 0xff931000 0x0 0x1000>;
 		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru ACLK_VOP0>, <&cru DCLK_VOP0>, <&cru HCLK_VOP0>;
 		clock-names = "aclk_vop", "dclk_vop", "hclk_vop";
@@ -1073,7 +1073,7 @@
 
 	vopl: vop@ff940000 {
 		compatible = "rockchip,rk3288-vop";
-		reg = <0x0 0xff940000 0x0 0x19c>;
+		reg = <0x0 0xff940000 0x0 0x19c>, <0x0 0xff941000 0x0 0x1000>;
 		interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru ACLK_VOP1>, <&cru DCLK_VOP1>, <&cru HCLK_VOP1>;
 		clock-names = "aclk_vop", "dclk_vop", "hclk_vop";
-- 
2.22.0

Re: [PATCH v4 2/3] drm/rockchip: Add optional support for CRTC gamma LUT

[Sean Paul]

On Tue, Oct 08, 2019 at 08:00:37PM -0300, Ezequiel Garcia wrote:
> Add an optional CRTC gamma LUT support, and enable it on RK3288.
> This is currently enabled via a separate address resource,
> which needs to be specified in the devicetree.
> 
> The address resource is required because on some SoCs, such as
> RK3288, the LUT address is after the MMU address, and the latter
> is supported by a different driver. This prevents the DRM driver
> from requesting an entire register space.
> 
> The current implementation works for RGB 10-bit tables, as that
> is what seems to work on RK3288.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Hey Ezequiel,
Just a few comments on the actual content of the patch as opposed to my higher
level comments yesterday. I think we're almost there, thanks for sticking this
out!

> ---
> Changes from v3:
> * Move to atomic_enable and atomic_begin,
>   as discussed with Sean Paul.
> * Dropped the Reviewed-bys.
> Changes from v2:
> * None.
> Changes from v1:
> * drop explicit linear LUT after finding a proper
>   way to disable gamma correction.
> * avoid setting gamma is the CRTC is not active.
> * s/int/unsigned int as suggested by Jacopo.
> * only enable color management and set gamma size
>   if gamma LUT is supported, suggested by Doug.
> * drop the reg-names usage, and instead just use indexed reg
>   specifiers, suggested by Doug.
> Changes from RFC:
> * Request (an optional) address resource for the LUT.
> * Drop support for RK3399, which doesn't seem to work
>   out of the box and needs more research.
> * Support pass-thru setting when GAMMA_LUT is NULL.
> * Add a check for the gamma size, as suggested by Ilia.
> * Move gamma setting to atomic_commit_tail, as pointed
>   out by Jacopo/Laurent, is the correct way.
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |   1 +
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 125 ++++++++++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   5 +
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   2 +
>  4 files changed, 133 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ca01234c037c..697ee04b85cf 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -17,6 +17,7 @@
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_fb.h"
>  #include "rockchip_drm_gem.h"
> +#include "rockchip_drm_vop.h"

Leftover from the previous version?

>  
>  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>  	.destroy       = drm_gem_fb_destroy,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 613404f86668..85c1269a1218 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -139,6 +139,7 @@ struct vop {
>  
>  	uint32_t *regsbak;
>  	void __iomem *regs;
> +	void __iomem *lut_regs;
>  
>  	/* physical map length of vop register */
>  	uint32_t len;
> @@ -1048,6 +1049,84 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
>  	return true;
>  }
>  
> +static bool vop_dsp_lut_is_enable(struct vop *vop)

*enabled

> +{
> +	return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en);
> +}
> +
> +static void vop_crtc_write_gamma_lut(struct vop *vop, struct drm_crtc *crtc)
> +{
> +	struct drm_color_lut *lut = crtc->state->gamma_lut->data;
> +	unsigned int i;
> +
> +	for (i = 0; i < crtc->gamma_size; i++) {
> +		u32 word;
> +
> +		word = (drm_color_lut_extract(lut[i].red, 10) << 20) |
> +		       (drm_color_lut_extract(lut[i].green, 10) << 10) |
> +			drm_color_lut_extract(lut[i].blue, 10);
> +		writel(word, vop->lut_regs + i * 4);
> +	}
> +}
> +
> +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> +			       struct drm_crtc_state *old_crtc_state)
> +{
> +	unsigned int idle;
> +	int ret;
> +

How about:

        if (!vop->lut_regs)
                return;

here and then you can remove that condition above the 2 callsites

> +	/*
> +	 * In order to write the LUT to the internal memory,
> +	 * we need to first make sure the dsp_lut_en bit is cleared.
> +	 */
> +	spin_lock(&vop->reg_lock);
> +	VOP_REG_SET(vop, common, dsp_lut_en, 0);
> +	vop_cfg_done(vop);
> +	spin_unlock(&vop->reg_lock);
> +
> +	/*
> +	 * If the CRTC is not active, dsp_lut_en will not get cleared.
> +	 * Apparently we still need to do the above step to for
> +	 * gamma correction to be disabled.
> +	 */
> +	if (!crtc->state->active)
> +		return;
> +
> +	ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> +				 idle, !idle, 5, 30 * 1000);
> +	if (ret) {
> +		DRM_DEV_ERROR(vop->dev, "display LUT RAM enable timeout!\n");
> +		return;
> +	}
> +
> +	if (crtc->state->gamma_lut &&
> +	   (!old_crtc_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> +					old_crtc_state->gamma_lut->base.id))) {

Silly question, but isn't the second part of this check redundant since you need 
color_mgmt_changed || active_changed to get into this function?

So maybe invert the conditional here and exit early (to save a level of
indentation in the block below):

        if (!crtc->state->gamma_lut)
                return;

        spin_lock(&vop->reg_lock);

        vop_crtc_write_gamma_lut(vop, crtc);
        VOP_REG_SET(vop, common, dsp_lut_en, 1);
        vop_cfg_done(vop);

        spin_unlock(&vop->reg_lock);

> +
> +		spin_lock(&vop->reg_lock);
> +
> +		vop_crtc_write_gamma_lut(vop, crtc);
> +		VOP_REG_SET(vop, common, dsp_lut_en, 1);
> +		vop_cfg_done(vop);
> +
> +		spin_unlock(&vop->reg_lock);
> +	}
> +}
> +
> +static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *old_crtc_state)
> +{
> +	struct vop *vop = to_vop(crtc);
> +
> +	/*
> +	 * Only update GAMMA if the 'active' flag is not changed,
> +	 * otherwise it's updated by .atomic_enable.
> +	 */
> +	if (vop->lut_regs && crtc->state->color_mgmt_changed &&
> +	    !crtc->state->active_changed)
> +		vop_crtc_gamma_set(vop, crtc, old_crtc_state);
> +}
> +
>  static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
>  				   struct drm_crtc_state *old_state)
>  {
> @@ -1075,6 +1154,14 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
>  		return;
>  	}
>  
> +	/*
> +	 * If we have a GAMMA LUT in the state, then let's make sure
> +	 * it's updated. We might be coming out of suspend,
> +	 * which means the LUT internal memory needs to be re-written.
> +	 */
> +	if (vop->lut_regs && crtc->state->gamma_lut)
> +		vop_crtc_gamma_set(vop, crtc, old_state);
> +
>  	mutex_lock(&vop->vop_lock);
>  
>  	WARN_ON(vop->event);
> @@ -1191,6 +1278,26 @@ static void vop_wait_for_irq_handler(struct vop *vop)
>  	synchronize_irq(vop->irq);
>  }
>  
> +static int vop_crtc_atomic_check(struct drm_crtc *crtc,
> +				 struct drm_crtc_state *crtc_state)
> +{
> +	struct vop *vop = to_vop(crtc);
> +
> +	if (vop->lut_regs && crtc_state->color_mgmt_changed &&
> +	    crtc_state->gamma_lut) {
> +		unsigned int len;
> +
> +		len = drm_color_lut_size(crtc_state->gamma_lut);
> +		if (len != crtc->gamma_size) {
> +			DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> +				      len, crtc->gamma_size);
> +			return -EINVAL;
> +		}

Overflow is avoided in drm_mode_gamma_set_ioctl(), so I don't think you need
this function.

Sean

> +	}
> +
> +	return 0;
> +}
> +
>  static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
>  				  struct drm_crtc_state *old_crtc_state)
>  {
> @@ -1243,6 +1350,8 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>  static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
>  	.mode_fixup = vop_crtc_mode_fixup,
> +	.atomic_check = vop_crtc_atomic_check,
> +	.atomic_begin = vop_crtc_atomic_begin,
>  	.atomic_flush = vop_crtc_atomic_flush,
>  	.atomic_enable = vop_crtc_atomic_enable,
>  	.atomic_disable = vop_crtc_atomic_disable,
> @@ -1361,6 +1470,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
>  	.disable_vblank = vop_crtc_disable_vblank,
>  	.set_crc_source = vop_crtc_set_crc_source,
>  	.verify_crc_source = vop_crtc_verify_crc_source,
> +	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  };
>  
>  static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
> @@ -1518,6 +1628,10 @@ static int vop_create_crtc(struct vop *vop)
>  		goto err_cleanup_planes;
>  
>  	drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs);
> +	if (vop->lut_regs) {
> +		drm_mode_crtc_set_gamma_size(crtc, vop_data->lut_size);
> +		drm_crtc_enable_color_mgmt(crtc, 0, false, vop_data->lut_size);
> +	}
>  
>  	/*
>  	 * Create drm_planes for overlay windows with possible_crtcs restricted
> @@ -1822,6 +1936,17 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>  	if (IS_ERR(vop->regs))
>  		return PTR_ERR(vop->regs);
>  
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (res) {
> +		if (!vop_data->lut_size) {
> +			DRM_DEV_ERROR(dev, "no gamma LUT size defined\n");
> +			return -EINVAL;
> +		}
> +		vop->lut_regs = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(vop->lut_regs))
> +			return PTR_ERR(vop->lut_regs);
> +	}
> +
>  	vop->regsbak = devm_kzalloc(dev, vop->len, GFP_KERNEL);
>  	if (!vop->regsbak)
>  		return -ENOMEM;
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 2149a889c29d..8192c90d48c4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -7,6 +7,8 @@
>  #ifndef _ROCKCHIP_DRM_VOP_H
>  #define _ROCKCHIP_DRM_VOP_H
>  
> +#include <drm/drm_atomic.h>
> +
>  /*
>   * major: IP major version, used for IP structure
>   * minor: big feature change under same structure
> @@ -67,6 +69,7 @@ struct vop_common {
>  	struct vop_reg dither_down_mode;
>  	struct vop_reg dither_down_en;
>  	struct vop_reg dither_up;
> +	struct vop_reg dsp_lut_en;
>  	struct vop_reg gate_en;
>  	struct vop_reg mmu_en;
>  	struct vop_reg out_mode;
> @@ -170,6 +173,7 @@ struct vop_data {
>  	const struct vop_win_yuv2yuv_data *win_yuv2yuv;
>  	const struct vop_win_data *win;
>  	unsigned int win_size;
> +	unsigned int lut_size;
>  
>  #define VOP_FEATURE_OUTPUT_RGB10	BIT(0)
>  #define VOP_FEATURE_INTERNAL_RGB	BIT(1)
> @@ -373,4 +377,5 @@ static inline int scl_vop_cal_lb_mode(int width, bool is_yuv)
>  }
>  
>  extern const struct component_ops vop_component_ops;
> +
>  #endif /* _ROCKCHIP_DRM_VOP_H */
> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> index d1494be14471..42ddcb698c82 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> @@ -598,6 +598,7 @@ static const struct vop_common rk3288_common = {
>  	.dither_down_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 2),
>  	.pre_dither_down = VOP_REG(RK3288_DSP_CTRL1, 0x1, 1),
>  	.dither_up = VOP_REG(RK3288_DSP_CTRL1, 0x1, 6),
> +	.dsp_lut_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 0),
>  	.data_blank = VOP_REG(RK3288_DSP_CTRL0, 0x1, 19),
>  	.dsp_blank = VOP_REG(RK3288_DSP_CTRL0, 0x3, 18),
>  	.out_mode = VOP_REG(RK3288_DSP_CTRL0, 0xf, 0),
> @@ -646,6 +647,7 @@ static const struct vop_data rk3288_vop = {
>  	.output = &rk3288_output,
>  	.win = rk3288_vop_win_data,
>  	.win_size = ARRAY_SIZE(rk3288_vop_win_data),
> +	.lut_size = 1024,
>  };
>  
>  static const int rk3368_vop_intrs[] = {
> -- 
> 2.22.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v4 2/3] drm/rockchip: Add optional support for CRTC gamma LUT

[Ezequiel Garcia]

Hello Sean,

Thanks for the thourough review.

On Wed, 9 Oct 2019 at 15:01, Sean Paul <sean@poorly.run> wrote:
>
> On Tue, Oct 08, 2019 at 08:00:37PM -0300, Ezequiel Garcia wrote:
> > Add an optional CRTC gamma LUT support, and enable it on RK3288.
> > This is currently enabled via a separate address resource,
> > which needs to be specified in the devicetree.
> >
> > The address resource is required because on some SoCs, such as
> > RK3288, the LUT address is after the MMU address, and the latter
> > is supported by a different driver. This prevents the DRM driver
> > from requesting an entire register space.
> >
> > The current implementation works for RGB 10-bit tables, as that
> > is what seems to work on RK3288.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>
> Hey Ezequiel,
> Just a few comments on the actual content of the patch as opposed to my higher
> level comments yesterday. I think we're almost there, thanks for sticking this
> out!
>
> > ---
> > Changes from v3:
> > * Move to atomic_enable and atomic_begin,
> >   as discussed with Sean Paul.
> > * Dropped the Reviewed-bys.
> > Changes from v2:
> > * None.
> > Changes from v1:
> > * drop explicit linear LUT after finding a proper
> >   way to disable gamma correction.
> > * avoid setting gamma is the CRTC is not active.
> > * s/int/unsigned int as suggested by Jacopo.
> > * only enable color management and set gamma size
> >   if gamma LUT is supported, suggested by Doug.
> > * drop the reg-names usage, and instead just use indexed reg
> >   specifiers, suggested by Doug.
> > Changes from RFC:
> > * Request (an optional) address resource for the LUT.
> > * Drop support for RK3399, which doesn't seem to work
> >   out of the box and needs more research.
> > * Support pass-thru setting when GAMMA_LUT is NULL.
> > * Add a check for the gamma size, as suggested by Ilia.
> > * Move gamma setting to atomic_commit_tail, as pointed
> >   out by Jacopo/Laurent, is the correct way.
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |   1 +
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 125 ++++++++++++++++++++
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   5 +
> >  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   2 +
> >  4 files changed, 133 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index ca01234c037c..697ee04b85cf 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -17,6 +17,7 @@
> >  #include "rockchip_drm_drv.h"
> >  #include "rockchip_drm_fb.h"
> >  #include "rockchip_drm_gem.h"
> > +#include "rockchip_drm_vop.h"
>
> Leftover from the previous version?
>

Yup.

> >
> >  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> >       .destroy       = drm_gem_fb_destroy,
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index 613404f86668..85c1269a1218 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -139,6 +139,7 @@ struct vop {
> >
> >       uint32_t *regsbak;
> >       void __iomem *regs;
> > +     void __iomem *lut_regs;
> >
> >       /* physical map length of vop register */
> >       uint32_t len;
> > @@ -1048,6 +1049,84 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
> >       return true;
> >  }
> >
> > +static bool vop_dsp_lut_is_enable(struct vop *vop)
>
> *enabled
>

Good catch.

> > +{
> > +     return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en);
> > +}
> > +
> > +static void vop_crtc_write_gamma_lut(struct vop *vop, struct drm_crtc *crtc)
> > +{
> > +     struct drm_color_lut *lut = crtc->state->gamma_lut->data;
> > +     unsigned int i;
> > +
> > +     for (i = 0; i < crtc->gamma_size; i++) {
> > +             u32 word;
> > +
> > +             word = (drm_color_lut_extract(lut[i].red, 10) << 20) |
> > +                    (drm_color_lut_extract(lut[i].green, 10) << 10) |
> > +                     drm_color_lut_extract(lut[i].blue, 10);
> > +             writel(word, vop->lut_regs + i * 4);
> > +     }
> > +}
> > +
> > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > +                            struct drm_crtc_state *old_crtc_state)
> > +{
> > +     unsigned int idle;
> > +     int ret;
> > +
>
> How about:
>
>         if (!vop->lut_regs)
>                 return;
>
> here and then you can remove that condition above the 2 callsites
>

Yes, sounds good.

> > +     /*
> > +      * In order to write the LUT to the internal memory,
> > +      * we need to first make sure the dsp_lut_en bit is cleared.
> > +      */
> > +     spin_lock(&vop->reg_lock);
> > +     VOP_REG_SET(vop, common, dsp_lut_en, 0);
> > +     vop_cfg_done(vop);
> > +     spin_unlock(&vop->reg_lock);
> > +
> > +     /*
> > +      * If the CRTC is not active, dsp_lut_en will not get cleared.
> > +      * Apparently we still need to do the above step to for
> > +      * gamma correction to be disabled.
> > +      */
> > +     if (!crtc->state->active)
> > +             return;
> > +

I have realized that the above might no longer be needed,
given we are now using atomic_enable and atomic_begin.

Not sure if the CRTC is supposed to clear its GAMMA
when disabled.

> > +     ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> > +                              idle, !idle, 5, 30 * 1000);
> > +     if (ret) {
> > +             DRM_DEV_ERROR(vop->dev, "display LUT RAM enable timeout!\n");
> > +             return;
> > +     }
> > +
> > +     if (crtc->state->gamma_lut &&
> > +        (!old_crtc_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> > +                                     old_crtc_state->gamma_lut->base.id))) {
>
> Silly question, but isn't the second part of this check redundant since you need
> color_mgmt_changed || active_changed to get into this function?
>
> So maybe invert the conditional here and exit early (to save a level of
> indentation in the block below):
>

I took this from malidp_atomic_commit_update_gamma. I _believe_
the rational for this is that color_mgmt_changed can be set by re-setting
the gamma property, to the same property. But I admit I haven't
tested it's the case.

OTOH, it won't really affect much to re-write the table, if the user
requested a change.

>         if (!crtc->state->gamma_lut)
>                 return;
>

In any case, inverting the condition makes sense.

>         spin_lock(&vop->reg_lock);
>
>         vop_crtc_write_gamma_lut(vop, crtc);
>         VOP_REG_SET(vop, common, dsp_lut_en, 1);
>         vop_cfg_done(vop);
>
>         spin_unlock(&vop->reg_lock);
>
> > +
> > +             spin_lock(&vop->reg_lock);
> > +
> > +             vop_crtc_write_gamma_lut(vop, crtc);
> > +             VOP_REG_SET(vop, common, dsp_lut_en, 1);
> > +             vop_cfg_done(vop);
> > +
> > +             spin_unlock(&vop->reg_lock);
> > +     }
> > +}
> > +
> > +static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
> > +                                struct drm_crtc_state *old_crtc_state)
> > +{
> > +     struct vop *vop = to_vop(crtc);
> > +
> > +     /*
> > +      * Only update GAMMA if the 'active' flag is not changed,
> > +      * otherwise it's updated by .atomic_enable.
> > +      */
> > +     if (vop->lut_regs && crtc->state->color_mgmt_changed &&
> > +         !crtc->state->active_changed)
> > +             vop_crtc_gamma_set(vop, crtc, old_crtc_state);
> > +}
> > +
> >  static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
> >                                  struct drm_crtc_state *old_state)
> >  {
> > @@ -1075,6 +1154,14 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
> >               return;
> >       }
> >
> > +     /*
> > +      * If we have a GAMMA LUT in the state, then let's make sure
> > +      * it's updated. We might be coming out of suspend,
> > +      * which means the LUT internal memory needs to be re-written.
> > +      */
> > +     if (vop->lut_regs && crtc->state->gamma_lut)
> > +             vop_crtc_gamma_set(vop, crtc, old_state);
> > +
> >       mutex_lock(&vop->vop_lock);
> >
> >       WARN_ON(vop->event);
> > @@ -1191,6 +1278,26 @@ static void vop_wait_for_irq_handler(struct vop *vop)
> >       synchronize_irq(vop->irq);
> >  }
> >
> > +static int vop_crtc_atomic_check(struct drm_crtc *crtc,
> > +                              struct drm_crtc_state *crtc_state)
> > +{
> > +     struct vop *vop = to_vop(crtc);
> > +
> > +     if (vop->lut_regs && crtc_state->color_mgmt_changed &&
> > +         crtc_state->gamma_lut) {
> > +             unsigned int len;
> > +
> > +             len = drm_color_lut_size(crtc_state->gamma_lut);
> > +             if (len != crtc->gamma_size) {
> > +                     DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> > +                                   len, crtc->gamma_size);
> > +                     return -EINVAL;
> > +             }
>
> Overflow is avoided in drm_mode_gamma_set_ioctl(), so I don't think you need
> this function.
>

But that only applies to the legacy path. Isn't this needed to ensure
a gamma blob
has the right size?

Thanks,
Ezequiel

Re: [PATCH v4 2/3] drm/rockchip: Add optional support for CRTC gamma LUT

[Sean Paul]

/snip

> > > +static void vop_crtc_write_gamma_lut(struct vop *vop, struct drm_crtc *crtc)
> > > +{
> > > +     struct drm_color_lut *lut = crtc->state->gamma_lut->data;
> > > +     unsigned int i;
> > > +
> > > +     for (i = 0; i < crtc->gamma_size; i++) {
> > > +             u32 word;
> > > +
> > > +             word = (drm_color_lut_extract(lut[i].red, 10) << 20) |
> > > +                    (drm_color_lut_extract(lut[i].green, 10) << 10) |
> > > +                     drm_color_lut_extract(lut[i].blue, 10);
> > > +             writel(word, vop->lut_regs + i * 4);
> > > +     }
> > > +}
> > > +
> > > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > > +                            struct drm_crtc_state *old_crtc_state)
> > > +{
> > > +     unsigned int idle;
> > > +     int ret;
> > > +
> >
> > How about:
> >
> >         if (!vop->lut_regs)
> >                 return;
> >
> > here and then you can remove that condition above the 2 callsites
> >
> 
> Yes, sounds good.
> 
> > > +     /*
> > > +      * In order to write the LUT to the internal memory,
> > > +      * we need to first make sure the dsp_lut_en bit is cleared.
> > > +      */
> > > +     spin_lock(&vop->reg_lock);
> > > +     VOP_REG_SET(vop, common, dsp_lut_en, 0);
> > > +     vop_cfg_done(vop);
> > > +     spin_unlock(&vop->reg_lock);
> > > +
> > > +     /*
> > > +      * If the CRTC is not active, dsp_lut_en will not get cleared.
> > > +      * Apparently we still need to do the above step to for
> > > +      * gamma correction to be disabled.
> > > +      */
> > > +     if (!crtc->state->active)
> > > +             return;
> > > +
> 
> I have realized that the above might no longer be needed,
> given we are now using atomic_enable and atomic_begin.
> 
> Not sure if the CRTC is supposed to clear its GAMMA
> when disabled.
> 

Yep, good catch. Since we use commit_tail_rpm, atomic_begin won't be called in
the disable path.

> > > +     ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> > > +                              idle, !idle, 5, 30 * 1000);
> > > +     if (ret) {
> > > +             DRM_DEV_ERROR(vop->dev, "display LUT RAM enable timeout!\n");
> > > +             return;
> > > +     }
> > > +
> > > +     if (crtc->state->gamma_lut &&
> > > +        (!old_crtc_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> > > +                                     old_crtc_state->gamma_lut->base.id))) {
> >
> > Silly question, but isn't the second part of this check redundant since you need
> > color_mgmt_changed || active_changed to get into this function?
> >
> > So maybe invert the conditional here and exit early (to save a level of
> > indentation in the block below):
> >
> 
> I took this from malidp_atomic_commit_update_gamma. I _believe_
> the rational for this is that color_mgmt_changed can be set by re-setting
> the gamma property, to the same property. But I admit I haven't
> tested it's the case.
> 
> OTOH, it won't really affect much to re-write the table, if the user
> requested a change.
> 

color_mgmt_changed is based on the output of drm_property_replace_blob() which
should return false if the blob is unchanged. So I don't think that case is
possible. 

Taking this a step further, this check could even be damaging since something
in the atomic check path could set color_mgmt_changed in order to explicitly
trigger a lut write and we'd be skipping it with the id check.


> >         if (!crtc->state->gamma_lut)
> >                 return;
> >
> 
> In any case, inverting the condition makes sense.
> 
> >         spin_lock(&vop->reg_lock);
> >
> >         vop_crtc_write_gamma_lut(vop, crtc);
> >         VOP_REG_SET(vop, common, dsp_lut_en, 1);
> >         vop_cfg_done(vop);
> >
> >         spin_unlock(&vop->reg_lock);
> >
> > > +
> > > +             spin_lock(&vop->reg_lock);
> > > +
> > > +             vop_crtc_write_gamma_lut(vop, crtc);
> > > +             VOP_REG_SET(vop, common, dsp_lut_en, 1);
> > > +             vop_cfg_done(vop);
> > > +
> > > +             spin_unlock(&vop->reg_lock);
> > > +     }
> > > +}

/snip

> > > +static int vop_crtc_atomic_check(struct drm_crtc *crtc,
> > > +                              struct drm_crtc_state *crtc_state)
> > > +{
> > > +     struct vop *vop = to_vop(crtc);
> > > +
> > > +     if (vop->lut_regs && crtc_state->color_mgmt_changed &&
> > > +         crtc_state->gamma_lut) {
> > > +             unsigned int len;
> > > +
> > > +             len = drm_color_lut_size(crtc_state->gamma_lut);
> > > +             if (len != crtc->gamma_size) {
> > > +                     DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> > > +                                   len, crtc->gamma_size);
> > > +                     return -EINVAL;
> > > +             }
> >
> > Overflow is avoided in drm_mode_gamma_set_ioctl(), so I don't think you need
> > this function.
> >
> 
> But that only applies to the legacy path. Isn't this needed to ensure
> a gamma blob
> has the right size?

Yeah, good point, we check the element size in the atomic path, but not the max
size. I haven't looked at enough color lut stuff to have an opinion whether this
check would be useful in a helper function or not, something to consider, I
suppose.

Sean

> 
> Thanks,
> Ezequiel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v4 2/3] drm/rockchip: Add optional support for CRTC gamma LUT

[Ilia Mirkin]

On Thu, Oct 10, 2019 at 12:01 PM Sean Paul <sean@poorly.run> wrote:
> > > > +static int vop_crtc_atomic_check(struct drm_crtc *crtc,
> > > > +                              struct drm_crtc_state *crtc_state)
> > > > +{
> > > > +     struct vop *vop = to_vop(crtc);
> > > > +
> > > > +     if (vop->lut_regs && crtc_state->color_mgmt_changed &&
> > > > +         crtc_state->gamma_lut) {
> > > > +             unsigned int len;
> > > > +
> > > > +             len = drm_color_lut_size(crtc_state->gamma_lut);
> > > > +             if (len != crtc->gamma_size) {
> > > > +                     DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> > > > +                                   len, crtc->gamma_size);
> > > > +                     return -EINVAL;
> > > > +             }
> > >
> > > Overflow is avoided in drm_mode_gamma_set_ioctl(), so I don't think you need
> > > this function.
> > >
> >
> > But that only applies to the legacy path. Isn't this needed to ensure
> > a gamma blob
> > has the right size?
>
> Yeah, good point, we check the element size in the atomic path, but not the max
> size. I haven't looked at enough color lut stuff to have an opinion whether this
> check would be useful in a helper function or not, something to consider, I
> suppose.

Some implementations support multiple sizes (e.g. 256 and 1024) but
not anything in between. It would be difficult to expose this
generically, I would imagine. The 256 size is kind of special, since
basically all legacy usage assumes that 256 is the one true quantity
of LUT entries...

  -ilia

Re: [PATCH v4 2/3] drm/rockchip: Add optional support for CRTC gamma LUT

[Ville Syrjälä]

On Thu, Oct 10, 2019 at 12:23:05PM -0400, Ilia Mirkin wrote:
> On Thu, Oct 10, 2019 at 12:01 PM Sean Paul <sean@poorly.run> wrote:
> > > > > +static int vop_crtc_atomic_check(struct drm_crtc *crtc,
> > > > > +                              struct drm_crtc_state *crtc_state)
> > > > > +{
> > > > > +     struct vop *vop = to_vop(crtc);
> > > > > +
> > > > > +     if (vop->lut_regs && crtc_state->color_mgmt_changed &&
> > > > > +         crtc_state->gamma_lut) {
> > > > > +             unsigned int len;
> > > > > +
> > > > > +             len = drm_color_lut_size(crtc_state->gamma_lut);
> > > > > +             if (len != crtc->gamma_size) {
> > > > > +                     DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> > > > > +                                   len, crtc->gamma_size);
> > > > > +                     return -EINVAL;
> > > > > +             }
> > > >
> > > > Overflow is avoided in drm_mode_gamma_set_ioctl(), so I don't think you need
> > > > this function.
> > > >
> > >
> > > But that only applies to the legacy path. Isn't this needed to ensure
> > > a gamma blob
> > > has the right size?
> >
> > Yeah, good point, we check the element size in the atomic path, but not the max
> > size. I haven't looked at enough color lut stuff to have an opinion whether this
> > check would be useful in a helper function or not, something to consider, I
> > suppose.
> 
> Some implementations support multiple sizes (e.g. 256 and 1024) but
> not anything in between. It would be difficult to expose this
> generically, I would imagine.
> The 256 size is kind of special, since
> basically all legacy usage assumes that 256 is the one true quantity
> of LUT entries...

What we do currently in i915 is:
crtc->gamma_size = 256
GAMMA_LUT_SIZE = platform specific (256, 129, 257, 2^10, or 2^18+1 (lol))
DEGAMMA_LUT_SIZE = platform specific (0, 33, 65, or 2^10)

i915 will accept:
- gamma lut of size 256, iff ctm==NULL and degamma==NULL (the so
  called "legacy gamma" mode)
- (de)gamma_lut of size (DE)GAMMA_LUT_SIZE if it passes the
  checks done by drm_color_lut_check()

Ie. just one or two gamma modes per platform is exposed. And that's
about all we can do with the current uapi even though our hardware
supports many more modes.

The resulting precision, interpolation vs. truncation behaviour,
and handling of out of gamut values are all totally unspecified
and userspace just has to make a guess.

We also cheat with the 2^10 sized LUTs a bit due to the hw sharing
the same LUT for gamma and degamma, and so if you enable both at
the same time we throw away every second entry and each stage
only gets a 2^9 entry LUT in the end.

Oh and for the 2^18+1 monstrosity we cheat even more and
throw away ~99.8% of the entries :(


This here was my idea for extending the uapi so that we
could expose the full hw capabilities and let userspace
decide which mode suits it best without having to guess
what it'll get:
https://github.com/vsyrjala/linux/commits/gamma_mode_prop

Maybe in a few years I'll find time to get back to it...

-- 
Ville Syrjälä
Intel