[PATCH v6 09/17] drm/vkms: Introduce pixel_read_direction enum

[Louis Chauvet]

[...]

> > +/**
> > + * direction_for_rotation() - Get the correct reading direction for a given rotation
> > + *
> > + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.
> > + *
> > + * This function will use the @rotation setting of a source plane to compute the reading
> > + * direction in this plane which correspond to a "left to right writing" in the CRTC.
> > + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> > + * to be written from left to right on the CRTC.
> > + */
> > +static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> > +{
> > +	struct drm_rect tmp_a, tmp_b;
> > +	int x, y;
> > +
> > +	/*
> > +	 * The direction is computed by rotating the vector AB (top-left to top-right) in a
> > +	 * 1x1 square.
> 
> Points A and B are depicted as zero-size rectangles on the CRTC.
> The CRTC writing direction is from A to B. The plane reading direction
> is discovered by inverse-transforming A and B.
> 
> (If you want, you can add that to the comment.)

It is better, thanks!
 
> > +	 */
> > +
> > +	tmp_a = DRM_RECT_INIT(0, 0, 0, 0);
> > +	tmp_b = DRM_RECT_INIT(1, 0, 0, 0);
> > +	drm_rect_rotate_inv(&tmp_a, 1, 1, rotation);
> > +	drm_rect_rotate_inv(&tmp_b, 1, 1, rotation);
> > +
> > +	x = tmp_b.x1 - tmp_a.x1;
> > +	y = tmp_b.y1 - tmp_a.y1;
> > +
> > +	if (x == 1)
> > +		return READ_LEFT_TO_RIGHT;
> > +	else if (x == -1)
> > +		return READ_RIGHT_TO_LEFT;
> > +	else if (y == 1)
> > +		return READ_TOP_TO_BOTTOM;
> > +	else if (y == -1)
> > +		return READ_BOTTOM_TO_TOP;
> 
> I find this code practically obvious. Excellent!
> 
> If you want to be more strict, each condition could also require the
> other component to be zero.

I will add it.

[...]

> > + */
> > +static int get_block_step_byte(struct drm_framebuffer *fb, enum pixel_read_direction direction,
> > +			       int plane_index)
> > +{
> > +	switch (direction) {
> > +	case READ_LEFT_TO_RIGHT:
> > +		return fb->format->char_per_block[plane_index];
> > +	case READ_RIGHT_TO_LEFT:
> > +		return -fb->format->char_per_block[plane_index];
> > +	case READ_TOP_TO_BOTTOM:
> > +		return (int)fb->pitches[plane_index];
> > +	case READ_BOTTOM_TO_TOP:
> > +		return -(int)fb->pitches[plane_index];
> 
> I'm not sure if this is correct for formats with block_h > 1.
> 
> If a pitch is the theoretical count of bytes per line, then this should
> return block_h * pitch. But I'm not exactly sure what is correct here.

I think it is related to my answer to patch 07/17. If pitch is what you 
describe, yes, the step should be block_h * DIV_ROUND_UP(fb_width / 
block_w) (or something similar).

If I take X0L2 with a buffer of 9x5 pixels, the step between two blocks 
verticaly must be 40 bytes. Your formula and interpretation of pitch will 
give only 36 bytes (a row only need 18 bytes). So a new "pitch" is needed.

> Aside from this problem, looks good.
> 
> 
> Thanks,
> pq
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * packed_pixels_addr_1x1() - Get the pointer to the block containing the pixel at the given
> >   * coordinates
> > 
> 



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