* multiple framebuffer drm maps @ 2011-04-25 21:54 Tormod Volden 2011-05-03 21:20 ` Tormod Volden 0 siblings, 1 reply; 12+ messages in thread From: Tormod Volden @ 2011-04-25 21:54 UTC (permalink / raw) To: dri-devel In commit 41c2e75e60200a860a74b7c84a6375c105e7437f "drm: Make drm_local_map use a resource_size_t offset" [1] the support for multiple _DRM_FRAMEBUFFER maps per device was taken away. This change made the savage drivers upset, since these cards have several apertures (the layout is different between card families) for which the kernel drm driver sets up maps. And these maps are now mixed up into one broken one. The drivers (drm, ddx, mesa) for instance expects a framebuffer map and a tiled aperture map, and the broken maps show up as rendering corruption [2] and allocation failures. I have tried to come up with userland workarounds but it seems impossible since the kernel will only return the handle to a broken map and there is no way to remap it correctly. Would it be possible to reintroduce this support? One solution could be a new flag _DRM_IGNORE_FB_OFFSET that can be used by those drivers that need it, or the other way around, a _DRM_CHECK_FB_OFFSET to be added by the savage drivers and others in the same situation. I can of course try to write a patch if people think this is a good idea. Best regards, Tormod [1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=41c2e75e60200a860a74b7c84a6375c105e7437f [2] https://bugs.freedesktop.org/show_bug.cgi?id=32511 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: multiple framebuffer drm maps 2011-04-25 21:54 multiple framebuffer drm maps Tormod Volden @ 2011-05-03 21:20 ` Tormod Volden 2011-05-03 21:54 ` Tormod Volden 0 siblings, 1 reply; 12+ messages in thread From: Tormod Volden @ 2011-05-03 21:20 UTC (permalink / raw) To: dri-devel From: Tormod Volden <debian.tormod@gmail.com> Subject: [PATCH] drm: Add flag for multiple framebuffer support Do not ignore the offset when looking for existing framebuffer maps if the new _DRM_MATCH_FB_OFFSET flag is passed to drm_addmap_core. --- > In commit 41c2e75e60200a860a74b7c84a6375c105e7437f "drm: Make > drm_local_map use a resource_size_t offset" [1] the support for multiple > _DRM_FRAMEBUFFER maps per device was taken away. This change made the > savage drivers upset, since these cards have several apertures (the > layout is different between card families) for which the kernel drm > driver sets up maps. And these maps are now mixed up into one broken one. > > The drivers (drm, ddx, mesa) for instance expects a framebuffer map and a > tiled aperture map, and the broken maps show up as rendering corruption > [2] and allocation failures. I have tried to come up with userland > workarounds but it seems impossible since the kernel will only return > the handle to a broken map and there is no way to remap it correctly. > > Would it be possible to reintroduce this support? One solution could be > a new flag _DRM_IGNORE_FB_OFFSET that can be used by those drivers that > need it, or the other way around, a _DRM_CHECK_FB_OFFSET to be added > by the savage drivers and others in the same situation. I can of course > try to write a patch if people think this is a good idea. This was what I had in mind. Probably controversial to add a new flag for this, so more elegant ideas are welcome. Cheers, Tormod drivers/gpu/drm/drm_bufs.c | 4 +++- include/drm/drm.h | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index 3e257a5..9b7b257 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -60,8 +60,10 @@ static struct drm_map_list *drm_find_matching_map(struct drm_device *dev, if (map->flags != _DRM_CONTAINS_LOCK) break; case _DRM_REGISTERS: - case _DRM_FRAME_BUFFER: return entry; + case _DRM_FRAME_BUFFER: + if (!(map->flags & _DRM_MATCH_FB_OFFSET)) + return entry; default: /* Make gcc happy */ ; } diff --git a/include/drm/drm.h b/include/drm/drm.h index 4be33b4..d4e2560 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -195,7 +195,8 @@ enum drm_map_flags { _DRM_WRITE_COMBINING = 0x10, /**< use write-combining if available */ _DRM_CONTAINS_LOCK = 0x20, /**< SHM page that contains lock */ _DRM_REMOVABLE = 0x40, /**< Removable mapping */ - _DRM_DRIVER = 0x80 /**< Managed by driver */ + _DRM_DRIVER = 0x80, /**< Managed by driver */ + _DRM_MATCH_FB_OFFSET = 0x100 /**< Multiple framebuffer support */ }; struct drm_ctx_priv_map { -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: multiple framebuffer drm maps 2011-05-03 21:20 ` Tormod Volden @ 2011-05-03 21:54 ` Tormod Volden 2011-05-22 19:27 ` [PATCH] drm/savage: Do not add framebuffer and aperture maps Tormod Volden 0 siblings, 1 reply; 12+ messages in thread From: Tormod Volden @ 2011-05-03 21:54 UTC (permalink / raw) To: dri-devel From: Tormod Volden <debian.tormod@gmail.com> Subject: [PATCH] drm/savage: Do not add framebuffer and aperture maps Since multiple framebuffer maps are not supported any longer (commit 41c2e75e60200a860a74b7c84a6375c105e7437f) these maps would be broken, and they are not used by the drm anyway. Leave it to userspace to create one working map instead. Signed-off-by: Tormod Volden <debian.tormod@gmail.com> --- And if we are absolutely sure that we do not want to support multiple framebuffers maps, I would instead suggest this change to the savage driver, which at least allows userspace to work around it. Tormod drivers/gpu/drm/savage/savage_bci.c | 13 +------------ drivers/gpu/drm/savage/savage_drv.h | 2 -- 2 files changed, 1 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/savage/savage_bci.c b/drivers/gpu/drm/savage/savage_bci.c index bf5f83e..91fe7b4 100644 --- a/drivers/gpu/drm/savage/savage_bci.c +++ b/drivers/gpu/drm/savage/savage_bci.c @@ -639,18 +639,7 @@ int savage_driver_firstopen(struct drm_device *dev) if (ret) return ret; - ret = drm_addmap(dev, fb_base, fb_size, _DRM_FRAME_BUFFER, - _DRM_WRITE_COMBINING, &dev_priv->fb); - if (ret) - return ret; - - ret = drm_addmap(dev, aperture_base, SAVAGE_APERTURE_SIZE, - _DRM_FRAME_BUFFER, _DRM_WRITE_COMBINING, - &dev_priv->aperture); - if (ret) - return ret; - - return ret; + return 0; } /* diff --git a/drivers/gpu/drm/savage/savage_drv.h b/drivers/gpu/drm/savage/savage_drv.h index df2aac6..2b49b3e 100644 --- a/drivers/gpu/drm/savage/savage_drv.h +++ b/drivers/gpu/drm/savage/savage_drv.h @@ -153,8 +153,6 @@ typedef struct drm_savage_private { /* memory regions in physical memory */ drm_local_map_t *sarea; drm_local_map_t *mmio; - drm_local_map_t *fb; - drm_local_map_t *aperture; drm_local_map_t *status; drm_local_map_t *agp_textures; drm_local_map_t *cmd_dma; -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] drm/savage: Do not add framebuffer and aperture maps 2011-05-03 21:54 ` Tormod Volden @ 2011-05-22 19:27 ` Tormod Volden 2011-05-27 18:19 ` [PATCH resend] " Tormod Volden 0 siblings, 1 reply; 12+ messages in thread From: Tormod Volden @ 2011-05-22 19:27 UTC (permalink / raw) To: dri-devel; +Cc: Dave Airlie From: Tormod Volden <debian.tormod@gmail.com> Since multiple framebuffer maps are not supported any longer (commit 41c2e75e60200a860a74b7c84a6375c105e7437f) these maps would be broken, and they are not used by the drm anyway. Leave it to userspace to create one working map instead. Signed-off-by: Tormod Volden <debian.tormod@gmail.com> --- > And if we are absolutely sure that we do not want to support multiple > framebuffers maps, I would instead suggest this change to the savage > driver, which at least allows userspace to work around it. I think there is not much interest for supporting multiple framebuffer maps at the moment, so I would suggest pushing this patch, hopefully into 2.6.40, so there is a chance of reworking the savage userspace (DDX and mesa) to deal with it instead. Regards, Tormod drivers/gpu/drm/savage/savage_bci.c | 13 +------------ drivers/gpu/drm/savage/savage_drv.h | 2 -- 2 files changed, 1 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/savage/savage_bci.c b/drivers/gpu/drm/savage/savage_bci.c index bf5f83e..91fe7b4 100644 --- a/drivers/gpu/drm/savage/savage_bci.c +++ b/drivers/gpu/drm/savage/savage_bci.c @@ -639,18 +639,7 @@ int savage_driver_firstopen(struct drm_device *dev) if (ret) return ret; - ret = drm_addmap(dev, fb_base, fb_size, _DRM_FRAME_BUFFER, - _DRM_WRITE_COMBINING, &dev_priv->fb); - if (ret) - return ret; - - ret = drm_addmap(dev, aperture_base, SAVAGE_APERTURE_SIZE, - _DRM_FRAME_BUFFER, _DRM_WRITE_COMBINING, - &dev_priv->aperture); - if (ret) - return ret; - - return ret; + return 0; } /* diff --git a/drivers/gpu/drm/savage/savage_drv.h b/drivers/gpu/drm/savage/savage_drv.h index df2aac6..2b49b3e 100644 --- a/drivers/gpu/drm/savage/savage_drv.h +++ b/drivers/gpu/drm/savage/savage_drv.h @@ -153,8 +153,6 @@ typedef struct drm_savage_private { /* memory regions in physical memory */ drm_local_map_t *sarea; drm_local_map_t *mmio; - drm_local_map_t *fb; - drm_local_map_t *aperture; drm_local_map_t *status; drm_local_map_t *agp_textures; drm_local_map_t *cmd_dma; -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH resend] drm/savage: Do not add framebuffer and aperture maps 2011-05-22 19:27 ` [PATCH] drm/savage: Do not add framebuffer and aperture maps Tormod Volden @ 2011-05-27 18:19 ` Tormod Volden 2011-05-27 20:48 ` Dave Airlie 0 siblings, 1 reply; 12+ messages in thread From: Tormod Volden @ 2011-05-27 18:19 UTC (permalink / raw) To: dri-devel; +Cc: Dave Airlie From: Tormod Volden <debian.tormod@gmail.com> Since multiple framebuffer maps are not supported any longer (commit 41c2e75e60200a860a74b7c84a6375c105e7437f) these maps would be broken, and they are not used by the drm anyway. Leave it to userspace to create one working map instead. Signed-off-by: Tormod Volden <debian.tormod@gmail.com> --- The drm driver is not required to set up fb maps, right? For instance radeon does, but its comment says "Create mappings for registers and framebuffer so userland doesn't necessarily have to find them." Tormod drivers/gpu/drm/savage/savage_bci.c | 13 +------------ drivers/gpu/drm/savage/savage_drv.h | 2 -- 2 files changed, 1 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/savage/savage_bci.c b/drivers/gpu/drm/savage/savage_bci.c index bf5f83e..91fe7b4 100644 --- a/drivers/gpu/drm/savage/savage_bci.c +++ b/drivers/gpu/drm/savage/savage_bci.c @@ -639,18 +639,7 @@ int savage_driver_firstopen(struct drm_device *dev) if (ret) return ret; - ret = drm_addmap(dev, fb_base, fb_size, _DRM_FRAME_BUFFER, - _DRM_WRITE_COMBINING, &dev_priv->fb); - if (ret) - return ret; - - ret = drm_addmap(dev, aperture_base, SAVAGE_APERTURE_SIZE, - _DRM_FRAME_BUFFER, _DRM_WRITE_COMBINING, - &dev_priv->aperture); - if (ret) - return ret; - - return ret; + return 0; } /* diff --git a/drivers/gpu/drm/savage/savage_drv.h b/drivers/gpu/drm/savage/savage_drv.h index df2aac6..2b49b3e 100644 --- a/drivers/gpu/drm/savage/savage_drv.h +++ b/drivers/gpu/drm/savage/savage_drv.h @@ -153,8 +153,6 @@ typedef struct drm_savage_private { /* memory regions in physical memory */ drm_local_map_t *sarea; drm_local_map_t *mmio; - drm_local_map_t *fb; - drm_local_map_t *aperture; drm_local_map_t *status; drm_local_map_t *agp_textures; drm_local_map_t *cmd_dma; -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH resend] drm/savage: Do not add framebuffer and aperture maps 2011-05-27 18:19 ` [PATCH resend] " Tormod Volden @ 2011-05-27 20:48 ` Dave Airlie 2011-05-27 23:44 ` Tormod Volden 0 siblings, 1 reply; 12+ messages in thread From: Dave Airlie @ 2011-05-27 20:48 UTC (permalink / raw) To: Tormod Volden; +Cc: Dave Airlie, dri-devel On Sat, May 28, 2011 at 4:19 AM, Tormod Volden <lists.tormod@gmail.com> wrote: > From: Tormod Volden <debian.tormod@gmail.com> > > Since multiple framebuffer maps are not supported any longer (commit > 41c2e75e60200a860a74b7c84a6375c105e7437f) these maps would be broken, > and they are not used by the drm anyway. > > Leave it to userspace to create one working map instead. Sorry Tormod, I'm really failing to get this far down my todo list. I'll try and get some time next week. I think I preferred the patch to make things work with current userspace. and undo the regression. Dave. > > Signed-off-by: Tormod Volden <debian.tormod@gmail.com> > --- > > The drm driver is not required to set up fb maps, right? > For instance radeon does, but its comment says "Create mappings > for registers and framebuffer so userland doesn't necessarily > have to find them." > > Tormod > > drivers/gpu/drm/savage/savage_bci.c | 13 +------------ > drivers/gpu/drm/savage/savage_drv.h | 2 -- > 2 files changed, 1 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/savage/savage_bci.c b/drivers/gpu/drm/savage/savage_bci.c > index bf5f83e..91fe7b4 100644 > --- a/drivers/gpu/drm/savage/savage_bci.c > +++ b/drivers/gpu/drm/savage/savage_bci.c > @@ -639,18 +639,7 @@ int savage_driver_firstopen(struct drm_device *dev) > if (ret) > return ret; > > - ret = drm_addmap(dev, fb_base, fb_size, _DRM_FRAME_BUFFER, > - _DRM_WRITE_COMBINING, &dev_priv->fb); > - if (ret) > - return ret; > - > - ret = drm_addmap(dev, aperture_base, SAVAGE_APERTURE_SIZE, > - _DRM_FRAME_BUFFER, _DRM_WRITE_COMBINING, > - &dev_priv->aperture); > - if (ret) > - return ret; > - > - return ret; > + return 0; > } > > /* > diff --git a/drivers/gpu/drm/savage/savage_drv.h b/drivers/gpu/drm/savage/savage_drv.h > index df2aac6..2b49b3e 100644 > --- a/drivers/gpu/drm/savage/savage_drv.h > +++ b/drivers/gpu/drm/savage/savage_drv.h > @@ -153,8 +153,6 @@ typedef struct drm_savage_private { > /* memory regions in physical memory */ > drm_local_map_t *sarea; > drm_local_map_t *mmio; > - drm_local_map_t *fb; > - drm_local_map_t *aperture; > drm_local_map_t *status; > drm_local_map_t *agp_textures; > drm_local_map_t *cmd_dma; > -- > 1.7.0.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH resend] drm/savage: Do not add framebuffer and aperture maps 2011-05-27 20:48 ` Dave Airlie @ 2011-05-27 23:44 ` Tormod Volden 2011-05-29 23:12 ` [PATCH] drm: Compare only lower 32 bits of framebuffer map offsets Tormod Volden 0 siblings, 1 reply; 12+ messages in thread From: Tormod Volden @ 2011-05-27 23:44 UTC (permalink / raw) To: Dave Airlie; +Cc: Dave Airlie, dri-devel On Fri, May 27, 2011 at 10:48 PM, Dave Airlie <airlied@gmail.com> wrote: > On Sat, May 28, 2011 at 4:19 AM, Tormod Volden <lists.tormod@gmail.com> wrote: >> From: Tormod Volden <debian.tormod@gmail.com> >> >> Since multiple framebuffer maps are not supported any longer (commit >> 41c2e75e60200a860a74b7c84a6375c105e7437f) these maps would be broken, >> and they are not used by the drm anyway. >> >> Leave it to userspace to create one working map instead. > > Sorry Tormod, I'm really failing to get this far down my todo list. > I'll try and get some > time next week. I think I preferred the patch to make things work with > current userspace. > and undo the regression. > > Dave. Thanks, I am glad to hear that. I would really prefer to not rewrite userspace, which seems rather painful. Cheers, Tormod ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm: Compare only lower 32 bits of framebuffer map offsets 2011-05-27 23:44 ` Tormod Volden @ 2011-05-29 23:12 ` Tormod Volden 2011-05-29 23:29 ` Dave Airlie 0 siblings, 1 reply; 12+ messages in thread From: Tormod Volden @ 2011-05-29 23:12 UTC (permalink / raw) To: dri-devel; +Cc: Dave Airlie From: Tormod Volden <debian.tormod@gmail.com> Drivers using multiple framebuffers got broken by commit 41c2e75e60200a860a74b7c84a6375c105e7437f which ignored the framebuffer (or register) map offset when looking for existing maps. The rationale was that the kernel-userspace ABI is fixed at a 32-bit offset, so the real offsets could not always be handed over for comparison. Instead of ignoring the offset we will compare the lower 32 bit. Drivers using multiple framebuffers should just make sure that the lower 32 bit are different. The existing drivers in question are practically limited to 32-bit systems so that should be fine for them. It is assumed that current drivers always specify a correct framebuffer map offset, even if this offset was ignored since above commit. So this patch should not change anything for drivers using only one framebuffer. Drivers needing multiple framebuffers with 64-bit map offsets will need to cook up something, for instance keeping an ID in the lower bits, which is to be aligned away when it comes to using the offset. Signed-off-by: Tormod Volden <debian.tormod@gmail.com> --- What about this idea? (Untested patch) Regards, Tormod drivers/gpu/drm/drm_bufs.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index 3e257a5..b4224eb 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -46,10 +46,11 @@ static struct drm_map_list *drm_find_matching_map(struct drm_device *dev, list_for_each_entry(entry, &dev->maplist, head) { /* * Because the kernel-userspace ABI is fixed at a 32-bit offset - * while PCI resources may live above that, we ignore the map - * offset for maps of type _DRM_FRAMEBUFFER or _DRM_REGISTERS. - * It is assumed that each driver will have only one resource of - * each type. + * while PCI resources may live above that, we only compare the + * lower 32 bits of the map offset for maps of type + * _DRM_FRAMEBUFFER or _DRM_REGISTERS. + * It is assumed that if a driver have more than one resource + * of each type, the lower 32 bits are different. */ if (!entry->map || map->type != entry->map->type || @@ -61,7 +62,9 @@ static struct drm_map_list *drm_find_matching_map(struct drm_device *dev, break; case _DRM_REGISTERS: case _DRM_FRAME_BUFFER: - return entry; + if ((entry->map->offset & 0xffffffff) == + (map->offset & 0xffffffff)) + return entry; default: /* Make gcc happy */ ; } -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: Compare only lower 32 bits of framebuffer map offsets 2011-05-29 23:12 ` [PATCH] drm: Compare only lower 32 bits of framebuffer map offsets Tormod Volden @ 2011-05-29 23:29 ` Dave Airlie 2011-05-30 19:45 ` [PATCH v2] " Tormod Volden 0 siblings, 1 reply; 12+ messages in thread From: Dave Airlie @ 2011-05-29 23:29 UTC (permalink / raw) To: Tormod Volden; +Cc: dri-devel On Mon, 2011-05-30 at 01:12 +0200, Tormod Volden wrote: > From: Tormod Volden <debian.tormod@gmail.com> > > Drivers using multiple framebuffers got broken by commit > 41c2e75e60200a860a74b7c84a6375c105e7437f which ignored the framebuffer > (or register) map offset when looking for existing maps. The rationale > was that the kernel-userspace ABI is fixed at a 32-bit offset, so the > real offsets could not always be handed over for comparison. > > Instead of ignoring the offset we will compare the lower 32 bit. Drivers > using multiple framebuffers should just make sure that the lower 32 bit > are different. The existing drivers in question are practically limited > to 32-bit systems so that should be fine for them. > > It is assumed that current drivers always specify a correct framebuffer > map offset, even if this offset was ignored since above commit. So this > patch should not change anything for drivers using only one framebuffer. > > Drivers needing multiple framebuffers with 64-bit map offsets will need > to cook up something, for instance keeping an ID in the lower bits, > which is to be aligned away when it comes to using the offset. > > Signed-off-by: Tormod Volden <debian.tormod@gmail.com> > --- > > What about this idea? (Untested patch) If you test it and it works I like it best. Simple and clear, and pretty close to what I was thinking was a good idea. As you say if someone needs this functionality in a new driver they can fix it, but really new drivers shouldn't be doing anything in this area. Dave. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] drm: Compare only lower 32 bits of framebuffer map offsets 2011-05-29 23:29 ` Dave Airlie @ 2011-05-30 19:45 ` Tormod Volden 2011-05-31 22:04 ` Tormod Volden 2011-06-10 22:59 ` Tormod Volden 0 siblings, 2 replies; 12+ messages in thread From: Tormod Volden @ 2011-05-30 19:45 UTC (permalink / raw) To: dri-devel; +Cc: Dave Airlie From: Tormod Volden <debian.tormod@gmail.com> Drivers using multiple framebuffers got broken by commit 41c2e75e60200a860a74b7c84a6375c105e7437f which ignored the framebuffer (or register) map offset when looking for existing maps. The rationale was that the kernel-userspace ABI is fixed at a 32-bit offset, so the real offsets could not always be handed over for comparison. Instead of ignoring the offset we will compare the lower 32 bit. Drivers using multiple framebuffers should just make sure that the lower 32 bit are different. The existing drivers in question are practically limited to 32-bit systems so that should be fine for them. It is assumed that current drivers always specify a correct framebuffer map offset, even if this offset was ignored since above commit. So this patch should not change anything for drivers using only one framebuffer. Drivers needing multiple framebuffers with 64-bit map offsets will need to cook up something, for instance keeping an ID in the lower bit which is to be aligned away when it comes to using the offset. All of above applies to _DRM_REGISTERS as well. Signed-off-by: Tormod Volden <debian.tormod@gmail.com> --- On Mon, May 30, 2011 at 1:29 AM, Dave Airlie wrote: > > If you test it and it works I like it best. Simple and clear, and pretty > close to what I was thinking was a good idea. > > As you say if someone needs this functionality in a new driver they can > fix it, but really new drivers shouldn't be doing anything in this area. > > Dave. Whoops, there was a less obvious fallthrough from the _DRM_SHM case above, where we do not want to compare offsets at all if it contains a lock(*). This patch has been tested on savage, and for verification also on radeon with DRI1 and DRI2. Tormod (*) It actually checks if _DRM_CONTAINS_LOCK is the /only/ flag set. I suppose this is intentional. My v2 patch does not change anything in the case of _DRM_SHM: If it contains a lock, it returns a match without comparing offsets. If no lock, it compares the full offsets. Is this because the only _DRM_SHM used by userspace is the one with a lock, so there is never a need to check a userspace-provided offset, or are those always within 32 bit so a full check is ok? drivers/gpu/drm/drm_bufs.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index 3e257a5..40ccfbc 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -46,10 +46,11 @@ static struct drm_map_list *drm_find_matching_map(struct drm_device *dev, list_for_each_entry(entry, &dev->maplist, head) { /* * Because the kernel-userspace ABI is fixed at a 32-bit offset - * while PCI resources may live above that, we ignore the map - * offset for maps of type _DRM_FRAMEBUFFER or _DRM_REGISTERS. - * It is assumed that each driver will have only one resource of - * each type. + * while PCI resources may live above that, we only compare the + * lower 32 bits of the map offset for maps of type + * _DRM_FRAMEBUFFER or _DRM_REGISTERS. + * It is assumed that if a driver have more than one resource + * of each type, the lower 32 bits are different. */ if (!entry->map || map->type != entry->map->type || @@ -59,9 +60,12 @@ static struct drm_map_list *drm_find_matching_map(struct drm_device *dev, case _DRM_SHM: if (map->flags != _DRM_CONTAINS_LOCK) break; + return entry; case _DRM_REGISTERS: case _DRM_FRAME_BUFFER: - return entry; + if ((entry->map->offset & 0xffffffff) == + (map->offset & 0xffffffff)) + return entry; default: /* Make gcc happy */ ; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] drm: Compare only lower 32 bits of framebuffer map offsets 2011-05-30 19:45 ` [PATCH v2] " Tormod Volden @ 2011-05-31 22:04 ` Tormod Volden 2011-06-10 22:59 ` Tormod Volden 1 sibling, 0 replies; 12+ messages in thread From: Tormod Volden @ 2011-05-31 22:04 UTC (permalink / raw) To: dri-devel; +Cc: Dave Airlie On Mon, May 30, 2011 at 9:45 PM, Tormod Volden wrote: > (*) It actually checks if _DRM_CONTAINS_LOCK is the /only/ flag set. I > suppose this is intentional. My v2 patch does not change anything in the > case of _DRM_SHM: If it contains a lock, it returns a match without > comparing offsets. If no lock, it compares the full offsets. Is this > because the only _DRM_SHM used by userspace is the one with a lock, so > there is never a need to check a userspace-provided offset, or are those > always within 32 bit so a full check is ok? After googling up an old "DRM map design" thread from 2005 (http://thread.gmane.org/gmane.comp.video.dri.devel/19545/focus=19689), and http://dri.freedesktop.org/wiki/DrmMapHandling I think I can answer this to some extent myself: For _DRM_SHM it is not an offset that is passed from/to userspace, but a handle, which I guess is kept within 32 bit. Although this is not fully clear to me after browsing through drm_addmap_core(), where the offset or handle is the result of vmalloc_user(). Tormod ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] drm: Compare only lower 32 bits of framebuffer map offsets 2011-05-30 19:45 ` [PATCH v2] " Tormod Volden 2011-05-31 22:04 ` Tormod Volden @ 2011-06-10 22:59 ` Tormod Volden 1 sibling, 0 replies; 12+ messages in thread From: Tormod Volden @ 2011-06-10 22:59 UTC (permalink / raw) To: Dave Airlie; +Cc: dri-devel On Mon, May 30, 2011 at 9:45 PM, Tormod Volden wrote: > From: Tormod Volden <debian.tormod@gmail.com> > > Drivers using multiple framebuffers got broken by commit > 41c2e75e60200a860a74b7c84a6375c105e7437f which ignored the framebuffer > (or register) map offset when looking for existing maps. The rationale > was that the kernel-userspace ABI is fixed at a 32-bit offset, so the > real offsets could not always be handed over for comparison. > > Instead of ignoring the offset we will compare the lower 32 bit. Drivers > using multiple framebuffers should just make sure that the lower 32 bit > are different. The existing drivers in question are practically limited > to 32-bit systems so that should be fine for them. > > It is assumed that current drivers always specify a correct framebuffer > map offset, even if this offset was ignored since above commit. So this > patch should not change anything for drivers using only one framebuffer. > > Drivers needing multiple framebuffers with 64-bit map offsets will need > to cook up something, for instance keeping an ID in the lower bit which > is to be aligned away when it comes to using the offset. > > All of above applies to _DRM_REGISTERS as well. > > Signed-off-by: Tormod Volden <debian.tormod@gmail.com> > --- > > > On Mon, May 30, 2011 at 1:29 AM, Dave Airlie wrote: >> >> If you test it and it works I like it best. Simple and clear, and pretty >> close to what I was thinking was a good idea. >> >> As you say if someone needs this functionality in a new driver they can >> fix it, but really new drivers shouldn't be doing anything in this area. >> >> Dave. > > > Whoops, there was a less obvious fallthrough from the _DRM_SHM case above, > where we do not want to compare offsets at all if it contains a lock(*). > > This patch has been tested on savage, and for verification also on radeon > with DRI1 and DRI2. > > Tormod Hi Dave, Is this good to go now and I can expect to see it in e.g. drm-next or are there any issues or doubts? Cheers, Tormod ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-06-10 22:59 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-04-25 21:54 multiple framebuffer drm maps Tormod Volden 2011-05-03 21:20 ` Tormod Volden 2011-05-03 21:54 ` Tormod Volden 2011-05-22 19:27 ` [PATCH] drm/savage: Do not add framebuffer and aperture maps Tormod Volden 2011-05-27 18:19 ` [PATCH resend] " Tormod Volden 2011-05-27 20:48 ` Dave Airlie 2011-05-27 23:44 ` Tormod Volden 2011-05-29 23:12 ` [PATCH] drm: Compare only lower 32 bits of framebuffer map offsets Tormod Volden 2011-05-29 23:29 ` Dave Airlie 2011-05-30 19:45 ` [PATCH v2] " Tormod Volden 2011-05-31 22:04 ` Tormod Volden 2011-06-10 22:59 ` Tormod Volden
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.