[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cc53dbbf-405b-452b-b007-00588d6c8839@kwiboo.se>
Date: Mon, 7 Apr 2025 17:07:43 +0200
From: Jonas Karlman <jonas@...boo.se>
To: Nicolas Dufresne <nicolas.dufresne@...labora.com>,
Hans Verkuil <hverkuil@...all.nl>
Cc: Sebastian Fricke <sebastian.fricke@...labora.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Boris Brezillon <boris.brezillon@...labora.com>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-rockchip@...ts.infradead.org, linux-staging@...ts.linux.dev,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
Alex Bee <knaerzche@...il.com>,
Benjamin Gaignard <benjamin.gaignard@...labora.com>,
Detlev Casanova <detlev.casanova@...labora.com>,
Dan Carpenter <dan.carpenter@...aro.org>,
Christopher Obbard <christopher.obbard@...aro.org>
Subject: Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops
On 2025-04-07 16:59, Nicolas Dufresne wrote:
> Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
>> On 07/04/2025 15:52, Nicolas Dufresne wrote:
>>> Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
>>>> On 25/02/2025 10:40, Sebastian Fricke wrote:
>>>>> From: Jonas Karlman <jonas@...boo.se>
>>>>>
>>>>> Add support for a get_image_fmt() ops that returns the required image
>>>>> format.
>>>>>
>>>>> The CAPTURE format is reset when the required image format changes and
>>>>> the buffer queue is not busy.
>>>>>
>>>>> Signed-off-by: Jonas Karlman <jonas@...boo.se>
>>>>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
>>>>> Tested-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
>>>>> Tested-by: Christopher Obbard <chris.obbard@...labora.com>
>>>>> ---
>>>>> drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
>>>>> drivers/staging/media/rkvdec/rkvdec.h | 2 ++
>>>>> 2 files changed, 49 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>>>> index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>>>> @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>>>> {
>>>>> struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>> const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>> + struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>> + enum rkvdec_image_fmt image_fmt;
>>>>> + struct vb2_queue *vq;
>>>>> + int ret;
>>>>> +
>>>>> + if (desc->ops->try_ctrl) {
>>>>> + ret = desc->ops->try_ctrl(ctx, ctrl);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + if (!desc->ops->get_image_fmt)
>>>>> + return 0;
>>>>>
>>>>> - if (desc->ops->try_ctrl)
>>>>> - return desc->ops->try_ctrl(ctx, ctrl);
>>>>> + image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>> + if (ctx->image_fmt == image_fmt)
>>>>> + return 0;
>>>>> +
>>>>> + if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
>>>>> + return 0;
>>>>> +
>>>>> + /* format change not allowed when queue is busy */
>>>>> + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>>>>> + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>>>> + if (vb2_is_busy(vq))
>>>>> + return -EINVAL;
>>
>> Looking closer, this code is just wrong. It does these format change
>> tests for any control, so if more controls are added in the future, then
>> those will be checked the same way, which makes no sense.
>
> "Just wrong" should be kept for code that is semantically incorrect,
> just a suggestion for choice of wording.
>
>>
>> These tests belong to the actual control that you 'try'. In this case
>> rkvdec_h264_validate_sps(). This function already checks the width and
>> height, but it should also check the image format. It is all in the
>> wrong place.
Keep in mind that rkvdec_try_ctrl and rkvdec_s_ctrl are only used for
CID_STATELESS_H264_SPS (and in future also CID_STATELESS_HEVC_SPS) not
any other control, so this is already in the correct place?
Maybe the naming of the functions are too generic, they could be named
rkvdec_sps_try_ctrl and rkvdec_sps_s_ctrl or similar if that makes more
sense?
Regards,
Jonas
>
> We can do that too. Though, this was generalized since once you enable
> the other codecs, you endup with code duplication. I know this series
> is an extract from a larger one.
>
> So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
> and the busy check. Though on that, please reply to my comment below
> (which you skipped).
>
>>
>>>>
>>>> This makes no sense to me. This just tries a control, and that should just
>>>> work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
>>>> changing anything.
>>>
>>> See comment below, notice that this code is only reached if the control
>>> introduce parameters that are not compatible with the current capture
>>> queue fmt. The entire function uses "success" early exit, so the
>>> further down you get in the function, the less likely your control is
>>> valid.
>>>
>>>>
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>> +{
>>
>> If there is a try_ctrl op specified, then the control framework
>> will call that first before calling s_ctrl. So any validation that
>> try_ctrl did does not need to be done again in s_ctrl.
>>
>> The same comment with try_ctrl is valid here as well: if there are
>> image format checks that need to be done, then those need to be done
>> per control and not as a generic check. If new controls are added in
>> the future, then you don't want the same checks to apply to the new
>> controls as well.
>
> I don't think the behaviour of try_ctrl and that being embedded in set
> calls was being questioned by anyone. Can you reply to the last
> paragraph below ?
>
>>
>> Regards,
>>
>> Hans
>>
>>>>> + struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>> + const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>> + struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>> + enum rkvdec_image_fmt image_fmt;
>>>>> +
>>>>> + if (!desc->ops->get_image_fmt)
>>>>> + return 0;
>>>>> +
>>>>> + image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>> + if (ctx->image_fmt == image_fmt)
>>>>> + return 0;
>>>>
>>>> If you really can't set a control when the queue is busy, then that should
>>>> be tested here, not in try_ctrl. And then you return -EBUSY.
>>>>
>>>> Am I missing something here?
>>>
>>> When I reviewed, I had imagine that s_ctrl on a request would just run
>>> a try. Now that I read that more careful, I see that it does a true set
>>> on separate copy. So yes, this can safely be moved here.
>>>
>>> Since you seem wondering "If you really can't set a control", let me
>>> explain what Jonas wants to protect against. RKVdec does not have any
>>> color conversion code, the header compound control (which header
>>> depends on the codec), contains details such as sub-sampling and color
>>> depth. Without color conversion, when the image format is locked (the
>>> busy queue), you can't request the HW to decode a frame witch does not
>>> fit. This could otherwise lead to buffer overflow in the HW,
>>> fortunately protected by the iommu, but you don't really want to depend
>>> on the mmu.
>>>
>>> I've never used try_ctrl in my decade of v4l2, so obviously, now that I
>>> know that s_ctrl on request is not a try, I'm fine with rejecting this
>>> PR, sending a new version and making a PR again. But if I was to use
>>> this API in userspace, my intuitive expectation would be that this
>>> should fail try(), even if its very rarely valid to check the queue
>>> state in try control.
>
> Here, since we seem to disagree on the behaviour try should have for
> this specific validation. What you asked on first pass is to make it so
> that TRY will succeed, and SET will fail. I don't really like that
> suggestion.
>
> Nicolas
>
>>>
>>> Nicolas
>>>
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>> +
>>>>> + ctx->image_fmt = image_fmt;
>>>>> + if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
>>>>> + rkvdec_reset_decoded_fmt(ctx);
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> 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[] = {
>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>>>> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>>>> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>>>>> struct vb2_v4l2_buffer *dst_buf,
>>>>> enum vb2_buffer_state result);
>>>>> int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>>>> + enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
>>>>> + struct v4l2_ctrl *ctrl);
>>>>> };
>>>>>
>>>>> enum rkvdec_image_fmt {
>>>>>
>>>
>
Powered by blists - more mailing lists