BUG: 4.10 i915 drm display noise regression - bisected to a6a7cc4b7

BUG: 4.10 i915 drm display noise regression - bisected to a6a7cc4b7

[lkml]

Hello all,

I'm experiencing display noise in the form of 8x1 pixel bars spuriously
appearing in random locations.  This doesn't happen on 4.9, the machine
is an X61s, a Core2Duo 1.8Ghz w/XGA via LVDS.

I was able to bisect the issue to a6a7cc4b7:

commit a6a7cc4b7db6deaeca11cdd38844ea147a354c7a
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Nov 18 21:17:46 2016 +0000

    drm/i915: Always flush the dirty CPU cache when pinning the scanout
    
    Currently we only clflush the scanout if it is in the CPU domain. Also
    flush if we have a pending CPU clflush. We also want to treat the
    dirtyfb path similar, and flush any pending writes there as well.
    
    v2: Only send the fb flush message if flushing the dirt on flip
    v3: Make flush-for-flip and dirtyfb look more alike since they serve
    similar roles as end-of-frame marker.

Reproduction is simple, just run this native drm eye candy program:
https://github.com/vcaputo/rototiller

Thanks,
Vito Caputo

[PATCH] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On a non-llc system, the objects are created with .cache_level =
CACHE_NONE and so the transition to uncached for scanout is a no-op.
However, if the object was never written to, it will still be in the CPU
domain (having been zeroed out by shmemfs). Those cachelines need to be
flushed prior to display.

Reported-by: Vito Caputo
Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
---
 drivers/gpu/drm/i915/i915_gem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 76689b59fc90..e64d0ea6113d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2327,6 +2327,7 @@ static void i915_sg_trim(struct sg_table *orig_st)
 	if (sg_alloc_table(&new_st, orig_st->nents, GFP_KERNEL | __GFP_NOWARN))
 		return;
 
+	new_st->orig_nents = orig_st->orig_nents; /* XXX lies for 
 	new_sg = new_st.sgl;
 	for_each_sg(orig_st->sgl, sg, orig_st->nents, i) {
 		sg_set_page(new_sg, sg_page(sg), sg->length, 0);
@@ -3514,7 +3515,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
 
 	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
-	if (obj->cache_dirty) {
+	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
 		i915_gem_clflush_object(obj, true);
 		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
 	}
-- 
2.11.0

[PATCH] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On a non-llc system, the objects are created with .cache_level =
CACHE_NONE and so the transition to uncached for scanout is a no-op.
However, if the object was never written to, it will still be in the CPU
domain (having been zeroed out by shmemfs). Those cachelines need to be
flushed prior to display.

Reported-by: Vito Caputo
Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
---
 drivers/gpu/drm/i915/i915_gem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 76689b59fc90..e64d0ea6113d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2327,6 +2327,7 @@ static void i915_sg_trim(struct sg_table *orig_st)
 	if (sg_alloc_table(&new_st, orig_st->nents, GFP_KERNEL | __GFP_NOWARN))
 		return;
 
+	new_st->orig_nents = orig_st->orig_nents; /* XXX lies for 
 	new_sg = new_st.sgl;
 	for_each_sg(orig_st->sgl, sg, orig_st->nents, i) {
 		sg_set_page(new_sg, sg_page(sg), sg->length, 0);
@@ -3514,7 +3515,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
 
 	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
-	if (obj->cache_dirty) {
+	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
 		i915_gem_clflush_object(obj, true);
 		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
 	}
-- 
2.11.0

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

Re: [PATCH] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On Mon, Jan 09, 2017 at 10:24:01AM +0000, Chris Wilson wrote:
> On a non-llc system, the objects are created with .cache_level =
> CACHE_NONE and so the transition to uncached for scanout is a no-op.
> However, if the object was never written to, it will still be in the CPU
> domain (having been zeroed out by shmemfs). Those cachelines need to be
> flushed prior to display.
> 
> Reported-by: Vito Caputo
> Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 76689b59fc90..e64d0ea6113d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2327,6 +2327,7 @@ static void i915_sg_trim(struct sg_table *orig_st)
>  	if (sg_alloc_table(&new_st, orig_st->nents, GFP_KERNEL | __GFP_NOWARN))
>  		return;
>  
> +	new_st->orig_nents = orig_st->orig_nents; /* XXX lies for 

Oops. Ignore this chunk!

>  	new_sg = new_st.sgl;
>  	for_each_sg(orig_st->sgl, sg, orig_st->nents, i) {
>  		sg_set_page(new_sg, sg_page(sg), sg->length, 0);
> @@ -3514,7 +3515,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>  
>  	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> -	if (obj->cache_dirty) {
> +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
>  		i915_gem_clflush_object(obj, true);
>  		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
>  	}
> -- 
> 2.11.0
> 

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On Mon, Jan 09, 2017 at 10:24:01AM +0000, Chris Wilson wrote:
> On a non-llc system, the objects are created with .cache_level =
> CACHE_NONE and so the transition to uncached for scanout is a no-op.
> However, if the object was never written to, it will still be in the CPU
> domain (having been zeroed out by shmemfs). Those cachelines need to be
> flushed prior to display.
> 
> Reported-by: Vito Caputo
> Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 76689b59fc90..e64d0ea6113d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2327,6 +2327,7 @@ static void i915_sg_trim(struct sg_table *orig_st)
>  	if (sg_alloc_table(&new_st, orig_st->nents, GFP_KERNEL | __GFP_NOWARN))
>  		return;
>  
> +	new_st->orig_nents = orig_st->orig_nents; /* XXX lies for 

Oops. Ignore this chunk!

>  	new_sg = new_st.sgl;
>  	for_each_sg(orig_st->sgl, sg, orig_st->nents, i) {
>  		sg_set_page(new_sg, sg_page(sg), sg->length, 0);
> @@ -3514,7 +3515,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>  
>  	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> -	if (obj->cache_dirty) {
> +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
>  		i915_gem_clflush_object(obj, true);
>  		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
>  	}
> -- 
> 2.11.0
> 

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Flush untouched framebuffers before display on !llc

On a non-llc system, the objects are created with .cache_level =
CACHE_NONE and so the transition to uncached for scanout is a no-op.
However, if the object was never written to, it will still be in the CPU
domain (having been zeroed out by shmemfs). Those cachelines need to be
flushed prior to display.

Reported-by: Vito Caputo
Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 76689b59fc90..bdb113ef8cfe 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3514,7 +3514,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
 
 	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
-	if (obj->cache_dirty) {
+	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
 		i915_gem_clflush_object(obj, true);
 		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
 	}
-- 
2.11.0

[PATCH v2] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On a non-llc system, the objects are created with .cache_level =
CACHE_NONE and so the transition to uncached for scanout is a no-op.
However, if the object was never written to, it will still be in the CPU
domain (having been zeroed out by shmemfs). Those cachelines need to be
flushed prior to display.

Reported-by: Vito Caputo
Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 76689b59fc90..bdb113ef8cfe 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3514,7 +3514,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
 
 	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
-	if (obj->cache_dirty) {
+	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
 		i915_gem_clflush_object(obj, true);
 		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
 	}
-- 
2.11.0

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

✗ Fi.CI.BAT: failure for drm/i915: Flush untouched framebuffers before display on !llc

[Patchwork]

== Series Details ==

Series: drm/i915: Flush untouched framebuffers before display on !llc
URL   : https://patchwork.freedesktop.org/series/17694/
State : failure

== Summary ==

                          ^
drivers/gpu/drm/i915/i915_gem.c:2301:2: note: in expansion of macro ‘GEM_BUG_ON’
  GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */
  ^
drivers/gpu/drm/i915/i915_gem.c:2303:2: error: data definition has no type or storage class [-Werror]
  sg_free_table(orig_st);
  ^
drivers/gpu/drm/i915/i915_gem.c:2303:2: error: type defaults to ‘int’ in declaration of ‘sg_free_table’ [-Werror=implicit-int]
drivers/gpu/drm/i915/i915_gem.c:2303:2: error: parameter names (without types) in function declaration [-Werror]
drivers/gpu/drm/i915/i915_gem.c:2303:2: error: conflicting types for ‘sg_free_table’
In file included from ./include/linux/dma-mapping.h:10:0,
                 from ./include/drm/drmP.h:37,
                 from drivers/gpu/drm/i915/i915_gem.c:28:
./include/linux/scatterlist.h:260:6: note: previous declaration of ‘sg_free_table’ was here
 void sg_free_table(struct sg_table *);
      ^
drivers/gpu/drm/i915/i915_gem.c:2305:2: error: data definition has no type or storage class [-Werror]
  *orig_st = new_st;
  ^
drivers/gpu/drm/i915/i915_gem.c:2305:3: error: type defaults to ‘int’ in declaration of ‘orig_st’ [-Werror=implicit-int]
  *orig_st = new_st;
   ^
drivers/gpu/drm/i915/i915_gem.c:2305:13: error: ‘new_st’ undeclared here (not in a function)
  *orig_st = new_st;
             ^
drivers/gpu/drm/i915/i915_gem.c:2306:1: error: expected identifier or ‘(’ before ‘}’ token
 }
 ^
  LD      drivers/tty/serial/8250/8250.o
  LD      drivers/iommu/built-in.o
  LD [M]  drivers/ssb/ssb.o
cc1: all warnings being treated as errors
scripts/Makefile.build:293: recipe for target 'drivers/gpu/drm/i915/i915_gem.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_gem.o] Error 1
make[4]: *** Waiting for unfinished jobs....
  LD      net/ipv6/ipv6.o
  LD      drivers/usb/storage/usb-storage.o
  LD      net/xfrm/built-in.o
  LD      drivers/usb/storage/built-in.o
  LD      drivers/pci/built-in.o
  LD      net/ipv6/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000/e1000.o
  LD [M]  sound/pci/hda/snd-hda-codec-generic.o
  LD [M]  drivers/usb/serial/usbserial.o
  LD      drivers/video/fbdev/core/fb.o
  LD      drivers/video/fbdev/core/built-in.o
  LD      sound/pci/built-in.o
  LD      drivers/tty/serial/8250/8250_base.o
  LD      drivers/tty/serial/8250/built-in.o
  LD      drivers/tty/serial/built-in.o
  LD      drivers/thermal/thermal_sys.o
  LD      drivers/video/fbdev/built-in.o
  LD      drivers/thermal/built-in.o
  LD      drivers/scsi/scsi_mod.o
  LD      sound/built-in.o
  LD      drivers/video/console/built-in.o
  AR      lib/lib.a
  LD      drivers/video/built-in.o
  EXPORTS lib/lib-ksyms.o
  LD      drivers/usb/gadget/libcomposite.o
  LD      lib/built-in.o
  LD      fs/btrfs/btrfs.o
  LD      drivers/spi/built-in.o
  LD      drivers/usb/gadget/udc/udc-core.o
  LD      drivers/usb/gadget/udc/built-in.o
  LD      drivers/usb/gadget/built-in.o
  LD      fs/btrfs/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000e/e1000e.o
  LD [M]  drivers/net/ethernet/intel/igb/igb.o
  LD      net/ipv4/built-in.o
  LD      drivers/scsi/sd_mod.o
  LD      drivers/scsi/built-in.o
  CC      arch/x86/kernel/cpu/capflags.o
  LD      arch/x86/kernel/cpu/built-in.o
  LD      arch/x86/kernel/built-in.o
  LD      arch/x86/built-in.o
  LD      drivers/usb/host/xhci-hcd.o
  LD      drivers/md/md-mod.o
  LD      drivers/md/built-in.o
  LD      drivers/usb/core/usbcore.o
  LD      drivers/usb/core/built-in.o
  LD      drivers/tty/vt/built-in.o
  LD      drivers/tty/built-in.o
  LD      fs/ext4/ext4.o
  LD      fs/ext4/built-in.o
  LD      fs/built-in.o
  LD      net/core/built-in.o
  LD      drivers/usb/host/built-in.o
  LD      net/built-in.o
  LD      drivers/usb/built-in.o
  LD      drivers/net/ethernet/built-in.o
  LD      drivers/net/built-in.o
scripts/Makefile.build:551: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:551: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:551: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:988: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

Re: [Intel-gfx] [PATCH] drm/i915: Flush untouched framebuffers before display on !llc

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6555 bytes --]

Hi Chris,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.10-rc3 next-20170106]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Flush-untouched-framebuffers-before-display-on-llc/20170109-190816
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s3-01092001 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_gem.c: In function 'i915_sg_trim':
   drivers/gpu/drm/i915/i915_gem.c:2303:8: error: invalid type argument of '->' (have 'struct sg_table')
     new_st->orig_nents = orig_st->orig_nents; /* XXX lies for
           ^~
>> drivers/gpu/drm/i915/i915_gem.c:2307:3: error: "/*" within comment [-Werror=comment]
      /* called before being DMA mapped, no need to copy sg->dma_* */
       
>> drivers/gpu/drm/i915/i915_gem.c:2295:15: error: unused variable 'i' [-Werror=unused-variable]
     unsigned int i;
                  ^
>> drivers/gpu/drm/i915/i915_gem.c:2294:22: error: unused variable 'sg' [-Werror=unused-variable]
     struct scatterlist *sg, *new_sg;
                         ^~
   In file included from include/linux/debug_locks.h:6:0,
                    from include/linux/lockdep.h:25,
                    from include/linux/spinlock_types.h:18,
                    from include/linux/mutex.h:15,
                    from include/linux/kernfs.h:13,
                    from include/linux/sysfs.h:15,
                    from include/linux/kobject.h:21,
                    from include/linux/cdev.h:4,
                    from include/drm/drmP.h:36,
                    from drivers/gpu/drm/i915/i915_gem.c:28:
   drivers/gpu/drm/i915/i915_gem.c: At top level:
   include/linux/bug.h:45:35: error: expected identifier or '(' before 'void'
    #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
                                      ^
   drivers/gpu/drm/i915/i915_gem.h:32:26: note: in expansion of macro 'BUILD_BUG_ON_INVALID'
    #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
                             ^~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2310:2: note: in expansion of macro 'GEM_BUG_ON'
     GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */
     ^~~~~~~~~~
   include/linux/bug.h:45:40: error: expected ')' before '(' token
    #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
                                           ^
   drivers/gpu/drm/i915/i915_gem.h:32:26: note: in expansion of macro 'BUILD_BUG_ON_INVALID'
    #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
                             ^~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2310:2: note: in expansion of macro 'GEM_BUG_ON'
     GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */
     ^~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2312:2: error: data definition has no type or storage class [-Werror]
     sg_free_table(orig_st);
     ^~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2312:2: error: type defaults to 'int' in declaration of 'sg_free_table' [-Werror=implicit-int]
   drivers/gpu/drm/i915/i915_gem.c:2312:2: error: parameter names (without types) in function declaration [-Werror]
   drivers/gpu/drm/i915/i915_gem.c:2312:2: error: conflicting types for 'sg_free_table'
   In file included from include/linux/dma-mapping.h:10:0,
                    from include/drm/drmP.h:37,
                    from drivers/gpu/drm/i915/i915_gem.c:28:
   include/linux/scatterlist.h:260:6: note: previous declaration of 'sg_free_table' was here
    void sg_free_table(struct sg_table *);
         ^~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2314:2: error: data definition has no type or storage class [-Werror]
     *orig_st = new_st;
     ^
   drivers/gpu/drm/i915/i915_gem.c:2314:3: error: type defaults to 'int' in declaration of 'orig_st' [-Werror=implicit-int]
     *orig_st = new_st;
      ^~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2314:13: error: 'new_st' undeclared here (not in a function)
     *orig_st = new_st;
                ^~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2315:1: error: expected identifier or '(' before '}' token
    }
    ^
   cc1: all warnings being treated as errors

vim +2307 drivers/gpu/drm/i915/i915_gem.c

871dfbd67 Chris Wilson   2016-10-11  2288  #endif
871dfbd67 Chris Wilson   2016-10-11  2289  }
871dfbd67 Chris Wilson   2016-10-11  2290  
0c40ce130 Tvrtko Ursulin 2016-11-09  2291  static void i915_sg_trim(struct sg_table *orig_st)
0c40ce130 Tvrtko Ursulin 2016-11-09  2292  {
0c40ce130 Tvrtko Ursulin 2016-11-09  2293  	struct sg_table new_st;
0c40ce130 Tvrtko Ursulin 2016-11-09 @2294  	struct scatterlist *sg, *new_sg;
0c40ce130 Tvrtko Ursulin 2016-11-09 @2295  	unsigned int i;
0c40ce130 Tvrtko Ursulin 2016-11-09  2296  
0c40ce130 Tvrtko Ursulin 2016-11-09  2297  	if (orig_st->nents == orig_st->orig_nents)
0c40ce130 Tvrtko Ursulin 2016-11-09  2298  		return;
0c40ce130 Tvrtko Ursulin 2016-11-09  2299  
8bfc478fa Chris Wilson   2016-12-23  2300  	if (sg_alloc_table(&new_st, orig_st->nents, GFP_KERNEL | __GFP_NOWARN))
0c40ce130 Tvrtko Ursulin 2016-11-09  2301  		return;
0c40ce130 Tvrtko Ursulin 2016-11-09  2302  
ccbf455f4 Chris Wilson   2017-01-09 @2303  	new_st->orig_nents = orig_st->orig_nents; /* XXX lies for 
0c40ce130 Tvrtko Ursulin 2016-11-09  2304  	new_sg = new_st.sgl;
0c40ce130 Tvrtko Ursulin 2016-11-09  2305  	for_each_sg(orig_st->sgl, sg, orig_st->nents, i) {
0c40ce130 Tvrtko Ursulin 2016-11-09  2306  		sg_set_page(new_sg, sg_page(sg), sg->length, 0);
0c40ce130 Tvrtko Ursulin 2016-11-09 @2307  		/* called before being DMA mapped, no need to copy sg->dma_* */
0c40ce130 Tvrtko Ursulin 2016-11-09  2308  		new_sg = sg_next(new_sg);
0c40ce130 Tvrtko Ursulin 2016-11-09  2309  	}
c2dc6cc94 Chris Wilson   2016-12-19  2310  	GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */

:::::: The code at line 2307 was first introduced by commit
:::::: 0c40ce130e38aeb9ddcee3ddcffbe5a79f27c080 drm/i915: Trim the object sg table

:::::: TO: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
:::::: CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27130 bytes --]

Re: [PATCH] drm/i915: Flush untouched framebuffers before display on !llc

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6555 bytes --]

Hi Chris,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.10-rc3 next-20170106]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Flush-untouched-framebuffers-before-display-on-llc/20170109-190816
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s3-01092001 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_gem.c: In function 'i915_sg_trim':
   drivers/gpu/drm/i915/i915_gem.c:2303:8: error: invalid type argument of '->' (have 'struct sg_table')
     new_st->orig_nents = orig_st->orig_nents; /* XXX lies for
           ^~
>> drivers/gpu/drm/i915/i915_gem.c:2307:3: error: "/*" within comment [-Werror=comment]
      /* called before being DMA mapped, no need to copy sg->dma_* */
       
>> drivers/gpu/drm/i915/i915_gem.c:2295:15: error: unused variable 'i' [-Werror=unused-variable]
     unsigned int i;
                  ^
>> drivers/gpu/drm/i915/i915_gem.c:2294:22: error: unused variable 'sg' [-Werror=unused-variable]
     struct scatterlist *sg, *new_sg;
                         ^~
   In file included from include/linux/debug_locks.h:6:0,
                    from include/linux/lockdep.h:25,
                    from include/linux/spinlock_types.h:18,
                    from include/linux/mutex.h:15,
                    from include/linux/kernfs.h:13,
                    from include/linux/sysfs.h:15,
                    from include/linux/kobject.h:21,
                    from include/linux/cdev.h:4,
                    from include/drm/drmP.h:36,
                    from drivers/gpu/drm/i915/i915_gem.c:28:
   drivers/gpu/drm/i915/i915_gem.c: At top level:
   include/linux/bug.h:45:35: error: expected identifier or '(' before 'void'
    #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
                                      ^
   drivers/gpu/drm/i915/i915_gem.h:32:26: note: in expansion of macro 'BUILD_BUG_ON_INVALID'
    #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
                             ^~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2310:2: note: in expansion of macro 'GEM_BUG_ON'
     GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */
     ^~~~~~~~~~
   include/linux/bug.h:45:40: error: expected ')' before '(' token
    #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
                                           ^
   drivers/gpu/drm/i915/i915_gem.h:32:26: note: in expansion of macro 'BUILD_BUG_ON_INVALID'
    #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
                             ^~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2310:2: note: in expansion of macro 'GEM_BUG_ON'
     GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */
     ^~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2312:2: error: data definition has no type or storage class [-Werror]
     sg_free_table(orig_st);
     ^~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2312:2: error: type defaults to 'int' in declaration of 'sg_free_table' [-Werror=implicit-int]
   drivers/gpu/drm/i915/i915_gem.c:2312:2: error: parameter names (without types) in function declaration [-Werror]
   drivers/gpu/drm/i915/i915_gem.c:2312:2: error: conflicting types for 'sg_free_table'
   In file included from include/linux/dma-mapping.h:10:0,
                    from include/drm/drmP.h:37,
                    from drivers/gpu/drm/i915/i915_gem.c:28:
   include/linux/scatterlist.h:260:6: note: previous declaration of 'sg_free_table' was here
    void sg_free_table(struct sg_table *);
         ^~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2314:2: error: data definition has no type or storage class [-Werror]
     *orig_st = new_st;
     ^
   drivers/gpu/drm/i915/i915_gem.c:2314:3: error: type defaults to 'int' in declaration of 'orig_st' [-Werror=implicit-int]
     *orig_st = new_st;
      ^~~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2314:13: error: 'new_st' undeclared here (not in a function)
     *orig_st = new_st;
                ^~~~~~
   drivers/gpu/drm/i915/i915_gem.c:2315:1: error: expected identifier or '(' before '}' token
    }
    ^
   cc1: all warnings being treated as errors

vim +2307 drivers/gpu/drm/i915/i915_gem.c

871dfbd67 Chris Wilson   2016-10-11  2288  #endif
871dfbd67 Chris Wilson   2016-10-11  2289  }
871dfbd67 Chris Wilson   2016-10-11  2290  
0c40ce130 Tvrtko Ursulin 2016-11-09  2291  static void i915_sg_trim(struct sg_table *orig_st)
0c40ce130 Tvrtko Ursulin 2016-11-09  2292  {
0c40ce130 Tvrtko Ursulin 2016-11-09  2293  	struct sg_table new_st;
0c40ce130 Tvrtko Ursulin 2016-11-09 @2294  	struct scatterlist *sg, *new_sg;
0c40ce130 Tvrtko Ursulin 2016-11-09 @2295  	unsigned int i;
0c40ce130 Tvrtko Ursulin 2016-11-09  2296  
0c40ce130 Tvrtko Ursulin 2016-11-09  2297  	if (orig_st->nents == orig_st->orig_nents)
0c40ce130 Tvrtko Ursulin 2016-11-09  2298  		return;
0c40ce130 Tvrtko Ursulin 2016-11-09  2299  
8bfc478fa Chris Wilson   2016-12-23  2300  	if (sg_alloc_table(&new_st, orig_st->nents, GFP_KERNEL | __GFP_NOWARN))
0c40ce130 Tvrtko Ursulin 2016-11-09  2301  		return;
0c40ce130 Tvrtko Ursulin 2016-11-09  2302  
ccbf455f4 Chris Wilson   2017-01-09 @2303  	new_st->orig_nents = orig_st->orig_nents; /* XXX lies for 
0c40ce130 Tvrtko Ursulin 2016-11-09  2304  	new_sg = new_st.sgl;
0c40ce130 Tvrtko Ursulin 2016-11-09  2305  	for_each_sg(orig_st->sgl, sg, orig_st->nents, i) {
0c40ce130 Tvrtko Ursulin 2016-11-09  2306  		sg_set_page(new_sg, sg_page(sg), sg->length, 0);
0c40ce130 Tvrtko Ursulin 2016-11-09 @2307  		/* called before being DMA mapped, no need to copy sg->dma_* */
0c40ce130 Tvrtko Ursulin 2016-11-09  2308  		new_sg = sg_next(new_sg);
0c40ce130 Tvrtko Ursulin 2016-11-09  2309  	}
c2dc6cc94 Chris Wilson   2016-12-19  2310  	GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */

:::::: The code at line 2307 was first introduced by commit
:::::: 0c40ce130e38aeb9ddcee3ddcffbe5a79f27c080 drm/i915: Trim the object sg table

:::::: TO: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
:::::: CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27130 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On Mon, Jan 09, 2017 at 11:19:32AM +0000, Chris Wilson wrote:
> On a non-llc system, the objects are created with .cache_level =
> CACHE_NONE and so the transition to uncached for scanout is a no-op.
> However, if the object was never written to, it will still be in the CPU
> domain (having been zeroed out by shmemfs). Those cachelines need to be
> flushed prior to display.
> 
> Reported-by: Vito Caputo
> Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+

Ping?

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 76689b59fc90..bdb113ef8cfe 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3514,7 +3514,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>  
>  	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> -	if (obj->cache_dirty) {
> +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
>  		i915_gem_clflush_object(obj, true);
>  		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
>  	}
> -- 
> 2.11.0

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On Mon, Jan 09, 2017 at 11:19:32AM +0000, Chris Wilson wrote:
> On a non-llc system, the objects are created with .cache_level =
> CACHE_NONE and so the transition to uncached for scanout is a no-op.
> However, if the object was never written to, it will still be in the CPU
> domain (having been zeroed out by shmemfs). Those cachelines need to be
> flushed prior to display.
> 
> Reported-by: Vito Caputo
> Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+

Ping?

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 76689b59fc90..bdb113ef8cfe 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3514,7 +3514,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>  
>  	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> -	if (obj->cache_dirty) {
> +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
>  		i915_gem_clflush_object(obj, true);
>  		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
>  	}
> -- 
> 2.11.0

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Flush untouched framebuffers before display on !llc

[lkml]

On Thu, Jan 12, 2017 at 09:17:06PM +0000, Chris Wilson wrote:
> On Mon, Jan 09, 2017 at 11:19:32AM +0000, Chris Wilson wrote:
> > On a non-llc system, the objects are created with .cache_level =
> > CACHE_NONE and so the transition to uncached for scanout is a no-op.
> > However, if the object was never written to, it will still be in the CPU
> > domain (having been zeroed out by shmemfs). Those cachelines need to be
> > flushed prior to display.
> > 
> > Reported-by: Vito Caputo
> > Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> 
> Ping?

This patch fixes the problem for me, in case that's what the ping's for.

Out of curiosity the bug I reported described here be getting fixed in 4.10?
https://lists.freedesktop.org/archives/dri-devel/2017-January/128405.html

Thanks.

> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 76689b59fc90..bdb113ef8cfe 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3514,7 +3514,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
> >  
> >  	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> > -	if (obj->cache_dirty) {
> > +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
> >  		i915_gem_clflush_object(obj, true);
> >  		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
> >  	}
> > -- 
> > 2.11.0
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On Thu, Jan 12, 2017 at 04:24:50PM -0600, lkml@pengaru.com wrote:
> On Thu, Jan 12, 2017 at 09:17:06PM +0000, Chris Wilson wrote:
> > On Mon, Jan 09, 2017 at 11:19:32AM +0000, Chris Wilson wrote:
> > > On a non-llc system, the objects are created with .cache_level =
> > > CACHE_NONE and so the transition to uncached for scanout is a no-op.
> > > However, if the object was never written to, it will still be in the CPU
> > > domain (having been zeroed out by shmemfs). Those cachelines need to be
> > > flushed prior to display.
> > > 
> > > Reported-by: Vito Caputo
> > > Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> > 
> > Ping?
> 
> This patch fixes the problem for me, in case that's what the ping's for.

Partly that, and trying to catch CI + reviewers.
 
> Out of curiosity the bug I reported described here be getting fixed in 4.10?
> https://lists.freedesktop.org/archives/dri-devel/2017-January/128405.html

It was fixed in the tree back in December, bit of a muddle to get that
particular patch into 4.10.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On Thu, Jan 12, 2017 at 04:24:50PM -0600, lkml@pengaru.com wrote:
> On Thu, Jan 12, 2017 at 09:17:06PM +0000, Chris Wilson wrote:
> > On Mon, Jan 09, 2017 at 11:19:32AM +0000, Chris Wilson wrote:
> > > On a non-llc system, the objects are created with .cache_level =
> > > CACHE_NONE and so the transition to uncached for scanout is a no-op.
> > > However, if the object was never written to, it will still be in the CPU
> > > domain (having been zeroed out by shmemfs). Those cachelines need to be
> > > flushed prior to display.
> > > 
> > > Reported-by: Vito Caputo
> > > Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> > 
> > Ping?
> 
> This patch fixes the problem for me, in case that's what the ping's for.

Partly that, and trying to catch CI + reviewers.
 
> Out of curiosity the bug I reported described here be getting fixed in 4.10?
> https://lists.freedesktop.org/archives/dri-devel/2017-January/128405.html

It was fixed in the tree back in December, bit of a muddle to get that
particular patch into 4.10.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: BUG: 4.10 i915 drm display noise regression - bisected to a6a7cc4b7

[lkml]

On Mon, Jan 09, 2017 at 12:32:40AM -0600, lkml@pengaru.com wrote:
> Hello all,
> 
> I'm experiencing display noise in the form of 8x1 pixel bars spuriously
> appearing in random locations.  This doesn't happen on 4.9, the machine
> is an X61s, a Core2Duo 1.8Ghz w/XGA via LVDS.
> 
> I was able to bisect the issue to a6a7cc4b7:
> 
> commit a6a7cc4b7db6deaeca11cdd38844ea147a354c7a
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Nov 18 21:17:46 2016 +0000
> 
>     drm/i915: Always flush the dirty CPU cache when pinning the scanout
>     
>     Currently we only clflush the scanout if it is in the CPU domain. Also
>     flush if we have a pending CPU clflush. We also want to treat the
>     dirtyfb path similar, and flush any pending writes there as well.
>     
>     v2: Only send the fb flush message if flushing the dirt on flip
>     v3: Make flush-for-flip and dirtyfb look more alike since they serve
>     similar roles as end-of-frame marker.
> 
> Reproduction is simple, just run this native drm eye candy program:
> https://github.com/vcaputo/rototiller
> 

This regression still remains as of 4.10.0-rc6.

Chris Wilson had posted a fix:
https://www.spinics.net/lists/kernel/msg2420777.html

But it seems to have been ignored so far.  How do we get this fixed in
4.10 before it ships?

Regards,
Vito Caputo

Re: [Intel-gfx] [PATCH v2] drm/i915: Flush untouched framebuffers before display on !llc

[Daniel Vetter]

