On 2023-10-30 09:29, Pekka Paalanen wrote: > On Thu, 19 Oct 2023 17:21:21 -0400 > Harry Wentland <harry.wentland@xxxxxxx> 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 floot LUT index. >> >> Blurb about LUT creation and how first element should be 0x0 and >> last one 0xffff. >> >> Hold on, is that even correct? What should the ends of a LUT be? >> How does UNORM work and how does it apply to LUTs? > > Do you mean how should UNORM input value map to LUT entries for LUT > indexing? > > I suppose UNORM 16-bit converts to nominal real values as: > - 0x0: 0.0 > - 0xffff: 1.0 > > And in LUT, you want 0.0 to map to the first LUT element exactly, and > 1.0 to map to the last LUT element exactly, even if whatever > interpolation may be in use, right? > > If so, it is important to make sure that, assuming linear interpolation > for instance, there is no "dead zone" at either end. Given high > interpolation precision, any step away from 0.0 or 1.0 needs to imply a > change in the real-valued output, assuming e.g. identity LUT. > > If LUT has N elements, and 16-bit UNORM input value is I, then (in > naive real-valued math, so no implicit truncation between operations) > > x = I / 0xffff * (N - 1) > ia = floor(x) > ib = min(ia + 1, N - 1) > > f = x - floor(x) > y = (1 - f) * LUT[ia] + f * LUT[ib] > > > Does that help? > Thanks. Yes, this is what the code is doing (with this commit). The commit description was an oversight and only reflect my initial thoughts when coding it, before I made sure this is the right way to go about it. I'll update it. Harry > In my mind, I'm thinking of a uniformly distributed LUT as a 1-D > texture, because that's how I have implemented them in GL. There you > have to be careful so that input values 0.0 and 1.0 map to the *center* > of the first and last texel, and not to the edges of the texture like > texture coordinates do. Then you can use the GL linear texture > interpolation as-is. > > > Thanks, > pq > > >> Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx> >> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> >> Cc: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx> >> Cc: Simon Ser <contact@xxxxxxxxxxx> >> Cc: Harry Wentland <harry.wentland@xxxxxxx> >> Cc: Melissa Wen <mwen@xxxxxxxxxx> >> Cc: Jonas Ådahl <jadahl@xxxxxxxxxx> >> Cc: Sebastian Wick <sebastian.wick@xxxxxxxxxx> >> Cc: Shashank Sharma <shashank.sharma@xxxxxxx> >> Cc: Alexander Goins <agoins@xxxxxxxxxx> >> Cc: Joshua Ashton <joshua@xxxxxxxxx> >> Cc: Michel Dänzer <mdaenzer@xxxxxxxxxx> >> Cc: Aleix Pol <aleixpol@xxxxxxx> >> Cc: Xaver Hugl <xaver.hugl@xxxxxxxxx> >> Cc: Victoria Brekenfeld <victoria@xxxxxxxxxxxx> >> Cc: Sima <daniel@xxxxxxxx> >> Cc: Uma Shankar <uma.shankar@xxxxxxxxx> >> Cc: Naseer Ahmed <quic_naseer@xxxxxxxxxxx> >> Cc: Christopher Braga <quic_cbraga@xxxxxxxxxxx> >> Cc: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> >> Cc: Arthur Grillo <arthurgrillo@xxxxxxxxxx> >> Cc: Hector Martin <marcan@xxxxxxxxx> >> Cc: Liviu Dudau <Liviu.Dudau@xxxxxxx> >> Cc: Sasha McIntosh <sashamcintosh@xxxxxxxxxx> >> --- >> 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 a0a3a6fd2926..cf1dff162920 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); >