[PATCH 0/3] RK3288 Gamma LUT

[PATCH 0/3] RK3288 Gamma LUT

[Ezequiel Garcia]

Let's support Gamma LUT configuration on RK3288 SoCs.

In order to do so, this series adds a new and optional
address resource.
    
A separate address resource is required because on this RK3288,
the LUT address is after the MMU address, which is requested
by the iommu 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.

This has been tested on Rock2 Square board, using
a hacked 'modetest' tool, with legacy and atomic APIs. 

Thanks,
Eze

Changes from RFC:
* Request (an optional) address resource for the LUT.
* Add devicetree changes.
* 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.

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         |  10 +-
 arch/arm/boot/dts/rk3288.dtsi                 |   6 +-
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |   3 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 106 ++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h   |   7 ++
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c   |   2 +
 6 files changed, 131 insertions(+), 3 deletions(-)

-- 
2.20.1

[PATCH 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>
---
 .../bindings/display/rockchip/rockchip-vop.txt         | 10 +++++++++-
 1 file changed, 9 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..97ad78cc7e03 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
@@ -20,6 +20,13 @@ 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.
+
+- reg-names: "base" for the base register space. If present, the CRTC
+	gamma LUT name should be "lut".
+
 - 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 +55,8 @@ Example:
 SoC specific DT entry:
 	vopb: vopb@ff930000 {
 		compatible = "rockchip,rk3288-vop";
-		reg = <0xff930000 0x19c>;
+		reg = <0x0 0xff930000 0x0 0x19c>, <0x0 0xff931000 0x0 0x1000>;
+		reg-names = "base", "lut";
 		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.20.1

[PATCH 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 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  |   3 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 106 ++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   7 ++
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   2 +
 4 files changed, 118 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 1c69066b6894..bf9ad6240971 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -16,6 +16,7 @@
 #include "rockchip_drm_fb.h"
 #include "rockchip_drm_gem.h"
 #include "rockchip_drm_psr.h"
+#include "rockchip_drm_vop.h"
 
 static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
 				 struct drm_file *file,
@@ -128,6 +129,8 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
+	rockchip_drm_vop_gamma_set(old_state);
+
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
 	drm_atomic_helper_commit_planes(dev, old_state,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 12ed5265a90b..5b6edbe2673f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -137,6 +137,7 @@ struct vop {
 
 	uint32_t *regsbak;
 	void __iomem *regs;
+	void __iomem *lut_regs;
 
 	/* physical map length of vop register */
 	uint32_t len;
@@ -1153,6 +1154,94 @@ static void vop_wait_for_irq_handler(struct vop *vop)
 	synchronize_irq(vop->irq);
 }
 
+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;
+	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_state)
+{
+	int idle, ret, i;
+
+	spin_lock(&vop->reg_lock);
+	VOP_REG_SET(vop, common, dsp_lut_en, 0);
+	vop_cfg_done(vop);
+	spin_unlock(&vop->reg_lock);
+
+	ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
+			   idle, !idle, 5, 30 * 1000);
+	if (ret)
+		return;
+
+	spin_lock(&vop->reg_lock);
+
+	if (crtc->state->gamma_lut) {
+		if (!old_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
+					      old_state->gamma_lut->base.id))
+			vop_crtc_write_gamma_lut(vop, crtc);
+	} else {
+		for (i = 0; i < crtc->gamma_size; i++) {
+			u32 word;
+
+			word = (i << 20) | (i << 10) | i;
+			writel(word, vop->lut_regs + i * 4);
+		}
+	}
+
+	VOP_REG_SET(vop, common, dsp_lut_en, 1);
+	vop_cfg_done(vop);
+	spin_unlock(&vop->reg_lock);
+}
+
+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) {
+		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;
+}
+
+void rockchip_drm_vop_gamma_set(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+		struct vop *vop = to_vop(crtc);
+
+		if (vop->lut_regs && crtc->state->color_mgmt_changed)
+			vop_crtc_gamma_set(vop, crtc, old_crtc_state);
+	}
+}
+
 static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 				  struct drm_crtc_state *old_crtc_state)
 {
@@ -1205,6 +1294,7 @@ 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_flush = vop_crtc_atomic_flush,
 	.atomic_enable = vop_crtc_atomic_enable,
 	.atomic_disable = vop_crtc_atomic_disable,
@@ -1323,6 +1413,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)
@@ -1480,6 +1571,10 @@ static int vop_create_crtc(struct vop *vop)
 		goto err_cleanup_planes;
 
 	drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs);
+	if (vop_data->lut_size) {
+		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
@@ -1776,6 +1871,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_byname(pdev, IORESOURCE_MEM, "lut");
+	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..bd1bcd5a14e9 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,7 @@ static inline int scl_vop_cal_lb_mode(int width, bool is_yuv)
 }
 
 extern const struct component_ops vop_component_ops;
+
+void rockchip_drm_vop_gamma_set(struct drm_atomic_state *state);
+
 #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 7b9c74750f6d..30d49eff3670 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -593,6 +593,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),
@@ -641,6 +642,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.20.1

[PATCH 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>
---
 arch/arm/boot/dts/rk3288.dtsi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index aa017abf4f42..dd40c189b1f0 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -1025,7 +1025,8 @@
 
 	vopb: vop@ff930000 {
 		compatible = "rockchip,rk3288-vop";
-		reg = <0x0 0xff930000 0x0 0x19c>;
+		reg = <0x0 0xff930000 0x0 0x19c>, <0x0 0xff931000 0x0 0x1000>;
+		reg-names = "base", "lut";
 		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";
@@ -1075,7 +1076,8 @@
 
 	vopl: vop@ff940000 {
 		compatible = "rockchip,rk3288-vop";
-		reg = <0x0 0xff940000 0x0 0x19c>;
+		reg = <0x0 0xff940000 0x0 0x19c>, <0x0 0xff941000 0x0 0x1000>;
+		reg-names = "base", "lut";
 		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.20.1

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

[Ilia Mirkin]

On Tue, Jun 18, 2019 at 5:43 PM Ezequiel Garcia <ezequiel@collabora.com> 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>
> ---
> 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.
> ---
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 12ed5265a90b..5b6edbe2673f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> +                              struct drm_crtc_state *old_state)
> +{
> +       int idle, ret, i;
> +
> +       spin_lock(&vop->reg_lock);
> +       VOP_REG_SET(vop, common, dsp_lut_en, 0);
> +       vop_cfg_done(vop);
> +       spin_unlock(&vop->reg_lock);
> +
> +       ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> +                          idle, !idle, 5, 30 * 1000);
> +       if (ret)
> +               return;
> +
> +       spin_lock(&vop->reg_lock);
> +
> +       if (crtc->state->gamma_lut) {
> +               if (!old_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> +                                             old_state->gamma_lut->base.id))
> +                       vop_crtc_write_gamma_lut(vop, crtc);
> +       } else {
> +               for (i = 0; i < crtc->gamma_size; i++) {
> +                       u32 word;
> +
> +                       word = (i << 20) | (i << 10) | i;
> +                       writel(word, vop->lut_regs + i * 4);
> +               }

Note - I'm not in any way familiar with the hardware, so take with a
grain of salt --

Could you just leave dsp_lut_en turned off in this case?

Cheers,

  -ilia

> +       }
> +
> +       VOP_REG_SET(vop, common, dsp_lut_en, 1);
> +       vop_cfg_done(vop);
> +       spin_unlock(&vop->reg_lock);
> +}

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

[Ezequiel Garcia]

