Hi Hyun and Jacopo,
On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> Hi Jacopo,
>
> Thanks for the patch.
>
> On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> > Introduce a new pad operation to allow retrieving the media bus
> > configuration on a subdevice pad.
> >
> > The newly introduced operation reassembles the s/g_mbus_config video
> > operation, which have been on their way to be deprecated since a long
> > time.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > ---
> > include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 67 insertions(+)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 761aa83a3f3c..3a1afc00e094 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> > unsigned short num_entries;
> > };
> >
> > +/**
> > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > + * @hsync_active: hsync active state: true for high, false for low
> > + * @vsync_active: vsync active state: true for high, false for low
> > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > + * @data_active: data lines active state: true for high, false for low
> > + */
> > +struct v4l2_mbus_parallel_config {
> > + bool hsync_active : 1;
> > + bool vsync_active : 1;
> > + bool pclk_rising : 1;
> > + bool data_active : 1;
> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > + * @data_lanes: number of data lanes in use
> > + * @clock_noncontinuous: non continuous clock enable flag
> > + */
> > +struct v4l2_mbus_csi2_dphy_config {
> > + unsigned int data_lanes : 3;
> > + bool clock_noncontinuous : 1;
> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > + *
> > + * TODO
> > + */
> > +struct v4l2_mbus_csi2_cphy_config {
> > + /* TODO */
> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_pad_config - media bus configuration
> > + *
> > + * Report the subdevice media bus information to inform the caller of the
> > + * current bus configuration. The structure describes bus configuration
> > + * parameters that might change in-between streaming sessions, in order to allow
> > + * the caller to adjust its media bus configuration to match what is reported
> > + * here.
> > + *
> > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > + * the bus type.
> > + *
> > + * @type: mbus type. See &enum v4l2_mbus_type
> > + * @parallel: parallel bus configuration parameters.
> > + * See &struct v4l2_mbus_parallel_config
> > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > + * See &struct v4l2_mbus_csi2_dphy_config
> > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > + * See &struct v4l2_mbus_csi2_cphy_config
> > + */
> > +struct v4l2_mbus_pad_config {
> > + enum v4l2_mbus_type type;
> > + union {
> > + struct v4l2_mbus_parallel_config parallel;
> > + struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > + struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > + };
> > +};
> > +
> > /**
> > * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> > * in video mode.
> > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> > *
> > * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > * may be adjusted by the subdev driver to device capabilities.
> > + * @get_mbus_config: get the current mbus configuration
> > */
> > struct v4l2_subdev_pad_ops {
> > int (*init_cfg)(struct v4l2_subdev *sd,
> > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> > struct v4l2_mbus_frame_desc *fd);
> > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > struct v4l2_mbus_frame_desc *fd);
> > + int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > + struct v4l2_mbus_pad_config *config);
>
> Because this can be used in many different ways, there's more chance it can
> be misused. That means, drivers call this in different locations, ex probe,
> get format, start stream,,,, and on differnt pads, src or sink. So imagine
> one set of drivers call on sink pad, and the other set call on source pad.
> It works well only until those are mixed together.
That subdevice operations can be called at both probe and s_stream() is
nothing new, I don't thin this is a new problem. But I agree maybe we
could limit get_mbus_config() in the core to only be valid four source
pads? Apart from this open question I think this patch looks good.
>
> So wouldn't it be better to put some restrictions? One is to document
> recommendations. I think this better be called in stream on because
> some bus config may change at runtime depending on other configuration.
> So any bus config prior to stream-on may be outdated. The other is to
> enforce in the code. Some, but maybe not all, can be handled in
> v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> source pad.
>
> Thanks,
> -hyun
>
> > };
> >
> > /**
> > --
> > 2.25.1
> >
--
Regards,
Niklas Söderlund
