lkml.org 
[lkml]   [2020]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation
    From
    Date
    On Fri, 2020-07-03 at 19:17 +0000, Jonas Karlman wrote:
    > On 2020-07-03 16:58, Ezequiel Garcia wrote:
    > > On Fri, 2020-07-03 at 06:55 +0000, Jonas Karlman wrote:
    > > > On 2020-07-03 05:14, Ezequiel Garcia wrote:
    > > > > Hi Jonas,
    > > > >
    > > > > Thanks for working on this.
    > > > >
    > > > > On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
    > > > > > Add an optional validate_fmt operation that is used to validate the
    > > > > > pixelformat of CAPTURE buffers.
    > > > > >
    > > > > > This is used in next patch to ensure correct pixelformat is used for 10-bit
    > > > > > and 4:2:2 content.
    > > > > >
    > > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
    > > > > > ---
    > > > > > drivers/staging/media/rkvdec/rkvdec.c | 8 ++++++++
    > > > > > drivers/staging/media/rkvdec/rkvdec.h | 1 +
    > > > > > 2 files changed, 9 insertions(+)
    > > > > >
    > > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
    > > > > > index b1de55aa6535..465444c58f13 100644
    > > > > > --- a/drivers/staging/media/rkvdec/rkvdec.c
    > > > > > +++ b/drivers/staging/media/rkvdec/rkvdec.c
    > > > > > @@ -239,6 +239,14 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
    > > > > > if (WARN_ON(!coded_desc))
    > > > > > return -EINVAL;
    > > > > >
    > > > > > + if (coded_desc->ops->validate_fmt) {
    > > > > > + int ret;
    > > > > > +
    > > > > > + ret = coded_desc->ops->validate_fmt(ctx, pix_mp->pixelformat);
    > > > > > + if (ret)
    > > > > > + return ret;
    > > > > > + }
    > > > > > +
    > > > >
    > > > > I don't think this approach will be enough. Unless I'm mistaken,
    > > > > it's perfectly legal as per the stateless spec to first
    > > > > call S_FMT on the OUTPUT queue (which is propagated to the CAPTURE side),
    > > > > and then set the SPS and other controls.
    > > >
    > > > I agree that this will not be enough to cover all use cases stated in the spec.
    > > >
    > > > > The application is not required to do a TRY_FMT after S_EXT_CTRLS.
    > > >
    > > > If I remember correctly we were required to implement a TRY_FMT loop in
    > > > ffmpeg due to cedrus defaulting to SUNXI_TILED_NV12 instead of linear NV12
    > > > on platforms where display controller did not support the tiled modifier.
    > > >
    > > > So having TRY_FMT as part of the init sequence has been my only test-case.
    > > >
    > > > > What I believe is needed is for the S_EXT_CTRLS to modify
    > > > > and restrict the CAPTURE format accordingly, so the application
    > > > > gets the correct format on G_FMT (and restrict future TRY_FMT).
    > > >
    > > > This sounds like a proper solution, I do belive we may have a chicken or
    > > > the egg problem depending on if application call S_EXT_CTRLS or S_FMT first.
    > > >
    > >
    > > IIUC, the order is specified in the stateless spec [1].
    > >
    > > 1) S_FMT on OUTPUT (to set the coded pixelformat). CAPTURE format
    > > format is propagated here and a default format is set.
    > >
    > > 2) S_EXT_CTRLS, parameters are set. We don't do anything here,
    > > but here we'd validate the SPS and restrict the CAPTURE pixelformat
    > > (and perhaps reset the default CAPTURE pixelformat).
    > >
    > > 3) G_FMT on CAPTURE.
    > >
    > > 4) (optional) ENUM_FMT / S_FMT on CAPTURE, to negotiate
    > > something different from default.
    >
    > There is also the following scenario that we may need to support:
    >
    > 1) S_FMT on OUTPUT, default CAPTURE format is set.
    >
    > 2) skip S_EXT_CTRLS, mandatory controls is only validated in req_validate.
    >
    > 3) G_FMT on CAPTURE, returns default CAPTURE format.
    >
    > 4) S_FMT on CAPTURE, CAPTURE format is changed from default to selected format.
    >
    > 5) STREAMON
    >
    > From this point on I would expect S_EXT_CTRLS with a V4L2_CTRL_WHICH_REQUEST_VAL
    > flag to reject any SPS not matching the selected CAPTURE format. Effectively
    > allowing S_FMT to lock down a format instead of an initial S_EXT_CTRLS during init.
    >
    > This means that we have to both allow and reject a SPS depending on the state.
    >

    Isn't it cleaner from an API to require the SPS at (2), right before
    G_FMT on CAPTURE?

    Unless you think it has clear advantages to provide more flexibility
    to the user, i.e. it will allow to support specific use-cases,
    then I would avoid making this flexible.

    The spec currently doesn mention step 2 as being optional, although
    there is a note mentioning controls can be overwritten.

    Thanks,
    Ezequiel

    > Regards,
    > Jonas
    >
    > > Regards,
    > > Ezequiel
    > >
    > > [1] Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
    > >
    > > > I guess we may need to lock down on a format at whatever comes first,
    > > > S_FMT on CAPTURE or S_EXT_CTRLS with SPS ctrl.
    > > >
    > > > I have an idea on how this could be addressed, will explore and see
    > > > if I can come up with something new.
    > > >
    > > > Regards,
    > > > Jonas
    > > >
    > > > > Also, V4L2 spec asks drivers not to fail on S_FMT
    > > > > format mismatch, but instead to adjust and return a legal format
    > > > > back to the application [1].
    > > > >
    > > > > Let me know what you think and thanks again.
    > > > >
    > > > > Ezequiel
    > > > >
    > > > > [1] Documentation/userspace-api/media/v4l/vidioc-g-fmt.rst
    > > > >
    > > > > > for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
    > > > > > if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
    > > > > > break;
    > > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
    > > > > > index 2fc9f46b6910..be4fc3645cde 100644
    > > > > > --- a/drivers/staging/media/rkvdec/rkvdec.h
    > > > > > +++ b/drivers/staging/media/rkvdec/rkvdec.h
    > > > > > @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
    > > > > > struct rkvdec_coded_fmt_ops {
    > > > > > int (*adjust_fmt)(struct rkvdec_ctx *ctx,
    > > > > > struct v4l2_format *f);
    > > > > > + int (*validate_fmt)(struct rkvdec_ctx *ctx, u32 pixelformat);
    > > > > > int (*start)(struct rkvdec_ctx *ctx);
    > > > > > void (*stop)(struct rkvdec_ctx *ctx);
    > > > > > int (*run)(struct rkvdec_ctx *ctx);


    \
     
     \ /
      Last update: 2020-07-03 21:33    [W:2.800 / U:0.308 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site