[PATCH 0/3] RFC: Common functions for GEM offset creation
>> On Tue, Jul 19, 2011 at 4:33 AM, Chris Wilson >> wrote: >>> My only concern is that for the common functions the mmap_offset to create >>> should be passed in a parameter, so that we could support more than one >>> mapping for an object. [ snip ] > On Tue, Jul 19, 2011 at 8:12 AM, Rob Clark wrote: > Chris, any suggestions? ?I still haven't found a good excuse for > offsets to be per-process. > > I'm just wondering if I should go ahead and send a non-RFC version of > the patches. ?I guess in the end it is not userspace visible so > completely possible to change later. ?But it seems these util fxns > should also be useful to a handful of other upcoming SoC DRM drivers > (such as the Samsung one that was recently posted). Imo you're patches looks nice and should go in as is. Fixing the mmap handling of gem objects for real looks like more work: For the second cpu-coherent mapping of i915 gem objects (as opposed to the gpu coherent mapping using the mmap_offset infrastructure) we directly create a vma for the underlying shmfs node in a hand-rolled mmap ioctl (using do_mmap), the core drm mmap handling is layered with a bit of historical cruft of it's own and ttm seems to do a bit of reinventing the wheel. So imo this needs some more cleanup to be nice and beautiful than just adding an additional argument. It's somewhere on one of my todo list ... ;-) Cheers, Daniel -- Daniel Vetter daniel.vetter at ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
Re: [PATCH 0/3] RFC: Common functions for GEM offset creation
>> On Tue, Jul 19, 2011 at 4:33 AM, Chris Wilson >> wrote: >>> My only concern is that for the common functions the mmap_offset to create >>> should be passed in a parameter, so that we could support more than one >>> mapping for an object. [ snip ] > On Tue, Jul 19, 2011 at 8:12 AM, Rob Clark wrote: > Chris, any suggestions? I still haven't found a good excuse for > offsets to be per-process. > > I'm just wondering if I should go ahead and send a non-RFC version of > the patches. I guess in the end it is not userspace visible so > completely possible to change later. But it seems these util fxns > should also be useful to a handful of other upcoming SoC DRM drivers > (such as the Samsung one that was recently posted). Imo you're patches looks nice and should go in as is. Fixing the mmap handling of gem objects for real looks like more work: For the second cpu-coherent mapping of i915 gem objects (as opposed to the gpu coherent mapping using the mmap_offset infrastructure) we directly create a vma for the underlying shmfs node in a hand-rolled mmap ioctl (using do_mmap), the core drm mmap handling is layered with a bit of historical cruft of it's own and ttm seems to do a bit of reinventing the wheel. So imo this needs some more cleanup to be nice and beautiful than just adding an additional argument. It's somewhere on one of my todo list ... ;-) Cheers, Daniel -- Daniel Vetter daniel.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] RFC: Common functions for GEM offset creation
On Tue, Jul 19, 2011 at 8:12 AM, Rob Clark wrote: > On Tue, Jul 19, 2011 at 4:33 AM, Chris Wilson > wrote: >> On Mon, 18 Jul 2011 19:20:56 -0500, Rob Clark wrote: >>> In the process of adding GEM support for OMAP DRM driver, I noticed that >>> I was adding code for creating/freeing mmap offsets which was virtually >>> identical to what was already duplicated in i915 and gma500 drivers. >>> >>> Rather than duplicating the code a 3rd time, it seemed like a good idea >>> to move it to the GEM core. >>> >>> Note that I don't actually have a way to test psb or i915, but the >>> changes seem straightforward enough. >> >> My only concern is that for the common functions the mmap_offset to create >> should be passed in a parameter, so that we could support more than one >> mapping for an object. > > I admit I've not got quite as far as dealing with this yet.. ?I'm just > starting on the dri2 part in xorg driver. ?(Previous pvr xorg driver > has some non-GEM way of sharing buffers.) ?So I'm figuring out some of > this stuff as I go. > > For me I think it isn't the end of the world to have same offset in > all processes, although I'm interested if there is a better way. > There is just one 'struct drm_local_map' in 'struct drm_gem_object', > so I admit that I'm not quite sure how this was intended to work. > Chris, any suggestions? I still haven't found a good excuse for offsets to be per-process. I'm just wondering if I should go ahead and send a non-RFC version of the patches. I guess in the end it is not userspace visible so completely possible to change later. But it seems these util fxns should also be useful to a handful of other upcoming SoC DRM drivers (such as the Samsung one that was recently posted). BR, -R
Re: [PATCH 0/3] RFC: Common functions for GEM offset creation
On Tue, Jul 19, 2011 at 8:12 AM, Rob Clark wrote: > On Tue, Jul 19, 2011 at 4:33 AM, Chris Wilson > wrote: >> On Mon, 18 Jul 2011 19:20:56 -0500, Rob Clark wrote: >>> In the process of adding GEM support for OMAP DRM driver, I noticed that >>> I was adding code for creating/freeing mmap offsets which was virtually >>> identical to what was already duplicated in i915 and gma500 drivers. >>> >>> Rather than duplicating the code a 3rd time, it seemed like a good idea >>> to move it to the GEM core. >>> >>> Note that I don't actually have a way to test psb or i915, but the >>> changes seem straightforward enough. >> >> My only concern is that for the common functions the mmap_offset to create >> should be passed in a parameter, so that we could support more than one >> mapping for an object. > > I admit I've not got quite as far as dealing with this yet.. I'm just > starting on the dri2 part in xorg driver. (Previous pvr xorg driver > has some non-GEM way of sharing buffers.) So I'm figuring out some of > this stuff as I go. > > For me I think it isn't the end of the world to have same offset in > all processes, although I'm interested if there is a better way. > There is just one 'struct drm_local_map' in 'struct drm_gem_object', > so I admit that I'm not quite sure how this was intended to work. > Chris, any suggestions? I still haven't found a good excuse for offsets to be per-process. I'm just wondering if I should go ahead and send a non-RFC version of the patches. I guess in the end it is not userspace visible so completely possible to change later. But it seems these util fxns should also be useful to a handful of other upcoming SoC DRM drivers (such as the Samsung one that was recently posted). BR, -R ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] RFC: Common functions for GEM offset creation
> ahh, ok.. I should check this out (although I'm not entirely sure > where your gma500 staging tree is) Its in GregKH's staging tree or linux-next. It's basically what you've done so if your patch is submitted it'll take me 2 minutes to fix the gma500 tree. > I'm already using your patch to add drm_gem_private_object_init() for > scanout buffers (which need to be physically contiguous). But perhaps > you have some other useful "gems" No I think that one and the offset patch is it. I do need to attack the fb console code next because I need a GEM object as console in some cases and don't have enough virtual address space to vremap it to look linear. Alan
[PATCH 0/3] RFC: Common functions for GEM offset creation
On Mon, 18 Jul 2011 19:20:56 -0500, Rob Clark wrote: > In the process of adding GEM support for OMAP DRM driver, I noticed that > I was adding code for creating/freeing mmap offsets which was virtually > identical to what was already duplicated in i915 and gma500 drivers. > > Rather than duplicating the code a 3rd time, it seemed like a good idea > to move it to the GEM core. > > Note that I don't actually have a way to test psb or i915, but the > changes seem straightforward enough. My only concern is that for the common functions the mmap_offset to create should be passed in a parameter, so that we could support more than one mapping for an object. -Chris -- Chris Wilson, Intel Open Source Technology Centre
[PATCH 0/3] RFC: Common functions for GEM offset creation
On Mon, 18 Jul 2011 19:20:56 -0500 Rob Clark wrote: > In the process of adding GEM support for OMAP DRM driver, I noticed that > I was adding code for creating/freeing mmap offsets which was virtually > identical to what was already duplicated in i915 and gma500 drivers. The gma500 one was taken from i915 and I'd reach the same conclusion as you - the current GMA500 driver (see staging git) has the mmap offset patches broken out in gem_glue.c ready to do what you are proposing. > Rather than duplicating the code a 3rd time, it seemed like a good idea > to move it to the GEM core. Agreed entirely. > Note that I don't actually have a way to test psb or i915, but the > changes seem straightforward enough. Your patch doesn't apply versus gma500 but its trivial to remove it from gem_glue.c once it goes into the mainstream GEM. (The other bits in the gma500 glue are to allow for GEM objects backed by things other than shmfs) Alan
Re: [PATCH 0/3] RFC: Common functions for GEM offset creation
> ahh, ok.. I should check this out (although I'm not entirely sure > where your gma500 staging tree is) Its in GregKH's staging tree or linux-next. It's basically what you've done so if your patch is submitted it'll take me 2 minutes to fix the gma500 tree. > I'm already using your patch to add drm_gem_private_object_init() for > scanout buffers (which need to be physically contiguous). But perhaps > you have some other useful "gems" No I think that one and the offset patch is it. I do need to attack the fb console code next because I need a GEM object as console in some cases and don't have enough virtual address space to vremap it to look linear. Alan ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] RFC: Common functions for GEM offset creation
On Tue, Jul 19, 2011 at 4:33 AM, Chris Wilson wrote: > On Mon, 18 Jul 2011 19:20:56 -0500, Rob Clark wrote: >> In the process of adding GEM support for OMAP DRM driver, I noticed that >> I was adding code for creating/freeing mmap offsets which was virtually >> identical to what was already duplicated in i915 and gma500 drivers. >> >> Rather than duplicating the code a 3rd time, it seemed like a good idea >> to move it to the GEM core. >> >> Note that I don't actually have a way to test psb or i915, but the >> changes seem straightforward enough. > > My only concern is that for the common functions the mmap_offset to create > should be passed in a parameter, so that we could support more than one > mapping for an object. I admit I've not got quite as far as dealing with this yet.. I'm just starting on the dri2 part in xorg driver. (Previous pvr xorg driver has some non-GEM way of sharing buffers.) So I'm figuring out some of this stuff as I go. For me I think it isn't the end of the world to have same offset in all processes, although I'm interested if there is a better way. There is just one 'struct drm_local_map' in 'struct drm_gem_object', so I admit that I'm not quite sure how this was intended to work. BR, -R > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
[PATCH 0/3] RFC: Common functions for GEM offset creation
On Tue, Jul 19, 2011 at 3:57 AM, Alan Cox wrote: > On Mon, 18 Jul 2011 19:20:56 -0500 > Rob Clark wrote: > >> In the process of adding GEM support for OMAP DRM driver, I noticed that >> I was adding code for creating/freeing mmap offsets which was virtually >> identical to what was already duplicated in i915 and gma500 drivers. > > The gma500 one was taken from i915 and I'd reach the same conclusion as > you - the current GMA500 driver (see staging git) has the mmap offset > patches broken out in gem_glue.c ready to do what you are proposing. ahh, ok.. I should check this out (although I'm not entirely sure where your gma500 staging tree is) I'm already using your patch to add drm_gem_private_object_init() for scanout buffers (which need to be physically contiguous). But perhaps you have some other useful "gems" BR, -R >> Rather than duplicating the code a 3rd time, it seemed like a good idea >> to move it to the GEM core. > > Agreed entirely. > >> Note that I don't actually have a way to test psb or i915, but the >> changes seem straightforward enough. > > Your patch doesn't apply versus gma500 but its trivial to remove it from > gem_glue.c once it goes into the mainstream GEM. > > (The other bits in the gma500 glue are to allow for GEM objects backed by > things other than shmfs) > > Alan > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
Re: [PATCH 0/3] RFC: Common functions for GEM offset creation
On Tue, Jul 19, 2011 at 4:33 AM, Chris Wilson wrote: > On Mon, 18 Jul 2011 19:20:56 -0500, Rob Clark wrote: >> In the process of adding GEM support for OMAP DRM driver, I noticed that >> I was adding code for creating/freeing mmap offsets which was virtually >> identical to what was already duplicated in i915 and gma500 drivers. >> >> Rather than duplicating the code a 3rd time, it seemed like a good idea >> to move it to the GEM core. >> >> Note that I don't actually have a way to test psb or i915, but the >> changes seem straightforward enough. > > My only concern is that for the common functions the mmap_offset to create > should be passed in a parameter, so that we could support more than one > mapping for an object. I admit I've not got quite as far as dealing with this yet.. I'm just starting on the dri2 part in xorg driver. (Previous pvr xorg driver has some non-GEM way of sharing buffers.) So I'm figuring out some of this stuff as I go. For me I think it isn't the end of the world to have same offset in all processes, although I'm interested if there is a better way. There is just one 'struct drm_local_map' in 'struct drm_gem_object', so I admit that I'm not quite sure how this was intended to work. BR, -R > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] RFC: Common functions for GEM offset creation
On Tue, Jul 19, 2011 at 3:57 AM, Alan Cox wrote: > On Mon, 18 Jul 2011 19:20:56 -0500 > Rob Clark wrote: > >> In the process of adding GEM support for OMAP DRM driver, I noticed that >> I was adding code for creating/freeing mmap offsets which was virtually >> identical to what was already duplicated in i915 and gma500 drivers. > > The gma500 one was taken from i915 and I'd reach the same conclusion as > you - the current GMA500 driver (see staging git) has the mmap offset > patches broken out in gem_glue.c ready to do what you are proposing. ahh, ok.. I should check this out (although I'm not entirely sure where your gma500 staging tree is) I'm already using your patch to add drm_gem_private_object_init() for scanout buffers (which need to be physically contiguous). But perhaps you have some other useful "gems" BR, -R >> Rather than duplicating the code a 3rd time, it seemed like a good idea >> to move it to the GEM core. > > Agreed entirely. > >> Note that I don't actually have a way to test psb or i915, but the >> changes seem straightforward enough. > > Your patch doesn't apply versus gma500 but its trivial to remove it from > gem_glue.c once it goes into the mainstream GEM. > > (The other bits in the gma500 glue are to allow for GEM objects backed by > things other than shmfs) > > Alan > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] RFC: Common functions for GEM offset creation
On Mon, 18 Jul 2011 19:20:56 -0500, Rob Clark wrote: > In the process of adding GEM support for OMAP DRM driver, I noticed that > I was adding code for creating/freeing mmap offsets which was virtually > identical to what was already duplicated in i915 and gma500 drivers. > > Rather than duplicating the code a 3rd time, it seemed like a good idea > to move it to the GEM core. > > Note that I don't actually have a way to test psb or i915, but the > changes seem straightforward enough. My only concern is that for the common functions the mmap_offset to create should be passed in a parameter, so that we could support more than one mapping for an object. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] RFC: Common functions for GEM offset creation
On Mon, 18 Jul 2011 19:20:56 -0500 Rob Clark wrote: > In the process of adding GEM support for OMAP DRM driver, I noticed that > I was adding code for creating/freeing mmap offsets which was virtually > identical to what was already duplicated in i915 and gma500 drivers. The gma500 one was taken from i915 and I'd reach the same conclusion as you - the current GMA500 driver (see staging git) has the mmap offset patches broken out in gem_glue.c ready to do what you are proposing. > Rather than duplicating the code a 3rd time, it seemed like a good idea > to move it to the GEM core. Agreed entirely. > Note that I don't actually have a way to test psb or i915, but the > changes seem straightforward enough. Your patch doesn't apply versus gma500 but its trivial to remove it from gem_glue.c once it goes into the mainstream GEM. (The other bits in the gma500 glue are to allow for GEM objects backed by things other than shmfs) Alan ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] RFC: Common functions for GEM offset creation
In the process of adding GEM support for OMAP DRM driver, I noticed that I was adding code for creating/freeing mmap offsets which was virtually identical to what was already duplicated in i915 and gma500 drivers. Rather than duplicating the code a 3rd time, it seemed like a good idea to move it to the GEM core. Note that I don't actually have a way to test psb or i915, but the changes seem straightforward enough. -- For the curious, OMAP DRM driver is here: https://github.com/robclark/kernel-omap4/commits/linux-omap-3.0-drm I'll send patches when it's dependencies are merged and it is slightly more than half baked ;-) Rob Clark (3): drm/gem: add functions for mmap offset creation drm/i915: use common functions for mmap offset creation drm/gma500: use common functions for mmap offset creation drivers/gpu/drm/drm_gem.c| 88 ++ drivers/gpu/drm/i915/i915_gem.c | 85 +--- drivers/staging/gma500/psb_gem.c | 63 +-- include/drm/drmP.h |3 + 4 files changed, 95 insertions(+), 144 deletions(-) -- 1.7.4.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
