Re: [RFC PATCH v3 01/23] drm: Don't treat 0 as -1 in drm_fixp2int_ceil
On 11/08, Harry Wentland wrote:
> Unit testing this in VKMS shows that passing 0 into
> this function returns -1, which is highly counter-
> intuitive. Fix it by checking whether the input is
> >= 0 instead of > 0.
>
Nice finding. Thanks!
Could you add the fixes tag? AFAIU, this one:
Fixes: 64566b5e767f9 ("drm: Add drm_fixp_from_fraction and drm_fixp2int_ceil")
Reviewed-by: Melissa Wen
> Signed-off-by: Harry Wentland
> Reviewed-by: Simon Ser
> ---
> include/drm/drm_fixed.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index 6ea339d5de08..0c9f917a4d4b 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -95,7 +95,7 @@ static inline int drm_fixp2int_round(s64 a)
>
> static inline int drm_fixp2int_ceil(s64 a)
> {
> - if (a > 0)
> + if (a >= 0)
> return drm_fixp2int(a + DRM_FIXED_ALMOST_ONE);
> else
> return drm_fixp2int(a - DRM_FIXED_ALMOST_ONE);
> --
> 2.42.1
>
Re: [RFC PATCH v3 03/23] drm/vkms: Create separate Kconfig file for VKMS
On 11/08, Harry Wentland wrote: > This aligns with most other DRM drivers and will allow > us to add new VKMS config options without polluting > the DRM Kconfig. > > v3: > - Change SPDX to GPL-2.0-only to match DRM KConfig >SPDX (Simon) LGTM. I plan to apply this one to drm-misc-next. Cc'ing the other VKMS maintainers/reviewers too. > > Signed-off-by: Harry Wentland > Reviewed-by: Simon Ser > --- > drivers/gpu/drm/Kconfig | 14 +- > drivers/gpu/drm/vkms/Kconfig | 15 +++ > 2 files changed, 16 insertions(+), 13 deletions(-) > create mode 100644 drivers/gpu/drm/vkms/Kconfig > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 48ca28a2e4ff..61ebd682c9b0 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -286,19 +286,7 @@ config DRM_VGEM > as used by Mesa's software renderer for enhanced performance. > If M is selected the module will be called vgem. > > -config DRM_VKMS > - tristate "Virtual KMS (EXPERIMENTAL)" > - depends on DRM && MMU > - select DRM_KMS_HELPER > - select DRM_GEM_SHMEM_HELPER > - select CRC32 > - default n > - help > - Virtual Kernel Mode-Setting (VKMS) is used for testing or for > - running GPU in a headless machines. Choose this option to get > - a VKMS. > - > - If M is selected the module will be called vkms. > +source "drivers/gpu/drm/vkms/Kconfig" > > source "drivers/gpu/drm/exynos/Kconfig" > > diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig > new file mode 100644 > index ..b9ecdebecb0b > --- /dev/null > +++ b/drivers/gpu/drm/vkms/Kconfig > @@ -0,0 +1,15 @@ > +# SPDX-License-Identifier: GPL-2.0-only > + > +config DRM_VKMS > + tristate "Virtual KMS (EXPERIMENTAL)" > + depends on DRM && MMU > + select DRM_KMS_HELPER > + select DRM_GEM_SHMEM_HELPER > + select CRC32 > + default n > + help > + Virtual Kernel Mode-Setting (VKMS) is used for testing or for > + running GPU in a headless machines. Choose this option to get > + a VKMS. > + > + If M is selected the module will be called vkms. > -- > 2.42.1 >
Re: [RFC PATCH v3 05/23] drm/vkms: Avoid reading beyond LUT array
On 11/10, Arthur Grillo wrote:
>
>
> On 08/11/23 13:36, Harry Wentland wrote:
> > When the floor LUT index (drm_fixp2int(lut_index) is the last
> > index of the array the ceil LUT index will point to an entry
> > beyond the array. Make sure we guard against it and use the
> > value of the floor LUT index.
> >
> > v3:
> > - Drop bits from commit description that didn't contribute
> >anything of value
> >
> > Signed-off-by: Harry Wentland
> > Cc: Arthur Grillo
>
> LGTM
> Reviewed-by: Arthur Grillo
Nice catch. Thanks!
I'll already apply this one to drm-misc-next too.
CC'ing other VKMS maintainers/reviewers.
Melissa
>
> Best Regards,
> ~Arthur Grillo
>
> > ---
> > drivers/gpu/drm/vkms/vkms_composer.c | 14 ++
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> > b/drivers/gpu/drm/vkms/vkms_composer.c
> > index 6f942896036e..25b6b73bece8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -123,6 +123,8 @@ static u16 apply_lut_to_channel_value(const struct
> > vkms_color_lut *lut, u16 chan
> > enum lut_channel channel)
> > {
> > s64 lut_index = get_lut_index(lut, channel_value);
> > + u16 *floor_lut_value, *ceil_lut_value;
> > + u16 floor_channel_value, ceil_channel_value;
> >
> > /*
> > * This checks if `struct drm_color_lut` has any gap added by the
> > compiler
> > @@ -130,11 +132,15 @@ static u16 apply_lut_to_channel_value(const struct
> > vkms_color_lut *lut, u16 chan
> > */
> > static_assert(sizeof(struct drm_color_lut) == sizeof(__u16) * 4);
> >
> > - u16 *floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
> > - u16 *ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
> > + floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
> > + if (drm_fixp2int(lut_index) == (lut->lut_length - 1))
> > + /* We're at the end of the LUT array, use same value for ceil
> > and floor */
> > + ceil_lut_value = floor_lut_value;
> > + else
> > + ceil_lut_value = (__u16
> > *)&lut->base[drm_fixp2int_ceil(lut_index)];
> >
> > - u16 floor_channel_value = floor_lut_value[channel];
> > - u16 ceil_channel_value = ceil_lut_value[channel];
> > + floor_channel_value = floor_lut_value[channel];
> > + ceil_channel_value = ceil_lut_value[channel];
> >
> > return lerp_u16(floor_channel_value, ceil_channel_value,
> > lut_index & DRM_FIXED_DECIMAL_MASK);
Re: [RFC PATCH v3 05/23] drm/vkms: Avoid reading beyond LUT array
On 12/06, Melissa Wen wrote:
> On 11/10, Arthur Grillo wrote:
> >
> >
> > On 08/11/23 13:36, Harry Wentland wrote:
> > > When the floor LUT index (drm_fixp2int(lut_index) is the last
> > > index of the array the ceil LUT index will point to an entry
> > > beyond the array. Make sure we guard against it and use the
> > > value of the floor LUT index.
> > >
> > > v3:
> > > - Drop bits from commit description that didn't contribute
> > >anything of value
> > >
> > > Signed-off-by: Harry Wentland
> > > Cc: Arthur Grillo
> >
> > LGTM
> > Reviewed-by: Arthur Grillo
>
> Nice catch. Thanks!
>
> I'll already apply this one to drm-misc-next too.
It's a reminder to myself to add the missing fixes tag:
Fixes: db1f254f2cfaf ("drm/vkms: Add support to 1D gamma LUT")
Reviewed-by: Melissa Wen
>
> CC'ing other VKMS maintainers/reviewers.
>
> Melissa
>
> >
> > Best Regards,
> > ~Arthur Grillo
> >
> > > ---
> > > drivers/gpu/drm/vkms/vkms_composer.c | 14 ++
> > > 1 file changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> > > b/drivers/gpu/drm/vkms/vkms_composer.c
> > > index 6f942896036e..25b6b73bece8 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > > @@ -123,6 +123,8 @@ static u16 apply_lut_to_channel_value(const struct
> > > vkms_color_lut *lut, u16 chan
> > > enum lut_channel channel)
> > > {
> > > s64 lut_index = get_lut_index(lut, channel_value);
> > > + u16 *floor_lut_value, *ceil_lut_value;
> > > + u16 floor_channel_value, ceil_channel_value;
> > >
> > > /*
> > >* This checks if `struct drm_color_lut` has any gap added by the
> > > compiler
> > > @@ -130,11 +132,15 @@ static u16 apply_lut_to_channel_value(const struct
> > > vkms_color_lut *lut, u16 chan
> > >*/
> > > static_assert(sizeof(struct drm_color_lut) == sizeof(__u16) * 4);
> > >
> > > - u16 *floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
> > > - u16 *ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
> > > + floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
> > > + if (drm_fixp2int(lut_index) == (lut->lut_length - 1))
> > > + /* We're at the end of the LUT array, use same value for ceil
> > > and floor */
> > > + ceil_lut_value = floor_lut_value;
> > > + else
> > > + ceil_lut_value = (__u16
> > > *)&lut->base[drm_fixp2int_ceil(lut_index)];
> > >
> > > - u16 floor_channel_value = floor_lut_value[channel];
> > > - u16 ceil_channel_value = ceil_lut_value[channel];
> > > + floor_channel_value = floor_lut_value[channel];
> > > + ceil_channel_value = ceil_lut_value[channel];
> > >
> > > return lerp_u16(floor_channel_value, ceil_channel_value,
> > > lut_index & DRM_FIXED_DECIMAL_MASK);
Re: Issues with cursors (cursor-shape-v1)
On Wed, Dec 6, 2023 at 10:18 AM Simon Ser wrote: > > For wlroots-based compositors this is passed down via XCURSOR_THEME and > XCURSOR_SIZE just like on X11 although env vars aren't a good fit for > configuration. Yes, I'm using these already, in practice though - even with these variables set - I can have 3 applications open and they have different cursor themes and/or sizes. (firefox, foot, blender & it's libdecor decorations for e.g.). Perhaps there needs to be more strict guidance on the correct cursor size with output scale for e.g. so applications aren't using different methods to calculate this. > There was an earlier xcursor-configuration protocol with per-seat live > XCursor config which ended up being abandoned. I wouldn't recommend this > approach, it's too complicated and roundabout. > > What exactly are the missing cursor shapes? I'm surprised it's not enough. In my case "pencil", "color-picker", "sb_up_arrow" & "sb_down_arrow". -- - Campbell
