Re: [PATCH v2 4/9] drm/vkms: Add typedef and documentation for pixel_read and pixel_write functions

[Pekka Paalanen <pekka.paalanen@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 5605 bytes --]

On Tue, 27 Feb 2024 16:02:13 +0100
Louis Chauvet <louis.chauvet@bootlin.com> wrote:

> Le 26/02/24 - 13:36, Pekka Paalanen a écrit :
> > On Fri, 23 Feb 2024 12:37:24 +0100
> > Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> >   
> > > Introduce two typedefs: pixel_read_t and pixel_write_t. It allows the
> > > compiler to check if the passed functions take the correct arguments.
> > > Such typedefs will help ensuring consistency across the code base in
> > > case of update of these prototypes.
> > > 
> > > Introduce a check around the get_pixel_*_functions to avoid using a
> > > nullptr as a function.
> > > 
> > > Document for those typedefs.
> > > 
> > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_drv.h       | 23 +++++++++++++++++++++--
> > >  drivers/gpu/drm/vkms/vkms_formats.c   |  8 ++++----
> > >  drivers/gpu/drm/vkms/vkms_formats.h   |  4 ++--
> > >  drivers/gpu/drm/vkms/vkms_plane.c     |  9 ++++++++-
> > >  drivers/gpu/drm/vkms/vkms_writeback.c |  9 ++++++++-
> > >  5 files changed, 43 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 18086423a3a7..886c885c8cf5 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -53,12 +53,31 @@ struct line_buffer {
> > >  	struct pixel_argb_u16 *pixels;
> > >  };
> > >  
> > > +/**
> > > + * typedef pixel_write_t - These functions are used to read a pixel from a
> > > + * `struct pixel_argb_u16*`, convert it in a specific format and write it in the @dst_pixels
> > > + * buffer.
> > > + *
> > > + * @dst_pixel: destination address to write the pixel
> > > + * @in_pixel: pixel to write
> > > + */
> > > +typedef void (*pixel_write_t)(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel);  
> > 
> > There are some inconsistencies in pixel_write_t and pixel_read_t. At
> > this point of the series they still operate on a single pixel, but you
> > use dst_pixels and src_pixels, plural. Yet the documentation correctly
> > talks about processing a single pixel.  
> 
> I will fix this for v4.
>  
> > I would also expect the source to be always const, but that's a whole
> > another patch to change.  
> 
> The v4 will contains a new patch "drm/vkms: Use const pointer for 
> pixel_read and pixel_write functions"

Sounds good!

> 
> [...]
> 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > index d5203f531d96..f68b1b03d632 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > @@ -106,6 +106,13 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> > >  		return;
> > >  
> > >  	fmt = fb->format->format;
> > > +	pixel_read_t pixel_read = get_pixel_read_function(fmt);
> > > +
> > > +	if (!pixel_read) {
> > > +		DRM_WARN("Pixel format is not supported by VKMS planes. State is inchanged\n");  
> > 
> > DRM_WARN() is the kernel equivalent to userspace assert(), right?  
> 
> For the DRM_WARN it is just a standard prinkt(KERN_WARN, ...) (hidden 
> behind drm internal macros).

My concern here is that does hitting this cause additional breakage of
the UAPI contract? For example, the UAPI contract expects that the old
FB is unreffed and the new FB is reffed by the plane in question. If
this early return causes that FB swap to be skipped, it could cause
secondary unexpected failures or misbehaviour for userspace later. That
could mislead debugging to think that there is a userspace bug.

Even if you cannot actually read FB due to an internal bug, it would be
good to still apply all the state changes that the UAPI contract
mandates.

Unless, this is a kernel bug kind of thing which explodes very
verbosely, but DRM_WARN is not that.

> > In that failing the check means an internal invariant was violated,
> > which means a code bug in kernel?
> >
> > Maybe this could be more specific about what invariant was violated?
> > E.g. atomic check should have rejected this attempt already.  
> 
> I'm not an expert (yet) in DRM, so please correct me:
> When atomic_update is called, the new state is always validated by 
> atomic_check before? There is no way to pass something not validated by 
> atomic_check to atomic_update? If this is the case, yes, it should be an 
> ERROR and not a WARN as an invalid format passed the atomic_check 
> verification.

I only know about the UAPI, I'm not familiar with kernel internals.

We see that atomic_update returns void, so I think it simply cannot
return errors. To my understanding, atomic_check needs to ensure that
atomic_update cannot fail. There is even UAPI to exercise atomic_check
alone: the atomic commit TEST_ONLY flag. Userspace trusts that flag, and
will not expect an identical atomic commit to fail without TEST_ONLY
when it succeeded with TEST_ONLY.

> If so, is this better?
> 
> 	if (!pixel_read) {
> 		/*
> 		 * This is a bug as the vkms_plane_atomic_check must forbid all unsupported formats.
> 		 */
> 		DRM_ERROR("Pixel format %4cc is not supported by VKMS planes.\n", fmt);
> 		return;
> 	}
> 
> I will put the same code in vkms_writeback.c.

Maybe maintainers can comment whether even DRM_ERROR is strong enough.

As for the message, what you wrote in the comment is the most important
part that I'd put in the log. It explains what's going on, while that
"format not supported" is a detail without context.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]