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). Thanks, Eze _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel