2015-04-15 12:38 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
> This patch adds 3 debugfs files for handling Displayport compliance testing
> and supercedes the previous patches that implemented debugfs support for
> compliance testing. Those patches were:
>
> - [PATCH 04/17] drm/i915: Add debugfs functions for Displayport
> compliance testing
> - [PATCH 08/17] drm/i915: Add new debugfs file for Displayport
> compliance test control
> - [PATCH 09/17] drm/i915: Add debugfs write and test param parsing
> functions for DP test control
>
> This new patch simplifies the debugfs implementation by places a single
> test control value into an individual file. Each file is readable by
> the usersapce application and the test_active file is writable to
> indicate to the kernel when userspace has completed its portion of the
> test sequence.
>
> Replacing the previous files simplifies operation and speeds response
> time for the user app, as it is required to poll on the test_active file
> in order to determine when it needs to begin its operations.
>
> V2:
> - Updated the test active variable name to match the change in
> the initial patch of the series
> V3:
> - Added a fix in the test_active_write function to prevent a NULL pointer
> dereference if the encoder on the connector is invalid
>
> Signed-off-by: Todd Previte <tprevite@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 209 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 209 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2394924..c33d390 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3937,6 +3937,212 @@ static const struct file_operations i915_display_crc_ctl_fops = {
> .write = display_crc_ctl_write
> };
>
> +static ssize_t i915_displayport_test_active_write(struct file *file,
> + const char __user *ubuf,
> + size_t len, loff_t *offp)
> +{
> + char *input_buffer;
> + int status = 0;
> + struct seq_file *m;
> + struct drm_device *dev;
> + struct drm_connector *connector;
> + struct list_head *connector_list;
> + struct intel_dp *intel_dp;
> + int val = 0;
> +
> + m = file->private_data;
> + if (!m) {
> + status = -ENODEV;
> + return status;
> + }
> + dev = m->private;
> +
> + if (!dev) {
> + status = -ENODEV;
> + return status;
> + }
> + connector_list = &dev->mode_config.connector_list;
> +
> + if (len == 0)
> + return 0;
> +
> + input_buffer = kmalloc(len + 1, GFP_KERNEL);
> + if (!input_buffer)
> + return -ENOMEM;
In 2 spots above you use "status = -ENODEV; return status;", but here
you just "return -ENOMEM". I'd be consistent, possibly using the
shorter form in the 3 cases.
> +
> + if (copy_from_user(input_buffer, ubuf, len)) {
> + status = -EFAULT;
> + goto out;
> + }
> +
> + input_buffer[len] = '\0';
> + DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
> +
> + list_for_each_entry(connector, connector_list, head) {
> +
> + if (connector->connector_type !=
> + DRM_MODE_CONNECTOR_DisplayPort)
> + continue;
> +
> + if (connector->connector_type ==
> + DRM_MODE_CONNECTOR_DisplayPort &&
> + connector->status == connector_status_connected &&
> + connector->encoder != NULL) {
> + intel_dp = enc_to_intel_dp(connector->encoder);
> + status = kstrtoint(input_buffer, 10, &val);
> + if (status < 0)
> + goto out;
> + DRM_DEBUG_DRIVER("Got %d for test active\n", val);
> + /* To prevent erroneous activation of the compliance
> + * testing code, only accept an actual value of 1 here
> + */
> + if (val == 1)
> + intel_dp->compliance_test_active = 1;
> + else
> + intel_dp->compliance_test_active = 0;
> + }
> + }
> +out:
> + kfree(input_buffer);
> + if (status < 0)
> + return status;
> +
> + *offp += len;
> + return len;
> +}
> +
> +static int i915_displayport_test_active_show(struct seq_file *m, void *data)
> +{
> + struct drm_device *dev = m->private;
> + struct drm_connector *connector;
> + struct list_head *connector_list = &dev->mode_config.connector_list;
> + struct intel_dp *intel_dp;
> +
> + if (!dev)
> + return -ENODEV;
> +
> + list_for_each_entry(connector, connector_list, head) {
> +
> + if (connector->connector_type !=
> + DRM_MODE_CONNECTOR_DisplayPort)
> + continue;
> +
> + if (connector->status == connector_status_connected &&
> + connector->encoder != NULL) {
> + intel_dp = enc_to_intel_dp(connector->encoder);
> + if (intel_dp->compliance_test_active)
> + seq_puts(m, "1");
> + else
> + seq_puts(m, "0");
> + } else
> + seq_puts(m, "0");
> + }
> +
> + return 0;
> +}
> +
> +static int i915_displayport_test_active_open(struct inode *inode,
> + struct file *file)
> +{
> + struct drm_device *dev = inode->i_private;
> +
> + return single_open(file, i915_displayport_test_active_show, dev);
> +}
> +
> +static const struct file_operations i915_displayport_test_active_fops = {
> + .owner = THIS_MODULE,
> + .open = i915_displayport_test_active_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> + .write = i915_displayport_test_active_write
> +};
> +
> +static int i915_displayport_test_data_show(struct seq_file *m, void *data)
> +{
> + struct drm_device *dev = m->private;
> + struct drm_connector *connector;
> + struct list_head *connector_list = &dev->mode_config.connector_list;
> + struct intel_dp *intel_dp;
> +
> + if (!dev)
> + return -ENODEV;
> +
> + list_for_each_entry(connector, connector_list, head) {
> +
> + if (connector->connector_type !=
> + DRM_MODE_CONNECTOR_DisplayPort)
> + continue;
> +
> + if (connector->status == connector_status_connected &&
> + connector->encoder != NULL) {
> + intel_dp = enc_to_intel_dp(connector->encoder);
> + seq_printf(m, "%lx", intel_dp->compliance_test_data);
> + } else
> + seq_puts(m, "0");
> + }
> +
> + return 0;
> +}
> +static int i915_displayport_test_data_open(struct inode *inode,
> + struct file *file)
> +{
> + struct drm_device *dev = inode->i_private;
> +
> + return single_open(file, i915_displayport_test_data_show, dev);
> +}
> +
> +static const struct file_operations i915_displayport_test_data_fops = {
> + .owner = THIS_MODULE,
> + .open = i915_displayport_test_data_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release
> +};
> +
> +static int i915_displayport_test_type_show(struct seq_file *m, void *data)
> +{
> + struct drm_device *dev = m->private;
> + struct drm_connector *connector;
> + struct list_head *connector_list = &dev->mode_config.connector_list;
> + struct intel_dp *intel_dp;
> +
> + if (!dev)
> + return -ENODEV;
> +
> + list_for_each_entry(connector, connector_list, head) {
> +
> + if (connector->connector_type !=
> + DRM_MODE_CONNECTOR_DisplayPort)
> + continue;
> +
> + if (connector->status == connector_status_connected &&
> + connector->encoder != NULL) {
> + intel_dp = enc_to_intel_dp(connector->encoder);
> + seq_printf(m, "%02lx", intel_dp->compliance_test_type);
> + } else
> + seq_puts(m, "0");
> + }
> +
> + return 0;
> +}
> +
> +static int i915_displayport_test_type_open(struct inode *inode,
> + struct file *file)
> +{
> + struct drm_device *dev = inode->i_private;
> +
> + return single_open(file, i915_displayport_test_type_show, dev);
> +}
> +
> +static const struct file_operations i915_displayport_test_type_fops = {
> + .owner = THIS_MODULE,
> + .open = i915_displayport_test_type_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release
> +};
> +
> static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> {
> struct drm_device *dev = m->private;
> @@ -4832,6 +5038,9 @@ static const struct i915_debugfs_files {
> {"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
> {"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
> {"i915_fbc_false_color", &i915_fbc_fc_fops},
> + {"i915_dp_test_data", &i915_displayport_test_data_fops},
> + {"i915_dp_test_type", &i915_displayport_test_type_fops},
Since both test_data and test_type are simpler, you can put them on
i915_debugfs_list instead of i915_debugfs_files. This will allow the
removal of the 2 _open functions and the 2 _fops structs, making your
patch ~30 lines smaller.
Since the stuff above is not required for the files to work:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> + {"i915_dp_test_active", &i915_displayport_test_active_fops}
> };
>
> void intel_display_crc_init(struct drm_device *dev)
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
