[Intel-gfx] [PATCH v2] drm/i915: add i915 specific connector debugfs file for DPCD

Wed Apr 1 05:42:44 PDT 2015

>-----Original Message-----
>From: Nikula, Jani
>Sent: Wednesday, April 1, 2015 6:05 PM
>To: R, Durgadoss; intel-gfx at lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: add i915 specific connector debugfs file for DPCD
>
>On Wed, 01 Apr 2015, "R, Durgadoss" <durgadoss.r at intel.com> wrote:
>> Hi Jani,
>>
>>>-----Original Message-----
>>>From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Jani Nikula
>>>Sent: Wednesday, April 1, 2015 1:45 PM
>>>To: intel-gfx at lists.freedesktop.org
>>>Cc: Nikula, Jani
>>>Subject: [Intel-gfx] [PATCH v2] drm/i915: add i915 specific connector debugfs file for DPCD
>>>
>>>Occasionally it would be interesting to read some of the DPCD registers
>>>for debug purposes, without having to resort to logging. Add an i915
>>>specific i915_dpcd debugfs file for DP and eDP connectors to dump parts
>>>of the DPCD. Currently the DPCD addresses to be dumped are statically
>>
>> This is very much needed. I personally use a sysfs variation of something like this.
>> But I think we will never have a list that is sufficient for all cases of debug.
>>
>> So, why not have one interface which takes the 'addr' and another
>> one which returns the 'value' of the DPCD register 'addr'.
>
>My main use case is for gathering debugging information from
>end-users. I would like to have a simple and fairly fool proof way of
>gathering the most relevant information in a dump like this. And we can
>extend on it as needed.

Ok, agreed.

>
>I don't think what you propose is user friendly (although it is powerful
>for developers), and pretty much requires a script to ensure we do get
>the data we want.
>
>However these two are not mutually exclusive... this is the patch to do
>what I want, please send us the patch doing what you want! :)

Sure...

Thanks,
Durga

>
>BR,
>Jani.
>
>
>PS. Would we need to whitelist/blacklist the registers for your
>approach?
>
>
>> Something like below:
>>
>> i915_dpcd_addr
>> i915_dpcd_val
>>
>> And we do:
>> echo 0x100 > i915_dpcd_addr
>>
>> and then:
>> cat i915_dpcd_val
>> [This fetches and prints the value of DPCD 0x100]
>>
>> Thanks,
>> Durga
>>
>> ..
>>>configured, and more can be added trivially.
>>>
>>>The implementation also makes it relatively easy to add other i915 and
>>>connector specific debugfs files in the future, as necessary.
>>>
>>>This is currently i915 specific just because there's no generic way to
>>>do AUX transactions given just a drm_connector. However it's all pretty
>>>straightforward to port to other drivers.
>>>
>>>v2: Add more DPCD registers to dump.
>>>
>>>Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>>>---
>>> drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/i915/i915_drv.h     |  1 +
>>> drivers/gpu/drm/i915/intel_dp.c     |  2 +
>>> 3 files changed, 99 insertions(+)
>>>
>>>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>>index 007c7d7d8295..7164aa7d5fd3 100644
>>>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>@@ -4780,3 +4780,99 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
>>> 		drm_debugfs_remove_files(info_list, 1, minor);
>>> 	}
>>> }
>>>+
>>>+struct dpcd_block {
>>>+	/* DPCD dump start address. */
>>>+	unsigned int offset;
>>>+	/* DPCD dump end address, inclusive. If unset, .size will be used. */
>>>+	unsigned int end;
>>>+	/* DPCD dump size. Used if .end is unset. If unset, defaults to 1. */
>>>+	size_t size;
>>>+	/* Only valid for eDP. */
>>>+	bool edp;
>>>+};
>>>+
>>>+static const struct dpcd_block i915_dpcd_debug[] = {
>>>+	{ .offset = DP_DPCD_REV, .size = DP_RECEIVER_CAP_SIZE },
>>>+	{ .offset = DP_PSR_SUPPORT, .end = DP_PSR_CAPS },
>>>+	{ .offset = DP_DOWNSTREAM_PORT_0, .size = 16 },
>>>+	{ .offset = DP_LINK_BW_SET, .end = DP_EDP_CONFIGURATION_SET },
>>>+	{ .offset = DP_SINK_COUNT, .end = DP_ADJUST_REQUEST_LANE2_3 },
>>>+	{ .offset = DP_SET_POWER },
>>>+	{ .offset = DP_EDP_DPCD_REV },
>>>+	{ .offset = DP_EDP_GENERAL_CAP_1, .end = DP_EDP_GENERAL_CAP_3 },
>>>+	{ .offset = DP_EDP_DISPLAY_CONTROL_REGISTER, .end =
>>>DP_EDP_BACKLIGHT_FREQ_CAP_MAX_LSB },
>>>+	{ .offset = DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET, .end =
>>>DP_EDP_DBC_MAXIMUM_BRIGHTNESS_SET },
>>>+};
>>>+
>>>+static int i915_dpcd_show(struct seq_file *m, void *data)
>>>+{
>>>+	struct drm_connector *connector = m->private;
>>>+	struct intel_dp *intel_dp =
>>>+		enc_to_intel_dp(&intel_attached_encoder(connector)->base);
>>>+	uint8_t buf[16];
>>>+	ssize_t err;
>>>+	int i;
>>>+
>>>+	for (i = 0; i < ARRAY_SIZE(i915_dpcd_debug); i++) {
>>>+		const struct dpcd_block *b = &i915_dpcd_debug[i];
>>>+		size_t size = b->end ? b->end - b->offset + 1 : (b->size ?: 1);
>>>+
>>>+		if (b->edp &&
>>>+		    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
>>>+			continue;
>>>+
>>>+		/* low tech for now */
>>>+		if (WARN_ON(size > sizeof(buf)))
>>>+			continue;
>>>+
>>>+		err = drm_dp_dpcd_read(&intel_dp->aux, b->offset, buf, size);
>>>+		if (err <= 0) {
>>>+			DRM_ERROR("dpcd read (%zu bytes at %u) failed (%zd)\n",
>>>+				  size, b->offset, err);
>>>+			continue;
>>>+		}
>>>+
>>>+		seq_printf(m, "%04x: %*ph\n", b->offset, (int) size, buf);
>>>+	};
>>>+
>>>+	return 0;
>>>+}
>>>+
>>>+static int i915_dpcd_open(struct inode *inode, struct file *file)
>>>+{
>>>+	return single_open(file, i915_dpcd_show, inode->i_private);
>>>+}
>>>+
>>>+static const struct file_operations i915_dpcd_fops = {
>>>+	.owner = THIS_MODULE,
>>>+	.open = i915_dpcd_open,
>>>+	.read = seq_read,
>>>+	.llseek = seq_lseek,
>>>+	.release = single_release,
>>>+};
>>>+
>>>+/**
>>>+ * i915_debugfs_connector_add - add i915 specific connector debugfs files
>>>+ * @connector: pointer to a registered drm_connector
>>>+ *
>>>+ * Cleanup will be done by drm_connector_unregister() through a call to
>>>+ * drm_debugfs_connector_remove().
>>>+ *
>>>+ * Returns 0 on success, negative error codes on error.
>>>+ */
>>>+int i915_debugfs_connector_add(struct drm_connector *connector)
>>>+{
>>>+	struct dentry *root = connector->debugfs_entry;
>>>+
>>>+	/* The connector must have been registered beforehands. */
>>>+	if (!root)
>>>+		return -ENODEV;
>>>+
>>>+	if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
>>>+	    connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>>>+		debugfs_create_file("i915_dpcd", S_IRUGO, root, connector,
>>>+				    &i915_dpcd_fops);
>>>+
>>>+	return 0;
>>>+}
>>>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>index aadada5a3fe9..868f26780115 100644
>>>--- a/drivers/gpu/drm/i915/i915_drv.h
>>>+++ b/drivers/gpu/drm/i915/i915_drv.h
>>>@@ -3011,6 +3011,7 @@ int i915_verify_lists(struct drm_device *dev);
>>> /* i915_debugfs.c */
>>> int i915_debugfs_init(struct drm_minor *minor);
>>> void i915_debugfs_cleanup(struct drm_minor *minor);
>>>+int i915_debugfs_connector_add(struct drm_connector *connector);
>>> #ifdef CONFIG_DEBUG_FS
>>> void intel_display_crc_init(struct drm_device *dev);
>>> #else
>>>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>index 7936155acbe8..f91ce9e94aa6 100644
>>>--- a/drivers/gpu/drm/i915/intel_dp.c
>>>+++ b/drivers/gpu/drm/i915/intel_dp.c
>>>@@ -5569,6 +5569,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>> 		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
>>> 	}
>>>
>>>+	i915_debugfs_connector_add(connector);
>>>+
>>> 	return true;
>>> }
>>>
>>>--
>>>2.1.4
>>>
>>>_______________________________________________
>>>Intel-gfx mailing list
>>>Intel-gfx at lists.freedesktop.org
>>>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Jani Nikula, Intel Open Source Technology Center