| Message ID | 20220404204515.42144-6-igormtorrente@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add new formats support to vkms | expand |
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? > > 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_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? What about naming them frame_to_line_func and line_to_frame_func? > + > +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? > + 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? > }; > > 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. > 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? To me it would be more idiomatic to write something like frame_info->fb = drm_framebuffer_get(fb); 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 > frame_info->offset = fb->offsets[0]; > frame_info->pitch = fb->pitches[0]; > frame_info->cpp = fb->format->cpp[0]; > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c > index 746cb0abc6ec..ad4bb1fb37ca 100644 > --- a/drivers/gpu/drm/vkms/vkms_writeback.c > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c > @@ -74,12 +74,15 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector, > if (!vkmsjob) > return -ENOMEM; > > - ret = drm_gem_fb_vmap(job->fb, vkmsjob->map, vkmsjob->data); > + ret = drm_gem_fb_vmap(job->fb, vkmsjob->frame_info.map, vkmsjob->data); > if (ret) { > DRM_ERROR("vmap failed: %d\n", ret); > goto err_kfree; > } > > + vkmsjob->frame_info.fb = job->fb; > + drm_framebuffer_get(vkmsjob->frame_info.fb); > + > job->priv = vkmsjob; > > return 0; > @@ -98,7 +101,9 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector, > if (!job->fb) > return; > > - drm_gem_fb_vunmap(job->fb, vkmsjob->map); > + drm_gem_fb_vunmap(job->fb, vkmsjob->frame_info.map); > + > + drm_framebuffer_put(vkmsjob->frame_info.fb); > > vkmsdev = drm_device_to_vkms_device(job->fb->dev); > vkms_set_composer(&vkmsdev->output, false); > @@ -115,14 +120,23 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, > struct drm_writeback_connector *wb_conn = &output->wb_connector; > struct drm_connector_state *conn_state = wb_conn->base.state; > struct vkms_crtc_state *crtc_state = output->composer_state; > + struct drm_framebuffer *fb = connector_state->writeback_job->fb; > + struct vkms_writeback_job *active_wb; > + struct vkms_frame_info *wb_frame_info; > > if (!conn_state) > return; > > vkms_set_composer(&vkmsdev->output, true); > > + active_wb = conn_state->writeback_job->priv; > + wb_frame_info = &active_wb->frame_info; > + > spin_lock_irq(&output->composer_lock); > - crtc_state->active_writeback = conn_state->writeback_job->priv; > + crtc_state->active_writeback = active_wb; > + wb_frame_info->offset = fb->offsets[0]; > + wb_frame_info->pitch = fb->pitches[0]; > + wb_frame_info->cpp = fb->format->cpp[0]; > crtc_state->wb_pending = true; > spin_unlock_irq(&output->composer_lock); > drm_writeback_queue_job(wb_conn, connector_state);
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_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, > > What about naming them frame_to_line_func and line_to_frame_func? Sounds good. I will rename it. > >> + >> +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? 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. > >> + 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. > > 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 > >> frame_info->offset = fb->offsets[0]; >> frame_info->pitch = fb->pitches[0]; >> frame_info->cpp = fb->format->cpp[0]; >> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c >> index 746cb0abc6ec..ad4bb1fb37ca 100644 >> --- a/drivers/gpu/drm/vkms/vkms_writeback.c >> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c >> @@ -74,12 +74,15 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector, >> if (!vkmsjob) >> return -ENOMEM; >> >> - ret = drm_gem_fb_vmap(job->fb, vkmsjob->map, vkmsjob->data); >> + ret = drm_gem_fb_vmap(job->fb, vkmsjob->frame_info.map, vkmsjob->data); >> if (ret) { >> DRM_ERROR("vmap failed: %d\n", ret); >> goto err_kfree; >> } >> >> + vkmsjob->frame_info.fb = job->fb; >> + drm_framebuffer_get(vkmsjob->frame_info.fb); >> + >> job->priv = vkmsjob; >> >> return 0; >> @@ -98,7 +101,9 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector, >> if (!job->fb) >> return; >> >> - drm_gem_fb_vunmap(job->fb, vkmsjob->map); >> + drm_gem_fb_vunmap(job->fb, vkmsjob->frame_info.map); >> + >> + drm_framebuffer_put(vkmsjob->frame_info.fb); >> >> vkmsdev = drm_device_to_vkms_device(job->fb->dev); >> vkms_set_composer(&vkmsdev->output, false); >> @@ -115,14 +120,23 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, >> struct drm_writeback_connector *wb_conn = &output->wb_connector; >> struct drm_connector_state *conn_state = wb_conn->base.state; >> struct vkms_crtc_state *crtc_state = output->composer_state; >> + struct drm_framebuffer *fb = connector_state->writeback_job->fb; >> + struct vkms_writeback_job *active_wb; >> + struct vkms_frame_info *wb_frame_info; >> >> if (!conn_state) >> return; >> >> vkms_set_composer(&vkmsdev->output, true); >> >> + active_wb = conn_state->writeback_job->priv; >> + wb_frame_info = &active_wb->frame_info; >> + >> spin_lock_irq(&output->composer_lock); >> - crtc_state->active_writeback = conn_state->writeback_job->priv; >> + crtc_state->active_writeback = active_wb; >> + wb_frame_info->offset = fb->offsets[0]; >> + wb_frame_info->pitch = fb->pitches[0]; >> + wb_frame_info->cpp = fb->format->cpp[0]; >> crtc_state->wb_pending = true; >> spin_unlock_irq(&output->composer_lock); >> drm_writeback_queue_job(wb_conn, connector_state); >
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_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. > > > > 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? Hmm, if it's not the wb buffer, then using DRM_FORMAT_MAX_PLANES is odd... > 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? > >> + 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
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_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
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#plane 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. > > > > >> 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
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#plane > > 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
On Tue, 26 Apr 2022 21:43:12 -0300 Igor Torrente <igormtorrente@gmail.com> wrote: > 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#plane > > > > 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. If I'm reading the code right, it's using the maximum number of image planes (four) as the maximum number of KMS planes (chosen by the driver). IOW, it confusing the two meanings of "plane" which have nothing to do with each other. Assuming that there is one data[] element for each KMS plane created by VKMS. That makes a further assumption that each KMS plane framebuffer has only one image plane. I think. Which they do when you are limited to RGB formats. Thanks, pq
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); + +struct vkms_writeback_job { + struct dma_buf_map data[DRM_FORMAT_MAX_PLANES]; + struct vkms_frame_info frame_info; + wb_format_transform_func format_func; +}; + /** * 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; }; 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; memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map)); - drm_framebuffer_get(&frame_info->fb); + drm_framebuffer_get(frame_info->fb); frame_info->offset = fb->offsets[0]; frame_info->pitch = fb->pitches[0]; frame_info->cpp = fb->format->cpp[0]; diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 746cb0abc6ec..ad4bb1fb37ca 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -74,12 +74,15 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector, if (!vkmsjob) return -ENOMEM; - ret = drm_gem_fb_vmap(job->fb, vkmsjob->map, vkmsjob->data); + ret = drm_gem_fb_vmap(job->fb, vkmsjob->frame_info.map, vkmsjob->data); if (ret) { DRM_ERROR("vmap failed: %d\n", ret); goto err_kfree; } + vkmsjob->frame_info.fb = job->fb; + drm_framebuffer_get(vkmsjob->frame_info.fb); + job->priv = vkmsjob; return 0; @@ -98,7 +101,9 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector, if (!job->fb) return; - drm_gem_fb_vunmap(job->fb, vkmsjob->map); + drm_gem_fb_vunmap(job->fb, vkmsjob->frame_info.map); + + drm_framebuffer_put(vkmsjob->frame_info.fb); vkmsdev = drm_device_to_vkms_device(job->fb->dev); vkms_set_composer(&vkmsdev->output, false); @@ -115,14 +120,23 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn, struct drm_writeback_connector *wb_conn = &output->wb_connector; struct drm_connector_state *conn_state = wb_conn->base.state; struct vkms_crtc_state *crtc_state = output->composer_state; + struct drm_framebuffer *fb = connector_state->writeback_job->fb; + struct vkms_writeback_job *active_wb; + struct vkms_frame_info *wb_frame_info; if (!conn_state) return; vkms_set_composer(&vkmsdev->output, true); + active_wb = conn_state->writeback_job->priv; + wb_frame_info = &active_wb->frame_info; + spin_lock_irq(&output->composer_lock); - crtc_state->active_writeback = conn_state->writeback_job->priv; + crtc_state->active_writeback = active_wb; + wb_frame_info->offset = fb->offsets[0]; + wb_frame_info->pitch = fb->pitches[0]; + wb_frame_info->cpp = fb->format->cpp[0]; crtc_state->wb_pending = true; spin_unlock_irq(&output->composer_lock); drm_writeback_queue_job(wb_conn, connector_state);
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. 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(-)