| Message ID | 20200923121320.v3.2.Ided0ab0808c4908238bd2eb9ebb6ffb2c9312789@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/4] dp/dp_mst: Add support for sink event notify messages | expand |
On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > For adapters behind an MST hub use the correct AUX channel. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > [sammc@chromium.org: rebased, removing redundant changes] > Signed-off-by: Sam McNally <sammc@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/drm_dp_mst_topology.c | 36 +++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 15b6cc39a754..0d753201adbd 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -2255,6 +2255,9 @@ drm_dp_mst_topology_unlink_port(struct > drm_dp_mst_topology_mgr *mgr, > drm_dp_mst_topology_put_port(port); > } > > +static ssize_t > +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); > + > static struct drm_dp_mst_port * > drm_dp_mst_add_port(struct drm_device *dev, > struct drm_dp_mst_topology_mgr *mgr, > @@ -2271,9 +2274,13 @@ drm_dp_mst_add_port(struct drm_device *dev, > port->port_num = port_number; > port->mgr = mgr; > port->aux.name = "DPMST"; > + mutex_init(&port->aux.hw_mutex); > + mutex_init(&port->aux.cec.lock); You're missing a matching mutex_destroy() for both of these With that fixed: Reviewed-by: Lyude Paul <lyude@redhat.com> > port->aux.dev = dev->dev; > port->aux.is_remote = true; > > + port->aux.transfer = drm_dp_mst_aux_transfer; > + > /* initialize the MST downstream port's AUX crc work queue */ > drm_dp_remote_aux_init(&port->aux); > > @@ -3503,6 +3510,35 @@ static int drm_dp_send_up_ack_reply(struct > drm_dp_mst_topology_mgr *mgr, > return 0; > } > > +static ssize_t > +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) > +{ > + struct drm_dp_mst_port *port = > + container_of(aux, struct drm_dp_mst_port, aux); > + int ret; > + > + switch (msg->request & ~DP_AUX_I2C_MOT) { > + case DP_AUX_NATIVE_WRITE: > + case DP_AUX_I2C_WRITE: > + case DP_AUX_I2C_WRITE_STATUS_UPDATE: > + ret = drm_dp_send_dpcd_write(port->mgr, port, msg->address, > + msg->size, msg->buffer); > + break; > + > + case DP_AUX_NATIVE_READ: > + case DP_AUX_I2C_READ: > + ret = drm_dp_send_dpcd_read(port->mgr, port, msg->address, > + msg->size, msg->buffer); > + break; > + > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; > +} > + > static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8 dp_link_count) > { > if (dp_link_bw == 0 || dp_link_count == 0)
On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > For adapters behind an MST hub use the correct AUX channel. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > [sammc@chromium.org: rebased, removing redundant changes] > Signed-off-by: Sam McNally <sammc@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/drm_dp_mst_topology.c | 36 +++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 15b6cc39a754..0d753201adbd 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -2255,6 +2255,9 @@ drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr, > drm_dp_mst_topology_put_port(port); > } > > +static ssize_t > +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); > + > static struct drm_dp_mst_port * > drm_dp_mst_add_port(struct drm_device *dev, > struct drm_dp_mst_topology_mgr *mgr, > @@ -2271,9 +2274,13 @@ drm_dp_mst_add_port(struct drm_device *dev, > port->port_num = port_number; > port->mgr = mgr; > port->aux.name = "DPMST"; > + mutex_init(&port->aux.hw_mutex); > + mutex_init(&port->aux.cec.lock); > port->aux.dev = dev->dev; > port->aux.is_remote = true; > > + port->aux.transfer = drm_dp_mst_aux_transfer; > + This was supposed to be handled via higher levels checking for is_remote==true. > /* initialize the MST downstream port's AUX crc work queue */ > drm_dp_remote_aux_init(&port->aux); > > @@ -3503,6 +3510,35 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr, > return 0; > } > > +static ssize_t > +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) > +{ > + struct drm_dp_mst_port *port = > + container_of(aux, struct drm_dp_mst_port, aux); > + int ret; > + > + switch (msg->request & ~DP_AUX_I2C_MOT) { > + case DP_AUX_NATIVE_WRITE: > + case DP_AUX_I2C_WRITE: > + case DP_AUX_I2C_WRITE_STATUS_UPDATE: > + ret = drm_dp_send_dpcd_write(port->mgr, port, msg->address, > + msg->size, msg->buffer); That doesn't make sense to me. I2c writes and DPCD writes are definitely not the same thing. aux->transfer is a very low level thing. I don't think it's the correct level of abstraction for sideband. > + break; > + > + case DP_AUX_NATIVE_READ: > + case DP_AUX_I2C_READ: > + ret = drm_dp_send_dpcd_read(port->mgr, port, msg->address, > + msg->size, msg->buffer); > + break; > + > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; > +} > + > static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8 dp_link_count) > { > if (dp_link_bw == 0 || dp_link_count == 0) > -- > 2.28.0.681.g6f77f65b4e-goog > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 01/02/2021 23:13, Ville Syrjälä wrote: > On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote: >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> For adapters behind an MST hub use the correct AUX channel. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> [sammc@chromium.org: rebased, removing redundant changes] >> Signed-off-by: Sam McNally <sammc@chromium.org> >> --- >> >> (no changes since v1) >> >> drivers/gpu/drm/drm_dp_mst_topology.c | 36 +++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c >> index 15b6cc39a754..0d753201adbd 100644 >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c >> @@ -2255,6 +2255,9 @@ drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr, >> drm_dp_mst_topology_put_port(port); >> } >> >> +static ssize_t >> +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); >> + >> static struct drm_dp_mst_port * >> drm_dp_mst_add_port(struct drm_device *dev, >> struct drm_dp_mst_topology_mgr *mgr, >> @@ -2271,9 +2274,13 @@ drm_dp_mst_add_port(struct drm_device *dev, >> port->port_num = port_number; >> port->mgr = mgr; >> port->aux.name = "DPMST"; >> + mutex_init(&port->aux.hw_mutex); >> + mutex_init(&port->aux.cec.lock); >> port->aux.dev = dev->dev; >> port->aux.is_remote = true; >> >> + port->aux.transfer = drm_dp_mst_aux_transfer; >> + > > This was supposed to be handled via higher levels checking for > is_remote==true. Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4 ("drm/dp_mst: Add MST support to DP DPCD R/W functions"). It looks like that commit basically solved what this older patch attempts to do as well. Sam, can you test if it works without this patch? Regards, Hans > >> /* initialize the MST downstream port's AUX crc work queue */ >> drm_dp_remote_aux_init(&port->aux); >> >> @@ -3503,6 +3510,35 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr, >> return 0; >> } >> >> +static ssize_t >> +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) >> +{ >> + struct drm_dp_mst_port *port = >> + container_of(aux, struct drm_dp_mst_port, aux); >> + int ret; >> + >> + switch (msg->request & ~DP_AUX_I2C_MOT) { >> + case DP_AUX_NATIVE_WRITE: >> + case DP_AUX_I2C_WRITE: >> + case DP_AUX_I2C_WRITE_STATUS_UPDATE: >> + ret = drm_dp_send_dpcd_write(port->mgr, port, msg->address, >> + msg->size, msg->buffer); > > That doesn't make sense to me. I2c writes and DPCD writes > are definitely not the same thing. > > aux->transfer is a very low level thing. I don't think it's the > correct level of abstraction for sideband. > >> + break; >> + >> + case DP_AUX_NATIVE_READ: >> + case DP_AUX_I2C_READ: >> + ret = drm_dp_send_dpcd_read(port->mgr, port, msg->address, >> + msg->size, msg->buffer); >> + break; >> + >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + return ret; >> +} >> + >> static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8 dp_link_count) >> { >> if (dp_link_bw == 0 || dp_link_count == 0) >> -- >> 2.28.0.681.g6f77f65b4e-goog >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Thu, 4 Feb 2021 at 21:19, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 01/02/2021 23:13, Ville Syrjälä wrote: > > On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote: > >> From: Hans Verkuil <hans.verkuil@cisco.com> > >> > >> For adapters behind an MST hub use the correct AUX channel. > >> > >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > >> [sammc@chromium.org: rebased, removing redundant changes] > >> Signed-off-by: Sam McNally <sammc@chromium.org> > >> --- > >> > >> (no changes since v1) > >> > >> drivers/gpu/drm/drm_dp_mst_topology.c | 36 +++++++++++++++++++++++++++ > >> 1 file changed, 36 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > >> index 15b6cc39a754..0d753201adbd 100644 > >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c > >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > >> @@ -2255,6 +2255,9 @@ drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr, > >> drm_dp_mst_topology_put_port(port); > >> } > >> > >> +static ssize_t > >> +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); > >> + > >> static struct drm_dp_mst_port * > >> drm_dp_mst_add_port(struct drm_device *dev, > >> struct drm_dp_mst_topology_mgr *mgr, > >> @@ -2271,9 +2274,13 @@ drm_dp_mst_add_port(struct drm_device *dev, > >> port->port_num = port_number; > >> port->mgr = mgr; > >> port->aux.name = "DPMST"; > >> + mutex_init(&port->aux.hw_mutex); > >> + mutex_init(&port->aux.cec.lock); > >> port->aux.dev = dev->dev; > >> port->aux.is_remote = true; > >> > >> + port->aux.transfer = drm_dp_mst_aux_transfer; > >> + > > > > This was supposed to be handled via higher levels checking for > > is_remote==true. > > Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4 > ("drm/dp_mst: Add MST support to DP DPCD R/W functions"). > > It looks like that commit basically solved what this older patch attempts to do > as well. > > Sam, can you test if it works without this patch? It almost just works; drm_dp_cec uses whether aux.transfer is non-null to filter out non-DP connectors. Using aux.is_remote as another signal indicating a DP connector seems plausible. We can drop this patch. Thanks all! > > Regards, > > Hans > > > > >> /* initialize the MST downstream port's AUX crc work queue */ > >> drm_dp_remote_aux_init(&port->aux); > >> > >> @@ -3503,6 +3510,35 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr, > >> return 0; > >> } > >> > >> +static ssize_t > >> +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) > >> +{ > >> + struct drm_dp_mst_port *port = > >> + container_of(aux, struct drm_dp_mst_port, aux); > >> + int ret; > >> + > >> + switch (msg->request & ~DP_AUX_I2C_MOT) { > >> + case DP_AUX_NATIVE_WRITE: > >> + case DP_AUX_I2C_WRITE: > >> + case DP_AUX_I2C_WRITE_STATUS_UPDATE: > >> + ret = drm_dp_send_dpcd_write(port->mgr, port, msg->address, > >> + msg->size, msg->buffer); > > > > That doesn't make sense to me. I2c writes and DPCD writes > > are definitely not the same thing. > > > > aux->transfer is a very low level thing. I don't think it's the > > correct level of abstraction for sideband. > > > >> + break; > >> + > >> + case DP_AUX_NATIVE_READ: > >> + case DP_AUX_I2C_READ: > >> + ret = drm_dp_send_dpcd_read(port->mgr, port, msg->address, > >> + msg->size, msg->buffer); > >> + break; > >> + > >> + default: > >> + ret = -EINVAL; > >> + break; > >> + } > >> + > >> + return ret; > >> +} > >> + > >> static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8 dp_link_count) > >> { > >> if (dp_link_bw == 0 || dp_link_count == 0) > >> -- > >> 2.28.0.681.g6f77f65b4e-goog > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > >
On Fri, Feb 05, 2021 at 04:17:51PM +1100, Sam McNally wrote: > On Thu, 4 Feb 2021 at 21:19, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > > On 01/02/2021 23:13, Ville Syrjälä wrote: > > > On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote: > > >> From: Hans Verkuil <hans.verkuil@cisco.com> > > >> > > >> For adapters behind an MST hub use the correct AUX channel. > > >> > > >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > >> [sammc@chromium.org: rebased, removing redundant changes] > > >> Signed-off-by: Sam McNally <sammc@chromium.org> > > >> --- > > >> > > >> (no changes since v1) > > >> > > >> drivers/gpu/drm/drm_dp_mst_topology.c | 36 +++++++++++++++++++++++++++ > > >> 1 file changed, 36 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > > >> index 15b6cc39a754..0d753201adbd 100644 > > >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > >> @@ -2255,6 +2255,9 @@ drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr, > > >> drm_dp_mst_topology_put_port(port); > > >> } > > >> > > >> +static ssize_t > > >> +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); > > >> + > > >> static struct drm_dp_mst_port * > > >> drm_dp_mst_add_port(struct drm_device *dev, > > >> struct drm_dp_mst_topology_mgr *mgr, > > >> @@ -2271,9 +2274,13 @@ drm_dp_mst_add_port(struct drm_device *dev, > > >> port->port_num = port_number; > > >> port->mgr = mgr; > > >> port->aux.name = "DPMST"; > > >> + mutex_init(&port->aux.hw_mutex); > > >> + mutex_init(&port->aux.cec.lock); > > >> port->aux.dev = dev->dev; > > >> port->aux.is_remote = true; > > >> > > >> + port->aux.transfer = drm_dp_mst_aux_transfer; > > >> + > > > > > > This was supposed to be handled via higher levels checking for > > > is_remote==true. > > > > Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4 > > ("drm/dp_mst: Add MST support to DP DPCD R/W functions"). > > > > It looks like that commit basically solved what this older patch attempts to do > > as well. > > > > Sam, can you test if it works without this patch? > > It almost just works; drm_dp_cec uses whether aux.transfer is non-null > to filter out non-DP connectors. Using aux.is_remote as another signal > indicating a DP connector seems plausible. We can drop this patch. Why would anyone even call this stuff on a non-DP connector? And where did they even get the struct drm_dp_aux to do so? > Thanks all! > > > > Regards, > > > > Hans > > > > > > > >> /* initialize the MST downstream port's AUX crc work queue */ > > >> drm_dp_remote_aux_init(&port->aux); > > >> > > >> @@ -3503,6 +3510,35 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr, > > >> return 0; > > >> } > > >> > > >> +static ssize_t > > >> +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) > > >> +{ > > >> + struct drm_dp_mst_port *port = > > >> + container_of(aux, struct drm_dp_mst_port, aux); > > >> + int ret; > > >> + > > >> + switch (msg->request & ~DP_AUX_I2C_MOT) { > > >> + case DP_AUX_NATIVE_WRITE: > > >> + case DP_AUX_I2C_WRITE: > > >> + case DP_AUX_I2C_WRITE_STATUS_UPDATE: > > >> + ret = drm_dp_send_dpcd_write(port->mgr, port, msg->address, > > >> + msg->size, msg->buffer); > > > > > > That doesn't make sense to me. I2c writes and DPCD writes > > > are definitely not the same thing. > > > > > > aux->transfer is a very low level thing. I don't think it's the > > > correct level of abstraction for sideband. > > > > > >> + break; > > >> + > > >> + case DP_AUX_NATIVE_READ: > > >> + case DP_AUX_I2C_READ: > > >> + ret = drm_dp_send_dpcd_read(port->mgr, port, msg->address, > > >> + msg->size, msg->buffer); > > >> + break; > > >> + > > >> + default: > > >> + ret = -EINVAL; > > >> + break; > > >> + } > > >> + > > >> + return ret; > > >> +} > > >> + > > >> static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8 dp_link_count) > > >> { > > >> if (dp_link_bw == 0 || dp_link_count == 0) > > >> -- > > >> 2.28.0.681.g6f77f65b4e-goog > > >> > > >> _______________________________________________ > > >> dri-devel mailing list > > >> dri-devel@lists.freedesktop.org > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > >
On 05/02/2021 14:24, Ville Syrjälä wrote: > On Fri, Feb 05, 2021 at 04:17:51PM +1100, Sam McNally wrote: >> On Thu, 4 Feb 2021 at 21:19, Hans Verkuil <hverkuil@xs4all.nl> wrote: >>> >>> On 01/02/2021 23:13, Ville Syrjälä wrote: >>>> On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote: >>>>> From: Hans Verkuil <hans.verkuil@cisco.com> >>>>> >>>>> For adapters behind an MST hub use the correct AUX channel. >>>>> >>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >>>>> [sammc@chromium.org: rebased, removing redundant changes] >>>>> Signed-off-by: Sam McNally <sammc@chromium.org> >>>>> --- >>>>> >>>>> (no changes since v1) >>>>> >>>>> drivers/gpu/drm/drm_dp_mst_topology.c | 36 +++++++++++++++++++++++++++ >>>>> 1 file changed, 36 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c >>>>> index 15b6cc39a754..0d753201adbd 100644 >>>>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c >>>>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c >>>>> @@ -2255,6 +2255,9 @@ drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr, >>>>> drm_dp_mst_topology_put_port(port); >>>>> } >>>>> >>>>> +static ssize_t >>>>> +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); >>>>> + >>>>> static struct drm_dp_mst_port * >>>>> drm_dp_mst_add_port(struct drm_device *dev, >>>>> struct drm_dp_mst_topology_mgr *mgr, >>>>> @@ -2271,9 +2274,13 @@ drm_dp_mst_add_port(struct drm_device *dev, >>>>> port->port_num = port_number; >>>>> port->mgr = mgr; >>>>> port->aux.name = "DPMST"; >>>>> + mutex_init(&port->aux.hw_mutex); >>>>> + mutex_init(&port->aux.cec.lock); >>>>> port->aux.dev = dev->dev; >>>>> port->aux.is_remote = true; >>>>> >>>>> + port->aux.transfer = drm_dp_mst_aux_transfer; >>>>> + >>>> >>>> This was supposed to be handled via higher levels checking for >>>> is_remote==true. >>> >>> Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4 >>> ("drm/dp_mst: Add MST support to DP DPCD R/W functions"). >>> >>> It looks like that commit basically solved what this older patch attempts to do >>> as well. >>> >>> Sam, can you test if it works without this patch? >> >> It almost just works; drm_dp_cec uses whether aux.transfer is non-null >> to filter out non-DP connectors. Using aux.is_remote as another signal >> indicating a DP connector seems plausible. We can drop this patch. > > Why would anyone even call this stuff on a non-DP connector? > And where did they even get the struct drm_dp_aux to do so? This check came in with commit 5ce70c799ac2 ("drm_dp_cec: check that aux has a transfer function"). It seems nouveau and amdgpu specific. A better approach would be to fix those drivers to only call these cec functions for DP outputs. I think I moved the test to drm_dp_cec.c primarily for robustness (i.e. do nothing if called for a non-DP output). But that might not be the right approach after all. Regards, Hans > >> Thanks all! >>> >>> Regards, >>> >>> Hans >>> >>>> >>>>> /* initialize the MST downstream port's AUX crc work queue */ >>>>> drm_dp_remote_aux_init(&port->aux); >>>>> >>>>> @@ -3503,6 +3510,35 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr, >>>>> return 0; >>>>> } >>>>> >>>>> +static ssize_t >>>>> +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) >>>>> +{ >>>>> + struct drm_dp_mst_port *port = >>>>> + container_of(aux, struct drm_dp_mst_port, aux); >>>>> + int ret; >>>>> + >>>>> + switch (msg->request & ~DP_AUX_I2C_MOT) { >>>>> + case DP_AUX_NATIVE_WRITE: >>>>> + case DP_AUX_I2C_WRITE: >>>>> + case DP_AUX_I2C_WRITE_STATUS_UPDATE: >>>>> + ret = drm_dp_send_dpcd_write(port->mgr, port, msg->address, >>>>> + msg->size, msg->buffer); >>>> >>>> That doesn't make sense to me. I2c writes and DPCD writes >>>> are definitely not the same thing. >>>> >>>> aux->transfer is a very low level thing. I don't think it's the >>>> correct level of abstraction for sideband. >>>> >>>>> + break; >>>>> + >>>>> + case DP_AUX_NATIVE_READ: >>>>> + case DP_AUX_I2C_READ: >>>>> + ret = drm_dp_send_dpcd_read(port->mgr, port, msg->address, >>>>> + msg->size, msg->buffer); >>>>> + break; >>>>> + >>>>> + default: >>>>> + ret = -EINVAL; >>>>> + break; >>>>> + } >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8 dp_link_count) >>>>> { >>>>> if (dp_link_bw == 0 || dp_link_count == 0) >>>>> -- >>>>> 2.28.0.681.g6f77f65b4e-goog >>>>> >>>>> _______________________________________________ >>>>> dri-devel mailing list >>>>> dri-devel@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>> >>> >
On Fri, Feb 05, 2021 at 02:46:44PM +0100, Hans Verkuil wrote: > On 05/02/2021 14:24, Ville Syrjälä wrote: > > On Fri, Feb 05, 2021 at 04:17:51PM +1100, Sam McNally wrote: > >> On Thu, 4 Feb 2021 at 21:19, Hans Verkuil <hverkuil@xs4all.nl> wrote: > >>> > >>> On 01/02/2021 23:13, Ville Syrjälä wrote: > >>>> On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote: > >>>>> From: Hans Verkuil <hans.verkuil@cisco.com> > >>>>> > >>>>> For adapters behind an MST hub use the correct AUX channel. > >>>>> > >>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > >>>>> [sammc@chromium.org: rebased, removing redundant changes] > >>>>> Signed-off-by: Sam McNally <sammc@chromium.org> > >>>>> --- > >>>>> > >>>>> (no changes since v1) > >>>>> > >>>>> drivers/gpu/drm/drm_dp_mst_topology.c | 36 +++++++++++++++++++++++++++ > >>>>> 1 file changed, 36 insertions(+) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > >>>>> index 15b6cc39a754..0d753201adbd 100644 > >>>>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c > >>>>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > >>>>> @@ -2255,6 +2255,9 @@ drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr, > >>>>> drm_dp_mst_topology_put_port(port); > >>>>> } > >>>>> > >>>>> +static ssize_t > >>>>> +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); > >>>>> + > >>>>> static struct drm_dp_mst_port * > >>>>> drm_dp_mst_add_port(struct drm_device *dev, > >>>>> struct drm_dp_mst_topology_mgr *mgr, > >>>>> @@ -2271,9 +2274,13 @@ drm_dp_mst_add_port(struct drm_device *dev, > >>>>> port->port_num = port_number; > >>>>> port->mgr = mgr; > >>>>> port->aux.name = "DPMST"; > >>>>> + mutex_init(&port->aux.hw_mutex); > >>>>> + mutex_init(&port->aux.cec.lock); > >>>>> port->aux.dev = dev->dev; > >>>>> port->aux.is_remote = true; > >>>>> > >>>>> + port->aux.transfer = drm_dp_mst_aux_transfer; > >>>>> + > >>>> > >>>> This was supposed to be handled via higher levels checking for > >>>> is_remote==true. > >>> > >>> Ah, I suspect this patch can be dropped entirely: it predates commit 2f221a5efed4 > >>> ("drm/dp_mst: Add MST support to DP DPCD R/W functions"). > >>> > >>> It looks like that commit basically solved what this older patch attempts to do > >>> as well. > >>> > >>> Sam, can you test if it works without this patch? > >> > >> It almost just works; drm_dp_cec uses whether aux.transfer is non-null > >> to filter out non-DP connectors. Using aux.is_remote as another signal > >> indicating a DP connector seems plausible. We can drop this patch. > > > > Why would anyone even call this stuff on a non-DP connector? > > And where did they even get the struct drm_dp_aux to do so? > > This check came in with commit 5ce70c799ac2 ("drm_dp_cec: check that aux > has a transfer function"). It seems nouveau and amdgpu specific. I see. > > A better approach would be to fix those drivers to only call these cec > functions for DP outputs. I think I moved the test to drm_dp_cec.c primarily > for robustness (i.e. do nothing if called for a non-DP output). But that > might not be the right approach after all. Shrug. I guess just extending to check is_remote (or maybe there is some other member that's always set?) is a good enough short term solution. Someone may want to have a look at adjusting amdgpu/nouveau to not need it, but who knows how much work that is.
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 15b6cc39a754..0d753201adbd 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2255,6 +2255,9 @@ drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr, drm_dp_mst_topology_put_port(port); } +static ssize_t +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); + static struct drm_dp_mst_port * drm_dp_mst_add_port(struct drm_device *dev, struct drm_dp_mst_topology_mgr *mgr, @@ -2271,9 +2274,13 @@ drm_dp_mst_add_port(struct drm_device *dev, port->port_num = port_number; port->mgr = mgr; port->aux.name = "DPMST"; + mutex_init(&port->aux.hw_mutex); + mutex_init(&port->aux.cec.lock); port->aux.dev = dev->dev; port->aux.is_remote = true; + port->aux.transfer = drm_dp_mst_aux_transfer; + /* initialize the MST downstream port's AUX crc work queue */ drm_dp_remote_aux_init(&port->aux); @@ -3503,6 +3510,35 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr, return 0; } +static ssize_t +drm_dp_mst_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) +{ + struct drm_dp_mst_port *port = + container_of(aux, struct drm_dp_mst_port, aux); + int ret; + + switch (msg->request & ~DP_AUX_I2C_MOT) { + case DP_AUX_NATIVE_WRITE: + case DP_AUX_I2C_WRITE: + case DP_AUX_I2C_WRITE_STATUS_UPDATE: + ret = drm_dp_send_dpcd_write(port->mgr, port, msg->address, + msg->size, msg->buffer); + break; + + case DP_AUX_NATIVE_READ: + case DP_AUX_I2C_READ: + ret = drm_dp_send_dpcd_read(port->mgr, port, msg->address, + msg->size, msg->buffer); + break; + + default: + ret = -EINVAL; + break; + } + + return ret; +} + static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8 dp_link_count) { if (dp_link_bw == 0 || dp_link_count == 0)