Hi Jacopo,
Thanks for the review.
On Fri, 2019-06-21 at 10:22 +0200, Jacopo Mondi wrote:
> Hi Ezequiel,
> just a few minor comments. Thanks for this new iteration.
>
> On Tue, Jun 18, 2019 at 06:34:05PM -0300, Ezequiel Garcia wrote:
> > Add an optional CRTC gamma LUT support, and enable it on RK3288.
> > This is currently enabled via a separate address resource,
> > which needs to be specified in the devicetree.
> >
> > The address resource is required because on some SoCs, such as
> > RK3288, the LUT address is after the MMU address, and the latter
> > is supported by a different driver. This prevents the DRM driver
> > from requesting an entire register space.
> >
> > The current implementation works for RGB 10-bit tables, as that
> > is what seems to work on RK3288.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > ---
> > Changes from RFC:
> > * Request (an optional) address resource for the LUT.
> > * Drop support for RK3399, which doesn't seem to work
> > out of the box and needs more research.
> > * Support pass-thru setting when GAMMA_LUT is NULL.
> > * Add a check for the gamma size, as suggested by Ilia.
> > * Move gamma setting to atomic_commit_tail, as pointed
> > out by Jacopo/Laurent, is the correct way.
> > ---
> > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 3 +
> > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 106 ++++++++++++++++++++
> > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 7 ++
> > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 2 +
> > 4 files changed, 118 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index 1c69066b6894..bf9ad6240971 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -16,6 +16,7 @@
> > #include "rockchip_drm_fb.h"
> > #include "rockchip_drm_gem.h"
> > #include "rockchip_drm_psr.h"
> > +#include "rockchip_drm_vop.h"
> >
> > static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
> > struct drm_file *file,
> > @@ -128,6 +129,8 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
> >
> > drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >
> > + rockchip_drm_vop_gamma_set(old_state);
> > +
> > drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >
> > drm_atomic_helper_commit_planes(dev, old_state,
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index 12ed5265a90b..5b6edbe2673f 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -137,6 +137,7 @@ struct vop {
> >
> > uint32_t *regsbak;
> > void __iomem *regs;
> > + void __iomem *lut_regs;
> >
> > /* physical map length of vop register */
> > uint32_t len;
> > @@ -1153,6 +1154,94 @@ 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_write_gamma_lut(struct vop *vop, struct drm_crtc *crtc)
> > +{
> > + struct drm_color_lut *lut = crtc->state->gamma_lut->data;
> > + int i;
>
> unsigned i
>
Sure, my bad.
> > +
> > + for (i = 0; i < crtc->gamma_size; i++) {
> > + u32 word;
>
> here and below you could declare and initialize in one line. Matter of
> tastes, up to you.
>
> > +
> > + 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);
> > + }
> > +}
> > +
> > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
> > + struct drm_crtc_state *old_state)
> > +{
> > + int idle, ret, i;
>
> idle and i could be unsigned
>
Ditto.
> > +
> > + 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, 30 * 1000);
> > + if (ret)
> > + return;
> > +
> > + spin_lock(&vop->reg_lock);
> > +
> > + if (crtc->state->gamma_lut) {
> > + if (!old_state->gamma_lut || (crtc->state->gamma_lut->base.id !=
> > + old_state->gamma_lut->base.id))
> > + vop_crtc_write_gamma_lut(vop, crtc);
> > + } else {
>
> i could also be declared here...
>
> > + for (i = 0; i < crtc->gamma_size; i++) {
> > + u32 word;
> > +
> > + word = (i << 20) | (i << 10) | i;
> > + writel(word, vop->lut_regs + i * 4);
> > + }
>
> I might be confused, but are you here configuring a linear LUT table?
> Isn't this equivalent to have it disabled?
>
Like I replied Ilia, I couldn't find a way to disable gamma correction.
However, after some more experiments, seems I could and so we can
drop this linear table.
> > + }
> > +
> > + VOP_REG_SET(vop, common, dsp_lut_en, 1);
> > + vop_cfg_done(vop);
> > + spin_unlock(&vop->reg_lock);
> > +}
> > +
> > +static int vop_crtc_atomic_check(struct drm_crtc *crtc,
> > + struct drm_crtc_state *crtc_state)
> > +{
> > + struct vop *vop = to_vop(crtc);
> > +
> > + if (vop->lut_regs && crtc_state->color_mgmt_changed &&
> > + crtc_state->gamma_lut) {
> > + int len;
> > +
> > + len = drm_color_lut_size(crtc_state->gamma_lut);
> > + if (len != crtc->gamma_size) {
>
> Don't you accept LUT tables whose size is < that the maximum
> crtc->gamma_size ?
>
Well, the hardware expects a 1024 LUT (because the format is RGB 10-bit).
If we accept smaller tables, the kernel would have to interpolate,
which I think it's nasty and not too useful.
Let's leave this to userspace.
> > + DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
> > + len, crtc->gamma_size);
> > + return -EINVAL;
> > + }
> > + }
>
> Most (but not all) functions in this file have an empty line before the
> return closing the function.
>
Ditto.
> > + return 0;
> > +}
> > +
> > +void rockchip_drm_vop_gamma_set(struct drm_atomic_state *state)
> > +{
> > + struct drm_crtc_state *old_crtc_state;
> > + struct drm_crtc *crtc;
> > + int i;
>
> unsigned i ?
>
Ditto.
> I'm glad you've been able to verify atomic_flush was not the right
> place where to update the LUT tables, as my testing is limited to
> run kms++ tests and I didn't notice any particular artifact. Thanks
> for checking!
>
Well, I just added a mdelay in the gamma_set function to simulate
an artifact. With this implementation, and using the atomic API,
even with the delay, the result rendered is atomic :-)
Thanks again,
Ezequiel
