Messages in this thread | | | Subject | Re: [PATCH v2] drm: omapdrm: Export correct scatterlist for TILER backed BOs | From | Ivaylo Dimitrov <> | Date | Mon, 15 Nov 2021 16:05:18 +0200 |
| |
Hi,
On 15.11.21 г. 12:37 ч., Tomi Valkeinen wrote: > On 15/11/2021 11:23, Matthijs van Duin wrote: >> On Mon, Nov 15, 2021 at 10:42:41AM +0200, Tomi Valkeinen wrote: >>> A BO's memory via the TILER memory is >>> contiguous, although with consistent gaps of >>> memory that should not be accessed. >> >> But pretending that these "gaps" are part of the buffer is a security >> vulnerability, since that memory which "should not be accessed" may >> belong to different security contexts, and exporting the entire >> contiguous region covering the buffer allows untrusted contexts (e.g. >> userspace) to access this memory. > > Yes, I fully agree. I wasn't criticizing the patch, just wanted to > highlight the unmentioned aspects. > >>> IPs that might use TILER >>> backed BOs only support contiguous memory. >>> >>> This means that the drivers for such IPs cannot >>> use the BOs exported like you do in this patch. >>> I believe the drivers could be improved by >>> writing a helper function which studies the >>> sg_table and concludes that it's actually >>> contiguous. >> >> That indeed sounds like the proper solution for such importers, rather >> than making the exporter lie about the buffer bounds to work around >> limitations of these importers. > > The annoying thing with this solution is that we need to add extra code > to all the drivers that want to import TILER buffers, even if the > drivers shouldn't know anything about TILER. > > Then again, the code is not really TILER or OMAP specific, and any IP on > any platform that only supports contiguous buffers, but supports stride, > could import such buffers. Which hints that maybe the code should be > somewhere in the framework, not in the driver. In practice it may be > better to just swallow the annoyance, add the code to the drivers and be > done with it =). > >>> Did you look at the userspace mmap of TILER >>> buffers? I wonder if that goes correctly or not. >>> Isn't memory to userspace mapped per page, and >>> lengths of the TILER lines are not page aligned? >> >> Mapping to userspace uses an ugly hack whereby small slabs of the >> buffer (4096x64 (8bpp), 2048x32 (16bpp), or 1024x32 (32bpp) pixels) are >> dynamically mapped to dedicated page-aligned regions of the TILER >> virtual space. For each of the three bitdepths only two such slabs can >> be mapped into userspace at any given time (on the entire system), so >> using this mechanism to render graphics from userspace can easily cause >> hundreds if not thousands of page faults per second. > > Ah, right, yes, now I remember. The userspace mmap of TILER buffers > isn't very usable. > >> The alternative (used e.g. in the pyra kernel) is to force all TILER >> buffers to be page-aligned, at the cost of wasting some TILER space. >> This will presumably also be necessary to allow SGX to import these >> buffers since its MMU can obviously also not map data which is not >> page-aligned, same for any other importer which uses an MMU to enforce >> memory security (rather than being trusted to simply refrain from >> accessing data outside the declared bounds). >> >> Ideally such page-alignment should only be applied to buffers which are >> intended to be consumed by importers which require this, though it's not >> clear how that might be accomplished. > > Do you mean that each TILER _line_ should be page aligned and the length > should be page divisible? Doesn't that cause quite a lot of wasted > space? Although that, of course, depends on the use. If the primary use > case is allocating a few full screen display buffers, perhaps the waste > is negligible. >
Yes, I think this is the idea, otherwise no MMU can be set correctly.
> In any case, I'm fine with that change, too, as it helps making TILER > usable. >
That's great, will send a patch ASAP.
> And while speaking about usable, if I recall right, the > omap_bo_new_tiled() is pretty annoying to use. You need to give the > width and OMAP_BO_TILED_x flag as a parameter, and if I recall right, > it's all but obvious how those need to be set for, e.g. NV12. But it > works so perhaps better to keep it as it is... >
To me the whole omap_bo_xxx stuff should go away and be replaced by gbm_bo_xxx stuff. The only issue there is with TILER BOs, but I think we'll be able to get away with that with a little abuse of GBM_BO_XXX flags (see the other mail)
> There was also some particular YUV format with some rotations that I > never got working, even after discussing with TI DSS HW guys. > > Tomi
Thanks, Ivo
| |