sna: Partly revert "sna: Compilation fixes for stable distros"
diff mbox

Message ID 20150605145122.GU27056@nuc-i3427.alporthouse.com
State New
Headers show

Commit Message

Chris Wilson June 5, 2015, 2:51 p.m. UTC
On Fri, Jun 05, 2015 at 04:39:32PM +0200, Sedat Dilek wrote:
> On Fri, Jun 5, 2015 at 4:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Fri, Jun 05, 2015 at 04:23:36PM +0200, Sedat Dilek wrote:
> >> Fixes a problem having no mouse cursor in the LightDM login-screen
> >> on Ubuntu/precise (see [1]).
> >>
> >> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-June/068096.html
> >>
> >> Fixes: 7d30ccea214b ("sna: Compilation fixes for stable distros")
> >> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
> >> ---
> >>  src/sna/kgem.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/src/sna/kgem.c b/src/sna/kgem.c
> >> index de604b0f3d77..720644b26750 100644
> >> --- a/src/sna/kgem.c
> >> +++ b/src/sna/kgem.c
> >> @@ -140,7 +140,6 @@ search_snoop_cache(struct kgem *kgem, unsigned int num_pages, unsigned flags);
> >>  #define LOCAL_I915_PARAM_HAS_WT                      27
> >>  #define LOCAL_I915_PARAM_MMAP_VERSION                30
> >>
> >> -#define LOCAL_I915_EXEC_BLT                  (2<<0)
> >>  #define LOCAL_I915_EXEC_IS_PINNED            (1<<10)
> >>  #define LOCAL_I915_EXEC_NO_RELOC             (1<<11)
> >>  #define LOCAL_I915_EXEC_HANDLE_LUT           (1<<12)
> >> @@ -1400,7 +1399,7 @@ static bool test_can_blt_y(struct kgem *kgem)
> >>               memset(&execbuf, 0, sizeof(execbuf));
> >>               execbuf.buffers_ptr = (uintptr_t)&object;
> >>               execbuf.buffer_count = 1;
> >> -             execbuf.flags = LOCAL_I915_EXEC_BLT;
> >> +             execbuf.flags = I915_EXEC_BLT;
> >
> > Again, I am confused. The effect is instead of executing on BLT we ended
> > up on BSD. Which is fine for the purposes of this test and the test did
> > succeed.
> >
> > What's the result of this function after your patch? It should still be
> > true. So I think we are looking at a funky underlying bug here...
> >
> 
> How can I test that or check?

Comments

Sedat Dilek June 5, 2015, 2:53 p.m. UTC | #1
On Fri, Jun 5, 2015 at 4:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Jun 05, 2015 at 04:39:32PM +0200, Sedat Dilek wrote:
>> On Fri, Jun 5, 2015 at 4:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Fri, Jun 05, 2015 at 04:23:36PM +0200, Sedat Dilek wrote:
>> >> Fixes a problem having no mouse cursor in the LightDM login-screen
>> >> on Ubuntu/precise (see [1]).
>> >>
>> >> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-June/068096.html
>> >>
>> >> Fixes: 7d30ccea214b ("sna: Compilation fixes for stable distros")
>> >> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
>> >> ---
>> >>  src/sna/kgem.c | 3 +--
>> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >>
>> >> diff --git a/src/sna/kgem.c b/src/sna/kgem.c
>> >> index de604b0f3d77..720644b26750 100644
>> >> --- a/src/sna/kgem.c
>> >> +++ b/src/sna/kgem.c
>> >> @@ -140,7 +140,6 @@ search_snoop_cache(struct kgem *kgem, unsigned int num_pages, unsigned flags);
>> >>  #define LOCAL_I915_PARAM_HAS_WT                      27
>> >>  #define LOCAL_I915_PARAM_MMAP_VERSION                30
>> >>
>> >> -#define LOCAL_I915_EXEC_BLT                  (2<<0)
>> >>  #define LOCAL_I915_EXEC_IS_PINNED            (1<<10)
>> >>  #define LOCAL_I915_EXEC_NO_RELOC             (1<<11)
>> >>  #define LOCAL_I915_EXEC_HANDLE_LUT           (1<<12)
>> >> @@ -1400,7 +1399,7 @@ static bool test_can_blt_y(struct kgem *kgem)
>> >>               memset(&execbuf, 0, sizeof(execbuf));
>> >>               execbuf.buffers_ptr = (uintptr_t)&object;
>> >>               execbuf.buffer_count = 1;
>> >> -             execbuf.flags = LOCAL_I915_EXEC_BLT;
>> >> +             execbuf.flags = I915_EXEC_BLT;
>> >
>> > Again, I am confused. The effect is instead of executing on BLT we ended
>> > up on BSD. Which is fine for the purposes of this test and the test did
>> > succeed.
>> >
>> > What's the result of this function after your patch? It should still be
>> > true. So I think we are looking at a funky underlying bug here...
>> >
>>
>> How can I test that or check?
>
> diff --git a/src/sna/kgem.c b/src/sna/kgem.c
> index 2058364..66f0087 100644
> --- a/src/sna/kgem.c
> +++ b/src/sna/kgem.c
> @@ -1753,6 +1753,9 @@ no_context_switch(struct kgem *kgem, int new_mode)
>         (void)new_mode;
>  }
>
> +#undef DBG
> +#define DBG(x) ErrorF x
> +
>  void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
>  {
>         struct drm_i915_gem_get_aperture aperture;
> @@ -2054,6 +2057,9 @@ void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
>         kgem_init_swizzling(kgem);
>  }
>
> +#undef DBG
> +#define DBG(x)
> +
>
> Then look for "kgem_init: can blit to Y-tiled surfaces?"
>

I will look into that later, on the move to "freibad".

- sed@ -
Sedat Dilek June 6, 2015, 8:03 a.m. UTC | #2
On Fri, Jun 5, 2015 at 4:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Jun 05, 2015 at 04:39:32PM +0200, Sedat Dilek wrote:
>> On Fri, Jun 5, 2015 at 4:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Fri, Jun 05, 2015 at 04:23:36PM +0200, Sedat Dilek wrote:
>> >> Fixes a problem having no mouse cursor in the LightDM login-screen
>> >> on Ubuntu/precise (see [1]).
>> >>
>> >> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-June/068096.html
>> >>
>> >> Fixes: 7d30ccea214b ("sna: Compilation fixes for stable distros")
>> >> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
>> >> ---
>> >>  src/sna/kgem.c | 3 +--
>> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >>
>> >> diff --git a/src/sna/kgem.c b/src/sna/kgem.c
>> >> index de604b0f3d77..720644b26750 100644
>> >> --- a/src/sna/kgem.c
>> >> +++ b/src/sna/kgem.c
>> >> @@ -140,7 +140,6 @@ search_snoop_cache(struct kgem *kgem, unsigned int num_pages, unsigned flags);
>> >>  #define LOCAL_I915_PARAM_HAS_WT                      27
>> >>  #define LOCAL_I915_PARAM_MMAP_VERSION                30
>> >>
>> >> -#define LOCAL_I915_EXEC_BLT                  (2<<0)
>> >>  #define LOCAL_I915_EXEC_IS_PINNED            (1<<10)
>> >>  #define LOCAL_I915_EXEC_NO_RELOC             (1<<11)
>> >>  #define LOCAL_I915_EXEC_HANDLE_LUT           (1<<12)
>> >> @@ -1400,7 +1399,7 @@ static bool test_can_blt_y(struct kgem *kgem)
>> >>               memset(&execbuf, 0, sizeof(execbuf));
>> >>               execbuf.buffers_ptr = (uintptr_t)&object;
>> >>               execbuf.buffer_count = 1;
>> >> -             execbuf.flags = LOCAL_I915_EXEC_BLT;
>> >> +             execbuf.flags = I915_EXEC_BLT;
>> >
>> > Again, I am confused. The effect is instead of executing on BLT we ended
>> > up on BSD. Which is fine for the purposes of this test and the test did
>> > succeed.
>> >
>> > What's the result of this function after your patch? It should still be
>> > true. So I think we are looking at a funky underlying bug here...
>> >
>>
>> How can I test that or check?
>
> diff --git a/src/sna/kgem.c b/src/sna/kgem.c
> index 2058364..66f0087 100644
> --- a/src/sna/kgem.c
> +++ b/src/sna/kgem.c
> @@ -1753,6 +1753,9 @@ no_context_switch(struct kgem *kgem, int new_mode)
>         (void)new_mode;
>  }
>
> +#undef DBG
> +#define DBG(x) ErrorF x
> +
>  void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
>  {
>         struct drm_i915_gem_get_aperture aperture;
> @@ -2054,6 +2057,9 @@ void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
>         kgem_init_swizzling(kgem);
>  }
>
> +#undef DBG
> +#define DBG(x)
> +
>
> Then look for "kgem_init: can blit to Y-tiled surfaces?"
>

Got some other tuff to do this weekend and really have less time.
If it is that important to you, please let me know, then I will test.

- sed@ -
Chris Wilson June 6, 2015, 11:33 a.m. UTC | #3
On Sat, Jun 06, 2015 at 10:03:19AM +0200, Sedat Dilek wrote:
> On Fri, Jun 5, 2015 at 4:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Fri, Jun 05, 2015 at 04:39:32PM +0200, Sedat Dilek wrote:
> >> On Fri, Jun 5, 2015 at 4:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > On Fri, Jun 05, 2015 at 04:23:36PM +0200, Sedat Dilek wrote:
> >> >> Fixes a problem having no mouse cursor in the LightDM login-screen
> >> >> on Ubuntu/precise (see [1]).
> >> >>
> >> >> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-June/068096.html
> >> >>
> >> >> Fixes: 7d30ccea214b ("sna: Compilation fixes for stable distros")
> >> >> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
> >> >> ---
> >> >>  src/sna/kgem.c | 3 +--
> >> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/src/sna/kgem.c b/src/sna/kgem.c
> >> >> index de604b0f3d77..720644b26750 100644
> >> >> --- a/src/sna/kgem.c
> >> >> +++ b/src/sna/kgem.c
> >> >> @@ -140,7 +140,6 @@ search_snoop_cache(struct kgem *kgem, unsigned int num_pages, unsigned flags);
> >> >>  #define LOCAL_I915_PARAM_HAS_WT                      27
> >> >>  #define LOCAL_I915_PARAM_MMAP_VERSION                30
> >> >>
> >> >> -#define LOCAL_I915_EXEC_BLT                  (2<<0)
> >> >>  #define LOCAL_I915_EXEC_IS_PINNED            (1<<10)
> >> >>  #define LOCAL_I915_EXEC_NO_RELOC             (1<<11)
> >> >>  #define LOCAL_I915_EXEC_HANDLE_LUT           (1<<12)
> >> >> @@ -1400,7 +1399,7 @@ static bool test_can_blt_y(struct kgem *kgem)
> >> >>               memset(&execbuf, 0, sizeof(execbuf));
> >> >>               execbuf.buffers_ptr = (uintptr_t)&object;
> >> >>               execbuf.buffer_count = 1;
> >> >> -             execbuf.flags = LOCAL_I915_EXEC_BLT;
> >> >> +             execbuf.flags = I915_EXEC_BLT;
> >> >
> >> > Again, I am confused. The effect is instead of executing on BLT we ended
> >> > up on BSD. Which is fine for the purposes of this test and the test did
> >> > succeed.
> >> >
> >> > What's the result of this function after your patch? It should still be
> >> > true. So I think we are looking at a funky underlying bug here...
> >> >
> >>
> >> How can I test that or check?
> >
> > diff --git a/src/sna/kgem.c b/src/sna/kgem.c
> > index 2058364..66f0087 100644
> > --- a/src/sna/kgem.c
> > +++ b/src/sna/kgem.c
> > @@ -1753,6 +1753,9 @@ no_context_switch(struct kgem *kgem, int new_mode)
> >         (void)new_mode;
> >  }
> >
> > +#undef DBG
> > +#define DBG(x) ErrorF x
> > +
> >  void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
> >  {
> >         struct drm_i915_gem_get_aperture aperture;
> > @@ -2054,6 +2057,9 @@ void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
> >         kgem_init_swizzling(kgem);
> >  }
> >
> > +#undef DBG
> > +#define DBG(x)
> > +
> >
> > Then look for "kgem_init: can blit to Y-tiled surfaces?"
> >
> 
> Got some other tuff to do this weekend and really have less time.
> If it is that important to you, please let me know, then I will test.

It's not urgent. I would like to work out how executing the LRI on the
BSD ring ended up loosing the cursor plane, on the first lightdm, but
since you found the fix, it is more curiosity than anything else.
-Chris
Sedat Dilek June 6, 2015, 1:58 p.m. UTC | #4
On Sat, Jun 6, 2015 at 1:33 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, Jun 06, 2015 at 10:03:19AM +0200, Sedat Dilek wrote:
>> On Fri, Jun 5, 2015 at 4:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Fri, Jun 05, 2015 at 04:39:32PM +0200, Sedat Dilek wrote:
>> >> On Fri, Jun 5, 2015 at 4:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> >> > On Fri, Jun 05, 2015 at 04:23:36PM +0200, Sedat Dilek wrote:
>> >> >> Fixes a problem having no mouse cursor in the LightDM login-screen
>> >> >> on Ubuntu/precise (see [1]).
>> >> >>
>> >> >> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-June/068096.html
>> >> >>
>> >> >> Fixes: 7d30ccea214b ("sna: Compilation fixes for stable distros")
>> >> >> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
>> >> >> ---
>> >> >>  src/sna/kgem.c | 3 +--
>> >> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >> >>
>> >> >> diff --git a/src/sna/kgem.c b/src/sna/kgem.c
>> >> >> index de604b0f3d77..720644b26750 100644
>> >> >> --- a/src/sna/kgem.c
>> >> >> +++ b/src/sna/kgem.c
>> >> >> @@ -140,7 +140,6 @@ search_snoop_cache(struct kgem *kgem, unsigned int num_pages, unsigned flags);
>> >> >>  #define LOCAL_I915_PARAM_HAS_WT                      27
>> >> >>  #define LOCAL_I915_PARAM_MMAP_VERSION                30
>> >> >>
>> >> >> -#define LOCAL_I915_EXEC_BLT                  (2<<0)
>> >> >>  #define LOCAL_I915_EXEC_IS_PINNED            (1<<10)
>> >> >>  #define LOCAL_I915_EXEC_NO_RELOC             (1<<11)
>> >> >>  #define LOCAL_I915_EXEC_HANDLE_LUT           (1<<12)
>> >> >> @@ -1400,7 +1399,7 @@ static bool test_can_blt_y(struct kgem *kgem)
>> >> >>               memset(&execbuf, 0, sizeof(execbuf));
>> >> >>               execbuf.buffers_ptr = (uintptr_t)&object;
>> >> >>               execbuf.buffer_count = 1;
>> >> >> -             execbuf.flags = LOCAL_I915_EXEC_BLT;
>> >> >> +             execbuf.flags = I915_EXEC_BLT;
>> >> >
>> >> > Again, I am confused. The effect is instead of executing on BLT we ended
>> >> > up on BSD. Which is fine for the purposes of this test and the test did
>> >> > succeed.
>> >> >
>> >> > What's the result of this function after your patch? It should still be
>> >> > true. So I think we are looking at a funky underlying bug here...
>> >> >
>> >>
>> >> How can I test that or check?
>> >
>> > diff --git a/src/sna/kgem.c b/src/sna/kgem.c
>> > index 2058364..66f0087 100644
>> > --- a/src/sna/kgem.c
>> > +++ b/src/sna/kgem.c
>> > @@ -1753,6 +1753,9 @@ no_context_switch(struct kgem *kgem, int new_mode)
>> >         (void)new_mode;
>> >  }
>> >
>> > +#undef DBG
>> > +#define DBG(x) ErrorF x
>> > +
>> >  void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
>> >  {
>> >         struct drm_i915_gem_get_aperture aperture;
>> > @@ -2054,6 +2057,9 @@ void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
>> >         kgem_init_swizzling(kgem);
>> >  }
>> >
>> > +#undef DBG
>> > +#define DBG(x)
>> > +
>> >
>> > Then look for "kgem_init: can blit to Y-tiled surfaces?"
>> >
>>
>> Got some other tuff to do this weekend and really have less time.
>> If it is that important to you, please let me know, then I will test.
>
> It's not urgent. I would like to work out how executing the LRI on the
> BSD ring ended up loosing the cursor plane, on the first lightdm, but
> since you found the fix, it is more curiosity than anything else.
>

[ for-ickle /o\ ]

I have tested against "BROKEN" intelddx-2.99.917-338-g7d30ccea214b
with the following 2 patches on top...

$ git log --oneline -4
a41601caccf4 Merge branch
'for-2.99.917-338/kgem_init-testing-BLT-with-y-tiling' into
for-2.99.917-338/sna-kgem_init-testing-for-ickle
f4838bbe9076 sna: Partly revert "sna: Compilation fixes for stable distros"
5114c67c1998 sna: kgem_init: can blit to Y-tiled surfaces?
7d30ccea214b sna: Ensure compat_output is sane after sorting outputs

The output for kgem_init in X log looks like this...

$ zgrep kgem_init Xorg.0.log_drm-debug-7_log_buf_len-4M_kgem_init.gz
[    29.511] kgem_init: fd=9, gen=48
[    29.511] kgem_init: has BLT ring? 1
[    29.511] kgem_init: has relaxed delta? 1
[    29.511] kgem_init: has relaxed fencing? 1
[    29.511] kgem_init: has shared last-level-cache? 1
[    29.511] kgem_init: has write-through caching for scanouts? 0
[    29.511] kgem_init: has wc-mmapping? 1
[    29.512] kgem_init: has set-cache-level? 1
[    29.512] kgem_init: has userptr? 1
[    29.512] kgem_init: has create2? 0
[    29.512] kgem_init: has no-reloc? 1
[    29.512] kgem_init: has handle-lut? 1
[    29.512] kgem_init: semaphores enabled? 1
[    29.512] kgem_init: can blt to cpu? 1
[    29.513] kgem_init: can blit to Y-tiled surfaces? 1
[    29.513] kgem_init: can render to Y-tiled surfaces? 1
[    29.513] kgem_init: can scanout Y-tiled surfaces? 0
[    29.513] kgem_init: can use privileged batchbuffers? 1
[    29.513] kgem_init: can use pinned batchbuffers (to avoid CS w/a)? 1
[    29.513] kgem_init_pinned_batches: new handle=1, num_pages=1
[    29.513] kgem_init: maximum batch size? 65528
[    29.513] kgem_init: last-level cache size: 3145728 bytes,
threshold in pages: 384
[    29.514] kgem_init: cpu bo enabled 1: llc? 1, set-cache-level? 1, userptr? 1
[    29.514] kgem_init: aperture size 2147483648, available now 2142838784
[    29.514] kgem_init: aperture low=715827882 [682], high=1610612736 [1536]
[    29.514] kgem_init: aperture mappable=268435456 [256 MiB]
[    29.514] kgem_init: aperture fenceable=268435456 [256 MiB]
[    29.514] kgem_init: buffer size=262144 [256 KiB]
[    29.514] kgem_init: total ram=4014354432
[    29.514] kgem_init: maximum object size=1207959552
[    29.514] kgem_init: large object thresold=268435456
[    29.514] kgem_init: max object sizes (gpu=1003588608,
cpu=1207959552, tile upload=134217728, copy=134217728)
[    29.514] kgem_init: max fences=14
[    29.514] kgem_init_swizzling: swizzle_mode=0, phys_swizzle_mode=0
[    29.514] kgem_init_swizzling: can fence?=1

To answer your question...

$ zgrep kgem_init Xorg.0.log_drm-debug-7_log_buf_len-4M_kgem_init.gz |
zgrep 'Y-tiled surfaces'
[    29.513] kgem_init: can blit to Y-tiled surfaces? 1 <--- Look at this one.
[    29.513] kgem_init: can render to Y-tiled surfaces? 1
[    29.513] kgem_init: can scanout Y-tiled surfaces? 0

Full dmesg and X logs attached.

- sed@ -

P.S.: Booted Linux v4.1-rc6 with '... drm.debug=7 log_buf_len=4M" and
intel-ddx was compiled with "--enable-debug=full".

Patch
diff mbox

diff --git a/src/sna/kgem.c b/src/sna/kgem.c
index 2058364..66f0087 100644
--- a/src/sna/kgem.c
+++ b/src/sna/kgem.c
@@ -1753,6 +1753,9 @@  no_context_switch(struct kgem *kgem, int new_mode)
        (void)new_mode;
 }
 
+#undef DBG
+#define DBG(x) ErrorF x
+
 void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
 {
        struct drm_i915_gem_get_aperture aperture;
@@ -2054,6 +2057,9 @@  void kgem_init(struct kgem *kgem, int fd, struct pci_device *dev, unsigned gen)
        kgem_init_swizzling(kgem);
 }
 
+#undef DBG
+#define DBG(x)
+

Then look for "kgem_init: can blit to Y-tiled surfaces?"
-Chris