linux-kernel - Re: [PATCH] drm/vkms: add support for gamma_set interface

Open Source and information security mailing list archives

Date:   Mon, 31 Aug 2020 16:48:52 +0300
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Sidong Yang <realwakka@...il.com>
Cc:     Simon Ser <contact@...rsion.fr>,
        Haneen Mohammed <hamohammed.sa@...il.com>,
        Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
        Emil Velikov <emil.l.velikov@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        Melissa Wen <melissa.srw@...il.com>
Subject: Re: [PATCH] drm/vkms: add support for gamma_set interface

On Mon, Aug 31, 2020 at 01:38:58PM +0000, Sidong Yang wrote:
> On Mon, Aug 31, 2020 at 11:39:10AM +0000, Simon Ser wrote:
> > On Saturday, August 29, 2020 4:06 PM, Sidong Yang <realwakka@...il.com> wrote:
> > 
> > > Currently vkms module doesn't support gamma function for userspace. so igt
> > > subtests in kms_plane(pixel-format-pipe-A-plan) failed for calling
> > > drmModeCrtcSetGamma().
> > 
> 
> Hi, Simon.
> Thanks for review.
> 
> > It doesn't seem like this IGT test's goal is to exercise support for
> > gamma LUTs. Does the test just tries to reset the gamma LUT to linear?
> > If so, I think the IGT test should be fixed to ignore "I don't support
> > gamma" errors.
> 
> It seems like that IGT test pixel-format is to make gamma lut like below.
> 
> for (i = 0; i < lut_size; i++)
> 	lut[i] = (i * 0xffff / (lut_size - 1)) & mask;
> 
> And set this table to drm driver. and test begins. It's the test about pixel 
> format. I think you're right. It's not about gamma lut.

The point of the gamma LUT stuff in the pixel format test is to throw
away a bunch of the lsbs so that the test passes when the result is
"close enough" to the 8bpc RGB reference image. Without it we would
never get a crc match when testing non-8bpc or YCbCr formats.

> > 
> > > This patch set gamma_set interface in vkms_crtc_funcs for
> > > support gamma function. With initializing crtc, added calls for setting gamma
> > > size. it pass the test after this patch.
> > >
> > > Cc: Daniel Vetter<daniel@...ll.ch>
> > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>
> > > Cc: Haneen Mohammed <hamohammed.sa@...il.com>
> > >
> > > Signed-off-by: Sidong Yang <realwakka@...il.com>
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_crtc.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > index ac85e17428f8..643435fb2ee6 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > @@ -160,6 +160,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > >  	.get_crc_sources	= vkms_get_crc_sources,
> > >  	.set_crc_source		= vkms_set_crc_source,
> > >  	.verify_crc_source	= vkms_verify_crc_source,
> > > +	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
> > 
> > Why does VKMS need to use a legacy helper?
> 
> drm_crtc_enable_color_mgmt() enables properties about gamma/degamma lut. And 
> legacy helper just saves lut data from userspace to drm property blob. It seems 
> that it's convenient way to implement .gamma_set.
> 
> > It seems like this patch just advertises support for gamma LUTs, but
> > ignores any value set by user-space. If VKMS advertises support for
> > gamma LUTs, it needs to take the LUT into account when blending planes.
> 
> Yes, This patch doesn't use gamma lut passed by user. lut should be used for 
> calculating pixel value. For vkms, Maybe lut will be used in making crc value?
> If so, I'll try to write next patch for it.
> 
> Thanks,
> -Sidong
> 
> > 
> > >  };
> > >
> > >  static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
> > > @@ -275,6 +276,13 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> > >  		return ret;
> > >  	}
> > >
> > > +	ret = drm_mode_crtc_set_gamma_size(crtc, 256);
> > > +	if (ret) {
> > > +		DRM_ERROR("Failed to set gamma size\n");
> > > +		return ret;
> > > +	}
> > > +	drm_crtc_enable_color_mgmt(crtc, 0, false, 256);
> > > +
> > >  	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
> > >
> > >  	spin_lock_init(&vkms_out->lock);
> > > --
> > > 2.17.1
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

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