On Friday, July 26th, 2024 at 10:55 AM, Andy Yan <andy.yan@xxxxxxxxxxxxxx> wrote: > Hi Piotr Hi Andy! > Thanks for your work. > > On 7/25/24 06:05, 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). > > > > Helped-by: Dragan Simic dsimic@xxxxxxxxxxx > > Signed-off-by: Piotr Zalewski pZ010001011111@xxxxxxxxx > > --- > > > > Notes: > > Changes in v3: > > - v3 is patch v2 "resend", by mistake the incremental patch were > > sent in v2 > > > > Changes in v2: > > - Apply code styling corrections [1] > > - Move gamma LUT write inside the vop2 lock > > > > Link to v2: https://lore.kernel.org/linux-rockchip/Hk03HDb6wSSHWtEFZHUye06HR0-9YzP5nCHx9A8_kHzWSZawDrU1o1pjEGkCOJFoRg0nTB4BWEv6V0XBOjF4-0Mj44lp2TrjaQfnytzp-Pk=@proton.me/T/#u > > Link to v1: https://lore.kernel.org/linux-rockchip/9736eadf6a9d8e97eef919c6b3d88828@xxxxxxxxxxx/T/#t > > > > [1] https://lore.kernel.org/linux-rockchip/d019761504b540600d9fc7a585d6f95f@xxxxxxxxxxx/ > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > > index 9873172e3fd3..37fcf544a5fd 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,95 @@ 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_video_port *vp = to_vop2_video_port(crtc); > > + const struct vop2_video_port_data *vp_data = &vop2->data->vp[vp->id]; > > + > > + 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) { > > > What's the purpose of checking !state->gamma_lut here, > > and you check it again at the end for return. > This makes me very confused. I understood it this way - since the vop2 lock is unlocked after disabling gamma LUT, the CRTC state can change while waiting for DSP_LUT_EN bit to be unset. With the change I sent in response to Daniel's reply, gamma LUT state modification should now be fully atomic so there shouldn't be a need for the second state check there anymore (if my logic is incorrect please explain). > > + /* > > + * 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); > > + > > + if (ret) { > > + DRM_DEV_ERROR(vop2->dev, "display LUT RAM enable timeout!\n"); > > + return; > > + } > > + > > + if (!state->gamma_lut) > > + return; > > + } > > + > > + > > + vop2_lock(vop2); > > + vop2_crtc_write_gamma_lut(vop2, crtc); > > + vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id); > > > We should select lut_port befor write gamma. > > And there is one thing to note here is that: > There is only one gamma for rk3568/rk3566, that means only one > of the three VPs can exclusively use it at a time, so we need > a exclusive check for rk3568/rk3566. > > For rk3588 and the new soc, there is no such limination. > > The another thing to note is that for rk35588 and other new soc, no need to disable dsp_lut before write it. Thank you for the clarification! I will change my patch according to these facts. > > + > > + 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 +2023,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 +2159,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 +2172,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 +2571,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 +2598,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 +2910,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) Best regards, Piotr Zalewski
