From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.manjaro.org (mail.manjaro.org [116.203.91.91]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3AD2F17BCD for ; Mon, 22 Jul 2024 06:37:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=116.203.91.91 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721630266; cv=none; b=HL3pfJPKI5GPStrCi5G6WGO6L4OM7SB4bbJDNZQf6MJ5kULgtLXz3HmwGqUiu70pyKARPGNsad2LqAqsZ/EY1QCQ56zKAmSXobTUumS2DrYiT3YlH7arR3uoGMoxIdZ7slAPbdQy/eLaad19HjTOpuMYmjrE7NVGKRDmyDesPKY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721630266; c=relaxed/simple; bh=hC1fIoL/k2MFkLE3UC5SpYr4SefsnDgH9S4tOjzD69Q=; h=MIME-Version:Date:From:To:Cc:Subject:In-Reply-To:References: Message-ID:Content-Type; b=mkXWwro+l7DJDrFx7UuIqssd0paDK6TC1sFlLRgnGqblXNV7W12/rpMcl2TpRuFkYbMbznZBpwnht/NzOC5EMO2AfqzUO4BIMNUrWvVbpwaWpxmG4lCAt1MX2uzk3jEhoHA8rprZnlOPLeWxaik0CS8qKE8Al+Utg47bM0xouN4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=manjaro.org; spf=pass smtp.mailfrom=manjaro.org; dkim=pass (2048-bit key) header.d=manjaro.org header.i=@manjaro.org header.b=JaTA7zee; arc=none smtp.client-ip=116.203.91.91 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=manjaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=manjaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=manjaro.org header.i=@manjaro.org header.b="JaTA7zee" Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=manjaro.org; s=2021; t=1721630259; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2X4G9phBgH8Lk9uVjmD3QFcV2wSgIIyVsCvsrmO1ghw=; b=JaTA7zeeVx9970mf1M4mWXUmDuKsVkDbenJ3dQ47aJdGLMe6Xf7Ek6TyVk5yHKdcDe99kq ogyCxxM8uFFjKf6318Mj1RtDTm5vdXpzOatgtmHbT9gBDlRBdWSwrYI3f6YU+8NHapxpvd uPZj9e5bWQTbyfWzOxO3u9QtSsrd1YXLddceWwkk3IifxEGqRtHmrmv545cO2OniV9G5uw OqeZdpOXk4Ho5nW8BoaGmgJwRPCEzOnr8EGs7KxJFttAtrsDjflS2N/h7/qcdCDuITpJI/ ELj+T/tuCXyvU58UMSXhnPZff8WOAqXhuhgG97g0Xjh14LVUiknFYw8v/ru7Wg== Date: Mon, 22 Jul 2024 08:37:37 +0200 From: Dragan Simic To: Piotr Zalewski Cc: linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, daniel@ffwll.ch, airlied@gmail.com, tzimmermann@suse.de, mripard@kernel.org, maarten.lankhorst@linux.intel.com, andy.yan@rock-chips.com, heiko@sntech.de, hjc@rock-chips.com Subject: Re: [PATCH RESEND] rockchip/drm: vop2: add support for gamma LUT In-Reply-To: References: Message-ID: X-Sender: dsimic@manjaro.org Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Authentication-Results: ORIGINATING; auth=pass smtp.auth=dsimic@manjaro.org smtp.mailfrom=dsimic@manjaro.org 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 > --- > > 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); > + > + 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)