[RFC/WIP] drm/rockchip: Support CRTC gamma LUT

[Ezequiel Garcia]

On Mon, 2019-06-17 at 12:06 +0200, Jacopo Mondi wrote:
> Hi Ezequiel,
>    one small question, as I'm working on supporting gamma LUT for
> rcar-du as well, and there's one point not totally clear to me
> 
> 
> On Thu, Jun 13, 2019 at 04:22:44PM -0300, Ezequiel Garcia wrote:
> > Add CRTC gamma LUT configuration on RK3288 and RK3399.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
> > ---
> > This patch seems to work well on RK3288, but produces
> > a distorted output on RK3399. I was hoping
> > someone could have any idea, so we can support both
> > platforms.
> > 
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 87 +++++++++++++++++++++
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  2 +
> >  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |  4 +
> >  drivers/gpu/drm/rockchip/rockchip_vop_reg.h |  1 +
> >  4 files changed, 94 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index 12ed5265a90b..8381679c1045 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -38,6 +38,8 @@
> >  #include "rockchip_drm_vop.h"
> >  #include "rockchip_rgb.h"
> > 
> > +#define VOP_GAMMA_LUT_SIZE 1024
> > +
> >  #define VOP_WIN_SET(vop, win, name, v) \
> >  		vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name)
> >  #define VOP_SCL_SET(vop, win, name, v) \
> > @@ -137,6 +139,7 @@ struct vop {
> > 
> >  	uint32_t *regsbak;
> >  	void __iomem *regs;
> > +	void __iomem *lut_regs;
> > 
> >  	/* physical map length of vop register */
> >  	uint32_t len;
> > @@ -1153,6 +1156,46 @@ 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_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > +			       struct drm_crtc_state *state)
> > +{
> > +	struct drm_color_lut *lut;
> > +	int i, idle, ret;
> > +
> > +	if (!state->gamma_lut)
> > +		return;
> > +	lut = state->gamma_lut->data;
> > +
> > +	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, 10 * 30000);
> > +	if (ret)
> > +		return;
> > +
> > +	spin_lock(&vop->reg_lock);
> > +	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);
> > +	}
> > +
> > +	VOP_REG_SET(vop, common, dsp_lut_en, 1);
> > +	vop_cfg_done(vop);
> > +	spin_unlock(&vop->reg_lock);
> > +}
> > +
> >  static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
> >  				  struct drm_crtc_state *old_crtc_state)
> >  {
> > @@ -1201,6 +1244,9 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
> >  		drm_flip_work_queue(&vop->fb_unref_work, old_plane_state->fb);
> >  		set_bit(VOP_PENDING_FB_UNREF, &vop->pending);
> >  	}
> > +
> > +	if (vop->lut_regs && crtc->state->color_mgmt_changed)
> > +		vop_crtc_gamma_set(vop, crtc, crtc->state);
> 
> Which one is the right point when to call LUT update functions?
> 
> I initially added my callback in atomic_flush as you did here, mostly
> because I've found examples in other drivers in drm and went in
> cargo cult mode. I've been then suggested by Laurent that atomic_flush()
> might not be the right place where to do so, as it gets called after
> the plane updates (iirc, the DRM atomic API is not something I'm that
> familiar with yet).
> 
> So I moved my LUT update function in the atomic_commit_tail callback,
> which is meant to actually commit a CRTC to the hw.
> 
> What's your opinion on this?
> 

I have to admit this is not exactly clear to me either.

Let me make sure I understand the issue. You are concerned about
getting some tearing if the CRTC gamma LUT is affected
in the atomic_flush?

If that's the case, it shouldn't be too hard to confirm (I think).

Thanks,
Eze