Message ID | 20231031163104.112469-36-benjamin.gaignard@collabora.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b90f:0:b0:403:3b70:6f57 with SMTP id t15csp364761vqg; Tue, 31 Oct 2023 09:35:07 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGw6rJEleZDnNY/RRy1EFe9v0ygBP0RfTQkzc14gyJVr16EAeG4Xq/ZleWJrcu5AngZGsyK X-Received: by 2002:a17:902:d50d:b0:1cc:3875:e654 with SMTP id b13-20020a170902d50d00b001cc3875e654mr6758636plg.26.1698770107556; Tue, 31 Oct 2023 09:35:07 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698770107; cv=none; d=google.com; s=arc-20160816; b=go6bzrN3md0ya9QYRrWPs/QVrNH5zT+2fAB5tQkLsQEmSO0h6iHBpG2fTOzbJ+eQkM 7JOHz/HCXr3XxqiNpDjqTwIbEdyvdoBb6ZAQjJ73cJfdWtAxGteij8Vf2JDv62MWO5pT pGWOlWFw43A0mw89LHOy6mTMyI/33Iag22nqVLXYgbKaivHpAxWxoQv3wc84HWFvPlAK hKFj3lENEh2JrHU1tGk9NlVLhaH/GtRwUYgDrr6XHuCYnY+qdWqDX0Qhx9NsvcDNquI2 fING6os6xy0DuSoa5RLZvmQyIqPNkejSSqbW4Ka/s9Ubzt+MIpfkkUAh30rjLzl8KT0B fuIQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=qa0gGpZNMAqHQmNMLRw2KJGOdZocq9+DqAJPNBVbPVs=; fh=aVUrqUP+Ljv+BSkNtX805Xk8KRvk/poCiUhiRUDoQMU=; b=ufKWCaFrzIu1NZZs88bci4NjRzBmvwswyOKFIFvQKNcdlTVjjl+qbNoYnh3Km3rJlV 4HLvzICQd/lehrpksb+jngeHWSA4ck88w5pXZ0FyZrtzGgLysZRXWJkvWsROk3JnEIbB /Jj2X1P/BLaPdtfCHJyFHomwTRRWELAnrvm80ZuD8jk1jZNCZFNvH9/XSMVRJCmdFj6g nQnZeRIixLyOtzFz43agjvqwvOmw1zmUIimV3Vu15wMSVuFYbQVnc0rz09hkqu+C7Rct kdLLXV/LIOsbYMu3JDqfumhh97m9XomRjaQrfAjq2vQq6V3RARohwxdehBCsTJMy+4jq Ny8g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=nyWFCGJR; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from agentk.vger.email (agentk.vger.email. [23.128.96.32]) by mx.google.com with ESMTPS id li12-20020a170903294c00b001c45291b6ebsi1185845plb.272.2023.10.31.09.35.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 Oct 2023 09:35:07 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32; Authentication-Results: mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=nyWFCGJR; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id A2FD380C47A9; Tue, 31 Oct 2023 09:34:59 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346386AbjJaQe3 (ORCPT <rfc822;chrisjones.unixmen@gmail.com> + 33 others); Tue, 31 Oct 2023 12:34:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46224 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1346536AbjJaQd4 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 31 Oct 2023 12:33:56 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E040B19B3; Tue, 31 Oct 2023 09:32:20 -0700 (PDT) Received: from benjamin-XPS-13-9310.. (unknown [IPv6:2a01:e0a:120:3210:c562:2ef4:80c0:92f]) (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 madras.collabora.co.uk (Postfix) with ESMTPSA id 71A3166073C5; Tue, 31 Oct 2023 16:32:18 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1698769938; bh=+N2NXYHRMMiyUC0gb7VJb1oDXBrovxuB6Pt09SJAcIk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=nyWFCGJRTiLSd8jxLolK9jZdPV9nPmfSKbaQ6fsrFS27GeVj70PBU75s7Wb9gwA9e CnY6GLGfAfyc7BW0SVOjCXWxGofzm/14RFHb2wXKVnF5CjrN0ye71snxb1emEKIXPN Wb8MQJBLKkgDOX4CHIQ2AP0H0wSBCqLZ4RdLKQHbCxL/1qo/6nr99kmu/1tJfdPcL0 EGaW0Km1JuBp8sxiR5/NdieknbuV2/bf1lfRVDWFxaNAqibh84/MBqCJF4HnaZLD9M z0TRPr5IbEbvfs4CMy4GA+DwvL+nnwYYZcgFlYo0CyBLuOJcmC6ciTC+ALAZx6iW7N KLovdv4wnE0gQ== From: Benjamin Gaignard <benjamin.gaignard@collabora.com> To: mchehab@kernel.org, tfiga@chromium.org, m.szyprowski@samsung.com, ming.qian@nxp.com, ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de, gregkh@linuxfoundation.org, hverkuil-cisco@xs4all.nl, nicolas.dufresne@collabora.com Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev, kernel@collabora.com, Benjamin Gaignard <benjamin.gaignard@collabora.com>, Paul Kocialkowski <paul.kocialkowski@bootlin.com>, Maxime Ripard <mripard@kernel.org> Subject: [PATCH v14 35/56] media: cedrus: Stop direct calls to queue num_buffers field Date: Tue, 31 Oct 2023 17:30:43 +0100 Message-Id: <20231031163104.112469-36-benjamin.gaignard@collabora.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20231031163104.112469-1-benjamin.gaignard@collabora.com> References: <20231031163104.112469-1-benjamin.gaignard@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Tue, 31 Oct 2023 09:34:59 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781289564305421151 X-GMAIL-MSGID: 1781289564305421151 |
Series |
Add DELETE_BUF ioctl
|
|
Commit Message
Benjamin Gaignard
Oct. 31, 2023, 4:30 p.m. UTC
Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. This allows us to change how the number of buffers is computed in the future. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> CC: Maxime Ripard <mripard@kernel.org> --- drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 9 +++++++-- drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-)
Comments
Hi Paul, W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Given you've alaredy A-b, would you be ok with adding this sentence: "While at it, check the return value of vb2_get_buffer()." to the commit message body? @Benjamin: With this change, you can add my Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Maxime Ripard <mripard@kernel.org> > --- > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 9 +++++++-- > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 9 +++++++-- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > index dfb401df138a..3e2843ef6cce 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > @@ -653,8 +653,13 @@ static void cedrus_h264_stop(struct cedrus_ctx *ctx) > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > > - for (i = 0; i < vq->num_buffers; i++) { > - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); > + for (i = 0; i < vb2_get_num_buffers(vq); i++) { > + struct vb2_buffer *vb = vb2_get_buffer(vq, i); > + > + if (!vb) > + continue; > + > + buf = vb2_to_cedrus_buffer(vb); > > if (buf->codec.h264.mv_col_buf_size > 0) { > dma_free_attrs(dev->dev, > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > index fc9297232456..52e94c8f2f01 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > @@ -869,8 +869,13 @@ static void cedrus_h265_stop(struct cedrus_ctx *ctx) > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > > - for (i = 0; i < vq->num_buffers; i++) { > - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); > + for (i = 0; i < vb2_get_num_buffers(vq); i++) { > + struct vb2_buffer *vb = vb2_get_buffer(vq, i); > + > + if (!vb) > + continue; > + > + buf = vb2_to_cedrus_buffer(vb); > > if (buf->codec.h265.mv_col_buf_size > 0) { > dma_free_attrs(dev->dev,
Hi Andrej, On Thu 09 Nov 23, 12:27, Andrzej Pietrasiewicz wrote: > Hi Paul, > > W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > > This allows us to change how the number of buffers is computed in the > > future. > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > Given you've alaredy A-b, would you be ok with adding this sentence: > > "While at it, check the return value of vb2_get_buffer()." > > to the commit message body? Not only do I agree, but because this is done in a function returning void, you could even: if (WARN_ON(!vb)) continue; so that it doesn't go completely unnoticed. What do you think? Cheers, Paul > @Benjamin: > > With this change, you can add my > > Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > > > CC: Maxime Ripard <mripard@kernel.org> > > --- > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 9 +++++++-- > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 9 +++++++-- > > 2 files changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > index dfb401df138a..3e2843ef6cce 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > @@ -653,8 +653,13 @@ static void cedrus_h264_stop(struct cedrus_ctx *ctx) > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > > - for (i = 0; i < vq->num_buffers; i++) { > > - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); > > + for (i = 0; i < vb2_get_num_buffers(vq); i++) { > > + struct vb2_buffer *vb = vb2_get_buffer(vq, i); > > + > > + if (!vb) > > + continue; > > + > > + buf = vb2_to_cedrus_buffer(vb); > > if (buf->codec.h264.mv_col_buf_size > 0) { > > dma_free_attrs(dev->dev, > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > index fc9297232456..52e94c8f2f01 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > @@ -869,8 +869,13 @@ static void cedrus_h265_stop(struct cedrus_ctx *ctx) > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > > - for (i = 0; i < vq->num_buffers; i++) { > > - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); > > + for (i = 0; i < vb2_get_num_buffers(vq); i++) { > > + struct vb2_buffer *vb = vb2_get_buffer(vq, i); > > + > > + if (!vb) > > + continue; > > + > > + buf = vb2_to_cedrus_buffer(vb); > > if (buf->codec.h265.mv_col_buf_size > 0) { > > dma_free_attrs(dev->dev, >
Hi Paul, W dniu 9.11.2023 o 15:14, Paul Kocialkowski pisze: > Hi Andrzej, > > On Thu 09 Nov 23, 12:27, Andrzej Pietrasiewicz wrote: >> Hi Paul, >> >> W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: >>> Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. >>> This allows us to change how the number of buffers is computed in the >>> future. >>> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>> Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> >> >> Given you've alaredy A-b, would you be ok with adding this sentence: >> >> "While at it, check the return value of vb2_get_buffer()." >> >> to the commit message body? > > Not only do I agree, but because this is done in a function returning void, > you could even: > > if (WARN_ON(!vb)) > continue; > > so that it doesn't go completely unnoticed. > > What do you think? > I'd ask Benjamin. But my take on the direction of changes is that ultimately there will be "holes" in the array of allocated buffers (hence the bitmap to track which slots are used and which are not). In other words, getting a NULL sometimes will be an expected situation, and a WARN() will not be appropriate for an expected situation. @Benjamin? Regards, Andrzej > Cheers, > > Paul > >> @Benjamin: >> >> With this change, you can add my >> >> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> >> >>> CC: Maxime Ripard <mripard@kernel.org> >>> --- >>> drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 9 +++++++-- >>> drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 9 +++++++-- >>> 2 files changed, 14 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c >>> index dfb401df138a..3e2843ef6cce 100644 >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c >>> @@ -653,8 +653,13 @@ static void cedrus_h264_stop(struct cedrus_ctx *ctx) >>> vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); >>> - for (i = 0; i < vq->num_buffers; i++) { >>> - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); >>> + for (i = 0; i < vb2_get_num_buffers(vq); i++) { >>> + struct vb2_buffer *vb = vb2_get_buffer(vq, i); >>> + >>> + if (!vb) >>> + continue; >>> + >>> + buf = vb2_to_cedrus_buffer(vb); >>> if (buf->codec.h264.mv_col_buf_size > 0) { >>> dma_free_attrs(dev->dev, >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >>> index fc9297232456..52e94c8f2f01 100644 >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >>> @@ -869,8 +869,13 @@ static void cedrus_h265_stop(struct cedrus_ctx *ctx) >>> vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); >>> - for (i = 0; i < vq->num_buffers; i++) { >>> - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); >>> + for (i = 0; i < vb2_get_num_buffers(vq); i++) { >>> + struct vb2_buffer *vb = vb2_get_buffer(vq, i); >>> + >>> + if (!vb) >>> + continue; >>> + >>> + buf = vb2_to_cedrus_buffer(vb); >>> if (buf->codec.h265.mv_col_buf_size > 0) { >>> dma_free_attrs(dev->dev, >> > > > _______________________________________________ > Kernel mailing list -- kernel@mailman.collabora.com > To unsubscribe send an email to kernel-leave@mailman.collabora.com
Le 09/11/2023 à 16:48, Andrzej Pietrasiewicz a écrit : > Hi Paul, > > W dniu 9.11.2023 o 15:14, Paul Kocialkowski pisze: >> Hi Andrzej, >> >> On Thu 09 Nov 23, 12:27, Andrzej Pietrasiewicz wrote: >>> Hi Paul, >>> >>> W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: >>>> Use vb2_get_num_buffers() to avoid using queue num_buffers field >>>> directly. >>>> This allows us to change how the number of buffers is computed in the >>>> future. >>>> >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>>> Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> >>> >>> Given you've alaredy A-b, would you be ok with adding this sentence: >>> >>> "While at it, check the return value of vb2_get_buffer()." >>> >>> to the commit message body? >> >> Not only do I agree, but because this is done in a function returning >> void, >> you could even: >> >> if (WARN_ON(!vb)) >> continue; >> >> so that it doesn't go completely unnoticed. >> >> What do you think? >> > > I'd ask Benjamin. > > But my take on the direction of changes is that ultimately > there will be "holes" in the array of allocated buffers (hence the > bitmap to track which slots are used and which are not). In other words, > getting a NULL sometimes will be an expected situation, and a WARN() will > not be appropriate for an expected situation. > > @Benjamin? That should never happens unless you add delete buffers capability to the driver and in this case it is normal to have holes. Other drivers do not use WARN_ON() so I will not do it for this one. Regards, Benjamin > > Regards, > > Andrzej > >> Cheers, >> >> Paul >> >>> @Benjamin: >>> >>> With this change, you can add my >>> >>> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> >>> >>>> CC: Maxime Ripard <mripard@kernel.org> >>>> --- >>>> drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 9 +++++++-- >>>> drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 9 +++++++-- >>>> 2 files changed, 14 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c >>>> b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c >>>> index dfb401df138a..3e2843ef6cce 100644 >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c >>>> @@ -653,8 +653,13 @@ static void cedrus_h264_stop(struct cedrus_ctx >>>> *ctx) >>>> vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, >>>> V4L2_BUF_TYPE_VIDEO_CAPTURE); >>>> - for (i = 0; i < vq->num_buffers; i++) { >>>> - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); >>>> + for (i = 0; i < vb2_get_num_buffers(vq); i++) { >>>> + struct vb2_buffer *vb = vb2_get_buffer(vq, i); >>>> + >>>> + if (!vb) >>>> + continue; >>>> + >>>> + buf = vb2_to_cedrus_buffer(vb); >>>> if (buf->codec.h264.mv_col_buf_size > 0) { >>>> dma_free_attrs(dev->dev, >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >>>> b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >>>> index fc9297232456..52e94c8f2f01 100644 >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >>>> @@ -869,8 +869,13 @@ static void cedrus_h265_stop(struct cedrus_ctx >>>> *ctx) >>>> vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, >>>> V4L2_BUF_TYPE_VIDEO_CAPTURE); >>>> - for (i = 0; i < vq->num_buffers; i++) { >>>> - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); >>>> + for (i = 0; i < vb2_get_num_buffers(vq); i++) { >>>> + struct vb2_buffer *vb = vb2_get_buffer(vq, i); >>>> + >>>> + if (!vb) >>>> + continue; >>>> + >>>> + buf = vb2_to_cedrus_buffer(vb); >>>> if (buf->codec.h265.mv_col_buf_size > 0) { >>>> dma_free_attrs(dev->dev, >>> >> >> >> _______________________________________________ >> Kernel mailing list -- kernel@mailman.collabora.com >> To unsubscribe send an email to kernel-leave@mailman.collabora.com >
Hi, On Thu 09 Nov 23, 16:54, Benjamin Gaignard wrote: > > Le 09/11/2023 à 16:48, Andrzej Pietrasiewicz a écrit : > > Hi Paul, > > > > W dniu 9.11.2023 o 15:14, Paul Kocialkowski pisze: > > > Hi Andrzej, > > > > > > On Thu 09 Nov 23, 12:27, Andrzej Pietrasiewicz wrote: > > > > Hi Paul, > > > > > > > > W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > > > > > Use vb2_get_num_buffers() to avoid using queue num_buffers > > > > > field directly. > > > > > This allows us to change how the number of buffers is computed in the > > > > > future. > > > > > > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > > > > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > > > > > Given you've alaredy A-b, would you be ok with adding this sentence: > > > > > > > > "While at it, check the return value of vb2_get_buffer()." > > > > > > > > to the commit message body? > > > > > > Not only do I agree, but because this is done in a function > > > returning void, > > > you could even: > > > > > > if (WARN_ON(!vb)) > > > continue; > > > > > > so that it doesn't go completely unnoticed. > > > > > > What do you think? > > > > > > > I'd ask Benjamin. > > > > But my take on the direction of changes is that ultimately > > there will be "holes" in the array of allocated buffers (hence the > > bitmap to track which slots are used and which are not). In other words, > > getting a NULL sometimes will be an expected situation, and a WARN() will > > not be appropriate for an expected situation. > > > > @Benjamin? > > That should never happens unless you add delete buffers capability to the driver > and in this case it is normal to have holes. > Other drivers do not use WARN_ON() so I will not do it for this one. Yeah I also don't expect that buffers can just disappear on us before introducing the delete buffers capability. But okay it's fine with me to not use WARN_ON. Cheers, Paul > Regards, > Benjamin > > > > > Regards, > > > > Andrzej > > > > > Cheers, > > > > > > Paul > > > > > > > @Benjamin: > > > > > > > > With this change, you can add my > > > > > > > > Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > > > > > > > > > CC: Maxime Ripard <mripard@kernel.org> > > > > > --- > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 9 +++++++-- > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 9 +++++++-- > > > > > 2 files changed, 14 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git > > > > > a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > index dfb401df138a..3e2843ef6cce 100644 > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > @@ -653,8 +653,13 @@ static void cedrus_h264_stop(struct > > > > > cedrus_ctx *ctx) > > > > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, > > > > > V4L2_BUF_TYPE_VIDEO_CAPTURE); > > > > > - for (i = 0; i < vq->num_buffers; i++) { > > > > > - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); > > > > > + for (i = 0; i < vb2_get_num_buffers(vq); i++) { > > > > > + struct vb2_buffer *vb = vb2_get_buffer(vq, i); > > > > > + > > > > > + if (!vb) > > > > > + continue; > > > > > + > > > > > + buf = vb2_to_cedrus_buffer(vb); > > > > > if (buf->codec.h264.mv_col_buf_size > 0) { > > > > > dma_free_attrs(dev->dev, > > > > > diff --git > > > > > a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > index fc9297232456..52e94c8f2f01 100644 > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > @@ -869,8 +869,13 @@ static void cedrus_h265_stop(struct > > > > > cedrus_ctx *ctx) > > > > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, > > > > > V4L2_BUF_TYPE_VIDEO_CAPTURE); > > > > > - for (i = 0; i < vq->num_buffers; i++) { > > > > > - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); > > > > > + for (i = 0; i < vb2_get_num_buffers(vq); i++) { > > > > > + struct vb2_buffer *vb = vb2_get_buffer(vq, i); > > > > > + > > > > > + if (!vb) > > > > > + continue; > > > > > + > > > > > + buf = vb2_to_cedrus_buffer(vb); > > > > > if (buf->codec.h265.mv_col_buf_size > 0) { > > > > > dma_free_attrs(dev->dev, > > > > > > > > > > > > > _______________________________________________ > > > Kernel mailing list -- kernel@mailman.collabora.com > > > To unsubscribe send an email to kernel-leave@mailman.collabora.com > >
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index dfb401df138a..3e2843ef6cce 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c @@ -653,8 +653,13 @@ static void cedrus_h264_stop(struct cedrus_ctx *ctx) vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); - for (i = 0; i < vq->num_buffers; i++) { - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); + for (i = 0; i < vb2_get_num_buffers(vq); i++) { + struct vb2_buffer *vb = vb2_get_buffer(vq, i); + + if (!vb) + continue; + + buf = vb2_to_cedrus_buffer(vb); if (buf->codec.h264.mv_col_buf_size > 0) { dma_free_attrs(dev->dev, diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index fc9297232456..52e94c8f2f01 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c @@ -869,8 +869,13 @@ static void cedrus_h265_stop(struct cedrus_ctx *ctx) vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); - for (i = 0; i < vq->num_buffers; i++) { - buf = vb2_to_cedrus_buffer(vb2_get_buffer(vq, i)); + for (i = 0; i < vb2_get_num_buffers(vq); i++) { + struct vb2_buffer *vb = vb2_get_buffer(vq, i); + + if (!vb) + continue; + + buf = vb2_to_cedrus_buffer(vb); if (buf->codec.h265.mv_col_buf_size > 0) { dma_free_attrs(dev->dev,