On 10/25/2016 11:57 AM, Tobias Jakobi wrote: > Hello Shuah, > > > Shuah Khan wrote: >> On 10/19/2016 04:27 PM, Shuah Khan wrote: >>> On 10/19/2016 08:16 AM, Inki Dae wrote: >>>> Hi Shuah, >>>> >>>> 2016-10-13 8:11 GMT+09:00 Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>: >>>>> Hi Inki, >>>>> >>>>> On 08/15/2016 10:40 PM, Inki Dae wrote: >>>>> >>>>>>> >>>>>>> okay the very first commit that added IOMMU support >>>>>>> introduced the code that rejects non-contig gem memory >>>>>>> type without IOMMU. >>>>>>> >>>>>>> commit 0519f9a12d0113caab78980c48a7902d2bd40c2c >>>>>>> Author: Inki Dae <inki.dae@xxxxxxxxxxx> >>>>>>> Date: Sat Oct 20 07:53:42 2012 -0700 >>>>>>> >>>>>>> drm/exynos: add iommu support for exynos drm framework >>>>>>> >>>>> >>>>> I haven't given up on this yet. I am still seeing the following failure: >>>>> >>>>> Additional debug messages I added: >>>>> [ 15.287403] exynos_drm_gem_create_ioctl() 1 >>>>> [ 15.287419] exynos_drm_gem_create() flags 1 >>>>> >>>>> [ 15.311511] [drm:exynos_drm_framebuffer_init] *ERROR* Non-contiguous GEM memory is not supported. >>>>> >>>>> Additional debug message I added: >>>>> [ 15.318981] [drm:exynos_user_fb_create] *ERROR* failed to initialize framebuffer >>>>> >>>>> This is what happens: >>>>> >>>>> 1. exynos_drm_gem_create_ioctl() gets called with EXYNOS_BO_NONCONTIG request >>>>> 2. exynos_drm_gem_create(0 goes ahead and creates the GEM buffers >>>>> 3. exynos_user_fb_create() tries to associate GEM to fb and fails during >>>>> check_fb_gem_memory_type() >>>>> >>>>> At this point, there is no recovery and lightdm fails >>>>> >>>>> xf86-video-armsoc/src/drmmode_exynos/drmmode_exynos.c assumes contiguous >>>>> allocations are not supported in some exynos drm versions: The following >>>>> commit introduced this change: >>>>> >>>>> https://git.linaro.org/arm/xorg/driver/xf86-video-armsoc.git/commitdiff/3be1f6273441fe95dd442f44064387322e16b7e9 >>>>> >>>>> excerpts from the diff:- if (create_gem->buf_type == ARMSOC_BO_SCANOUT) >>>>> - create_exynos.flags = EXYNOS_BO_CONTIG; >>>>> - else >>>>> - create_exynos.flags = EXYNOS_BO_NONCONTIG; >>>>> + >>>>> + /* Contiguous allocations are not supported in some exynos drm versions. >>>>> + * When they are supported all allocations are effectively contiguous >>>>> + * anyway, so for simplicity we always request non contiguous buffers. >>>>> + */ >>>>> + create_exynos.flags = EXYNOS_BO_NONCONTIG; >>>>> >>>> >>>> Above comment, "Contiguous allocations are not supported in some >>>> exynos drm versions.", seems wrong assumption. >>>> The root cause, contiguous allocation is not supported, would be that >>>> they used Linux kernel which didn't have CMA region enough - as >>>> default 16MB, or didn't declare CMA region enough for the DMA device. >>>> So I think they should not force to flag EXYNOS_BO_NONCONTIG and they >>>> should manage the error case if the allocation failed. >>> >>> This assumption doesn't sound correct and forcing NONCONTIG isn't right >>> either. >>> >>>> >>>>> There might have been logic on exynos_drm that forced Contig when it coudn't >>>>> support NONCONTIG. At least, that is what this comment suggests. This assumption >>>>> doesn't appear to be a good one and not sure if this change was made to fix a bug. >>>>> >>>>> After the IOMMU support, this assumption is no longer true. Hence, with IOMMU >>>>> support, latest kernels have a mismatch with the installed xf86-video-armsoc >>>>> >>>>> This is what I am running into. This leads to the following question: >>>>> >>>>> 1. How do we ensure exynos_drm kernel changes don't break user-space >>>>> specifically xf86-video-armsoc >>>>> 2. This seems to have gone undetected for a while. I see a change in >>>>> exynos_drm_gem_dumb_create() that is probably addressing this type >>>>> of breakage. Commit 122beea84bb90236b1ae545f08267af58591c21b adds >>>>> handling for IOMMU NONCONTIG case. >>>> >>>> Seems this patch has a problem. This patch forces to flag NONCONTIG if >>>> iommu is enabled. The flag should be depend on the argument from >>>> user-space. >>>> I.e., if user-space requested a gem allocation with CONTIG flag, then >>>> Exynos drm driver should allocate contiguous memory even though iommu >>>> is enabled. >>>> >>>>> >>>>> Anyway, I am interested in getting the exynos_drm kernel side code >>>>> and xf86-video-armsoc in sync to resolve the issue. >>>>> >>>>> Could you recommend a going forward plan? >>>> >>>> My opinion are, >>>> >>>> 1. Do not force to flag EXYNOS_BO_NONCONTIG at xf86-video-armsoc >> >> Okay more on this. I fixed xf86-video-armso to ask for EXYNOS_BO_CONTIG >> for ARMSOC_BO_SCANOUT and EXYNOS_BO_NONCONTIG in all other cases. >> >> Revert of the commit: 3be1f6273441fe95dd442f44064387322e16b7e9 >> >> With this change, now display manager starts just fine. However, it turns >> out xf86-video-armsoc is obsoleted in favor of xf86-video-modesetting. The >> last update to xf86-video-armsoc git was 3 years ago. > IIRC xf86-video-armsoc was created to facilitate integration with the > proprietary Mali userspace blob. I don't think that can be done with the > modesetting DDX. > > > >> I am not sure where to send the fix and doesn't look like it is being >> maintained actively. I can pursue it further and try to get this into >> xf86-video-armsoc provided I can find the maintainer for this seemingly >> inactive project. > I was wondering if anyone has actually complained about this issue? I sent email to the last person that modified the xf86-video-armsoc. I will pursue fix to this. > > > >> I brought in the latest xf86-video-modesetting bits from >> >> https://cgit.freedesktop.org/xorg/driver/xf86-video-modesetting >> >> I removed xf86-video-armsoc and installed xf86-video-modesetting and that >> worked just fine. xf86-video-modesetting uses dumb_create interface instead >> of DRM_IOCTL_EXYNOS_GEM_CREATE. >> >> dumb_create interface eliminates the need for DRM_IOCTL_EXYNOS_GEM_CREATE >> in userspace. libdrm-2.4.71 does call DRM_IOCTL_EXYNOS_GEM_CREATE. >> >> The question is do we still need to continue to support the custom gem >> create interface DRM_IOCTL_EXYNOS_GEM_CREATE? > I'd say yes. With just the dumb_create interface it is not possible to > change the type of buffer you get. And if you want to support any kind > of hw acceleration in the DDX, you probably want to control at least the > caching behaviour of the buffer. Right. Okay that makes sense. > > > >> Some drm drivers seem to support it and some don't. > Not sure I understand this. I don't think any other DRM driver except > for exynos supports this ioctl. But all drivers have their own ioctls to > request memory (DRM_IOCTL_ETNAVIV_GEM_NEW, DRM_IOCTL_VC4_CREATE_BO, etc.) > > > >> Unfortunately, if userspace requests noncontig >> for scanout, we will continue to see problems with display manager when >> iommu is disabled. dumb create hides all of that, are there good reasons >> to continue to support it in exynos drm? > In addition to the stuff I said above, you can use it for render nodes. > That doesn't work for dumb_create. > > >> Exposing CONTIG and NONCONTIG to userspace appears to be causing problems >> when exynos drm determines it can't support non-contig GEM buffers in fb >> init after userspace allocates them. > Might be missing something here, but this whole thing just looks like a > bug in xf86-video-armsoc. No matter from which perspective you look, the > commit that changed all allocations to noncontig was plain wrong. > > My guess: This was done to paper over some bug in some vendor kernel for > a product using an Exynos SoC. > Yeah. The right fix in this case is fixing xf86-video-armsoc as you explained dump_create is too generic for some use-cases. thanks, -- Shuah -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |