From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (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 5CF854C8C for ; Wed, 16 Oct 2024 08:20:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729066860; cv=none; b=qHH8yBKt4p4v7ZaF5SwrLqTOdjyL1it9ZLTYe9lsjtwfILnESlb6hu1nqNLCm7J1WVrweHi1sBhFrAOosAWN1JzUPma/RJFNZ/y063Oy2sIB2H2MRhKJddTbM41cBwNKXMPVubYUIYE8ZUtaSwfI/xJ/Bm1oXQAhFG79qpZ49Wk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729066860; c=relaxed/simple; bh=Ijfqe7gf8UL5kxettkZiw6QherlN/17dXw8zumhPmt4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Y1OHTOxco8Fsl6tZFTltMf3icUzt5OEccb5pIOzNJLsRR5Gjn+uGmArRLP07q6uXd814NgOX/nzEuxYNiM0LzDZow8hFgvZooTMP3q+YpY+Casv+1xmurcOON2xE9bviBtQYL6dPho/EiZaObd/caXTWiUisAIILTJ0TyVv0BbY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=iPT+SCfy; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="iPT+SCfy" Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id C48A6827; Wed, 16 Oct 2024 10:19:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1729066750; bh=Ijfqe7gf8UL5kxettkZiw6QherlN/17dXw8zumhPmt4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=iPT+SCfyZz+tcaJMamCWNpfaCHWlQj35fj+LvgMOhn9z46XJ456czRptYJhFHmpHu ka/6IAPheEUrERCssOcSIi7CSSzebEF8LnJWnbFvoK3FkAx4vNmFj2O6N4L84iHUH2 fpeLCy8LfVPlUVcIlgK2DJiG1BAdtCnX4VmLJB2c= Date: Wed, 16 Oct 2024 11:20:48 +0300 From: Laurent Pinchart 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 , 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?Q?S=C3=B6derlund?= , Fabrizio Castro , Kieran Bingham , Mikhail Ulyanov , Jacob Chen , Heiko Stuebner , Dafna Hirschfeld , Krzysztof Kozlowski , Alim Akhtar , Sylwester Nawrocki , =?utf-8?Q?=C5=81ukasz?= 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: <20241016082048.GD2712@pendragon.ideasonboard.com> References: <20241014-vb2-wait-v1-0-8c3ee25c618c@xs4all.nl> <20241014-vb2-wait-v1-1-8c3ee25c618c@xs4all.nl> <20241014191549.GB5522@pendragon.ideasonboard.com> 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=utf-8 Content-Disposition: inline In-Reply-To: Hi Hans, On Tue, Oct 15, 2024 at 08:56:30AM +0200, Hans Verkuil wrote: > On 14/10/2024 21:15, Laurent Pinchart wrote: > > On Mon, Oct 14, 2024 at 05:06:28PM +0200, Hans Verkuil wrote: > >> 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); > > > > I would still prefer moving vb2_ops_wait_prepare() and > > vb2_ops_wait_finish() to videobuf2-core.c and calling the functions > > here, instead of locking the mutex directly. I think it would make the > > code more readable. I won't block the patch for this, but I think it > > would be better. > > The whole point of this series is to prepare for the removal of the > wait_finish/prepare callbacks. So this patch is just a temporary change. > > Eventually this code will change to just a mutex_lock. OK. > > Also, should we check at queue init time that drivers either set a queue > > lock or provide the .wait_prepare() and .wait_finish() operations ? > > It does that already, from videobuf2-core.c, vb2_core_queue_init(): > > /* Warn if q->lock is NULL and no custom wait_prepare is provided */ > if (WARN_ON(!q->lock && !q->ops->wait_prepare)) > return -EINVAL; Perfect :-) > >> 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); > >> @@ -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) > >> break; > >> } > >> -- Regards, Laurent Pinchart