Re: [PATCH] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM
I demand that Christian König may or may not have written... > Am 11.12.20 um 02:42 schrieb Darren Salt: >> I demand that Christian König may or may not have written... > [SNIP] > Well I did wrote that :) “did write”, surely… >> I used dd: >> # dd if=/sys/kernel/debug/dri/0/amdgpu_vram bs=1048576 count=1 skip=6127 | >> hexdump -C |tail > That won't work. amdgpu_vram uses a MMIO register pair to access VRAM which > works even when it isn't CPU visible. > Thinking more about it umr would probably use this as well, so that won't > work either. > You could try to use dd on /dev/mem with the offset of the BAR. Looks like that's RAM accessed by physical address, so that won't work either. And I do see dd reporting ‘bad address’. >> Anyway I agree that a PCI subsystem quirk might be appropriated. > I'm going to discuss AMD internally why you have such strange values in > the RBAR registers. I'm thinking probably an error by somebody at Sapphire, but we'll see… Hopefully, that'll sort it out, at least for new cards. I doubt that mine's the only one like this, and it seems likely that most already out there won't be updated (shoudl there be new VBIOS releases as a reault). Anyway, I have a quirk patch written now – untested as yet, and probably going to be changed due to other changes before I do test it. [snip] ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)
I demand that Christian König may or may not have written...
> Am 11.12.20 um 01:55 schrieb Darren Salt:
[snip]
>> +rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
>> +current_size = pci_rebar_get_current_size(adev->pdev, 0);
>> +
>> +/* Skip if the BIOS has already enabled large BAR, covering the VRAM */
>> +if (current_size >= rbar_size)
> You should probably keep the comparison as it is and check the resource
> length against the VRAM size instead.
Perhaps. I wonder, though, if I should do
if (adev->gmc.real_vram_size == 0)
return;
instead of the first part of the original condition.
[snip]
>> +dev_dbg(adev->dev, "BIOS-allocated BAR0 was %lluMB; trying to get
>> %lluMB",
>> +current_size < 0 ? 0 : (pci_rebar_size_to_bytes(current_size)
>> >> 20),
>> +pci_rebar_size_to_bytes(rbar_size) >> 20);
> Please no extra debugging output, we spam syslog that enough with the
> existing resize.
Okay, I'll dispose of that.
[snip]
>> -r = pci_resize_resource(adev->pdev, 0, rbar_size);
>> -if (r == -ENOSPC)
>> -DRM_INFO("Not enough PCI address space for a large BAR.");
>> -else if (r && r != -ENOTSUPP)
>> -DRM_ERROR("Problem resizing BAR0 (%d).", r);
>> +r = 0;
>> +for (; rbar_size >= 0 && rbar_size > current_size; --rbar_size) {
>> +/* Skip this size if it isn't advertised.
>> + * This avoids pci_resize_resources returning -EINVAL for that
>> reason.
>> + */
>> +if (!(available_sizes & BIT(rbar_size)))
>> +continue;
> Well exactly that try and error is a rather big NAK.
> What you need to do instead is to look at the return value from
> pci_rebar_get_possible_sizes() and determine the size closed to the desired
> one. […]
Well… there's that rapid reject immediately following; and the override patch
alters that condition.
> E.g. when need a size of 13 is needed you first check if any bit >= 13
> are set. You can use the ffs() for this.
So… find the lowest bit set, after masking out bits 0 to (rbar_size-1),
and try to re-allocate accordingly.
I could have it check for larger sizes if that fails, but I don't think that
it's worth it. If the BAR size is >= 2× the VRAM size, it's a waste of
address space; and the advertisement of such a size is arguably a VBIOS bug
anyway.
> If that isn't the case use fls() to get the highest set bit < 13.
That suggests that it'll be easiest to clear each bit after the corresponding
size is checked, I think. Also, this looks like it's adding complexity to
try to make rarely-executed code slightly faster in some cases (I can't see
it helping where available_sizes == 0x3F00, for example).
Incidentally, is it worth trying to reduce the BAR size at all? Thinking
mainly of two situations – limiting the maximum size, and the BIOS having
allocated one much too large.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)
On 2020-12-11 4:44 p.m., Luben Tuikov wrote: > If however, you never decide to send it to the hardware and simply > abandon it (in hopes that the DRM or the Application Client will > reissue it), then you should send it back to DRM with status "COMPLETE". Correction: "... decide to *not* send it to ..." Regards, Luben ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)
On 2020-12-10 4:46 a.m., Steven Price wrote:
> On 10/12/2020 02:14, Luben Tuikov wrote:
>> This patch does not change current behaviour.
>>
>> The driver's job timeout handler now returns
>> status indicating back to the DRM layer whether
>> the task (job) was successfully aborted or whether
>> more time should be given to the task to complete.
>
> I find the definitions given a little confusing, see below.
>
>> Default behaviour as of this patch, is preserved,
>> except in obvious-by-comment case in the Panfrost
>> driver, as documented below.
>>
>> All drivers which make use of the
>> drm_sched_backend_ops' .timedout_job() callback
>> have been accordingly renamed and return the
>> would've-been default value of
>> DRM_TASK_STATUS_ALIVE to restart the task's
>> timeout timer--this is the old behaviour, and
>> is preserved by this patch.
>>
>> In the case of the Panfrost driver, its timedout
>> callback correctly first checks if the job had
>> completed in due time and if so, it now returns
>> DRM_TASK_STATUS_COMPLETE to notify the DRM layer
>> that the task can be moved to the done list, to be
>> freed later. In the other two subsequent checks,
>> the value of DRM_TASK_STATUS_ALIVE is returned, as
>> per the default behaviour.
>>
>> A more involved driver's solutions can be had
>> in subequent patches.
>
> NIT: ^ subsequent
Thank you--no idea how my spellcheck and checkpatch.pl missed that.
>
>>
>> v2: Use enum as the status of a driver's job
>> timeout callback method.
>>
>> Cc: Alexander Deucher
>> Cc: Andrey Grodzovsky
>> Cc: Christian König
>> Cc: Daniel Vetter
>> Cc: Lucas Stach
>> Cc: Russell King
>> Cc: Christian Gmeiner
>> Cc: Qiang Yu
>> Cc: Rob Herring
>> Cc: Tomeu Vizoso
>> Cc: Steven Price
>> Cc: Alyssa Rosenzweig
>> Cc: Eric Anholt
>> Reported-by: kernel test robot
>
> This reported-by seems a little odd for this patch.
I got this because I wasn't (originally) changing all drivers
which were using the task timeout callback.
Should I remove it?
>
>> Signed-off-by: Luben Tuikov
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 +++--
>> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 10 +++-
>> drivers/gpu/drm/lima/lima_sched.c | 4 +++-
>> drivers/gpu/drm/panfrost/panfrost_job.c | 9 ---
>> drivers/gpu/drm/scheduler/sched_main.c | 4 +---
>> drivers/gpu/drm/v3d/v3d_sched.c | 32 +
>> include/drm/gpu_scheduler.h | 20 +---
>> 7 files changed, 57 insertions(+), 28 deletions(-)
>>
>
> []
>
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 2e0c368e19f6..cedfc5394e52 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -206,6 +206,11 @@ static inline bool drm_sched_invalidate_job(struct
>> drm_sched_job *s_job,
>> return s_job && atomic_inc_return(_job->karma) > threshold;
>> }
>>
>> +enum drm_task_status {
>> +DRM_TASK_STATUS_COMPLETE,
>> +DRM_TASK_STATUS_ALIVE
>> +};
>> +
>> /**
>>* struct drm_sched_backend_ops
>>*
>> @@ -230,10 +235,19 @@ struct drm_sched_backend_ops {
>> struct dma_fence *(*run_job)(struct drm_sched_job *sched_job);
>>
>> /**
>> - * @timedout_job: Called when a job has taken too long to execute,
>> - * to trigger GPU recovery.
>> + * @timedout_job: Called when a job has taken too long to execute,
>> + * to trigger GPU recovery.
>> + *
>> + * Return DRM_TASK_STATUS_ALIVE, if the task (job) is healthy
>> + * and executing in the hardware, i.e. it needs more time.
>
> So 'alive' means the job (was) alive, and GPU recovery is happening.
> I.e. it's the job just takes too long. Panfrost will trigger a GPU reset
> (killing the job) in this case while returning DRM_TASK_STATUS_ALIVE.
"ALIVE" means "at this moment the task is in the hardware executing,
using memories, DMA engines, compute units, the whole thing." You,
can also call it active, executing, busy, etc.
"ALIVE" makes no assumptions about how the task got there. Maybe
this was original submission, and the task is taking its sweet time.
Maybe the driver aborted it and reissued it, all in the timer timeout
callback, etc.
It merely tells the DRM layer that the task is actively executing
in the hardware, so no assumptions can be made about freeing memories,
decrementing krefs, etc. IOW, it's executing, check back later.
>
>> + *
>> + * Return DRM_TASK_STATUS_COMPLETE, if the task (job) has
>> + * been aborted or is unknown to the hardware, i.e. if
>> + * the task is out of the hardware, and maybe it is now
>> + * in the done list, or it was completed long ago, or
>> + * if it is unknown to the hardware.
>
> Where 'complete' seems to mean a variety of things:
>
> * The job completed successfully (i.e. the timeout raced), this is the
> situation that Panfrost detects. In this case (and only this case) the
> GPU reset will
Re: [PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)
On 2020-12-10 4:41 a.m., Christian König wrote:
> Am 10.12.20 um 10:31 schrieb Lucas Stach:
>> Hi Luben,
>>
>> Am Mittwoch, den 09.12.2020, 21:14 -0500 schrieb Luben Tuikov:
>>> [SNIP]
>>> -static void etnaviv_sched_timedout_job(struct drm_sched_job *sched_job)
>>> +static enum drm_task_status etnaviv_sched_timedout_job(struct drm_sched_job
>>> + *sched_job)
>>> {
>>> struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job);
>>> struct etnaviv_gpu *gpu = submit->gpu;
>>> @@ -120,9 +121,16 @@ static void etnaviv_sched_timedout_job(struct
>>> drm_sched_job *sched_job)
>>>
>>> drm_sched_resubmit_jobs(>sched);
>>>
>>> + /* Tell the DRM scheduler that this task needs
>>> +* more time.
>>> +*/
>> This comment doesn't match the kernel coding style, but it's also moot
>> as the whole added code block isn't needed. The code just below is
>> identical, so letting execution continue here instead of returning
>> would be the right thing to do, but maybe you mean to return
>> DRM_TASK_STATUS_COMPLETE? It's a bit confusing that aborted and job
>> successfully finished should be signaled with the same return code.
>
> Yes and no. As I tried to describe in my previous mail the naming of the
> enum values is also not very good.
I tried to make the naming as minimal as possible:
COMPLETE: the task is out of the hardware.
ALIVE: the task is in the hardware.
>
> See even when the job has completed we need to restart the timer for the
> potential next job.
Sure, yes. But this is something which the DRM decides--why should
drivers know of the internals of the DRM? (i.e. that it restarts
the timer or that there is a timer, etc.) Return minimal
value and let the DRM decide what to do next.
>
> Only when the device is completely gone and unrecoverable we should not
> restart the timer.
Yes, agreed.
>
> I suggest to either make this an int and return -ENODEV when that
> happens or rename the enum to something like DRM_SCHED_NODEV.
It was an int, but you suggested that it'd be a macro, so I made
it an enum so that the complier can check the values against the macros
returned.
I suggested, DRM_TASK_SCHED_ENODEV, but let Andrey add it
when he adds his patches on top of my patch here, because his
work adds hotplug/unplug support.
Also, note that if the pending list is freed, while the DRM
had been blocked, i.e. notified that the device is gone,
then returning DRM_TASK_SCHED_ENODEV would be moot, as there
are no tasks in the pending list.
This patch here is good as it is, since it is minimal and doesn't make
assumptions on DRM behaviour.
Regards,
Luben
>
> Regards,
> Christian.
>
>>
>>> + drm_sched_start(>sched, true);
>>> + return DRM_TASK_STATUS_ALIVE;
>>> +
>>> out_no_timeout:
>>> /* restart scheduler after GPU is usable again */
>>> drm_sched_start(>sched, true);
>>> + return DRM_TASK_STATUS_ALIVE;
>>> }
>> Regards,
>> Lucas
>>
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)
On 2020-12-10 4:31 a.m., Lucas Stach wrote:
> Hi Luben,
>
> Am Mittwoch, den 09.12.2020, 21:14 -0500 schrieb Luben Tuikov:
>> This patch does not change current behaviour.
>>
>> The driver's job timeout handler now returns
>> status indicating back to the DRM layer whether
>> the task (job) was successfully aborted or whether
>> more time should be given to the task to complete.
>>
>> Default behaviour as of this patch, is preserved,
>> except in obvious-by-comment case in the Panfrost
>> driver, as documented below.
>>
>> All drivers which make use of the
>> drm_sched_backend_ops' .timedout_job() callback
>> have been accordingly renamed and return the
>> would've-been default value of
>> DRM_TASK_STATUS_ALIVE to restart the task's
>> timeout timer--this is the old behaviour, and
>> is preserved by this patch.
>>
>> In the case of the Panfrost driver, its timedout
>> callback correctly first checks if the job had
>> completed in due time and if so, it now returns
>> DRM_TASK_STATUS_COMPLETE to notify the DRM layer
>> that the task can be moved to the done list, to be
>> freed later. In the other two subsequent checks,
>> the value of DRM_TASK_STATUS_ALIVE is returned, as
>> per the default behaviour.
>>
>> A more involved driver's solutions can be had
>> in subequent patches.
>>
>> v2: Use enum as the status of a driver's job
>> timeout callback method.
>>
>> Cc: Alexander Deucher
>> Cc: Andrey Grodzovsky
>> Cc: Christian König
>> Cc: Daniel Vetter
>> Cc: Lucas Stach
>> Cc: Russell King
>> Cc: Christian Gmeiner
>> Cc: Qiang Yu
>> Cc: Rob Herring
>> Cc: Tomeu Vizoso
>> Cc: Steven Price
>> Cc: Alyssa Rosenzweig
>> Cc: Eric Anholt
>> Reported-by: kernel test robot
>> Signed-off-by: Luben Tuikov
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 +++--
>> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 10 +++-
>> drivers/gpu/drm/lima/lima_sched.c | 4 +++-
>> drivers/gpu/drm/panfrost/panfrost_job.c | 9 ---
>> drivers/gpu/drm/scheduler/sched_main.c | 4 +---
>> drivers/gpu/drm/v3d/v3d_sched.c | 32 +
>> include/drm/gpu_scheduler.h | 20 +---
>> 7 files changed, 57 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> index ff48101bab55..a111326cbdde 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> @@ -28,7 +28,7 @@
>> #include "amdgpu.h"
>> #include "amdgpu_trace.h"
>>
>>
>>
>>
>> -static void amdgpu_job_timedout(struct drm_sched_job *s_job)
>> +static enum drm_task_status amdgpu_job_timedout(struct drm_sched_job *s_job)
>> {
>> struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
>> struct amdgpu_job *job = to_amdgpu_job(s_job);
>> @@ -41,7 +41,7 @@ static void amdgpu_job_timedout(struct drm_sched_job
>> *s_job)
>> amdgpu_ring_soft_recovery(ring, job->vmid, s_job->s_fence->parent))
>> {
>> DRM_ERROR("ring %s timeout, but soft recovered\n",
>> s_job->sched->name);
>> -return;
>> +return DRM_TASK_STATUS_ALIVE;
>> }
>>
>>
>>
>>
>> amdgpu_vm_get_task_info(ring->adev, job->pasid, );
>> @@ -53,10 +53,12 @@ static void amdgpu_job_timedout(struct drm_sched_job
>> *s_job)
>>
>>
>>
>>
>> if (amdgpu_device_should_recover_gpu(ring->adev)) {
>> amdgpu_device_gpu_recover(ring->adev, job);
>> +return DRM_TASK_STATUS_ALIVE;
>> } else {
>> drm_sched_suspend_timeout(>sched);
>> if (amdgpu_sriov_vf(adev))
>> adev->virt.tdr_debug = true;
>> +return DRM_TASK_STATUS_ALIVE;
>> }
>> }
>>
>>
>>
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> index cd46c882269c..c49516942328 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> @@ -82,7 +82,8 @@ static struct dma_fence *etnaviv_sched_run_job(struct
>> drm_sched_job *sched_job)
>> return fence;
>> }
>>
>>
>>
>>
>> -static void etnaviv_sched_timedout_job(struct drm_sched_job *sched_job)
>> +static enum drm_task_status etnaviv_sched_timedout_job(struct drm_sched_job
>> + *sched_job)
>> {
>> struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job);
>> struct etnaviv_gpu *gpu = submit->gpu;
>> @@ -120,9 +121,16 @@ static void etnaviv_sched_timedout_job(struct
>> drm_sched_job *sched_job)
>>
>> drm_sched_resubmit_jobs(>sched);
>>
>> +/* Tell the DRM scheduler that this task needs
>> + * more time.
>> + */
>
> This comment doesn't match the kernel coding style, but it's also moot
> as the whole added code block isn't needed. The code just below is
> identical, so letting execution continue here instead of returning
> would be the right thing
[PATCH] drm/amd/display: Fixed kernel test robot warning
Kernel test robot throws below warning -> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:5349:5: warning: no previous prototype for 'amdgpu_dm_crtc_atomic_set_property' [-Wmissing-prototypes] drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:5349:5: warning: no previous prototype for function 'amdgpu_dm_crtc_atomic_set_property' [-Wmissing-prototypes] drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:5373:5: warning: no previous prototype for 'amdgpu_dm_crtc_atomic_get_property' [-Wmissing-prototypes] drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:5373:5: warning: no previous prototype for function 'amdgpu_dm_crtc_atomic_get_property' [-Wmissing-prototypes] As these functions are only used inside amdgpu_dm.c, these can be made static. Reported-by: kernel test robot Signed-off-by: Souptick Joarder --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 313501c..e6d069d 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5328,7 +5328,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc) } #ifdef CONFIG_DEBUG_FS -int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc *crtc, +static int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state, struct drm_property *property, uint64_t val) @@ -5352,7 +5352,7 @@ int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc *crtc, return 0; } -int amdgpu_dm_crtc_atomic_get_property(struct drm_crtc *crtc, +static int amdgpu_dm_crtc_atomic_get_property(struct drm_crtc *crtc, const struct drm_crtc_state *state, struct drm_property *property, uint64_t *val) -- 1.9.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)
On Fri, Dec 11, 2020 at 1:49 PM Darren Salt wrote:
>
> I demand that Christian König may or may not have written...
>
> > Am 11.12.20 um 01:55 schrieb Darren Salt:
> [snip]
> >> +rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
> >> +current_size = pci_rebar_get_current_size(adev->pdev, 0);
> >> +
> >> +/* Skip if the BIOS has already enabled large BAR, covering the VRAM
> >> */
> >> +if (current_size >= rbar_size)
>
> > You should probably keep the comparison as it is and check the resource
> > length against the VRAM size instead.
>
> Perhaps. I wonder, though, if I should do
>
> if (adev->gmc.real_vram_size == 0)
> return;
>
> instead of the first part of the original condition.
>
> [snip]
> >> +dev_dbg(adev->dev, "BIOS-allocated BAR0 was %lluMB; trying to get
> >> %lluMB",
> >> +current_size < 0 ? 0 : (pci_rebar_size_to_bytes(current_size)
> >> >> 20),
> >> +pci_rebar_size_to_bytes(rbar_size) >> 20);
>
> > Please no extra debugging output, we spam syslog that enough with the
> > existing resize.
>
> Okay, I'll dispose of that.
>
> [snip]
> >> -r = pci_resize_resource(adev->pdev, 0, rbar_size);
> >> -if (r == -ENOSPC)
> >> -DRM_INFO("Not enough PCI address space for a large BAR.");
> >> -else if (r && r != -ENOTSUPP)
> >> -DRM_ERROR("Problem resizing BAR0 (%d).", r);
> >> +r = 0;
> >> +for (; rbar_size >= 0 && rbar_size > current_size; --rbar_size) {
> >> +/* Skip this size if it isn't advertised.
> >> + * This avoids pci_resize_resources returning -EINVAL for
> >> that reason.
> >> + */
> >> +if (!(available_sizes & BIT(rbar_size)))
> >> +continue;
>
> > Well exactly that try and error is a rather big NAK.
>
> > What you need to do instead is to look at the return value from
> > pci_rebar_get_possible_sizes() and determine the size closed to the desired
> > one. […]
>
> Well… there's that rapid reject immediately following; and the override patch
> alters that condition.
>
> > E.g. when need a size of 13 is needed you first check if any bit >= 13
> > are set. You can use the ffs() for this.
>
> So… find the lowest bit set, after masking out bits 0 to (rbar_size-1),
> and try to re-allocate accordingly.
>
> I could have it check for larger sizes if that fails, but I don't think that
> it's worth it. If the BAR size is >= 2× the VRAM size, it's a waste of
> address space; and the advertisement of such a size is arguably a VBIOS bug
> anyway.
>
> > If that isn't the case use fls() to get the highest set bit < 13.
>
> That suggests that it'll be easiest to clear each bit after the corresponding
> size is checked, I think. Also, this looks like it's adding complexity to
> try to make rarely-executed code slightly faster in some cases (I can't see
> it helping where available_sizes == 0x3F00, for example).
>
> Incidentally, is it worth trying to reduce the BAR size at all? Thinking
> mainly of two situations – limiting the maximum size, and the BIOS having
> allocated one much too large.
In theory we could on resource constrained systems. E.g., if you have
a lot of devices and a limited MMIO window, but I think on most recent
AMD GPUs, 256M is the smallest size and the default.
Alex
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
On 11/12/20 9:50 pm, Kazlauskas, Nicholas wrote:
> On 2020-12-11 10:35 a.m., Shashank Sharma wrote:
>> On 11/12/20 8:19 pm, Kazlauskas, Nicholas wrote:
>>> On 2020-12-11 12:08 a.m., Shashank Sharma wrote:
On 10/12/20 11:20 pm, Aurabindo Pillai wrote:
> On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
>> On 10/12/20 8:15 am, Aurabindo Pillai wrote:
>>> [Why]
>>> Inorder to enable freesync video mode, driver adds extra
>>> modes based on preferred modes for common freesync frame rates.
>>> When commiting these mode changes, a full modeset is not needed.
>>> If the change in only in the front porch timing value, skip full
>>> modeset and continue using the same stream.
>>>
>>> Signed-off-by: Aurabindo Pillai <
>>> aurabindo.pil...@amd.com
>>> ---
>>>.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
>>> --
>>>.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
>>>2 files changed, 153 insertions(+), 17 deletions(-)
>>>
>>> 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 f699a3d41cad..c8c72887906a 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
>>> amdgpu_display_manager *dm);
>>>static const struct drm_format_info *
>>>amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>>>
>>> +static bool
>>> +is_timing_unchanged_for_freesync(struct drm_crtc_state
>>> *old_crtc_state,
>>> +struct drm_crtc_state
>>> *new_crtc_state);
>>>/*
>>> * dm_vblank_get_counter
>>> *
>>> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
>>> struct drm_display_mode *src_mode,
>>>static void
>>>decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
>>> *drm_mode,
>>> const struct drm_display_mode
>>> *native_mode,
>>> - bool scale_enabled)
>>> + bool scale_enabled, bool
>>> fs_mode)
>>>{
>>> + if (fs_mode)
>>> + return;
>> so we are adding an input flag just so that we can return from the
>> function at top ? How about adding this check at the caller without
>> changing the function parameters ?
> Will fix this.
>
>>> +
>>> if (scale_enabled) {
>>> copy_crtc_timing_for_drm_display_mode(native_mode,
>>> drm_mode);
>>> } else if (native_mode->clock == drm_mode->clock &&
>>> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
>>> amdgpu_dm_connector *aconnector,
>>> return m_high;
>>>}
>>>
>>> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
>>> + struct amdgpu_dm_connector
>>> *aconnector)
>>> +{
>>> + struct drm_display_mode *high_mode;
>>> +
>> I thought we were adding a string "_FSV" in the end for the mode-
>>> name, why can't we check that instead of going through the whole
>> list of modes again ?
> Actually I only added _FSV to distinguish the newly added modes easily.
> On second thoughts, I'm not sure if there are any userspace
> applications that might depend on parsing the mode name, for maybe to
> print the resolution. I think its better not to break any such
> assumptions if they do exist by any chance. I think I'll just remove
> _FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
> userspace to recognize these additional modes, so it shouldnt be a
> problem.
Actually, I am rather happy with this, as in when we want to test out this
feature with a IGT type stuff, or if a userspace wants to utilize this
option in any way, this method of differentiation would be useful.
DRM_MODE_DRIVER is being used by some other places apart from freesync, so
it might not be a unique identifier. So my recommendation would be to keep
this.
My comment was, if we have already parsed the whole connector list once,
and added the mode, there should be a better way of doing it instead of
checking it again by calling "get_highest_freesync_mod"
Some things I can think on top of my mind would be:
- Add a read-only amdgpu driver private flag (not DRM flag), while adding
a new freesync mode, which will uniquely identify if a mode is FS mode. On
modeset, you have to just check that flag.
- As we are not handling a lot of modes, cache the FS modes locally and
check only from that DB (instead of the whole modelist)
8353d30e747f ("drm/amd/display: disable stream if pixel clock changed with link active")
Hi, patch in $Subject breaks booting on a laptop here, GPU details are below. The machine stops booting right when it attempts to switch modes during boot, to a higher mode than the default VGA one. Machine doesn't ping and is otherwise unresponsive so that a hard reset is the only thing that helps. Reverting that patch ontop of -rc7 fixes it and the machine boots just fine. Thx. [1.628086] ata1.00: supports DRM functions and may not be fully accessible [1.632050] ata1.00: supports DRM functions and may not be fully accessible [1.895818] [drm] amdgpu kernel modesetting enabled. [1.897628] [drm] initializing kernel modesetting (CARRIZO 0x1002:0x9874 0x103C:0x807E 0xC4). [1.898256] [drm] register mmio base: 0xD0C0 [1.898422] [drm] register mmio size: 262144 [1.898583] [drm] add ip block number 0 [1.898759] [drm] add ip block number 1 [1.898931] [drm] add ip block number 2 [1.899082] [drm] add ip block number 3 [1.899241] [drm] add ip block number 4 [1.899439] [drm] add ip block number 5 [1.899573] [drm] add ip block number 6 [1.899693] [drm] add ip block number 7 [1.899827] [drm] add ip block number 8 [1.911458] [drm] BIOS signature incorrect 5b 7 [1.912551] [drm] UVD is enabled in physical mode [1.912707] [drm] VCE enabled in physical mode [1.912921] [drm] vm size is 64 GB, 2 levels, block size is 10-bit, fragment size is 9-bit [1.913837] [drm] Detected VRAM RAM=512M, BAR=512M [1.913998] [drm] RAM width 128bits UNKNOWN [1.915149] [drm] amdgpu: 512M of VRAM memory ready [1.915306] [drm] amdgpu: 3072M of GTT memory ready. [1.915468] [drm] GART: num cpu pages 262144, num gpu pages 262144 [1.916139] [drm] PCIE GART of 1024M enabled (table at 0x00F40090). [1.918733] [drm] Found UVD firmware Version: 1.91 Family ID: 11 [1.918950] [drm] UVD ENC is disabled [1.919680] [drm] Found VCE firmware Version: 52.4 Binary ID: 3 [1.925963] [drm] DM_PPLIB: values for Engine clock [1.926106] [drm] DM_PPLIB: 30 [1.926205] [drm] DM_PPLIB: 36 [1.926304] [drm] DM_PPLIB: 423530 [1.926404] [drm] DM_PPLIB: 514290 [1.926516] [drm] DM_PPLIB: 626090 [1.926629] [drm] DM_PPLIB: 72 [1.926743] [drm] DM_PPLIB: Validation clocks: [1.926952] [drm] DM_PPLIB:engine_max_clock: 72000 [1.927117] [drm] DM_PPLIB:memory_max_clock: 8 [1.927281] [drm] DM_PPLIB:level : 8 [1.927435] [drm] DM_PPLIB: values for Display clock [1.927594] [drm] DM_PPLIB: 30 [1.927708] [drm] DM_PPLIB: 40 [1.927822] [drm] DM_PPLIB: 496560 [1.927936] [drm] DM_PPLIB: 626090 [1.928048] [drm] DM_PPLIB: 685720 [1.928161] [drm] DM_PPLIB: 757900 [1.928275] [drm] DM_PPLIB: Validation clocks: [1.928419] [drm] DM_PPLIB:engine_max_clock: 72000 [1.928584] [drm] DM_PPLIB:memory_max_clock: 8 [1.928748] [drm] DM_PPLIB:level : 8 [1.928901] [drm] DM_PPLIB: values for Memory clock [1.929058] [drm] DM_PPLIB: 333000 [1.929172] [drm] DM_PPLIB: 80 [1.929403] [drm] DM_PPLIB: Validation clocks: [1.929549] [drm] DM_PPLIB:engine_max_clock: 72000 [1.929716] [drm] DM_PPLIB:memory_max_clock: 8 [1.929919] [drm] DM_PPLIB:level : 8 [1.930148] [drm] Display Core initialized with v3.2.104! [2.003938] [drm] UVD initialized successfully. [2.204023] [drm] VCE initialized successfully. [2.206228] [drm] fb mappable at 0xA0EE4000 [2.206375] [drm] vram apper at 0xA000 [2.206514] [drm] size 14745600 [2.206654] [drm] fb depth is 24 [2.206760] [drm]pitch is 10240 [2.207123] fbcon: amdgpudrmfb (fb0) is primary device [2.301263] amdgpu :00:01.0: [drm] fb0: amdgpudrmfb frame buffer device [2.320735] [drm] Initialized amdgpu 3.40.0 20150101 for :00:01.0 on minor 0 -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM
Am 11.12.20 um 02:42 schrieb Darren Salt: I demand that Christian König may or may not have written... [SNIP] Well I did wrote that :) I used dd: # dd if=/sys/kernel/debug/dri/0/amdgpu_vram bs=1048576 count=1 skip=6127 | hexdump -C |tail That won't work. amdgpu_vram uses a MMIO register pair to access VRAM which works even when it isn't CPU visible. Thinking more about it umr would probably use this as well, so that won't work either. You could try to use dd on /dev/mem with the offset of the BAR. Anyway I agree that a PCI subsystem quirk might be appropriated. I'm going to discuss AMD internally why you have such strange values in the RBAR registers. Just send that to me as a complete and clean patchset. Done, though only to the list. I have a few comments on the patches. They can use some polishing, but in general the approach looks solid to me. Regards, Christian. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)
Am 11.12.20 um 01:55 schrieb Darren Salt:
This allows BAR0 resizing to be done for cards which don't advertise support
for a size large enough to cover the VRAM but which do advertise at least
one size larger than the default. For example, my RX 5600 XT, which
advertises 256MB, 512MB and 1GB.
[v2] rewritten to use PCI helper functions; some extra log text.
Signed-off-by: Darren Salt
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 53 ++
1 file changed, 43 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6637b84aeb85..1e99ca62a4d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1106,21 +1106,24 @@ void amdgpu_device_wb_free(struct amdgpu_device *adev,
u32 wb)
*/
int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
{
- u64 space_needed = roundup_pow_of_two(adev->gmc.real_vram_size);
- u32 rbar_size = order_base_2(((space_needed >> 20) | 1)) - 1;
+ int rbar_size, current_size;
+ u32 available_sizes;
struct pci_bus *root;
struct resource *res;
unsigned i;
u16 cmd;
int r;
+ bool nospc = false;
/* Bypass for VF */
if (amdgpu_sriov_vf(adev))
return 0;
- /* skip if the bios has already enabled large BAR */
- if (adev->gmc.real_vram_size &&
- (pci_resource_len(adev->pdev, 0) >= adev->gmc.real_vram_size))
+ rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
+ current_size = pci_rebar_get_current_size(adev->pdev, 0);
+
+ /* Skip if the BIOS has already enabled large BAR, covering the VRAM */
+ if (current_size >= rbar_size)
You should probably keep the comparison as it is and check the resource
length against the VRAM size instead.
return 0;
/* Check if the root BUS has 64bit memory resources */
@@ -1138,6 +1141,14 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device
*adev)
if (!res)
return 0;
+ available_sizes = pci_rebar_get_possible_sizes(adev->pdev, 0);
+ if (available_sizes == 0)
+ return 0;
+
+ dev_dbg(adev->dev, "BIOS-allocated BAR0 was %lluMB; trying to get
%lluMB",
+ current_size < 0 ? 0 : (pci_rebar_size_to_bytes(current_size)
>> 20),
+ pci_rebar_size_to_bytes(rbar_size) >> 20);
Please no extra debugging output, we spam syslog that enough with the
existing resize.
+
/* Disable memory decoding while we change the BAR addresses and size */
pci_read_config_word(adev->pdev, PCI_COMMAND, );
pci_write_config_word(adev->pdev, PCI_COMMAND,
@@ -1150,11 +1161,33 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device
*adev)
pci_release_resource(adev->pdev, 0);
- r = pci_resize_resource(adev->pdev, 0, rbar_size);
- if (r == -ENOSPC)
- DRM_INFO("Not enough PCI address space for a large BAR.");
- else if (r && r != -ENOTSUPP)
- DRM_ERROR("Problem resizing BAR0 (%d).", r);
+ r = 0;
+ for (; rbar_size >= 0 && rbar_size > current_size; --rbar_size) {
Well exactly that try and error is a rather big NAK.
What you need to do instead is to look at the return value from
pci_rebar_get_possible_sizes() and determine the size closed to the
desired one.
E.g. when need a size of 13 is needed you first check if any bit >= 13
are set. You can use the ffs() for this.
If that isn't the case use fls() to get the highest set bit < 13.
Regards,
Christian.
+ /* Skip this size if it isn't advertised.
+* This avoids pci_resize_resources returning -EINVAL for that
reason.
+*/
+ if (!(available_sizes & BIT(rbar_size)))
+ continue;
+
+ r = pci_resize_resource(adev->pdev, 0, rbar_size);
+ if (r == 0) {
+ dev_dbg(adev->dev, "Succeeded in resizing to %lluMB.",
+ pci_rebar_size_to_bytes(rbar_size) >> 20);
+ break;
+ } else if (r == -ENOTSUPP) {
+ dev_info(adev->dev, "BAR resizing not supported.");
+ break;
+ } else if (r == -ENOSPC) {
+ if (!nospc) {
+ /* Warn only the first time */
+ dev_info(adev->dev, "Not enough PCI address space
for a large BAR.");
+ nospc = true;
+ }
+ } else {
+ dev_err(adev->dev, "Problem resizing BAR0 (%d).", r);
+ break;
+ }
+ }
pci_assign_unassigned_bus_resources(adev->pdev->bus);
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
On 2020-12-11 10:35 a.m., Shashank Sharma wrote:
On 11/12/20 8:19 pm, Kazlauskas, Nicholas wrote:
On 2020-12-11 12:08 a.m., Shashank Sharma wrote:
On 10/12/20 11:20 pm, Aurabindo Pillai wrote:
On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
On 10/12/20 8:15 am, Aurabindo Pillai wrote:
[Why]
Inorder to enable freesync video mode, driver adds extra
modes based on preferred modes for common freesync frame rates.
When commiting these mode changes, a full modeset is not needed.
If the change in only in the front porch timing value, skip full
modeset and continue using the same stream.
Signed-off-by: Aurabindo Pillai <
aurabindo.pil...@amd.com
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
--
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
2 files changed, 153 insertions(+), 17 deletions(-)
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 f699a3d41cad..c8c72887906a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
amdgpu_display_manager *dm);
static const struct drm_format_info *
amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
+static bool
+is_timing_unchanged_for_freesync(struct drm_crtc_state
*old_crtc_state,
+struct drm_crtc_state
*new_crtc_state);
/*
* dm_vblank_get_counter
*
@@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
struct drm_display_mode *src_mode,
static void
decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
*drm_mode,
const struct drm_display_mode
*native_mode,
- bool scale_enabled)
+ bool scale_enabled, bool
fs_mode)
{
+ if (fs_mode)
+ return;
so we are adding an input flag just so that we can return from the
function at top ? How about adding this check at the caller without
changing the function parameters ?
Will fix this.
+
if (scale_enabled) {
copy_crtc_timing_for_drm_display_mode(native_mode,
drm_mode);
} else if (native_mode->clock == drm_mode->clock &&
@@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
amdgpu_dm_connector *aconnector,
return m_high;
}
+static bool is_freesync_video_mode(struct drm_display_mode *mode,
+ struct amdgpu_dm_connector
*aconnector)
+{
+ struct drm_display_mode *high_mode;
+
I thought we were adding a string "_FSV" in the end for the mode-
name, why can't we check that instead of going through the whole
list of modes again ?
Actually I only added _FSV to distinguish the newly added modes easily.
On second thoughts, I'm not sure if there are any userspace
applications that might depend on parsing the mode name, for maybe to
print the resolution. I think its better not to break any such
assumptions if they do exist by any chance. I think I'll just remove
_FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
userspace to recognize these additional modes, so it shouldnt be a
problem.
Actually, I am rather happy with this, as in when we want to test out this
feature with a IGT type stuff, or if a userspace wants to utilize this option
in any way, this method of differentiation would be useful. DRM_MODE_DRIVER is
being used by some other places apart from freesync, so it might not be a
unique identifier. So my recommendation would be to keep this.
My comment was, if we have already parsed the whole connector list once, and added the
mode, there should be a better way of doing it instead of checking it again by calling
"get_highest_freesync_mod"
Some things I can think on top of my mind would be:
- Add a read-only amdgpu driver private flag (not DRM flag), while adding a new
freesync mode, which will uniquely identify if a mode is FS mode. On modeset,
you have to just check that flag.
- As we are not handling a lot of modes, cache the FS modes locally and check
only from that DB (instead of the whole modelist)
- Cache the VIC of the mode (if available) and then look into the VIC table
(not sure if detailed modes provide VIC, like CEA-861 modes)
or something better than this.
- Shashank
I'd rather we not make mode name part of a UAPI or to identify a
"FreeSync mode". This is already behind a module option and from the
driver's perspective we only need the timing to understand whether or
not we can do an optimized modeset using FreeSync into it. Driver
private flags can optimize the check away but it's only a few
comparisons so I don't see much benefit.
The module parameter is just to control the addition of freesync modes or not,
but that doesn't let you differentiate between a genuine EDID mode or Freesync
modes added by us, to accommodate
Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
On 11/12/20 8:19 pm, Kazlauskas, Nicholas wrote:
> On 2020-12-11 12:08 a.m., Shashank Sharma wrote:
>> On 10/12/20 11:20 pm, Aurabindo Pillai wrote:
>>> On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
On 10/12/20 8:15 am, Aurabindo Pillai wrote:
> [Why]
> Inorder to enable freesync video mode, driver adds extra
> modes based on preferred modes for common freesync frame rates.
> When commiting these mode changes, a full modeset is not needed.
> If the change in only in the front porch timing value, skip full
> modeset and continue using the same stream.
>
> Signed-off-by: Aurabindo Pillai <
> aurabindo.pil...@amd.com
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
> --
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
> 2 files changed, 153 insertions(+), 17 deletions(-)
>
> 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 f699a3d41cad..c8c72887906a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
> amdgpu_display_manager *dm);
> static const struct drm_format_info *
> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state
> *old_crtc_state,
> + struct drm_crtc_state
> *new_crtc_state);
> /*
>* dm_vblank_get_counter
>*
> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
> struct drm_display_mode *src_mode,
> static void
> decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
> *drm_mode,
> const struct drm_display_mode
> *native_mode,
> - bool scale_enabled)
> + bool scale_enabled, bool
> fs_mode)
> {
> + if (fs_mode)
> + return;
so we are adding an input flag just so that we can return from the
function at top ? How about adding this check at the caller without
changing the function parameters ?
>>> Will fix this.
>>>
> +
> if (scale_enabled) {
> copy_crtc_timing_for_drm_display_mode(native_mode,
> drm_mode);
> } else if (native_mode->clock == drm_mode->clock &&
> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
> amdgpu_dm_connector *aconnector,
> return m_high;
> }
>
> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> +struct amdgpu_dm_connector
> *aconnector)
> +{
> + struct drm_display_mode *high_mode;
> +
I thought we were adding a string "_FSV" in the end for the mode-
> name, why can't we check that instead of going through the whole
list of modes again ?
>>> Actually I only added _FSV to distinguish the newly added modes easily.
>>> On second thoughts, I'm not sure if there are any userspace
>>> applications that might depend on parsing the mode name, for maybe to
>>> print the resolution. I think its better not to break any such
>>> assumptions if they do exist by any chance. I think I'll just remove
>>> _FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
>>> userspace to recognize these additional modes, so it shouldnt be a
>>> problem.
>> Actually, I am rather happy with this, as in when we want to test out this
>> feature with a IGT type stuff, or if a userspace wants to utilize this
>> option in any way, this method of differentiation would be useful.
>> DRM_MODE_DRIVER is being used by some other places apart from freesync, so
>> it might not be a unique identifier. So my recommendation would be to keep
>> this.
>>
>> My comment was, if we have already parsed the whole connector list once, and
>> added the mode, there should be a better way of doing it instead of checking
>> it again by calling "get_highest_freesync_mod"
>>
>> Some things I can think on top of my mind would be:
>>
>> - Add a read-only amdgpu driver private flag (not DRM flag), while adding a
>> new freesync mode, which will uniquely identify if a mode is FS mode. On
>> modeset, you have to just check that flag.
>>
>> - As we are not handling a lot of modes, cache the FS modes locally and
>> check only from that DB (instead of the whole modelist)
>>
>> - Cache the VIC of the mode (if available) and then look into the VIC table
>> (not sure if detailed modes provide VIC, like CEA-861 modes)
>>
>> or something better than this.
>>
>> - Shashank
> I'd rather we not make mode name part of a UAPI or to identify a
> "FreeSync mode". This is already behind a module option and from the
> driver's
Re: [PATCH] drm/amdgpu: skip vram operation for BAMACO runtime
[AMD Public Use]
Instead of checking the global parameters everywhere, let's check the runtime
pm parameter and then set a local adev variable per device. That way we can
have a mix of devices that support different runtime pm modes in the same
system and everything works.
Alex
From: amd-gfx on behalf of Likun Gao
Sent: Friday, December 11, 2020 4:04 AM
To: amd-gfx@lists.freedesktop.org
Cc: Gao, Likun ; Feng, Kenneth ;
Zhang, Hawking
Subject: [PATCH] drm/amdgpu: skip vram operation for BAMACO runtime
From: Likun Gao
Skip vram related operation for bamaco rumtime suspend and resume as
vram is alive when BAMACO.
It can save about 32ms when suspend and about 15ms when resume.
Signed-off-by: Likun Gao
Change-Id: I6ad39765de5ed1aac2dc51e96ed7a21a727272cd
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 9 ++-
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c| 72 +-
2 files changed, 50 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 0ec7c28c4d5a..66b790dfb151 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2464,7 +2464,8 @@ static int amdgpu_device_ip_late_init(struct
amdgpu_device *adev)
amdgpu_device_set_cg_state(adev, AMD_CG_STATE_GATE);
amdgpu_device_set_pg_state(adev, AMD_PG_STATE_GATE);
- amdgpu_device_fill_reset_magic(adev);
+ if ((amdgpu_runtime_pm != 2) || !adev->in_runpm)
+ amdgpu_device_fill_reset_magic(adev);
r = amdgpu_device_enable_mgpu_fan_boost();
if (r)
@@ -3706,7 +3707,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
amdgpu_amdkfd_suspend(adev, !fbcon);
/* evict vram memory */
- amdgpu_bo_evict_vram(adev);
+ if ((amdgpu_runtime_pm != 2) || !adev->in_runpm)
+ amdgpu_bo_evict_vram(adev);
amdgpu_fence_driver_suspend(adev);
@@ -3718,7 +3720,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
* This second call to evict vram is to evict the gart page table
* using the CPU.
*/
- amdgpu_bo_evict_vram(adev);
+ if ((amdgpu_runtime_pm != 2) || !adev->in_runpm)
+ amdgpu_bo_evict_vram(adev);
return 0;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 523d22db094b..67e74b43a1ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -397,10 +397,12 @@ static int psp_tmr_init(struct psp_context *psp)
}
}
- pptr = amdgpu_sriov_vf(psp->adev) ? _buf : NULL;
- ret = amdgpu_bo_create_kernel(psp->adev, tmr_size, PSP_TMR_SIZE,
+ if ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm) {
+ pptr = amdgpu_sriov_vf(psp->adev) ? _buf : NULL;
+ ret = amdgpu_bo_create_kernel(psp->adev, tmr_size, PSP_TMR_SIZE,
AMDGPU_GEM_DOMAIN_VRAM,
>tmr_bo, >tmr_mc_addr, pptr);
+ }
return ret;
}
@@ -504,8 +506,10 @@ static int psp_tmr_terminate(struct psp_context *psp)
return ret;
/* free TMR memory buffer */
- pptr = amdgpu_sriov_vf(psp->adev) ? _buf : NULL;
- amdgpu_bo_free_kernel(>tmr_bo, >tmr_mc_addr, pptr);
+ if ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm) {
+ pptr = amdgpu_sriov_vf(psp->adev) ? _buf : NULL;
+ amdgpu_bo_free_kernel(>tmr_bo, >tmr_mc_addr, pptr);
+ }
return 0;
}
@@ -795,9 +799,10 @@ int psp_xgmi_terminate(struct psp_context *psp)
psp->xgmi_context.initialized = 0;
/* free xgmi shared memory */
- amdgpu_bo_free_kernel(>xgmi_context.xgmi_shared_bo,
- >xgmi_context.xgmi_shared_mc_addr,
- >xgmi_context.xgmi_shared_buf);
+ if ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm)
+ amdgpu_bo_free_kernel(>xgmi_context.xgmi_shared_bo,
+ >xgmi_context.xgmi_shared_mc_addr,
+ >xgmi_context.xgmi_shared_buf);
return 0;
}
@@ -812,7 +817,8 @@ int psp_xgmi_initialize(struct psp_context *psp)
!psp->adev->psp.ta_xgmi_start_addr)
return -ENOENT;
- if (!psp->xgmi_context.initialized) {
+ if (!psp->xgmi_context.initialized &&
+ ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm)) {
ret = psp_xgmi_init_shared_buf(psp);
if (ret)
return ret;
@@ -1122,9 +1128,10 @@ static int psp_ras_terminate(struct psp_context *psp)
psp->ras.ras_initialized = false;
/* free ras shared memory */
- amdgpu_bo_free_kernel(>ras.ras_shared_bo,
-
Re: [V2] drm/amdgpu: add judgement for suspend/resume sequence
[AMD Official Use Only - Internal Distribution Only]
Reviewed-by: Alex Deucher
From: amd-gfx on behalf of Likun Gao
Sent: Friday, December 11, 2020 4:01 AM
To: amd-gfx@lists.freedesktop.org
Cc: Gao, Likun ; Feng, Kenneth ;
Zhang, Hawking
Subject: [V2] drm/amdgpu: add judgement for suspend/resume sequence
From: Likun Gao
S0ix only makes sense on APUs since they are part of the platform, so
only when the ASIC is APU should set amdgpu_acpi_is_s0ix_supported flag
to deal with the related situation.
Signed-off-by: Likun Gao
Change-Id: Ic89df206734fa7e6ce3e5a784171f149a07edc80
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h| 4 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 +---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 +++---
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 83ac06a3ec05..eed5410947e9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1314,11 +1314,11 @@ int amdgpu_acpi_pcie_notify_device_ready(struct
amdgpu_device *adev);
void amdgpu_acpi_get_backlight_caps(struct amdgpu_device *adev,
struct amdgpu_dm_backlight_caps *caps);
-bool amdgpu_acpi_is_s0ix_supported(void);
+bool amdgpu_acpi_is_s0ix_supported(struct amdgpu_device *adev);
#else
static inline int amdgpu_acpi_init(struct amdgpu_device *adev) { return 0; }
static inline void amdgpu_acpi_fini(struct amdgpu_device *adev) { }
-static inline bool amdgpu_acpi_is_s0ix_supported(void) { return false; }
+static inline bool amdgpu_acpi_is_s0ix_supported(struct amdgpu_device *adev) {
return false; }
#endif
int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index fd66ac00c7f5..8155c54392c8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -901,10 +901,12 @@ void amdgpu_acpi_fini(struct amdgpu_device *adev)
*
* returns true if supported, false if not.
*/
-bool amdgpu_acpi_is_s0ix_supported()
+bool amdgpu_acpi_is_s0ix_supported(struct amdgpu_device *adev)
{
- if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
- return true;
+ if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
+ if (adev->flags & AMD_IS_APU)
+ return true;
+ }
return false;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 52d6f1fbe890..66b790dfb151 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2651,7 +2651,7 @@ static int amdgpu_device_ip_suspend_phase1(struct
amdgpu_device *adev)
{
int i, r;
- if (!amdgpu_acpi_is_s0ix_supported() || amdgpu_in_reset(adev)) {
+ if (!amdgpu_acpi_is_s0ix_supported(adev) || amdgpu_in_reset(adev)) {
amdgpu_device_set_pg_state(adev, AMD_PG_STATE_UNGATE);
amdgpu_device_set_cg_state(adev, AMD_CG_STATE_UNGATE);
}
@@ -3712,7 +3712,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
amdgpu_fence_driver_suspend(adev);
- if (!amdgpu_acpi_is_s0ix_supported() || amdgpu_in_reset(adev))
+ if (!amdgpu_acpi_is_s0ix_supported(adev) || amdgpu_in_reset(adev))
r = amdgpu_device_ip_suspend_phase2(adev);
else
amdgpu_gfx_state_change_set(adev, sGpuChangeState_D3Entry);
@@ -3747,7 +3747,7 @@ int amdgpu_device_resume(struct drm_device *dev, bool
fbcon)
if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- if (amdgpu_acpi_is_s0ix_supported())
+ if (amdgpu_acpi_is_s0ix_supported(adev))
amdgpu_gfx_state_change_set(adev, sGpuChangeState_D0Entry);
/* post card */
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Calexander.deucher%40amd.com%7Cf91d4568bfcc442e773808d89db36073%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637432741044140311%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=sxM%2BF4tq7x73lRdc%2FftMGE8yrN%2BoBgs3syxFoK0q8Cc%3Dreserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
On 2020-12-11 12:08 a.m., Shashank Sharma wrote:
On 10/12/20 11:20 pm, Aurabindo Pillai wrote:
On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
On 10/12/20 8:15 am, Aurabindo Pillai wrote:
[Why]
Inorder to enable freesync video mode, driver adds extra
modes based on preferred modes for common freesync frame rates.
When commiting these mode changes, a full modeset is not needed.
If the change in only in the front porch timing value, skip full
modeset and continue using the same stream.
Signed-off-by: Aurabindo Pillai <
aurabindo.pil...@amd.com
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
--
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
2 files changed, 153 insertions(+), 17 deletions(-)
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 f699a3d41cad..c8c72887906a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
amdgpu_display_manager *dm);
static const struct drm_format_info *
amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
+static bool
+is_timing_unchanged_for_freesync(struct drm_crtc_state
*old_crtc_state,
+struct drm_crtc_state
*new_crtc_state);
/*
* dm_vblank_get_counter
*
@@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
struct drm_display_mode *src_mode,
static void
decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
*drm_mode,
const struct drm_display_mode
*native_mode,
- bool scale_enabled)
+ bool scale_enabled, bool
fs_mode)
{
+ if (fs_mode)
+ return;
so we are adding an input flag just so that we can return from the
function at top ? How about adding this check at the caller without
changing the function parameters ?
Will fix this.
+
if (scale_enabled) {
copy_crtc_timing_for_drm_display_mode(native_mode,
drm_mode);
} else if (native_mode->clock == drm_mode->clock &&
@@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
amdgpu_dm_connector *aconnector,
return m_high;
}
+static bool is_freesync_video_mode(struct drm_display_mode *mode,
+ struct amdgpu_dm_connector
*aconnector)
+{
+ struct drm_display_mode *high_mode;
+
I thought we were adding a string "_FSV" in the end for the mode-
name, why can't we check that instead of going through the whole
list of modes again ?
Actually I only added _FSV to distinguish the newly added modes easily.
On second thoughts, I'm not sure if there are any userspace
applications that might depend on parsing the mode name, for maybe to
print the resolution. I think its better not to break any such
assumptions if they do exist by any chance. I think I'll just remove
_FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
userspace to recognize these additional modes, so it shouldnt be a
problem.
Actually, I am rather happy with this, as in when we want to test out this
feature with a IGT type stuff, or if a userspace wants to utilize this option
in any way, this method of differentiation would be useful. DRM_MODE_DRIVER is
being used by some other places apart from freesync, so it might not be a
unique identifier. So my recommendation would be to keep this.
My comment was, if we have already parsed the whole connector list once, and added the
mode, there should be a better way of doing it instead of checking it again by calling
"get_highest_freesync_mod"
Some things I can think on top of my mind would be:
- Add a read-only amdgpu driver private flag (not DRM flag), while adding a new
freesync mode, which will uniquely identify if a mode is FS mode. On modeset,
you have to just check that flag.
- As we are not handling a lot of modes, cache the FS modes locally and check
only from that DB (instead of the whole modelist)
- Cache the VIC of the mode (if available) and then look into the VIC table
(not sure if detailed modes provide VIC, like CEA-861 modes)
or something better than this.
- Shashank
I'd rather we not make mode name part of a UAPI or to identify a
"FreeSync mode". This is already behind a module option and from the
driver's perspective we only need the timing to understand whether or
not we can do an optimized modeset using FreeSync into it. Driver
private flags can optimize the check away but it's only a few
comparisons so I don't see much benefit.
We will always need to reference the original preferred mode regardless
of how the FreeSync mode is identified since there could be a case where
we're enabling the CRTC from disabled -> enabled. The display was
previously blank and we need to reprogram the OTG timing to the mode
that doesn't have
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
Am 11.12.20 um 14:58 schrieb Michel Dänzer: On 2020-12-14 9:57 p.m., Christian König wrote: Am 11.12.20 um 13:20 schrieb Pekka Paalanen: On Fri, 11 Dec 2020 11:28:36 +0100 Christian König wrote: I think the general idea we settled on is that we specify an earliest display time for each frame and give feedback to the application when a frame was actually displayed. That is indeed something completely different, and feels like it would be several years in the future, while the proposed scheme or the min/max properties could be done fairly quickly. But I'm not in a hurry. X11 already supports that with the DRI3 extension. Also see VDPAU present and the Vulkan extension for reference application API implementations, so that stuff is already present. I suspect you mean the Present extension, specifically PresentOptionUST. There is no working implementation of this yet (the xserver tree has never had any code which would even advertise PresentCapabilityUST, let alone do anything with a timestamp passed in by the client). Good point, what I wanted to say is that this is already specified in those extensions. What we are missing is somehow wiring it all together and see how it works. Christian. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
On 2020-12-14 9:57 p.m., Christian König wrote: Am 11.12.20 um 13:20 schrieb Pekka Paalanen: On Fri, 11 Dec 2020 11:28:36 +0100 Christian König wrote: I think the general idea we settled on is that we specify an earliest display time for each frame and give feedback to the application when a frame was actually displayed. That is indeed something completely different, and feels like it would be several years in the future, while the proposed scheme or the min/max properties could be done fairly quickly. But I'm not in a hurry. X11 already supports that with the DRI3 extension. Also see VDPAU present and the Vulkan extension for reference application API implementations, so that stuff is already present. I suspect you mean the Present extension, specifically PresentOptionUST. There is no working implementation of this yet (the xserver tree has never had any code which would even advertise PresentCapabilityUST, let alone do anything with a timestamp passed in by the client). -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
On Mon, 14 Dec 2020 21:57:25 +0100 Christian König wrote: > Am 11.12.20 um 13:20 schrieb Pekka Paalanen: > > On Fri, 11 Dec 2020 11:28:36 +0100 > > Christian König wrote: > > > >> Am 11.12.20 um 10:55 schrieb Pekka Paalanen: > >>> On Fri, 11 Dec 2020 09:56:07 +0530 > >>> Shashank Sharma wrote: > >>> > Hello Simon, > > Hope you are doing well, > > I was helping out Aurabindo and the team with the design, so I have > taken the liberty of adding some comments on behalf of the team, > Inline. > > On 11/12/20 3:31 am, Simon Ser wrote: > > Hi, > > > > (CC dri-devel, Pekka and Martin who might be interested in this as > > well.) > >>> Thanks for the Cc! This is very interesting to me, and because it was > >>> not Cc'd to dri-devel@ originally, I would have missed this otherwise. > >>> > > On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai > > wrote: > > > >> This patchset enables freesync video mode usecase where the userspace > >> can request a freesync compatible video mode such that switching to > >> this > >> mode does not trigger blanking. > >> > >> This feature is guarded by a module parameter which is disabled by > >> default. Enabling this paramters adds additional modes to the driver > >> modelist, and also enables the optimization to skip modeset when using > >> one of these modes. > > Thanks for working on this, it's an interesting feature! However I'd > > like to > > take some time to think about the user-space API for this. > > > > As I understand it, some new synthetic modes are added, and user-space > > can > > perform a test-only atomic *without* ALLOW_MODESET to figure out > > whether it can > > switch to a mode without blanking the screen. > The implementation is in those lines, but a bit different. The idea > is to: > > - check if the monitor supports VRR, > > - If it does, add some new modes which are in the VRR tolerance > range, as new video modes in the list (with driver flag). > > - when you get modeset on any of these modes, skip the full modeset, > and just adjust the front_porch timing > > so they are not test-only as such, for any user-space these modes > will be as real as any other probed modes of the list. > >>> But is it worth to allow a modeset to be glitch-free if the userspace > >>> does not know they are glitch-free? I think if this is going in, it > >>> would be really useful to give the guarantees to userspace explicitly, > >>> and not leave this feature at an "accidentally no glitch sometimes" > >>> level. > >>> > >>> > >>> I have been expecting and hoping for the ability to change video mode > >>> timings without a modeset ever since I learnt that VRR is about > >>> front-porch adjustment, quite a while ago. > >>> > >>> This is how I envision userspace making use of it: > >>> > >>> Let us have a Wayland compositor, which uses fixed-frequency video > >>> modes, because it wants predictable vblank cycles. IOW, it will not > >>> enable VRR as such. > >> Well in general please keep in mind that this is just a short term > >> solution for X11 applications. > > I guess someone should have mentioned that. :-) > > > > Do we really want to add more Xorg-only features in the kernel? > > Well, my preferred solution was to add the mode in userspace instead :) > > > It feels like "it's a short term solution for X11" could be almost used > > as an excuse to avoid proper uAPI design process. However, with uAPI > > there is no "short term". Once it's in, it's there for decades. So why > > does it matter if you design it for Xorg foremost? Are others forbidden > > to make use of it? Or do you deliberately want to design it so that > > it's not generally useful and it will indeed end up being a short term > > feature? Planned obsolescence from the start? > > Yes exactly. We need something which works for now and can be removed > again later on when we have a better solution. Adding some extra modes > is not considered UAPI since both displays and drivers are doing this > for a couple of different purposes already. > > Another main reason is that this approach works with existing media > players like mpv and kodi without changing them because we do pretty > much the same thing in the kernel which TVs do in their EDID. > > E.g. when starting to play a video kodi will automatically try to switch > to a mode which has the same fps as the video. Aha! That is very valuable information to put this proposal into perspective. I'll leave you to it, then. :-) Thanks, pq pgpAvNm2ZCg1F.pgp Description: OpenPGP digital signature ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
Am 11.12.20 um 13:20 schrieb Pekka Paalanen: On Fri, 11 Dec 2020 11:28:36 +0100 Christian König wrote: Am 11.12.20 um 10:55 schrieb Pekka Paalanen: On Fri, 11 Dec 2020 09:56:07 +0530 Shashank Sharma wrote: Hello Simon, Hope you are doing well, I was helping out Aurabindo and the team with the design, so I have taken the liberty of adding some comments on behalf of the team, Inline. On 11/12/20 3:31 am, Simon Ser wrote: Hi, (CC dri-devel, Pekka and Martin who might be interested in this as well.) Thanks for the Cc! This is very interesting to me, and because it was not Cc'd to dri-devel@ originally, I would have missed this otherwise. On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai wrote: This patchset enables freesync video mode usecase where the userspace can request a freesync compatible video mode such that switching to this mode does not trigger blanking. This feature is guarded by a module parameter which is disabled by default. Enabling this paramters adds additional modes to the driver modelist, and also enables the optimization to skip modeset when using one of these modes. Thanks for working on this, it's an interesting feature! However I'd like to take some time to think about the user-space API for this. As I understand it, some new synthetic modes are added, and user-space can perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can switch to a mode without blanking the screen. The implementation is in those lines, but a bit different. The idea is to: - check if the monitor supports VRR, - If it does, add some new modes which are in the VRR tolerance range, as new video modes in the list (with driver flag). - when you get modeset on any of these modes, skip the full modeset, and just adjust the front_porch timing so they are not test-only as such, for any user-space these modes will be as real as any other probed modes of the list. But is it worth to allow a modeset to be glitch-free if the userspace does not know they are glitch-free? I think if this is going in, it would be really useful to give the guarantees to userspace explicitly, and not leave this feature at an "accidentally no glitch sometimes" level. I have been expecting and hoping for the ability to change video mode timings without a modeset ever since I learnt that VRR is about front-porch adjustment, quite a while ago. This is how I envision userspace making use of it: Let us have a Wayland compositor, which uses fixed-frequency video modes, because it wants predictable vblank cycles. IOW, it will not enable VRR as such. Well in general please keep in mind that this is just a short term solution for X11 applications. I guess someone should have mentioned that. :-) Do we really want to add more Xorg-only features in the kernel? Well, my preferred solution was to add the mode in userspace instead :) It feels like "it's a short term solution for X11" could be almost used as an excuse to avoid proper uAPI design process. However, with uAPI there is no "short term". Once it's in, it's there for decades. So why does it matter if you design it for Xorg foremost? Are others forbidden to make use of it? Or do you deliberately want to design it so that it's not generally useful and it will indeed end up being a short term feature? Planned obsolescence from the start? Yes exactly. We need something which works for now and can be removed again later on when we have a better solution. Adding some extra modes is not considered UAPI since both displays and drivers are doing this for a couple of different purposes already. Another main reason is that this approach works with existing media players like mpv and kodi without changing them because we do pretty much the same thing in the kernel which TVs do in their EDID. E.g. when starting to play a video kodi will automatically try to switch to a mode which has the same fps as the video. For things like Wayland we probably want to approach this from a completely different vector. When the Wayland compositor starts, it will choose *some* video mode for an output. It may or may not be what a KMS driver calls "preferred mode", because it depends on things like user preferences. The compositor makes the initial modeset to this mode. I think the general idea we settled on is that we specify an earliest display time for each frame and give feedback to the application when a frame was actually displayed. That is indeed something completely different, and feels like it would be several years in the future, while the proposed scheme or the min/max properties could be done fairly quickly. But I'm not in a hurry. X11 already supports that with the DRI3 extension. Also see VDPAU present and the Vulkan extension for reference application API implementations, so that stuff is already present. It's just not wired up correctly with KMS and we don't have anything similar in Wayland as far as I know.
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
On Fri, 11 Dec 2020 11:28:36 +0100 Christian König wrote: > Am 11.12.20 um 10:55 schrieb Pekka Paalanen: > > On Fri, 11 Dec 2020 09:56:07 +0530 > > Shashank Sharma wrote: > > > >> Hello Simon, > >> > >> Hope you are doing well, > >> > >> I was helping out Aurabindo and the team with the design, so I have > >> taken the liberty of adding some comments on behalf of the team, > >> Inline. > >> > >> On 11/12/20 3:31 am, Simon Ser wrote: > >>> Hi, > >>> > >>> (CC dri-devel, Pekka and Martin who might be interested in this as > >>> well.) > > Thanks for the Cc! This is very interesting to me, and because it was > > not Cc'd to dri-devel@ originally, I would have missed this otherwise. > > > >>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai > >>> wrote: > >>> > This patchset enables freesync video mode usecase where the userspace > can request a freesync compatible video mode such that switching to this > mode does not trigger blanking. > > This feature is guarded by a module parameter which is disabled by > default. Enabling this paramters adds additional modes to the driver > modelist, and also enables the optimization to skip modeset when using > one of these modes. > >>> Thanks for working on this, it's an interesting feature! However I'd like > >>> to > >>> take some time to think about the user-space API for this. > >>> > >>> As I understand it, some new synthetic modes are added, and user-space can > >>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether > >>> it can > >>> switch to a mode without blanking the screen. > >> The implementation is in those lines, but a bit different. The idea > >> is to: > >> > >> - check if the monitor supports VRR, > >> > >> - If it does, add some new modes which are in the VRR tolerance > >> range, as new video modes in the list (with driver flag). > >> > >> - when you get modeset on any of these modes, skip the full modeset, > >> and just adjust the front_porch timing > >> > >> so they are not test-only as such, for any user-space these modes > >> will be as real as any other probed modes of the list. > > But is it worth to allow a modeset to be glitch-free if the userspace > > does not know they are glitch-free? I think if this is going in, it > > would be really useful to give the guarantees to userspace explicitly, > > and not leave this feature at an "accidentally no glitch sometimes" > > level. > > > > > > I have been expecting and hoping for the ability to change video mode > > timings without a modeset ever since I learnt that VRR is about > > front-porch adjustment, quite a while ago. > > > > This is how I envision userspace making use of it: > > > > Let us have a Wayland compositor, which uses fixed-frequency video > > modes, because it wants predictable vblank cycles. IOW, it will not > > enable VRR as such. > > Well in general please keep in mind that this is just a short term > solution for X11 applications. I guess someone should have mentioned that. :-) Do we really want to add more Xorg-only features in the kernel? It feels like "it's a short term solution for X11" could be almost used as an excuse to avoid proper uAPI design process. However, with uAPI there is no "short term". Once it's in, it's there for decades. So why does it matter if you design it for Xorg foremost? Are others forbidden to make use of it? Or do you deliberately want to design it so that it's not generally useful and it will indeed end up being a short term feature? Planned obsolescence from the start? > For things like Wayland we probably want to approach this from a > completely different vector. > > > When the Wayland compositor starts, it will choose *some* video mode > > for an output. It may or may not be what a KMS driver calls "preferred > > mode", because it depends on things like user preferences. The > > compositor makes the initial modeset to this mode. > > I think the general idea we settled on is that we specify an earliest > display time for each frame and give feedback to the application when a > frame was actually displayed. That is indeed something completely different, and feels like it would be several years in the future, while the proposed scheme or the min/max properties could be done fairly quickly. But I'm not in a hurry. Setting the earliest display time for a flip does not fully cover what video mode timing adjustment or min/max frame time or refresh rate property would offer: prediction of when the next flip can happen. Choosing display times requires knowing the possible display times, so something more is needed. The min/max properties would fit in. > This approach should also be able to handle multiple applications with > different refresh rates. E.g. just think of a video playback with 25 and > another one with 30 Hz in two windows when the max refresh rate is > something like 120Hz. But that's just an extension to what I
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
Am 11.12.20 um 10:55 schrieb Pekka Paalanen: On Fri, 11 Dec 2020 09:56:07 +0530 Shashank Sharma wrote: Hello Simon, Hope you are doing well, I was helping out Aurabindo and the team with the design, so I have taken the liberty of adding some comments on behalf of the team, Inline. On 11/12/20 3:31 am, Simon Ser wrote: Hi, (CC dri-devel, Pekka and Martin who might be interested in this as well.) Thanks for the Cc! This is very interesting to me, and because it was not Cc'd to dri-devel@ originally, I would have missed this otherwise. On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai wrote: This patchset enables freesync video mode usecase where the userspace can request a freesync compatible video mode such that switching to this mode does not trigger blanking. This feature is guarded by a module parameter which is disabled by default. Enabling this paramters adds additional modes to the driver modelist, and also enables the optimization to skip modeset when using one of these modes. Thanks for working on this, it's an interesting feature! However I'd like to take some time to think about the user-space API for this. As I understand it, some new synthetic modes are added, and user-space can perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can switch to a mode without blanking the screen. The implementation is in those lines, but a bit different. The idea is to: - check if the monitor supports VRR, - If it does, add some new modes which are in the VRR tolerance range, as new video modes in the list (with driver flag). - when you get modeset on any of these modes, skip the full modeset, and just adjust the front_porch timing so they are not test-only as such, for any user-space these modes will be as real as any other probed modes of the list. But is it worth to allow a modeset to be glitch-free if the userspace does not know they are glitch-free? I think if this is going in, it would be really useful to give the guarantees to userspace explicitly, and not leave this feature at an "accidentally no glitch sometimes" level. I have been expecting and hoping for the ability to change video mode timings without a modeset ever since I learnt that VRR is about front-porch adjustment, quite a while ago. This is how I envision userspace making use of it: Let us have a Wayland compositor, which uses fixed-frequency video modes, because it wants predictable vblank cycles. IOW, it will not enable VRR as such. Well in general please keep in mind that this is just a short term solution for X11 applications. For things like Wayland we probably want to approach this from a completely different vector. When the Wayland compositor starts, it will choose *some* video mode for an output. It may or may not be what a KMS driver calls "preferred mode", because it depends on things like user preferences. The compositor makes the initial modeset to this mode. I think the general idea we settled on is that we specify an earliest display time for each frame and give feedback to the application when a frame was actually displayed. This approach should also be able to handle multiple applications with different refresh rates. E.g. just think of a video playback with 25 and another one with 30 Hz in two windows when the max refresh rate is something like 120Hz. Regards, Christian. Use case 1: A Wayland client comes up and determines that its window would really like a refresh rate of, say, 47.5 Hz. Yes, it's not a traditional video player rate, but let's assume the application has its reasons. The client tells the compositor this (Wayland protocol still to be designed to be able to do that). (Hey, this could be how future games should implement refresh rate controls in cooperation with the window system.) The compositor sees the wish, and according to its complex policy rules, determines that yes, it shall try to honor that wish by changing the whole output temporarily to 47.5 Hz if possible. The compositor takes the original video mode it modeset on the output, and adjusts the front-porch to create a new custom 47.5 Hz mode. Using this mode, the compositor does a TEST_ONLY atomic commit *without* ALLOW_MODESET. If the test commit succeeds, the compositor knows that changing timings will not cause any kind of glitch, flicker, blanking, or freeze, and proceeds to commit this video mode without ALLOW_MODESET. The whole output becomes 47.5 Hz until the compositor policy again determines that it is time to change, e.g. to go back to the original mode. Going back to the original mode also needs to work without ALLOW_MODESET - but a compositor cannot test for this with atomic TEST_ONLY commits. If the test commit fails, the compositor knows that it cannot change the timings like this without risking a visible glitch. Therefore the compositor does not change the video mode timings, and the client's wish is not granted. The client adapts to
[bug report] drm/amd/display: Implement VSIF V3 extended refresh rate feature
Hello Reza Amini, The patch 9bc416266582: "drm/amd/display: Implement VSIF V3 extended refresh rate feature" from Jul 9, 2020, leads to the following static checker warning: drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:617 build_vrr_infopacket_data_v3() warn: both sides of ternary the same: 'max_refresh' max_refresh max_refresh drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c 606 607 min_refresh = (vrr->min_refresh_in_uhz + 50) / 100; 608 max_refresh = (vrr->max_refresh_in_uhz + 50) / 100; 609 fixed_refresh = (vrr->fixed_refresh_in_uhz + 50) / 100; 610 611 min_programmed = (vrr->state == VRR_STATE_ACTIVE_FIXED) ? fixed_refresh : 612 (vrr->state == VRR_STATE_ACTIVE_VARIABLE) ? min_refresh : 613 (vrr->state == VRR_STATE_INACTIVE) ? min_refresh : 614 max_refresh; // Non-fs case, program nominal range 615 616 max_programmed = (vrr->state == VRR_STATE_ACTIVE_FIXED) ? fixed_refresh : 617 (vrr->state == VRR_STATE_ACTIVE_VARIABLE) ? max_refresh : ^^^ Probably "min_refresh" was intended here? 618 max_refresh;// Non-fs case, program nominal range ^^^ 619 620 /* PB7 = FreeSync Minimum refresh rate (Hz) */ 621 infopacket->sb[7] = min_programmed & 0xFF; 622 623 /* PB8 = FreeSync Maximum refresh rate (Hz) */ 624 infopacket->sb[8] = max_programmed & 0xFF; 625 regards, dan carpenter ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
On Fri, 11 Dec 2020 09:56:07 +0530 Shashank Sharma wrote: > Hello Simon, > > Hope you are doing well, > > I was helping out Aurabindo and the team with the design, so I have > taken the liberty of adding some comments on behalf of the team, > Inline. > > On 11/12/20 3:31 am, Simon Ser wrote: > > Hi, > > > > (CC dri-devel, Pekka and Martin who might be interested in this as > > well.) Thanks for the Cc! This is very interesting to me, and because it was not Cc'd to dri-devel@ originally, I would have missed this otherwise. > > > > On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai > > wrote: > > > >> This patchset enables freesync video mode usecase where the userspace > >> can request a freesync compatible video mode such that switching to this > >> mode does not trigger blanking. > >> > >> This feature is guarded by a module parameter which is disabled by > >> default. Enabling this paramters adds additional modes to the driver > >> modelist, and also enables the optimization to skip modeset when using > >> one of these modes. > > Thanks for working on this, it's an interesting feature! However I'd like to > > take some time to think about the user-space API for this. > > > > As I understand it, some new synthetic modes are added, and user-space can > > perform a test-only atomic *without* ALLOW_MODESET to figure out whether it > > can > > switch to a mode without blanking the screen. > > The implementation is in those lines, but a bit different. The idea > is to: > > - check if the monitor supports VRR, > > - If it does, add some new modes which are in the VRR tolerance > range, as new video modes in the list (with driver flag). > > - when you get modeset on any of these modes, skip the full modeset, > and just adjust the front_porch timing > > so they are not test-only as such, for any user-space these modes > will be as real as any other probed modes of the list. But is it worth to allow a modeset to be glitch-free if the userspace does not know they are glitch-free? I think if this is going in, it would be really useful to give the guarantees to userspace explicitly, and not leave this feature at an "accidentally no glitch sometimes" level. I have been expecting and hoping for the ability to change video mode timings without a modeset ever since I learnt that VRR is about front-porch adjustment, quite a while ago. This is how I envision userspace making use of it: Let us have a Wayland compositor, which uses fixed-frequency video modes, because it wants predictable vblank cycles. IOW, it will not enable VRR as such. When the Wayland compositor starts, it will choose *some* video mode for an output. It may or may not be what a KMS driver calls "preferred mode", because it depends on things like user preferences. The compositor makes the initial modeset to this mode. Use case 1: A Wayland client comes up and determines that its window would really like a refresh rate of, say, 47.5 Hz. Yes, it's not a traditional video player rate, but let's assume the application has its reasons. The client tells the compositor this (Wayland protocol still to be designed to be able to do that). (Hey, this could be how future games should implement refresh rate controls in cooperation with the window system.) The compositor sees the wish, and according to its complex policy rules, determines that yes, it shall try to honor that wish by changing the whole output temporarily to 47.5 Hz if possible. The compositor takes the original video mode it modeset on the output, and adjusts the front-porch to create a new custom 47.5 Hz mode. Using this mode, the compositor does a TEST_ONLY atomic commit *without* ALLOW_MODESET. If the test commit succeeds, the compositor knows that changing timings will not cause any kind of glitch, flicker, blanking, or freeze, and proceeds to commit this video mode without ALLOW_MODESET. The whole output becomes 47.5 Hz until the compositor policy again determines that it is time to change, e.g. to go back to the original mode. Going back to the original mode also needs to work without ALLOW_MODESET - but a compositor cannot test for this with atomic TEST_ONLY commits. If the test commit fails, the compositor knows that it cannot change the timings like this without risking a visible glitch. Therefore the compositor does not change the video mode timings, and the client's wish is not granted. The client adapts to whatever the refresh rate is in any case. Use case 2: A client comes up, and starts presenting frames with a target timestamp (Wayland protocol for this still to be designed). The compositor analyzes the target timestamp, and according to the complex compositor policy, determines that it should try to adjust video mode timings to better meet the target timestamps. Like in use case 1, the compositor creates a new custom video mode and tests if it can be applied without any glitch. If yes, it is used. If not, it is not used. This use case is
Re: [PATCH 4/7] amdgpu: module option controlling whether BAR0 resizing is done
NAK, that is exactly what we are trying to avoid.
Am 11.12.20 um 01:55 schrieb Darren Salt:
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 9 +
3 files changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index c228e7470d51..2efce7fa6a4b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -201,6 +201,7 @@ static const bool __maybe_unused no_system_mem_limit;
extern int amdgpu_tmz;
extern int amdgpu_reset_method;
+extern bool amdgpu_resize_bar;
#ifdef CONFIG_DRM_AMDGPU_SI
extern int amdgpu_si_support;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 1e99ca62a4d2..292796e9f83d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1115,6 +1115,9 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device
*adev)
int r;
bool nospc = false;
+ if (!amdgpu_resize_bar)
+ return 0;
+
/* Bypass for VF */
if (amdgpu_sriov_vf(adev))
return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index cac2724e7615..6df33df74775 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -161,6 +161,7 @@ int amdgpu_force_asic_type = -1;
int amdgpu_tmz;
int amdgpu_reset_method = -1; /* auto */
int amdgpu_num_kcq = -1;
+bool amdgpu_resize_bar = true;
struct amdgpu_mgpu_info mgpu_info = {
.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
@@ -807,6 +808,14 @@ module_param_named(bad_page_threshold,
amdgpu_bad_page_threshold, int, 0444);
MODULE_PARM_DESC(num_kcq, "number of kernel compute queue user want to setup (8 if
set to greater than 8 or less than 0, only affect gfx 8+)");
module_param_named(num_kcq, amdgpu_num_kcq, int, 0444);
+/**
+ * DOC: resize_bar (bool)
+ * Control whether FB BAR should be resized.
+ * Enabled by default.
+ */
+module_param_named(resize_bar, amdgpu_resize_bar, bool, 0444);
+MODULE_PARM_DESC(resize_bar, "Controls whether the FB BAR should be resized
(default = true).");
+
static const struct pci_device_id pciidlist[] = {
#ifdef CONFIG_DRM_AMDGPU_SI
{0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TAHITI},
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/7] pci: export PCI BAR size-reading functions
Am 11.12.20 um 01:55 schrieb Darren Salt:
This is to assist driver modules which do BAR resizing.
Signed-off-by: Darren Salt
---
drivers/pci/pci.c | 2 ++
drivers/pci/pci.h | 2 --
include/linux/pci.h | 4
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e578d34095e9..3f6042d9ad83 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3579,6 +3579,7 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev,
int bar)
pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, );
return (cap & PCI_REBAR_CAP_SIZES) >> 4;
}
+EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
/**
* pci_rebar_get_current_size - get the current size of a BAR
@@ -3600,6 +3601,7 @@ int pci_rebar_get_current_size(struct pci_dev *pdev, int
bar)
pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, );
return (ctrl & PCI_REBAR_CTRL_BAR_SIZE) >> PCI_REBAR_CTRL_BAR_SHIFT;
}
+EXPORT_SYMBOL(pci_rebar_get_current_size);
This is unnecessary. You can just look at the resource size instead
which is also more defensive regarding problems/errors.
Christian.
/**
* pci_rebar_set_size - set a new size for a BAR
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f86cae9aa1f4..8373d94414e9 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -608,8 +608,6 @@ int acpi_get_rc_resources(struct device *dev, const char
*hid, u16 segment,
struct resource *res);
#endif
-u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar);
-int pci_rebar_get_current_size(struct pci_dev *pdev, int bar);
int pci_rebar_set_size(struct pci_dev *pdev, int bar, int size);
static inline u64 pci_rebar_size_to_bytes(int size)
{
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 22207a79762c..5aa035622741 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1226,7 +1226,11 @@ void pci_update_resource(struct pci_dev *dev, int resno);
int __must_check pci_assign_resource(struct pci_dev *dev, int i);
int __must_check pci_reassign_resource(struct pci_dev *dev, int i,
resource_size_t add_size, resource_size_t align);
void pci_release_resource(struct pci_dev *dev, int resno);
+
+u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar);
+int pci_rebar_get_current_size(struct pci_dev *pdev, int bar);
int __must_check pci_resize_resource(struct pci_dev *dev, int i, int size);
+
int pci_select_bars(struct pci_dev *dev, unsigned long flags);
bool pci_device_is_present(struct pci_dev *pdev);
void pci_ignore_hotplug(struct pci_dev *dev);
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: skip vram operation for BAMACO runtime
From: Likun Gao
Skip vram related operation for bamaco rumtime suspend and resume as
vram is alive when BAMACO.
It can save about 32ms when suspend and about 15ms when resume.
Signed-off-by: Likun Gao
Change-Id: I6ad39765de5ed1aac2dc51e96ed7a21a727272cd
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 9 ++-
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c| 72 +-
2 files changed, 50 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 0ec7c28c4d5a..66b790dfb151 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2464,7 +2464,8 @@ static int amdgpu_device_ip_late_init(struct
amdgpu_device *adev)
amdgpu_device_set_cg_state(adev, AMD_CG_STATE_GATE);
amdgpu_device_set_pg_state(adev, AMD_PG_STATE_GATE);
- amdgpu_device_fill_reset_magic(adev);
+ if ((amdgpu_runtime_pm != 2) || !adev->in_runpm)
+ amdgpu_device_fill_reset_magic(adev);
r = amdgpu_device_enable_mgpu_fan_boost();
if (r)
@@ -3706,7 +3707,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
amdgpu_amdkfd_suspend(adev, !fbcon);
/* evict vram memory */
- amdgpu_bo_evict_vram(adev);
+ if ((amdgpu_runtime_pm != 2) || !adev->in_runpm)
+ amdgpu_bo_evict_vram(adev);
amdgpu_fence_driver_suspend(adev);
@@ -3718,7 +3720,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
* This second call to evict vram is to evict the gart page table
* using the CPU.
*/
- amdgpu_bo_evict_vram(adev);
+ if ((amdgpu_runtime_pm != 2) || !adev->in_runpm)
+ amdgpu_bo_evict_vram(adev);
return 0;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 523d22db094b..67e74b43a1ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -397,10 +397,12 @@ static int psp_tmr_init(struct psp_context *psp)
}
}
- pptr = amdgpu_sriov_vf(psp->adev) ? _buf : NULL;
- ret = amdgpu_bo_create_kernel(psp->adev, tmr_size, PSP_TMR_SIZE,
+ if ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm) {
+ pptr = amdgpu_sriov_vf(psp->adev) ? _buf : NULL;
+ ret = amdgpu_bo_create_kernel(psp->adev, tmr_size, PSP_TMR_SIZE,
AMDGPU_GEM_DOMAIN_VRAM,
>tmr_bo, >tmr_mc_addr, pptr);
+ }
return ret;
}
@@ -504,8 +506,10 @@ static int psp_tmr_terminate(struct psp_context *psp)
return ret;
/* free TMR memory buffer */
- pptr = amdgpu_sriov_vf(psp->adev) ? _buf : NULL;
- amdgpu_bo_free_kernel(>tmr_bo, >tmr_mc_addr, pptr);
+ if ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm) {
+ pptr = amdgpu_sriov_vf(psp->adev) ? _buf : NULL;
+ amdgpu_bo_free_kernel(>tmr_bo, >tmr_mc_addr, pptr);
+ }
return 0;
}
@@ -795,9 +799,10 @@ int psp_xgmi_terminate(struct psp_context *psp)
psp->xgmi_context.initialized = 0;
/* free xgmi shared memory */
- amdgpu_bo_free_kernel(>xgmi_context.xgmi_shared_bo,
- >xgmi_context.xgmi_shared_mc_addr,
- >xgmi_context.xgmi_shared_buf);
+ if ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm)
+ amdgpu_bo_free_kernel(>xgmi_context.xgmi_shared_bo,
+ >xgmi_context.xgmi_shared_mc_addr,
+ >xgmi_context.xgmi_shared_buf);
return 0;
}
@@ -812,7 +817,8 @@ int psp_xgmi_initialize(struct psp_context *psp)
!psp->adev->psp.ta_xgmi_start_addr)
return -ENOENT;
- if (!psp->xgmi_context.initialized) {
+ if (!psp->xgmi_context.initialized &&
+ ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm)) {
ret = psp_xgmi_init_shared_buf(psp);
if (ret)
return ret;
@@ -1122,9 +1128,10 @@ static int psp_ras_terminate(struct psp_context *psp)
psp->ras.ras_initialized = false;
/* free ras shared memory */
- amdgpu_bo_free_kernel(>ras.ras_shared_bo,
- >ras.ras_shared_mc_addr,
- >ras.ras_shared_buf);
+ if ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm)
+ amdgpu_bo_free_kernel(>ras.ras_shared_bo,
+ >ras.ras_shared_mc_addr,
+ >ras.ras_shared_buf);
return 0;
}
@@ -1145,7 +1152,8 @@ static int psp_ras_initialize(struct psp_context *psp)
return 0;
}
- if (!psp->ras.ras_initialized) {
+ if (!psp->ras.ras_initialized &&
+ ((amdgpu_runtime_pm != 2) || !psp->adev->in_runpm))
[V2] drm/amdgpu: add judgement for suspend/resume sequence
From: Likun Gao
S0ix only makes sense on APUs since they are part of the platform, so
only when the ASIC is APU should set amdgpu_acpi_is_s0ix_supported flag
to deal with the related situation.
Signed-off-by: Likun Gao
Change-Id: Ic89df206734fa7e6ce3e5a784171f149a07edc80
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h| 4 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 +---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 +++---
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 83ac06a3ec05..eed5410947e9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1314,11 +1314,11 @@ int amdgpu_acpi_pcie_notify_device_ready(struct
amdgpu_device *adev);
void amdgpu_acpi_get_backlight_caps(struct amdgpu_device *adev,
struct amdgpu_dm_backlight_caps *caps);
-bool amdgpu_acpi_is_s0ix_supported(void);
+bool amdgpu_acpi_is_s0ix_supported(struct amdgpu_device *adev);
#else
static inline int amdgpu_acpi_init(struct amdgpu_device *adev) { return 0; }
static inline void amdgpu_acpi_fini(struct amdgpu_device *adev) { }
-static inline bool amdgpu_acpi_is_s0ix_supported(void) { return false; }
+static inline bool amdgpu_acpi_is_s0ix_supported(struct amdgpu_device *adev) {
return false; }
#endif
int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index fd66ac00c7f5..8155c54392c8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -901,10 +901,12 @@ void amdgpu_acpi_fini(struct amdgpu_device *adev)
*
* returns true if supported, false if not.
*/
-bool amdgpu_acpi_is_s0ix_supported()
+bool amdgpu_acpi_is_s0ix_supported(struct amdgpu_device *adev)
{
- if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
- return true;
+ if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
+ if (adev->flags & AMD_IS_APU)
+ return true;
+ }
return false;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 52d6f1fbe890..66b790dfb151 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2651,7 +2651,7 @@ static int amdgpu_device_ip_suspend_phase1(struct
amdgpu_device *adev)
{
int i, r;
- if (!amdgpu_acpi_is_s0ix_supported() || amdgpu_in_reset(adev)) {
+ if (!amdgpu_acpi_is_s0ix_supported(adev) || amdgpu_in_reset(adev)) {
amdgpu_device_set_pg_state(adev, AMD_PG_STATE_UNGATE);
amdgpu_device_set_cg_state(adev, AMD_CG_STATE_UNGATE);
}
@@ -3712,7 +3712,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
amdgpu_fence_driver_suspend(adev);
- if (!amdgpu_acpi_is_s0ix_supported() || amdgpu_in_reset(adev))
+ if (!amdgpu_acpi_is_s0ix_supported(adev) || amdgpu_in_reset(adev))
r = amdgpu_device_ip_suspend_phase2(adev);
else
amdgpu_gfx_state_change_set(adev, sGpuChangeState_D3Entry);
@@ -3747,7 +3747,7 @@ int amdgpu_device_resume(struct drm_device *dev, bool
fbcon)
if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- if (amdgpu_acpi_is_s0ix_supported())
+ if (amdgpu_acpi_is_s0ix_supported(adev))
amdgpu_gfx_state_change_set(adev, sGpuChangeState_D0Entry);
/* post card */
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
