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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel