Date:   Tue, 1 Sep 2020 10:46:03 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...com>
To:     Swapnil Jakhade <sjakhade@...ence.com>,
        <Laurent.pinchart@...asonboard.com>,
        <dri-devel@...ts.freedesktop.org>
CC:     <airlied@...ux.ie>, <daniel@...ll.ch>, <robh+dt@...nel.org>,
        <a.hajda@...sung.com>, <narmstrong@...libre.com>,
        <jonas@...boo.se>, <jernej.skrabec@...l.net>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <mparab@...ence.com>, <yamonkar@...ence.com>, <jsarha@...com>,
        <nsekhar@...com>, <praneeth@...com>, <nikhil.nd@...com>
Subject: Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546
 DPI/DP bridge

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.

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.

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.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Powered by blists - more mailing lists