[PATCH] drm/i915/dp: Update connector status for DP MST hotplugs

[PATCH] drm/i915/dp: Update connector status for DP MST hotplugs

[Dhinakaran Pandiyan]

Hotplugging a monitor in DP MST configuration triggers short pulses.
Although the short pulse handling path ends up in the MST hotplug function,
we do not perform a detect before sending the hotplug uvent. This leads to
the connector status not being updated when the userspace calls
DRM_IOCTL_MODE_GETCONNECTOR.

So, let's call the connector function ->detect() to update the connector
status before sending the uevent.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 3ffbd69..a28b12b 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -492,8 +492,25 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
 	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_connector *intel_connector;
+	bool change = false;
+	enum drm_connector_status old_status;
+
+	for_each_intel_connector(dev, intel_connector) {
+		struct drm_connector *connector = &(intel_connector->base);
+
+		if (intel_connector->mst_port != intel_dp)
+			continue;
+
+		old_status = connector->status;
+		connector->status = connector->funcs->detect(connector, false);
+
+		if (old_status != connector->status)
+			changed = true;
+	}
 
-	drm_kms_helper_hotplug_event(dev);
+	if (changed)
+		drm_kms_helper_hotplug_event(dev);
 }
 
 static const struct drm_dp_mst_topology_cbs mst_cbs = {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/dp: Update connector status for DP MST hotplugs

[Dhinakaran Pandiyan]

Hotplugging a monitor in DP MST configuration triggers short pulses.
Although the short pulse handling path ends up in the MST hotplug function,
we do not perform a detect before sending the hotplug uvent. This leads to
the connector status not being updated when the userspace calls
DRM_IOCTL_MODE_GETCONNECTOR.

So, let's call the connector function ->detect() to update the connector
status before sending the uevent.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 3ffbd69..80810c9 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -492,8 +492,25 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
 	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_connector *intel_connector;
+	bool changed = false;
+	enum drm_connector_status old_status;
+
+	for_each_intel_connector(dev, intel_connector) {
+		struct drm_connector *connector = &(intel_connector->base);
+
+		if (intel_connector->mst_port != intel_dp)
+			continue;
+
+		old_status = connector->status;
+		connector->status = connector->funcs->detect(connector, false);
+
+		if (old_status != connector->status)
+			changed = true;
+	}
 
-	drm_kms_helper_hotplug_event(dev);
+	if (changed)
+		drm_kms_helper_hotplug_event(dev);
 }
 
 static const struct drm_dp_mst_topology_cbs mst_cbs = {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915/dp: Update connector status for DP MST hotplugs (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/dp: Update connector status for DP MST hotplugs (rev2)
URL   : https://patchwork.freedesktop.org/series/14821/
State : warning

== Summary ==

Series 14821v2 drm/i915/dp: Update connector status for DP MST hotplugs
https://patchwork.freedesktop.org/api/1.0/series/14821/revisions/2/mbox/

Test kms_force_connector_basic:
        Subgroup force-edid:
                pass       -> DMESG-WARN (fi-snb-2520m)
        Subgroup force-load-detect:
                dmesg-warn -> PASS       (fi-snb-2520m)

fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:241  pass:188  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:241  pass:208  dwarn:1   dfail:0   fail:0   skip:32 
fi-snb-2600      total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 
fi-byt-n2820 failed to collect. IGT log at Patchwork_2901/fi-byt-n2820/igt.log

21f242e536b5077c046df785a8c4c28374941c15 drm-intel-nightly: 2016y-11m-03d-21h-01m-03s UTC integration manifest
bc09ce1 drm/i915/dp: Update connector status for DP MST hotplugs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2901/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Update connector status for DP MST hotplugs

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]

Hi Dhinakaran,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.9-rc3 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dhinakaran-Pandiyan/drm-i915-dp-Update-connector-status-for-DP-MST-hotplugs/20161104-113409
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x010-201644 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_dp_mst.c: In function 'intel_dp_mst_hotplug':
>> drivers/gpu/drm/i915/intel_dp_mst.c:509:4: error: 'changed' undeclared (first use in this function)
       changed = true;
       ^~~~~~~
   drivers/gpu/drm/i915/intel_dp_mst.c:509:4: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/i915/intel_dp_mst.c:496:7: warning: unused variable 'change' [-Wunused-variable]
     bool change = false;
          ^~~~~~

vim +/changed +509 drivers/gpu/drm/i915/intel_dp_mst.c

   490	static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
   491	{
   492		struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
   493		struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
   494		struct drm_device *dev = intel_dig_port->base.base.dev;
   495		struct intel_connector *intel_connector;
 > 496		bool change = false;
   497		enum drm_connector_status old_status;
   498	
   499		for_each_intel_connector(dev, intel_connector) {
   500			struct drm_connector *connector = &(intel_connector->base);
   501	
   502			if (intel_connector->mst_port != intel_dp)
   503				continue;
   504	
   505			old_status = connector->status;
   506			connector->status = connector->funcs->detect(connector, false);
   507	
   508			if (old_status != connector->status)
 > 509				changed = true;
   510		}
   511	
   512		if (changed)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27836 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915/dp: Update connector status for DP MST hotplugs (rev2)

[Saarinen, Jani]

> == Series Details ==
> 
> Series: drm/i915/dp: Update connector status for DP MST hotplugs (rev2)
> URL   : https://patchwork.freedesktop.org/series/14821/
> State : warning
> 
> == Summary ==
> 
> Series 14821v2 drm/i915/dp: Update connector status for DP MST hotplugs
> https://patchwork.freedesktop.org/api/1.0/series/14821/revisions/2/mbox/
> 
> Test kms_force_connector_basic:
>         Subgroup force-edid:
>                 pass       -> DMESG-WARN (fi-snb-2520m)
https://bugs.freedesktop.org/show_bug.cgi?id=74102 ?

>         Subgroup force-load-detect:
>                 dmesg-warn -> PASS       (fi-snb-2520m)
> 
> fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15
> fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40
> fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28
> fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28
> fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20
> fi-hsw-4770r     total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20
> fi-ilk-650       total:241  pass:188  dwarn:0   dfail:0   fail:0   skip:53
> fi-ivb-3520m     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22
> fi-ivb-3770      total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22
> fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22
> fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14
> fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21
> fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21
> fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14
> fi-snb-2520m     total:241  pass:208  dwarn:1   dfail:0   fail:0   skip:32
> fi-snb-2600      total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33
> fi-byt-n2820 failed to collect. IGT log at Patchwork_2901/fi-byt-n2820/igt.log
> 
> 21f242e536b5077c046df785a8c4c28374941c15 drm-intel-nightly: 2016y-11m-
> 03d-21h-01m-03s UTC integration manifest
> bc09ce1 drm/i915/dp: Update connector status for DP MST hotplugs
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2901/


Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Update connector status for DP MST hotplugs

[Ville Syrjälä]

On Thu, Nov 03, 2016 at 08:38:32PM -0700, Dhinakaran Pandiyan wrote:
> Hotplugging a monitor in DP MST configuration triggers short pulses.
> Although the short pulse handling path ends up in the MST hotplug function,
> we do not perform a detect before sending the hotplug uvent. This leads to
> the connector status not being updated when the userspace calls
> DRM_IOCTL_MODE_GETCONNECTOR.
> 
> So, let's call the connector function ->detect() to update the connector
> status before sending the uevent.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 3ffbd69..80810c9 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -492,8 +492,25 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
>  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct intel_connector *intel_connector;
> +	bool changed = false;
> +	enum drm_connector_status old_status;
> +
> +	for_each_intel_connector(dev, intel_connector) {
> +		struct drm_connector *connector = &(intel_connector->base);
> +
> +		if (intel_connector->mst_port != intel_dp)
> +			continue;
> +
> +		old_status = connector->status;
> +		connector->status = connector->funcs->detect(connector, false);
> +
> +		if (old_status != connector->status)
> +			changed = true;
> +	}

So what's the story with the locking? This sort of stuff needs the
protection of the mode_config mutex.

>  
> -	drm_kms_helper_hotplug_event(dev);
> +	if (changed)
> +		drm_kms_helper_hotplug_event(dev);
>  }
>  
>  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Update connector status for DP MST hotplugs

[Pandiyan, Dhinakaran]

On Mon, 2016-11-07 at 18:14 +0200, Ville Syrjälä wrote:
> On Thu, Nov 03, 2016 at 08:38:32PM -0700, Dhinakaran Pandiyan wrote:
> > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > Although the short pulse handling path ends up in the MST hotplug function,
> > we do not perform a detect before sending the hotplug uvent. This leads to
> > the connector status not being updated when the userspace calls
> > DRM_IOCTL_MODE_GETCONNECTOR.
> > 
> > So, let's call the connector function ->detect() to update the connector
> > status before sending the uevent.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 3ffbd69..80810c9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -492,8 +492,25 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> >  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > +	struct intel_connector *intel_connector;
> > +	bool changed = false;
> > +	enum drm_connector_status old_status;
> > +
> > +	for_each_intel_connector(dev, intel_connector) {
> > +		struct drm_connector *connector = &(intel_connector->base);
> > +
> > +		if (intel_connector->mst_port != intel_dp)
> > +			continue;
> > +
> > +		old_status = connector->status;
> > +		connector->status = connector->funcs->detect(connector, false);
> > +
> > +		if (old_status != connector->status)
> > +			changed = true;
> > +	}
> 
> So what's the story with the locking? This sort of stuff needs the
> protection of the mode_config mutex.
> 

I wasn't sure, will send another patch.


-DK

> >  
> > -	drm_kms_helper_hotplug_event(dev);
> > +	if (changed)
> > +		drm_kms_helper_hotplug_event(dev);
> >  }
> >  
> >  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> > -- 
> > 2.7.4
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Dhinakaran Pandiyan]

Hotplugging a monitor in DP MST configuration triggers short pulses.
Although the short pulse handling path ends up in the MST hotplug function,
we do not perform a detect before sending the hotplug uvent. This leads to
the connector status not being updated when the userspace calls
DRM_IOCTL_MODE_GETCONNECTOR.

So, let's call the connector function ->detect() to update the connector
status before sending the uevent.

v2: Update connector status inside mode_config mutex (Ville)

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 3ffbd69..8e36a96 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
 	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct intel_connector *intel_connector;
+	bool changed = false;
+	enum drm_connector_status old_status;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+
+	mutex_lock(&mode_config->mutex);
+	for_each_intel_connector(dev, intel_connector) {
+		struct drm_connector *connector = &(intel_connector->base);
+
+		if (intel_connector->mst_port != intel_dp)
+			continue;
+
+		old_status = connector->status;
+		connector->status = connector->funcs->detect(connector, false);
+
+		if (old_status != connector->status)
+			changed = true;
+	}
+	mutex_unlock(&mode_config->mutex);
 
-	drm_kms_helper_hotplug_event(dev);
+	if (changed)
+		drm_kms_helper_hotplug_event(dev);
 }
 
 static const struct drm_dp_mst_topology_cbs mst_cbs = {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/dp: Update connector status for DP MST hotplugs (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/dp: Update connector status for DP MST hotplugs (rev3)
URL   : https://patchwork.freedesktop.org/series/14821/
State : success

== Summary ==

Series 14821v3 drm/i915/dp: Update connector status for DP MST hotplugs
https://patchwork.freedesktop.org/api/1.0/series/14821/revisions/3/mbox/


fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:241  pass:188  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 

0cb521ab1e777031c1fbb5fb2b0a5b13ccb9c461 drm-intel-nightly: 2016y-11m-07d-20h-58m-10s UTC integration manifest
2358960 drm/i915/dp: Update connector status for DP MST hotplugs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2928/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Ville Syrjälä]

On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> Hotplugging a monitor in DP MST configuration triggers short pulses.
> Although the short pulse handling path ends up in the MST hotplug function,
> we do not perform a detect before sending the hotplug uvent. This leads to
> the connector status not being updated when the userspace calls
> DRM_IOCTL_MODE_GETCONNECTOR.
> 
> So, let's call the connector function ->detect() to update the connector
> status before sending the uevent.
> 
> v2: Update connector status inside mode_config mutex (Ville)
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 3ffbd69..8e36a96 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
>  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct intel_connector *intel_connector;
> +	bool changed = false;
> +	enum drm_connector_status old_status;
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +
> +	mutex_lock(&mode_config->mutex);
> +	for_each_intel_connector(dev, intel_connector) {
> +		struct drm_connector *connector = &(intel_connector->base);
> +
> +		if (intel_connector->mst_port != intel_dp)
> +			continue;
> +
> +		old_status = connector->status;
> +		connector->status = connector->funcs->detect(connector, false);
> +
> +		if (old_status != connector->status)
> +			changed = true;
> +	}
> +	mutex_unlock(&mode_config->mutex);

To help reviewers it might be nice to have a short explanation of the
locking situation in the commit message, mainly why is it safe to simply
take mode_config.mutex here.

>  
> -	drm_kms_helper_hotplug_event(dev);
> +	if (changed)
> +		drm_kms_helper_hotplug_event(dev);
>  }
>  
>  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Ville Syrjälä]

