| Message ID | 20231108163647.106853-6-harry.wentland@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Color Pipeline API w/ VKMS | expand |
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 <harry.wentland@amd.com> > Cc: Arthur Grillo <arthurgrillo@riseup.net> LGTM Reviewed-by: Arthur Grillo <arthurgrillo@riseup.net> 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);
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 <harry.wentland@amd.com> > > Cc: Arthur Grillo <arthurgrillo@riseup.net> > > LGTM > Reviewed-by: Arthur Grillo <arthurgrillo@riseup.net> 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);
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 <harry.wentland@amd.com> > > > Cc: Arthur Grillo <arthurgrillo@riseup.net> > > > > LGTM > > Reviewed-by: Arthur Grillo <arthurgrillo@riseup.net> > > 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 <mwen@igalia.com> > > 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);
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);
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 <harry.wentland@amd.com> Cc: Arthur Grillo <arthurgrillo@riseup.net> --- drivers/gpu/drm/vkms/vkms_composer.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)