* [PATCH] drm/i915: Fix enc_to_dig_port() for MST encoders
@ 2016-04-27 17:34 Lyude
2016-04-28 6:17 ` Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Lyude @ 2016-04-27 17:34 UTC (permalink / raw)
To: intel-gfx, Rob Clark
Cc: Lyude, stable, Daniel Vetter, Jani Nikula, David Airlie,
open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list)
For MST encoders, the encoder struct is stored in the intel_dp_mst
struct, not a intel_digital_port struct.
This fixes issues with hotplugging MST displays that support MST audio,
where hotplugging had a surprisingly good chance of accidentally
overwriting other parts of the kernel leading to seemingly unrelated
backtraces in sysfs, ext4, etc.
Cc: stable@vger.kernel.org
Signed-off-by: Lyude <cpaul@redhat.com>
---
drivers/gpu/drm/i915/intel_drv.h | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4c027d6..81f2212 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -918,18 +918,23 @@ intel_attached_encoder(struct drm_connector *connector)
return to_intel_connector(connector)->encoder;
}
-static inline struct intel_digital_port *
-enc_to_dig_port(struct drm_encoder *encoder)
-{
- return container_of(encoder, struct intel_digital_port, base.base);
-}
-
static inline struct intel_dp_mst_encoder *
enc_to_mst(struct drm_encoder *encoder)
{
return container_of(encoder, struct intel_dp_mst_encoder, base.base);
}
+static inline struct intel_digital_port *
+enc_to_dig_port(struct drm_encoder *encoder)
+{
+ if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST)
+ return enc_to_mst(encoder)->primary;
+ else {
+ return container_of(encoder, struct intel_digital_port,
+ base.base);
+ }
+}
+
static inline struct intel_dp *enc_to_intel_dp(struct drm_encoder *encoder)
{
return &enc_to_dig_port(encoder)->dp;
--
2.5.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Fix enc_to_dig_port() for MST encoders
2016-04-27 17:34 [PATCH] drm/i915: Fix enc_to_dig_port() for MST encoders Lyude
@ 2016-04-28 6:17 ` Jani Nikula
2016-04-28 10:40 ` [Intel-gfx] " Ville Syrjälä
0 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2016-04-28 6:17 UTC (permalink / raw)
To: Lyude, intel-gfx, Rob Clark
Cc: Lyude, stable, Daniel Vetter, David Airlie,
open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list)
On Wed, 27 Apr 2016, Lyude <cpaul@redhat.com> wrote:
> For MST encoders, the encoder struct is stored in the intel_dp_mst
> struct, not a intel_digital_port struct.
>
> This fixes issues with hotplugging MST displays that support MST audio,
> where hotplugging had a surprisingly good chance of accidentally
> overwriting other parts of the kernel leading to seemingly unrelated
> backtraces in sysfs, ext4, etc.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lyude <cpaul@redhat.com>
Ugh. Just ugh.
At a glance, it's probably this one that starts calling
intel_audio_codec_enable() and intel_audio_codec_disable() on DP MST:
commit 3d52ccf52f2c51f613e42e65be0f06e4e6788093
Author: Libin Yang <libin.yang@linux.intel.com>
Date: Wed Dec 2 14:09:44 2015 +0800
drm/i915: start adding dp mst audio
which has been there since v4.5. Would it be feasible for you to revert
the commit or change it so that it stops calling the audio codec
enable/disable, to verify this is the culprit? So we could add a proper
Fixes: line too.
The non-MST aware enc_to_dig_port() calls on platforms that might have
MST were added in:
commit 51e1d83cab9988716ae68801a721f4df0aaa374b
Author: David Henningsson <david.henningsson@canonical.com>
Date: Wed Aug 19 10:48:56 2015 +0200
drm/i915: Call audio pin/ELD notify function
and:
commit 7e8275c2f2bbb384e18af37066b8b2f32b7d092f
Author: Libin Yang <libin.yang@intel.com>
Date: Fri Sep 25 09:36:12 2015 +0800
drm/i915: set proper N/CTS in modeset
but I don't think the codec enable/disable were called on MST at that
time.
Some of the problem was probably seen by Takashi in
commit 0bdf5a05647a66dcc6394986e061daeac9b1cf96
Author: Takashi Iwai <tiwai@suse.de>
Date: Mon Nov 30 18:19:39 2015 +0100
drm/i915: Add reverse mapping between port and intel_encoder
which has a comment /* intel_encoder might be NULL for DP MST */. Maybe
that needs to be updated to DTRT too.
Lyude, thanks for your efforts in DP MST area. Much appreciated.
BR,
Jani.
> ---
> drivers/gpu/drm/i915/intel_drv.h | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4c027d6..81f2212 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -918,18 +918,23 @@ intel_attached_encoder(struct drm_connector *connector)
> return to_intel_connector(connector)->encoder;
> }
>
> -static inline struct intel_digital_port *
> -enc_to_dig_port(struct drm_encoder *encoder)
> -{
> - return container_of(encoder, struct intel_digital_port, base.base);
> -}
> -
> static inline struct intel_dp_mst_encoder *
> enc_to_mst(struct drm_encoder *encoder)
> {
> return container_of(encoder, struct intel_dp_mst_encoder, base.base);
> }
>
> +static inline struct intel_digital_port *
> +enc_to_dig_port(struct drm_encoder *encoder)
> +{
> + if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST)
> + return enc_to_mst(encoder)->primary;
> + else {
> + return container_of(encoder, struct intel_digital_port,
> + base.base);
> + }
> +}
> +
> static inline struct intel_dp *enc_to_intel_dp(struct drm_encoder *encoder)
> {
> return &enc_to_dig_port(encoder)->dp;
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix enc_to_dig_port() for MST encoders
2016-04-28 6:17 ` Jani Nikula
@ 2016-04-28 10:40 ` Ville Syrjälä
2016-04-28 12:23 ` Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2016-04-28 10:40 UTC (permalink / raw)
To: Jani Nikula
Cc: Lyude, intel-gfx, Rob Clark, David Airlie, Daniel Vetter,
open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list),
stable
On Thu, Apr 28, 2016 at 09:17:00AM +0300, Jani Nikula wrote:
> On Wed, 27 Apr 2016, Lyude <cpaul@redhat.com> wrote:
> > For MST encoders, the encoder struct is stored in the intel_dp_mst
> > struct, not a intel_digital_port struct.
> >
> > This fixes issues with hotplugging MST displays that support MST audio,
> > where hotplugging had a surprisingly good chance of accidentally
> > overwriting other parts of the kernel leading to seemingly unrelated
> > backtraces in sysfs, ext4, etc.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Lyude <cpaul@redhat.com>
>
> Ugh. Just ugh.
>
> At a glance, it's probably this one that starts calling
> intel_audio_codec_enable() and intel_audio_codec_disable() on DP MST:
>
> commit 3d52ccf52f2c51f613e42e65be0f06e4e6788093
> Author: Libin Yang <libin.yang@linux.intel.com>
> Date: Wed Dec 2 14:09:44 2015 +0800
>
> drm/i915: start adding dp mst audio
>
> which has been there since v4.5. Would it be feasible for you to revert
> the commit or change it so that it stops calling the audio codec
> enable/disable, to verify this is the culprit? So we could add a proper
> Fixes: line too.
I don't particularly like making enc_to_dig_port() work on MST encoders
because it'll likely result in the wrong thing anyway. I already asked
before how MST audio port handling is supposed to work, but never got
any answer. Right now, if you just use the ->primary dig_port in the
audio paths, there's no way audio can work correctly with MST, at least
if you enable multiple streams on the same port.
So what I would do is just revert the audio calls from the MST paths and
go back to the drawing board.
>
> The non-MST aware enc_to_dig_port() calls on platforms that might have
> MST were added in:
>
> commit 51e1d83cab9988716ae68801a721f4df0aaa374b
> Author: David Henningsson <david.henningsson@canonical.com>
> Date: Wed Aug 19 10:48:56 2015 +0200
>
> drm/i915: Call audio pin/ELD notify function
>
> and:
>
> commit 7e8275c2f2bbb384e18af37066b8b2f32b7d092f
> Author: Libin Yang <libin.yang@intel.com>
> Date: Fri Sep 25 09:36:12 2015 +0800
>
> drm/i915: set proper N/CTS in modeset
>
> but I don't think the codec enable/disable were called on MST at that
> time.
>
> Some of the problem was probably seen by Takashi in
>
> commit 0bdf5a05647a66dcc6394986e061daeac9b1cf96
> Author: Takashi Iwai <tiwai@suse.de>
> Date: Mon Nov 30 18:19:39 2015 +0100
>
> drm/i915: Add reverse mapping between port and intel_encoder
>
> which has a comment /* intel_encoder might be NULL for DP MST */. Maybe
> that needs to be updated to DTRT too.
>
>
> Lyude, thanks for your efforts in DP MST area. Much appreciated.
>
>
> BR,
> Jani.
>
>
>
> > ---
> > drivers/gpu/drm/i915/intel_drv.h | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 4c027d6..81f2212 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -918,18 +918,23 @@ intel_attached_encoder(struct drm_connector *connector)
> > return to_intel_connector(connector)->encoder;
> > }
> >
> > -static inline struct intel_digital_port *
> > -enc_to_dig_port(struct drm_encoder *encoder)
> > -{
> > - return container_of(encoder, struct intel_digital_port, base.base);
> > -}
> > -
> > static inline struct intel_dp_mst_encoder *
> > enc_to_mst(struct drm_encoder *encoder)
> > {
> > return container_of(encoder, struct intel_dp_mst_encoder, base.base);
> > }
> >
> > +static inline struct intel_digital_port *
> > +enc_to_dig_port(struct drm_encoder *encoder)
> > +{
> > + if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST)
> > + return enc_to_mst(encoder)->primary;
> > + else {
> > + return container_of(encoder, struct intel_digital_port,
> > + base.base);
> > + }
> > +}
> > +
> > static inline struct intel_dp *enc_to_intel_dp(struct drm_encoder *encoder)
> > {
> > return &enc_to_dig_port(encoder)->dp;
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrj�l�
Intel OTC
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix enc_to_dig_port() for MST encoders
2016-04-28 10:40 ` [Intel-gfx] " Ville Syrjälä
@ 2016-04-28 12:23 ` Jani Nikula
2016-05-02 15:11 ` Lyude Paul
0 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2016-04-28 12:23 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Lyude, intel-gfx, Rob Clark, David Airlie, Daniel Vetter,
open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list),
stable
On Thu, 28 Apr 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Apr 28, 2016 at 09:17:00AM +0300, Jani Nikula wrote:
>> On Wed, 27 Apr 2016, Lyude <cpaul@redhat.com> wrote:
>> > For MST encoders, the encoder struct is stored in the intel_dp_mst
>> > struct, not a intel_digital_port struct.
>> >
>> > This fixes issues with hotplugging MST displays that support MST audio,
>> > where hotplugging had a surprisingly good chance of accidentally
>> > overwriting other parts of the kernel leading to seemingly unrelated
>> > backtraces in sysfs, ext4, etc.
>> >
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Lyude <cpaul@redhat.com>
>>
>> Ugh. Just ugh.
>>
>> At a glance, it's probably this one that starts calling
>> intel_audio_codec_enable() and intel_audio_codec_disable() on DP MST:
>>
>> commit 3d52ccf52f2c51f613e42e65be0f06e4e6788093
>> Author: Libin Yang <libin.yang@linux.intel.com>
>> Date: Wed Dec 2 14:09:44 2015 +0800
>>
>> drm/i915: start adding dp mst audio
>>
>> which has been there since v4.5. Would it be feasible for you to revert
>> the commit or change it so that it stops calling the audio codec
>> enable/disable, to verify this is the culprit? So we could add a proper
>> Fixes: line too.
>
> I don't particularly like making enc_to_dig_port() work on MST encoders
> because it'll likely result in the wrong thing anyway. I already asked
> before how MST audio port handling is supposed to work, but never got
> any answer. Right now, if you just use the ->primary dig_port in the
> audio paths, there's no way audio can work correctly with MST, at least
> if you enable multiple streams on the same port.
>
> So what I would do is just revert the audio calls from the MST paths and
> go back to the drawing board.
I don't mind that approach either.
I was thinking we should probably add some dynamic type checking to
enc_to_dig_port to catch this type of bugs.
BR,
Jani.
>
>>
>> The non-MST aware enc_to_dig_port() calls on platforms that might have
>> MST were added in:
>>
>> commit 51e1d83cab9988716ae68801a721f4df0aaa374b
>> Author: David Henningsson <david.henningsson@canonical.com>
>> Date: Wed Aug 19 10:48:56 2015 +0200
>>
>> drm/i915: Call audio pin/ELD notify function
>>
>> and:
>>
>> commit 7e8275c2f2bbb384e18af37066b8b2f32b7d092f
>> Author: Libin Yang <libin.yang@intel.com>
>> Date: Fri Sep 25 09:36:12 2015 +0800
>>
>> drm/i915: set proper N/CTS in modeset
>>
>> but I don't think the codec enable/disable were called on MST at that
>> time.
>>
>> Some of the problem was probably seen by Takashi in
>>
>> commit 0bdf5a05647a66dcc6394986e061daeac9b1cf96
>> Author: Takashi Iwai <tiwai@suse.de>
>> Date: Mon Nov 30 18:19:39 2015 +0100
>>
>> drm/i915: Add reverse mapping between port and intel_encoder
>>
>> which has a comment /* intel_encoder might be NULL for DP MST */. Maybe
>> that needs to be updated to DTRT too.
>>
>>
>> Lyude, thanks for your efforts in DP MST area. Much appreciated.
>>
>>
>> BR,
>> Jani.
>>
>>
>>
>> > ---
>> > drivers/gpu/drm/i915/intel_drv.h | 17 +++++++++++------
>> > 1 file changed, 11 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 4c027d6..81f2212 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -918,18 +918,23 @@ intel_attached_encoder(struct drm_connector *connector)
>> > return to_intel_connector(connector)->encoder;
>> > }
>> >
>> > -static inline struct intel_digital_port *
>> > -enc_to_dig_port(struct drm_encoder *encoder)
>> > -{
>> > - return container_of(encoder, struct intel_digital_port, base.base);
>> > -}
>> > -
>> > static inline struct intel_dp_mst_encoder *
>> > enc_to_mst(struct drm_encoder *encoder)
>> > {
>> > return container_of(encoder, struct intel_dp_mst_encoder, base.base);
>> > }
>> >
>> > +static inline struct intel_digital_port *
>> > +enc_to_dig_port(struct drm_encoder *encoder)
>> > +{
>> > + if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST)
>> > + return enc_to_mst(encoder)->primary;
>> > + else {
>> > + return container_of(encoder, struct intel_digital_port,
>> > + base.base);
>> > + }
>> > +}
>> > +
>> > static inline struct intel_dp *enc_to_intel_dp(struct drm_encoder *encoder)
>> > {
>> > return &enc_to_dig_port(encoder)->dp;
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix enc_to_dig_port() for MST encoders
2016-04-28 12:23 ` Jani Nikula
@ 2016-05-02 15:11 ` Lyude Paul
2016-05-02 20:24 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Lyude Paul @ 2016-05-02 15:11 UTC (permalink / raw)
To: Jani Nikula, Ville Syrjälä
Cc: intel-gfx, Rob Clark, David Airlie, Daniel Vetter,
open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list),
stable
I gave a short try at fixing MST audio, but it definitely looks like it's going
to require quite a bit of troubleshooting and a few more patches :(.
Since I can't find an immediate fix to actually make MST audio work I'm totally
in favor of reverting the MST audio support for the time being
On Thu, 2016-04-28 at 15:23 +0300, Jani Nikula wrote:
> On Thu, 28 Apr 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >
> > On Thu, Apr 28, 2016 at 09:17:00AM +0300, Jani Nikula wrote:
> > >
> > > On Wed, 27 Apr 2016, Lyude <cpaul@redhat.com> wrote:
> > > >
> > > > For MST encoders, the encoder struct is stored in the intel_dp_mst
> > > > struct, not a intel_digital_port struct.
> > > >
> > > > This fixes issues with hotplugging MST displays that support MST audio,
> > > > where hotplugging had a surprisingly good chance of accidentally
> > > > overwriting other parts of the kernel leading to seemingly unrelated
> > > > backtraces in sysfs, ext4, etc.
> > > >
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > Ugh. Just ugh.
> > >
> > > At a glance, it's probably this one that starts calling
> > > intel_audio_codec_enable() and intel_audio_codec_disable() on DP MST:
> > >
> > > commit 3d52ccf52f2c51f613e42e65be0f06e4e6788093
> > > Author: Libin Yang <libin.yang@linux.intel.com>
> > > Date: Wed Dec 2 14:09:44 2015 +0800
> > >
> > > drm/i915: start adding dp mst audio
> > >
> > > which has been there since v4.5. Would it be feasible for you to revert
> > > the commit or change it so that it stops calling the audio codec
> > > enable/disable, to verify this is the culprit? So we could add a proper
> > > Fixes: line too.
> > I don't particularly like making enc_to_dig_port() work on MST encoders
> > because it'll likely result in the wrong thing anyway. I already asked
> > before how MST audio port handling is supposed to work, but never got
> > any answer. Right now, if you just use the ->primary dig_port in the
> > audio paths, there's no way audio can work correctly with MST, at least
> > if you enable multiple streams on the same port.
> >
> > So what I would do is just revert the audio calls from the MST paths and
> > go back to the drawing board.
> I don't mind that approach either.
>
> I was thinking we should probably add some dynamic type checking to
> enc_to_dig_port to catch this type of bugs.
>
> BR,
> Jani.
>
> >
> >
> > >
> > >
> > > The non-MST aware enc_to_dig_port() calls on platforms that might have
> > > MST were added in:
> > >
> > > commit 51e1d83cab9988716ae68801a721f4df0aaa374b
> > > Author: David Henningsson <david.henningsson@canonical.com>
> > > Date: Wed Aug 19 10:48:56 2015 +0200
> > >
> > > drm/i915: Call audio pin/ELD notify function
> > >
> > > and:
> > >
> > > commit 7e8275c2f2bbb384e18af37066b8b2f32b7d092f
> > > Author: Libin Yang <libin.yang@intel.com>
> > > Date: Fri Sep 25 09:36:12 2015 +0800
> > >
> > > drm/i915: set proper N/CTS in modeset
> > >
> > > but I don't think the codec enable/disable were called on MST at that
> > > time.
> > >
> > > Some of the problem was probably seen by Takashi in
> > >
> > > commit 0bdf5a05647a66dcc6394986e061daeac9b1cf96
> > > Author: Takashi Iwai <tiwai@suse.de>
> > > Date: Mon Nov 30 18:19:39 2015 +0100
> > >
> > > drm/i915: Add reverse mapping between port and intel_encoder
> > >
> > > which has a comment /* intel_encoder might be NULL for DP MST */. Maybe
> > > that needs to be updated to DTRT too.
> > >
> > >
> > > Lyude, thanks for your efforts in DP MST area. Much appreciated.
> > >
> > >
> > > BR,
> > > Jani.
> > >
> > >
> > >
> > > >
> > > > ---
> > > > drivers/gpu/drm/i915/intel_drv.h | 17 +++++++++++------
> > > > 1 file changed, 11 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 4c027d6..81f2212 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -918,18 +918,23 @@ intel_attached_encoder(struct drm_connector
> > > > *connector)
> > > > return to_intel_connector(connector)->encoder;
> > > > }
> > > >
> > > > -static inline struct intel_digital_port *
> > > > -enc_to_dig_port(struct drm_encoder *encoder)
> > > > -{
> > > > - return container_of(encoder, struct intel_digital_port,
> > > > base.base);
> > > > -}
> > > > -
> > > > static inline struct intel_dp_mst_encoder *
> > > > enc_to_mst(struct drm_encoder *encoder)
> > > > {
> > > > return container_of(encoder, struct intel_dp_mst_encoder,
> > > > base.base);
> > > > }
> > > >
> > > > +static inline struct intel_digital_port *
> > > > +enc_to_dig_port(struct drm_encoder *encoder)
> > > > +{
> > > > + if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST)
> > > > + return enc_to_mst(encoder)->primary;
> > > > + else {
> > > > + return container_of(encoder, struct intel_digital_port,
> > > > + base.base);
> > > > + }
> > > > +}
> > > > +
> > > > static inline struct intel_dp *enc_to_intel_dp(struct drm_encoder
> > > > *encoder)
> > > > {
> > > > return &enc_to_dig_port(encoder)->dp;
> > > --
> > > Jani Nikula, Intel Open Source Technology Center
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix enc_to_dig_port() for MST encoders
2016-05-02 15:11 ` Lyude Paul
@ 2016-05-02 20:24 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2016-05-02 20:24 UTC (permalink / raw)
To: Lyude Paul
Cc: Jani Nikula, Ville Syrjälä, Rob Clark, intel-gfx,
stable,
open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list),
Daniel Vetter
On Mon, May 02, 2016 at 11:11:31AM -0400, Lyude Paul wrote:
> I gave a short try at fixing MST audio, but it definitely looks like it's going
> to require quite a bit of troubleshooting and a few more patches :(.
>
> Since I can't find an immediate fix to actually make MST audio work I'm totally
> in favor of reverting the MST audio support for the time being
Can you please send in that revert?
Thanks, Daniel
>
> On Thu, 2016-04-28 at 15:23 +0300, Jani Nikula wrote:
> > On Thu, 28 Apr 2016, Ville Syrj�l� <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Thu, Apr 28, 2016 at 09:17:00AM +0300, Jani Nikula wrote:
> > > >
> > > > On Wed, 27 Apr 2016, Lyude <cpaul@redhat.com> wrote:
> > > > >
> > > > > For MST encoders, the encoder struct is stored in the intel_dp_mst
> > > > > struct, not a intel_digital_port struct.
> > > > >
> > > > > This fixes issues with hotplugging MST displays that support MST audio,
> > > > > where hotplugging had a surprisingly good chance of accidentally
> > > > > overwriting other parts of the kernel leading to seemingly unrelated
> > > > > backtraces in sysfs, ext4, etc.
> > > > >
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > > Ugh. Just ugh.
> > > >
> > > > At a glance, it's probably this one that starts calling
> > > > intel_audio_codec_enable() and intel_audio_codec_disable() on DP MST:
> > > >
> > > > commit 3d52ccf52f2c51f613e42e65be0f06e4e6788093
> > > > Author: Libin Yang <libin.yang@linux.intel.com>
> > > > Date:���Wed Dec 2 14:09:44 2015 +0800
> > > >
> > > > ����drm/i915: start adding dp mst audio
> > > >
> > > > which has been there since v4.5. Would it be feasible for you to revert
> > > > the commit or change it so that it stops calling the audio codec
> > > > enable/disable, to verify this is the culprit? So we could add a proper
> > > > Fixes: line too.
> > > I don't particularly like making enc_to_dig_port() work on MST encoders
> > > because it'll likely result in the wrong thing anyway. I already asked
> > > before how MST audio port handling is supposed to work, but never got
> > > any answer. Right now, if you just use the ->primary dig_port in the
> > > audio paths, there's no way audio can work correctly with MST, at least
> > > if you enable multiple streams on the same port.
> > >
> > > So what I would do is just revert the audio calls from the MST paths and
> > > go back to the drawing board.
> > I don't mind that approach either.
> >
> > I was thinking we should probably add some dynamic type checking to
> > enc_to_dig_port to catch this type of bugs.
> >
> > BR,
> > Jani.
> >
> > >
> > >
> > > >
> > > >
> > > > The non-MST aware enc_to_dig_port() calls on platforms that might have
> > > > MST were added in:
> > > >
> > > > commit 51e1d83cab9988716ae68801a721f4df0aaa374b
> > > > Author: David Henningsson <david.henningsson@canonical.com>
> > > > Date:���Wed Aug 19 10:48:56 2015 +0200
> > > >
> > > > ����drm/i915: Call audio pin/ELD notify function
> > > >
> > > > and:
> > > >
> > > > commit 7e8275c2f2bbb384e18af37066b8b2f32b7d092f
> > > > Author: Libin Yang <libin.yang@intel.com>
> > > > Date:���Fri Sep 25 09:36:12 2015 +0800
> > > >
> > > > ����drm/i915: set proper N/CTS in modeset
> > > >
> > > > but I don't think the codec enable/disable were called on MST at that
> > > > time.
> > > >
> > > > Some of the problem was probably seen by Takashi in
> > > >
> > > > commit 0bdf5a05647a66dcc6394986e061daeac9b1cf96
> > > > Author: Takashi Iwai <tiwai@suse.de>
> > > > Date:���Mon Nov 30 18:19:39 2015 +0100
> > > >
> > > > ����drm/i915: Add reverse mapping between port and intel_encoder
> > > >
> > > > which has a comment /* intel_encoder might be NULL for DP MST */. Maybe
> > > > that needs to be updated to DTRT too.
> > > >
> > > >
> > > > Lyude, thanks for your efforts in DP MST area. Much appreciated.
> > > >
> > > >
> > > > BR,
> > > > Jani.
> > > >
> > > >
> > > >
> > > > >
> > > > > ---
> > > > > �drivers/gpu/drm/i915/intel_drv.h | 17 +++++++++++------
> > > > > �1 file changed, 11 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 4c027d6..81f2212 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -918,18 +918,23 @@ intel_attached_encoder(struct drm_connector
> > > > > *connector)
> > > > > � return to_intel_connector(connector)->encoder;
> > > > > �}
> > > > > �
> > > > > -static inline struct intel_digital_port *
> > > > > -enc_to_dig_port(struct drm_encoder *encoder)
> > > > > -{
> > > > > - return container_of(encoder, struct intel_digital_port,
> > > > > base.base);
> > > > > -}
> > > > > -
> > > > > �static inline struct intel_dp_mst_encoder *
> > > > > �enc_to_mst(struct drm_encoder *encoder)
> > > > > �{
> > > > > � return container_of(encoder, struct intel_dp_mst_encoder,
> > > > > base.base);
> > > > > �}
> > > > > �
> > > > > +static inline struct intel_digital_port *
> > > > > +enc_to_dig_port(struct drm_encoder *encoder)
> > > > > +{
> > > > > + if (encoder->encoder_type == DRM_MODE_ENCODER_DPMST)
> > > > > + return enc_to_mst(encoder)->primary;
> > > > > + else {
> > > > > + return container_of(encoder, struct intel_digital_port,
> > > > > + ����base.base);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > �static inline struct intel_dp *enc_to_intel_dp(struct drm_encoder
> > > > > *encoder)
> > > > > �{
> > > > > � return &enc_to_dig_port(encoder)->dp;
> > > > --�
> > > > Jani Nikula, Intel Open Source Technology Center
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-02 20:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-27 17:34 [PATCH] drm/i915: Fix enc_to_dig_port() for MST encoders Lyude
2016-04-28 6:17 ` Jani Nikula
2016-04-28 10:40 ` [Intel-gfx] " Ville Syrjälä
2016-04-28 12:23 ` Jani Nikula
2016-05-02 15:11 ` Lyude Paul
2016-05-02 20:24 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).