On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> Hotplugging a monitor in DP MST configuration triggers short pulses.
> Although the short pulse handling path ends up in the MST hotplug function,
> we do not perform a detect before sending the hotplug uvent. This leads to
> the connector status not being updated when the userspace calls
> DRM_IOCTL_MODE_GETCONNECTOR.
> 
> So, let's call the connector function ->detect() to update the connector
> status before sending the uevent.
> 
> v2: Update connector status inside mode_config mutex (Ville)
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Possibly 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98306

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 3ffbd69..8e36a96 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
>  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct intel_connector *intel_connector;
> +	bool changed = false;
> +	enum drm_connector_status old_status;
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +
> +	mutex_lock(&mode_config->mutex);
> +	for_each_intel_connector(dev, intel_connector) {
> +		struct drm_connector *connector = &(intel_connector->base);
> +
> +		if (intel_connector->mst_port != intel_dp)
> +			continue;
> +
> +		old_status = connector->status;
> +		connector->status = connector->funcs->detect(connector, false);
> +
> +		if (old_status != connector->status)
> +			changed = true;
> +	}
> +	mutex_unlock(&mode_config->mutex);
>  
> -	drm_kms_helper_hotplug_event(dev);
> +	if (changed)
> +		drm_kms_helper_hotplug_event(dev);
>  }
>  
>  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Pandiyan, Dhinakaran]

On Tue, 2016-11-08 at 13:04 +0200, Ville Syrjälä wrote:
> On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > Although the short pulse handling path ends up in the MST hotplug function,
> > we do not perform a detect before sending the hotplug uvent. This leads to
> > the connector status not being updated when the userspace calls
> > DRM_IOCTL_MODE_GETCONNECTOR.
> > 
> > So, let's call the connector function ->detect() to update the connector
> > status before sending the uevent.
> > 
> > v2: Update connector status inside mode_config mutex (Ville)
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 3ffbd69..8e36a96 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> >  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > +	struct intel_connector *intel_connector;
> > +	bool changed = false;
> > +	enum drm_connector_status old_status;
> > +	struct drm_mode_config *mode_config = &dev->mode_config;
> > +
> > +	mutex_lock(&mode_config->mutex);
> > +	for_each_intel_connector(dev, intel_connector) {
> > +		struct drm_connector *connector = &(intel_connector->base);
> > +
> > +		if (intel_connector->mst_port != intel_dp)
> > +			continue;
> > +
> > +		old_status = connector->status;
> > +		connector->status = connector->funcs->detect(connector, false);
> > +
> > +		if (old_status != connector->status)
> > +			changed = true;
> > +	}
> > +	mutex_unlock(&mode_config->mutex);
> 
> To help reviewers it might be nice to have a short explanation of the
> locking situation in the commit message, mainly why is it safe to simply
> take mode_config.mutex here.
> 

I can't convince myself this locking is correct yet.
drm_dp_send_link_address(), one of the callers of this function is
recursive.
drm_dp_send_link_address()->drm_dp_add_port()->drm_dp_send_link_address()

I am wondering if hpd_pulse should return IRQ_NONE so that we can have
->detect() called from the hotplug work function. For this to work,
we'll need to change drm_dp_mst_hpd_irq().


-DK

> >  
> > -	drm_kms_helper_hotplug_event(dev);
> > +	if (changed)
> > +		drm_kms_helper_hotplug_event(dev);
> >  }
> >  
> >  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> > -- 
> > 2.7.4
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Ville Syrjälä]

On Fri, Nov 11, 2016 at 08:43:53PM +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2016-11-08 at 13:04 +0200, Ville Syrjälä wrote:
> > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> > > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > > Although the short pulse handling path ends up in the MST hotplug function,
> > > we do not perform a detect before sending the hotplug uvent. This leads to
> > > the connector status not being updated when the userspace calls
> > > DRM_IOCTL_MODE_GETCONNECTOR.
> > > 
> > > So, let's call the connector function ->detect() to update the connector
> > > status before sending the uevent.
> > > 
> > > v2: Update connector status inside mode_config mutex (Ville)
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 3ffbd69..8e36a96 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> > >  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> > >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > +	struct intel_connector *intel_connector;
> > > +	bool changed = false;
> > > +	enum drm_connector_status old_status;
> > > +	struct drm_mode_config *mode_config = &dev->mode_config;
> > > +
> > > +	mutex_lock(&mode_config->mutex);
> > > +	for_each_intel_connector(dev, intel_connector) {
> > > +		struct drm_connector *connector = &(intel_connector->base);
> > > +
> > > +		if (intel_connector->mst_port != intel_dp)
> > > +			continue;
> > > +
> > > +		old_status = connector->status;
> > > +		connector->status = connector->funcs->detect(connector, false);
> > > +
> > > +		if (old_status != connector->status)
> > > +			changed = true;
> > > +	}
> > > +	mutex_unlock(&mode_config->mutex);
> > 
> > To help reviewers it might be nice to have a short explanation of the
> > locking situation in the commit message, mainly why is it safe to simply
> > take mode_config.mutex here.
> > 
> 
> I can't convince myself this locking is correct yet.
> drm_dp_send_link_address(), one of the callers of this function is
> recursive.
> drm_dp_send_link_address()->drm_dp_add_port()->drm_dp_send_link_address()

Hmm. I wonder how deep that can get. As in is there an upper bound on
the stack usage...

> 
> I am wondering if hpd_pulse should return IRQ_NONE so that we can have
> ->detect() called from the hotplug work function. For this to work,
> we'll need to change drm_dp_mst_hpd_irq().

Does the topology manager do all the processing in a blocking fashion
from the dig_port_work, or does is schedule additional works and return
before they're done?

If it all happens from the dig_port_work, then I guess it should be
possible to just set some flag from the mst .hotplug() hook and check
that flag after all the processing is done to know whether the schedule
the a full detect work. I don't think you can just return IRQ_NONE from
the short pulse since that would schedule the full hotplug on the main
DP connector, not the MST one. In fact our current hotplug code entirely
skips MST connectors. But you could have a separate work perhaps just
to deal with the MST connectors, or we'd need to redo the main hotplug
handling in some way to handle MST connectors as well.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Daniel Vetter]

