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

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