| Message ID | 20200701215616.30874-9-jonas@kwiboo.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: rkvdec: Add H.264 High 10 and 4:2:2 profile support | expand |
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. The application is not required to do a TRY_FMT after S_EXT_CTRLS. 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). 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);
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. 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); > >
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. 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);
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. 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); > >
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);
On Fri, Jul 3, 2020 at 9:18 PM Jonas Karlman <jonas@kwiboo.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@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. > 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); > > > >
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; + } + 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);
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(+)