Hi Eugen, On Tue, Feb 08, 2022 at 11:36:16AM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote: > On 2/8/22 1:06 PM, Laurent Pinchart wrote: > > On Tue, Feb 08, 2022 at 08:00:19AM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote: > >> On 2/8/22 7:11 AM, Mauro Carvalho Chehab wrote: > >>> Em Wed, 2 Feb 2022 17:36:09 +0200 Sakari Ailus escreveu: > >>> > >>>> As part of removing mbus config flags, remove VC flag use in the > >>>> microchip-csi2dc driver. The support can be reintroduced later on as part > >>>> of the streams patches. > >>>> > >>>> Cc: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> > >>> > >>> Hmm... that sounds a regression to me. What effects this will cause at > >>> the driver? > >>> > >>> Eugen, any comments? > >> > >> Hi , > >> > >> I am not happy with this change. It looks like I wasn't even CC-ed on > >> the original patch e-mail. > >> > >> The effect on the driver will be that everything will be treated as > >> virtual channel=0 . > > > > I don't think there's any risk of regression, as we have no driver > > setting any of the V4L2_MBUS_CSI2_CHANNEL_[123] flags in the kernel. > > > >> I do not yet understand why we are about to remove > >> V4L2_MBUS_CSI2_CHANNEL_* as I remember this was just introduced. > > > > Those flags were added in 2011. If you think of that as "just > > introduced" then I understand why you would be unhappy about "sudden > > changes" mentioned below ;-) > > Hello Laurent, > > I am sorry, I was referring to the get_mbus_config which was recently > added, which in my case uses V4L2_MBUS_CSI2_CHANNEL_* . > Without the get_mbus_config, I wasn't supposed to use the > V4L2_MBUS_CSI2_CHANNEL_* at all . > Hence my confusion in adding them into the same bucket. > > My driver uses these flags because my initial implementation to get the > VC from the devicetree was rejected and this was suggested instead. > > >> Is there any alternative in place ? > > > > Virtual channels have never been properly supported in V4L2. This is > > going to change with "[PATCH v10 00/38] v4l: subdev internal routing and > > streams" ([1]). > > > > [1] https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@xxxxxxxxxxxxxxxx > > In that case then I suppose the support in the csi2dc driver has to be > rewritten Only when you'll need to support virtual channels other than VC 0. It's a non-negligible effort if you need to capture multiple streams from different VCs simultaneously, but if it's only about selecting the correct VC number for one stream, it's much easier. As far as I understand, your hardware supports the latter, not the former, right ? Do you have any particular CSI-2 source in mind that produces different VCs by the way ? > >> My opinion is that if we want to replace something existing with a new > >> API or something else, we should first add the new support, block any > >> new adopters for the old API such that everyone uses the new API, and > >> only after that convert the old API clients to the new API. > >> So 'can be reintroduced later on' is not okay. We can't remove things in > >> the hope that it would be reintroduced later. Just my personal take on > >> this, feel free to have a different opinion. > > > > When regressions are introduced this makes sense, but here we're > > dropping a feature that isn't used as no kernel driver selects a VC > > different than 0. > > I still disagree that these should be removed before the patch you > mentioned earlier about adding 'subdev internal routing and streams' is > applied. I see things in the way that after the new support is > available, old flags , even if unused, can be removed. > You can disagree, of course. I agree it would be nice, but the multiplexed streams series will still need time to land (it's been out there for ages, we've resumed work on it about a year ago, and Tomi has done a really amazing job moving this forward). I'd like to avoid blocking the V4L2_MBUS_CSI2_CHANNEL_* removal until multiplexed streams get merged if possible. If there was an urgent need to support different VCs in your driver the situation would of course be different, but if that's not foreseen until we get proper support for multiplexed streams, I think moving forward with all the rework in parallel is best. V4L2 has few active contributors for core code, it's really hard modernizing the internals of the subsystem with the resource shortage, I'd like to avoid making it even harder :-) > Thank you for sharing your thoughts, > Eugen > > >> In the end you guys are the maintainers for the subsystem and can have > >> this change if you like, I am more unhappy about the fact that changes > >> happen suddenly and without notice. > >> > >>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > >>>> --- > >>>> .../media/platform/atmel/microchip-csi2dc.c | 18 ++---------------- > >>>> 1 file changed, 2 insertions(+), 16 deletions(-) > >>>> > >>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c > >>>> index 6bc549c28e05..6a7f5b4b0e3b 100644 > >>>> --- a/drivers/media/platform/atmel/microchip-csi2dc.c > >>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c > >>>> @@ -348,24 +348,15 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >>>> if (ret == -ENOIOCTLCMD) { > >>>> dev_dbg(csi2dc->dev, > >>>> "no remote mbus configuration available\n"); > >>>> - goto csi2dc_get_mbus_config_defaults; > >>>> + return 0; > >>>> } > >>>> > >>>> if (ret) { > >>>> dev_err(csi2dc->dev, > >>>> "failed to get remote mbus configuration\n"); > >>>> - goto csi2dc_get_mbus_config_defaults; > >>>> + return 0; > >>>> } > >>>> > >>>> - if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) > >>>> - csi2dc->vc = 0; > >>>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) > >>>> - csi2dc->vc = 1; > >>>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) > >>>> - csi2dc->vc = 2; > >>>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) > >>>> - csi2dc->vc = 3; > >>>> - > >>>> dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); > >>>> > >>>> csi2dc->clk_gated = mbus_config.flags & > >>>> @@ -375,11 +366,6 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >>>> csi2dc->clk_gated ? "gated" : "free running"); > >>>> > >>>> return 0; > >>>> - > >>>> -csi2dc_get_mbus_config_defaults: > >>>> - csi2dc->vc = 0; /* Virtual ID 0 by default */ > >>>> - > >>>> - return 0; > >>>> } > >>>> > >>>> static void csi2dc_vp_update(struct csi2dc_device *csi2dc) -- Regards, Laurent Pinchart
