Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Am 07.03.2018 um 09:42 schrieb Michel Dänzer:
> On 2018-03-06 07:23 PM, Christian König wrote:
>> Am 06.03.2018 um 19:15 schrieb Michel Dänzer:
>>> On 2018-03-06 07:02 PM, Christian König wrote:
>>>> Am 06.03.2018 um 18:51 schrieb Michel Dänzer:
>>>>> On 2018-03-06 06:44 PM, Christian König wrote:
>>>>>> Am 06.03.2018 um 18:22 schrieb Li, Samuel:
>>>>>>>> addition to that I agree with Michel that the module parameter is
>>>>>>>> overkill.
>>>>>>> That I already explained. Currently SG display feature needs to
>>>>>>> provide options for all kinds of use cases. All amd drivers so far
>>>>>>> provides options, and the default configuration is actually to
>>>>>>> allocate everything from GTT when possible.
>>>>>> That isn't job of the kernel to have this parameter. Saying that we
>>>>>> should probably add a DDX and/or environment option to control that.
>>>>> Why would we need that? It's the kernel driver's job to handle this as
>>>>> needed.
>>>> Buffer placement is specified by the DDX/Mesa, when we override that in
>>>> the kernel we just force unnecessary buffer moves.
>>> Userspace just needs a way to know which domains can/should be used for
>>> scanout buffers, e.g. via an info ioctl.
>> Yeah, but why shouldn't they then make the decision where to place it as
>> well?
>>
>> See when we enforce a certain placement in the kernel we will just get
>> an unnecessary buffer move with old user space components.
>>
>> In other words the kernel just needs to advertise that the hw/sw
>> combination allows scanout from GTT as well and then an updated
>> userspace can actually make use of that placement or decide to not use
>> it in certain situations.
> I think we'll need to always pin BOs to GTT for scanout in some cases,
> because some hardware has issues when transitioning between scanout from
> VRAM and GTT.
>
> How about:
>
> Userspace can query from the kernel which domain(s) can be used for
> scanout: only VRAM / VRAM or GTT / only GTT. Userspace sets the allowed
> domains correspondingly for scanout BOs.

Well that sounds close to what I had in mind as well, we just can't 
specify only GTT when we want to keep backward compatibility with 
existing user space.

>
> If you can think of a scenario where this won't work, please
> specifically describe it and how you propose addressing it.
>
>
>> E.g. the last time I tested it placing things into GTT still resulted
>> in quite a performance penalty for rendering.
> FWIW, I think the penalty is most likely IOMMU related. Last time I
> tested, I couldn't measure a big difference with IOMMU disabled.

No, the penalty I'm talking about came from the ping/pong we did with 
the scanout buffers.

See when I tested this the DDX and Mesa where unmodified, so both still 
assumed VRAM as placement for scanout BOs, but the kernel forced scanout 
BOs into GTT for testing.

So what happened was that on scanout we moved the VRAM BO to GTT and 
after unpinning it on the first command submission which used the BO we 
moved it back to VRAM again.

So as long as we don't want a severe performance penalty when you 
combine old userspace with new kernel using a kernel parameter to force 
scanout from GTT is not possible.

Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx