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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Aug 2020 05:17:29 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Tomi Valkeinen <tomi.valkeinen@...com>
Cc:     Swapnil Jakhade <sjakhade@...ence.com>, airlied@...ux.ie,
        daniel@...ll.ch, robh+dt@...nel.org, a.hajda@...sung.com,
        narmstrong@...libre.com, jonas@...boo.se, jernej.skrabec@...l.net,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, mparab@...ence.com,
        yamonkar@...ence.com, jsarha@...com, nsekhar@...com,
        praneeth@...com
Subject: Re: [PATCH v8 2/3] drm: bridge: Add support for Cadence MHDP DPI/DP
 bridge

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