Re: [Freedreno] [PATCH] drm/msm/dp: Use the connector passed to dp_debug_get()

From: abhinavk
Date: Tue Oct 12 2021 - 19:45:01 EST

Next message: Bart Van Assche: "Re: [PATCH v2 2/3] acornscsi: remove tagged queuing vestiges"
Previous message: Stephen Rothwell: "linux-next: manual merge of the net-next tree with the net tree"
In reply to: Bjorn Andersson: "Re: [PATCH] drm/msm/dp: Use the connector passed to dp_debug_get()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2021-10-12 16:38, Bjorn Andersson wrote:

On Tue 12 Oct 16:03 PDT 2021, abhinavk@xxxxxxxxxxxxxx wrote:

On 2021-10-09 20:04, Bjorn Andersson wrote:
> The debugfs code is provided an array of a single drm_connector. Then to
> access the connector, the list of all connectors of the DRM device is
> traversed and all non-DisplayPort connectors are skipped, to find the
> one and only DisplayPort connector.
>
> But as we move to support multiple DisplayPort controllers this will now
> find multiple connectors and has no way to distinguish them.
>
> Pass the single connector to dp_debug_get() and use this in the debugfs
> functions instead, both to simplify the code and the support the
> multiple instances.
>
Change itself is fine, hence
Reviewed-by: Abhinav Kumar <abhinavk@xxxxxxxxxxxxxx>

Thanks.

What has to be checked now is now to create multiple DP nodes for multi-DP
cases.
Today, the debug node will be created only once :

https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L206

This also needs to be expanded for multi-DP to make the solution complete.

In my multi-DP support patch I end up invoking msm_dp_debugfs_init() for
each of the DP/eDP instances and in its current form the first one gets
registered and any others will fail because of the resulting name
collision.

This patch came as a byproduct of the effort of addressing that problem.

Regards,
Bjorn

Ah, okay. Yes i see the part you are referring to in https://patchwork.freedesktop.org/patch/457667/:

@@ -203,8 +204,10 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
dpu_debugfs_vbif_init(dpu_kms, entry);
dpu_debugfs_core_irq_init(dpu_kms, entry);

- if (priv->dp)
- msm_dp_debugfs_init(priv->dp, minor);
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (priv->dp[i])
+ msm_dp_debugfs_init(priv->dp[i], minor);
+ }

That clarifies it. Thanks.

> Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/dp/dp_debug.c | 131 ++++++++++------------------
> drivers/gpu/drm/msm/dp/dp_debug.h | 2 +-
> drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
> 3 files changed, 46 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
> b/drivers/gpu/drm/msm/dp/dp_debug.c
> index af709d93bb9f..da4323556ef3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_debug.c
> +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> @@ -24,7 +24,7 @@ struct dp_debug_private {
> struct dp_usbpd *usbpd;
> struct dp_link *link;
> struct dp_panel *panel;
> - struct drm_connector **connector;
> + struct drm_connector *connector;
> struct device *dev;
> struct drm_device *drm_dev;
>
> @@ -97,59 +97,35 @@ DEFINE_SHOW_ATTRIBUTE(dp_debug);
>
> static int dp_test_data_show(struct seq_file *m, void *data)
> {
> - struct drm_device *dev;
> - struct dp_debug_private *debug;
> - struct drm_connector *connector;
> - struct drm_connector_list_iter conn_iter;
> + const struct dp_debug_private *debug = m->private;
> + const struct drm_connector *connector = debug->connector;
> u32 bpc;
>
> - debug = m->private;
> - dev = debug->drm_dev;
> - drm_connector_list_iter_begin(dev, &conn_iter);
> - drm_for_each_connector_iter(connector, &conn_iter) {
> -
> - if (connector->connector_type !=
> - DRM_MODE_CONNECTOR_DisplayPort)
> - continue;
> -
> - if (connector->status == connector_status_connected) {
> - bpc = debug->link->test_video.test_bit_depth;
> - seq_printf(m, "hdisplay: %d\n",
> - debug->link->test_video.test_h_width);
> - seq_printf(m, "vdisplay: %d\n",
> - debug->link->test_video.test_v_height);
> - seq_printf(m, "bpc: %u\n",
> - dp_link_bit_depth_to_bpc(bpc));
> - } else
> - seq_puts(m, "0");
> + if (connector->status == connector_status_connected) {
> + bpc = debug->link->test_video.test_bit_depth;
> + seq_printf(m, "hdisplay: %d\n",
> + debug->link->test_video.test_h_width);
> + seq_printf(m, "vdisplay: %d\n",
> + debug->link->test_video.test_v_height);
> + seq_printf(m, "bpc: %u\n",
> + dp_link_bit_depth_to_bpc(bpc));
> + } else {
> + seq_puts(m, "0");
> }
>
> - drm_connector_list_iter_end(&conn_iter);
> -
> return 0;
> }
> DEFINE_SHOW_ATTRIBUTE(dp_test_data);
>
> static int dp_test_type_show(struct seq_file *m, void *data)
> {
> - struct dp_debug_private *debug = m->private;
> - struct drm_device *dev = debug->drm_dev;
> - struct drm_connector *connector;
> - struct drm_connector_list_iter conn_iter;
> -
> - drm_connector_list_iter_begin(dev, &conn_iter);
> - drm_for_each_connector_iter(connector, &conn_iter) {
> -
> - if (connector->connector_type !=
> - DRM_MODE_CONNECTOR_DisplayPort)
> - continue;
> + const struct dp_debug_private *debug = m->private;
> + const struct drm_connector *connector = debug->connector;
>
> - if (connector->status == connector_status_connected)
> - seq_printf(m, "%02x", DP_TEST_LINK_VIDEO_PATTERN);
> - else
> - seq_puts(m, "0");
> - }
> - drm_connector_list_iter_end(&conn_iter);
> + if (connector->status == connector_status_connected)
> + seq_printf(m, "%02x", DP_TEST_LINK_VIDEO_PATTERN);
> + else
> + seq_puts(m, "0");
>
> return 0;
> }
> @@ -161,14 +137,12 @@ static ssize_t dp_test_active_write(struct file
> *file,
> {
> char *input_buffer;
> int status = 0;
> - struct dp_debug_private *debug;
> - struct drm_device *dev;
> - struct drm_connector *connector;
> - struct drm_connector_list_iter conn_iter;
> + const struct dp_debug_private *debug;
> + const struct drm_connector *connector;
> int val = 0;
>
> debug = ((struct seq_file *)file->private_data)->private;
> - dev = debug->drm_dev;
> + connector = debug->connector;
>
> if (len == 0)
> return 0;
> @@ -179,30 +153,22 @@ static ssize_t dp_test_active_write(struct file
> *file,
>
> DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
>
> - drm_connector_list_iter_begin(dev, &conn_iter);
> - drm_for_each_connector_iter(connector, &conn_iter) {
> - if (connector->connector_type !=
> - DRM_MODE_CONNECTOR_DisplayPort)
> - continue;
> -
> - if (connector->status == connector_status_connected) {
> - status = kstrtoint(input_buffer, 10, &val);
> - if (status < 0)
> - break;
> - 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)
> - debug->panel->video_test = true;
> - else
> - debug->panel->video_test = false;
> + if (connector->status == connector_status_connected) {
> + status = kstrtoint(input_buffer, 10, &val);
> + if (status < 0) {
> + kfree(input_buffer);
> + return status;
> }
> + 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)
> + debug->panel->video_test = true;
> + else
> + debug->panel->video_test = false;
> }
> - drm_connector_list_iter_end(&conn_iter);
> kfree(input_buffer);
> - if (status < 0)
> - return status;
>
> *offp += len;
> return len;
> @@ -211,25 +177,16 @@ static ssize_t dp_test_active_write(struct file
> *file,
> static int dp_test_active_show(struct seq_file *m, void *data)
> {
> struct dp_debug_private *debug = m->private;
> - struct drm_device *dev = debug->drm_dev;
> - struct drm_connector *connector;
> - struct drm_connector_list_iter conn_iter;
> -
> - drm_connector_list_iter_begin(dev, &conn_iter);
> - drm_for_each_connector_iter(connector, &conn_iter) {
> - if (connector->connector_type !=
> - DRM_MODE_CONNECTOR_DisplayPort)
> - continue;
> -
> - if (connector->status == connector_status_connected) {
> - if (debug->panel->video_test)
> - seq_puts(m, "1");
> - else
> - seq_puts(m, "0");
> - } else
> + struct drm_connector *connector = debug->connector;
> +
> + if (connector->status == connector_status_connected) {
> + if (debug->panel->video_test)
> + seq_puts(m, "1");
> + else
> seq_puts(m, "0");
> + } else {
> + seq_puts(m, "0");
> }
> - drm_connector_list_iter_end(&conn_iter);
>
> return 0;
> }
> @@ -278,7 +235,7 @@ static int dp_debug_init(struct dp_debug
> *dp_debug, struct drm_minor *minor)
>
> struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel
> *panel,
> struct dp_usbpd *usbpd, struct dp_link *link,
> - struct drm_connector **connector, struct drm_minor *minor)
> + struct drm_connector *connector, struct drm_minor *minor)
> {
> int rc = 0;
> struct dp_debug_private *debug;
> diff --git a/drivers/gpu/drm/msm/dp/dp_debug.h
> b/drivers/gpu/drm/msm/dp/dp_debug.h
> index 7eaedfbb149c..3f90acfffc5a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_debug.h
> +++ b/drivers/gpu/drm/msm/dp/dp_debug.h
> @@ -43,7 +43,7 @@ struct dp_debug {
> */
> struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel
> *panel,
> struct dp_usbpd *usbpd, struct dp_link *link,
> - struct drm_connector **connector,
> + struct drm_connector *connector,
> struct drm_minor *minor);
>
> /**
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 1708b7cdc1b3..41a6f58916e6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1464,7 +1464,7 @@ void msm_dp_debugfs_init(struct msm_dp
> *dp_display, struct drm_minor *minor)
> dev = &dp->pdev->dev;
>
> dp->debug = dp_debug_get(dev, dp->panel, dp->usbpd,
> - dp->link, &dp->dp_display.connector,
> + dp->link, dp->dp_display.connector,
> minor);
> if (IS_ERR(dp->debug)) {
> rc = PTR_ERR(dp->debug);

Next message: Bart Van Assche: "Re: [PATCH v2 2/3] acornscsi: remove tagged queuing vestiges"
Previous message: Stephen Rothwell: "linux-next: manual merge of the net-next tree with the net tree"
In reply to: Bjorn Andersson: "Re: [PATCH] drm/msm/dp: Use the connector passed to dp_debug_get()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]