Re: [PATCH] [RFC] drm/i915/dp: add support for load detect Apple DP->VGA dongles

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 06 Jun 2014, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> The DP1.2 spec has some bits for forced load sensing on VGA dongles,
> however the apple dongle I have doesn't do DP 1.2 yet, so I dug into
> its vendor specific area and found a register that seems to correspond
> to load detected on the outputs.
>
> The main reason I need this was at LCA this year the slide capture system
> failed to provide EDID, but I realised OSX worked. Really need to add support
> to nouveau, but hey i915 is a start.
>
> The dongle also appears to use short IRQs to denote a plug/unplug event,
> though something seems to be failing in reading back the dpcd values from it.
> (also this is based on my MST work just because.)

Good timing for making use of the OUI! See

http://mid.gmane.org/1401920981-3137-1-git-send-email-clinton.a.taylor@intel.com

> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  include/drm/drm_dp_helper.h      | 19 +++++++++++++++++++
>  3 files changed, 49 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c500f63..49afd7d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3173,6 +3173,16 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>  		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>  			      buf[0], buf[1], buf[2]);
>  
> +	intel_dp->is_apple_vga = false;
> +	if (drm_dp_branch_is_apple(buf)) {
> +		u8 mybuf[8];
> +		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI + 3, mybuf, 8)) {

That may return negative values for errors.

FWIW I've thought about reading the sink/branch device identification
strings unconditionally along with the OUI just for debug purposes. I
wouldn't be opposed to such a change here.

> +			if (drm_dp_apple_has_load_detect(mybuf)) {
> +				DRM_DEBUG_KMS("detected Apple DP VGA dongle\n");
> +				intel_dp->is_apple_vga = true;
> +			}
> +		}
> +	}

I think I'd like this abstracted to a dedicated function and doing:

	intel_dp->is_apple_vga = is_apple_vga(intel_dp, buf);

or something.

>  	edp_panel_vdd_off(intel_dp, false);
>  }
>  
> @@ -3404,6 +3414,21 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	if (drm_probe_ddc(&intel_dp->aux.ddc))
>  		return connector_status_connected;
>  
> +	if (intel_dp->is_apple_vga) {
> +		u8 detect_buf;
> +		int ret;
> +		ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_APPLE_LOAD_DETECT,
> +					&detect_buf);
> +
> +		if (ret == 1) {
> +			DRM_DEBUG_KMS("Apple VGA detect is 0x%x\n", detect_buf);
> +			/* I suspect 4 == load, 8 == edid */
> +			if (detect_buf)
> +				return connector_status_connected;
> +			else
> +				return connector_status_disconnected;
> +		}
> +	}
>  	/* Well we tried, say unknown for unreliable port types */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
>  		type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> @@ -3854,6 +3879,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  			 * but for short hpds we should check it now
>  			 */
>  			intel_dp_check_link_status(intel_dp);
> +
> +			/* if we get a short hpd on apple vga - its a hotplug */
> +			if (intel_dp->is_apple_vga)
> +				return true;
>  		}
>  	}
>  	return false;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1edb38a..23f767c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -549,6 +549,7 @@ struct intel_dp {
>  	bool use_tps3;
>  	bool can_mst; /* this port supports mst */
>  	bool is_mst;
> +	bool is_apple_vga;
>  	int active_mst_links;
>  	/* connector directly attached - won't be use for modeset in mst world */
>  	struct intel_connector *attached_connector;
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 517d6c1..c8ebd27 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -612,4 +612,23 @@ int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
>  int drm_dp_aux_register(struct drm_dp_aux *aux);
>  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>  
> +#define DP_APPLE_OUI 0x10fa
> +
> +#define DP_APPLE_LOAD_DETECT (DP_BRANCH_OUI + 12)
> +
> +static inline bool drm_dp_branch_is_apple(const u8 buf[3])

Naming buf and its size similar to other drm_dp_* helpers use would be
nice.

> +{
> +	if (buf[0] == ((DP_APPLE_OUI >> 16) & 0xff) &&
> +	    buf[1] == ((DP_APPLE_OUI >> 8) & 0xff) &&
> +	    buf[2] == ((DP_APPLE_OUI & 0xff)))
> +		return true;
> +	return false;
> +}
> +
> +static inline bool drm_dp_apple_has_load_detect(const u8 buf[8])

Ditto.

> +{
> +	if (!memcmp((const char *)buf, "mVGAa", 5))

Why the cast?

BR,
Jani.


> +		return true;
> +	return false;
> +}
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center