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;
Date:   Mon, 18 Jul 2022 18:48:08 -0300
From:   Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
To:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Nicolas Dufresne <nicolas.dufresne@collabora.com>,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Tomasz Figa <tfiga@chromium.org>,
        Chris Healy <cphealy@gmail.com>
Subject: Re: [PATCH] hantro: Remove incorrect HEVC SPS validation
Message-ID: <YtXVGBRUDy5K9+WK@eze-laptop>
References: <20220718214123.73275-1-ezequiel@vanguardiasur.com.ar>
 <20220718214123.73275-3-ezequiel@vanguardiasur.com.ar>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220718214123.73275-3-ezequiel@vanguardiasur.com.ar>
Precedence: bulk

On Mon, Jul 18, 2022 at 06:41:23PM -0300, Ezequiel Garcia wrote:
> Currently, the driver tries to validat the HEVC SPS
> against the CAPTURE queue format (i.e. the decoded format).
> This is not correct, because typically the SPS control is set
> before the CAPTURE queue is negotiated.
> 
> Fixes: 135ad96cb4d6b ("media: hantro: Be more accurate on pixel formats step_width constraints")
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

This is actually a v2. I've chosen to move the
control checks back to try_ctrl, but leave the tiled
format alignment check.

This ALIGN() check can be optimized, moving the alignment
to the TRY_FMT step (instead of failing at device_run).
However, this is a minor optimization, so I'd like to
avoid the scope creep, and pospone the changes regarding
the tiled format resolution alignment.

Thanks,
Ezequiel

> ---
>  drivers/media/platform/verisilicon/hantro_drv.c  | 12 ++++++------
>  drivers/media/platform/verisilicon/hantro_hevc.c |  9 +--------
>  drivers/media/platform/verisilicon/hantro_hw.h   |  1 -
>  3 files changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index e5fc0a99b728..2036f72eeb4a 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -251,11 +251,6 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>  
>  static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
>  {
> -	struct hantro_ctx *ctx;
> -
> -	ctx = container_of(ctrl->handler,
> -			   struct hantro_ctx, ctrl_handler);
> -
>  	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
>  		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
>  
> @@ -271,7 +266,12 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
>  	} else if (ctrl->id == V4L2_CID_STATELESS_HEVC_SPS) {
>  		const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
>  
> -		return hantro_hevc_validate_sps(ctx, sps);
> +		if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
> +			/* Luma and chroma bit depth mismatch */
> +			return -EINVAL;
> +		if (sps->bit_depth_luma_minus8 != 0)
> +			/* Only 8-bit is supported */
> +			return -EINVAL;
>  	} else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) {
>  		const struct v4l2_ctrl_vp9_frame *dec_params = ctrl->p_new.p_vp9_frame;
>  
> diff --git a/drivers/media/platform/verisilicon/hantro_hevc.c b/drivers/media/platform/verisilicon/hantro_hevc.c
> index 5984c5fa6f83..b990bc98164c 100644
> --- a/drivers/media/platform/verisilicon/hantro_hevc.c
> +++ b/drivers/media/platform/verisilicon/hantro_hevc.c
> @@ -154,15 +154,8 @@ static int tile_buffer_reallocate(struct hantro_ctx *ctx)
>  	return -ENOMEM;
>  }
>  
> -int hantro_hevc_validate_sps(struct hantro_ctx *ctx, const struct v4l2_ctrl_hevc_sps *sps)
> +static int hantro_hevc_validate_sps(struct hantro_ctx *ctx, const struct v4l2_ctrl_hevc_sps *sps)
>  {
> -	if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
> -		/* Luma and chroma bit depth mismatch */
> -		return -EINVAL;
> -	if (sps->bit_depth_luma_minus8 != 0)
> -		/* Only 8-bit is supported */
> -		return -EINVAL;
> -
>  	/*
>  	 * for tile pixel format check if the width and height match
>  	 * hardware constraints
> diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h
> index 762631d15fdc..e83f0c523a30 100644
> --- a/drivers/media/platform/verisilicon/hantro_hw.h
> +++ b/drivers/media/platform/verisilicon/hantro_hw.h
> @@ -360,7 +360,6 @@ int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx);
>  void hantro_hevc_ref_init(struct hantro_ctx *ctx);
>  dma_addr_t hantro_hevc_get_ref_buf(struct hantro_ctx *ctx, s32 poc);
>  int hantro_hevc_add_ref_buf(struct hantro_ctx *ctx, int poc, dma_addr_t addr);
> -int hantro_hevc_validate_sps(struct hantro_ctx *ctx, const struct v4l2_ctrl_hevc_sps *sps);
>  
>  
>  static inline unsigned short hantro_vp9_num_sbs(unsigned short dimension)
> -- 
> 2.34.3
>