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 <ezequ...@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
