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

[Ezequiel Garcia <ezequiel@collabora.com>]

On Tue, 2019-06-18 at 02:15 -0300, Ezequiel Garcia wrote:
> 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@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).
> 

As we suspected, indeed setting the gamma lut in atomic_flush
is exposed to tearing, if the atomic API is used.

As Laurent suggested you, setting from atomic_commit_tail seems correct,
as an example you can see the malidp driver.

I'm preparing a v2 patch.

Thanks,
Ezequiel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel