Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

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

 




Hi Laurent,

On Wed, Aug 23, 2017 at 03:59:35PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wednesday, 23 August 2017 12:01:24 EEST Sakari Ailus wrote:
> > On Tue, Aug 22, 2017 at 03:52:33PM +0300, Laurent Pinchart wrote:
> > > On Friday, 18 August 2017 14:23:16 EEST Sakari Ailus wrote:
> > >> The current practice is that drivers iterate over their endpoints and
> > >> parse each endpoint separately. This is very similar in a number of
> > >> drivers, implement a generic function for the job. Driver specific
> > >> matters can be taken into account in the driver specific callback.
> > >> 
> > >> Convert the omap3isp as an example.
> > > 
> > > It would be nice to convert at least two drivers to show that the code can
> > > indeed be shared between multiple drivers. Even better, you could convert
> > > all drivers.
> > 
> > That's the intent in the long run. There's still no definite need to do
> > this in a single, big, hot patchset.
> 
> You don't need to convert them all in a single patch, but I'd still prefer 
> converting them all in a single patch series (and I'd split this patch in two 
> to make backporting easier). Otherwise we'll likely end up with multiple 
> competing implementations.

I don't think that presumption is founded. The fact that there is a driver
doing something on its own does not prevent add a helper function to the
framework which could be used to remove driver code. We have, as an
example, the pipeline power management code in v4l2-mc.c. It was introduced
with the omap3isp driver and is now used by a number of drivers.

This is also related preparation for supporting associations that will be
needed for lens and flash devices.

> 
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > >> ---
> > >> drivers/media/platform/omap3isp/isp.c | 116 ++++++++++------------------
> > >> drivers/media/v4l2-core/v4l2-fwnode.c | 125 +++++++++++++++++++++++++
> > >> include/media/v4l2-async.h            |   4 +-
> > >> include/media/v4l2-fwnode.h           |   9 +++
> > >> 4 files changed, 173 insertions(+), 81 deletions(-)
> > > 
> > > [snip]
> > > 
> > >> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > >> b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687310fe..cb0fc4b4e3bf
> > >> 100644
> > >> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > >> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > >> @@ -26,6 +26,7 @@
> > >>  #include <linux/string.h>
> > >>  #include <linux/types.h>
> > >> 
> > >> +#include <media/v4l2-async.h>
> > >>  #include <media/v4l2-fwnode.h>
> > >>  
> > >>  enum v4l2_fwnode_bus_type {
> > >> @@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link
> > >> *link) }
> > >> 
> > >>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> > >> 
> > >> +static int notifier_realloc(struct device *dev,
> > >> +			    struct v4l2_async_notifier *notifier,
> > >> +			    unsigned int max_subdevs)
> > > 
> > > It looks like you interpret the variable as an increment. You shouldn't
> > > call it max_subdevs in that case. I would however keep the name and pass
> > > the total number of subdevs instead of an increment, to mimic the realloc
> > > API.
> > 
> > Works for me.
> > 
> > >> +{
> > >> +	struct v4l2_async_subdev **subdevs;
> > >> +	unsigned int i;
> > >> +
> > >> +	if (max_subdevs + notifier->num_subdevs <= notifier->max_subdevs)
> > >> +		return 0;
> > >> +
> > >> +	subdevs = devm_kcalloc(
> > >> +		dev, max_subdevs + notifier->num_subdevs,
> > >> +		sizeof(*notifier->subdevs), GFP_KERNEL);
> > > 
> > > We know that we'll have to move away from devm_* allocation to fix object
> > > lifetime management, so we could as well start now.
> > 
> > The memory is in practice allocated using devm_() interface in existing
> > drivers.
> 
> Yes, and that's bad :-)
> 
> > The fact that it's in a single location makes it much easier getting rid of
> > it.
> 
> Great, so let's get rid of it :-)
> 
> > I'd rather get rid of memory allocation here in the first place, to be
> > replaced by a linked list. But first the user of notifier->subdevs in
> > drivers need to go. The framework interface doesn't need to change as a
> > result.
> 
> How are you going to allocate and free the linked list entries ?

Thinking about it some more --- we can't actually use other objects (not
allocated here) to hold the list struct, so we'll keep the allocation.

I think we'll need v4l2_async_notifier_release() as a result. I'll add that
to v2. This could be used to release resources related to a notifier which
is not yet registered.

> 
> > >> +	if (!subdevs)
> > >> +		return -ENOMEM;
> > >> +
> > >> +	if (notifier->subdevs) {
> > >> +		for (i = 0; i < notifier->num_subdevs; i++)
> > >> +			subdevs[i] = notifier->subdevs[i];
> > > 
> > > Is there a reason to use a loop here instead of a memcpy() covering the
> > > whole array ?
> > 
> > You could do that, yes, although I'd think this looks nicer. Performance is
> > hardly a concern here.
> 
> It's certainly not a performance issue, I would find the code easier to read.

Would you find

	memcpy(subdevs, notifier->subdevs, sizeof(*subdevs) * num_subdevs);

easier to read?

You also lose type checking as a result.

> 
> > >> +		devm_kfree(dev, notifier->subdevs);
> > >> +	}
> > >> +
> > >> +	notifier->subdevs = subdevs;
> > >> +	notifier->max_subdevs = max_subdevs + notifier->num_subdevs;
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static int __v4l2_fwnode_endpoint_parse(
> > >> +	struct device *dev, struct v4l2_async_notifier *notifier,
> > >> +	struct fwnode_handle *endpoint, struct v4l2_async_subdev *asd,
> > >> +	int (*parse_single)(struct device *dev,
> > >> +			    struct v4l2_fwnode_endpoint *vep,
> > >> +			    struct v4l2_async_subdev *asd))
> > >> +{
> > >> +	struct v4l2_fwnode_endpoint *vep;
> > >> +	int ret;
> > >> +
> > >> +	/* Ignore endpoints the parsing of which failed. */
> > > 
> > > Silently ignoring invalid DT sounds bad, I'd rather catch errors and
> > > return with an error code to make sure that DT gets fixed.
> > 
> > That would mean that if a single node is bad, none of the correct ones can
> > be used either. I'm not sure everyone would be happy about it.
> 
> We should be tolerant towards hardware failures and make as much of the device 
> usable as possible, but be very pedantic when parsing DT to catch errors as 
> early as possible.

How about shouting out loud about this, and continuing then?

> 
> > >> +	vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> > >> +	if (IS_ERR(vep))
> > >> +		return 0;
> > >> +
> > >> +	notifier->subdevs[notifier->num_subdevs] = asd;
> > >> +
> > >> +	ret = parse_single(dev, vep, asd);
> > >> +	v4l2_fwnode_endpoint_free(vep);
> > >> +	if (ret)
> > >> +		return ret;
> > >> +
> > >> +	asd->match.fwnode.fwnode =
> > >> +		fwnode_graph_get_remote_port_parent(endpoint);
> > >> +	if (!asd->match.fwnode.fwnode) {
> > >> +		dev_warn(dev, "bad remote port parent\n");
> > >> +		return -EINVAL;
> > >> +	}
> > >> +
> > >> +	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > >> +	notifier->num_subdevs++;
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +/**
> > >> + * v4l2_fwnode_endpoint_parse - Parse V4L2 fwnode endpoints in a device
> > >> node
> > > 
> > > This doesn't match the function name.
> > 
> > Will fix.
> > 
> > >> + * @dev: local struct device
> > > 
> > > Based on the documentation only and without a priori knowledge of the API,
> > > local struct device is very vague.
> > 
> > What would you use then?
> > 
> > Write a fairytale about it? :-)
> 
> Just explain which struct device is expected, and how it relates to the 
> purpose of the function and its other parameters. If you find that hard to 
> explain, imagine how confused someone trying to use the API without any a 
> priori knowledge of the subsystem will feel.

Having a good example is often more important than perfect documentation.
I'll add more documentation to the next version.

> 
> > >> + * @notifier: async notifier related to @dev
> > > 
> > > Ditto. You need more documentation, especially given that this is the
> > > first function in the core that fills a notifier from DT. You might also
> > > want to reflect that fact in the function name.
> > 
> > I can add more documentation.
> > 
> > >> + * @asd_struct_size: size of the driver's async sub-device struct,
> > >> including
> > >> + *		     sizeof(struct v4l2_async_subdev)
> > >> + * @parse_single: driver's callback function called on each V4L2 fwnode
> > >> endpoint
> > > 
> > > The parse_single return values should be documented.
> > 
> > Agreed.
> > 
> > >> + * Parse all V4L2 fwnode endpoints related to the device.
> > >> + *
> > >> + * Note that this function is intended for drivers to replace the
> > >> existing
> > >> + * implementation that loops over all ports and endpoints. It is NOT
> > >> INTENDED TO
> > >> + * BE USED BY NEW DRIVERS.
> > > 
> > > You should document what the preferred way is. And I'd much rather convert
> > > drivers to the preferred way instead of adding a helper function that is
> > > already deprecated.
> > 
> > The preferred way is not a part of this patch but the second one. This is
> > intended for moving the existing copies of the same code away from drivers.
> 
> You remove the code from a single driver here. How many drivers do we need to 
> convert ?

About 15, based on how many drivers use v4l2_async_notifier_register().

> 
> > The preferred way would be to explicitly check ports and endpoints in them
> > for connections. I'm not sure if the existing DT documentation is enough to
> > cover this for it does not generally document endpoint numbering.
> > 
> > >> + */
> > >> +int v4l2_fwnode_endpoints_parse(
> > > 
> > > v4l2_fwnode_parse_endpoints() would sound more natural.
> > 
> > We'll need to think more about naming this. v4l2_fwnode_endpoint_parse()
> > will parse a single endpoint and is entirely unaware of the notifier.
> > 
> > How about v4l2_async_notifier_parse_endpoints()? It's a big lengthy though.
> 
> And it's missing fwnode in the name :-)

Yes.

v4l2_async_notifier_parse_fwnode_endpoints()?

It'll still fit in practice. :-)

> 
> > >> +	struct device *dev, struct v4l2_async_notifier *notifier,
> > >> +	size_t asd_struct_size,
> > >> +	int (*parse_single)(struct device *dev,
> > >> +			    struct v4l2_fwnode_endpoint *vep,
> > >> +			    struct v4l2_async_subdev *asd))
> > >> +{
> > >> +	struct fwnode_handle *fwnode = NULL;
> > >> +	unsigned int max_subdevs = notifier->max_subdevs;
> > >> +	int ret;
> > >> +
> > >> +	if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> > >> +		return -EINVAL;
> > >> +
> > >> +	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
> > >> +							fwnode)))
> > >> +		max_subdevs++;
> > >> +
> > >> +	ret = notifier_realloc(dev, notifier, max_subdevs);
> > >> +	if (ret)
> > >> +		return ret;
> > >> +
> > >> +	for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> > >> +				     dev_fwnode(dev), fwnode)) &&
> > >> +		     !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);
> > > 
> > > It's nice to warn that the kernel will crash, but it would be even nicer
> > > to prevent the crash by returning an error instead of continuing parsing
> > > endpoints :-)
> > 
> > I'm not quite sure what do you mean. If the number of sub-devices reaches
> > what's allocated for them in the array, this will stop with a warning.
> 
> Oops, my bad, I've misread the code. Your for loop is not very readable :-/ 
> How about moving the num_subdevs test inside the loop with
> 
> 		if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> 			break;
> 
> That should make it more readable.

I can move it inside the loop.

> 
> > >> +		) {
> > >> +		struct v4l2_async_subdev *asd;
> > >> +
> > >> +		asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
> > >> +		if (!asd) {
> > >> +			ret = -ENOMEM;
> > >> +			goto error;
> > >> +		}
> > >> +
> > >> +		ret = __v4l2_fwnode_endpoint_parse(dev, notifier, fwnode, asd,
> > >> +						   parse_single);
> > >> +		if (ret < 0)
> > >> +			goto error;
> > >> +	}
> > >> +
> > >> +error:
> > >> +	fwnode_handle_put(fwnode);
> > >> +	return ret;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux