Re: [RFC 06/13] drm/dp: Read eDP version from DPCD

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Thierry Reding <thierry.reding@gmail.com>,
	dri-devel@lists.freedesktop.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [RFC 06/13] drm/dp: Read eDP version from DPCD
Date: Fri, 14 Aug 2015 13:37:06 +0300	[thread overview]
Message-ID: <87d1yqqhpp.fsf@intel.com> (raw)
In-Reply-To: <1439392001-29637-6-git-send-email-thierry.reding@gmail.com>

On Wed, 12 Aug 2015, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> If the sink support eDP, read the eDP revision from it's DPCD.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 30 +++++++++++++++++++++++++++++-
>  include/drm/drm_dp_helper.h     |  1 +
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 1fe181525604..c711b690508b 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -295,7 +295,7 @@ EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
>   */
>  int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
>  {
> -	u8 values[7];
> +	u8 values[16], value;
>  	int err;
>  
>  	memset(link, 0, sizeof(*link));
> @@ -323,6 +323,34 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
>  	if (values[6] & DP_SET_ANSI_8B10B)
>  		link->capabilities |= DP_LINK_CAP_ANSI_8B10B;
>  
> +	if (values[13] & DP_ALTERNATE_SCRAMBLER_RESET_CAP) {

That's not indicative of eDP, that's indicative of edp && alternate
scrambler support. DPCD_DISPLAY_CONTROL_CAPABLE bit is a better match:

"To allow Source devices to easily identify panels that use DPCD
Addresses 00700h through 007FFh, the DPCD_DISPLAY_CONTROL_CAPABLE bit in
the eDP_CONFIGURATION_CAP register (DPCD Address 0000Dh, bit 3) has been
assigned (eDP v1.2 (and higher)) to indicate this capability, as
described in Table 3-3."

Also, I'd really appreciate using the macros for DPCD offsets. I do not
remember these offsets by heart, and we've defined the macros according
to the spec so it would be faster to look things up there.

> +		err = drm_dp_dpcd_readb(aux, DP_EDP_DPCD_REV, &value);
> +		if (err < 0)
> +			return err;
> +
> +		switch (value) {
> +		case DP_EDP_11:
> +			link->edp = 0x11;

I'm really not sure if this is a good idea, since DP_EDP_11 ==
0. Essentially you're promoting to use the magic values for the version
checking in code. Sure, we've done that with the DPCD version, but now
link->revision is directly from DPCD, and this would not be.

BR,
Jani.

> +			break;
> +
> +		case DP_EDP_12:
> +			link->edp = 0x12;
> +			break;
> +
> +		case DP_EDP_13:
> +			link->edp = 0x13;
> +			break;
> +
> +		case DP_EDP_14:
> +			link->edp = 0x14;
> +			break;
> +
> +		default:
> +			DRM_ERROR("unsupported eDP version: %02x\n", value);
> +			break;
> +		}
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_dp_link_probe);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 9e70ea8b907d..f9e7f484a225 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -749,6 +749,7 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
>  
>  struct drm_dp_link {
>  	unsigned char revision;
> +	unsigned char edp;
>  	unsigned int rate;
>  	unsigned int num_lanes;
>  	unsigned long capabilities;
> -- 
> 2.4.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel