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

[Ville Syrjälä]

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@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.

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@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
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel