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

[Paul Kocialkowski]

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