On Mon, Jan 09, 2017 at 11:19:32AM +0000, Chris Wilson wrote:
> On a non-llc system, the objects are created with .cache_level =
> CACHE_NONE and so the transition to uncached for scanout is a no-op.
> However, if the object was never written to, it will still be in the CPU
> domain (having been zeroed out by shmemfs). Those cachelines need to be
> flushed prior to display.
> 
> Reported-by: Vito Caputo
> Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 76689b59fc90..bdb113ef8cfe 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3514,7 +3514,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>  
>  	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> -	if (obj->cache_dirty) {
> +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {

Alternatively, should we set cache_dirty when initially allocating an
object? Ofc only if cpu_cache_is_coherent, like we do in other places.

Anyway, up to you which one you like more I'd say, this one looks correct
too.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  		i915_gem_clflush_object(obj, true);
>  		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
>  	}
> -- 
> 2.11.0
> 
> _______________________________________________
> 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 v2] drm/i915: Flush untouched framebuffers before display on !llc

[Daniel Vetter]

On Mon, Jan 09, 2017 at 11:19:32AM +0000, Chris Wilson wrote:
> On a non-llc system, the objects are created with .cache_level =
> CACHE_NONE and so the transition to uncached for scanout is a no-op.
> However, if the object was never written to, it will still be in the CPU
> domain (having been zeroed out by shmemfs). Those cachelines need to be
> flushed prior to display.
> 
> Reported-by: Vito Caputo
> Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 76689b59fc90..bdb113ef8cfe 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3514,7 +3514,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>  
>  	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> -	if (obj->cache_dirty) {
> +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {

Alternatively, should we set cache_dirty when initially allocating an
object? Ofc only if cpu_cache_is_coherent, like we do in other places.

Anyway, up to you which one you like more I'd say, this one looks correct
too.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  		i915_gem_clflush_object(obj, true);
>  		intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
>  	}
> -- 
> 2.11.0
> 
> _______________________________________________
> 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 v2] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On Wed, Feb 01, 2017 at 11:24:32AM +0100, Daniel Vetter wrote:
> On Mon, Jan 09, 2017 at 11:19:32AM +0000, Chris Wilson wrote:
> > On a non-llc system, the objects are created with .cache_level =
> > CACHE_NONE and so the transition to uncached for scanout is a no-op.
> > However, if the object was never written to, it will still be in the CPU
> > domain (having been zeroed out by shmemfs). Those cachelines need to be
> > flushed prior to display.
> > 
> > Reported-by: Vito Caputo
> > Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 76689b59fc90..bdb113ef8cfe 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3514,7 +3514,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
> >  
> >  	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> > -	if (obj->cache_dirty) {
> > +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
> 
> Alternatively, should we set cache_dirty when initially allocating an
> object? Ofc only if cpu_cache_is_coherent, like we do in other places.

I thought about it and didn't come to any firm conclusion. Currently
"cache_dirty" means omission of clflush, and that's been a source of
confusion ever since. I've a patch/plan to do async clflushing which
similarly impacts upon the meaning of obj->cache_dirty and interation
with frontbuffer tracking, so that seems a reasonable point to which to
defer further thought.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2] drm/i915: Flush untouched framebuffers before display on !llc

[Chris Wilson]

On Wed, Feb 01, 2017 at 11:24:32AM +0100, Daniel Vetter wrote:
> On Mon, Jan 09, 2017 at 11:19:32AM +0000, Chris Wilson wrote:
> > On a non-llc system, the objects are created with .cache_level =
> > CACHE_NONE and so the transition to uncached for scanout is a no-op.
> > However, if the object was never written to, it will still be in the CPU
> > domain (having been zeroed out by shmemfs). Those cachelines need to be
> > flushed prior to display.
> > 
> > Reported-by: Vito Caputo
> > Fixes: a6a7cc4b7db6 ("drm/i915: Always flush the dirty CPU cache when pinning the scanout")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 76689b59fc90..bdb113ef8cfe 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3514,7 +3514,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
> >  
> >  	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> > -	if (obj->cache_dirty) {
> > +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
> 
> Alternatively, should we set cache_dirty when initially allocating an
> object? Ofc only if cpu_cache_is_coherent, like we do in other places.

I thought about it and didn't come to any firm conclusion. Currently
"cache_dirty" means omission of clflush, and that's been a source of
confusion ever since. I've a patch/plan to do async clflushing which
similarly impacts upon the meaning of obj->cache_dirty and interation
with frontbuffer tracking, so that seems a reasonable point to which to
defer further thought.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx