Re: [media-workshop] [PATCH v8.4 81/83] [media] media-entity: init pads on entity init if was registered before

Re: [media-workshop] [PATCH v8.4 81/83] [media] media-entity: init pads on entity init if was registered before

[Laurent Pinchart]

Hi Javier,

(Replying to linux-media instead of media-workshop, I can't find this patch in 
my linux-media folder)

Thank you for the patch.

On Monday 12 October 2015 13:44:10 Mauro Carvalho Chehab wrote:
> From: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> If an entity is registered with a media device before is initialized
> with media_device_register_entity(), the number of pads won't be set
> so media_device_register_entity() won't create pad objects and add
> it to the media device pads list.
> 
> Do this at entity initialization time if the entity was registered
> before so the graph is complete and correct regardless of the order
> in which the entities are initialized and registered.

We now have two places where the pads graph objects are initialized and that 
looks like a bug to me as media_gobj_init() allocates IDs and, even worse, 
adds the entity to the media device entities list.

We need to standardize on where graph objects are initialized across the 
different kind of objects and document this clearly otherwise drivers will get 
it wrong.

> Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>  drivers/media/media-entity.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index f28265864b76..2c984fb7d497 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -238,6 +238,7 @@ int
>  media_entity_init(struct media_entity *entity, u16 num_pads,
>  		  struct media_pad *pads)
>  {
> +	struct media_device *mdev = entity->graph_obj.mdev;
>  	unsigned int i;
> 
>  	entity->group_id = 0;
> @@ -246,11 +247,20 @@ media_entity_init(struct media_entity *entity, u16
> num_pads, entity->num_pads = num_pads;
>  	entity->pads = pads;
> 
> +	if (mdev)
> +		spin_lock(&mdev->lock);
> +
>  	for (i = 0; i < num_pads; i++) {
>  		pads[i].entity = entity;
>  		pads[i].index = i;
> +		if (mdev)
> +			media_gobj_init(mdev, MEDIA_GRAPH_PAD,
> +					&entity->pads[i].graph_obj);
>  	}
> 
> +	if (mdev)
> +		spin_unlock(&mdev->lock);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(media_entity_init);

-- 
Regards,

Laurent Pinchart

Re: [media-workshop] [PATCH v8.4 81/83] [media] media-entity: init pads on entity init if was registered before

[Javier Martinez Canillas]

Hello Laurent,

On 11/23/2015 01:20 PM, Laurent Pinchart wrote:
> Hi Javier,
> 
> (Replying to linux-media instead of media-workshop, I can't find this patch in 
> my linux-media folder)
>
> Thank you for the patch.
>

Thanks for your feedback.
 
> On Monday 12 October 2015 13:44:10 Mauro Carvalho Chehab wrote:
>> From: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> If an entity is registered with a media device before is initialized
>> with media_device_register_entity(), the number of pads won't be set
>> so media_device_register_entity() won't create pad objects and add
>> it to the media device pads list.
>>
>> Do this at entity initialization time if the entity was registered
>> before so the graph is complete and correct regardless of the order
>> in which the entities are initialized and registered.
> 
> We now have two places where the pads graph objects are initialized and that 
> looks like a bug to me as media_gobj_init() allocates IDs and, even worse, 
> adds the entity to the media device entities list.
>

Yes but the idea of this patch was in fact to make it less error prone and
more robust since entities could either be initialized and later registered
or first be registered and then later initialized.
 
> We need to standardize on where graph objects are initialized across the 
> different kind of objects and document this clearly otherwise drivers will get 
> it wrong.
>

I'm OK with having more strict rules of the order in which objects should
be initialized and registered and force all drivers to follow these docs.

>> Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

This patch was suggested by Mauro so I'll also let him to comment what he
prefers or if he has another opinion on this.

>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>  drivers/media/media-entity.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>> index f28265864b76..2c984fb7d497 100644
>> --- a/drivers/media/media-entity.c
>> +++ b/drivers/media/media-entity.c
>> @@ -238,6 +238,7 @@ int
>>  media_entity_init(struct media_entity *entity, u16 num_pads,
>>  		  struct media_pad *pads)
>>  {
>> +	struct media_device *mdev = entity->graph_obj.mdev;
>>  	unsigned int i;
>>
>>  	entity->group_id = 0;
>> @@ -246,11 +247,20 @@ media_entity_init(struct media_entity *entity, u16
>> num_pads, entity->num_pads = num_pads;
>>  	entity->pads = pads;
>>
>> +	if (mdev)
>> +		spin_lock(&mdev->lock);
>> +
>>  	for (i = 0; i < num_pads; i++) {
>>  		pads[i].entity = entity;
>>  		pads[i].index = i;
>> +		if (mdev)
>> +			media_gobj_init(mdev, MEDIA_GRAPH_PAD,
>> +					&entity->pads[i].graph_obj);
>>  	}
>>
>> +	if (mdev)
>> +		spin_unlock(&mdev->lock);
>> +
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(media_entity_init);
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [media-workshop] [PATCH v8.4 81/83] [media] media-entity: init pads on entity init if was registered before

[Mauro Carvalho Chehab]

Em Mon, 23 Nov 2015 17:21:22 -0300
Javier Martinez Canillas <javier@osg.samsung.com> escreveu:

> Hello Laurent,
> 
> On 11/23/2015 01:20 PM, Laurent Pinchart wrote:
> > Hi Javier,
> > 
> > (Replying to linux-media instead of media-workshop, I can't find this patch in 
> > my linux-media folder)
> >
> > Thank you for the patch.
> >
> 
> Thanks for your feedback.
>  
> > On Monday 12 October 2015 13:44:10 Mauro Carvalho Chehab wrote:
> >> From: Javier Martinez Canillas <javier@osg.samsung.com>
> >>
> >> If an entity is registered with a media device before is initialized
> >> with media_device_register_entity(), the number of pads won't be set
> >> so media_device_register_entity() won't create pad objects and add
> >> it to the media device pads list.
> >>
> >> Do this at entity initialization time if the entity was registered
> >> before so the graph is complete and correct regardless of the order
> >> in which the entities are initialized and registered.
> > 
> > We now have two places where the pads graph objects are initialized and that 
> > looks like a bug to me as media_gobj_init() allocates IDs and, even worse, 
> > adds the entity to the media device entities list.
> >

As Sakari commented on one of his reviews, media_gobj_init() should
actually be renamed to media_gobj_create(), as it is the function that
creates and registers a graph object on a given media device.

The hole idea is that any kind of graph object should be created at one
single point: media_gobj_init - to be renamed to media_gobj_create(), as
proposed by Sakari.

There's indeed bug on the existing subdev drivers: they want to create
pads and links without even knowing to what media_device those will be
associated! Every driver should be doing, instead:

1) create the media_device (or get a reference to an already created one);
2) create the entities associated with the mdev;
3) create the pads associated with the already created entity(ies);
4) register the entity;
5) create the links associated with two already created pads.

The above workflow is the correct one, as all graph objects should
be added to the mdev graph object lists that will be used by the
ioctl handler.

Also, please notice that, if all drivers follow the above procedure,
we could also merge entity create and register functions into one,
with is, IMHO, a good thing, as entity creation/registration will
be atomic.

However, several subdevs do, instead:

1) create entity(ies);
2) create pads;
3) get a reference for the media device;
4) register the entity;
5) create the links associated with two already created pads.

Changing such logic is not trivial, and require tests with devices
that we don't have.

So, the approach we took is to allow the above workflow to keep
working. That's what this patch does.

If you think it is worth to push for a change on those drivers, we
could eventually add a printk_once() to warn that the driver is
doing the wrong thing, and give some time for the drivers maintainers
to fix their drivers, and remove the backward compat code after done.

> Yes but the idea of this patch was in fact to make it less error prone and
> more robust since entities could either be initialized and later registered
> or first be registered and then later initialized.
>  
> > We need to standardize on where graph objects are initialized across the 
> > different kind of objects and document this clearly otherwise drivers will get 
> > it wrong.
> >
> 
> I'm OK with having more strict rules of the order in which objects should
> be initialized and registered and force all drivers to follow these docs.
> 
> >> Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> This patch was suggested by Mauro so I'll also let him to comment what he
> prefers or if he has another opinion on this.
> 
> >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> >> ---
> >>  drivers/media/media-entity.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> >> index f28265864b76..2c984fb7d497 100644
> >> --- a/drivers/media/media-entity.c
> >> +++ b/drivers/media/media-entity.c
> >> @@ -238,6 +238,7 @@ int
> >>  media_entity_init(struct media_entity *entity, u16 num_pads,
> >>  		  struct media_pad *pads)
> >>  {
> >> +	struct media_device *mdev = entity->graph_obj.mdev;
> >>  	unsigned int i;
> >>
> >>  	entity->group_id = 0;
> >> @@ -246,11 +247,20 @@ media_entity_init(struct media_entity *entity, u16
> >> num_pads, entity->num_pads = num_pads;
> >>  	entity->pads = pads;
> >>
> >> +	if (mdev)
> >> +		spin_lock(&mdev->lock);
> >> +
> >>  	for (i = 0; i < num_pads; i++) {
> >>  		pads[i].entity = entity;
> >>  		pads[i].index = i;
> >> +		if (mdev)
> >> +			media_gobj_init(mdev, MEDIA_GRAPH_PAD,
> >> +					&entity->pads[i].graph_obj);
> >>  	}
> >>
> >> +	if (mdev)
> >> +		spin_unlock(&mdev->lock);
> >> +
> >>  	return 0;
> >>  }
> >>  EXPORT_SYMBOL_GPL(media_entity_init);
> > 
> 
> Best regards,

Re: [media-workshop] [PATCH v8.4 81/83] [media] media-entity: init pads on entity init if was registered before

[Sakari Ailus]

Hi Mauro and Laurent,

On Tue, Nov 24, 2015 at 08:22:24AM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 23 Nov 2015 17:21:22 -0300
> Javier Martinez Canillas <javier@osg.samsung.com> escreveu:
> 
> > Hello Laurent,
> > 
> > On 11/23/2015 01:20 PM, Laurent Pinchart wrote:
> > > Hi Javier,
> > > 
> > > (Replying to linux-media instead of media-workshop, I can't find this patch in 
> > > my linux-media folder)
> > >
> > > Thank you for the patch.
> > >
> > 
> > Thanks for your feedback.
> >  
> > > On Monday 12 October 2015 13:44:10 Mauro Carvalho Chehab wrote:
> > >> From: Javier Martinez Canillas <javier@osg.samsung.com>
> > >>
> > >> If an entity is registered with a media device before is initialized
> > >> with media_device_register_entity(), the number of pads won't be set
> > >> so media_device_register_entity() won't create pad objects and add
> > >> it to the media device pads list.
> > >>
> > >> Do this at entity initialization time if the entity was registered
> > >> before so the graph is complete and correct regardless of the order
> > >> in which the entities are initialized and registered.
> > > 
> > > We now have two places where the pads graph objects are initialized and that 
> > > looks like a bug to me as media_gobj_init() allocates IDs and, even worse, 
> > > adds the entity to the media device entities list.
> > >
> 
> As Sakari commented on one of his reviews, media_gobj_init() should
> actually be renamed to media_gobj_create(), as it is the function that
> creates and registers a graph object on a given media device.
> 
> The hole idea is that any kind of graph object should be created at one
> single point: media_gobj_init - to be renamed to media_gobj_create(), as
> proposed by Sakari.

I think I proposed "register"; what I indeed wanted to underline with that
was that it was added to an external data structure, and cannot be released
without that reference being removed.

"init" or "create" lack that suggestion, well, with the exception of
possible memory allocation. But that's not relevant to the problem at
hand...

> 
> There's indeed bug on the existing subdev drivers: they want to create
> pads and links without even knowing to what media_device those will be
> associated! Every driver should be doing, instead:

This used to be at least allowed if not the preferred until recently. But I
agree we need to agree on the rules; earlier drivers just did what they
wanted to do. :-)

> 
> 1) create the media_device (or get a reference to an already created one);
> 2) create the entities associated with the mdev;
> 3) create the pads associated with the already created entity(ies);
> 4) register the entity;
> 5) create the links associated with two already created pads.
> 
> The above workflow is the correct one, as all graph objects should
> be added to the mdev graph object lists that will be used by the
> ioctl handler.
> 
> Also, please notice that, if all drivers follow the above procedure,
> we could also merge entity create and register functions into one,
> with is, IMHO, a good thing, as entity creation/registration will
> be atomic.
> 
> However, several subdevs do, instead:
> 
> 1) create entity(ies);
> 2) create pads;
> 3) get a reference for the media device;
> 4) register the entity;
> 5) create the links associated with two already created pads.
> 
> Changing such logic is not trivial, and require tests with devices
> that we don't have.
> 
> So, the approach we took is to allow the above workflow to keep
> working. That's what this patch does.
> 
> If you think it is worth to push for a change on those drivers, we
> could eventually add a printk_once() to warn that the driver is
> doing the wrong thing, and give some time for the drivers maintainers
> to fix their drivers, and remove the backward compat code after done.

Sounds good to me. Such warnings tend to be a lot more efficient than
changing the documentation, whether in code or elsewhere.

> > Yes but the idea of this patch was in fact to make it less error prone and
> > more robust since entities could either be initialized and later registered
> > or first be registered and then later initialized.
> >  
> > > We need to standardize on where graph objects are initialized across the 
> > > different kind of objects and document this clearly otherwise drivers will get 
> > > it wrong.
> > >
> > 
> > I'm OK with having more strict rules of the order in which objects should
> > be initialized and registered and force all drivers to follow these docs.
> > 
> > >> Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > 
> > This patch was suggested by Mauro so I'll also let him to comment what he
> > prefers or if he has another opinion on this.
> > 
> > >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> > >> ---
> > >>  drivers/media/media-entity.c | 10 ++++++++++
> > >>  1 file changed, 10 insertions(+)
> > >>
> > >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > >> index f28265864b76..2c984fb7d497 100644
> > >> --- a/drivers/media/media-entity.c
> > >> +++ b/drivers/media/media-entity.c
> > >> @@ -238,6 +238,7 @@ int
> > >>  media_entity_init(struct media_entity *entity, u16 num_pads,
> > >>  		  struct media_pad *pads)
> > >>  {
> > >> +	struct media_device *mdev = entity->graph_obj.mdev;
> > >>  	unsigned int i;
> > >>
> > >>  	entity->group_id = 0;
> > >> @@ -246,11 +247,20 @@ media_entity_init(struct media_entity *entity, u16
> > >> num_pads, entity->num_pads = num_pads;
> > >>  	entity->pads = pads;
> > >>
> > >> +	if (mdev)
> > >> +		spin_lock(&mdev->lock);
> > >> +
> > >>  	for (i = 0; i < num_pads; i++) {
> > >>  		pads[i].entity = entity;
> > >>  		pads[i].index = i;
> > >> +		if (mdev)
> > >> +			media_gobj_init(mdev, MEDIA_GRAPH_PAD,
> > >> +					&entity->pads[i].graph_obj);
> > >>  	}
> > >>
> > >> +	if (mdev)
> > >> +		spin_unlock(&mdev->lock);
> > >> +
> > >>  	return 0;
> > >>  }
> > >>  EXPORT_SYMBOL_GPL(media_entity_init);
> > > 
> > 
> > Best regards,

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [media-workshop] [PATCH v8.4 43/83] [media] v4l2-subdev: use MEDIA_ENT_T_UNKNOWN for new subdevs

[Laurent Pinchart]

Hello,

CC'ing the linux-media mailing list, for real this time.

On Sunday 08 November 2015 00:02:55 Sakari Ailus wrote:
> On Wed, Oct 14, 2015 at 06:35:48PM -0300, Mauro Carvalho Chehab wrote:
> > Em Wed, 14 Oct 2015 13:15:40 +0300 Sakari Ailus escreveu:
> >> On Mon, Oct 12, 2015 at 09:26:04PM -0300, Mauro Carvalho Chehab wrote:
> >>> Em Tue, 13 Oct 2015 01:25:35 +0300 Sakari Ailus escreveu:
> >>>> On Mon, Oct 12, 2015 at 01:43:32PM -0300, Mauro Carvalho Chehab wrote:
> >>>>> Instead of abusing MEDIA_ENT_T_V4L2_SUBDEV, initialize
> >>>>> new subdev entities as MEDIA_ENT_T_UNKNOWN.
> >>>>> 
> >>>>> Change-Id: I294ee20f49b6c40dd95339d6730d90fa85b0dea9
> >>>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >>>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/media/media-device.c          |  6 ++++++
> >>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  2 +-
> >>>>>  include/uapi/linux/media.h            | 17 +++++++++++++++++
> >>>>>  3 files changed, 24 insertions(+), 1 deletion(-)
> >>>>> 
> >>>>> diff --git a/drivers/media/media-device.c
> >>>>> b/drivers/media/media-device.c
> >>>>> index 659507bce63f..134fe7510195 100644
> >>>>> --- a/drivers/media/media-device.c
> >>>>> +++ b/drivers/media/media-device.c
> >>>>> @@ -435,6 +435,12 @@ int __must_check
> >>>>> media_device_register_entity(struct media_device *mdev,
> >>>>>  {
> >>>>>  	int i;
> >>>>> 
> >>>>> +	if (entity->type == MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN ||
> >>>>> +	    entity->type == MEDIA_ENT_T_UNKNOWN)
> >>>>> +		dev_warn(mdev->dev,
> >>>>> +			 "Entity type for entity %s was not initialized!\n",
> >>>>> +			 entity->name);

First of all the subject of the patch is very misleading as you initialize the 
entity type for new subdevs to MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN, not 
MEDIA_ENT_T_UNKNOWN.

> >>>> I don't think I'd warn about MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN ---
> >>>> there are entities that do not fall into any category of existing
> >>>> functions. For instance image signal processors have such; they are
> >>>> hardware specific and often their functionality is somewhat so as
> >>>> well. Some of them perform a variety of functions (image processing
> >>>> algorithms) but I doubt it'd make sense to start e.g. listing those
> >>>> until we have any standardised interface for them.

Most of the subdevs we have today are of the "unknown" type, which isn't very 
user-friendly. Part of the reason is that there has never been a big incentive 
for driver writes to add proper subdev types, as the type is ignored in 
userspace in most cases. This should hopefully change with functions, and 
we'll need to push for new subdevs to have at least one function defined, even 
if it's a generic function such as image processing for lack of a better 
alternative. Warning on MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN has the advantage that 
it will push driver authors to think about the issue instead of just ignoring 
it and setting the type/function to unknown. That might be a wrong solution 
though, as if we introduce a generic image processing function (which we'll 
need for lack of a better or more precise alternative in some cases) driver 
authors might just use that one instead of MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN, 
and it would become the equivalent of the unknown type for all practical 
purpose. We would only have pushed the problem one step further without 
solving it. I wonder how we could improve that.

> >>>> The two entities in smiapp don't have a specific function either.
> >>>> Adding a new one (scaler) might make sense for the two, but I think
> >>>> I'd leave that out from this set.
> >>> 
> >>> IMHO, if the entity function is really unknown, it shouldn't even be
> >>> at the graph in the first place, as an unknown entity can't be
> >>> controlled.
> >> 
> >> These used to be just "sub-devices" without a type that 1) did not fall
> >> into any existing category
> > 
> > No problem. You can create a new function
> > 
> >> and 2) was not generic enough to warrant adding a
> >> specific type for them. I don't think that has really changed with
> >> functions.
> >
> > Well, you could add a device-specific function name. We have already
> > other device specific things (like device-only FOURCCs, device-specific
> > controls). I don't see why not having device-specific functions when
> > we want/need to map such entities.
> 
> What would be the benefit of having device specific functions?
> 
> The user who would need to access such a device would probably use the name
> instead, probably combined with the bus information in the future (or serial
> number etc.).
> 
> >>> So, I think we should either add a new function for those entities,
> >>> for them to be used on userspace, or simply remove them, if they won't
> >>> be used on userspace, because they aren't documented.
> >> 
> >> What kind of "function" could you use for e.g. OMAP3ISP CCDC or preview
> >> blocks?
> > 
> > ENT_F_OMAP3ISP_CCDC
> > 
> > if the preview is generic enough, it could be ENT_F_PREVIEW. If not,
> > ENT_F_OMAP3ISP_PREVIEW.
> > 
> >> The issue is that for the user to meaningfully use the devices, one has
> >> to know exactly what they are. The fact they do "image processing" for
> >> instance is not really useful alone.
> >> 
> >> Flash devices, for instance, have a well defined control interface.
> > 
> > Yes, but a ENT_F_OMAP3_CCDC would also have a well defined control
> > interface.
> 
> ..and its private IOCTLs as well.
> 
> I'd like to have Laurent's and Hans's opinion on this.

I don't think it makes sense to create a device-specific function for those 
subdevs. Functions should be defined generically, not in a hardware-specific 
fashion. Otherwise we would mix two very different usages, the identification 
of what an entity does and the identification of what an entity is. Locating 
an entity precisely in the graph ("where is the OMAP3 ISP CCDC entity?") is 
the purpose of the entity name. Using ENT_F_OMAP3_CCDC would mean "the OMAP3 
ISP CCDC entity does ENT_F_OMAP3_CCDC, defined as whatever the OMAP3 ISP CCDC 
entity does". That sounds tautological to me. I'd rather know that "the OMAP3 
ISP CCDC entity does image processing", or possibly "the OMAP3 ISP CCDC entity 
does black level compensation, fault pixel correction and lens shading 
correction".

> We'll also start needing multiple functions per entity in this case, since
> existing device specific functions would need to be amended with
> standardised functions. Supposing functions can be device specific, I think
> functions such as "scaler" should be standardised as well.

I agree with both statements, we'll need multiple functions per entity, and 
the scaler function should be standardized.

-- 
Regards,

Laurent Pinchart