Re: [PATCH v9 21/28] rcar-vin: add group allocator functions

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

On Monday, 8 January 2018 19:24:47 EET Niklas Söderlund wrote:
> On 2017-12-08 22:12:56 +0200, Laurent Pinchart wrote:
> > On Friday, 8 December 2017 03:08:35 EET Niklas Söderlund wrote:
> >> In media controller mode all VIN instances needs to be part of the same
> >> media graph. There is also a need to each VIN instance to know and in
> >> some cases be able to communicate with other VIN instances.
> >> 
> >> Add an allocator framework where the first VIN instance to be probed
> >> creates a shared data structure and creates a media device.
> >> 
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >> 
> >>  drivers/media/platform/rcar-vin/rcar-core.c | 179 +++++++++++++++++++++-
> >>  drivers/media/platform/rcar-vin/rcar-vin.h  |  38 ++++++
> >>  2 files changed, 215 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> >> b/drivers/media/platform/rcar-vin/rcar-core.c index
> >> 45de4079fd835759..a6713fd61dd87a88 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-core.c

[snip]

> >> +static struct rvin_group *__rvin_group_allocate(struct rvin_dev *vin)
> >> +{
> >> +	struct rvin_group *group;
> >> +
> >> +	if (rvin_group_data) {
> >> +		group = rvin_group_data;
> >> +		kref_get(&group->refcount);
> >> +		vin_dbg(vin, "%s: get group=%p\n", __func__, group);
> >> +		return group;
> >> +	}
> >> +
> >> +	group = kzalloc(sizeof(*group), GFP_KERNEL);
> >> +	if (!group)
> >> +		return NULL;
> >> +
> >> +	kref_init(&group->refcount);
> >> +	rvin_group_data = group;
> > 
> > Ouch. While I agree with the global mutex, a single global group variable
> > reminds me of the days when per-device data was happily stored in global
> > variables because, you know, we will never have more than one instance of
> > that device, right ? (Or, sometimes, because the driver author didn't
> > know what an instance was.)
> > 
> > Ideally we'd want a linked list of groups, and this function would either
> > retrieve the group that the VIN instance is part of, or allocate a new
> > one.
> 
> I agree that removing the global group variable would be for the better,
> and I was hoping to be able to use the device allocator API for this
> once it's ready. However for now on all supported hardware (I know of)
> there would only be one group for the whole system.
> 
> - On H3 and M3-W CSI20 is connected to all VINs.
> - On V3M there is only one CSI40 and it is connected to all VINs.
> 
> Is this satisfactory for you or do you believe it would be better to
> complete the device allocator first. Or implement parts of the ideas
> from that API locally to this driver? If I understand things correctly
> there are still obstacles blocking that API which are not trivial. And
> even if they are cleared it have no DT support AFIK so functionality to
> iterate over all nodes in a graph would be needed which in itself is no
> trivial task.

I don't think we need to wait for the API to be finalized. My dislike for 
global variables pushes me to ask for a local implementation based on a 
linked-list, but that would be a bit overkill given that all platforms will 
have a single group. I'm OK with your current implementation, but please add a 
FIXME comment that explains that the single global pointer should really be a 
linked list, and that it should eventually be replaced by usage of a global 
device allocator API.

> >> +	vin_dbg(vin, "%s: alloc group=%p\n", __func__, group);
> > 
> > Do you still need those two debug statements (and all of the other ones
> > below) ?
> 
> Wops, no they can be dropped.
> 
> >> +	return group;
> >> +}
> >> +
> >> +static int rvin_group_add_vin(struct rvin_dev *vin)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = rvin_group_read_id(vin, vin->dev->of_node);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	mutex_lock(&vin->group->lock);
> >> +
> >> +	if (vin->group->vin[ret]) {
> >> +		mutex_unlock(&vin->group->lock);
> >> +		vin_err(vin, "VIN number %d already occupied\n", ret);
> >> +		return -EINVAL;
> > 
> > Can this happen ?
> 
> Sure, the values are read from DT so if the renesas,id property have the
> same value for more then one node. This is a incorrect DT of course but
> better to handle and warn for such a case I think?

Good point. How about phrasing the error as "Duplicate renesas,id %u" or even 
"Duplicate renesas,id property value %u" ? That would more clearly point to a 
DT error.

> >> +	}
> >> +
> >> +	vin->group->vin[ret] = vin;
> >> +
> >> +	mutex_unlock(&vin->group->lock);
> >> +
> >> +	vin_dbg(vin, "I'm VIN number %d", ret);
> >> +
> >> +	return 0;
> >> +}

[snip]

-- 
Regards,

Laurent Pinchart