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@xxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
