On Tue, 26 Mar 2024 16:57:02 +0100 Louis Chauvet <louis.chau...@bootlin.com> wrote:
> [...]
>
> > > @@ -215,34 +188,146 @@ static void blend(struct vkms_writeback_job *wb,
> > > {
> > > struct vkms_plane_state **plane = crtc_state->active_planes;
> > > u32 n_active_planes = crtc_state->num_active_planes;
> > > - int y_pos, x_dst, x_limit;
> > >
> > > const struct pixel_argb_u16 background_color = { .a = 0xffff };
> > >
> > > - size_t crtc_y_limit = crtc_state->base.crtc->mode.vdisplay;
> > > + int crtc_y_limit = crtc_state->base.crtc->mode.vdisplay;
> > > + int crtc_x_limit = crtc_state->base.crtc->mode.hdisplay;
> > >
> > > /*
> > > * The planes are composed line-by-line to avoid heavy memory usage. It
> > > is a necessary
> > > * complexity to avoid poor blending performance.
> > > *
> > > - * The function vkms_compose_row is used to read a line,
> > > pixel-by-pixel, into the staging
> > > - * buffer.
> > > + * The function pixel_read_line callback is used to read a line, using
> > > an efficient
> > > + * algorithm for a specific format, into the staging buffer.
> > > */
> > > for (size_t y = 0; y < crtc_y_limit; y++) {
> > > fill_background(&background_color, output_buffer);
> > >
> > > /* The active planes are composed associatively in z-order. */
> > > for (size_t i = 0; i < n_active_planes; i++) {
> > > - x_dst = plane[i]->frame_info->dst.x1;
> > > - x_limit = min_t(size_t,
> > > drm_rect_width(&plane[i]->frame_info->dst),
> > > - stage_buffer->n_pixels);
> > > - y_pos = get_y_pos(plane[i]->frame_info, y);
> > > + struct vkms_plane_state *current_plane = plane[i];
> > >
> > > - if (!check_limit(plane[i]->frame_info, y_pos))
> > > + /* Avoid rendering useless lines */
> > > + if (y < current_plane->frame_info->dst.y1 ||
> > > + y >= current_plane->frame_info->dst.y2)
> > > continue;
> > >
> > > - vkms_compose_row(stage_buffer, plane[i], y_pos);
> > > - pre_mul_alpha_blend(stage_buffer, output_buffer, x_dst,
> > > x_limit);
> > > + /*
> > > + * dst_line is the line to copy. The initial
> > > coordinates are inside the
>
> [...]
>
> > > + */
> > > + pixel_count = drm_rect_width(&src_line);
> > > + if (x_start < 0) {
> > > + pixel_count += x_start;
> > > + x_start = 0;
> > > + }
> > > + if (x_start + pixel_count >
> > > current_plane->frame_info->fb->width) {
> > > + pixel_count =
> > > +
> > > (int)current_plane->frame_info->fb->width - x_start;
> > > + }
> > > + } else {
> > > + /*
> > > + * In vertical reading, the src_line height is
> > > the number of pixel
> > > + * to read
> > > + */
> > > + pixel_count = drm_rect_height(&src_line);
> > > + if (y_start < 0) {
> > > + pixel_count += y_start;
> > > + y_start = 0;
> > > + }
> > > + if (y_start + pixel_count >
> > > current_plane->frame_info->fb->height) {
> > > + pixel_count =
> > > +
> > > (int)current_plane->frame_info->fb->width - y_start;
> > > + }
> >
> > When you are clamping x_start or y_start or pixel_count to be inside
> > the source FB, should you not equally adjust the destination
> > coordinates as well?
>
> I did not think about it. Currently it is not an issue and it will not
> read or write outside a buffer because the pixel count is properly
> limited. But indeed, there is an issue here. I will fix it in the v6.
>
> > If we take a step back and look at the UAPI, I believe the answer is
> > "no", but it's in no way obvious. It results from the combination of
> > several facts:
> >
> > - UAPI checks reject any source rectangle that extends outside of the
> > source FB.
> >
> > - The source rectangle stretches to fill the destination rectangle
> > exactly.
> >
> > - VKMS does not support stretching (scaling), so its UAPI checks reject
> > any commit with source and destination rectangles of different sizes
> > after accounting for rotation. (Right?)
>
> I don't know what are exactly the UAPI contract but as the dst can be
> outside the CRTC, I assumed that the src can be outside the source plane.
> After thinking it doesn't really make sense.
The UAPI contract for source and destination rectangles is here:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-plane-properties
I assume there is some shared (helper?) code in DRM that enforces the
contract and returns error to userspace if it is violated.
> > I think this results in the clamping code being actually dead code.
> > However, I would not delete the clamping code, because it is a cheap
> > safety net in case something goes wrong.
>
> If UAPI check that the source rectangle is inside the plane, yes it is
> just a safety net. Otherwise, it is required to manage properly the
> userspace requests. In the v6, the outside of a source buffer is
> transparent.
>
> > If you agree that it's just a safety net, then maybe explain that in a
> > comment? If the safety net catches anything, the composition result
> > will be wrong anyway, so it doesn't matter to adjust the destination
> > rectangle to match.
>
> I will extract this whole clamping stuff in a function, is this comment
> enough?
>
> * This function is mainly a safety net to avoid reading outside the source
> buffer. As the
> * userspace should never ask to read outside the source plane, all the cases
> covered here should
> * be dead code.
Sure. Perhaps use a bit more assertive tone about what the UAPI
contract guarantees. Yes, userspace "should not", but the kernel DRM
code ensures that it does not.
> > When the last point is relaxed and VKMS gains scaling support, I think
> > it won't change the fact that the clamping remains as a safety net. It
> > just increases the risk of bugs that would be caught by the net.
> >
> > Going outside of FB boundaries is a serious bug and deserves to be
> > checked. Going outside of the source rectangle would be a bug too,
> > assuming that partially included pixels are considered fully included,
> > but it's not serious enough to warrant explicit checks. Ideally IGT
> > would catch it.
>
> That was exactly the idea behind all those check and clamping: avoid
> access outside the buffers.
Good.
To catch a driver using pixels outside of a source rectangle, the test
FB in IGT should be painted to have a different non-black color outside
of the source rectangle.
> > > + }
> > > +
> > > + if (pixel_count <= 0) {
> > > + /* Nothing to read, so avoid multiple function
> > > calls for nothing */
> > > + continue;
> > > + }
> > > +
> > > + /*
> > > + * Modify the starting point to take in account the
> > > rotation
> > > + *
> > > + * src_line is the top-left corner, so when reading
> > > READ_RIGHT_TO_LEFT or
> > > + * READ_BOTTOM_TO_TOP, it must be changed to the
> > > top-right/bottom-left
> > > + * corner.
> > > + */
> > > + if (direction == READ_RIGHT_TO_LEFT) {
> > > + // x_start is now the right point
> > > + x_start += pixel_count - 1;
> > > + } else if (direction == READ_BOTTOM_TO_TOP) {
> > > + // y_start is now the bottom point
> > > + y_start += pixel_count - 1;
> > > + }
> > > +
> > > + /*
> > > + * Perform the conversion and the blending
> > > + *
> > > + * Here we know that the read line (x_start, y_start,
> > > pixel_count) is
> > > + * inside the source buffer [2] and we don't write
> > > outside the stage
> > > + * buffer [1]
> > > + */
> > > + current_plane->pixel_read_line(
> > > + current_plane, x_start, y_start, direction,
> > > pixel_count,
> > > +
> > > &stage_buffer->pixels[current_plane->frame_info->dst.x1]);
> > > +
> > > + pre_mul_alpha_blend(stage_buffer, output_buffer,
> > > + current_plane->frame_info->dst.x1,
> > > + pixel_count);
> > > }
> > >
> > > apply_lut(crtc_state, output_buffer);
> > > @@ -250,7 +335,7 @@ static void blend(struct vkms_writeback_job *wb,
> > > *crc32 = crc32_le(*crc32, (void *)output_buffer->pixels,
> > > row_size);
> > >
> > > if (wb)
> > > - vkms_writeback_row(wb, output_buffer, y_pos);
> > > + vkms_writeback_row(wb, output_buffer, y);
> > > }
> > > }
Thanks,
pq
pgpBx5tJk0ITA.pgp
Description: OpenPGP digital signature
