[PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Lyude]

As it turns out, resuming DP MST is racey since we don't make sure MST
is ready before we start modesetting, it just usually happens to be
ready in time. This isn't the case on all systems, particularly a
ThinkPad T560 with displays connected through the dock. On these
systems, resuming the laptop while connected to the dock usually results
in blank monitors. Making sure MST is ready before doing any kind of
modesetting fixes this issue.

We originally changed the resume order in

	commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")

to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
issue now anyhow.

CC: stable@vger.kernel.org
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f357058..4dcf3dd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_opregion_setup(dev);
 
 	intel_init_pch_refclk(dev);
+
+	/*
+	 * We need to make sure that we resume MST before doing anything
+	 * display related, otherwise we risk trying to bring up a display on
+	 * MST before the hub is actually ready
+	 */
+	intel_dp_mst_resume(dev);
+
 	drm_mode_config_reset(dev);
 
 	/*
@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_display_resume(dev);
 	drm_modeset_unlock_all(dev);
 
-	intel_dp_mst_resume(dev);
-
 	/*
 	 * ... but also need to make sure that hotplug processing
 	 * doesn't cause havoc. Like in the driver load code we don't
-- 
2.5.0

[PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Lyude]

As it turns out, resuming DP MST is racey since we don't make sure MST
is ready before we start modesetting, it just usually happens to be
ready in time. This isn't the case on all systems, particularly a
ThinkPad T560 with displays connected through the dock. On these
systems, resuming the laptop while connected to the dock usually results
in blank monitors. Making sure MST is ready before doing any kind of
modesetting fixes this issue.

We originally changed the resume order in

	commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")

to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
issue now anyhow.

CC: stable@vger.kernel.org
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f357058..4dcf3dd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_opregion_setup(dev);
 
 	intel_init_pch_refclk(dev);
+
+	/*
+	 * We need to make sure that we resume MST before doing anything
+	 * display related, otherwise we risk trying to bring up a display on
+	 * MST before the hub is actually ready
+	 */
+	intel_dp_mst_resume(dev);
+
 	drm_mode_config_reset(dev);
 
 	/*
@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
 	intel_display_resume(dev);
 	drm_modeset_unlock_all(dev);
 
-	intel_dp_mst_resume(dev);
-
 	/*
 	 * ... but also need to make sure that hotplug processing
 	 * doesn't cause havoc. Like in the driver load code we don't
-- 
2.5.0

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Thulasimani, Sivakumar]

On 2/24/2016 3:41 AM, Lyude wrote:
> As it turns out, resuming DP MST is racey since we don't make sure MST
> is ready before we start modesetting, it just usually happens to be
> ready in time. This isn't the case on all systems, particularly a
> ThinkPad T560 with displays connected through the dock. On these
> systems, resuming the laptop while connected to the dock usually results
> in blank monitors. Making sure MST is ready before doing any kind of
> modesetting fixes this issue.
basic question since i haven't worked on MST much. MST should work like any
other digital panel on resume. i.e detect followed by modeset. in the 
modified
commit mentioned below is it failing to detect the panel or failing at 
the modeset ?
if we are depending on the intel_display_resume, how about moving the
intel_dp_mst_resume just above intel_display_resume?


Generic question to others in mail list on i915_drm_resume
we are doing a modeset and then doing the detect/hpd init.
shouldn't this be the other way round ? almost all displays
will pass a modeset even if display is not connected so we
are spending time on modeset even for displays that were
removed during the suspend state. if this is to simply
drm_state being saved and restored,
> We originally changed the resume order in
>
> 	commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
>
> to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> issue now anyhow.
>
> CC: stable@vger.kernel.org
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f357058..4dcf3dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>   	intel_opregion_setup(dev);
>   
>   	intel_init_pch_refclk(dev);
> +
> +	/*
> +	 * We need to make sure that we resume MST before doing anything
> +	 * display related, otherwise we risk trying to bring up a display on
> +	 * MST before the hub is actually ready
> +	 */
> +	intel_dp_mst_resume(dev);
> +
This does not look proper. if the CD clock is turned off during suspend
our dpcd read itself might fail till we enable CD Clock.

regards,
Sivakumar
>   	drm_mode_config_reset(dev);
>   
>   	/*
> @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>   	intel_display_resume(dev);
>   	drm_modeset_unlock_all(dev);
>   
> -	intel_dp_mst_resume(dev);
> -
>   	/*
>   	 * ... but also need to make sure that hotplug processing
>   	 * doesn't cause havoc. Like in the driver load code we don't

Re: [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Thulasimani, Sivakumar]

On 2/24/2016 3:41 AM, Lyude wrote:
> As it turns out, resuming DP MST is racey since we don't make sure MST
> is ready before we start modesetting, it just usually happens to be
> ready in time. This isn't the case on all systems, particularly a
> ThinkPad T560 with displays connected through the dock. On these
> systems, resuming the laptop while connected to the dock usually results
> in blank monitors. Making sure MST is ready before doing any kind of
> modesetting fixes this issue.
basic question since i haven't worked on MST much. MST should work like any
other digital panel on resume. i.e detect followed by modeset. in the 
modified
commit mentioned below is it failing to detect the panel or failing at 
the modeset ?
if we are depending on the intel_display_resume, how about moving the
intel_dp_mst_resume just above intel_display_resume?


Generic question to others in mail list on i915_drm_resume
we are doing a modeset and then doing the detect/hpd init.
shouldn't this be the other way round ? almost all displays
will pass a modeset even if display is not connected so we
are spending time on modeset even for displays that were
removed during the suspend state. if this is to simply
drm_state being saved and restored,
> We originally changed the resume order in
>
> 	commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
>
> to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> issue now anyhow.
>
> CC: stable@vger.kernel.org
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f357058..4dcf3dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>   	intel_opregion_setup(dev);
>   
>   	intel_init_pch_refclk(dev);
> +
> +	/*
> +	 * We need to make sure that we resume MST before doing anything
> +	 * display related, otherwise we risk trying to bring up a display on
> +	 * MST before the hub is actually ready
> +	 */
> +	intel_dp_mst_resume(dev);
> +
This does not look proper. if the CD clock is turned off during suspend
our dpcd read itself might fail till we enable CD Clock.

regards,
Sivakumar
>   	drm_mode_config_reset(dev);
>   
>   	/*
> @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>   	intel_display_resume(dev);
>   	drm_modeset_unlock_all(dev);
>   
> -	intel_dp_mst_resume(dev);
> -
>   	/*
>   	 * ... but also need to make sure that hotplug processing
>   	 * doesn't cause havoc. Like in the driver load code we don't

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

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Lyude Paul]

Unfortunately MST is a wild beast, and doesn't work at all like other
connectors. This being said, putting it above intel_display_resume() was the
first thing I tried but that didn't work. Originally I had thought putting it
this high up was required because I had assumed drm_mode_config_reset() was
doing modesetting as well but doing some investigation, it doesn't look like
that call actually does anything. It looks like the real culprit here is
intel_runtime_pm_enable_interrupts(), so long as the call to
intel_dp_mst_resume() is above that everything works. So now I'm a little unsure
why this is working like it is, although I can definitely see the patch fixes
the issue. I'm going to edit the patch to reflect this, and see if I can get any
more insight as to why this fixes it.


On Wed, 2016-02-24 at 08:03 +0530, Thulasimani, Sivakumar wrote:
> 
> On 2/24/2016 3:41 AM, Lyude wrote:
> > As it turns out, resuming DP MST is racey since we don't make sure MST
> > is ready before we start modesetting, it just usually happens to be
> > ready in time. This isn't the case on all systems, particularly a
> > ThinkPad T560 with displays connected through the dock. On these
> > systems, resuming the laptop while connected to the dock usually results
> > in blank monitors. Making sure MST is ready before doing any kind of
> > modesetting fixes this issue.
> basic question since i haven't worked on MST much. MST should work like any
> other digital panel on resume. i.e detect followed by modeset. in the 
> modified
> commit mentioned below is it failing to detect the panel or failing at 
> the modeset ?
> if we are depending on the intel_display_resume, how about moving the
> intel_dp_mst_resume just above intel_display_resume?
> 
> 
> Generic question to others in mail list on i915_drm_resume
> we are doing a modeset and then doing the detect/hpd init.
> shouldn't this be the other way round ? almost all displays
> will pass a modeset even if display is not connected so we
> are spending time on modeset even for displays that were
> removed during the suspend state. if this is to simply
> drm_state being saved and restored,
> > We originally changed the resume order in
> > 
> > 	commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
> > 
> > to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> > issue now anyhow.
> > 
> > CC: stable@vger.kernel.org
> > Signed-off-by: Lyude <cpaul@redhat.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index f357058..4dcf3dd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
> >   	intel_opregion_setup(dev);
> >   
> >   	intel_init_pch_refclk(dev);
> > +
> > +	/*
> > +	 * We need to make sure that we resume MST before doing anything
> > +	 * display related, otherwise we risk trying to bring up a display
> > on
> > +	 * MST before the hub is actually ready
> > +	 */
> > +	intel_dp_mst_resume(dev);
> > +
> This does not look proper. if the CD clock is turned off during suspend
> our dpcd read itself might fail till we enable CD Clock.
> 
> regards,
> Sivakumar
> >   	drm_mode_config_reset(dev);
> >   
> >   	/*
> > @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >   	intel_display_resume(dev);
> >   	drm_modeset_unlock_all(dev);
> >   
> > -	intel_dp_mst_resume(dev);
> > -
> >   	/*
> >   	 * ... but also need to make sure that hotplug processing
> >   	 * doesn't cause havoc. Like in the driver load code we don't
> 

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Lyude Paul]

Unfortunately MST is a wild beast, and doesn't work at all like other
connectors. This being said, putting it above intel_display_resume() was the
first thing I tried but that didn't work. Originally I had thought putting it
this high up was required because I had assumed drm_mode_config_reset() was
doing modesetting as well but doing some investigation, it doesn't look like
that call actually does anything. It looks like the real culprit here is
intel_runtime_pm_enable_interrupts(), so long as the call to
intel_dp_mst_resume() is above that everything works. So now I'm a little unsure
why this is working like it is, although I can definitely see the patch fixes
the issue. I'm going to edit the patch to reflect this, and see if I can get any
more insight as to why this fixes it.


On Wed, 2016-02-24 at 08:03 +0530, Thulasimani, Sivakumar wrote:
> 
> On 2/24/2016 3:41 AM, Lyude wrote:
> > As it turns out, resuming DP MST is racey since we don't make sure MST
> > is ready before we start modesetting, it just usually happens to be
> > ready in time. This isn't the case on all systems, particularly a
> > ThinkPad T560 with displays connected through the dock. On these
> > systems, resuming the laptop while connected to the dock usually results
> > in blank monitors. Making sure MST is ready before doing any kind of
> > modesetting fixes this issue.
> basic question since i haven't worked on MST much. MST should work like any
> other digital panel on resume. i.e detect followed by modeset. in the 
> modified
> commit mentioned below is it failing to detect the panel or failing at 
> the modeset ?
> if we are depending on the intel_display_resume, how about moving the
> intel_dp_mst_resume just above intel_display_resume?
> 
> 
> Generic question to others in mail list on i915_drm_resume
> we are doing a modeset and then doing the detect/hpd init.
> shouldn't this be the other way round ? almost all displays
> will pass a modeset even if display is not connected so we
> are spending time on modeset even for displays that were
> removed during the suspend state. if this is to simply
> drm_state being saved and restored,
> > We originally changed the resume order in
> > 
> > 	commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
> > 
> > to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> > issue now anyhow.
> > 
> > CC: stable@vger.kernel.org
> > Signed-off-by: Lyude <cpaul@redhat.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index f357058..4dcf3dd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
> >   	intel_opregion_setup(dev);
> >   
> >   	intel_init_pch_refclk(dev);
> > +
> > +	/*
> > +	 * We need to make sure that we resume MST before doing anything
> > +	 * display related, otherwise we risk trying to bring up a display
> > on
> > +	 * MST before the hub is actually ready
> > +	 */
> > +	intel_dp_mst_resume(dev);
> > +
> This does not look proper. if the CD clock is turned off during suspend
> our dpcd read itself might fail till we enable CD Clock.
> 
> regards,
> Sivakumar
> >   	drm_mode_config_reset(dev);
> >   
> >   	/*
> > @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >   	intel_display_resume(dev);
> >   	drm_modeset_unlock_all(dev);
> >   
> > -	intel_dp_mst_resume(dev);
> > -
> >   	/*
> >   	 * ... but also need to make sure that hotplug processing
> >   	 * doesn't cause havoc. Like in the driver load code we don't
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Rob Clark]

On Tue, Feb 23, 2016 at 9:33 PM, Thulasimani, Sivakumar
<sivakumar.thulasimani@intel.com> wrote:
>
>
> On 2/24/2016 3:41 AM, Lyude wrote:
>>
>> As it turns out, resuming DP MST is racey since we don't make sure MST
>> is ready before we start modesetting, it just usually happens to be
>> ready in time. This isn't the case on all systems, particularly a
>> ThinkPad T560 with displays connected through the dock. On these
>> systems, resuming the laptop while connected to the dock usually results
>> in blank monitors. Making sure MST is ready before doing any kind of
>> modesetting fixes this issue.
>
> basic question since i haven't worked on MST much. MST should work like any
> other digital panel on resume. i.e detect followed by modeset. in the
> modified
> commit mentioned below is it failing to detect the panel or failing at the
> modeset ?
> if we are depending on the intel_display_resume, how about moving the
> intel_dp_mst_resume just above intel_display_resume?
>

so I think the issue is there needs to be communication with (for
example) hubs that sit downstream..  we probably do need enough clks
and irq's going so that dpcd xfer's work, but then I think we should
do mst_resume() first and then display_resume()

BR,
-R

>
> Generic question to others in mail list on i915_drm_resume
> we are doing a modeset and then doing the detect/hpd init.
> shouldn't this be the other way round ? almost all displays
> will pass a modeset even if display is not connected so we
> are spending time on modeset even for displays that were
> removed during the suspend state. if this is to simply
> drm_state being saved and restored,
>>
>> We originally changed the resume order in
>>
>>         commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw
>> state")
>>
>> to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>> issue now anyhow.
>>
>> CC: stable@vger.kernel.org
>> Signed-off-by: Lyude <cpaul@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index f357058..4dcf3dd 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>>         intel_opregion_setup(dev);
>>         intel_init_pch_refclk(dev);
>> +
>> +       /*
>> +        * We need to make sure that we resume MST before doing anything
>> +        * display related, otherwise we risk trying to bring up a display
>> on
>> +        * MST before the hub is actually ready
>> +        */
>> +       intel_dp_mst_resume(dev);
>> +
>
> This does not look proper. if the CD clock is turned off during suspend
> our dpcd read itself might fail till we enable CD Clock.
>
> regards,
> Sivakumar
>>
>>         drm_mode_config_reset(dev);
>>         /*
>> @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>>         intel_display_resume(dev);
>>         drm_modeset_unlock_all(dev);
>>   -     intel_dp_mst_resume(dev);
>> -
>>         /*
>>          * ... but also need to make sure that hotplug processing
>>          * doesn't cause havoc. Like in the driver load code we don't
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Rob Clark]

On Tue, Feb 23, 2016 at 9:33 PM, Thulasimani, Sivakumar
<sivakumar.thulasimani@intel.com> wrote:
>
>
> On 2/24/2016 3:41 AM, Lyude wrote:
>>
>> As it turns out, resuming DP MST is racey since we don't make sure MST
>> is ready before we start modesetting, it just usually happens to be
>> ready in time. This isn't the case on all systems, particularly a
>> ThinkPad T560 with displays connected through the dock. On these
>> systems, resuming the laptop while connected to the dock usually results
>> in blank monitors. Making sure MST is ready before doing any kind of
>> modesetting fixes this issue.
>
> basic question since i haven't worked on MST much. MST should work like any
> other digital panel on resume. i.e detect followed by modeset. in the
> modified
> commit mentioned below is it failing to detect the panel or failing at the
> modeset ?
> if we are depending on the intel_display_resume, how about moving the
> intel_dp_mst_resume just above intel_display_resume?
>

so I think the issue is there needs to be communication with (for
example) hubs that sit downstream..  we probably do need enough clks
and irq's going so that dpcd xfer's work, but then I think we should
do mst_resume() first and then display_resume()

BR,
-R

>
> Generic question to others in mail list on i915_drm_resume
> we are doing a modeset and then doing the detect/hpd init.
> shouldn't this be the other way round ? almost all displays
> will pass a modeset even if display is not connected so we
> are spending time on modeset even for displays that were
> removed during the suspend state. if this is to simply
> drm_state being saved and restored,
>>
>> We originally changed the resume order in
>>
>>         commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw
>> state")
>>
>> to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>> issue now anyhow.
>>
>> CC: stable@vger.kernel.org
>> Signed-off-by: Lyude <cpaul@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index f357058..4dcf3dd 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>>         intel_opregion_setup(dev);
>>         intel_init_pch_refclk(dev);
>> +
>> +       /*
>> +        * We need to make sure that we resume MST before doing anything
>> +        * display related, otherwise we risk trying to bring up a display
>> on
>> +        * MST before the hub is actually ready
>> +        */
>> +       intel_dp_mst_resume(dev);
>> +
>
> This does not look proper. if the CD clock is turned off during suspend
> our dpcd read itself might fail till we enable CD Clock.
>
> regards,
> Sivakumar
>>
>>         drm_mode_config_reset(dev);
>>         /*
>> @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>>         intel_display_resume(dev);
>>         drm_modeset_unlock_all(dev);
>>   -     intel_dp_mst_resume(dev);
>> -
>>         /*
>>          * ... but also need to make sure that hotplug processing
>>          * doesn't cause havoc. Like in the driver load code we don't
>
>
> _______________________________________________
> 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

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Daniel Vetter]

On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 2/24/2016 3:41 AM, Lyude wrote:
> >As it turns out, resuming DP MST is racey since we don't make sure MST
> >is ready before we start modesetting, it just usually happens to be
> >ready in time. This isn't the case on all systems, particularly a
> >ThinkPad T560 with displays connected through the dock. On these
> >systems, resuming the laptop while connected to the dock usually results
> >in blank monitors. Making sure MST is ready before doing any kind of
> >modesetting fixes this issue.
> basic question since i haven't worked on MST much. MST should work like any
> other digital panel on resume. i.e detect followed by modeset. in the
> modified
> commit mentioned below is it failing to detect the panel or failing at the
> modeset ?
> if we are depending on the intel_display_resume, how about moving the
> intel_dp_mst_resume just above intel_display_resume?
> 
> 
> Generic question to others in mail list on i915_drm_resume
> we are doing a modeset and then doing the detect/hpd init.
> shouldn't this be the other way round ? almost all displays
> will pass a modeset even if display is not connected so we
> are spending time on modeset even for displays that were
> removed during the suspend state. if this is to simply
> drm_state being saved and restored,

We must restore anyway, we're not really allowed to shut down a display
without userspace's consent. DP mst breaks this, and it's not fun really.
So for hotunplug the flow should always be:
1. kernel notices something has changed in the output config.
2. kernel sends out uevent
3. userspace figures out what changed, and what needs to be done
4. userspace asks the kernel to change display configuration through
setCrtc and Atomic ioctl calls.

Irrespective of hotunplug handling, the kernel also _must_ restore the
entire display configuration before userspace resumes. We can do that
asynchronously (and there's plans for that), but even then we must stall
userspace on the first KMS ioclt to keep up the illusion that a system s/r
is transparent.

In short, even if we do hpd processing before resuming the display,
nothing will be faster - we must wait for userspace to make up its mind,
and that can only happen once we've restored the display config.

And again, mst is kinda breaking this, since and mst unplug results in a
force-disable. Which can upset userspace and in general results in the
need for lots of fragile error handling all over.

> >We originally changed the resume order in
> >
> >	commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
> >
> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> >issue now anyhow.
> >
> >CC: stable@vger.kernel.org
> >Signed-off-by: Lyude <cpaul@redhat.com>

Wrt the patch itself: I think only in 4.6 we've actually fixed up all
these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
modeset state readout for mst is still not entirely correct (it still
relies on software state), so I'm not sure we've actually managed to shut
up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
while suspended. In short the get_hw_state functions for mst should not
rely on any mst software state, but instead just look at hw registers and
dp aux registers to figure out what's going on. But not sure whether this
will result on WARNINGs on resume, since generally the bios doesn't enable
anything in that case.

Furthermore MST still does a force-modeset when processing a hotunplug.
Doing that before we've resumed the display is likely a very bad idea.
What we need to fix that part is to separate the dp mst connector
hotplug/unplugging from actually updating the modeset change. This needs
reference-counting on drm_connector (so that we can lazily free
drm_connector structs after hot-unplug), and is a major change.

In short: I'm scared about this patch ;-)

Thanks, Daniel


> >---
> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >index f357058..4dcf3dd 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
> >  	intel_opregion_setup(dev);
> >  	intel_init_pch_refclk(dev);
> >+
> >+	/*
> >+	 * We need to make sure that we resume MST before doing anything
> >+	 * display related, otherwise we risk trying to bring up a display on
> >+	 * MST before the hub is actually ready
> >+	 */
> >+	intel_dp_mst_resume(dev);
> >+
> This does not look proper. if the CD clock is turned off during suspend
> our dpcd read itself might fail till we enable CD Clock.
> 
> regards,
> Sivakumar
> >  	drm_mode_config_reset(dev);
> >  	/*
> >@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >  	intel_display_resume(dev);
> >  	drm_modeset_unlock_all(dev);
> >-	intel_dp_mst_resume(dev);
> >-
> >  	/*
> >  	 * ... but also need to make sure that hotplug processing
> >  	 * doesn't cause havoc. Like in the driver load code we don't
> 
> _______________________________________________
> 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

Re: [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Daniel Vetter]

On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 2/24/2016 3:41 AM, Lyude wrote:
> >As it turns out, resuming DP MST is racey since we don't make sure MST
> >is ready before we start modesetting, it just usually happens to be
> >ready in time. This isn't the case on all systems, particularly a
> >ThinkPad T560 with displays connected through the dock. On these
> >systems, resuming the laptop while connected to the dock usually results
> >in blank monitors. Making sure MST is ready before doing any kind of
> >modesetting fixes this issue.
> basic question since i haven't worked on MST much. MST should work like any
> other digital panel on resume. i.e detect followed by modeset. in the
> modified
> commit mentioned below is it failing to detect the panel or failing at the
> modeset ?
> if we are depending on the intel_display_resume, how about moving the
> intel_dp_mst_resume just above intel_display_resume?
> 
> 
> Generic question to others in mail list on i915_drm_resume
> we are doing a modeset and then doing the detect/hpd init.
> shouldn't this be the other way round ? almost all displays
> will pass a modeset even if display is not connected so we
> are spending time on modeset even for displays that were
> removed during the suspend state. if this is to simply
> drm_state being saved and restored,

We must restore anyway, we're not really allowed to shut down a display
without userspace's consent. DP mst breaks this, and it's not fun really.
So for hotunplug the flow should always be:
1. kernel notices something has changed in the output config.
2. kernel sends out uevent
3. userspace figures out what changed, and what needs to be done
4. userspace asks the kernel to change display configuration through
setCrtc and Atomic ioctl calls.

Irrespective of hotunplug handling, the kernel also _must_ restore the
entire display configuration before userspace resumes. We can do that
asynchronously (and there's plans for that), but even then we must stall
userspace on the first KMS ioclt to keep up the illusion that a system s/r
is transparent.

In short, even if we do hpd processing before resuming the display,
nothing will be faster - we must wait for userspace to make up its mind,
and that can only happen once we've restored the display config.

And again, mst is kinda breaking this, since and mst unplug results in a
force-disable. Which can upset userspace and in general results in the
need for lots of fragile error handling all over.

> >We originally changed the resume order in
> >
> >	commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
> >
> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> >issue now anyhow.
> >
> >CC: stable@vger.kernel.org
> >Signed-off-by: Lyude <cpaul@redhat.com>

Wrt the patch itself: I think only in 4.6 we've actually fixed up all
these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
modeset state readout for mst is still not entirely correct (it still
relies on software state), so I'm not sure we've actually managed to shut
up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
while suspended. In short the get_hw_state functions for mst should not
rely on any mst software state, but instead just look at hw registers and
dp aux registers to figure out what's going on. But not sure whether this
will result on WARNINGs on resume, since generally the bios doesn't enable
anything in that case.

Furthermore MST still does a force-modeset when processing a hotunplug.
Doing that before we've resumed the display is likely a very bad idea.
What we need to fix that part is to separate the dp mst connector
hotplug/unplugging from actually updating the modeset change. This needs
reference-counting on drm_connector (so that we can lazily free
drm_connector structs after hot-unplug), and is a major change.

In short: I'm scared about this patch ;-)

Thanks, Daniel


> >---
> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >index f357058..4dcf3dd 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
> >  	intel_opregion_setup(dev);
> >  	intel_init_pch_refclk(dev);
> >+
> >+	/*
> >+	 * We need to make sure that we resume MST before doing anything
> >+	 * display related, otherwise we risk trying to bring up a display on
> >+	 * MST before the hub is actually ready
> >+	 */
> >+	intel_dp_mst_resume(dev);
> >+
> This does not look proper. if the CD clock is turned off during suspend
> our dpcd read itself might fail till we enable CD Clock.
> 
> regards,
> Sivakumar
> >  	drm_mode_config_reset(dev);
> >  	/*
> >@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >  	intel_display_resume(dev);
> >  	drm_modeset_unlock_all(dev);
> >-	intel_dp_mst_resume(dev);
> >-
> >  	/*
> >  	 * ... but also need to make sure that hotplug processing
> >  	 * doesn't cause havoc. Like in the driver load code we don't
> 
> _______________________________________________
> 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: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Dave Airlie]

On 1 March 2016 at 02:12, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
>>
>>
>> On 2/24/2016 3:41 AM, Lyude wrote:
>> >As it turns out, resuming DP MST is racey since we don't make sure MST
>> >is ready before we start modesetting, it just usually happens to be
>> >ready in time. This isn't the case on all systems, particularly a
>> >ThinkPad T560 with displays connected through the dock. On these
>> >systems, resuming the laptop while connected to the dock usually results
>> >in blank monitors. Making sure MST is ready before doing any kind of
>> >modesetting fixes this issue.
>> basic question since i haven't worked on MST much. MST should work like any
>> other digital panel on resume. i.e detect followed by modeset. in the
>> modified
>> commit mentioned below is it failing to detect the panel or failing at the
>> modeset ?
>> if we are depending on the intel_display_resume, how about moving the
>> intel_dp_mst_resume just above intel_display_resume?
>>
>>
>> Generic question to others in mail list on i915_drm_resume
>> we are doing a modeset and then doing the detect/hpd init.
>> shouldn't this be the other way round ? almost all displays
>> will pass a modeset even if display is not connected so we
>> are spending time on modeset even for displays that were
>> removed during the suspend state. if this is to simply
>> drm_state being saved and restored,
>
> We must restore anyway, we're not really allowed to shut down a display
> without userspace's consent. DP mst breaks this, and it's not fun really.
> So for hotunplug the flow should always be:
> 1. kernel notices something has changed in the output config.
> 2. kernel sends out uevent
> 3. userspace figures out what changed, and what needs to be done
> 4. userspace asks the kernel to change display configuration through
> setCrtc and Atomic ioctl calls.
>
> Irrespective of hotunplug handling, the kernel also _must_ restore the
> entire display configuration before userspace resumes. We can do that
> asynchronously (and there's plans for that), but even then we must stall
> userspace on the first KMS ioclt to keep up the illusion that a system s/r
> is transparent.
>
> In short, even if we do hpd processing before resuming the display,
> nothing will be faster - we must wait for userspace to make up its mind,
> and that can only happen once we've restored the display config.
>
> And again, mst is kinda breaking this, since and mst unplug results in a
> force-disable. Which can upset userspace and in general results in the
> need for lots of fragile error handling all over.

>
>> >We originally changed the resume order in
>> >
>> >     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
>> >
>> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>> >issue now anyhow.
>> >
>> >CC: stable@vger.kernel.org
>> >Signed-off-by: Lyude <cpaul@redhat.com>
>
> Wrt the patch itself: I think only in 4.6 we've actually fixed up all
> these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
> modeset state readout for mst is still not entirely correct (it still
> relies on software state), so I'm not sure we've actually managed to shut
> up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
> while suspended. In short the get_hw_state functions for mst should not
> rely on any mst software state, but instead just look at hw registers and
> dp aux registers to figure out what's going on. But not sure whether this
> will result on WARNINGs on resume, since generally the bios doesn't enable
> anything in that case.
>
> Furthermore MST still does a force-modeset when processing a hotunplug.
> Doing that before we've resumed the display is likely a very bad idea.
> What we need to fix that part is to separate the dp mst connector
> hotplug/unplugging from actually updating the modeset change. This needs
> reference-counting on drm_connector (so that we can lazily free
> drm_connector structs after hot-unplug), and is a major change.

I agree on the dropping stable on this, just put it in 4.6 for now, actually I'm
happy to send in for 4.5 at this stage as it fixes some laptops I have as well.

However I'm not sure how you think we can keep hw state programmed when
the hw is gone away until userspace comes along to do something about it,
the hw state is mostly on the other side of the MST link which has also gone
away. My experience with leaving the crtcs programmed was and I might not
be 100% sure was hw hangs, I even managed to get some MCEs over time.

Though I might be tempted to go investigate this a bit more.

Dave.

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Rob Clark]

On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
>>
>>
>> On 2/24/2016 3:41 AM, Lyude wrote:
>> >As it turns out, resuming DP MST is racey since we don't make sure MST
>> >is ready before we start modesetting, it just usually happens to be
>> >ready in time. This isn't the case on all systems, particularly a
>> >ThinkPad T560 with displays connected through the dock. On these
>> >systems, resuming the laptop while connected to the dock usually results
>> >in blank monitors. Making sure MST is ready before doing any kind of
>> >modesetting fixes this issue.
>> basic question since i haven't worked on MST much. MST should work like any
>> other digital panel on resume. i.e detect followed by modeset. in the
>> modified
>> commit mentioned below is it failing to detect the panel or failing at the
>> modeset ?
>> if we are depending on the intel_display_resume, how about moving the
>> intel_dp_mst_resume just above intel_display_resume?
>>
>>
>> Generic question to others in mail list on i915_drm_resume
>> we are doing a modeset and then doing the detect/hpd init.
>> shouldn't this be the other way round ? almost all displays
>> will pass a modeset even if display is not connected so we
>> are spending time on modeset even for displays that were
>> removed during the suspend state. if this is to simply
>> drm_state being saved and restored,
>
> We must restore anyway, we're not really allowed to shut down a display
> without userspace's consent. DP mst breaks this, and it's not fun really.

well, that isn't completely true.. I mean, if the user unplugs (for
example) an hdmi monitor, it isn't with userspace's consent..

I wonder if we could just have flag per connector, ie.
connector->no_auto_resume and just automatically send unplug events
for those to userspace (followed shortly by a plug event if it is
still connected and the normal hpd mechanism kicks in?  I mean, for
all we know, the user unplugged the DP monitor/hub/etc and plugged in
a different one while we were suspended..

BR,
-R

> So for hotunplug the flow should always be:
> 1. kernel notices something has changed in the output config.
> 2. kernel sends out uevent
> 3. userspace figures out what changed, and what needs to be done
> 4. userspace asks the kernel to change display configuration through
> setCrtc and Atomic ioctl calls.
>
> Irrespective of hotunplug handling, the kernel also _must_ restore the
> entire display configuration before userspace resumes. We can do that
> asynchronously (and there's plans for that), but even then we must stall
> userspace on the first KMS ioclt to keep up the illusion that a system s/r
> is transparent.
>
> In short, even if we do hpd processing before resuming the display,
> nothing will be faster - we must wait for userspace to make up its mind,
> and that can only happen once we've restored the display config.
>
> And again, mst is kinda breaking this, since and mst unplug results in a
> force-disable. Which can upset userspace and in general results in the
> need for lots of fragile error handling all over.
>
>> >We originally changed the resume order in
>> >
>> >     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
>> >
>> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>> >issue now anyhow.
>> >
>> >CC: stable@vger.kernel.org
>> >Signed-off-by: Lyude <cpaul@redhat.com>
>
> Wrt the patch itself: I think only in 4.6 we've actually fixed up all
> these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
> modeset state readout for mst is still not entirely correct (it still
> relies on software state), so I'm not sure we've actually managed to shut
> up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
> while suspended. In short the get_hw_state functions for mst should not
> rely on any mst software state, but instead just look at hw registers and
> dp aux registers to figure out what's going on. But not sure whether this
> will result on WARNINGs on resume, since generally the bios doesn't enable
> anything in that case.
>
> Furthermore MST still does a force-modeset when processing a hotunplug.
> Doing that before we've resumed the display is likely a very bad idea.
> What we need to fix that part is to separate the dp mst connector
> hotplug/unplugging from actually updating the modeset change. This needs
> reference-counting on drm_connector (so that we can lazily free
> drm_connector structs after hot-unplug), and is a major change.
>
> In short: I'm scared about this patch ;-)
>
> Thanks, Daniel
>
>
>> >---
>> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>> >  1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> >index f357058..4dcf3dd 100644
>> >--- a/drivers/gpu/drm/i915/i915_drv.c
>> >+++ b/drivers/gpu/drm/i915/i915_drv.c
>> >@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>> >     intel_opregion_setup(dev);
>> >     intel_init_pch_refclk(dev);
>> >+
>> >+    /*
>> >+     * We need to make sure that we resume MST before doing anything
>> >+     * display related, otherwise we risk trying to bring up a display on
>> >+     * MST before the hub is actually ready
>> >+     */
>> >+    intel_dp_mst_resume(dev);
>> >+
>> This does not look proper. if the CD clock is turned off during suspend
>> our dpcd read itself might fail till we enable CD Clock.
>>
>> regards,
>> Sivakumar
>> >     drm_mode_config_reset(dev);
>> >     /*
>> >@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>> >     intel_display_resume(dev);
>> >     drm_modeset_unlock_all(dev);
>> >-    intel_dp_mst_resume(dev);
>> >-
>> >     /*
>> >      * ... but also need to make sure that hotplug processing
>> >      * doesn't cause havoc. Like in the driver load code we don't
>>
>> _______________________________________________
>> 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: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Rob Clark]

On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
>>
>>
>> On 2/24/2016 3:41 AM, Lyude wrote:
>> >As it turns out, resuming DP MST is racey since we don't make sure MST
>> >is ready before we start modesetting, it just usually happens to be
>> >ready in time. This isn't the case on all systems, particularly a
>> >ThinkPad T560 with displays connected through the dock. On these
>> >systems, resuming the laptop while connected to the dock usually results
>> >in blank monitors. Making sure MST is ready before doing any kind of
>> >modesetting fixes this issue.
>> basic question since i haven't worked on MST much. MST should work like any
>> other digital panel on resume. i.e detect followed by modeset. in the
>> modified
>> commit mentioned below is it failing to detect the panel or failing at the
>> modeset ?
>> if we are depending on the intel_display_resume, how about moving the
>> intel_dp_mst_resume just above intel_display_resume?
>>
>>
>> Generic question to others in mail list on i915_drm_resume
>> we are doing a modeset and then doing the detect/hpd init.
>> shouldn't this be the other way round ? almost all displays
>> will pass a modeset even if display is not connected so we
>> are spending time on modeset even for displays that were
>> removed during the suspend state. if this is to simply
>> drm_state being saved and restored,
>
> We must restore anyway, we're not really allowed to shut down a display
> without userspace's consent. DP mst breaks this, and it's not fun really.

well, that isn't completely true.. I mean, if the user unplugs (for
example) an hdmi monitor, it isn't with userspace's consent..

I wonder if we could just have flag per connector, ie.
connector->no_auto_resume and just automatically send unplug events
for those to userspace (followed shortly by a plug event if it is
still connected and the normal hpd mechanism kicks in?  I mean, for
all we know, the user unplugged the DP monitor/hub/etc and plugged in
a different one while we were suspended..

BR,
-R

> So for hotunplug the flow should always be:
> 1. kernel notices something has changed in the output config.
> 2. kernel sends out uevent
> 3. userspace figures out what changed, and what needs to be done
> 4. userspace asks the kernel to change display configuration through
> setCrtc and Atomic ioctl calls.
>
> Irrespective of hotunplug handling, the kernel also _must_ restore the
> entire display configuration before userspace resumes. We can do that
> asynchronously (and there's plans for that), but even then we must stall
> userspace on the first KMS ioclt to keep up the illusion that a system s/r
> is transparent.
>
> In short, even if we do hpd processing before resuming the display,
> nothing will be faster - we must wait for userspace to make up its mind,
> and that can only happen once we've restored the display config.
>
> And again, mst is kinda breaking this, since and mst unplug results in a
> force-disable. Which can upset userspace and in general results in the
> need for lots of fragile error handling all over.
>
>> >We originally changed the resume order in
>> >
>> >     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
>> >
>> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>> >issue now anyhow.
>> >
>> >CC: stable@vger.kernel.org
>> >Signed-off-by: Lyude <cpaul@redhat.com>
>
> Wrt the patch itself: I think only in 4.6 we've actually fixed up all
> these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
> modeset state readout for mst is still not entirely correct (it still
> relies on software state), so I'm not sure we've actually managed to shut
> up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
> while suspended. In short the get_hw_state functions for mst should not
> rely on any mst software state, but instead just look at hw registers and
> dp aux registers to figure out what's going on. But not sure whether this
> will result on WARNINGs on resume, since generally the bios doesn't enable
> anything in that case.
>
> Furthermore MST still does a force-modeset when processing a hotunplug.
> Doing that before we've resumed the display is likely a very bad idea.
> What we need to fix that part is to separate the dp mst connector
> hotplug/unplugging from actually updating the modeset change. This needs
> reference-counting on drm_connector (so that we can lazily free
> drm_connector structs after hot-unplug), and is a major change.
>
> In short: I'm scared about this patch ;-)
>
> Thanks, Daniel
>
>
>> >---
>> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>> >  1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> >index f357058..4dcf3dd 100644
>> >--- a/drivers/gpu/drm/i915/i915_drv.c
>> >+++ b/drivers/gpu/drm/i915/i915_drv.c
>> >@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>> >     intel_opregion_setup(dev);
>> >     intel_init_pch_refclk(dev);
>> >+
>> >+    /*
>> >+     * We need to make sure that we resume MST before doing anything
>> >+     * display related, otherwise we risk trying to bring up a display on
>> >+     * MST before the hub is actually ready
>> >+     */
>> >+    intel_dp_mst_resume(dev);
>> >+
>> This does not look proper. if the CD clock is turned off during suspend
>> our dpcd read itself might fail till we enable CD Clock.
>>
>> regards,
>> Sivakumar
>> >     drm_mode_config_reset(dev);
>> >     /*
>> >@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>> >     intel_display_resume(dev);
>> >     drm_modeset_unlock_all(dev);
>> >-    intel_dp_mst_resume(dev);
>> >-
>> >     /*
>> >      * ... but also need to make sure that hotplug processing
>> >      * doesn't cause havoc. Like in the driver load code we don't
>>
>> _______________________________________________
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Thulasimani, Sivakumar]

On 3/1/2016 5:03 AM, Rob Clark wrote:
> On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
>>>
>>> On 2/24/2016 3:41 AM, Lyude wrote:
>>>> As it turns out, resuming DP MST is racey since we don't make sure MST
>>>> is ready before we start modesetting, it just usually happens to be
>>>> ready in time. This isn't the case on all systems, particularly a
>>>> ThinkPad T560 with displays connected through the dock. On these
>>>> systems, resuming the laptop while connected to the dock usually results
>>>> in blank monitors. Making sure MST is ready before doing any kind of
>>>> modesetting fixes this issue.
>>> basic question since i haven't worked on MST much. MST should work like any
>>> other digital panel on resume. i.e detect followed by modeset. in the
>>> modified
>>> commit mentioned below is it failing to detect the panel or failing at the
>>> modeset ?
>>> if we are depending on the intel_display_resume, how about moving the
>>> intel_dp_mst_resume just above intel_display_resume?
>>>
>>>
>>> Generic question to others in mail list on i915_drm_resume
>>> we are doing a modeset and then doing the detect/hpd init.
>>> shouldn't this be the other way round ? almost all displays
>>> will pass a modeset even if display is not connected so we
>>> are spending time on modeset even for displays that were
>>> removed during the suspend state. if this is to simply
>>> drm_state being saved and restored,
>> We must restore anyway, we're not really allowed to shut down a display
>> without userspace's consent. DP mst breaks this, and it's not fun really.
> well, that isn't completely true.. I mean, if the user unplugs (for
> example) an hdmi monitor, it isn't with userspace's consent..
>
> I wonder if we could just have flag per connector, ie.
> connector->no_auto_resume and just automatically send unplug events
> for those to userspace (followed shortly by a plug event if it is
> still connected and the normal hpd mechanism kicks in?  I mean, for
> all we know, the user unplugged the DP monitor/hub/etc and plugged in
> a different one while we were suspended..
>
> BR,
> -R
i agree. performing a modeset on a display without even confirming
if it is the same one when we had suspended the system will result in
issues such as applying a mode that may not be supported on the
new display or corruption or blankout or simply failing the modeset
restore or worst case of doing modeset without a display connected.
if we will not allow such a scenario during normal operation, i.e prevent
incorrect modeset requests during normal functioning, why allow such
a modeset during suspend-resume alone ?
we can store the edid hash and if the display has the same hash
post resume then we can allow mode to be restored.

regards,
Sivakumar
>> So for hotunplug the flow should always be:
>> 1. kernel notices something has changed in the output config.
>> 2. kernel sends out uevent
>> 3. userspace figures out what changed, and what needs to be done
>> 4. userspace asks the kernel to change display configuration through
>> setCrtc and Atomic ioctl calls.
>>
>> Irrespective of hotunplug handling, the kernel also _must_ restore the
>> entire display configuration before userspace resumes. We can do that
>> asynchronously (and there's plans for that), but even then we must stall
>> userspace on the first KMS ioclt to keep up the illusion that a system s/r
>> is transparent.
>>
>> In short, even if we do hpd processing before resuming the display,
>> nothing will be faster - we must wait for userspace to make up its mind,
>> and that can only happen once we've restored the display config.
>>
>> And again, mst is kinda breaking this, since and mst unplug results in a
>> force-disable. Which can upset userspace and in general results in the
>> need for lots of fragile error handling all over.
>>
>>>> We originally changed the resume order in
>>>>
>>>>      commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
>>>>
>>>> to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>>>> issue now anyhow.
>>>>
>>>> CC: stable@vger.kernel.org
>>>> Signed-off-by: Lyude <cpaul@redhat.com>
>> Wrt the patch itself: I think only in 4.6 we've actually fixed up all
>> these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
>> modeset state readout for mst is still not entirely correct (it still
>> relies on software state), so I'm not sure we've actually managed to shut
>> up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
>> while suspended. In short the get_hw_state functions for mst should not
>> rely on any mst software state, but instead just look at hw registers and
>> dp aux registers to figure out what's going on. But not sure whether this
>> will result on WARNINGs on resume, since generally the bios doesn't enable
>> anything in that case.
>>
>> Furthermore MST still does a force-modeset when processing a hotunplug.
>> Doing that before we've resumed the display is likely a very bad idea.
>> What we need to fix that part is to separate the dp mst connector
>> hotplug/unplugging from actually updating the modeset change. This needs
>> reference-counting on drm_connector (so that we can lazily free
>> drm_connector structs after hot-unplug), and is a major change.
>>
>> In short: I'm scared about this patch ;-)
>>
>> Thanks, Daniel
>>
>>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>> index f357058..4dcf3dd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>>>>      intel_opregion_setup(dev);
>>>>      intel_init_pch_refclk(dev);
>>>> +
>>>> +    /*
>>>> +     * We need to make sure that we resume MST before doing anything
>>>> +     * display related, otherwise we risk trying to bring up a display on
>>>> +     * MST before the hub is actually ready
>>>> +     */
>>>> +    intel_dp_mst_resume(dev);
>>>> +
>>> This does not look proper. if the CD clock is turned off during suspend
>>> our dpcd read itself might fail till we enable CD Clock.
>>>
>>> regards,
>>> Sivakumar
>>>>      drm_mode_config_reset(dev);
>>>>      /*
>>>> @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>>>>      intel_display_resume(dev);
>>>>      drm_modeset_unlock_all(dev);
>>>> -    intel_dp_mst_resume(dev);
>>>> -
>>>>      /*
>>>>       * ... but also need to make sure that hotplug processing
>>>>       * doesn't cause havoc. Like in the driver load code we don't
>>> _______________________________________________
>>> 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: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Thulasimani, Sivakumar]

On 3/1/2016 5:03 AM, Rob Clark wrote:
> On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
>>>
>>> On 2/24/2016 3:41 AM, Lyude wrote:
>>>> As it turns out, resuming DP MST is racey since we don't make sure MST
>>>> is ready before we start modesetting, it just usually happens to be
>>>> ready in time. This isn't the case on all systems, particularly a
>>>> ThinkPad T560 with displays connected through the dock. On these
>>>> systems, resuming the laptop while connected to the dock usually results
>>>> in blank monitors. Making sure MST is ready before doing any kind of
>>>> modesetting fixes this issue.
>>> basic question since i haven't worked on MST much. MST should work like any
>>> other digital panel on resume. i.e detect followed by modeset. in the
>>> modified
>>> commit mentioned below is it failing to detect the panel or failing at the
>>> modeset ?
>>> if we are depending on the intel_display_resume, how about moving the
>>> intel_dp_mst_resume just above intel_display_resume?
>>>
>>>
>>> Generic question to others in mail list on i915_drm_resume
>>> we are doing a modeset and then doing the detect/hpd init.
>>> shouldn't this be the other way round ? almost all displays
>>> will pass a modeset even if display is not connected so we
>>> are spending time on modeset even for displays that were
>>> removed during the suspend state. if this is to simply
>>> drm_state being saved and restored,
>> We must restore anyway, we're not really allowed to shut down a display
>> without userspace's consent. DP mst breaks this, and it's not fun really.
> well, that isn't completely true.. I mean, if the user unplugs (for
> example) an hdmi monitor, it isn't with userspace's consent..
>
> I wonder if we could just have flag per connector, ie.
> connector->no_auto_resume and just automatically send unplug events
> for those to userspace (followed shortly by a plug event if it is
> still connected and the normal hpd mechanism kicks in?  I mean, for
> all we know, the user unplugged the DP monitor/hub/etc and plugged in
> a different one while we were suspended..
>
> BR,
> -R
i agree. performing a modeset on a display without even confirming
if it is the same one when we had suspended the system will result in
issues such as applying a mode that may not be supported on the
new display or corruption or blankout or simply failing the modeset
restore or worst case of doing modeset without a display connected.
if we will not allow such a scenario during normal operation, i.e prevent
incorrect modeset requests during normal functioning, why allow such
a modeset during suspend-resume alone ?
we can store the edid hash and if the display has the same hash
post resume then we can allow mode to be restored.

regards,
Sivakumar
>> So for hotunplug the flow should always be:
>> 1. kernel notices something has changed in the output config.
>> 2. kernel sends out uevent
>> 3. userspace figures out what changed, and what needs to be done
>> 4. userspace asks the kernel to change display configuration through
>> setCrtc and Atomic ioctl calls.
>>
>> Irrespective of hotunplug handling, the kernel also _must_ restore the
>> entire display configuration before userspace resumes. We can do that
>> asynchronously (and there's plans for that), but even then we must stall
>> userspace on the first KMS ioclt to keep up the illusion that a system s/r
>> is transparent.
>>
>> In short, even if we do hpd processing before resuming the display,
>> nothing will be faster - we must wait for userspace to make up its mind,
>> and that can only happen once we've restored the display config.
>>
>> And again, mst is kinda breaking this, since and mst unplug results in a
>> force-disable. Which can upset userspace and in general results in the
>> need for lots of fragile error handling all over.
>>
>>>> We originally changed the resume order in
>>>>
>>>>      commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
>>>>
>>>> to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>>>> issue now anyhow.
>>>>
>>>> CC: stable@vger.kernel.org
>>>> Signed-off-by: Lyude <cpaul@redhat.com>
>> Wrt the patch itself: I think only in 4.6 we've actually fixed up all
>> these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
>> modeset state readout for mst is still not entirely correct (it still
>> relies on software state), so I'm not sure we've actually managed to shut
>> up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
>> while suspended. In short the get_hw_state functions for mst should not
>> rely on any mst software state, but instead just look at hw registers and
>> dp aux registers to figure out what's going on. But not sure whether this
>> will result on WARNINGs on resume, since generally the bios doesn't enable
>> anything in that case.
>>
>> Furthermore MST still does a force-modeset when processing a hotunplug.
>> Doing that before we've resumed the display is likely a very bad idea.
>> What we need to fix that part is to separate the dp mst connector
>> hotplug/unplugging from actually updating the modeset change. This needs
>> reference-counting on drm_connector (so that we can lazily free
>> drm_connector structs after hot-unplug), and is a major change.
>>
>> In short: I'm scared about this patch ;-)
>>
>> Thanks, Daniel
>>
>>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>> index f357058..4dcf3dd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>>>>      intel_opregion_setup(dev);
>>>>      intel_init_pch_refclk(dev);
>>>> +
>>>> +    /*
>>>> +     * We need to make sure that we resume MST before doing anything
>>>> +     * display related, otherwise we risk trying to bring up a display on
>>>> +     * MST before the hub is actually ready
>>>> +     */
>>>> +    intel_dp_mst_resume(dev);
>>>> +
>>> This does not look proper. if the CD clock is turned off during suspend
>>> our dpcd read itself might fail till we enable CD Clock.
>>>
>>> regards,
>>> Sivakumar
>>>>      drm_mode_config_reset(dev);
>>>>      /*
>>>> @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>>>>      intel_display_resume(dev);
>>>>      drm_modeset_unlock_all(dev);
>>>> -    intel_dp_mst_resume(dev);
>>>> -
>>>>      /*
>>>>       * ... but also need to make sure that hotplug processing
>>>>       * doesn't cause havoc. Like in the driver load code we don't
>>> _______________________________________________
>>> 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Rob Clark]

On Mon, Feb 29, 2016 at 7:47 PM, Thulasimani, Sivakumar
<sivakumar.thulasimani@intel.com> wrote:
>
>
> On 3/1/2016 5:03 AM, Rob Clark wrote:
>>
>> On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>
>>> On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
>>>>
>>>>
>>>> On 2/24/2016 3:41 AM, Lyude wrote:
>>>>>
>>>>> As it turns out, resuming DP MST is racey since we don't make sure MST
>>>>> is ready before we start modesetting, it just usually happens to be
>>>>> ready in time. This isn't the case on all systems, particularly a
>>>>> ThinkPad T560 with displays connected through the dock. On these
>>>>> systems, resuming the laptop while connected to the dock usually
>>>>> results
>>>>> in blank monitors. Making sure MST is ready before doing any kind of
>>>>> modesetting fixes this issue.
>>>>
>>>> basic question since i haven't worked on MST much. MST should work like
>>>> any
>>>> other digital panel on resume. i.e detect followed by modeset. in the
>>>> modified
>>>> commit mentioned below is it failing to detect the panel or failing at
>>>> the
>>>> modeset ?
>>>> if we are depending on the intel_display_resume, how about moving the
>>>> intel_dp_mst_resume just above intel_display_resume?
>>>>
>>>>
>>>> Generic question to others in mail list on i915_drm_resume
>>>> we are doing a modeset and then doing the detect/hpd init.
>>>> shouldn't this be the other way round ? almost all displays
>>>> will pass a modeset even if display is not connected so we
>>>> are spending time on modeset even for displays that were
>>>> removed during the suspend state. if this is to simply
>>>> drm_state being saved and restored,
>>>
>>> We must restore anyway, we're not really allowed to shut down a display
>>> without userspace's consent. DP mst breaks this, and it's not fun really.
>>
>> well, that isn't completely true.. I mean, if the user unplugs (for
>> example) an hdmi monitor, it isn't with userspace's consent..
>>
>> I wonder if we could just have flag per connector, ie.
>> connector->no_auto_resume and just automatically send unplug events
>> for those to userspace (followed shortly by a plug event if it is
>> still connected and the normal hpd mechanism kicks in?  I mean, for
>> all we know, the user unplugged the DP monitor/hub/etc and plugged in
>> a different one while we were suspended..
>>
>> BR,
>> -R
>
> i agree. performing a modeset on a display without even confirming
> if it is the same one when we had suspended the system will result in
> issues such as applying a mode that may not be supported on the
> new display or corruption or blankout or simply failing the modeset
> restore or worst case of doing modeset without a display connected.
> if we will not allow such a scenario during normal operation, i.e prevent
> incorrect modeset requests during normal functioning, why allow such
> a modeset during suspend-resume alone ?
> we can store the edid hash and if the display has the same hash
> post resume then we can allow mode to be restored.

that seems like a good idea for HDMI/DVI/etc.. although I suspect for
DP there may be 50 more shades of grey.. ie. same monitor re-attached
through different sequence of hubs.. :-(

BR,
-R


> regards,
> Sivakumar
>
>>> So for hotunplug the flow should always be:
>>> 1. kernel notices something has changed in the output config.
>>> 2. kernel sends out uevent
>>> 3. userspace figures out what changed, and what needs to be done
>>> 4. userspace asks the kernel to change display configuration through
>>> setCrtc and Atomic ioctl calls.
>>>
>>> Irrespective of hotunplug handling, the kernel also _must_ restore the
>>> entire display configuration before userspace resumes. We can do that
>>> asynchronously (and there's plans for that), but even then we must stall
>>> userspace on the first KMS ioclt to keep up the illusion that a system
>>> s/r
>>> is transparent.
>>>
>>> In short, even if we do hpd processing before resuming the display,
>>> nothing will be faster - we must wait for userspace to make up its mind,
>>> and that can only happen once we've restored the display config.
>>>
>>> And again, mst is kinda breaking this, since and mst unplug results in a
>>> force-disable. Which can upset userspace and in general results in the
>>> need for lots of fragile error handling all over.
>>>
>>>>> We originally changed the resume order in
>>>>>
>>>>>      commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw
>>>>> state")
>>>>>
>>>>> to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>>>>> issue now anyhow.
>>>>>
>>>>> CC: stable@vger.kernel.org
>>>>> Signed-off-by: Lyude <cpaul@redhat.com>
>>>
>>> Wrt the patch itself: I think only in 4.6 we've actually fixed up all
>>> these mst WARN_ON. Cc: stable seems extremely risky on this one. Also,
>>> the
>>> modeset state readout for mst is still not entirely correct (it still
>>> relies on software state), so I'm not sure we've actually managed to shut
>>> up all the WARNINGs. Iirc the way to repro them is to hot-unplug
>>> something
>>> while suspended. In short the get_hw_state functions for mst should not
>>> rely on any mst software state, but instead just look at hw registers and
>>> dp aux registers to figure out what's going on. But not sure whether this
>>> will result on WARNINGs on resume, since generally the bios doesn't
>>> enable
>>> anything in that case.
>>>
>>> Furthermore MST still does a force-modeset when processing a hotunplug.
>>> Doing that before we've resumed the display is likely a very bad idea.
>>> What we need to fix that part is to separate the dp mst connector
>>> hotplug/unplugging from actually updating the modeset change. This needs
>>> reference-counting on drm_connector (so that we can lazily free
>>> drm_connector structs after hot-unplug), and is a major change.
>>>
>>> In short: I'm scared about this patch ;-)
>>>
>>> Thanks, Daniel
>>>
>>>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>>>>> b/drivers/gpu/drm/i915/i915_drv.c
>>>>> index f357058..4dcf3dd 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>>> @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>>>>>      intel_opregion_setup(dev);
>>>>>      intel_init_pch_refclk(dev);
>>>>> +
>>>>> +    /*
>>>>> +     * We need to make sure that we resume MST before doing anything
>>>>> +     * display related, otherwise we risk trying to bring up a display
>>>>> on
>>>>> +     * MST before the hub is actually ready
>>>>> +     */
>>>>> +    intel_dp_mst_resume(dev);
>>>>> +
>>>>
>>>> This does not look proper. if the CD clock is turned off during suspend
>>>> our dpcd read itself might fail till we enable CD Clock.
>>>>
>>>> regards,
>>>> Sivakumar
>>>>>
>>>>>      drm_mode_config_reset(dev);
>>>>>      /*
>>>>> @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>>>>>      intel_display_resume(dev);
>>>>>      drm_modeset_unlock_all(dev);
>>>>> -    intel_dp_mst_resume(dev);
>>>>> -
>>>>>      /*
>>>>>       * ... but also need to make sure that hotplug processing
>>>>>       * doesn't cause havoc. Like in the driver load code we don't
>>>>
>>>> _______________________________________________
>>>> 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: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Daniel Vetter]

On Tue, Mar 01, 2016 at 07:33:12AM +1000, Dave Airlie wrote:
> On 1 March 2016 at 02:12, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
> >>
> >>
> >> On 2/24/2016 3:41 AM, Lyude wrote:
> >> >As it turns out, resuming DP MST is racey since we don't make sure MST
> >> >is ready before we start modesetting, it just usually happens to be
> >> >ready in time. This isn't the case on all systems, particularly a
> >> >ThinkPad T560 with displays connected through the dock. On these
> >> >systems, resuming the laptop while connected to the dock usually results
> >> >in blank monitors. Making sure MST is ready before doing any kind of
> >> >modesetting fixes this issue.
> >> basic question since i haven't worked on MST much. MST should work like any
> >> other digital panel on resume. i.e detect followed by modeset. in the
> >> modified
> >> commit mentioned below is it failing to detect the panel or failing at the
> >> modeset ?
> >> if we are depending on the intel_display_resume, how about moving the
> >> intel_dp_mst_resume just above intel_display_resume?
> >>
> >>
> >> Generic question to others in mail list on i915_drm_resume
> >> we are doing a modeset and then doing the detect/hpd init.
> >> shouldn't this be the other way round ? almost all displays
> >> will pass a modeset even if display is not connected so we
> >> are spending time on modeset even for displays that were
> >> removed during the suspend state. if this is to simply
> >> drm_state being saved and restored,
> >
> > We must restore anyway, we're not really allowed to shut down a display
> > without userspace's consent. DP mst breaks this, and it's not fun really.
> > So for hotunplug the flow should always be:
> > 1. kernel notices something has changed in the output config.
> > 2. kernel sends out uevent
> > 3. userspace figures out what changed, and what needs to be done
> > 4. userspace asks the kernel to change display configuration through
> > setCrtc and Atomic ioctl calls.
> >
> > Irrespective of hotunplug handling, the kernel also _must_ restore the
> > entire display configuration before userspace resumes. We can do that
> > asynchronously (and there's plans for that), but even then we must stall
> > userspace on the first KMS ioclt to keep up the illusion that a system s/r
> > is transparent.
> >
> > In short, even if we do hpd processing before resuming the display,
> > nothing will be faster - we must wait for userspace to make up its mind,
> > and that can only happen once we've restored the display config.
> >
> > And again, mst is kinda breaking this, since and mst unplug results in a
> > force-disable. Which can upset userspace and in general results in the
> > need for lots of fragile error handling all over.
> 
> >
> >> >We originally changed the resume order in
> >> >
> >> >     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
> >> >
> >> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> >> >issue now anyhow.
> >> >
> >> >CC: stable@vger.kernel.org
> >> >Signed-off-by: Lyude <cpaul@redhat.com>
> >
> > Wrt the patch itself: I think only in 4.6 we've actually fixed up all
> > these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
> > modeset state readout for mst is still not entirely correct (it still
> > relies on software state), so I'm not sure we've actually managed to shut
> > up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
> > while suspended. In short the get_hw_state functions for mst should not
> > rely on any mst software state, but instead just look at hw registers and
> > dp aux registers to figure out what's going on. But not sure whether this
> > will result on WARNINGs on resume, since generally the bios doesn't enable
> > anything in that case.
> >
> > Furthermore MST still does a force-modeset when processing a hotunplug.
> > Doing that before we've resumed the display is likely a very bad idea.
> > What we need to fix that part is to separate the dp mst connector
> > hotplug/unplugging from actually updating the modeset change. This needs
> > reference-counting on drm_connector (so that we can lazily free
> > drm_connector structs after hot-unplug), and is a major change.
> 
> I agree on the dropping stable on this, just put it in 4.6 for now, actually I'm
> happy to send in for 4.5 at this stage as it fixes some laptops I have as well.
> 
> However I'm not sure how you think we can keep hw state programmed when
> the hw is gone away until userspace comes along to do something about it,
> the hw state is mostly on the other side of the MST link which has also gone
> away. My experience with leaving the crtcs programmed was and I might not
> be 100% sure was hw hangs, I even managed to get some MCEs over time.
> 
> Though I might be tempted to go investigate this a bit more.

Hm ... The issue is that stopping the pipe behind userspace back is kinda
really unfriendly. Means you need to sprinkle error handling checks and
fallback code all over, and ofc that code is little-exercise and hence
likely buggy. If it exists at all. That's kinda why I think keeping the
pipe up&running would be the ideal ABI to present to userspace, if we can
do it. Also consistent with what we do for non-mst.

But yeah if this MCEs then not much luck. Otoh I can't see how this could
MCE if we really don't touch anything of the source-side state and just
keep feeding the exact same MST config to the sinks, whether they're still
there or not. That /should/ work, otherwise you could just die when you
unplug. Ofc we need to make any unplugged mst connector as dead, so that
the only thing userspace is allowed to do is disable it. And at that point
it'll get garbage collected.

But hey, that's just my long-term mst dream land ;-) For reality, whatever
floats. But I'd like to figure out what exactly is going on here first,
and Lyude is still investigating (and now also pulling in our intel hw
architects to figure this out).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Daniel Vetter]

On Mon, Feb 29, 2016 at 06:33:42PM -0500, Rob Clark wrote:
> On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
> >>
> >>
> >> On 2/24/2016 3:41 AM, Lyude wrote:
> >> >As it turns out, resuming DP MST is racey since we don't make sure MST
> >> >is ready before we start modesetting, it just usually happens to be
> >> >ready in time. This isn't the case on all systems, particularly a
> >> >ThinkPad T560 with displays connected through the dock. On these
> >> >systems, resuming the laptop while connected to the dock usually results
> >> >in blank monitors. Making sure MST is ready before doing any kind of
> >> >modesetting fixes this issue.
> >> basic question since i haven't worked on MST much. MST should work like any
> >> other digital panel on resume. i.e detect followed by modeset. in the
> >> modified
> >> commit mentioned below is it failing to detect the panel or failing at the
> >> modeset ?
> >> if we are depending on the intel_display_resume, how about moving the
> >> intel_dp_mst_resume just above intel_display_resume?
> >>
> >>
> >> Generic question to others in mail list on i915_drm_resume
> >> we are doing a modeset and then doing the detect/hpd init.
> >> shouldn't this be the other way round ? almost all displays
> >> will pass a modeset even if display is not connected so we
> >> are spending time on modeset even for displays that were
> >> removed during the suspend state. if this is to simply
> >> drm_state being saved and restored,
> >
> > We must restore anyway, we're not really allowed to shut down a display
> > without userspace's consent. DP mst breaks this, and it's not fun really.
> 
> well, that isn't completely true.. I mean, if the user unplugs (for
> example) an hdmi monitor, it isn't with userspace's consent..

But the pipe keeps running, which means the next pageflip or vblank wait
won't just fail and result in hilarity.

> I wonder if we could just have flag per connector, ie.
> connector->no_auto_resume and just automatically send unplug events
> for those to userspace (followed shortly by a plug event if it is
> still connected and the normal hpd mechanism kicks in?  I mean, for
> all we know, the user unplugged the DP monitor/hub/etc and plugged in
> a different one while we were suspended..

We need to be able to restore mst to avoid upsetting the desktop. I don't
think this would work.
-Daniel

> 
> BR,
> -R
> 
> > So for hotunplug the flow should always be:
> > 1. kernel notices something has changed in the output config.
> > 2. kernel sends out uevent
> > 3. userspace figures out what changed, and what needs to be done
> > 4. userspace asks the kernel to change display configuration through
> > setCrtc and Atomic ioctl calls.
> >
> > Irrespective of hotunplug handling, the kernel also _must_ restore the
> > entire display configuration before userspace resumes. We can do that
> > asynchronously (and there's plans for that), but even then we must stall
> > userspace on the first KMS ioclt to keep up the illusion that a system s/r
> > is transparent.
> >
> > In short, even if we do hpd processing before resuming the display,
> > nothing will be faster - we must wait for userspace to make up its mind,
> > and that can only happen once we've restored the display config.
> >
> > And again, mst is kinda breaking this, since and mst unplug results in a
> > force-disable. Which can upset userspace and in general results in the
> > need for lots of fragile error handling all over.
> >
> >> >We originally changed the resume order in
> >> >
> >> >     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
> >> >
> >> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> >> >issue now anyhow.
> >> >
> >> >CC: stable@vger.kernel.org
> >> >Signed-off-by: Lyude <cpaul@redhat.com>
> >
> > Wrt the patch itself: I think only in 4.6 we've actually fixed up all
> > these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
> > modeset state readout for mst is still not entirely correct (it still
> > relies on software state), so I'm not sure we've actually managed to shut
> > up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
> > while suspended. In short the get_hw_state functions for mst should not
> > rely on any mst software state, but instead just look at hw registers and
> > dp aux registers to figure out what's going on. But not sure whether this
> > will result on WARNINGs on resume, since generally the bios doesn't enable
> > anything in that case.
> >
> > Furthermore MST still does a force-modeset when processing a hotunplug.
> > Doing that before we've resumed the display is likely a very bad idea.
> > What we need to fix that part is to separate the dp mst connector
> > hotplug/unplugging from actually updating the modeset change. This needs
> > reference-counting on drm_connector (so that we can lazily free
> > drm_connector structs after hot-unplug), and is a major change.
> >
> > In short: I'm scared about this patch ;-)
> >
> > Thanks, Daniel
> >
> >
> >> >---
> >> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
> >> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> >index f357058..4dcf3dd 100644
> >> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >> >@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
> >> >     intel_opregion_setup(dev);
> >> >     intel_init_pch_refclk(dev);
> >> >+
> >> >+    /*
> >> >+     * We need to make sure that we resume MST before doing anything
> >> >+     * display related, otherwise we risk trying to bring up a display on
> >> >+     * MST before the hub is actually ready
> >> >+     */
> >> >+    intel_dp_mst_resume(dev);
> >> >+
> >> This does not look proper. if the CD clock is turned off during suspend
> >> our dpcd read itself might fail till we enable CD Clock.
> >>
> >> regards,
> >> Sivakumar
> >> >     drm_mode_config_reset(dev);
> >> >     /*
> >> >@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >> >     intel_display_resume(dev);
> >> >     drm_modeset_unlock_all(dev);
> >> >-    intel_dp_mst_resume(dev);
> >> >-
> >> >     /*
> >> >      * ... but also need to make sure that hotplug processing
> >> >      * doesn't cause havoc. Like in the driver load code we don't
> >>
> >> _______________________________________________
> >> 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Daniel Vetter]

On Mon, Feb 29, 2016 at 06:33:42PM -0500, Rob Clark wrote:
> On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
> >>
> >>
> >> On 2/24/2016 3:41 AM, Lyude wrote:
> >> >As it turns out, resuming DP MST is racey since we don't make sure MST
> >> >is ready before we start modesetting, it just usually happens to be
> >> >ready in time. This isn't the case on all systems, particularly a
> >> >ThinkPad T560 with displays connected through the dock. On these
> >> >systems, resuming the laptop while connected to the dock usually results
> >> >in blank monitors. Making sure MST is ready before doing any kind of
> >> >modesetting fixes this issue.
> >> basic question since i haven't worked on MST much. MST should work like any
> >> other digital panel on resume. i.e detect followed by modeset. in the
> >> modified
> >> commit mentioned below is it failing to detect the panel or failing at the
> >> modeset ?
> >> if we are depending on the intel_display_resume, how about moving the
> >> intel_dp_mst_resume just above intel_display_resume?
> >>
> >>
> >> Generic question to others in mail list on i915_drm_resume
> >> we are doing a modeset and then doing the detect/hpd init.
> >> shouldn't this be the other way round ? almost all displays
> >> will pass a modeset even if display is not connected so we
> >> are spending time on modeset even for displays that were
> >> removed during the suspend state. if this is to simply
> >> drm_state being saved and restored,
> >
> > We must restore anyway, we're not really allowed to shut down a display
> > without userspace's consent. DP mst breaks this, and it's not fun really.
> 
> well, that isn't completely true.. I mean, if the user unplugs (for
> example) an hdmi monitor, it isn't with userspace's consent..

But the pipe keeps running, which means the next pageflip or vblank wait
won't just fail and result in hilarity.

> I wonder if we could just have flag per connector, ie.
> connector->no_auto_resume and just automatically send unplug events
> for those to userspace (followed shortly by a plug event if it is
> still connected and the normal hpd mechanism kicks in?  I mean, for
> all we know, the user unplugged the DP monitor/hub/etc and plugged in
> a different one while we were suspended..

We need to be able to restore mst to avoid upsetting the desktop. I don't
think this would work.
-Daniel

> 
> BR,
> -R
> 
> > So for hotunplug the flow should always be:
> > 1. kernel notices something has changed in the output config.
> > 2. kernel sends out uevent
> > 3. userspace figures out what changed, and what needs to be done
> > 4. userspace asks the kernel to change display configuration through
> > setCrtc and Atomic ioctl calls.
> >
> > Irrespective of hotunplug handling, the kernel also _must_ restore the
> > entire display configuration before userspace resumes. We can do that
> > asynchronously (and there's plans for that), but even then we must stall
> > userspace on the first KMS ioclt to keep up the illusion that a system s/r
> > is transparent.
> >
> > In short, even if we do hpd processing before resuming the display,
> > nothing will be faster - we must wait for userspace to make up its mind,
> > and that can only happen once we've restored the display config.
> >
> > And again, mst is kinda breaking this, since and mst unplug results in a
> > force-disable. Which can upset userspace and in general results in the
> > need for lots of fragile error handling all over.
> >
> >> >We originally changed the resume order in
> >> >
> >> >     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
> >> >
> >> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> >> >issue now anyhow.
> >> >
> >> >CC: stable@vger.kernel.org
> >> >Signed-off-by: Lyude <cpaul@redhat.com>
> >
> > Wrt the patch itself: I think only in 4.6 we've actually fixed up all
> > these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
> > modeset state readout for mst is still not entirely correct (it still
> > relies on software state), so I'm not sure we've actually managed to shut
> > up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
> > while suspended. In short the get_hw_state functions for mst should not
> > rely on any mst software state, but instead just look at hw registers and
> > dp aux registers to figure out what's going on. But not sure whether this
> > will result on WARNINGs on resume, since generally the bios doesn't enable
> > anything in that case.
> >
> > Furthermore MST still does a force-modeset when processing a hotunplug.
> > Doing that before we've resumed the display is likely a very bad idea.
> > What we need to fix that part is to separate the dp mst connector
> > hotplug/unplugging from actually updating the modeset change. This needs
> > reference-counting on drm_connector (so that we can lazily free
> > drm_connector structs after hot-unplug), and is a major change.
> >
> > In short: I'm scared about this patch ;-)
> >
> > Thanks, Daniel
> >
> >
> >> >---
> >> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
> >> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> >index f357058..4dcf3dd 100644
> >> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >> >@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
> >> >     intel_opregion_setup(dev);
> >> >     intel_init_pch_refclk(dev);
> >> >+
> >> >+    /*
> >> >+     * We need to make sure that we resume MST before doing anything
> >> >+     * display related, otherwise we risk trying to bring up a display on
> >> >+     * MST before the hub is actually ready
> >> >+     */
> >> >+    intel_dp_mst_resume(dev);
> >> >+
> >> This does not look proper. if the CD clock is turned off during suspend
> >> our dpcd read itself might fail till we enable CD Clock.
> >>
> >> regards,
> >> Sivakumar
> >> >     drm_mode_config_reset(dev);
> >> >     /*
> >> >@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >> >     intel_display_resume(dev);
> >> >     drm_modeset_unlock_all(dev);
> >> >-    intel_dp_mst_resume(dev);
> >> >-
> >> >     /*
> >> >      * ... but also need to make sure that hotplug processing
> >> >      * doesn't cause havoc. Like in the driver load code we don't
> >>
> >> _______________________________________________
> >> 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

-- 
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: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Daniel Vetter]

On Tue, Mar 01, 2016 at 06:17:33AM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 3/1/2016 5:03 AM, Rob Clark wrote:
> >On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >>On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
> >>>
> >>>On 2/24/2016 3:41 AM, Lyude wrote:
> >>>>As it turns out, resuming DP MST is racey since we don't make sure MST
> >>>>is ready before we start modesetting, it just usually happens to be
> >>>>ready in time. This isn't the case on all systems, particularly a
> >>>>ThinkPad T560 with displays connected through the dock. On these
> >>>>systems, resuming the laptop while connected to the dock usually results
> >>>>in blank monitors. Making sure MST is ready before doing any kind of
> >>>>modesetting fixes this issue.
> >>>basic question since i haven't worked on MST much. MST should work like any
> >>>other digital panel on resume. i.e detect followed by modeset. in the
> >>>modified
> >>>commit mentioned below is it failing to detect the panel or failing at the
> >>>modeset ?
> >>>if we are depending on the intel_display_resume, how about moving the
> >>>intel_dp_mst_resume just above intel_display_resume?
> >>>
> >>>
> >>>Generic question to others in mail list on i915_drm_resume
> >>>we are doing a modeset and then doing the detect/hpd init.
> >>>shouldn't this be the other way round ? almost all displays
> >>>will pass a modeset even if display is not connected so we
> >>>are spending time on modeset even for displays that were
> >>>removed during the suspend state. if this is to simply
> >>>drm_state being saved and restored,
> >>We must restore anyway, we're not really allowed to shut down a display
> >>without userspace's consent. DP mst breaks this, and it's not fun really.
> >well, that isn't completely true.. I mean, if the user unplugs (for
> >example) an hdmi monitor, it isn't with userspace's consent..
> >
> >I wonder if we could just have flag per connector, ie.
> >connector->no_auto_resume and just automatically send unplug events
> >for those to userspace (followed shortly by a plug event if it is
> >still connected and the normal hpd mechanism kicks in?  I mean, for
> >all we know, the user unplugged the DP monitor/hub/etc and plugged in
> >a different one while we were suspended..
> >
> >BR,
> >-R
> i agree. performing a modeset on a display without even confirming
> if it is the same one when we had suspended the system will result in
> issues such as applying a mode that may not be supported on the
> new display or corruption or blankout or simply failing the modeset
> restore or worst case of doing modeset without a display connected.
> if we will not allow such a scenario during normal operation, i.e prevent
> incorrect modeset requests during normal functioning, why allow such
> a modeset during suspend-resume alone ?
> we can store the edid hash and if the display has the same hash
> post resume then we can allow mode to be restored.

We do allow such a situation for non-mst. Userspace can set whatever mode
it wants, on even disconnected outputs. Kernel simply executes. MST is
different here because we're not talking about the sink connected to the
MST branch that's unplugged, but the branch itself (with all it's
connectors).

One of those things is connector->status (enabling an unplugged screen).
The other is the existence of the connector itself (mst branch unplug).
-Daniel

> 
> regards,
> Sivakumar
> >>So for hotunplug the flow should always be:
> >>1. kernel notices something has changed in the output config.
> >>2. kernel sends out uevent
> >>3. userspace figures out what changed, and what needs to be done
> >>4. userspace asks the kernel to change display configuration through
> >>setCrtc and Atomic ioctl calls.
> >>
> >>Irrespective of hotunplug handling, the kernel also _must_ restore the
> >>entire display configuration before userspace resumes. We can do that
> >>asynchronously (and there's plans for that), but even then we must stall
> >>userspace on the first KMS ioclt to keep up the illusion that a system s/r
> >>is transparent.
> >>
> >>In short, even if we do hpd processing before resuming the display,
> >>nothing will be faster - we must wait for userspace to make up its mind,
> >>and that can only happen once we've restored the display config.
> >>
> >>And again, mst is kinda breaking this, since and mst unplug results in a
> >>force-disable. Which can upset userspace and in general results in the
> >>need for lots of fragile error handling all over.
> >>
> >>>>We originally changed the resume order in
> >>>>
> >>>>     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
> >>>>
> >>>>to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> >>>>issue now anyhow.
> >>>>
> >>>>CC: stable@vger.kernel.org
> >>>>Signed-off-by: Lyude <cpaul@redhat.com>
> >>Wrt the patch itself: I think only in 4.6 we've actually fixed up all
> >>these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
> >>modeset state readout for mst is still not entirely correct (it still
> >>relies on software state), so I'm not sure we've actually managed to shut
> >>up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
> >>while suspended. In short the get_hw_state functions for mst should not
> >>rely on any mst software state, but instead just look at hw registers and
> >>dp aux registers to figure out what's going on. But not sure whether this
> >>will result on WARNINGs on resume, since generally the bios doesn't enable
> >>anything in that case.
> >>
> >>Furthermore MST still does a force-modeset when processing a hotunplug.
> >>Doing that before we've resumed the display is likely a very bad idea.
> >>What we need to fix that part is to separate the dp mst connector
> >>hotplug/unplugging from actually updating the modeset change. This needs
> >>reference-counting on drm_connector (so that we can lazily free
> >>drm_connector structs after hot-unplug), and is a major change.
> >>
> >>In short: I'm scared about this patch ;-)
> >>
> >>Thanks, Daniel
> >>
> >>
> >>>>---
> >>>>  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
> >>>>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>>>index f357058..4dcf3dd 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_drv.c
> >>>>+++ b/drivers/gpu/drm/i915/i915_drv.c
> >>>>@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
> >>>>     intel_opregion_setup(dev);
> >>>>     intel_init_pch_refclk(dev);
> >>>>+
> >>>>+    /*
> >>>>+     * We need to make sure that we resume MST before doing anything
> >>>>+     * display related, otherwise we risk trying to bring up a display on
> >>>>+     * MST before the hub is actually ready
> >>>>+     */
> >>>>+    intel_dp_mst_resume(dev);
> >>>>+
> >>>This does not look proper. if the CD clock is turned off during suspend
> >>>our dpcd read itself might fail till we enable CD Clock.
> >>>
> >>>regards,
> >>>Sivakumar
> >>>>     drm_mode_config_reset(dev);
> >>>>     /*
> >>>>@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >>>>     intel_display_resume(dev);
> >>>>     drm_modeset_unlock_all(dev);
> >>>>-    intel_dp_mst_resume(dev);
> >>>>-
> >>>>     /*
> >>>>      * ... but also need to make sure that hotplug processing
> >>>>      * doesn't cause havoc. Like in the driver load code we don't
> >>>_______________________________________________
> >>>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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Rob Clark]

On Wed, Mar 2, 2016 at 4:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Feb 29, 2016 at 06:33:42PM -0500, Rob Clark wrote:
>> On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
>> >>
>> >>
>> >> On 2/24/2016 3:41 AM, Lyude wrote:
>> >> >As it turns out, resuming DP MST is racey since we don't make sure MST
>> >> >is ready before we start modesetting, it just usually happens to be
>> >> >ready in time. This isn't the case on all systems, particularly a
>> >> >ThinkPad T560 with displays connected through the dock. On these
>> >> >systems, resuming the laptop while connected to the dock usually results
>> >> >in blank monitors. Making sure MST is ready before doing any kind of
>> >> >modesetting fixes this issue.
>> >> basic question since i haven't worked on MST much. MST should work like any
>> >> other digital panel on resume. i.e detect followed by modeset. in the
>> >> modified
>> >> commit mentioned below is it failing to detect the panel or failing at the
>> >> modeset ?
>> >> if we are depending on the intel_display_resume, how about moving the
>> >> intel_dp_mst_resume just above intel_display_resume?
>> >>
>> >>
>> >> Generic question to others in mail list on i915_drm_resume
>> >> we are doing a modeset and then doing the detect/hpd init.
>> >> shouldn't this be the other way round ? almost all displays
>> >> will pass a modeset even if display is not connected so we
>> >> are spending time on modeset even for displays that were
>> >> removed during the suspend state. if this is to simply
>> >> drm_state being saved and restored,
>> >
>> > We must restore anyway, we're not really allowed to shut down a display
>> > without userspace's consent. DP mst breaks this, and it's not fun really.
>>
>> well, that isn't completely true.. I mean, if the user unplugs (for
>> example) an hdmi monitor, it isn't with userspace's consent..
>
> But the pipe keeps running, which means the next pageflip or vblank wait
> won't just fail and result in hilarity.

well, then shut it down?  But what do we do on hot unplug?  It seems
like we should be able to make it look like an unplug plus later
replug.

>> I wonder if we could just have flag per connector, ie.
>> connector->no_auto_resume and just automatically send unplug events
>> for those to userspace (followed shortly by a plug event if it is
>> still connected and the normal hpd mechanism kicks in?  I mean, for
>> all we know, the user unplugged the DP monitor/hub/etc and plugged in
>> a different one while we were suspended..
>
> We need to be able to restore mst to avoid upsetting the desktop. I don't
> think this would work.

I think the desktop already supports hotplug, or we'd have bigger issues.

What happens if the DP topology changes while suspended.  Ie. user
plugs in a different monitor with a hub in between, or things like
that?  Seems like you can end up in scenarios where previous mode is
no longer valid/possible.

BR,
-R

> -Daniel
>
>>
>> BR,
>> -R
>>
>> > So for hotunplug the flow should always be:
>> > 1. kernel notices something has changed in the output config.
>> > 2. kernel sends out uevent
>> > 3. userspace figures out what changed, and what needs to be done
>> > 4. userspace asks the kernel to change display configuration through
>> > setCrtc and Atomic ioctl calls.
>> >
>> > Irrespective of hotunplug handling, the kernel also _must_ restore the
>> > entire display configuration before userspace resumes. We can do that
>> > asynchronously (and there's plans for that), but even then we must stall
>> > userspace on the first KMS ioclt to keep up the illusion that a system s/r
>> > is transparent.
>> >
>> > In short, even if we do hpd processing before resuming the display,
>> > nothing will be faster - we must wait for userspace to make up its mind,
>> > and that can only happen once we've restored the display config.
>> >
>> > And again, mst is kinda breaking this, since and mst unplug results in a
>> > force-disable. Which can upset userspace and in general results in the
>> > need for lots of fragile error handling all over.
>> >
>> >> >We originally changed the resume order in
>> >> >
>> >> >     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
>> >> >
>> >> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>> >> >issue now anyhow.
>> >> >
>> >> >CC: stable@vger.kernel.org
>> >> >Signed-off-by: Lyude <cpaul@redhat.com>
>> >
>> > Wrt the patch itself: I think only in 4.6 we've actually fixed up all
>> > these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
>> > modeset state readout for mst is still not entirely correct (it still
>> > relies on software state), so I'm not sure we've actually managed to shut
>> > up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
>> > while suspended. In short the get_hw_state functions for mst should not
>> > rely on any mst software state, but instead just look at hw registers and
>> > dp aux registers to figure out what's going on. But not sure whether this
>> > will result on WARNINGs on resume, since generally the bios doesn't enable
>> > anything in that case.
>> >
>> > Furthermore MST still does a force-modeset when processing a hotunplug.
>> > Doing that before we've resumed the display is likely a very bad idea.
>> > What we need to fix that part is to separate the dp mst connector
>> > hotplug/unplugging from actually updating the modeset change. This needs
>> > reference-counting on drm_connector (so that we can lazily free
>> > drm_connector structs after hot-unplug), and is a major change.
>> >
>> > In short: I'm scared about this patch ;-)
>> >
>> > Thanks, Daniel
>> >
>> >
>> >> >---
>> >> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>> >> >  1 file changed, 8 insertions(+), 2 deletions(-)
>> >> >
>> >> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> >> >index f357058..4dcf3dd 100644
>> >> >--- a/drivers/gpu/drm/i915/i915_drv.c
>> >> >+++ b/drivers/gpu/drm/i915/i915_drv.c
>> >> >@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>> >> >     intel_opregion_setup(dev);
>> >> >     intel_init_pch_refclk(dev);
>> >> >+
>> >> >+    /*
>> >> >+     * We need to make sure that we resume MST before doing anything
>> >> >+     * display related, otherwise we risk trying to bring up a display on
>> >> >+     * MST before the hub is actually ready
>> >> >+     */
>> >> >+    intel_dp_mst_resume(dev);
>> >> >+
>> >> This does not look proper. if the CD clock is turned off during suspend
>> >> our dpcd read itself might fail till we enable CD Clock.
>> >>
>> >> regards,
>> >> Sivakumar
>> >> >     drm_mode_config_reset(dev);
>> >> >     /*
>> >> >@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>> >> >     intel_display_resume(dev);
>> >> >     drm_modeset_unlock_all(dev);
>> >> >-    intel_dp_mst_resume(dev);
>> >> >-
>> >> >     /*
>> >> >      * ... but also need to make sure that hotplug processing
>> >> >      * doesn't cause havoc. Like in the driver load code we don't
>> >>
>> >> _______________________________________________
>> >> 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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Rob Clark]

On Wed, Mar 2, 2016 at 4:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Feb 29, 2016 at 06:33:42PM -0500, Rob Clark wrote:
>> On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
>> >>
>> >>
>> >> On 2/24/2016 3:41 AM, Lyude wrote:
>> >> >As it turns out, resuming DP MST is racey since we don't make sure MST
>> >> >is ready before we start modesetting, it just usually happens to be
>> >> >ready in time. This isn't the case on all systems, particularly a
>> >> >ThinkPad T560 with displays connected through the dock. On these
>> >> >systems, resuming the laptop while connected to the dock usually results
>> >> >in blank monitors. Making sure MST is ready before doing any kind of
>> >> >modesetting fixes this issue.
>> >> basic question since i haven't worked on MST much. MST should work like any
>> >> other digital panel on resume. i.e detect followed by modeset. in the
>> >> modified
>> >> commit mentioned below is it failing to detect the panel or failing at the
>> >> modeset ?
>> >> if we are depending on the intel_display_resume, how about moving the
>> >> intel_dp_mst_resume just above intel_display_resume?
>> >>
>> >>
>> >> Generic question to others in mail list on i915_drm_resume
>> >> we are doing a modeset and then doing the detect/hpd init.
>> >> shouldn't this be the other way round ? almost all displays
>> >> will pass a modeset even if display is not connected so we
>> >> are spending time on modeset even for displays that were
>> >> removed during the suspend state. if this is to simply
>> >> drm_state being saved and restored,
>> >
>> > We must restore anyway, we're not really allowed to shut down a display
>> > without userspace's consent. DP mst breaks this, and it's not fun really.
>>
>> well, that isn't completely true.. I mean, if the user unplugs (for
>> example) an hdmi monitor, it isn't with userspace's consent..
>
> But the pipe keeps running, which means the next pageflip or vblank wait
> won't just fail and result in hilarity.

well, then shut it down?  But what do we do on hot unplug?  It seems
like we should be able to make it look like an unplug plus later
replug.

>> I wonder if we could just have flag per connector, ie.
>> connector->no_auto_resume and just automatically send unplug events
>> for those to userspace (followed shortly by a plug event if it is
>> still connected and the normal hpd mechanism kicks in?  I mean, for
>> all we know, the user unplugged the DP monitor/hub/etc and plugged in
>> a different one while we were suspended..
>
> We need to be able to restore mst to avoid upsetting the desktop. I don't
> think this would work.

I think the desktop already supports hotplug, or we'd have bigger issues.

What happens if the DP topology changes while suspended.  Ie. user
plugs in a different monitor with a hub in between, or things like
that?  Seems like you can end up in scenarios where previous mode is
no longer valid/possible.

BR,
-R

> -Daniel
>
>>
>> BR,
>> -R
>>
>> > So for hotunplug the flow should always be:
>> > 1. kernel notices something has changed in the output config.
>> > 2. kernel sends out uevent
>> > 3. userspace figures out what changed, and what needs to be done
>> > 4. userspace asks the kernel to change display configuration through
>> > setCrtc and Atomic ioctl calls.
>> >
>> > Irrespective of hotunplug handling, the kernel also _must_ restore the
>> > entire display configuration before userspace resumes. We can do that
>> > asynchronously (and there's plans for that), but even then we must stall
>> > userspace on the first KMS ioclt to keep up the illusion that a system s/r
>> > is transparent.
>> >
>> > In short, even if we do hpd processing before resuming the display,
>> > nothing will be faster - we must wait for userspace to make up its mind,
>> > and that can only happen once we've restored the display config.
>> >
>> > And again, mst is kinda breaking this, since and mst unplug results in a
>> > force-disable. Which can upset userspace and in general results in the
>> > need for lots of fragile error handling all over.
>> >
>> >> >We originally changed the resume order in
>> >> >
>> >> >     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
>> >> >
>> >> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
>> >> >issue now anyhow.
>> >> >
>> >> >CC: stable@vger.kernel.org
>> >> >Signed-off-by: Lyude <cpaul@redhat.com>
>> >
>> > Wrt the patch itself: I think only in 4.6 we've actually fixed up all
>> > these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
>> > modeset state readout for mst is still not entirely correct (it still
>> > relies on software state), so I'm not sure we've actually managed to shut
>> > up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
>> > while suspended. In short the get_hw_state functions for mst should not
>> > rely on any mst software state, but instead just look at hw registers and
>> > dp aux registers to figure out what's going on. But not sure whether this
>> > will result on WARNINGs on resume, since generally the bios doesn't enable
>> > anything in that case.
>> >
>> > Furthermore MST still does a force-modeset when processing a hotunplug.
>> > Doing that before we've resumed the display is likely a very bad idea.
>> > What we need to fix that part is to separate the dp mst connector
>> > hotplug/unplugging from actually updating the modeset change. This needs
>> > reference-counting on drm_connector (so that we can lazily free
>> > drm_connector structs after hot-unplug), and is a major change.
>> >
>> > In short: I'm scared about this patch ;-)
>> >
>> > Thanks, Daniel
>> >
>> >
>> >> >---
>> >> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
>> >> >  1 file changed, 8 insertions(+), 2 deletions(-)
>> >> >
>> >> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> >> >index f357058..4dcf3dd 100644
>> >> >--- a/drivers/gpu/drm/i915/i915_drv.c
>> >> >+++ b/drivers/gpu/drm/i915/i915_drv.c
>> >> >@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
>> >> >     intel_opregion_setup(dev);
>> >> >     intel_init_pch_refclk(dev);
>> >> >+
>> >> >+    /*
>> >> >+     * We need to make sure that we resume MST before doing anything
>> >> >+     * display related, otherwise we risk trying to bring up a display on
>> >> >+     * MST before the hub is actually ready
>> >> >+     */
>> >> >+    intel_dp_mst_resume(dev);
>> >> >+
>> >> This does not look proper. if the CD clock is turned off during suspend
>> >> our dpcd read itself might fail till we enable CD Clock.
>> >>
>> >> regards,
>> >> Sivakumar
>> >> >     drm_mode_config_reset(dev);
>> >> >     /*
>> >> >@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
>> >> >     intel_display_resume(dev);
>> >> >     drm_modeset_unlock_all(dev);
>> >> >-    intel_dp_mst_resume(dev);
>> >> >-
>> >> >     /*
>> >> >      * ... but also need to make sure that hotplug processing
>> >> >      * doesn't cause havoc. Like in the driver load code we don't
>> >>
>> >> _______________________________________________
>> >> 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
>
> --
> 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