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

Tue Mar 6 17:44:37 UTC 2018

Am 06.03.2018 um 18:22 schrieb Li, Samuel:
>> And exactly that's the problematical assumption.
> Not assumption, I tested. You have any typical use case that it might become a problem?

Not 100% sure, but I doubt you have tried that with different userspace 
programs.

Especially animated boot screens could allocated quite a number of 
framebuffers.

>
>> 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.

Regards,
Christian.

>
> Regards,
> Samuel Li
>
>
>> -----Original Message-----
>> From: Koenig, Christian
>> Sent: Tuesday, March 06, 2018 12:12 PM
>> To: Li, Samuel <Samuel.Li at amd.com>; Alex Deucher
>> <alexdeucher at gmail.com>
>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
>> Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display support
>>
>> And exactly that's the problematical assumption.
>>
>> This doesn't print only when the module is loaded, but rather when a
>> framebuffer object is created.
>>
>> And depending on the use case that can even be many many times per
>> second.
>>
>> Please remove that printing, addition to that I agree with Michel that the
>> module parameter is overkill.
>>
>> Regards,
>> Christian.
>>
>> Am 06.03.2018 um 18:09 schrieb Li, Samuel:
>>> This information is kind of important, and it only prints once typically when
>> module is loaded.
>>> Regards,
>>> Samuel Li
>>>
>>>
>>>> -----Original Message-----
>>>> From: Alex Deucher [mailto:alexdeucher at gmail.com]
>>>> Sent: Tuesday, March 06, 2018 12:04 PM
>>>> To: Li, Samuel <Samuel.Li at amd.com>
>>>> Cc: Koenig, Christian <Christian.Koenig at amd.com>; amd-gfx list <amd-
>>>> gfx at lists.freedesktop.org>
>>>> Subject: Re: [PATCH 1/2] drm/amdgpu: Enable scatter gather display
>>>> support
>>>>
>>>> On Tue, Mar 6, 2018 at 11:49 AM, Samuel Li <samuel.li at amd.com> wrote:
>>>>>>>         domain = amdgpu_display_framebuffer_domains(adev);
>>>>>>> +    if (domain == AMDGPU_GEM_DOMAIN_GTT) {
>>>>>>> +        DRM_INFO("Scatter gather display: enabled\n");
>>>>>>> +    } else if (domain & AMDGPU_GEM_DOMAIN_GTT) {
>>>>>>> +        DRM_INFO("Scatter gather display: auto\n");
>>>>>>> +    }
>>>>>> Dito and printing that here is not a good idea as far as I can see.
>>>>>>
>>>>>> Christian.
>>>>> The intention here is to print out when fb is created. You have
>>>>> other
>>>> suggestions?
>>>>
>>>> Make it DRM_DEBUG?  otherwise you'll spam the logs.
>>>>
>>>>> Sam
>>>>>
>>>>>
>>>>> On 2018-03-03 08:41 AM, Christian König wrote:
>>>>>> Am 03.03.2018 um 00:25 schrieb Samuel Li:
>>>>>>> It's enabled by default. -1 is auto, to allow both vram and gtt
>>>>>>> memory be available, for testing purpose only.
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu.h         | 1 +
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++--
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     | 4 ++++
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c      | 5 +++++
>>>>>>>     4 files changed, 17 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>> index 292c7e7..6b0ee34 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..dfa11b1 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>> @@ -513,8 +513,13 @@ 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) {
>>>>>>> +            domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>>> +        }
>>>>>> Coding style, that if shouldn't use "{" and "}".
>>>>>>
>>>>>>> +    }
>>>>>>>     #endif
>>>>>>>           return domain;
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>> index e670936..f0ada24 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..10f1f4f 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>> @@ -138,6 +138,11 @@ 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_INFO("Scatter gather display: enabled\n");
>>>>>>> +    } else if (domain & AMDGPU_GEM_DOMAIN_GTT) {
>>>>>>> +        DRM_INFO("Scatter gather display: auto\n");
>>>>>>> +    }
>>>>>> Dito and printing that here is not a good idea as far as I can see.
>>>>>>
>>>>>> Christian.
>>>>>>
>>>>>>>           height = ALIGN(mode_cmd->height, 8);
>>>>>>>         size = mode_cmd->pitches[0] * height;
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx at lists.freedesktop.org
>>>>> 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