From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@arm.linux.org.uk (Russell King - ARM Linux) Date: Wed, 12 Jun 2013 18:05:12 +0100 Subject: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver In-Reply-To: <20130612164914.GT18614@n2100.arm.linux.org.uk> References: <20130610211516.GZ18614@n2100.arm.linux.org.uk> <20130610225607.GE18614@n2100.arm.linux.org.uk> <20130610233611.GK18614@n2100.arm.linux.org.uk> <20130612134845.GQ18614@n2100.arm.linux.org.uk> <20130612164914.GT18614@n2100.arm.linux.org.uk> Message-ID: <20130612170512.GU18614@n2100.arm.linux.org.uk> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Jun 12, 2013 at 05:49:14PM +0100, Russell King - ARM Linux wrote: > On Wed, Jun 12, 2013 at 09:56:22AM -0400, Rob Clark wrote: > > On Wed, Jun 12, 2013 at 9:48 AM, Russell King - ARM Linux > > wrote: > > > And having thought about this driver, DRM some more, I'm now of the > > > opinion that DRM is not suitable for driving hardware where the GPU is > > > an entirely separate IP block from the display side. > > > > > > DRM is modelled after the PC setup where your "graphics card" does > > > everything - it has the GPU, display and connectors all integrated > > > together. This is not the case on embedded SoCs, which can be a > > > collection of different IPs all integrated together. > > > > actually it isn't even the case on desktop/laptop anymore, where you > > can have one gpu with scanout and a second one without (or just with > > display controller not hooked up to anything, etc, etc) > > > > That is the point of dmabuf and the upcoming fence/reservation stuff. > > Okay, but dmabuf really needs to be fixed, because as it stands this API > is really quite broken wrt the DMA API. dma_map_sg() is (a) not supposed > to have its return value ignored - mappings can fail, and (b) the returned > number indicates how many entries are valid for the _mapped_ version of > the scatterlist. > > Both these points are important if your DMA API implementation uses an > IOMMU, which may coalesce the scatterlist array when creating mappings - > much as described in Documentation/DMA-API.txt and > Documentation/DMA-API-HOWTO.txt. > > That is not all DRMs fault - (a) is attributable to DRM's prime > implementation: > > sgt = obj->dev->driver->gem_prime_get_sg_table(obj); > > if (!IS_ERR_OR_NULL(sgt)) > dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir); > > and quite why it does the dma_map_sg() beneath the struct_mutex is > beyond me - if the array of pages isn't safe without the mutex being > held, then it isn't safe after the dma_map_sg() operation has completed > and the mutex has been released. > > However, (b) is more a problem for dmabuf (which I just rather aptly > mistyped as dmabug) because either dmabuf's .map_dma_buf method needs > to return the value from dma_map_sg(), or it needs to stop requiring > this of the .map_dma_buf method and have it done by the caller of > this method so the caller can have access to that returned value. > > Added Sumit Semwal to this email for the dmabuf issue. > > Thankfully, this being correct isn't a requirement for me, but it's > something which _should_ be fixed. Okay, so Sumit Semwal has been a victim of TI's cuts, so don't expect dma_buf to get sorted by the original author... Now we come to this: drivers/gpu/drm/exynos/exynos_drm_dmabuf.c: return dma_buf_export(exynos_gem_obj, &exynos_dmabuf_ops, drivers/gpu/drm/exynos/exynos_drm_dmabuf.c- exynos_gem_obj->base.size, flags); drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c: return dma_buf_export(obj, &omap_dmabuf_ops, obj->size, flags); drivers/gpu/drm/drm_prime.c: return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size, drivers/gpu/drm/drm_prime.c- 0600); drivers/gpu/drm/i915/i915_gem_dmabuf.c: return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, flags); Of the three implementations which don't use the generic version, they all pass 'flags' to dma_buf_export. drm_prime.c doesn't, it passes a fixed file mode. What's the correct version, or is flags | 0600 actually the right one (as flags only contains O_CLOEXEC)? From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver Date: Wed, 12 Jun 2013 18:05:12 +0100 Message-ID: <20130612170512.GU18614@n2100.arm.linux.org.uk> References: <20130610211516.GZ18614@n2100.arm.linux.org.uk> <20130610225607.GE18614@n2100.arm.linux.org.uk> <20130610233611.GK18614@n2100.arm.linux.org.uk> <20130612134845.GQ18614@n2100.arm.linux.org.uk> <20130612164914.GT18614@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20130612164914.GT18614@n2100.arm.linux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Rob Clark Cc: dri-devel@lists.freedesktop.org, Dave Airlie , Jason Cooper , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth List-Id: dri-devel@lists.freedesktop.org On Wed, Jun 12, 2013 at 05:49:14PM +0100, Russell King - ARM Linux wrote: > On Wed, Jun 12, 2013 at 09:56:22AM -0400, Rob Clark wrote: > > On Wed, Jun 12, 2013 at 9:48 AM, Russell King - ARM Linux > > wrote: > > > And having thought about this driver, DRM some more, I'm now of the > > > opinion that DRM is not suitable for driving hardware where the GPU is > > > an entirely separate IP block from the display side. > > > > > > DRM is modelled after the PC setup where your "graphics card" does > > > everything - it has the GPU, display and connectors all integrated > > > together. This is not the case on embedded SoCs, which can be a > > > collection of different IPs all integrated together. > > > > actually it isn't even the case on desktop/laptop anymore, where you > > can have one gpu with scanout and a second one without (or just with > > display controller not hooked up to anything, etc, etc) > > > > That is the point of dmabuf and the upcoming fence/reservation stuff. > > Okay, but dmabuf really needs to be fixed, because as it stands this API > is really quite broken wrt the DMA API. dma_map_sg() is (a) not supposed > to have its return value ignored - mappings can fail, and (b) the returned > number indicates how many entries are valid for the _mapped_ version of > the scatterlist. > > Both these points are important if your DMA API implementation uses an > IOMMU, which may coalesce the scatterlist array when creating mappings - > much as described in Documentation/DMA-API.txt and > Documentation/DMA-API-HOWTO.txt. > > That is not all DRMs fault - (a) is attributable to DRM's prime > implementation: > > sgt = obj->dev->driver->gem_prime_get_sg_table(obj); > > if (!IS_ERR_OR_NULL(sgt)) > dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir); > > and quite why it does the dma_map_sg() beneath the struct_mutex is > beyond me - if the array of pages isn't safe without the mutex being > held, then it isn't safe after the dma_map_sg() operation has completed > and the mutex has been released. > > However, (b) is more a problem for dmabuf (which I just rather aptly > mistyped as dmabug) because either dmabuf's .map_dma_buf method needs > to return the value from dma_map_sg(), or it needs to stop requiring > this of the .map_dma_buf method and have it done by the caller of > this method so the caller can have access to that returned value. > > Added Sumit Semwal to this email for the dmabuf issue. > > Thankfully, this being correct isn't a requirement for me, but it's > something which _should_ be fixed. Okay, so Sumit Semwal has been a victim of TI's cuts, so don't expect dma_buf to get sorted by the original author... Now we come to this: drivers/gpu/drm/exynos/exynos_drm_dmabuf.c: return dma_buf_export(exynos_gem_obj, &exynos_dmabuf_ops, drivers/gpu/drm/exynos/exynos_drm_dmabuf.c- exynos_gem_obj->base.size, flags); drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c: return dma_buf_export(obj, &omap_dmabuf_ops, obj->size, flags); drivers/gpu/drm/drm_prime.c: return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size, drivers/gpu/drm/drm_prime.c- 0600); drivers/gpu/drm/i915/i915_gem_dmabuf.c: return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, flags); Of the three implementations which don't use the generic version, they all pass 'flags' to dma_buf_export. drm_prime.c doesn't, it passes a fixed file mode. What's the correct version, or is flags | 0600 actually the right one (as flags only contains O_CLOEXEC)?