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

[Sidong Yang]

On Mon, Aug 31, 2020 at 11:39:10AM +0000, Simon Ser wrote:
> On Saturday, August 29, 2020 4:06 PM, Sidong Yang <realwakka@xxxxxxxxx> 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.
> 
> > 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@xxxxxxxx>
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> > Cc: Haneen Mohammed <hamohammed.sa@xxxxxxxxx>
> >
> > Signed-off-by: Sidong Yang <realwakka@xxxxxxxxx>
> > ---
> >  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
>