On 4/26/22 04:09, Pekka Paalanen wrote:
On Mon, 25 Apr 2022 21:56:12 -0300 Igor Torrente igormtorrente@gmail.com wrote:
Hi Pekka,
On 4/25/22 04:56, Pekka Paalanen wrote:
On Sat, 23 Apr 2022 12:12:51 -0300 Igor Torrente igormtorrente@gmail.com wrote:
Hi Pekka,
On 4/20/22 08:23, Pekka Paalanen wrote:
On Mon, 4 Apr 2022 17:45:11 -0300 Igor Torrente igormtorrente@gmail.com wrote:
This commit is the groundwork to introduce new formats to the planes and writeback buffer. As part of it, a new buffer metadata field is added to `vkms_writeback_job`, this metadata is represented by the `vkms_composer` struct.
Hi,
should this be talking about vkms_frame_info struct instead?
Yes it should. I will fix this. Thanks.
Also adds two new function pointers (`{wb,plane}_format_transform_func`) are defined to handle format conversion to/from internal format.
These things will allow us, in the future, to have different compositing and wb format types.
V2: Change the code to get the drm_framebuffer reference and not copy its contents(Thomas Zimmermann). V3: Drop the refcount in the wb code(Thomas Zimmermann). V5: Add {wb,plane}_format_transform_func to vkms_writeback_job and vkms_plane_state (Pekka Paalanen)
Signed-off-by: Igor Torrente igormtorrente@gmail.com
drivers/gpu/drm/vkms/vkms_composer.c | 4 ++-- drivers/gpu/drm/vkms/vkms_drv.h | 31 +++++++++++++++++++++------ drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++----- drivers/gpu/drm/vkms/vkms_writeback.c | 20 ++++++++++++++--- 4 files changed, 49 insertions(+), 16 deletions(-)...
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 2e6342164bef..2704cfb6904b 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h
...
+struct vkms_writeback_job {
- struct dma_buf_map data[DRM_FORMAT_MAX_PLANES];
- struct vkms_frame_info frame_info;
Which frame_info is this? Should the field be called wb_frame_info?
Considering it's already in the writeback_job struct do you think this necessary?
This struct has 'data' too, and that is not the wb buffer, right?
AFAIU, no. Each plane has its own `iosys_map map[]`.
Hmm, if it's not the wb buffer, then using DRM_FORMAT_MAX_PLANES is odd...
Yeah. I honestly don't have any clue why we have an array of `iosys_map` for each plane, why we only use the map[0] and why we only call `iosys_map_is_null` only to the `primary_composer`.
Maybe @tzimmermann or @rodrigosiqueiramelo can shed some light on these questions.
Yeah, those questions would be really good to figure out.
FWIW, I can tell you this: "plane" has two different meanings in the context of KMS:
https://gitlab.freedesktop.org/pq/color-and-hdr/-/blob/main/doc/glossary.md#...
DRM_FORMAT_MAX_PLANES refers to the number of planes (or the number of memory buffers) for a single image (single framebuffer). Most often with RGB there is just one plane in one memory buffer. RGB buffer could be accompanied with e.g. a compression data buffer, so two planes, one buffer each. Different YUV formats have different numbers of planes from N=1 to 3, and those plane may be stored in 1 to N memory buffers (with dmabuf handles pointing to them).
The number of planes and the number of memory buffers are often conflated in APIs by just passing the same memory buffer multiple times when multiple planes are stored in the same buffer (with different offset).
The number of planes is determined by the pixel format and the format modifier. The number of memory buffers is more... vaguely defined and may vary sometimes.
Right. So probably this answers the first two questions. And also probably my initial implementation of YUV420 and NV12 is wrong.
In other words, what kind of misudertanding do you think can happen if this variable stay without the `wb_` prefix?
I spent a few minutes trying to find a case, but nothing came to my mind.
My question above is the confusion.
If all these members are about the wb destination buffer only, then where do the inputs come from and how are they reference-counted to make sure they are available when needed?
Ok. Got it.
Thanks, pq