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

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

Hi Tomi,

On Fri, Aug 14, 2020 at 11:22:09AM +0300, Tomi Valkeinen wrote:
> On 11/08/2020 05:36, Laurent Pinchart wrote:
> 
> >> +static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
> >> +{
> >> +	u32 bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
> >> +	struct drm_connector *conn = &mhdp->connector;
> >> +	struct drm_bridge *bridge = &mhdp->bridge;
> >> +	int ret;
> >> +
> >> +	if (!bridge->encoder) {
> >> +		DRM_ERROR("Parent encoder object not found");
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	conn->polled = DRM_CONNECTOR_POLL_HPD;
> >> +
> >> +	ret = drm_connector_init(bridge->dev, conn, &cdns_mhdp_conn_funcs,
> >> +				 DRM_MODE_CONNECTOR_DisplayPort);
> >> +	if (ret) {
> >> +		DRM_ERROR("Failed to initialize connector with drm\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
> >> +
> >> +	ret = drm_display_info_set_bus_formats(&conn->display_info,
> >> +					       &bus_format, 1);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	conn->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH;
> > 
> > Aren't these supposed to be retrieved from the display ? Why do we need
> > to override them here ?
> 
> DE_HIGH is meant for the display controller. I think this should be in bridge->timings->input_bus_flags
> 
> >> +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;
> >> +
> >> +	if (!mhdp->plugged)
> >> +		return 0;
> >> +
> >> +	mutex_lock(&mhdp->link_mutex);
> >> +
> >> +	if (!mhdp->link_up) {
> >> +		ret = cdns_mhdp_link_up(mhdp);
> >> +		if (ret < 0)
> >> +			goto err_check;
> >> +	}
> > 
> > atomic_check isn't supposed to access the hardware. Is there a reason
> > this is needed ?
> 
> We have been going back and forth with this. The basic problem is that
> to understand which videomodes can be used, you need to do link
> training to see the bandwidth available.
> 
> I'm not sure if we strictly need to do LT in atomic check, though...
> It would then pass modes that can't be used, but perhaps that's not a
> big issue.
> 
> I think the main point with doing LT in certain places is to filter
> the list of video modes passed to userspace, as we can't pass the
> modes from EDID directly without filtering them based on the link
> bandwidth.

I've discussed this on #dri-devel with Daniel a week or two ago. His
advice was to drop LT from atomic check, relying on DPCD (DisplayPort
Configuration Data) instead. If LT then fails to negotiate a high-enough
bandwidth for the mode when enabling the output, the link-status
property should be set to bad, and userspace should retry. I think
you've seen the discussion, I can provide a log if needed.

-- 
Regards,

Laurent Pinchart