Re: [PATCH v2 2/7] drm/vkms: Add support for multy-planar framebuffers

[Arthur Grillo]

On 01/02/24 14:38, Louis Chauvet wrote:
>>  
>>  /*
>> @@ -23,27 +23,25 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int
>>   * @frame_info: Buffer metadata
>>   * @x: The x(width) coordinate of the 2D buffer
>>   * @y: The y(Heigth) coordinate of the 2D buffer
>> + * @index: The index of the plane on the 2D buffer
>>   *
>>   * Takes the information stored in the frame_info, a pair of coordinates, and
>> - * returns the address of the first color channel.
>> - * This function assumes the channels are packed together, i.e. a color channel
>> - * comes immediately after another in the memory. And therefore, this function
>> - * doesn't work for YUV with chroma subsampling (e.g. YUV420 and NV21).
>> + * returns the address of the first color channel on the desired index.
>>   */
>>  static void *packed_pixels_addr(const struct vkms_frame_info *frame_info,
>> -				int x, int y)
>> +				int x, int y, size_t index)
>>  {
>> -	size_t offset = pixel_offset(frame_info, x, y);
>> +	size_t offset = pixel_offset(frame_info, x, y, index);
>>  
>>  	return (u8 *)frame_info->map[0].vaddr + offset;
>>  }
> 
> This implementation of packed_pixels_addr will only work with
> block_w == block_h == 1. For packed or tiled formats we will need to use
> x/y information to extract the correct address, and this address will not 
> be a single pixel. See below my explanation.

You're right, currently, VKMS only supports non-packed/tiled formats. As
all the formats I plan to add are too not packed or tiled, I haven't
added support to it. But if you want to add it, please do :).

>> @@ -130,17 +128,28 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
>>  {
>>  	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
>>  	struct vkms_frame_info *frame_info = plane->frame_info;
>> -	u8 *src_pixels = get_packed_src_addr(frame_info, y);
>> +	const struct drm_format_info *frame_format = frame_info->fb->format;
>>  	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
>> +	u8 *src_pixels[DRM_FORMAT_MAX_PLANES];
>>  
>> -	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->fb->format->cpp[0]) {
>> +	for (size_t i = 0; i < frame_format->num_planes; i++)
>> +		src_pixels[i] = get_packed_src_addr(frame_info, y, i);
>> +
>> +	for (size_t x = 0; x < limit; x++) {
>>  		int x_pos = get_x_position(frame_info, limit, x);
>>  
>> -		if (drm_rotation_90_or_270(frame_info->rotation))
>> -			src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1)
>> -				+ frame_info->fb->format->cpp[0] * y;
>> +		if (drm_rotation_90_or_270(frame_info->rotation)) {
>> +			for (size_t i = 0; i < frame_format->num_planes; i++) {
>> +				src_pixels[i] = get_packed_src_addr(frame_info,
>> +								    x + frame_info->rotated.y1, i);
>> +				src_pixels[i] += frame_format->cpp[i] * y;
> 
> I find the current rotation management a bit complex to understand. This 
> is not related to your patch, but as I had to understand this to create my 
> second patch, I think this could be significanlty simplified.

I also found the rotation logic complex when implementing this. I would
appreciate it if it were simplified.

> 
> Please see the below comment about frame_format->cpp, it applies here too. 
> I think the "easy" way here is simply to reuse the method 
> get_packed_src_addr every time you need a new pixel.
> 
>> +			}
>> +		}
>>  
>> 		plane->pixel_read(src_pixels, &out_pixels[x_pos]);
>> +
> 
> The usage of cpp and pointer to specific pixel only work for non-packed 
> and non-blocked pixels, but for example NV30 or Y0L0 need more 
> informations about the exact location of the pixel to convert and write 
> the correct pixel value (each pixel can't be referenced directly by a 
> pointer). For example NV30 uses 5 bytes to store 3 pixels (10 bits each), 
> so to access the "middle" one you need to read the 5 bytes and do a small 
> computation to extract it's value.

Great explanation, I can see what is the problem here.

> 
> I think a simple solution to handle most cases would be to profide two 
> more parameters: the x and y positions of the pixel to copy, using 
> "absolute coordinates" (i.e x=0,y=0 means the first byte of the src 
> buffer, not the first pixel in the `drm_rect src`, this way the method 
> `pixel_read` can extract the correct value).
> 
> This way it become easy to manage "complex" pixel representations in this 
> loop: simply increment x/y and let the pixel_read method handle 
> everything.
> 
> The second patch I will send is doing this. And as explained before, it 
> will also simplify a lot the code related to rotation and translation (no 
> more switch case everywhere to add offset to x/y, it simply use drm_rect_* 
> helpers).

I like this, expect my review soon :).

> 
> It's not optimal in term of performance (in some situation it will read 
> the same block multiple time to generate different pixels), but I 
> believe it still is an intersting trade-off.
> 
> In the future, if performance is actally critical, the whole composition 
> loop will have to be specialized for each pixel formats: some can be 
> treated line by line (as it's done today), but with blocks or packed 
> pixels it's more complex.
> 
>> +		for (size_t i = 0; i < frame_format->num_planes; i++)
>> +			src_pixels[i] += frame_format->cpp[i];
> 
> This is likely working with format with block_w != 1, see explanation 
> above.

I think you meant that is _not_ working. Yeah, as I already explained,
it was never my plan to add support for packed or tiled formats.

Best Regards,
~Arthur Grillo