On 09/20/17 14:50, Sakari Ailus wrote: > Hi Hans and others, > > On Wed, Sep 20, 2017 at 01:24:02PM +0200, Hans Verkuil wrote: >> On 09/20/17 13:00, Dave Stevenson wrote: >>> On 20 September 2017 at 11:23, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote: >>>> Hi, >>>> >>>> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote: >>>>> Hi Mauro & Philipp >>>>> >>>>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab >>>>> <mchehab@xxxxxxxxxxxxxxxx> wrote: >>>>>> Em Tue, 19 Sep 2017 17:24:45 +0200 >>>>>> Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> escreveu: >>>>>> >>>>>>> Hi Dave, >>>>>>> >>>>>>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote: >>>>>>>> The existing fixed value of 16 worked for UYVY 720P60 over >>>>>>>> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888 >>>>>>>> 1080P60 needs 6 lanes at 594MHz). >>>>>>>> It doesn't allow for lower resolutions to work as the FIFO >>>>>>>> underflows. >>>>>>>> >>>>>>>> Using a value of 300 works for all resolutions down to VGA60, >>>>>>>> and the increase in frame delay is <4usecs for 1080P60 UYVY >>>>>>>> (2.55usecs for RGB888). >>>>>>>> >>>>>>>> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> >>>>>>> >>>>>>> Can we increase this to 320? This would also allow >>>>>>> 720p60 at 594 Mbps / 4 lanes, according to the xls. >>>>> >>>>> Unless I've missed something then the driver would currently request >>>>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO >>>>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works >>>>> on a FIFO setting of 16. >>>>> How/why were you thinking we need to run all four lanes for 720p60 >>>>> without other significant driver mods around lane config? >>>> >>>> The driver currently silently changes the number of active lanes >>>> depending on required data rate, with no way to communicate it to the >>>> receiver. >>> >>> It is communicated over the subdevice API - tc358743_g_mbus_config >>> reports back the appropriate number of lanes to the receiver >>> subdevice. >>> A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call >>> as you're starting streaming therefore gives you the correct >>> information. That's what I've just done for the BCM283x Unicam >>> driver[1], but admittedly I'm not using the media controller API which >>> i.MX6 is. >> >> Shouldn't this information come from the device tree? The g_mbus_config >> op is close to being deprecated or even removed. There are currently only >> two obscure V4L2 bridge drivers that call it. It dates from pre-DT times >> I rather not see it used in a new bridge driver. >> >> The problem is that contains data that belongs to the DT (hardware >> capabilities). Things that can actually change dynamically should be >> communicated via another op. We don't have that, so that should be created. > > The DT tells how many lanes are connected in hardware, but up to now that's > also been the number of lanes actually used. > > The g_mbus_config() is there, and I'd like to replace that with the more > generic frame descriptors, with CSI-2 virtual channel and data type > support. They're however not quite ready yet nor I've recently worked on > them. > > I think using g_mbus_config() for the purpose right now is entirely > acceptable, we can rework that later on when adding support for frame > descriptors. > I don't like it :-) Currently g_mbus_config returns (and I quote from v4l2-mediabus.h): "How many lanes the client can use". I.e. the capabilities of the HW. If we are going to use this to communicate how many lines are currently in use, then I would propose that we add a lane mask, i.e. something like this: /* Number of lanes in use, 0 == use all available lanes (default) */ #define V4L2_MBUS_CSI2_LANE_MASK (3 << 10) And add comments along the lines that this is a temporary fix. I would feel a lot happier (or a lot less unhappy) if we'd do it this way. Rather than re-interpreting bits that are not quite what they should be. I'd also add a comment that all other flags must be 0 if the device tree is used. This to avoid mixing the two. Regards, Hans
