Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230101-patch-series-v2-6-2-rc1-v2-0-fa1897efac14@collabora.com> <20230101-patch-series-v2-6-2-rc1-v2-9-fa1897efac14@collabora.com>
In-Reply-To: <20230101-patch-series-v2-6-2-rc1-v2-9-fa1897efac14@collabora.com>
From:   Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Date:   Thu, 12 Jan 2023 12:21:26 -0300
Message-ID: <CAAEAJfDkTX=EwDCs+uN0bFwMb_JhJfkQiwRR9+b-9v3cJnPTsw@mail.gmail.com>
Subject: Re: [PATCH v2 09/12] staging: media: rkvdec: h264: Add callbacks for h264
To:     Sebastian Fricke <sebastian.fricke@collabora.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev,
        Jonas Karlman <jonas@kwiboo.se>,
        Alex Bee <knaerzche@gmail.com>,
        Nicolas Dufresne <nicolas.dufresne@collabora.com>,
        Collabora Kernel-domain <kernel@collabora.com>,
        Robert Beckett <bob.beckett@collabora.com>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 12, 2023 at 9:56 AM Sebastian Fricke
<sebastian.fricke@collabora.com> wrote:
>
> Implement the valid format and sps validation callbacks for H264.
> H264 already has a SPS validation function, adjust it to fit the
> function the declaration and add error messages.
> Additionally, add a function to fetch attributes from the SPS in a human
> readable format to make the code more self documenting.
>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 105 ++++++++++++++++++++++-------
>  1 file changed, 80 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 4fc167b42cf0..17b215874ddd 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1026,40 +1026,80 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
>         return 0;
>  }
>
> -static int rkvdec_h264_validate_sps(struct rkvdec_ctx *ctx,
> -                                   const struct v4l2_ctrl_h264_sps *sps)
> +/*
> + * Convert some fields from the SPS structure into human readable attributes.
> + */
> +static int rkvdec_h264_get_sps_attributes(struct rkvdec_ctx *ctx, void *sps,
> +                                         struct sps_attributes *attributes)
> +{
> +       struct v4l2_ctrl_h264_sps *h264_sps = sps;
> +       unsigned int width = (h264_sps->pic_width_in_mbs_minus1 + 1) * 16;
> +       unsigned int height = (h264_sps->pic_height_in_map_units_minus1 + 1) * 16;
> +       /*
> +        * When frame_mbs_only_flag is not set, this is field height,
> +        * which is half the final height (see (7-18) in the
> +        * specification)
> +        */
> +       if (!(h264_sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY))
> +               height *= 2;
> +
> +       attributes->width = width;
> +       attributes->height = height;
> +       attributes->luma_bitdepth = h264_sps->bit_depth_luma_minus8 + 8;
> +       attributes->chroma_bitdepth = h264_sps->bit_depth_chroma_minus8 + 8;
> +       switch (h264_sps->chroma_format_idc) {
> +       case 0:
> +               attributes->subsampling = 400;
> +               break;
> +       case 1:
> +               attributes->subsampling = 420;
> +               break;
> +       case 2:
> +               attributes->subsampling = 422;
> +               break;
> +       case 3:
> +               attributes->subsampling = 444;
> +               break;
> +       };
> +       return 0;
> +}
> +
> +static int rkvdec_h264_validate_sps(struct rkvdec_ctx *ctx, void *sps,
> +                                   struct v4l2_pix_format_mplane *pix_mp)
>  {
> -       unsigned int width, height;
> +       struct sps_attributes attributes = {0};
> +
> +       rkvdec_h264_get_sps_attributes(ctx, sps, &attributes);
>
>         /*
>          * TODO: The hardware supports 10-bit and 4:2:2 profiles,
>          * but it's currently broken in the driver.
>          * Reject them for now, until it's fixed.
>          */
> -       if (sps->chroma_format_idc > 1)
> -               /* Only 4:0:0 and 4:2:0 are supported */
> +       if (attributes.subsampling > 420) {
> +               dev_err(ctx->dev->dev,
> +                       "Only 4:0:0 and 4:2:0 subsampling schemes are supported, got: %d\n",
> +                       attributes.subsampling);
>                 return -EINVAL;
> -       if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
> -               /* Luma and chroma bit depth mismatch */
> +       }
> +       if (attributes.luma_bitdepth != attributes.chroma_bitdepth) {
> +               dev_err(ctx->dev->dev,
> +                       "Luma and chroma bit depth mismatch, luma %d, chroma %d\n",
> +                       attributes.luma_bitdepth, attributes.chroma_bitdepth);
>                 return -EINVAL;
> -       if (sps->bit_depth_luma_minus8 != 0)
> -               /* Only 8-bit is supported */
> +       }
> +       if (attributes.luma_bitdepth != 8) {
> +               dev_err(ctx->dev->dev, "Only 8-bit H264 formats supported, SPS %d\n",
> +                       attributes.luma_bitdepth);
>                 return -EINVAL;
> +       }
>
> -       width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
> -       height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
> -
> -       /*
> -        * When frame_mbs_only_flag is not set, this is field height,
> -        * which is half the final height (see (7-18) in the
> -        * specification)
> -        */
> -       if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY))
> -               height *= 2;
> -
> -       if (width > ctx->coded_fmt.fmt.pix_mp.width ||
> -           height > ctx->coded_fmt.fmt.pix_mp.height)
> +       if (attributes.width > pix_mp->width || attributes.height > pix_mp->height) {
> +               dev_err(ctx->dev->dev,
> +                       "Incompatible SPS dimension, SPS %dx%d, Pixel format %dx%d.",
> +                       attributes.width, attributes.height, pix_mp->width, pix_mp->height);
>                 return -EINVAL;
> +       }
>
>         return 0;
>  }
> @@ -1077,8 +1117,9 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>         if (!ctrl)
>                 return -EINVAL;
>
> -       ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
> -       if (ret)
> +       ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps,
> +                                      &ctx->coded_fmt.fmt.pix_mp);

Not a problem with this patch, but I wonder why we accepted a validation
in the start_streaming step.

At this point, the driver accepted all the format negotiations in try_fmt.
It's difficult for applications to recover from this, as there would
be no way to tell what failed,
we would be returning EINVAL, which as per the spec is "buffer type is
not supported,
or no buffers have been allocated (memory mapping) or enqueued (output) yet.".

I think it would really make a lot of sense to fix this now, instead of continue
abusing it. And also, I'd like to prevent a possible anti-pattern from
spreading.

Thanks!
Ezequiel

> +       if (ret < 0)
>                 return ret;
>
>         h264_ctx = kzalloc(sizeof(*h264_ctx), GFP_KERNEL);
> @@ -1175,10 +1216,21 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>         return 0;
>  }
>
> +static u32 rkvdec_h264_valid_fmt(struct rkvdec_ctx *ctx)
> +{
> +       /*
> +        * Only 8 bit 4:0:0 and 4:2:0 formats supported for now.
> +        * The SPS is validated at a different function thus we can assume that
> +        * it is correct.
> +        */
> +       return V4L2_PIX_FMT_NV12;
> +}
> +
>  static int rkvdec_h264_try_ctrl(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
>  {
>         if (ctrl->id == V4L2_CID_STATELESS_H264_SPS)
> -               return rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
> +               return rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps,
> +                                               &ctx->coded_fmt.fmt.pix_mp);
>
>         return 0;
>  }
> @@ -1189,4 +1241,7 @@ const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
>         .stop = rkvdec_h264_stop,
>         .run = rkvdec_h264_run,
>         .try_ctrl = rkvdec_h264_try_ctrl,
> +       .valid_fmt = rkvdec_h264_valid_fmt,
> +       .sps_check = rkvdec_h264_validate_sps,
> +       .get_sps_attributes = rkvdec_h264_get_sps_attributes,
>  };
>
> --
> 2.25.1