[PATCH v5 5/9] drm: vkms: Add fb information to `vkms_writeback_job`
Igor Torrente
igormtorrente at gmail.com
Tue Apr 26 00:56:12 UTC 2022
Hi Pekka,
On 4/25/22 04:56, Pekka Paalanen wrote:
> On Sat, 23 Apr 2022 12:12:51 -0300
> Igor Torrente <igormtorrente at 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 at 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 at 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_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>>>> index 2d946368a561..95029d2ebcac 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>>>> @@ -153,7 +153,7 @@ static void compose_plane(struct vkms_frame_info *primary_plane_info,
>>>> struct vkms_frame_info *plane_frame_info,
>>>> void *vaddr_out)
>>>> {
>>>> - struct drm_framebuffer *fb = &plane_frame_info->fb;
>>>> + struct drm_framebuffer *fb = plane_frame_info->fb;
>>>> void *vaddr;
>>>> void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>>>>
>>>> @@ -175,7 +175,7 @@ static int compose_active_planes(void **vaddr_out,
>>>> struct vkms_frame_info *primary_plane_info,
>>>> struct vkms_crtc_state *crtc_state)
>>>> {
>>>> - struct drm_framebuffer *fb = &primary_plane_info->fb;
>>>> + struct drm_framebuffer *fb = primary_plane_info->fb;
>>>> struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>>>> const void *vaddr;
>>>> int i;
>>>> 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
>>>> @@ -22,13 +22,8 @@
>>>>
>>>> #define NUM_OVERLAY_PLANES 8
>>>>
>>>> -struct vkms_writeback_job {
>>>> - struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
>>>> - struct dma_buf_map data[DRM_FORMAT_MAX_PLANES];
>>>> -};
>>>> -
>>>> struct vkms_frame_info {
>>>> - struct drm_framebuffer fb;
>>>> + struct drm_framebuffer *fb;
>>>> struct drm_rect src, dst;
>>>> struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
>>>> unsigned int offset;
>>>> @@ -36,6 +31,29 @@ struct vkms_frame_info {
>>>> unsigned int cpp;
>>>> };
>>>>
>>>> +struct pixel_argb_u16 {
>>>> + u16 a, r, g, b;
>>>> +};
>>>> +
>>>> +struct line_buffer {
>>>> + size_t n_pixels;
>>>> + struct pixel_argb_u16 *pixels;
>>>> +};
>>>> +
>>>> +typedef void
>>>> +(*wb_format_transform_func)(struct vkms_frame_info *frame_info,
>>>> + const struct line_buffer *buffer, int y);
>>>> +
>>>> +typedef void
>>>> +(*plane_format_transform_func)(struct line_buffer *buffer,
>>>> + const struct vkms_frame_info *frame_info, int y);
>>>
>>> It wasn't immediately obvious to me in which direction these function
>>> types work from their names. The arguments are not wb and plane but
>>> vkms_frame_info and line_buffer in both. The implementations of these
>>> functions would have nothing specific to a wb or a plane either, would
>>> they?
>>
>> No, there's nothing specific.
>>
>> Do you think adding {dst_,src_} would be enough?
>>
>> (*wb_format_transform_func)(struct vkms_frame_info *dst_frame_info,
>> const struct line_buffer *src_buffer
>>
>> (*plane_format_transform_func)(struct line_buffer *dst_buffer,
>> const struct vkms_frame_info *src_frame_info,
>
> No, because I was looking at the function pointer type names, and not
> the function arguments.
Ohhh.
>
>>>
>>> What about naming them frame_to_line_func and line_to_frame_func?
>>
>> Sounds good. I will rename it.
>
> Thanks!
>
>>>> +
>>>> +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.
>
>> 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.
>
>>>> + wb_format_transform_func format_func;
>>>
>>> line_to_frame_func wb_write;
>>>
>>> perhaps? The type explains the general type of the function, and the
>>> field name refers to what it is used for.
>>>
>>>> +};
>>>> +
>>>> /**
>>>> * vkms_plane_state - Driver specific plane state
>>>> * @base: base plane state
>>>> @@ -44,6 +62,7 @@ struct vkms_frame_info {
>>>> struct vkms_plane_state {
>>>> struct drm_shadow_plane_state base;
>>>> struct vkms_frame_info *frame_info;
>>>> + plane_format_transform_func format_func;
>>>
>>> Similarly here, maybe
>>>
>>> frame_to_line_func plane_read;
>>>
>>> perhaps?
>>
>> Yeah, sure.
>>
>>>
>>>> };
>>>>
>>>> struct vkms_plane {
>>>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>>>> index a56b0f76eddd..28752af0118c 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>>>> @@ -50,12 +50,12 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
>>>> struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
>>>> struct drm_crtc *crtc = vkms_state->base.base.crtc;
>>>>
>>>> - if (crtc) {
>>>> + if (crtc && vkms_state->frame_info->fb) {
>>>> /* dropping the reference we acquired in
>>>> * vkms_primary_plane_update()
>>>> */
>>>> - if (drm_framebuffer_read_refcount(&vkms_state->frame_info->fb))
>>>> - drm_framebuffer_put(&vkms_state->frame_info->fb);
>>>> + if (drm_framebuffer_read_refcount(vkms_state->frame_info->fb))
>>>> + drm_framebuffer_put(vkms_state->frame_info->fb);
>>>> }
>>>>
>>>> kfree(vkms_state->frame_info);
>>>> @@ -110,9 +110,9 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>>>> frame_info = vkms_plane_state->frame_info;
>>>> memcpy(&frame_info->src, &new_state->src, sizeof(struct drm_rect));
>>>> memcpy(&frame_info->dst, &new_state->dst, sizeof(struct drm_rect));
>>>> - memcpy(&frame_info->fb, fb, sizeof(struct drm_framebuffer));
>>>> + frame_info->fb = fb;
>>>
>>> This change, replacing the memcpy with storing a pointer, seems to be
>>> another major point of this patch. Should it be a separate patch?
>>> It doesn't seem to fit with the current commit message.
>>>
>>> I have no idea what kind of locking or referencing a drm_framebuffer
>>> would need, and I suspect that would be easier to review if it was a
>>> patch of its own.
>>
>> Makes sense. I will do that.
>>
>>>
>>>> memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
>>>> - drm_framebuffer_get(&frame_info->fb);
>>>> + drm_framebuffer_get(frame_info->fb);
>>>
>>> Does drm_framebuffer_get() not return anything?
>>
>> No, it doesn't actually. This function increments the ref count of this
>> struct and doesn't return anything.
>
> D'oh. Oh well.
>
>
> Thanks,
> pq
>
>>>
>>> To me it would be more idiomatic to write something like
>>>
>>> frame_info->fb = drm_framebuffer_get(fb);
>>> I spend few minutes trying to find a case but nothing comes to my mind.
>>> I don't know if that pattern is used in the kernel, but I use it in
>>> userspace to emphasise that frame_info owns a new reference rather than
>>> borrowing someone else's.
>>>
>>>
>>> Thanks,
>>> pq
More information about the dri-devel
mailing list