Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

On Tue, Sep 01, 2020 at 10:46:03AM +0300, Tomi Valkeinen wrote:
> Hi Swapnil,
> 
> On 31/08/2020 11:23, Swapnil Jakhade wrote:
> 
> > +static int cdns_mhdp_validate_mode_params(struct cdns_mhdp_device *mhdp,
> > +					  const struct drm_display_mode *mode,
> > +					  struct drm_bridge_state *bridge_state)
> > +{
> > +	u32 tu_size = 30, line_thresh1, line_thresh2, line_thresh = 0;
> > +	u32 rate, vs, vs_f, required_bandwidth, available_bandwidth;
> > +	struct cdns_mhdp_bridge_state *state;
> > +	int pxlclock;
> > +	u32 bpp;
> > +
> > +	state = to_cdns_mhdp_bridge_state(bridge_state);
> > +
> > +	pxlclock = mode->crtc_clock;
> > +
> > +	/* Get rate in MSymbols per second per lane */
> > +	rate = mhdp->link.rate / 1000;
> > +
> > +	bpp = cdns_mhdp_get_bpp(&mhdp->display_fmt);
> 
> None of the above are used when calling cdns_mhdp_bandwidth_ok(). For clarity, I'd move the above
> lines a bit closer to where they are needed, as currently it makes me think the above values are
> used when checking the bandwidth.
> 
> > +	if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
> > +				    mhdp->link.rate)) {
> > +		dev_err(mhdp->dev, "%s: Not enough BW for %s (%u lanes at %u Mbps)\n",
> > +			__func__, mode->name, mhdp->link.num_lanes,
> > +			mhdp->link.rate / 100);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* find optimal tu_size */
> > +	required_bandwidth = pxlclock * bpp / 8;
> > +	available_bandwidth = mhdp->link.num_lanes * rate;
> > +	do {
> > +		tu_size += 2;
> > +
> > +		vs_f = tu_size * required_bandwidth / available_bandwidth;
> > +		vs = vs_f / 1000;
> > +		vs_f = vs_f % 1000;
> > +		/* Downspreading is unused currently */
> > +	} while ((vs == 1 || ((vs_f > 850 || vs_f < 100) && vs_f != 0) ||
> > +		 tu_size - vs < 2) && tu_size < 64);
> > +
> > +	if (vs > 64) {
> > +		dev_err(mhdp->dev,
> > +			"%s: No space for framing %s (%u lanes at %u Mbps)\n",
> > +			__func__, mode->name, mhdp->link.num_lanes,
> > +			mhdp->link.rate / 100);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (vs == tu_size)
> > +		vs = tu_size - 1;
> > +
> > +	line_thresh1 = ((vs + 1) << 5) * 8 / bpp;
> > +	line_thresh2 = (pxlclock << 5) / 1000 / rate * (vs + 1) - (1 << 5);
> > +	line_thresh = line_thresh1 - line_thresh2 / mhdp->link.num_lanes;
> > +	line_thresh = (line_thresh >> 5) + 2;
> > +
> > +	state->vs = vs;
> > +	state->tu_size = tu_size;
> > +	state->line_thresh = line_thresh;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
> > +				  struct drm_bridge_state *bridge_state,
> > +				  struct drm_crtc_state *crtc_state,
> > +				  struct drm_connector_state *conn_state)
> > +{
> > +	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> > +	const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> > +	int ret;
> > +
> > +	mutex_lock(&mhdp->link_mutex);
> > +
> > +	if (!mhdp->plugged) {
> > +		mhdp->link.rate = mhdp->host.link_rate;
> > +		mhdp->link.num_lanes = mhdp->host.lanes_cnt;
> > +	}
> > +
> > +	ret = cdns_mhdp_validate_mode_params(mhdp, mode, bridge_state);
> > +
> > +	mutex_unlock(&mhdp->link_mutex);
> > +
> > +	return ret;
> > +}
> 
> Laurent mentioned that atomic_check should not change state. Note that
> cdns_mhdp_validate_mode_params also changes state, as it calculates tu_size, vs and line_thresh.

.atomic_check() isn't allowed to change any global state, which means
both hardware state and data in cdns_mhdp_device. The drm_bridge_state
(and thus the cdns_mhdp_bridge_state) can be modified as it stores the
state for the atomic commit being checked.

> There seems to be issues with mode changes, but I think the first step would be to clarify the
> related code a bit. cdns_mhdp_validate_mode_params() is misnamed, I think it should be renamed to
> calculate_tu or something like that.
> 
> cdns_mhdp_bandwidth_ok() should take display_fmt or bpp as a parameter, as currently it digs that up
> from the current state.
> 
> Probably cdns_mhdp_validate_mode_params() would be better if it doesn't write the result to the
> state, but returns the values. That way it could also be used to verify if suitable settings can be
> found, without changing the state.

This use case is actually a very good example of proper usage of the
atomic state :-) .atomic_check() has to perform computations to verify
the atomic commit, and storing the results in the commit's state
prevents duplicating the same calculation at .atomic_commit() time.

> The are two issues I see with some testing, which are probably related.
> 
> The first one is that if I run kmstest with a new mode, I see tu-size & co being calculated. But the
> calculation happens before link training, which doesn't make sense. So I think what's done here is
> that atomic_check causes tu-size calculations, then atomic_enable does link training and enables the
> video.
> 
> The second happens when my monitor fails with the first CR after power-on, and the driver drops
> number-of-lanes to 2. It goes like this:
> 
> The driver is loaded. Based on EDID, fbdev is created with 1920x1200. Link training is done, which
> has the CR issue, and because of that the actual mode that we get is 1280x960. I get a proper
> picture here, so far so good.
> 
> Then if I run kmstest, it only allows 1280x960 as the link doesn't support higher modes (that's ok).
> It the does link training and gets a 4 lane link, and enables 1280x960. But the picture is not ok.
> 
> If I then exit kmstest, it goes back to fbdev, but now that picture is broken also.
> 
> Running kmstest again gives me 1920x1200 (as the link has been 4 lane now), and the picture is fine.
> 
> I think the above suggests that the driver is not properly updating all the registers based on the
> new mode and link. I tried adding cdns_mhdp_validate_mode_params() call to
> cdns_mhdp_atomic_enable(), so that tu-size etc will be calculated, but that did not fix the problem.

-- 
Regards,

Laurent Pinchart