On 6/30/22 07:02, Sebastian Fricke wrote:
> Hey Ezequiel,
>
> On 29.06.2022 16:56, Ezequiel Garcia wrote:
>> Currently, the driver tries to validat the HEVC SPS
>
> s/validat/validate/
>
>> 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.
>>
>> In addition to this, a format validation in hantro_hevc_dec_prepare_run()
>> is also suboptimal, because hantro_hevc_dec_prepare_run() runs in the context
>> of v4l2_m2m_ops.device_run, as part of a decoding job.
>>
>> Format and control validations should happen before decoding starts,
>> in the context of ioctls such as S_CTRL, S_FMT, or STREAMON.
>>
>> Remove the validation for now.
>
> Couldn't we add a small wrapper around STREAMON to perform that
> validation? I feel like "remove the validation for now", seems like a
> vague statement.
I agree. Basically two things happen in this patch: two sanity checks
for the SPS control are moved to try_ctrl, and that part looks good.
So that can be a separate patch.
The second part is the removal of the format+control validation, but it
is not clear why removing it altogether is wrong. Shouldn't it still be
done somewhere? And if not, why not?
Regards,
Hans
>
> Greetings,
> Sebastian
>
>>
>> Fixes: 135ad96cb4d6b ("media: hantro: Be more accurate on pixel formats step_width constraints")
>> Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/staging/media/hantro/hantro_drv.c | 12 ++++-----
>> drivers/staging/media/hantro/hantro_hevc.c | 30 ----------------------
>> drivers/staging/media/hantro/hantro_hw.h | 1 -
>> 3 files changed, 6 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
>> index afddf7ac0731..2387ca85ab54 100644
>> --- a/drivers/staging/media/hantro/hantro_drv.c
>> +++ b/drivers/staging/media/hantro/hantro_drv.c
>> @@ -253,11 +253,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;
>>
>> @@ -273,7 +268,12 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
>> } else if (ctrl->id == V4L2_CID_MPEG_VIDEO_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/staging/media/hantro/hantro_hevc.c b/drivers/staging/media/hantro/hantro_hevc.c
>> index bd924896e409..f86c98e19177 100644
>> --- a/drivers/staging/media/hantro/hantro_hevc.c
>> +++ b/drivers/staging/media/hantro/hantro_hevc.c
>> @@ -154,32 +154,6 @@ 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)
>> -{
>> - 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
>> - */
>> - if (ctx->vpu_dst_fmt->fourcc == V4L2_PIX_FMT_NV12_4L4) {
>> - if (ctx->dst_fmt.width !=
>> - ALIGN(sps->pic_width_in_luma_samples, ctx->vpu_dst_fmt->frmsize.step_width))
>> - return -EINVAL;
>> -
>> - if (ctx->dst_fmt.height !=
>> - ALIGN(sps->pic_height_in_luma_samples, ctx->vpu_dst_fmt->frmsize.step_height))
>> - return -EINVAL;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx)
>> {
>> struct hantro_hevc_dec_hw_ctx *hevc_ctx = &ctx->hevc_dec;
>> @@ -203,10 +177,6 @@ int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx)
>> if (WARN_ON(!ctrls->sps))
>> return -EINVAL;
>>
>> - ret = hantro_hevc_validate_sps(ctx, ctrls->sps);
>> - if (ret)
>> - return ret;
>> -
>> ctrls->pps =
>> hantro_get_ctrl(ctx, V4L2_CID_MPEG_VIDEO_HEVC_PPS);
>> if (WARN_ON(!ctrls->pps))
>> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
>> index a2e0f0836281..5edff0f0be20 100644
>> --- a/drivers/staging/media/hantro/hantro_hw.h
>> +++ b/drivers/staging/media/hantro/hantro_hw.h
>> @@ -359,7 +359,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, int 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.31.1
>>
