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(). 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<[email protected]>
Cc: Rodrigo Siqueira <[email protected]>
Cc: Haneen Mohammed <[email protected]>
Signed-off-by: Sidong Yang <[email protected]>
---
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,
};
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
On Saturday, August 29, 2020 4:06 PM, Sidong Yang <[email protected]> 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().
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.
> 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<[email protected]>
> Cc: Rodrigo Siqueira <[email protected]>
> Cc: Haneen Mohammed <[email protected]>
>
> Signed-off-by: Sidong Yang <[email protected]>
> ---
> 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?
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.
> };
>
> 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
On Mon, Aug 31, 2020 at 11:39:10AM +0000, Simon Ser wrote:
> On Saturday, August 29, 2020 4:06 PM, Sidong Yang <[email protected]> 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<[email protected]>
> > Cc: Rodrigo Siqueira <[email protected]>
> > Cc: Haneen Mohammed <[email protected]>
> >
> > Signed-off-by: Sidong Yang <[email protected]>
> > ---
> > 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
>
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 <[email protected]> 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<[email protected]>
> > > Cc: Rodrigo Siqueira <[email protected]>
> > > Cc: Haneen Mohammed <[email protected]>
> > >
> > > Signed-off-by: Sidong Yang <[email protected]>
> > > ---
> > > 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel
On Monday, August 31, 2020 3:48 PM, Ville Syrjälä <[email protected]> wrote:
> > > 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.
OK, that makes sense. Would it be sensible to:
- Don't set gamma if the pixel format being tested is 8bpc
- Make the test skip if the pixel format is >8bpc and gamma isn't
supported
On Tue, Sep 01, 2020 at 08:57:37AM +0000, Simon Ser wrote:
> On Monday, August 31, 2020 3:48 PM, Ville Syrj?l? <[email protected]> wrote:
>
> > > > 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.
>
> OK, that makes sense. Would it be sensible to:
>
> - Don't set gamma if the pixel format being tested is 8bpc
Hm not sure what 8bpc format you mean here, because we have C8 (needs
gamma table or doesn't work) and the 8b greyscale one with the R8 one. If
you ask for legacy 8bpc you get C8.
> - Make the test skip if the pixel format is >8bpc and gamma isn't
> supported
Yeah the test should skip if gamma isn't there.
-Daniel
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tuesday, September 1, 2020 3:26 PM, Daniel Vetter <[email protected]> wrote:
> On Tue, Sep 01, 2020 at 08:57:37AM +0000, Simon Ser wrote:
>
> > On Monday, August 31, 2020 3:48 PM, Ville Syrjälä [email protected] wrote:
> >
> > > > > 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.
> >
> > OK, that makes sense. Would it be sensible to:
> >
> > - Don't set gamma if the pixel format being tested is 8bpc
>
> Hm not sure what 8bpc format you mean here, because we have C8 (needs
> gamma table or doesn't work) and the 8b greyscale one with the R8 one. If
> you ask for legacy 8bpc you get C8.
Why do we need a gamma LUT for C8 and R8? There shouldn't be any
precision loss, right?
> > - Make the test skip if the pixel format is >8bpc and gamma isn't
> > supported
> >
>
> Yeah the test should skip if gamma isn't there.
> -Daniel
>
> > dri-devel mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
>
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
On Wed, Sep 02, 2020 at 09:09:11AM +0000, Simon Ser wrote:
> On Tuesday, September 1, 2020 3:26 PM, Daniel Vetter <[email protected]> wrote:
>
> > On Tue, Sep 01, 2020 at 08:57:37AM +0000, Simon Ser wrote:
> >
> > > On Monday, August 31, 2020 3:48 PM, Ville Syrj?l? [email protected] wrote:
> > >
> > > > > > 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.
> > >
> > > OK, that makes sense. Would it be sensible to:
> > >
> > > - Don't set gamma if the pixel format being tested is 8bpc
> >
> > Hm not sure what 8bpc format you mean here, because we have C8 (needs
> > gamma table or doesn't work) and the 8b greyscale one with the R8 one. If
> > you ask for legacy 8bpc you get C8.
>
> Why do we need a gamma LUT for C8 and R8? There shouldn't be any
> precision loss, right?
C8 always needs a LUT. Somewhat annoying legacy uapi thing that the
crtc's gamma LUT is also the LUT for C8 scanout, but at least it does
match how eg. Intel hw works. I think ideally there should a separate
per-plane LUT for this (with i915 then having to check that the same
LUT is used for all C8 planes on the crtc).
As for why we might need the LUT even for 8bpc formats.
The test does the following:
capture a reference CRC using XRGB8888
for_each_format
capture CRC using the format
compare CRC to the reference CRC
So all formats need to use the LUT to preserve the same number
of msbs and throw away the unwanted lsbs.
I guess we could add a special case for the "plane only
supports 8bpc formats" situation and omit the LUT in that case...
>
> > > - Make the test skip if the pixel format is >8bpc and gamma isn't
> > > supported
> > >
> >
> > Yeah the test should skip if gamma isn't there.
> > -Daniel
> >
> > > dri-devel mailing list
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> >
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
--
Ville Syrj?l?
Intel