Re: [Intel-gfx] [PATCH v4 2/5] drm: Add HPD state to drm_connector_oob_hotplug_event()
On Mon, May 02, 2022 at 09:53:13AM -0700, 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
Acked-by: Heikki Krogerus
> ---
>
> 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(&i915->irq_lock);
> - i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> + if (hpd_high != test_bit(hpd_pin,
> &i915->hotplug.oob_hotplug_last_state)) {
> + i915->hotplug.event_bits |= BIT(hpd_pin);
> +
> + __assign_bit(hpd_pin, &i915->hotplug.oob_hotplug_last_state,
> hpd_high);
> + need_work = true;
> + }
> spin_unlock_irq(&i915->irq_lock);
> - queue_delayed_work(system_wq, &i915->hotplug.hotplug_work, 0);
> +
> + if (need_work)
> + queue_delayed_work(system_wq, &i915->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 */
>
Re: [Intel-gfx] [PATCH v4 2/5] drm: Add HPD state to drm_connector_oob_hotplug_event()
+Hans
Hans, do you have any comments?
On Mon, May 02, 2022 at 09:53:13AM -0700, 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
> ---
>
> 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(&i915->irq_lock);
> - i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> + if (hpd_high != test_bit(hpd_pin,
> &i915->hotplug.oob_hotplug_last_state)) {
> + i915->hotplug.event_bits |= BIT(hpd_pin);
> +
> + __assign_bit(hpd_pin, &i915->hotplug.oob_hotplug_last_state,
> hpd_high);
> + need_work = true;
> + }
> spin_unlock_irq(&i915->irq_lock);
> - queue_delayed_work(system_wq, &i915->hotplug.hotplug_work, 0);
> +
> + if (need_work)
> + queue_delayed_work(system_wq, &i915->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/dis
Re: [Intel-gfx] [PATCH 1/1] usb: typec: altmodes/displayport: reorder dp_altmode_configured()
On Tue, Oct 05, 2021 at 09:32:57PM -0700, lindsey.stanp...@gmail.com wrote:
> From: Cameron Nemo
>
> A recent commit [1] introduced an unintended behavioral change by
> reordering certain function calls. The sysfs_notify call for
> pin_assignment should only be invoked when the dp_altmode_notify call
> returns 0, and in the dp->data.conf == 0 case.
>
> [1] https://lore.kernel.org/r/20210817215201.795062-8-hdego...@redhat.com
You should refer the commit, not the mail. I think you could also use
the Fixes tag in this case, but then I guess Hans should pick this:
Fixes: fc27e04630e9 ("usb: typec: altmodes/displayport: Make
dp_altmode_notify() more generic")
Either way, this is OK by me, so once you have cleaned the commit
message, please feel free to include my:
Reviewed-by: Heikki Krogerus
> Signed-off-by: Cameron Nemo
> ---
> drivers/usb/typec/altmodes/displayport.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index c1d8c23baa39..a15ae78066e3 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -154,10 +154,17 @@ static int dp_altmode_status_update(struct dp_altmode
> *dp)
>
> static int dp_altmode_configured(struct dp_altmode *dp)
> {
> + int ret;
> +
> sysfs_notify(&dp->alt->dev.kobj, "displayport", "configuration");
> +
> + ret = dp_altmode_notify(dp);
> + if (ret || !dp->data.conf)
> + return ret;
> +
> sysfs_notify(&dp->alt->dev.kobj, "displayport", "pin_assignment");
>
> - return dp_altmode_notify(dp);
> + return 0;
> }
>
> static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf)
> --
> 2.33.0
--
heikki
Re: [Intel-gfx] [PATCH 0/8] drm + usb-type-c: Add support for out-of-band hotplug notification (v3)
On Tue, May 11, 2021 at 10:05:26AM +0300, Heikki Krogerus wrote: > On Wed, May 05, 2021 at 06:24:07PM +0200, Hans de Goede wrote: > > Hi All, > > > > Here is v3 of my patchset making DP over Type-C work on devices where the > > Type-C controller does not drive the HPD pin on the GPU, but instead > > we need to forward HPD events from the Type-C controller to the DRM driver. > > These look good to me. I can also test these next week if needed. I'll > give my Tested-by tag after that if these haven't been taken by > anybody by that. It's almost weird to see console output from the Type-C connector on my good old GPD printed to an actual display :-) At least in my tests, the DP alt mode driver now calls drm_connector_oob_hotplug_event() only when it should. This is pretty cool! FWIW: Tested-by: Heikki Krogerus thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/8] drm + usb-type-c: Add support for out-of-band hotplug notification (v3)
On Wed, May 05, 2021 at 06:24:07PM +0200, Hans de Goede wrote: > Hi All, > > Here is v3 of my patchset making DP over Type-C work on devices where the > Type-C controller does not drive the HPD pin on the GPU, but instead > we need to forward HPD events from the Type-C controller to the DRM driver. These look good to me. I can also test these next week if needed. I'll give my Tested-by tag after that if these haven't been taken by anybody by that. thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 8/8] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
On Wed, May 05, 2021 at 06:24:15PM +0200, Hans de Goede wrote:
> Use the new drm_connector_oob_hotplug_event() functions to let drm/kms
> drivers know about DisplayPort over Type-C hotplug events.
>
> Signed-off-by: Hans de Goede
Reviewed-by: Heikki Krogerus
> ---
> Changes in v3:
> - Only call drm_connector_oob_hotplug_event() on hpd status bit change
> - Adjust for drm_connector_oob_hotplug_event() no longer having a data
> argument
>
> Changes in v2:
> - Add missing depends on DRM to TYPEC_DP_ALTMODE Kconfig entry
> ---
> drivers/usb/typec/altmodes/Kconfig | 1 +
> drivers/usb/typec/altmodes/displayport.c | 23 +++
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/usb/typec/altmodes/Kconfig
> b/drivers/usb/typec/altmodes/Kconfig
> index 60d375e9c3c7..1a6b5e872b0d 100644
> --- a/drivers/usb/typec/altmodes/Kconfig
> +++ b/drivers/usb/typec/altmodes/Kconfig
> @@ -4,6 +4,7 @@ menu "USB Type-C Alternate Mode drivers"
>
> config TYPEC_DP_ALTMODE
> tristate "DisplayPort Alternate Mode driver"
> + depends on DRM
> help
> DisplayPort USB Type-C Alternate Mode allows DisplayPort
> displays and adapters to be attached to the USB Type-C
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index aa669b9cf70e..c1d8c23baa39 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -11,8 +11,10 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> +#include
> #include "displayport.h"
>
> #define DP_HEADER(_dp, ver, cmd) (VDO((_dp)->alt->svid, 1, ver, cmd)
> \
> @@ -57,11 +59,13 @@ struct dp_altmode {
> struct typec_displayport_data data;
>
> enum dp_state state;
> + bool hpd;
>
> struct mutex lock; /* device lock */
> struct work_struct work;
> struct typec_altmode *alt;
> const struct typec_altmode *port;
> + struct fwnode_handle *connector_fwnode;
> };
>
> static int dp_altmode_notify(struct dp_altmode *dp)
> @@ -125,6 +129,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8
> con)
> static int dp_altmode_status_update(struct dp_altmode *dp)
> {
> bool configured = !!DP_CONF_GET_PIN_ASSIGN(dp->data.conf);
> + bool hpd = !!(dp->data.status & DP_STATUS_HPD_STATE);
> u8 con = DP_STATUS_CONNECTION(dp->data.status);
> int ret = 0;
>
> @@ -137,6 +142,11 @@ static int dp_altmode_status_update(struct dp_altmode
> *dp)
> ret = dp_altmode_configure(dp, con);
> if (!ret)
> dp->state = DP_STATE_CONFIGURE;
> + } else {
> + if (dp->hpd != hpd) {
> + drm_connector_oob_hotplug_event(dp->connector_fwnode);
> + dp->hpd = hpd;
> + }
> }
>
> return ret;
> @@ -512,6 +522,7 @@ static const struct attribute_group dp_altmode_group = {
> int dp_altmode_probe(struct typec_altmode *alt)
> {
> const struct typec_altmode *port = typec_altmode_get_partner(alt);
> + struct fwnode_handle *fwnode;
> struct dp_altmode *dp;
> int ret;
>
> @@ -540,6 +551,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
> alt->desc = "DisplayPort";
> alt->ops = &dp_altmode_ops;
>
> + fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */
> + dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0);
> + if (IS_ERR(dp->connector_fwnode))
> + dp->connector_fwnode = NULL;
> +
> typec_altmode_set_drvdata(alt, dp);
>
> dp->state = DP_STATE_ENTER;
> @@ -555,6 +571,13 @@ void dp_altmode_remove(struct typec_altmode *alt)
>
> sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group);
> cancel_work_sync(&dp->work);
> +
> + if (dp->connector_fwnode) {
> + if (dp->hpd)
> + drm_connector_oob_hotplug_event(dp->connector_fwnode);
> +
> + fwnode_handle_put(dp->connector_fwnode);
> + }
> }
> EXPORT_SYMBOL_GPL(dp_altmode_remove);
>
> --
> 2.31.1
--
heikki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] usb: typec: altmodes/displayport: Make dp_altmode_notify() more generic
On Wed, May 05, 2021 at 06:24:14PM +0200, Hans de Goede wrote:
> Make dp_altmode_notify() handle the dp->data.conf == 0 case too,
> rather then having separate code-paths for this in various places
> which call it.
>
> Signed-off-by: Hans de Goede
Reviewed-by: Heikki Krogerus
> ---
> drivers/usb/typec/altmodes/displayport.c | 35 +---
> 1 file changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index b7f094435b00..aa669b9cf70e 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -66,10 +66,17 @@ struct dp_altmode {
>
> static int dp_altmode_notify(struct dp_altmode *dp)
> {
> - u8 state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
> + unsigned long conf;
> + u8 state;
> +
> + if (dp->data.conf) {
> + state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
> + conf = TYPEC_MODAL_STATE(state);
> + } else {
> + conf = TYPEC_STATE_USB;
> + }
>
> - return typec_altmode_notify(dp->alt, TYPEC_MODAL_STATE(state),
> -&dp->data);
> + return typec_altmode_notify(dp->alt, conf, &dp->data);
> }
>
> static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
> @@ -137,21 +144,10 @@ static int dp_altmode_status_update(struct dp_altmode
> *dp)
>
> static int dp_altmode_configured(struct dp_altmode *dp)
> {
> - int ret;
> -
> sysfs_notify(&dp->alt->dev.kobj, "displayport", "configuration");
> -
> - if (!dp->data.conf)
> - return typec_altmode_notify(dp->alt, TYPEC_STATE_USB,
> - &dp->data);
> -
> - ret = dp_altmode_notify(dp);
> - if (ret)
> - return ret;
> -
> sysfs_notify(&dp->alt->dev.kobj, "displayport", "pin_assignment");
>
> - return 0;
> + return dp_altmode_notify(dp);
> }
>
> static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf)
> @@ -172,13 +168,8 @@ static int dp_altmode_configure_vdm(struct dp_altmode
> *dp, u32 conf)
> }
>
> ret = typec_altmode_vdm(dp->alt, header, &conf, 2);
> - if (ret) {
> - if (DP_CONF_GET_PIN_ASSIGN(dp->data.conf))
> - dp_altmode_notify(dp);
> - else
> - typec_altmode_notify(dp->alt, TYPEC_STATE_USB,
> - &dp->data);
> - }
> + if (ret)
> + dp_altmode_notify(dp);
>
> return ret;
> }
> --
> 2.31.1
--
heikki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 8/9] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
Hi Hans,
On Mon, May 03, 2021 at 05:46:46PM +0200, Hans de Goede wrote:
> Use the new drm_connector_find_by_fwnode() and
> drm_connector_oob_hotplug_event() functions to let drm/kms drivers
> know about DisplayPort over Type-C hotplug events.
>
> Signed-off-by: Hans de Goede
> ---
> Changes in v2:
> - Add missing depends on DRM to TYPEC_DP_ALTMODE Kconfig entry
> ---
> drivers/usb/typec/altmodes/Kconfig | 1 +
> drivers/usb/typec/altmodes/displayport.c | 40 +++-
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/altmodes/Kconfig
> b/drivers/usb/typec/altmodes/Kconfig
> index 60d375e9c3c7..1a6b5e872b0d 100644
> --- a/drivers/usb/typec/altmodes/Kconfig
> +++ b/drivers/usb/typec/altmodes/Kconfig
> @@ -4,6 +4,7 @@ menu "USB Type-C Alternate Mode drivers"
>
> config TYPEC_DP_ALTMODE
> tristate "DisplayPort Alternate Mode driver"
> + depends on DRM
> help
> DisplayPort USB Type-C Alternate Mode allows DisplayPort
> displays and adapters to be attached to the USB Type-C
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index aa669b9cf70e..f00dfc5c14b6 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -11,8 +11,10 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> +#include
> #include "displayport.h"
>
> #define DP_HEADER(_dp, ver, cmd) (VDO((_dp)->alt->svid, 1, ver, cmd)
> \
> @@ -62,12 +64,30 @@ struct dp_altmode {
> struct work_struct work;
> struct typec_altmode *alt;
> const struct typec_altmode *port;
> + struct fwnode_handle *connector_fwnode;
> };
>
> +static void dp_altmode_notify_connector(struct dp_altmode *dp)
> +{
> + struct drm_connector_oob_hotplug_event_data data = { };
> + u8 pin_assign = DP_CONF_GET_PIN_ASSIGN(dp->data.conf);
> +
> + data.connected = dp->data.status & DP_STATUS_HPD_STATE;
> + data.orientation = typec_altmode_get_orientation(dp->alt);
> +
> + if (pin_assign & DP_PIN_ASSIGN_DP_ONLY_MASK)
> + data.dp_lanes = 4;
> + else if (pin_assign & DP_PIN_ASSIGN_MULTI_FUNC_MASK)
> + data.dp_lanes = 2;
> +
> + drm_connector_oob_hotplug_event(dp->connector_fwnode, &data);
> +}
> +
> static int dp_altmode_notify(struct dp_altmode *dp)
> {
> unsigned long conf;
> u8 state;
> + int ret;
>
> if (dp->data.conf) {
> state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
> @@ -76,7 +96,12 @@ static int dp_altmode_notify(struct dp_altmode *dp)
> conf = TYPEC_STATE_USB;
> }
>
> - return typec_altmode_notify(dp->alt, conf, &dp->data);
> + ret = typec_altmode_notify(dp->alt, conf, &dp->data);
> + if (ret)
> + return ret;
> +
> + dp_altmode_notify_connector(dp);
That is now going to be always called after configuration, right? The
HPD is not necessarily issued at that point.
I think that should be called from dp_altmode_status_update(), and
probable only if there really is HPD IRQ or HPD State changes... I
think?
> + return 0;
> }
thanks,
--
heikki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification (v2)
On Tue, May 04, 2021 at 05:35:49PM +0200, Hans de Goede wrote:
> Hi,
>
> On 5/4/21 5:10 PM, Heikki Krogerus wrote:
> >> +/**
> >> + * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> >> connector
> >> + * @connector: connector to report the event on
> >> + * @data: data related to the event
> >> + *
> >> + * 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
> >> + * muxes the DisplayPort data and aux-lines but does not pass the altmode
> >> HPD
> >> + * status bit to the GPU's DP HPD pin.
> >> + *
> >> + * 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,
> >> + struct
> >> drm_connector_oob_hotplug_event_data *data)
> >> +{
> >> + struct drm_connector *connector;
> >> +
> >> + connector = drm_connector_find_by_fwnode(connector_fwnode);
> >> + if (IS_ERR(connector))
> >> + return;
> >> +
> >> + if (connector->funcs->oob_hotplug_event)
> >> + connector->funcs->oob_hotplug_event(connector, data);
> >> +
> >> + drm_connector_put(connector);
> >> +}
> >> +EXPORT_SYMBOL(drm_connector_oob_hotplug_event);
> >
> > So it does looks like the "data" parameter is not needed at all:
>
> Well Imre did indicate that having the number of lanes is useful, so
> for the next version I'll drop the orientation but I plan to keep
> the number of lanes if that is ok with you.
>
> Not having passing along this info was one of the reasons why my
> previous attempt at this was nacked, so dropping it all together
> feels wrong.
If you need to pass any information to the function, then you need to
pass all the information that we have. Don't start with abstraction.
First create a dedicated API, and then, only if we really have another
user for it, we can add an abstraction layer that both users can use.
All cases are going to be different. We don't know how the abstraction
/ generalisation should look like before we have at least two real
users (ideally more than two, actually). Right now we can not even say
for sure that generalising the API is even possible.
I would not make a huge deal out of this unless I wasn't myself being
told by guys like Greg KH in the past to drop my attempts to
"generalize" things from the beginning when I only had a single user.
By doing so you'll not only force all ends, the source of the data
(the typec drivers in this case) as well as the consumer (i915), to be
always changed together, it will also confuse things. We are not
always going to be able to tell the lane count for example like we can
with USB Type-C, so i915 can't really rely on that information.
Right now we also don't know what exact details i915 (or what ever GPU
driver) needs. We can only say for sure that some details are going to
be needed. Trying to guess and cherry-pick the details now does not
makes sense because of that reason too.
So just make this API USB Type-C DP Alt Mode specific API first, and
supply it everything we have.
thanks,
--
heikki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/9] drm + usb-type-c: Add support for out-of-band hotplug notification (v2)
On Mon, May 03, 2021 at 05:46:38PM +0200, Hans de Goede wrote: > Hi All, > > Here is v2 of my work on making DP over Type-C work on devices where the > Type-C controller does not drive the HPD pin on the GPU, but instead > we need to forward HPD events from the Type-C controller to the DRM driver. > > Changes in v2: > - Replace the bogus "drm/connector: Make the drm_sysfs connector->kdev > device hold a reference to the connector" patch with: > "drm/connector: Give connector sysfs devices there own device_type" > the new patch is a dep for patch 2/9 see the patches > > - Stop using a class-dev-iter, instead at a global connector list > to drm_connector.c and use that to find the connector by the fwnode, > similar to how we already do this in drm_panel.c and drm_bridge.c > > - Make drm_connector_oob_hotplug_event() take a fwnode pointer as > argument, rather then a drm_connector pointer and let it do the > lookup itself. This allows making drm_connector_find_by_fwnode() a > drm-internal function and avoids code outside the drm subsystem > potentially holding on the a drm_connector reference for a longer > period. > > This series not only touches drm subsys files but it also touches > drivers/usb/typec/altmodes/typec_displayport.c, that file usually > does not see a whole lot of changes. So I believe it would be best > to just merge the entire series through drm-misc, Assuming we can > get an ack from Greg for merging the typec_displayport.c changes > this way. > > ### > > As already mentioned in the v1 cover-letter this series replaces > a previous attempt from quite some time ago. > For anyone interested here are the old (2019!) patches for this: > > https://patchwork.freedesktop.org/patch/288491/ > https://patchwork.freedesktop.org/patch/288493/ > https://patchwork.freedesktop.org/patch/288495/ > > Last time I posted this the biggest change requested was for more info to > be included in the event send to the DRM-subsystem, specifically sending > the following info was requested: > > 1. Which DP connector on the GPU the event is for > 2. How many lanes are available > 3. Connector orientation > > This series is basically an entirely new approach, which no longer > uses the notifier framework at all. Instead the Type-C code looksup > a connector based on a fwnode (this was suggested by Heikki Krogerus) > and then calls a new oob_hotplug_event drm_connector_func directly > on the connector, passing the requested info as argument. > > Info such as the orientation and the number of dp-lanes is now passed > to the drm_connector_oob_hotplug_event() function as requested in the > review of the old code, but nothing is done with it for now. > Using this info falls well outside of my knowledge of the i915 driver > so this is left to a follow-up patch (I will be available to test > patches for this). Thanks for taking care of these! It's really great that you spent the time to do this series. I'm already thinking about what we can add after these are in. I think support for re-configuration, so support for changing the pin-configuration in runtime is going to be needed soon after these. But first things first (sorry, I'm getting ahead of myself). thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification (v2)
> +/**
> + * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> connector
> + * @connector: connector to report the event on
> + * @data: data related to the event
> + *
> + * 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
> + * muxes the DisplayPort data and aux-lines but does not pass the altmode HPD
> + * status bit to the GPU's DP HPD pin.
> + *
> + * 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,
> + struct
> drm_connector_oob_hotplug_event_data *data)
> +{
> + struct drm_connector *connector;
> +
> + connector = drm_connector_find_by_fwnode(connector_fwnode);
> + if (IS_ERR(connector))
> + return;
> +
> + if (connector->funcs->oob_hotplug_event)
> + connector->funcs->oob_hotplug_event(connector, data);
> +
> + drm_connector_put(connector);
> +}
> +EXPORT_SYMBOL(drm_connector_oob_hotplug_event);
So it does looks like the "data" parameter is not needed at all:
void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
{
struct drm_connector *connector;
connector = drm_connector_find_by_fwnode(connector_fwnode);
if (IS_ERR(connector))
return;
if (connector->funcs->oob_hotplug_event)
connector->funcs->oob_hotplug_event(connector);
drm_connector_put(connector);
}
thanks,
--
heikki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries
Hi Andy,
> > +/* NOTE: The connector order must be final before this is called. */
> > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915)
> > +{
> > + struct drm_connector_list_iter conn_iter;
> > + struct drm_device *drm_dev = &i915->drm;
> > + struct device *kdev = &drm_dev->pdev->dev;
> > + struct fwnode_handle *fwnode = NULL;
> > + struct drm_connector *connector;
> > + struct acpi_device *adev;
> > +
> > + drm_connector_list_iter_begin(drm_dev, &conn_iter);
> > + drm_for_each_connector_iter(connector, &conn_iter) {
> > + /* Always getting the next, even when the last was not
> > used. */
> > + fwnode = device_get_next_child_node(kdev, fwnode);
> > + if (!fwnode)
> > + break;
>
> Who is dropping reference counting on fwnode ?
>
> Iām in the middle of a pile of fixes for fwnode refcounting when
> for_each_child or get_next_child is used. So, please double check you drop
> a reference.
Sorry Andy. This patch is from time before the software nodes
implementation of the get_next_child callback handled the ref counting
properly.
Br,
--
heikki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification
On Mon, May 03, 2021 at 04:35:29PM +0200, Hans de Goede wrote:
> Hi,
>
> On 5/3/21 10:00 AM, Heikki Krogerus wrote:
> > Hi Hans,
> >
> > On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote:
> >> +/**
> >> + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data
> >> + *
> >> + * Contains data about out-of-band hotplug events, signalled through
> >> + * drm_connector_oob_hotplug_event().
> >> + */
> >> +struct drm_connector_oob_hotplug_event_data {
> >> + /**
> >> + * @connected: New connected status for the connector.
> >> + */
> >> + bool connected;
> >> + /**
> >> + * @dp_lanes: Number of available displayport lanes, 0 if unknown.
> >> + */
> >> + int dp_lanes;
> >> + /**
> >> + * @orientation: Connector orientation.
> >> + */
> >> + enum typec_orientation orientation;
> >> +};
> >
> > I don't think the orientation is relevant. It will always be "normal"
> > from DP PoW after muxing, no?
>
> That is what I thought to, but during the discussion of my previous attempt
> at this one of the i915 devs mentioned that in some cases the muxes manage
> to swap the lane order when the connector upside-down and at least the
> Intel GPUs can correct for this on the GPU side, so they asked for this
> info to be included.
>
> > I'm also not sure those deatils are enough in the long run. Based on
> > what I've understood from our graphics team guys, for example knowing
> > if multi-function is preferred may be important in some cases.
>
> The current data being passed is just intended as a starting point,
> this is purely a kernel internal API so we can easily add more
> data to the struct. As I mentioned in the cover-letter the current
> oob_hotplug handler which the i915 patch adds to the i915 driver does
> not actually do anything with the data. ATM it is purely there to
> demonstrate that the ability to pass relevant data is there now
> (which was an issue with the previous attempt). I believe the current
> code is fine as a PoC of "pass event data" once GPU drivers actually
> start doing something with the data we can extend or outright replace
> it without issues.
Ah, if there is nothing using that information yet, then just don't
pass it at all for now. As you said, it's kernel internal API, we can
change it later if needed.
> > All of that, and more, is already available in the Configuration VDO
> > Status VDO that the we have negotiated with the DP partner. Both those
> > VDOs are part of struct typec_displayport_data. I think we should
> > simply supply that structure to the DRM code instead of picking those
> > details out of it...
>
> I'm not sure I like the idea of passing the raw VDO, but if the
> DRM folks think that would be useful we can certainly add it.
Why are you against passing all the data that we have? What is the
benefit in picking only certain details out of an object that has a
standard format, and constructing a customised object for those
details instead?
thanks,
--
heikki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification
Hi Hans,
On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote:
> +/**
> + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data
> + *
> + * Contains data about out-of-band hotplug events, signalled through
> + * drm_connector_oob_hotplug_event().
> + */
> +struct drm_connector_oob_hotplug_event_data {
> + /**
> + * @connected: New connected status for the connector.
> + */
> + bool connected;
> + /**
> + * @dp_lanes: Number of available displayport lanes, 0 if unknown.
> + */
> + int dp_lanes;
> + /**
> + * @orientation: Connector orientation.
> + */
> + enum typec_orientation orientation;
> +};
I don't think the orientation is relevant. It will always be "normal"
from DP PoW after muxing, no?
I'm also not sure those deatils are enough in the long run. Based on
what I've understood from our graphics team guys, for example knowing
if multi-function is preferred may be important in some cases.
+Imre.
All of that, and more, is already available in the Configuration VDO
Status VDO that the we have negotiated with the DP partner. Both those
VDOs are part of struct typec_displayport_data. I think we should
simply supply that structure to the DRM code instead of picking those
details out of it...
> /**
> * struct drm_tv_connector_state - TV connector related states
> * @subconnector: selected subconnector
> @@ -1110,6 +1132,15 @@ struct drm_connector_funcs {
>*/
> void (*atomic_print_state)(struct drm_printer *p,
> const struct drm_connector_state *state);
> +
> + /**
> + * @oob_hotplug_event:
> + *
> + * This will get called when a hotplug-event for a drm-connector
> + * has been received from a source outside the display driver / device.
> + */
> + void (*oob_hotplug_event)(struct drm_connector *connector,
> + struct drm_connector_oob_hotplug_event_data
> *data);
So I would not try to generalise this like that. This callback should
be USB Type-C DP altmode specific:
void (*oob_hotplug_event)(struct drm_connector *connector,
struct typec_displayport_data *data);
Or like this if the orientation can really be reversed after muxing:
void (*oob_hotplug_event)(struct drm_connector *connector,
struct typec_altmode *altmode,
struct typec_displayport_data *data);
You can now check the orientation separately with
typec_altmode_get_orientation() if necessary.
thanks,
--
heikki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] USB-C DP mode problem on linux
Hi Andrzej, On Sat, Oct 17, 2020 at 01:34:54PM +0200, Andrzej Kre wrote: > Hi, > > I know that You were involved in working on USB-C DP drivers. > You are my last chance to resolve my issue. > > On my HP laptop I have Intel UHD Graphics 620. > When I'm connecting my 4K monitor to Display Port. It is assigning to > DP-2-2 socket and I have full 3840x2160 with 60.00Hz > But, when I'm connecting the same monitor to the USB-C port, then it is > connecting to the DP-1 socket and the maximum that it can achieve is > 3840x2160 with only 30.00Hz. > But I'm making some trick: I'm connecting the same monitor through HDMI, so > it is connecting to DP-1 socket, and simultaneously I'm connecting USB-C, > and now USB-C is connecting to DP-2-2 socket (because DP-1 is occupied by > HDMI) and I can have full 4K with 60Hz. > Please, help me, how to force USB-C to connect always to DP-2-2 socket? > Or do You know maybe where is the problem? Unfortunately we have no control over the mux in the operating system on Skylakes, at least in USB subsystem. It all happens in firmware. Maybe graphics side can do something. Adding Jani, Imre, Ville and the Intel GFX list. > Thank You in advance > Andy > > Here is some logs: > > Display Port: > > andy@andy-HP:~$ xrandr --current > Screen 0: minimum 320 x 200, current 5760 x 2160, maximum 16384 x 16384 > eDP-1 connected primary 1920x1080+0+0 (normal left inverted right x axis y > axis) 293mm x 165mm >1920x1080 60.05*+ 60.0159.9759.9659.9340.03 >1680x1050 59.9559.88 >1600x1024 60.17 >1400x1050 59.98 >1600x900 59.9959.9459.9559.82 >1280x1024 60.02 >1440x900 59.89 >1400x900 59.9659.88 >1280x960 60.00 >1440x810 60.0059.97 >1368x768 59.8859.85 >1360x768 59.8059.96 >1280x800 59.9959.9759.8159.91 >1152x864 60.00 >1280x720 60.0059.9959.8659.74 >1024x768 60.0460.00 >960x720 60.00 >928x696 60.05 >896x672 60.01 >1024x576 59.9559.9659.9059.82 >960x600 59.9360.00 >960x540 59.9659.9959.6359.82 >800x600 60.0060.3256.25 >840x525 60.0159.88 >864x486 59.9259.57 >800x512 60.17 >700x525 59.98 >800x450 59.9559.82 >640x512 60.02 >720x450 59.89 >700x450 59.9659.88 >640x480 60.0059.94 >720x405 59.5158.99 >684x384 59.8859.85 >680x384 59.8059.96 >640x400 59.8859.98 >576x432 60.06 >640x360 59.8659.8359.8459.32 >512x384 60.00 >512x288 60.0059.92 >480x270 59.6359.82 >400x300 60.3256.34 >432x243 59.9259.57 >320x240 60.05 >360x202 59.5159.13 >320x180 59.8459.32 > DP-1 disconnected (normal left inverted right x axis y axis) > HDMI-1 disconnected (normal left inverted right x axis y axis) > DP-2 disconnected (normal left inverted right x axis y axis) > HDMI-2 disconnected (normal left inverted right x axis y axis) > DP-2-1 disconnected (normal left inverted right x axis y axis) > DP-2-2 connected 3840x2160+1920+0 (normal left inverted right x axis y > axis) 600mm x 340mm >3840x2160 60.00*+ 30.00 >2560x1440 59.95 >1920x1080 60.0059.94 >1600x900 60.00 >1280x1024 60.02 >1280x800 59.81 >1152x864 59.97 >1280x720 60.0059.94 >1024x768 60.00 >800x600 60.32 >720x480 60.0059.94 >640x480 60.0059.94 > DP-2-3 disconnected (normal left inverted right x axis y axis) > > > > USB-C: > > andy@andy-HP:~$ xrandr --current > Screen 0: minimum 320 x 200, current 5760 x 2160, maximum 16384 x 16384 > eDP-1 connected primary 1920x1080+0+0 (normal left inverted right x axis y > axis) 293mm x 165mm >1920x1080 60.05*+ 60.0159.9759.9659.9340.03 >1680x1050 59.9559.88 >1600x1024 60.17 >1400x1050 59.98 >1600x900 59.9959.9459.9559.82 >1280x1024 60.02 >1440x900 59.89 >1400x900 59.9659.88 >1280x960 60.00 >1440x810 60.0059.97 >1368x768 59.8859.85 >1360x768 59.8059.96 >1280x800 59.9959.9759.8159.91 >1152x864 60.00 >1280x720 60.0059.9959.8659.74 >1024x768 60.0460.00 >960x720 60.00 >928x696 60.05 >896x672 60.01 >1024x576 59.9559.9659.9059.82 >960x600 59.9360.00 >960x540 59.9659.9959.6359.82 >800x600 60.0060.3256.25 >840x525 60.0159.88 >864x486 59.9259.57 >800x512
Re: [Intel-gfx] Linux 5.2, usb: typec: Support for Alternate Modes
Hi Matthew, On Thu, Aug 01, 2019 at 01:16:34PM +0100, Matthew Nicholson wrote: > [Resending as plain text email with attachments.] > > Hi, > The kernel version testing I'm testing on is: v5.2.4-arch1 > I have disabled gmd, which seems to struggle with not being able to > configure displays and becomes unresponsive. > I'm running startx and have an xprofile script that is setting the > displays with xrandr. > > *1. dmesg output* > I have attached two copies of dmesg and lsmod ouputs. > Both are for v5.2.4, one set is where ucsi_acpi is blacklisted, > another with no blacklisted modules. > > *2. The exact XPS13 version* > XPS 13 (9370) Developer edition, ships with Ubuntu. > i7-8550U Processor > > *3. BIOS version* > The output from fwupdmgr get-devices is attached. > XPS 13 9370 Thunderbolt Controller: v33.00 > XPS 13 9370 System Firmware: v0.1.10.0 > Synaptics VMM3332 inside Dell WD15/TB16/TB18 wired Dock: v3.10.002 > > At boot the firmware version listed is 1.10.0 > > *Can you unload the UCSI driver to see if it has any effect?* > No changes in being display functionality. I tried to unload and to > blacklist with config file in /etc/modprode.d In that case the problem is not caused by the Type-C drivers. This is more likely a regression in the Thunderbolt drivers, or the graphics drivers. Adding Mika and the graphics guys. Mika is the Thunderbolt maintainer in Linux kernel. He's away now, but he'll be back on Monday. > When ucsi_acpi is not blacked the error message: > > typec_displayport port1-partner.0: failed to enter mode You can ignore that message for now. It is not fatal in this case. It happens because on this platform the embedded controller firmware does not allow the operating system to do anything to the alternate modes besides detecting them, not even enter or exit them (so the firmware handles the alternate modes on its own). The DisplayPort alt mode driver in Linux kernel does not know that, so it tries to enter DisplayPort mode (most likely the firmware has already entered the mode at this point). That attempt fails, and the driver prints the message, but it really is not fatal in any way. I'll see what could be done about that message, but for now you can just ignore it. > is displayed in dmseg and in getty. > > *Are you able to build you own test kernels?* > It is not something that I have done but it is something I should be able to > do. > > A few more details. > The external monitors are detected and listed as available in xrandr. > I can enable one of them at a time, however attempting to enable both > of them will fail. > The returned error message is: > xrandr: Configure crtc 2 failed > > > On Tue, 30 Jul 2019 at 15:27, Heikki Krogerus > wrote: > > > > Hi Matthew, > > > > Copying the respective mailing list. > > > > On Wed, Jul 17, 2019 at 09:22:10AM +0100, Matthew Nicholson wrote: > > > Hi, > > > > > > Thanks for your work on the linux. > > > > > > I am using dell xps13 with a wd15 type-c docking station, on Archlinux. > > > Under kernel version 5.2 (and 5.2.1) I was running into some issue with > > > having the docking station connected to multiple monitors (Only one > > > monitor > > > would work at a time). > > > I tried to get the monitors working under X/xrandr and wayland/gnome. > > > The issue is not present after downgrading back to linux 5.1.7. > > > > > > I am wondering what I should do to report this or help testing. > > > > I'm going to need some details about your platform: > > > > 1. dmesg output > > 2. The exact XPS13 version > > 3. BIOS version > > > > The UCSI driver got support for alternate modes in v5.2, so I'm > > guessing that is causing this problem, but to be sure, can you unload > > the UCSI driver to see if it has any effect? > > > > % modprobe -r ucsi_acpi > > > > Are you able to build you own test kernels? thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ā Fi.CI.IGT: failure for drm: connector firmware nodes
Hi Jani, On Tue, Mar 26, 2019 at 03:16:29PM +, Saarinen, Jani wrote: > > -Original Message- > > From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf > > Of Heikki > > Krogerus > > Sent: tiistai 26. maaliskuuta 2019 10.32 > > To: intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] ā Fi.CI.IGT: failure for drm: connector firmware > > nodes > > > > On Mon, Mar 25, 2019 at 08:57:44PM -, Patchwork wrote: > > > == Series Details == > > > > > > Series: drm: connector firmware nodes > > > URL : https://patchwork.freedesktop.org/series/58531/ > > > State : failure > > > > > > == Summary == > > > > > > CI Bug Log - changes from CI_DRM_5810_full -> Patchwork_12592_full > > > > > > > > > Summary > > > --- > > > > > > **FAILURE** > > > > > > Serious unknown changes coming with Patchwork_12592_full absolutely > > > need to > > be > > > verified manually. > > > > > > If you think the reported changes have nothing to do with the changes > > > introduced in Patchwork_12592_full, please notify your bug team to > > > allow them > > > to document this new failure mode, which will reduce false positives in > > > CI. > > > > > > > > > > > > Possible new issues > > > --- > > > > > > Here are the unknown changes that may have been introduced in > > Patchwork_12592_full: > > > > > > ### IGT changes ### > > > > > > Possible regressions > > > > > > * igt@perf_pmu@idle-no-semaphores-bcs0: > > > - shard-iclb: PASS -> INCOMPLETE > > > > > > I have not idea what this is about? Is there something I need to do? > Do you see from logs if relevant to your patche(s) ? Sorry, but I can't really say, as I don't know how to interpret the logs. I don't understand what that test is doing. I'm assuming this log shows the failure: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12592/shard-iclb8/run7.log The failure happens when running test (I'm guessing): "[30/82] (859s left) gem_tiled_swapping (non-threaded)" In the log there is a line that says: "FATAL: Unable to delete script file /tmp/jenkins7657231043286309557.sh" That certainly does not sound like anything that these patches would affect, but since I really have no idea what the test is doing, I can't really be sure. thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ā Fi.CI.IGT: failure for drm: connector firmware nodes
On Mon, Mar 25, 2019 at 08:57:44PM -, Patchwork wrote: > == Series Details == > > Series: drm: connector firmware nodes > URL : https://patchwork.freedesktop.org/series/58531/ > State : failure > > == Summary == > > CI Bug Log - changes from CI_DRM_5810_full -> Patchwork_12592_full > > > Summary > --- > > **FAILURE** > > Serious unknown changes coming with Patchwork_12592_full absolutely need to > be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_12592_full, please notify your bug team to allow > them > to document this new failure mode, which will reduce false positives in CI. > > > > Possible new issues > --- > > Here are the unknown changes that may have been introduced in > Patchwork_12592_full: > > ### IGT changes ### > > Possible regressions > > * igt@perf_pmu@idle-no-semaphores-bcs0: > - shard-iclb: PASS -> INCOMPLETE I have not idea what this is about? Is there something I need to do? thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm: Add fwnode member to the struct drm_connector
If the system firmware supplies nodes that represent the connectors,
they need to be associated with the connector entries.
ACPI tables at least always supply a device node for every connector
on integrated video hardware - this is explained in ACPI
specification's ch. "Appendix B Video Extension". Many drivers appear
to already deal with those connector firmware nodes "indirectly"
in order to support custom Operation Regions, _DSM (device specific
method) and access other resources the nodes supply for the
connectors.
This commit will only add the fwnode member. It's first up to the
drivers to assign and take advantage of it. For convenience, the nodes
are assigned to the device entries that are used for exposing the
connectors to the user space via sysfs. That makes it possible to see
the link between the connector and its node also from user space. In
case of ACPI, the connector's sysfs directory will have a symlink
named "firmware_node" pointing to the node object directory in sysfs
if a node is associated with the connector.
Signed-off-by: Heikki Krogerus
---
drivers/gpu/drm/drm_sysfs.c | 49 +
include/drm/drm_connector.h | 2 ++
2 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ecb7b33002bb..7667939ef1c0 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -264,26 +264,50 @@ static const struct attribute_group
*connector_dev_groups[] = {
NULL
};
+static void drm_sysfs_release(struct device *dev)
+{
+ kfree(dev);
+}
+
int drm_sysfs_connector_add(struct drm_connector *connector)
{
struct drm_device *dev = connector->dev;
+ struct device *kdev;
+ int ret;
if (connector->kdev)
return 0;
- connector->kdev =
- device_create_with_groups(drm_class, dev->primary->kdev, 0,
- connector, connector_dev_groups,
- "card%d-%s", dev->primary->index,
- connector->name);
- DRM_DEBUG("adding \"%s\" to sysfs\n",
- connector->name);
+ kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
+ if (!kdev)
+ return -ENOMEM;
- if (IS_ERR(connector->kdev)) {
- DRM_ERROR("failed to register connector device: %ld\n",
PTR_ERR(connector->kdev));
- return PTR_ERR(connector->kdev);
+ device_initialize(kdev);
+ kdev->class = drm_class;
+ kdev->parent = dev->primary->kdev;
+ kdev->fwnode = connector->fwnode;
+ kdev->groups = connector_dev_groups;
+ kdev->release = drm_sysfs_release;
+ dev_set_drvdata(kdev, connector);
+
+ ret = dev_set_name(kdev, "card%d-%s", dev->primary->index,
+ connector->name);
+ if (ret) {
+ kfree(kdev);
+ return ret;
+ }
+
+ DRM_DEBUG("adding \"%s\" to sysfs\n", connector->name);
+
+ ret = device_add(kdev);
+ if (ret) {
+ DRM_ERROR("failed to register connector device: %d\n", ret);
+ put_device(kdev);
+ return ret;
}
+ connector->kdev = kdev;
+
/* Let userspace know we have a new connector */
drm_sysfs_hotplug_event(dev);
@@ -330,11 +354,6 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_sysfs_hotplug_event);
-static void drm_sysfs_release(struct device *dev)
-{
- kfree(dev);
-}
-
struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
{
const char *minor_str;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c8061992d6cb..b8977c4eab14 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -886,6 +886,8 @@ struct drm_connector {
struct device *kdev;
/** @attr: sysfs attributes */
struct device_attribute *attr;
+ /** @fwnode: associated device node supplied by platform firmware */
+ struct fwnode_handle *fwnode;
/**
* @head:
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/2] drm: connector firmware nodes
Hi, If the system firmware supplies device nodes representing the display connectors on integrated video hardware, those nodes should probable always be associated with the display connector entries (struct drm_connector). With the USB Type-C DisplayPort alt mode, we will need a way to inform the correct drm connector entry about HPD IRQ, lane counts and other details. In ACPI (and most likely in DT too) the device node representing a DisplayPort behind USB Type-C connector should have a reference to the device node representing the USB Type-C connector (or vise versa). Once we have associated the DP connector device nodes with the drm connector entries, we can use those references to find the correct drm connector that the information the USB Type-C drivers are sending is meant for. Because I think the connector firmware nodes should be associated with the connector entries in any case (those nodes do seem to be supplying the connectors all kinds of resources, not only references to other components), I'm proposing this now instead of waiting for the USB Type-C patches. thanks, Heikki Krogerus (2): drm: Add fwnode member to the struct drm_connector drm/i915: Associate ACPI connector nodes with connector entries drivers/gpu/drm/drm_sysfs.c | 49 +++- drivers/gpu/drm/i915/intel_display.c | 40 +++ include/drm/drm_connector.h | 2 ++ 3 files changed, 76 insertions(+), 15 deletions(-) -- 2.20.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Associate ACPI connector nodes with connector entries
On Intel platforms we know that the ACPI connector device
node order will follow the order the driver (i915) decides.
The decision is made using the custom Intel ACPI OpRegion
(intel_opregion.c), though the driver does not actually know
that the values it sends to ACPI there are used for
associating a device node for the connectors, and assigning
address for them.
In reality that custom Intel ACPI OpRegion actually violates
ACPI specification (we supply dynamic information to objects
that are defined static, for example _ADR), however, it
makes assigning correct connector node for a connector entry
straightforward (it's one-on-one mapping).
Signed-off-by: Heikki Krogerus
---
drivers/gpu/drm/i915/intel_display.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 008560ef4db0..27aea2ef80ac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15526,6 +15526,45 @@ static int intel_initial_commit(struct drm_device *dev)
return ret;
}
+/* NOTE: The connector order must be final before this is called. */
+static void intel_assign_connector_fwnodes(struct drm_device *dev)
+{
+ struct drm_connector_list_iter conn_iter;
+ struct device *kdev = &dev->pdev->dev;
+ struct fwnode_handle *fwnode = NULL;
+ struct drm_connector *connector;
+ struct acpi_device *adev;
+
+ drm_connector_list_iter_begin(dev, &conn_iter);
+ drm_for_each_connector_iter(connector, &conn_iter) {
+ /* Always getting the next, even when the last was not used. */
+ fwnode = device_get_next_child_node(kdev, fwnode);
+ if (!fwnode)
+ break;
+
+ switch (connector->connector_type) {
+ case DRM_MODE_CONNECTOR_LVDS:
+ case DRM_MODE_CONNECTOR_eDP:
+ case DRM_MODE_CONNECTOR_DSI:
+ /*
+* Integrated displays have a specific address 0x1f on
+* most Intel platforms, but not on all of them.
+*/
+ adev = acpi_find_child_device(ACPI_COMPANION(kdev),
+ 0x1f, 0);
+ if (adev) {
+ connector->fwnode = acpi_fwnode_handle(adev);
+ break;
+ }
+ /* fallthrough */
+ default:
+ connector->fwnode = fwnode;
+ break;
+ }
+ }
+ drm_connector_list_iter_end(&conn_iter);
+}
+
int intel_modeset_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -15630,6 +15669,7 @@ int intel_modeset_init(struct drm_device *dev)
drm_modeset_lock_all(dev);
intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
+ intel_assign_connector_fwnodes(dev);
drm_modeset_unlock_all(dev);
for_each_intel_crtc(dev, crtc) {
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Thu, Feb 28, 2019 at 05:54:21PM +0100, Hans de Goede wrote: > Hi Heikki, > > On 28-02-19 15:47, Heikki Krogerus wrote: > > Hi Hans, > > > > On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 28-02-19 10:15, Heikki Krogerus wrote: > > > > > > > I've been thinking about this... Do we actually need to link the > > > > correct drm_connector to the Type-C connector? Perhaps we can make > > > > this work by just linking the GFX device to the Type-C connector. > > > > > > What use is it to the kms driver if it gets an event there is a DP > > > hotplug with x lanes and orientation foo, if we are not telling it > > > on which DP port it is ? kms devices already have multiple DP ports > > > and more then one could be hooked-up to type-c connectors. > > > > I was looking at this series. You walk trough every DP port in the > > system when the DP alt mode driver broadcasts the event, but maybe > > that's different. Never mind. > > Right, my "simple / naive" solution simply tells the kms driver to > check all DP ports for connection state changes, similar to how > running "xrandr" under Xorg causes the kms driver to re-check the > connection status of all ports. Actually running xrandr under Xorg > after plugging in the cable, is how I did my initial DP over Type-C > testing on the GPD win. > > But once we start passing extra-info, I believe the kms driver needs > to know to which connector that info belongs. > > > > > > > Well, I don't think we can deny the GPU driver (in this case) the > > > > information that we have and that is relevant to it, just because it > > > > seems difficult to deliver that information to the right location. > > > > > > Right, but this does not require a tight-coupling. My original > > > proposal can do this if we pass a data struct with an identifier > > > for the DP port for which the event is to the notifier. I suggest using > > > a string for identifier, something like: ":00:02.0/DP-1" this > > > event struct could then also contain all the info we want to pass. > > > > I do agree that we should not tie the objects (device entries) > > representing these components in kernel together, but I assume that we > > agree now that the connection between the two - the USB Type-C > > connector and the DisplayPort - must be described somewhere, either in > > firmware or build-in? So I guess we are talking implementation details > > here, right? > > Right. > > > If that is the case... > > > > That string identifier you proposed would basically provide the > > details about the connection, so if we know those details, we might as > > well use "normal" ways to describe the connection and just extract > > them in runtime in the function that our DP alt mode driver calls. I > > think the connection has to be defined in i915 on CHT in any case. > > Interesting, I think the connection should be described in the fwnode > for the fusb302 device for the CHT/GPD win case. Specifically I think > this fits well as a property of the dp altmode. OK, you are correct. I was stupidly still thinking about the driver loading order, but the order does not matter. > > The DP alt mode driver should definitely not need to pass anything > > else to the notifier other than handle to itself (actually, handle > > straight to the port device would be better) as an identifier. The > > notifier function needs to be the one that determines the actual > > connection using that handle. Even if the target DP is described using > > a string like you propose, then that string has to come from > > somewhere, most likely from a device property. The notifier function > > can just as well extract it, we don't need to pass it separately. > > > > Here's my suggestion for function prototype: > > > > int drm_typec_dp_notification(struct device *typec_port_dev, > >struct typec_displayport_data *data); > > How about instead of the port_dev we pass in the altmode object and > we have a method to get the fwnode for the altmode? Then the > drm_typec_dp_notification() function can get info from that fwnode > to implement the connection finding you describe below: We can pass the altmode object, np, but let's not decide which fwnode we'll ultimately use. I'm still leaning towards the connector node. > > So that function finds the connection between typec_port_dev
Re: [Intel-gfx] [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote: > Hi, > > On 28-02-19 10:15, Heikki Krogerus wrote: > > On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 27-02-19 12:16, Jani Nikula wrote: > > > > On Wed, 27 Feb 2019, Heikki Krogerus > > > > wrote: > > > > > One thing that this series does not consider is the DP lane count > > > > > problem. The GPU drivers (i915 in this case) does not know is four, > > > > > two or one DP lanes in use. > > > > > > > > Also, orientation. > > > > > > The orientation should simply be correct with Type-C over DP. The mux / > > > orientation-switch used will take care of (physically) swapping the > > > lanes if the connector is inserted upside-down. > > > > > > > > I guess that is not a critical issue since there is a workaround (I > > > > > think) where the driver basically does trial and error, but ideally we > > > > > should be able to tell i915 also the pin assignment that was > > > > > negotiated with the partner device so it knows the DP lane count. > > > > > > > > Yeah, if the information is there, we'd like to know. > > > > > > So far machines actually using a setup where the kernel does the > > > USB-PD / Type-C negotiation rather then this being handled in firmware > > > in say the Alpine Ridge controller, are very rare. > > > > > > AFAIK in the Alpine Ridge controller case, there is no communication > > > with the i915 driver, the only thing the Alpine Ridge controller does > > > which the everything done in the kernel approach does not, is that > > > it actually has a pin connected to the HDP pin of the displayport in > > > question. But that just lets the i915 driver know about hotplug-events, > > > not how many lanes are used. > > > > > > Currently I'm aware of only 2 x86 models which actually need the > > > hotplug event propagation from the Type-C subsystem to the i915 driver. > > > Do we really want to come up with a much more complex solution to > > > optimize for this corner case, while the much more common case > > > (Alpine Ridge controller) does not provide this info to the i915 driver? > > > > The HPD signal is often handled with a GPIO on Intel Platforms. Either > > the PD controller or EC controller, after receiving the Attention > > message, triggers the GPIO. On some systems though that GPIO method > > could not be used, so instead a side channel communication is used: > > the GFX device (so i915 driver) receives a specific custom interrupt. > > > > Unfortunately it now appears that there may be products coming where > > using the GPIO is not going to be possible, and also side channel > > communication is going to be out of the question. I've started an > > internal discussion inside Intel about the possibility to extend the > > UCSI specification to be able to support that kind of products. > > > > Alpine Ridge uses TI's Power Delivery controllers. The platforms that > > have Alpine Ridge TBT controller(s) often expose the USB Type-C > > connectors on them to the OS via UCSI, however, it appears the Alpine > > Ridge actually allows direct access to the PD controllers from the OS. > > That would mean we can supply the same information to the GPU drivers > > that we will deliver on CHT also on every platform that uses Alpine > > Ridge. > > Ok. > > > > To give some idea of the complexity, first of all we need some > > > mechanism to let the kernel know which drm_connector is connected > > > to which Type-C port. For the 2 models I know if which need this, this > > > info is not available and we would need to hardcode it in the kernel > > > based on e.g. DMI matching. > > > > I've been thinking about this... Do we actually need to link the > > correct drm_connector to the Type-C connector? Perhaps we can make > > this work by just linking the GFX device to the Type-C connector. > > What use is it to the kms driver if it gets an event there is a DP > hotplug with x lanes and orientation foo, if we are not telling it > on which DP port it is ? kms devices already have multiple DP ports > and more then one could be hooked-up to type-c connectors. I was looking at this series. You walk trough every DP port in the system when the DP alt mode driver broadcasts the event, but maybe that's different. Never mind. > > > T
Re: [Intel-gfx] [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote: > Hi, > > On 27-02-19 12:16, Jani Nikula wrote: > > On Wed, 27 Feb 2019, Heikki Krogerus > > wrote: > > > One thing that this series does not consider is the DP lane count > > > problem. The GPU drivers (i915 in this case) does not know is four, > > > two or one DP lanes in use. > > > > Also, orientation. > > The orientation should simply be correct with Type-C over DP. The mux / > orientation-switch used will take care of (physically) swapping the > lanes if the connector is inserted upside-down. > > > > I guess that is not a critical issue since there is a workaround (I > > > think) where the driver basically does trial and error, but ideally we > > > should be able to tell i915 also the pin assignment that was > > > negotiated with the partner device so it knows the DP lane count. > > > > Yeah, if the information is there, we'd like to know. > > So far machines actually using a setup where the kernel does the > USB-PD / Type-C negotiation rather then this being handled in firmware > in say the Alpine Ridge controller, are very rare. > > AFAIK in the Alpine Ridge controller case, there is no communication > with the i915 driver, the only thing the Alpine Ridge controller does > which the everything done in the kernel approach does not, is that > it actually has a pin connected to the HDP pin of the displayport in > question. But that just lets the i915 driver know about hotplug-events, > not how many lanes are used. > > Currently I'm aware of only 2 x86 models which actually need the > hotplug event propagation from the Type-C subsystem to the i915 driver. > Do we really want to come up with a much more complex solution to > optimize for this corner case, while the much more common case > (Alpine Ridge controller) does not provide this info to the i915 driver? The HPD signal is often handled with a GPIO on Intel Platforms. Either the PD controller or EC controller, after receiving the Attention message, triggers the GPIO. On some systems though that GPIO method could not be used, so instead a side channel communication is used: the GFX device (so i915 driver) receives a specific custom interrupt. Unfortunately it now appears that there may be products coming where using the GPIO is not going to be possible, and also side channel communication is going to be out of the question. I've started an internal discussion inside Intel about the possibility to extend the UCSI specification to be able to support that kind of products. Alpine Ridge uses TI's Power Delivery controllers. The platforms that have Alpine Ridge TBT controller(s) often expose the USB Type-C connectors on them to the OS via UCSI, however, it appears the Alpine Ridge actually allows direct access to the PD controllers from the OS. That would mean we can supply the same information to the GPU drivers that we will deliver on CHT also on every platform that uses Alpine Ridge. > To give some idea of the complexity, first of all we need some > mechanism to let the kernel know which drm_connector is connected > to which Type-C port. For the 2 models I know if which need this, this > info is not available and we would need to hardcode it in the kernel > based on e.g. DMI matching. I've been thinking about this... Do we actually need to link the correct drm_connector to the Type-C connector? Perhaps we can make this work by just linking the GFX device to the Type-C connector. > Then once we have this link, we need to start thinking about probe > ordering. Likely we need the typec framework to find the drm_connector, > since the typec-partner device is only created when there actually is > a DP capable "dongle" connected, making doing it the other way around > tricky. Then the typec-partner device needs to get a reference or some > such to make sure the drm_connector does not fgo away during the lifetime > of the typec-partner device. No! We must not link the partner device with anything other than the Type-C connector. We link the USB Type-C connector to the DisplayPort, and we link the USB Type-C connector to the partner. The Type-C connector is the proxy here. > I would really like to avoid this, so if we want to send more info to > the i915 driver, I suggest we create some event struct for this which > gets passed to the notifier, this could include a string to > describe the DP connector which the Type-C connector is connected to > when the mux is set to DP mode, e.g. "i915/DP-1" should be unique > or probably better, use the PCI device name, so: ":00:02.0/DP-1" > > Then we still have a loose coupling avoiding lifetime issues, while > the receiving drm driver can check whi
Re: [Intel-gfx] [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
On Wed, Feb 27, 2019 at 01:16:27PM +0200, Jani Nikula wrote: > On Wed, 27 Feb 2019, Heikki Krogerus wrote: > > One thing that this series does not consider is the DP lane count > > problem. The GPU drivers (i915 in this case) does not know is four, > > two or one DP lanes in use. > > Also, orientation. > > > I guess that is not a critical issue since there is a workaround (I > > think) where the driver basically does trial and error, but ideally we > > should be able to tell i915 also the pin assignment that was > > negotiated with the partner device so it knows the DP lane count. > > Yeah, if the information is there, we'd like to know. With the > orientation, there's a worst case of sixth attempt of finding out > there's just one lane in a certain orientation. Couple that with link > rate selection (did it not work because too high link rate or because > the lanes are just not there?) we get pretty confused about what we > should try. The orientation is also considered in the alt mode API. We have a function for that typec_altmode_get_orientation(), but it of course requires that the caller has a handle to the alt mode object. So basically we would again need tight coupling between the DP connector and USB Type-C connector. Hans, I'm not so sure we should, or can, rule out the "tight coupling" option after all. thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Mon, Feb 25, 2019 at 02:20:34PM +0100, Hans de Goede wrote: > Hi All, > > On some Cherry Trail devices, DisplayPort over Type-C is supported through > a USB-PD microcontroller (e.g. a fusb302) + a mux to switch the superspeed > datalines between USB-3 and DP (e.g. a pi3usb30532). The kernel in this > case does the PD/alt-mode negotiation itself, rather then everything being > handled in firmware. > > So the kernel itself picks an alt-mode, tells the Type-C "dongle" to switch > to DP mode and sets the mux accordingly. In this setup the HPD pin is not > connected, so the i915 driver needs to respond to a software event and scan > the DP port for changes manually. > > Thanks to Heikki's great work on the DisplayPort altmode support in the > typec subsys, we now correctly tell the dongle to switch to DP altmode > and we correctly set the mux and orientation switches to connect the > DP lines to the Type-C connector. > > This just leaves sending an out-of-band hotplug event from the Type-C > subsystem to the i915 driver and then we've fully working DP over Type-C > on these devices. > > This series implements this. The first patch adds a generic mechanism > for oob hotplug events to be send to the drm subsys, the second patch > adds support for this mechanism to the i915 driver and the third patch > makes the typec displayport_altmode driver send these events. > > The commit message of the first patch explains why I've chosen to things > the way these patches do them. One thing that this series does not consider is the DP lane count problem. The GPU drivers (i915 in this case) does not know is four, two or one DP lanes in use. I guess that is not a critical issue since there is a workaround (I think) where the driver basically does trial and error, but ideally we should be able to tell i915 also the pin assignment that was negotiated with the partner device so it knows the DP lane count. My original idea was that we pass that struct typec_displayport_data as payload in the event. From the Configuration VDO the GPU drivers can then see the negotiated DP pin assignment and determine the lane count. thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
On Mon, Feb 25, 2019 at 02:20:37PM +0100, Hans de Goede wrote: > Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load > drm/kms drivers know about DisplayPort over Type-C hotplug events. > > Signed-off-by: Hans de Goede I'm OK with this. I'll wait for the v2 and see if I can test these. thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v1] ACPI: Switch to use generic UUID API
On Thu, May 04, 2017 at 12:21:51PM +0300, Andy Shevchenko wrote: > acpi_evaluate_dsm() and friends take a pointer to a raw buffer of 16 > bytes. Instead we convert them to use uuid_le type. At the same time we > convert current users. > > acpi_str_to_uuid() becomes useless after the conversion and it's safe to > get rid of it. > > The conversion fixes a potential bug in int340x_thermal as well since > we have to use memcmp() on binary data. > > Cc: Rafael J. Wysocki > Cc: Mika Westerberg > Cc: Borislav Petkov > Cc: Dan Williams > Cc: Amir Goldstein > Cc: Jarkko Sakkinen > Cc: Jani Nikula > Cc: Ben Skeggs > Cc: Benjamin Tissoires > Cc: Joerg Roedel > Cc: Adrian Hunter > Cc: Yisen Zhuang > Cc: Bjorn Helgaas > Cc: Zhang Rui > Cc: Felipe Balbi > Cc: Mathias Nyman > Cc: Heikki Krogerus > Cc: Liam Girdwood > Cc: Mark Brown > Signed-off-by: Andy Shevchenko OK by me, FWIW: Reviewed-by: Heikki Krogerus Thanks, -- heikki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
