linux-kernel - Re:Re:[PATCH v5] rockchip/drm: vop2: add support for gamma LUT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1974DYrs9gLrQrZ5VwCglFgKDDK686iyqnS_g6uPB-s9wZ_4CqfZXPjmYWihLgrkRu7ptNjpkFeqB0uTt73RFId6cL8FowQ8LFltPmaKCoI=@proton.me>
Date: Tue, 15 Oct 2024 20:13:40 +0000
From: Piotr Zalewski <pZ010001011111@...ton.me>
To: Andy Yan <andyshrk@....com>
Cc: hjc@...k-chips.com, heiko@...ech.de, andy.yan@...k-chips.com, maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch, dri-devel@...ts.freedesktop.org, linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, Daniel Stone <daniel@...ishbar.org>, Dragan Simic <dsimic@...jaro.org>, Diederik de Haas <didi.debian@...ow.org>
Subject: Re:Re:[PATCH v5] rockchip/drm: vop2: add support for gamma LUT

Hi Andy

On Tuesday, October 15th, 2024 at 2:22 PM, Andy Yan <andyshrk@....com> wrote:

> > > > + struct vop2_video_port *vp,
> > > > + struct drm_crtc *crtc,
> > > > + struct drm_crtc_state *crtc_state)
> > > > +{
> > > > +
> > > > + if (vop2->lut_regs && crtc_state->color_mgmt_changed) {
> > > > + if (!crtc_state->gamma_lut) {
> > > > + vop2_vp_dsp_lut_disable(vp);
> > > > + return;
> > > > + }
> > > > +
> > > > + if (vop2_supports_seamless_gamma_lut_update(vop2)) {
> > > 
> > > I think it's bettery to check for rk3568/rk3566 here, the newer soc will all follow
> > > rk3588 support seamless gamma lut update.
> > 
> > I will change in the next version.
> > 
> > > > + vop2_writel(vop2, RK3568_LUT_PORT_SEL, FIELD_PREP(
> > > > + RK3588_LUT_PORT_SEL__GAMMA_AHB_WRITE_SEL,
> > > > + vp->id));
> > > > + vop2_crtc_write_gamma_lut(vop2, crtc);
> > > > + vop2_vp_dsp_lut_enable(vp);
> > > > + vop2_vp_dsp_lut_update_enable(vp);
> > > > + } else {
> > > 
> > > As for rk3566/68, we should do exclusive check here, because there is only
> > > one gamma , only one VP can use it at a time. See my comments in V3:
> > 
> > What do you mean exactly by exclusive check in this case.It's true that
> > gamma LUT is shared across video ports in rk356x but, if I correctly
> > understand, this doesn't forbid to reprogram LUT port sel and allow other
> > VP to use gamma LUT.
> 
> 
> Yes, we can reprogram LUT port sel, but we need to make sure the the dsp_lut_en bit in VPx is cleared if we
> want reprogram LUT port sel form VPx to VPy.
> 

Ok I get it now. Is such rework correct? - when gamma LUT for rk356x is
being set, instead of disabling the LUT before the gamma LUT write for the
current CRTC's video port, active video port is selected. Selection is 
based on if DSP LUT EN bit is set for particular video port. eg:
```
static struct vop2_video_port *vop2_vp_dsp_lut_get_active_vp(struct vop2 *vop2)
{
	struct vop2_video_port *vp;
	int i;
	for (i = 0; i < vop2->data->nr_vps; i++) {
		vp = &vop2->vps[i];

		if (vp->crtc.dev != NULL && vop2_vp_dsp_lut_is_enabled(vp)) {
			return vp;
		}
	}
	return NULL;
}

(...)

struct vop2_video_port *active_vp = vop2_vp_dsp_lut_get_active_vp(vop2);

if (active_vp) {
	vop2_vp_dsp_lut_disable(active_vp);
	vop2_cfg_done(active_vp);
	if (!vop2_vp_dsp_lut_poll_disable(active_vp))
		return;
}

vop2_writel(vop2, RK3568_LUT_PORT_SEL, vp->id);
vop2_crtc_write_gamma_lut(vop2, crtc);
vop2_vp_dsp_lut_enable(vp);
```


> > > > 
> > > > 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);
> > > 
> > > It seems that we can keep it in one line, the default limit of linux kernel coding style is 100 characters now.
> > 
> > Thanks. I didn't know, I will amend it.
> 
> 
> See bdc48fa11e46("checkpatch/coding-style: deprecate 80-column warning")
> 

Interesting.

Best regards, Piotr Zalewski