On Tue, 2019-06-18 at 17:47 -0400, Ilia Mirkin wrote:
> On Tue, Jun 18, 2019 at 5:43 PM Ezequiel Garcia <ezequiel@collabora.com> 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>
> > ---
> > 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.
> > ---
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index 12ed5265a90b..5b6edbe2673f 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > +                              struct drm_crtc_state *old_state)
> > +{
> > +       int idle, ret, i;
> > +
> > +       spin_lock(&vop->reg_lock);
> > +       VOP_REG_SET(vop, common, dsp_lut_en, 0);
> > +       vop_cfg_done(vop);
> > +       spin_unlock(&vop->reg_lock);
> > +
> > +       ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> > +                          idle, !idle, 5, 30 * 1000);
> > +       if (ret)
> > +               return;
> > +
> > +       spin_lock(&vop->reg_lock);
> > +
> > +       if (crtc->state->gamma_lut) {
> > +               if (!old_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> > +                                             old_state->gamma_lut->base.id))
> > +                       vop_crtc_write_gamma_lut(vop, crtc);
> > +       } else {
> > +               for (i = 0; i < crtc->gamma_size; i++) {
> > +                       u32 word;
> > +
> > +                       word = (i << 20) | (i << 10) | i;
> > +                       writel(word, vop->lut_regs + i * 4);
> > +               }
> 
> Note - I'm not in any way familiar with the hardware, so take with a
> grain of salt --
> 
> Could you just leave dsp_lut_en turned off in this case?
> 

That was the first thing I tried :-)

It seems dsp_lut_en is not to enable the CRTC gamma LUT stage,
but to enable writing the gamma LUT to the internal RAM.

And the specs list no register to enable/disable the gamma LUT.

Thanks!
Eze

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

[Heiko Stübner]

Am Mittwoch, 19. Juni 2019, 00:09:57 CEST schrieb Ezequiel Garcia:
> On Tue, 2019-06-18 at 17:47 -0400, Ilia Mirkin wrote:
> > On Tue, Jun 18, 2019 at 5:43 PM Ezequiel Garcia <ezequiel@collabora.com> 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>
> > > ---
> > > 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.
> > > ---
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > index 12ed5265a90b..5b6edbe2673f 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > > +                              struct drm_crtc_state *old_state)
> > > +{
> > > +       int idle, ret, i;
> > > +
> > > +       spin_lock(&vop->reg_lock);
> > > +       VOP_REG_SET(vop, common, dsp_lut_en, 0);
> > > +       vop_cfg_done(vop);
> > > +       spin_unlock(&vop->reg_lock);
> > > +
> > > +       ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> > > +                          idle, !idle, 5, 30 * 1000);
> > > +       if (ret)
> > > +               return;
> > > +
> > > +       spin_lock(&vop->reg_lock);
> > > +
> > > +       if (crtc->state->gamma_lut) {
> > > +               if (!old_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> > > +                                             old_state->gamma_lut->base.id))
> > > +                       vop_crtc_write_gamma_lut(vop, crtc);
> > > +       } else {
> > > +               for (i = 0; i < crtc->gamma_size; i++) {
> > > +                       u32 word;
> > > +
> > > +                       word = (i << 20) | (i << 10) | i;
> > > +                       writel(word, vop->lut_regs + i * 4);
> > > +               }
> > 
> > Note - I'm not in any way familiar with the hardware, so take with a
> > grain of salt --
> > 
> > Could you just leave dsp_lut_en turned off in this case?
> > 
> 
> That was the first thing I tried :-)
> 
> It seems dsp_lut_en is not to enable the CRTC gamma LUT stage,
> but to enable writing the gamma LUT to the internal RAM.

I guess that warants a code comment stating this, so we don't end
up with well-meant cleanup patches in the future :-) .


> 
> And the specs list no register to enable/disable the gamma LUT.
> 
> Thanks!
> Eze
> 
> 

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

[Ezequiel Garcia]

