| Message ID | 20241210075906.609490-6-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use V4L2 mbus config for conveying MEI CSI link frequency | expand |
Hi Sakari, Thank you for the patch. I think this should come before 4/5. On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote: > Obtain the link frequency from the sub-device instead of a control > handler. This allows obtaining it using the get_mbus_config() sub-device > pad op that which is only supported by the IVSC driver. "which is the only method supported by the IVSC driver" > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > index 051898ce53f4..da8581a37e22 100644 > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = { > s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2) > { > struct media_pad *src_pad; > - struct v4l2_subdev *ext_sd; > - struct device *dev; > > if (!csi2) > return -EINVAL; > > - dev = &csi2->isys->adev->auxdev.dev; > src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity); Not a candidate for this patch, but can the source change, or can it be cached at probe time (or notifier bound time) ? > if (IS_ERR(src_pad)) { > - dev_err(dev, "can't get source pad of %s (%ld)\n", > + dev_err(&csi2->isys->adev->auxdev.dev, > + "can't get source pad of %s (%ld)\n", > csi2->asd.sd.name, PTR_ERR(src_pad)); > return PTR_ERR(src_pad); > } > > - ext_sd = media_entity_to_v4l2_subdev(src_pad->entity); > - if (WARN(!ext_sd, "Failed to get subdev for %s\n", csi2->asd.sd.name)) > - return -ENODEV; > - > - return v4l2_get_link_freq(ext_sd->ctrl_handler, 0, 0); > + return v4l2_get_link_freq(src_pad, 0, 0); > } > > static int csi2_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
Hi Laurent, On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. Thank you for the review. I asked you to review a set but it wasn't this one: <URL:https://lore.kernel.org/linux-media/20241129095142.87196-1-sakari.ailus@linux.intel.com/T/#t>. :-) > > I think this should come before 4/5. > > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote: > > Obtain the link frequency from the sub-device instead of a control > > handler. This allows obtaining it using the get_mbus_config() sub-device > > pad op that which is only supported by the IVSC driver. > > "which is the only method supported by the IVSC driver" > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++--------- > > 1 file changed, 3 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > index 051898ce53f4..da8581a37e22 100644 > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = { > > s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2) > > { > > struct media_pad *src_pad; > > - struct v4l2_subdev *ext_sd; > > - struct device *dev; > > > > if (!csi2) > > return -EINVAL; > > > > - dev = &csi2->isys->adev->auxdev.dev; > > src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity); > > Not a candidate for this patch, but can the source change, or can it be > cached at probe time (or notifier bound time) ? It could be, but why would you do that? This would also prevent connecting multiple sensors to a single CSI-2 receiver. > > > if (IS_ERR(src_pad)) { > > - dev_err(dev, "can't get source pad of %s (%ld)\n", > > + dev_err(&csi2->isys->adev->auxdev.dev, > > + "can't get source pad of %s (%ld)\n", > > csi2->asd.sd.name, PTR_ERR(src_pad)); > > return PTR_ERR(src_pad); > > } > > > > - ext_sd = media_entity_to_v4l2_subdev(src_pad->entity); > > - if (WARN(!ext_sd, "Failed to get subdev for %s\n", csi2->asd.sd.name)) > > - return -ENODEV; > > - > > - return v4l2_get_link_freq(ext_sd->ctrl_handler, 0, 0); > > + return v4l2_get_link_freq(src_pad, 0, 0); > > } > > > > static int csi2_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh, >
On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > I think this should come before 4/5. Because of...? Both are needed for this to work. > > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote: > > Obtain the link frequency from the sub-device instead of a control > > handler. This allows obtaining it using the get_mbus_config() sub-device > > pad op that which is only supported by the IVSC driver. > > "which is the only method supported by the IVSC driver" Yes.
On Mon, Dec 16, 2024 at 07:47:12AM +0000, Sakari Ailus wrote: > On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > Thank you for the review. I asked you to review a set but it wasn't this > one: > <URL:https://lore.kernel.org/linux-media/20241129095142.87196-1-sakari.ailus@linux.intel.com/T/#t>. > :-) Are you complaining that I review too many patches ? :-) > > I think this should come before 4/5. > > > > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote: > > > Obtain the link frequency from the sub-device instead of a control > > > handler. This allows obtaining it using the get_mbus_config() sub-device > > > pad op that which is only supported by the IVSC driver. > > > > "which is the only method supported by the IVSC driver" > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++--------- > > > 1 file changed, 3 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > index 051898ce53f4..da8581a37e22 100644 > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = { > > > s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2) > > > { > > > struct media_pad *src_pad; > > > - struct v4l2_subdev *ext_sd; > > > - struct device *dev; > > > > > > if (!csi2) > > > return -EINVAL; > > > > > > - dev = &csi2->isys->adev->auxdev.dev; > > > src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity); > > > > Not a candidate for this patch, but can the source change, or can it be > > cached at probe time (or notifier bound time) ? > > It could be, but why would you do that? > > This would also prevent connecting multiple sensors to a single CSI-2 > receiver. Precisely because people shouldn't do this :-) It would be more efficient to get the pad at probe time and cache it, and would remove an error path at runtime. Until we have a use case where we need to support more than one sensor on the same CSI-2 receiver for this driver, I think that would be best. > > > if (IS_ERR(src_pad)) { > > > - dev_err(dev, "can't get source pad of %s (%ld)\n", > > > + dev_err(&csi2->isys->adev->auxdev.dev, > > > + "can't get source pad of %s (%ld)\n", > > > csi2->asd.sd.name, PTR_ERR(src_pad)); > > > return PTR_ERR(src_pad); > > > } > > > > > > - ext_sd = media_entity_to_v4l2_subdev(src_pad->entity); > > > - if (WARN(!ext_sd, "Failed to get subdev for %s\n", csi2->asd.sd.name)) > > > - return -ENODEV; > > > - > > > - return v4l2_get_link_freq(ext_sd->ctrl_handler, 0, 0); > > > + return v4l2_get_link_freq(src_pad, 0, 0); > > > } > > > > > > static int csi2_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
Hi Laurent, On Mon, Dec 16, 2024 at 11:07:36AM +0200, Laurent Pinchart wrote: > On Mon, Dec 16, 2024 at 07:47:12AM +0000, Sakari Ailus wrote: > > On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote: > > > Hi Sakari, > > > > > > Thank you for the patch. > > > > Thank you for the review. I asked you to review a set but it wasn't this > > one: > > <URL:https://lore.kernel.org/linux-media/20241129095142.87196-1-sakari.ailus@linux.intel.com/T/#t>. > > :-) > > Are you complaining that I review too many patches ? :-) No, I'm complaining your selection of patches to review. ;-) > > > > I think this should come before 4/5. > > > > > > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote: > > > > Obtain the link frequency from the sub-device instead of a control > > > > handler. This allows obtaining it using the get_mbus_config() sub-device > > > > pad op that which is only supported by the IVSC driver. > > > > > > "which is the only method supported by the IVSC driver" > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++--------- > > > > 1 file changed, 3 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > > index 051898ce53f4..da8581a37e22 100644 > > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = { > > > > s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2) > > > > { > > > > struct media_pad *src_pad; > > > > - struct v4l2_subdev *ext_sd; > > > > - struct device *dev; > > > > > > > > if (!csi2) > > > > return -EINVAL; > > > > > > > > - dev = &csi2->isys->adev->auxdev.dev; > > > > src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity); > > > > > > Not a candidate for this patch, but can the source change, or can it be > > > cached at probe time (or notifier bound time) ? > > > > It could be, but why would you do that? > > > > This would also prevent connecting multiple sensors to a single CSI-2 > > receiver. > > Precisely because people shouldn't do this :-) > > It would be more efficient to get the pad at probe time and cache it, > and would remove an error path at runtime. Until we have a use case I presume it'd take a bug somewhere for that to fail. I don't think a relatively small number of instructions would make measurable a difference in performance. If we thought this was a problem, there would be a lot to work on elsewhere in the framework, starting with the control framework and IOCTL handling. The problem is just that nearly all that code is there for sound and important reasons. > where we need to support more than one sensor on the same CSI-2 receiver > for this driver, I think that would be best. I don't disagree as such but we can't affect hardware design here. Nothing currently prevents that and adding a driver bug that will cause whatever ills when hit is not a solution either, even if the former were true. Well, if this were Windows, the situation could be different.
On Mon, Dec 16, 2024 at 09:18:31AM +0000, Sakari Ailus wrote: > Hi Laurent, > > On Mon, Dec 16, 2024 at 11:07:36AM +0200, Laurent Pinchart wrote: > > On Mon, Dec 16, 2024 at 07:47:12AM +0000, Sakari Ailus wrote: > > > On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote: > > > > Hi Sakari, > > > > > > > > Thank you for the patch. > > > > > > Thank you for the review. I asked you to review a set but it wasn't this > > > one: > > > <URL:https://lore.kernel.org/linux-media/20241129095142.87196-1-sakari.ailus@linux.intel.com/T/#t>. > > > :-) > > > > Are you complaining that I review too many patches ? :-) > > No, I'm complaining your selection of patches to review. ;-) > > > > > > > I think this should come before 4/5. > > > > > > > > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote: > > > > > Obtain the link frequency from the sub-device instead of a control > > > > > handler. This allows obtaining it using the get_mbus_config() sub-device > > > > > pad op that which is only supported by the IVSC driver. > > > > > > > > "which is the only method supported by the IVSC driver" > > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > > --- > > > > > drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++--------- > > > > > 1 file changed, 3 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > > > index 051898ce53f4..da8581a37e22 100644 > > > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > > > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = { > > > > > s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2) > > > > > { > > > > > struct media_pad *src_pad; > > > > > - struct v4l2_subdev *ext_sd; > > > > > - struct device *dev; > > > > > > > > > > if (!csi2) > > > > > return -EINVAL; > > > > > > > > > > - dev = &csi2->isys->adev->auxdev.dev; > > > > > src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity); > > > > > > > > Not a candidate for this patch, but can the source change, or can it be > > > > cached at probe time (or notifier bound time) ? > > > > > > It could be, but why would you do that? > > > > > > This would also prevent connecting multiple sensors to a single CSI-2 > > > receiver. > > > > Precisely because people shouldn't do this :-) > > > > It would be more efficient to get the pad at probe time and cache it, > > and would remove an error path at runtime. Until we have a use case > > I presume it'd take a bug somewhere for that to fail. I don't think a > relatively small number of instructions would make measurable a difference > in performance. If we thought this was a problem, there would be a lot to > work on elsewhere in the framework, starting with the control framework and > IOCTL handling. The problem is just that nearly all that code is there for > sound and important reasons. > > > where we need to support more than one sensor on the same CSI-2 receiver > > for this driver, I think that would be best. > > I don't disagree as such but we can't affect hardware design here. Nothing > currently prevents that and adding a driver bug that will cause whatever It wouldn't be a bug, the driver would just ignore all bug the first source. If that doesn't work for someone, we would hear about it. > ills when hit is not a solution either, even if the former were true. Well, > if this were Windows, the situation could be different.
On Mon, Dec 16, 2024 at 08:03:08AM +0000, Sakari Ailus wrote: > On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > > > I think this should come before 4/5. > > Because of...? Both are needed for this to work. Because, unless I'm mistaken, it would otherwise introduce a bisection breakage. 4/5 drops the LINK_FREQ control from the IVSC driver, and obtaining the link frequency through .get_mbus_config() is only supported when using the v4l2_get_link_freq() variant that takes a pad argument. > > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote: > > > Obtain the link frequency from the sub-device instead of a control > > > handler. This allows obtaining it using the get_mbus_config() sub-device > > > pad op that which is only supported by the IVSC driver. > > > > "which is the only method supported by the IVSC driver" > > Yes.
On Mon, Dec 16, 2024 at 01:13:26PM +0200, Laurent Pinchart wrote: > On Mon, Dec 16, 2024 at 08:03:08AM +0000, Sakari Ailus wrote: > > On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote: > > > Hi Sakari, > > > > > > Thank you for the patch. > > > > > > I think this should come before 4/5. > > > > Because of...? Both are needed for this to work. > > Because, unless I'm mistaken, it would otherwise introduce a bisection > breakage. 4/5 drops the LINK_FREQ control from the IVSC driver, and > obtaining the link frequency through .get_mbus_config() is only > supported when using the v4l2_get_link_freq() variant that takes a pad > argument. That's a fair point, I can change the order.
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c index 051898ce53f4..da8581a37e22 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = { s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2) { struct media_pad *src_pad; - struct v4l2_subdev *ext_sd; - struct device *dev; if (!csi2) return -EINVAL; - dev = &csi2->isys->adev->auxdev.dev; src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity); if (IS_ERR(src_pad)) { - dev_err(dev, "can't get source pad of %s (%ld)\n", + dev_err(&csi2->isys->adev->auxdev.dev, + "can't get source pad of %s (%ld)\n", csi2->asd.sd.name, PTR_ERR(src_pad)); return PTR_ERR(src_pad); } - ext_sd = media_entity_to_v4l2_subdev(src_pad->entity); - if (WARN(!ext_sd, "Failed to get subdev for %s\n", csi2->asd.sd.name)) - return -ENODEV; - - return v4l2_get_link_freq(ext_sd->ctrl_handler, 0, 0); + return v4l2_get_link_freq(src_pad, 0, 0); } static int csi2_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
Obtain the link frequency from the sub-device instead of a control handler. This allows obtaining it using the get_mbus_config() sub-device pad op that which is only supported by the IVSC driver. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)