diff mbox

[v2] drm/i915/fbdev: Enable late fbdev initial configuration

Message ID 20180418234158.9388-1-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Souza, Jose April 18, 2018, 11:41 p.m. UTC 
If the initial fbdev configuration(intel_fbdev_initial_config()) runs and
there still no sink connected it will cause
drm_fb_helper_initial_config() to return 0 as no error happened(but
internally the return is -EAGAIN).
Because no framebuffer was allocated, when a sink is connected
intel_fbdev_output_poll_changed() will not execute
drm_fb_helper_hotplug_event() that would trigger another try to do the
initial fbdev configuration.

So here allowing drm_fb_helper_hotplug_event() to be executed when there
is not frambebuffer allocated and fbdev was not setup yet.

This issue also happens when a MST DP sink is connected since boot, as
the MST topology is discovered in parallel if intel_fbdev_initial_config()
is executed before the first sink MST is discovered it will cause this
same issue.

This is a follow up patch of
https://patchwork.freedesktop.org/patch/196089/

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104158
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104425
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---

Changes from v1:
- not creating a dump framebuffer anymore, instead just allowing
drm_fb_helper_hotplug_event() to execute when fbdev is not setup yet.

 drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jani Nikula April 19, 2018, 12:50 p.m. UTC | #1 
On Wed, 18 Apr 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> If the initial fbdev configuration(intel_fbdev_initial_config()) runs and
> there still no sink connected it will cause
> drm_fb_helper_initial_config() to return 0 as no error happened(but
> internally the return is -EAGAIN).
> Because no framebuffer was allocated, when a sink is connected
> intel_fbdev_output_poll_changed() will not execute
> drm_fb_helper_hotplug_event() that would trigger another try to do the
> initial fbdev configuration.
>
> So here allowing drm_fb_helper_hotplug_event() to be executed when there
> is not frambebuffer allocated and fbdev was not setup yet.
>
> This issue also happens when a MST DP sink is connected since boot, as
> the MST topology is discovered in parallel if intel_fbdev_initial_config()
> is executed before the first sink MST is discovered it will cause this
> same issue.
>
> This is a follow up patch of
> https://patchwork.freedesktop.org/patch/196089/

What does this mean? Is that patch a required dependency? What?

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104158
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104425

People responded to v1, please ask them to test v2.

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>
> Changes from v1:
> - not creating a dump framebuffer anymore, instead just allowing
> drm_fb_helper_hotplug_event() to execute when fbdev is not setup yet.

Please look at git log in drm, and observe how we include the changelog
in the commit message itself.

BR,
Jani.

>
>  drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 7d41d139341b..e9e02b58b7be 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct drm_device *dev)
>  		return;
>  
>  	intel_fbdev_sync(ifbdev);
> -	if (ifbdev->vma)
> +	if (ifbdev->vma || ifbdev->helper.deferred_setup)
>  		drm_fb_helper_hotplug_event(&ifbdev->helper);
>  }
Souza, Jose April 19, 2018, 8:12 p.m. UTC | #2 
On Thu, 2018-04-19 at 15:50 +0300, Jani Nikula wrote:
> On Wed, 18 Apr 2018, José Roberto de Souza <jose.souza@intel.com>

> wrote:

> > If the initial fbdev configuration(intel_fbdev_initial_config())

> > runs and

> > there still no sink connected it will cause

> > drm_fb_helper_initial_config() to return 0 as no error happened(but

> > internally the return is -EAGAIN).

> > Because no framebuffer was allocated, when a sink is connected

> > intel_fbdev_output_poll_changed() will not execute

> > drm_fb_helper_hotplug_event() that would trigger another try to do

> > the

> > initial fbdev configuration.

> > 

> > So here allowing drm_fb_helper_hotplug_event() to be executed when

> > there

> > is not frambebuffer allocated and fbdev was not setup yet.

> > 

> > This issue also happens when a MST DP sink is connected since boot,

> > as

> > the MST topology is discovered in parallel if

> > intel_fbdev_initial_config()

> > is executed before the first sink MST is discovered it will cause

> > this

> > same issue.

> > 

> > This is a follow up patch of

> > https://patchwork.freedesktop.org/patch/196089/

> 

> What does this mean? Is that patch a required dependency? What?


No requirement, this is just a follow up patch of that one, that patch
was not accepted and Chris did a suggestion that was implemented in the
first version.

> 

> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104158

> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104425

> 

> People responded to v1, please ask them to test v2.


Okay, I just asked people to test the version 2 but I'm also able to
reproduce the issue and it was already tested.

> 

> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

> > ---

> > 

> > Changes from v1:

> > - not creating a dump framebuffer anymore, instead just allowing

> > drm_fb_helper_hotplug_event() to execute when fbdev is not setup

> > yet.

> 

> Please look at git log in drm, and observe how we include the

> changelog

> in the commit message itself.


Okay, I will do that from now on.

> 

> BR,

> Jani.

> 

> > 

> >  drivers/gpu/drm/i915/intel_fbdev.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c

> > b/drivers/gpu/drm/i915/intel_fbdev.c

> > index 7d41d139341b..e9e02b58b7be 100644

> > --- a/drivers/gpu/drm/i915/intel_fbdev.c

> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c

> > @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct

> > drm_device *dev)

> >  		return;

> >  

> >  	intel_fbdev_sync(ifbdev);

> > -	if (ifbdev->vma)

> > +	if (ifbdev->vma || ifbdev->helper.deferred_setup)

> >  		drm_fb_helper_hotplug_event(&ifbdev->helper);

> >  }

> 

>
Paul Menzel April 20, 2018, 9:33 a.m. UTC | #3 
Dear Jose,


On 04/19/18 01:41, Souza, Jose wrote:
> If the initial fbdev configuration(intel_fbdev_initial_config()) runs and

Nit: Space before (.

> there still no sink connected it will cause
> drm_fb_helper_initial_config() to return 0 as no error happened(but
> internally the return is -EAGAIN).

Nit: Space before (.

> Because no framebuffer was allocated, when a sink is connected
> intel_fbdev_output_poll_changed() will not execute
> drm_fb_helper_hotplug_event() that would trigger another try to do the
> initial fbdev configuration.
> 
> So here allowing drm_fb_helper_hotplug_event() to be executed when there
> is not frambebuffer allocated and fbdev was not setup yet.

s/not/no/
s/setup/set up/

> This issue also happens when a MST DP sink is connected since boot, as
> the MST topology is discovered in parallel if intel_fbdev_initial_config()
> is executed before the first sink MST is discovered it will cause this
> same issue.
> 
> This is a follow up patch of

follow-up

> https://patchwork.freedesktop.org/patch/196089/
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104158
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104425
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> Changes from v1:
> - not creating a dump framebuffer anymore, instead just allowing
> drm_fb_helper_hotplug_event() to execute when fbdev is not setup yet.

s/setup/set up/

>   drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 7d41d139341b..e9e02b58b7be 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct drm_device *dev)
>   		return;
>   
>   	intel_fbdev_sync(ifbdev);
> -	if (ifbdev->vma)
> +	if (ifbdev->vma || ifbdev->helper.deferred_setup)
>   		drm_fb_helper_hotplug_event(&ifbdev->helper);
>   }
>   

Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul
Jani Nikula April 23, 2018, 2:05 p.m. UTC | #4 
On Fri, 20 Apr 2018, Paul Menzel <pmenzel+intel-gfx@molgen.mpg.de> wrote:
> Dear Jose,
>
>
> On 04/19/18 01:41, Souza, Jose wrote:
>> If the initial fbdev configuration(intel_fbdev_initial_config()) runs and
>
> Nit: Space before (.
>
>> there still no sink connected it will cause
>> drm_fb_helper_initial_config() to return 0 as no error happened(but
>> internally the return is -EAGAIN).
>
> Nit: Space before (.
>
>> Because no framebuffer was allocated, when a sink is connected
>> intel_fbdev_output_poll_changed() will not execute
>> drm_fb_helper_hotplug_event() that would trigger another try to do the
>> initial fbdev configuration.
>> 
>> So here allowing drm_fb_helper_hotplug_event() to be executed when there
>> is not frambebuffer allocated and fbdev was not setup yet.
>
> s/not/no/
> s/setup/set up/
>
>> This issue also happens when a MST DP sink is connected since boot, as
>> the MST topology is discovered in parallel if intel_fbdev_initial_config()
>> is executed before the first sink MST is discovered it will cause this
>> same issue.
>> 
>> This is a follow up patch of
>
> follow-up
>
>> https://patchwork.freedesktop.org/patch/196089/
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104158
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104425
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> ---
>> 
>> Changes from v1:
>> - not creating a dump framebuffer anymore, instead just allowing
>> drm_fb_helper_hotplug_event() to execute when fbdev is not setup yet.
>
> s/setup/set up/
>
>>   drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>> index 7d41d139341b..e9e02b58b7be 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct drm_device *dev)
>>   		return;
>>   
>>   	intel_fbdev_sync(ifbdev);
>> -	if (ifbdev->vma)
>> +	if (ifbdev->vma || ifbdev->helper.deferred_setup)
>>   		drm_fb_helper_hotplug_event(&ifbdev->helper);
>>   }
>>   
>
> Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>

Also from the bug,

Tested-by: frederik <frederik.schwan@linux.com> # 4.15.17
Tested-by: Ian Pilcher <arequipeno@gmail.com>

>
>
> Kind regards,
>
> Paul
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson April 24, 2018, 8:47 a.m. UTC | #5 
Quoting José Roberto de Souza (2018-04-19 00:41:58)
> If the initial fbdev configuration(intel_fbdev_initial_config()) runs and
> there still no sink connected it will cause
> drm_fb_helper_initial_config() to return 0 as no error happened(but
> internally the return is -EAGAIN).
> Because no framebuffer was allocated, when a sink is connected
> intel_fbdev_output_poll_changed() will not execute
> drm_fb_helper_hotplug_event() that would trigger another try to do the
> initial fbdev configuration.
> 
> So here allowing drm_fb_helper_hotplug_event() to be executed when there
> is not frambebuffer allocated and fbdev was not setup yet.
> 
> This issue also happens when a MST DP sink is connected since boot, as
> the MST topology is discovered in parallel if intel_fbdev_initial_config()
> is executed before the first sink MST is discovered it will cause this
> same issue.
> 
> This is a follow up patch of
> https://patchwork.freedesktop.org/patch/196089/
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104158
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104425
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> Changes from v1:
> - not creating a dump framebuffer anymore, instead just allowing
> drm_fb_helper_hotplug_event() to execute when fbdev is not setup yet.
> 
>  drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 7d41d139341b..e9e02b58b7be 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct drm_device *dev)
>                 return;
>  
>         intel_fbdev_sync(ifbdev);
> -       if (ifbdev->vma)
> +       if (ifbdev->vma || ifbdev->helper.deferred_setup)

Feels slightly dodgy, vma can only be set and deferred_setup only be
cleared at runtime, so at worst we may miss sending a hpd event during a
runtime configuration phase. Which is probably just as well.

What happens if we send two hpd events, and the first gets a deferred setup
that fails. Do we hit the previous error condition? At least we are not
hitting the condition of running the hotplug event while trying to do
the initial configuration, but does the deferred setup have the same
bugs?

>                 drm_fb_helper_hotplug_event(&ifbdev->helper);

I fear this is just poking the bear,
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Jani Nikula April 24, 2018, 2:50 p.m. UTC | #6 
On Tue, 24 Apr 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting José Roberto de Souza (2018-04-19 00:41:58)
>> If the initial fbdev configuration(intel_fbdev_initial_config()) runs and
>> there still no sink connected it will cause
>> drm_fb_helper_initial_config() to return 0 as no error happened(but
>> internally the return is -EAGAIN).
>> Because no framebuffer was allocated, when a sink is connected
>> intel_fbdev_output_poll_changed() will not execute
>> drm_fb_helper_hotplug_event() that would trigger another try to do the
>> initial fbdev configuration.
>> 
>> So here allowing drm_fb_helper_hotplug_event() to be executed when there
>> is not frambebuffer allocated and fbdev was not setup yet.
>> 
>> This issue also happens when a MST DP sink is connected since boot, as
>> the MST topology is discovered in parallel if intel_fbdev_initial_config()
>> is executed before the first sink MST is discovered it will cause this
>> same issue.
>> 
>> This is a follow up patch of
>> https://patchwork.freedesktop.org/patch/196089/
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104158
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104425
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> ---
>> 
>> Changes from v1:
>> - not creating a dump framebuffer anymore, instead just allowing
>> drm_fb_helper_hotplug_event() to execute when fbdev is not setup yet.
>> 
>>  drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>> index 7d41d139341b..e9e02b58b7be 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -807,7 +807,7 @@ void intel_fbdev_output_poll_changed(struct drm_device *dev)
>>                 return;
>>  
>>         intel_fbdev_sync(ifbdev);
>> -       if (ifbdev->vma)
>> +       if (ifbdev->vma || ifbdev->helper.deferred_setup)
>
> Feels slightly dodgy, vma can only be set and deferred_setup only be
> cleared at runtime, so at worst we may miss sending a hpd event during a
> runtime configuration phase. Which is probably just as well.
>
> What happens if we send two hpd events, and the first gets a deferred setup
> that fails. Do we hit the previous error condition? At least we are not
> hitting the condition of running the hotplug event while trying to do
> the initial configuration, but does the deferred setup have the same
> bugs?
>
>>                 drm_fb_helper_hotplug_event(&ifbdev->helper);
>
> I fear this is just poking the bear,
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for the patch and the ack. Pushed. If it's not a fix, at least it
papers over the regression for now...

BR,
Jani.



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

Patch

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 7d41d139341b..e9e02b58b7be 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -807,7 +807,7 @@  void intel_fbdev_output_poll_changed(struct drm_device *dev)
 		return;
 
 	intel_fbdev_sync(ifbdev);
-	if (ifbdev->vma)
+	if (ifbdev->vma || ifbdev->helper.deferred_setup)
 		drm_fb_helper_hotplug_event(&ifbdev->helper);
 }