| Message ID | 20240424235741.17093-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 2ef9a1e722688ccea824e5f224b91ab4b6fb1a47 |
| Headers | show |
| Series | media: vimc improvements | expand |
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
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; > > }
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
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.
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
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. >
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.
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; > }
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
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; }
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(-)