Re: [PATCH v3 4/4] drm_dp_cec: add MST support
On Thu, 4 Feb 2021 at 21:42, Hans Verkuil wrote:
>
> On 23/09/2020 04:13, Sam McNally wrote:
> > With DP v2.0 errata E5, CEC tunneling can be supported through an MST
> > topology.
> >
> > There are some minor differences for CEC tunneling through an MST
> > topology compared to CEC tunneling to an SST port:
> > - CEC IRQs are delivered via a sink event notify message
> > - CEC-related DPCD registers are accessed via remote DPCD reads and
> > writes.
> >
> > This results in the MST implementation diverging from the existing SST
> > implementation:
> > - sink event notify messages with CEC_IRQ ID set indicate CEC IRQ rather
> > than ESI1
> > - setting edid and handling CEC IRQs, which can be triggered from
> > contexts where locks held preclude HPD handling, are deferred to avoid
> > remote DPCD access which would block until HPD handling is performed
> > or a timeout
> >
> > Register and unregister for all MST connectors, ensuring their
> > drm_dp_aux_cec struct won't be accessed uninitialized.
> >
> > Reviewed-by: Hans Verkuil
> > Signed-off-by: Sam McNally
> > ---
> >
> > Changes in v3:
> > - Fixed whitespace in drm_dp_cec_mst_irq_work()
> > - Moved drm_dp_cec_mst_set_edid_work() with the other set_edid functions
> >
> > Changes in v2:
> > - Used aux->is_remote instead of aux->cec.is_mst, removing the need for
> > the previous patch in the series
> > - Added a defensive check for null edid in the deferred set_edid work,
> > in case the edid is no longer valid at that point
> >
> > drivers/gpu/drm/drm_dp_cec.c | 68 +--
> > drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++
> > include/drm/drm_dp_helper.h | 4 ++
> > 3 files changed, 91 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > index 3ab2609f9ec7..1020b2cffdf0 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
> > @@ -248,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> > if (!aux->transfer)
> > return;
> >
> > + if (aux->is_remote) {
> > + schedule_work(&aux->cec.mst_irq_work);
> > + return;
> > + }
>
> Are these workqueues for cec_irq and cec_set_edid actually needed? They are
> called
> directly from drm_dp_mst_topology.c, and I think they can be handled
> identically to
> a normal, non-remote, aux device.
>
> Avoiding using a workqueue probably also means that there is no need for
> exporting
> drm_dp_mst_topology_get_port_validated and drm_dp_mst_topology_put_port.
>
> Provided that I am not missing something, this should simplify the code quite
> a bit.
Indeed; with commit 9408cc94eb04 ("drm/dp_mst: Handle UP requests
asynchronously") they're unnecessary. This all simplifies quite
nicely.
>
> Regards,
>
> Hans
>
> > mutex_lock(&aux->cec.lock);
> > if (!aux->cec.adap)
> > goto unlock;
> > @@ -276,6 +281,23 @@ static bool drm_dp_cec_cap(struct drm_dp_aux *aux, u8
> > *cec_cap)
> > return true;
> > }
> >
> > +static void drm_dp_cec_mst_irq_work(struct work_struct *work)
> > +{
> > + struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
> > + cec.mst_irq_work);
> > + struct drm_dp_mst_port *port =
> > + container_of(aux, struct drm_dp_mst_port, aux);
> > +
> > + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> > + if (!port)
> > + return;
> > + mutex_lock(&aux->cec.lock);
> > + if (aux->cec.adap)
> > + drm_dp_cec_handle_irq(aux);
> > + mutex_unlock(&aux->cec.lock);
> > + drm_dp_mst_topology_put_port(port);
> > +}
> > +
> > /*
> > * Called if the HPD was low for more than drm_dp_cec_unregister_delay
> > * seconds. This unregisters the CEC adapter.
> > @@ -297,7 +319,8 @@ static void drm_dp_cec_unregister_work(struct
> > work_struct *work)
> > * were unchanged and just update the CEC physical address. Otherwise
> > * unregister the old CEC adapter and create a new one.
> > */
> > -void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> > +static void drm_dp_cec_handle_set_edid(struct drm_dp_aux *aux,
> > +const struct edid *edid)
> > {
> > struct drm_connector *connector = aux->cec.connector;
> > u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
> > @@ -306,10 +329,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> > struct edid *edid)
> > unsigned int num_las = 1;
> > u8 cap;
> >
> > - /* No transfer function was set, so not a DP connector */
> > - if (!aux->transfer)
> > - return;
> > -
> > #ifndef CONFIG_MEDIA_CEC_RC
> > /*
> >* CEC_C
Re: [PATCH v3 4/4] drm_dp_cec: add MST support
On 23/09/2020 04:13, Sam McNally wrote:
> With DP v2.0 errata E5, CEC tunneling can be supported through an MST
> topology.
>
> There are some minor differences for CEC tunneling through an MST
> topology compared to CEC tunneling to an SST port:
> - CEC IRQs are delivered via a sink event notify message
> - CEC-related DPCD registers are accessed via remote DPCD reads and
> writes.
>
> This results in the MST implementation diverging from the existing SST
> implementation:
> - sink event notify messages with CEC_IRQ ID set indicate CEC IRQ rather
> than ESI1
> - setting edid and handling CEC IRQs, which can be triggered from
> contexts where locks held preclude HPD handling, are deferred to avoid
> remote DPCD access which would block until HPD handling is performed
> or a timeout
>
> Register and unregister for all MST connectors, ensuring their
> drm_dp_aux_cec struct won't be accessed uninitialized.
>
> Reviewed-by: Hans Verkuil
> Signed-off-by: Sam McNally
> ---
>
> Changes in v3:
> - Fixed whitespace in drm_dp_cec_mst_irq_work()
> - Moved drm_dp_cec_mst_set_edid_work() with the other set_edid functions
>
> Changes in v2:
> - Used aux->is_remote instead of aux->cec.is_mst, removing the need for
> the previous patch in the series
> - Added a defensive check for null edid in the deferred set_edid work,
> in case the edid is no longer valid at that point
>
> drivers/gpu/drm/drm_dp_cec.c | 68 +--
> drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++
> include/drm/drm_dp_helper.h | 4 ++
> 3 files changed, 91 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> index 3ab2609f9ec7..1020b2cffdf0 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
> @@ -248,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> if (!aux->transfer)
> return;
>
> + if (aux->is_remote) {
> + schedule_work(&aux->cec.mst_irq_work);
> + return;
> + }
Are these workqueues for cec_irq and cec_set_edid actually needed? They are
called
directly from drm_dp_mst_topology.c, and I think they can be handled
identically to
a normal, non-remote, aux device.
Avoiding using a workqueue probably also means that there is no need for
exporting
drm_dp_mst_topology_get_port_validated and drm_dp_mst_topology_put_port.
Provided that I am not missing something, this should simplify the code quite a
bit.
Regards,
Hans
> mutex_lock(&aux->cec.lock);
> if (!aux->cec.adap)
> goto unlock;
> @@ -276,6 +281,23 @@ static bool drm_dp_cec_cap(struct drm_dp_aux *aux, u8
> *cec_cap)
> return true;
> }
>
> +static void drm_dp_cec_mst_irq_work(struct work_struct *work)
> +{
> + struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
> + cec.mst_irq_work);
> + struct drm_dp_mst_port *port =
> + container_of(aux, struct drm_dp_mst_port, aux);
> +
> + port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
> + if (!port)
> + return;
> + mutex_lock(&aux->cec.lock);
> + if (aux->cec.adap)
> + drm_dp_cec_handle_irq(aux);
> + mutex_unlock(&aux->cec.lock);
> + drm_dp_mst_topology_put_port(port);
> +}
> +
> /*
> * Called if the HPD was low for more than drm_dp_cec_unregister_delay
> * seconds. This unregisters the CEC adapter.
> @@ -297,7 +319,8 @@ static void drm_dp_cec_unregister_work(struct work_struct
> *work)
> * were unchanged and just update the CEC physical address. Otherwise
> * unregister the old CEC adapter and create a new one.
> */
> -void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> +static void drm_dp_cec_handle_set_edid(struct drm_dp_aux *aux,
> +const struct edid *edid)
> {
> struct drm_connector *connector = aux->cec.connector;
> u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
> @@ -306,10 +329,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> struct edid *edid)
> unsigned int num_las = 1;
> u8 cap;
>
> - /* No transfer function was set, so not a DP connector */
> - if (!aux->transfer)
> - return;
> -
> #ifndef CONFIG_MEDIA_CEC_RC
> /*
>* CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> @@ -320,6 +339,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> struct edid *edid)
>*/
> cec_caps &= ~CEC_CAP_RC;
> #endif
> + cancel_work_sync(&aux->cec.mst_irq_work);
> cancel_delayed_work_sync(&aux->cec.unregister_work);
>
> mutex_lock(&aux->cec.lock);
> @@ -375,8 +395,40 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *
[PATCH v3 4/4] drm_dp_cec: add MST support
With DP v2.0 errata E5, CEC tunneling can be supported through an MST
topology.
There are some minor differences for CEC tunneling through an MST
topology compared to CEC tunneling to an SST port:
- CEC IRQs are delivered via a sink event notify message
- CEC-related DPCD registers are accessed via remote DPCD reads and
writes.
This results in the MST implementation diverging from the existing SST
implementation:
- sink event notify messages with CEC_IRQ ID set indicate CEC IRQ rather
than ESI1
- setting edid and handling CEC IRQs, which can be triggered from
contexts where locks held preclude HPD handling, are deferred to avoid
remote DPCD access which would block until HPD handling is performed
or a timeout
Register and unregister for all MST connectors, ensuring their
drm_dp_aux_cec struct won't be accessed uninitialized.
Reviewed-by: Hans Verkuil
Signed-off-by: Sam McNally
---
Changes in v3:
- Fixed whitespace in drm_dp_cec_mst_irq_work()
- Moved drm_dp_cec_mst_set_edid_work() with the other set_edid functions
Changes in v2:
- Used aux->is_remote instead of aux->cec.is_mst, removing the need for
the previous patch in the series
- Added a defensive check for null edid in the deferred set_edid work,
in case the edid is no longer valid at that point
drivers/gpu/drm/drm_dp_cec.c | 68 +--
drivers/gpu/drm/drm_dp_mst_topology.c | 24 ++
include/drm/drm_dp_helper.h | 4 ++
3 files changed, 91 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
index 3ab2609f9ec7..1020b2cffdf0 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
@@ -248,6 +249,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
if (!aux->transfer)
return;
+ if (aux->is_remote) {
+ schedule_work(&aux->cec.mst_irq_work);
+ return;
+ }
mutex_lock(&aux->cec.lock);
if (!aux->cec.adap)
goto unlock;
@@ -276,6 +281,23 @@ static bool drm_dp_cec_cap(struct drm_dp_aux *aux, u8
*cec_cap)
return true;
}
+static void drm_dp_cec_mst_irq_work(struct work_struct *work)
+{
+ struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
+ cec.mst_irq_work);
+ struct drm_dp_mst_port *port =
+ container_of(aux, struct drm_dp_mst_port, aux);
+
+ port = drm_dp_mst_topology_get_port_validated(port->mgr, port);
+ if (!port)
+ return;
+ mutex_lock(&aux->cec.lock);
+ if (aux->cec.adap)
+ drm_dp_cec_handle_irq(aux);
+ mutex_unlock(&aux->cec.lock);
+ drm_dp_mst_topology_put_port(port);
+}
+
/*
* Called if the HPD was low for more than drm_dp_cec_unregister_delay
* seconds. This unregisters the CEC adapter.
@@ -297,7 +319,8 @@ static void drm_dp_cec_unregister_work(struct work_struct
*work)
* were unchanged and just update the CEC physical address. Otherwise
* unregister the old CEC adapter and create a new one.
*/
-void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
+static void drm_dp_cec_handle_set_edid(struct drm_dp_aux *aux,
+ const struct edid *edid)
{
struct drm_connector *connector = aux->cec.connector;
u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
@@ -306,10 +329,6 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
struct edid *edid)
unsigned int num_las = 1;
u8 cap;
- /* No transfer function was set, so not a DP connector */
- if (!aux->transfer)
- return;
-
#ifndef CONFIG_MEDIA_CEC_RC
/*
* CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
@@ -320,6 +339,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
struct edid *edid)
*/
cec_caps &= ~CEC_CAP_RC;
#endif
+ cancel_work_sync(&aux->cec.mst_irq_work);
cancel_delayed_work_sync(&aux->cec.unregister_work);
mutex_lock(&aux->cec.lock);
@@ -375,8 +395,40 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
struct edid *edid)
unlock:
mutex_unlock(&aux->cec.lock);
}
+
+void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
+{
+ /* No transfer function was set, so not a DP connector */
+ if (!aux->transfer)
+ return;
+
+ if (aux->is_remote)
+ schedule_work(&aux->cec.mst_set_edid_work);
+ else
+ drm_dp_cec_handle_set_edid(aux, edid);
+}
EXPORT_SYMBOL(drm_dp_cec_set_edid);
+static void drm_dp_cec_mst_set_edid_work(struct work_struct *work)
+{
+ struct drm_dp_aux *aux =
+ container_of(work, struct drm_dp_aux, cec.mst_set_edid_work);
+ struct drm_dp
