Re: [Intel-gfx] [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
On Tue, 8 Sep 2020 at 18:41, Hans Verkuil wrote:
>
> On 01/09/2020 08:22, Sam McNally wrote:
> > From: Hans Verkuil
> >
> > Signed-off-by: Hans Verkuil
> > [sa...@chromium.org:
> > - rebased
> > - removed polling-related changes
> > - moved the calls to drm_dp_cec_(un)set_edid() into the next patch
> > ]
> > Signed-off-by: Sam McNally
> > ---
> >
> > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
> > drivers/gpu/drm/drm_dp_cec.c | 22 ++-
> > drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
> > include/drm/drm_dp_helper.h | 6 +++--
> > 5 files changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index 461fa4da0a34..6e7075893ec9 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > @@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct
> > amdgpu_display_manager *dm,
> >
> > drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
> > drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> > - &aconnector->base);
> > + &aconnector->base, false);
> >
> > if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> > return;
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 3ab2609f9ec7..04ab7b88055c 100644
> > --- a/drivers/gpu/drm/drm_dp_cec.c
> > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > @@ -14,6 +14,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * Unfortunately it turns out that we have a chicken-and-egg situation
> > @@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> > struct edid *edid)
> > if (aux->cec.adap) {
> > if (aux->cec.adap->capabilities == cec_caps &&
> > aux->cec.adap->available_log_addrs == num_las) {
> > - /* Unchanged, so just set the phys addr */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > goto unlock;
> > }
> > /*
> > @@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux,
> > const struct edid *edid)
> > if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
> > cec_delete_adapter(aux->cec.adap);
> > aux->cec.adap = NULL;
> > - } else {
> > - /*
> > - * Update the phys addr for the new CEC adapter. When called
> > - * from drm_dp_cec_register_connector() edid == NULL, so in
> > - * that case the phys addr is just invalidated.
> > - */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > }
> > unlock:
> > + /*
> > + * Update the phys addr for the new CEC adapter. When called
> > + * from drm_dp_cec_register_connector() edid == NULL, so in
> > + * that case the phys addr is just invalidated.
> > + */
>
> The comment is no longer in sync with the code: if EDID == NULL, then
> nothing is done due to the edid check in the 'if' below.
>
> > + if (aux->cec.adap && edid) {
>
> I think this should just be: if (aux->cec.adap)
>
> Also, the {} aren't necessary here.
>
> > + cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > + }
> > mutex_unlock(&aux->cec.lock);
> > }
> > EXPORT_SYMBOL(drm_dp_cec_set_edid);
>
> Frankly, the changes to this function should be dropped completely, from
> what I can see they are not necessary. It was done in my original patch
> because of the way I handled mst, but you did it differently (and I think
> better), so these changes are no longer needed.
>
> I know I am actually commenting on my old patch, but that patch was from a
> work-in-progress git branch and was never meant as a 'proper' patch.
>
> However, what complicates matters is that after digging a bit more I
> discovered
> that commit 732300154980 ("drm: Do not call drm_dp_cec_set_edid() while
> registering
> DP con
Re: [Intel-gfx] [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
On Wed, 2 Sep 2020 at 04:12, Lyude Paul wrote:
>
> Super minor nitpicks:
>
> On Tue, 2020-09-01 at 16:22 +1000, Sam McNally wrote:
> > From: Hans Verkuil
> >
> > Signed-off-by: Hans Verkuil
> > [sa...@chromium.org:
> > - rebased
> > - removed polling-related changes
> > - moved the calls to drm_dp_cec_(un)set_edid() into the next patch
> > ]
> > Signed-off-by: Sam McNally
> > ---
> >
> > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
> > drivers/gpu/drm/drm_dp_cec.c | 22 ++-
> > drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
> > include/drm/drm_dp_helper.h | 6 +++--
> > 5 files changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index 461fa4da0a34..6e7075893ec9 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > @@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct
> > amdgpu_display_manager *dm,
> >
> > drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
> > drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> > - &aconnector->base);
> > + &aconnector->base, false);
> >
> > if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> > return;
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 3ab2609f9ec7..04ab7b88055c 100644
> > --- a/drivers/gpu/drm/drm_dp_cec.c
> > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > @@ -14,6 +14,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * Unfortunately it turns out that we have a chicken-and-egg situation
> > @@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> > struct edid *edid)
> > if (aux->cec.adap) {
> > if (aux->cec.adap->capabilities == cec_caps &&
> > aux->cec.adap->available_log_addrs == num_las) {
> > - /* Unchanged, so just set the phys addr */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > goto unlock;
> > }
>
> May as well drop the braces here
>
> > /*
> > @@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> > struct edid *edid)
> > if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
> > cec_delete_adapter(aux->cec.adap);
> > aux->cec.adap = NULL;
> > - } else {
> > - /*
> > - * Update the phys addr for the new CEC adapter. When called
> > - * from drm_dp_cec_register_connector() edid == NULL, so in
> > - * that case the phys addr is just invalidated.
> > - */
> > - cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > }
> > unlock:
> > + /*
> > + * Update the phys addr for the new CEC adapter. When called
> > + * from drm_dp_cec_register_connector() edid == NULL, so in
> > + * that case the phys addr is just invalidated.
> > + */
> > + if (aux->cec.adap && edid) {
> > + cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> > + }
>
> And here
>
> > mutex_unlock(&aux->cec.lock);
> > }
> > EXPORT_SYMBOL(drm_dp_cec_set_edid);
> > @@ -418,6 +418,7 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> > * drm_dp_cec_register_connector() - register a new connector
> > * @aux: DisplayPort AUX channel
> > * @connector: drm connector
> > + * @is_mst: set to true if this is an MST branch
> > *
> > * A new connector was registered with associated CEC adapter name and
> > * CEC adapter parent device. After registering the name and parent
> > @@ -425,12 +426,13 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
> > * CEC and to register a CEC adapter if that is the case.
> > */
> > void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> > -struct drm_connector *connector)
> > +struct drm_connector *connector,
[Intel-gfx] [PATCH 4/5] drm_dp_cec: add plumbing in preparation for MST support
From: Hans Verkuil
Signed-off-by: Hans Verkuil
[sa...@chromium.org:
- rebased
- removed polling-related changes
- moved the calls to drm_dp_cec_(un)set_edid() into the next patch
]
Signed-off-by: Sam McNally
---
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
drivers/gpu/drm/drm_dp_cec.c | 22 ++-
drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
include/drm/drm_dp_helper.h | 6 +++--
5 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 461fa4da0a34..6e7075893ec9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -419,7 +419,7 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
drm_dp_aux_init(&aconnector->dm_dp_aux.aux);
drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
- &aconnector->base);
+ &aconnector->base, false);
if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
return;
diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
index 3ab2609f9ec7..04ab7b88055c 100644
--- a/drivers/gpu/drm/drm_dp_cec.c
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
/*
* Unfortunately it turns out that we have a chicken-and-egg situation
@@ -338,8 +339,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
struct edid *edid)
if (aux->cec.adap) {
if (aux->cec.adap->capabilities == cec_caps &&
aux->cec.adap->available_log_addrs == num_las) {
- /* Unchanged, so just set the phys addr */
- cec_s_phys_addr_from_edid(aux->cec.adap, edid);
goto unlock;
}
/*
@@ -364,15 +363,16 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
struct edid *edid)
if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
cec_delete_adapter(aux->cec.adap);
aux->cec.adap = NULL;
- } else {
- /*
-* Update the phys addr for the new CEC adapter. When called
-* from drm_dp_cec_register_connector() edid == NULL, so in
-* that case the phys addr is just invalidated.
-*/
- cec_s_phys_addr_from_edid(aux->cec.adap, edid);
}
unlock:
+ /*
+* Update the phys addr for the new CEC adapter. When called
+* from drm_dp_cec_register_connector() edid == NULL, so in
+* that case the phys addr is just invalidated.
+*/
+ if (aux->cec.adap && edid) {
+ cec_s_phys_addr_from_edid(aux->cec.adap, edid);
+ }
mutex_unlock(&aux->cec.lock);
}
EXPORT_SYMBOL(drm_dp_cec_set_edid);
@@ -418,6 +418,7 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
* drm_dp_cec_register_connector() - register a new connector
* @aux: DisplayPort AUX channel
* @connector: drm connector
+ * @is_mst: set to true if this is an MST branch
*
* A new connector was registered with associated CEC adapter name and
* CEC adapter parent device. After registering the name and parent
@@ -425,12 +426,13 @@ EXPORT_SYMBOL(drm_dp_cec_unset_edid);
* CEC and to register a CEC adapter if that is the case.
*/
void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
- struct drm_connector *connector)
+ struct drm_connector *connector, bool is_mst)
{
WARN_ON(aux->cec.adap);
if (WARN_ON(!aux->transfer))
return;
aux->cec.connector = connector;
+ aux->cec.is_mst = is_mst;
INIT_DELAYED_WORK(&aux->cec.unregister_work,
drm_dp_cec_unregister_work);
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 82b9de274f65..744cb55572f9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6261,7 +6261,7 @@ intel_dp_connector_register(struct drm_connector
*connector)
intel_dp->aux.dev = connector->kdev;
ret = drm_dp_aux_register(&intel_dp->aux);
if (!ret)
- drm_dp_cec_register_connector(&intel_dp->aux, connector);
+ drm_dp_cec_register_connector(&intel_dp->aux, connector, false);
return ret;
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 49dd0cbc33
[Intel-gfx] [PATCH] drm/dp_mst: Support remote i2c writes
For DP MST outputs, the i2c device currently only supports transfers
that can be implemented using remote i2c reads. Such transfers must
consist of zero or more write transactions followed by one read
transaction. DDC/CI commands require standalone write transactions and
hence aren't supported.
Since each remote i2c write is handled as a separate transfer, remote
i2c writes can support transfers consisting of write transactions, where
all but the last have I2C_M_STOP set. According to the DDC/CI 1.1
standard, DDC/CI commands only require a single write or read
transaction in a transfer, so this is sufficient.
For i2c transfers meeting the above criteria, generate and send a remote
i2c write message for each transaction. Add the trivial remote i2c write
reply parsing support so remote i2c write acks bubble up correctly.
Signed-off-by: Sam McNally
---
drivers/gpu/drm/drm_dp_mst_topology.c | 106 ++
1 file changed, 90 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 09b32289497e..1ac874e4e7a1 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -961,6 +961,8 @@ static bool drm_dp_sideband_parse_reply(struct
drm_dp_sideband_msg_rx *raw,
return drm_dp_sideband_parse_remote_dpcd_write(raw, msg);
case DP_REMOTE_I2C_READ:
return drm_dp_sideband_parse_remote_i2c_read_ack(raw, msg);
+ case DP_REMOTE_I2C_WRITE:
+ return true; /* since there's nothing to parse */
case DP_ENUM_PATH_RESOURCES:
return drm_dp_sideband_parse_enum_path_resources_ack(raw, msg);
case DP_ALLOCATE_PAYLOAD:
@@ -5326,29 +5328,29 @@ static bool remote_i2c_read_ok(const struct i2c_msg
msgs[], int num)
msgs[num - 1].len <= 0xff;
}
-/* I2C device */
-static int drm_dp_mst_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg
*msgs,
- int num)
+static bool remote_i2c_write_ok(const struct i2c_msg msgs[], int num)
+{
+ int i;
+
+ for (i = 0; i < num - 1; i++) {
+ if (msgs[i].flags & I2C_M_RD || !(msgs[i].flags & I2C_M_STOP) ||
+ msgs[i].len > 0xff)
+ return false;
+ }
+
+ return !(msgs[num - 1].flags & I2C_M_RD) && msgs[num - 1].len <= 0xff;
+}
+
+static int drm_dp_mst_i2c_read(struct drm_dp_mst_branch *mstb,
+ struct drm_dp_mst_port *port,
+ struct i2c_msg *msgs, int num)
{
- struct drm_dp_aux *aux = adapter->algo_data;
- struct drm_dp_mst_port *port = container_of(aux, struct
drm_dp_mst_port, aux);
- struct drm_dp_mst_branch *mstb;
struct drm_dp_mst_topology_mgr *mgr = port->mgr;
unsigned int i;
struct drm_dp_sideband_msg_req_body msg;
struct drm_dp_sideband_msg_tx *txmsg = NULL;
int ret;
- mstb = drm_dp_mst_topology_get_mstb_validated(mgr, port->parent);
- if (!mstb)
- return -EREMOTEIO;
-
- if (!remote_i2c_read_ok(msgs, num)) {
- DRM_DEBUG_KMS("Unsupported I2C transaction for MST device\n");
- ret = -EIO;
- goto out;
- }
-
memset(&msg, 0, sizeof(msg));
msg.req_type = DP_REMOTE_I2C_READ;
msg.u.i2c_read.num_transactions = num - 1;
@@ -5389,6 +5391,78 @@ static int drm_dp_mst_i2c_xfer(struct i2c_adapter
*adapter, struct i2c_msg *msgs
}
out:
kfree(txmsg);
+ return ret;
+}
+
+static int drm_dp_mst_i2c_write(struct drm_dp_mst_branch *mstb,
+ struct drm_dp_mst_port *port,
+ struct i2c_msg *msgs, int num)
+{
+ struct drm_dp_mst_topology_mgr *mgr = port->mgr;
+ unsigned int i;
+ struct drm_dp_sideband_msg_req_body msg;
+ struct drm_dp_sideband_msg_tx *txmsg = NULL;
+ int ret;
+
+ txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
+ if (!txmsg) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ for (i = 0; i < num; i++) {
+ memset(&msg, 0, sizeof(msg));
+ msg.req_type = DP_REMOTE_I2C_WRITE;
+ msg.u.i2c_write.port_number = port->port_num;
+ msg.u.i2c_write.write_i2c_device_id = msgs[i].addr;
+ msg.u.i2c_write.num_bytes = msgs[i].len;
+ msg.u.i2c_write.bytes = msgs[i].buf;
+
+ memset(txmsg, 0, sizeof(*txmsg));
+ txmsg->dst = mstb;
+
+ drm_dp_encode_sideband_req(&msg, txmsg);
+ drm_dp_queue_down_tx(mgr, txmsg);
+
+ ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
+ if (ret > 0) {
+ if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) {
