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

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 4818 bytes --]

Hi,

On Fri 07 Aug 20, 11:44, Ezequiel Garcia wrote:
> On Thu, 2020-08-06 at 17:50 +0200, Paul Kocialkowski wrote:
> > 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.
> > 
> 
> If we later extend the API, another control (possibly
> another decoding mode?) shall be introduced.
> 
> This API covers single-slice-per-request as specified
> and documented in patch 9/14 "Clarify SLICE_BASED mode".
> 
> This is along the lines of the proposal drafted by Nicolas,
> see my reply: https://lkml.org/lkml/2020/8/5/791.
> 
> This applies to num_slices, slice size and slice start offset.
> 
> There are multiple ways of doing this.

If feels a bit problematic to remove these fields without a clear plan yet
on how to support multiple slices in the future. These may need to be added
again later, except that it will be too late and new controls will need to be
introduced.

Also, could we consider adding more reserved fields to handle such future needs?

Cheers,

Paul

> Thanks!
> Ezequiel
> 
> > 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@collabora.com>
> > > ---
> > >  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]