[v1,1/9] media: vimc: Don't iterate over single pad

Message ID 20240424235741.17093-2-laurent.pinchart@ideasonboard.com (mailing list archive)
State New
Headers
Series media: vimc improvements |

Commit Message

Laurent Pinchart April 24, 2024, 11:57 p.m. UTC
  The .init_state() operations of the debayer and sensor entities iterate
over the entity's pads. In practice, the iteration covers a single pad
only. Access the pad directly and remove the loops.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/test-drivers/vimc/vimc-debayer.c |  9 +++------
 drivers/media/test-drivers/vimc/vimc-sensor.c  | 10 +++-------
 2 files changed, 6 insertions(+), 13 deletions(-)
  

Comments

Shuah Khan May 30, 2024, 7:27 p.m. UTC | #1
On 4/24/24 17:57, Laurent Pinchart wrote:
> The .init_state() operations of the debayer and sensor entities iterate
> over the entity's pads. In practice, the iteration covers a single pad
> only. Access the pad directly and remove the loops.
> 

I am not seeing much of a reason to do this. This code is good
even when num_pads change.

Don't change the loops.

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/test-drivers/vimc/vimc-debayer.c |  9 +++------
>   drivers/media/test-drivers/vimc/vimc-sensor.c  | 10 +++-------
>   2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
> index d72ed086e00b..e1bf6db73050 100644
> --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
> @@ -155,16 +155,13 @@ static int vimc_debayer_init_state(struct v4l2_subdev *sd,
>   {
>   	struct vimc_debayer_device *vdebayer = v4l2_get_subdevdata(sd);
>   	struct v4l2_mbus_framefmt *mf;
> -	unsigned int i;
>   
>   	mf = v4l2_subdev_state_get_format(sd_state, 0);
>   	*mf = sink_fmt_default;
>   
> -	for (i = 1; i < sd->entity.num_pads; i++) {
> -		mf = v4l2_subdev_state_get_format(sd_state, i);
> -		*mf = sink_fmt_default;
> -		mf->code = vdebayer->src_code;
> -	}
> +	mf = v4l2_subdev_state_get_format(sd_state, 1);
> +	*mf = sink_fmt_default;
> +	mf->code = vdebayer->src_code;
>   
>   	return 0;
>   }
> diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
> index 5e34b1aed95e..b535b3ffecff 100644
> --- a/drivers/media/test-drivers/vimc/vimc-sensor.c
> +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
> @@ -44,14 +44,10 @@ static const struct v4l2_mbus_framefmt fmt_default = {
>   static int vimc_sensor_init_state(struct v4l2_subdev *sd,
>   				  struct v4l2_subdev_state *sd_state)
>   {
> -	unsigned int i;
> +	struct v4l2_mbus_framefmt *mf;
>   
> -	for (i = 0; i < sd->entity.num_pads; i++) {
> -		struct v4l2_mbus_framefmt *mf;
> -
> -		mf = v4l2_subdev_state_get_format(sd_state, i);
> -		*mf = fmt_default;
> -	}
> +	mf = v4l2_subdev_state_get_format(sd_state, 0);
> +	*mf = fmt_default;
>   
>   	return 0;
>   }

thanks,
-- Shuah
  
Laurent Pinchart May 30, 2024, 7:45 p.m. UTC | #2
Hi Shuah,

On Thu, May 30, 2024 at 01:27:53PM -0600, Shuah Khan wrote:
> On 4/24/24 17:57, Laurent Pinchart wrote:
> > The .init_state() operations of the debayer and sensor entities iterate
> > over the entity's pads. In practice, the iteration covers a single pad
> > only. Access the pad directly and remove the loops.
> 
> I am not seeing much of a reason to do this. This code is good
> even when num_pads change.
> 
> Don't change the loops.

Why so ? Beside the fact that the loop wastes some CPU cycles, the
current code implies that there would be multiple source pads, which is
confusing for the reader. I think the result of this patch is both
improved efficiency and improved readability.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/media/test-drivers/vimc/vimc-debayer.c |  9 +++------
> >   drivers/media/test-drivers/vimc/vimc-sensor.c  | 10 +++-------
> >   2 files changed, 6 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
> > index d72ed086e00b..e1bf6db73050 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
> > @@ -155,16 +155,13 @@ static int vimc_debayer_init_state(struct v4l2_subdev *sd,
> >   {
> >   	struct vimc_debayer_device *vdebayer = v4l2_get_subdevdata(sd);
> >   	struct v4l2_mbus_framefmt *mf;
> > -	unsigned int i;
> >   
> >   	mf = v4l2_subdev_state_get_format(sd_state, 0);
> >   	*mf = sink_fmt_default;
> >   
> > -	for (i = 1; i < sd->entity.num_pads; i++) {
> > -		mf = v4l2_subdev_state_get_format(sd_state, i);
> > -		*mf = sink_fmt_default;
> > -		mf->code = vdebayer->src_code;
> > -	}
> > +	mf = v4l2_subdev_state_get_format(sd_state, 1);
> > +	*mf = sink_fmt_default;
> > +	mf->code = vdebayer->src_code;
> >   
> >   	return 0;
> >   }
> > diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
> > index 5e34b1aed95e..b535b3ffecff 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-sensor.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
> > @@ -44,14 +44,10 @@ static const struct v4l2_mbus_framefmt fmt_default = {
> >   static int vimc_sensor_init_state(struct v4l2_subdev *sd,
> >   				  struct v4l2_subdev_state *sd_state)
> >   {
> > -	unsigned int i;
> > +	struct v4l2_mbus_framefmt *mf;
> >   
> > -	for (i = 0; i < sd->entity.num_pads; i++) {
> > -		struct v4l2_mbus_framefmt *mf;
> > -
> > -		mf = v4l2_subdev_state_get_format(sd_state, i);
> > -		*mf = fmt_default;
> > -	}
> > +	mf = v4l2_subdev_state_get_format(sd_state, 0);
> > +	*mf = fmt_default;
> >   
> >   	return 0;
> >   }
  
Shuah Khan May 30, 2024, 8:18 p.m. UTC | #3
On 5/30/24 13:45, Laurent Pinchart wrote:
> Hi Shuah,
> 
> On Thu, May 30, 2024 at 01:27:53PM -0600, Shuah Khan wrote:
>> On 4/24/24 17:57, Laurent Pinchart wrote:
>>> The .init_state() operations of the debayer and sensor entities iterate
>>> over the entity's pads. In practice, the iteration covers a single pad
>>> only. Access the pad directly and remove the loops.
>>
>> I am not seeing much of a reason to do this. This code is good
>> even when num_pads change.
>>
>> Don't change the loops.
> 
> Why so ? Beside the fact that the loop wastes some CPU cycles, the
> current code implies that there would be multiple source pads, which is
> confusing for the reader. I think the result of this patch is both
> improved efficiency and improved readability.

It is currently flexible and if and when more pads get added,
there is no need to change it. I am not concerned about the
efficiency on this test driver. Also people use the test
driver as a sample.

Please leave it the way it is.

thanks,
-- Shuah
  
Laurent Pinchart May 30, 2024, 8:21 p.m. UTC | #4
On Thu, May 30, 2024 at 02:18:05PM -0600, Shuah Khan wrote:
> On 5/30/24 13:45, Laurent Pinchart wrote:
> > On Thu, May 30, 2024 at 01:27:53PM -0600, Shuah Khan wrote:
> >> On 4/24/24 17:57, Laurent Pinchart wrote:
> >>> The .init_state() operations of the debayer and sensor entities iterate
> >>> over the entity's pads. In practice, the iteration covers a single pad
> >>> only. Access the pad directly and remove the loops.
> >>
> >> I am not seeing much of a reason to do this. This code is good
> >> even when num_pads change.
> >>
> >> Don't change the loops.
> > 
> > Why so ? Beside the fact that the loop wastes some CPU cycles, the
> > current code implies that there would be multiple source pads, which is
> > confusing for the reader. I think the result of this patch is both
> > improved efficiency and improved readability.
> 
> It is currently flexible and if and when more pads get added,
> there is no need to change it. I am not concerned about the
> efficiency on this test driver. Also people use the test
> driver as a sample.

If pad gets added, we don't know yet if the code will work as-is.
Chances are it will need to change anyway. I don't think it's a good
idea to prepare for a situation that we can't foresee without having
good reasons to make assumptions.

I have plans to refactor the vimc driver exteensively, changing how the
different entities behave, to bring it closer to how a real inline ISP
is architectured. The .init_state() functions will likely be rewritten
completely.

I agree with the sample argument, and that's one more reason why I think
this patch does the right thing :-)

> Please leave it the way it is.
  
Shuah Khan May 30, 2024, 8:58 p.m. UTC | #5
On 5/30/24 14:21, Laurent Pinchart wrote:
> On Thu, May 30, 2024 at 02:18:05PM -0600, Shuah Khan wrote:
>> On 5/30/24 13:45, Laurent Pinchart wrote:
>>> On Thu, May 30, 2024 at 01:27:53PM -0600, Shuah Khan wrote:
>>>> On 4/24/24 17:57, Laurent Pinchart wrote:
>>>>> The .init_state() operations of the debayer and sensor entities iterate
>>>>> over the entity's pads. In practice, the iteration covers a single pad
>>>>> only. Access the pad directly and remove the loops.
>>>>
>>>> I am not seeing much of a reason to do this. This code is good
>>>> even when num_pads change.
>>>>
>>>> Don't change the loops.
>>>
>>> Why so ? Beside the fact that the loop wastes some CPU cycles, the
>>> current code implies that there would be multiple source pads, which is
>>> confusing for the reader. I think the result of this patch is both
>>> improved efficiency and improved readability.
>>
>> It is currently flexible and if and when more pads get added,
>> there is no need to change it. I am not concerned about the
>> efficiency on this test driver. Also people use the test
>> driver as a sample.
> 
> If pad gets added, we don't know yet if the code will work as-is.

If it doesn't that would be part of the work to add more pads? Did
you test with more than one pad t say for sure that it doesn't work?

> Chances are it will need to change anyway. I don't think it's a good
> idea to prepare for a situation that we can't foresee without having
> good reasons to make assumptions> 
> I have plans to refactor the vimc driver exteensively, changing how the
> different entities behave, to bring it closer to how a real inline ISP
> is architectured. The .init_state() functions will likely be rewritten
> completely.
> 

You can change that at that time if need be.

> I agree with the sample argument, and that's one more reason why I think
> this patch does the right thing :-)

I am not sure about that.
> 
>> Please leave it the way it is.

thanks,
-- Shuah
  
Hans Verkuil June 20, 2024, 10:47 a.m. UTC | #6
On 30/05/2024 22:21, Laurent Pinchart wrote:
> On Thu, May 30, 2024 at 02:18:05PM -0600, Shuah Khan wrote:
>> On 5/30/24 13:45, Laurent Pinchart wrote:
>>> On Thu, May 30, 2024 at 01:27:53PM -0600, Shuah Khan wrote:
>>>> On 4/24/24 17:57, Laurent Pinchart wrote:
>>>>> The .init_state() operations of the debayer and sensor entities iterate
>>>>> over the entity's pads. In practice, the iteration covers a single pad
>>>>> only. Access the pad directly and remove the loops.
>>>>
>>>> I am not seeing much of a reason to do this. This code is good
>>>> even when num_pads change.
>>>>
>>>> Don't change the loops.
>>>
>>> Why so ? Beside the fact that the loop wastes some CPU cycles, the
>>> current code implies that there would be multiple source pads, which is
>>> confusing for the reader. I think the result of this patch is both
>>> improved efficiency and improved readability.
>>
>> It is currently flexible and if and when more pads get added,
>> there is no need to change it. I am not concerned about the
>> efficiency on this test driver. Also people use the test
>> driver as a sample.
> 
> If pad gets added, we don't know yet if the code will work as-is.
> Chances are it will need to change anyway. I don't think it's a good
> idea to prepare for a situation that we can't foresee without having
> good reasons to make assumptions.
> 
> I have plans to refactor the vimc driver exteensively, changing how the
> different entities behave, to bring it closer to how a real inline ISP
> is architectured. The .init_state() functions will likely be rewritten
> completely.
> 
> I agree with the sample argument, and that's one more reason why I think
> this patch does the right thing :-)

I agree with Laurent on this. Sensor and debayer subdev devices have
hardwired pads determined by the hardware, it is not something that is
flexible. Since this is also serves as an example of such a driver, it
makes sense to hardcode it, as that is how it is done in practice.

It would be nice though to use defines rather than hardcoding it to 1
in a line like this:

	mf = v4l2_subdev_state_get_format(sd_state, 1);

It would make it easier to read and see which pad index is source and
which is sink.

The sensor has only one pad, so there you can use the index 0, creating
PAD defines doesn't add anything for a sensor.

Regards,

	Hans

> 
>> Please leave it the way it is.
>
  
Sakari Ailus June 20, 2024, 1 p.m. UTC | #7
On Thu, Jun 20, 2024 at 12:47:15PM +0200, Hans Verkuil wrote:
> I agree with Laurent on this. Sensor and debayer subdev devices have
> hardwired pads determined by the hardware, it is not something that is
> flexible. Since this is also serves as an example of such a driver, it
> makes sense to hardcode it, as that is how it is done in practice.

Me, too. If you know you have one of something, there's indeed no need for
a loop.
  
Sakari Ailus June 20, 2024, 1:01 p.m. UTC | #8
Hi Laurent,

On Thu, Apr 25, 2024 at 02:57:33AM +0300, Laurent Pinchart wrote:
> The .init_state() operations of the debayer and sensor entities iterate
> over the entity's pads. In practice, the iteration covers a single pad
> only. Access the pad directly and remove the loops.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/test-drivers/vimc/vimc-debayer.c |  9 +++------
>  drivers/media/test-drivers/vimc/vimc-sensor.c  | 10 +++-------
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
> index d72ed086e00b..e1bf6db73050 100644
> --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
> @@ -155,16 +155,13 @@ static int vimc_debayer_init_state(struct v4l2_subdev *sd,
>  {
>  	struct vimc_debayer_device *vdebayer = v4l2_get_subdevdata(sd);
>  	struct v4l2_mbus_framefmt *mf;
> -	unsigned int i;
>  
>  	mf = v4l2_subdev_state_get_format(sd_state, 0);
>  	*mf = sink_fmt_default;
>  
> -	for (i = 1; i < sd->entity.num_pads; i++) {
> -		mf = v4l2_subdev_state_get_format(sd_state, i);
> -		*mf = sink_fmt_default;
> -		mf->code = vdebayer->src_code;
> -	}
> +	mf = v4l2_subdev_state_get_format(sd_state, 1);

You can assign in variable declaration as this is just about obtaining the
appropriate state pointer.

> +	*mf = sink_fmt_default;
> +	mf->code = vdebayer->src_code;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
> index 5e34b1aed95e..b535b3ffecff 100644
> --- a/drivers/media/test-drivers/vimc/vimc-sensor.c
> +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
> @@ -44,14 +44,10 @@ static const struct v4l2_mbus_framefmt fmt_default = {
>  static int vimc_sensor_init_state(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state)
>  {
> -	unsigned int i;
> +	struct v4l2_mbus_framefmt *mf;
>  
> -	for (i = 0; i < sd->entity.num_pads; i++) {
> -		struct v4l2_mbus_framefmt *mf;
> -
> -		mf = v4l2_subdev_state_get_format(sd_state, i);
> -		*mf = fmt_default;
> -	}
> +	mf = v4l2_subdev_state_get_format(sd_state, 0);

Ditto.

Up to you though.

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +	*mf = fmt_default;
>  
>  	return 0;
>  }
  
Shuah Khan June 20, 2024, 3:33 p.m. UTC | #9
On 6/20/24 07:00, Sakari Ailus wrote:
> On Thu, Jun 20, 2024 at 12:47:15PM +0200, Hans Verkuil wrote:
>> I agree with Laurent on this. Sensor and debayer subdev devices have
>> hardwired pads determined by the hardware, it is not something that is
>> flexible. Since this is also serves as an example of such a driver, it
>> makes sense to hardcode it, as that is how it is done in practice.
> 
> Me, too. If you know you have one of something, there's indeed no need for
> a loop.
> 

If you all agree, I am fine with it.

thanks,
-- Shuah
  

Patch

diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index d72ed086e00b..e1bf6db73050 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -155,16 +155,13 @@  static int vimc_debayer_init_state(struct v4l2_subdev *sd,
 {
 	struct vimc_debayer_device *vdebayer = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *mf;
-	unsigned int i;
 
 	mf = v4l2_subdev_state_get_format(sd_state, 0);
 	*mf = sink_fmt_default;
 
-	for (i = 1; i < sd->entity.num_pads; i++) {
-		mf = v4l2_subdev_state_get_format(sd_state, i);
-		*mf = sink_fmt_default;
-		mf->code = vdebayer->src_code;
-	}
+	mf = v4l2_subdev_state_get_format(sd_state, 1);
+	*mf = sink_fmt_default;
+	mf->code = vdebayer->src_code;
 
 	return 0;
 }
diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
index 5e34b1aed95e..b535b3ffecff 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -44,14 +44,10 @@  static const struct v4l2_mbus_framefmt fmt_default = {
 static int vimc_sensor_init_state(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state)
 {
-	unsigned int i;
+	struct v4l2_mbus_framefmt *mf;
 
-	for (i = 0; i < sd->entity.num_pads; i++) {
-		struct v4l2_mbus_framefmt *mf;
-
-		mf = v4l2_subdev_state_get_format(sd_state, i);
-		*mf = fmt_default;
-	}
+	mf = v4l2_subdev_state_get_format(sd_state, 0);
+	*mf = fmt_default;
 
 	return 0;
 }