From patchwork Mon May 30 19:45:43 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tormod Volden X-Patchwork-Id: 830832 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p4UJjuMn022305 for ; Mon, 30 May 2011 19:46:17 GMT Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8C2119F754 for ; Mon, 30 May 2011 12:45:55 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ww0-f43.google.com (mail-ww0-f43.google.com [74.125.82.43]) by gabe.freedesktop.org (Postfix) with ESMTP id 01D2D9E74D for ; Mon, 30 May 2011 12:45:46 -0700 (PDT) Received: by wwb17 with SMTP id 17so3465237wwb.12 for ; Mon, 30 May 2011 12:45:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:from:to:cc:subject:date:message-id:x-mailer :in-reply-to:references; bh=9IjSOAr3PmP5AD0Slv5CJT87mcDZ7IFY8HMjC2MqrDI=; b=JTbCH5bXCOnzqtk6MmyTRwCX+NCaLxbWO4WFaWDu9HPRfYJV6b/YQOTG8oA6CxSziT fnhphoEcKmBO5eZJV25uksLNpCgFZ0KMRGuBDAQ5iuiN4JVaIS/pSNuj2tw/t1N484ic sqdAH+dBJN7EHr1JDysmkOc/XnHxavXn8gWqU= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references; b=jndFOiCw1ZS6gJEtNBrZhKj2uAKSdU/OEyMPGuSacY16wpfXkGQrh6PLrjYIkzm0Ag MMkBwyZL7v2f2/++Q9v8AdjWhfe/evQSrc9eV0GhNL6CgbYzG64L2HGfvz5A1hYxWlAF MkZLeBVKa6gz0W8nZ+bl/Rzk8/lXzOumoxndU= Received: by 10.216.233.211 with SMTP id p61mr452750weq.107.1306784746015; Mon, 30 May 2011 12:45:46 -0700 (PDT) Received: from localhost.localdomain (80-219-113-251.dclient.hispeed.ch [80.219.113.251]) by mx.google.com with ESMTPS id h43sm865974wes.11.2011.05.30.12.45.44 (version=SSLv3 cipher=OTHER); Mon, 30 May 2011 12:45:45 -0700 (PDT) From: Tormod Volden To: dri-devel@lists.freedesktop.org Subject: [PATCH v2] drm: Compare only lower 32 bits of framebuffer map offsets Date: Mon, 30 May 2011 21:45:43 +0200 Message-Id: <1306784743-2430-1-git-send-email-lists.tormod@gmail.com> X-Mailer: git-send-email 1.7.0.4 In-Reply-To: <1306711744.21613.14.camel@clockmaker-el6> References: <1306711744.21613.14.camel@clockmaker-el6> Cc: Dave Airlie X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Mon, 30 May 2011 19:46:25 +0000 (UTC) From: Tormod Volden 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 --- 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 */ ; }