On Wed, 2019-06-19 at 00:18 +0200, Heiko Stübner wrote:
> Am Mittwoch, 19. Juni 2019, 00:09:57 CEST schrieb Ezequiel Garcia:
> > On Tue, 2019-06-18 at 17:47 -0400, Ilia Mirkin wrote:
> > > On Tue, Jun 18, 2019 at 5:43 PM Ezequiel Garcia <ezequiel@collabora.com> 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>
> > > > ---
> > > > 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.
> > > > ---
> > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > > index 12ed5265a90b..5b6edbe2673f 100644
> > > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > > > +                              struct drm_crtc_state *old_state)
> > > > +{
> > > > +       int idle, ret, i;
> > > > +
> > > > +       spin_lock(&vop->reg_lock);
> > > > +       VOP_REG_SET(vop, common, dsp_lut_en, 0);
> > > > +       vop_cfg_done(vop);
> > > > +       spin_unlock(&vop->reg_lock);
> > > > +
> > > > +       ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> > > > +                          idle, !idle, 5, 30 * 1000);
> > > > +       if (ret)
> > > > +               return;
> > > > +
> > > > +       spin_lock(&vop->reg_lock);
> > > > +
> > > > +       if (crtc->state->gamma_lut) {
> > > > +               if (!old_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> > > > +                                             old_state->gamma_lut->base.id))
> > > > +                       vop_crtc_write_gamma_lut(vop, crtc);
> > > > +       } else {
> > > > +               for (i = 0; i < crtc->gamma_size; i++) {
> > > > +                       u32 word;
> > > > +
> > > > +                       word = (i << 20) | (i << 10) | i;
> > > > +                       writel(word, vop->lut_regs + i * 4);
> > > > +               }
> > > 
> > > Note - I'm not in any way familiar with the hardware, so take with a
> > > grain of salt --
> > > 
> > > Could you just leave dsp_lut_en turned off in this case?
> > > 
> > 
> > That was the first thing I tried :-)
> > 
> > It seems dsp_lut_en is not to enable the CRTC gamma LUT stage,
> > but to enable writing the gamma LUT to the internal RAM.
> 
> I guess that warants a code comment stating this, so we don't end
> up with well-meant cleanup patches in the future :-) .
> 

Sure, makes sense.

Any other feedback aside from this?

Thanks,
Ezequiel

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

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 2:43 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> Add the register specifier description for an
> optional gamma LUT address.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../bindings/display/rockchip/rockchip-vop.txt         | 10 +++++++++-
>  1 file changed, 9 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..97ad78cc7e03 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
> @@ -20,6 +20,13 @@ 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.
> +
> +- reg-names: "base" for the base register space. If present, the CRTC
> +       gamma LUT name should be "lut".

As per Rob Herring, current suggestion is to avoid reg-names when
possible.  The code should just look for the presence of a 2nd entry
and assume that if it's there that it's the lut range.  Full context:

https://lore.kernel.org/lkml/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@mail.gmail.com/

-Doug

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

[Ezequiel Garcia]

On Thu, 2019-06-20 at 09:43 -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 18, 2019 at 2:43 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > Add the register specifier description for an
> > optional gamma LUT address.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  .../bindings/display/rockchip/rockchip-vop.txt         | 10 +++++++++-
> >  1 file changed, 9 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..97ad78cc7e03 100644
> > --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
> > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
> > @@ -20,6 +20,13 @@ 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.
> > +
> > +- reg-names: "base" for the base register space. If present, the CRTC
> > +       gamma LUT name should be "lut".
> 
> As per Rob Herring, current suggestion is to avoid reg-names when
> possible.  The code should just look for the presence of a 2nd entry
> and assume that if it's there that it's the lut range.  Full context:
> 
> > https://lore.kernel.org/lkml/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@mail.gmail.com/
> 

Oh, that's news to me. I was assuming having reg-names was preferred.

