Le 27/03/24 - 14:29, Pekka Paalanen a écrit :
> On Tue, 26 Mar 2024 16:57:02 +0100
> Louis Chauvet <louis.chauvet@xxxxxxxxxxx> 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.
* This function is mainly a safety net to avoid reading outside the source buffer. As the
* userspace can't ask to read outside the source plane, all the cases covered here should
* be dead code.
> > > 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.
The IGT test kms_plane already does that, you can use an option to ask for
a border around the framebuffer.
Thanks,
Louis Chauvet
> > > > + }
> > > > +
> > > > + 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
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
