| Message ID | 1447931396-19147-1-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote: > Currently a DDI port may register the DP hotplug handler even though > it's used with HDMI, and the DP HPD handler overrides the encoder > type forcibly to DP. This caused the inconsistency on a machine > connected with a HDMI monitor; upon a hotplug event, the DDI port is > suddenly switched to be handled as a DP although the same monitor is > kept connected, and this leads to the erroneous blank output. > > This patch papers over the bug by excluding the previous HDMI encoder > type from this override. This should be fixed more fundamentally, > e.g. by moving the encoder type reset from the HPD or by having > individual encoder objects for HDMI and DP. But since the bug has > been present for a long time (3.17), it's better to have a > quick-n-dirty fix for now, and keep working on a cleaner fix. > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190 > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)') > Cc: <stable@vger.kernel.org> # v3.17+ > Signed-off-by: Takashi Iwai <tiwai@suse.de> If you plug in a hmid screen (using a level-shifter adapter), then a DP screen, does still everything work? We rely upon the detection code to correctly adjust the encoder type, otherwise the subsequent modeset will pick the wrong code (even when userspace asked the kernel to light up the right connector). I'm not sure whether this is now broken ... -Daniel > --- > drivers/gpu/drm/i915/intel_dp.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 09bdd94ca3ba..d34e64300d66 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5153,7 +5153,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > enum intel_display_power_domain power_domain; > enum irqreturn ret = IRQ_NONE; > > - if (intel_dig_port->base.type != INTEL_OUTPUT_EDP) > + if (intel_dig_port->base.type != INTEL_OUTPUT_EDP && > + intel_dig_port->base.type != INTEL_OUTPUT_HDMI) > intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT; > > if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) { > -- > 2.6.3 >
On Thu, 19 Nov 2015 16:51:05 +0100, Daniel Vetter wrote: > > On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote: > > Currently a DDI port may register the DP hotplug handler even though > > it's used with HDMI, and the DP HPD handler overrides the encoder > > type forcibly to DP. This caused the inconsistency on a machine > > connected with a HDMI monitor; upon a hotplug event, the DDI port is > > suddenly switched to be handled as a DP although the same monitor is > > kept connected, and this leads to the erroneous blank output. > > > > This patch papers over the bug by excluding the previous HDMI encoder > > type from this override. This should be fixed more fundamentally, > > e.g. by moving the encoder type reset from the HPD or by having > > individual encoder objects for HDMI and DP. But since the bug has > > been present for a long time (3.17), it's better to have a > > quick-n-dirty fix for now, and keep working on a cleaner fix. > > > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190 > > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)') > > Cc: <stable@vger.kernel.org> # v3.17+ > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > If you plug in a hmid screen (using a level-shifter adapter), then a DP > screen, does still everything work? That was my slight concern, too, although it seems working as long as I tested several HSW machines. Maybe better to double-check. > We rely upon the detection code to correctly adjust the encoder type, > otherwise the subsequent modeset will pick the wrong code (even when > userspace asked the kernel to light up the right connector). I'm not sure > whether this is now broken ... I guess the only case this might matter is MST, as the encoder override didn't exist before the MST support patch, so the non-MST case should keep working even with this change. thanks, Takashi > -Daniel > > > --- > > drivers/gpu/drm/i915/intel_dp.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 09bdd94ca3ba..d34e64300d66 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -5153,7 +5153,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > > enum intel_display_power_domain power_domain; > > enum irqreturn ret = IRQ_NONE; > > > > - if (intel_dig_port->base.type != INTEL_OUTPUT_EDP) > > + if (intel_dig_port->base.type != INTEL_OUTPUT_EDP && > > + intel_dig_port->base.type != INTEL_OUTPUT_HDMI) > > intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT; > > > > if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) { > > -- > > 2.6.3 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch >
On Thu, 19 Nov 2015 17:04:20 +0100, Takashi Iwai wrote: > > On Thu, 19 Nov 2015 16:51:05 +0100, > Daniel Vetter wrote: > > > > On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote: > > > Currently a DDI port may register the DP hotplug handler even though > > > it's used with HDMI, and the DP HPD handler overrides the encoder > > > type forcibly to DP. This caused the inconsistency on a machine > > > connected with a HDMI monitor; upon a hotplug event, the DDI port is > > > suddenly switched to be handled as a DP although the same monitor is > > > kept connected, and this leads to the erroneous blank output. > > > > > > This patch papers over the bug by excluding the previous HDMI encoder > > > type from this override. This should be fixed more fundamentally, > > > e.g. by moving the encoder type reset from the HPD or by having > > > individual encoder objects for HDMI and DP. But since the bug has > > > been present for a long time (3.17), it's better to have a > > > quick-n-dirty fix for now, and keep working on a cleaner fix. > > > > > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190 > > > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)') > > > Cc: <stable@vger.kernel.org> # v3.17+ > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > If you plug in a hmid screen (using a level-shifter adapter), then a DP > > screen, does still everything work? > > That was my slight concern, too, although it seems working as long as > I tested several HSW machines. Maybe better to double-check. I checked again some machines, and they all seem working. Actually the encoder type is set again in each detection function (intel_hdmi_detect(), intel_dp_detect()) no matter with or without my workaround. The problem happens when a HPD is triggered only to DP while the HDMI is kept on. Then intel_hdmi_detect() won't be called so the wrongly overridden encoder type remains. Why the override is still needed in intel_dp_hpd_pulse() isn't clear to me, but at least it influences on the port power domain handling. And my change won't affect in this regard. Takashi > > > We rely upon the detection code to correctly adjust the encoder type, > > otherwise the subsequent modeset will pick the wrong code (even when > > userspace asked the kernel to light up the right connector). I'm not sure > > whether this is now broken ... > > I guess the only case this might matter is MST, as the encoder > override didn't exist before the MST support patch, so the non-MST > case should keep working even with this change. > > > thanks, > > Takashi > > > -Daniel > > > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 09bdd94ca3ba..d34e64300d66 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -5153,7 +5153,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > > > enum intel_display_power_domain power_domain; > > > enum irqreturn ret = IRQ_NONE; > > > > > > - if (intel_dig_port->base.type != INTEL_OUTPUT_EDP) > > > + if (intel_dig_port->base.type != INTEL_OUTPUT_EDP && > > > + intel_dig_port->base.type != INTEL_OUTPUT_HDMI) > > > intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT; > > > > > > if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) { > > > -- > > > 2.6.3 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > >
On Fri, Nov 20, 2015 at 12:18:41PM +0100, Takashi Iwai wrote: > On Thu, 19 Nov 2015 17:04:20 +0100, > Takashi Iwai wrote: > > > > On Thu, 19 Nov 2015 16:51:05 +0100, > > Daniel Vetter wrote: > > > > > > On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote: > > > > Currently a DDI port may register the DP hotplug handler even though > > > > it's used with HDMI, and the DP HPD handler overrides the encoder > > > > type forcibly to DP. This caused the inconsistency on a machine > > > > connected with a HDMI monitor; upon a hotplug event, the DDI port is > > > > suddenly switched to be handled as a DP although the same monitor is > > > > kept connected, and this leads to the erroneous blank output. > > > > > > > > This patch papers over the bug by excluding the previous HDMI encoder > > > > type from this override. This should be fixed more fundamentally, > > > > e.g. by moving the encoder type reset from the HPD or by having > > > > individual encoder objects for HDMI and DP. But since the bug has > > > > been present for a long time (3.17), it's better to have a > > > > quick-n-dirty fix for now, and keep working on a cleaner fix. > > > > > > > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190 > > > > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)') > > > > Cc: <stable@vger.kernel.org> # v3.17+ > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > > > If you plug in a hmid screen (using a level-shifter adapter), then a DP > > > screen, does still everything work? > > > > That was my slight concern, too, although it seems working as long as > > I tested several HSW machines. Maybe better to double-check. > > I checked again some machines, and they all seem working. > > Actually the encoder type is set again in each detection function > (intel_hdmi_detect(), intel_dp_detect()) no matter with or without my > workaround. The problem happens when a HPD is triggered only to DP > while the HDMI is kept on. Then intel_hdmi_detect() won't be called > so the wrongly overridden encoder type remains. > > Why the override is still needed in intel_dp_hpd_pulse() isn't clear > to me, but at least it influences on the port power domain handling. > And my change won't affect in this regard. My suspicion is that we need this to reset between INTEL_OUTPUT_DISPLAYPORT and INTEL_OUTPUT_DP_MST. Thanks for testing this. With the above explanation added to the commit message: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Jani, can you please pick this up? Thanks, Daniel
On Tue, Nov 24, 2015 at 01:44:46PM +0100, Daniel Vetter wrote: > On Fri, Nov 20, 2015 at 12:18:41PM +0100, Takashi Iwai wrote: > > On Thu, 19 Nov 2015 17:04:20 +0100, > > Takashi Iwai wrote: > > > > > > On Thu, 19 Nov 2015 16:51:05 +0100, > > > Daniel Vetter wrote: > > > > > > > > On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote: > > > > > Currently a DDI port may register the DP hotplug handler even though > > > > > it's used with HDMI, and the DP HPD handler overrides the encoder > > > > > type forcibly to DP. This caused the inconsistency on a machine > > > > > connected with a HDMI monitor; upon a hotplug event, the DDI port is > > > > > suddenly switched to be handled as a DP although the same monitor is > > > > > kept connected, and this leads to the erroneous blank output. > > > > > > > > > > This patch papers over the bug by excluding the previous HDMI encoder > > > > > type from this override. This should be fixed more fundamentally, > > > > > e.g. by moving the encoder type reset from the HPD or by having > > > > > individual encoder objects for HDMI and DP. But since the bug has > > > > > been present for a long time (3.17), it's better to have a > > > > > quick-n-dirty fix for now, and keep working on a cleaner fix. > > > > > > > > > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190 > > > > > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)') > > > > > Cc: <stable@vger.kernel.org> # v3.17+ > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > > > > > If you plug in a hmid screen (using a level-shifter adapter), then a DP > > > > screen, does still everything work? > > > > > > That was my slight concern, too, although it seems working as long as > > > I tested several HSW machines. Maybe better to double-check. > > > > I checked again some machines, and they all seem working. > > > > Actually the encoder type is set again in each detection function > > (intel_hdmi_detect(), intel_dp_detect()) no matter with or without my > > workaround. The problem happens when a HPD is triggered only to DP > > while the HDMI is kept on. Then intel_hdmi_detect() won't be called > > so the wrongly overridden encoder type remains. > > > > Why the override is still needed in intel_dp_hpd_pulse() isn't clear > > to me, but at least it influences on the port power domain handling. > > And my change won't affect in this regard. > > My suspicion is that we need this to reset between > INTEL_OUTPUT_DISPLAYPORT and INTEL_OUTPUT_DP_MST. INTEL_OUTPUT_DP_MST is the type for fake mst encoders, and those don't have a .hpd_pulse() hook to begin with.
On Tue, Nov 24, 2015 at 03:08:17PM +0200, Ville Syrjälä wrote: > On Tue, Nov 24, 2015 at 01:44:46PM +0100, Daniel Vetter wrote: > > On Fri, Nov 20, 2015 at 12:18:41PM +0100, Takashi Iwai wrote: > > > On Thu, 19 Nov 2015 17:04:20 +0100, > > > Takashi Iwai wrote: > > > > > > > > On Thu, 19 Nov 2015 16:51:05 +0100, > > > > Daniel Vetter wrote: > > > > > > > > > > On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote: > > > > > > Currently a DDI port may register the DP hotplug handler even though > > > > > > it's used with HDMI, and the DP HPD handler overrides the encoder > > > > > > type forcibly to DP. This caused the inconsistency on a machine > > > > > > connected with a HDMI monitor; upon a hotplug event, the DDI port is > > > > > > suddenly switched to be handled as a DP although the same monitor is > > > > > > kept connected, and this leads to the erroneous blank output. > > > > > > > > > > > > This patch papers over the bug by excluding the previous HDMI encoder > > > > > > type from this override. This should be fixed more fundamentally, > > > > > > e.g. by moving the encoder type reset from the HPD or by having > > > > > > individual encoder objects for HDMI and DP. But since the bug has > > > > > > been present for a long time (3.17), it's better to have a > > > > > > quick-n-dirty fix for now, and keep working on a cleaner fix. > > > > > > > > > > > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190 > > > > > > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)') > > > > > > Cc: <stable@vger.kernel.org> # v3.17+ > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > > > > > > > If you plug in a hmid screen (using a level-shifter adapter), then a DP > > > > > screen, does still everything work? > > > > > > > > That was my slight concern, too, although it seems working as long as > > > > I tested several HSW machines. Maybe better to double-check. > > > > > > I checked again some machines, and they all seem working. > > > > > > Actually the encoder type is set again in each detection function > > > (intel_hdmi_detect(), intel_dp_detect()) no matter with or without my > > > workaround. The problem happens when a HPD is triggered only to DP > > > while the HDMI is kept on. Then intel_hdmi_detect() won't be called > > > so the wrongly overridden encoder type remains. > > > > > > Why the override is still needed in intel_dp_hpd_pulse() isn't clear > > > to me, but at least it influences on the port power domain handling. > > > And my change won't affect in this regard. > > > > My suspicion is that we need this to reset between > > INTEL_OUTPUT_DISPLAYPORT and INTEL_OUTPUT_DP_MST. > > INTEL_OUTPUT_DP_MST is the type for fake mst encoders, and those > don't have a .hpd_pulse() hook to begin with. Hm ... that would mean we only need this for the initial hotplug when we go _UNKOWN -> _DISPLAYPORT. Oh well, putting it into the state properly, derived from what userspace wants, should be the proper fix anyway. But for stable this is good enough. -Daniel
On Tue, 24 Nov 2015, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Nov 24, 2015 at 03:08:17PM +0200, Ville Syrjälä wrote: >> On Tue, Nov 24, 2015 at 01:44:46PM +0100, Daniel Vetter wrote: >> > On Fri, Nov 20, 2015 at 12:18:41PM +0100, Takashi Iwai wrote: >> > > On Thu, 19 Nov 2015 17:04:20 +0100, >> > > Takashi Iwai wrote: >> > > > >> > > > On Thu, 19 Nov 2015 16:51:05 +0100, >> > > > Daniel Vetter wrote: >> > > > > >> > > > > On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote: >> > > > > > Currently a DDI port may register the DP hotplug handler even though >> > > > > > it's used with HDMI, and the DP HPD handler overrides the encoder >> > > > > > type forcibly to DP. This caused the inconsistency on a machine >> > > > > > connected with a HDMI monitor; upon a hotplug event, the DDI port is >> > > > > > suddenly switched to be handled as a DP although the same monitor is >> > > > > > kept connected, and this leads to the erroneous blank output. >> > > > > > >> > > > > > This patch papers over the bug by excluding the previous HDMI encoder >> > > > > > type from this override. This should be fixed more fundamentally, >> > > > > > e.g. by moving the encoder type reset from the HPD or by having >> > > > > > individual encoder objects for HDMI and DP. But since the bug has >> > > > > > been present for a long time (3.17), it's better to have a >> > > > > > quick-n-dirty fix for now, and keep working on a cleaner fix. >> > > > > > >> > > > > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190 >> > > > > > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)') >> > > > > > Cc: <stable@vger.kernel.org> # v3.17+ >> > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> >> > > > > >> > > > > If you plug in a hmid screen (using a level-shifter adapter), then a DP >> > > > > screen, does still everything work? >> > > > >> > > > That was my slight concern, too, although it seems working as long as >> > > > I tested several HSW machines. Maybe better to double-check. >> > > >> > > I checked again some machines, and they all seem working. >> > > >> > > Actually the encoder type is set again in each detection function >> > > (intel_hdmi_detect(), intel_dp_detect()) no matter with or without my >> > > workaround. The problem happens when a HPD is triggered only to DP >> > > while the HDMI is kept on. Then intel_hdmi_detect() won't be called >> > > so the wrongly overridden encoder type remains. >> > > >> > > Why the override is still needed in intel_dp_hpd_pulse() isn't clear >> > > to me, but at least it influences on the port power domain handling. >> > > And my change won't affect in this regard. >> > >> > My suspicion is that we need this to reset between >> > INTEL_OUTPUT_DISPLAYPORT and INTEL_OUTPUT_DP_MST. >> >> INTEL_OUTPUT_DP_MST is the type for fake mst encoders, and those >> don't have a .hpd_pulse() hook to begin with. > > Hm ... that would mean we only need this for the initial hotplug when we > go _UNKOWN -> _DISPLAYPORT. Oh well, putting it into the state properly, > derived from what userspace wants, should be the proper fix anyway. But > for stable this is good enough. Pushed to drm-intel-fixes, thanks for the patch and review. BR, Jani.
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 09bdd94ca3ba..d34e64300d66 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5153,7 +5153,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) enum intel_display_power_domain power_domain; enum irqreturn ret = IRQ_NONE; - if (intel_dig_port->base.type != INTEL_OUTPUT_EDP) + if (intel_dig_port->base.type != INTEL_OUTPUT_EDP && + intel_dig_port->base.type != INTEL_OUTPUT_HDMI) intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT; if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
Currently a DDI port may register the DP hotplug handler even though it's used with HDMI, and the DP HPD handler overrides the encoder type forcibly to DP. This caused the inconsistency on a machine connected with a HDMI monitor; upon a hotplug event, the DDI port is suddenly switched to be handled as a DP although the same monitor is kept connected, and this leads to the erroneous blank output. This patch papers over the bug by excluding the previous HDMI encoder type from this override. This should be fixed more fundamentally, e.g. by moving the encoder type reset from the HPD or by having individual encoder objects for HDMI and DP. But since the bug has been present for a long time (3.17), it's better to have a quick-n-dirty fix for now, and keep working on a cleaner fix. Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190 Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)') Cc: <stable@vger.kernel.org> # v3.17+ Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/gpu/drm/i915/intel_dp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)