Thanks for the feedback, I'll send a new version.

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

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 2:43 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> +                              struct drm_crtc_state *old_state)
> +{
> +       int idle, ret, i;
> +
> +       spin_lock(&vop->reg_lock);
> +       VOP_REG_SET(vop, common, dsp_lut_en, 0);
> +       vop_cfg_done(vop);
> +       spin_unlock(&vop->reg_lock);
> +
> +       ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> +                          idle, !idle, 5, 30 * 1000);
> +       if (ret)

Worth an error message?


> @@ -1205,6 +1294,7 @@ 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,

At first I was worried that there was a bug here since in the context
of dw_hdmi (an encoder) adding ".atomic_check" caused ".mode_fixup" to
stop getting called as per mode_fixup() in
"drivers/gpu/drm/drm_atomic_helper.c".

...but it seems like it's OK for CRTCs, so I think we're fine.


> @@ -1323,6 +1413,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,

Are there any issues in adding this ".gamma_set" property even though
we may or may not actually have the ability to set the gamma
(depending on whether or not the LUT register range was provided in
the device tree)?  I am a DRM noob but
drm_atomic_helper_legacy_gamma_set() is not a trivial little function
and now we'll be running it in some cases where we don't actually have
gamma.

I also notice that there's at least one bit of code that seems to
check if ".gamma_set" is NULL.  ...and if it is, it'll return -ENOSYS
right away.  Do we still properly return -ENOSYS on devices that don't
have the register range?

It seems like the absolute safest would be to have two copies of this
struct: one used for VOPs that have the range and one for VOPs that
don't.

...but possibly I'm just paranoid and as I've said I'm a clueless
noob.  If someone says it's fine to always provide the .gamma_set
property that's fine too.


>  static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
> @@ -1480,6 +1571,10 @@ static int vop_create_crtc(struct vop *vop)
>                 goto err_cleanup_planes;
>
>         drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs);
> +       if (vop_data->lut_size) {
> +               drm_mode_crtc_set_gamma_size(crtc, vop_data->lut_size);
> +               drm_crtc_enable_color_mgmt(crtc, 0, false, vop_data->lut_size);

Should we only do the above calls if we successfully mapped the resources?


> @@ -1776,6 +1871,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_byname(pdev, IORESOURCE_MEM, "lut");

As per comments in the bindings, shouldn't use the name "lut" but
should just pick resource #1.

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

[Jacopo Mondi]

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

Hi Ezequiel,
   just a few minor comments. Thanks for this new iteration.

On Tue, Jun 18, 2019 at 06:34:05PM -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>
> ---
> 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  |   3 +
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 106 ++++++++++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   7 ++
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   2 +
>  4 files changed, 118 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 1c69066b6894..bf9ad6240971 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -16,6 +16,7 @@
>  #include "rockchip_drm_fb.h"
>  #include "rockchip_drm_gem.h"
>  #include "rockchip_drm_psr.h"
> +#include "rockchip_drm_vop.h"
>
>  static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
>  				 struct drm_file *file,
> @@ -128,6 +129,8 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
>
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>
> +	rockchip_drm_vop_gamma_set(old_state);
> +
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>
>  	drm_atomic_helper_commit_planes(dev, old_state,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 12ed5265a90b..5b6edbe2673f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -137,6 +137,7 @@ struct vop {
>
>  	uint32_t *regsbak;
>  	void __iomem *regs;
> +	void __iomem *lut_regs;
>
>  	/* physical map length of vop register */
>  	uint32_t len;
> @@ -1153,6 +1154,94 @@ static void vop_wait_for_irq_handler(struct vop *vop)
>  	synchronize_irq(vop->irq);
>  }
>
> +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;
> +	int i;

unsigned i

> +
> +	for (i = 0; i < crtc->gamma_size; i++) {
> +		u32 word;

here and below you could declare and initialize in one line. Matter of
tastes, up to you.

> +
> +		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_state)
> +{
> +	int idle, ret, i;

idle and i could be unsigned

> +
> +	spin_lock(&vop->reg_lock);
> +	VOP_REG_SET(vop, common, dsp_lut_en, 0);
> +	vop_cfg_done(vop);
> +	spin_unlock(&vop->reg_lock);
> +
> +	ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> +			   idle, !idle, 5, 30 * 1000);
> +	if (ret)
> +		return;
> +
> +	spin_lock(&vop->reg_lock);
> +
> +	if (crtc->state->gamma_lut) {
> +		if (!old_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> +					      old_state->gamma_lut->base.id))
> +			vop_crtc_write_gamma_lut(vop, crtc);
> +	} else {

i could also be declared here...

> +		for (i = 0; i < crtc->gamma_size; i++) {
> +			u32 word;
> +
> +			word = (i << 20) | (i << 10) | i;
> +			writel(word, vop->lut_regs + i * 4);
> +		}

I might be confused, but are you here configuring a linear LUT table?
Isn't this equivalent to have it disabled?

> +	}
> +
> +	VOP_REG_SET(vop, common, dsp_lut_en, 1);
> +	vop_cfg_done(vop);
> +	spin_unlock(&vop->reg_lock);
> +}
> +
> +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) {
> +		int len;
> +
> +		len = drm_color_lut_size(crtc_state->gamma_lut);
> +		if (len != crtc->gamma_size) {

Don't you accept LUT tables whose size is < that the maximum
crtc->gamma_size ?

> +			DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> +				      len, crtc->gamma_size);
> +			return -EINVAL;
> +		}
> +	}

