| Message ID | 20240223-yuv-v2-4-aa6be2827bb7@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Reimplement line-per-line pixel conversion for plane reading | expand |
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 would also expect the source to be always const, but that's a whole another patch to change. > + > struct vkms_writeback_job { > struct iosys_map data[DRM_FORMAT_MAX_PLANES]; > struct vkms_frame_info wb_frame_info; > - void (*pixel_write)(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel); > + pixel_write_t pixel_write; > }; > > +/** > + * typedef pixel_read_t - These functions are used to read a pixel in the source frame, > + * convert it to `struct pixel_argb_u16` and write it to @out_pixel. > + * > + * @src_pixels: Pointer to the pixel to read > + * @out_pixel: Pointer to write the converted pixel > + */ > +typedef void (*pixel_read_t)(u8 *src_pixels, struct pixel_argb_u16 *out_pixel); > + > /** > * vkms_plane_state - Driver specific plane state > * @base: base plane state > @@ -69,7 +88,7 @@ struct vkms_writeback_job { > struct vkms_plane_state { > struct drm_shadow_plane_state base; > struct vkms_frame_info *frame_info; > - void (*pixel_read)(u8 *src_buffer, struct pixel_argb_u16 *out_pixel); > + pixel_read_t pixel_read; > }; > > struct vkms_plane { > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c > index cb7a49b7c8e7..1f5aeba57ad6 100644 > --- a/drivers/gpu/drm/vkms/vkms_formats.c > +++ b/drivers/gpu/drm/vkms/vkms_formats.c > @@ -262,7 +262,7 @@ void vkms_writeback_row(struct vkms_writeback_job *wb, > * > * @format: 4cc of the format > */ > -void *get_pixel_conversion_function(u32 format) > +pixel_read_t get_pixel_read_function(u32 format) > { > switch (format) { > case DRM_FORMAT_ARGB8888: > @@ -276,7 +276,7 @@ void *get_pixel_conversion_function(u32 format) > case DRM_FORMAT_RGB565: > return &RGB565_to_argb_u16; > default: > - return NULL; > + return (pixel_read_t)NULL; > } > } > > @@ -287,7 +287,7 @@ void *get_pixel_conversion_function(u32 format) > * > * @format: 4cc of the format > */ > -void *get_pixel_write_function(u32 format) > +pixel_write_t get_pixel_write_function(u32 format) > { > switch (format) { > case DRM_FORMAT_ARGB8888: > @@ -301,6 +301,6 @@ void *get_pixel_write_function(u32 format) > case DRM_FORMAT_RGB565: > return &argb_u16_to_RGB565; > default: > - return NULL; > + return (pixel_write_t)NULL; > } > } > diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h > index cf59c2ed8e9a..3ecea4563254 100644 > --- a/drivers/gpu/drm/vkms/vkms_formats.h > +++ b/drivers/gpu/drm/vkms/vkms_formats.h > @@ -5,8 +5,8 @@ > > #include "vkms_drv.h" > > -void *get_pixel_conversion_function(u32 format); > +pixel_read_t get_pixel_read_function(u32 format); > > -void *get_pixel_write_function(u32 format); > +pixel_write_t get_pixel_write_function(u32 format); > > #endif /* _VKMS_FORMATS_H_ */ > 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? 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. Thanks, pq > + return; > + } > + > vkms_plane_state = to_vkms_plane_state(new_state); > shadow_plane_state = &vkms_plane_state->base; > > @@ -124,7 +131,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane, > drm_rect_rotate(&frame_info->rotated, drm_rect_width(&frame_info->rotated), > drm_rect_height(&frame_info->rotated), frame_info->rotation); > > - vkms_plane_state->pixel_read = get_pixel_conversion_function(fmt); > + vkms_plane_state->pixel_read = pixel_read; > } > > static int vkms_plane_atomic_check(struct drm_plane *plane, > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c > index c8582df1f739..c92b9f06c4a4 100644 > --- a/drivers/gpu/drm/vkms/vkms_writeback.c > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c > @@ -140,6 +140,13 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, > if (!conn_state) > return; > > + pixel_write_t pixel_write = get_pixel_write_function(wb_format); > + > + if (!pixel_write) { > + DRM_WARN("Pixel format is not supported by VKMS writeback. State is inchanged\n"); > + return; > + } > + > vkms_set_composer(&vkmsdev->output, true); > > active_wb = conn_state->writeback_job->priv; > @@ -150,7 +157,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, > crtc_state->wb_pending = true; > spin_unlock_irq(&output->composer_lock); > drm_writeback_queue_job(wb_conn, connector_state); > - active_wb->pixel_write = get_pixel_write_function(wb_format); > + active_wb->pixel_write = pixel_write; > drm_rect_init(&wb_frame_info->src, 0, 0, crtc_width, crtc_height); > drm_rect_init(&wb_frame_info->dst, 0, 0, crtc_width, crtc_height); > } >
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" [...] > > 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). > 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. 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. [...] Kind regards, Louis Chauvet
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
Le 29/02/24 - 11:07, Pekka Paalanen a écrit : > 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. You are right. In this case I maybe can just create a dummy "pixel_read" which always return black? (The v4 will have it so you can see how it look) This way, I can: - keep the check and avoid null function pointers (and OOPS); - log (WARN, DRM_WARN, DRM_ERROR or whatever suggested by DRM maintainers to warn very loudly); - Apply the requested change from userspace (and don't break the UAPI contract). The resulting behavior will be "the virtual plane is black", which is, I think, not very important. As the primary usage of VKMS is testing, it will just broke all the tests, and a quick look at the kernel logs will show this bug. > > > 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. That my understanding of the UAPI/DRM internals too, is my suggestion above sufficient? It will always succeed, no kernel OOPS. > > 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. > Is something like this better? /* * This is a bug in vkms_plane_atomic_check. All the supported * format must: * - Be listed in vkms_formats * - Have a pixel_read_line callback */ WARN(true, "Pixel format %4cc is not supported by VKMS planes. This is a kernel bug. Atomic check must forbid this configuration.\n", fmt) > Thanks, > pq [...] Kind regards, Louis Chauvet
On Mon, 4 Mar 2024 16:28:32 +0100 Louis Chauvet <louis.chauvet@bootlin.com> wrote: > Le 29/02/24 - 11:07, Pekka Paalanen a écrit : > > 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. > > You are right. In this case I maybe can just create a dummy > "pixel_read" which always return black? (The v4 will have it so you can > see how it look) > > This way, I can: > - keep the check and avoid null function pointers (and OOPS); > - log (WARN, DRM_WARN, DRM_ERROR or whatever suggested by DRM maintainers > to warn very loudly); > - Apply the requested change from userspace (and don't break the UAPI > contract). > > The resulting behavior will be "the virtual plane is black", which is, I > think, not very important. As the primary usage of VKMS is testing, it > will just broke all the tests, and a quick look at the kernel logs will > show this bug. That's fine by me. After all, atomic check should have already prevented this, and this can only happen due to a kernel bug AFAIU. > > > > 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. > > That my understanding of the UAPI/DRM internals too, is my suggestion > above sufficient? It will always succeed, no kernel OOPS. > > > > 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. > > > > Is something like this better? > > /* > * This is a bug in vkms_plane_atomic_check. All the supported > * format must: > * - Be listed in vkms_formats > * - Have a pixel_read_line callback > */ > WARN(true, "Pixel format %4cc is not supported by VKMS planes. This is a kernel bug. Atomic check must forbid this configuration.\n", fmt) > Sure. Thanks, pq
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); + struct vkms_writeback_job { struct iosys_map data[DRM_FORMAT_MAX_PLANES]; struct vkms_frame_info wb_frame_info; - void (*pixel_write)(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel); + pixel_write_t pixel_write; }; +/** + * typedef pixel_read_t - These functions are used to read a pixel in the source frame, + * convert it to `struct pixel_argb_u16` and write it to @out_pixel. + * + * @src_pixels: Pointer to the pixel to read + * @out_pixel: Pointer to write the converted pixel + */ +typedef void (*pixel_read_t)(u8 *src_pixels, struct pixel_argb_u16 *out_pixel); + /** * vkms_plane_state - Driver specific plane state * @base: base plane state @@ -69,7 +88,7 @@ struct vkms_writeback_job { struct vkms_plane_state { struct drm_shadow_plane_state base; struct vkms_frame_info *frame_info; - void (*pixel_read)(u8 *src_buffer, struct pixel_argb_u16 *out_pixel); + pixel_read_t pixel_read; }; struct vkms_plane { diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c index cb7a49b7c8e7..1f5aeba57ad6 100644 --- a/drivers/gpu/drm/vkms/vkms_formats.c +++ b/drivers/gpu/drm/vkms/vkms_formats.c @@ -262,7 +262,7 @@ void vkms_writeback_row(struct vkms_writeback_job *wb, * * @format: 4cc of the format */ -void *get_pixel_conversion_function(u32 format) +pixel_read_t get_pixel_read_function(u32 format) { switch (format) { case DRM_FORMAT_ARGB8888: @@ -276,7 +276,7 @@ void *get_pixel_conversion_function(u32 format) case DRM_FORMAT_RGB565: return &RGB565_to_argb_u16; default: - return NULL; + return (pixel_read_t)NULL; } } @@ -287,7 +287,7 @@ void *get_pixel_conversion_function(u32 format) * * @format: 4cc of the format */ -void *get_pixel_write_function(u32 format) +pixel_write_t get_pixel_write_function(u32 format) { switch (format) { case DRM_FORMAT_ARGB8888: @@ -301,6 +301,6 @@ void *get_pixel_write_function(u32 format) case DRM_FORMAT_RGB565: return &argb_u16_to_RGB565; default: - return NULL; + return (pixel_write_t)NULL; } } diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h index cf59c2ed8e9a..3ecea4563254 100644 --- a/drivers/gpu/drm/vkms/vkms_formats.h +++ b/drivers/gpu/drm/vkms/vkms_formats.h @@ -5,8 +5,8 @@ #include "vkms_drv.h" -void *get_pixel_conversion_function(u32 format); +pixel_read_t get_pixel_read_function(u32 format); -void *get_pixel_write_function(u32 format); +pixel_write_t get_pixel_write_function(u32 format); #endif /* _VKMS_FORMATS_H_ */ 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"); + return; + } + vkms_plane_state = to_vkms_plane_state(new_state); shadow_plane_state = &vkms_plane_state->base; @@ -124,7 +131,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane, drm_rect_rotate(&frame_info->rotated, drm_rect_width(&frame_info->rotated), drm_rect_height(&frame_info->rotated), frame_info->rotation); - vkms_plane_state->pixel_read = get_pixel_conversion_function(fmt); + vkms_plane_state->pixel_read = pixel_read; } static int vkms_plane_atomic_check(struct drm_plane *plane, diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index c8582df1f739..c92b9f06c4a4 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -140,6 +140,13 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, if (!conn_state) return; + pixel_write_t pixel_write = get_pixel_write_function(wb_format); + + if (!pixel_write) { + DRM_WARN("Pixel format is not supported by VKMS writeback. State is inchanged\n"); + return; + } + vkms_set_composer(&vkmsdev->output, true); active_wb = conn_state->writeback_job->priv; @@ -150,7 +157,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, crtc_state->wb_pending = true; spin_unlock_irq(&output->composer_lock); drm_writeback_queue_job(wb_conn, connector_state); - active_wb->pixel_write = get_pixel_write_function(wb_format); + active_wb->pixel_write = pixel_write; drm_rect_init(&wb_frame_info->src, 0, 0, crtc_width, crtc_height); drm_rect_init(&wb_frame_info->dst, 0, 0, crtc_width, crtc_height); }
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(-)