All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
@ 2010-11-16 17:05 Sedat Dilek
  2010-11-16 17:30 ` Daniel Vetter
  0 siblings, 1 reply; 11+ messages in thread
From: Sedat Dilek @ 2010-11-16 17:05 UTC (permalink / raw)
  To: DRI; +Cc: daniel.vetter, Arnd Bergmann

[ CCing Arnd Bergmann ]

Hi,

I have tested both patchsets from Daniel (see [1] and [2]) again on a
Radeon RV250 in a none-BKL-config and it looks like

       agd5f-pflip/0002-drm-radeon-kms-add-pageflip-ioctl-support.patch

is the culprit in combination with the below listed drm patches.

I have switched to a "normal" DDX (w/o pageflip-support).

The OpenArena benchmarks is now between 17.5 - 18.6 [fps] @1024x768 resolution.
( w/o below patchset: 840 frames 50.6 seconds 16.6 fps 12.0/60.2/267.0/21.3 ms )


Kind Regards,
- Sedat -

[1] http://lists.freedesktop.org/archives/dri-devel/2010-November/005420.html
[2] http://lists.freedesktop.org/archives/dri-devel/2010-November/005441.html


$ cd $HOME/src/linux-2.6/linux-2.6.37-rc2/debian/build/source_i386_none/

$ cat .pc/applied-patches
danvet-drm-for-sedat-dilek/0001-drm-nouveau-don-t-munge-in-drm_mm-internals.patch
danvet-drm-for-sedat-dilek/0002-drm_mm-add-support-for-range-restricted-fair-lru-sca.patch
danvet-drm-for-sedat-dilek/0003-drm-mm-track-free-areas-implicitly.patch
danvet-drm-for-sedat-dilek/0004-drm-mm-extract-node-insert-helper-functions.patch
danvet-drm-for-sedat-dilek/0005-drm-mm-add-api-for-embedding-struct-drm_mm_node.patch
danvet-drm-for-sedat-dilek/0006-drm-mm-add-helper-to-unwind-scan-state.patch
danvet-embed-drm_gem_object-into-radeon_bo/1-3-drm-radeon-embed-struct-drm_gem_object.patch
danvet-embed-drm_gem_object-into-radeon_bo/2-3-drm-radeon-introduce-gem_to_radeon_bo-helper.patch
danvet-embed-drm_gem_object-into-radeon_bo/3-3-drm-radeon-kill-radeon_bo--gobj-pointer.patch
drm-vblank-timestamping/0001-drm-vblank-Add-support-for-precise-vblank-timestampi.patch
drm-vblank-timestamping/0002-drm-radeon-Add-support-for-precise-vblank-timestampi.patch
for-drm-radeon-testing/drm-radeon-kms-enable-writeback-on-radeon-AGP-boards.patch

$ cd $HOME/src/mesa/
$ ./scripts/run_openarena-benchmark.sh
840 frames 46.5 seconds 18.1 fps 10.0/55.3/147.0/18.9 ms

$ grep OK LATEST_linux-2.6/logs/setup_linux-2.6_git0.sd.1.log
  (+) OK   bkl-config/0002-drm-i810-remove-the-BKL.patch
  (+) OK   bkl-config/0003-staging-stradis-mark-as-depends-on-BKL.patch
  (+) OK   bkl-config/0004-BKL-remove-extraneous-include-smp_lock.h.patch
  (+) OK   bkl-config/0005-BKL-remove-references-to-lock_kernel-from-comments.patch
  (+) OK   bkl-config/0006-BKL-disable-by-default.patch
  (+) OK   bkl-config/0007-BKL-mark-lock_kernel-as-deprecated.patch
  (+) OK   bkl-config/0008-BKL-move-CONFIG_BKL-to-staging.patch
  (+) OK   debian/version.patch
  (+) OK   debian/kernelvariables-2.6.37.patch
  (+) OK   debian/doc-build-parallel.patch
  (+) OK   bugfix/ia64/hardcode-arch-script-output.patch
  (+) OK   bugfix/mips/disable-advansys.patch
  (+) OK   bugfix/arm/disable-scsi_acard.patch
  (+) OK   debian/mips-disable-werror.patch
  (+) OK   bugfix/powerpc/lpar-console.patch
  (+) OK   features/all/i915-autoload-without-CONFIG_DRM_I915_KMS.patch
  (+) OK   debian/arch-sh4-fix-uimage-build.patch
  (+) OK   bugfix/mips/mips-ide-flush-dcache.patch
  (+) OK   bugfix/all/qla4xxx-Fix-build-on-some-architectures-lacking-64-bit-I-O.patch
  (+) OK   bugfix/x86/Skip-looking-for-ioapic-overrides-when-ioapics-are-not-present.patch

( NOTE: bkl-config/0001-preempt-fix-kernel-build-with-CONFIG_BKL.patch
is already in Linux 2.6.37-rc2 )

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
  2010-11-16 17:05 [PATCH 0/3] embed drm_gem_object into radeon_bo Sedat Dilek
@ 2010-11-16 17:30 ` Daniel Vetter
  2010-11-16 19:37   ` Sedat Dilek
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2010-11-16 17:30 UTC (permalink / raw)
  To: sedat.dilek; +Cc: daniel.vetter, Arnd Bergmann, DRI

On Tue, Nov 16, 2010 at 06:05:25PM +0100, Sedat Dilek wrote:
> I have tested both patchsets from Daniel (see [1] and [2]) again on a
> Radeon RV250 in a none-BKL-config and it looks like
> 
>        agd5f-pflip/0002-drm-radeon-kms-add-pageflip-ioctl-support.patch
> 
> is the culprit in combination with the below listed drm patches.
Likely a gem_bo->driver_private access. My patches set this to NULL to
catch conversion bugs.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
  2010-11-16 17:30 ` Daniel Vetter
@ 2010-11-16 19:37   ` Sedat Dilek
  2010-11-16 19:54     ` Sedat Dilek
  0 siblings, 1 reply; 11+ messages in thread
From: Sedat Dilek @ 2010-11-16 19:37 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: daniel.vetter, Arnd Bergmann, DRI

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

On Tue, Nov 16, 2010 at 6:30 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Nov 16, 2010 at 06:05:25PM +0100, Sedat Dilek wrote:
>> I have tested both patchsets from Daniel (see [1] and [2]) again on a
>> Radeon RV250 in a none-BKL-config and it looks like
>>
>>        agd5f-pflip/0002-drm-radeon-kms-add-pageflip-ioctl-support.patch
>>
>> is the culprit in combination with the below listed drm patches.
> Likely a gem_bo->driver_private access. My patches set this to NULL to
> catch conversion bugs.
> -Daniel
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48
>

[ CCing Alex Deucher ]

With the attached diff to the original patch from [1], OpenArena works
with pageflip-support for radeon-KMS.
Unfortunately, there is a drop in fps from 18.5 down to 13.5.

- Sedat -

[1] http://people.freedesktop.org/~agd5f/pflip/0002-drm-radeon-kms-add-pageflip-ioctl-support.patch

$ cd ~/src/linux-2.6/linux-2.6.37-rc2/debian/build/source_i386_none/

$ cat .pc/applied-patches
danvet-drm-for-sedat-dilek/0001-drm-nouveau-don-t-munge-in-drm_mm-internals.patch
danvet-drm-for-sedat-dilek/0002-drm_mm-add-support-for-range-restricted-fair-lru-sca.patch
danvet-drm-for-sedat-dilek/0003-drm-mm-track-free-areas-implicitly.patch
danvet-drm-for-sedat-dilek/0004-drm-mm-extract-node-insert-helper-functions.patch
danvet-drm-for-sedat-dilek/0005-drm-mm-add-api-for-embedding-struct-drm_mm_node.patch
danvet-drm-for-sedat-dilek/0006-drm-mm-add-helper-to-unwind-scan-state.patch
danvet-embed-drm_gem_object-into-radeon_bo/1-3-drm-radeon-embed-struct-drm_gem_object.patch
danvet-embed-drm_gem_object-into-radeon_bo/2-3-drm-radeon-introduce-gem_to_radeon_bo-helper.patch
danvet-embed-drm_gem_object-into-radeon_bo/3-3-drm-radeon-kill-radeon_bo--gobj-pointer.patch
drm-vblank-timestamping/0001-drm-vblank-Add-support-for-precise-vblank-timestampi.patch
drm-vblank-timestamping/0002-drm-radeon-Add-support-for-precise-vblank-timestampi.patch
for-drm-radeon-testing/drm-radeon-kms-enable-writeback-on-radeon-AGP-boards.patch
agd5f-pflip/0002-drm-radeon-kms-add-pageflip-ioctl-support-for-danvet-v2.patch

