[Intel-gfx] [PATCH xf86-video-intel 1/2] sna: Fix dirtyfb detection

[Intel-gfx] [PATCH xf86-video-intel 1/2] sna: Fix dirtyfb detection

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Fix the accidentally swapped bpp and depth values passed to
the addfb ioctl when we're testing for dirtyfb presence.
Currently the addfb fails every time so we don't even test
the actual dirtyfb ioctl.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 src/sna/kgem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/sna/kgem.c b/src/sna/kgem.c
index 9c0708a635fb..6a35067c4107 100644
--- a/src/sna/kgem.c
+++ b/src/sna/kgem.c
@@ -1538,8 +1538,8 @@ static bool test_has_dirtyfb(struct kgem *kgem)
 	create.width = 32;
 	create.height = 32;
 	create.pitch = 4*32;
-	create.bpp = 24;
-	create.depth = 32; /* {bpp:24, depth:32} -> x8r8g8b8 */
+	create.bpp = 32;
+	create.depth = 24; /* {bpp:32, depth:24} -> x8r8g8b8 */
 	create.handle = gem_create(kgem->fd, 1);
 	if (create.handle == 0)
 		return false;
-- 
2.23.0

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

[Intel-gfx] [PATCH xf86-video-intel 2/2] sna: Eliminate sna_mode_wants_tear_free()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The modparam checks performed by sna_mode_wants_tear_free() don't
generally work when the server is running as a regular user. Hence
we can't rely on them to indicate whether FBC/PSR/etc is enabled.
A lso the "Panel Self-Refresh" connector property doesn't actually
exist so we can nuke that part as well. Let's just nuke the whole
thing and assume we want dirtyfb always when tearfree is not enabled.

I'll anyway want to enable FBC by default across the board soonish
so the check wouldn't really buy us much (would just exclude i830
and a few old desktop chipsets which don't have FBC hardware).

Additionally if we don't have working dirtyfb we really should
enable tearfree by default because otherwise we're going to
get horrible lag due to missing frontbuffer flushes.

Without WC mmaps we could in theory rely on the hw gtt tracking
except the kernel no longer differentiates between GTT/WC/CPU
access in its software frontbuffer tracking code so it'll just
deactivate FBC even for a GTT mmap and potentially never re-enable
it due to the missing frontbuffer flush from dirtyfb. So dirtyfb
is always needed.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 src/sna/sna_display.c | 59 -------------------------------------------
 src/sna/sna_driver.c  |  8 ++----
 2 files changed, 2 insertions(+), 65 deletions(-)

diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index 874292bcab31..966ad9638a2a 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -7951,65 +7951,6 @@ bool sna_mode_pre_init(ScrnInfoPtr scrn, struct sna *sna)
 	return scrn->modes != NULL;
 }
 
-bool
-sna_mode_wants_tear_free(struct sna *sna)
-{
-	xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(sna->scrn);
-	bool found = false;
-	FILE *file;
-	int i;
-
-	file = fopen("/sys/module/i915/parameters/enable_fbc", "r");
-	if (file) {
-		int fbc_enabled = 0;
-		int value;
-
-		if (fscanf(file, "%d", &value) == 1)
-			fbc_enabled = value > 0;
-		fclose(file);
-
-		DBG(("%s: module parameter 'enable_fbc' enabled? %d\n",
-		     __FUNCTION__, fbc_enabled));
-
-		if (fbc_enabled)
-			return true;
-	}
-
-	for (i = 0; i < sna->mode.num_real_output; i++) {
-		struct sna_output *output = to_sna_output(config->output[i]);
-		int id = find_property(sna, output, "Panel Self-Refresh");
-		if (id == -1)
-			continue;
-
-		found = true;
-		if (output->prop_values[id] != -1) {
-			DBG(("%s: Panel Self-Refresh detected on %s\n",
-			     __FUNCTION__, config->output[i]->name));
-			return true;
-		}
-	}
-
-	if (!found) {
-		file = fopen("/sys/module/i915/parameters/enable_psr", "r");
-		if (file) {
-			int psr_enabled = 0;
-			int value;
-
-			if (fscanf(file, "%d", &value) == 1)
-				psr_enabled = value > 0;
-			fclose(file);
-
-			DBG(("%s: module parameter 'enable_psr' enabled? %d\n",
-			     __FUNCTION__, psr_enabled));
-
-			if (psr_enabled)
-				return true;
-		}
-	}
-
-	return false;
-}
-
 void
 sna_mode_set_primary(struct sna *sna)
 {
diff --git a/src/sna/sna_driver.c b/src/sna/sna_driver.c
index 4f8895f1eda2..2f61de63072f 100644
--- a/src/sna/sna_driver.c
+++ b/src/sna/sna_driver.c
@@ -459,11 +459,7 @@ static bool enable_tear_free(struct sna *sna)
 	if (sna->flags & SNA_LINEAR_FB)
 		return false;
 
-	/* Under certain conditions, we should enable TearFree by default,
-	 * for example when the hardware requires pageflipping to run within
-	 * its power/performance budget.
-	 */
-	if (sna_mode_wants_tear_free(sna))
+	if (!sna->kgem.has_dirtyfb)
 		return true;
 
 	return ENABLE_TEAR_FREE;
@@ -663,7 +659,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.23.0

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

Re: [Intel-gfx] [PATCH xf86-video-intel 2/2] sna: Eliminate sna_mode_wants_tear_free()

[Chris Wilson]

Quoting Ville Syrjala (2019-12-09 15:01:37)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The modparam checks performed by sna_mode_wants_tear_free() don't
> generally work when the server is running as a regular user. Hence
> we can't rely on them to indicate whether FBC/PSR/etc is enabled.
> A lso the "Panel Self-Refresh" connector property doesn't actually
> exist so we can nuke that part as well. Let's just nuke the whole
> thing and assume we want dirtyfb always when tearfree is not enabled.
> 
> I'll anyway want to enable FBC by default across the board soonish
> so the check wouldn't really buy us much (would just exclude i830
> and a few old desktop chipsets which don't have FBC hardware).
> 
> Additionally if we don't have working dirtyfb we really should
> enable tearfree by default because otherwise we're going to
> get horrible lag due to missing frontbuffer flushes.

But we also want to enable TearFree anyway in most cases, and here we
are defaulting to off in cases where it was already on.

I still don't know on what grounds the cut-off should be based, the
primary question is can we afford to keep an extra framebuffer plus any
gubbins memory? The worry about perf are now larger moot, so it boils
down to available memory -- in quite a few cases TearFree is a big
improvement on power management, but that I guess is currently snb+
(although we can fix ilk render powerstandby).

How about GTT > mappable aperture, based on the idea that we have room
to spare that can't be used for scanout? That would only disable gen2 by
default.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH xf86-video-intel 2/2] sna: Eliminate sna_mode_wants_tear_free()

[Ville Syrjälä]

On Mon, Dec 09, 2019 at 03:13:13PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2019-12-09 15:01:37)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The modparam checks performed by sna_mode_wants_tear_free() don't
> > generally work when the server is running as a regular user. Hence
> > we can't rely on them to indicate whether FBC/PSR/etc is enabled.
> > A lso the "Panel Self-Refresh" connector property doesn't actually
> > exist so we can nuke that part as well. Let's just nuke the whole
> > thing and assume we want dirtyfb always when tearfree is not enabled.
> > 
> > I'll anyway want to enable FBC by default across the board soonish
> > so the check wouldn't really buy us much (would just exclude i830
> > and a few old desktop chipsets which don't have FBC hardware).
> > 
> > Additionally if we don't have working dirtyfb we really should
> > enable tearfree by default because otherwise we're going to
> > get horrible lag due to missing frontbuffer flushes.
> 
> But we also want to enable TearFree anyway in most cases, and here we
> are defaulting to off in cases where it was already on.
> 
> I still don't know on what grounds the cut-off should be based, the
> primary question is can we afford to keep an extra framebuffer plus any
> gubbins memory? The worry about perf are now larger moot, so it boils
> down to available memory -- in quite a few cases TearFree is a big
> improvement on power management, but that I guess is currently snb+
> (although we can fix ilk render powerstandby).
> 
> How about GTT > mappable aperture, based on the idea that we have room
> to spare that can't be used for scanout? That would only disable gen2 by
> default.

Not sure. Ideally we should perhaps make it dynamic and enable tearfree
only when the extra framebuffers won't hog too much of the gtt/physical
RAM? But since it's not dynamic currently I guess that would involve
some actual work.

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

Re: [Intel-gfx] [PATCH xf86-video-intel 2/2] sna: Eliminate sna_mode_wants_tear_free()

[Ville Syrjälä]

On Mon, Dec 09, 2019 at 03:13:13PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2019-12-09 15:01:37)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The modparam checks performed by sna_mode_wants_tear_free() don't
> > generally work when the server is running as a regular user. Hence
> > we can't rely on them to indicate whether FBC/PSR/etc is enabled.
> > A lso the "Panel Self-Refresh" connector property doesn't actually
> > exist so we can nuke that part as well. Let's just nuke the whole
> > thing and assume we want dirtyfb always when tearfree is not enabled.
> > 
> > I'll anyway want to enable FBC by default across the board soonish
> > so the check wouldn't really buy us much (would just exclude i830
> > and a few old desktop chipsets which don't have FBC hardware).
> > 
> > Additionally if we don't have working dirtyfb we really should
> > enable tearfree by default because otherwise we're going to
> > get horrible lag due to missing frontbuffer flushes.
> 
> But we also want to enable TearFree anyway in most cases, and here we
> are defaulting to off in cases where it was already on.
> 
> I still don't know on what grounds the cut-off should be based, the
> primary question is can we afford to keep an extra framebuffer plus any
> gubbins memory? The worry about perf are now larger moot, so it boils
> down to available memory -- in quite a few cases TearFree is a big
> improvement on power management, but that I guess is currently snb+
> (although we can fix ilk render powerstandby).
> 
> How about GTT > mappable aperture, based on the idea that we have room
> to spare that can't be used for scanout? That would only disable gen2 by
> default.

So thinking about this thing again. If we go with the mappable vs. gtt
size check, what do we want to do with the meson/autoconf tearfree knob.
Just nuke it? Or maybe we want it to override all the heuristics?

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

Re: [Intel-gfx] [PATCH xf86-video-intel 1/2] sna: Fix dirtyfb detection

[Chris Wilson]

Quoting Ville Syrjala (2019-12-09 15:01:36)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Fix the accidentally swapped bpp and depth values passed to
> the addfb ioctl when we're testing for dirtyfb presence.
> Currently the addfb fails every time so we don't even test
> the actual dirtyfb ioctl.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  src/sna/kgem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/sna/kgem.c b/src/sna/kgem.c
> index 9c0708a635fb..6a35067c4107 100644
> --- a/src/sna/kgem.c
> +++ b/src/sna/kgem.c
> @@ -1538,8 +1538,8 @@ static bool test_has_dirtyfb(struct kgem *kgem)
>         create.width = 32;
>         create.height = 32;
>         create.pitch = 4*32;
> -       create.bpp = 24;
> -       create.depth = 32; /* {bpp:24, depth:32} -> x8r8g8b8 */
> +       create.bpp = 32;
> +       create.depth = 24; /* {bpp:32, depth:24} -> x8r8g8b8 */

I did good.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [xf86-video-intel,1/2] sna: Fix dirtyfb detection

[Patchwork]

== Series Details ==

Series: series starting with [xf86-video-intel,1/2] sna: Fix dirtyfb detection
URL   : https://patchwork.freedesktop.org/series/70636/
State : failure

== Summary ==

Applying: sna: Fix dirtyfb detection
error: sha1 information is lacking or useless (src/sna/kgem.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 sna: Fix dirtyfb detection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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