On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> Hotplugging a monitor in DP MST configuration triggers short pulses.
> Although the short pulse handling path ends up in the MST hotplug function,
> we do not perform a detect before sending the hotplug uvent. This leads to
> the connector status not being updated when the userspace calls
> DRM_IOCTL_MODE_GETCONNECTOR.
> 
> So, let's call the connector function ->detect() to update the connector
> status before sending the uevent.
> 
> v2: Update connector status inside mode_config mutex (Ville)
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

quick comments from me on irc:

<danvet> dhnkrn, really tricky I think
<danvet> dhnkrn, also feels wrong from an entire design pov
<danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came
<danvet> which means we should be able to get at that connector without taking the global lock
<danvet> i.e. at least avoid the for_each_connector
<danvet> on the detect itself, I need to think
<danvet> but we might need to essentially postpone the ->detect work to the hotplug worker
<danvet> except that it's not a direct hpd from our chip
<danvet> but a downstream one
<danvet> this is going to be fun
<dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ?
<danvet> yup
<danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports)
<danvet> or at least edid reads
<danvet> both recurse back into dp mst helper code like ville said
<danvet> dhnkrn, so instead of doing the detect work from the hotplug code
<danvet> put them on some list
<danvet> need to be careful in case they are already there
<danvet> list only protected with dedicated lock
<danvet> then launch the hpd worker
<danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe
<danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo
<danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised
<danvet> needs more thought probably
<danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex
<danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 3ffbd69..8e36a96 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
>  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct intel_connector *intel_connector;
> +	bool changed = false;
> +	enum drm_connector_status old_status;
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +
> +	mutex_lock(&mode_config->mutex);
> +	for_each_intel_connector(dev, intel_connector) {
> +		struct drm_connector *connector = &(intel_connector->base);
> +
> +		if (intel_connector->mst_port != intel_dp)
> +			continue;
> +
> +		old_status = connector->status;
> +		connector->status = connector->funcs->detect(connector, false);
> +
> +		if (old_status != connector->status)
> +			changed = true;
> +	}
> +	mutex_unlock(&mode_config->mutex);
>  
> -	drm_kms_helper_hotplug_event(dev);
> +	if (changed)
> +		drm_kms_helper_hotplug_event(dev);
>  }
>  
>  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Daniel Vetter]

On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote:
> On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > Although the short pulse handling path ends up in the MST hotplug function,
> > we do not perform a detect before sending the hotplug uvent. This leads to
> > the connector status not being updated when the userspace calls
> > DRM_IOCTL_MODE_GETCONNECTOR.
> > 
> > So, let's call the connector function ->detect() to update the connector
> > status before sending the uevent.
> > 
> > v2: Update connector status inside mode_config mutex (Ville)
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> quick comments from me on irc:
> 
> <danvet> dhnkrn, really tricky I think
> <danvet> dhnkrn, also feels wrong from an entire design pov
> <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came
> <danvet> which means we should be able to get at that connector without taking the global lock
> <danvet> i.e. at least avoid the for_each_connector
> <danvet> on the detect itself, I need to think
> <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker
> <danvet> except that it's not a direct hpd from our chip
> <danvet> but a downstream one
> <danvet> this is going to be fun
> <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ?
> <danvet> yup
> <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports)
> <danvet> or at least edid reads
> <danvet> both recurse back into dp mst helper code like ville said
> <danvet> dhnkrn, so instead of doing the detect work from the hotplug code
> <danvet> put them on some list
> <danvet> need to be careful in case they are already there
> <danvet> list only protected with dedicated lock
> <danvet> then launch the hpd worker
> <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe
> <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo
> <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised
> <danvet> needs more thought probably
> <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex
> <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then
> 
> Cheers, Daniel
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 3ffbd69..8e36a96 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> >  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > +	struct intel_connector *intel_connector;
> > +	bool changed = false;
> > +	enum drm_connector_status old_status;
> > +	struct drm_mode_config *mode_config = &dev->mode_config;
> > +
> > +	mutex_lock(&mode_config->mutex);
> > +	for_each_intel_connector(dev, intel_connector) {
> > +		struct drm_connector *connector = &(intel_connector->base);
> > +
> > +		if (intel_connector->mst_port != intel_dp)
> > +			continue;
> > +
> > +		old_status = connector->status;
> > +		connector->status = connector->funcs->detect(connector, false);
> > +
> > +		if (old_status != connector->status)
> > +			changed = true;
> > +	}
> > +	mutex_unlock(&mode_config->mutex);
> >  
> > -	drm_kms_helper_hotplug_event(dev);

I just realized that this call here will call down into the fbdev helper,
which already does the above ->detect calls. At least if there's no
compositor running. So I wonder why the deadlock ville pointed out isn't
blowing up already ...
-Daniel

> > +	if (changed)
> > +		drm_kms_helper_hotplug_event(dev);
> >  }
> >  
> >  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Pandiyan, Dhinakaran]

On Fri, 2016-11-11 at 23:05 +0200, Ville Syrjälä wrote:
> On Fri, Nov 11, 2016 at 08:43:53PM +0000, Pandiyan, Dhinakaran wrote:
> > On Tue, 2016-11-08 at 13:04 +0200, Ville Syrjälä wrote:
> > > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> > > > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > > > Although the short pulse handling path ends up in the MST hotplug function,
> > > > we do not perform a detect before sending the hotplug uvent. This leads to
> > > > the connector status not being updated when the userspace calls
> > > > DRM_IOCTL_MODE_GETCONNECTOR.
> > > > 
> > > > So, let's call the connector function ->detect() to update the connector
> > > > status before sending the uevent.
> > > > 
> > > > v2: Update connector status inside mode_config mutex (Ville)
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
> > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > index 3ffbd69..8e36a96 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> > > >  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> > > >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > > +	struct intel_connector *intel_connector;
> > > > +	bool changed = false;
> > > > +	enum drm_connector_status old_status;
> > > > +	struct drm_mode_config *mode_config = &dev->mode_config;
> > > > +
> > > > +	mutex_lock(&mode_config->mutex);
> > > > +	for_each_intel_connector(dev, intel_connector) {
> > > > +		struct drm_connector *connector = &(intel_connector->base);
> > > > +
> > > > +		if (intel_connector->mst_port != intel_dp)
> > > > +			continue;
> > > > +
> > > > +		old_status = connector->status;
> > > > +		connector->status = connector->funcs->detect(connector, false);
> > > > +
> > > > +		if (old_status != connector->status)
> > > > +			changed = true;
> > > > +	}
> > > > +	mutex_unlock(&mode_config->mutex);
> > > 
> > > To help reviewers it might be nice to have a short explanation of the
> > > locking situation in the commit message, mainly why is it safe to simply
> > > take mode_config.mutex here.
> > > 
> > 
> > I can't convince myself this locking is correct yet.
> > drm_dp_send_link_address(), one of the callers of this function is
> > recursive.
> > drm_dp_send_link_address()->drm_dp_add_port()->drm_dp_send_link_address()
> 
> Hmm. I wonder how deep that can get. As in is there an upper bound on
> the stack usage...
> 
The upper bound is going to be the number of downstream ports for the
branch device that was hotplugged.

I went through the code again, the recursive call that I mentioned above
is done before hotplug() is called. It does look like we are safe there.

But, what is going to be a problem is, as Daniel pointed out,
intel_dp_mst_hotplug()->drm_kms_helper_hotplug_event()
->output_poll_changed(dev) ->drm_fb_helper_hotplug_event()

This sequence leads to an recursive call of the mode_config.mutex 
with fbdev_emulation enabled.


> > 
> > I am wondering if hpd_pulse should return IRQ_NONE so that we can have
> > ->detect() called from the hotplug work function. For this to work,
> > we'll need to change drm_dp_mst_hpd_irq().
> 
> Does the topology manager do all the processing in a blocking fashion
> from the dig_port_work, or does is schedule additional works and return
> before they're done?

drm_dp_mst_link_probe_work() gets scheduled inside drm_dp_update_port()
when a device was plugged in. 

-DK

> 
> If it all happens from the dig_port_work, then I guess it should be
> possible to just set some flag from the mst .hotplug() hook and check
> that flag after all the processing is done to know whether the schedule
> the a full detect work. I don't think you can just return IRQ_NONE from
> the short pulse since that would schedule the full hotplug on the main
> DP connector, not the MST one. In fact our current hotplug code entirely
> skips MST connectors. But you could have a separate work perhaps just
> to deal with the MST connectors, or we'd need to redo the main hotplug
> handling in some way to handle MST connectors as well.
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Pandiyan, Dhinakaran]

On Sun, 2016-11-13 at 11:39 +0100, Daniel Vetter wrote:
> On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote:
> > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> > > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > > Although the short pulse handling path ends up in the MST hotplug function,
> > > we do not perform a detect before sending the hotplug uvent. This leads to
> > > the connector status not being updated when the userspace calls
> > > DRM_IOCTL_MODE_GETCONNECTOR.
> > > 
> > > So, let's call the connector function ->detect() to update the connector
> > > status before sending the uevent.
> > > 
> > > v2: Update connector status inside mode_config mutex (Ville)
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 
> > quick comments from me on irc:
> > 
> > <danvet> dhnkrn, really tricky I think
> > <danvet> dhnkrn, also feels wrong from an entire design pov
> > <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came
> > <danvet> which means we should be able to get at that connector without taking the global lock
> > <danvet> i.e. at least avoid the for_each_connector
> > <danvet> on the detect itself, I need to think
> > <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker
> > <danvet> except that it's not a direct hpd from our chip
> > <danvet> but a downstream one
> > <danvet> this is going to be fun
> > <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ?
> > <danvet> yup
> > <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports)
> > <danvet> or at least edid reads
> > <danvet> both recurse back into dp mst helper code like ville said
> > <danvet> dhnkrn, so instead of doing the detect work from the hotplug code
> > <danvet> put them on some list
> > <danvet> need to be careful in case they are already there
> > <danvet> list only protected with dedicated lock
> > <danvet> then launch the hpd worker
> > <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe
> > <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo
> > <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised
> > <danvet> needs more thought probably
> > <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex
> > <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then
> > 
> > Cheers, Daniel
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 3ffbd69..8e36a96 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> > >  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> > >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > +	struct intel_connector *intel_connector;
> > > +	bool changed = false;
> > > +	enum drm_connector_status old_status;
> > > +	struct drm_mode_config *mode_config = &dev->mode_config;
> > > +
> > > +	mutex_lock(&mode_config->mutex);
> > > +	for_each_intel_connector(dev, intel_connector) {
> > > +		struct drm_connector *connector = &(intel_connector->base);
> > > +
> > > +		if (intel_connector->mst_port != intel_dp)
> > > +			continue;
> > > +
> > > +		old_status = connector->status;
> > > +		connector->status = connector->funcs->detect(connector, false);
> > > +
> > > +		if (old_status != connector->status)
> > > +			changed = true;
> > > +	}
> > > +	mutex_unlock(&mode_config->mutex);
> > >  
> > > -	drm_kms_helper_hotplug_event(dev);
> 
> I just realized that this call here will call down into the fbdev helper,
> which already does the above ->detect calls. At least if there's no
> compositor running. So I wonder why the deadlock ville pointed out isn't
> blowing up already ...
> -Daniel
> 

You are right, that call sequence does end up taking the
mode_config.mutex lock recursively. Are you referring to CI results for
this patch?

-DK

> > > +	if (changed)
> > > +		drm_kms_helper_hotplug_event(dev);
> > >  }
> > >  
> > >  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Daniel Vetter]

On Thu, Nov 17, 2016 at 01:53:31AM +0000, Pandiyan, Dhinakaran wrote:
> On Sun, 2016-11-13 at 11:39 +0100, Daniel Vetter wrote:
> > On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote:
> > > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> > > > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > > > Although the short pulse handling path ends up in the MST hotplug function,
> > > > we do not perform a detect before sending the hotplug uvent. This leads to
> > > > the connector status not being updated when the userspace calls
> > > > DRM_IOCTL_MODE_GETCONNECTOR.
> > > > 
> > > > So, let's call the connector function ->detect() to update the connector
> > > > status before sending the uevent.
> > > > 
> > > > v2: Update connector status inside mode_config mutex (Ville)
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > 
> > > quick comments from me on irc:
> > > 
> > > <danvet> dhnkrn, really tricky I think
> > > <danvet> dhnkrn, also feels wrong from an entire design pov
> > > <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came
> > > <danvet> which means we should be able to get at that connector without taking the global lock
> > > <danvet> i.e. at least avoid the for_each_connector
> > > <danvet> on the detect itself, I need to think
> > > <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker
> > > <danvet> except that it's not a direct hpd from our chip
> > > <danvet> but a downstream one
> > > <danvet> this is going to be fun
> > > <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ?
> > > <danvet> yup
> > > <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports)
> > > <danvet> or at least edid reads
> > > <danvet> both recurse back into dp mst helper code like ville said
> > > <danvet> dhnkrn, so instead of doing the detect work from the hotplug code
> > > <danvet> put them on some list
> > > <danvet> need to be careful in case they are already there
> > > <danvet> list only protected with dedicated lock
> > > <danvet> then launch the hpd worker
> > > <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe
> > > <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo
> > > <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised
> > > <danvet> needs more thought probably
> > > <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex
> > > <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then
> > > 
> > > Cheers, Daniel
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
> > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > index 3ffbd69..8e36a96 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> > > >  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> > > >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > > +	struct intel_connector *intel_connector;
> > > > +	bool changed = false;
> > > > +	enum drm_connector_status old_status;
> > > > +	struct drm_mode_config *mode_config = &dev->mode_config;
> > > > +
> > > > +	mutex_lock(&mode_config->mutex);
> > > > +	for_each_intel_connector(dev, intel_connector) {
> > > > +		struct drm_connector *connector = &(intel_connector->base);
> > > > +
> > > > +		if (intel_connector->mst_port != intel_dp)
> > > > +			continue;
> > > > +
> > > > +		old_status = connector->status;
> > > > +		connector->status = connector->funcs->detect(connector, false);
> > > > +
> > > > +		if (old_status != connector->status)
> > > > +			changed = true;
> > > > +	}
> > > > +	mutex_unlock(&mode_config->mutex);
> > > >  
> > > > -	drm_kms_helper_hotplug_event(dev);
> > 
> > I just realized that this call here will call down into the fbdev helper,
> > which already does the above ->detect calls. At least if there's no
> > compositor running. So I wonder why the deadlock ville pointed out isn't
> > blowing up already ...
> > -Daniel
> > 
> 
> You are right, that call sequence does end up taking the
> mode_config.mutex lock recursively. Are you referring to CI results for
> this patch?

Well I'm confused, since per our discussion here it should blow up, but it
doesn't. What are we missing? CI can't be trusted on this since we can't
(yet) do MST hotplug cycles, hence it doesn't exercise this codepath.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Pandiyan, Dhinakaran]

On Thu, 2016-11-17 at 08:53 +0100, Daniel Vetter wrote:
> On Thu, Nov 17, 2016 at 01:53:31AM +0000, Pandiyan, Dhinakaran wrote:
> > On Sun, 2016-11-13 at 11:39 +0100, Daniel Vetter wrote:
> > > On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote:
> > > > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> > > > > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > > > > Although the short pulse handling path ends up in the MST hotplug function,
> > > > > we do not perform a detect before sending the hotplug uvent. This leads to
> > > > > the connector status not being updated when the userspace calls
> > > > > DRM_IOCTL_MODE_GETCONNECTOR.
> > > > > 
> > > > > So, let's call the connector function ->detect() to update the connector
> > > > > status before sending the uevent.
> > > > > 
> > > > > v2: Update connector status inside mode_config mutex (Ville)
> > > > > 
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > 
> > > > quick comments from me on irc:
> > > > 
> > > > <danvet> dhnkrn, really tricky I think
> > > > <danvet> dhnkrn, also feels wrong from an entire design pov
> > > > <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came
> > > > <danvet> which means we should be able to get at that connector without taking the global lock
> > > > <danvet> i.e. at least avoid the for_each_connector
> > > > <danvet> on the detect itself, I need to think
> > > > <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker
> > > > <danvet> except that it's not a direct hpd from our chip
> > > > <danvet> but a downstream one
> > > > <danvet> this is going to be fun
> > > > <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ?
> > > > <danvet> yup
> > > > <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports)
> > > > <danvet> or at least edid reads
> > > > <danvet> both recurse back into dp mst helper code like ville said
> > > > <danvet> dhnkrn, so instead of doing the detect work from the hotplug code
> > > > <danvet> put them on some list
> > > > <danvet> need to be careful in case they are already there
> > > > <danvet> list only protected with dedicated lock
> > > > <danvet> then launch the hpd worker
> > > > <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe
> > > > <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo
> > > > <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised
> > > > <danvet> needs more thought probably
> > > > <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex
> > > > <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then
> > > > 
> > > > Cheers, Daniel
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
> > > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > index 3ffbd69..8e36a96 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> > > > >  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> > > > >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > > > +	struct intel_connector *intel_connector;
> > > > > +	bool changed = false;
> > > > > +	enum drm_connector_status old_status;
> > > > > +	struct drm_mode_config *mode_config = &dev->mode_config;
> > > > > +
> > > > > +	mutex_lock(&mode_config->mutex);
> > > > > +	for_each_intel_connector(dev, intel_connector) {
> > > > > +		struct drm_connector *connector = &(intel_connector->base);
> > > > > +
> > > > > +		if (intel_connector->mst_port != intel_dp)
> > > > > +			continue;
> > > > > +
> > > > > +		old_status = connector->status;
> > > > > +		connector->status = connector->funcs->detect(connector, false);
> > > > > +
> > > > > +		if (old_status != connector->status)
> > > > > +			changed = true;
> > > > > +	}
> > > > > +	mutex_unlock(&mode_config->mutex);
> > > > >  
> > > > > -	drm_kms_helper_hotplug_event(dev);
> > > 
> > > I just realized that this call here will call down into the fbdev helper,
> > > which already does the above ->detect calls. At least if there's no
> > > compositor running. So I wonder why the deadlock ville pointed out isn't
> > > blowing up already ...
> > > -Daniel
> > > 
> > 
> > You are right, that call sequence does end up taking the
> > mode_config.mutex lock recursively. Are you referring to CI results for
> > this patch?
> 
> Well I'm confused, since per our discussion here it should blow up, but it
> doesn't. What are we missing? CI can't be trusted on this since we can't
> (yet) do MST hotplug cycles, hence it doesn't exercise this codepath.
> -Daniel

The mutex is unlocked after the ->detect()'s that I added are completed.
The hotplug event is sent only after that. Or am I missing something
here?


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx