Re: [PATCH] drm/amdkfd: Move process doorbell allocation into kfd device
Am 2020-09-01 um 8:21 p.m. schrieb Mukul Joshi:
> Move doorbell allocation for a process into kfd device and
> allocate doorbell space in each PDD during process creation.
> Currently, KFD manages its own doorbell space but for some
> devices, amdgpu would allocate the complete doorbell
> space instead of leaving a chunk of doorbell space for KFD to
> manage. In a system with mix of such devices, KFD would need
> to request process doorbell space based on the type of device,
> either from amdgpu or from its own doorbell space.
>
> Signed-off-by: Mukul Joshi
Two nit-picks inline. With those fixed, the patch is
Reviewed-by: Felix Kuehling
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 30 +--
> drivers/gpu/drm/amd/amdkfd/kfd_device.c | 3 ++
> .../drm/amd/amdkfd/kfd_device_queue_manager.c | 3 +-
> drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c | 37 ++-
> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 17 ++---
> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 21 ++-
> 6 files changed, 64 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index b7b16adb0615..b23caa78328b 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1290,18 +1290,6 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file
> *filep,
> return -EINVAL;
> }
>
> - if (flags & KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL) {
> - if (args->size != kfd_doorbell_process_slice(dev))
> - return -EINVAL;
> - offset = kfd_get_process_doorbells(dev, p);
> - } else if (flags & KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP) {
> - if (args->size != PAGE_SIZE)
> - return -EINVAL;
> - offset = amdgpu_amdkfd_get_mmio_remap_phys_addr(dev->kgd);
> - if (!offset)
> - return -ENOMEM;
> - }
> -
> mutex_lock(&p->mutex);
>
> pdd = kfd_bind_process_to_device(dev, p);
> @@ -1310,6 +1298,24 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file
> *filep,
> goto err_unlock;
> }
>
> + if (flags & KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL) {
> + if (args->size != kfd_doorbell_process_slice(dev)) {
> + err = -EINVAL;
> + goto err_unlock;
> + }
> + offset = kfd_get_process_doorbells(dev, pdd);
> + } else if (flags & KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP) {
> + if (args->size != PAGE_SIZE) {
> + err = -EINVAL;
> + goto err_unlock;
> + }
> + offset = amdgpu_amdkfd_get_mmio_remap_phys_addr(dev->kgd);
> + if (!offset) {
> + err = -ENOMEM;
> + goto err_unlock;
> + }
> + }
> +
> err = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
> dev->kgd, args->va_addr, args->size,
> pdd->vm, (struct kgd_mem **) &mem, &offset,
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 0e71a0543f98..a857282f3d09 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -583,6 +583,8 @@ struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd,
>
> atomic_set(&kfd->sram_ecc_flag, 0);
>
> + ida_init(&kfd->doorbell_ida);
> +
> return kfd;
> }
>
> @@ -798,6 +800,7 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
> kfd_interrupt_exit(kfd);
> kfd_topology_remove_device(kfd);
> kfd_doorbell_fini(kfd);
> + ida_destroy(&kfd->doorbell_ida);
> kfd_gtt_sa_fini(kfd);
> amdgpu_amdkfd_free_gtt_mem(kfd->kgd, kfd->gtt_mem);
> if (kfd->gws)
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index 560adc57a050..b9d1359c6fe0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -191,9 +191,8 @@ static int allocate_doorbell(struct qcm_process_device
> *qpd, struct queue *q)
> }
>
> q->properties.doorbell_off =
> - kfd_get_doorbell_dw_offset_in_bar(dev, q->process,
> + kfd_get_doorbell_dw_offset_in_bar(dev, qpd_to_pdd(qpd),
> q->doorbell_id);
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> index 8e0c00b9555e..5946bfb6b75c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
> @@ -31,9 +31,6 @@
> * kernel queues using the first doorbell page reserved for the kernel.
> */
>
> -static DEFINE_IDA(doorbell_ida);
> -st
RE: [PATCH 2/4] drm/amdgpu: fix xgmi perfmon a-b-a problem
[AMD Official Use Only - Internal Distribution Only]
Few minor comments.
-Original Message-
From: amd-gfx On Behalf Of Jonathan Kim
Sent: Tuesday, September 8, 2020 9:06 AM
To: amd-gfx@lists.freedesktop.org
Cc: Kuehling, Felix ; Kim, Jonathan
Subject: [PATCH 2/4] drm/amdgpu: fix xgmi perfmon a-b-a problem
Mapping hw counters per event config will cause ABA problems so map per event
instead.
Signed-off-by: Jonathan Kim
---
drivers/gpu/drm/amd/amdgpu/amdgpu_df.h | 6 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c | 23 --
drivers/gpu/drm/amd/amdgpu/df_v3_6.c| 104 +++-
3 files changed, 65 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_df.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_df.h
index 373cdebe0e2f..52488bb45112 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_df.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_df.h
@@ -44,11 +44,11 @@ struct amdgpu_df_funcs {
void (*enable_ecc_force_par_wr_rmw)(struct amdgpu_device *adev,
bool enable);
int (*pmc_start)(struct amdgpu_device *adev, uint64_t config,
-int is_add);
+int counter_idx, int is_add);
int (*pmc_stop)(struct amdgpu_device *adev, uint64_t config,
-int is_remove);
+int counter_idx, int is_remove);
void (*pmc_get_count)(struct amdgpu_device *adev, uint64_t config,
-uint64_t *count);
+int counter_idx, uint64_t *count);
uint64_t (*get_fica)(struct amdgpu_device *adev, uint32_t ficaa_val);
void (*set_fica)(struct amdgpu_device *adev, uint32_t ficaa_val,
uint32_t ficadl_val, uint32_t ficadh_val); diff --git
a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
index 69af462db34d..915c580d30be 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
@@ -74,9 +74,11 @@ static void amdgpu_perf_start(struct perf_event *event, int
flags)
switch (pe->pmu_perf_type) {
case PERF_TYPE_AMDGPU_DF:
if (!(flags & PERF_EF_RELOAD))
- pe->adev->df.funcs->pmc_start(pe->adev, hwc->config, 1);
+ pe->adev->df.funcs->pmc_start(pe->adev, hwc->config,
+ hwc->idx, 1);
The previous pmc_start() can fail if there is no slot available. The code will
still continue.
- pe->adev->df.funcs->pmc_start(pe->adev, hwc->config, 0);
+ pe->adev->df.funcs->pmc_start(pe->adev, hwc->config,
+ hwc->idx, 0);
break;
default:
break;
@@ -101,8 +103,8 @@ static void amdgpu_perf_read(struct perf_event *event)
switch (pe->pmu_perf_type) {
case PERF_TYPE_AMDGPU_DF:
- pe->adev->df.funcs->pmc_get_count(pe->adev, hwc->config,
- &count);
+ pe->adev->df.funcs->pmc_get_count(pe->adev,
+ hwc->config, hwc->idx, &count);
break;
default:
count = 0;
@@ -126,7 +128,8 @@ static void amdgpu_perf_stop(struct perf_event *event, int
flags)
switch (pe->pmu_perf_type) {
case PERF_TYPE_AMDGPU_DF:
- pe->adev->df.funcs->pmc_stop(pe->adev, hwc->config, 0);
+ pe->adev->df.funcs->pmc_stop(pe->adev, hwc->config, hwc->idx,
+ 0);
break;
default:
break;
@@ -157,7 +160,12 @@ static int amdgpu_perf_add(struct perf_event *event, int
flags)
switch (pe->pmu_perf_type) {
case PERF_TYPE_AMDGPU_DF:
retval = pe->adev->df.funcs->pmc_start(pe->adev,
- hwc->config, 1);
+ hwc->config, hwc->idx, 1);
Passing hwc->idx to pmc_start() is confusing as that variable is not used in
this case. Either add /*used*/ comment and/or pass 0 with /*used*/ comment.
And may be add a small comment saying that when "is_add" == 1, the function
returns a counter slot.
+ if (retval >= 0) {
+ hwc->idx = retval;
+ retval = 0;
+ }
+
break;
default:
return 0;
@@ -185,7 +193,8 @@ static void amdgpu_perf_del(struct perf_event *event, int
flags)
switch (pe->pmu_perf_type) {
case PERF_TYPE_AMDGPU_DF:
- pe->adev->df.funcs->pmc_stop(pe->adev, hwc->config
RE: [PATCH 1/4] drm/amdgpu: stop resetting xgmi perfmons on disable
[AMD Official Use Only - Internal Distribution Only]
Reviewed-by: Harish Kasiviswanathan
-Original Message-
From: amd-gfx On Behalf Of Jonathan Kim
Sent: Tuesday, September 8, 2020 9:06 AM
To: amd-gfx@lists.freedesktop.org
Cc: Kuehling, Felix ; Kim, Jonathan
Subject: [PATCH 1/4] drm/amdgpu: stop resetting xgmi perfmons on disable
Disabling perf events does not specify reset in ABI so stop doing it in
hardware.
Signed-off-by: Jonathan Kim
---
drivers/gpu/drm/amd/amdgpu/amdgpu_df.h | 4 ++--
drivers/gpu/drm/amd/amdgpu/df_v3_6.c | 23 ++-
2 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_df.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_df.h
index 61a26c15c8dd..373cdebe0e2f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_df.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_df.h
@@ -44,9 +44,9 @@ struct amdgpu_df_funcs {
void (*enable_ecc_force_par_wr_rmw)(struct amdgpu_device *adev,
bool enable);
int (*pmc_start)(struct amdgpu_device *adev, uint64_t config,
-int is_enable);
+int is_add);
int (*pmc_stop)(struct amdgpu_device *adev, uint64_t config,
-int is_disable);
+int is_remove);
void (*pmc_get_count)(struct amdgpu_device *adev, uint64_t config,
uint64_t *count);
uint64_t (*get_fica)(struct amdgpu_device *adev, uint32_t ficaa_val);
diff --git a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
index 2eab808fffeb..7b89fd2aa44a 100644
--- a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
+++ b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
@@ -455,7 +455,8 @@ static int df_v3_6_pmc_get_ctrl_settings(struct
amdgpu_device *adev,
uint32_t *lo_base_addr,
uint32_t *hi_base_addr,
uint32_t *lo_val,
- uint32_t *hi_val)
+ uint32_t *hi_val,
+ bool is_enable)
{
uint32_t eventsel, instance, unitmask;
@@ -477,7 +478,8 @@ static int df_v3_6_pmc_get_ctrl_settings(struct
amdgpu_device *adev,
instance_5432 = (instance >> 2) & 0xf;
instance_76 = (instance >> 6) & 0x3;
- *lo_val = (unitmask << 8) | (instance_10 << 6) | eventsel | (1 << 22);
+ *lo_val = (unitmask << 8) | (instance_10 << 6) | eventsel;
+ *lo_val = is_enable ? *lo_val | (1 << 22) : *lo_val & ~(1 << 22);
*hi_val = (instance_76 << 29) | instance_5432;
DRM_DEBUG_DRIVER("config=%llx addr=%08x:%08x val=%08x:%08x",
@@ -572,14 +574,14 @@ static void df_v3_6_reset_perfmon_cntr(struct
amdgpu_device *adev,
}
static int df_v3_6_pmc_start(struct amdgpu_device *adev, uint64_t config,
-int is_enable)
+int is_add)
{
uint32_t lo_base_addr, hi_base_addr, lo_val, hi_val;
int err = 0, ret = 0;
switch (adev->asic_type) {
case CHIP_VEGA20:
- if (is_enable)
+ if (is_add)
return df_v3_6_pmc_add_cntr(adev, config);
df_v3_6_reset_perfmon_cntr(adev, config);
@@ -589,7 +591,8 @@ static int df_v3_6_pmc_start(struct amdgpu_device *adev,
uint64_t config,
&lo_base_addr,
&hi_base_addr,
&lo_val,
- &hi_val);
+ &hi_val,
+ true);
if (ret)
return ret;
@@ -612,7 +615,7 @@ static int df_v3_6_pmc_start(struct amdgpu_device *adev,
uint64_t config,
}
static int df_v3_6_pmc_stop(struct amdgpu_device *adev, uint64_t config,
- int is_disable)
+ int is_remove)
{
uint32_t lo_base_addr, hi_base_addr, lo_val, hi_val;
int ret = 0;
@@ -624,15 +627,17 @@ static int df_v3_6_pmc_stop(struct amdgpu_device *adev,
uint64_t config,
&lo_base_addr,
&hi_base_addr,
&lo_val,
- &hi_val);
+ &hi_val,
+ false);
if (ret)
return ret;
- df_v3_6_reset_perfmon_cntr(adev, config);
- if (is_disable)
+ if (is_remove) {
+ df_v3_6_reset_perfmon_cntr(adev, config);
df_v3_6_pmc_release_cntr(adev, config);
+ }
break;
default:
--
2.17.1
___
Re: [PATCH 1/1] drm/amdgpu: Convert to using devm_drm_dev_alloc()
On Fri, Sep 11, 2020 at 4:50 PM Luben Tuikov wrote:
>
> On 2020-09-08 16:09, Luben Tuikov wrote:
> > On 2020-09-07 04:07, Daniel Vetter wrote:
> >> On Mon, Sep 07, 2020 at 10:06:08AM +0200, Daniel Vetter wrote:
> >>> On Sat, Sep 05, 2020 at 11:50:05AM -0400, Alex Deucher wrote:
> On Thu, Sep 3, 2020 at 9:22 PM Luben Tuikov wrote:
> >
> > Convert to using devm_drm_dev_alloc(),
> > as drm_dev_init() is going away.
> >
> > Signed-off-by: Luben Tuikov
>
> I think we can drop the final drm_put in the error case? I think the
> unwinding in current devm code should take care of it.
> >>>
> >>> Same applies for the pci remove hook too.
> >>
> >> KASAN run with unload should have caught this.
> >
> > But it didn't? Why?
> > Could it be that drm_dev_put() actually got
> > the kref to 0 and then drm_dev_release()
> > was called which did a kfree()?
> >
> > Could you try that same unload KASAN run but
> > with your suggestion of removing drm_dev_put() from
> > amdgpu_pci_remove()? What do you get then?
>
> Hi Daniel,
>
> Have you had a chance to run this unload KASAN run with
> your suggestion of removing drm_dev_put() from
> the PCI release hook?
>
> If it "should have caught this", but it didn't,
> perhaps it did catch it when you removed the drm_dev_put()
> hook from the PCI release hook, when you did a KASAN unload run?
> Showing that drm_dev_put() is still necessary, since,
> 1) we're still using kref,
> 2) kref is kref-init-ed under devm_drm_dev_alloc() as I pointed
>out in my reply to Alex in this thread.
>
> I believe KASAN (and logic) show this patch to be solid.
>
> >
> >> I strongly recommend doing
> >> that for any changes to the unload code, it's way to easy to mix up
> >> something and release it in the wrong order or from the wrong callback or
> >> with the wrong managed (devm_ vs drmm_) functions.
> >
> > Sorry, I don't understand what you mean by "doing that"? Do
> > you mean "not calling drm_dev_put()"? Sure, but what
> > are we supposed to call instead?
> >
> > I also don't understand what you mean by "easy to mix up something
> > and release it in wrong order or from the wrong callback..." etc.
> >
> > If you want things to happen in certain order,
> > you can either put the correct-order-sequence
> > behind the non-zero-->0 transition of kref, say in
> > drm_dev_release() as it is right now,
> >
> > static void drm_dev_release(struct kref *ref)
> > {
> > struct drm_device *dev = container_of(ref, struct drm_device, ref);
> >
> > if (dev->driver->release)
> > dev->driver->release(dev);
> >
> > drm_managed_release(dev);
> >
> > kfree(dev->managed.final_kfree);
> > }
> >
> > Or you can remove kref from DRM dev (which I do not
> > recommend), and stipulate the release sequence
> > as I asked in Message-ID: <165961bb-3b5b-cedc-2fc0-838b7999d...@amd.com>,
> > "Re: [PATCH] drm/managed: Cleanup of unused functions and polishing docs".
> >
> > Then we can follow that and submit patches to conform.
>
> Eagerly awaiting your response on this so that we can conform
> to the direction you're setting forth.
>
> Are you removing kref (release() cb) from DRM and if so,
> what function should we call in order to do the "final"
> (although without kref, the notion of "final" is obviated)
> free, OR kref stays in and this patch, which conforms
> to using devm_drm_dev_alloc(), as postulated by you,
> can go in.
devm_drm_dev_init() calls devm_add_action() which adds
devm_drm_dev_init_release() as the function which gets called for
resource unwinding. That calls drm_dev_put() which handles the ref
counting and clean up, so I don't think we need to call drm_dev_put()
in any of our unwinding paths anymore. All of the drm bits are
handled for us.
Alex
>
> Regards,
> Luben
>
> >
> > Regards,
> > Luben
> >
> >
> >
> >> -Daniel
> >>
> >>> -Daniel
>
> Alex
>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 11 +++
> > 1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 146a85c8df1c..06d994187c24 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -1142,18 +1142,13 @@ static int amdgpu_pci_probe(struct pci_dev
> > *pdev,
> > if (ret)
> > return ret;
> >
> > - adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> > - if (!adev)
> > - return -ENOMEM;
> > + adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver,
> > typeof(*adev), ddev);
> > + if (IS_ERR(adev))
> > + return PTR_ERR(adev);
> >
> > adev->dev = &pdev->dev;
> > adev->pdev = pdev;
> > ddev = adev_to_drm(adev);
> > - ret = drm_dev_init(ddev, &kms_driver, &pdev->dev);
>
Re: [PATCH] drm/amdgpu: Update RAS init handling
[AMD Public Use]
Also, general nit, per kernel coding style, braces should be on the same line
as the if or else, E.g.,
} else {
Alex
From: amd-gfx on behalf of Zhang,
Hawking
Sent: Friday, September 11, 2020 2:02 AM
To: Clements, John ; amd-gfx list
; Chen, Guchun
Subject: RE: [PATCH] drm/amdgpu: Update RAS init handling
[AMD Public Use]
+ {
+ dev_warn(psp->adev->dev, "RAS
Init Status: 0x%X\n", ras_cmd->ras_status);
+ }
Please remove the redundant bracket.
Other than that, the patch is
Reviewed-by: Hawking Zhang
In addition, please create another patch to move the nbio ras controller irq
source registry to sw_init, which is the consistent as what we did for other ip
blocks, register the irq source in IP sw_init funcs.
Regards,
Hawking
From: Clements, John
Sent: Friday, September 11, 2020 12:03
To: amd-gfx list ; Chen, Guchun
; Zhang, Hawking
Subject: [PATCH] drm/amdgpu: Update RAS init handling
[AMD Official Use Only - Internal Distribution Only]
Added RAS status check and tear down RAS context if RAS init fails
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 01/20] drm/amdgpu: Introduce GEM object functions
Am 14.09.20 um 17:05 schrieb Thomas Zimmermann:
Hi
Am 13.08.20 um 12:22 schrieb Christian König:
Am 13.08.20 um 10:36 schrieb Thomas Zimmermann:
GEM object functions deprecate several similar callback interfaces in
struct drm_driver. This patch replaces the per-driver callbacks with
per-instance callbacks in amdgpu. The only exception is gem_prime_mmap,
which is non-trivial to convert.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 --
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 12
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 81a79760ca61..51525b8774c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1468,19 +1468,13 @@ static struct drm_driver kms_driver = {
.lastclose = amdgpu_driver_lastclose_kms,
.irq_handler = amdgpu_irq_handler,
.ioctls = amdgpu_ioctls_kms,
- .gem_free_object_unlocked = amdgpu_gem_object_free,
- .gem_open_object = amdgpu_gem_object_open,
- .gem_close_object = amdgpu_gem_object_close,
.dumb_create = amdgpu_mode_dumb_create,
.dumb_map_offset = amdgpu_mode_dumb_mmap,
.fops = &amdgpu_driver_kms_fops,
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
- .gem_prime_export = amdgpu_gem_prime_export,
.gem_prime_import = amdgpu_gem_prime_import,
- .gem_prime_vmap = amdgpu_gem_prime_vmap,
- .gem_prime_vunmap = amdgpu_gem_prime_vunmap,
.gem_prime_mmap = amdgpu_gem_prime_mmap,
.name = DRIVER_NAME,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 43f4966331dd..ca2b79f94e99 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -36,6 +36,7 @@
#include
#include
#include "amdgpu.h"
+#include "amdgpu_dma_buf.h"
#include "amdgpu_trace.h"
#include "amdgpu_amdkfd.h"
@@ -510,6 +511,15 @@ bool amdgpu_bo_support_uswc(u64 bo_flags)
#endif
}
+static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {
+ .free = amdgpu_gem_object_free,
+ .open = amdgpu_gem_object_open,
+ .close = amdgpu_gem_object_close,
+ .export = amdgpu_gem_prime_export,
+ .vmap = amdgpu_gem_prime_vmap,
+ .vunmap = amdgpu_gem_prime_vunmap,
+};
+
Wrong file, this belongs into amdgpu_gem.c
static int amdgpu_bo_do_create(struct amdgpu_device *adev,
struct amdgpu_bo_param *bp,
struct amdgpu_bo **bo_ptr)
@@ -552,6 +562,8 @@ static int amdgpu_bo_do_create(struct
amdgpu_device *adev,
bo = kzalloc(sizeof(struct amdgpu_bo), GFP_KERNEL);
if (bo == NULL)
return -ENOMEM;
+
+ bo->tbo.base.funcs = &amdgpu_gem_object_funcs;
And this should probably go into amdgpu_gem_object_create().
I'm trying to understand what amdgpu does. What about all the places
where amdgpu calls amdgpu_bo_create() internally? Wouldn't these miss
the free callback for the GEM object?
Those shouldn't have a GEM object in the first place.
Or otherwise we would have a reference counting issue.
Regards,
Christian.
Best regards
Thomas
Apart from that looks like a good idea to me.
Christian.
drm_gem_private_object_init(adev->ddev, &bo->tbo.base, size);
INIT_LIST_HEAD(&bo->shadow_list);
bo->vm_bo = NULL;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/1] drm/amdgpu: fix a typo
[AMD Public Use]
This is not upstream, but
Acked-by: Alex Deucher
From: amd-gfx on behalf of Nirmoy Das
Sent: Tuesday, September 8, 2020 11:57 AM
To: amd-gfx@lists.freedesktop.org
Cc: Das, Nirmoy ; Kazlauskas, Nicholas
Subject: [PATCH 1/1] drm/amdgpu: fix a typo
Fixes: 9a0154630e958a2f (drm/amdgpu: Bring back support for non-upstream
FreeSync)
Signed-off-by: Nirmoy Das
---
include/uapi/drm/amdgpu_drm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index b826f2d6efe1..d3dadf10b13d 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -1096,7 +1096,7 @@ struct drm_amdgpu_info_vce_clock_table {
struct drm_amdgpu_freesync {
__u32 op; /* AMDGPU_FREESYNC_FULLSCREEN_ENTER or
*/
- /* AMDGPU_FREESYNC_FULLSCREEN_ENTER */
+ /* AMDGPU_FREESYNC_FULLSCREEN_EXIT */
__u32 spare[7];
};
--
2.28.0
___
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-gfx&data=02%7C01%7Calexander.deucher%40amd.com%7C7f09b7d46fde47c781cf08d8540f3bb0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637351771962233619&sdata=t9hpqDYdTNU2bKoOTiGOi3bJvRhYZqCdzVdQ3Xv8dUk%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/gmc9: remove mmhub client duplicated case
Am 2020-09-14 um 11:42 a.m. schrieb Alex Deucher:
> Copy paste typo.
>
> Reported-by: kernel test robot
> Signed-off-by: Alex Deucher
Reviewed-by: Felix Kuehling
> ---
> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index d0645ad3446e..2bdfc861028a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -269,7 +269,6 @@ static const char *mmhub_client_ids_arcturus[][2] = {
> [14][1] = "HDP",
> [15][1] = "SDMA0",
> [32+15][1] = "SDMA1",
> - [32+15][1] = "SDMA1",
> [64+15][1] = "SDMA2",
> [96+15][1] = "SDMA3",
> [128+15][1] = "SDMA4",
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
On 2020-09-14 5:33 p.m., Kazlauskas, Nicholas wrote: On 2020-09-14 11:22 a.m., Michel Dänzer wrote: On 2020-09-14 4:37 p.m., Kazlauskas, Nicholas wrote: On 2020-09-14 3:52 a.m., Michel Dänzer wrote: P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack". I only know that atomictest tries to enable this configuration. Not sure if ChromiumOS or other "real" userspace tries this configuration. Someone mentioned that ChromiumOS uses this for video playback with black bars (when the video aspect ratio doesn't match the display's). We only expose support for NV12 on the primary plane so we wouldn't be hitting this case at least. Interesting, so if we're lucky this really won't affect any real-world apps. We can always go back to lying to userspace about the cursor being visible if the commit fails in that case I guess [...] I'm not sure what you mean by that / how it could work. Dropping the check you added in this patch: + if (state->enable && + !(state->plane_mask & drm_plane_mask(crtc->primary))) return -EINVAL; That way we can still allow the cursor plane to be enabled while primary/overlay are not, it's just not going to be actually visible on the screen. It'll fail some IGT tests but nothing really uses this configuration as more than just a temporary state. As Daniel pointed out in <20200901075432.GW2352366@phenom.ffwll.local> in the v1 patch thread, that won't fly, since atomic userspace can legitimately expect the cursor plane to be visible in that case. -- 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
[PATCH] drm/amdgpu/gmc9: remove mmhub client duplicated case
Copy paste typo.
Reported-by: kernel test robot
Signed-off-by: Alex Deucher
---
drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index d0645ad3446e..2bdfc861028a 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -269,7 +269,6 @@ static const char *mmhub_client_ids_arcturus[][2] = {
[14][1] = "HDP",
[15][1] = "SDMA0",
[32+15][1] = "SDMA1",
- [32+15][1] = "SDMA1",
[64+15][1] = "SDMA2",
[96+15][1] = "SDMA3",
[128+15][1] = "SDMA4",
--
2.25.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
On 2020-09-14 11:22 a.m., Michel Dänzer wrote: On 2020-09-14 4:37 p.m., Kazlauskas, Nicholas wrote: On 2020-09-14 3:52 a.m., Michel Dänzer wrote: On 2020-09-07 9:57 a.m., Daniel Vetter wrote: On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote: From: Michel Dänzer Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h: * Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit. atomic_remove_fb disables the CRTC as needed for disabling the primary plane. This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode): * The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID (which enables the cursor plane). * If the cursor plane was enabled, changing the legacy DPMS property value from off to on returned EINVAL. v2: * Minor changes to code comment and commit log, per review feedback. GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter Signed-off-by: Michel Dänzer Commit message agrees with my understand of this maze now, so: Acked-by: Daniel Vetter Thanks Daniel! Nick / Harry, any more feedback? If not, can you apply this? P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack". I only know that atomictest tries to enable this configuration. Not sure if ChromiumOS or other "real" userspace tries this configuration. Someone mentioned that ChromiumOS uses this for video playback with black bars (when the video aspect ratio doesn't match the display's). We only expose support for NV12 on the primary plane so we wouldn't be hitting this case at least. Maybe for now this can go in and if something breaks we can deal with the fallout then. We can always go back to lying to userspace about the cursor being visible if the commit fails in that case I guess [...] I'm not sure what you mean by that / how it could work. Dropping the check you added in this patch: + if (state->enable && + !(state->plane_mask & drm_plane_mask(crtc->primary))) return -EINVAL; That way we can still allow the cursor plane to be enabled while primary/overlay are not, it's just not going to be actually visible on the screen. It'll fail some IGT tests but nothing really uses this configuration as more than just a temporary state. Regards, Nicholas Kazlauskas Reviewed-by: Nicholas Kazlauskas Thanks! ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
On 2020-09-14 4:37 p.m., Kazlauskas, Nicholas wrote: On 2020-09-14 3:52 a.m., Michel Dänzer wrote: On 2020-09-07 9:57 a.m., Daniel Vetter wrote: On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote: From: Michel Dänzer Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h: * Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit. atomic_remove_fb disables the CRTC as needed for disabling the primary plane. This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode): * The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID (which enables the cursor plane). * If the cursor plane was enabled, changing the legacy DPMS property value from off to on returned EINVAL. v2: * Minor changes to code comment and commit log, per review feedback. GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter Signed-off-by: Michel Dänzer Commit message agrees with my understand of this maze now, so: Acked-by: Daniel Vetter Thanks Daniel! Nick / Harry, any more feedback? If not, can you apply this? P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack". I only know that atomictest tries to enable this configuration. Not sure if ChromiumOS or other "real" userspace tries this configuration. Someone mentioned that ChromiumOS uses this for video playback with black bars (when the video aspect ratio doesn't match the display's). Maybe for now this can go in and if something breaks we can deal with the fallout then. We can always go back to lying to userspace about the cursor being visible if the commit fails in that case I guess [...] I'm not sure what you mean by that / how it could work. Reviewed-by: Nicholas Kazlauskas Thanks! -- 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] drm/radeon: revert "Prefer lower feedback dividers"
Reviewed-by: Alex Deucher
On Fri, Sep 11, 2020 at 3:35 AM Christian König
wrote:
>
> Ping, we need to revert this ASAP.
>
> Christian.
>
> Am 09.09.20 um 13:16 schrieb Christian König:
> > Turns out this breaks a lot of different hardware.
> >
> > This reverts commit 522ff3a8b6d73a31084b4b087b458f7fa0ac1e14.
> >
> > Signed-off-by: Christian König
> > ---
> > drivers/gpu/drm/radeon/radeon_display.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_display.c
> > b/drivers/gpu/drm/radeon/radeon_display.c
> > index 7b69d6dfe44a..e0ae911ef427 100644
> > --- a/drivers/gpu/drm/radeon/radeon_display.c
> > +++ b/drivers/gpu/drm/radeon/radeon_display.c
> > @@ -933,7 +933,7 @@ static void avivo_get_fb_ref_div(unsigned nom, unsigned
> > den, unsigned post_div,
> >
> > /* get matching reference and feedback divider */
> > *ref_div = min(max(den/post_div, 1u), ref_div_max);
> > - *fb_div = max(nom * *ref_div * post_div / den, 1u);
> > + *fb_div = DIV_ROUND_CLOSEST(nom * *ref_div * post_div, den);
> >
> > /* limit fb divider to its maximum */
> > if (*fb_div > fb_div_max) {
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 01/20] drm/amdgpu: Introduce GEM object functions
Hi
Am 13.08.20 um 12:22 schrieb Christian König:
> Am 13.08.20 um 10:36 schrieb Thomas Zimmermann:
>> GEM object functions deprecate several similar callback interfaces in
>> struct drm_driver. This patch replaces the per-driver callbacks with
>> per-instance callbacks in amdgpu. The only exception is gem_prime_mmap,
>> which is non-trivial to convert.
>>
>> Signed-off-by: Thomas Zimmermann
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 --
>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 12
>> 2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 81a79760ca61..51525b8774c9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -1468,19 +1468,13 @@ static struct drm_driver kms_driver = {
>> .lastclose = amdgpu_driver_lastclose_kms,
>> .irq_handler = amdgpu_irq_handler,
>> .ioctls = amdgpu_ioctls_kms,
>> - .gem_free_object_unlocked = amdgpu_gem_object_free,
>> - .gem_open_object = amdgpu_gem_object_open,
>> - .gem_close_object = amdgpu_gem_object_close,
>> .dumb_create = amdgpu_mode_dumb_create,
>> .dumb_map_offset = amdgpu_mode_dumb_mmap,
>> .fops = &amdgpu_driver_kms_fops,
>> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> - .gem_prime_export = amdgpu_gem_prime_export,
>> .gem_prime_import = amdgpu_gem_prime_import,
>> - .gem_prime_vmap = amdgpu_gem_prime_vmap,
>> - .gem_prime_vunmap = amdgpu_gem_prime_vunmap,
>> .gem_prime_mmap = amdgpu_gem_prime_mmap,
>> .name = DRIVER_NAME,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 43f4966331dd..ca2b79f94e99 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -36,6 +36,7 @@
>> #include
>> #include
>> #include "amdgpu.h"
>> +#include "amdgpu_dma_buf.h"
>> #include "amdgpu_trace.h"
>> #include "amdgpu_amdkfd.h"
>> @@ -510,6 +511,15 @@ bool amdgpu_bo_support_uswc(u64 bo_flags)
>> #endif
>> }
>> +static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {
>> + .free = amdgpu_gem_object_free,
>> + .open = amdgpu_gem_object_open,
>> + .close = amdgpu_gem_object_close,
>> + .export = amdgpu_gem_prime_export,
>> + .vmap = amdgpu_gem_prime_vmap,
>> + .vunmap = amdgpu_gem_prime_vunmap,
>> +};
>> +
>
> Wrong file, this belongs into amdgpu_gem.c
>
>> static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>> struct amdgpu_bo_param *bp,
>> struct amdgpu_bo **bo_ptr)
>> @@ -552,6 +562,8 @@ static int amdgpu_bo_do_create(struct
>> amdgpu_device *adev,
>> bo = kzalloc(sizeof(struct amdgpu_bo), GFP_KERNEL);
>> if (bo == NULL)
>> return -ENOMEM;
>> +
>> + bo->tbo.base.funcs = &amdgpu_gem_object_funcs;
>
> And this should probably go into amdgpu_gem_object_create().
I'm trying to understand what amdgpu does. What about all the places
where amdgpu calls amdgpu_bo_create() internally? Wouldn't these miss
the free callback for the GEM object?
Best regards
Thomas
>
> Apart from that looks like a good idea to me.
>
> Christian.
>
>> drm_gem_private_object_init(adev->ddev, &bo->tbo.base, size);
>> INIT_LIST_HEAD(&bo->shadow_list);
>> bo->vm_bo = NULL;
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
signature.asc
Description: OpenPGP digital signature
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: Include sienna_cichlid in USBC PD FW support.
[AMD Public Use]
Reviewed-by: Alex Deucher
From: Grodzovsky, Andrey
Sent: Monday, September 14, 2020 10:32 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander
Subject: Re: [PATCH] drm/amdgpu: Include sienna_cichlid in USBC PD FW support.
Ping
Andrey
On 9/10/20 2:05 PM, Andrey Grodzovsky wrote:
> Create sysfs interface also for sienna_cichlid.
>
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index a7771aa..600015e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -178,7 +178,7 @@ static int psp_sw_init(void *handle)
>return ret;
>}
>
> - if (adev->asic_type == CHIP_NAVI10) {
> + if (adev->asic_type == CHIP_NAVI10 || adev->asic_type ==
> CHIP_SIENNA_CICHLID) {
>ret= psp_sysfs_init(adev);
>if (ret) {
>return ret;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
On 2020-09-14 3:52 a.m., Michel Dänzer wrote: On 2020-09-07 9:57 a.m., Daniel Vetter wrote: On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote: From: Michel Dänzer Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h: * Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit. atomic_remove_fb disables the CRTC as needed for disabling the primary plane. This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode): * The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID (which enables the cursor plane). * If the cursor plane was enabled, changing the legacy DPMS property value from off to on returned EINVAL. v2: * Minor changes to code comment and commit log, per review feedback. GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter Signed-off-by: Michel Dänzer Commit message agrees with my understand of this maze now, so: Acked-by: Daniel Vetter Thanks Daniel! Nick / Harry, any more feedback? If not, can you apply this? P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack". I only know that atomictest tries to enable this configuration. Not sure if ChromiumOS or other "real" userspace tries this configuration. Maybe for now this can go in and if something breaks we can deal with the fallout then. We can always go back to lying to userspace about the cursor being visible if the commit fails in that case I guess since the blank plane hack is going to add a significant amount of complexity to DM. Reviewed-by: Nicholas Kazlauskas Regards, Nicholas Kazlauskas ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: Power-saving/performance toggles for amdgpu
On Mon, 2020-09-14 at 01:46 -0400, Alex Deucher wrote: > > On older radeons (e.g., pre-GCN hardware), there were separate power > states for battery and AC, but these asics are supported by the > radeon > kernel driver. None of the hardware supported by amdgpu exposes > anything like that anymore. The rest is mainly for profiling and > debugging. For more information see the relevant kernel > documentation: > https://www.kernel.org/doc/html/latest/gpu/amdgpu.html#gpu-power-thermal-controls-and-monitoring > I don't think there is anything you'd want to tweak there. That was very helpful, thanks very much! ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: Include sienna_cichlid in USBC PD FW support.
Ping
Andrey
On 9/10/20 2:05 PM, Andrey Grodzovsky wrote:
Create sysfs interface also for sienna_cichlid.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index a7771aa..600015e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -178,7 +178,7 @@ static int psp_sw_init(void *handle)
return ret;
}
- if (adev->asic_type == CHIP_NAVI10) {
+ if (adev->asic_type == CHIP_NAVI10 || adev->asic_type ==
CHIP_SIENNA_CICHLID) {
ret= psp_sysfs_init(adev);
if (ret) {
return ret;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu: drop BOOLEAN define in display part
[AMD Public Use]
Reviewed-by: Guchun Chen
Regards,
Guchun
-Original Message-
From: amd-gfx On Behalf Of Flora Cui
Sent: Monday, September 14, 2020 2:37 PM
To: amd-gfx@lists.freedesktop.org
Cc: Cui, Flora
Subject: [PATCH] drm/amdgpu: drop BOOLEAN define in display part
use bool directly
Signed-off-by: Flora Cui
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c | 2 +-
drivers/gpu/drm/amd/display/dc/dm_pp_smu.h | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
index c5f2216e59c4..6a28fdd50e05 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
@@ -810,7 +810,7 @@ pp_nv_set_hard_min_uclk_by_freq(struct pp_smu *pp, int mhz)
}
static enum pp_smu_status pp_nv_set_pstate_handshake_support(
- struct pp_smu *pp, BOOLEAN pstate_handshake_supported)
+ struct pp_smu *pp, bool pstate_handshake_supported)
{
const struct dc_context *ctx = pp->dm;
struct amdgpu_device *adev = ctx->driver_context; diff --git
a/drivers/gpu/drm/amd/display/dc/dm_pp_smu.h
b/drivers/gpu/drm/amd/display/dc/dm_pp_smu.h
index ae608c329366..3586934df25f 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_pp_smu.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_pp_smu.h
@@ -30,8 +30,6 @@
* interface to PPLIB/SMU to setup clocks and pstate requirements on SoC
*/
-typedef bool BOOLEAN;
-
enum pp_smu_ver {
/*
* PP_SMU_INTERFACE_X should be interpreted as the interface defined @@
-240,7 +238,7 @@ struct pp_smu_funcs_nv {
* DC hardware
*/
enum pp_smu_status (*set_pstate_handshake_support)(struct pp_smu *pp,
- BOOLEAN pstate_handshake_supported);
+ bool pstate_handshake_supported);
};
#define PP_SMU_NUM_SOCCLK_DPM_LEVELS 8
--
2.17.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-gfx&data=02%7C01%7Cguchun.chen%40amd.com%7Cd81ec3c18004410be66408d858789afb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637356622910218361&sdata=pnVn%2BaYYczysuFlEljvG93IZO0TsLnxvMv09F1YYV7w%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amd/pm: support runtime pptable update for sienna_cichlid etc.
[AMD Official Use Only - Internal Distribution Only]
Better to split this into two patches: one for the dpm disablement skipping and
another for gfxoff ctrl.
Either way this patch is reviewed-by: Evan Quan
-Original Message-
From: Jiansong Chen
Sent: Monday, September 14, 2020 4:10 PM
To: amd-gfx@lists.freedesktop.org
Cc: Zhou1, Tao ; Feng, Kenneth ; Quan,
Evan ; Chen, Jiansong (Simon)
Subject: [PATCH] drm/amd/pm: support runtime pptable update for sienna_cichlid
etc.
This avoids smu issue when enabling runtime pptable update for
sienna_cichlid and so on. Runtime pptable udpate is needed for test
and debug purpose.
Signed-off-by: Jiansong Chen
Change-Id: I70b704ab4d6efd169f579c392e5dbc2737dc1fb2
---
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 7a55ece1f124..7618f9972b8c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1129,7 +1129,7 @@ static int smu_disable_dpms(struct smu_context *smu)
*/
if (smu->uploading_custom_pp_table &&
(adev->asic_type >= CHIP_NAVI10) &&
-(adev->asic_type <= CHIP_NAVI12))
+(adev->asic_type <= CHIP_NAVY_FLOUNDER))
return 0;
/*
@@ -1214,7 +1214,9 @@ static int smu_hw_fini(void *handle)
int smu_reset(struct smu_context *smu)
{
struct amdgpu_device *adev = smu->adev;
-int ret = 0;
+int ret;
+
+amdgpu_gfx_off_ctrl(smu->adev, false);
ret = smu_hw_fini(adev);
if (ret)
@@ -1225,8 +1227,12 @@ int smu_reset(struct smu_context *smu)
return ret;
ret = smu_late_init(adev);
+if (ret)
+return ret;
-return ret;
+amdgpu_gfx_off_ctrl(smu->adev, true);
+
+return 0;
}
static int smu_suspend(void *handle)
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amd/pm: support runtime pptable update for sienna_cichlid etc.
It makes nonsense to call gfxoff when smu failure has happened.
Regards,
Jiansong
-Original Message-
From: Chen, Guchun
Sent: Monday, September 14, 2020 4:14 PM
To: Chen, Jiansong (Simon) ;
amd-gfx@lists.freedesktop.org
Cc: Zhou1, Tao ; Feng, Kenneth ; Quan,
Evan ; Chen, Jiansong (Simon)
Subject: RE: [PATCH] drm/amd/pm: support runtime pptable update for
sienna_cichlid etc.
[AMD Public Use]
ret = smu_late_init(adev);
+ if (ret)
+ return ret;
One counter leak happens? It needs to call amdgpu_gfx_off_ctrl(smu->adev, true)
before returning?
Regards,
Guchun
-Original Message-
From: amd-gfx On Behalf Of Jiansong Chen
Sent: Monday, September 14, 2020 4:10 PM
To: amd-gfx@lists.freedesktop.org
Cc: Zhou1, Tao ; Feng, Kenneth ; Quan,
Evan ; Chen, Jiansong (Simon)
Subject: [PATCH] drm/amd/pm: support runtime pptable update for sienna_cichlid
etc.
This avoids smu issue when enabling runtime pptable update for sienna_cichlid
and so on. Runtime pptable udpate is needed for test and debug purpose.
Signed-off-by: Jiansong Chen
Change-Id: I70b704ab4d6efd169f579c392e5dbc2737dc1fb2
---
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 7a55ece1f124..7618f9972b8c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1129,7 +1129,7 @@ static int smu_disable_dpms(struct smu_context *smu)
*/
if (smu->uploading_custom_pp_table &&
(adev->asic_type >= CHIP_NAVI10) &&
- (adev->asic_type <= CHIP_NAVI12))
+ (adev->asic_type <= CHIP_NAVY_FLOUNDER))
return 0;
/*
@@ -1214,7 +1214,9 @@ static int smu_hw_fini(void *handle) int
smu_reset(struct smu_context *smu) {
struct amdgpu_device *adev = smu->adev;
- int ret = 0;
+ int ret;
+
+ amdgpu_gfx_off_ctrl(smu->adev, false);
ret = smu_hw_fini(adev);
if (ret)
@@ -1225,8 +1227,12 @@ int smu_reset(struct smu_context *smu)
return ret;
ret = smu_late_init(adev);
+ if (ret)
+ return ret;
- return ret;
+ amdgpu_gfx_off_ctrl(smu->adev, true);
+
+ return 0;
}
static int smu_suspend(void *handle)
--
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-gfx&data=02%7C01%7Cguchun.chen%40amd.com%7Cd73dd73ccf3c444c710508d858859f53%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637356678262125090&sdata=p0sqrDLhD4vaNesLHIq6Jfd57sAeu8wHDH69bDwTAvA%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amd/pm: support runtime pptable update for sienna_cichlid etc.
[AMD Public Use]
ret = smu_late_init(adev);
+ if (ret)
+ return ret;
One counter leak happens? It needs to call amdgpu_gfx_off_ctrl(smu->adev, true)
before returning?
Regards,
Guchun
-Original Message-
From: amd-gfx On Behalf Of Jiansong Chen
Sent: Monday, September 14, 2020 4:10 PM
To: amd-gfx@lists.freedesktop.org
Cc: Zhou1, Tao ; Feng, Kenneth ; Quan,
Evan ; Chen, Jiansong (Simon)
Subject: [PATCH] drm/amd/pm: support runtime pptable update for sienna_cichlid
etc.
This avoids smu issue when enabling runtime pptable update for sienna_cichlid
and so on. Runtime pptable udpate is needed for test and debug purpose.
Signed-off-by: Jiansong Chen
Change-Id: I70b704ab4d6efd169f579c392e5dbc2737dc1fb2
---
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 7a55ece1f124..7618f9972b8c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1129,7 +1129,7 @@ static int smu_disable_dpms(struct smu_context *smu)
*/
if (smu->uploading_custom_pp_table &&
(adev->asic_type >= CHIP_NAVI10) &&
- (adev->asic_type <= CHIP_NAVI12))
+ (adev->asic_type <= CHIP_NAVY_FLOUNDER))
return 0;
/*
@@ -1214,7 +1214,9 @@ static int smu_hw_fini(void *handle) int
smu_reset(struct smu_context *smu) {
struct amdgpu_device *adev = smu->adev;
- int ret = 0;
+ int ret;
+
+ amdgpu_gfx_off_ctrl(smu->adev, false);
ret = smu_hw_fini(adev);
if (ret)
@@ -1225,8 +1227,12 @@ int smu_reset(struct smu_context *smu)
return ret;
ret = smu_late_init(adev);
+ if (ret)
+ return ret;
- return ret;
+ amdgpu_gfx_off_ctrl(smu->adev, true);
+
+ return 0;
}
static int smu_suspend(void *handle)
--
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-gfx&data=02%7C01%7Cguchun.chen%40amd.com%7Cd73dd73ccf3c444c710508d858859f53%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637356678262125090&sdata=p0sqrDLhD4vaNesLHIq6Jfd57sAeu8wHDH69bDwTAvA%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amd/pm: support runtime pptable update for sienna_cichlid etc.
[AMD Official Use Only - Internal Distribution Only]
Reviewed-by: Kenneth Feng
-Original Message-
From: Jiansong Chen
Sent: Monday, September 14, 2020 4:10 PM
To: amd-gfx@lists.freedesktop.org
Cc: Zhou1, Tao ; Feng, Kenneth ; Quan,
Evan ; Chen, Jiansong (Simon)
Subject: [PATCH] drm/amd/pm: support runtime pptable update for sienna_cichlid
etc.
This avoids smu issue when enabling runtime pptable update for sienna_cichlid
and so on. Runtime pptable udpate is needed for test and debug purpose.
Signed-off-by: Jiansong Chen
Change-Id: I70b704ab4d6efd169f579c392e5dbc2737dc1fb2
---
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 7a55ece1f124..7618f9972b8c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1129,7 +1129,7 @@ static int smu_disable_dpms(struct smu_context *smu)
*/
if (smu->uploading_custom_pp_table &&
(adev->asic_type >= CHIP_NAVI10) &&
- (adev->asic_type <= CHIP_NAVI12))
+ (adev->asic_type <= CHIP_NAVY_FLOUNDER))
return 0;
/*
@@ -1214,7 +1214,9 @@ static int smu_hw_fini(void *handle) int
smu_reset(struct smu_context *smu) {
struct amdgpu_device *adev = smu->adev;
- int ret = 0;
+ int ret;
+
+ amdgpu_gfx_off_ctrl(smu->adev, false);
ret = smu_hw_fini(adev);
if (ret)
@@ -1225,8 +1227,12 @@ int smu_reset(struct smu_context *smu)
return ret;
ret = smu_late_init(adev);
+ if (ret)
+ return ret;
- return ret;
+ amdgpu_gfx_off_ctrl(smu->adev, true);
+
+ return 0;
}
static int smu_suspend(void *handle)
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/pm: support runtime pptable update for sienna_cichlid etc.
This avoids smu issue when enabling runtime pptable update for
sienna_cichlid and so on. Runtime pptable udpate is needed for test
and debug purpose.
Signed-off-by: Jiansong Chen
Change-Id: I70b704ab4d6efd169f579c392e5dbc2737dc1fb2
---
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 7a55ece1f124..7618f9972b8c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1129,7 +1129,7 @@ static int smu_disable_dpms(struct smu_context *smu)
*/
if (smu->uploading_custom_pp_table &&
(adev->asic_type >= CHIP_NAVI10) &&
- (adev->asic_type <= CHIP_NAVI12))
+ (adev->asic_type <= CHIP_NAVY_FLOUNDER))
return 0;
/*
@@ -1214,7 +1214,9 @@ static int smu_hw_fini(void *handle)
int smu_reset(struct smu_context *smu)
{
struct amdgpu_device *adev = smu->adev;
- int ret = 0;
+ int ret;
+
+ amdgpu_gfx_off_ctrl(smu->adev, false);
ret = smu_hw_fini(adev);
if (ret)
@@ -1225,8 +1227,12 @@ int smu_reset(struct smu_context *smu)
return ret;
ret = smu_late_init(adev);
+ if (ret)
+ return ret;
- return ret;
+ amdgpu_gfx_off_ctrl(smu->adev, true);
+
+ return 0;
}
static int smu_suspend(void *handle)
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
On 2020-09-07 9:57 a.m., Daniel Vetter wrote: On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote: From: Michel Dänzer Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h: * Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit. atomic_remove_fb disables the CRTC as needed for disabling the primary plane. This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode): * The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID (which enables the cursor plane). * If the cursor plane was enabled, changing the legacy DPMS property value from off to on returned EINVAL. v2: * Minor changes to code comment and commit log, per review feedback. GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter Signed-off-by: Michel Dänzer Commit message agrees with my understand of this maze now, so: Acked-by: Daniel Vetter Thanks Daniel! Nick / Harry, any more feedback? If not, can you apply this? P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack". -- 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
