On Friday, July 26th, 2024 at 1:31 PM, Daniel Stone <daniel@xxxxxxxxxxxxx> wrote:
> Hi Piotr,
Hi Daniel, sorry for delayed response.
>
> > static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl)
> > @@ -2152,6 +2127,9 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> >
> > vop2_post_config(crtc);
> >
> > + if (crtc->state->gamma_lut)
> > + vop2_crtc_gamma_set(vop2, crtc, old_state, &dsp_ctrl);
>
>
> I think this call should be unconditional, so that we correctly
> program LUT_DIS if there is no LUT set up during enable.
>
Noted
> > @@ -2599,8 +2575,17 @@ static void vop2_crtc_atomic_begin(struct drm_crtc *crtc,
> > 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);
> > + if (crtc_state->color_mgmt_changed && !crtc_state->active_changed) {
> > + u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);;
> > +
> > + vop2_lock(vop2);
> > +
> > + vop2_crtc_gamma_set(vop2, crtc, old_crtc_state, &dsp_ctrl);
> > +
> > + vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> > + vop2_cfg_done(vp);
> > + vop2_unlock(vop2);
> > + }
>
>
> Calling lock/set/write/done/unlock here seems like an anti-pattern;
> the cfg_done is already written in atomic_flush, so at least that part
> is not necessary.
Right sorry for sending such code. I wanted to demonstrate the idea.
> On platforms like RK3588, it looks like the new LUT can just be
> written directly from atomic_begin without needing to program
> DSP_CTRL, take locks, or synchronise against anything, so that should
> be an easy straight-line path.
>
> On platforms like RK3568, it would probably be better to set
> mode_changed when the colour management configuration changes. That
> will give you a good point to synchronise the cross-VOP dependencies
> (i.e. claim or release the shared LUT when it is being
> enabled/disabled), and also a hint to userspace that it is not going
> to be a seamless transition as the LUT is disabled, programmed, then
> re-enabled.
>
> I think this would end up in a net reduction of LoC as well as a net
> reduction of code weirdness.
Okay so if I understood you correctly you suggest setting mode_changed in
order to trigger full modeset (check->begin->flush->enable) to cleanly
handle the RK356x case and for RK3588 just write the LUT in begin and
don't do anything more.
I tried to do this but couldn't get the thing to work. It seems like
setting the mode_changed manually in atomic_check, messes something up
with the CRTC states. Namely, the retrieved new state in subsequent
atomic_begin call isn't the same state (as a result, flags
color_mgmt_changed and mode_changed are both false when they are checked
in atomic_begin which stops further gamma LUT reconfiguration). Below is
how I reworked this (included only parts which changed).
As already mentioned, in atomic check the mode_changed flag is set, then in
atomic begin gamma LUT is disabled and DSP_LUT_EN bit unset (or gamma LUT
is written directly based on if it's RK356x or not). Then in atomic_flush
video port is selected and gamma LUT is written. Gamma LUT is enabled in
atomic_enable.
Perhaps I'm missing something important, if so please hint what exactly.
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 9873172e3fd3..9c5ee2d85a58 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2051,6 +2092,13 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
clk_set_rate(vp->dclk, clock);
+ /**
+ * Enable gamma LUT in atomic enable
+ */
+ if (crtc_state->gamma_lut && (vop2->data->soc_id == 3566 ||
+ vop2->data->soc_id == 3568))
+ dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_LUT_EN;
+
vop2_post_config(crtc);
vop2_cfg_done(vp);
@@ -2062,6 +2110,39 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
vop2_unlock(vop2);
}
+static int vop2_crtc_atomic_check_gamma(struct vop2_video_port *vp,
+ struct drm_crtc *crtc,
+ struct drm_crtc_state *crtc_state)
+{
+ struct vop2 *vop2 = vp->vop2;
+ unsigned int len;
+
+ if (!vp->vop2->lut_regs || !crtc_state->color_mgmt_changed ||
+ !crtc_state->gamma_lut)
+ return 0;
+
+ 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;
+ }
+
+ if (!crtc_state->mode_changed && (vop2->data->soc_id == 3566 ||
+ vop2->data->soc_id == 3568)) {
+ int ret;
+
+ crtc_state->mode_changed = true;
+
+ ret = drm_atomic_helper_check_modeset(crtc->dev,
+ crtc_state->state);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -2069,6 +2150,11 @@ static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_plane *plane;
int nplanes = 0;
struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ int ret;
+
+ ret = vop2_crtc_atomic_check_gamma(vp, crtc, crtc_state);
+ if (ret)
+ return ret;
drm_atomic_crtc_state_for_each_plane(plane, crtc_state)
nplanes++;
@@ -2456,9 +2542,36 @@ static void vop2_setup_dly_for_windows(struct vop2 *vop2)
vop2_writel(vop2, RK3568_SMART_DLY_NUM, sdly);
}
+static void vop2_crtc_atomic_begin_gamma(struct vop2 *vop2,
+ struct drm_crtc *crtc) {
+ struct vop2_video_port *vp = to_vop2_video_port(crtc);
+ int ret;
+ u32 dsp_ctrl;
+
+ vop2_lock(vop2);
+ /**
+ * Gamma lut always has to be disabled in both cases. When gamma
+ * lut is enabled it needs to be disabled before writing (Applies
+ * only to RK356x SoCs).
+ */
+ vop2_vp_dsp_lut_disable(vp);
+
+ vop2_cfg_done(vp);
+ vop2_unlock(vop2);
+
+ 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");
+ }
+}
+
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 vop2_video_port *vp = to_vop2_video_port(crtc);
struct vop2 *vop2 = vp->vop2;
struct drm_plane *plane;
@@ -2482,13 +2595,50 @@ 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) {
+ /**
+ * When the SoC is RK356x gamma lut change always triggers
+ * mode_changed - proceed with gamma lut disable
+ */
+ if (crtc_state->mode_changed &&
+ (vop2->data->soc_id == 3566 ||
+ vop2->data->soc_id == 3568))
+ vop2_crtc_atomic_begin_gamma(vop2, crtc);
+ /**
+ * When the Soc isn't RK356x (eg. RK3588) gamma lut can be
+ * written directly
+ */
+ else if (crtc_state->gamma_lut)
+ vop2_crtc_write_gamma_lut(vop2, crtc);
+ }
+}
+
+static void vop2_crtc_atomic_flush_gamma(struct vop2 *vop2, struct vop2_video_port *vp, struct drm_crtc *crtc)
+{
+ if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568) {
+ vop2_lock(vop2);
+
+ vop2_crtc_write_gamma_lut(vop2, crtc);
+ vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id);
+
+ vop2_unlock(vop2);
+ }
}
static void vop2_crtc_atomic_flush(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
+ struct drm_crtc_state *crtc_state =
+ drm_atomic_get_new_crtc_state(state, crtc);
struct vop2_video_port *vp = to_vop2_video_port(crtc);
+ /**
+ * Write gamma LUT in atomic flush
+ */
+ if (crtc_state->mode_changed && crtc_state->gamma_lut)
+ vop2_crtc_atomic_flush_gamma(vp->vop2, vp, crtc);
+
vop2_post_config(crtc);
vop2_cfg_done(vp);
