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
