Message-ID: <9736eadf6a9d8e97eef919c6b3d88828@manjaro.org>
Date: Wed, 24 Jul 2024 06:55:52 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Piotr Zalewski <pZ010001011111@...ton.me>
Cc: linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, dri-devel@...ts.freedesktop.org,
 daniel@...ll.ch, airlied@...il.com, tzimmermann@...e.de, mripard@...nel.org,
 maarten.lankhorst@...ux.intel.com, andy.yan@...k-chips.com, heiko@...ech.de,
 hjc@...k-chips.com
Subject: Re: [PATCH RESEND] rockchip/drm: vop2: add support for gamma LUT

Hello Piotr,

On 2024-07-22 20:26, Piotr Zalewski wrote:
> Thank you for the comment/review. Your code styling suggestions are
> naturally justified. Also, thanks to your comment, I noticed one extra
> thing. Namely, in my patch, gamma LUT write happens outside of the vop2
> lock - which should happen inside of the lock.

I haven't had enough time to look at the patch code in detail, but doing
the LUT adjustment inside the lock certainly makes sense.  If I get a
chance, I'll perform a detailed review of your patch.

> I'm not sure in what form should I supply corrections to my patch now.
> Should it be whole new patch with said fixes, or a patch on top of my
> patch?

You should send the v2, tagged as "[PATCH v2]", with the described fixes
applied, and with the description of changes in the v2 in the patch 
notes.
You may want to have a look at some examples of such v2 submissions, [1]
and it's the best to produce patch notes using git-notes(1). [2]

[1] 
https://lore.kernel.org/linux-rockchip/bdb60f1f793166cd65f58ab7aea025347076019c.1719679068.git.dsimic@manjaro.org/
[2] https://git-scm.com/docs/git-notes


