lkml.org 
[lkml]   [2021]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] drm: omapdrm: Export correct scatterlist for TILER backed BOs
From
Date

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

\
 
 \ /
  Last update: 2021-11-15 15:10    [W:0.044 / U:1.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site