Re: [PATCH v2 17/29] media: v4l: Acquire a reference to the media device for every video device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 21, 2024 at 10:43:47AM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> Thank you for reviewing the set!
> 
> On Wed, Feb 07, 2024 at 01:13:44PM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 05, 2024 at 03:56:22PM +0100, Hans Verkuil wrote:
> > > On 20/12/2023 11:37, Sakari Ailus wrote:
> > > > The video device depends on the existence of its media device --- if there
> > > > is one. Acquire a reference to it.
> > > > 
> > > > Note that when the media device release callback is used, then the V4L2
> > > > device release callback is ignored and a warning is issued if both are
> > > > set.
> > 
> > Why is that ? The two are distinct objects, why can't they both have a
> > release function ?
> 
> You could, in principle, but in practice both of the structs are part of
> the same driver's device context struct which is a single allocation. You
> can only have a single release callback for it.

If both release callbacks freed the same data structure, that would
indeed be a problem. There could be other use cases though. For
instance, in the uvcvideo driver, the top-level structure is
reference-counted, and the release callbacks of the video devices
decrement that reference count. I don't expect drivers to do something
similar with media_device and v4l2_device, but I'm not sure if we should
forbid it completely. If we do, I would then rather deprecate the
release callback of v4l2_device completely.

> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-dev.c | 51 ++++++++++++++++++++----------
> > > >  1 file changed, 34 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > > index d13954bd31fd..c1e4995eaf5c 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > > @@ -176,6 +176,11 @@ static void v4l2_device_release(struct device *cd)
> > > >  {
> > > >  	struct video_device *vdev = to_video_device(cd);
> > > >  	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
> > > > +	bool v4l2_dev_has_release = v4l2_dev->release;
> > > > +#ifdef CONFIG_MEDIA_CONTROLLER
> > > > +	struct media_device *mdev = v4l2_dev->mdev;
> > > > +	bool mdev_has_release = mdev && mdev->ops && mdev->ops->release;
> > > > +#endif
> > > >  
> > > >  	mutex_lock(&videodev_lock);
> > > >  	if (WARN_ON(video_devices[vdev->minor] != vdev)) {
> > > > @@ -198,8 +203,8 @@ static void v4l2_device_release(struct device *cd)
> > > >  
> > > >  	mutex_unlock(&videodev_lock);
> > > >  
> > > > -#if defined(CONFIG_MEDIA_CONTROLLER)
> > > > -	if (v4l2_dev->mdev && vdev->vfl_dir != VFL_DIR_M2M) {
> > > > +#ifdef CONFIG_MEDIA_CONTROLLER
> > > > +	if (mdev && vdev->vfl_dir != VFL_DIR_M2M) {
> > > >  		/* Remove interfaces and interface links */
> > > >  		media_devnode_remove(vdev->intf_devnode);
> > > >  		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
> > > > @@ -207,23 +212,31 @@ static void v4l2_device_release(struct device *cd)
> > > >  	}
> > > >  #endif
> > > >  
> > > > -	/* Do not call v4l2_device_put if there is no release callback set.
> > > > -	 * Drivers that have no v4l2_device release callback might free the
> > > > -	 * v4l2_dev instance in the video_device release callback below, so we
> > > > -	 * must perform this check here.
> > > > -	 *
> > > > -	 * TODO: In the long run all drivers that use v4l2_device should use the
> > > > -	 * v4l2_device release callback. This check will then be unnecessary.
> > > > -	 */
> > > > -	if (v4l2_dev->release == NULL)
> > > > -		v4l2_dev = NULL;
> > > > -
> > > >  	/* Release video_device and perform other
> > > >  	   cleanups as needed. */
> > > >  	vdev->release(vdev);
> > > >  
> > > > -	/* Decrease v4l2_device refcount */
> > > > -	if (v4l2_dev)
> > > > +#ifdef CONFIG_MEDIA_CONTROLLER
> > > > +	if (mdev)
> > > > +		media_device_put(mdev);
> > > > +
> > > > +	/*
> > > > +	 * Generally both struct media_device and struct v4l2_device are
> > > > +	 * embedded in the same driver's context struct so having a release
> > > > +	 * callback in both is a bug.
> > > > +	 */
> > > > +	WARN_ON(v4l2_dev_has_release && mdev_has_release);
> > > 
> > > How about:
> > > 
> > > 	if (WARN_ON(v4l2_dev_has_release && mdev_has_release))
> > > 		v4l2_dev_has_release = false;
> > > 
> > > > +#endif
> > > > +
> > > > +	/*
> > > > +	 * Decrease v4l2_device refcount, but only if the media device doesn't
> > > > +	 * have a release callback.
> > > > +	 */
> > > > +	if (v4l2_dev_has_release
> > > > +#ifdef CONFIG_MEDIA_CONTROLLER
> > > > +	    && !mdev_has_release
> > > > +#endif
> > > > +	    )
> > > 
> > > Then this change is no longer needed.
> > > 
> > > General question: do we have drivers today that set both release functions?
> > > Because that would now cause a WARN in the kernel log with this patch.
> > > 
> > > >  		v4l2_device_put(v4l2_dev);
> > > >  }
> > > >  
> > > > @@ -792,11 +805,14 @@ static int video_register_media_controller(struct video_device *vdev)
> > > >  	u32 intf_type;
> > > >  	int ret;
> > > >  
> > > > -	/* Memory-to-memory devices are more complex and use
> > > > +	/*
> > > > +	 * Memory-to-memory devices are more complex and use
> > > >  	 * their own function to register its mc entities.
> > 
> > If you fix the comment style as a drive-by change, you could as well
> > reflow it to 80 columns.
> 
> I'll update this for the next version.
> 
> > > >  	 */
> > > > -	if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M)
> > > > +	if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M) {
> > > > +		media_device_get(vdev->v4l2_dev->mdev);
> > > >  		return 0;
> > > > +	}
> > > >  
> > > >  	vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
> > > >  	vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
> > > > @@ -875,6 +891,7 @@ static int video_register_media_controller(struct video_device *vdev)
> > > >  
> > > >  	/* FIXME: how to create the other interface links? */
> > > >  
> > > > +	media_device_get(vdev->v4l2_dev->mdev);
> > > >  #endif
> > > >  	return 0;
> > > >  }

-- 
Regards,

Laurent Pinchart




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux