Re: [Intel-gfx] [PATCH 1/4] drm: Add helper for DP++ adaptors

On Thu, Mar 31, 2016 at 07:25:36PM +0000, Zanoni, Paulo R wrote:

Em Ter, 2016-02-23 às 18:46 +0200, ville.syrjala@linux.intel.com escreveu:

...

From: Ville Syrjälä ville.syrjala@linux.intel.com

Add a helper which aids in he identification of DP dual mode (aka. DP++) adaptors. There are several types of adaptors specified: type 1 DVI, type 1 HDMI, type 2 DVI, type 2 HDMI

Type 1 adaptors have a max TMDS clock limit of 165MHz, type 2 adaptors may go as high as 300MHz and they provide a register informing the source device what the actual limit is. Supposedly also type 1 adaptors may optionally implement this register. This TMDS clock limit is the main reason why we need to identify these adaptors.

Type 1 adaptors provide access to their internal registers and the sink DDC bus through I2C. Type 2 adaptors provide this access both via I2C and I2C-over-AUX. A type 2 source device may choose to implement either or both of these methods. If a source device implements only the I2C-over-AUX method, then the driver will obviously need specific support for such adaptors since the port is driven like an HDMI port, but DDC communication happes over the AUX channel.

This helper should be enough to identify the adaptor type (some type 1 DVI adaptors may be a slight exception) and the maximum TMDS clock limit. Another feature that may be available is control over the TMDS output buffers on the adaptor, possibly allowing for some power saving when the TMDS link is down.

Other user controllable features that may be available in the adaptors are downstream i2c bus speed control when using i2c-over-aux, and some control over the CEC pin. I chose not to provide any helper functions for those since I have no use for them in i915 at this time. The rest of the registers in the adaptor are mostly just information, eg. IEEE OUI, hardware and firmware revision, etc.

Please run a spell checker and do some proof-reading both in the commit message and in the code comments. Multiple instances of "sizo", "Hoever", "adator", "Identyfy", etc. I also spotted some typos in the next commits.

...

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/Makefile                  |   2 +-  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 328 ++++++++++++++++++++++++++++++  include/drm/drm_dp_dual_mode_helper.h     |  80 ++++++++  3 files changed, 409 insertions(+), 1 deletion(-)  create mode 100644 drivers/gpu/drm/drm_dp_dual_mode_helper.c  create mode 100644 include/drm/drm_dp_dual_mode_helper.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 6eb94fc561dc..22228ef50f36 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o    drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \   drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \

• drm_kms_helper_common.o
  

• drm_kms_helper_common.o drm_dp_dual_mode_helper.o
  

drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c new file mode 100644 index 000000000000..bfe511c09568 --- /dev/null +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c @@ -0,0 +1,328 @@ +/*

• • Copyright © 2016 Intel Corporation
• • Permission is hereby granted, free of charge, to any person

obtaining a

• • copy of this software and associated documentation files (the

"Software"),

• • to deal in the Software without restriction, including without

limitation

• • the rights to use, copy, modify, merge, publish, distribute,

sublicense,

• • and/or sell copies of the Software, and to permit persons to whom

the

• • Software is furnished to do so, subject to the following

conditions:

• • The above copyright notice and this permission notice shall be

included in

• • all copies or substantial portions of the Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,

EXPRESS OR

• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF

MERCHANTABILITY,

• • FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO

EVENT SHALL

• • THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,

DAMAGES OR

• • OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR

OTHERWISE,

• • ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE

USE OR

• • OTHER DEALINGS IN THE SOFTWARE.
• */

+#include <linux/errno.h> +#include <linux/export.h> +#include <linux/i2c.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <drm/drm_dp_dual_mode_helper.h> +#include <drm/drmP.h>

+/**

• • DOC: DP dual mode (aka. DP++) adaptor helpers
• • Helper functions to deal with DP dual mode adaptors.
• • Type 1:
• • Adaptor registers (if any) and the sink DDC bus may be accessed

via I2C.

• • Type 2:
• • Adaptor registers and sink DDC bus can be accessed either via I2C

or

• • I2C-over-AUX. Source devices may choose to implement either one

or

• • both of these access methods.
• */

+#define DP_DUAL_MODE_SLAVE_ADDRESS 0x40

+/**

• • drm_dp_dual_mode_read - Read from the DP dual mode adaptor

register(s)

• • adapter: I2C adapter for the DDC bus
• • offset: register offset
• • buffer: buffer for return data
• • size: sizo of the buffer
• • Reads @size bytes from the DP dual mode adaptor registers
• • starting at @offset.
• • Returns:
• • 0 on success, negative error code on failure
• */

+ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,

• 	      u8 offset, void *buffer, size_t size)
  

+{

• struct i2c_msg msgs[] = {

• {
  

• 	.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
  

• 	.flags = 0,
  

• 	.len = 1,
  

• 	.buf = &offset,
  

• },
  

• {
  

• 	.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
  

• 	.flags = I2C_M_RD,
  

• 	.len = size,
  

• 	.buf = buffer,
  

• },
  

• };
• int ret;
• ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
• if (ret < 0)

• return ret;
  

• if (ret != ARRAY_SIZE(msgs))

• return -EPROTO;
  

No retries needed here or below? (asking this due to the comment in drm_edid.c about retries)

The EDID retries are mostly cargo culted. I suspect you don't really need them unless the connector is about to fall off, or the wiring is made of cotton.

...

• return 0;

+}

+/**

• • drm_dp_dual_mode_write - Write to the DP dual mode adaptor

register(s)

• • adapter: I2C adapter for the DDC bus
• • offset: register offset
• • buffer: buffer for write data
• • size: sizo of the buffer
• • Writes @size bytes to the DP dual mode adaptor registers
• • starting at @offset.
• • Returns:
• • 0 on success, negative error code on failure
• */

+ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,

• 	       u8 offset, const void *buffer, size_t
  

size) +{

• struct i2c_msg msg = {

• .addr = DP_DUAL_MODE_SLAVE_ADDRESS,
  

• .flags = 0,
  

• .len = 1 + size,
  

• .buf = NULL,
  

• };
• void *data;
• int ret;
• data = kmalloc(msg.len, GFP_TEMPORARY);
• if (!data)

• return -ENOMEM;
  

• msg.buf = data;
• memcpy(data, &offset, 1);
• memcpy(data + 1, buffer, size);
• ret = i2c_transfer(adapter, &msg, 1);
• kfree(data);
• if (ret < 0)

• return ret;
  

• if (ret != 1)

• return -EPROTO;
  

• return 0;

+} +EXPORT_SYMBOL(drm_dp_dual_mode_write);

+static bool is_hdmi_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN]) +{

• static const char

dp_dual_mode_hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN] =

• "DP-HDMI ADAPTOR\x04";
  

• return memcmp(hdmi_id, dp_dual_mode_hdmi_id,

•       sizeof(dp_dual_mode_hdmi_id)) == 0;
  

+}

+/**

• • drm_dp_dual_mode_detect - Identyfy the DP dual mode adaptor
• • adapter: I2C adapter for the DDC bus
• • Attempt to identify the type of the DP dual mode adaptor used.
• • Note that when the answer is @DRM_DP_DUAL_MODE_NONE it's not

So how about adding a new or just renaming it to DRM_DP_DUAL_MODE_UNKNOWN? Let's not assume everybody is going to read the docs.

I suppose adding an extra enum value can't hurt.

...

• • certain whether we're dealing with a native HDMI port or
• • a type 1 DVI dual mode adaptor. The driver will have to use
• • some other hardware/driver specific mechanism to make that
• • distinction.
• • Returns:
• • The type of the DP dual mode adaptor used
• */

+enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter) +{

• char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN] = {};
• uint8_t adaptor_id = 0x00;
• ssize_t ret;
• /*
• * Let's see if the adaptor is there the by reading the
• * HDMI ID registers.
• *
• * Note that type 1 DVI adaptors are not required to

implemnt

• * any registers, and that presents a problem for detection.
• * If the i2c transfer is nacked, we may or may not be

dealing

• * with a type 1 DVI adaptor. Some other mechanism of

detecting

• * the presence of the adaptor is required. One way would be
• * to check the state of the CONFIG1 pin, Another method

would

• * simply require the driver to know whether the port is a

DP++

• * port or a native HDMI port. Both of these methods are

entirely

• * hardware/driver specific so we can't deal with them here.
• */
• ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_HDMI_ID,

• 		    hdmi_id, sizeof(hdmi_id));
  

• if (ret)

• return DRM_DP_DUAL_MODE_NONE;
  

• /*
• * Sigh. Some (maybe all?) type 1 adaptors are broken and

ack

• * the offset but ignore it, and instead they just always

return

• * data from the start of the HDMI ID buffer. So for a

broken

• * type 1 HDMI adaptor a single byte read will always give

us

• * 0x44, and for a type 1 DVI adaptor it should give 0x00
• * (assuming it implements any registers).

So shouldn't we just try to read from 0x00 to 0x1F in a single shot? Would this work for your specific adaptor? It would be interesting to know.

I think I did consider that after giving up on another strange detection scheme that would have perhaps allowed me to detect decent type 1 adaptors. IIRC the adaptors I tested returned 0x00 or 0xff for the 0x10-0x20 range when doing a single read, but I suspect they might as well just wrap after 0x0f since the spec says they don't need to implement anything beyond 0x00-0x0f. While all of those results would work just as well as reading the single byte, I decided that there's no point in doing that since I had not seem a single sane type 1 adaptor. They're clearly all just bottom of the barrel hardware so seems unlikely anyone would go to the effort of implementing one decently. So in the end I just figured that minimizing the amount of data we have to read is the sane thing to do.

...

Fortunately neither

• * of those values will match the type 2 signature of the
• * DP_DUAL_MODE_ADAPTOR_ID register so we can proceed with
• * the type 2 adaptor detection safely even in the presence
• * of broken type 1 adaptors.
• */
• ret = drm_dp_dual_mode_read(adapter,

DP_DUAL_MODE_ADAPTOR_ID,

• 		    &adaptor_id,
  

sizeof(adaptor_id));

• if (ret || (adaptor_id != (DP_DUAL_MODE_TYPE_TYPE2 |

• 		   DP_DUAL_MODE_REV_TYPE2))) {
  

• if (is_hdmi_adaptor(hdmi_id))
  

• 	return DRM_DP_DUAL_MODE_TYPE1_HDMI;
  

• else
  

• 	return DRM_DP_DUAL_MODE_TYPE1_DVI;
  

• } else {

• if (is_hdmi_adaptor(hdmi_id))
  

• 	return DRM_DP_DUAL_MODE_TYPE2_HDMI;
  

• else
  

• 	return DRM_DP_DUAL_MODE_TYPE2_DVI;
  

• }

+} +EXPORT_SYMBOL(drm_dp_dual_mode_detect);

+/**

• • drm_dp_dual_mode_max_tmds_clock - Max TMDS clock for DP dual mode

adaptor

• • adapter: I2C adapter for the DDC bus
• • Determine the max TMDS clock the adaptor supports based on the
• • DP_DUAL_MODE_MAX_TMDS_CLOCK register. The register is mandatory

for

• • type 2 adaptors, optional for type 1 adaptors. Hoever, as some

type 1

• • adaptors are broken (see comments in drm_dp_dual_mode_detect()

for the

• • details) one probably shouldn't use this with type 1 adaptors at

all.

• • Type 1 adaptors should anyway be always limited to 165 MHz.

I always assume programmers are as bad as possible, so I wonder if we should just also call drm_dp_dual_mode_detect() here.

This crossed my mind but I didn't want to slow down things needlessly.

I did in fact consider that we might want to raise the abstraction level a bit and introduce some kind of dp_dual_mode object that keeps around the relevant information/state and would allow drivers to avoid the type checks and whatnot. Unfortunately those cursed type 1 DVI adaptors would still require the driver to step in if the detection fails to find anything, so we'd need a callback into the driver or something.

And so, since the driver anyway has to deal with the type 1 DVI adaptors, I decided that trying to hide these sort of details inside the helper is pretty much pointless.

Also try the trick of reading the whole thing at once instead of the specific address. Same goes for the other functions. I also see that this would make patches 2-4 different, so this is just an idea, not a requirement.

As I suspect some adaptors might just wrap around at 0xf, doing a big read doesn't feel entirely safe here.

...

• • Returns:
• • Maximum supported TMDS clock rate for the DP dual mode adaptor in

kHz.

• */

+int drm_dp_dual_mode_max_tmds_clock(struct i2c_adapter *adapter) +{

• uint8_t max_tmds_clock;

• ssize_t ret;

• /*

• * Type 1 adaptors are limited to 165MHz

• * Type 2 adaptors can tells us their limit

"can tells"

...

• */
• ret = drm_dp_dual_mode_read(adapter,

DP_DUAL_MODE_MAX_TMDS_CLOCK,

• 		    &max_tmds_clock,
  

sizeof(max_tmds_clock));

• if (ret)

Maybe also "if (ret || max_tmds_clock == 0 || max_tmds_clock == 0xFF)"?

That would seem sensible indeed.

...

• return 165000;
  

• return max_tmds_clock * 5000 / 2;

+} +EXPORT_SYMBOL(drm_dp_dual_mode_max_tmds_clock);

+/**

• • drm_dp_dual_mode_get_tmds_output - Get the state of the TMDS

output buffers in the DP dual mode adaptor

• • adapter: I2C adapter for the DDC bus
• • enabled: current state of the TMDS output buffers
• • Get the state of the TMDS output buffers in the adaptor.
• • DP_DUAL_MODE_TMDS_OEN register is mandatory for type 2 adaptors,
• • optionals for type 1 adaptors. Hoever, as some type 1 adaptors

are

• • broken (see comments in drm_dp_dual_mode_detect() for the

details)

• • one probably shouldn't use this with type 1 adaptors at all.
• • Returns:
• • 0 on success, negative error code on failure
• */

+int drm_dp_dual_mode_get_tmds_output(struct i2c_adapter *adapter, bool *enabled) +{

• uint8_t tmds_oen;
• ssize_t ret;
• ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_TMDS_OEN,

• 		    &tmds_oen, sizeof(tmds_oen));
  

• if (ret)

• return ret;
  

• *enabled = !(tmds_oen & DP_DUAL_MODE_TMDS_DISABLE);
• return 0;

+} +EXPORT_SYMBOL(drm_dp_dual_mode_get_tmds_output);

+/**

• • drm_dp_dual_mode_set_tmds_output - Enable/disable TMDS output

buffers in the DP dual mode adaptor

• • adapter: I2C adapter for the DDC bus
• • enable: enable (as opposed to disable) the TMDS output buffers
• • Set the state of the TMDS output buffers in the adaptor.
• • DP_DUAL_MODE_TMDS_OEN register is mandatory for type 2 adaptors,
• • optionals for type 1 adaptors. Hoever, as some type 1 adaptors

are

• • broken (see comments in drm_dp_dual_mode_detect() for the

details)

• • one probably shouldn't use this with type 1 adaptors at all.
• • Returns:
• • 0 on success, negative error code on failure
• */

+int drm_dp_dual_mode_set_tmds_output(struct i2c_adapter *adapter, bool enable) +{

• uint8_t tmds_oen = enable ? 0 : DP_DUAL_MODE_TMDS_DISABLE;
• ssize_t ret;
• ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN,

• 		     &tmds_oen, sizeof(tmds_oen));
  

• if (ret)

• return ret;
  

• return 0;

+} +EXPORT_SYMBOL(drm_dp_dual_mode_set_tmds_output);

+/**

• • drm_dp_get_dual_mode_type_name - Get the name of the DP dual mode

adaptor type as a string

• • type: DP dual mode adaptor type
• • Returns:
• • String representation of the DP dual mode adaptor type
• */

+const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type) +{

• switch (type) {
• case DRM_DP_DUAL_MODE_NONE:

• return "none";
  

• case DRM_DP_DUAL_MODE_TYPE1_DVI:

• return "type 1 DVI";
  

• case DRM_DP_DUAL_MODE_TYPE1_HDMI:

• return "type 1 HDMI";
  

• case DRM_DP_DUAL_MODE_TYPE2_DVI:

• return "type 2 DVI";
  

• case DRM_DP_DUAL_MODE_TYPE2_HDMI:

• return "type 2 HDMI";
  

• default:

• return "unknown";
  

Please at least WARN_ON(1)?

We don't generally warn in these case (in the drm core).

And, if you implement my suggestion of adding an actual unknown value to the enum, don't forget to change the default case to return something that's not "unknown".

Most of the things I wrote above are just ideas, not requirements, and having this patch as-is looks better than not having it, so if you at least fix the spelling errors:

Reviewed-by: Paulo Zanoni paulo.r.zanoni@intel.com

...

• };

+} +EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name); diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h new file mode 100644 index 000000000000..a2f6b4587f5f --- /dev/null +++ b/include/drm/drm_dp_dual_mode_helper.h @@ -0,0 +1,80 @@ +/*

• • Copyright © 2016 Intel Corporation
• • Permission is hereby granted, free of charge, to any person

obtaining a

• • copy of this software and associated documentation files (the

"Software"),

• • to deal in the Software without restriction, including without

limitation

• • the rights to use, copy, modify, merge, publish, distribute,

sublicense,

• • and/or sell copies of the Software, and to permit persons to whom

the

• • Software is furnished to do so, subject to the following

conditions:

• • The above copyright notice and this permission notice shall be

included in

• • all copies or substantial portions of the Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,

EXPRESS OR

• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF

MERCHANTABILITY,

• • FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO

EVENT SHALL

• • THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,

DAMAGES OR

• • OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR

OTHERWISE,

• • ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE

USE OR

• • OTHER DEALINGS IN THE SOFTWARE.
• */

+#ifndef DRM_DP_DUAL_MODE_HELPER_H +#define DRM_DP_DUAL_MODE_HELPER_H

+#include <linux/types.h>

+/*

• • Optional for type 1 DVI adaptors
• • Mandatory for type 1 HDMI and type 2 adators
• */

+#define DP_DUAL_MODE_HDMI_ID 0x00 /* 00-0f */ +#define  DP_DUAL_MODE_HDMI_ID_LEN 16 +/*

• • Optional for type 1 adaptors
• • Mandatory for type 2 adators
• */

+#define DP_DUAL_MODE_ADAPTOR_ID 0x10 +#define  DP_DUAL_MODE_REV_MASK 0x07 +#define  DP_DUAL_MODE_REV_TYPE2 0x00 +#define  DP_DUAL_MODE_TYPE_MASK 0xf0 +#define  DP_DUAL_MODE_TYPE_TYPE2 0xa0 +#define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/ +#define  DP_DUAL_IEEE_OUI_LEN 3 +#define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */ +#define  DP_DUAL_DEVICE_ID_LEN 6 +#define DP_DUAL_MODE_HARDWARE_REV 0x1a +#define DP_DUAL_MODE_FIRMWARE_MAJOR_REV 0x1b +#define DP_DUAL_MODE_FIRMWARE_MINOR_REV 0x1c +#define DP_DUAL_MODE_MAX_TMDS_CLOCK 0x1d +#define DP_DUAL_MODE_I2C_SPEED_CAP 0x1e +#define DP_DUAL_MODE_TMDS_OEN 0x20 +#define  DP_DUAL_MODE_TMDS_DISABLE 0x01 +#define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21 +#define  DP_DUAL_MODE_CEC_ENABLE 0x01 +#define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22 +#define DP_DUAL_MODE_LAST_RESERVED 0xff

+struct i2c_adapter;

+ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,

• 	      u8 offset, void *buffer, size_t size);
  

+ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,

• 	       u8 offset, const void *buffer, size_t
  

size);

+enum drm_dp_dual_mode_type {

• DRM_DP_DUAL_MODE_NONE,
• DRM_DP_DUAL_MODE_TYPE1_DVI,
• DRM_DP_DUAL_MODE_TYPE1_HDMI,
• DRM_DP_DUAL_MODE_TYPE2_DVI,
• DRM_DP_DUAL_MODE_TYPE2_HDMI,

+};

+enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter); +int drm_dp_dual_mode_max_tmds_clock(struct i2c_adapter *adapter); +int drm_dp_dual_mode_get_tmds_output(struct i2c_adapter *adapter, bool *enabled); +int drm_dp_dual_mode_set_tmds_output(struct i2c_adapter *adapter, bool enable); +const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);

+#endif