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