Most (but not all) functions in this file have an empty line before the
return closing the function.

> +	return 0;
> +}
> +
> +void rockchip_drm_vop_gamma_set(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *old_crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;

unsigned i ?

I'm glad you've been able to verify atomic_flush was not the right
place where to update the LUT tables, as my testing is limited to
run kms++ tests and I didn't notice any particular artifact. Thanks
for checking!


> +
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> +		struct vop *vop = to_vop(crtc);
> +
> +		if (vop->lut_regs && crtc->state->color_mgmt_changed)
> +			vop_crtc_gamma_set(vop, crtc, old_crtc_state);
> +	}
> +}
> +
>  static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
>  				  struct drm_crtc_state *old_crtc_state)
>  {
> @@ -1205,6 +1294,7 @@ 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_flush = vop_crtc_atomic_flush,
>  	.atomic_enable = vop_crtc_atomic_enable,
>  	.atomic_disable = vop_crtc_atomic_disable,
> @@ -1323,6 +1413,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)
> @@ -1480,6 +1571,10 @@ static int vop_create_crtc(struct vop *vop)
>  		goto err_cleanup_planes;
>
>  	drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs);
> +	if (vop_data->lut_size) {
> +		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
> @@ -1776,6 +1871,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_byname(pdev, IORESOURCE_MEM, "lut");
> +	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..bd1bcd5a14e9 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,7 @@ static inline int scl_vop_cal_lb_mode(int width, bool is_yuv)
>  }
>
>  extern const struct component_ops vop_component_ops;
> +
> +void rockchip_drm_vop_gamma_set(struct drm_atomic_state *state);
> +
>  #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 7b9c74750f6d..30d49eff3670 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> @@ -593,6 +593,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),
> @@ -641,6 +642,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.20.1
>

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

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

[Ezequiel Garcia]

On Thu, 2019-06-20 at 10:25 -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 18, 2019 at 2:43 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > +                              struct drm_crtc_state *old_state)
> > +{
> > +       int idle, ret, i;
> > +
> > +       spin_lock(&vop->reg_lock);
> > +       VOP_REG_SET(vop, common, dsp_lut_en, 0);
> > +       vop_cfg_done(vop);
> > +       spin_unlock(&vop->reg_lock);
> > +
> > +       ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> > +                          idle, !idle, 5, 30 * 1000);
> > +       if (ret)
> 
> Worth an error message?
> 

Sure.

> 
> > @@ -1205,6 +1294,7 @@ 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,
> 
> At first I was worried that there was a bug here since in the context
> of dw_hdmi (an encoder) adding ".atomic_check" caused ".mode_fixup" to
> stop getting called as per mode_fixup() in
> "drivers/gpu/drm/drm_atomic_helper.c".
> 
> ...but it seems like it's OK for CRTCs, so I think we're fine.
> 
> 
> > @@ -1323,6 +1413,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,
> 
> Are there any issues in adding this ".gamma_set" property even though
> we may or may not actually have the ability to set the gamma
> (depending on whether or not the LUT register range was provided in
> the device tree)?  I am a DRM noob but
> drm_atomic_helper_legacy_gamma_set() is not a trivial little function
> and now we'll be running it in some cases where we don't actually have
> gamma.
> 
> I also notice that there's at least one bit of code that seems to
> check if ".gamma_set" is NULL.  ...and if it is, it'll return -ENOSYS
> right away.  Do we still properly return -ENOSYS on devices that don't
> have the register range?
> 
> It seems like the absolute safest would be to have two copies of this
> struct: one used for VOPs that have the range and one for VOPs that
> don't.
> 
> ...but possibly I'm just paranoid and as I've said I'm a clueless
> noob.  If someone says it's fine to always provide the .gamma_set
> property that's fine too.
> 

Provided we do the suggestion below (setting gamma_size and enabling
color management) when lut_regs is set, then I think we are fine.

Before this change:

* GAMMA_LUT property doesn't exist, and so can't be set.
* DRM_IOCTL_MODE_SETGAMMA (legacy) return ENOSYS.

After this change, on platforms that doesn't support this:

* GAMMA_LUT property doesn't exist, and so can't be set.
* DRM_IOCTL_MODE_SETGAMMA (legacy) return EINVAL.

The only difference is the ENOSYS/EINVAL errno, which I doubt
will regress anything.

I don't think this difference deserves assigning (the legacy)
.gamma_set hook conditionally, which would make the
implementation too ugly.

> 
> >  static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
> > @@ -1480,6 +1571,10 @@ static int vop_create_crtc(struct vop *vop)
> >                 goto err_cleanup_planes;
> > 
> >         drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs);
> > +       if (vop_data->lut_size) {
> > +               drm_mode_crtc_set_gamma_size(crtc, vop_data->lut_size);
> > +               drm_crtc_enable_color_mgmt(crtc, 0, false, vop_data->lut_size);
> 
> Should we only do the above calls if we successfully mapped the resources?
> 

Yes, totally. See above.

> 
> > @@ -1776,6 +1871,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_byname(pdev, IORESOURCE_MEM, "lut");
> 
> As per comments in the bindings, shouldn't use the name "lut" but
> should just pick resource #1.

Yes.

Thanks a lot for the review,
Ezequiel

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

[Ezequiel Garcia]

Hi Jacopo,

Thanks for the review.

On Fri, 2019-06-21 at 10:22 +0200, Jacopo Mondi wrote:
> Hi Ezequiel,
>    just a few minor comments. Thanks for this new iteration.
> 
> On Tue, Jun 18, 2019 at 06:34:05PM -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>
> > ---
> > 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  |   3 +
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 106 ++++++++++++++++++++
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   7 ++
> >  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   2 +
> >  4 files changed, 118 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index 1c69066b6894..bf9ad6240971 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -16,6 +16,7 @@
> >  #include "rockchip_drm_fb.h"
> >  #include "rockchip_drm_gem.h"
> >  #include "rockchip_drm_psr.h"
> > +#include "rockchip_drm_vop.h"
> > 
> >  static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
> >  				 struct drm_file *file,
> > @@ -128,6 +129,8 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
> > 
> >  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > 
> > +	rockchip_drm_vop_gamma_set(old_state);
> > +
> >  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> > 
> >  	drm_atomic_helper_commit_planes(dev, old_state,
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index 12ed5265a90b..5b6edbe2673f 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -137,6 +137,7 @@ struct vop {
> > 
> >  	uint32_t *regsbak;
> >  	void __iomem *regs;
> > +	void __iomem *lut_regs;
> > 
> >  	/* physical map length of vop register */
> >  	uint32_t len;
> > @@ -1153,6 +1154,94 @@ static void vop_wait_for_irq_handler(struct vop *vop)
> >  	synchronize_irq(vop->irq);
> >  }
> > 
> > +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;
> > +	int i;
> 
> unsigned i
> 

Sure, my bad.

> > +
> > +	for (i = 0; i < crtc->gamma_size; i++) {
> > +		u32 word;
> 
> here and below you could declare and initialize in one line. Matter of
> tastes, up to you.
> 
> > +
> > +		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_state)
> > +{
> > +	int idle, ret, i;
> 
> idle and i could be unsigned
> 

Ditto.

> > +
> > +	spin_lock(&vop->reg_lock);
> > +	VOP_REG_SET(vop, common, dsp_lut_en, 0);
> > +	vop_cfg_done(vop);
> > +	spin_unlock(&vop->reg_lock);
> > +
> > +	ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop,
> > +			   idle, !idle, 5, 30 * 1000);
> > +	if (ret)
> > +		return;
> > +
> > +	spin_lock(&vop->reg_lock);
> > +
> > +	if (crtc->state->gamma_lut) {
> > +		if (!old_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> > +					      old_state->gamma_lut->base.id))
> > +			vop_crtc_write_gamma_lut(vop, crtc);
> > +	} else {
> 
> i could also be declared here...
> 
> > +		for (i = 0; i < crtc->gamma_size; i++) {
> > +			u32 word;
> > +
> > +			word = (i << 20) | (i << 10) | i;
> > +			writel(word, vop->lut_regs + i * 4);
> > +		}
> 
> I might be confused, but are you here configuring a linear LUT table?
> Isn't this equivalent to have it disabled?
> 

Like I replied Ilia, I couldn't find a way to disable gamma correction.

However, after some more experiments, seems I could and so we can
drop this linear table.

> > +	}
> > +
> > +	VOP_REG_SET(vop, common, dsp_lut_en, 1);
> > +	vop_cfg_done(vop);
> > +	spin_unlock(&vop->reg_lock);
> > +}
> > +
> > +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) {
> > +		int len;
> > +
> > +		len = drm_color_lut_size(crtc_state->gamma_lut);
> > +		if (len != crtc->gamma_size) {
> 
> Don't you accept LUT tables whose size is < that the maximum
> crtc->gamma_size ?
> 

Well, the hardware expects a 1024 LUT (because the format is RGB 10-bit).

If we accept smaller tables, the kernel would have to interpolate,
which I think it's nasty and not too useful.

Let's leave this to userspace.

> > +			DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> > +				      len, crtc->gamma_size);
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> Most (but not all) functions in this file have an empty line before the
> return closing the function.
> 

Ditto.

> > +	return 0;
> > +}
> > +
> > +void rockchip_drm_vop_gamma_set(struct drm_atomic_state *state)
> > +{
> > +	struct drm_crtc_state *old_crtc_state;
> > +	struct drm_crtc *crtc;
> > +	int i;
> 
> unsigned i ?
> 

Ditto.

> I'm glad you've been able to verify atomic_flush was not the right
> place where to update the LUT tables, as my testing is limited to
> run kms++ tests and I didn't notice any particular artifact. Thanks
> for checking!
> 

Well, I just added a mdelay in the gamma_set function to simulate
an artifact. With this implementation, and using the atomic API,
even with the delay, the result rendered is atomic :-)

Thanks again,
Ezequiel