Re: [PATCH v5 6/9] drm: vkms: Refactor the plane composer to accept new formats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On April 26, 2022 10:03:09 PM GMT-03:00, Igor Torrente <igormtorrente@xxxxxxxxx> wrote:
>
>
>On 4/25/22 22:54, Igor Torrente wrote:
>> Hi Pekka,
>> 
>> On 4/25/22 05:10, Pekka Paalanen wrote:
>>> On Sat, 23 Apr 2022 15:53:20 -0300
>>> Igor Torrente <igormtorrente@xxxxxxxxx> wrote:
>>> 
>>>> I forgot to respond some points from your review.
>>>> 
>>>> On 4/23/22 13:04, Igor Torrente wrote:
>>>>> Hi Pekka,
>>>>> 
>>>>> On 4/20/22 09:36, Pekka Paalanen wrote:
>>>>>> On Mon,  4 Apr 2022 17:45:12 -0300
>>>>>> Igor Torrente <igormtorrente@xxxxxxxxx> wrote:
>>>>>>    
>>>>>>> Currently the blend function only accepts XRGB_8888 and ARGB_8888
>>>>>>> as a color input.
>>>>>>> 
>>>>>>> This patch refactors all the functions related to the plane composition
>>>>>>> to overcome this limitation.
>>>>>>> 
>>>>>>> A new internal format(`struct pixel`) is introduced to deal with all
>>>>>> 
>>>>>> Hi,
>>>>>> 
>>>>>> struct pixel_argb_u16 was added in the previous patch.
>>>>> 
>>>>> I will fix it. Thanks!
>>> 
>>> ...
>>> 
>>>>>>> +static int compose_active_planes(struct vkms_writeback_job *active_wb,
>>>>>>> +				 struct vkms_crtc_state *crtc_state,
>>>>>>> +				 u32 *crc32)
>>>>>>>      {
>>>>>>> +	int line_width, ret = 0, pixel_size = sizeof(struct pixel_argb_u16);
>>>>>>> +	struct vkms_frame_info *primary_plane_info = NULL;
>>>>>>> +	struct line_buffer output_buffer, stage_buffer;
>>>>>>> +	struct vkms_plane_state *act_plane = NULL;
>>>>>>> +	u32 wb_format;
>>>>>>>      +	if (WARN_ON(pixel_size != 8))
>>>>>> 
>>>>>> Isn't there a compile-time assert macro for this? Having to actually
>>>>>> run VKMS to check for this reduces the chances of finding it early.
>>>>>> What's the reason for this check anyway?
>>>> 
>>>> Yes, it exists.
>>>> 
>>>> include/linux/build_bug.h:1:#define static_assert(expr, ...)
>>>> __static_assert(expr, ##__VA_ARGS__, #expr)
>>>> 
>>>> I didn't add it because I can imagine some people very mad if the kernel
>>>> did not compile because of vkms.
>>> 
>>> But that would mean that VKMS is broken on those platforms. You'd
>>> better know which platforms VKMS is broken, so the Kconfig can stop
>>> VKMS from being built there at all. Or better, fix it before anyone
>>> needs VKMS there.
>> 
>> Right. Makes sense. I will add it then.
>> 
>>> 
>>>> This check exists so we can call `crc32_le` for the entire line instead
>>>> doing it for each channel of each pixel in case `struct `pixel_argb_u16`
>>>> had any gap added by the compiler between the struct fields.
>>> 
>>> Oh the CRC computation. Good point.
>>> 
>>> Can you add a comment about that with the check?
>> 
>> Yeah, np.
>> 
>> I will copy the explanation above :)
>> 
>>> 
>>>>>>    
>>>>>>> +		return -EINVAL;
>>>>>>> +
>>>>>>> +	if (crtc_state->num_active_planes >= 1) {
>>>>>>> +		act_plane = crtc_state->active_planes[0];
>>>>>>> +		if (act_plane->base.base.plane->type == DRM_PLANE_TYPE_PRIMARY)
>>>>>>> +			primary_plane_info = act_plane->frame_info;
>>>>>> 
>>>>>> After the next patch, do you even need the primary plane for anything
>>>>>> specifically?
>>>> 
>>>> Yeah, I will not need it anymore.
>>>> 
>>>>>> There is the map_is_null check below, but that should be
>>>>>> done on all planes in the array, right?
>>>> 
>>>> Yes, I guess so. And I don't know why it only checks for the
>>>> primary_plane TBH.
>>> 
>>> Maybe a left-over from times when it didn't have anything but a primary
>>> plane?
>> 
>> Maybe.
>> 
>> Anyway, I have added this verification to all active planes in the  next
>> version.
>> 
>>> 
>>>>>> 
>>>>>> I suspect the next patch, or another patch in this series, should just
>>>>>> delete this chunk.
>>>> I should, and I will in the V6 of next patch.
>>>> 
>>>>> 
>>>>> 
>>>>>     
>>>>>>    
>>>>>>>      	}
>>>>>>>      +	if (!primary_plane_info)
>>>>>>> +		return -EINVAL;
>>>>>>> +
>>>>>>>      	if (WARN_ON(dma_buf_map_is_null(&primary_plane_info->map[0])))
>>>>>>>      		return -EINVAL;
>>>>>>>      +	if (WARN_ON(check_format_funcs(crtc_state, active_wb)))
>>>>>>> +		return -EINVAL;
>>>>>>>      +	line_width = drm_rect_width(&primary_plane_info->dst);
>>>>>>> +	stage_buffer.n_pixels = line_width;
>>>>>>> +	output_buffer.n_pixels = line_width;
>>>>>>>      +	stage_buffer.pixels = kvmalloc(line_width * pixel_size, GFP_KERNEL);
>>>>>>> +	if (!stage_buffer.pixels) {
>>>>>>> +		DRM_ERROR("Cannot allocate memory for the output line buffer");
>>>>>>> +		return -ENOMEM;
>>>>>>> +	}
>>>>>>>      +	output_buffer.pixels = kvmalloc(line_width * pixel_size, GFP_KERNEL);
>>>>>>> +	if (!output_buffer.pixels) {
>>>>>>> +		DRM_ERROR("Cannot allocate memory for intermediate line buffer");
>>>>>>> +		ret = -ENOMEM;
>>>>>>> +		goto free_stage_buffer;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (active_wb) {
>>>>>>> +		struct vkms_frame_info *wb_frame_info = &active_wb->frame_info;
>>>>>>> +
>>>>>>> +		wb_format = wb_frame_info->fb->format->format;
>>>>>> 
>>>>>> I don't see wb_format being used, is it?
>>>>> 
>>>>> This is probably a leftover from the last versions. Thanks for catching
>>>>> it.
>>>>>     
>>>>>>    
>>>>>>> +		wb_frame_info->src = primary_plane_info->src;
>>>>>>> +		wb_frame_info->dst = primary_plane_info->dst;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	blend(active_wb, crtc_state, crc32, &stage_buffer,
>>>>>>> +	      &output_buffer, (s64)line_width * pixel_size);
>>>>>> 
>>>>>> What's the (s64) doing here?
>>>>>> 
>>>>>> Are byte sizes not usually expressed with size_t or ssize_t types, or
>>>>>> is the kernel convention to use u64 and s64?
>>>>>> 
>>>>>> This makes me suspect that pixel_offset() and friends in vkms_format.c
>>>>>> are going to need fixing as well. int type overflows at 2G.
>>>>> 
>>>>> 
>>>>> Yeah, I should be using size_t in all these places.
>>>>>     
>>>>>>    
>>>>>>> +
>>>>>>> +	kvfree(output_buffer.pixels);
>>>>>>> +free_stage_buffer:
>>>>>>> +	kvfree(stage_buffer.pixels);
>>>>>>> +can
>>>>>>> +	return ret;
>>>>>>>      }
>>>>>>>           /**
>>>>>>> @@ -222,13 +204,11 @@ void vkms_composer_worker(struct work_struct *work)
>>>>>>>      						struct vkms_crtc_state,
>>>>>>>      						composer_work);
>>>>>>>      	struct drm_crtc *crtc = crtc_state->base.crtc;
>>>>>>> +	struct vkms_writeback_job *active_wb = crtc_state->active_writeback;
>>>>>>>      	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>>>>>>>      	bool crc_pending, wb_pending;
>>>>>>>      	u64 frame_start, frame_end;
>>>>>>> +	u32 crc32 = 0;
>>>>>>>      	int ret;
>>>>>>>           	spin_lock_irq(&out->composer_lock);
>>>>>>> @@ -248,35 +228,19 @@ void vkms_composer_worker(struct work_struct *work)
>>>>>>>      	if (!crc_pending)
>>>>>>>      		return;
>>>>>>>           	if (wb_pending)
>>>>>>> +		ret = compose_active_planes(active_wb, crtc_state, &crc32);
>>>>>>> +	else
>>>>>>> +		ret = compose_active_planes(NULL, crtc_state, &crc32);
>>>>>>>      +	if (ret)
>>>>>>>      		return;
>>>>>>>           	if (wb_pending) {
>>>>>>>      		drm_writeback_signal_completion(&out->wb_connector, 0);
>>>>>>>      		spin_lock_irq(&out->composer_lock);
>>>>>>>      		crtc_state->wb_pending = false;
>>>>>>>      		spin_unlock_irq(&out->composer_lock);
>>>>>>>      	}
>>>>>>>           	/*
>>>>>>> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..931a61405d6a
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
>>>>>>> @@ -0,0 +1,151 @@
>>>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>>>> +
>>>>>>> +#include <drm/drm_rect.h>
>>>>>>> +#include <linux/minmax.h>
>>>>>>> +
>>>>>>> +#include "vkms_formats.h"
>>>>>>> +
>>>>>>> +static int pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
>>>>>>> +{
>>>>>>> +	return frame_info->offset + (y * frame_info->pitch)
>>>>>>> +				  + (x * frame_info->cpp);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * packed_pixels_addr - Get the pointer to pixel of a given pair of coordinates
>>>>>>> + *
>>>>>>> + * @frame_info: Buffer metadata
>>>>>>> + * @x: The x(width) coordinate of the 2D buffer
>>>>>>> + * @y: The y(Heigth) coordinate of the 2D buffercan
>>>>>>> + *
>>>>>>> + * 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).
>>>>>>> + */
>>>>>>> +static void *packed_pixels_addr(const struct vkms_frame_info *frame_info,
>>>>>>> +				int x, int y)
>>>>>>> +{
>>>>>>> +	int offset = pixel_offset(frame_info, x, y);
>>>>>>> +
>>>>>>> +	return (u8 *)frame_info->map[0].vaddr + offset;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y)
>>>>>>> +{
>>>>>>> +	int x_src = frame_info->src.x1 >> 16;
>>>>>>> +	int y_src = y - frame_info->dst.y1 + (frame_info->src.y1 >> 16);
>>>>>>> +
>>>>>>> +	return packed_pixels_addr(frame_info, x_src, y_src);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void ARGB8888_to_argb_u16(struct line_buffer *stage_buffer,
>>>>>>> +				 const struct vkms_frame_info *frame_info, int y)
>>>>>>> +{
>>>>>>> +	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
>>>>>>> +	u8 *src_pixels = get_packed_src_addr(frame_info, y);
>>>>>>> +	int x, x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
>>>>>>> +			       stage_buffer->n_pixels);
>>>>>>> +
>>>>>>> +	for (x = 0; x < x_limit; x++, src_pixels += 4) {
>>>>>>> +		/*
>>>>>>> +		 * The 257 is the "conversion ratio". This number is obtained by the
>>>>>>> +		 * (2^16 - 1) / (2^8 - 1) division. Which, in this case, tries to get
>>>>>>> +		 * the best color value in a pixel format with more possibilities.
>>>>>>> +		 * A similar idea applies to others RGB color conversions.
>>>>>>> +		 */
>>>>>>> +		out_pixels[x].a = (u16)src_pixels[3] * 257;
>>>>>>> +		out_pixels[x].r = (u16)src_pixels[2] * 257;
>>>>>>> +		out_pixels[x].g = (u16)src_pixels[1] * 257;
>>>>>>> +		out_pixels[x].b = (u16)src_pixels[0] * 257;
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void XRGB8888_to_argb_u16(struct line_buffer *stage_buffer,
>>>>>>> +				 const struct vkms_frame_info *frame_info, int y)
>>>>>>> +{
>>>>>>> +	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
>>>>>>> +	u8 *src_pixels = get_packed_src_addr(frame_info, y);
>>>>>>> +	int x, x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
>>>>>>> +			       stage_buffer->n_pixels);
>>>>>>> +
>>>>>>> +	for (x = 0; x < x_limit; x++, src_pixels += 4) {
>>>>>>> +		out_pixels[x].a = (u16)0xffff;
>>>>>>> +		out_pixels[x].r = (u16)src_pixels[2] * 257;
>>>>>>> +		out_pixels[x].g = (u16)src_pixels[1] * 257;
>>>>>>> +		out_pixels[x].b = (u16)src_pixels[0] * 257;
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * The following  functions take an line of argb_u16 pixels from the
>>>>>>> + * src_buffer, convert them to a specific format, and store them in the
>>>>>>> + * destination.
>>>>>>> + *
>>>>>>> + * They are used in the `compose_active_planes` to convert and store a line
>>>>>>> + * from the src_buffer to the writeback buffer.
>>>>>>> + */
>>>>>>> +static void argb_u16_to_ARGB8888(struct vkms_frame_info *frame_info,
>>>>>>> +				 const struct line_buffer *src_buffer, int y)
>>>>>>> +{
>>>>>>> +	int x, x_dst = frame_info->dst.x1;
>>>>>>> +	u8 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y);
>>>>>>> +	struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
>>>>>>> +	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
>>>>>>> +			    src_buffer->n_pixels);
>>>>>>> +
>>>>>>> +	for (x = 0; x < x_limit; x++, dst_pixels += 4) {
>>>>>>> +		/*
>>>>>>> +		 * This sequence below is important because the format's byte order is
>>>>>>> +		 * in little-endian. In the case of the ARGB8888 the memory is
>>>>>>> +		 * organized this way:
>>>>>>> +		 *
>>>>>>> +		 * | Addr     | = blue channel
>>>>>>> +		 * | Addr + 1 | = green channel
>>>>>>> +		 * | Addr + 2 | = Red channel
>>>>>>> +		 * | Addr + 3 | = Alpha channel
>>>>>>> +		 */
>>>>>>> +		dst_pixels[3] = DIV_ROUND_CLOSEST(in_pixels[x].a, 257);
>>>>>>> +		dst_pixels[2] = DIV_ROUND_CLOSEST(in_pixels[x].r, 257);
>>>>>>> +		dst_pixels[1] = DIV_ROUND_CLOSEST(in_pixels[x].g, 257);
>>>>>>> +		dst_pixels[0] = DIV_ROUND_CLOSEST(in_pixels[x].b, 257);
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void argb_u16_to_XRGB8888(struct vkms_frame_info *frame_info,
>>>>>>> +				 const struct line_buffer *src_buffer, int y)
>>>>>>> +{
>>>>>>> +	int x, x_dst = frame_info->dst.x1;
>>>>>>> +	u8 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y);
>>>>>>> +	struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
>>>>>>> +	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
>>>>>>> +			    src_buffer->n_pixels);
>>>>>>> +
>>>>>>> +	for (x = 0; x < x_limit; x++, dst_pixels += 4) {
>>>>>>> +		dst_pixels[3] = (u8)0xff;
>>>>>> 
>>>>>> When writing to XRGB, it's not necessary to ensure the X channel has
>>>>>> any sensible value. Anyone reading from XRGB must ignore that value
>>>>>> anyway. So why not write something wacky here, like 0xa1, that is far
>>>>>> enough from both 0x00 or 0xff to not be confused with them even
>>>>>> visually? Also not 0x7f or 0x80 which are close to half of 0xff.
>>>>>> 
>>>>>> Or, you could save a whole function and just use argb_u16_to_ARGBxxxx()
>>>>>> instead, even for XRGB destination.
>>>>> 
>>>>> 
>>>>> Right. Maybe I could just leave the channel untouched.
>>> 
>>> Untouched may not be a good idea. Leaving anything untouched always has
>>> the risk of leaking information through uninitialized memory. Maybe not
>>> in this case because the destination is allocated by userspace already,
>>> but nothing beats being obviously correct.
>> 
>> Makes sense.
>> 
>>> 
>>> Whatever you decide here, be prepared for it becoming de-facto kernel
>>> UABI, because it is easy for userspace to (accidentally) rely on the
>>> value, no matter what you pick.
>> 
>> I hope to make the right decision then.
>
>The de-facto UABI seems to be already in place for {A, X}RGB8888.

"Only XRGB_8888

>
>I changed from 0xff to 0xbe and the `writeback-check-output` started to fail.
>
>> 
>>> 
>>> 
>>> Thanks,
>>> pq
>>> 
>>> 
>>>>>     
>>>>>>    
>>>>>>> +		dst_pixels[2] = DIV_ROUND_CLOSEST(in_pixels[x].r, 257);
>>>>>>> +		dst_pixels[1] = DIV_ROUND_CLOSEST(in_pixels[x].g, 257);
>>>>>>> +		dst_pixels[0] = DIV_ROUND_CLOSEST(in_pixels[x].b, 257);
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +
>>>>>>> +plane_format_transform_func get_plane_fmt_transform_function(u32 format)
>>>>>>> +{
>>>>>>> +	if (format == DRM_FORMAT_ARGB8888)
>>>>>>> +		return &ARGB8888_to_argb_u16;
>>>>>>> +	else if (format == DRM_FORMAT_XRGB8888)
>>>>>>> +		return &XRGB8888_to_argb_u16;
>>>>>>> +	else
>>>>>>> +		return NULL;
>>>>>> 
>>>>>> This works for now, but when more formats are added, I'd think a switch
>>>>>> statement would look better.
>>>>> 
>>>>> ok.
>>>>>     
>>>>>>    
>>>>>>> +}
>>>>>>> +
>>>>>>> +wb_format_transform_func get_wb_fmt_transform_function(u32 format)
>>>>>>> +{
>>>>>>> +	if (format == DRM_FORMAT_ARGB8888)
>>>>>>> +		return &argb_u16_to_ARGB8888;
>>>>>>> +	else if (format == DRM_FORMAT_XRGB8888)
>>>>>>> +		return &argb_u16_to_XRGB8888;
>>>>>>> +	else
>>>>>>> +		return NULL;
>>>>>>> +}




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux