| Message ID | 20240410-enable-streams-impro-v3-5-e5e7a5da7420@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: subdev: Improve stream enable/disable machinery | expand |
Hi Tomi, Thank you for the patch. On Wed, Apr 10, 2024 at 03:35:52PM +0300, Tomi Valkeinen wrote: > v4l2_subdev_enable/disable_streams_fallback() supports falling back to > .s_stream() for subdevs with a single source pad. It also tracks the > enabled streams for that one pad in the sd->enabled_streams field. > > Tracking the enabled streams with sd->enabled_streams does not make > sense, as with .s_stream() there can only be a single stream per pad. > Thus, as the v4l2_subdev_enable/disable_streams_fallback() only supports > a single source pad, all we really need is a boolean which tells whether > streaming has been enabled on this pad or not. > > However, as we only need a true/false state for a pad (instead of > tracking which streams have been enabled for a pad), we can easily > extend the fallback mechanism to support multiple source pads as we only > need to keep track of which pads have been enabled. > > Change the sd->enabled_streams field to sd->enabled_pads, which is a > 64-bit bitmask tracking the enabled source pads. With this change we can > remove the restriction that > v4l2_subdev_enable/disable_streams_fallback() only supports a single > source pad. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 68 ++++++++++++++++++++--------------- > include/media/v4l2-subdev.h | 9 +++-- > 2 files changed, 44 insertions(+), 33 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 6cd353d83dfc..3d2c9c224b8f 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -2104,37 +2104,43 @@ static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad, > u64 streams_mask) > { > struct device *dev = sd->entity.graph_obj.mdev->dev; > - unsigned int i; > int ret; > > /* > * The subdev doesn't implement pad-based stream enable, fall back > - * on the .s_stream() operation. This can only be done for subdevs that > - * have a single source pad, as sd->enabled_streams is global to the > - * subdev. > + * to the .s_stream() operation. > */ > if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)) > return -EOPNOTSUPP; > > - for (i = 0; i < sd->entity.num_pads; ++i) { > - if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE) > - return -EOPNOTSUPP; > - } > + /* > + * .s_stream() means there is no streams support, so only allowed stream s/only/the only/ > + * is the implicit stream 0. > + */ > + if (streams_mask != BIT_ULL(0)) > + return -EOPNOTSUPP; > + > + /* > + * We use a 64-bit bitmask for tracking enabled pads, so only subdevices > + * with 64 pads or less can be supported. > + */ > + if (pad >= sizeof(sd->enabled_pads) * BITS_PER_BYTE) > + return -EOPNOTSUPP; > > - if (sd->enabled_streams & streams_mask) { > - dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n", > - streams_mask, sd->entity.name, pad); > + if (sd->enabled_pads & BIT_ULL(pad)) { > + dev_dbg(dev, "pad %u already enabled on %s\n", > + pad, sd->entity.name); > return -EALREADY; > } > > - /* Start streaming when the first streams are enabled. */ > - if (!sd->enabled_streams) { > + /* Start streaming when the first pad is enabled. */ > + if (!sd->enabled_pads) { > ret = v4l2_subdev_call(sd, video, s_stream, 1); > if (ret) > return ret; > } > > - sd->enabled_streams |= streams_mask; > + sd->enabled_pads |= BIT_ULL(pad); > > return 0; > } > @@ -2221,37 +2227,43 @@ static int v4l2_subdev_disable_streams_fallback(struct v4l2_subdev *sd, u32 pad, > u64 streams_mask) > { > struct device *dev = sd->entity.graph_obj.mdev->dev; > - unsigned int i; > int ret; > > /* > - * If the subdev doesn't implement pad-based stream enable, fall back > - * on the .s_stream() operation. This can only be done for subdevs that > - * have a single source pad, as sd->enabled_streams is global to the > - * subdev. > + * If the subdev doesn't implement pad-based stream enable, fall back > + * to the .s_stream() operation. > */ > if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)) > return -EOPNOTSUPP; > > - for (i = 0; i < sd->entity.num_pads; ++i) { > - if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE) > - return -EOPNOTSUPP; > - } > + /* > + * .s_stream() means there is no streams support, so only allowed stream Same here. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * is the implicit stream 0. > + */ > + if (streams_mask != BIT_ULL(0)) > + return -EOPNOTSUPP; > + > + /* > + * We use a 64-bit bitmask for tracking enabled pads, so only subdevices > + * with 64 pads or less can be supported. > + */ > + if (pad >= sizeof(sd->enabled_pads) * BITS_PER_BYTE) > + return -EOPNOTSUPP; > > - if ((sd->enabled_streams & streams_mask) != streams_mask) { > - dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n", > - streams_mask, sd->entity.name, pad); > + if (!(sd->enabled_pads & BIT_ULL(pad))) { > + dev_dbg(dev, "pad %u already disabled on %s\n", > + pad, sd->entity.name); > return -EALREADY; > } > > /* Stop streaming when the last streams are disabled. */ > - if (!(sd->enabled_streams & ~streams_mask)) { > + if (!(sd->enabled_pads & ~BIT_ULL(pad))) { > ret = v4l2_subdev_call(sd, video, s_stream, 0); > if (ret) > return ret; > } > > - sd->enabled_streams &= ~streams_mask; > + sd->enabled_pads &= ~BIT_ULL(pad); > > return 0; > } > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index f55d03e0acc1..d6867511e9cf 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data { > * @active_state: Active state for the subdev (NULL for subdevs tracking the > * state internally). Initialized by calling > * v4l2_subdev_init_finalize(). > - * @enabled_streams: Bitmask of enabled streams used by > - * v4l2_subdev_enable_streams() and > - * v4l2_subdev_disable_streams() helper functions for fallback > - * cases. > + * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams() > + * and v4l2_subdev_disable_streams() helper functions for > + * fallback cases. > * @streaming_enabled: Tracks whether streaming has been enabled with s_stream. > * This is only for call_s_stream() internal use. > * > @@ -1092,7 +1091,7 @@ struct v4l2_subdev { > * doesn't support it. > */ > struct v4l2_subdev_state *active_state; > - u64 enabled_streams; > + u64 enabled_pads; > bool streaming_enabled; > }; > >
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 6cd353d83dfc..3d2c9c224b8f 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -2104,37 +2104,43 @@ static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad, u64 streams_mask) { struct device *dev = sd->entity.graph_obj.mdev->dev; - unsigned int i; int ret; /* * The subdev doesn't implement pad-based stream enable, fall back - * on the .s_stream() operation. This can only be done for subdevs that - * have a single source pad, as sd->enabled_streams is global to the - * subdev. + * to the .s_stream() operation. */ if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)) return -EOPNOTSUPP; - for (i = 0; i < sd->entity.num_pads; ++i) { - if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE) - return -EOPNOTSUPP; - } + /* + * .s_stream() means there is no streams support, so only allowed stream + * is the implicit stream 0. + */ + if (streams_mask != BIT_ULL(0)) + return -EOPNOTSUPP; + + /* + * We use a 64-bit bitmask for tracking enabled pads, so only subdevices + * with 64 pads or less can be supported. + */ + if (pad >= sizeof(sd->enabled_pads) * BITS_PER_BYTE) + return -EOPNOTSUPP; - if (sd->enabled_streams & streams_mask) { - dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n", - streams_mask, sd->entity.name, pad); + if (sd->enabled_pads & BIT_ULL(pad)) { + dev_dbg(dev, "pad %u already enabled on %s\n", + pad, sd->entity.name); return -EALREADY; } - /* Start streaming when the first streams are enabled. */ - if (!sd->enabled_streams) { + /* Start streaming when the first pad is enabled. */ + if (!sd->enabled_pads) { ret = v4l2_subdev_call(sd, video, s_stream, 1); if (ret) return ret; } - sd->enabled_streams |= streams_mask; + sd->enabled_pads |= BIT_ULL(pad); return 0; } @@ -2221,37 +2227,43 @@ static int v4l2_subdev_disable_streams_fallback(struct v4l2_subdev *sd, u32 pad, u64 streams_mask) { struct device *dev = sd->entity.graph_obj.mdev->dev; - unsigned int i; int ret; /* - * If the subdev doesn't implement pad-based stream enable, fall back - * on the .s_stream() operation. This can only be done for subdevs that - * have a single source pad, as sd->enabled_streams is global to the - * subdev. + * If the subdev doesn't implement pad-based stream enable, fall back + * to the .s_stream() operation. */ if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)) return -EOPNOTSUPP; - for (i = 0; i < sd->entity.num_pads; ++i) { - if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE) - return -EOPNOTSUPP; - } + /* + * .s_stream() means there is no streams support, so only allowed stream + * is the implicit stream 0. + */ + if (streams_mask != BIT_ULL(0)) + return -EOPNOTSUPP; + + /* + * We use a 64-bit bitmask for tracking enabled pads, so only subdevices + * with 64 pads or less can be supported. + */ + if (pad >= sizeof(sd->enabled_pads) * BITS_PER_BYTE) + return -EOPNOTSUPP; - if ((sd->enabled_streams & streams_mask) != streams_mask) { - dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n", - streams_mask, sd->entity.name, pad); + if (!(sd->enabled_pads & BIT_ULL(pad))) { + dev_dbg(dev, "pad %u already disabled on %s\n", + pad, sd->entity.name); return -EALREADY; } /* Stop streaming when the last streams are disabled. */ - if (!(sd->enabled_streams & ~streams_mask)) { + if (!(sd->enabled_pads & ~BIT_ULL(pad))) { ret = v4l2_subdev_call(sd, video, s_stream, 0); if (ret) return ret; } - sd->enabled_streams &= ~streams_mask; + sd->enabled_pads &= ~BIT_ULL(pad); return 0; } diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index f55d03e0acc1..d6867511e9cf 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data { * @active_state: Active state for the subdev (NULL for subdevs tracking the * state internally). Initialized by calling * v4l2_subdev_init_finalize(). - * @enabled_streams: Bitmask of enabled streams used by - * v4l2_subdev_enable_streams() and - * v4l2_subdev_disable_streams() helper functions for fallback - * cases. + * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams() + * and v4l2_subdev_disable_streams() helper functions for + * fallback cases. * @streaming_enabled: Tracks whether streaming has been enabled with s_stream. * This is only for call_s_stream() internal use. * @@ -1092,7 +1091,7 @@ struct v4l2_subdev { * doesn't support it. */ struct v4l2_subdev_state *active_state; - u64 enabled_streams; + u64 enabled_pads; bool streaming_enabled; };
v4l2_subdev_enable/disable_streams_fallback() supports falling back to .s_stream() for subdevs with a single source pad. It also tracks the enabled streams for that one pad in the sd->enabled_streams field. Tracking the enabled streams with sd->enabled_streams does not make sense, as with .s_stream() there can only be a single stream per pad. Thus, as the v4l2_subdev_enable/disable_streams_fallback() only supports a single source pad, all we really need is a boolean which tells whether streaming has been enabled on this pad or not. However, as we only need a true/false state for a pad (instead of tracking which streams have been enabled for a pad), we can easily extend the fallback mechanism to support multiple source pads as we only need to keep track of which pads have been enabled. Change the sd->enabled_streams field to sd->enabled_pads, which is a 64-bit bitmask tracking the enabled source pads. With this change we can remove the restriction that v4l2_subdev_enable/disable_streams_fallback() only supports a single source pad. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 68 ++++++++++++++++++++--------------- include/media/v4l2-subdev.h | 9 +++-- 2 files changed, 44 insertions(+), 33 deletions(-)