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

[Samuel Li <samuel.li-5C7GfCeVMHo@public.gmane.org>]

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@gmail.com <mailto:ckoenig.leichtzumerken@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@amd.com
>>>>          <mailto:Samuel.Li@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@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>>>>      https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>      <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>>>>
>>>>
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx