Message ID | 20240119094944.26763-9-benjamin.gaignard@collabora.com (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from ny.mirrors.kernel.org ([147.75.199.223]) by www.linuxtv.org with esmtp (Exim 4.96) (envelope-from <linux-media+bounces-3920-patchwork=linuxtv.org@vger.kernel.org>) id 1rQlXc-0003Sh-00 for patchwork@linuxtv.org; Fri, 19 Jan 2024 09:52:00 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id 0157F1C232D7 for <patchwork@linuxtv.org>; Fri, 19 Jan 2024 09:51:59 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id A68DC3D554; Fri, 19 Jan 2024 09:49:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="pg9chlwj" X-Original-To: linux-media@vger.kernel.org Received: from madrid.collaboradmins.com (madrid.collaboradmins.com [46.235.227.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CAD4D3D0B5; Fri, 19 Jan 2024 09:49:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=46.235.227.194 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705657798; cv=none; b=bZ+nZBx1cvV1Jb0sUq1XZrjgVVHquH4rsKsDFbMIx6+LYrIP3+oyaHgI+Z4juD9jwNkiwbNPmmnqZy8dRdKtl/NlN0SEV+e4FJMuBzbQtW1Mptq1OXrlCVL9zTDZDHTz2BjHlkp4zOWzBavaaO3HDDB3zcwueAmqQ0D5GgoExDw= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705657798; c=relaxed/simple; bh=fLBUyGpx4ptCR4u0+nI0SI/vY0QxIITEt56gQ9tS950=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=p5vnk9iYPVR4Cv2trjv4iCN3F/u2paNhvL4EpGFjM0R4CdcHho0ZrdrkusY4uNiGw4j6N5q3u19C1az+YOD3dN+xClUaFuzO2oIAIdkqgL9bO/twd2PuTz3omEbhfanhrgWTjljQyd5aTAhEXix602mYn34fC0UsN4fr+q2PRxk= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=pg9chlwj; arc=none smtp.client-ip=46.235.227.194 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1705657795; bh=fLBUyGpx4ptCR4u0+nI0SI/vY0QxIITEt56gQ9tS950=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=pg9chlwjoHyrTA2NiIAkcTF2jCfjWWhutFa1umFQ5ZOD2q7syOCUcOy8WMJMvxef6 ubN9Snzzpa8fhPUdzzgaEUQ3mOFrX524oilX5DRPaJDzSfPN2QroPtAsWiq19PMu0U X3z9zgQpbasYGzF5zcKl5wkmNdZwALnkhXt6OtLA83yjFc9GWXypWpFulhxo6U6JgK FITi+0Q1TzyrSHDYI7xZPWwGMW03BnkMxJ0w4UM0CaKyI7k664jQsGqHSGUSuFl+cT CLQxTj+XMxw5cySyR0lRl6k2cuyuYZqD1CEP0Ptq39FyipDrQX0Df47MPuHq9u6u1u mfSELge+wfvQQ== Received: from benjamin-XPS-13-9310.. (cola.collaboradmins.com [195.201.22.229]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: benjamin.gaignard) by madrid.collaboradmins.com (Postfix) with ESMTPSA id EB2B33782081; Fri, 19 Jan 2024 09:49:54 +0000 (UTC) From: Benjamin Gaignard <benjamin.gaignard@collabora.com> To: hverkuil@xs4all.nl, mchehab@kernel.org Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, kernel@collabora.com, Benjamin Gaignard <benjamin.gaignard@collabora.com> Subject: [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue Date: Fri, 19 Jan 2024 10:49:44 +0100 Message-Id: <20240119094944.26763-9-benjamin.gaignard@collabora.com> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20240119094944.26763-1-benjamin.gaignard@collabora.com> References: <20240119094944.26763-1-benjamin.gaignard@collabora.com> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-LSpam-Score: -3.0 (---) X-LSpam-Report: No, score=-3.0 required=5.0 tests=ARC_SIGNED=0.001,ARC_VALID=-0.1,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,DMARC_PASS=-0.001,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3,SPF_HELO_NONE=0.001,SPF_PASS=-0.001 autolearn=ham autolearn_force=no |
Series |
Add DELETE_BUF ioctl
|
|
Commit Message
Benjamin Gaignard
Jan. 19, 2024, 9:49 a.m. UTC
Allow to delete buffers on capture queue because it the one which
own the decoded buffers. After a dynamic resolution change lot of
them could remain allocated but won't be used anymore so deleting
them save memory.
Do not add this feature on output queue because the buffers are
smaller, fewer and always recycled even after a dynamic resolution
change.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/platform/verisilicon/hantro_drv.c | 1 +
drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
2 files changed, 2 insertions(+)
Comments
On 19/01/2024 10:49, Benjamin Gaignard wrote: > Allow to delete buffers on capture queue because it the one which > own the decoded buffers. After a dynamic resolution change lot of > them could remain allocated but won't be used anymore so deleting > them save memory. > Do not add this feature on output queue because the buffers are > smaller, fewer and always recycled even after a dynamic resolution > change. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/media/platform/verisilicon/hantro_drv.c | 1 + > drivers/media/platform/verisilicon/hantro_v4l2.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > index db3df6cc4513..f6b0a676a740 100644 > --- a/drivers/media/platform/verisilicon/hantro_drv.c > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) > dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; > dst_vq->lock = &ctx->dev->vpu_mutex; > dst_vq->dev = ctx->dev->v4l2_dev.dev; > + src_vq->supports_delete_bufs = true; As I mentioned, I remain unconvinced by this. It is just making the API inconsistent since if you support delete_bufs, then why support it for one queue only and not both? > > return vb2_queue_init(dst_vq); > } > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c > index 941fa23c211a..34eab90e8a42 100644 > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > @@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = { > .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf, > .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf, > .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs, > + .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs, > .vidioc_expbuf = v4l2_m2m_ioctl_expbuf, > > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, In my view setting vidioc_delete_bufs should enable this feature, and if for some strange reason only one queue support it, then make a wrapper callback that returns an error when used with the wrong queue. Also note that patch 6/8 never checks for q->supports_delete_bufs in vb2_core_delete_bufs(), which is wrong! Regards, Hans
Le 24/01/2024 à 13:52, Hans Verkuil a écrit : > On 19/01/2024 10:49, Benjamin Gaignard wrote: >> Allow to delete buffers on capture queue because it the one which >> own the decoded buffers. After a dynamic resolution change lot of >> them could remain allocated but won't be used anymore so deleting >> them save memory. >> Do not add this feature on output queue because the buffers are >> smaller, fewer and always recycled even after a dynamic resolution >> change. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> drivers/media/platform/verisilicon/hantro_drv.c | 1 + >> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c >> index db3df6cc4513..f6b0a676a740 100644 >> --- a/drivers/media/platform/verisilicon/hantro_drv.c >> +++ b/drivers/media/platform/verisilicon/hantro_drv.c >> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) >> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; >> dst_vq->lock = &ctx->dev->vpu_mutex; >> dst_vq->dev = ctx->dev->v4l2_dev.dev; >> + src_vq->supports_delete_bufs = true; > As I mentioned, I remain unconvinced by this. It is just making the API inconsistent > since if you support delete_bufs, then why support it for one queue only and not both? Because the both queues don't handle the same type of data. For example for a stateless decoder, for me, it makes sense to allow delete decoded frames if they won't be used anymore but that won't makes sense for bitstream buffers. > >> >> return vb2_queue_init(dst_vq); >> } >> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c >> index 941fa23c211a..34eab90e8a42 100644 >> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c >> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c >> @@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = { >> .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf, >> .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf, >> .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs, >> + .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs, >> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf, >> >> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, > In my view setting vidioc_delete_bufs should enable this feature, and if > for some strange reason only one queue support it, then make a wrapper > callback that returns an error when used with the wrong queue. > > Also note that patch 6/8 never checks for q->supports_delete_bufs in > vb2_core_delete_bufs(), which is wrong! I will fix that in next version. Regards, Benjamin > > Regards, > > Hans >
On 24/01/2024 16:35, Benjamin Gaignard wrote: > > Le 24/01/2024 à 13:52, Hans Verkuil a écrit : >> On 19/01/2024 10:49, Benjamin Gaignard wrote: >>> Allow to delete buffers on capture queue because it the one which >>> own the decoded buffers. After a dynamic resolution change lot of >>> them could remain allocated but won't be used anymore so deleting >>> them save memory. >>> Do not add this feature on output queue because the buffers are >>> smaller, fewer and always recycled even after a dynamic resolution >>> change. >>> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>> --- >>> drivers/media/platform/verisilicon/hantro_drv.c | 1 + >>> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 + >>> 2 files changed, 2 insertions(+) >>> >>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c >>> index db3df6cc4513..f6b0a676a740 100644 >>> --- a/drivers/media/platform/verisilicon/hantro_drv.c >>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c >>> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) >>> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; >>> dst_vq->lock = &ctx->dev->vpu_mutex; >>> dst_vq->dev = ctx->dev->v4l2_dev.dev; >>> + src_vq->supports_delete_bufs = true; >> As I mentioned, I remain unconvinced by this. It is just making the API inconsistent >> since if you support delete_bufs, then why support it for one queue only and not both? > > Because the both queues don't handle the same type of data. > For example for a stateless decoder, for me, it makes sense to allow delete decoded frames > if they won't be used anymore but that won't makes sense for bitstream buffers. But is there any reason why you wouldn't support this feature? We support VIDIOC_CREATE_BUFS as well, even though most drivers do not need it, but it is cheap to add. Deleting buffers is a generic feature, and I don't see why you wouldn't just offer it for both queues. Regards, Hans > >> >>> return vb2_queue_init(dst_vq); >>> } >>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c >>> index 941fa23c211a..34eab90e8a42 100644 >>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c >>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c >>> @@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = { >>> .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf, >>> .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf, >>> .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs, >>> + .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs, >>> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf, >>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, >> In my view setting vidioc_delete_bufs should enable this feature, and if >> for some strange reason only one queue support it, then make a wrapper >> callback that returns an error when used with the wrong queue. >> >> Also note that patch 6/8 never checks for q->supports_delete_bufs in >> vb2_core_delete_bufs(), which is wrong! > > I will fix that in next version. > Regards, > Benjamin > >> >> Regards, >> >> Hans >>
Le 24/01/2024 à 16:44, Hans Verkuil a écrit : > On 24/01/2024 16:35, Benjamin Gaignard wrote: >> Le 24/01/2024 à 13:52, Hans Verkuil a écrit : >>> On 19/01/2024 10:49, Benjamin Gaignard wrote: >>>> Allow to delete buffers on capture queue because it the one which >>>> own the decoded buffers. After a dynamic resolution change lot of >>>> them could remain allocated but won't be used anymore so deleting >>>> them save memory. >>>> Do not add this feature on output queue because the buffers are >>>> smaller, fewer and always recycled even after a dynamic resolution >>>> change. >>>> >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>>> --- >>>> drivers/media/platform/verisilicon/hantro_drv.c | 1 + >>>> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 + >>>> 2 files changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c >>>> index db3df6cc4513..f6b0a676a740 100644 >>>> --- a/drivers/media/platform/verisilicon/hantro_drv.c >>>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c >>>> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) >>>> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; >>>> dst_vq->lock = &ctx->dev->vpu_mutex; >>>> dst_vq->dev = ctx->dev->v4l2_dev.dev; >>>> + src_vq->supports_delete_bufs = true; >>> As I mentioned, I remain unconvinced by this. It is just making the API inconsistent >>> since if you support delete_bufs, then why support it for one queue only and not both? >> Because the both queues don't handle the same type of data. >> For example for a stateless decoder, for me, it makes sense to allow delete decoded frames >> if they won't be used anymore but that won't makes sense for bitstream buffers. > But is there any reason why you wouldn't support this feature? We support VIDIOC_CREATE_BUFS > as well, even though most drivers do not need it, but it is cheap to add. > > Deleting buffers is a generic feature, and I don't see why you wouldn't just offer it > for both queues. You want me to remove supports_delete_bufs and V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS ? This way we can remove buffers from the both queues. Sound good for you ? Regards, Benjamin > > Regards, > > Hans > >>>> return vb2_queue_init(dst_vq); >>>> } >>>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c >>>> index 941fa23c211a..34eab90e8a42 100644 >>>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c >>>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c >>>> @@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = { >>>> .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf, >>>> .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf, >>>> .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs, >>>> + .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs, >>>> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf, >>>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, >>> In my view setting vidioc_delete_bufs should enable this feature, and if >>> for some strange reason only one queue support it, then make a wrapper >>> callback that returns an error when used with the wrong queue. >>> >>> Also note that patch 6/8 never checks for q->supports_delete_bufs in >>> vb2_core_delete_bufs(), which is wrong! >> I will fix that in next version. >> Regards, >> Benjamin >> >>> Regards, >>> >>> Hans >>> >
On 25/01/2024 10:27, Benjamin Gaignard wrote: > > Le 24/01/2024 à 16:44, Hans Verkuil a écrit : >> On 24/01/2024 16:35, Benjamin Gaignard wrote: >>> Le 24/01/2024 à 13:52, Hans Verkuil a écrit : >>>> On 19/01/2024 10:49, Benjamin Gaignard wrote: >>>>> Allow to delete buffers on capture queue because it the one which >>>>> own the decoded buffers. After a dynamic resolution change lot of >>>>> them could remain allocated but won't be used anymore so deleting >>>>> them save memory. >>>>> Do not add this feature on output queue because the buffers are >>>>> smaller, fewer and always recycled even after a dynamic resolution >>>>> change. >>>>> >>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>>>> --- >>>>> drivers/media/platform/verisilicon/hantro_drv.c | 1 + >>>>> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 + >>>>> 2 files changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c >>>>> index db3df6cc4513..f6b0a676a740 100644 >>>>> --- a/drivers/media/platform/verisilicon/hantro_drv.c >>>>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c >>>>> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) >>>>> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; >>>>> dst_vq->lock = &ctx->dev->vpu_mutex; >>>>> dst_vq->dev = ctx->dev->v4l2_dev.dev; >>>>> + src_vq->supports_delete_bufs = true; >>>> As I mentioned, I remain unconvinced by this. It is just making the API inconsistent >>>> since if you support delete_bufs, then why support it for one queue only and not both? >>> Because the both queues don't handle the same type of data. >>> For example for a stateless decoder, for me, it makes sense to allow delete decoded frames >>> if they won't be used anymore but that won't makes sense for bitstream buffers. >> But is there any reason why you wouldn't support this feature? We support VIDIOC_CREATE_BUFS >> as well, even though most drivers do not need it, but it is cheap to add. >> >> Deleting buffers is a generic feature, and I don't see why you wouldn't just offer it >> for both queues. > > You want me to remove supports_delete_bufs and V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS ? > This way we can remove buffers from the both queues. > Sound good for you ? The capability is still nice to have, but it is a bit tricky to set it since you need to check if the vidioc_delete_bufs callback is set. For now drop this cap, I'll have to think about it. Regards, Hans > > Regards, > Benjamin > >> >> Regards, >> >> Hans >> >>>>> return vb2_queue_init(dst_vq); >>>>> } >>>>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c >>>>> index 941fa23c211a..34eab90e8a42 100644 >>>>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c >>>>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c >>>>> @@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = { >>>>> .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf, >>>>> .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf, >>>>> .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs, >>>>> + .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs, >>>>> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf, >>>>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, >>>> In my view setting vidioc_delete_bufs should enable this feature, and if >>>> for some strange reason only one queue support it, then make a wrapper >>>> callback that returns an error when used with the wrong queue. >>>> >>>> Also note that patch 6/8 never checks for q->supports_delete_bufs in >>>> vb2_core_delete_bufs(), which is wrong! >>> I will fix that in next version. >>> Regards, >>> Benjamin >>> >>>> Regards, >>>> >>>> Hans >>>> >>
Hi, Le mercredi 24 janvier 2024 à 16:35 +0100, Benjamin Gaignard a écrit : > Le 24/01/2024 à 13:52, Hans Verkuil a écrit : > > On 19/01/2024 10:49, Benjamin Gaignard wrote: > > > Allow to delete buffers on capture queue because it the one which > > > own the decoded buffers. After a dynamic resolution change lot of > > > them could remain allocated but won't be used anymore so deleting > > > them save memory. > > > Do not add this feature on output queue because the buffers are > > > smaller, fewer and always recycled even after a dynamic resolution > > > change. > > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > > --- > > > drivers/media/platform/verisilicon/hantro_drv.c | 1 + > > > drivers/media/platform/verisilicon/hantro_v4l2.c | 1 + > > > 2 files changed, 2 insertions(+) > > > > > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > > > index db3df6cc4513..f6b0a676a740 100644 > > > --- a/drivers/media/platform/verisilicon/hantro_drv.c > > > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > > > @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) > > > dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; > > > dst_vq->lock = &ctx->dev->vpu_mutex; > > > dst_vq->dev = ctx->dev->v4l2_dev.dev; > > > + src_vq->supports_delete_bufs = true; > > As I mentioned, I remain unconvinced by this. It is just making the API inconsistent > > since if you support delete_bufs, then why support it for one queue only and not both? > > Because the both queues don't handle the same type of data. > For example for a stateless decoder, for me, it makes sense to allow delete decoded frames > if they won't be used anymore but that won't makes sense for bitstream buffers. I personally think that for stateless and stateful decoder bitstream queue this can be useful. We currently guess the size we need, and there is no way to allocate bigger ones without the driver forgetting about reference frames. In stateful, some drivers allow to split the bitstream (I tested wave5 notably), but I was told this is not always the case. A bit of a gray zone in that API, with lack of capabilities to describe it. On stateless, the entire bitstream slice must be in one buffer. Though, for the asymmetry, most stateful decoders won't allow delete bufs on capture, because the buffers are registered in the firmware ahead of time. wave5 can't even implement create_bufs on capture. We had an argument about having create_bufs on only one queue for that specific driver, as we wanted something upstream, we flex to removing create bufs completely. I think the all or nothing rule on per queue create/delete_bufs is not aligned with the reality. Nicolas > > > > > > > > > return vb2_queue_init(dst_vq); > > > } > > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c > > > index 941fa23c211a..34eab90e8a42 100644 > > > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > > > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > > > @@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = { > > > .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf, > > > .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf, > > > .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs, > > > + .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs, > > > .vidioc_expbuf = v4l2_m2m_ioctl_expbuf, > > > > > > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, > > In my view setting vidioc_delete_bufs should enable this feature, and if > > for some strange reason only one queue support it, then make a wrapper > > callback that returns an error when used with the wrong queue. > > > > Also note that patch 6/8 never checks for q->supports_delete_bufs in > > vb2_core_delete_bufs(), which is wrong! > > I will fix that in next version. > Regards, > Benjamin > > > > > Regards, > > > > Hans > > >
On 25/01/2024 17:27, Nicolas Dufresne wrote: > Hi, > > Le mercredi 24 janvier 2024 à 16:35 +0100, Benjamin Gaignard a écrit : >> Le 24/01/2024 à 13:52, Hans Verkuil a écrit : >>> On 19/01/2024 10:49, Benjamin Gaignard wrote: >>>> Allow to delete buffers on capture queue because it the one which >>>> own the decoded buffers. After a dynamic resolution change lot of >>>> them could remain allocated but won't be used anymore so deleting >>>> them save memory. >>>> Do not add this feature on output queue because the buffers are >>>> smaller, fewer and always recycled even after a dynamic resolution >>>> change. >>>> >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>>> --- >>>> drivers/media/platform/verisilicon/hantro_drv.c | 1 + >>>> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 + >>>> 2 files changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c >>>> index db3df6cc4513..f6b0a676a740 100644 >>>> --- a/drivers/media/platform/verisilicon/hantro_drv.c >>>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c >>>> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) >>>> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; >>>> dst_vq->lock = &ctx->dev->vpu_mutex; >>>> dst_vq->dev = ctx->dev->v4l2_dev.dev; >>>> + src_vq->supports_delete_bufs = true; >>> As I mentioned, I remain unconvinced by this. It is just making the API inconsistent >>> since if you support delete_bufs, then why support it for one queue only and not both? >> >> Because the both queues don't handle the same type of data. >> For example for a stateless decoder, for me, it makes sense to allow delete decoded frames >> if they won't be used anymore but that won't makes sense for bitstream buffers. > > I personally think that for stateless and stateful decoder bitstream queue this > can be useful. We currently guess the size we need, and there is no way to > allocate bigger ones without the driver forgetting about reference frames. > > In stateful, some drivers allow to split the bitstream (I tested wave5 notably), > but I was told this is not always the case. A bit of a gray zone in that API, > with lack of capabilities to describe it. On stateless, the entire bitstream > slice must be in one buffer. > > Though, for the asymmetry, most stateful decoders won't allow delete bufs on > capture, because the buffers are registered in the firmware ahead of time. wave5 > can't even implement create_bufs on capture. We had an argument about having > create_bufs on only one queue for that specific driver, as we wanted something > upstream, we flex to removing create bufs completely. I think the all or nothing > rule on per queue create/delete_bufs is not aligned with the reality. I think the default should be that it supports DELETE_BUFS for both queues, but it should still be possible to only have it on one queue. When v18 is posted I want to play around with that, because I am not certain what the easiest way is to implement this. Another thing that needs to be added is a check that DELETE_BUFS is only enabled if CREATE_BUFS is also present, it makes no sense otherwise. Finally I want to take another look at the work required to make a CREATE_BUFS replacement since that ioctl is horrible. Whether that will become part of this series or done as a follow-up I am not sure about, but this series should definitely make it possible to cleanly integrate it. Regards, Hans > > Nicolas >> >>> >>>> >>>> return vb2_queue_init(dst_vq); >>>> } >>>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c >>>> index 941fa23c211a..34eab90e8a42 100644 >>>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c >>>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c >>>> @@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = { >>>> .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf, >>>> .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf, >>>> .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs, >>>> + .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs, >>>> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf, >>>> >>>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, >>> In my view setting vidioc_delete_bufs should enable this feature, and if >>> for some strange reason only one queue support it, then make a wrapper >>> callback that returns an error when used with the wrong queue. >>> >>> Also note that patch 6/8 never checks for q->supports_delete_bufs in >>> vb2_core_delete_bufs(), which is wrong! >> >> I will fix that in next version. >> Regards, >> Benjamin >> >>> >>> Regards, >>> >>> Hans >>> >> >
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c index db3df6cc4513..f6b0a676a740 100644 --- a/drivers/media/platform/verisilicon/hantro_drv.c +++ b/drivers/media/platform/verisilicon/hantro_drv.c @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; dst_vq->lock = &ctx->dev->vpu_mutex; dst_vq->dev = ctx->dev->v4l2_dev.dev; + src_vq->supports_delete_bufs = true; return vb2_queue_init(dst_vq); } diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c index 941fa23c211a..34eab90e8a42 100644 --- a/drivers/media/platform/verisilicon/hantro_v4l2.c +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c @@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = { .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf, .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf, .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs, + .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs, .vidioc_expbuf = v4l2_m2m_ioctl_expbuf, .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,