Hi Hans,
On Tue, Nov 03, 2020 at 12:02:41PM +0100, Hans Verkuil wrote:
> Hi Laurent,
>
> This was still on my TODO list to review. Of the other patch only my comment
> for 100/108 needs to be addressed in a v3.
>
> I have just one comment for this patch:
>
> On 06/07/2020 20:37, Laurent Pinchart wrote:
>
> > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > index 293cbac905b3..2ce2b6404c92 100644
> > --- a/drivers/media/platform/ti-vpe/cal.c
> > +++ b/drivers/media/platform/ti-vpe/cal.c
> > @@ -43,6 +43,10 @@ unsigned int cal_debug;
> > module_param_named(debug, cal_debug, uint, 0644);
> > MODULE_PARM_DESC(debug, "activates debug info");
> >
> > +bool cal_mc_api;
> > +module_param_named(mc_api, cal_mc_api, bool, 0444);
> > +MODULE_PARM_DESC(mc_api, "activates the MC API");
>
> I think it would be very useful if a Kconfig option is added that selects
> the default of cal_mc_api. If you know that you want the MC API, then you
> can select the option and that will be the default.
I expect this to spread to more drivers (the R-Car VIN driver already
supports two different APIs based on the SoC generation, which is an
entirely artificial split), either upstream, or in downstream kernels
(the Raspberry Pi unicam driver, for instance, may move to the MC API
for upstream, and retain video-node-centric behaviour controlled by an
option downstream). We should thus think about how we want to handle
this globally.
Personally, I think new drivers for embedded SoCs should use the MC API
only. By embedded, I mean here any system where the sensor needs to be
controlled directly by a subdev driver. The rationale is that we'll see
an increasing number of sensors exposing multiple subdevs, which would
require complex logic in the kernel if they were to be controlled
through video nodes only. Such logic would also need to implement
heuristics that will not be a good match for all use cases. This can
only work with a proper solution to support MC-based drivers in
userspace, and fortunately we're getting there :-)
Even if we mandate an MC-centric approach for new drivers, we will need
to deal with backward compatibility for both drivers that are currently
in-tree and need to move to the MC API (we have a known number of such
drivers, which shouldn't grow if we don't accept new ones), and for
drivers that are currently available through vendor kernels and don't
implement the MC API. The latter category is technically not our
problem, but ensuring that vendors will be able to preserve backward
compatibility with the existing user base will help getting drivers
mainlined, so it benefits us too. The solution for downstream kernel
should be the same as for existing upstream drivers (unless someone has
a good reason that would require a different solution).
So, if we consider that this problem will become more widespread, how do
we deal with it ? Do we need to select the API globally at the subsystem
level, per driver, or per device instance ? Should it be a compile-time
option only, a runtime option only, or a runtime option with a
compile-time default ? Controlling the option at runtime would be best I
believe, as that provides additional flexibility without much complexity
increase. Per-device compile-time selection (both for the option and for
its default value) would be difficult, I'd prefer ruling that out.
The only compile-time option would thus be either a subsystem-wide
default, or a per-driver default. The former seems of limited use to me.
What are the use cases for the latter, what default value would we pick
for the Kconfig option, and how do we expect distributions to select an
option ? I'm trying to figure out here whether that kernel option would
really be useful, or would just make the kernel configuration more
complex without a real use case.
> It is probably best if you rebase this series, fix 100/108 and (hopefully)
> this patch and post it as a v3. I'll take it.
Working on it now. If that's OK with you, I'll leave the Kconfig change
out for this patch for now, it can easily be done on top after we
finalize the discussion and won't cause any regression.
> > +
> > /* ------------------------------------------------------------------
> > * Format Handling
> > * ------------------------------------------------------------------
> > @@ -660,13 +664,17 @@ static int cal_async_notifier_complete(struct v4l2_async_notifier *notifier)
> > {
> > struct cal_dev *cal = container_of(notifier, struct cal_dev, notifier);
> > unsigned int i;
> > + int ret = 0;
> >
> > for (i = 0; i < ARRAY_SIZE(cal->ctx); ++i) {
> > if (cal->ctx[i])
> > cal_ctx_v4l2_register(cal->ctx[i]);
> > }
> >
> > - return 0;
> > + if (cal_mc_api)
> > + ret = v4l2_device_register_subdev_nodes(&cal->v4l2_dev);
> > +
> > + return ret;
> > }
> >
> > static const struct v4l2_async_notifier_operations cal_async_notifier_ops = {
> > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h
> > index 2d935691bf75..f5609216b7c6 100644
> > --- a/drivers/media/platform/ti-vpe/cal.h
> > +++ b/drivers/media/platform/ti-vpe/cal.h
> > @@ -160,6 +160,7 @@ struct cal_camerarx {
> > struct device_node *sensor_ep_node;
> > struct device_node *sensor_node;
> > struct v4l2_subdev *sensor;
> > + struct media_pipeline pipe;
> >
> > struct v4l2_subdev subdev;
> > struct media_pad pads[2];
> > @@ -224,6 +225,7 @@ struct cal_ctx {
> >
> > extern unsigned int cal_debug;
> > extern int cal_video_nr;
> > +extern bool cal_mc_api;
> >
> > #define cal_dbg(level, cal, fmt, arg...) \
> > do { \
--
Regards,
Laurent Pinchart
