Re: [PATCH v2 08/14] media: uapi: h264: Drop SLICE_PARAMS 'size' field

From: Paul Kocialkowski
Date: Thu Aug 06 2020 - 12:59:11 EST


Hi,

On Thu 06 Aug 20, 12:13, Ezequiel Garcia wrote:
> The SLICE_PARAMS control is intended for slice-based
> devices. In this mode, the OUTPUT buffer contains
> a single slice, and so the buffer's plane payload size
> can be used to query the slice size.

If we later extend the API for supporting multiple slices with dynamic array
controls, I guess we'll need to know the size of each slice in each control
elements. So I'd rather keep that even if it's indeed redundant with
vb2_get_plane_payload in single-slice mode.

What do you think?

Paul

> To reduce the API surface drop the size from the
> SLICE_PARAMS control.
>
> A follow-up change will remove other members in SLICE_PARAMS
> so we don't need to add padding fields here.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> ---
> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 3 ---
> drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 7 +++----
> include/media/h264-ctrls.h | 3 ---
> 3 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 427fc5727ec0..fff74b7bf32a 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1760,9 +1760,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> :stub-columns: 0
> :widths: 1 1 2
>
> - * - __u32
> - - ``size``
> - -
> * - __u32
> - ``start_byte_offset``
> Offset (in bytes) from the beginning of the OUTPUT buffer to the start
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index a9ba78b15907..8b6f05aadbe8 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -324,17 +324,16 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
> struct vb2_buffer *src_buf = &run->src->vb2_buf;
> struct cedrus_dev *dev = ctx->dev;
> dma_addr_t src_buf_addr;
> - u32 len = slice->size * 8;
> + size_t slice_bytes = vb2_get_plane_payload(src_buf, 0);
> unsigned int pic_width_in_mbs;
> bool mbaff_pic;
> u32 reg;
>
> - cedrus_write(dev, VE_H264_VLD_LEN, len);
> + cedrus_write(dev, VE_H264_VLD_LEN, slice_bytes * 8);
> cedrus_write(dev, VE_H264_VLD_OFFSET, 0);
>
> src_buf_addr = vb2_dma_contig_plane_dma_addr(src_buf, 0);
> - cedrus_write(dev, VE_H264_VLD_END,
> - src_buf_addr + vb2_get_plane_payload(src_buf, 0));
> + cedrus_write(dev, VE_H264_VLD_END, src_buf_addr + slice_bytes);
> cedrus_write(dev, VE_H264_VLD_ADDR,
> VE_H264_VLD_ADDR_VAL(src_buf_addr) |
> VE_H264_VLD_ADDR_FIRST | VE_H264_VLD_ADDR_VALID |
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index 4f05ee265997..f74736fcfa00 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -158,9 +158,6 @@ struct v4l2_h264_reference {
> };
>
> struct v4l2_ctrl_h264_slice_params {
> - /* Size in bytes, including header */
> - __u32 size;
> -
> /* Offset in bytes to the start of slice in the OUTPUT buffer. */
> __u32 start_byte_offset;
>
> --
> 2.27.0
>

--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature