| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YWYcXxZzjU8gLNf5@ripper>
Date: Tue, 12 Oct 2021 16:38:07 -0700
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: abhinavk@...eaurora.org
Cc: Rob Clark <robdclark@...il.com>,
Stephen Boyd <swboyd@...omium.org>,
Kuogee Hsieh <khsieh@...eaurora.org>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>, linux-arm-msm@...r.kernel.org,
dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/msm/dp: Use the connector passed to dp_debug_get()
On Tue 12 Oct 16:03 PDT 2021, abhinavk@...eaurora.org 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@...eaurora.org>
>
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
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> > ---
> > 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);
Powered by blists - more mailing lists