diff mbox series

[RFC,4/4] staging: media: omap4iss: use v4l2_get_link_freq() to get the external rate

Message ID 20210303180817.12285-5-andrey.konovalov@linaro.org
State New
Headers show
Series use v4l2_get_link_freq() in CSI receiver drivers | expand

Commit Message

Andrey Konovalov March 3, 2021, 6:08 p.m. UTC 
This driver uses V4L2_CID_PIXEL_RATE to calculate the CSI2 link frequency,
but this may give incorrect result in some cases. Use v4l2_get_link_freq()
instead.

Also the driver used the external_rate field in struct iss_pipeline as a
flag to prevent excessive v4l2_subdev_call's when processing the frames
in single-shot mode. Replace the external_rate with external_lfreq, and
use external_bpp and external_lfreq to call v4l2_subdev_call(get_fmt) and
v4l2_get_link_freq() respectively only once per iss_video_streamon().

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>

---
 drivers/staging/media/omap4iss/iss.c        | 12 +-----------
 drivers/staging/media/omap4iss/iss_csiphy.c | 19 ++++++++++++++++---
 drivers/staging/media/omap4iss/iss_video.c  |  2 +-
 drivers/staging/media/omap4iss/iss_video.h  |  2 +-
 4 files changed, 19 insertions(+), 16 deletions(-)

-- 
2.17.1

Comments

Sakari Ailus March 5, 2021, 3:41 p.m. UTC | #1 
Hi Andrey,

Thanks for the set.

On Wed, Mar 03, 2021 at 09:08:17PM +0300, Andrey Konovalov wrote:
> This driver uses V4L2_CID_PIXEL_RATE to calculate the CSI2 link frequency,

> but this may give incorrect result in some cases. Use v4l2_get_link_freq()

> instead.

> 

> Also the driver used the external_rate field in struct iss_pipeline as a

> flag to prevent excessive v4l2_subdev_call's when processing the frames

> in single-shot mode. Replace the external_rate with external_lfreq, and

> use external_bpp and external_lfreq to call v4l2_subdev_call(get_fmt) and

> v4l2_get_link_freq() respectively only once per iss_video_streamon().

> 

> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>

> ---

>  drivers/staging/media/omap4iss/iss.c        | 12 +-----------

>  drivers/staging/media/omap4iss/iss_csiphy.c | 19 ++++++++++++++++---

>  drivers/staging/media/omap4iss/iss_video.c  |  2 +-

>  drivers/staging/media/omap4iss/iss_video.h  |  2 +-

>  4 files changed, 19 insertions(+), 16 deletions(-)

> 

> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c

> index dae9073e7d3c..0eb7b1b5dcc4 100644

> --- a/drivers/staging/media/omap4iss/iss.c

> +++ b/drivers/staging/media/omap4iss/iss.c

> @@ -131,7 +131,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,

>  	if (!pipe->external)

>  		return 0;

>  

> -	if (pipe->external_rate)

> +	if (pipe->external_bpp)

>  		return 0;

>  

>  	memset(&fmt, 0, sizeof(fmt));

> @@ -145,16 +145,6 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,

>  

>  	pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)->bpp;

>  

> -	ctrl = v4l2_ctrl_find(pipe->external->ctrl_handler,

> -			      V4L2_CID_PIXEL_RATE);

> -	if (!ctrl) {

> -		dev_warn(iss->dev, "no pixel rate control in subdev %s\n",

> -			 pipe->external->name);

> -		return -EPIPE;

> -	}

> -

> -	pipe->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);

> -

>  	return 0;

>  }

>  

> diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c

> index 96f2ce045138..cec0cd21f7e0 100644

> --- a/drivers/staging/media/omap4iss/iss_csiphy.c

> +++ b/drivers/staging/media/omap4iss/iss_csiphy.c

> @@ -119,6 +119,7 @@ int omap4iss_csiphy_config(struct iss_device *iss,

>  	struct iss_pipeline *pipe = to_iss_pipeline(&csi2_subdev->entity);

>  	struct iss_v4l2_subdevs_group *subdevs = pipe->external->host_priv;

>  	struct iss_csiphy_dphy_cfg csi2phy;

> +	s64 link_freq;

>  	int csi2_ddrclk_khz;

>  	struct iss_csiphy_lanes_cfg *lanes;

>  	unsigned int used_lanes = 0;

> @@ -193,9 +194,21 @@ int omap4iss_csiphy_config(struct iss_device *iss,

>  	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))

>  		return -EINVAL;

>  

> -	csi2_ddrclk_khz = pipe->external_rate / 1000

> -		/ (2 * csi2->phy->used_data_lanes)

> -		* pipe->external_bpp;

> +	if (!pipe->external_lfreq) {

> +		link_freq = v4l2_get_link_freq(pipe->external->ctrl_handler,


I wonder if you could this unconditionally, and remove external_lfreq field
altogether. The same could be done for external_bpp but that's out of scope
for this patch.

> +					       pipe->external_bpp,

> +					       2 * csi2->phy->used_data_lanes);

> +		if (link_freq < 0) {

> +			dev_warn(iss->dev,

> +				 "failed to read the link frequency fromn subdev %s\n",

> +				 pipe->external->name);

> +			return -EINVAL;

> +		}

> +

> +		pipe->external_lfreq = link_freq;

> +	}

> +

> +	csi2_ddrclk_khz = div_s64(pipe->external_lfreq, 1000);

>  

>  	/*

>  	 * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.

> diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c

> index 66975a37dc85..a654c8d18bbc 100644

> --- a/drivers/staging/media/omap4iss/iss_video.c

> +++ b/drivers/staging/media/omap4iss/iss_video.c

> @@ -872,7 +872,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)

>  	pipe = entity->pipe

>  	     ? to_iss_pipeline(entity) : &video->pipe;

>  	pipe->external = NULL;

> -	pipe->external_rate = 0;

> +	pipe->external_lfreq = 0;

>  	pipe->external_bpp = 0;

>  

>  	ret = media_entity_enum_init(&pipe->ent_enum, entity->graph_obj.mdev);

> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h

> index 526281bf0051..2ad5c8483958 100644

> --- a/drivers/staging/media/omap4iss/iss_video.h

> +++ b/drivers/staging/media/omap4iss/iss_video.h

> @@ -86,7 +86,7 @@ struct iss_pipeline {

>  	bool error;

>  	struct v4l2_fract max_timeperframe;

>  	struct v4l2_subdev *external;

> -	unsigned int external_rate;

> +	s64 external_lfreq;

>  	int external_bpp;

>  };

>  


-- 
Sakari Ailus
Andrey Konovalov March 9, 2021, 11:24 a.m. UTC | #2 
Hi Sakari,

On 05.03.2021 18:41, Sakari Ailus wrote:
> Hi Andrey,

> 

> Thanks for the set.

> 

> On Wed, Mar 03, 2021 at 09:08:17PM +0300, Andrey Konovalov wrote:

>> This driver uses V4L2_CID_PIXEL_RATE to calculate the CSI2 link frequency,

>> but this may give incorrect result in some cases. Use v4l2_get_link_freq()

>> instead.

>>

>> Also the driver used the external_rate field in struct iss_pipeline as a

>> flag to prevent excessive v4l2_subdev_call's when processing the frames

>> in single-shot mode. Replace the external_rate with external_lfreq, and

>> use external_bpp and external_lfreq to call v4l2_subdev_call(get_fmt) and

>> v4l2_get_link_freq() respectively only once per iss_video_streamon().

>>

>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>

>> ---

>>   drivers/staging/media/omap4iss/iss.c        | 12 +-----------

>>   drivers/staging/media/omap4iss/iss_csiphy.c | 19 ++++++++++++++++---

>>   drivers/staging/media/omap4iss/iss_video.c  |  2 +-

>>   drivers/staging/media/omap4iss/iss_video.h  |  2 +-

>>   4 files changed, 19 insertions(+), 16 deletions(-)

>>

>> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c

>> index dae9073e7d3c..0eb7b1b5dcc4 100644

>> --- a/drivers/staging/media/omap4iss/iss.c

>> +++ b/drivers/staging/media/omap4iss/iss.c

>> @@ -131,7 +131,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,

>>   	if (!pipe->external)

>>   		return 0;

>>   

>> -	if (pipe->external_rate)

>> +	if (pipe->external_bpp)

>>   		return 0;

>>   

>>   	memset(&fmt, 0, sizeof(fmt));

>> @@ -145,16 +145,6 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,

>>   

>>   	pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)->bpp;

>>   

>> -	ctrl = v4l2_ctrl_find(pipe->external->ctrl_handler,

>> -			      V4L2_CID_PIXEL_RATE);

>> -	if (!ctrl) {

>> -		dev_warn(iss->dev, "no pixel rate control in subdev %s\n",

>> -			 pipe->external->name);

>> -		return -EPIPE;

>> -	}

>> -

>> -	pipe->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);

>> -

>>   	return 0;

>>   }

>>   

>> diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c

>> index 96f2ce045138..cec0cd21f7e0 100644

>> --- a/drivers/staging/media/omap4iss/iss_csiphy.c

>> +++ b/drivers/staging/media/omap4iss/iss_csiphy.c

>> @@ -119,6 +119,7 @@ int omap4iss_csiphy_config(struct iss_device *iss,

>>   	struct iss_pipeline *pipe = to_iss_pipeline(&csi2_subdev->entity);

>>   	struct iss_v4l2_subdevs_group *subdevs = pipe->external->host_priv;

>>   	struct iss_csiphy_dphy_cfg csi2phy;

>> +	s64 link_freq;

>>   	int csi2_ddrclk_khz;

>>   	struct iss_csiphy_lanes_cfg *lanes;

>>   	unsigned int used_lanes = 0;

>> @@ -193,9 +194,21 @@ int omap4iss_csiphy_config(struct iss_device *iss,

>>   	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))

>>   		return -EINVAL;

>>   

>> -	csi2_ddrclk_khz = pipe->external_rate / 1000

>> -		/ (2 * csi2->phy->used_data_lanes)

>> -		* pipe->external_bpp;

>> +	if (!pipe->external_lfreq) {

>> +		link_freq = v4l2_get_link_freq(pipe->external->ctrl_handler,

> 

> I wonder if you could this unconditionally, and remove external_lfreq field

> altogether. The same could be done for external_bpp but that's out of scope

> for this patch.


Hard to tell.
This might be possible as all this logic to prevent multiple v4l2_subdev_call(get_fmt)
and v4l2_get_link_freq() calls per single iss_video_streamon() seems to be needed
only when ISS operates in memory-to-memory mode. Not sure how link frequency, and
used_lanes are related to memory-to-memory mode... Will try to find out.

Thanks,
Andrey

>> +					       pipe->external_bpp,

>> +					       2 * csi2->phy->used_data_lanes);

>> +		if (link_freq < 0) {

>> +			dev_warn(iss->dev,

>> +				 "failed to read the link frequency fromn subdev %s\n",

>> +				 pipe->external->name);

>> +			return -EINVAL;

>> +		}

>> +

>> +		pipe->external_lfreq = link_freq;

>> +	}

>> +

>> +	csi2_ddrclk_khz = div_s64(pipe->external_lfreq, 1000);

>>   

>>   	/*

>>   	 * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.

>> diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c

>> index 66975a37dc85..a654c8d18bbc 100644

>> --- a/drivers/staging/media/omap4iss/iss_video.c

>> +++ b/drivers/staging/media/omap4iss/iss_video.c

>> @@ -872,7 +872,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)

>>   	pipe = entity->pipe

>>   	     ? to_iss_pipeline(entity) : &video->pipe;

>>   	pipe->external = NULL;

>> -	pipe->external_rate = 0;

>> +	pipe->external_lfreq = 0;

>>   	pipe->external_bpp = 0;

>>   

>>   	ret = media_entity_enum_init(&pipe->ent_enum, entity->graph_obj.mdev);

>> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h

>> index 526281bf0051..2ad5c8483958 100644

>> --- a/drivers/staging/media/omap4iss/iss_video.h

>> +++ b/drivers/staging/media/omap4iss/iss_video.h

>> @@ -86,7 +86,7 @@ struct iss_pipeline {

>>   	bool error;

>>   	struct v4l2_fract max_timeperframe;

>>   	struct v4l2_subdev *external;

>> -	unsigned int external_rate;

>> +	s64 external_lfreq;

>>   	int external_bpp;

>>   };

>>   

>
Sakari Ailus March 23, 2021, 12:54 p.m. UTC | #3 
Hi Andrey,

On Tue, Mar 09, 2021 at 02:24:41PM +0300, Andrey Konovalov wrote:
> Hi Sakari,

> 

> On 05.03.2021 18:41, Sakari Ailus wrote:

> > Hi Andrey,

> > 

> > Thanks for the set.

> > 

> > On Wed, Mar 03, 2021 at 09:08:17PM +0300, Andrey Konovalov wrote:

> > > This driver uses V4L2_CID_PIXEL_RATE to calculate the CSI2 link frequency,

> > > but this may give incorrect result in some cases. Use v4l2_get_link_freq()

> > > instead.

> > > 

> > > Also the driver used the external_rate field in struct iss_pipeline as a

> > > flag to prevent excessive v4l2_subdev_call's when processing the frames

> > > in single-shot mode. Replace the external_rate with external_lfreq, and

> > > use external_bpp and external_lfreq to call v4l2_subdev_call(get_fmt) and

> > > v4l2_get_link_freq() respectively only once per iss_video_streamon().

> > > 

> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>

> > > ---

> > >   drivers/staging/media/omap4iss/iss.c        | 12 +-----------

> > >   drivers/staging/media/omap4iss/iss_csiphy.c | 19 ++++++++++++++++---

> > >   drivers/staging/media/omap4iss/iss_video.c  |  2 +-

> > >   drivers/staging/media/omap4iss/iss_video.h  |  2 +-

> > >   4 files changed, 19 insertions(+), 16 deletions(-)

> > > 

> > > diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c

> > > index dae9073e7d3c..0eb7b1b5dcc4 100644

> > > --- a/drivers/staging/media/omap4iss/iss.c

> > > +++ b/drivers/staging/media/omap4iss/iss.c

> > > @@ -131,7 +131,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,

> > >   	if (!pipe->external)

> > >   		return 0;

> > > -	if (pipe->external_rate)

> > > +	if (pipe->external_bpp)

> > >   		return 0;

> > >   	memset(&fmt, 0, sizeof(fmt));

> > > @@ -145,16 +145,6 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,

> > >   	pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)->bpp;

> > > -	ctrl = v4l2_ctrl_find(pipe->external->ctrl_handler,

> > > -			      V4L2_CID_PIXEL_RATE);

> > > -	if (!ctrl) {

> > > -		dev_warn(iss->dev, "no pixel rate control in subdev %s\n",

> > > -			 pipe->external->name);

> > > -		return -EPIPE;

> > > -	}

> > > -

> > > -	pipe->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);

> > > -

> > >   	return 0;

> > >   }

> > > diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c

> > > index 96f2ce045138..cec0cd21f7e0 100644

> > > --- a/drivers/staging/media/omap4iss/iss_csiphy.c

> > > +++ b/drivers/staging/media/omap4iss/iss_csiphy.c

> > > @@ -119,6 +119,7 @@ int omap4iss_csiphy_config(struct iss_device *iss,

> > >   	struct iss_pipeline *pipe = to_iss_pipeline(&csi2_subdev->entity);

> > >   	struct iss_v4l2_subdevs_group *subdevs = pipe->external->host_priv;

> > >   	struct iss_csiphy_dphy_cfg csi2phy;

> > > +	s64 link_freq;

> > >   	int csi2_ddrclk_khz;

> > >   	struct iss_csiphy_lanes_cfg *lanes;

> > >   	unsigned int used_lanes = 0;

> > > @@ -193,9 +194,21 @@ int omap4iss_csiphy_config(struct iss_device *iss,

> > >   	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))

> > >   		return -EINVAL;

> > > -	csi2_ddrclk_khz = pipe->external_rate / 1000

> > > -		/ (2 * csi2->phy->used_data_lanes)

> > > -		* pipe->external_bpp;

> > > +	if (!pipe->external_lfreq) {

> > > +		link_freq = v4l2_get_link_freq(pipe->external->ctrl_handler,

> > 

> > I wonder if you could this unconditionally, and remove external_lfreq field

> > altogether. The same could be done for external_bpp but that's out of scope

> > for this patch.

> 

> Hard to tell.

> This might be possible as all this logic to prevent multiple v4l2_subdev_call(get_fmt)

> and v4l2_get_link_freq() calls per single iss_video_streamon() seems to be needed

> only when ISS operates in memory-to-memory mode. Not sure how link frequency, and

> used_lanes are related to memory-to-memory mode... Will try to find out.


It seems the same pattern is used in the omap3isp driver. Both should be
changed at the same time. May well be out of scope now.

-- 
Kind regards,

Sakari Ailus
diff mbox series

Patch

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index dae9073e7d3c..0eb7b1b5dcc4 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -131,7 +131,7 @@  int omap4iss_get_external_info(struct iss_pipeline *pipe,
 	if (!pipe->external)
 		return 0;
 
-	if (pipe->external_rate)
+	if (pipe->external_bpp)
 		return 0;
 
 	memset(&fmt, 0, sizeof(fmt));
@@ -145,16 +145,6 @@  int omap4iss_get_external_info(struct iss_pipeline *pipe,
 
 	pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)->bpp;
 
-	ctrl = v4l2_ctrl_find(pipe->external->ctrl_handler,
-			      V4L2_CID_PIXEL_RATE);
-	if (!ctrl) {
-		dev_warn(iss->dev, "no pixel rate control in subdev %s\n",
-			 pipe->external->name);
-		return -EPIPE;
-	}
-
-	pipe->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);
-
 	return 0;
 }
 
diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
index 96f2ce045138..cec0cd21f7e0 100644
--- a/drivers/staging/media/omap4iss/iss_csiphy.c
+++ b/drivers/staging/media/omap4iss/iss_csiphy.c
@@ -119,6 +119,7 @@  int omap4iss_csiphy_config(struct iss_device *iss,
 	struct iss_pipeline *pipe = to_iss_pipeline(&csi2_subdev->entity);
 	struct iss_v4l2_subdevs_group *subdevs = pipe->external->host_priv;
 	struct iss_csiphy_dphy_cfg csi2phy;
+	s64 link_freq;
 	int csi2_ddrclk_khz;
 	struct iss_csiphy_lanes_cfg *lanes;
 	unsigned int used_lanes = 0;
@@ -193,9 +194,21 @@  int omap4iss_csiphy_config(struct iss_device *iss,
 	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
 		return -EINVAL;
 
-	csi2_ddrclk_khz = pipe->external_rate / 1000
-		/ (2 * csi2->phy->used_data_lanes)
-		* pipe->external_bpp;
+	if (!pipe->external_lfreq) {
+		link_freq = v4l2_get_link_freq(pipe->external->ctrl_handler,
+					       pipe->external_bpp,
+					       2 * csi2->phy->used_data_lanes);
+		if (link_freq < 0) {
+			dev_warn(iss->dev,
+				 "failed to read the link frequency fromn subdev %s\n",
+				 pipe->external->name);
+			return -EINVAL;
+		}
+
+		pipe->external_lfreq = link_freq;
+	}
+
+	csi2_ddrclk_khz = div_s64(pipe->external_lfreq, 1000);
 
 	/*
 	 * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.
diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
index 66975a37dc85..a654c8d18bbc 100644
--- a/drivers/staging/media/omap4iss/iss_video.c
+++ b/drivers/staging/media/omap4iss/iss_video.c
@@ -872,7 +872,7 @@  iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	pipe = entity->pipe
 	     ? to_iss_pipeline(entity) : &video->pipe;
 	pipe->external = NULL;
-	pipe->external_rate = 0;
+	pipe->external_lfreq = 0;
 	pipe->external_bpp = 0;
 
 	ret = media_entity_enum_init(&pipe->ent_enum, entity->graph_obj.mdev);
diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
index 526281bf0051..2ad5c8483958 100644
--- a/drivers/staging/media/omap4iss/iss_video.h
+++ b/drivers/staging/media/omap4iss/iss_video.h
@@ -86,7 +86,7 @@  struct iss_pipeline {
 	bool error;
 	struct v4l2_fract max_timeperframe;
 	struct v4l2_subdev *external;
-	unsigned int external_rate;
+	s64 external_lfreq;
 	int external_bpp;
 };