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