$ cd ~/src/mesa/

$ ./scripts/run_openarena-benchmark.sh
840 frames 62.6 seconds 13.4 fps 16.0/74.5/224.0/19.7 ms

[-- Attachment #2: agd5f-pflip.diff --]
[-- Type: text/x-diff, Size: 841 bytes --]

diff -Naur 0002-drm-radeon-kms-add-pageflip-ioctl-support.patch 0002-drm-radeon-kms-add-pageflip-ioctl-support-for-danvet-v2.patch
--- 0002-drm-radeon-kms-add-pageflip-ioctl-support.patch	2010-10-31 18:53:38.000000000 +0100
+++ 0002-drm-radeon-kms-add-pageflip-ioctl-support-for-danvet-v2.patch	2010-11-16 20:07:12.380393000 +0100
@@ -1319,7 +1319,7 @@
 +	new_radeon_fb = to_radeon_framebuffer(fb);
 +	/* schedule unpin of the old buffer */
 +	obj = old_radeon_fb->obj;
-+	rbo = obj->driver_private;
++	rbo = gem_to_radeon_bo(obj);
 +	work->old_rbo = rbo;
 +	INIT_WORK(&work->work, radeon_unpin_work_func);
 +
@@ -1338,7 +1338,7 @@
 +
 +	/* pin the new buffer */
 +	obj = new_radeon_fb->obj;
-+	rbo = obj->driver_private;
++	rbo = gem_to_radeon_bo(obj);
 +	r = radeon_bo_reserve(rbo, false);
 +	if (unlikely(r != 0))
 +		goto pflip_cleanup;

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
  2010-11-16 19:37   ` Sedat Dilek
@ 2010-11-16 19:54     ` Sedat Dilek
  0 siblings, 0 replies; 11+ messages in thread
From: Sedat Dilek @ 2010-11-16 19:54 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: daniel.vetter, Arnd Bergmann, DRI

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

On Tue, Nov 16, 2010 at 8:37 PM, Sedat Dilek <sedat.dilek@googlemail.com> wrote:
> On Tue, Nov 16, 2010 at 6:30 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Nov 16, 2010 at 06:05:25PM +0100, Sedat Dilek wrote:
>>> I have tested both patchsets from Daniel (see [1] and [2]) again on a
>>> Radeon RV250 in a none-BKL-config and it looks like
>>>
>>>        agd5f-pflip/0002-drm-radeon-kms-add-pageflip-ioctl-support.patch
>>>
>>> is the culprit in combination with the below listed drm patches.
>> Likely a gem_bo->driver_private access. My patches set this to NULL to
>> catch conversion bugs.
>> -Daniel
>> --
>> Daniel Vetter
>> Mail: daniel@ffwll.ch
>> Mobile: +41 (0)79 365 57 48
>>
>
> [ CCing Alex Deucher ]
>
> With the attached diff to the original patch from [1], OpenArena works
> with pageflip-support for radeon-KMS.
> Unfortunately, there is a drop in fps from 18.5 down to 13.5.
>
> - Sedat -
>
> [1] http://people.freedesktop.org/~agd5f/pflip/0002-drm-radeon-kms-add-pageflip-ioctl-support.patch
>
> $ cd ~/src/linux-2.6/linux-2.6.37-rc2/debian/build/source_i386_none/
>
> $ cat .pc/applied-patches
> danvet-drm-for-sedat-dilek/0001-drm-nouveau-don-t-munge-in-drm_mm-internals.patch
> danvet-drm-for-sedat-dilek/0002-drm_mm-add-support-for-range-restricted-fair-lru-sca.patch
> danvet-drm-for-sedat-dilek/0003-drm-mm-track-free-areas-implicitly.patch
> danvet-drm-for-sedat-dilek/0004-drm-mm-extract-node-insert-helper-functions.patch
> danvet-drm-for-sedat-dilek/0005-drm-mm-add-api-for-embedding-struct-drm_mm_node.patch
> danvet-drm-for-sedat-dilek/0006-drm-mm-add-helper-to-unwind-scan-state.patch
> danvet-embed-drm_gem_object-into-radeon_bo/1-3-drm-radeon-embed-struct-drm_gem_object.patch
> danvet-embed-drm_gem_object-into-radeon_bo/2-3-drm-radeon-introduce-gem_to_radeon_bo-helper.patch
> danvet-embed-drm_gem_object-into-radeon_bo/3-3-drm-radeon-kill-radeon_bo--gobj-pointer.patch
> drm-vblank-timestamping/0001-drm-vblank-Add-support-for-precise-vblank-timestampi.patch
> drm-vblank-timestamping/0002-drm-radeon-Add-support-for-precise-vblank-timestampi.patch
> for-drm-radeon-testing/drm-radeon-kms-enable-writeback-on-radeon-AGP-boards.patch
> agd5f-pflip/0002-drm-radeon-kms-add-pageflip-ioctl-support-for-danvet-v2.patch
>
> $ cd ~/src/mesa/
>
> $ ./scripts/run_openarena-benchmark.sh
> 840 frames 62.6 seconds 13.4 fps 16.0/74.5/224.0/19.7 ms
>

Attached are all the patches I used with quilt-series file and kernel-config.

- Sedat -

[-- Attachment #2: for-danvet_patches.tar.xz --]
[-- Type: application/octet-stream, Size: 39512 bytes --]

[-- Attachment #3: for-danvet_patches.tar.xz.sha256sum --]
[-- Type: application/octet-stream, Size: 92 bytes --]

626533723a3828b5d54303216fe81e5e28bb9fa8a5b30dd9cd1fb63d37df04ed  for-danvet_patches.tar.xz

[-- Attachment #4: bkl-config.tar.xz --]
[-- Type: application/octet-stream, Size: 40432 bytes --]

[-- Attachment #5: bkl-config.tar.xz.sha256sum --]
[-- Type: application/octet-stream, Size: 84 bytes --]

e8ccf66b3eda0a33c87bf025491493a7600a3234c7ce0db77a8856faeb818157  bkl-config.tar.xz

[-- Attachment #6: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 0/3] embed drm_gem_object into radeon_bo
@ 2010-11-28  0:29 Sedat Dilek
  0 siblings, 0 replies; 11+ messages in thread
From: Sedat Dilek @ 2010-11-28  0:29 UTC (permalink / raw)
  To: daniel.vetter, DRI

Hi Daniel,

I have tested this upgraded patchset again with linux-next
(next-20101126), they work fine.
Can you next time label the complete series as "v2": [PATCH 0/3 v2]
embed drm_gem_object into radeon_bo (don't ask me if git can create
this automatically for you).

Feel free to add:

      Tested-by: Sedat Dilek <sedat.dilek@gmail.com>

Unfortunately, with
drm-radeon-kms-improve-pflip-precision-on-r1xx-r4xx.patch from [1] in
addition and running Eric Anholt's OpenArena benchmark... it is really
smoother with pflip now but after benchmark I cannot return to
desktop.
That is a problem not of your patchset, Daniel. I already reported on
IRC to Alex.

Hope to see also your embed-gtt-space patchset soonish in drm-next (I
have still the patches from [2] in my kernel patch-series).


Kind Regards,
- Sedat -

[1] https://patchwork.kernel.org/patch/348981/
[2] http://cgit.freedesktop.org/~danvet/drm/log/?h=for-sedat-dilek

$ cd src/linux-2.6/linux-2.6.37-rc3/debian/build/source_i386_none/

$ cat .pc/applied-patches
danvet-embed-drm_gem_object-into-radeon_bo/1-3-drm-radeon-embed-struct-drm_gem_object.patch
danvet-embed-drm_gem_object-into-radeon_bo/2-3-drm-radeon-introduce-gem_to_radeon_bo-helper.patch
danvet-embed-drm_gem_object-into-radeon_bo/3-3-drm-radeon-kill-radeon_bo--gobj-pointer.patch

$ ls -l patches/danvet-embed-drm_gem_object-into-radeon_bo/insgesamt 32
-rw-r--r-- 1 sd sd 12520 28. Nov 00:51
1-3-drm-radeon-embed-struct-drm_gem_object.patch
-rw-r--r-- 1 sd sd 11560 28. Nov 00:53
2-3-drm-radeon-introduce-gem_to_radeon_bo-helper.patch
-rw-r--r-- 1 sd sd  2556 28. Nov 00:53
3-3-drm-radeon-kill-radeon_bo--gobj-pointer.patch

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 0/3] embed drm_gem_object into radeon_bo
@ 2010-11-27  9:47 Daniel Vetter
  0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2010-11-27  9:47 UTC (permalink / raw)
  To: airlied; +Cc: Daniel Vetter, dri-devel

Hi Dave,

As promised rebased on top of latest drm-next to resolve a conflict the
pageflipping code.

Tested on my agp rv570.

Please review and consider merging for -next.

Thanks, Daniel

Daniel Vetter (3):
  drm/radeon: embed struct drm_gem_object
  drm/radeon: introduce gem_to_radeon_bo helper
  drm/radeon: kill radeon_bo->gobj pointer

 drivers/gpu/drm/radeon/atombios_crtc.c      |    8 ++--
 drivers/gpu/drm/radeon/evergreen_blit_kms.c |    2 +-
 drivers/gpu/drm/radeon/r600.c               |    2 +-
 drivers/gpu/drm/radeon/r600_blit_kms.c      |    2 +-
 drivers/gpu/drm/radeon/radeon.h             |    3 +-
 drivers/gpu/drm/radeon/radeon_benchmark.c   |    4 +-
 drivers/gpu/drm/radeon/radeon_cs.c          |    2 +-
 drivers/gpu/drm/radeon/radeon_device.c      |    4 +-
 drivers/gpu/drm/radeon/radeon_display.c     |    4 +-
 drivers/gpu/drm/radeon/radeon_fb.c          |   10 +++---
 drivers/gpu/drm/radeon/radeon_gart.c        |    2 +-
 drivers/gpu/drm/radeon/radeon_gem.c         |   43 ++++++++++++---------------
 drivers/gpu/drm/radeon/radeon_legacy_crtc.c |    4 +-
 drivers/gpu/drm/radeon/radeon_object.c      |   24 +++++++--------
 drivers/gpu/drm/radeon/radeon_object.h      |    7 ++--
 drivers/gpu/drm/radeon/radeon_ring.c        |    4 +-
 drivers/gpu/drm/radeon/radeon_test.c        |    4 +-
 drivers/gpu/drm/radeon/radeon_ttm.c         |    2 +-
 drivers/gpu/drm/radeon/rv770.c              |    2 +-
 19 files changed, 63 insertions(+), 70 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
  2010-11-15 18:45   ` Daniel Vetter
@ 2010-11-15 20:48     ` Thomas Hellstrom
  0 siblings, 0 replies; 11+ messages in thread
From: Thomas Hellstrom @ 2010-11-15 20:48 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, dri-devel

On 11/15/2010 07:45 PM, Daniel Vetter wrote:
> Hi Thomas,
>
> Thanks for your comments about ttm and vmwgfx. Some of my own ideas about
> where this might all be heading below.
>
> On Mon, Nov 15, 2010 at 08:25:16AM +0100, Thomas Hellstrom wrote:
>    
>> Hi, Daniel,
>>
>> My main concerns previously for embedding GEM objects as user-space
>> references for TTM has been twofold and implementation specific.
>>
>> 1) The locking has been using global mutexes where local spin- or
>> RCU locks have been more appropriate. It looks like this has finally
>> been / is finally going to be addressed.
>>      
> gem object lookup / insert has always (iirc) been protected by a spinlock.
> drm/i915 is just a bit inefficient with lookups, hence this spinlock
> showing up in profiling.
>
>    
>> 2) The gem object is too specialized for general purpose use:
>> a) The shmem member has no natural use with TTM except possibly as a
>> persistent swap storage, but in an ideal world, TTM would talk to
>> the swap cache directly so in the longer term there is no need for
>> the shmem object at all.
>>      
> Chris Wilson is working on a gem_vm for i915 that employs a address_space
> per bo and directly manages swap entries and has its own page allocator
> (actualy cflushed page cache). I haven't yet looked into this closely
> (especially the ttm part), but this might (at least in parts) be shareable
> between i915 and ttm.  At least it gets rid of the shmfs inode in struct
> drm_gem_object!
>
>    
>> b) Implementations may want to use other user-space visible objects
>> than buffer objects:
>> For example fence objects or CPU synchronization objects. The common
>> traits of / operations on these are user-space visibility,
>> inter-process visibility, refcounting and destruction when the
>> relevant file is closed.
>>
>> Hence a class that provides only the user-space handles, naming
>> (flink), refcounting and registering with a file handle would be the
>> best choice of a "base" class. Traditional Gem objects could derive
>> from those and provide any extra *generic* members needed for buffer
>> objects.
>>
>> This doesn't really affect your work though. Just some comments on
>> why vmwgfx don't use GEM objects by default and how they could be
>> made optimal for TTM-aware drivers.
>>      
> Yeah, I've noticed that vmwgfx is the (sometimes) sole user of many of the
> more fancy stuff in ttm. And I also don't like the duplication between gem
> and ttm. My plan (i.e. wishful thinking) is to have a common set of helper
> functions that can be shared between i915, radeon and nouveau and any
> other driver with a gem-like interface (i.e. buffer-objects + execbuffer,
> gpu<->cpu sync abstracted away by the kernel). Leaving the cracy stuff for
> drivers that need it (vmwgfx). Nothing concrete though (besides a new
> testing rig to start hacking on nouveau), I'll intend to simply plow
> along, hunting for common patterns.
>    

Sounds good. I actually think the only thing which is currently 
vmwgfx-specific are the ttm_objects and the ttm_locks. TTM objects could 
really be replaced by gem objects with the bo-specific part stripped, as 
discussed above. And then there is the ttm lock to allow concurrent 
execbufs running.

/Thomas






>    
>> Thanks,
>> /Thomas
>>      
> Yours, Daniel
>    

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
  2010-11-15  7:25 ` Thomas Hellstrom
@ 2010-11-15 18:45   ` Daniel Vetter
  2010-11-15 20:48     ` Thomas Hellstrom
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2010-11-15 18:45 UTC (permalink / raw)
  To: Thomas Hellstrom; +Cc: Daniel Vetter, dri-devel

Hi Thomas,

Thanks for your comments about ttm and vmwgfx. Some of my own ideas about
where this might all be heading below.

On Mon, Nov 15, 2010 at 08:25:16AM +0100, Thomas Hellstrom wrote:
> Hi, Daniel,
> 
> My main concerns previously for embedding GEM objects as user-space
> references for TTM has been twofold and implementation specific.
> 
> 1) The locking has been using global mutexes where local spin- or
> RCU locks have been more appropriate. It looks like this has finally
> been / is finally going to be addressed.

gem object lookup / insert has always (iirc) been protected by a spinlock.
drm/i915 is just a bit inefficient with lookups, hence this spinlock
showing up in profiling.
 
> 2) The gem object is too specialized for general purpose use:
> a) The shmem member has no natural use with TTM except possibly as a
> persistent swap storage, but in an ideal world, TTM would talk to
> the swap cache directly so in the longer term there is no need for
> the shmem object at all.

Chris Wilson is working on a gem_vm for i915 that employs a address_space
per bo and directly manages swap entries and has its own page allocator
(actualy cflushed page cache). I haven't yet looked into this closely
(especially the ttm part), but this might (at least in parts) be shareable
between i915 and ttm.  At least it gets rid of the shmfs inode in struct
drm_gem_object!

> b) Implementations may want to use other user-space visible objects
> than buffer objects:
> For example fence objects or CPU synchronization objects. The common
> traits of / operations on these are user-space visibility,
> inter-process visibility, refcounting and destruction when the
> relevant file is closed.
> 
> Hence a class that provides only the user-space handles, naming
> (flink), refcounting and registering with a file handle would be the
> best choice of a "base" class. Traditional Gem objects could derive
> from those and provide any extra *generic* members needed for buffer
> objects.
> 
> This doesn't really affect your work though. Just some comments on
> why vmwgfx don't use GEM objects by default and how they could be
> made optimal for TTM-aware drivers.

Yeah, I've noticed that vmwgfx is the (sometimes) sole user of many of the
more fancy stuff in ttm. And I also don't like the duplication between gem
and ttm. My plan (i.e. wishful thinking) is to have a common set of helper
functions that can be shared between i915, radeon and nouveau and any
other driver with a gem-like interface (i.e. buffer-objects + execbuffer,
gpu<->cpu sync abstracted away by the kernel). Leaving the cracy stuff for
drivers that need it (vmwgfx). Nothing concrete though (besides a new
testing rig to start hacking on nouveau), I'll intend to simply plow
along, hunting for common patterns.

> Thanks,
> /Thomas

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
@ 2010-11-15 10:20 Sedat Dilek
  0 siblings, 0 replies; 11+ messages in thread
From: Sedat Dilek @ 2010-11-15 10:20 UTC (permalink / raw)
  To: DRI; +Cc: daniel.vetter

Hi Daniel,

I have tried this patchset on a RV250 with linux-next (next-20101115)
in combination w/ patchset from "[PATCH 0/9] make struct drm_mm_node
embeddable" [1].

glxgears works nice.
2nd test-case is Eric Anholt's OpenArena benchmark.
The screen gets blank and system is unusable (cold start, poweroff-button).

Mesa is from master GIT branch:

$ cd ~/src/mesa
$ grep -A4 "git log" mesa.log
+ git log --pretty=short -1
commit 9cf25b3d1cd2910ae33e1faafa04629638bff0fe
Author: Marek Olšák <maraeo@gmail.com>

    r300g: return shader caps from Draw for SWTCL vertex shaders

Daniel requested me to bisect:

bad: 3-3-drm-radeon-kill-radeon_bo--gobj-pointer.patch +
2-3-drm-radeon-introduce-gem_to_radeon_bo-helper.patch
good: 1-3-drm-radeon-embed-struct-drm_gem_object.patch

$cd ~/src/linux-2.6/linux-2.6.37-rc1/debian/build/source_i386_none
$ cat .pc/applied-patches
danvet-embed-drm_gem_object-into-radeon_bo/1-3-drm-radeon-embed-struct-drm_gem_object.patch

Looks like patch #2 is culprit according to Daniel.

Kind Regards,
- Sedat -


[1] http://lists.freedesktop.org/archives/dri-devel/2010-November/005420.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
  2010-11-13 20:57 Daniel Vetter
@ 2010-11-15  7:25 ` Thomas Hellstrom
  2010-11-15 18:45   ` Daniel Vetter
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Hellstrom @ 2010-11-15  7:25 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: dri-devel

Hi, Daniel,

My main concerns previously for embedding GEM objects as user-space 
references for TTM has been twofold and implementation specific.

1) The locking has been using global mutexes where local spin- or RCU 
locks have been more appropriate. It looks like this has finally been / 
is finally going to be addressed.

2) The gem object is too specialized for general purpose use:
a) The shmem member has no natural use with TTM except possibly as a 
persistent swap storage, but in an ideal world, TTM would talk to the 
swap cache directly so in the longer term there is no need for the shmem 
object at all.
b) Implementations may want to use other user-space visible objects than 
buffer objects:
For example fence objects or CPU synchronization objects. The common 
traits of / operations on these are user-space visibility, inter-process 
visibility, refcounting and destruction when the relevant file is closed.

Hence a class that provides only the user-space handles, naming (flink), 
refcounting and registering with a file handle would be the best choice 
of a "base" class. Traditional Gem objects could derive from those and 
provide any extra *generic* members needed for buffer objects.

This doesn't really affect your work though. Just some comments on why 
vmwgfx don't use GEM objects by default and how they could be made 
optimal for TTM-aware drivers.

Thanks,
/Thomas


On 11/13/2010 09:57 PM, Daniel Vetter wrote:
> Hi all,
>
> This patch series embeds drm_gem_object into struct radeon_bo and adjusts
> any references. I've decided against embedding the gem stuff into struct
> ttm_bo because
> - vmwgfx doesn't use gem and
> - ttm is used for implementing the memory management, whereas gem provides
>    the userspace interface (I know, there's some overlap there, but that's
>    not really a new problem).
>
> Please review and consider merging for -next.
>
> Yours, Daniel
>
> Daniel Vetter (3):
>    drm/radeon: embed struct drm_gem_object
>    drm/radeon: introduce gem_to_radeon_bo helper
>    drm/radeon: kill radeon_bo->gobj pointer
>
>   drivers/gpu/drm/radeon/atombios_crtc.c      |    8 ++--
>   drivers/gpu/drm/radeon/evergreen_blit_kms.c |    2 +-
>   drivers/gpu/drm/radeon/r600.c               |    2 +-
>   drivers/gpu/drm/radeon/r600_blit_kms.c      |    2 +-
>   drivers/gpu/drm/radeon/radeon.h             |    3 +-
>   drivers/gpu/drm/radeon/radeon_benchmark.c   |    4 +-
>   drivers/gpu/drm/radeon/radeon_cs.c          |    2 +-
>   drivers/gpu/drm/radeon/radeon_device.c      |    4 +-
>   drivers/gpu/drm/radeon/radeon_fb.c          |   10 +++---
>   drivers/gpu/drm/radeon/radeon_gart.c        |    2 +-
>   drivers/gpu/drm/radeon/radeon_gem.c         |   43 ++++++++++++---------------
>   drivers/gpu/drm/radeon/radeon_legacy_crtc.c |    4 +-
>   drivers/gpu/drm/radeon/radeon_object.c      |   24 +++++++--------
>   drivers/gpu/drm/radeon/radeon_object.h      |    2 +-
>   drivers/gpu/drm/radeon/radeon_ring.c        |    4 +-
>   drivers/gpu/drm/radeon/radeon_test.c        |    4 +-
>   drivers/gpu/drm/radeon/radeon_ttm.c         |    2 +-
>   drivers/gpu/drm/radeon/rv770.c              |    2 +-
>   18 files changed, 59 insertions(+), 65 deletions(-)
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>    

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 0/3] embed drm_gem_object into radeon_bo
@ 2010-11-13 20:57 Daniel Vetter
  2010-11-15  7:25 ` Thomas Hellstrom
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2010-11-13 20:57 UTC (permalink / raw)
  To: dri-devel; +Cc: Daniel Vetter

Hi all,

This patch series embeds drm_gem_object into struct radeon_bo and adjusts
any references. I've decided against embedding the gem stuff into struct
ttm_bo because
- vmwgfx doesn't use gem and
- ttm is used for implementing the memory management, whereas gem provides
  the userspace interface (I know, there's some overlap there, but that's
  not really a new problem).

Please review and consider merging for -next.

Yours, Daniel

Daniel Vetter (3):
  drm/radeon: embed struct drm_gem_object
  drm/radeon: introduce gem_to_radeon_bo helper
  drm/radeon: kill radeon_bo->gobj pointer

 drivers/gpu/drm/radeon/atombios_crtc.c      |    8 ++--
 drivers/gpu/drm/radeon/evergreen_blit_kms.c |    2 +-
 drivers/gpu/drm/radeon/r600.c               |    2 +-
 drivers/gpu/drm/radeon/r600_blit_kms.c      |    2 +-
 drivers/gpu/drm/radeon/radeon.h             |    3 +-
 drivers/gpu/drm/radeon/radeon_benchmark.c   |    4 +-
 drivers/gpu/drm/radeon/radeon_cs.c          |    2 +-
 drivers/gpu/drm/radeon/radeon_device.c      |    4 +-
 drivers/gpu/drm/radeon/radeon_fb.c          |   10 +++---
 drivers/gpu/drm/radeon/radeon_gart.c        |    2 +-
 drivers/gpu/drm/radeon/radeon_gem.c         |   43 ++++++++++++---------------
 drivers/gpu/drm/radeon/radeon_legacy_crtc.c |    4 +-
 drivers/gpu/drm/radeon/radeon_object.c      |   24 +++++++--------
 drivers/gpu/drm/radeon/radeon_object.h      |    2 +-
 drivers/gpu/drm/radeon/radeon_ring.c        |    4 +-
 drivers/gpu/drm/radeon/radeon_test.c        |    4 +-
 drivers/gpu/drm/radeon/radeon_ttm.c         |    2 +-
 drivers/gpu/drm/radeon/rv770.c              |    2 +-
 18 files changed, 59 insertions(+), 65 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2010-11-28  0:29 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-16 17:05 [PATCH 0/3] embed drm_gem_object into radeon_bo Sedat Dilek
2010-11-16 17:30 ` Daniel Vetter
2010-11-16 19:37   ` Sedat Dilek
2010-11-16 19:54     ` Sedat Dilek
  -- strict thread matches above, loose matches on Subject: below --
2010-11-28  0:29 Sedat Dilek
2010-11-27  9:47 Daniel Vetter
2010-11-15 10:20 Sedat Dilek
2010-11-13 20:57 Daniel Vetter
2010-11-15  7:25 ` Thomas Hellstrom
2010-11-15 18:45   ` Daniel Vetter
2010-11-15 20:48     ` Thomas Hellstrom

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.