| Message ID | 1306784743-2430-1-git-send-email-lists.tormod@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 */ ; }