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@xxxxxxxxxxxx>
> >> Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> ---
> >>
> >> 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
