On Thu, 28 Apr 2016, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Thu, Apr 28, 2016 at 09:17:00AM +0300, Jani Nikula wrote:
>> On Wed, 27 Apr 2016, Lyude <cpaul@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>> > Signed-off-by: Lyude <cpaul@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxx>
>> 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@xxxxxxxxx>
>> 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@xxxxxxx>
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html