Re: [Intel-gfx] [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA
On Thu, Sep 11, 2014 at 12:53:30PM -0700, Keith Packard wrote: Chris Wilson ch...@chris-wilson.co.uk writes: That extra alignment is due to gen2 and early gen3 (if (!intel-has_relaxed_fencing) covers them). Here's the patch which changed the alignment requirment: [snip commits picked at random] This is the root commit d21d781466785c317131a8a57606925867265dc8 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Tue Feb 22 18:31:44 2011 +0100 Fix relaxed tiling on gen2 Later we went on to disable relaxed tiling even after believing we had fixed all the kernel bugs: commit 686018f283f1d131073ef5917213e6a8ac013f26 Author: Chris Wilson ch...@chris-wilson.co.uk Date: Tue Apr 12 08:23:04 2011 +0100 Turn relaxed-fencing off by default for older (pre-G33) chipsets I believe the even-tile row alignment is still key to having gen2/gen3 function with relaxed fencing. If you have specific bug reports that were resolved by this patch, or specific hardware documentation which indicates that this patch is required, especially as it relates to gen2 and gen3 hardware, I'd love to see them. Try enabling relaxed fencing again. In any case, we've now got four versions of the pixmap alignment code (libdrm, uxa and sna in two varieties). They're all subtly different; one suspects that each one works on some set of problems and fails on others... Here's what the height alignment requirements are: libdrm uxa uxa sna sna +keithp=2.6.38 2.6.38 gen2 none 2 2 2 1 2 gen3 none 2 2 2 1 2 gen4+ none2 2 2 1 1 gen2 X 16 16 32 16 32 gen3 X8 8 16 8 16 gen4+ X 8 8 16 8 8 gen2 Y 16 16 32 16 32 i915 Y8 32 64 8 16 i945 Y 32 32 64 8 16 gen4+ Y 32 32 64 32 32 It looks like the SNA alignment for untiled buffers is incorrect? 965 hardware is documented to read buffers in 2x2 chunks, so a failure to height align allocations to 2 can result in reads off the end of the buffer. Reading from the scratch page is not a problem. Reading from neighbouring surfaces is of no concern. The allocation must be suitable and aligned appropriately for writes, but writes themselves are appropriately clipped. Otherwise one extra row doesn't save you from scribbling over anywhere in your gtt. For uxa's intel_set_pixmap_bo, and sna's sna_dri3_pixmap_from_fd, there's a clear requirement that the 2D driver impose no stricter alignment than libdrm, so that, buffer passing from Mesa to X will work. No. The clearest requirement is that the ddx (or other display server) must treat incoming surfaces as tainted and validate them to be sure that they work with its code paths. If it can't we have a choice of either rejecting them outright, or staging them. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/4] drm/i915: Move the cursor_base setup to i{845, 9xx}_update_cursor()
On Fri, Sep 12, 2014 at 08:53:32PM +0300, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com To make the code a bit more undestandable move the intel_crtc-cursor_base assignment into the low level update cursor routines. That's were we compare the current value with the new one so immediately seeing that it gets assigned only afterwards helps one to understand that it gets assigned only after the comparison. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com I'd been tempted to do this myself, but lacked justification. Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/4] drm/i915: Only set CURSOR_PIPE_CSC_ENABLE when cursor is enabled
On Fri, Sep 12, 2014 at 08:53:33PM +0300, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com It seems cleaner if we keep CURCNTR at 0 when the cursor is disabled, so don't set the CURSOR_PIPE_CSC_ENABLE bit unless the cursor is enabled. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 3/4] drm/i915: Skip CURBASE write when nothing changed
On Fri, Sep 12, 2014 at 08:53:34PM +0300, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Only write CURBASE when something about the cursor changed. Also
eliminate the unnecessary posting read after writing CURCNTR.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_display.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 82c0ad1..60c1aa4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8322,16 +8322,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc,
u32 base)
cntl |= CURSOR_PIPE_CSC_ENABLE;
}
- if (intel_crtc-cursor_cntl != cntl) {
+ if (intel_crtc-cursor_cntl != cntl)
I915_WRITE(CURCNTR(pipe), cntl);
- POSTING_READ(CURCNTR(pipe));
- intel_crtc-cursor_cntl = cntl;
- }
+
+ if (intel_crtc-cursor_cntl == cntl
+ intel_crtc-cursor_base == base)
+ return;
I'd vote for doing this first and then
I915_WRITE(CURCNTR(pipe), cntl);
unconditionally along with the CURBASE flush.
/* and commit changes on next vblank */
I915_WRITE(CURBASE(pipe), base);
POSTING_READ(CURBASE(pipe));
+ intel_crtc-cursor_cntl = cntl;
intel_crtc-cursor_base = base;
}
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915: Support variable cursor height on ivb+
On Fri, Sep 12, 2014 at 08:53:35PM +0300, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com IVB introduced the CUR_FBC_CTL register which allows reducing the cursor height down to 8 lines from the otherwise square cursor dimensions. Implement support for it. Commandeer the otherwise unused intel_crtc-cursor_size to track the current value of CUR_FBC_CTL so that we can avoid duplicating the complicated device type checks in i9xx_update_cursor(). One caveat to note is that CUR_FBC_CTL can't be used with 180 degree rotation, so when cursor rotation gets introduced some extra checks are needed. Where would be a good place to put that note into a comment? So other than the mystery of rotated cursors, the code looks clear enough. One side question, is the CHV/VLV conflation correct here? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/19] drm: Don't update vblank timestamp when the counter didn't change
The current drm-next misses Ville's original Patch 14/19, the one i first objected, then objected to my objection. It is needed to avoid actual regressions. Attached a trivially rebased (v2) of Ville's patch to go on top of drm-next, also as tgz in case my e-mail client mangles the patch again, because it's one of those email hates me weeks. -mario On 08/06/2014 01:49 PM, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com If we already have a timestamp for the current vblank counter, don't update it with a new timestmap. Small errors can creep in between two timestamp queries for the same vblank count, which could be confusing to userspace when it queries the timestamp for the same vblank sequence number twice. This problem gets exposed when the vblank disable timer is not used (or is set to expire quickly) and thus we can get multiple vblank disable-enable transition during the same frame which would all attempt to update the timestamp with the latest estimate. Testcase: igt/kms_flip/flip-vs-expired-vblank Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index af33df1..0523f5b 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -106,6 +106,9 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) DRM_DEBUG(enabling vblank interrupts on crtc %d, missed %d\n, crtc, diff); + if (diff == 0) + return; + /* Reinitialize corresponding vblank timestamp if high-precision query * available. Skip this step if query unsupported or failed. Will * reinitialize delayed at next vblank interrupt in that case. From c0a5228a7fc43d4c3615a471c340b68bcb2caa16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= ville.syrj...@linux.intel.com Date: Wed, 6 Aug 2014 14:49:57 +0300 Subject: [PATCH] drm: Don't update vblank timestamp when the counter didn't change (v2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we already have a timestamp for the current vblank counter, don't update it with a new timestmap. Small errors can creep in between two timestamp queries for the same vblank count, which could be confusing to userspace when it queries the timestamp for the same vblank sequence number twice. This problem gets exposed when the vblank disable timer is not used (or is set to expire quickly) and thus we can get multiple vblank disable-enable transition during the same frame which would all attempt to update the timestamp with the latest estimate. Testcase: igt/kms_flip/flip-vs-expired-vblank Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com Reviewed-by: Mario Kleiner mario.kleiner...@gmail.com v2:Mario: Trivial rebase on top of current drm-next (13-Sep-2014) Signed-off-by: Mario Kleiner mario.kleiner...@gmail.com --- drivers/gpu/drm/drm_irq.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 80ff94a..e73cbda 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -126,6 +126,9 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) DRM_DEBUG(updating vblank count on crtc %d, missed %d\n, crtc, diff); + if (diff == 0) + return; + /* Reinitialize corresponding vblank timestamp if high-precision query * available. Skip this step if query unsupported or failed. Will * reinitialize delayed at next vblank interrupt in that case. -- 1.9.1 0001-drm-Don-t-update-vblank-timestamp-when-the-counter-d.patch.tar.gz Description: application/gzip ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] intel_driver.c:1182:2: error: implicit declaration of function 'intel_sync_close' is invalid in C99
Building intel-ddx from GIT HEAD breaks here with LLVM v3.4.2 like this...
...
intel_driver.c:1182:2: error: implicit declaration of function
'intel_sync_close' is invalid in C99
[-Werror,-Wimplicit-function-declaration]
intel_sync_close(screen);
^
In file included from intel_uxa.c:44:
./intel_glamor.h:92:1: warning: unused function
'intel_glamor_fd_from_pixmap' [-Wunused-function]
intel_glamor_fd_from_pixmap(ScreenPtr screen,
^
intel_driver.c:1182:2: note: did you mean 'intel_mode_close'?
./intel.h:356:13: note: 'intel_mode_close' declared here
extern void intel_mode_close(intel_screen_private *intel);
^
...
Please have a look at the full build-log.
Thanks.
- Sedat -
P:S:: GIT HEAD is here...
commit 48f6406a62c06a09b173d82b8eb79761188ff717 (Use intel_uxa.h in
all uxa-specific files)
P.S.S.: FYI... v2.99.916 is OK.
autoreconf: Entering directory `.'
autoreconf: configure.ac: not using Gettext
autoreconf: running: aclocal ${ACLOCAL_FLAGS} -I m4
autoreconf: configure.ac: tracing
autoreconf: running: libtoolize --install --copy
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, `.'.
libtoolize: copying file `./config.guess'
libtoolize: copying file `./config.sub'
libtoolize: copying file `./install-sh'
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
autoreconf: running: /usr/bin/autoconf
autoreconf: running: /usr/bin/autoheader
autoreconf: running: automake --add-missing --copy --no-force
configure.ac:35: installing `./missing'
libobj/Makefile.am: installing `./depcomp'
autoreconf: Leaving directory `.'
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking for style of include used by make... GNU
checking for gcc... clang
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether clang accepts -g... yes
checking for clang option to accept ISO C89... none needed
checking dependency style of clang... gcc3
checking for clang option to accept ISO C99... none needed
checking how to run the C preprocessor... clang-cpp
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for ANSI C header files... yes
checking for sys/types.h... yes
checking for sys/stat.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for memory.h... yes
checking for strings.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for unistd.h... yes
checking whether __clang__ is declared... yes
checking whether __INTEL_COMPILER is declared... no
checking whether __SUNPRO_C is declared... no
checking for pkg-config... /usr/bin/pkg-config
checking pkg-config is at least version 0.9.0... yes
checking build system type... x86_64-unknown-linux-gnu
checking host system type... x86_64-unknown-linux-gnu
checking for a sed that does not truncate output... /bin/sed
checking if clang supports -Werror=unknown-warning-option... yes
checking if clang supports -Werror=unused-command-line-argument... yes
checking if clang supports -Wall... yes
checking if clang supports -Wpointer-arith... yes
checking if clang supports -Wmissing-declarations... yes
checking if clang supports -Wformat=2... yes
checking if clang supports -Wstrict-prototypes... yes
checking if clang supports -Wmissing-prototypes... yes
checking if clang supports -Wnested-externs... yes
checking if clang supports -Wbad-function-cast... yes
checking if clang supports -Wold-style-definition... yes
checking if clang supports -Wdeclaration-after-statement... yes
checking if clang supports -Wunused... yes
checking if clang supports -Wuninitialized... yes
checking if clang supports -Wshadow... yes
checking if clang supports -Wcast-qual... yes
checking if clang supports -Wmissing-noreturn... yes
checking if clang supports -Wmissing-format-attribute... yes
checking if clang supports -Wredundant-decls... yes
checking if clang supports -Werror=implicit... yes
checking if clang supports -Werror=nonnull... yes
checking if clang supports -Werror=init-self... yes
checking if clang supports -Werror=main... yes
checking if clang supports -Werror=missing-braces... yes
checking if clang supports -Werror=sequence-point... yes
checking if clang supports -Werror=return-type... yes
checking if clang supports -Werror=trigraphs... yes
checking if clang supports -Werror=array-bounds... yes
checking if clang
Re: [Intel-gfx] intel_driver.c:1182:2: error: implicit declaration of function 'intel_sync_close' is invalid in C99
On Sat, Sep 13, 2014 at 7:15 PM, Sedat Dilek sedat.di...@gmail.com wrote: Building intel-ddx from GIT HEAD breaks here with LLVM v3.4.2 like this... ... intel_driver.c:1182:2: error: implicit declaration of function 'intel_sync_close' is invalid in C99 [-Werror,-Wimplicit-function-declaration] intel_sync_close(screen); ^ In file included from intel_uxa.c:44: ./intel_glamor.h:92:1: warning: unused function 'intel_glamor_fd_from_pixmap' [-Wunused-function] intel_glamor_fd_from_pixmap(ScreenPtr screen, ^ intel_driver.c:1182:2: note: did you mean 'intel_mode_close'? ./intel.h:356:13: note: 'intel_mode_close' declared here extern void intel_mode_close(intel_screen_private *intel); ^ ... Please have a look at the full build-log. Thanks. - Sedat - P:S:: GIT HEAD is here... commit 48f6406a62c06a09b173d82b8eb79761188ff717 (Use intel_uxa.h in all uxa-specific files) P.S.S.: FYI... v2.99.916 is OK. Looking at the sources... ./src/uxa/intel.h-516-#if HAVE_DRI3 ./src/uxa/intel.h-517-Bool intel_sync_init(ScreenPtr screen); ./src/uxa/intel.h:518:void intel_sync_close(ScreenPtr screen); ./src/uxa/intel.h-519-#endif ...and in my logs... $ grep DRI3 ../logs/build-and-install-log_XF86-Video-Intel-v2-99-916-38-g48f6406_llvm-3-4-2.txt checking for X11_DRI3... yes checking if DRI3 is defined... no checking whether to include DRI3 support... no Then intel_sync_close() should be embedded into a HAVE_DRI3 check? $ git diff diff --git a/src/uxa/intel_driver.c b/src/uxa/intel_driver.c index f31f7bd..9f527fd 100644 --- a/src/uxa/intel_driver.c +++ b/src/uxa/intel_driver.c @@ -1179,7 +1179,9 @@ static Bool I830CloseScreen(CLOSE_SCREEN_ARGS_DECL) intel-dri3 = DRI_NONE; } +#if HAVE_DRI3 intel_sync_close(screen); +#endif xf86GARTCloseScreen(scrn-scrnIndex); Regards, - Sedat - ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA
Chris Wilson ch...@chris-wilson.co.uk writes: commit d21d781466785c317131a8a57606925867265dc8 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Tue Feb 22 18:31:44 2011 +0100 Fix relaxed tiling on gen2 This one matches libdrm in using 16 for the tile height alignment on gen2. Try enabling relaxed fencing again. No. The clearest requirement is that the ddx (or other display server) must treat incoming surfaces as tainted and validate them to be sure that they work with its code paths. If it can't we have a choice of either rejecting them outright, or staging them. If there's a stricter alignment requirement, then we must fix both the 2D driver and libdrm. Otherwise, the user's session will simply crash at startup. However, I still see absolutely no evidence that gen2 requires tile alignment to 32 rows, or that gen3+ requires tile alignment to 16 rows in any software configuration at all. -- keith.pack...@intel.com pgpB1HedFzRjW.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] uxa: intel_sync_close() is only available when HAVE_DRI3
With LLVM v3.4.2 I got this error reported: ... intel_driver.c:1182:2: error: implicit declaration of function 'intel_sync_close' is invalid in C99 [-Werror,-Wimplicit-function-declaration] intel_sync_close(screen); ^ In file included from intel_uxa.c:44: ./intel_glamor.h:92:1: warning: unused function 'intel_glamor_fd_from_pixmap' [-Wunused-function] intel_glamor_fd_from_pixmap(ScreenPtr screen, ^ intel_driver.c:1182:2: note: did you mean 'intel_mode_close'? ./intel.h:356:13: note: 'intel_mode_close' declared here extern void intel_mode_close(intel_screen_private *intel); ... Looking at uxa/intel.h intel_sync_close() is only available when DRI3 is supported. 516: #if HAVE_DRI3 517: Bool intel_sync_init(ScreenPtr screen); 518: void intel_sync_close(ScreenPtr screen); 519: #endif Fix the issue by embedding intel_sync_close() with a HAVE_DRI3 ifdef check. Signed-off-by: Sedat Dilek sedat.di...@gmail.com --- src/uxa/intel_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/uxa/intel_driver.c b/src/uxa/intel_driver.c index f31f7bd..9f527fd 100644 --- a/src/uxa/intel_driver.c +++ b/src/uxa/intel_driver.c @@ -1179,7 +1179,9 @@ static Bool I830CloseScreen(CLOSE_SCREEN_ARGS_DECL) intel-dri3 = DRI_NONE; } +#if HAVE_DRI3 intel_sync_close(screen); +#endif xf86GARTCloseScreen(scrn-scrnIndex); -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
