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

[Ezequiel Garcia]

Hi Sebastian,

On Thu, Jan 12, 2023 at 9:57 AM Sebastian Fricke
<sebastian.fricke@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> Signed-off-by: Jonas Karlman <jonas@xxxxxxxxx>
> ---
>  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?

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