[PATCH v2 1/1] drm/amdgpu: Enable scatter gather display support
Samuel Li
samuel.li at amd.com
Thu Apr 12 20:42:12 UTC 2018
Hi Christian,
If you have any good proposal, please describe here.
Otherwise kindly avoid saying anything based on your emotion.
Regards,
Sam
On 2018-04-12 04:13 PM, Christian König wrote:
> Am 12.04.2018 um 22:01 schrieb Samuel Li:
>> The 4th proposal :)
>>
>>> In other words add something like the following:
>>>
>>> if (domain & AMDGPU_GEM_DOMAIN_GTT && bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT)
>>> domain = AMDGPU_GEM_DOMAIN_GTT;
>>>
>>> That should be everything we need here.
>> This is basically against the idea of Marek's change: in Mesa, both GTT/VRAM are allowed; but now in your kernel change, all buffers uses GTT only(not limited to display buffer now).
>> To compare, current patch still seems better, since it only circumscribes display buffer.
>
> What I suggested here is for pinning preference, that only affects BOs used for scanout and it also only affects them while they are scanned out.
>
> Please implement as advised or otherwise we need to assign the work to somebody else.
>
> Thanks,
> Christian.
>
>>
>> Sam
>>
>>
>> On 2018-04-12 02:47 PM, Christian König wrote:
>>> Patch #1: Avoid the hardware bug!
>>>
>>> E.g. even when we avoid different placements it would be good to have a patch which turns off immediate flipping when switching from VRAM to GTT.
>>>
>>> That is as safety net and to document that we need to avoid this condition on the hardware.
>>>
>>> Patch #2: Go into amdgpu_bo_pin_restricted() and change the pinning preference.
>>>
>>> In other words add something like the following:
>>>
>>> if (domain & AMDGPU_GEM_DOMAIN_GTT && bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT)
>>> domain = AMDGPU_GEM_DOMAIN_GTT;
>>>
>>> That should be everything we need here.
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 12.04.2018 um 20:07 schrieb Samuel Li:
>>>> Please clarify, Christian. How would you like it to be implemented?
>>>>
>>>> Sam
>>>>
>>>>
>>>> On 2018-04-12 02:00 PM, Christian König wrote:
>>>>>> 1) Turn off immediate mode when flipping between VRAM/GTT.
>>>>> That must be implemented independently.
>>>>>
>>>>> See working around the hardware bug should be a different patch than implementing a placement policy.
>>>>>
>>>>>> As per discussion, the 3rd one, which is the current patch, seems the best so far.
>>>>> And I can only repeat myself. Alex and I are the maintainers of the kernel module, so we are the one who decide on how to implement things here.
>>>>>
>>>>> And we both noted that this approach of overriding user space decisions is not a good design.
>>>>>
>>>>> The placement policy I suggest by preferring GTT over VRAM on APUs should be trivial to implement and as far as I can see avoids all negative side effects.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>> Am 12.04.2018 um 19:21 schrieb Samuel Li:
>>>>>>> The point is this kernel change now needs to be reworked and adapted to what Mesa is doing.
>>>>>> Three options have been brought up for kernel,
>>>>>> 1) Turn off immediate mode when flipping between VRAM/GTT.
>>>>>> 2) Check the domain of the current fb and then adjust the new one before pinning it.
>>>>>> 3) Use only VRAM or GTT depending on a threshhold.
>>>>>>
>>>>>> As per discussion, the 3rd one, which is the current patch, seems the best so far.
>>>>>>
>>>>>> Regards,
>>>>>> Samuel Li
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2018-04-12 01:03 PM, Christian König wrote:
>>>>>>>> Can you be more specific, Christian? Mesa has this, I don't think it needs anything else:
>>>>>>> Completely agree, that's what I suggested to implement.
>>>>>>>
>>>>>>> The point is this kernel change now needs to be reworked and adapted to what Mesa is doing.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>> Am 12.04.2018 um 18:40 schrieb Marek Olšák:
>>>>>>>> Can you be more specific, Christian? Mesa has this, I don't think it needs anything else:
>>>>>>>> https://cgit.freedesktop.org/mesa/mesa/commit/?id=7d2079908d9ef05ec3f35b7078833e57846cab5b
>>>>>>>>
>>>>>>>> Marek
>>>>>>>>
>>>>>>>> On Wed, Mar 28, 2018 at 3:46 AM, Christian König <ckoenig.leichtzumerken at gmail.com <mailto:ckoenig.leichtzumerken at gmail.com>> wrote:
>>>>>>>>
>>>>>>>> Am 28.03.2018 um 00:22 schrieb Samuel Li:
>>>>>>>>
>>>>>>>> It's auto by default. For CZ/ST, auto setting enables sg display
>>>>>>>> when vram size is small; otherwise still uses vram.
>>>>>>>> This patch fixed some potential hang issue introduced by change
>>>>>>>> "allow framebuffer in GART memory as well" due to CZ/ST hardware
>>>>>>>> limitation.
>>>>>>>>
>>>>>>>>
>>>>>>>> Well that is still a NAK.
>>>>>>>>
>>>>>>>> As discussed now multiple times please implement the necessary
>>>>>>>> changes in Mesa.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> v2: Change default setting to auto, also some misc changes.
>>>>>>>> Signed-off-by: Samuel Li <Samuel.Li at amd.com
>>>>>>>> <mailto:Samuel.Li at amd.com>>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 10 ++++++++--
>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_display.h | 2 ++
>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 ++++
>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 2 ++
>>>>>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
>>>>>>>> 6 files changed, 19 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>>> index a7e2229..c942362 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>>> @@ -129,6 +129,7 @@ extern int amdgpu_lbpw;
>>>>>>>> extern int amdgpu_compute_multipipe;
>>>>>>>> extern int amdgpu_gpu_recovery;
>>>>>>>> extern int amdgpu_emu_mode;
>>>>>>>> +extern int amdgpu_sg_display;
>>>>>>>> #ifdef CONFIG_DRM_AMDGPU_SI
>>>>>>>> extern int amdgpu_si_support;
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>>> index 5495b29..1e7b950 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>>> @@ -513,8 +513,14 @@ uint32_t
>>>>>>>> amdgpu_display_framebuffer_domains(struct amdgpu_device *adev)
>>>>>>>> #if defined(CONFIG_DRM_AMD_DC)
>>>>>>>> if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type
>>>>>>>> < CHIP_RAVEN &&
>>>>>>>> adev->flags & AMD_IS_APU &&
>>>>>>>> - amdgpu_device_asic_has_dc_support(adev->asic_type))
>>>>>>>> - domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>>>> + amdgpu_device_asic_has_dc_support(adev->asic_type)) {
>>>>>>>> + if (amdgpu_sg_display == 1)
>>>>>>>> + domain = AMDGPU_GEM_DOMAIN_GTT;
>>>>>>>> + else if (amdgpu_sg_display == -1) {
>>>>>>>> + if (adev->gmc.real_vram_size <
>>>>>>>> AMDGPU_SG_THRESHOLD)
>>>>>>>> + domain = AMDGPU_GEM_DOMAIN_GTT;
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>> #endif
>>>>>>>> return domain;
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>>>>>>>> index 2b11d80..2b25393 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>>>>>>>> @@ -23,6 +23,8 @@
>>>>>>>> #ifndef __AMDGPU_DISPLAY_H__
>>>>>>>> #define __AMDGPU_DISPLAY_H__
>>>>>>>> +#define AMDGPU_SG_THRESHOLD (256*1024*1024)
>>>>>>>> +
>>>>>>>> uint32_t amdgpu_display_framebuffer_domains(struct
>>>>>>>> amdgpu_device *adev);
>>>>>>>> struct drm_framebuffer *
>>>>>>>> amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>>> index 1bfce79..19f11a5 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>>> @@ -132,6 +132,7 @@ int amdgpu_lbpw = -1;
>>>>>>>> int amdgpu_compute_multipipe = -1;
>>>>>>>> int amdgpu_gpu_recovery = -1; /* auto */
>>>>>>>> int amdgpu_emu_mode = 0;
>>>>>>>> +int amdgpu_sg_display = -1;
>>>>>>>> MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in
>>>>>>>> megabytes");
>>>>>>>> module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
>>>>>>>> @@ -290,6 +291,9 @@ module_param_named(gpu_recovery,
>>>>>>>> amdgpu_gpu_recovery, int, 0444);
>>>>>>>> MODULE_PARM_DESC(emu_mode, "Emulation mode, (1 = enable, 0 =
>>>>>>>> disable)");
>>>>>>>> module_param_named(emu_mode, amdgpu_emu_mode, int, 0444);
>>>>>>>> +MODULE_PARM_DESC(sg_display, "Enable scatter gather
>>>>>>>> display, (1 = enable, 0 = disable, -1 = auto");
>>>>>>>> +module_param_named(sg_display, amdgpu_sg_display, int, 0444);
>>>>>>>> +
>>>>>>>> #ifdef CONFIG_DRM_AMDGPU_SI
>>>>>>>> #if defined(CONFIG_DRM_RADEON) ||
>>>>>>>> defined(CONFIG_DRM_RADEON_MODULE)
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>>> index 1206301..f57c355 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>>> @@ -138,6 +138,8 @@ static int
>>>>>>>> amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev,
>>>>>>>> mode_cmd->pitches[0] = amdgpu_align_pitch(adev,
>>>>>>>> mode_cmd->width, cpp,
>>>>>>>> fb_tiled);
>>>>>>>> domain = amdgpu_display_framebuffer_domains(adev);
>>>>>>>> + if (domain & AMDGPU_GEM_DOMAIN_GTT)
>>>>>>>> + DRM_DEBUG_DRIVER("Scatter gather display:
>>>>>>>> enabled\n");
>>>>>>>> height = ALIGN(mode_cmd->height, 8);
>>>>>>>> size = mode_cmd->pitches[0] * height;
>>>>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>> index 68ab325..7e9f247 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>> @@ -3074,7 +3074,8 @@ static int
>>>>>>>> dm_plane_helper_prepare_fb(struct drm_plane *plane,
>>>>>>>> domain = AMDGPU_GEM_DOMAIN_VRAM;
>>>>>>>> r = amdgpu_bo_pin(rbo, domain, &afb->address);
>>>>>>>> -
>>>>>>>> + rbo->preferred_domains = domain;
>>>>>>>> + rbo->allowed_domains = domain;
>>>>>>>> amdgpu_bo_unreserve(rbo);
>>>>>>>> if (unlikely(r != 0)) {
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> amd-gfx mailing list
>>>>>>>> amd-gfx at lists.freedesktop.org <mailto:amd-gfx at lists.freedesktop.org>
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>>> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>>>>>>>>
>>>>>>>>
>
More information about the amd-gfx
mailing list