diff mbox

[RFC,xf86-video-intel] sna: Call dirtyfb for all non-tear-free cases

Message ID 1458588418-31129-3-git-send-email-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R March 21, 2016, 7:26 p.m. UTC
The sna_mode_wants_tear_free() function tries to detect FBC and PSR
based on Kernel module parameters. Currently it fails to detect FBC
due to the default enable_fbc value being -1. While this can easily be
fixed in the Kernel, I had a conversation with Daniel and he expressed
unhappiness with that solution, claiming that yet another different
code path just for a feature that should be transparent is not a good
way to go, and that we should do proper frontbuffer rendering.

So with this patch, we'll have the DDX issuing dirtyfb calls even if
TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.

This fixes a bug that happens on SKL with FBC enabled: if you run
lightdm, your login/password won't appear as you type on your
keyboard. You have to move the mouse over the input box for them to be
displayed.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 src/sna/sna_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Vetter March 22, 2016, 11:31 a.m. UTC | #1
On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:
> The sna_mode_wants_tear_free() function tries to detect FBC and PSR
> based on Kernel module parameters. Currently it fails to detect FBC
> due to the default enable_fbc value being -1. While this can easily be
> fixed in the Kernel, I had a conversation with Daniel and he expressed
> unhappiness with that solution, claiming that yet another different
> code path just for a feature that should be transparent is not a good
> way to go, and that we should do proper frontbuffer rendering.
> 
> So with this patch, we'll have the DDX issuing dirtyfb calls even if
> TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.
> 
> This fixes a bug that happens on SKL with FBC enabled: if you run
> lightdm, your login/password won't appear as you type on your
> keyboard. You have to move the mouse over the input box for them to be
> displayed.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I thought we need this anyway to get the kernel to allow fbc, since SNA
ends up mmap some of the drm_framebuffer. Even when they're not
frontbuffers.
-Daniel

> ---
>  src/sna/sna_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/sna/sna_driver.c b/src/sna/sna_driver.c
> index b245594..84e8e55 100644
> --- a/src/sna/sna_driver.c
> +++ b/src/sna/sna_driver.c
> @@ -654,7 +654,7 @@ static Bool sna_pre_init(ScrnInfoPtr scrn, int probe)
>  	}
>  	scrn->currentMode = scrn->modes;
>  
> -	if (!setup_tear_free(sna) && sna_mode_wants_tear_free(sna))
> +	if (!setup_tear_free(sna))
>  		sna->kgem.needs_dirtyfb = sna->kgem.has_dirtyfb;
>  
>  	xf86SetGamma(scrn, zeros);
> -- 
> 2.7.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson March 22, 2016, 11:36 a.m. UTC | #2
On Tue, Mar 22, 2016 at 12:31:09PM +0100, Daniel Vetter wrote:
> On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:
> > The sna_mode_wants_tear_free() function tries to detect FBC and PSR
> > based on Kernel module parameters. Currently it fails to detect FBC
> > due to the default enable_fbc value being -1. While this can easily be
> > fixed in the Kernel, I had a conversation with Daniel and he expressed
> > unhappiness with that solution, claiming that yet another different
> > code path just for a feature that should be transparent is not a good
> > way to go, and that we should do proper frontbuffer rendering.
> > 
> > So with this patch, we'll have the DDX issuing dirtyfb calls even if
> > TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.
> > 
> > This fixes a bug that happens on SKL with FBC enabled: if you run
> > lightdm, your login/password won't appear as you type on your
> > keyboard. You have to move the mouse over the input box for them to be
> > displayed.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I thought we need this anyway to get the kernel to allow fbc, since SNA
> ends up mmap some of the drm_framebuffer. Even when they're not
> frontbuffers.

Nope. FBC => sna_mode_wants_tear_free() is true.
-Chris
Zanoni, Paulo R March 22, 2016, 9:35 p.m. UTC | #3
Em Ter, 2016-03-22 às 12:31 +0100, Daniel Vetter escreveu:
> On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:

> > 

> > The sna_mode_wants_tear_free() function tries to detect FBC and PSR

> > based on Kernel module parameters. Currently it fails to detect FBC

> > due to the default enable_fbc value being -1. While this can easily

> > be

> > fixed in the Kernel, I had a conversation with Daniel and he

> > expressed

> > unhappiness with that solution, claiming that yet another different

> > code path just for a feature that should be transparent is not a

> > good

> > way to go, and that we should do proper frontbuffer rendering.

> > 

> > So with this patch, we'll have the DDX issuing dirtyfb calls even

> > if

> > TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.

> > 

> > This fixes a bug that happens on SKL with FBC enabled: if you run

> > lightdm, your login/password won't appear as you type on your

> > keyboard. You have to move the mouse over the input box for them to

> > be

> > displayed.

> > 

> > Cc: Chris Wilson <chris@chris-wilson.co.uk>

> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> I thought we need this anyway to get the kernel to allow fbc, since

> SNA

> ends up mmap some of the drm_framebuffer. Even when they're not

> frontbuffers.


If we merge patch 2/4, we won't need this one since TearFree will be in
use, and it seems TearFree doesn't touch frontbuffers, so we'll always
get the flush calls during page flips.

> -Daniel

> 

> > 

> > ---

> >  src/sna/sna_driver.c | 2 +-

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

> > 

> > diff --git a/src/sna/sna_driver.c b/src/sna/sna_driver.c

> > index b245594..84e8e55 100644

> > --- a/src/sna/sna_driver.c

> > +++ b/src/sna/sna_driver.c

> > @@ -654,7 +654,7 @@ static Bool sna_pre_init(ScrnInfoPtr scrn, int

> > probe)

> >  	}

> >  	scrn->currentMode = scrn->modes;

> >  

> > -	if (!setup_tear_free(sna) &&

> > sna_mode_wants_tear_free(sna))

> > +	if (!setup_tear_free(sna))

> >  		sna->kgem.needs_dirtyfb = sna->kgem.has_dirtyfb;

> >  

> >  	xf86SetGamma(scrn, zeros);

> > -- 

> > 2.7.0

> > 

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter March 23, 2016, 8:50 a.m. UTC | #4
On Tue, Mar 22, 2016 at 09:35:36PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-03-22 às 12:31 +0100, Daniel Vetter escreveu:
> > On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:
> > > 
> > > The sna_mode_wants_tear_free() function tries to detect FBC and PSR
> > > based on Kernel module parameters. Currently it fails to detect FBC
> > > due to the default enable_fbc value being -1. While this can easily
> > > be
> > > fixed in the Kernel, I had a conversation with Daniel and he
> > > expressed
> > > unhappiness with that solution, claiming that yet another different
> > > code path just for a feature that should be transparent is not a
> > > good
> > > way to go, and that we should do proper frontbuffer rendering.
> > > 
> > > So with this patch, we'll have the DDX issuing dirtyfb calls even
> > > if
> > > TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.
> > > 
> > > This fixes a bug that happens on SKL with FBC enabled: if you run
> > > lightdm, your login/password won't appear as you type on your
> > > keyboard. You have to move the mouse over the input box for them to
> > > be
> > > displayed.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > I thought we need this anyway to get the kernel to allow fbc, since
> > SNA
> > ends up mmap some of the drm_framebuffer. Even when they're not
> > frontbuffers.
> 
> If we merge patch 2/4, we won't need this one since TearFree will be in
> use, and it seems TearFree doesn't touch frontbuffers, so we'll always
> get the flush calls during page flips.

I'd thought that TearFree would still do rendering with the cpu sometimes,
but only always to the back buffer. So we'd still have cpu mmaps, and
hence the kernel w/a would still potentially kick in and prevent fbc. So
that's not the case then, and we get fbc reliably with the kernel patch,
even without this patch?
-Daniel
Zanoni, Paulo R March 23, 2016, 1:16 p.m. UTC | #5
Em Qua, 2016-03-23 às 09:50 +0100, Daniel Vetter escreveu:
> On Tue, Mar 22, 2016 at 09:35:36PM +0000, Zanoni, Paulo R wrote:

> > 

> > Em Ter, 2016-03-22 às 12:31 +0100, Daniel Vetter escreveu:

> > > 

> > > On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:

> > > > 

> > > > 

> > > > The sna_mode_wants_tear_free() function tries to detect FBC and

> > > > PSR

> > > > based on Kernel module parameters. Currently it fails to detect

> > > > FBC

> > > > due to the default enable_fbc value being -1. While this can

> > > > easily

> > > > be

> > > > fixed in the Kernel, I had a conversation with Daniel and he

> > > > expressed

> > > > unhappiness with that solution, claiming that yet another

> > > > different

> > > > code path just for a feature that should be transparent is not

> > > > a

> > > > good

> > > > way to go, and that we should do proper frontbuffer rendering.

> > > > 

> > > > So with this patch, we'll have the DDX issuing dirtyfb calls

> > > > even

> > > > if

> > > > TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.

> > > > 

> > > > This fixes a bug that happens on SKL with FBC enabled: if you

> > > > run

> > > > lightdm, your login/password won't appear as you type on your

> > > > keyboard. You have to move the mouse over the input box for

> > > > them to

> > > > be

> > > > displayed.

> > > > 

> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>

> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> > > I thought we need this anyway to get the kernel to allow fbc,

> > > since

> > > SNA

> > > ends up mmap some of the drm_framebuffer. Even when they're not

> > > frontbuffers.

> > If we merge patch 2/4, we won't need this one since TearFree will

> > be in

> > use, and it seems TearFree doesn't touch frontbuffers, so we'll

> > always

> > get the flush calls during page flips.

> I'd thought that TearFree would still do rendering with the cpu

> sometimes,

> but only always to the back buffer. So we'd still have cpu mmaps, and

> hence the kernel w/a would still potentially kick in and prevent fbc.

> So

> that's not the case then, and we get fbc reliably with the kernel

> patch,

> even without this patch?


Oh, you're right. But at least we won't have bugs, and we'll have FBC
on things like xf86-video-modesetting, which some distros seem to
already be using for SKL.

Well, now that I think a little more about this, maybe we could add
little hack to DDX so it does a single sw_finish/dirty_fb call to
TearFree framebuffers upon creation so the workaround gets disabled...
Hmm..

> -Daniel
diff mbox

Patch

diff --git a/src/sna/sna_driver.c b/src/sna/sna_driver.c
index b245594..84e8e55 100644
--- a/src/sna/sna_driver.c
+++ b/src/sna/sna_driver.c
@@ -654,7 +654,7 @@  static Bool sna_pre_init(ScrnInfoPtr scrn, int probe)
 	}
 	scrn->currentMode = scrn->modes;
 
-	if (!setup_tear_free(sna) && sna_mode_wants_tear_free(sna))
+	if (!setup_tear_free(sna))
 		sna->kgem.needs_dirtyfb = sna->kgem.has_dirtyfb;
 
 	xf86SetGamma(scrn, zeros);