From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 DC2FA10F2 for ; Thu, 17 Oct 2024 14:38:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729175917; cv=none; b=XvGwddEA28aeEkwHcQ+3hugHo+bi1TpY+Tf87tjvGI1ryM2IMAWJ2YB4DZWAPjdKEFhUgNAVn8zNYofIQ3eJNAX3OKViETOy2A9NVQSk4jpHSt3TwP7/8z9sY1Ja75EL+5G3QTtzcO9YC+m/NUTMuLDYOrEQpBFFm/1MbJ4/cgI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729175917; c=relaxed/simple; bh=ZEzI6YFPB94EDyer1HiqW8jsQ9FBMKMl/iITzHMW3sE=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=NT7olJbtlOM+J/YyiIhkELThUW7/BKuHRoREVjPB90KvR0YiBvJbEQLwcA5oHmSJXrpLUGJKOw5HCJbIfIjt9qVBn+5eK5LUFMvmNB0TQAZvu6dMLv4b08XaEWCqI1BLRXigkt1QEirrpR6uZGEYLsPoezvNOs/guy/5xIoex9s= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=B1wFPcqs; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="B1wFPcqs" Received: by smtp.kernel.org (Postfix) with ESMTPSA id C050FC4CEC3; Thu, 17 Oct 2024 14:38:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1729175917; bh=ZEzI6YFPB94EDyer1HiqW8jsQ9FBMKMl/iITzHMW3sE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=B1wFPcqs+c95g+5x89HRyMPWKY3TgBt1WIGvCoM12btz3xFVvstbad43PwHUwdVv5 pzwA6VehvQFm0jFVyZW309VLsSDLSkkQCLumZswpKS/pPkmIaO7oNAB2PEPMqfmWpa s9zwugqQ2amNKvcMpGqBF9x9eA5eJnG01x9Q7DcX3GUpN1HzOv1dWTbxoDyekORXxy MOshzFp6+Ttsj0fYhlwddiM3jorP6IbcyJYXwiIutb16fwZhCpJOzXBcG+xNuiFWDE NGN3Vd3f9IvqiV/pXhBvkex6hr0a81f7jPeukktqw8NvfGpPhn6qr4gQ7rmpPZRTpz 3QYhUZ48lbDGw== Date: Thu, 17 Oct 2024 16:38:13 +0200 From: Mauro Carvalho Chehab To: Hans Verkuil Cc: Tomasz Figa , Marek Szyprowski , Mauro Carvalho Chehab , Shuah Khan , Kieran Bingham , Daniel Almeida , Andy Walls , Yong Zhi , Sakari Ailus , Bingbu Cao , Dan Scally , Tianshu Qiu , Martin Tuma , Bluecherry Maintainers , Andrey Utkin , Ismael Luceno , Ezequiel Garcia , Corentin Labbe , Michael Krufky , Laurent Pinchart , Matt Ranostay , Michael Tretter , Pengutronix Kernel Team , Neil Armstrong , Kevin Hilman , Jerome Brunet , Martin Blumenstingl , Ming Qian , Zhou Peng , Eddie James , Joel Stanley , Andrew Jeffery , Eugen Hristev , Nicolas Ferre , Alexandre Belloni , Claudiu Beznea , Raspberry Pi Kernel Maintenance , Florian Fainelli , Broadcom internal kernel review list , Ray Jui , Scott Branden , Philipp Zabel , Nas Chung , Jackson Lee , Devarsh Thakkar , Bin Liu , Matthias Brugger , AngeloGioacchino Del Regno , Minghsiu Tsai , Houlong Wei , Andrew-CT Chen , Tiffany Lin , Yunfei Dong , Joseph Liu , Marvin Lin , Dmitry Osipenko , Thierry Reding , Jonathan Hunter , Xavier Roumegue , Mirela Rabulea , Shawn Guo , Sascha Hauer , Fabio Estevam , Rui Miguel Silva , Martin Kepplinger , Purism Kernel Team , Robert Foss , Todor Tomov , Bryan O'Donoghue , Stanimir Varbanov , Vikash Garodia , Jacopo Mondi , Niklas =?UTF-8?B?U8O2ZGVybHVuZA==?= , Fabrizio Castro , Kieran Bingham , Mikhail Ulyanov , Jacob Chen , Heiko Stuebner , Dafna Hirschfeld , Krzysztof Kozlowski , Alim Akhtar , Sylwester Nawrocki , =?UTF-8?B?xYF1a2Fzeg==?= Stelmach , Andrzej Pietrasiewicz , Jacek Anaszewski , Andrzej Hajda , Fabien Dessenne , Hugues Fruchet , Jean-Christophe Trotin , Maxime Coquelin , Alexandre Torgue , Alain Volmat , Maxime Ripard , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Yong Deng , Paul Kocialkowski , Benoit Parrot , Jai Luthra , Michal Simek , Andy Shevchenko , Hans de Goede , Greg Kroah-Hartman , Steve Longerbeam , Jack Zhu , Changhuang Liang , Sowjanya Komatineni , Luca Ceresoli , linux-media@vger.kernel.org Subject: Re: [PATCH 01/10] media: videobuf2-core: update vb2_thread if wait_finish/prepare are NULL Message-ID: <20241017163813.25a38a67@foz.lan> In-Reply-To: <20241014-vb2-wait-v1-1-8c3ee25c618c@xs4all.nl> References: <20241014-vb2-wait-v1-0-8c3ee25c618c@xs4all.nl> <20241014-vb2-wait-v1-1-8c3ee25c618c@xs4all.nl> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.43; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Em Mon, 14 Oct 2024 17:06:28 +0200 Hans Verkuil escreveu: > For read/write support the vb2_thread is used. This will queue and > dequeue buffers automatically to provide the read() or write() feature. > > It calls wait_finish/prepare around vb2_core_dqbuf() and vb2_core_qbuf(), > but that assumes all drivers have these ops set. But that will change > due to commit 88785982a19d ("media: vb2: use lock if wait_prepare/finish > are NULL"). > > So instead check if the callback is available, and if not, use q->lock, > just as __vb2_wait_for_done_vb() does. > > It was also used around vb2_core_qbuf(), but VIDIOC_QBUF doesn't > need this since it doesn't do a blocking wait, so just drop the > wait_finish/prepare callbacks around vb2_core_qbuf(). > > Signed-off-by: Hans Verkuil > --- > drivers/media/common/videobuf2/videobuf2-core.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index d064e0664851b26b2da71e0a374c49a2d2c5e217..e9c1d9e3222323be50b3039eb463384a3d558239 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -3218,10 +3218,16 @@ static int vb2_thread(void *data) > continue; > prequeue--; > } else { > - call_void_qop(q, wait_finish, q); > + if (q->ops->wait_finish) > + call_void_qop(q, wait_finish, q); > + else if (q->lock) > + mutex_lock(q->lock); > if (!threadio->stop) > ret = vb2_core_dqbuf(q, &index, NULL, 0); > - call_void_qop(q, wait_prepare, q); > + if (q->ops->wait_prepare) > + call_void_qop(q, wait_prepare, q); > + else if (q->lock) > + mutex_unlock(q->lock); > dprintk(q, 5, "file io: vb2_dqbuf result: %d\n", ret); > if (!ret) > vb = vb2_get_buffer(q, index); Looks OK to me. > @@ -3233,12 +3239,10 @@ static int vb2_thread(void *data) > if (vb->state != VB2_BUF_STATE_ERROR) > if (threadio->fnc(vb, threadio->priv)) > break; > - call_void_qop(q, wait_finish, q); > if (copy_timestamp) > vb->timestamp = ktime_get_ns(); > if (!threadio->stop) > ret = vb2_core_qbuf(q, vb, NULL, NULL); > - call_void_qop(q, wait_prepare, q); > if (ret || threadio->stop) This hunk looks suspicious on my eyes. Why this is not needed? Had you test with all vb2 callers including the DVB one? If this is fixing a pre-existing condition, which seems the case, please place it on a separate patch, clearly stating why we need to drop an old code. An alternative would be, for now, apply the same change as you did at the first hunk, e. g.: if (q->ops->wait_finish) call_void_qop(q, wait_finish, q); else if (q->lock) mutex_lock(q->lock); ... if (q->ops->wait_prepare) call_void_qop(q, wait_prepare, q); else if (q->lock) mutex_unlock(q->lock); And only drop those when we're certain that none of the devices would break with such change. Once patch 1 is fixed, feel free to add my: Reviewed-by: Mauro Carvalho Chehab for patches 2-10. Thanks, Mauro