Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel
On Fri, Feb 05, 2021 at 04:17:51PM +1100, Sam McNally wrote: > On Thu, 4 Feb 2021 at 21:19, Hans Verkuil 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 > > >> > > >> For adapters behind an MST hub use the correct AUX channel. > > >> > > >> Signed-off-by: Hans Verkuil > > >> [sa...@chromium.org: rebased, removing redundant changes] > > >> Signed-off-by: Sam McNally > > >> --- > > >> > > >> (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(>aux.hw_mutex); > > >> +mutex_init(>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(>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-de...@lists.freedesktop.org > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > -- Ville Syrjälä Intel
Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel
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 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 > > For adapters behind an MST hub use the correct AUX channel. > > Signed-off-by: Hans Verkuil > [sa...@chromium.org: rebased, removing redundant changes] > Signed-off-by: Sam McNally > --- > > (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(>aux.hw_mutex); > +mutex_init(>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(>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-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >
Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel
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 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 > > > > For adapters behind an MST hub use the correct AUX channel. > > > > Signed-off-by: Hans Verkuil > > [sa...@chromium.org: rebased, removing redundant changes] > > Signed-off-by: Sam McNally > > --- > > > > (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(>aux.hw_mutex); > > +mutex_init(>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. -- Ville Syrjälä Intel
Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel
On Thu, 4 Feb 2021 at 21:19, Hans Verkuil 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 > >> > >> For adapters behind an MST hub use the correct AUX channel. > >> > >> Signed-off-by: Hans Verkuil > >> [sa...@chromium.org: rebased, removing redundant changes] > >> Signed-off-by: Sam McNally > >> --- > >> > >> (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(>aux.hw_mutex); > >> +mutex_init(>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(>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-de...@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > >
Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel
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 >> >> For adapters behind an MST hub use the correct AUX channel. >> >> Signed-off-by: Hans Verkuil >> [sa...@chromium.org: rebased, removing redundant changes] >> Signed-off-by: Sam McNally >> --- >> >> (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(>aux.hw_mutex); >> +mutex_init(>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(>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-de...@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel
On Wed, Sep 23, 2020 at 12:13:53PM +1000, Sam McNally wrote: > From: Hans Verkuil > > For adapters behind an MST hub use the correct AUX channel. > > Signed-off-by: Hans Verkuil > [sa...@chromium.org: rebased, removing redundant changes] > Signed-off-by: Sam McNally > --- > > (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(>aux.hw_mutex); > + mutex_init(>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(>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-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel
Re: [PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel
On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote: > From: Hans Verkuil > > For adapters behind an MST hub use the correct AUX channel. > > Signed-off-by: Hans Verkuil > [sa...@chromium.org: rebased, removing redundant changes] > Signed-off-by: Sam McNally > --- > > (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(>aux.hw_mutex); > + mutex_init(>aux.cec.lock); You're missing a matching mutex_destroy() for both of these With that fixed: Reviewed-by: Lyude Paul > 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(>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) -- Sincerely, Lyude Paul (she/her) Software Engineer at Red Hat Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've asked me a question, are waiting for a review/merge on a patch, etc. and I haven't responded in a while, please feel free to send me another email to check on my status. I don't bite!
[PATCH v3 2/4] drm_dp_mst_topology: use correct AUX channel
From: Hans Verkuil For adapters behind an MST hub use the correct AUX channel. Signed-off-by: Hans Verkuil [sa...@chromium.org: rebased, removing redundant changes] Signed-off-by: Sam McNally --- (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(>aux.hw_mutex); + mutex_init(>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(>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) -- 2.28.0.681.g6f77f65b4e-goog