media: rcar-vin: Move media_device_register to async completion

Commit Message

Michael Rodin June 16, 2020, 5:31 p.m. UTC

From: Steve Longerbeam <[e-mail]>

The media_device is registered during driver probe, before async
completion, so it is possible for .link_notify to be called before
all devices are bound.

Fix this by moving media_device_register() to rvin_group_notify_complete().
This ensures that all devices are now bound (the rcar-csi2 subdevices and
and video capture devices) before .link_notify can be called.

Signed-off-by: Steve Longerbeam <[e-mail]>
Signed-off-by: Michael Rodin <[e-mail]>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Niklas Söderlund June 17, 2020, 10:56 a.m. UTC | #1

Hi Michael and Steve,

On 2020-06-16 19:31:36 +0200, Michael Rodin wrote:
> From: Steve Longerbeam <[e-mail]>
> 
> The media_device is registered during driver probe, before async
> completion, so it is possible for .link_notify to be called before
> all devices are bound.
> 
> Fix this by moving media_device_register() to rvin_group_notify_complete().
> This ensures that all devices are now bound (the rcar-csi2 subdevices and
> and video capture devices) before .link_notify can be called.

I'm curious to what situation created the need for this change. I'm 
currently trying to take the VIN driver in the opposite direction [1] 
with the end goal of registering video devices at probe time and then 
allow the media graph to populate as devices becomes available.

My reason for this is that we could have a functional pipeline inside 
the graph even if it's not complete. This came out of the GMSL work done
a while pack where I had a faulty camera that would prevent the other 7 
in the system to function.

1. [PATCH 0/5] media-device: Report if graph is complete

> 
> Signed-off-by: Steve Longerbeam <[e-mail]>
> Signed-off-by: Michael Rodin <[e-mail]>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 7440c89..e70f83b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -253,7 +253,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>  	struct media_device *mdev = &group->mdev;
>  	const struct of_device_id *match;
>  	struct device_node *np;
> -	int ret;
>  
>  	mutex_init(&group->lock);
>  
> @@ -266,7 +265,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>  	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
>  
>  	mdev->dev = vin->dev;
> -	mdev->ops = &rvin_media_ops;
>  
>  	match = of_match_node(vin->dev->driver->of_match_table,
>  			      vin->dev->of_node);
> @@ -278,11 +276,7 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>  
>  	media_device_init(mdev);
>  
> -	ret = media_device_register(&group->mdev);
> -	if (ret)
> -		rvin_group_cleanup(group);
> -
> -	return ret;
> +	return 0;
>  }
>  
>  static void rvin_group_release(struct kref *kref)
> @@ -688,6 +682,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  		return ret;
>  	}
>  
> +	vin->group->mdev.ops = &rvin_media_ops;
> +
>  	/* Register all video nodes for the group. */
>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
>  		if (vin->group->vin[i] &&
> @@ -736,8 +732,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  		}
>  	}
>  	mutex_unlock(&vin->group->lock);
> +	if (ret)
> +		return ret;
>  
> -	return ret;
> +	return media_device_register(&vin->group->mdev);
>  }
>  
>  static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> -- 
> 2.7.4
>

Michael Rodin June 17, 2020, 3:15 p.m. UTC | #2

Hi Niklas and Steve,

On Wed, Jun 17, 2020 at 12:56:46PM +0200, Niklas Söderlund wrote:
> Hi Michael and Steve,
> 
> On 2020-06-16 19:31:36 +0200, Michael Rodin wrote:
> > From: Steve Longerbeam <[e-mail]>
> > 
> > The media_device is registered during driver probe, before async
> > completion, so it is possible for .link_notify to be called before
> > all devices are bound.
> > 
> > Fix this by moving media_device_register() to rvin_group_notify_complete().
> > This ensures that all devices are now bound (the rcar-csi2 subdevices and
> > and video capture devices) before .link_notify can be called.
> 
> I'm curious to what situation created the need for this change. I'm 
> currently trying to take the VIN driver in the opposite direction [1] 
> with the end goal of registering video devices at probe time and then 
> allow the media graph to populate as devices becomes available.

It looks like almost all platform drivers call media_device_register() in
the completion callback. From my understaning it is necessary to ensure
that all subdevices are bound and all links are created before the user
can enable any link (which would trigger link_notify callback execution)
and set formats. If I am not mistaken, Steve could observe an "OOPS" or
at least it is theoretically possible.

Actually I found that this patch alone is not enough even if it is correct,
because we also have to register the media device in rvin_parallel_notify_complete()
in case if there is only a parallel video input device attached.

> My reason for this is that we could have a functional pipeline inside 
> the graph even if it's not complete. This came out of the GMSL work done
> a while pack where I had a faulty camera that would prevent the other 7 
> in the system to function.

I agree that if a probe of a faulty subdevice fails, this should not affect
functionality of the other attached subdevices. The "complete" callback of
the async notifier is probably not executed in this case, so I guess, we
would have to register the media device in the "bound" callback after the first
subdevice has been probed? Otherwise there is not much sense to have video
capture devices, which are not connected to any source.

(Delayed) population of the media graph after media device registration
sounds also like a requirement for device tree overlay support, which would
also be a nice feature.

> 1. [PATCH 0/5] media-device: Report if graph is complete
> 
> > 
> > Signed-off-by: Steve Longerbeam <[e-mail]>
> > Signed-off-by: Michael Rodin <[e-mail]>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 7440c89..e70f83b 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -253,7 +253,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> >  	struct media_device *mdev = &group->mdev;
> >  	const struct of_device_id *match;
> >  	struct device_node *np;
> > -	int ret;
> >  
> >  	mutex_init(&group->lock);
> >  
> > @@ -266,7 +265,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> >  	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
> >  
> >  	mdev->dev = vin->dev;
> > -	mdev->ops = &rvin_media_ops;
> >  
> >  	match = of_match_node(vin->dev->driver->of_match_table,
> >  			      vin->dev->of_node);
> > @@ -278,11 +276,7 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> >  
> >  	media_device_init(mdev);
> >  
> > -	ret = media_device_register(&group->mdev);
> > -	if (ret)
> > -		rvin_group_cleanup(group);
> > -
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  static void rvin_group_release(struct kref *kref)
> > @@ -688,6 +682,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> >  		return ret;
> >  	}
> >  
> > +	vin->group->mdev.ops = &rvin_media_ops;
> > +
> >  	/* Register all video nodes for the group. */
> >  	for (i = 0; i < RCAR_VIN_NUM; i++) {
> >  		if (vin->group->vin[i] &&
> > @@ -736,8 +732,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> >  		}
> >  	}
> >  	mutex_unlock(&vin->group->lock);
> > +	if (ret)
> > +		return ret;
> >  
> > -	return ret;
> > +	return media_device_register(&vin->group->mdev);
> >  }
> >  
> >  static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

Niklas Söderlund June 17, 2020, 3:28 p.m. UTC | #3

Hi Michael,

On 2020-06-17 17:15:37 +0200, Michael Rodin wrote:
> Hi Niklas and Steve,
> 
> On Wed, Jun 17, 2020 at 12:56:46PM +0200, Niklas Söderlund wrote:
> > Hi Michael and Steve,
> > 
> > On 2020-06-16 19:31:36 +0200, Michael Rodin wrote:
> > > From: Steve Longerbeam <[e-mail]>
> > > 
> > > The media_device is registered during driver probe, before async
> > > completion, so it is possible for .link_notify to be called before
> > > all devices are bound.
> > > 
> > > Fix this by moving media_device_register() to rvin_group_notify_complete().
> > > This ensures that all devices are now bound (the rcar-csi2 subdevices and
> > > and video capture devices) before .link_notify can be called.
> > 
> > I'm curious to what situation created the need for this change. I'm 
> > currently trying to take the VIN driver in the opposite direction [1] 
> > with the end goal of registering video devices at probe time and then 
> > allow the media graph to populate as devices becomes available.
> 
> It looks like almost all platform drivers call media_device_register() in
> the completion callback. From my understaning it is necessary to ensure
> that all subdevices are bound and all links are created before the user
> can enable any link (which would trigger link_notify callback execution)
> and set formats. If I am not mistaken, Steve could observe an "OOPS" or
> at least it is theoretically possible.

If an OOPS have been observed I would be interested to see it. That way 
we can fix the OOPS and keep the media graph registration where it is 
today.

> 
> Actually I found that this patch alone is not enough even if it is correct,
> because we also have to register the media device in rvin_parallel_notify_complete()
> in case if there is only a parallel video input device attached.
> 
> > My reason for this is that we could have a functional pipeline inside 
> > the graph even if it's not complete. This came out of the GMSL work done
> > a while pack where I had a faulty camera that would prevent the other 7 
> > in the system to function.
> 
> I agree that if a probe of a faulty subdevice fails, this should not affect
> functionality of the other attached subdevices. The "complete" callback of
> the async notifier is probably not executed in this case, so I guess, we
> would have to register the media device in the "bound" callback after the first
> subdevice has been probed? Otherwise there is not much sense to have video
> capture devices, which are not connected to any source.

Calling it in the bound callback is mostly the same as it is today, as 
link_notify could then be called when not all entities are in the graph.  
In fact even if we where tp move the media device registration to the t
complete callback we have this problem if any of the subdevices are 
unbound. Then we are back to the state with a registerd media device 
where not all entities are present.

I think the solution here is to address the issue (if any) in the 
link_notify callback when the graph is not fully populated.

> 
> (Delayed) population of the media graph after media device registration
> sounds also like a requirement for device tree overlay support, which would
> also be a nice feature.
> 
> > 1. [PATCH 0/5] media-device: Report if graph is complete
> > 
> > > 
> > > Signed-off-by: Steve Longerbeam <[e-mail]>
> > > Signed-off-by: Michael Rodin <[e-mail]>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index 7440c89..e70f83b 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -253,7 +253,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> > >  	struct media_device *mdev = &group->mdev;
> > >  	const struct of_device_id *match;
> > >  	struct device_node *np;
> > > -	int ret;
> > >  
> > >  	mutex_init(&group->lock);
> > >  
> > > @@ -266,7 +265,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> > >  	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
> > >  
> > >  	mdev->dev = vin->dev;
> > > -	mdev->ops = &rvin_media_ops;
> > >  
> > >  	match = of_match_node(vin->dev->driver->of_match_table,
> > >  			      vin->dev->of_node);
> > > @@ -278,11 +276,7 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> > >  
> > >  	media_device_init(mdev);
> > >  
> > > -	ret = media_device_register(&group->mdev);
> > > -	if (ret)
> > > -		rvin_group_cleanup(group);
> > > -
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >  
> > >  static void rvin_group_release(struct kref *kref)
> > > @@ -688,6 +682,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> > >  		return ret;
> > >  	}
> > >  
> > > +	vin->group->mdev.ops = &rvin_media_ops;
> > > +
> > >  	/* Register all video nodes for the group. */
> > >  	for (i = 0; i < RCAR_VIN_NUM; i++) {
> > >  		if (vin->group->vin[i] &&
> > > @@ -736,8 +732,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> > >  		}
> > >  	}
> > >  	mutex_unlock(&vin->group->lock);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > > -	return ret;
> > > +	return media_device_register(&vin->group->mdev);
> > >  }
> > >  
> > >  static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Regards,
> > Niklas Söderlund
> 
> -- 
> Best Regards,
> Michael

Steve Longerbeam June 17, 2020, 6:18 p.m. UTC | #4

Hi Niklas, Michael,

On 6/17/20 8:28 AM, Niklas Söderlund wrote:
> Hi Michael,
>
> On 2020-06-17 17:15:37 +0200, Michael Rodin wrote:
>> Hi Niklas and Steve,
>>
>> On Wed, Jun 17, 2020 at 12:56:46PM +0200, Niklas Söderlund wrote:
>>> Hi Michael and Steve,
>>>
>>> On 2020-06-16 19:31:36 +0200, Michael Rodin wrote:
>>>> From: Steve Longerbeam <[e-mail]>
>>>>
>>>> The media_device is registered during driver probe, before async
>>>> completion, so it is possible for .link_notify to be called before
>>>> all devices are bound.
>>>>
>>>> Fix this by moving media_device_register() to rvin_group_notify_complete().
>>>> This ensures that all devices are now bound (the rcar-csi2 subdevices and
>>>> and video capture devices) before .link_notify can be called.
>>> I'm curious to what situation created the need for this change. I'm
>>> currently trying to take the VIN driver in the opposite direction [1]
>>> with the end goal of registering video devices at probe time and then
>>> allow the media graph to populate as devices becomes available.
>> It looks like almost all platform drivers call media_device_register() in
>> the completion callback. From my understaning it is necessary to ensure
>> that all subdevices are bound and all links are created before the user
>> can enable any link (which would trigger link_notify callback execution)
>> and set formats. If I am not mistaken, Steve could observe an "OOPS" or
>> at least it is theoretically possible.
> If an OOPS have been observed I would be interested to see it. That way
> we can fix the OOPS and keep the media graph registration where it is
> today.

It's been a long time since I looked at this on a Salvator-X. But I do 
remember there was an OOPS if an attempt was made to enable a link 
before all devices had bound. Unfortunately I no longer have access to 
RCAR h/w to prove this point.

>> Actually I found that this patch alone is not enough even if it is correct,
>> because we also have to register the media device in rvin_parallel_notify_complete()
>> in case if there is only a parallel video input device attached.
>>
>>> My reason for this is that we could have a functional pipeline inside
>>> the graph even if it's not complete. This came out of the GMSL work done
>>> a while pack where I had a faulty camera that would prevent the other 7
>>> in the system to function.
>> I agree that if a probe of a faulty subdevice fails, this should not affect
>> functionality of the other attached subdevices. The "complete" callback of
>> the async notifier is probably not executed in this case, so I guess, we
>> would have to register the media device in the "bound" callback after the first
>> subdevice has been probed? Otherwise there is not much sense to have video
>> capture devices, which are not connected to any source.
> Calling it in the bound callback is mostly the same as it is today, as
> link_notify could then be called when not all entities are in the graph.
> In fact even if we where tp move the media device registration to the t
> complete callback we have this problem if any of the subdevices are
> unbound. Then we are back to the state with a registerd media device
> where not all entities are present.
>
> I think the solution here is to address the issue (if any) in the
> link_notify callback when the graph is not fully populated.

I like the idea of allowing a functional pipeline without all devices 
bound. So I understand the desire to keep media device registration in 
probe.

So in that case I agree link_notify should be fixed if it still needs 
fixing to handle non-bound subdevices.

Steve

>> (Delayed) population of the media graph after media device registration
>> sounds also like a requirement for device tree overlay support, which would
>> also be a nice feature.
>>
>>> 1. [PATCH 0/5] media-device: Report if graph is complete
>>>
>>>> Signed-off-by: Steve Longerbeam <[e-mail]>
>>>> Signed-off-by: Michael Rodin <[e-mail]>
>>>> ---
>>>>   drivers/media/platform/rcar-vin/rcar-core.c | 14 ++++++--------
>>>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
>>>> index 7440c89..e70f83b 100644
>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
>>>> @@ -253,7 +253,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>>>>   	struct media_device *mdev = &group->mdev;
>>>>   	const struct of_device_id *match;
>>>>   	struct device_node *np;
>>>> -	int ret;
>>>>   
>>>>   	mutex_init(&group->lock);
>>>>   
>>>> @@ -266,7 +265,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>>>>   	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
>>>>   
>>>>   	mdev->dev = vin->dev;
>>>> -	mdev->ops = &rvin_media_ops;
>>>>   
>>>>   	match = of_match_node(vin->dev->driver->of_match_table,
>>>>   			      vin->dev->of_node);
>>>> @@ -278,11 +276,7 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>>>>   
>>>>   	media_device_init(mdev);
>>>>   
>>>> -	ret = media_device_register(&group->mdev);
>>>> -	if (ret)
>>>> -		rvin_group_cleanup(group);
>>>> -
>>>> -	return ret;
>>>> +	return 0;
>>>>   }
>>>>   
>>>>   static void rvin_group_release(struct kref *kref)
>>>> @@ -688,6 +682,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>>>>   		return ret;
>>>>   	}
>>>>   
>>>> +	vin->group->mdev.ops = &rvin_media_ops;
>>>> +
>>>>   	/* Register all video nodes for the group. */
>>>>   	for (i = 0; i < RCAR_VIN_NUM; i++) {
>>>>   		if (vin->group->vin[i] &&
>>>> @@ -736,8 +732,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>>>>   		}
>>>>   	}
>>>>   	mutex_unlock(&vin->group->lock);
>>>> +	if (ret)
>>>> +		return ret;
>>>>   
>>>> -	return ret;
>>>> +	return media_device_register(&vin->group->mdev);
>>>>   }
>>>>   
>>>>   static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>>>> -- 
>>>> 2.7.4
>>>>
>>> -- 
>>> Regards,
>>> Niklas Söderlund
>> -- 
>> Best Regards,
>> Michael

Hans Verkuil June 24, 2020, 1:31 p.m. UTC | #5

On 17/06/2020 17:28, Niklas Söderlund wrote:
> Hi Michael,
> 
> On 2020-06-17 17:15:37 +0200, Michael Rodin wrote:
>> Hi Niklas and Steve,
>>
>> On Wed, Jun 17, 2020 at 12:56:46PM +0200, Niklas Söderlund wrote:
>>> Hi Michael and Steve,
>>>
>>> On 2020-06-16 19:31:36 +0200, Michael Rodin wrote:
>>>> From: Steve Longerbeam <[e-mail]>
>>>>
>>>> The media_device is registered during driver probe, before async
>>>> completion, so it is possible for .link_notify to be called before
>>>> all devices are bound.
>>>>
>>>> Fix this by moving media_device_register() to rvin_group_notify_complete().
>>>> This ensures that all devices are now bound (the rcar-csi2 subdevices and
>>>> and video capture devices) before .link_notify can be called.
>>>
>>> I'm curious to what situation created the need for this change. I'm 
>>> currently trying to take the VIN driver in the opposite direction [1] 
>>> with the end goal of registering video devices at probe time and then 
>>> allow the media graph to populate as devices becomes available.
>>
>> It looks like almost all platform drivers call media_device_register() in
>> the completion callback. From my understaning it is necessary to ensure
>> that all subdevices are bound and all links are created before the user
>> can enable any link (which would trigger link_notify callback execution)
>> and set formats. If I am not mistaken, Steve could observe an "OOPS" or
>> at least it is theoretically possible.
> 
> If an OOPS have been observed I would be interested to see it. That way 
> we can fix the OOPS and keep the media graph registration where it is 
> today.
> 
>>
>> Actually I found that this patch alone is not enough even if it is correct,
>> because we also have to register the media device in rvin_parallel_notify_complete()
>> in case if there is only a parallel video input device attached.
>>
>>> My reason for this is that we could have a functional pipeline inside 
>>> the graph even if it's not complete. This came out of the GMSL work done
>>> a while pack where I had a faulty camera that would prevent the other 7 
>>> in the system to function.
>>
>> I agree that if a probe of a faulty subdevice fails, this should not affect
>> functionality of the other attached subdevices. The "complete" callback of
>> the async notifier is probably not executed in this case, so I guess, we
>> would have to register the media device in the "bound" callback after the first
>> subdevice has been probed? Otherwise there is not much sense to have video
>> capture devices, which are not connected to any source.
> 
> Calling it in the bound callback is mostly the same as it is today, as 
> link_notify could then be called when not all entities are in the graph.  
> In fact even if we where tp move the media device registration to the t
> complete callback we have this problem if any of the subdevices are 
> unbound. Then we are back to the state with a registerd media device 
> where not all entities are present.
> 
> I think the solution here is to address the issue (if any) in the 
> link_notify callback when the graph is not fully populated.

As I have mentioned in other threads in this mailinglist (e.g.
https://www.spinics.net/lists/linux-media/msg171067.html), this is part
of a bigger problem: what to do if only part of a graph comes up, or
a part disappears. Today this is not supported.

It is certainly desirable to support this, but I want to see an RFC first
detailing the interaction with userspace. And remember that today the
expectation of applications is that device nodes are only created if all
components of the device probed successfully.

My preference is that rcar-vin is first changed to conform to current
expectations (i.e. /dev/mediaX is only registered at async completion).

After a proper discussion on how to handle partial initialization and
after we have a good plan, then this can be implemented in rcar-vin.

Note that I really want to support partial initialization, but I want
to see an RFC with a proper analysis and proposal first.

Regards,

	Hans

> 
>>
>> (Delayed) population of the media graph after media device registration
>> sounds also like a requirement for device tree overlay support, which would
>> also be a nice feature.
>>
>>> 1. [PATCH 0/5] media-device: Report if graph is complete
>>>
>>>>
>>>> Signed-off-by: Steve Longerbeam <[e-mail]>
>>>> Signed-off-by: Michael Rodin <[e-mail]>
>>>> ---
>>>>  drivers/media/platform/rcar-vin/rcar-core.c | 14 ++++++--------
>>>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
>>>> index 7440c89..e70f83b 100644
>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
>>>> @@ -253,7 +253,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>>>>  	struct media_device *mdev = &group->mdev;
>>>>  	const struct of_device_id *match;
>>>>  	struct device_node *np;
>>>> -	int ret;
>>>>  
>>>>  	mutex_init(&group->lock);
>>>>  
>>>> @@ -266,7 +265,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>>>>  	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
>>>>  
>>>>  	mdev->dev = vin->dev;
>>>> -	mdev->ops = &rvin_media_ops;
>>>>  
>>>>  	match = of_match_node(vin->dev->driver->of_match_table,
>>>>  			      vin->dev->of_node);
>>>> @@ -278,11 +276,7 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>>>>  
>>>>  	media_device_init(mdev);
>>>>  
>>>> -	ret = media_device_register(&group->mdev);
>>>> -	if (ret)
>>>> -		rvin_group_cleanup(group);
>>>> -
>>>> -	return ret;
>>>> +	return 0;
>>>>  }
>>>>  
>>>>  static void rvin_group_release(struct kref *kref)
>>>> @@ -688,6 +682,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> +	vin->group->mdev.ops = &rvin_media_ops;
>>>> +
>>>>  	/* Register all video nodes for the group. */
>>>>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
>>>>  		if (vin->group->vin[i] &&
>>>> @@ -736,8 +732,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>>>>  		}
>>>>  	}
>>>>  	mutex_unlock(&vin->group->lock);
>>>> +	if (ret)
>>>> +		return ret;
>>>>  
>>>> -	return ret;
>>>> +	return media_device_register(&vin->group->mdev);
>>>>  }
>>>>  
>>>>  static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>>> -- 
>>> Regards,
>>> Niklas Söderlund
>>
>> -- 
>> Best Regards,
>> Michael
>

Niklas Söderlund June 24, 2020, 2:54 p.m. UTC | #6

Hi Hans,

On 2020-06-24 15:31:01 +0200, Hans Verkuil wrote:
> On 17/06/2020 17:28, Niklas Söderlund wrote:
> > Hi Michael,
> > 
> > On 2020-06-17 17:15:37 +0200, Michael Rodin wrote:
> >> Hi Niklas and Steve,
> >>
> >> On Wed, Jun 17, 2020 at 12:56:46PM +0200, Niklas Söderlund wrote:
> >>> Hi Michael and Steve,
> >>>
> >>> On 2020-06-16 19:31:36 +0200, Michael Rodin wrote:
> >>>> From: Steve Longerbeam <[e-mail]>
> >>>>
> >>>> The media_device is registered during driver probe, before async
> >>>> completion, so it is possible for .link_notify to be called before
> >>>> all devices are bound.
> >>>>
> >>>> Fix this by moving media_device_register() to rvin_group_notify_complete().
> >>>> This ensures that all devices are now bound (the rcar-csi2 subdevices and
> >>>> and video capture devices) before .link_notify can be called.
> >>>
> >>> I'm curious to what situation created the need for this change. I'm 
> >>> currently trying to take the VIN driver in the opposite direction [1] 
> >>> with the end goal of registering video devices at probe time and then 
> >>> allow the media graph to populate as devices becomes available.
> >>
> >> It looks like almost all platform drivers call media_device_register() in
> >> the completion callback. From my understaning it is necessary to ensure
> >> that all subdevices are bound and all links are created before the user
> >> can enable any link (which would trigger link_notify callback execution)
> >> and set formats. If I am not mistaken, Steve could observe an "OOPS" or
> >> at least it is theoretically possible.
> > 
> > If an OOPS have been observed I would be interested to see it. That way 
> > we can fix the OOPS and keep the media graph registration where it is 
> > today.
> > 
> >>
> >> Actually I found that this patch alone is not enough even if it is correct,
> >> because we also have to register the media device in rvin_parallel_notify_complete()
> >> in case if there is only a parallel video input device attached.
> >>
> >>> My reason for this is that we could have a functional pipeline inside 
> >>> the graph even if it's not complete. This came out of the GMSL work done
> >>> a while pack where I had a faulty camera that would prevent the other 7 
> >>> in the system to function.
> >>
> >> I agree that if a probe of a faulty subdevice fails, this should not affect
> >> functionality of the other attached subdevices. The "complete" callback of
> >> the async notifier is probably not executed in this case, so I guess, we
> >> would have to register the media device in the "bound" callback after the first
> >> subdevice has been probed? Otherwise there is not much sense to have video
> >> capture devices, which are not connected to any source.
> > 
> > Calling it in the bound callback is mostly the same as it is today, as 
> > link_notify could then be called when not all entities are in the graph.  
> > In fact even if we where tp move the media device registration to the t
> > complete callback we have this problem if any of the subdevices are 
> > unbound. Then we are back to the state with a registerd media device 
> > where not all entities are present.
> > 
> > I think the solution here is to address the issue (if any) in the 
> > link_notify callback when the graph is not fully populated.
> 
> As I have mentioned in other threads in this mailinglist (e.g.
> https://www.spinics.net/lists/linux-media/msg171067.html), this is part
> of a bigger problem: what to do if only part of a graph comes up, or
> a part disappears. Today this is not supported.
> 
> It is certainly desirable to support this, but I want to see an RFC first
> detailing the interaction with userspace. And remember that today the
> expectation of applications is that device nodes are only created if all
> components of the device probed successfully.
> 
> My preference is that rcar-vin is first changed to conform to current
> expectations (i.e. /dev/mediaX is only registered at async completion).

I do not object to first making rcar-vin behave in this way. However for 
that work to be truly useful it should also solve what happens with all 
video, subdevices and media devices in the scenario where one device of 
the graph is unbound after complete have been called and how they can 
reregisterd if the graph becomes complete again.

If I understand things correctly the idea is to register video and 
subdevices at the time complete is called. However I'm unsure what is 
suppose to happen when unbound is called, shall all video and subdevices 
of the graph then be unregistered? Currently this happens for video 
devices in rcar-vin but it has a huge drawback, if the device that was 
unbound is rebound and complete() is called a second time the kernel 
prints big warnings on newer kernels and crashes in older. This is due 
to the kref involved in the V4L2 core. Also I'm unaware of how one could 
unregister all subdevies if another subdev in the graph is removed.

Figuring this out would make me happy as it's been a pet problem of mine 
for many years. I also think making this work could help in the 
discussion on how we can build partial initialization on top.

> 
> After a proper discussion on how to handle partial initialization and
> after we have a good plan, then this can be implemented in rcar-vin.
> 
> Note that I really want to support partial initialization, but I want
> to see an RFC with a proper analysis and proposal first.
> 
> Regards,
> 
> 	Hans
> 
> > 
> >>
> >> (Delayed) population of the media graph after media device registration
> >> sounds also like a requirement for device tree overlay support, which would
> >> also be a nice feature.
> >>
> >>> 1. [PATCH 0/5] media-device: Report if graph is complete
> >>>
> >>>>
> >>>> Signed-off-by: Steve Longerbeam <[e-mail]>
> >>>> Signed-off-by: Michael Rodin <[e-mail]>
> >>>> ---
> >>>>  drivers/media/platform/rcar-vin/rcar-core.c | 14 ++++++--------
> >>>>  1 file changed, 6 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> >>>> index 7440c89..e70f83b 100644
> >>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> >>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> >>>> @@ -253,7 +253,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> >>>>  	struct media_device *mdev = &group->mdev;
> >>>>  	const struct of_device_id *match;
> >>>>  	struct device_node *np;
> >>>> -	int ret;
> >>>>  
> >>>>  	mutex_init(&group->lock);
> >>>>  
> >>>> @@ -266,7 +265,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> >>>>  	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
> >>>>  
> >>>>  	mdev->dev = vin->dev;
> >>>> -	mdev->ops = &rvin_media_ops;
> >>>>  
> >>>>  	match = of_match_node(vin->dev->driver->of_match_table,
> >>>>  			      vin->dev->of_node);
> >>>> @@ -278,11 +276,7 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> >>>>  
> >>>>  	media_device_init(mdev);
> >>>>  
> >>>> -	ret = media_device_register(&group->mdev);
> >>>> -	if (ret)
> >>>> -		rvin_group_cleanup(group);
> >>>> -
> >>>> -	return ret;
> >>>> +	return 0;
> >>>>  }
> >>>>  
> >>>>  static void rvin_group_release(struct kref *kref)
> >>>> @@ -688,6 +682,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> >>>>  		return ret;
> >>>>  	}
> >>>>  
> >>>> +	vin->group->mdev.ops = &rvin_media_ops;
> >>>> +
> >>>>  	/* Register all video nodes for the group. */
> >>>>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
> >>>>  		if (vin->group->vin[i] &&
> >>>> @@ -736,8 +732,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> >>>>  		}
> >>>>  	}
> >>>>  	mutex_unlock(&vin->group->lock);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>>  
> >>>> -	return ret;
> >>>> +	return media_device_register(&vin->group->mdev);
> >>>>  }
> >>>>  
> >>>>  static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> >>>> -- 
> >>>> 2.7.4
> >>>>
> >>>
> >>> -- 
> >>> Regards,
> >>> Niklas Söderlund
> >>
> >> -- 
> >> Best Regards,
> >> Michael
> > 
>

Hans Verkuil July 16, 2020, 9:18 a.m. UTC | #7

On 24/06/2020 16:54, Niklas Söderlund wrote:
> Hi Hans,
> 
> On 2020-06-24 15:31:01 +0200, Hans Verkuil wrote:
>> On 17/06/2020 17:28, Niklas Söderlund wrote:
>>> Hi Michael,
>>>
>>> On 2020-06-17 17:15:37 +0200, Michael Rodin wrote:
>>>> Hi Niklas and Steve,
>>>>
>>>> On Wed, Jun 17, 2020 at 12:56:46PM +0200, Niklas Söderlund wrote:
>>>>> Hi Michael and Steve,
>>>>>
>>>>> On 2020-06-16 19:31:36 +0200, Michael Rodin wrote:
>>>>>> From: Steve Longerbeam <[e-mail]>
>>>>>>
>>>>>> The media_device is registered during driver probe, before async
>>>>>> completion, so it is possible for .link_notify to be called before
>>>>>> all devices are bound.
>>>>>>
>>>>>> Fix this by moving media_device_register() to rvin_group_notify_complete().
>>>>>> This ensures that all devices are now bound (the rcar-csi2 subdevices and
>>>>>> and video capture devices) before .link_notify can be called.
>>>>>
>>>>> I'm curious to what situation created the need for this change. I'm 
>>>>> currently trying to take the VIN driver in the opposite direction [1] 
>>>>> with the end goal of registering video devices at probe time and then 
>>>>> allow the media graph to populate as devices becomes available.
>>>>
>>>> It looks like almost all platform drivers call media_device_register() in
>>>> the completion callback. From my understaning it is necessary to ensure
>>>> that all subdevices are bound and all links are created before the user
>>>> can enable any link (which would trigger link_notify callback execution)
>>>> and set formats. If I am not mistaken, Steve could observe an "OOPS" or
>>>> at least it is theoretically possible.
>>>
>>> If an OOPS have been observed I would be interested to see it. That way 
>>> we can fix the OOPS and keep the media graph registration where it is 
>>> today.
>>>
>>>>
>>>> Actually I found that this patch alone is not enough even if it is correct,
>>>> because we also have to register the media device in rvin_parallel_notify_complete()
>>>> in case if there is only a parallel video input device attached.
>>>>
>>>>> My reason for this is that we could have a functional pipeline inside 
>>>>> the graph even if it's not complete. This came out of the GMSL work done
>>>>> a while pack where I had a faulty camera that would prevent the other 7 
>>>>> in the system to function.
>>>>
>>>> I agree that if a probe of a faulty subdevice fails, this should not affect
>>>> functionality of the other attached subdevices. The "complete" callback of
>>>> the async notifier is probably not executed in this case, so I guess, we
>>>> would have to register the media device in the "bound" callback after the first
>>>> subdevice has been probed? Otherwise there is not much sense to have video
>>>> capture devices, which are not connected to any source.
>>>
>>> Calling it in the bound callback is mostly the same as it is today, as 
>>> link_notify could then be called when not all entities are in the graph.  
>>> In fact even if we where tp move the media device registration to the t
>>> complete callback we have this problem if any of the subdevices are 
>>> unbound. Then we are back to the state with a registerd media device 
>>> where not all entities are present.
>>>
>>> I think the solution here is to address the issue (if any) in the 
>>> link_notify callback when the graph is not fully populated.
>>
>> As I have mentioned in other threads in this mailinglist (e.g.
>> https://www.spinics.net/lists/linux-media/msg171067.html), this is part
>> of a bigger problem: what to do if only part of a graph comes up, or
>> a part disappears. Today this is not supported.
>>
>> It is certainly desirable to support this, but I want to see an RFC first
>> detailing the interaction with userspace. And remember that today the
>> expectation of applications is that device nodes are only created if all
>> components of the device probed successfully.
>>
>> My preference is that rcar-vin is first changed to conform to current
>> expectations (i.e. /dev/mediaX is only registered at async completion).
> 
> I do not object to first making rcar-vin behave in this way. However for 
> that work to be truly useful it should also solve what happens with all 
> video, subdevices and media devices in the scenario where one device of 
> the graph is unbound after complete have been called and how they can 
> reregisterd if the graph becomes complete again.
> 
> If I understand things correctly the idea is to register video and 
> subdevices at the time complete is called. However I'm unsure what is 
> suppose to happen when unbound is called, shall all video and subdevices 
> of the graph then be unregistered? Currently this happens for video 
> devices in rcar-vin but it has a huge drawback, if the device that was 
> unbound is rebound and complete() is called a second time the kernel 
> prints big warnings on newer kernels and crashes in older. This is due 
> to the kref involved in the V4L2 core. Also I'm unaware of how one could 
> unregister all subdevies if another subdev in the graph is removed.
> 
> Figuring this out would make me happy as it's been a pet problem of mine 
> for many years. I also think making this work could help in the 
> discussion on how we can build partial initialization on top.

What exactly are these warnings? I remember faintly seeing backtraces of
this before, but it is so long ago. It would be good to see what happens with
the latest kernel (i.e. media master branch).

Regards,

	Hans

> 
>>
>> After a proper discussion on how to handle partial initialization and
>> after we have a good plan, then this can be implemented in rcar-vin.
>>
>> Note that I really want to support partial initialization, but I want
>> to see an RFC with a proper analysis and proposal first.
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>>>
>>>> (Delayed) population of the media graph after media device registration
>>>> sounds also like a requirement for device tree overlay support, which would
>>>> also be a nice feature.
>>>>
>>>>> 1. [PATCH 0/5] media-device: Report if graph is complete
>>>>>
>>>>>>
>>>>>> Signed-off-by: Steve Longerbeam <[e-mail]>
>>>>>> Signed-off-by: Michael Rodin <[e-mail]>
>>>>>> ---
>>>>>>  drivers/media/platform/rcar-vin/rcar-core.c | 14 ++++++--------
>>>>>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
>>>>>> index 7440c89..e70f83b 100644
>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
>>>>>> @@ -253,7 +253,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>>>>>>  	struct media_device *mdev = &group->mdev;
>>>>>>  	const struct of_device_id *match;
>>>>>>  	struct device_node *np;
>>>>>> -	int ret;
>>>>>>  
>>>>>>  	mutex_init(&group->lock);
>>>>>>  
>>>>>> @@ -266,7 +265,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>>>>>>  	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
>>>>>>  
>>>>>>  	mdev->dev = vin->dev;
>>>>>> -	mdev->ops = &rvin_media_ops;
>>>>>>  
>>>>>>  	match = of_match_node(vin->dev->driver->of_match_table,
>>>>>>  			      vin->dev->of_node);
>>>>>> @@ -278,11 +276,7 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>>>>>>  
>>>>>>  	media_device_init(mdev);
>>>>>>  
>>>>>> -	ret = media_device_register(&group->mdev);
>>>>>> -	if (ret)
>>>>>> -		rvin_group_cleanup(group);
>>>>>> -
>>>>>> -	return ret;
>>>>>> +	return 0;
>>>>>>  }
>>>>>>  
>>>>>>  static void rvin_group_release(struct kref *kref)
>>>>>> @@ -688,6 +682,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>>>>>>  		return ret;
>>>>>>  	}
>>>>>>  
>>>>>> +	vin->group->mdev.ops = &rvin_media_ops;
>>>>>> +
>>>>>>  	/* Register all video nodes for the group. */
>>>>>>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
>>>>>>  		if (vin->group->vin[i] &&
>>>>>> @@ -736,8 +732,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>>>>>>  		}
>>>>>>  	}
>>>>>>  	mutex_unlock(&vin->group->lock);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>>  
>>>>>> -	return ret;
>>>>>> +	return media_device_register(&vin->group->mdev);
>>>>>>  }
>>>>>>  
>>>>>>  static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>>>>>> -- 
>>>>>> 2.7.4
>>>>>>
>>>>>
>>>>> -- 
>>>>> Regards,
>>>>> Niklas Söderlund
>>>>
>>>> -- 
>>>> Best Regards,
>>>> Michael
>>>
>>
>

Niklas Söderlund July 17, 2020, 1:59 p.m. UTC | #8

Hi Hans,

Thanks for taking an interest in this.

On 2020-07-16 11:18:29 +0200, Hans Verkuil wrote:
> What exactly are these warnings? I remember faintly seeing backtraces 
> of this before, but it is so long ago. It would be good to see what 
> happens with the latest kernel (i.e. media master branch).

Produced on latest media-tree 6f01dfb760c027d5 ("media: cros-ec-cec: do 
not bail on device_init_wakeup failure") on R-Car M3-N. That is the 
video devices are registered in complete() and unregistered in unbind().

To provoke the trace I unbind and re-bind one of the two CSI-2 receivers 
in the system. The CSI-2 receiver is subdevice V4L2 driver (rcar-csi2) 
that is register in the VIN (rcar-vin) async notifier.

# echo fea80000.csi2 > /sys/bus/platform/drivers/rcar-csi2/unbind
[   42.646841] rcar-vin e6ef0000.video: Removing video11
[   42.652521] rcar-vin e6ef1000.video: Removing video12
[   42.658967] rcar-vin e6ef2000.video: Removing video13
[   42.664574] rcar-vin e6ef3000.video: Removing video14
[   42.670185] rcar-vin e6ef4000.video: Removing video15
[   42.675740] rcar-vin e6ef5000.video: Removing video16
[   42.681403] rcar-vin e6ef6000.video: Removing video17
[   42.687030] rcar-vin e6ef7000.video: Removing video18

# echo fea80000.csi2 > /sys/bus/platform/drivers/rcar-csi2/bind
[   50.065343] kobject (000000001e7d7cf3): tried to init an initialized object, something is seriously wrong.
[   50.075324] CPU: 0 PID: 399 Comm: bash Not tainted 5.8.0-rc1-arm64-renesas-00122-g6f01dfb760c027d5 #1
[   50.084660] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
[   50.092673] Call trace:
[   50.095160]  dump_backtrace+0x0/0x190
[   50.098869]  show_stack+0x14/0x20
[   50.102229]  dump_stack+0xe0/0x128
[   50.105675]  kobject_init+0x8c/0xa0
[   50.109210]  device_initialize+0x40/0x150
[   50.113269]  device_register+0x14/0x30
[   50.117068]  __video_register_device+0xcf4/0x1780
[   50.121830]  rvin_v4l2_register+0xe4/0x150
[   50.125976]  rvin_group_notify_complete+0x58/0x18c
[   50.130827]  v4l2_async_notifier_try_complete.part.0+0x50/0x60
[   50.136730]  v4l2_async_register_subdev+0xfc/0x1b0
[   50.141578]  rcsi2_probe+0x338/0x41c
[   50.145198]  platform_drv_probe+0x50/0xa0
[   50.149256]  really_probe+0xdc/0x430
[   50.152873]  driver_probe_device+0x54/0xb0
[   50.157019]  device_driver_attach+0xbc/0xc4
[   50.161253]  bind_store+0xb8/0x11c
[   50.164697]  drv_attr_store+0x20/0x30
[   50.168407]  sysfs_kf_write+0x48/0x70
[   50.172114]  kernfs_fop_write+0x120/0x230
[   50.176175]  __vfs_write+0x18/0x40
[   50.179619]  vfs_write+0xdc/0x1c4
[   50.182974]  ksys_write+0x68/0xf0
[   50.186329]  __arm64_sys_write+0x18/0x20
[   50.190303]  el0_svc_common.constprop.0+0x70/0x170
[   50.195151]  do_el0_svc+0x20/0x80
[   50.198507]  el0_sync_handler+0x94/0x1d0
[   50.202476]  el0_sync+0x140/0x180
[   50.206891] rcar-vin e6ef0000.video: Device registered as video11
[   50.214480] kobject (000000009d23aabd): tried to init an initialized object, something is seriously wrong.
[   50.225601] CPU: 0 PID: 399 Comm: bash Not tainted 5.8.0-rc1-arm64-renesas-00122-g6f01dfb760c027d5 #1
[   50.236162] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
[   50.245406] Call trace:
[   50.249131]  dump_backtrace+0x0/0x190
[   50.254092]  show_stack+0x14/0x20
[   50.258718]  dump_stack+0xe0/0x128
[   50.263432]  kobject_init+0x8c/0xa0
[   50.268247]  device_initialize+0x40/0x150
[   50.273604]  device_register+0x14/0x30
[   50.278710]  __video_register_device+0xcf4/0x1780
[   50.284773]  rvin_v4l2_register+0xe4/0x150
[   50.290241]  rvin_group_notify_complete+0x58/0x18c
[   50.296427]  v4l2_async_notifier_try_complete.part.0+0x50/0x60
[   50.303692]  v4l2_async_register_subdev+0xfc/0x1b0
[   50.309867]  rcsi2_probe+0x338/0x41c
[   50.314779]  platform_drv_probe+0x50/0xa0
[   50.320087]  really_probe+0xdc/0x430
[   50.324928]  driver_probe_device+0x54/0xb0
[   50.330298]  device_driver_attach+0xbc/0xc4
[   50.335759]  bind_store+0xb8/0x11c
[   50.340444]  drv_attr_store+0x20/0x30
[   50.345395]  sysfs_kf_write+0x48/0x70
[   50.350337]  kernfs_fop_write+0x120/0x230
[   50.355637]  __vfs_write+0x18/0x40
[   50.360314]  vfs_write+0xdc/0x1c4
[   50.364894]  ksys_write+0x68/0xf0
[   50.369465]  __arm64_sys_write+0x18/0x20
[   50.374643]  el0_svc_common.constprop.0+0x70/0x170
[   50.380697]  do_el0_svc+0x20/0x80
[   50.385260]  el0_sync_handler+0x94/0x1d0
[   50.390437]  el0_sync+0x140/0x180
[   50.395627] rcar-vin e6ef1000.video: Device registered as video12
[   50.403212] kobject (000000003aa48aaf): tried to init an initialized object, something is seriously wrong.
[   50.419514] CPU: 0 PID: 399 Comm: bash Not tainted 5.8.0-rc1-arm64-renesas-00122-g6f01dfb760c027d5 #1
[   50.430193] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
[   50.439576] Call trace:
[   50.443461]  dump_backtrace+0x0/0x190
[   50.448569]  show_stack+0x14/0x20
[   50.453325]  dump_stack+0xe0/0x128
[   50.458141]  kobject_init+0x8c/0xa0
[   50.463045]  device_initialize+0x40/0x150
[   50.468473]  device_register+0x14/0x30
[   50.473635]  __video_register_device+0xcf4/0x1780
[   50.479755]  rvin_v4l2_register+0xe4/0x150
[   50.485262]  rvin_group_notify_complete+0x58/0x18c
[   50.491479]  v4l2_async_notifier_try_complete.part.0+0x50/0x60
[   50.498745]  v4l2_async_register_subdev+0xfc/0x1b0
[   50.504921]  rcsi2_probe+0x338/0x41c
[   50.509834]  platform_drv_probe+0x50/0xa0
[   50.515142]  really_probe+0xdc/0x430
[   50.519983]  driver_probe_device+0x54/0xb0
[   50.525353]  device_driver_attach+0xbc/0xc4
[   50.530814]  bind_store+0xb8/0x11c
[   50.535497]  drv_attr_store+0x20/0x30
[   50.540449]  sysfs_kf_write+0x48/0x70
[   50.545392]  kernfs_fop_write+0x120/0x230
[   50.550693]  __vfs_write+0x18/0x40
[   50.555371]  vfs_write+0xdc/0x1c4
[   50.559950]  ksys_write+0x68/0xf0
[   50.564522]  __arm64_sys_write+0x18/0x20
[   50.569702]  el0_svc_common.constprop.0+0x70/0x170
[   50.575757]  do_el0_svc+0x20/0x80
[   50.580319]  el0_sync_handler+0x94/0x1d0
[   50.585494]  el0_sync+0x140/0x180
[   50.591035] rcar-vin e6ef2000.video: Device registered as video13
[   50.599329] kobject (0000000010e2c914): tried to init an initialized object, something is seriously wrong.
[   50.610935] CPU: 0 PID: 399 Comm: bash Not tainted 5.8.0-rc1-arm64-renesas-00122-g6f01dfb760c027d5 #1
[   50.621606] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
[   50.630964] Call trace:
[   50.634831]  dump_backtrace+0x0/0x190
[   50.639919]  show_stack+0x14/0x20
[   50.644675]  dump_stack+0xe0/0x128
[   50.649492]  kobject_init+0x8c/0xa0
[   50.654401]  device_initialize+0x40/0x150
[   50.659828]  device_register+0x14/0x30
[   50.664984]  __video_register_device+0xcf4/0x1780
[   50.671102]  rvin_v4l2_register+0xe4/0x150
[   50.676612]  rvin_group_notify_complete+0x58/0x18c
[   50.682833]  v4l2_async_notifier_try_complete.part.0+0x50/0x60
[   50.690103]  v4l2_async_register_subdev+0xfc/0x1b0
[   50.696281]  rcsi2_probe+0x338/0x41c
[   50.701193]  platform_drv_probe+0x50/0xa0
[   50.706503]  really_probe+0xdc/0x430
[   50.711346]  driver_probe_device+0x54/0xb0
[   50.716716]  device_driver_attach+0xbc/0xc4
[   50.722178]  bind_store+0xb8/0x11c
[   50.726861]  drv_attr_store+0x20/0x30
[   50.731813]  sysfs_kf_write+0x48/0x70
[   50.736754]  kernfs_fop_write+0x120/0x230
[   50.742053]  __vfs_write+0x18/0x40
[   50.746731]  vfs_write+0xdc/0x1c4
[   50.751311]  ksys_write+0x68/0xf0
[   50.755884]  __arm64_sys_write+0x18/0x20
[   50.761063]  el0_svc_common.constprop.0+0x70/0x170
[   50.767120]  do_el0_svc+0x20/0x80
[   50.771682]  el0_sync_handler+0x94/0x1d0
[   50.776858]  el0_sync+0x140/0x180
[   50.782393] rcar-vin e6ef3000.video: Device registered as video14
[   50.795556] kobject (00000000cedb41f6): tried to init an initialized object, something is seriously wrong.
[   50.807583] CPU: 0 PID: 399 Comm: bash Not tainted 5.8.0-rc1-arm64-renesas-00122-g6f01dfb760c027d5 #1
[   50.818256] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
[   50.827613] Call trace:
[   50.831481]  dump_backtrace+0x0/0x190
[   50.836566]  show_stack+0x14/0x20
[   50.841301]  dump_stack+0xe0/0x128
[   50.846112]  kobject_init+0x8c/0xa0
[   50.851015]  device_initialize+0x40/0x150
[   50.856443]  device_register+0x14/0x30
[   50.861600]  __video_register_device+0xcf4/0x1780
[   50.867718]  rvin_v4l2_register+0xe4/0x150
[   50.873225]  rvin_group_notify_complete+0x58/0x18c
[   50.879446]  v4l2_async_notifier_try_complete.part.0+0x50/0x60
[   50.886713]  v4l2_async_register_subdev+0xfc/0x1b0
[   50.892891]  rcsi2_probe+0x338/0x41c
[   50.897804]  platform_drv_probe+0x50/0xa0
[   50.903114]  really_probe+0xdc/0x430
[   50.907957]  driver_probe_device+0x54/0xb0
[   50.913327]  device_driver_attach+0xbc/0xc4
[   50.918789]  bind_store+0xb8/0x11c
[   50.923473]  drv_attr_store+0x20/0x30
[   50.928425]  sysfs_kf_write+0x48/0x70
[   50.933367]  kernfs_fop_write+0x120/0x230
[   50.938668]  __vfs_write+0x18/0x40
[   50.943345]  vfs_write+0xdc/0x1c4
[   50.947926]  ksys_write+0x68/0xf0
[   50.952500]  __arm64_sys_write+0x18/0x20
[   50.957681]  el0_svc_common.constprop.0+0x70/0x170
[   50.963738]  do_el0_svc+0x20/0x80
[   50.968304]  el0_sync_handler+0x94/0x1d0
[   50.973490]  el0_sync+0x140/0x180
[   50.980453] rcar-vin e6ef4000.video: Device registered as video15
[   50.989061] kobject (000000005db6c01f): tried to init an initialized object, something is seriously wrong.
[   51.003514] CPU: 0 PID: 399 Comm: bash Not tainted 5.8.0-rc1-arm64-renesas-00122-g6f01dfb760c027d5 #1
[   51.014192] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
[   51.023553] Call trace:
[   51.027422]  dump_backtrace+0x0/0x190
[   51.032510]  show_stack+0x14/0x20
[   51.037246]  dump_stack+0xe0/0x128
[   51.042061]  kobject_init+0x8c/0xa0
[   51.046967]  device_initialize+0x40/0x150
[   51.052396]  device_register+0x14/0x30
[   51.057554]  __video_register_device+0xcf4/0x1780
[   51.063674]  rvin_v4l2_register+0xe4/0x150
[   51.069182]  rvin_group_notify_complete+0x58/0x18c
[   51.075408]  v4l2_async_notifier_try_complete.part.0+0x50/0x60
[   51.082676]  v4l2_async_register_subdev+0xfc/0x1b0
[   51.088857]  rcsi2_probe+0x338/0x41c
[   51.093772]  platform_drv_probe+0x50/0xa0
[   51.099084]  really_probe+0xdc/0x430
[   51.103928]  driver_probe_device+0x54/0xb0
[   51.109299]  device_driver_attach+0xbc/0xc4
[   51.114766]  bind_store+0xb8/0x11c
[   51.119453]  drv_attr_store+0x20/0x30
[   51.124408]  sysfs_kf_write+0x48/0x70
[   51.129353]  kernfs_fop_write+0x120/0x230
[   51.134656]  __vfs_write+0x18/0x40
[   51.139336]  vfs_write+0xdc/0x1c4
[   51.143921]  ksys_write+0x68/0xf0
[   51.148496]  __arm64_sys_write+0x18/0x20
[   51.153678]  el0_svc_common.constprop.0+0x70/0x170
[   51.159735]  do_el0_svc+0x20/0x80
[   51.164300]  el0_sync_handler+0x94/0x1d0
[   51.169479]  el0_sync+0x140/0x180
[   51.175178] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[   51.185832] Mem abort info:
[   51.190414]   ESR = 0x96000044
[   51.195202]   EC = 0x25: DABT (current EL), IL = 32 bits
[   51.204679]   SET = 0, FnV = 0
[   51.209464]   EA = 0, S1PTW = 0
[   51.214841] Data abort info:
[   51.219542]   ISV = 0, ISS = 0x00000044
[   51.229557]   CM = 0, WnR = 1
[   51.235004] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000a9422000
[   51.243309] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
[   51.252227] Internal error: Oops: 96000044 [#1] PREEMPT SMP
[   51.259230] CPU: 0 PID: 399 Comm: bash Not tainted 5.8.0-rc1-arm64-renesas-00122-g6f01dfb760c027d5 #1
[   51.269914] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
[   51.279327] pstate: 60000005 (nZCv daif -PAN -UAO BTYPE=--)
[   51.286391] pc : media_create_intf_link+0x50/0xc0
[   51.292542] lr : media_create_intf_link+0x34/0xc0
[   51.298631] sp : ffff800012ecba10
[   51.303282] x29: ffff800012ecba10 x28: ffff0000735cd250 
[   51.309923] x27: ffff800011ea54c8 x26: ffff0000735cd098 
[   51.316557] x25: ffff800010f30a60 x24: 0000000000000010 
[   51.323192] x23: 0000000000000001 x22: 0000000000000000 
[   51.329831] x21: ffff0000735cd098 x20: 0000000000000003 
[   51.336468] x19: ffff00007cc5f600 x18: 0000005a215e8dc3 
[   51.343101] x17: 00016a37eb2cda4e x16: 0000000000000014 
[   51.349741] x15: ffff800010c06f60 x14: 0000000000000005 
[   51.356376] x13: 000000000000026a x12: ffff8000112c5c28 
[   51.363006] x11: 000000000000007e x10: 0000000000000001 
[   51.369628] x9 : 0000000000000000 x8 : ffff00007cc5f680 
[   51.376237] x7 : 0000000000000000 x6 : 000000000000003f 
[   51.382831] x5 : 0000000000000040 x4 : 0000000000000dc0 
[   51.389407] x3 : 0000000000000000 x2 : 0000000010000003 
[   51.395973] x1 : ffff00007cc5f620 x0 : ffff00007cc5f600 
[   51.402539] Call trace:
[   51.406223]  media_create_intf_link+0x50/0xc0
[   51.411843]  __video_register_device+0xffc/0x1780
[   51.417814]  rvin_v4l2_register+0xe4/0x150
[   51.423176]  rvin_group_notify_complete+0x58/0x18c
[   51.429252]  v4l2_async_notifier_try_complete.part.0+0x50/0x60
[   51.436390]  v4l2_async_register_subdev+0xfc/0x1b0
[   51.442479]  rcsi2_probe+0x338/0x41c
[   51.447356]  platform_drv_probe+0x50/0xa0
[   51.452672]  really_probe+0xdc/0x430
[   51.457550]  driver_probe_device+0x54/0xb0
[   51.462944]  device_driver_attach+0xbc/0xc4
[   51.468426]  bind_store+0xb8/0x11c
[   51.473116]  drv_attr_store+0x20/0x30
[   51.478077]  sysfs_kf_write+0x48/0x70
[   51.482993]  kernfs_fop_write+0x120/0x230
[   51.488229]  __vfs_write+0x18/0x40
[   51.492852]  vfs_write+0xdc/0x1c4
[   51.497375]  ksys_write+0x68/0xf0
[   51.501885]  __arm64_sys_write+0x18/0x20
[   51.507019]  el0_svc_common.constprop.0+0x70/0x170
[   51.513037]  do_el0_svc+0x20/0x80
[   51.517573]  el0_sync_handler+0x94/0x1d0
[   51.522722]  el0_sync+0x140/0x180
[   51.527237] Code: 91008262 a9020c02 32040282 f9001661 (f9000061) 
[   51.534555] ---[ end trace 94f72cbb1552c089 ]---

Patch

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 7440c89..e70f83b 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -253,7 +253,6 @@  static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
 	struct media_device *mdev = &group->mdev;
 	const struct of_device_id *match;
 	struct device_node *np;
-	int ret;
 
 	mutex_init(&group->lock);
 
@@ -266,7 +265,6 @@  static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
 	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
 
 	mdev->dev = vin->dev;
-	mdev->ops = &rvin_media_ops;
 
 	match = of_match_node(vin->dev->driver->of_match_table,
 			      vin->dev->of_node);
@@ -278,11 +276,7 @@  static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
 
 	media_device_init(mdev);
 
-	ret = media_device_register(&group->mdev);
-	if (ret)
-		rvin_group_cleanup(group);
-
-	return ret;
+	return 0;
 }
 
 static void rvin_group_release(struct kref *kref)
@@ -688,6 +682,8 @@  static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 		return ret;
 	}
 
+	vin->group->mdev.ops = &rvin_media_ops;
+
 	/* Register all video nodes for the group. */
 	for (i = 0; i < RCAR_VIN_NUM; i++) {
 		if (vin->group->vin[i] &&
@@ -736,8 +732,10 @@  static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 		}
 	}
 	mutex_unlock(&vin->group->lock);
+	if (ret)
+		return ret;
 
-	return ret;
+	return media_device_register(&vin->group->mdev);
 }
 
 static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,