Hi Sam, yeah sorry for that. It's already rather late here and I got a bit taken away. But I still insist that you at least try as I advised, I'm pretty sure that this approach will work and cover all use cases discussed so far. Regards, Christian. Am 12.04.2018 um 22:42 schrieb Samuel Li: > 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> >>>>>>>>> >>>>>>>>> > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
