Re: [PATCH v13 6/8] media: i2c: add DS90UB960 driver

[Tomi Valkeinen]

On 16/05/2023 16:32, Ludwig Zenz wrote:
> > Hi,
> >
> > On 16/05/2023 15:35, Ludwig Zenz wrote:
> > > On Wed, 26 Apr 2023 14:51:12 +0300, Tomi Valkeinen wrote:
> > >
> > > [...]
> > >
> > > >    +static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
> > > >    +                                         struct v4l2_subdev_state *state)
> > > >    +{
> > > >    +  u8 fwd_ctl;
> > > >    +  struct {
> > > >    +          u32 num_streams;
> > > >    +          u8 pixel_dt;
> > > >    +          u8 meta_dt;
> > > >    +          u32 meta_lines;
> > > >    +          u32 tx_port;
> > > >    +  } rx_data[UB960_MAX_RX_NPORTS] = {};
> > > >    +  u8 vc_map[UB960_MAX_RX_NPORTS] = {};
> > > >    +  struct v4l2_subdev_route *route;
> > > >    +  unsigned int nport;
> > > >    +  int ret;
> > > >    +
> > > >    +  ret = ub960_validate_stream_vcs(priv);
> > > >    +  if (ret)
> > > >    +          return ret;
> > > >    +
> > > >    +  ub960_get_vc_maps(priv, state, vc_map);
> > > >    +
> > > >    +  for_each_active_route(&state->routing, route) {
> > > >    +          struct ub960_rxport *rxport;
> > > >    +          struct ub960_txport *txport;
> > > >    +          struct v4l2_mbus_framefmt *fmt;
> > > >    +          const struct ub960_format_info *ub960_fmt;
> > > >    +          unsigned int nport;
> > > >    +
> > > >    +          nport = ub960_pad_to_port(priv, route->sink_pad);
> > > >    +
> > > >    +          rxport = priv->rxports[nport];
> > > >    +          if (!rxport)
> > > >    +                  return -EINVAL;
> > > >    +
> > > >    +          txport = priv->txports[ub960_pad_to_port(priv, route->source_pad)];
> > > >    +          if (!txport)
> > > >    +                  return -EINVAL;
> > > >    +
> > > >    +          rx_data[nport].tx_port = ub960_pad_to_port(priv, route->source_pad);
> > > >    +
> > > >    +          rx_data[nport].num_streams++;
> > > >    +
> > > >    +          /* For the rest, we are only interested in parallel busses */
> > > >    +          if (rxport->rx_mode == RXPORT_MODE_CSI2_SYNC ||
> > > >    +              rxport->rx_mode == RXPORT_MODE_CSI2_ASYNC)
> > > >    +                  continue;
> > > >    +
> > > >    +          if (rx_data[nport].num_streams > 2)
> > > >    +                  return -EPIPE;
> > > >    +
> > > >    +          fmt = v4l2_subdev_state_get_stream_format(state,
> > > >    +                                                    route->sink_pad,
> > > >    +                                                    route->sink_stream);
> > > >    +          if (!fmt)
> > > >    +                  return -EPIPE;
> > > >    +
> > > >    +          ub960_fmt = ub960_find_format(fmt->code);
> > > >    +          if (!ub960_fmt)
> > > >    +                  return -EPIPE;
> > > >    +
> > > >    +          if (ub960_fmt->meta) {
> > > >    +                  if (fmt->height > 3) {
> > > >    +                          dev_err(&priv->client->dev,
> > > >    +                                  "rx%u: unsupported metadata height %u\n",
> > > >    +                                  nport, fmt->height);
> > > >    +                          return -EPIPE;
> > > >    +                  }
> > > >    +
> > > >    +                  rx_data[nport].meta_dt = ub960_fmt->datatype;
> > > >    +                  rx_data[nport].meta_lines = fmt->height;
> > > >    +          } else {
> > > >    +                  rx_data[nport].pixel_dt = ub960_fmt->datatype;
> > > >    +          }
> > > >    +  }
> > > >    +
> > > >    +  /* Configure RX ports */
> > > >    +
> > > >    +  fwd_ctl = 0;
> > >
> > > Hello, I have only used the first RX port in my setup (ds90ub933 to ds90ub964). The logic for activating/deactivating the Rx ports did not work for me. My suggestion is:
> >
> > Why doesn't it work? What happens?
> >
> >   Tomi
>
> Hello Tomi,
>
> the port rx0 which I need was disabled and the other ports rx1 to rx3 were enabled. In other words, the exact inverse of the required selection.
>
> > > >   +		/* Forwarding */
> > > >   +
> > > >   +		fwd_ctl |= BIT(4 + nport); /* forward disable */
> According to the data sheet, a set bit4-7 in fwd_ctl means that the channel is disabled. So the comment 'forward disable' is correct. While debugging, however, this code was only reached for the ports to be enabled but not for the ones which should be disabled.

This is just a setup phase, where we initialize the registers for the 
ports we want to use. The forwarding is then enabled later, in 
ub960_enable_rx_port, and even later disabled in ub960_disable_rx_port.

This assumes that the forwarding is disabled in the registers by default 
(which it is in UB960).

I need to try this on my HW to verify my understanding is correct, but 
looking at the code, it is indeed a bit buggy.

At this setup phase we disable the forwarding for ports we'll use, and 
enable the forwarding for ports we don't use (which doesn't make sense). 
Later, when the streaming is started for that port, we enable the 
forwarding. So here we should just always disable the forwarding for all 
ports.

Saying "disable the forwarding" is perhaps a bit confusing here, as the 
the forwarding should already be disabled in the HW here anyway. But as 
we write the UB960_SR_FWD_CTL1, we need to set that bit.

So. You should see the rx0 getting enabled (later, in 
ub960_enable_rx_port), and I'm curious why you don't see that.

 Tomi