> On Monday, July 22nd, 2024 at 8:37 AM, Dragan Simic 
> <dsimic@...jaro.org> wrote:
> 
>> Hello Piotr,
>> 
>> Thanks for the patch. Please see a few general comments below.
>> 
>> On 2024-07-21 12:06, Piotr Zalewski wrote:
>> 
>> > Add support for gamma LUT in VOP2 driver. The implementation is based
>> > on
>> > the one found in VOP driver and modified to be compatible with VOP2.
>> > Blue
>> > and red channels in gamma LUT register write were swapped with respect
>> > to
>> > how gamma LUT values are written in VOP. Write of the current video
>> > port id
>> > to VOP2_SYS_LUT_PORT_SEL register was added before the write of
>> > DSP_LUT_EN
>> > bit. Gamma size is set and drm color management is enabled for each
>> > video
>> > port's CRTC except ones which have no associated device. Tested on
>> > RK3566
>> > (Pinetab2).
>> >
>> > Signed-off-by: Piotr Zalewski pZ010001011111@...ton.me
>> > ---
>> >
>> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> > index 9873172e3fd3..16abdc4a59a8 100644
>> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> > @@ -278,6 +278,15 @@ static u32 vop2_readl(struct vop2 *vop2, u32
>> > offset)
>> > return val;
>> > }
>> >
>> > +static u32 vop2_vp_read(struct vop2_video_port *vp, u32 offset)
>> > +{
>> > + u32 val;
>> > +
>> > + regmap_read(vp->vop2->map, vp->data->offset + offset, &val);
>> > +
>> > + return val;
>> > +}
>> > +
>> > static void vop2_win_write(const struct vop2_win *win, unsigned int
>> > reg, u32 v)
>> > {
>> > regmap_field_write(win->reg[reg], v);
>> > @@ -1482,6 +1491,97 @@ static bool vop2_crtc_mode_fixup(struct drm_crtc
>> > *crtc,
>> > return true;
>> > }
>> >
>> > +static bool vop2_vp_dsp_lut_is_enabled(struct vop2_video_port *vp)
>> > +{
>> > + return (u32) (vop2_vp_read(vp, RK3568_VP_DSP_CTRL) &
>> > RK3568_VP_DSP_CTRL__DSP_LUT_EN) >
>> > + 0;
>> > +}
>> > +
>> > +static void vop2_vp_dsp_lut_enable(struct vop2_video_port *vp)
>> > +{
>> > + u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);
>> > +
>> > + dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_LUT_EN;
>> > + vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
>> > +}
>> > +
>> > +static void vop2_vp_dsp_lut_disable(struct vop2_video_port *vp)
>> > +{
>> > + u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);
>> > +
>> > + dsp_ctrl &= ~RK3568_VP_DSP_CTRL__DSP_LUT_EN;
>> > + vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
>> > +}
>> > +
>> > +static void vop2_crtc_write_gamma_lut(struct vop2 *vop2, struct
>> > drm_crtc *crtc)
>> > +{
>> > + const struct vop2_data *vop2_data = vop2->data;
>> > + const struct vop2_video_port *vp = to_vop2_video_port(crtc);
>> > + const struct vop2_video_port_data *vp_data = &vop2_data->vp[vp->id];
>> 
>> 
>> Perhaps vop2_data could be dropped as a separate variable.
>> 
>> > +
>> > + struct drm_color_lut *lut = crtc->state->gamma_lut->data;
>> > + unsigned int i, bpc = ilog2(vp_data->gamma_lut_len);
>> > + u32 word;
>> > +
>> > + for (i = 0; i < crtc->gamma_size; i++) {
>> > + word = (drm_color_lut_extract(lut[i].blue, bpc) << (2 * bpc)) |
>> > + (drm_color_lut_extract(lut[i].green, bpc) << bpc) |
>> > + drm_color_lut_extract(lut[i].red, bpc);
>> > +
>> > + writel(word, vop2->lut_regs + i * 4);
>> > + }
>> > +}
>> > +
>> > +static void vop2_crtc_gamma_set(struct vop2 *vop2, struct drm_crtc
>> > *crtc,
>> > + struct drm_crtc_state *old_state)
>> > +{
>> > + struct drm_crtc_state *state = crtc->state;
>> > + struct vop2_video_port vp = to_vop2_video_port(crtc);
>> > + u32 dsp_ctrl;
>> > + int ret;
>> > +
>> > + if (!vop2->lut_regs)
>> > + return;
>> > +
>> > + if (!state->gamma_lut) {
>> > + /
>> > + * To disable gamma (gamma_lut is null) or to write
>> > + * an update to the LUT, clear dsp_lut_en.
>> > + /
>> > + vop2_lock(vop2);
>> > +
>> > + vop2_vp_dsp_lut_disable(vp);
>> > +
>> > + vop2_cfg_done(vp);
>> > + vop2_unlock(vop2);
>> > + /
>> > + * In order to write the LUT to the internal memory,
>> > + * we need to first make sure the dsp_lut_en bit is cleared.
>> > + */
>> > + ret =
>> > + readx_poll_timeout(vop2_vp_dsp_lut_is_enabled, vp, dsp_ctrl,
>> > !dsp_ctrl, 5,
>> > + 30 * 1000);
>> 
>> 
>> It would look nicer to keep "ret =" and "readx_poll_timeout(..." in 
>> the
>> same line,
>> and to introduce line breaks later in the same line.
>> 
>> > +
>> > + if (ret) {
>> > + DRM_DEV_ERROR(vop2->dev, "display LUT RAM enable timeout!\n");
>> > + return;
>> > + }
>> > +
>> > + if (!state->gamma_lut)
>> > + return;
>> > + }
>> > +
>> > + vop2_crtc_write_gamma_lut(vop2, crtc);
>> > +
> 
> Here the gamma LUT write happens outside of the vop2 lock.
> 
>> > + vop2_lock(vop2);
>> > + vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id);
>> > +
>> > + vop2_vp_dsp_lut_enable(vp);
>> > +
>> > + vop2_cfg_done(vp);
>> > + vop2_unlock(vop2);
>> > +}
>> > +
>> > static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl)
>> > {
>> > struct rockchip_crtc_state *vcstate =
>> > to_rockchip_crtc_state(crtc->state);
>> > @@ -1925,6 +2025,7 @@ static void vop2_crtc_atomic_enable(struct
>> > drm_crtc *crtc,
>> > const struct vop2_data *vop2_data = vop2->data;
>> > const struct vop2_video_port_data *vp_data = &vop2_data->vp[vp->id];
>> > struct drm_crtc_state *crtc_state =
>> > drm_atomic_get_new_crtc_state(state, crtc);
>> > + struct drm_crtc_state *old_state =
>> > drm_atomic_get_old_crtc_state(state, crtc);
>> > struct rockchip_crtc_state *vcstate =
>> > to_rockchip_crtc_state(crtc->state);
>> > struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>> > unsigned long clock = mode->crtc_clock * 1000;
>> > @@ -2060,6 +2161,9 @@ static void vop2_crtc_atomic_enable(struct
>> > drm_crtc *crtc,
>> > drm_crtc_vblank_on(crtc);
>> >
>> > vop2_unlock(vop2);
>> > +
>> > + if (crtc->state->gamma_lut)
>> > + vop2_crtc_gamma_set(vop2, crtc, old_state);
>> > }
>> >
>> > static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
>> > @@ -2070,6 +2174,16 @@ static int vop2_crtc_atomic_check(struct
>> > drm_crtc *crtc,
>> > int nplanes = 0;
>> > struct drm_crtc_state *crtc_state =
>> > drm_atomic_get_new_crtc_state(state, crtc);
>> >
>> > + if (vp->vop2->lut_regs && crtc_state->color_mgmt_changed &&
>> > crtc_state->gamma_lut) {
>> > + unsigned int len = drm_color_lut_size(crtc_state->gamma_lut);
>> > +
>> > + if (len != crtc->gamma_size) {
>> > + DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
>> > + len, crtc->gamma_size);
>> > + return -EINVAL;
>> > + }
>> > + }
>> > +
>> > drm_atomic_crtc_state_for_each_plane(plane, crtc_state)
>> > nplanes++;
>> >
>> > @@ -2459,6 +2573,10 @@ static void vop2_setup_dly_for_windows(struct
>> > vop2 *vop2)
>> > static void vop2_crtc_atomic_begin(struct drm_crtc *crtc,
>> > struct drm_atomic_state *state)
>> > {
>> > + struct drm_crtc_state *crtc_state =
>> > drm_atomic_get_new_crtc_state(state,
>> > + crtc);
>> > + struct drm_crtc_state *old_crtc_state =
>> > drm_atomic_get_old_crtc_state(state,
>> > + crtc);
>> > struct vop2_video_port *vp = to_vop2_video_port(crtc);
>> > struct vop2 *vop2 = vp->vop2;
>> > struct drm_plane *plane;
>> > @@ -2482,6 +2600,9 @@ static void vop2_crtc_atomic_begin(struct
>> > drm_crtc *crtc,
>> > vop2_setup_layer_mixer(vp);
>> > vop2_setup_alpha(vp);
>> > vop2_setup_dly_for_windows(vop2);
>> > +
>> > + if (crtc_state->color_mgmt_changed && !crtc_state->active_changed)
>> > + vop2_crtc_gamma_set(vop2, crtc, old_crtc_state);
>> > }
>> >
>> > static void vop2_crtc_atomic_flush(struct drm_crtc *crtc,
>> > @@ -2791,6 +2912,14 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>> >
>> > drm_crtc_helper_add(&vp->crtc, &vop2_crtc_helper_funcs);
>> >
>> > + if (vop2->lut_regs && vp->crtc.dev != NULL) {
>> > + const struct vop2_video_port_data *vp_data =
>> > &vop2_data->vp[vp->id];
>> > +
>> > + drm_mode_crtc_set_gamma_size(&vp->crtc, vp_data->gamma_lut_len);
>> > + drm_crtc_enable_color_mgmt(&vp->crtc, 0, false,
>> > + vp_data->gamma_lut_len);
>> > + }
>> > +
>> > init_completion(&vp->dsp_hold_completion);
>> > }
>> >
>> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
>> > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
>> > index 615a16196aff..3a58b73fa876 100644
>> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
>> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
>> > @@ -394,6 +394,7 @@ enum dst_factor_mode {
>> > #define RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN BIT(15)
>> >
>> > #define RK3568_VP_DSP_CTRL__STANDBY BIT(31)
>> > +#define RK3568_VP_DSP_CTRL__DSP_LUT_EN BIT(28)
>> > #define RK3568_VP_DSP_CTRL__DITHER_DOWN_MODE BIT(20)
>> > #define RK3568_VP_DSP_CTRL__DITHER_DOWN_SEL GENMASK(19, 18)
>> > #define RK3568_VP_DSP_CTRL__DITHER_DOWN_EN BIT(17)

Powered by blists - more mailing lists