Re: [PATCH v2 11/12] staging: media: rkvdec: Enable S_CTRL IOCTL

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Le jeudi 12 janvier 2023 à 12:09 -0300, Ezequiel Garcia a écrit :
> Hi Sebastian,
> 
> On Thu, Jan 12, 2023 at 9:57 AM Sebastian Fricke
> <sebastian.fricke@collabora.com> wrote:
> > 
> > Enable user-space to set the SPS of the current byte-stream on the
> > decoder. This action will enable the decoder to pick the optimal
> > pixel-format for the capture queue, whenever it is required.
> > 
> > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec.c | 81 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > index b303c6e0286d..3d413c5ad1d2 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > @@ -93,6 +93,79 @@ static int rkvdec_get_sps_attributes(struct rkvdec_ctx *ctx, void *sps,
> >         return 0;
> >  }
> > 
> > +static int rkvdec_set_sps(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
> > +{
> > +       struct v4l2_pix_format_mplane *pix_mp;
> > +       struct sps_attributes attributes = {0};
> > +       void *new_sps = NULL;
> > +
> > +       /*
> > +        * SPS structures are not filled until the control handler is set up
> > +        */
> > +       if (!ctx->fh.ctrl_handler)
> > +               return 0;
> 
> The control handler is embedded in the context, and the fh.ctrl_handler
> is initialized when the context is returned.
> 
> You cannot have a context without a control handler (see hantro_open).
> 
> > +
> > +       switch (ctrl->id) {
> > +       case V4L2_CID_STATELESS_H264_SPS:
> > +               new_sps = (void *)ctrl->p_new.p_h264_sps;
> > +               break;
> > +       case V4L2_CID_STATELESS_HEVC_SPS:
> > +               new_sps = (void *)ctrl->p_new.p_hevc_sps;
> > +               break;
> > +       default:
> > +               dev_err(ctx->dev->dev, "Unsupported stateless control ID: %x\n", ctrl->id);
> > +               return -EINVAL;
> > +       };
> > +       rkvdec_get_sps_attributes(ctx, new_sps, &attributes);
> > +
> > +       /*
> > +        * Providing an empty SPS is valid but we do not store it.
> > +        */
> > +       if (attributes.width == 0 && attributes.height == 0)
> > +               return 0;
> > +
> > +       pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> > +
> > +       /*
> > +        * SPS must match the provided format dimension, if it doesn't userspace has to
> > +        * first reset the output format
> 
> This comment says it's a mismatch check, but the check is checking for
> "larger than".
> 
> Other than that, the general idea looks good, can you rework the series to avoid
> the extra storage of the SPS control in the context?

I'm not fan, but the careless alignment code states that the coded size cannot
be lower then 64x64, but while running the conformance, there is bunch of files
that have coded dimensions lower. And here's the reason why its not ==. I don't
really like this, but confusing allocation padding and coded size capability
seems to be deep down into rkvdec driver design.

If I only look at the FMT(OUTPUT) behaviour, anything < 64x64 is strictly not
supported by the driver, this the interpretation ffmpeg request code have, and
they are actually right. GStreamer simply does not check anything, and try
(there is not validation code there yet, and I don't want to add any until we
figure-out a solution).

> 
> Thanks,
> Ezequiel
> 
> > +        */
> > +       if ((attributes.width > pix_mp->width) || (attributes.height > pix_mp->height)) {
> > +               dev_err(ctx->dev->dev,
> > +                       "Dimension mismatch. [%s SPS] W: %d, H: %d, [Format] W: %d, H: %d)\n",
> > +                       ctrl->id == V4L2_CID_STATELESS_HEVC_SPS ? "HEVC" : "H264",
> > +                       attributes.width, attributes.height, pix_mp->width, pix_mp->height);
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (ctx->sps && pix_mp->pixelformat == rkvdec_get_valid_fmt(ctx)) {
> > +               /*
> > +                * Userspace is allowed to change the SPS at any point, if the
> > +                * pixel format doesn't differ from the format in the context,
> > +                * just accept the change even if buffers are queued
> > +                */
> > +               ctx->sps = new_sps;
> > +       } else {
> > +               /*
> > +                * Do not accept changing the SPS, while buffers are queued,
> > +                * when the new SPS would cause switching the CAPTURE pixel format
> > +                */
> > +               if (pix_mp->pixelformat != rkvdec_get_valid_fmt(ctx)) {
> > +                       if (rkvdec_queue_busy(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE))
> > +                               return -EBUSY;
> > +               }
> > +               ctx->sps = new_sps;
> > +               /*
> > +                * For the initial SPS setting and when the pixel format is
> > +                * changed adjust the pixel format stored in the context
> > +                */
> > +               pix_mp->pixelformat = rkvdec_get_valid_fmt(ctx);
> > +               rkvdec_fill_decoded_pixfmt(ctx, pix_mp);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >         struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > @@ -104,8 +177,16 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> >         return 0;
> >  }
> > 
> > +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +       struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > +
> > +       return rkvdec_set_sps(ctx, ctrl);
> > +}
> > +
> >  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
> >         .try_ctrl = rkvdec_try_ctrl,
> > +       .s_ctrl = rkvdec_s_ctrl,
> >  };
> > 
> >  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
> > 
> > --
> > 2.25.1