Re: [PATCH v11 28/32] rcar-vin: add link notify for Gen3

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

 



Hi Laurent,

Thanks for your feedback.

On 2018-03-02 14:00:19 +0200, Laurent Pinchart wrote:

[snip]

> > +
> > +	/* If any entity is in use don't allow link changes. */
> > +	media_device_for_each_entity(entity, &group->mdev)
> > +		if (entity->use_count)
> > +			return -EBUSY;
> 
> This means that you disallow link setup when any video node is open. According 
> to the comment above, isn't it stream_count that you want to check ? If so the 
> MC core does it for you (unless you create links with the MEDIA_LNK_FL_DYNAMIC 
> flag set), see __media_entity_setup_link().

You are correct that the comment and code don't align. I rather update 
the comment and keep denying link enablement if any video devices are 
open. I'm sure this is not a real issue but this group concept feels a 
bit fragile, so better safe then sorry. Or do you feel there is a 
benefit for the user to be able to change the graph with video nodes 
open? We can always loosen the constraint later if it becomes a problem 
but introducing it if we would need it could be considered a regression?


> 
> > +	mutex_lock(&group->lock);
> > +
> > +	/*
> > +	 * Figure out which VIN the link concern's and lookup
> > +	 * which master VIN controls the routes for that VIN.
> > +	 */
> 
> Can't you simply use a container_of to cast from vdev to vin ?

Thank you for this, made the code much more readable!

> 
> > +	vdev = media_entity_to_video_device(link->sink->entity);
> > +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> > +		if (group->vin[i] && &group->vin[i]->vdev == vdev) {
> > +			vin = group->vin[i];
> > +			master_id = rvin_group_id_to_master(vin->id);
> > +			break;
> > +		}
> > +	}

[snip]

-- 
Regards,
Niklas Söderlund



[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