Re: [PATCH v8 16/55] [media] media: Don't accept early-created links

2015-12-05 Thread Laurent Pinchart 
Hi Mauro,

Thank you for the patch.

On Sunday 06 September 2015 09:02:47 Mauro Carvalho Chehab wrote:
> Links are graph objects that represent the links of two already
> existing objects in the graph.
> 
> While with the current implementation, it is possible to create
> the links earlier, It doesn't make any sense to allow linking
> two objects when they are not both created.
> 
> So, remove the code that would be handling those early-created
> links and add a BUG_ON() to ensure that.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> Acked-by: Hans Verkuil 
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 138b18416460..0d85c6c28004 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -443,13 +443,6 @@ int __must_check media_device_register_entity(struct
> media_device *mdev, media_gobj_init(mdev, MEDIA_GRAPH_ENTITY,
> >graph_obj);
>   list_add_tail(>list, >entities);
> 
> - /*
> -  * Initialize objects at the links
> -  * in the case where links got created before entity register
> -  */
> - for (i = 0; i < entity->num_links; i++)
> - media_gobj_init(mdev, MEDIA_GRAPH_LINK,
> - >links[i].graph_obj);
>   /* Initialize objects at the pads */
>   for (i = 0; i < entity->num_pads; i++)
>   media_gobj_init(mdev, MEDIA_GRAPH_PAD,
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 27fce6224972..0926f08be981 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -161,6 +161,8 @@ void media_gobj_init(struct media_device *mdev,
>  enum media_gobj_type type,
>  struct media_gobj *gobj)
>  {
> + BUG_ON(!mdev);
> +

Please use a WARN_ON() and return (and possibly make the function return an 
int error code), we don't want to panic completely for this.

>   gobj->mdev = mdev;
> 
>   /* Create a per-type unique object ID */

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 16/55] [media] media: Don't accept early-created links

2015-09-06 Thread Mauro Carvalho Chehab 
Links are graph objects that represent the links of two already
existing objects in the graph.

While with the current implementation, it is possible to create
the links earlier, It doesn't make any sense to allow linking
two objects when they are not both created.

So, remove the code that would be handling those early-created
links and add a BUG_ON() to ensure that.

Signed-off-by: Mauro Carvalho Chehab 
Acked-by: Hans Verkuil 

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 138b18416460..0d85c6c28004 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -443,13 +443,6 @@ int __must_check media_device_register_entity(struct 
media_device *mdev,
media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, >graph_obj);
list_add_tail(>list, >entities);
 
-   /*
-* Initialize objects at the links
-* in the case where links got created before entity register
-*/
-   for (i = 0; i < entity->num_links; i++)
-   media_gobj_init(mdev, MEDIA_GRAPH_LINK,
-   >links[i].graph_obj);
/* Initialize objects at the pads */
for (i = 0; i < entity->num_pads; i++)
media_gobj_init(mdev, MEDIA_GRAPH_PAD,
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 27fce6224972..0926f08be981 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -161,6 +161,8 @@ void media_gobj_init(struct media_device *mdev,
   enum media_gobj_type type,
   struct media_gobj *gobj)
 {
+   BUG_ON(!mdev);
+
gobj->mdev = mdev;
 
/* Create a per-type unique object ID */
-- 
2.4.3


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 16/55] [media] media: Don't accept early-created links

2015-08-31 Thread Mauro Carvalho Chehab 
Em Mon, 31 Aug 2015 16:39:34 +0200
Javier Martinez Canillas  escreveu:

> Hello Hans,
> 
> On 08/31/2015 01:01 PM, Hans Verkuil wrote:
> > On 08/31/2015 12:54 PM, Mauro Carvalho Chehab wrote:
> >> Em Mon, 31 Aug 2015 12:30:16 +0200
> >> Hans Verkuil  escreveu:
> >>
> >>> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
>  Links are graph objects that represent the links of two already
>  existing objects in the graph.
> 
>  While with the current implementation, it is possible to create
>  the links earlier, It doesn't make any sense to allow linking
>  two objects when they are not both created.
> 
>  So, remove the code that would be handling those early-created
>  links and add a BUG_ON() to ensure that.
> 
>  Signed-off-by: Mauro Carvalho Chehab 
> >>>
> >>> The code is OK, so:
> >>>
> >>> Acked-by: Hans Verkuil 
> >>>
> >>> But shouldn't this go *after* the omap3isp fixes? After this patch the
> >>> omap3isp will call BUG_ON, and that's not what you want.
> >>
> >> Yes. I'll change the order on my git tree.
> >>
> >>> It is also not clear if the omap3isp driver is the only one that has this
> >>> 'create link before objects' problem. I would expect that the omap4 
> >>> staging
> >>> driver has the same issue and possibly others as well.
> >>>
> >>> Did someone look at that?
> >>
> >> I guess other drivers are doing the same.
> >>
> >> Javier's planning to review the other platform drivers in order to add the 
> >> needed fixes there too, and to do more tests with some other platform
> >> drivers that he has hardware for testing.
> > 
> > OK, good. Just wanted to know that.
> >
> 
> Yes, the omap4iss driver in staging has a similar logic and thus the
> same issues than the omap3isp driver. I'll write patches for this as
> well but I don't have an OMAP4 board here so testing is appreciated.

Well, I can try to test it here on my PandaBoard. Not sure if it will work,
as I think that the sensor connector was not properly soldered.

> 
> As Mauro mentioned, I was reviewing the others driver that are creating
> pad links and found more that are creating links before registering:
> 
> drivers/media/usb/uvc
> drivers/media/platform/vsp1
> drivers/media/i2c/smiapp
> 
> I'll try to take care of these too.
> 
> The other drivers that are creating pad links are doing the right thing
> AFAICT by registering the entity with the media device before.
> 
> > Perhaps it is a good idea to add a TODO list to the cover letter. This would
> > be one item on that list.
> >
> 
> A TODO list is a good idea indeed.
>  
> > Regards,
> > 
> > Hans
> > 
> 
> Best regards,
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 16/55] [media] media: Don't accept early-created links

2015-08-31 Thread Hans Verkuil 
On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> Links are graph objects that represent the links of two already
> existing objects in the graph.
> 
> While with the current implementation, it is possible to create
> the links earlier, It doesn't make any sense to allow linking
> two objects when they are not both created.
> 
> So, remove the code that would be handling those early-created
> links and add a BUG_ON() to ensure that.
> 
> Signed-off-by: Mauro Carvalho Chehab 

The code is OK, so:

Acked-by: Hans Verkuil 

But shouldn't this go *after* the omap3isp fixes? After this patch the
omap3isp will call BUG_ON, and that's not what you want.

It is also not clear if the omap3isp driver is the only one that has this
'create link before objects' problem. I would expect that the omap4 staging
driver has the same issue and possibly others as well.

Did someone look at that?

Regards,

Hans

> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 138b18416460..0d85c6c28004 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -443,13 +443,6 @@ int __must_check media_device_register_entity(struct 
> media_device *mdev,
>   media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, >graph_obj);
>   list_add_tail(>list, >entities);
>  
> - /*
> -  * Initialize objects at the links
> -  * in the case where links got created before entity register
> -  */
> - for (i = 0; i < entity->num_links; i++)
> - media_gobj_init(mdev, MEDIA_GRAPH_LINK,
> - >links[i].graph_obj);
>   /* Initialize objects at the pads */
>   for (i = 0; i < entity->num_pads; i++)
>   media_gobj_init(mdev, MEDIA_GRAPH_PAD,
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 01946baa32d5..9f8e0145db7a 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -161,6 +161,8 @@ void media_gobj_init(struct media_device *mdev,
>  enum media_gobj_type type,
>  struct media_gobj *gobj)
>  {
> + BUG_ON(!mdev);
> +
>   gobj->mdev = mdev;
>  
>   /* Create a per-type unique object ID */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 16/55] [media] media: Don't accept early-created links

2015-08-31 Thread Mauro Carvalho Chehab 
Em Mon, 31 Aug 2015 12:30:16 +0200
Hans Verkuil  escreveu:

> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> > Links are graph objects that represent the links of two already
> > existing objects in the graph.
> > 
> > While with the current implementation, it is possible to create
> > the links earlier, It doesn't make any sense to allow linking
> > two objects when they are not both created.
> > 
> > So, remove the code that would be handling those early-created
> > links and add a BUG_ON() to ensure that.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> 
> The code is OK, so:
> 
> Acked-by: Hans Verkuil 
> 
> But shouldn't this go *after* the omap3isp fixes? After this patch the
> omap3isp will call BUG_ON, and that's not what you want.

Yes. I'll change the order on my git tree.

> It is also not clear if the omap3isp driver is the only one that has this
> 'create link before objects' problem. I would expect that the omap4 staging
> driver has the same issue and possibly others as well.
> 
> Did someone look at that?

I guess other drivers are doing the same.

Javier's planning to review the other platform drivers in order to add the 
needed fixes there too, and to do more tests with some other platform
drivers that he has hardware for testing.

> 
> Regards,
> 
>   Hans
> 
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 138b18416460..0d85c6c28004 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -443,13 +443,6 @@ int __must_check media_device_register_entity(struct 
> > media_device *mdev,
> > media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, >graph_obj);
> > list_add_tail(>list, >entities);
> >  
> > -   /*
> > -* Initialize objects at the links
> > -* in the case where links got created before entity register
> > -*/
> > -   for (i = 0; i < entity->num_links; i++)
> > -   media_gobj_init(mdev, MEDIA_GRAPH_LINK,
> > -   >links[i].graph_obj);
> > /* Initialize objects at the pads */
> > for (i = 0; i < entity->num_pads; i++)
> > media_gobj_init(mdev, MEDIA_GRAPH_PAD,
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index 01946baa32d5..9f8e0145db7a 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -161,6 +161,8 @@ void media_gobj_init(struct media_device *mdev,
> >enum media_gobj_type type,
> >struct media_gobj *gobj)
> >  {
> > +   BUG_ON(!mdev);
> > +
> > gobj->mdev = mdev;
> >  
> > /* Create a per-type unique object ID */
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 16/55] [media] media: Don't accept early-created links

2015-08-31 Thread Hans Verkuil 
On 08/31/2015 12:54 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 31 Aug 2015 12:30:16 +0200
> Hans Verkuil  escreveu:
> 
>> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
>>> Links are graph objects that represent the links of two already
>>> existing objects in the graph.
>>>
>>> While with the current implementation, it is possible to create
>>> the links earlier, It doesn't make any sense to allow linking
>>> two objects when they are not both created.
>>>
>>> So, remove the code that would be handling those early-created
>>> links and add a BUG_ON() to ensure that.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab 
>>
>> The code is OK, so:
>>
>> Acked-by: Hans Verkuil 
>>
>> But shouldn't this go *after* the omap3isp fixes? After this patch the
>> omap3isp will call BUG_ON, and that's not what you want.
> 
> Yes. I'll change the order on my git tree.
> 
>> It is also not clear if the omap3isp driver is the only one that has this
>> 'create link before objects' problem. I would expect that the omap4 staging
>> driver has the same issue and possibly others as well.
>>
>> Did someone look at that?
> 
> I guess other drivers are doing the same.
> 
> Javier's planning to review the other platform drivers in order to add the 
> needed fixes there too, and to do more tests with some other platform
> drivers that he has hardware for testing.

OK, good. Just wanted to know that.

Perhaps it is a good idea to add a TODO list to the cover letter. This would
be one item on that list.

Regards,

Hans

> 
>>
>> Regards,
>>
>>  Hans
>>
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index 138b18416460..0d85c6c28004 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -443,13 +443,6 @@ int __must_check media_device_register_entity(struct 
>>> media_device *mdev,
>>> media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, >graph_obj);
>>> list_add_tail(>list, >entities);
>>>  
>>> -   /*
>>> -* Initialize objects at the links
>>> -* in the case where links got created before entity register
>>> -*/
>>> -   for (i = 0; i < entity->num_links; i++)
>>> -   media_gobj_init(mdev, MEDIA_GRAPH_LINK,
>>> -   >links[i].graph_obj);
>>> /* Initialize objects at the pads */
>>> for (i = 0; i < entity->num_pads; i++)
>>> media_gobj_init(mdev, MEDIA_GRAPH_PAD,
>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>>> index 01946baa32d5..9f8e0145db7a 100644
>>> --- a/drivers/media/media-entity.c
>>> +++ b/drivers/media/media-entity.c
>>> @@ -161,6 +161,8 @@ void media_gobj_init(struct media_device *mdev,
>>>enum media_gobj_type type,
>>>struct media_gobj *gobj)
>>>  {
>>> +   BUG_ON(!mdev);
>>> +
>>> gobj->mdev = mdev;
>>>  
>>> /* Create a per-type unique object ID */
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 16/55] [media] media: Don't accept early-created links

2015-08-31 Thread Javier Martinez Canillas 
Hello Hans,

On 08/31/2015 01:01 PM, Hans Verkuil wrote:
> On 08/31/2015 12:54 PM, Mauro Carvalho Chehab wrote:
>> Em Mon, 31 Aug 2015 12:30:16 +0200
>> Hans Verkuil  escreveu:
>>
>>> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
 Links are graph objects that represent the links of two already
 existing objects in the graph.

 While with the current implementation, it is possible to create
 the links earlier, It doesn't make any sense to allow linking
 two objects when they are not both created.

 So, remove the code that would be handling those early-created
 links and add a BUG_ON() to ensure that.

 Signed-off-by: Mauro Carvalho Chehab 
>>>
>>> The code is OK, so:
>>>
>>> Acked-by: Hans Verkuil 
>>>
>>> But shouldn't this go *after* the omap3isp fixes? After this patch the
>>> omap3isp will call BUG_ON, and that's not what you want.
>>
>> Yes. I'll change the order on my git tree.
>>
>>> It is also not clear if the omap3isp driver is the only one that has this
>>> 'create link before objects' problem. I would expect that the omap4 staging
>>> driver has the same issue and possibly others as well.
>>>
>>> Did someone look at that?
>>
>> I guess other drivers are doing the same.
>>
>> Javier's planning to review the other platform drivers in order to add the 
>> needed fixes there too, and to do more tests with some other platform
>> drivers that he has hardware for testing.
> 
> OK, good. Just wanted to know that.
>

Yes, the omap4iss driver in staging has a similar logic and thus the
same issues than the omap3isp driver. I'll write patches for this as
well but I don't have an OMAP4 board here so testing is appreciated.

As Mauro mentioned, I was reviewing the others driver that are creating
pad links and found more that are creating links before registering:

drivers/media/usb/uvc
drivers/media/platform/vsp1
drivers/media/i2c/smiapp

I'll try to take care of these too.

The other drivers that are creating pad links are doing the right thing
AFAICT by registering the entity with the media device before.

> Perhaps it is a good idea to add a TODO list to the cover letter. This would
> be one item on that list.
>

A TODO list is a good idea indeed.
 
> Regards,
> 
>   Hans
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html