Re: [Intel-gfx] [PATCH v4 5/5] drm/msm/dp: Implement hpd_notify()
On Mon 02 May 15:49 PDT 2022, Kuogee Hsieh wrote:
>
> On 5/2/2022 3:29 PM, Bjorn Andersson wrote:
> > On Mon 02 May 13:59 PDT 2022, Kuogee Hsieh wrote:
> >
> > > On 5/2/2022 9:53 AM, Bjorn Andersson wrote:
> > > > The Qualcomm DisplayPort driver contains traces of the necessary
> > > > plumbing to hook up USB HPD, in the form of the dp_hpd module and the
> > > > dp_usbpd_cb struct. Use this as basis for implementing the
> > > > hpd_notify() callback, by amending the dp_hpd module with the
> > > > missing logic.
> > > >
> > > > Overall the solution is similar to what's done downstream, but upstream
> > > > all the code to disect the HPD notification lives on the calling side of
> > > > drm_connector_oob_hotplug_event().
> > > >
> > > > drm_connector_oob_hotplug_event() performs the lookup of the
> > > > drm_connector based on fwnode, hence the need to assign the fwnode in
> > > > dp_drm_connector_init().
> > > >
> > > > Signed-off-by: Bjorn Andersson
> > > > ---
> > > >
> > > > Changes since v3:
> > > > - Implements hpd_notify instead of oob_hotplug_event
> > > > - Rebased on new cleanup patch from Dmitry
> > > > - Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure()
> > > > succeeds
> > > >
> > > >drivers/gpu/drm/msm/dp/dp_display.c | 26 ++
> > > >drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> > > >drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++
> > > >drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++
> > > >4 files changed, 32 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > index b447446d75e9..080294ac6144 100644
> > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > @@ -83,6 +83,8 @@ struct dp_display_private {
> > > > bool hpd_irq_on;
> > > > bool audio_supported;
> > > > + bool connected;
> > > > +
> > > > struct drm_device *drm_dev;
> > > > struct platform_device *pdev;
> > > > struct dentry *root;
> > > > @@ -1271,6 +1273,7 @@ static int dp_display_probe(struct
> > > > platform_device *pdev)
> > > > if (!desc)
> > > > return -EINVAL;
> > > > + dp->dp_display.dev = >dev;
> > > > dp->pdev = pdev;
> > > > dp->name = "drm_dp";
> > > > dp->dp_display.connector_type = desc->connector_type;
> > > > @@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge
> > > > *drm_bridge,
> > > > dp_display->dp_mode.h_active_low =
> > > > !!(dp_display->dp_mode.drm_mode.flags &
> > > > DRM_MODE_FLAG_NHSYNC);
> > > >}
> > > > +
> > > > +void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> > > > + enum drm_connector_status status)
> > > > +{
> > > > + struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> > > > + struct msm_dp *dp = dp_bridge->dp_display;
> > > > + struct dp_display_private *dp_display = container_of(dp, struct
> > > > dp_display_private, dp_display);
> > > > + int ret;
> > > > +
> > > > + drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n",
> > > > status, dp_display->connected);
> > > > +
> > > > + if (!dp_display->connected && status ==
> > > > connector_status_connected) {
> > > > + dp_display->connected = true;
> > > > + ret = dp_display_usbpd_configure(dp_display);
> > > > + if (!ret)
> > > > + dp_display->hpd_state = ST_MAINLINK_READY;
> > > > + } else if (status != connector_status_connected) {
> > > > + dp_display->connected = false;
> > > > + dp_display_notify_disconnect(dp_display);
> > > > + } else {
> > > > + dp_display_usbpd_attention(dp_display);
> > > > + }
> > > > +}
> > > I would ass
[Intel-gfx] [PATCH v4 5/5] drm/msm/dp: Implement hpd_notify()
The Qualcomm DisplayPort driver contains traces of the necessary
plumbing to hook up USB HPD, in the form of the dp_hpd module and the
dp_usbpd_cb struct. Use this as basis for implementing the
hpd_notify() callback, by amending the dp_hpd module with the
missing logic.
Overall the solution is similar to what's done downstream, but upstream
all the code to disect the HPD notification lives on the calling side of
drm_connector_oob_hotplug_event().
drm_connector_oob_hotplug_event() performs the lookup of the
drm_connector based on fwnode, hence the need to assign the fwnode in
dp_drm_connector_init().
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- Implements hpd_notify instead of oob_hotplug_event
- Rebased on new cleanup patch from Dmitry
- Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure() succeeds
drivers/gpu/drm/msm/dp/dp_display.c | 26 ++
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++
drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++
4 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index b447446d75e9..080294ac6144 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -83,6 +83,8 @@ struct dp_display_private {
bool hpd_irq_on;
bool audio_supported;
+ bool connected;
+
struct drm_device *drm_dev;
struct platform_device *pdev;
struct dentry *root;
@@ -1271,6 +1273,7 @@ static int dp_display_probe(struct platform_device *pdev)
if (!desc)
return -EINVAL;
+ dp->dp_display.dev = >dev;
dp->pdev = pdev;
dp->name = "drm_dp";
dp->dp_display.connector_type = desc->connector_type;
@@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
dp_display->dp_mode.h_active_low =
!!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
}
+
+void dp_bridge_hpd_notify(struct drm_bridge *bridge,
+ enum drm_connector_status status)
+{
+ struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
+ struct msm_dp *dp = dp_bridge->dp_display;
+ struct dp_display_private *dp_display = container_of(dp, struct
dp_display_private, dp_display);
+ int ret;
+
+ drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n", status,
dp_display->connected);
+
+ if (!dp_display->connected && status == connector_status_connected) {
+ dp_display->connected = true;
+ ret = dp_display_usbpd_configure(dp_display);
+ if (!ret)
+ dp_display->hpd_state = ST_MAINLINK_READY;
+ } else if (status != connector_status_connected) {
+ dp_display->connected = false;
+ dp_display_notify_disconnect(dp_display);
+ } else {
+ dp_display_usbpd_attention(dp_display);
+ }
+}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index 4f9fe4d7610b..2d2614bc5a14 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -11,6 +11,7 @@
#include "disp/msm_disp_snapshot.h"
struct msm_dp {
+ struct device *dev;
struct drm_device *drm_dev;
struct device *codec_dev;
struct drm_bridge *bridge;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 62d58b9c4647..821cfd37b1fb 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -68,6 +68,7 @@ static const struct drm_bridge_funcs dp_bridge_ops = {
.mode_valid = dp_bridge_mode_valid,
.get_modes= dp_bridge_get_modes,
.detect = dp_bridge_detect,
+ .hpd_notify = dp_bridge_hpd_notify,
};
struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device
*dev,
@@ -138,6 +139,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display)
if (IS_ERR(connector))
return connector;
+ connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev));
+
drm_connector_attach_encoder(connector, dp_display->encoder);
return connector;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index f4b1ed1e24f7..3b7480a86844 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -32,5 +32,7 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge
*bridge,
void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
const struct drm_display_mode *mode,
const struct drm_display_mode *adjusted_mode);
+void dp_bridge_hpd_notify(struct drm_bridge *bridge,
+ enum drm_connector_status status);
#endif /* _DP_DRM_H_ */
--
2.35.1
[Intel-gfx] [PATCH v4 1/5] drm/bridge_connector: stop filtering events in drm_bridge_connector_hpd_cb()
From: Dmitry Baryshkov In some cases the bridge drivers would like to receive hotplug events even in the case new status is equal to the old status. In the DP case this is used to deliver "attention" messages to the DP host. Stop filtering the events in the drm_bridge_connector_hpd_cb() and let drivers decide whether they would like to receive the event or not. Signed-off-by: Dmitry Baryshkov Signed-off-by: Bjorn Andersson --- Changes since v3: - New patch, needed due to the move to drm_bridge_connector drivers/gpu/drm/drm_bridge_connector.c | 5 - 1 file changed, 5 deletions(-) diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c index 6b3dad03d77d..0f6f3f653f65 100644 --- a/drivers/gpu/drm/drm_bridge_connector.c +++ b/drivers/gpu/drm/drm_bridge_connector.c @@ -113,16 +113,11 @@ static void drm_bridge_connector_hpd_cb(void *cb_data, struct drm_bridge_connector *drm_bridge_connector = cb_data; struct drm_connector *connector = _bridge_connector->base; struct drm_device *dev = connector->dev; - enum drm_connector_status old_status; mutex_lock(>mode_config.mutex); - old_status = connector->status; connector->status = status; mutex_unlock(>mode_config.mutex); - if (old_status == status) - return; - drm_bridge_connector_hpd_notify(connector, status); drm_kms_helper_hotplug_event(dev); -- 2.35.1
Re: [Intel-gfx] [PATCH v4 5/5] drm/msm/dp: Implement hpd_notify()
On Mon 02 May 15:29 PDT 2022, Bjorn Andersson wrote:
> On Mon 02 May 13:59 PDT 2022, Kuogee Hsieh wrote:
>
> >
> > On 5/2/2022 9:53 AM, Bjorn Andersson wrote:
> > > The Qualcomm DisplayPort driver contains traces of the necessary
> > > plumbing to hook up USB HPD, in the form of the dp_hpd module and the
> > > dp_usbpd_cb struct. Use this as basis for implementing the
> > > hpd_notify() callback, by amending the dp_hpd module with the
> > > missing logic.
> > >
> > > Overall the solution is similar to what's done downstream, but upstream
> > > all the code to disect the HPD notification lives on the calling side of
> > > drm_connector_oob_hotplug_event().
> > >
> > > drm_connector_oob_hotplug_event() performs the lookup of the
> > > drm_connector based on fwnode, hence the need to assign the fwnode in
> > > dp_drm_connector_init().
> > >
> > > Signed-off-by: Bjorn Andersson
> > > ---
> > >
> > > Changes since v3:
> > > - Implements hpd_notify instead of oob_hotplug_event
> > > - Rebased on new cleanup patch from Dmitry
> > > - Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure()
> > > succeeds
> > >
> > > drivers/gpu/drm/msm/dp/dp_display.c | 26 ++
> > > drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> > > drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++
> > > drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++
> > > 4 files changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index b447446d75e9..080294ac6144 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -83,6 +83,8 @@ struct dp_display_private {
> > > bool hpd_irq_on;
> > > bool audio_supported;
> > > + bool connected;
> > > +
> > > struct drm_device *drm_dev;
> > > struct platform_device *pdev;
> > > struct dentry *root;
> > > @@ -1271,6 +1273,7 @@ static int dp_display_probe(struct platform_device
> > > *pdev)
> > > if (!desc)
> > > return -EINVAL;
> > > + dp->dp_display.dev = >dev;
> > > dp->pdev = pdev;
> > > dp->name = "drm_dp";
> > > dp->dp_display.connector_type = desc->connector_type;
> > > @@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge
> > > *drm_bridge,
> > > dp_display->dp_mode.h_active_low =
> > > !!(dp_display->dp_mode.drm_mode.flags &
> > > DRM_MODE_FLAG_NHSYNC);
> > > }
> > > +
> > > +void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> > > + enum drm_connector_status status)
> > > +{
> > > + struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> > > + struct msm_dp *dp = dp_bridge->dp_display;
> > > + struct dp_display_private *dp_display = container_of(dp, struct
> > > dp_display_private, dp_display);
> > > + int ret;
> > > +
> > > + drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n", status,
> > > dp_display->connected);
> > > +
> > > + if (!dp_display->connected && status == connector_status_connected) {
> > > + dp_display->connected = true;
> > > + ret = dp_display_usbpd_configure(dp_display);
> > > + if (!ret)
> > > + dp_display->hpd_state = ST_MAINLINK_READY;
> > > + } else if (status != connector_status_connected) {
> > > + dp_display->connected = false;
> > > + dp_display_notify_disconnect(dp_display);
> > > + } else {
> > > + dp_display_usbpd_attention(dp_display);
> > > + }
> > > +}
> >
> > I would assume dp_bridge_hpd_notify() will server same purpose as
> > dp_display_irq_handler() if hpd_notification is enabled.
> >
>
> I agree with this statement.
>
> > In that case, should dp_bridge_hpd_notify() add
> > EV_HPD_PLUG_INT/EV_IRQ_HPD_INT/EV_HPD_UNPLUG_INT
> >
>
> I tried this originally, but couldn't get it to work and expected that
> as the downstream driver doesn't do this, there was some good reason for
> me not to do it either.
>
> > into event q to kick off corresponding
> > dp_hpd_p
[Intel-gfx] [PATCH v4 3/5] drm/bridge_connector: implement oob_hotplug_event
From: Dmitry Baryshkov
Implement the oob_hotplug_event() callback. Translate it to the HPD
notification sent to the HPD bridge in the chain.
Signed-off-by: Dmitry Baryshkov
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- New patch
drivers/gpu/drm/drm_bridge_connector.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge_connector.c
b/drivers/gpu/drm/drm_bridge_connector.c
index 0f6f3f653f65..6a0a6b14360a 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -123,6 +123,17 @@ static void drm_bridge_connector_hpd_cb(void *cb_data,
drm_kms_helper_hotplug_event(dev);
}
+static void drm_bridge_connector_oob_hotplug_event(struct drm_connector
*connector,
+ enum drm_connector_status
status)
+{
+ struct drm_bridge_connector *bridge_connector =
+ to_drm_bridge_connector(connector);
+ struct drm_bridge *hpd = bridge_connector->bridge_hpd;
+
+ if (hpd)
+ drm_bridge_hpd_notify(hpd, status);
+}
+
/**
* drm_bridge_connector_enable_hpd - Enable hot-plug detection for the
connector
* @connector: The DRM bridge connector
@@ -233,6 +244,7 @@ static const struct drm_connector_funcs
drm_bridge_connector_funcs = {
.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
.debugfs_init = drm_bridge_connector_debugfs_init,
+ .oob_hotplug_event = drm_bridge_connector_oob_hotplug_event,
};
/*
-
--
2.35.1
[Intel-gfx] [PATCH v4 4/5] drm/msm/dp: remove most of usbpd-related remains
From: Dmitry Baryshkov
Remove most of remains of downstream usbpd code. Mainline kernel uses
different approach for managing Type-C / USB-PD, so this remains unused.
Do not touch usbpd callbacks for now, since they look useful enough as
an example of how to handle connect/disconnect (to be rewritten into .
Signed-off-by: Dmitry Baryshkov
[bjorn: Cleaned up dp_display_usbpd_attention() prototype as well]
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- New patch
drivers/gpu/drm/msm/Makefile| 1 -
drivers/gpu/drm/msm/dp/dp_ctrl.h| 1 -
drivers/gpu/drm/msm/dp/dp_debug.c | 6 +--
drivers/gpu/drm/msm/dp/dp_debug.h | 4 +-
drivers/gpu/drm/msm/dp/dp_display.c | 55
drivers/gpu/drm/msm/dp/dp_hpd.c | 67 -
drivers/gpu/drm/msm/dp/dp_hpd.h | 78 -
drivers/gpu/drm/msm/dp/dp_panel.h | 1 -
drivers/gpu/drm/msm/dp/dp_power.c | 2 +-
drivers/gpu/drm/msm/dp/dp_power.h | 3 +-
10 files changed, 16 insertions(+), 202 deletions(-)
delete mode 100644 drivers/gpu/drm/msm/dp/dp_hpd.c
delete mode 100644 drivers/gpu/drm/msm/dp/dp_hpd.h
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 66395ee0862a..c417443168f6 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -123,7 +123,6 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
dp/dp_ctrl.o \
dp/dp_display.o \
dp/dp_drm.o \
- dp/dp_hpd.o \
dp/dp_link.o \
dp/dp_panel.o \
dp/dp_parser.o \
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 0745fde01b45..52648b56f54b 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -14,7 +14,6 @@
#include "dp_catalog.h"
struct dp_ctrl {
- bool orientation;
atomic_t aborted;
u32 pixel_rate;
bool wide_bus_en;
diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
b/drivers/gpu/drm/msm/dp/dp_debug.c
index 075969da9418..25ea2d3e3e12 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.c
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c
@@ -21,7 +21,6 @@
struct dp_debug_private {
struct dentry *root;
- struct dp_usbpd *usbpd;
struct dp_link *link;
struct dp_panel *panel;
struct drm_connector *connector;
@@ -234,14 +233,14 @@ static void 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 dp_link *link,
struct drm_connector *connector, struct drm_minor *minor)
{
struct dp_debug_private *debug;
struct dp_debug *dp_debug;
int rc;
- if (!dev || !panel || !usbpd || !link) {
+ if (!dev || !panel || !link) {
DRM_ERROR("invalid input\n");
rc = -EINVAL;
goto error;
@@ -254,7 +253,6 @@ struct dp_debug *dp_debug_get(struct device *dev, struct
dp_panel *panel,
}
debug->dp_debug.debug_en = false;
- debug->usbpd = usbpd;
debug->link = link;
debug->panel = panel;
debug->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_debug.h
b/drivers/gpu/drm/msm/dp/dp_debug.h
index 8c0d0b5178fd..be350cb393ee 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.h
+++ b/drivers/gpu/drm/msm/dp/dp_debug.h
@@ -42,7 +42,7 @@ struct dp_debug {
* for debugfs input to be communicated with existing modules
*/
struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel *panel,
- struct dp_usbpd *usbpd, struct dp_link *link,
+ struct dp_link *link,
struct drm_connector *connector,
struct drm_minor *minor);
@@ -59,7 +59,7 @@ void dp_debug_put(struct dp_debug *dp_debug);
static inline
struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel *panel,
- struct dp_usbpd *usbpd, struct dp_link *link,
+ struct dp_link *link,
struct drm_connector *connector, struct drm_minor *minor)
{
return ERR_PTR(-EINVAL);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index c68d6007c2c6..b447446d75e9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -14,7 +14,6 @@
#include "msm_drv.h"
#include "msm_kms.h"
-#include "dp_hpd.h"
#include "dp_parser.h"
#include "dp_power.h"
#include "dp_catalog.h"
@@ -88,7 +87,6 @@ struct dp_display_private {
struct platform_device *pdev;
struct dentry *root;
- struct dp_usbpd *usbpd;
struct dp_parser *parser;
struct dp_power *power;
struct dp_catalog *catalog;
@@ -98,7 +96,6 @@ struct dp_display_private {
struct dp_ctrl*ctrl;
Re: [Intel-gfx] [PATCH v4 5/5] drm/msm/dp: Implement hpd_notify()
On Mon 02 May 13:59 PDT 2022, Kuogee Hsieh wrote:
>
> On 5/2/2022 9:53 AM, Bjorn Andersson wrote:
> > The Qualcomm DisplayPort driver contains traces of the necessary
> > plumbing to hook up USB HPD, in the form of the dp_hpd module and the
> > dp_usbpd_cb struct. Use this as basis for implementing the
> > hpd_notify() callback, by amending the dp_hpd module with the
> > missing logic.
> >
> > Overall the solution is similar to what's done downstream, but upstream
> > all the code to disect the HPD notification lives on the calling side of
> > drm_connector_oob_hotplug_event().
> >
> > drm_connector_oob_hotplug_event() performs the lookup of the
> > drm_connector based on fwnode, hence the need to assign the fwnode in
> > dp_drm_connector_init().
> >
> > Signed-off-by: Bjorn Andersson
> > ---
> >
> > Changes since v3:
> > - Implements hpd_notify instead of oob_hotplug_event
> > - Rebased on new cleanup patch from Dmitry
> > - Set hpd_state to ST_MAINLINK_READY when dp_display_usbpd_configure()
> > succeeds
> >
> > drivers/gpu/drm/msm/dp/dp_display.c | 26 ++
> > drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> > drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++
> > drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++
> > 4 files changed, 32 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index b447446d75e9..080294ac6144 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -83,6 +83,8 @@ struct dp_display_private {
> > bool hpd_irq_on;
> > bool audio_supported;
> > + bool connected;
> > +
> > struct drm_device *drm_dev;
> > struct platform_device *pdev;
> > struct dentry *root;
> > @@ -1271,6 +1273,7 @@ static int dp_display_probe(struct platform_device
> > *pdev)
> > if (!desc)
> > return -EINVAL;
> > + dp->dp_display.dev = >dev;
> > dp->pdev = pdev;
> > dp->name = "drm_dp";
> > dp->dp_display.connector_type = desc->connector_type;
> > @@ -1760,3 +1763,26 @@ void dp_bridge_mode_set(struct drm_bridge
> > *drm_bridge,
> > dp_display->dp_mode.h_active_low =
> > !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
> > }
> > +
> > +void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> > + enum drm_connector_status status)
> > +{
> > + struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> > + struct msm_dp *dp = dp_bridge->dp_display;
> > + struct dp_display_private *dp_display = container_of(dp, struct
> > dp_display_private, dp_display);
> > + int ret;
> > +
> > + drm_dbg_dp(dp_display->drm_dev, "status: %d connected: %d\n", status,
> > dp_display->connected);
> > +
> > + if (!dp_display->connected && status == connector_status_connected) {
> > + dp_display->connected = true;
> > + ret = dp_display_usbpd_configure(dp_display);
> > + if (!ret)
> > + dp_display->hpd_state = ST_MAINLINK_READY;
> > + } else if (status != connector_status_connected) {
> > + dp_display->connected = false;
> > + dp_display_notify_disconnect(dp_display);
> > + } else {
> > + dp_display_usbpd_attention(dp_display);
> > + }
> > +}
>
> I would assume dp_bridge_hpd_notify() will server same purpose as
> dp_display_irq_handler() if hpd_notification is enabled.
>
I agree with this statement.
> In that case, should dp_bridge_hpd_notify() add
> EV_HPD_PLUG_INT/EV_IRQ_HPD_INT/EV_HPD_UNPLUG_INT
>
I tried this originally, but couldn't get it to work and expected that
as the downstream driver doesn't do this, there was some good reason for
me not to do it either.
> into event q to kick off corresponding
> dp_hpd_plug_handle()/dp_irq_hpd_handle()/dp_hpd_unplug_handle()?
>
But since then the driver has been cleaned up significantly, so I
decided to give it a test again.
Unfortunately it still doesn't work, but now it's easier to trace.
Replacing the 3 cases with relevant calls to dp_add_event() results in
us inserting a EV_HPD_UNPLUG_INT event really early, before things has
been brought up. This will result in dp_hpd_unplug_handle() trying to
disable the dp_catalog_hpd_config_intr(), which will crash as the
hardware isn't yet clocked up.
Further more, this points out the main difference between the normal HP
[Intel-gfx] [PATCH v4 0/5] drm/msm/dp: implement HPD notifications handling
USB altmodes code would send OOB notifications to the drm_connector specified in the device tree. However as the MSM DP driver uses drm_bridge_connector, there is no way to receive these event directly. Implement a bridge between oob_hotplug_event and drm_bridge's hpd_notify and use it to deliver altmode messages to the MSM DP driver. Note, I left the original 'bool connected' field to be used by the notifiers. However I think that it should be replaced in favour of using the dp->hpd_state properly. Bjorn Andersson (2): drm: Add HPD state to drm_connector_oob_hotplug_event() drm/msm/dp: Implement hpd_notify() Dmitry Baryshkov (3): drm/bridge_connector: stop filtering events in drm_bridge_connector_hpd_cb() drm/bridge_connector: implement oob_hotplug_event drm/msm/dp: remove most of usbpd-related remains drivers/gpu/drm/drm_bridge_connector.c | 17 +++-- drivers/gpu/drm/drm_connector.c | 6 +- drivers/gpu/drm/i915/display/intel_dp.c | 17 - drivers/gpu/drm/i915/i915_drv.h | 3 + drivers/gpu/drm/msm/Makefile | 1 - drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 - drivers/gpu/drm/msm/dp/dp_debug.c| 6 +- drivers/gpu/drm/msm/dp/dp_debug.h| 4 +- drivers/gpu/drm/msm/dp/dp_display.c | 81 +++- drivers/gpu/drm/msm/dp/dp_display.h | 1 + drivers/gpu/drm/msm/dp/dp_drm.c | 3 + drivers/gpu/drm/msm/dp/dp_drm.h | 2 + drivers/gpu/drm/msm/dp/dp_hpd.c | 67 drivers/gpu/drm/msm/dp/dp_hpd.h | 78 --- drivers/gpu/drm/msm/dp/dp_panel.h| 1 - drivers/gpu/drm/msm/dp/dp_power.c| 2 +- drivers/gpu/drm/msm/dp/dp_power.h| 3 +- drivers/usb/typec/altmodes/displayport.c | 10 +-- include/drm/drm_connector.h | 6 +- 19 files changed, 88 insertions(+), 221 deletions(-) delete mode 100644 drivers/gpu/drm/msm/dp/dp_hpd.c delete mode 100644 drivers/gpu/drm/msm/dp/dp_hpd.h -- 2.35.1
[Intel-gfx] [PATCH v4 2/5] drm: Add HPD state to drm_connector_oob_hotplug_event()
In some implementations, such as the Qualcomm platforms, the display
driver has no way to query the current HPD state and as such it's
impossible to distinguish between disconnect and attention events.
Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
state.
Also push the test for unchanged state in the displayport altmode driver
into the i915 driver, to allow other drivers to act upon each update.
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- Transition to drm_connector_status instead of custom hpd_state
drivers/gpu/drm/drm_connector.c | 6 --
drivers/gpu/drm/i915/display/intel_dp.c | 17 ++---
drivers/gpu/drm/i915/i915_drv.h | 3 +++
drivers/usb/typec/altmodes/displayport.c | 10 +++---
include/drm/drm_connector.h | 6 --
5 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..e86c69f0640f 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2794,6 +2794,7 @@ struct drm_connector *drm_connector_find_by_fwnode(struct
fwnode_handle *fwnode)
/**
* drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
connector
* @connector_fwnode: fwnode_handle to report the event on
+ * @status: hot plug detect logical state
*
* On some hardware a hotplug event notification may come from outside the
display
* driver / device. An example of this is some USB Type-C setups where the
hardware
@@ -2803,7 +2804,8 @@ struct drm_connector *drm_connector_find_by_fwnode(struct
fwnode_handle *fwnode)
* This function can be used to report these out-of-band events after obtaining
* a drm_connector reference through calling drm_connector_find_by_fwnode().
*/
-void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
+void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
+enum drm_connector_status status)
{
struct drm_connector *connector;
@@ -2812,7 +2814,7 @@ void drm_connector_oob_hotplug_event(struct fwnode_handle
*connector_fwnode)
return;
if (connector->funcs->oob_hotplug_event)
- connector->funcs->oob_hotplug_event(connector);
+ connector->funcs->oob_hotplug_event(connector, status);
drm_connector_put(connector);
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index e4a79c11fd25..56cc023f7bbd 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4951,15 +4951,26 @@ static int intel_dp_connector_atomic_check(struct
drm_connector *conn,
return intel_modeset_synced_crtcs(state, conn);
}
-static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
+static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
+ enum drm_connector_status hpd_state)
{
struct intel_encoder *encoder =
intel_attached_encoder(to_intel_connector(connector));
struct drm_i915_private *i915 = to_i915(connector->dev);
+ bool hpd_high = hpd_state == connector_status_connected;
+ unsigned int hpd_pin = encoder->hpd_pin;
+ bool need_work = false;
spin_lock_irq(>irq_lock);
- i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
+ if (hpd_high != test_bit(hpd_pin,
>hotplug.oob_hotplug_last_state)) {
+ i915->hotplug.event_bits |= BIT(hpd_pin);
+
+ __assign_bit(hpd_pin, >hotplug.oob_hotplug_last_state,
hpd_high);
+ need_work = true;
+ }
spin_unlock_irq(>irq_lock);
- queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
+
+ if (need_work)
+ queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
}
static const struct drm_connector_funcs intel_dp_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 24111bf42ce0..96c088bb5522 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -135,6 +135,9 @@ struct i915_hotplug {
/* Whether or not to count short HPD IRQs in HPD storms */
u8 hpd_short_storm_enabled;
+ /* Last state reported by oob_hotplug_event for each encoder */
+ unsigned long oob_hotplug_last_state;
+
/*
* if we get a HPD irq from DP and a HPD irq from non-DP
* the non-DP HPD could block the workqueue on a mode config
diff --git a/drivers/usb/typec/altmodes/displayport.c
b/drivers/usb/typec/altmodes/displayport.c
index c1d8c23baa39..9360ca177c7d 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -59,7 +59,6 @@ struct dp_altmode {
struct typec_displayport_data data;
enum dp_state s
Re: [Intel-gfx] [PATCH v5 09/10] arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller
On Mon 11 Apr 13:47 PDT 2022, Sean Paul wrote:
> From: Sean Paul
>
> This patch adds the register ranges required for HDCP key injection and
> HDCP TrustZone interaction as described in the dt-bindings for the
> sc7180 dp controller.
Can you please mention why this is only done for trogdor and not sc7180
as a whole?
> Now that these are supported, change the compatible string to
> "dp-hdcp".
>
I don't see this change in the patch.
> Signed-off-by: Sean Paul
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-s...@poorly.run
> #v1
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-14-s...@poorly.run
> #v2
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-14-s...@poorly.run
> #v3
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20211105030434.2828845-14-s...@poorly.run
> #v4
>
> Changes in v3:
> -Split off into a new patch containing just the dts change (Stephen)
> -Add hdcp compatible string (Stephen)
> Changes in v4:
> -Rebase on Bjorn's multi-dp patchset
> Changes in v5:
> -Put the tz register offsets in trogdor dtsi (Rob C)
> ---
> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 8
> arch/arm64/boot/dts/qcom/sc7180.dtsi | 6 +-
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index 732e1181af48..c3559253aefc 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -815,6 +815,14 @@ _dp {
> data-lanes = <0 1>;
> vdda-1p2-supply = <_usb_ss_dp_1p2>;
> vdda-0p9-supply = <_usb_ss_dp_core>;
> +
> + reg = <0 0x0ae9 0 0x200>,
> + <0 0x0ae90200 0 0x200>,
> + <0 0x0ae90400 0 0xc00>,
> + <0 0x0ae91000 0 0x400>,
> + <0 0x0ae91400 0 0x400>,
> + <0 0x0aed1000 0 0x175>,
> + <0 0x0aee1000 0 0x2c>;
> };
>
> _adc {
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index e1c46b80f14a..3c3eef7a7d52 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -3089,7 +3089,11 @@ mdss_dp: displayport-controller@ae9 {
> compatible = "qcom,sc7180-dp";
> status = "disabled";
>
> - reg = <0 0x0ae9 0 0x1400>;
> + reg = <0 0x0ae9 0 0x200>,
> + <0 0x0ae90200 0 0x200>,
> + <0 0x0ae90400 0 0xc00>,
> + <0 0x0ae91000 0 0x400>,
> + <0 0x0ae91400 0 0x400>;
This hunk stands on its own, following the DT binding changes I did
earlier. Would you mind spinning it off so I can merge it separately?
Thanks,
Bjorn
>
> interrupt-parent = <>;
> interrupts = <12>;
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
Re: [Intel-gfx] [PATCH v3 2/2] drm/msm/dp: Implement oob_hotplug_event()
On Fri 22 Apr 16:07 PDT 2022, Dmitry Baryshkov wrote:
> On 23/04/2022 01:32, Bjorn Andersson wrote:
[..]
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> > b/drivers/gpu/drm/msm/dp/dp_drm.c
> > index 80f59cf99089..76904b1601b1 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -123,6 +123,14 @@ static enum drm_mode_status dp_connector_mode_valid(
> > return dp_display_validate_mode(dp_disp, mode->clock);
> > }
> > +static void dp_oob_hotplug_event(struct drm_connector *connector,
> > +enum drm_connector_hpd_state hpd_state)
> > +{
> > + struct msm_dp *dp_disp = to_dp_connector(connector)->dp_display;
> > +
> > + dp_display_oob_hotplug_event(dp_disp, hpd_state);
> > +}
> > +
> > static const struct drm_connector_funcs dp_connector_funcs = {
> > .detect = dp_connector_detect,
> > .fill_modes = drm_helper_probe_single_connector_modes,
> > @@ -130,6 +138,7 @@ static const struct drm_connector_funcs
> > dp_connector_funcs = {
> > .reset = drm_atomic_helper_connector_reset,
> > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > + .oob_hotplug_event = dp_oob_hotplug_event,
>
> We were (are) going to switch dp driver to use drm_bridge_connector (to fix
> support for bridge chains, eDP panels, etc.
>
> So these changes must be ported to drm_bridge_connector (or we must
> abandon/defer the idea of using the bridge_connector).
>
> For the oob_hotplug_event() callback proper support might be as simple as
> calling drm_bridge_connector_hpd_cb().
>
Are you saying that you have code ready and being merged into linux-next
that I should redesign this on top of, or that you're planning to write
some code in the future and DisplayPort support have to wait until then?
> > };
> > static const struct drm_connector_helper_funcs dp_connector_helper_funcs
> > = {
> > @@ -160,6 +169,8 @@ struct drm_connector *dp_drm_connector_init(struct
> > msm_dp *dp_display)
> > if (ret)
> > return ERR_PTR(ret);
> > + connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev));
> > +
>
> This would be much more interesting. Supporting this in a generic way might
> be tricky. But we can still set the fwnode manually from the dp code.
>
There's a slight mishmash here, because the device used to initialize
the connector is the drm_dev, but we need the actual fwnode of the DP
device associated with the connector.
So I think this is how it needs to be done.
Regards,
Bjorn
[Intel-gfx] [PATCH v3 1/2] drm: Add HPD state to drm_connector_oob_hotplug_event()
In some implementations, such as the Qualcomm platforms, the display
driver has no way to query the current HPD state and as such it's
impossible to distinguish between disconnect and attention events.
Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
state.
Also push the test for unchanged state in the displayport altmode driver
into the i915 driver, to allow other drivers to act upon each update.
Signed-off-by: Bjorn Andersson
---
Changs since v2:
- The i915 cached hpd_state is tracked per encoder.
drivers/gpu/drm/drm_connector.c | 6 --
drivers/gpu/drm/i915/display/intel_dp.c | 17 ++---
drivers/gpu/drm/i915/i915_drv.h | 3 +++
drivers/usb/typec/altmodes/displayport.c | 10 +++---
include/drm/drm_connector.h | 11 +--
5 files changed, 33 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 76a8c707c34b..fff8c74d1ae6 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2828,6 +2828,7 @@ struct drm_connector *drm_connector_find_by_fwnode(struct
fwnode_handle *fwnode)
/**
* drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
connector
* @connector_fwnode: fwnode_handle to report the event on
+ * @hpd_state: hot plug detect logical state
*
* On some hardware a hotplug event notification may come from outside the
display
* driver / device. An example of this is some USB Type-C setups where the
hardware
@@ -2837,7 +2838,8 @@ struct drm_connector *drm_connector_find_by_fwnode(struct
fwnode_handle *fwnode)
* This function can be used to report these out-of-band events after obtaining
* a drm_connector reference through calling drm_connector_find_by_fwnode().
*/
-void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
+void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
+enum drm_connector_hpd_state hpd_state)
{
struct drm_connector *connector;
@@ -2846,7 +2848,7 @@ void drm_connector_oob_hotplug_event(struct fwnode_handle
*connector_fwnode)
return;
if (connector->funcs->oob_hotplug_event)
- connector->funcs->oob_hotplug_event(connector);
+ connector->funcs->oob_hotplug_event(connector, hpd_state);
drm_connector_put(connector);
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index d55acc4a028a..2907d8e1f80e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4950,15 +4950,26 @@ static int intel_dp_connector_atomic_check(struct
drm_connector *conn,
return intel_modeset_synced_crtcs(state, conn);
}
-static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
+static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
+ enum drm_connector_hpd_state hpd_state)
{
struct intel_encoder *encoder =
intel_attached_encoder(to_intel_connector(connector));
struct drm_i915_private *i915 = to_i915(connector->dev);
+ bool hpd_high = hpd_state == DRM_CONNECTOR_HPD_HIGH;
+ unsigned int hpd_pin = encoder->hpd_pin;
+ bool need_work = false;
spin_lock_irq(>irq_lock);
- i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
+ if (hpd_high != test_bit(hpd_pin,
>hotplug.oob_hotplug_last_state)) {
+ i915->hotplug.event_bits |= BIT(hpd_pin);
+
+ __assign_bit(hpd_pin, >hotplug.oob_hotplug_last_state,
hpd_high);
+ need_work = true;
+ }
spin_unlock_irq(>irq_lock);
- queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
+
+ if (need_work)
+ queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
}
static const struct drm_connector_funcs intel_dp_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3711d618a372..71d0c7130ddd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -134,6 +134,9 @@ struct i915_hotplug {
/* Whether or not to count short HPD IRQs in HPD storms */
u8 hpd_short_storm_enabled;
+ /* Last state reported by oob_hotplug_event for each encoder */
+ unsigned long oob_hotplug_last_state;
+
/*
* if we get a HPD irq from DP and a HPD irq from non-DP
* the non-DP HPD could block the workqueue on a mode config
diff --git a/drivers/usb/typec/altmodes/displayport.c
b/drivers/usb/typec/altmodes/displayport.c
index c1d8c23baa39..ea9cb1d71fd2 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -59,7 +59,6 @@ struct dp_altmode {
struct typec_displayport_data data;
enum dp_state s
[Intel-gfx] [PATCH v3 2/2] drm/msm/dp: Implement oob_hotplug_event()
The Qualcomm DisplayPort driver contains traces of the necessary
plumbing to hook up USB HPD, in the form of the dp_hpd module and the
dp_usbpd_cb struct. Use this as basis for implementing the
oob_hotplug_event() callback, by amending the dp_hpd module with the
missing logic.
Overall the solution is similar to what's done downstream, but upstream
all the code to disect the HPD notification lives on the calling side of
drm_connector_oob_hotplug_event().
drm_connector_oob_hotplug_event() performs the lookup of the
drm_connector based on fwnode, hence the need to assign the fwnode in
dp_drm_connector_init().
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- Rebased patch
drivers/gpu/drm/msm/dp/dp_display.c | 9 +
drivers/gpu/drm/msm/dp/dp_display.h | 3 +++
drivers/gpu/drm/msm/dp/dp_drm.c | 11 +++
drivers/gpu/drm/msm/dp/dp_hpd.c | 21 +
drivers/gpu/drm/msm/dp/dp_hpd.h | 5 +
5 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index a42732b67349..1019f6d8fd03 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -449,6 +449,14 @@ static int dp_display_usbpd_configure_cb(struct device
*dev)
return dp_display_process_hpd_high(dp);
}
+void dp_display_oob_hotplug_event(struct msm_dp *dp_display,
+ enum drm_connector_hpd_state hpd_state)
+{
+ struct dp_display_private *dp = container_of(dp_display, struct
dp_display_private, dp_display);
+
+ dp->usbpd->oob_event(dp->usbpd, hpd_state);
+}
+
static int dp_display_usbpd_disconnect_cb(struct device *dev)
{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
@@ -1302,6 +1310,7 @@ static int dp_display_probe(struct platform_device *pdev)
dp->pdev = pdev;
dp->name = "drm_dp";
dp->dp_display.connector_type = desc->connector_type;
+ dp->dp_display.dev = >dev;
rc = dp_init_sub_modules(dp);
if (rc) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index 7af2b186d2d9..16658270df2c 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -11,6 +11,7 @@
#include "disp/msm_disp_snapshot.h"
struct msm_dp {
+ struct device *dev;
struct drm_device *drm_dev;
struct device *codec_dev;
struct drm_bridge *bridge;
@@ -40,5 +41,7 @@ bool dp_display_check_video_test(struct msm_dp *dp_display);
int dp_display_get_test_bpp(struct msm_dp *dp_display);
void dp_display_signal_audio_start(struct msm_dp *dp_display);
void dp_display_signal_audio_complete(struct msm_dp *dp_display);
+void dp_display_oob_hotplug_event(struct msm_dp *dp_display,
+ enum drm_connector_hpd_state hpd_state);
#endif /* _DP_DISPLAY_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 80f59cf99089..76904b1601b1 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -123,6 +123,14 @@ static enum drm_mode_status dp_connector_mode_valid(
return dp_display_validate_mode(dp_disp, mode->clock);
}
+static void dp_oob_hotplug_event(struct drm_connector *connector,
+enum drm_connector_hpd_state hpd_state)
+{
+ struct msm_dp *dp_disp = to_dp_connector(connector)->dp_display;
+
+ dp_display_oob_hotplug_event(dp_disp, hpd_state);
+}
+
static const struct drm_connector_funcs dp_connector_funcs = {
.detect = dp_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
@@ -130,6 +138,7 @@ static const struct drm_connector_funcs dp_connector_funcs
= {
.reset = drm_atomic_helper_connector_reset,
.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+ .oob_hotplug_event = dp_oob_hotplug_event,
};
static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
@@ -160,6 +169,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display)
if (ret)
return ERR_PTR(ret);
+ connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev));
+
drm_connector_helper_add(connector, _connector_helper_funcs);
/*
diff --git a/drivers/gpu/drm/msm/dp/dp_hpd.c b/drivers/gpu/drm/msm/dp/dp_hpd.c
index db98a1d431eb..cdb1feea5ebf 100644
--- a/drivers/gpu/drm/msm/dp/dp_hpd.c
+++ b/drivers/gpu/drm/msm/dp/dp_hpd.c
@@ -7,6 +7,8 @@
#include
#include
+#include
+#include
#include "dp_hpd.h"
@@ -45,6 +47,24 @@ int dp_hpd_connect(struct dp_usbpd *dp_usbpd, bool hpd)
return rc;
}
+static void dp_hpd_oob_event(struct dp_usbpd *dp_usbpd,
+enum drm_conn
[Intel-gfx] [PATCH v2 1/2] drm: Add HPD state to drm_connector_oob_hotplug_event()
In some implementations, such as the Qualcomm platforms, the display
driver has no way to query the current HPD state and as such it's
impossible to distinguish between disconnect and attention events.
Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
state.
Also push the test for unchanged state in the displayport altmode driver
into the i915 driver, to allow other drivers to act upon each update.
Changes in v2:
- Replace bool with drm_connector_hpd_state enum to represent "state" better
- Track old hpd state per encoder in i915
Signed-off-by: Bjorn Andersson
---
drivers/gpu/drm/drm_connector.c | 6 --
drivers/gpu/drm/i915/display/intel_dp.c | 17 ++---
drivers/gpu/drm/i915/i915_drv.h | 3 +++
drivers/usb/typec/altmodes/displayport.c | 10 +++---
include/drm/drm_connector.h | 11 +--
5 files changed, 33 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index a50c82bc2b2f..a44f082ebd9d 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2825,6 +2825,7 @@ struct drm_connector *drm_connector_find_by_fwnode(struct
fwnode_handle *fwnode)
/**
* drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
connector
* @connector_fwnode: fwnode_handle to report the event on
+ * @hpd_state: hot plug detect logical state
*
* On some hardware a hotplug event notification may come from outside the
display
* driver / device. An example of this is some USB Type-C setups where the
hardware
@@ -2834,7 +2835,8 @@ struct drm_connector *drm_connector_find_by_fwnode(struct
fwnode_handle *fwnode)
* This function can be used to report these out-of-band events after obtaining
* a drm_connector reference through calling drm_connector_find_by_fwnode().
*/
-void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
+void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
+enum drm_connector_hpd_state hpd_state)
{
struct drm_connector *connector;
@@ -2843,7 +2845,7 @@ void drm_connector_oob_hotplug_event(struct fwnode_handle
*connector_fwnode)
return;
if (connector->funcs->oob_hotplug_event)
- connector->funcs->oob_hotplug_event(connector);
+ connector->funcs->oob_hotplug_event(connector, hpd_state);
drm_connector_put(connector);
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 1046e7fe310a..a3c9dbae5cee 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4825,15 +4825,26 @@ static int intel_dp_connector_atomic_check(struct
drm_connector *conn,
return intel_modeset_synced_crtcs(state, conn);
}
-static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
+static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
+ enum drm_connector_hpd_state hpd_state)
{
struct intel_encoder *encoder =
intel_attached_encoder(to_intel_connector(connector));
struct drm_i915_private *i915 = to_i915(connector->dev);
+ bool hpd_high = hpd_state == DRM_CONNECTOR_HPD_HIGH;
+ unsigned int hpd_pin = encoder->hpd_pin;
+ bool need_work = false;
spin_lock_irq(>irq_lock);
- i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
+ if (hpd_high != test_bit(hpd_pin,
>hotplug.oob_hotplug_last_state)) {
+ i915->hotplug.event_bits |= BIT(hpd_pin);
+
+ __assign_bit(hpd_pin, >hotplug.oob_hotplug_last_state,
hpd_high);
+ need_work = true;
+ }
spin_unlock_irq(>irq_lock);
- queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
+
+ if (need_work)
+ queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
}
static const struct drm_connector_funcs intel_dp_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5cfe69b30841..80a4615a38e2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -138,6 +138,9 @@ struct i915_hotplug {
/* Whether or not to count short HPD IRQs in HPD storms */
u8 hpd_short_storm_enabled;
+ /* Last state reported by oob_hotplug_event for each encoder */
+ unsigned long oob_hotplug_last_state;
+
/*
* if we get a HPD irq from DP and a HPD irq from non-DP
* the non-DP HPD could block the workqueue on a mode config
diff --git a/drivers/usb/typec/altmodes/displayport.c
b/drivers/usb/typec/altmodes/displayport.c
index c1d8c23baa39..ea9cb1d71fd2 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -59,7 +59,6 @@ struct dp_altmode
[Intel-gfx] [PATCH v2 2/2] drm/msm/dp: Implement oob_hotplug_event()
The Qualcomm DisplayPort driver contains traces of the necessary
plumbing to hook up USB HPD, in the form of the dp_hpd module and the
dp_usbpd_cb struct. Use this as basis for implementing the
oob_hotplug_event() callback, by amending the dp_hpd module with the
missing logic.
Overall the solution is similar to what's done downstream, but upstream
all the code to disect the HPD notification lives on the calling side of
drm_connector_oob_hotplug_event().
drm_connector_oob_hotplug_event() performs the lookup of the
drm_connector based on fwnode, hence the need to assign the fwnode in
dp_drm_connector_init().
Changes in v2:
- Adopt enum drm_connector_hpd_state
Signed-off-by: Bjorn Andersson
---
drivers/gpu/drm/msm/dp/dp_display.c | 9 +
drivers/gpu/drm/msm/dp/dp_display.h | 3 +++
drivers/gpu/drm/msm/dp/dp_drm.c | 11 +++
drivers/gpu/drm/msm/dp/dp_hpd.c | 21 +
drivers/gpu/drm/msm/dp/dp_hpd.h | 5 +
5 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 178b774a5fbd..3d9d754a75f3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -449,6 +449,14 @@ static int dp_display_usbpd_configure_cb(struct device
*dev)
return dp_display_process_hpd_high(dp);
}
+void dp_display_oob_hotplug_event(struct msm_dp *dp_display,
+ enum drm_connector_hpd_state hpd_state)
+{
+ struct dp_display_private *dp = container_of(dp_display, struct
dp_display_private, dp_display);
+
+ dp->usbpd->oob_event(dp->usbpd, hpd_state);
+}
+
static int dp_display_usbpd_disconnect_cb(struct device *dev)
{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
@@ -1296,6 +1304,7 @@ static int dp_display_probe(struct platform_device *pdev)
dp->pdev = pdev;
dp->name = "drm_dp";
dp->dp_display.connector_type = desc->connector_type;
+ dp->dp_display.dev = >dev;
rc = dp_init_sub_modules(dp);
if (rc) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index 7af2b186d2d9..16658270df2c 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -11,6 +11,7 @@
#include "disp/msm_disp_snapshot.h"
struct msm_dp {
+ struct device *dev;
struct drm_device *drm_dev;
struct device *codec_dev;
struct drm_bridge *bridge;
@@ -40,5 +41,7 @@ bool dp_display_check_video_test(struct msm_dp *dp_display);
int dp_display_get_test_bpp(struct msm_dp *dp_display);
void dp_display_signal_audio_start(struct msm_dp *dp_display);
void dp_display_signal_audio_complete(struct msm_dp *dp_display);
+void dp_display_oob_hotplug_event(struct msm_dp *dp_display,
+ enum drm_connector_hpd_state hpd_state);
#endif /* _DP_DISPLAY_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 80f59cf99089..76904b1601b1 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -123,6 +123,14 @@ static enum drm_mode_status dp_connector_mode_valid(
return dp_display_validate_mode(dp_disp, mode->clock);
}
+static void dp_oob_hotplug_event(struct drm_connector *connector,
+enum drm_connector_hpd_state hpd_state)
+{
+ struct msm_dp *dp_disp = to_dp_connector(connector)->dp_display;
+
+ dp_display_oob_hotplug_event(dp_disp, hpd_state);
+}
+
static const struct drm_connector_funcs dp_connector_funcs = {
.detect = dp_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
@@ -130,6 +138,7 @@ static const struct drm_connector_funcs dp_connector_funcs
= {
.reset = drm_atomic_helper_connector_reset,
.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+ .oob_hotplug_event = dp_oob_hotplug_event,
};
static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
@@ -160,6 +169,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display)
if (ret)
return ERR_PTR(ret);
+ connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev));
+
drm_connector_helper_add(connector, _connector_helper_funcs);
/*
diff --git a/drivers/gpu/drm/msm/dp/dp_hpd.c b/drivers/gpu/drm/msm/dp/dp_hpd.c
index db98a1d431eb..cdb1feea5ebf 100644
--- a/drivers/gpu/drm/msm/dp/dp_hpd.c
+++ b/drivers/gpu/drm/msm/dp/dp_hpd.c
@@ -7,6 +7,8 @@
#include
#include
+#include
+#include
#include "dp_hpd.h"
@@ -45,6 +47,24 @@ int dp_hpd_connect(struct dp_usbpd *dp_usbpd, bool hpd)
return rc;
}
+static void dp_hpd_oob_event(struct dp_usbpd *dp_usbpd,
+enu
Re: [Intel-gfx] [PATCH 1/2] drm: Add HPD state to drm_connector_oob_hotplug_event()
On Mon 14 Feb 09:59 PST 2022, Imre Deak wrote:
> On Mon, Feb 07, 2022 at 08:43:27PM -0800, Bjorn Andersson wrote:
> > In some implementations, such as the Qualcomm platforms, the display
> > driver has no way to query the current HPD state and as such it's
> > impossible to distinguish between disconnect and attention events.
> >
> > Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
> > state.
> >
> > Also push the test for unchanged state in the displayport altmode driver
> > into the i915 driver, to allow other drivers to act upon each update.
> >
> > Signed-off-by: Bjorn Andersson
> > ---
> >
> > Note that the Intel driver has only been compile tested with this patch.
> >
> > drivers/gpu/drm/drm_connector.c | 6 --
> > drivers/gpu/drm/i915/display/intel_dp.c | 14 +++---
> > drivers/gpu/drm/i915/i915_drv.h | 3 +++
> > drivers/usb/typec/altmodes/displayport.c | 9 ++---
> > include/drm/drm_connector.h | 5 +++--
> > 5 files changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index a50c82bc2b2f..ad7295597c0f 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -2825,6 +2825,7 @@ struct drm_connector
> > *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> > /**
> > * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> > connector
> > * @connector_fwnode: fwnode_handle to report the event on
> > + * @hpd_state: number of data lanes available
> > *
> > * On some hardware a hotplug event notification may come from outside the
> > display
> > * driver / device. An example of this is some USB Type-C setups where the
> > hardware
> > @@ -2834,7 +2835,8 @@ struct drm_connector
> > *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> > * This function can be used to report these out-of-band events after
> > obtaining
> > * a drm_connector reference through calling
> > drm_connector_find_by_fwnode().
> > */
> > -void drm_connector_oob_hotplug_event(struct fwnode_handle
> > *connector_fwnode)
> > +void drm_connector_oob_hotplug_event(struct fwnode_handle
> > *connector_fwnode,
> > +bool hpd_state)
> > {
> > struct drm_connector *connector;
> >
> > @@ -2843,7 +2845,7 @@ void drm_connector_oob_hotplug_event(struct
> > fwnode_handle *connector_fwnode)
> > return;
> >
> > if (connector->funcs->oob_hotplug_event)
> > - connector->funcs->oob_hotplug_event(connector);
> > + connector->funcs->oob_hotplug_event(connector, hpd_state);
> >
> > drm_connector_put(connector);
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 146b83916005..00520867d37b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4816,15 +4816,23 @@ static int intel_dp_connector_atomic_check(struct
> > drm_connector *conn,
> > return intel_modeset_synced_crtcs(state, conn);
> > }
> >
> > -static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
> > +static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
> > bool hpd_state)
> > {
> > struct intel_encoder *encoder =
> > intel_attached_encoder(to_intel_connector(connector));
> > struct drm_i915_private *i915 = to_i915(connector->dev);
> > + bool need_work = false;
> >
> > spin_lock_irq(>irq_lock);
> > - i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> > + if (hpd_state != i915->hotplug.oob_hotplug_state) {
>
> hpd_state is speific to the encoder (pin) so similarly to event_bits
> oob_hotplug_state should be a bitmask as well.
>
That makes sense, thanks for point it out!
Regards,
Bjorn
Re: [Intel-gfx] [PATCH 1/2] drm: Add HPD state to drm_connector_oob_hotplug_event()
On Tue 08 Feb 02:39 PST 2022, Greg Kroah-Hartman wrote:
> On Mon, Feb 07, 2022 at 08:43:27PM -0800, Bjorn Andersson wrote:
> > In some implementations, such as the Qualcomm platforms, the display
> > driver has no way to query the current HPD state and as such it's
> > impossible to distinguish between disconnect and attention events.
> >
> > Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
> > state.
> >
> > Also push the test for unchanged state in the displayport altmode driver
> > into the i915 driver, to allow other drivers to act upon each update.
> >
> > Signed-off-by: Bjorn Andersson
> > ---
> >
> > Note that the Intel driver has only been compile tested with this patch.
> >
> > drivers/gpu/drm/drm_connector.c | 6 --
> > drivers/gpu/drm/i915/display/intel_dp.c | 14 +++---
> > drivers/gpu/drm/i915/i915_drv.h | 3 +++
> > drivers/usb/typec/altmodes/displayport.c | 9 ++---
> > include/drm/drm_connector.h | 5 +++--
> > 5 files changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index a50c82bc2b2f..ad7295597c0f 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -2825,6 +2825,7 @@ struct drm_connector
> > *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> > /**
> > * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> > connector
> > * @connector_fwnode: fwnode_handle to report the event on
> > + * @hpd_state: number of data lanes available
>
> "number"?
>
> > *
> > * On some hardware a hotplug event notification may come from outside the
> > display
> > * driver / device. An example of this is some USB Type-C setups where the
> > hardware
> > @@ -2834,7 +2835,8 @@ struct drm_connector
> > *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> > * This function can be used to report these out-of-band events after
> > obtaining
> > * a drm_connector reference through calling
> > drm_connector_find_by_fwnode().
> > */
> > -void drm_connector_oob_hotplug_event(struct fwnode_handle
> > *connector_fwnode)
> > +void drm_connector_oob_hotplug_event(struct fwnode_handle
> > *connector_fwnode,
> > +bool hpd_state)
>
> This is a boolean, how can it be a number?
>
The kerneldoc wasn't appropriately updated as this went from being
"number of data lanes" to "the hot plug detect (hpd) state".
> And having a "flag" like this is a pain, how do you know what the
> parameter really means?
>
You're right, "state" isn't a boolean property, let's rename it
"hpd_high" to clarify it.
> > {
> > struct drm_connector *connector;
> >
> > @@ -2843,7 +2845,7 @@ void drm_connector_oob_hotplug_event(struct
> > fwnode_handle *connector_fwnode)
> > return;
> >
> > if (connector->funcs->oob_hotplug_event)
> > - connector->funcs->oob_hotplug_event(connector);
> > + connector->funcs->oob_hotplug_event(connector, hpd_state);
> >
> > drm_connector_put(connector);
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 146b83916005..00520867d37b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4816,15 +4816,23 @@ static int intel_dp_connector_atomic_check(struct
> > drm_connector *conn,
> > return intel_modeset_synced_crtcs(state, conn);
> > }
> >
> > -static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
> > +static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
> > bool hpd_state)
> > {
> > struct intel_encoder *encoder =
> > intel_attached_encoder(to_intel_connector(connector));
> > struct drm_i915_private *i915 = to_i915(connector->dev);
> > + bool need_work = false;
> >
> > spin_lock_irq(>irq_lock);
> > - i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> > + if (hpd_state != i915->hotplug.oob_hotplug_state) {
> > + i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> > +
> > + i915->hotplug.oob_hotplug_state = hpd_state;
> > + need_work = true;
> > + }
> >
Re: [Intel-gfx] [PATCH 2/2] drm/msm/dp: Implement oob_hotplug_event()
On Mon 07 Feb 23:40 PST 2022, Greg Kroah-Hartman wrote:
> On Mon, Feb 07, 2022 at 08:43:28PM -0800, Bjorn Andersson wrote:
> > The Qualcomm DisplayPort driver contains traces of the necessary
> > plumbing to hook up USB HPD, in the form of the dp_hpd module and the
> > dp_usbpd_cb struct. Use this as basis for implementing the
> > oob_hotplug_event() callback, by amending the dp_hpd module with the
> > missing logic.
> >
> > Overall the solution is similar to what's done downstream, but upstream
> > all the code to disect the HPD notification lives on the calling side of
> > drm_connector_oob_hotplug_event().
> >
> > drm_connector_oob_hotplug_event() performs the lookup of the
> > drm_connector based on fwnode, hence the need to assign the fwnode in
> > dp_drm_connector_init().
> >
> > Signed-off-by: Bjorn Andersson
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 8
> > drivers/gpu/drm/msm/dp/dp_display.h | 2 ++
> > drivers/gpu/drm/msm/dp/dp_drm.c | 10 ++
> > drivers/gpu/drm/msm/dp/dp_hpd.c | 19 +++
> > drivers/gpu/drm/msm/dp/dp_hpd.h | 4
> > 5 files changed, 43 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 7cc4d21f2091..124a2f794382 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -414,6 +414,13 @@ static int dp_display_usbpd_configure_cb(struct device
> > *dev)
> > return dp_display_process_hpd_high(dp);
> > }
> >
> > +void dp_display_oob_hotplug_event(struct msm_dp *dp_display, bool
> > hpd_state)
> > +{
> > + struct dp_display_private *dp = container_of(dp_display, struct
> > dp_display_private, dp_display);
> > +
> > + dp->usbpd->oob_event(dp->usbpd, hpd_state);
> > +}
> > +
> > static int dp_display_usbpd_disconnect_cb(struct device *dev)
> > {
> > struct dp_display_private *dp = dev_get_dp_display_private(dev);
> > @@ -1251,6 +1258,7 @@ static int dp_display_probe(struct platform_device
> > *pdev)
> > dp->pdev = pdev;
> > dp->name = "drm_dp";
> > dp->dp_display.connector_type = desc->connector_type;
> > + dp->dp_display.dev = >dev;
>
> You did not properly reference count this pointer you just saved. What
> is to keep that pointer from going away without you knowing about it?
>
The "dp" object only lives while >dev is alive, both logically and
as its devres allocated on So for this reference I don't see
that we should refcount it.
> And you already have a pointer to pdev, why save another one here?
>
The Qualcomm DisplayPort driver has per-c-file private context structs
and "dp" is one such object. So I simply can't dereference it and get to
pdev from the other c-file in the same driver...
But I only need it in dp_drm.c to during initialization to get a
reference to the associated fwnode, so it seems that I can rework this
and pass the pointer as a parameter to dp_drm_connector_init().
That looks to be cleaner as well.
Thanks,
Bjorn
Re: [Intel-gfx] [PATCH 1/2] drm: Add HPD state to drm_connector_oob_hotplug_event()
On Thu 10 Feb 13:12 PST 2022, Dmitry Baryshkov wrote:
> On Thu, 10 Feb 2022 at 23:54, Bjorn Andersson
> wrote:
> >
> > On Tue 08 Feb 02:39 PST 2022, Greg Kroah-Hartman wrote:
> >
> > > On Mon, Feb 07, 2022 at 08:43:27PM -0800, Bjorn Andersson wrote:
> > > > In some implementations, such as the Qualcomm platforms, the display
> > > > driver has no way to query the current HPD state and as such it's
> > > > impossible to distinguish between disconnect and attention events.
> > > >
> > > > Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
> > > > state.
> > > >
> > > > Also push the test for unchanged state in the displayport altmode driver
> > > > into the i915 driver, to allow other drivers to act upon each update.
> > > >
> > > > Signed-off-by: Bjorn Andersson
> > > > ---
> > > >
> > > > Note that the Intel driver has only been compile tested with this patch.
> > > >
> > > > drivers/gpu/drm/drm_connector.c | 6 --
> > > > drivers/gpu/drm/i915/display/intel_dp.c | 14 +++---
> > > > drivers/gpu/drm/i915/i915_drv.h | 3 +++
> > > > drivers/usb/typec/altmodes/displayport.c | 9 ++---
> > > > include/drm/drm_connector.h | 5 +++--
> > > > 5 files changed, 23 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_connector.c
> > > > b/drivers/gpu/drm/drm_connector.c
> > > > index a50c82bc2b2f..ad7295597c0f 100644
> > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > @@ -2825,6 +2825,7 @@ struct drm_connector
> > > > *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> > > > /**
> > > > * drm_connector_oob_hotplug_event - Report out-of-band hotplug event
> > > > to connector
> > > > * @connector_fwnode: fwnode_handle to report the event on
> > > > + * @hpd_state: number of data lanes available
> > >
> > > "number"?
> > >
> > > > *
> > > > * On some hardware a hotplug event notification may come from outside
> > > > the display
> > > > * driver / device. An example of this is some USB Type-C setups where
> > > > the hardware
> > > > @@ -2834,7 +2835,8 @@ struct drm_connector
> > > > *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> > > > * This function can be used to report these out-of-band events after
> > > > obtaining
> > > > * a drm_connector reference through calling
> > > > drm_connector_find_by_fwnode().
> > > > */
> > > > -void drm_connector_oob_hotplug_event(struct fwnode_handle
> > > > *connector_fwnode)
> > > > +void drm_connector_oob_hotplug_event(struct fwnode_handle
> > > > *connector_fwnode,
> > > > +bool hpd_state)
> > >
> > > This is a boolean, how can it be a number?
> > >
> >
> > The kerneldoc wasn't appropriately updated as this went from being
> > "number of data lanes" to "the hot plug detect (hpd) state".
> >
> > > And having a "flag" like this is a pain, how do you know what the
> > > parameter really means?
> > >
> >
> > You're right, "state" isn't a boolean property, let's rename it
> > "hpd_high" to clarify it.
>
> "connected" ?
>
I've been trying to find some references to point to, but my
understanding is that in a DisplayPort or HDMI connector/cable you have
a dedicated HPD pin, which when high denotes the sink is alive _and_
EDID can be read.
So in a situation where you have a multifunction USB & DP/HDMI hub where
you connect a display, you might have the USB hub connected to the host
and you might even have your sink connected, but HPD could still be low
until the display is ready to talk to you. So physically everything is
connected, but this property will still be "not connected".
As such I don't think it's appropriate to name it "connected".
Regards,
Bjorn
> >
> > > > {
> > > > struct drm_connector *connector;
> > > >
> > > > @@ -2843,7 +2845,7 @@ void drm_connector_oob_hotplug_event(struct
> > > > fwnode_handle *connector_fwnode)
> > > > return;
> > > >
> > > > if (conn
[Intel-gfx] [PATCH 1/2] drm: Add HPD state to drm_connector_oob_hotplug_event()
In some implementations, such as the Qualcomm platforms, the display
driver has no way to query the current HPD state and as such it's
impossible to distinguish between disconnect and attention events.
Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
state.
Also push the test for unchanged state in the displayport altmode driver
into the i915 driver, to allow other drivers to act upon each update.
Signed-off-by: Bjorn Andersson
---
Note that the Intel driver has only been compile tested with this patch.
drivers/gpu/drm/drm_connector.c | 6 --
drivers/gpu/drm/i915/display/intel_dp.c | 14 +++---
drivers/gpu/drm/i915/i915_drv.h | 3 +++
drivers/usb/typec/altmodes/displayport.c | 9 ++---
include/drm/drm_connector.h | 5 +++--
5 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index a50c82bc2b2f..ad7295597c0f 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2825,6 +2825,7 @@ struct drm_connector *drm_connector_find_by_fwnode(struct
fwnode_handle *fwnode)
/**
* drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
connector
* @connector_fwnode: fwnode_handle to report the event on
+ * @hpd_state: number of data lanes available
*
* On some hardware a hotplug event notification may come from outside the
display
* driver / device. An example of this is some USB Type-C setups where the
hardware
@@ -2834,7 +2835,8 @@ struct drm_connector *drm_connector_find_by_fwnode(struct
fwnode_handle *fwnode)
* This function can be used to report these out-of-band events after obtaining
* a drm_connector reference through calling drm_connector_find_by_fwnode().
*/
-void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
+void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
+bool hpd_state)
{
struct drm_connector *connector;
@@ -2843,7 +2845,7 @@ void drm_connector_oob_hotplug_event(struct fwnode_handle
*connector_fwnode)
return;
if (connector->funcs->oob_hotplug_event)
- connector->funcs->oob_hotplug_event(connector);
+ connector->funcs->oob_hotplug_event(connector, hpd_state);
drm_connector_put(connector);
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 146b83916005..00520867d37b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4816,15 +4816,23 @@ static int intel_dp_connector_atomic_check(struct
drm_connector *conn,
return intel_modeset_synced_crtcs(state, conn);
}
-static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
+static void intel_dp_oob_hotplug_event(struct drm_connector *connector, bool
hpd_state)
{
struct intel_encoder *encoder =
intel_attached_encoder(to_intel_connector(connector));
struct drm_i915_private *i915 = to_i915(connector->dev);
+ bool need_work = false;
spin_lock_irq(>irq_lock);
- i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
+ if (hpd_state != i915->hotplug.oob_hotplug_state) {
+ i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
+
+ i915->hotplug.oob_hotplug_state = hpd_state;
+ need_work = true;
+ }
spin_unlock_irq(>irq_lock);
- queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
+
+ if (need_work)
+ queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
}
static const struct drm_connector_funcs intel_dp_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8c1706fd81f9..543ebf1cfcf4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -149,6 +149,9 @@ struct i915_hotplug {
/* Whether or not to count short HPD IRQs in HPD storms */
u8 hpd_short_storm_enabled;
+ /* Last state reported by oob_hotplug_event */
+ bool oob_hotplug_state;
+
/*
* if we get a HPD irq from DP and a HPD irq from non-DP
* the non-DP HPD could block the workqueue on a mode config
diff --git a/drivers/usb/typec/altmodes/displayport.c
b/drivers/usb/typec/altmodes/displayport.c
index c1d8c23baa39..a4596be4d34a 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -59,7 +59,6 @@ struct dp_altmode {
struct typec_displayport_data data;
enum dp_state state;
- bool hpd;
struct mutex lock; /* device lock */
struct work_struct work;
@@ -143,10 +142,7 @@ static int dp_altmode_status_update(struct dp_altmode *dp)
if (!ret)
dp->state = DP_STATE_CONFIGUR
[Intel-gfx] [PATCH 2/2] drm/msm/dp: Implement oob_hotplug_event()
The Qualcomm DisplayPort driver contains traces of the necessary
plumbing to hook up USB HPD, in the form of the dp_hpd module and the
dp_usbpd_cb struct. Use this as basis for implementing the
oob_hotplug_event() callback, by amending the dp_hpd module with the
missing logic.
Overall the solution is similar to what's done downstream, but upstream
all the code to disect the HPD notification lives on the calling side of
drm_connector_oob_hotplug_event().
drm_connector_oob_hotplug_event() performs the lookup of the
drm_connector based on fwnode, hence the need to assign the fwnode in
dp_drm_connector_init().
Signed-off-by: Bjorn Andersson
---
drivers/gpu/drm/msm/dp/dp_display.c | 8
drivers/gpu/drm/msm/dp/dp_display.h | 2 ++
drivers/gpu/drm/msm/dp/dp_drm.c | 10 ++
drivers/gpu/drm/msm/dp/dp_hpd.c | 19 +++
drivers/gpu/drm/msm/dp/dp_hpd.h | 4
5 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 7cc4d21f2091..124a2f794382 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -414,6 +414,13 @@ static int dp_display_usbpd_configure_cb(struct device
*dev)
return dp_display_process_hpd_high(dp);
}
+void dp_display_oob_hotplug_event(struct msm_dp *dp_display, bool hpd_state)
+{
+ struct dp_display_private *dp = container_of(dp_display, struct
dp_display_private, dp_display);
+
+ dp->usbpd->oob_event(dp->usbpd, hpd_state);
+}
+
static int dp_display_usbpd_disconnect_cb(struct device *dev)
{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
@@ -1251,6 +1258,7 @@ static int dp_display_probe(struct platform_device *pdev)
dp->pdev = pdev;
dp->name = "drm_dp";
dp->dp_display.connector_type = desc->connector_type;
+ dp->dp_display.dev = >dev;
rc = dp_init_sub_modules(dp);
if (rc) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index e3adcd578a90..1f856b3bca79 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -11,6 +11,7 @@
#include "disp/msm_disp_snapshot.h"
struct msm_dp {
+ struct device *dev;
struct drm_device *drm_dev;
struct device *codec_dev;
struct drm_bridge *bridge;
@@ -40,5 +41,6 @@ bool dp_display_check_video_test(struct msm_dp *dp_display);
int dp_display_get_test_bpp(struct msm_dp *dp_display);
void dp_display_signal_audio_start(struct msm_dp *dp_display);
void dp_display_signal_audio_complete(struct msm_dp *dp_display);
+void dp_display_oob_hotplug_event(struct msm_dp *dp_display, bool hpd_state);
#endif /* _DP_DISPLAY_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index d4d360d19eba..665568197c49 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -123,6 +123,13 @@ static enum drm_mode_status dp_connector_mode_valid(
return dp_display_validate_mode(dp_disp, mode->clock);
}
+static void dp_oob_hotplug_event(struct drm_connector *connector, bool
hpd_state)
+{
+ struct msm_dp *dp_disp = to_dp_connector(connector)->dp_display;
+
+ dp_display_oob_hotplug_event(dp_disp, hpd_state);
+}
+
static const struct drm_connector_funcs dp_connector_funcs = {
.detect = dp_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
@@ -130,6 +137,7 @@ static const struct drm_connector_funcs dp_connector_funcs
= {
.reset = drm_atomic_helper_connector_reset,
.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+ .oob_hotplug_event = dp_oob_hotplug_event,
};
static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
@@ -160,6 +168,8 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display)
if (ret)
return ERR_PTR(ret);
+ connector->fwnode = fwnode_handle_get(dev_fwnode(dp_display->dev));
+
drm_connector_helper_add(connector, _connector_helper_funcs);
/*
diff --git a/drivers/gpu/drm/msm/dp/dp_hpd.c b/drivers/gpu/drm/msm/dp/dp_hpd.c
index db98a1d431eb..3e62852a18b4 100644
--- a/drivers/gpu/drm/msm/dp/dp_hpd.c
+++ b/drivers/gpu/drm/msm/dp/dp_hpd.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include "dp_hpd.h"
@@ -45,6 +46,23 @@ int dp_hpd_connect(struct dp_usbpd *dp_usbpd, bool hpd)
return rc;
}
+static void dp_hpd_oob_event(struct dp_usbpd *dp_usbpd, bool hpd_state)
+{
+ struct dp_hpd_private *hpd_priv = container_of(dp_usbpd, struct
dp_hpd_private, dp_usbpd);
+
+ DRM_DEBUG_DP("hpd_state: %d connected: %d\n", hpd_state,
dp_usbpd->connected);
+
+ if (!dp_usbpd->connected &&
Re: [Intel-gfx] [PATCH v3 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers
On Fri 01 Oct 10:11 CDT 2021, Sean Paul wrote:
> From: Sean Paul
>
> This patch adds the bindings for the MSM DisplayPort HDCP registers
> which are required to write the HDCP key into the display controller as
> well as the registers to enable HDCP authentication/key
> exchange/encryption.
>
> We'll use a new compatible string for this since the fields are optional.
>
> Cc: Rob Herring
> Cc: Stephen Boyd
> Signed-off-by: Sean Paul
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-s...@poorly.run
> #v1
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-s...@poorly.run
> #v2
>
> Changes in v2:
> -Drop register range names (Stephen)
> -Fix yaml errors (Rob)
> Changes in v3:
> -Add new compatible string for dp-hdcp
> -Add descriptions to reg
> -Add minItems/maxItems to reg
> -Make reg depend on the new hdcp compatible string
> ---
>
> Disclaimer: I really don't know if this is the right way to approach
> this. I tried using examples from other bindings, but feedback would be
> very much welcome on how I could add the optional register ranges.
>
>
> .../bindings/display/msm/dp-controller.yaml | 34 ---
> 1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 64d8d9e5e47a..a176f97b2f4c 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -17,9 +17,10 @@ properties:
>compatible:
> enum:
>- qcom,sc7180-dp
> + - qcom,sc7180-dp-hdcp
>
> - reg:
> -maxItems: 1
> + # See compatible-specific constraints below.
> + reg: true
>
>interrupts:
> maxItems: 1
> @@ -89,6 +90,29 @@ required:
>- power-domains
>- ports
>
> +allOf:
> + - if:
> + properties:
> +compatible:
> + contains:
> +const: qcom,sc7180-dp-hdcp
> +then:
> + properties:
> +reg:
> + minItems: 3
> + maxItems: 3
> + items:
> +- description: Registers for base DP functionality
> +- description: (Optional) Registers for HDCP device key injection
> +- description: (Optional) Registers for HDCP TrustZone
> interaction
> +else:
> + properties:
> +reg:
> + minItems: 1
> + maxItems: 1
> + items:
> +- description: Registers for base DP functionality
> +
> additionalProperties: false
>
> examples:
> @@ -99,8 +123,10 @@ examples:
> #include
>
> displayport-controller@ae9 {
> -compatible = "qcom,sc7180-dp";
> -reg = <0xae9 0x1400>;
> +compatible = "qcom,sc7180-dp-hdcp";
> +reg = <0 0x0ae9 0 0x1400>,
> + <0 0x0aed1000 0 0x174>,
> + <0 0x0aee1000 0 0x2c>;
Forgot to mention, #address-cells = #size-cells = <1> in the example
"environment", so you have to omit the lone 0s in the example to make it
pass the tests.
Regards,
Bjorn
> interrupt-parent = <>;
> interrupts = <12>;
> clocks = < DISP_CC_MDSS_AHB_CLK>,
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
Re: [Intel-gfx] [PATCH v3 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers
On Fri 01 Oct 10:11 CDT 2021, Sean Paul wrote:
> From: Sean Paul
>
> This patch adds the bindings for the MSM DisplayPort HDCP registers
> which are required to write the HDCP key into the display controller as
> well as the registers to enable HDCP authentication/key
> exchange/encryption.
>
> We'll use a new compatible string for this since the fields are optional.
>
I don't think you need a new compatible, in particular since I presume
we should use the hdcp compatible in all platforms? Or is there a reason
for not picking that one?
Instead I suggest that you simply do minItems: 1, maxItems: 3 and detect
which of the two cases you have in the driver.
PS. I hope to get
https://lore.kernel.org/linux-arm-msm/20211001174400.981707-1-bjorn.anders...@linaro.org/
landed before we add these new optional regions...
Regards,
Bjorn
> Cc: Rob Herring
> Cc: Stephen Boyd
> Signed-off-by: Sean Paul
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-s...@poorly.run
> #v1
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-s...@poorly.run
> #v2
>
> Changes in v2:
> -Drop register range names (Stephen)
> -Fix yaml errors (Rob)
> Changes in v3:
> -Add new compatible string for dp-hdcp
> -Add descriptions to reg
> -Add minItems/maxItems to reg
> -Make reg depend on the new hdcp compatible string
> ---
>
> Disclaimer: I really don't know if this is the right way to approach
> this. I tried using examples from other bindings, but feedback would be
> very much welcome on how I could add the optional register ranges.
>
>
> .../bindings/display/msm/dp-controller.yaml | 34 ---
> 1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 64d8d9e5e47a..a176f97b2f4c 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -17,9 +17,10 @@ properties:
>compatible:
> enum:
>- qcom,sc7180-dp
> + - qcom,sc7180-dp-hdcp
>
> - reg:
> -maxItems: 1
> + # See compatible-specific constraints below.
> + reg: true
>
>interrupts:
> maxItems: 1
> @@ -89,6 +90,29 @@ required:
>- power-domains
>- ports
>
> +allOf:
> + - if:
> + properties:
> +compatible:
> + contains:
> +const: qcom,sc7180-dp-hdcp
> +then:
> + properties:
> +reg:
> + minItems: 3
> + maxItems: 3
> + items:
> +- description: Registers for base DP functionality
> +- description: (Optional) Registers for HDCP device key injection
> +- description: (Optional) Registers for HDCP TrustZone
> interaction
> +else:
> + properties:
> +reg:
> + minItems: 1
> + maxItems: 1
> + items:
> +- description: Registers for base DP functionality
> +
> additionalProperties: false
>
> examples:
> @@ -99,8 +123,10 @@ examples:
> #include
>
> displayport-controller@ae9 {
> -compatible = "qcom,sc7180-dp";
> -reg = <0xae9 0x1400>;
> +compatible = "qcom,sc7180-dp-hdcp";
> +reg = <0 0x0ae9 0 0x1400>,
> + <0 0x0aed1000 0 0x174>,
> + <0 0x0aee1000 0 0x2c>;
> interrupt-parent = <>;
> interrupts = <12>;
> clocks = < DISP_CC_MDSS_AHB_CLK>,
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
