dri-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Thomas Hellstrom <thomas@shipmail.org>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 0/3] embed drm_gem_object into radeon_bo
Date: Mon, 15 Nov 2010 08:25:16 +0100
Message-ID: <4CE0E05C.7030507@shipmail.org> (raw)
In-Reply-To: <1289681854-15505-1-git-send-email-daniel.vetter@ffwll.ch>

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
>    

  parent reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-11-13 20:57 Daniel Vetter
2010-11-13 20:57 ` [PATCH 1/3] drm/radeon: embed struct drm_gem_object Daniel Vetter
2010-11-13 20:57 ` [PATCH 2/3] drm/radeon: introduce gem_to_radeon_bo helper Daniel Vetter
2010-11-13 20:57 ` [PATCH 3/3] drm/radeon: kill radeon_bo->gobj pointer Daniel Vetter
2010-11-15  7:25 ` Thomas Hellstrom [this message]
2010-11-15 18:45   ` [PATCH 0/3] embed drm_gem_object into radeon_bo Daniel Vetter
2010-11-15 20:48     ` Thomas Hellstrom
2010-11-15 10:20 Sedat Dilek
2010-11-16 17:05 Sedat Dilek
2010-11-16 17:30 ` Daniel Vetter
2010-11-16 19:37   ` Sedat Dilek
2010-11-16 19:54     ` Sedat Dilek
2010-11-27  9:47 Daniel Vetter
2010-11-28  0:29 Sedat Dilek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4CE0E05C.7030507@shipmail.org \
    --to=thomas@shipmail.org \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git
	git clone --mirror https://lore.kernel.org/dri-devel/1 dri-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git