lkml.org 
[lkml]   [2024]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 05/16] drm/vkms: Add dummy pixel_read/pixel_write callbacks to avoid NULL pointers
    Le 25/03/24 - 14:05, Pekka Paalanen a écrit :
    > On Wed, 13 Mar 2024 18:44:59 +0100
    > Louis Chauvet <louis.chauvet@bootlin.com> wrote:
    >
    > > Introduce two callbacks which does nothing. They are used in replacement
    > > of NULL and it avoid kernel OOPS if this NULL is called.
    > >
    > > If those callback are used, it means that there is a mismatch between
    > > what formats are announced by atomic_check and what is realy supported by
    > > atomic_update.
    > >
    > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
    > > ---
    > > drivers/gpu/drm/vkms/vkms_formats.c | 43 +++++++++++++++++++++++++++++++------
    > > 1 file changed, 37 insertions(+), 6 deletions(-)
    > >
    > > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
    > > index 55a4365d21a4..b57d85b8b935 100644
    > > --- a/drivers/gpu/drm/vkms/vkms_formats.c
    > > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
    > > @@ -136,6 +136,21 @@ static void RGB565_to_argb_u16(u8 *in_pixel, struct pixel_argb_u16 *out_pixel)
    > > out_pixel->b = drm_fixp2int_round(drm_fixp_mul(fp_b, fp_rb_ratio));
    > > }
    > >
    > > +/**
    > > + * black_to_argb_u16() - pixel_read callback which always read black
    > > + *
    > > + * This callback is used when an invalid format is requested for plane reading.
    > > + * It is used to avoid null pointer to be used as a function. In theory, this function should
    > > + * never be called, except if you found a bug in the driver/DRM core.
    > > + */
    > > +static void black_to_argb_u16(u8 *in_pixel, struct pixel_argb_u16 *out_pixel)
    > > +{
    > > + out_pixel->a = (u16)0xFFFF;
    > > + out_pixel->r = 0;
    > > + out_pixel->g = 0;
    > > + out_pixel->b = 0;
    > > +}
    > > +
    > > /**
    > > * vkms_compose_row - compose a single row of a plane
    > > * @stage_buffer: output line with the composed pixels
    > > @@ -238,6 +253,16 @@ static void argb_u16_to_RGB565(u8 *out_pixel, struct pixel_argb_u16 *in_pixel)
    > > *pixel = cpu_to_le16(r << 11 | g << 5 | b);
    > > }
    > >
    > > +/**
    > > + * argb_u16_to_nothing() - pixel_write callback with no effect
    > > + *
    > > + * This callback is used when an invalid format is requested for writeback.
    > > + * It is used to avoid null pointer to be used as a function. In theory, this should never
    > > + * happen, except if there is a bug in the driver
    > > + */
    > > +static void argb_u16_to_nothing(u8 *out_pixel, struct pixel_argb_u16 *in_pixel)
    > > +{}
    > > +
    > > /**
    > > * Generic loop for all supported writeback format. It is executed just after the blending to
    > > * write a line in the writeback buffer.
    > > @@ -261,8 +286,8 @@ void vkms_writeback_row(struct vkms_writeback_job *wb,
    > >
    > > /**
    > > * Retrieve the correct read_pixel function for a specific format.
    > > - * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
    > > - * pointer is valid before using it in a vkms_plane_state.
    > > + * If the format is not supported by VKMS a warn is emitted and a dummy "always read black"
    > > + * function is returned.
    > > *
    > > * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
    > > */
    > > @@ -285,18 +310,21 @@ pixel_read_t get_pixel_read_function(u32 format)
    > > * format must:
    > > * - Be listed in vkms_formats in vkms_plane.c
    > > * - Have a pixel_read callback defined here
    > > + *
    > > + * To avoid kernel crash, a dummy "always read black" function is used. It means
    > > + * that during the composition, this plane will always be black.
    > > */
    > > WARN(true,
    > > "Pixel format %p4cc is not supported by VKMS planes. This is a kernel bug, atomic check must forbid this configuration.\n",
    > > &format);
    > > - return (pixel_read_t)NULL;
    > > + return &black_to_argb_u16;
    >
    > Hi Louis,
    >
    > I'm perhaps a bit paranoid in these things, but I'd make this not
    > black. Maybe something more "screaming" like magenta. There is a slight
    > chance that black might sometimes be expected, or not affect the
    > result. After all, blending something into black with pre-multiplied
    > alpha is equivalent to no-blending (a copy). The kernel warning is
    > good, the magenta is more like an assurance.

    Changed to 0xFF/0x00/0XFF in the V6.

    Thanks,
    Louis Chauvet

    > Anyway,
    >
    > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
    >
    >
    > Thanks,
    > pq
    >
    >
    > > }
    > > }
    > >
    > > /**
    > > * Retrieve the correct write_pixel function for a specific format.
    > > - * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
    > > - * pointer is valid before using it in a vkms_writeback_job.
    > > + * If the format is not supported by VKMS a warn is emitted and a dummy "don't do anything"
    > > + * function is returned.
    > > *
    > > * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
    > > */
    > > @@ -319,10 +347,13 @@ pixel_write_t get_pixel_write_function(u32 format)
    > > * format must:
    > > * - Be listed in vkms_wb_formats in vkms_writeback.c
    > > * - Have a pixel_write callback defined here
    > > + *
    > > + * To avoid kernel crash, a dummy "don't do anything" function is used. It means
    > > + * that the resulting writeback buffer is not composed and can contains any values.
    > > */
    > > WARN(true,
    > > "Pixel format %p4cc is not supported by VKMS writeback. This is a kernel bug, atomic check must forbid this configuration.\n",
    > > &format);
    > > - return (pixel_write_t)NULL;
    > > + return &argb_u16_to_nothing;
    > > }
    > > }
    > >
    >



    --
    Louis Chauvet, Bootlin
    Embedded Linux and Kernel engineering
    https://bootlin.com

    \
     
     \ /
      Last update: 2024-05-27 16:09    [W:4.561 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site