diff mbox series

[v3,05/19] media: v4l2-subdev: De-deprecate init() subdev op

Message ID 20210319164148.199192-6-jacopo+renesas@jmondi.org (mailing list archive)
State New, archived
Headers show
Series media: gmsl: Reliability improvement | expand

Commit Message

Jacopo Mondi March 19, 2021, 4:41 p.m. UTC
The init() subdev core operation is deemed to be deprecated for new
subdevice drivers. However it could prove useful for complex
architectures to defer operation that require access to the
communication bus if said bus is not available (or fully configured)
at the time when the subdevice probe() function is run.

As an example, the GMSL architecture requires the GMSL configuration
link to be configured on the host side after the remote subdevice
has completed its probe function. After the configuration on the host
side has been performed, the subdevice registers can be accessed through
the communication bus.

In particular:

	HOST			REMOTE

	probe()
	   |
	   ---------------------> |
				  probe() {
				     bus config()
				  }
	   |<--------------------|
	v4l2 async bound {
	    bus config()
	    call subdev init()
	   |-------------------->|
				 init() {
				     access register on the bus()
				}
	   |<-------------------
	}

In the GMSL use case the bus configuration requires the enablement of the
noise immunity threshold on the remote side which ensures reliability
of communications in electrically noisy environments. After the subdevice
has enabled the threshold at the end of its probe() sequence the host
side shall compensate it with an higher signal amplitude. Once this
sequence has completed the bus can be accessed with noise protection
enabled and all the operations that require a considerable number of
transactions on the bus (such as the image sensor configuration
sequence) are run in the subdevice init() operation implementation.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart March 20, 2021, 3:42 p.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

CC'ing Sakari on v3 to get feedback.

On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> The init() subdev core operation is deemed to be deprecated for new
> subdevice drivers. However it could prove useful for complex
> architectures to defer operation that require access to the
> communication bus if said bus is not available (or fully configured)
> at the time when the subdevice probe() function is run.
> 
> As an example, the GMSL architecture requires the GMSL configuration
> link to be configured on the host side after the remote subdevice
> has completed its probe function. After the configuration on the host
> side has been performed, the subdevice registers can be accessed through
> the communication bus.
> 
> In particular:
> 
> 	HOST			REMOTE
> 
> 	probe()
> 	   |
> 	   ---------------------> |
> 				  probe() {
> 				     bus config()
> 				  }
> 	   |<--------------------|
> 	v4l2 async bound {
> 	    bus config()
> 	    call subdev init()
> 	   |-------------------->|
> 				 init() {
> 				     access register on the bus()
> 				}
> 	   |<-------------------
> 	}
> 
> In the GMSL use case the bus configuration requires the enablement of the
> noise immunity threshold on the remote side which ensures reliability
> of communications in electrically noisy environments. After the subdevice
> has enabled the threshold at the end of its probe() sequence the host
> side shall compensate it with an higher signal amplitude. Once this
> sequence has completed the bus can be accessed with noise protection
> enabled and all the operations that require a considerable number of
> transactions on the bus (such as the image sensor configuration
> sequence) are run in the subdevice init() operation implementation.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d0e9a5bdb08b..3068d9940669 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
>   *	each pin being configured.  This function could be called at times
>   *	other than just subdevice initialization.
>   *
> - * @init: initialize the sensor registers to some sort of reasonable default
> - *	values. Do not use for new drivers and should be removed in existing
> - *	drivers.
> + * @init: initialize the subdevice registers to some sort of reasonable default
> + *	values. Do not use for new drivers (and should be removed in existing
> + *	ones) for regular architectures where the image sensor is connected to
> + *	the host receiver. For more complex architectures where the subdevice
> + *	initialization should be deferred to the completion of the probe
> + *	sequence of some intermediate component, or the communication bus
> + *	requires configurations on the host side that depend on the completion
> + *	of the probe sequence of the remote subdevices, the usage of this
> + *	operation could be considered to allow the devices along the pipeline to
> + *	probe and register in the media graph and to defer any operation that
> + *	require actual access to the communication bus to their init() function
> + *	implementation.
>   *
>   * @load_fw: load firmware.
>   *
Sakari Ailus March 21, 2021, 8:52 p.m. UTC | #2
Hi Laurent and Jacopo,

Thanks for cc'ing me.

On Sat, Mar 20, 2021 at 05:42:12PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> CC'ing Sakari on v3 to get feedback.
> 
> On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> > The init() subdev core operation is deemed to be deprecated for new
> > subdevice drivers. However it could prove useful for complex
> > architectures to defer operation that require access to the
> > communication bus if said bus is not available (or fully configured)
> > at the time when the subdevice probe() function is run.
> > 
> > As an example, the GMSL architecture requires the GMSL configuration
> > link to be configured on the host side after the remote subdevice
> > has completed its probe function. After the configuration on the host
> > side has been performed, the subdevice registers can be accessed through
> > the communication bus.

What does the remote device's probe do that needs to be done before bus
config on the host side?

Alternatively, could the remote init() work be done at the time streaming
is started?

> > 
> > In particular:
> > 
> > 	HOST			REMOTE
> > 
> > 	probe()
> > 	   |
> > 	   ---------------------> |
> > 				  probe() {
> > 				     bus config()
> > 				  }
> > 	   |<--------------------|
> > 	v4l2 async bound {
> > 	    bus config()
> > 	    call subdev init()
> > 	   |-------------------->|
> > 				 init() {
> > 				     access register on the bus()
> > 				}
> > 	   |<-------------------
> > 	}
> > 
> > In the GMSL use case the bus configuration requires the enablement of the
> > noise immunity threshold on the remote side which ensures reliability
> > of communications in electrically noisy environments. After the subdevice
> > has enabled the threshold at the end of its probe() sequence the host
> > side shall compensate it with an higher signal amplitude. Once this
> > sequence has completed the bus can be accessed with noise protection
> > enabled and all the operations that require a considerable number of
> > transactions on the bus (such as the image sensor configuration
> > sequence) are run in the subdevice init() operation implementation.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index d0e9a5bdb08b..3068d9940669 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> >   *	each pin being configured.  This function could be called at times
> >   *	other than just subdevice initialization.
> >   *
> > - * @init: initialize the sensor registers to some sort of reasonable default
> > - *	values. Do not use for new drivers and should be removed in existing
> > - *	drivers.
> > + * @init: initialize the subdevice registers to some sort of reasonable default
> > + *	values. Do not use for new drivers (and should be removed in existing
> > + *	ones) for regular architectures where the image sensor is connected to
> > + *	the host receiver. For more complex architectures where the subdevice
> > + *	initialization should be deferred to the completion of the probe
> > + *	sequence of some intermediate component, or the communication bus
> > + *	requires configurations on the host side that depend on the completion
> > + *	of the probe sequence of the remote subdevices, the usage of this
> > + *	operation could be considered to allow the devices along the pipeline to
> > + *	probe and register in the media graph and to defer any operation that
> > + *	require actual access to the communication bus to their init() function
> > + *	implementation.
> >   *
> >   * @load_fw: load firmware.
> >   *
Jacopo Mondi March 22, 2021, 12:51 p.m. UTC | #3
Hi Sakari,

On Sun, Mar 21, 2021 at 10:52:56PM +0200, Sakari Ailus wrote:
> Hi Laurent and Jacopo,
>
> Thanks for cc'ing me.
>
> On Sat, Mar 20, 2021 at 05:42:12PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > CC'ing Sakari on v3 to get feedback.
> >
> > On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> > > The init() subdev core operation is deemed to be deprecated for new
> > > subdevice drivers. However it could prove useful for complex
> > > architectures to defer operation that require access to the
> > > communication bus if said bus is not available (or fully configured)
> > > at the time when the subdevice probe() function is run.
> > >
> > > As an example, the GMSL architecture requires the GMSL configuration
> > > link to be configured on the host side after the remote subdevice
> > > has completed its probe function. After the configuration on the host
> > > side has been performed, the subdevice registers can be accessed through
> > > the communication bus.
>
> What does the remote device's probe do that needs to be done before bus
> config on the host side?

A few lines here below:

 In the GMSL use case the bus configuration requires the enablement of the
 noise immunity threshold on the remote side which ensures reliability
 of communications in electrically noisy environments. After the subdevice
 has enabled the threshold at the end of its probe() sequence the host
 side shall compensate it with an higher signal amplitude. Once this
 sequence has completed the bus can be accessed with noise protection
 enabled and all the operations that require a considerable number of
 transactions on the bus (such as the image sensor configuration
 sequence) are run in the subdevice init() operation implementation.

>
> Alternatively, could the remote init() work be done at the time streaming
> is started?

That would require programing the sensor, the embedded ISP at s_stream
time which would take some time.

I'll take this suggestion into account though and run some more tests.

Thanks
  j

>
> > >
> > > In particular:
> > >
> > > 	HOST			REMOTE
> > >
> > > 	probe()
> > > 	   |
> > > 	   ---------------------> |
> > > 				  probe() {
> > > 				     bus config()
> > > 				  }
> > > 	   |<--------------------|
> > > 	v4l2 async bound {
> > > 	    bus config()
> > > 	    call subdev init()
> > > 	   |-------------------->|
> > > 				 init() {
> > > 				     access register on the bus()
> > > 				}
> > > 	   |<-------------------
> > > 	}
> > >
> > > In the GMSL use case the bus configuration requires the enablement of the
> > > noise immunity threshold on the remote side which ensures reliability
> > > of communications in electrically noisy environments. After the subdevice
> > > has enabled the threshold at the end of its probe() sequence the host
> > > side shall compensate it with an higher signal amplitude. Once this
> > > sequence has completed the bus can be accessed with noise protection
> > > enabled and all the operations that require a considerable number of
> > > transactions on the bus (such as the image sensor configuration
> > > sequence) are run in the subdevice init() operation implementation.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  include/media/v4l2-subdev.h | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index d0e9a5bdb08b..3068d9940669 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> > >   *	each pin being configured.  This function could be called at times
> > >   *	other than just subdevice initialization.
> > >   *
> > > - * @init: initialize the sensor registers to some sort of reasonable default
> > > - *	values. Do not use for new drivers and should be removed in existing
> > > - *	drivers.
> > > + * @init: initialize the subdevice registers to some sort of reasonable default
> > > + *	values. Do not use for new drivers (and should be removed in existing
> > > + *	ones) for regular architectures where the image sensor is connected to
> > > + *	the host receiver. For more complex architectures where the subdevice
> > > + *	initialization should be deferred to the completion of the probe
> > > + *	sequence of some intermediate component, or the communication bus
> > > + *	requires configurations on the host side that depend on the completion
> > > + *	of the probe sequence of the remote subdevices, the usage of this
> > > + *	operation could be considered to allow the devices along the pipeline to
> > > + *	probe and register in the media graph and to defer any operation that
> > > + *	require actual access to the communication bus to their init() function
> > > + *	implementation.
> > >   *
> > >   * @load_fw: load firmware.
> > >   *
>
> --
> Kind regards,
>
> Sakari Ailus
Jacopo Mondi March 26, 2021, 11:37 a.m. UTC | #4
Hi Sakari,

On Mon, Mar 22, 2021 at 01:51:44PM +0100, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Sun, Mar 21, 2021 at 10:52:56PM +0200, Sakari Ailus wrote:
> > Hi Laurent and Jacopo,
> >
> > Thanks for cc'ing me.
> >
> > On Sat, Mar 20, 2021 at 05:42:12PM +0200, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> > >
> > > CC'ing Sakari on v3 to get feedback.
> > >
> > > On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> > > > The init() subdev core operation is deemed to be deprecated for new
> > > > subdevice drivers. However it could prove useful for complex
> > > > architectures to defer operation that require access to the
> > > > communication bus if said bus is not available (or fully configured)
> > > > at the time when the subdevice probe() function is run.
> > > >
> > > > As an example, the GMSL architecture requires the GMSL configuration
> > > > link to be configured on the host side after the remote subdevice
> > > > has completed its probe function. After the configuration on the host
> > > > side has been performed, the subdevice registers can be accessed through
> > > > the communication bus.
> >
> > What does the remote device's probe do that needs to be done before bus
> > config on the host side?
>
> A few lines here below:
>
>  In the GMSL use case the bus configuration requires the enablement of the
>  noise immunity threshold on the remote side which ensures reliability
>  of communications in electrically noisy environments. After the subdevice
>  has enabled the threshold at the end of its probe() sequence the host
>  side shall compensate it with an higher signal amplitude. Once this
>  sequence has completed the bus can be accessed with noise protection
>  enabled and all the operations that require a considerable number of
>  transactions on the bus (such as the image sensor configuration
>  sequence) are run in the subdevice init() operation implementation.
>
> >
> > Alternatively, could the remote init() work be done at the time streaming
> > is started?
>
> That would require programing the sensor, the embedded ISP at s_stream
> time which would take some time.

I'm afraid but from my testing also the chip identification is more
reliable if run in init(). As identifying chips is something that has
to happen at probe/initialization I fear it is not possible to move it
to s_stream time.

>
> I'll take this suggestion into account though and run some more tests.
>
> Thanks
>   j
>
> >
> > > >
> > > > In particular:
> > > >
> > > > 	HOST			REMOTE
> > > >
> > > > 	probe()
> > > > 	   |
> > > > 	   ---------------------> |
> > > > 				  probe() {
> > > > 				     bus config()
> > > > 				  }
> > > > 	   |<--------------------|
> > > > 	v4l2 async bound {
> > > > 	    bus config()
> > > > 	    call subdev init()
> > > > 	   |-------------------->|
> > > > 				 init() {
> > > > 				     access register on the bus()
> > > > 				}
> > > > 	   |<-------------------
> > > > 	}
> > > >
> > > > In the GMSL use case the bus configuration requires the enablement of the
> > > > noise immunity threshold on the remote side which ensures reliability
> > > > of communications in electrically noisy environments. After the subdevice
> > > > has enabled the threshold at the end of its probe() sequence the host
> > > > side shall compensate it with an higher signal amplitude. Once this
> > > > sequence has completed the bus can be accessed with noise protection
> > > > enabled and all the operations that require a considerable number of
> > > > transactions on the bus (such as the image sensor configuration
> > > > sequence) are run in the subdevice init() operation implementation.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  include/media/v4l2-subdev.h | 15 ++++++++++++---
> > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index d0e9a5bdb08b..3068d9940669 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> > > >   *	each pin being configured.  This function could be called at times
> > > >   *	other than just subdevice initialization.
> > > >   *
> > > > - * @init: initialize the sensor registers to some sort of reasonable default
> > > > - *	values. Do not use for new drivers and should be removed in existing
> > > > - *	drivers.
> > > > + * @init: initialize the subdevice registers to some sort of reasonable default
> > > > + *	values. Do not use for new drivers (and should be removed in existing
> > > > + *	ones) for regular architectures where the image sensor is connected to
> > > > + *	the host receiver. For more complex architectures where the subdevice
> > > > + *	initialization should be deferred to the completion of the probe
> > > > + *	sequence of some intermediate component, or the communication bus
> > > > + *	requires configurations on the host side that depend on the completion
> > > > + *	of the probe sequence of the remote subdevices, the usage of this
> > > > + *	operation could be considered to allow the devices along the pipeline to
> > > > + *	probe and register in the media graph and to defer any operation that
> > > > + *	require actual access to the communication bus to their init() function
> > > > + *	implementation.
> > > >   *
> > > >   * @load_fw: load firmware.
> > > >   *
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus
diff mbox series

Patch

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d0e9a5bdb08b..3068d9940669 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -148,9 +148,18 @@  struct v4l2_subdev_io_pin_config {
  *	each pin being configured.  This function could be called at times
  *	other than just subdevice initialization.
  *
- * @init: initialize the sensor registers to some sort of reasonable default
- *	values. Do not use for new drivers and should be removed in existing
- *	drivers.
+ * @init: initialize the subdevice registers to some sort of reasonable default
+ *	values. Do not use for new drivers (and should be removed in existing
+ *	ones) for regular architectures where the image sensor is connected to
+ *	the host receiver. For more complex architectures where the subdevice
+ *	initialization should be deferred to the completion of the probe
+ *	sequence of some intermediate component, or the communication bus
+ *	requires configurations on the host side that depend on the completion
+ *	of the probe sequence of the remote subdevices, the usage of this
+ *	operation could be considered to allow the devices along the pipeline to
+ *	probe and register in the media graph and to defer any operation that
+ *	require actual access to the communication bus to their init() function
+ *	implementation.
  *
  * @load_fw: load firmware.
  *