Date:   Fri, 3 Jul 2020 21:34:24 +0200
From:   Tomasz Figa <tfiga@...omium.org>
To:     Jonas Karlman <jonas@...boo.se>
Cc:     Ezequiel Garcia <ezequiel@...labora.com>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        Alexandre Courbot <acourbot@...omium.org>
Subject: Re: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation

On Fri, Jul 3, 2020 at 9:18 PM Jonas Karlman <jonas@...boo.se> 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@...boo.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.
>

That would end up with a really bad API behavior inconsistency. Rather
than that, we have to define what is the authoritative source of the
width and height and I believe that for streams that have this kind of
information in the metadata (e.g. header controls), that should be the
metadata.

So, for example, for H.264, the driver would have to always keep the
width and height of the OUTPUT format fixed to whatever the SPS
control specifies, regardless of what one attempts to set via S_FMT.
If an SPS is set that has different width and height values, the
OUTPUT format should be updated by the driver, in turn the CAPTURE
format should be reset as well and then a SOURCE_CHANGE event should
be signaled, like with the stateful decoders.

Best regards,
Tomasz

> 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);
> >
> >

Powered by blists - more mailing lists