Re: [RFC v4 08/11] drm/amdgpu: Move reset sem into reset_domain
Am 09.02.22 um 01:23 schrieb Andrey Grodzovsky: We want single instance of reset sem across all reset clients because in case of XGMI we should stop access cross device MMIO because any of them could be in a reset in the moment. Signed-off-by: Andrey Grodzovsky Reviewed-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 23 +-- .../gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c| 18 --- drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h | 1 + drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c| 6 +++-- drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 14 ++- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 4 ++-- 10 files changed, 46 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index cb9764513df8..ddfbcc8fd3d3 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1058,7 +1058,6 @@ struct amdgpu_device { atomic_t in_gpu_reset; enum pp_mp1_state mp1_state; - struct rw_semaphore reset_sem; struct amdgpu_doorbell_index doorbell_index; struct mutex notifier_lock; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c index 25e2e5bf90eb..c3728061d65a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c @@ -37,6 +37,8 @@ #include "amdgpu_fw_attestation.h" #include "amdgpu_umr.h" +#include "amdgpu_reset.h" + #if defined(CONFIG_DEBUG_FS) /** @@ -1279,7 +1281,7 @@ static int amdgpu_debugfs_test_ib_show(struct seq_file *m, void *unused) } /* Avoid accidently unparking the sched thread during GPU reset */ - r = down_write_killable(>reset_sem); + r = down_write_killable(>reset_domain->sem); if (r) return r; @@ -1308,7 +1310,7 @@ static int amdgpu_debugfs_test_ib_show(struct seq_file *m, void *unused) kthread_unpark(ring->sched.thread); } - up_write(>reset_sem); + up_write(>reset_domain->sem); pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev); @@ -1517,7 +1519,7 @@ static int amdgpu_debugfs_ib_preempt(void *data, u64 val) return -ENOMEM; /* Avoid accidently unparking the sched thread during GPU reset */ - r = down_read_killable(>reset_sem); + r = down_read_killable(>reset_domain->sem); if (r) goto pro_end; @@ -1560,7 +1562,7 @@ static int amdgpu_debugfs_ib_preempt(void *data, u64 val) /* restart the scheduler */ kthread_unpark(ring->sched.thread); - up_read(>reset_sem); + up_read(>reset_domain->sem); ttm_bo_unlock_delayed_workqueue(>mman.bdev, resched); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index d61bc0a0457c..dcbb175d336f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -424,10 +424,10 @@ bool amdgpu_device_skip_hw_access(struct amdgpu_device *adev) * the lock. */ if (in_task()) { - if (down_read_trylock(>reset_sem)) - up_read(>reset_sem); + if (down_read_trylock(>reset_domain->sem)) + up_read(>reset_domain->sem); else - lockdep_assert_held(>reset_sem); + lockdep_assert_held(>reset_domain->sem); } #endif return false; @@ -453,9 +453,9 @@ uint32_t amdgpu_device_rreg(struct amdgpu_device *adev, if ((reg * 4) < adev->rmmio_size) { if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) && - down_read_trylock(>reset_sem)) { + down_read_trylock(>reset_domain->sem)) { ret = amdgpu_kiq_rreg(adev, reg); - up_read(>reset_sem); + up_read(>reset_domain->sem); } else { ret = readl(((void __iomem *)adev->rmmio) + (reg * 4)); } @@ -538,9 +538,9 @@ void amdgpu_device_wreg(struct amdgpu_device *adev, if ((reg * 4) < adev->rmmio_size) { if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) && - down_read_trylock(>reset_sem)) { + down_read_trylock(>reset_domain->sem)) { amdgpu_kiq_wreg(adev, reg, v); - up_read(>reset_sem); + up_read(>reset_domain->sem); }
Re: [RFC v4 07/11] drm/amdgpu: Rework reset domain to be refcounted.
Am 09.02.22 um 01:23 schrieb Andrey Grodzovsky: The reset domain contains register access semaphor now and so needs to be present as long as each device in a hive needs it and so it cannot be binded to XGMI hive life cycle. Adress this by making reset domain refcounted and pointed by each member of the hive and the hive itself. v4: Fix crash on boot witrh XGMI hive by adding type to reset_domain. XGMI will only create a new reset_domain if prevoius was of single device type meaning it's first boot. Otherwsie it will take a refocunt to exsiting reset_domain from the amdgou device. Add a wrapper around reset_domain->refcount get/put and a wrapper around send to reset wq (Lijo) Signed-off-by: Andrey Grodzovsky Acked-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu.h| 6 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 44 +- drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 40 drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h | 35 + drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 29 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 2 +- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 6 ++- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 6 ++- drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 6 ++- 9 files changed, 140 insertions(+), 34 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 540a38fe5cd6..cb9764513df8 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -813,9 +813,7 @@ struct amd_powerplay { #define AMDGPU_RESET_MAGIC_NUM 64 #define AMDGPU_MAX_DF_PERFMONS 4 #define AMDGPU_PRODUCT_NAME_LEN 64 -struct amdgpu_reset_domain { - struct workqueue_struct *wq; -}; +struct amdgpu_reset_domain; struct amdgpu_device { struct device *dev; @@ -1104,7 +1102,7 @@ struct amdgpu_device { uint32_t ip_versions[MAX_HWIP][HWIP_MAX_INSTANCE]; boolram_is_direct_mapped; - struct amdgpu_reset_domain reset_domain; + struct amdgpu_reset_domain *reset_domain; }; static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index e3c0ec684a85..d61bc0a0457c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -2316,7 +2316,7 @@ static int amdgpu_device_init_schedulers(struct amdgpu_device *adev) r = drm_sched_init(>sched, _sched_ops, ring->num_hw_submission, amdgpu_job_hang_limit, - timeout, adev->reset_domain.wq, ring->sched_score, ring->name); + timeout, adev->reset_domain->wq, ring->sched_score, ring->name); if (r) { DRM_ERROR("Failed to create scheduler on ring %s.\n", ring->name); @@ -2439,24 +2439,22 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev) if (r) goto init_failed; + /** +* In case of XGMI grab extra reference for reset domain for this device +*/ if (adev->gmc.xgmi.num_physical_nodes > 1) { - struct amdgpu_hive_info *hive; + if (amdgpu_xgmi_add_device(adev) == 0) { + struct amdgpu_hive_info *hive = amdgpu_get_xgmi_hive(adev); - amdgpu_xgmi_add_device(adev); + if (!hive->reset_domain || + !amdgpu_reset_get_reset_domain(hive->reset_domain)) { + r = -ENOENT; + goto init_failed; + } - hive = amdgpu_get_xgmi_hive(adev); - if (!hive || !hive->reset_domain.wq) { - DRM_ERROR("Failed to obtain reset domain info for XGMI hive:%llx", hive->hive_id); - r = -EINVAL; - goto init_failed; - } - - adev->reset_domain.wq = hive->reset_domain.wq; - } else { - adev->reset_domain.wq = alloc_ordered_workqueue("amdgpu-reset-dev", 0); - if (!adev->reset_domain.wq) { - r = -ENOMEM; - goto init_failed; + /* Drop the early temporary reset domain we created for device */ + amdgpu_reset_put_reset_domain(adev->reset_domain); + adev->reset_domain = hive->reset_domain; } } @@ -3640,6 +3638,15 @@ int amdgpu_device_init(struct amdgpu_device *adev, return r; } + /* +* Reset domain needs to be present early, before XGMI hive discovered +* (if any) and intitialized to use reset sem and in_gpu reset flag +* early on
Re: [RFC v4 04/11] drm/amd/virt: For SRIOV send GPU reset directly to TDR queue.
Am 09.02.22 um 01:23 schrieb Andrey Grodzovsky: No need to to trigger another work queue inside the work queue. v3: Problem: Extra reset caused by host side FLR notification following guest side triggered reset. Fix: Preven qeuing flr_work from mailbox irq if guest already executing a reset. Suggested-by: Liu Shaoyun Signed-off-by: Andrey Grodzovsky Feel free to add an Acked-by: Christian König , but an rb from somebody more familiar with the code would be better. Regards, Christian. --- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 9 ++--- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 9 ++--- drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 9 ++--- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c index 56da5ab82987..5869d51d8bee 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c @@ -282,7 +282,7 @@ static void xgpu_ai_mailbox_flr_work(struct work_struct *work) if (amdgpu_device_should_recover_gpu(adev) && (!amdgpu_device_has_job_running(adev) || adev->sdma_timeout == MAX_SCHEDULE_TIMEOUT)) - amdgpu_device_gpu_recover(adev, NULL); + amdgpu_device_gpu_recover_imp(adev, NULL); } static int xgpu_ai_set_mailbox_rcv_irq(struct amdgpu_device *adev, @@ -307,8 +307,11 @@ static int xgpu_ai_mailbox_rcv_irq(struct amdgpu_device *adev, switch (event) { case IDH_FLR_NOTIFICATION: - if (amdgpu_sriov_runtime(adev)) - schedule_work(>virt.flr_work); + if (amdgpu_sriov_runtime(adev) && !amdgpu_in_reset(adev)) + WARN_ONCE(!queue_work(adev->reset_domain.wq, + >virt.flr_work), + "Failed to queue work! at %s", + __func__); break; case IDH_QUERY_ALIVE: xgpu_ai_mailbox_send_ack(adev); diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c index 477d0dde19c5..5728a6401d73 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c @@ -309,7 +309,7 @@ static void xgpu_nv_mailbox_flr_work(struct work_struct *work) adev->gfx_timeout == MAX_SCHEDULE_TIMEOUT || adev->compute_timeout == MAX_SCHEDULE_TIMEOUT || adev->video_timeout == MAX_SCHEDULE_TIMEOUT)) - amdgpu_device_gpu_recover(adev, NULL); + amdgpu_device_gpu_recover_imp(adev, NULL); } static int xgpu_nv_set_mailbox_rcv_irq(struct amdgpu_device *adev, @@ -337,8 +337,11 @@ static int xgpu_nv_mailbox_rcv_irq(struct amdgpu_device *adev, switch (event) { case IDH_FLR_NOTIFICATION: - if (amdgpu_sriov_runtime(adev)) - schedule_work(>virt.flr_work); + if (amdgpu_sriov_runtime(adev) && !amdgpu_in_reset(adev)) + WARN_ONCE(!queue_work(adev->reset_domain.wq, + >virt.flr_work), + "Failed to queue work! at %s", + __func__); break; /* READY_TO_ACCESS_GPU is fetched by kernel polling, IRQ can ignore * it byfar since that polling thread will handle it, diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c index aef9d059ae52..02290febfcf4 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c @@ -521,7 +521,7 @@ static void xgpu_vi_mailbox_flr_work(struct work_struct *work) /* Trigger recovery due to world switch failure */ if (amdgpu_device_should_recover_gpu(adev)) - amdgpu_device_gpu_recover(adev, NULL); + amdgpu_device_gpu_recover_imp(adev, NULL); } static int xgpu_vi_set_mailbox_rcv_irq(struct amdgpu_device *adev, @@ -550,8 +550,11 @@ static int xgpu_vi_mailbox_rcv_irq(struct amdgpu_device *adev, r = xgpu_vi_mailbox_rcv_msg(adev, IDH_FLR_NOTIFICATION); /* only handle FLR_NOTIFY now */ - if (!r) - schedule_work(>virt.flr_work); + if (!r && !amdgpu_in_reset(adev)) + WARN_ONCE(!queue_work(adev->reset_domain.wq, + >virt.flr_work), + "Failed to queue work! at %s", + __func__); } return 0;
Re: [RFC v4 02/11] drm/amdgpu: Move scheduler init to after XGMI is ready
Am 09.02.22 um 01:23 schrieb Andrey Grodzovsky: Before we initialize schedulers we must know which reset domain are we in - for single device there iis a single domain per device and so single wq per device. For XGMI the reset domain spans the entire XGMI hive and so the reset wq is per hive. Signed-off-by: Andrey Grodzovsky One more comment below, with that fixed Reviewed-by: Christian König . --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 45 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 34 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 + 3 files changed, 51 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 9704b0e1fd82..00123b0013d3 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -2287,6 +2287,47 @@ static int amdgpu_device_fw_loading(struct amdgpu_device *adev) return r; } +static int amdgpu_device_init_schedulers(struct amdgpu_device *adev) +{ + long timeout; + int r, i; + + for (i = 0; i < AMDGPU_MAX_RINGS; ++i) { + struct amdgpu_ring *ring = adev->rings[i]; + + /* No need to setup the GPU scheduler for rings that don't need it */ + if (!ring || ring->no_scheduler) + continue; + + switch (ring->funcs->type) { + case AMDGPU_RING_TYPE_GFX: + timeout = adev->gfx_timeout; + break; + case AMDGPU_RING_TYPE_COMPUTE: + timeout = adev->compute_timeout; + break; + case AMDGPU_RING_TYPE_SDMA: + timeout = adev->sdma_timeout; + break; + default: + timeout = adev->video_timeout; + break; + } + + r = drm_sched_init(>sched, _sched_ops, + ring->num_hw_submission, amdgpu_job_hang_limit, + timeout, adev->reset_domain.wq, ring->sched_score, ring->name); + if (r) { + DRM_ERROR("Failed to create scheduler on ring %s.\n", + ring->name); + return r; + } + } + + return 0; +} + + /** * amdgpu_device_ip_init - run init for hardware IPs * @@ -2419,6 +2460,10 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev) } } + r = amdgpu_device_init_schedulers(adev); + if (r) + goto init_failed; + /* Don't init kfd if whole hive need to be reset during init */ if (!adev->gmc.xgmi.pending_reset) amdgpu_amdkfd_device_init(adev); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index 45977a72b5dd..fa302540c69a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -457,8 +457,6 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring, atomic_t *sched_score) { struct amdgpu_device *adev = ring->adev; - long timeout; - int r; if (!adev) return -EINVAL; @@ -478,36 +476,12 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring, spin_lock_init(>fence_drv.lock); ring->fence_drv.fences = kcalloc(num_hw_submission * 2, sizeof(void *), GFP_KERNEL); - if (!ring->fence_drv.fences) - return -ENOMEM; - /* No need to setup the GPU scheduler for rings that don't need it */ - if (ring->no_scheduler) - return 0; + ring->num_hw_submission = num_hw_submission; + ring->sched_score = sched_score; Let's move this into the caller and then use ring->num_hw_submission in the fence code as well. The maximum number of jobs on the ring is not really fence specific. Regards, Christian. - switch (ring->funcs->type) { - case AMDGPU_RING_TYPE_GFX: - timeout = adev->gfx_timeout; - break; - case AMDGPU_RING_TYPE_COMPUTE: - timeout = adev->compute_timeout; - break; - case AMDGPU_RING_TYPE_SDMA: - timeout = adev->sdma_timeout; - break; - default: - timeout = adev->video_timeout; - break; - } - - r = drm_sched_init(>sched, _sched_ops, - num_hw_submission, amdgpu_job_hang_limit, - timeout, NULL, sched_score, ring->name); - if (r) { - DRM_ERROR("Failed to create scheduler on ring %s.\n", - ring->name); - return r; - } + if (!ring->fence_drv.fences) +
Re: [RFC v4] drm/amdgpu: Rework reset domain to be refcounted.
Am 08.02.22 um 17:19 schrieb Andrey Grodzovsky: On 2022-02-08 06:25, Lazar, Lijo wrote: On 2/2/2022 10:56 PM, Andrey Grodzovsky wrote: The reset domain contains register access semaphor now and so needs to be present as long as each device in a hive needs it and so it cannot be binded to XGMI hive life cycle. Adress this by making reset domain refcounted and pointed by each member of the hive and the hive itself. v4: Fix crash on boot with XGMI hive by adding type to reset_domain. XGMI will only create a new reset_domain if prevoius was of single device type meaning it's first boot. Otherwsie it will take a refocunt to exsiting reset_domain from the amdgou device. Signed-off-by: Andrey Grodzovsky --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 6 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 44 +- drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 38 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h | 18 + drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 29 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 2 +- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 4 +- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 4 +- drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 4 +- 9 files changed, 118 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 8e96b9a14452..f2ba460bfd59 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -813,9 +813,7 @@ struct amd_powerplay { #define AMDGPU_RESET_MAGIC_NUM 64 #define AMDGPU_MAX_DF_PERFMONS 4 -struct amdgpu_reset_domain { - struct workqueue_struct *wq; -}; +struct amdgpu_reset_domain; struct amdgpu_device { struct device *dev; @@ -1102,7 +1100,7 @@ struct amdgpu_device { struct amdgpu_reset_control *reset_cntl; uint32_t ip_versions[HW_ID_MAX][HWIP_MAX_INSTANCE]; - struct amdgpu_reset_domain reset_domain; + struct amdgpu_reset_domain *reset_domain; }; static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index fef952ca8db5..cd1b7af69c35 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -2313,7 +2313,7 @@ static int amdgpu_device_init_schedulers(struct amdgpu_device *adev) r = drm_sched_init(>sched, _sched_ops, ring->num_hw_submission, amdgpu_job_hang_limit, - timeout, adev->reset_domain.wq, ring->sched_score, ring->name); + timeout, adev->reset_domain->wq, ring->sched_score, ring->name); if (r) { DRM_ERROR("Failed to create scheduler on ring %s.\n", ring->name); @@ -2432,24 +2432,22 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev) if (r) goto init_failed; + /** + * In case of XGMI grab extra reference for reset domain for this device + */ if (adev->gmc.xgmi.num_physical_nodes > 1) { - struct amdgpu_hive_info *hive; - - amdgpu_xgmi_add_device(adev); + if (amdgpu_xgmi_add_device(adev) == 0) { + struct amdgpu_hive_info *hive = amdgpu_get_xgmi_hive(adev); - hive = amdgpu_get_xgmi_hive(adev); - if (!hive || !hive->reset_domain.wq) { - DRM_ERROR("Failed to obtain reset domain info for XGMI hive:%llx", hive->hive_id); - r = -EINVAL; - goto init_failed; - } + if (!hive->reset_domain || + !kref_get_unless_zero(>reset_domain->refcount)) { + r = -ENOENT; + goto init_failed; + } - adev->reset_domain.wq = hive->reset_domain.wq; - } else { - adev->reset_domain.wq = alloc_ordered_workqueue("amdgpu-reset-dev", 0); - if (!adev->reset_domain.wq) { - r = -ENOMEM; - goto init_failed; + /* Drop the early temporary reset domain we created for device */ + kref_put(>reset_domain->refcount, amdgpu_reset_destroy_reset_domain); + adev->reset_domain = hive->reset_domain; } } @@ -3599,6 +3597,15 @@ int amdgpu_device_init(struct amdgpu_device *adev, return r; } + /* + * Reset domain needs to be present early, before XGMI hive discovered + * (if any) and intitialized to use reset sem and in_gpu reset flag + * early on during init. + */ + adev->reset_domain = amdgpu_reset_create_reset_domain(SINGLE_DEVICE ,"amdgpu-reset-dev"); + if (!adev->reset_domain) + return -ENOMEM; + /* early init functions */ r = amdgpu_device_ip_early_init(adev); if (r) @@ -3949,6 +3956,9 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev) if (adev->mman.discovery_bin)
Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
On Tue, 2022-02-08 at 17:55 -0800, Abhinav Kumar wrote: > > Are you suggesting something like below? > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c > index 42dcf96..14203d0 100644 > --- a/fs/sysfs/file.c > No, for sure not, but I guess from the looks of this patch there's no way to do something like that for just an individual attribute... Oh well. johannes
Re: [PATCH 23/23] drm/omap: plane: Remove redundant color encoding and range initialisation
On 07/02/2022 18:35, Maxime Ripard wrote: The omap KMS driver will call drm_plane_create_color_properties() with a default encoding and range values of BT601 and Full Range, respectively. Since the initial value wasn't carried over in the state, the driver had to set it again in omap_plane_reset(). However, the helpers have been adjusted to set it properly at reset, so this is not needed anymore. Cc: Tomi Valkeinen Signed-off-by: Maxime Ripard --- drivers/gpu/drm/omapdrm/omap_plane.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index d96bc929072a..b83d91ec030a 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -403,7 +403,6 @@ void omap_plane_install_properties(struct drm_plane *plane, static void omap_plane_reset(struct drm_plane *plane) { - struct omap_plane *omap_plane = to_omap_plane(plane); struct omap_plane_state *omap_state; if (plane->state) @@ -414,8 +413,6 @@ static void omap_plane_reset(struct drm_plane *plane) return; __drm_atomic_helper_plane_reset(plane, _state->base); - plane->state->color_encoding = DRM_COLOR_YCBCR_BT601; - plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE; } static struct drm_plane_state * Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 15/23] drm/omap: plane: Remove redundant zpos initialisation
On 07/02/2022 18:35, Maxime Ripard wrote: The omap KMS driver will call drm_plane_create_zpos_property() with an init value of the plane index and the plane type. Since the initial value wasn't carried over in the state, the driver had to set it again in omap_plane_reset(). However, the helpers have been adjusted to set it properly at reset, so this is not needed anymore. Cc: Tomi Valkeinen Signed-off-by: Maxime Ripard --- drivers/gpu/drm/omapdrm/omap_plane.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index e67baf9a942c..d96bc929072a 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -414,13 +414,6 @@ static void omap_plane_reset(struct drm_plane *plane) return; __drm_atomic_helper_plane_reset(plane, _state->base); - - /* -* Set the zpos default depending on whether we are a primary or overlay -* plane. -*/ - plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY - ? 0 : omap_plane->id; plane->state->color_encoding = DRM_COLOR_YCBCR_BT601; plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE; } Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 14/23] drm/omap: plane: Fix zpos initial value mismatch
On 07/02/2022 18:35, Maxime Ripard wrote: While the omap_plane_init() function calls drm_plane_create_zpos_property() with an initial value of 0, omap_plane_reset() will force it to another value depending on the plane type. Fix the discrepancy by setting the initial zpos value to the same value in the drm_plane_create_zpos_property() call. Cc: Tomi Valkeinen Signed-off-by: Maxime Ripard --- drivers/gpu/drm/omapdrm/omap_plane.c | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index b35205c4e979..e67baf9a942c 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -533,6 +533,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, unsigned int num_planes = dispc_get_num_ovls(priv->dispc); struct drm_plane *plane; struct omap_plane *omap_plane; + unsigned int zpos; int ret; u32 nformats; const u32 *formats; @@ -564,7 +565,16 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, drm_plane_helper_add(plane, _plane_helper_funcs); omap_plane_install_properties(plane, >base); - drm_plane_create_zpos_property(plane, 0, 0, num_planes - 1); + + /* +* Set the zpos default depending on whether we are a primary or overlay +* plane. +*/ + if (plane->type == DRM_PLANE_TYPE_PRIMARY) + zpos = 0; + else + zpos = omap_plane->id; + drm_plane_create_zpos_property(plane, zpos, 0, num_planes - 1); drm_plane_create_alpha_property(plane); drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) | BIT(DRM_MODE_BLEND_COVERAGE)); Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH] drm/doc: pull in drm_buddy.c
Am 08.02.22 um 16:12 schrieb Matthew Auld: Make sure we pull in the kernel-doc for this. Reported-by: Daniel Vetter Signed-off-by: Matthew Auld Cc: Arunpravin Cc: Christian König Reviewed-by: Christian König --- Documentation/gpu/drm-mm.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index 198bcc1affa1..f32ccce5722d 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -466,6 +466,15 @@ DRM MM Range Allocator Function References .. kernel-doc:: drivers/gpu/drm/drm_mm.c :export: +DRM Buddy Allocator +=== + +DRM Buddy Function References +- + +.. kernel-doc:: drivers/gpu/drm/drm_buddy.c + :export: + DRM Cache Handling and Fast WC memcpy() ===
RE: [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD
Hi Bjorn, 1. I will change the edp_out label to mdss_edp_out. 2. The "pm8350c_pwm" node is part of the dependent series mentioned in the cover letter. Below is the patch for the same: https://patchwork.kernel.org/project/linux-arm-msm/patch/1637917920-22041-4-git-send-email-quic_c_ska...@quicinc.com/ 3. I will move the edp_backlight and edp_panel nodes to the root. Thank you, Sankeerth -Original Message- From: Bjorn Andersson Sent: Wednesday, February 9, 2022 5:23 AM To: Sankeerth Billakanti (QUIC) Cc: dri-devel@lists.freedesktop.org; linux-arm-...@vger.kernel.org; freedr...@lists.freedesktop.org; linux-ker...@vger.kernel.org; devicet...@vger.kernel.org; agr...@kernel.org; robh...@kernel.org; robdcl...@gmail.com; seanp...@chromium.org; swb...@chromium.org; diand...@chromium.org; krzysztof.kozlow...@canonical.com; thierry.red...@gmail.com; s...@ravnborg.org; airl...@linux.ie; dan...@ffwll.ch; quic_kalyant ; Abhinav Kumar (QUIC) ; Kuogee Hsieh (QUIC) ; quic_mkrishn Subject: Re: [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD On Tue 08 Feb 07:18 PST 2022, Sankeerth Billakanti wrote: > Enable the eDP display panel support without HPD on sc7280 platform. > > Signed-off-by: Sankeerth Billakanti > --- > > Changes in v2: > - sort node references alphabetically > - improve readability > - move the pwm pinctrl to pwm node > - move the regulators to root > - define backlight power > - remove dummy regulator node > - cleanup pinctrl definitions > > arch/arm64/boot/dts/qcom/sc7280-crd.dts | 122 > > arch/arm64/boot/dts/qcom/sc7280.dtsi| 2 - > 2 files changed, 122 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > index e2efbdd..bff2707 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > @@ -21,6 +21,34 @@ > chosen { > stdout-path = "serial0:115200n8"; > }; > + > + backlight_power: backlight-power { > + compatible = "regulator-fixed"; > + regulator-name = "backlight_power"; > + > + regulator-min-microvolt = <180>; > + regulator-max-microvolt = <180>; > + > + gpio = <_gpios 7 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_bl_power>; > + }; > + > + edp_power: edp-power { > + compatible = "regulator-fixed"; > + regulator-name = "edp_power"; > + > + regulator-min-microvolt = <330>; > + regulator-max-microvolt = <330>; > + > + gpio = < 80 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_panel_power>; > + }; > }; > > _rsc { > @@ -76,6 +104,42 @@ ap_ts_pen_1v8: { > }; > }; > > +_out { Sorry for missing this while merging changes in sc7280.dtsi. But it would be really nice if this was labeled mdss_edp_out instead (or possibly defined within the _edp node). Now you will have _out and _out floating around away from the edp and dp nodes... > + remote-endpoint = <_panel_in>; > +}; > + > + { > + status = "okay"; > +}; > + > +_edp { > + status = "okay"; > + > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l10c_0p8>; > +}; > + > +_edp_phy { > + status = "okay"; > + > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l10c_0p8>; > +}; > + > +_dp { > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_hot_plug_det>; > + data-lanes = <0 1>; > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l1b_0p8>; > +}; > + > +_mdp { > + status = "okay"; > +}; > + > _3v3_regulator { > gpio = < 51 GPIO_ACTIVE_HIGH>; > }; > @@ -84,7 +148,65 @@ ap_ts_pen_1v8: { > pins = "gpio51"; > }; > > +_pwm { This label doesn't exist, so I won't be able to merge this patch. > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_bl_pwm>; > +}; > + > +_gpios { > + edp_bl_power: edp-bl-power { > + pins = "gpio7"; > + function = "normal"; > + qcom,drive-strength = ; > + bias-disable; > + output-low; > + }; > + > + edp_bl_pwm: edp-bl-pwm { > + pins = "gpio8"; > + function = "func1"; > + qcom,drive-strength = ; > + bias-disable; > + output-low; > + }; > +}; > + > + { > + edp_backlight: edp-backlight { > + compatible = "pwm-backlight"; This is not a device on the mmio bus, so it should not love within the > + > + power-supply = <_power>; > + pwms = <_pwm 3 65535>; > + }; > + > + edp_panel: edp_panel { Ditto. Regards, Bjorn > +
RE: [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD
Hi Matthias, I will implement the changes. Thank you, Sankeerth -Original Message- From: Matthias Kaehlcke Sent: Wednesday, February 9, 2022 3:54 AM To: Sankeerth Billakanti (QUIC) Cc: dri-devel@lists.freedesktop.org; linux-arm-...@vger.kernel.org; freedr...@lists.freedesktop.org; linux-ker...@vger.kernel.org; devicet...@vger.kernel.org; agr...@kernel.org; bjorn.anders...@linaro.org; robh...@kernel.org; robdcl...@gmail.com; seanp...@chromium.org; swb...@chromium.org; diand...@chromium.org; krzysztof.kozlow...@canonical.com; thierry.red...@gmail.com; s...@ravnborg.org; airl...@linux.ie; dan...@ffwll.ch; quic_kalyant ; Abhinav Kumar (QUIC) ; Kuogee Hsieh (QUIC) ; quic_mkrishn Subject: Re: [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD On Tue, Feb 08, 2022 at 08:48:43PM +0530, Sankeerth Billakanti wrote: > Enable the eDP display panel support without HPD on sc7280 platform. > > Signed-off-by: Sankeerth Billakanti > --- > > Changes in v2: > - sort node references alphabetically > - improve readability > - move the pwm pinctrl to pwm node > - move the regulators to root > - define backlight power > - remove dummy regulator node > - cleanup pinctrl definitions > > arch/arm64/boot/dts/qcom/sc7280-crd.dts | 122 > > arch/arm64/boot/dts/qcom/sc7280.dtsi| 2 - > 2 files changed, 122 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > index e2efbdd..bff2707 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > @@ -21,6 +21,34 @@ > chosen { > stdout-path = "serial0:115200n8"; > }; > + > + backlight_power: backlight-power { nit: the other fixed regulator in sc7280-idp.dtsi is called 'nvme_3v3_regulator', if you wanted to be consistent you could call this backlight_3v3_regulator. > + compatible = "regulator-fixed"; > + regulator-name = "backlight_power"; > + > + regulator-min-microvolt = <180>; > + regulator-max-microvolt = <180>; > + > + gpio = <_gpios 7 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_bl_power>; > + }; > + > + edp_power: edp-power { nit: see above > + compatible = "regulator-fixed"; > + regulator-name = "edp_power"; > + > + regulator-min-microvolt = <330>; > + regulator-max-microvolt = <330>; > + > + gpio = < 80 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_panel_power>; > + }; > }; > > _rsc { > @@ -76,6 +104,42 @@ ap_ts_pen_1v8: { > }; > }; > > +_out { > + remote-endpoint = <_panel_in>; > +}; > + > + { > + status = "okay"; > +}; > + > +_edp { > + status = "okay"; > + > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l10c_0p8>; > +}; > + > +_edp_phy { > + status = "okay"; > + > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l10c_0p8>; > +}; > + > +_dp { should be before 'mdss_edp'. > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_hot_plug_det>; > + data-lanes = <0 1>; > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l1b_0p8>; > +}; > + > +_mdp { > + status = "okay"; > +}; > + > _3v3_regulator { > gpio = < 51 GPIO_ACTIVE_HIGH>; > }; > @@ -84,7 +148,65 @@ ap_ts_pen_1v8: { > pins = "gpio51"; > }; > > +_pwm { > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_bl_pwm>; > +}; > + > +_gpios { should be before 'pm8350c_pwm' > + edp_bl_power: edp-bl-power { > + pins = "gpio7"; > + function = "normal"; > + qcom,drive-strength = ; > + bias-disable; > + output-low; > + }; > + > + edp_bl_pwm: edp-bl-pwm { > + pins = "gpio8"; > + function = "func1"; > + qcom,drive-strength = ; > + bias-disable; > + output-low; > + }; > +}; > + > + { > + edp_backlight: edp-backlight { > + compatible = "pwm-backlight"; > + > + power-supply = <_power>; > + pwms = <_pwm 3 65535>; > + }; > + > + edp_panel: edp_panel { in difference to labels node names should use dashes as separator, not underscores (i.e. 'edp-panel') > + compatible = "sharp,lq140m1jw46"; > + > + power-supply = <_power>; > + backlight = <_backlight>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + edp_panel_in:
Re: [Intel-gfx] [PATCH v3] drm/i915/dg2: Define GuC firmware version for DG2
On Mon, Feb 07, 2022 at 12:36:42PM -0800, john.c.harri...@intel.com wrote: From: John Harrison First release of GuC for DG2. Signed-off-by: John Harrison CC: Tomasz Mistat CC: Ramalingam C CC: Daniele Ceraolo Spurio Reviewed-by: Lucas De Marchi Lucas De Marchi --- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c index ba4f0970749b..efe0a6dcf9f7 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -50,6 +50,7 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, * firmware as TGL. */ #define INTEL_GUC_FIRMWARE_DEFS(fw_def, guc_def) \ + fw_def(DG2, 0, guc_def(dg2, 69, 0, 3)) \ fw_def(ALDERLAKE_P, 0, guc_def(adlp, 69, 0, 3)) \ fw_def(ALDERLAKE_S, 0, guc_def(tgl, 69, 0, 3)) \ fw_def(DG1, 0, guc_def(dg1, 69, 0, 3)) \ -- 2.25.1
Re: Kconfig CONFIG_FB dependency regression
On Tue, Feb 8, 2022 at 11:42 PM Thinh Nguyen wrote: > Randy Dunlap wrote: > > On 2/8/22 12:10, Thinh Nguyen wrote: > >> Randy Dunlap wrote: > >>> On 2/3/22 19:21, Thinh Nguyen wrote: > Ah.. It's because I don't use old.config as the base config. I use > x86_64_defconfig as the base plus some additional configs I need, and it > has CONFIG_FB_EFI set by default. Does it hang if you just disable CONFIG_FB_EFI on an otherwise working kernel? This is supposed to only disable the framebuffer, but it could be the actual cause if something else depends on its presence. Arnd
[PATCH] drm/nouveau: Remove the unused header file nvif/list.h
The nouveau driver depends on include/linux/list.h instead of nvif/list.h, so remove the obstacle-nvif/list.h. Signed-off-by: Cai Huoqing --- drivers/gpu/drm/nouveau/include/nvif/list.h | 353 1 file changed, 353 deletions(-) delete mode 100644 drivers/gpu/drm/nouveau/include/nvif/list.h diff --git a/drivers/gpu/drm/nouveau/include/nvif/list.h b/drivers/gpu/drm/nouveau/include/nvif/list.h deleted file mode 100644 index 8af5d144ecb0.. --- a/drivers/gpu/drm/nouveau/include/nvif/list.h +++ /dev/null @@ -1,353 +0,0 @@ -/* - * Copyright © 2010 Intel Corporation - * Copyright © 2010 Francisco Jerez - * - * Permission is hereby granted, free of charge, to any person obtaining a - * copy of this software and associated documentation files (the "Software"), - * to deal in the Software without restriction, including without limitation - * the rights to use, copy, modify, merge, publish, distribute, sublicense, - * and/or sell copies of the Software, and to permit persons to whom the - * Software is furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice (including the next - * paragraph) shall be included in all copies or substantial portions of the - * Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS - * IN THE SOFTWARE. - * - */ - -/* Modified by Ben Skeggs to match kernel list APIs */ - -#ifndef _XORG_LIST_H_ -#define _XORG_LIST_H_ - -/** - * @file Classic doubly-link circular list implementation. - * For real usage examples of the linked list, see the file test/list.c - * - * Example: - * We need to keep a list of struct foo in the parent struct bar, i.e. what - * we want is something like this. - * - * struct bar { - * ... - * struct foo *list_of_foos; -> struct foo {}, struct foo {}, struct foo{} - * ... - * } - * - * We need one list head in bar and a list element in all list_of_foos (both are of - * data type 'struct list_head'). - * - * struct bar { - * ... - * struct list_head list_of_foos; - * ... - * } - * - * struct foo { - * ... - * struct list_head entry; - * ... - * } - * - * Now we initialize the list head: - * - * struct bar bar; - * ... - * INIT_LIST_HEAD(_of_foos); - * - * Then we create the first element and add it to this list: - * - * struct foo *foo = malloc(...); - * - * list_add(>entry, _of_foos); - * - * Repeat the above for each element you want to add to the list. Deleting - * works with the element itself. - * list_del(>entry); - * free(foo); - * - * Note: calling list_del(_of_foos) will set bar.list_of_foos to an empty - * list again. - * - * Looping through the list requires a 'struct foo' as iterator and the - * name of the field the subnodes use. - * - * struct foo *iterator; - * list_for_each_entry(iterator, _of_foos, entry) { - * if (iterator->something == ...) - * ... - * } - * - * Note: You must not call list_del() on the iterator if you continue the - * loop. You need to run the safe for-each loop instead: - * - * struct foo *iterator, *next; - * list_for_each_entry_safe(iterator, next, _of_foos, entry) { - * if (...) - * list_del(>entry); - * } - * - */ - -/** - * The linkage struct for list nodes. This struct must be part of your - * to-be-linked struct. struct list_head is required for both the head of the - * list and for each list node. - * - * Position and name of the struct list_head field is irrelevant. - * There are no requirements that elements of a list are of the same type. - * There are no requirements for a list head, any struct list_head can be a list - * head. - */ -struct list_head { -struct list_head *next, *prev; -}; - -/** - * Initialize the list as an empty list. - * - * Example: - * INIT_LIST_HEAD(>list_of_foos); - * - * @param The list to initialized. - */ -#define LIST_HEAD_INIT(name) { &(name), &(name) } - -#define LIST_HEAD(name) \ - struct list_head name = LIST_HEAD_INIT(name) - -static inline void -INIT_LIST_HEAD(struct list_head *list) -{ -list->next = list->prev = list; -} - -static inline void -__list_add(struct list_head *entry, -struct list_head *prev, struct list_head *next) -{ -next->prev = entry; -entry->next = next; -entry->prev = prev; -prev->next = entry; -} - -/** - * Insert a new element after the given list head. The new element does not - * need to be initialised as empty list.
Re: [PATCH v2 02/18] iosys-map: Add a few more helpers
On Wed, Feb 09, 2022 at 07:23:04AM +0100, Thomas Zimmermann wrote: Hi Am 08.02.22 um 11:45 schrieb Lucas De Marchi: First the simplest ones: - iosys_map_memset(): when abstracting system and I/O memory, just like the memcpy() use case, memset() also has dedicated functions to be called for using IO memory. - iosys_map_memcpy_from(): we may need to copy data from I/O memory, not only to. In certain situations it's useful to be able to read or write to an offset that is calculated by having the memory layout given by a struct declaration. Usually we are going to read/write a u8, u16, u32 or u64. As a pre-requisite for the implementation, add iosys_map_memcpy_from() to be the equivalent of iosys_map_memcpy_to(), but in the other direction. Then add 2 pairs of macros: - iosys_map_rd() / iosys_map_wr() - iosys_map_rd_field() / iosys_map_wr_field() The first pair takes the C-type and offset to read/write. The second pair uses a struct describing the layout of the mapping in order to calculate the offset and size being read/written. We could use readb, readw, readl, readq and the write* counterparts, however due to alignment issues this may not work on all architectures. If alignment needs to be checked to call the right function, it's not possible to decide at compile-time which function to call: so just leave the decision to the memcpy function that will do exactly that. Finally, in order to use the above macros with a map derived from another, add another initializer: IOSYS_MAP_INIT_OFFSET(). v2: - Rework IOSYS_MAP_INIT_OFFSET() so it doesn't rely on aliasing rules within the union - Add offset to both iosys_map_rd_field() and iosys_map_wr_field() to allow the struct itself to be at an offset from the mapping - Add documentation to iosys_map_rd_field() with example and expected memory layout Cc: Sumit Semwal Cc: Christian König Cc: Thomas Zimmermann Cc: Mauro Carvalho Chehab Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Lucas De Marchi --- include/linux/iosys-map.h | 202 ++ 1 file changed, 202 insertions(+) diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h index edd730b1e899..c6b223534b21 100644 --- a/include/linux/iosys-map.h +++ b/include/linux/iosys-map.h @@ -6,6 +6,7 @@ #ifndef __IOSYS_MAP_H__ #define __IOSYS_MAP_H__ +#include #include #include Alphabetically sorted, please. What requires kernel.h? Can this be reduced to another include statement? Maybe stddef.h for offsetof() ? Humn... I believe it was something in the previous implementation that is not there anymore. Because this builds fine without the include now and I don't think it is something being included by the headers already here. So this additional include can just be removed. Lucas De Marchi
[PATCH v7 6/6] drm/i915/gt: replace cache_clflush_range
Replace all occurance of cache_clflush_range with drm_clflush_virt_range. This will prevent compile errors on non-x86 platforms. Signed-off-by: Michael Cheng --- drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 12 ++-- drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 2 +- drivers/gpu/drm/i915/gt/intel_gtt.c | 2 +- drivers/gpu/drm/i915/gt/intel_ppgtt.c| 2 +- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c| 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c index c43e724afa9f..d0999e92621b 100644 --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c @@ -444,11 +444,11 @@ gen8_ppgtt_insert_pte(struct i915_ppgtt *ppgtt, pd = pdp->entry[gen8_pd_index(idx, 2)]; } - clflush_cache_range(vaddr, PAGE_SIZE); + drm_clflush_virt_range(vaddr, PAGE_SIZE); vaddr = px_vaddr(i915_pt_entry(pd, gen8_pd_index(idx, 1))); } } while (1); - clflush_cache_range(vaddr, PAGE_SIZE); + drm_clflush_virt_range(vaddr, PAGE_SIZE); return idx; } @@ -532,7 +532,7 @@ static void gen8_ppgtt_insert_huge(struct i915_address_space *vm, } } while (rem >= page_size && index < I915_PDES); - clflush_cache_range(vaddr, PAGE_SIZE); + drm_clflush_virt_range(vaddr, PAGE_SIZE); /* * Is it safe to mark the 2M block as 64K? -- Either we have @@ -548,7 +548,7 @@ static void gen8_ppgtt_insert_huge(struct i915_address_space *vm, I915_GTT_PAGE_SIZE_2M { vaddr = px_vaddr(pd); vaddr[maybe_64K] |= GEN8_PDE_IPS_64K; - clflush_cache_range(vaddr, PAGE_SIZE); + drm_clflush_virt_range(vaddr, PAGE_SIZE); page_size = I915_GTT_PAGE_SIZE_64K; /* @@ -569,7 +569,7 @@ static void gen8_ppgtt_insert_huge(struct i915_address_space *vm, for (i = 1; i < index; i += 16) memset64(vaddr + i, encode, 15); - clflush_cache_range(vaddr, PAGE_SIZE); + drm_clflush_virt_range(vaddr, PAGE_SIZE); } } @@ -617,7 +617,7 @@ static void gen8_ppgtt_insert_entry(struct i915_address_space *vm, vaddr = px_vaddr(i915_pt_entry(pd, gen8_pd_index(idx, 1))); vaddr[gen8_pd_index(idx, 0)] = gen8_pte_encode(addr, level, flags); - clflush_cache_range([gen8_pd_index(idx, 0)], sizeof(*vaddr)); + drm_clflush_virt_range([gen8_pd_index(idx, 0)], sizeof(*vaddr)); } static int gen8_init_scratch(struct i915_address_space *vm) diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index cc561cfae808..bbe33794b34d 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -2822,7 +2822,7 @@ static void execlists_sanitize(struct intel_engine_cs *engine) sanitize_hwsp(engine); /* And scrub the dirty cachelines for the HWSP */ - clflush_cache_range(engine->status_page.addr, PAGE_SIZE); + drm_clflush_virt_range(engine->status_page.addr, PAGE_SIZE); intel_engine_reset_pinned_contexts(engine); } diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c index 0d6bbc8c57f2..9b594be9102f 100644 --- a/drivers/gpu/drm/i915/gt/intel_gtt.c +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c @@ -255,7 +255,7 @@ fill_page_dma(struct drm_i915_gem_object *p, const u64 val, unsigned int count) void *vaddr = __px_vaddr(p); memset64(vaddr, val, count); - clflush_cache_range(vaddr, PAGE_SIZE); + drm_clflush_virt_range(vaddr, PAGE_SIZE); } static void poison_scratch_page(struct drm_i915_gem_object *scratch) diff --git a/drivers/gpu/drm/i915/gt/intel_ppgtt.c b/drivers/gpu/drm/i915/gt/intel_ppgtt.c index 48e6e2f87700..bd474a5123cb 100644 --- a/drivers/gpu/drm/i915/gt/intel_ppgtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ppgtt.c @@ -90,7 +90,7 @@ write_dma_entry(struct drm_i915_gem_object * const pdma, u64 * const vaddr = __px_vaddr(pdma); vaddr[idx] = encoded_entry; - clflush_cache_range([idx], sizeof(u64)); + drm_clflush_virt_range([idx], sizeof(u64)); } void diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index b3a429a92c0d..89020706adc4 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++
[PATCH v7 5/6] drm/i915/: Re-work clflush_write32
Use drm_clflush_virt_range instead of clflushopt and remove the memory barrier, since drm_clflush_virt_range takes care of that. Signed-off-by: Michael Cheng --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 498b458fd784..0854276ff7ba 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1332,10 +1332,8 @@ static void *reloc_vaddr(struct i915_vma *vma, static void clflush_write32(u32 *addr, u32 value, unsigned int flushes) { if (unlikely(flushes & (CLFLUSH_BEFORE | CLFLUSH_AFTER))) { - if (flushes & CLFLUSH_BEFORE) { - clflushopt(addr); - mb(); - } + if (flushes & CLFLUSH_BEFORE) + drm_clflush_virt_range(addr, sizeof(addr)); *addr = value; @@ -1347,7 +1345,7 @@ static void clflush_write32(u32 *addr, u32 value, unsigned int flushes) * to ensure ordering of clflush wrt to the system. */ if (flushes & CLFLUSH_AFTER) - clflushopt(addr); + drm_clflush_virt_range(addr, sizeof(addr)); } else *addr = value; } -- 2.25.1
[PATCH v7 0/6] Use drm_clflush* instead of clflush
This patch series re-work a few i915 functions to use drm_clflush_virt_range instead of calling clflush or clflushopt directly. This will prevent errors when building for non-x86 architectures. v2: s/PAGE_SIZE/sizeof(value) for Re-work intel_write_status_page and added more patches to convert additional clflush/clflushopt to use drm_clflush*. (Michael Cheng) v3: Drop invalidate_csb_entries and directly invoke drm_clflush_virt_ran v4: Remove extra memory barriers v5: s/cache_clflush_range/drm_clflush_virt_range v6: Fix up "Drop invalidate_csb_entries" to use correct parameters. Also added in arm64 support for drm_clflush_virt_range. v7: Re-order patches, and use correct macro for dcache flush for arm64. Michael Cheng (6): drm: Add arch arm64 for drm_clflush_virt_range drm/i915/gt: Re-work intel_write_status_page drm/i915/gt: Drop invalidate_csb_entries drm/i915/gt: Re-work reset_csb drm/i915/: Re-work clflush_write32 drm/i915/gt: replace cache_clflush_range drivers/gpu/drm/drm_cache.c | 8 .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 8 +++- drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 12 ++-- drivers/gpu/drm/i915/gt/intel_engine.h| 13 - .../drm/i915/gt/intel_execlists_submission.c | 19 ++- drivers/gpu/drm/i915/gt/intel_gtt.c | 2 +- drivers/gpu/drm/i915/gt/intel_ppgtt.c | 2 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- 8 files changed, 30 insertions(+), 36 deletions(-) -- 2.25.1
[PATCH v7 3/6] drm/i915/gt: Drop invalidate_csb_entries
Drop invalidate_csb_entries and directly call drm_clflush_virt_range. This allows for one less function call, and prevent complier errors when building for non-x86 architectures. v2(Michael Cheng): Drop invalidate_csb_entries function and directly invoke drm_clflush_virt_range. Thanks to Tvrtko for the sugguestion. v3(Michael Cheng): Use correct parameters for drm_clflush_virt_range. Thanks to Tvrtko for pointing this out. Signed-off-by: Michael Cheng --- drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 12 +++- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 9bb7c863172f..28f2581d3046 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -1646,12 +1646,6 @@ cancel_port_requests(struct intel_engine_execlists * const execlists, return inactive; } -static void invalidate_csb_entries(const u64 *first, const u64 *last) -{ - clflush((void *)first); - clflush((void *)last); -} - /* * Starting with Gen12, the status has a new format: * @@ -1999,7 +1993,7 @@ process_csb(struct intel_engine_cs *engine, struct i915_request **inactive) * the wash as hardware, working or not, will need to do the * invalidation before. */ - invalidate_csb_entries([0], [num_entries - 1]); + drm_clflush_virt_range([0], num_entries * sizeof(buf[0])); /* * We assume that any event reflects a change in context flow @@ -2783,8 +2777,8 @@ static void reset_csb_pointers(struct intel_engine_cs *engine) /* Check that the GPU does indeed update the CSB entries! */ memset(execlists->csb_status, -1, (reset_value + 1) * sizeof(u64)); - invalidate_csb_entries(>csb_status[0], - >csb_status[reset_value]); + drm_clflush_virt_range(>csb_status[0], + execlists->csb_size * sizeof(execlists->csb_status[0])); /* Once more for luck and our trusty paranoia */ ENGINE_WRITE(engine, RING_CONTEXT_STATUS_PTR, -- 2.25.1
[PATCH v7 4/6] drm/i915/gt: Re-work reset_csb
Use drm_clflush_virt_range instead of directly invoking clflush. This will prevent compiler errors when building for non-x86 architectures. v2(Michael Cheng): Remove extra clflush v3(Michael Cheng): Remove memory barrier since drm_clflush_virt_range takes care of it. Signed-off-by: Michael Cheng --- drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 28f2581d3046..cc561cfae808 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -2944,9 +2944,8 @@ reset_csb(struct intel_engine_cs *engine, struct i915_request **inactive) { struct intel_engine_execlists * const execlists = >execlists; - mb(); /* paranoia: read the CSB pointers from after the reset */ - clflush(execlists->csb_write); - mb(); + drm_clflush_virt_range(execlists->csb_write, + sizeof(execlists->csb_write)); inactive = process_csb(engine, inactive); /* drain preemption events */ -- 2.25.1
[PATCH v7 2/6] drm/i915/gt: Re-work intel_write_status_page
Re-work intel_write_status_page to use drm_clflush_virt_range. This will prevent compiler errors when building for non-x86 architectures. Signed-off-by: Michael Cheng --- drivers/gpu/drm/i915/gt/intel_engine.h | 13 - 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index 0e353d8c2bc8..986777c2430d 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -143,15 +144,9 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value) * of extra paranoia to try and ensure that the HWS takes the value * we give and that it doesn't end up trapped inside the CPU! */ - if (static_cpu_has(X86_FEATURE_CLFLUSH)) { - mb(); - clflush(>status_page.addr[reg]); - engine->status_page.addr[reg] = value; - clflush(>status_page.addr[reg]); - mb(); - } else { - WRITE_ONCE(engine->status_page.addr[reg], value); - } + drm_clflush_virt_range(>status_page.addr[reg], sizeof(value)); + WRITE_ONCE(engine->status_page.addr[reg], value); + drm_clflush_virt_range(>status_page.addr[reg], sizeof(value)); } /* -- 2.25.1
[PATCH v7 1/6] drm: Add arch arm64 for drm_clflush_virt_range
Add arm64 support for drm_clflush_virt_range. dcache_clean_inval_poc performs a flush by first performing a clean, follow by an invalidation operation. v2 (Michael Cheng): Use correct macro for cleaning and invalidation the dcache. Signed-off-by: Michael Cheng --- drivers/gpu/drm/drm_cache.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c index f19d9acbe959..94b3cc3fd482 100644 --- a/drivers/gpu/drm/drm_cache.c +++ b/drivers/gpu/drm/drm_cache.c @@ -39,6 +39,10 @@ /* A small bounce buffer that fits on the stack. */ #define MEMCPY_BOUNCE_SIZE 128 +#if defined(CONFIG_ARM64) +#include +#endif + #if defined(CONFIG_X86) #include @@ -176,6 +180,10 @@ drm_clflush_virt_range(void *addr, unsigned long length) if (wbinvd_on_all_cpus()) pr_err("Timed out waiting for cache flush\n"); + +#elif defined(CONFIG_ARM64) + void *end = addr + length; + dcache_clean_inval_poc((unsigned long)addr, (unsigned long)end); #else pr_err("Architecture has no drm_cache.c support\n"); WARN_ON_ONCE(1); -- 2.25.1
Re: [PATCH v2 02/18] iosys-map: Add a few more helpers
Hi Am 08.02.22 um 11:45 schrieb Lucas De Marchi: First the simplest ones: - iosys_map_memset(): when abstracting system and I/O memory, just like the memcpy() use case, memset() also has dedicated functions to be called for using IO memory. - iosys_map_memcpy_from(): we may need to copy data from I/O memory, not only to. In certain situations it's useful to be able to read or write to an offset that is calculated by having the memory layout given by a struct declaration. Usually we are going to read/write a u8, u16, u32 or u64. As a pre-requisite for the implementation, add iosys_map_memcpy_from() to be the equivalent of iosys_map_memcpy_to(), but in the other direction. Then add 2 pairs of macros: - iosys_map_rd() / iosys_map_wr() - iosys_map_rd_field() / iosys_map_wr_field() The first pair takes the C-type and offset to read/write. The second pair uses a struct describing the layout of the mapping in order to calculate the offset and size being read/written. We could use readb, readw, readl, readq and the write* counterparts, however due to alignment issues this may not work on all architectures. If alignment needs to be checked to call the right function, it's not possible to decide at compile-time which function to call: so just leave the decision to the memcpy function that will do exactly that. Finally, in order to use the above macros with a map derived from another, add another initializer: IOSYS_MAP_INIT_OFFSET(). v2: - Rework IOSYS_MAP_INIT_OFFSET() so it doesn't rely on aliasing rules within the union - Add offset to both iosys_map_rd_field() and iosys_map_wr_field() to allow the struct itself to be at an offset from the mapping - Add documentation to iosys_map_rd_field() with example and expected memory layout Cc: Sumit Semwal Cc: Christian König Cc: Thomas Zimmermann Cc: Mauro Carvalho Chehab Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Lucas De Marchi --- include/linux/iosys-map.h | 202 ++ 1 file changed, 202 insertions(+) diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h index edd730b1e899..c6b223534b21 100644 --- a/include/linux/iosys-map.h +++ b/include/linux/iosys-map.h @@ -6,6 +6,7 @@ #ifndef __IOSYS_MAP_H__ #define __IOSYS_MAP_H__ +#include #include #include Alphabetically sorted, please. What requires kernel.h? Can this be reduced to another include statement? Maybe stddef.h for offsetof() ? Best regards Thomas @@ -120,6 +121,45 @@ struct iosys_map { .is_iomem = false, \ } +/** + * IOSYS_MAP_INIT_OFFSET - Initializes struct iosys_map from another iosys_map + * @map_: The dma-buf mapping structure to copy from + * @offset_: Offset to add to the other mapping + * + * Initializes a new iosys_map struct based on another passed as argument. It + * does a shallow copy of the struct so it's possible to update the back storage + * without changing where the original map points to. It is the equivalent of + * doing: + * + * .. code-block:: c + * + * iosys_map map = other_map; + * iosys_map_incr(, ); + * + * Example usage: + * + * .. code-block:: c + * + * void foo(struct device *dev, struct iosys_map *base_map) + * { + * ... + * struct iosys_map map = IOSYS_MAP_INIT_OFFSET(base_map, FIELD_OFFSET); + * ... + * } + * + * The advantage of using the initializer over just increasing the offset with + * iosys_map_incr() like above is that the new map will always point to the + * right place of the buffer during its scope. It reduces the risk of updating + * the wrong part of the buffer and having no compiler warning about that. If + * the assignment to IOSYS_MAP_INIT_OFFSET() is forgotten, the compiler can warn + * about the use of uninitialized variable. + */ +#define IOSYS_MAP_INIT_OFFSET(map_, offset_) ({ \ + struct iosys_map copy = *map_; \ + iosys_map_incr(, offset_); \ + copy; \ +}) + /** * iosys_map_set_vaddr - Sets a iosys mapping structure to an address in system memory * @map: The iosys_map structure @@ -239,6 +279,26 @@ static inline void iosys_map_memcpy_to(struct iosys_map *dst, size_t dst_offset, memcpy(dst->vaddr + dst_offset, src, len); } +/** + * iosys_map_memcpy_from - Memcpy from iosys_map into system memory + * @dst: Destination in system memory + * @src: The iosys_map structure + * @src_offset:The offset from which to copy + * @len: The number of byte in src + * + * Copies data from a iosys_map with an offset. The dest buffer is in + * system memory. Depending on the mapping location, the helper picks the + * correct method
Re: [PATCH v2 02/18] iosys-map: Add a few more helpers
Em Tue, 8 Feb 2022 02:45:08 -0800 Lucas De Marchi escreveu: > First the simplest ones: > > - iosys_map_memset(): when abstracting system and I/O memory, > just like the memcpy() use case, memset() also has dedicated > functions to be called for using IO memory. > - iosys_map_memcpy_from(): we may need to copy data from I/O > memory, not only to. > > In certain situations it's useful to be able to read or write to an > offset that is calculated by having the memory layout given by a struct > declaration. Usually we are going to read/write a u8, u16, u32 or u64. > > As a pre-requisite for the implementation, add iosys_map_memcpy_from() > to be the equivalent of iosys_map_memcpy_to(), but in the other > direction. Then add 2 pairs of macros: > > - iosys_map_rd() / iosys_map_wr() > - iosys_map_rd_field() / iosys_map_wr_field() > > The first pair takes the C-type and offset to read/write. The second > pair uses a struct describing the layout of the mapping in order to > calculate the offset and size being read/written. > > We could use readb, readw, readl, readq and the write* counterparts, > however due to alignment issues this may not work on all architectures. > If alignment needs to be checked to call the right function, it's not > possible to decide at compile-time which function to call: so just leave > the decision to the memcpy function that will do exactly that. > > Finally, in order to use the above macros with a map derived from > another, add another initializer: IOSYS_MAP_INIT_OFFSET(). > > v2: > - Rework IOSYS_MAP_INIT_OFFSET() so it doesn't rely on aliasing rules > within the union > - Add offset to both iosys_map_rd_field() and iosys_map_wr_field() to > allow the struct itself to be at an offset from the mapping > - Add documentation to iosys_map_rd_field() with example and expected > memory layout > > Cc: Sumit Semwal > Cc: Christian König > Cc: Thomas Zimmermann > Cc: Mauro Carvalho Chehab > Cc: dri-devel@lists.freedesktop.org > Cc: linux-ker...@vger.kernel.org > Signed-off-by: Lucas De Marchi LGTM. Reviewed-by: Mauro Carvalho Chehab Regards, Mauro > --- > include/linux/iosys-map.h | 202 ++ > 1 file changed, 202 insertions(+) > > diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h > index edd730b1e899..c6b223534b21 100644 > --- a/include/linux/iosys-map.h > +++ b/include/linux/iosys-map.h > @@ -6,6 +6,7 @@ > #ifndef __IOSYS_MAP_H__ > #define __IOSYS_MAP_H__ > > +#include > #include > #include > > @@ -120,6 +121,45 @@ struct iosys_map { > .is_iomem = false, \ > } > > +/** > + * IOSYS_MAP_INIT_OFFSET - Initializes struct iosys_map from another > iosys_map > + * @map_:The dma-buf mapping structure to copy from > + * @offset_: Offset to add to the other mapping > + * > + * Initializes a new iosys_map struct based on another passed as argument. It > + * does a shallow copy of the struct so it's possible to update the back > storage > + * without changing where the original map points to. It is the equivalent of > + * doing: > + * > + * .. code-block:: c > + * > + * iosys_map map = other_map; > + * iosys_map_incr(, ); > + * > + * Example usage: > + * > + * .. code-block:: c > + * > + * void foo(struct device *dev, struct iosys_map *base_map) > + * { > + * ... > + * struct iosys_map map = IOSYS_MAP_INIT_OFFSET(base_map, > FIELD_OFFSET); > + * ... > + * } > + * > + * The advantage of using the initializer over just increasing the offset > with > + * iosys_map_incr() like above is that the new map will always point to the > + * right place of the buffer during its scope. It reduces the risk of > updating > + * the wrong part of the buffer and having no compiler warning about that. If > + * the assignment to IOSYS_MAP_INIT_OFFSET() is forgotten, the compiler can > warn > + * about the use of uninitialized variable. > + */ > +#define IOSYS_MAP_INIT_OFFSET(map_, offset_) ({ > \ > + struct iosys_map copy = *map_; \ > + iosys_map_incr(, offset_); \ > + copy; \ > +}) > + > /** > * iosys_map_set_vaddr - Sets a iosys mapping structure to an address in > system memory > * @map: The iosys_map structure > @@ -239,6 +279,26 @@ static inline void iosys_map_memcpy_to(struct iosys_map > *dst, size_t dst_offset, > memcpy(dst->vaddr + dst_offset, src, len); > } > > +/** > + * iosys_map_memcpy_from - Memcpy from iosys_map into system memory > + * @dst: Destination in system memory > + * @src: The iosys_map structure > + * @src_offset: The offset from which to copy > + * @len: The number of byte in src > + * > + * Copies data from a iosys_map with an offset. The dest buffer is in > + *
Re: [RFC v3 00/12] Define and use reset domain for GPU recovery in amdgpu
Hi Andrey, I have been testing your patch and it seems fine till now. Best Regards, Jingwen Chen On 2022/2/3 上午2:57, Andrey Grodzovsky wrote: > Just another ping, with Shyun's help I was able to do some smoke testing on > XGMI SRIOV system (booting and triggering hive reset) > and for now looks good. > > Andrey > > On 2022-01-28 14:36, Andrey Grodzovsky wrote: >> Just a gentle ping if people have more comments on this patch set ? >> Especially last 5 patches >> as first 7 are exact same as V2 and we already went over them mostly. >> >> Andrey >> >> On 2022-01-25 17:37, Andrey Grodzovsky wrote: >>> This patchset is based on earlier work by Boris[1] that allowed to have an >>> ordered workqueue at the driver level that will be used by the different >>> schedulers to queue their timeout work. On top of that I also serialized >>> any GPU reset we trigger from within amdgpu code to also go through the same >>> ordered wq and in this way simplify somewhat our GPU reset code so we don't >>> need >>> to protect from concurrency by multiple GPU reset triggeres such as TDR on >>> one >>> hand and sysfs trigger or RAS trigger on the other hand. >>> >>> As advised by Christian and Daniel I defined a reset_domain struct such that >>> all the entities that go through reset together will be serialized one >>> against >>> another. >>> >>> TDR triggered by multiple entities within the same domain due to the same >>> reason will not >>> be triggered as the first such reset will cancel all the pending resets. >>> This is >>> relevant only to TDR timers and not to triggered resets coming from RAS or >>> SYSFS, >>> those will still happen after the in flight resets finishes. >>> >>> v2: >>> Add handling on SRIOV configuration, the reset notify coming from host >>> and driver already trigger a work queue to handle the reset so drop this >>> intermediate wq and send directly to timeout wq. (Shaoyun) >>> >>> v3: >>> Lijo suggested puting 'adev->in_gpu_reset' in amdgpu_reset_domain struct. >>> I followed his advise and also moved adev->reset_sem into same place. This >>> in turn caused to do some follow-up refactor of the original patches >>> where i decoupled amdgpu_reset_domain life cycle frolm XGMI hive because >>> hive is destroyed and >>> reconstructed for the case of reset the devices in the XGMI hive during >>> probe for SRIOV See [2] >>> while we need the reset sem and gpu_reset flag to always be present. This >>> was attained >>> by adding refcount to amdgpu_reset_domain so each device can safely point >>> to it as long as >>> it needs. >>> >>> >>> [1] >>> https://patchwork.kernel.org/project/dri-devel/patch/20210629073510.2764391-3-boris.brezil...@collabora.com/ >>> [2] https://www.spinics.net/lists/amd-gfx/msg58836.html >>> >>> P.S Going through drm-misc-next and not amd-staging-drm-next as Boris work >>> hasn't landed yet there. >>> >>> P.P.S Patches 8-12 are the refactor on top of the original V2 patchset. >>> >>> P.P.P.S I wasn't able yet to test the reworked code on XGMI SRIOV system >>> because drm-misc-next fails to load there. >>> Would appriciate if maybe jingwech can try it on his system like he tested >>> V2. >>> >>> Andrey Grodzovsky (12): >>> drm/amdgpu: Introduce reset domain >>> drm/amdgpu: Move scheduler init to after XGMI is ready >>> drm/amdgpu: Fix crash on modprobe >>> drm/amdgpu: Serialize non TDR gpu recovery with TDRs >>> drm/amd/virt: For SRIOV send GPU reset directly to TDR queue. >>> drm/amdgpu: Drop hive->in_reset >>> drm/amdgpu: Drop concurrent GPU reset protection for device >>> drm/amdgpu: Rework reset domain to be refcounted. >>> drm/amdgpu: Move reset sem into reset_domain >>> drm/amdgpu: Move in_gpu_reset into reset_domain >>> drm/amdgpu: Rework amdgpu_device_lock_adev >>> Revert 'drm/amdgpu: annotate a false positive recursive locking' >>> >>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 15 +- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 +- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 275 ++ >>> drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 43 +-- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- >>> .../gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 18 +- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 39 +++ >>> drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h | 12 + >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 + >>> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 24 +- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 3 +- >>> drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 6 +- >>> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 14 +- >>> drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 19 +- >>> drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 19 +- >>> drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 11 +- >>> 16 files changed, 313 insertions(+), 199 deletions(-) >>>
[PATCH] drm/nouveau: Fix a potential theorical leak in nouveau_get_backlight_name()
If successful ida_simple_get() calls are not undone when needed, some additional memory may be allocated and wasted. Here, an ID between 0 and MAX_INT is required. If this ID is >=100, it is not taken into account and is wasted. It should be released. Instead of calling ida_simple_remove(), take advantage of the 'max' parameter to require the ID not to be too big. Should it be too big, it is not allocated and don't need to be freed. While at it, use ida_alloc_xxx()/ida_free() instead to ida_simple_get()/ida_simple_remove(). The latter is deprecated and more verbose. Fixes: db1a0ae21461 ("drm/nouveau/bl: Assign different names to interfaces") Signed-off-by: Christophe JAILLET --- This patch is more a clean-up than a fix. It is unlikely than >= 100 backlight devices will be registered, and the over allocation would occur even much later when the underlying xarray is full. I also think that the 'if (bl->id >= 0)' before the ida_simple_remove() calls are useless. We don't store the id if a negative (i.e. error) is returned by ida_simple_get(). Finally, having a '#define BL_MAX_MINORS 99' could be better than a magic number in the code. --- drivers/gpu/drm/nouveau/nouveau_backlight.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c index ae2f2abc8f5a..ccd080ba30bf 100644 --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c @@ -46,8 +46,8 @@ static bool nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE], struct nouveau_backlight *bl) { - const int nb = ida_simple_get(_ida, 0, 0, GFP_KERNEL); - if (nb < 0 || nb >= 100) + const int nb = ida_alloc_max(_ida, 99, GFP_KERNEL); + if (nb < 0) return false; if (nb > 0) snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb); @@ -414,7 +414,7 @@ nouveau_backlight_init(struct drm_connector *connector) nv_encoder, ops, ); if (IS_ERR(bl->dev)) { if (bl->id >= 0) - ida_simple_remove(_ida, bl->id); + ida_free(_ida, bl->id); ret = PTR_ERR(bl->dev); goto fail_alloc; } @@ -442,7 +442,7 @@ nouveau_backlight_fini(struct drm_connector *connector) return; if (bl->id >= 0) - ida_simple_remove(_ida, bl->id); + ida_free(_ida, bl->id); backlight_device_unregister(bl->dev); nv_conn->backlight = NULL; -- 2.32.0
Re: [PATCH 20/27] arm64: dts: rockchip: rk356x: Add VOP2 nodes
On Wed, 26 Jan 2022 15:55:42 +0100, Sascha Hauer wrote: > The VOP2 is the display output controller on the RK3568. Add the node > for it to the dtsi file along with the required display-subsystem node > and the iommu node. > > changes since v3: > - Bring back gamma_lut regs > - Drop redundant _vop suffix from clock names > > Signed-off-by: Sascha Hauer > --- > arch/arm64/boot/dts/rockchip/rk3566.dtsi | 4 ++ > arch/arm64/boot/dts/rockchip/rk3568.dtsi | 4 ++ > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 51 > include/dt-bindings/soc/rockchip,vop2.h | 14 +++ > 4 files changed, 73 insertions(+) > create mode 100644 include/dt-bindings/soc/rockchip,vop2.h > Acked-by: Rob Herring
Re: [PATCH 16/27] dt-bindings: display: rockchip: dw-hdmi: Add additional clock
On Wed, 26 Jan 2022 15:55:38 +0100, Sascha Hauer wrote: > The rk3568 HDMI has an additional clock that needs to be enabled for the > HDMI controller to work. The purpose of that clock is not clear. It is > named "hclk" in the downstream driver, so use the same name. > > Signed-off-by: Sascha Hauer > --- > .../bindings/display/rockchip/rockchip,dw-hdmi.yaml| 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > Acked-by: Rob Herring
Re: [PATCH 15/27] dt-bindings: display: rockchip: dw-hdmi: Add regulator support
On Wed, 26 Jan 2022 15:55:37 +0100, Sascha Hauer wrote: > The RK3568 has HDMI_TX_AVDD0V9 and HDMI_TX_AVDD_1V8 supply inputs > needed for the HDMI port. Add the binding for these supplies. > > Signed-off-by: Sascha Hauer > --- > .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 11 +++ > 1 file changed, 11 insertions(+) > Acked-by: Rob Herring
Re: [PATCH 7/8] mm: remove the extra ZONE_DEVICE struct page refcount
On Sun, Feb 6, 2022 at 10:33 PM Christoph Hellwig wrote: [..] > @@ -500,28 +482,27 @@ void free_devmap_managed_page(struct page *page) > */ > page->mapping = NULL; > page->pgmap->ops->page_free(page); > + > + /* > +* Reset the page count to 1 to prepare for handing out the page > again. > +*/ > + set_page_count(page, 1); Interesting. I had expected that to really fix the refcount problem that fs/dax.c would need to start taking real page references as pages were added to a mapping, just like page cache. This looks ok to me, and passes my tests. So given I'm still working my way back to fixing the references properly I'm ok for this hack to replace the more broken hack that is there presently. Reviewed-by: Dan Williams
Re: [PATCH 14/27] dt-bindings: display: rockchip: dw-hdmi: use "ref" as clock name
On Wed, 26 Jan 2022 15:55:36 +0100, Sascha Hauer wrote: > "vpll" is a misnomer. A clock input to a device should be named after > the usage in the device, not after the clock that drives it. On the > rk3568 the same clock is driven by the HPLL. > This patch adds "ref" as a new alternative clock name for "vpll" > > Changes since v3: > - Keep old clock name for compatibility reasons > > Signed-off-by: Sascha Hauer > --- > .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > Acked-by: Rob Herring
Re: [PATCH 13/27] dt-bindings: display: rockchip: dw-hdmi: Make unwedge pinctrl optional
On Wed, 26 Jan 2022 15:55:35 +0100, Sascha Hauer wrote: > None of the upstream device tree files has a "unwedge" pinctrl > specified. Make it optional. > > Signed-off-by: Sascha Hauer > --- > .../devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring
Re: [PATCH 5/6] dt-bindings: vendor-prefixes: add vendor prefix for SHIFT
On Sun, 23 Jan 2022 17:38:08 +, Caleb Connolly wrote: > Add SHIFT vendor prefix, SHIFT make various devices such as the SHIF6mq > phone. > > Signed-off-by: Caleb Connolly > --- > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring
Re: [PATCH 3/6] dt-bindings: display: visionox-rm69299: document new compatible string
On Sun, 23 Jan 2022 17:37:41 +, Caleb Connolly wrote: > Document a new compatible string for the second panel variant. > > Signed-off-by: Caleb Connolly > --- > .../devicetree/bindings/display/panel/visionox,rm69299.yaml | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Rob Herring
Re: [PATCH v1, 2/3] dt-bindings: media: mtk-vcodec: Adds decoder dt-bindings for mt8186
On Sat, 22 Jan 2022 15:56:05 +0800, Yunfei Dong wrote: > Adds decoder dt-bindings for mt8186. > > Signed-off-by: Yunfei Dong > --- > .../bindings/media/mediatek,vcodec-subdev-decoder.yaml| 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Rob Herring
Re: [PATCH 3/3] dt-bindings: display: msm: Add binding for msm8998 dpu
On Thu, 13 Jan 2022 16:51:11 +0200, Jami Kettunen wrote: > From: AngeloGioacchino Del Regno > > Add yaml binding for msm8998 dpu1 support. > > Signed-off-by: AngeloGioacchino Del Regno > > Signed-off-by: Jami Kettunen > --- > .../bindings/display/msm/dpu-msm8998.yaml | 219 ++ > 1 file changed, 219 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/msm/dpu-msm8998.yaml > Reviewed-by: Rob Herring
Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
Hi Johannes On 2/8/2022 1:54 PM, Johannes Berg wrote: On Tue, 2022-02-08 at 13:40 -0800, Abhinav Kumar wrote: I am checking what usermode sees and will get back ( I didnt see an error do most likely it was EOF ). I didnt follow the second part. I think probably it got -ENODEV, looking at kernfs_file_read_iter(). If the file descriptor read returns EOF, even if we consider them separate how will it resolve this issue? My earlier questions were related to fixing it in devcoredump to detect and fix it there. Are you suggesting to fix in usermode instead? How? Yeah, no, you cannot fix it in userspace. But I just followed the rabbit hole down kernfs and all, and it looks like indeed the read would be cut short with -ENODEV, sorry. It doesn't look like there's good API for this, but it seems at least from the underlying kernfs POV it should be possible to get_device() in open and put_device() in release, so that the device sticks around while somebody has the file open? It's entirely virtual, so this should be OK? johannes Are you suggesting something like below? diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index 42dcf96..14203d0 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -32,6 +32,22 @@ static const struct sysfs_ops *sysfs_file_ops(struct kernfs_node *kn) return kobj->ktype ? kobj->ktype->sysfs_ops : NULL; } +static int sysfs_kf_open(struct kernfs_open_file *of) +{ + struct kobject *kobj = of->kn->parent->priv; + struct device *dev = kobj_to_dev(kobj); + + get_device(dev); +} + +static void sysfs_kf_release(struct kernfs_open_file *of) +{ + struct kobject *kobj = of->kn->parent->priv; + struct device *dev = kobj_to_dev(kobj); + + put_device(dev); +} + /* * Reads on sysfs are handled through seq_file, which takes care of hairy * details like buffering and seeking. The following function pipes @@ -211,6 +227,8 @@ static const struct kernfs_ops sysfs_file_kfops_wo = { }; static const struct kernfs_ops sysfs_file_kfops_rw = { + .open = sysfs_kf_open; + .release= sysfs_kf_release; .seq_show = sysfs_kf_seq_show, .write = sysfs_kf_write, }; If so, dont you think this will be a more intrusive change just for the sake of devcoredump? Any other way to keep the changes limited to devcoredump? Thanks Abhinav
Re: [PATCH 1/3] drm/msm/dpu1: Add DMA2, DMA3 clock control to enum
On 13/01/2022 17:51, Jami Kettunen wrote: From: AngeloGioacchino Del Regno The enum dpu_clk_ctrl_type misses DPU_CLK_CTRL_DMA{2,3} even though this driver does actually handle both, if present: add the two in preparation for adding support for SoCs having them. Signed-off-by: AngeloGioacchino Del Regno Signed-off-by: Jami Kettunen Reviewed-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index 31af04afda7d..736f52c742fb 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -435,6 +435,8 @@ enum dpu_clk_ctrl_type { DPU_CLK_CTRL_RGB3, DPU_CLK_CTRL_DMA0, DPU_CLK_CTRL_DMA1, + DPU_CLK_CTRL_DMA2, + DPU_CLK_CTRL_DMA3, DPU_CLK_CTRL_CURSOR0, DPU_CLK_CTRL_CURSOR1, DPU_CLK_CTRL_INLINE_ROT0_SSPP, -- With best wishes Dmitry
[PATCH -next] drm/amdkfd: Fix NULL but dereferenced coccicheck error
Eliminate the following coccicheck warning: ./drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:2087:27-38: ERROR: bo_buckets is NULL but dereferenced. Reported-by: Abaci Robot Signed-off-by: Yang Li --- drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c index 64e3b4e3a712..636391c61caf 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c @@ -1982,10 +1982,8 @@ static int criu_checkpoint_bos(struct kfd_process *p, void *mem; bo_buckets = kvzalloc(num_bos * sizeof(*bo_buckets), GFP_KERNEL); - if (!bo_buckets) { - ret = -ENOMEM; - goto exit; - } + if (!bo_buckets) + return -ENOMEM; bo_privs = kvzalloc(num_bos * sizeof(*bo_privs), GFP_KERNEL); if (!bo_privs) { -- 2.20.1.7.g153144c
Re: [PATCH v4 7/9] drm: vkms: Refactor the plane composer to accept new formats
Hi Melissa, On 2/8/22 07:40, Melissa Wen wrote: On 01/21, Igor Torrente wrote: Currently the blend function only accepts XRGB_ and ARGB_ as a color input. This patch refactors all the functions related to the plane composition to overcome this limitation. A new internal format(`struct pixel`) is introduced to deal with all possible inputs. It consists of 16 bits fields that represent each of the channels. The pixels blend is done using this internal format. And new handlers are being added to convert a specific format to/from this internal format. So the blend operation depends on these handlers to convert to this common format. The blended result, if necessary, is converted to the writeback buffer format. This patch introduces three major differences to the blend function. 1 - All the planes are blended at once. 2 - The blend calculus is done as per line instead of per pixel. 3 - It is responsible to calculates the CRC and writing the writeback buffer(if necessary). These changes allow us to allocate way less memory in the intermediate buffer to compute these operations. Because now we don't need to have the entire intermediate image lines at once, just one line is enough. | Memory consumption (output dimensions) | |:--:| | Current | This patch| |:--:|:-:| | Width * Heigth | 2 * Width | Beyond memory, we also have a minor performance benefit from all these changes. Results running the IGT tests `*kms_cursor_crc*`: First, thanks for this improvement. Some recent changes in kms_cursor_crc caused VKMS to fail in most test cases (iirc, only size-change and alpha-opaque are passing currently). I updated my igt and kernel(from drm_misc/drm-misc-next) to the latest commit[1][2] and I'm getting mixed results. Sometimes most of the test passes, sometimes almost nothing passes. [1] a96674e7 (tests/api_intel_bb: Handle different alignments in delta-check) [2] b21a142fd205 (drm/nouveau/backlight: Just set all backlight types as RAW) But saying that performance improvement here would cause a misunderstanding when reviewing the change history. Can you update this statistics here? I think you can specify the IGT hash to specify the test case version or you can pick another test for comparison. OK, I will do both. | Frametime | |:--:| | Implementation | Current | This commit | |:---:|:-:|::| | frametime range | 8~22 ms |5~18 ms | | Average | 10.0 ms |7.3 ms| Reported-by: kernel test robot A little confusing for me to have this reported-by tag without any explanation of what was reported and fixed. Can you specify it? Signed-off-by: Igor Torrente --- V2: Improves the performance drastically, by perfoming the operations per-line and not per-pixel(Pekka Paalanen). Minor improvements(Pekka Paalanen). V3: Changes the code to blend the planes all at once. This improves performance, memory consumption, and removes much of the weirdness of the V2(Pekka Paalanen and me). Minor improvements(Pekka Paalanen and me). V4: Rebase the code and adapt it to the new NUM_OVERLAY_PLANES constant. Can you move version changes up so that they are not ignored? I also pointed out minor code style issue below. With these comments addressed, you can add my r-b tag in the next version. --- drivers/gpu/drm/vkms/Makefile| 1 + drivers/gpu/drm/vkms/vkms_composer.c | 335 +-- drivers/gpu/drm/vkms/vkms_formats.c | 138 +++ drivers/gpu/drm/vkms/vkms_formats.h | 31 +++ 4 files changed, 333 insertions(+), 172 deletions(-) create mode 100644 drivers/gpu/drm/vkms/vkms_formats.c create mode 100644 drivers/gpu/drm/vkms/vkms_formats.h diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile index 72f779cbfedd..1b28a6a32948 100644 --- a/drivers/gpu/drm/vkms/Makefile +++ b/drivers/gpu/drm/vkms/Makefile @@ -3,6 +3,7 @@ vkms-y := \ vkms_drv.o \ vkms_plane.o \ vkms_output.o \ + vkms_formats.o \ vkms_crtc.o \ vkms_composer.o \ vkms_writeback.o diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 95029d2ebcac..9f70fcf84fb9 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -9,202 +9,210 @@ #include #include "vkms_drv.h" +#include "vkms_formats.h" -static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer, -const struct vkms_frame_info *frame_info) +static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha) { - u32 pixel; - int src_offset = frame_info->offset + (y * frame_info->pitch) - + (x * frame_info->cpp); + u32 new_color; - pixel
linux-next: manual merge of the drm-intel tree with the drm tree
Hi all, Today's linux-next merge of the drm-intel tree got a conflict in: include/linux/dma-buf-map.h between commit: e8c1f36157ce ("dma-buf-map: Fix dot vs comma in example") from the drm tree and commit: 7938f4218168 ("dma-buf-map: Rename to iosys-map") from the drm-intel tree. I fixed it up (I just removed the file - the changes from the former commit are only on comments) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts. -- Cheers, Stephen Rothwell pgpLKbHiNUlgs.pgp Description: OpenPGP digital signature
RE: [RFC v4 04/11] drm/amd/virt: For SRIOV send GPU reset directly to TDR queue.
[AMD Official Use Only] This patch is reviewed by Shaoyun.liu Since other patches are suggested by other engineer and they may already od some review on them , so I will leave them to continue review the rest patches. Regards Shaoyun.liu -Original Message- From: Grodzovsky, Andrey Sent: Tuesday, February 8, 2022 7:23 PM To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org Cc: Koenig, Christian ; dan...@ffwll.ch; Liu, Monk ; Chen, Horace ; Lazar, Lijo ; Chen, JingWen ; Grodzovsky, Andrey ; Liu, Shaoyun Subject: [RFC v4 04/11] drm/amd/virt: For SRIOV send GPU reset directly to TDR queue. No need to to trigger another work queue inside the work queue. v3: Problem: Extra reset caused by host side FLR notification following guest side triggered reset. Fix: Preven qeuing flr_work from mailbox irq if guest already executing a reset. Suggested-by: Liu Shaoyun Signed-off-by: Andrey Grodzovsky --- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 9 ++--- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 9 ++--- drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 9 ++--- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c index 56da5ab82987..5869d51d8bee 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c @@ -282,7 +282,7 @@ static void xgpu_ai_mailbox_flr_work(struct work_struct *work) if (amdgpu_device_should_recover_gpu(adev) && (!amdgpu_device_has_job_running(adev) || adev->sdma_timeout == MAX_SCHEDULE_TIMEOUT)) - amdgpu_device_gpu_recover(adev, NULL); + amdgpu_device_gpu_recover_imp(adev, NULL); } static int xgpu_ai_set_mailbox_rcv_irq(struct amdgpu_device *adev, @@ -307,8 +307,11 @@ static int xgpu_ai_mailbox_rcv_irq(struct amdgpu_device *adev, switch (event) { case IDH_FLR_NOTIFICATION: - if (amdgpu_sriov_runtime(adev)) - schedule_work(>virt.flr_work); + if (amdgpu_sriov_runtime(adev) && !amdgpu_in_reset(adev)) + WARN_ONCE(!queue_work(adev->reset_domain.wq, + >virt.flr_work), + "Failed to queue work! at %s", + __func__); break; case IDH_QUERY_ALIVE: xgpu_ai_mailbox_send_ack(adev); diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c index 477d0dde19c5..5728a6401d73 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c @@ -309,7 +309,7 @@ static void xgpu_nv_mailbox_flr_work(struct work_struct *work) adev->gfx_timeout == MAX_SCHEDULE_TIMEOUT || adev->compute_timeout == MAX_SCHEDULE_TIMEOUT || adev->video_timeout == MAX_SCHEDULE_TIMEOUT)) - amdgpu_device_gpu_recover(adev, NULL); + amdgpu_device_gpu_recover_imp(adev, NULL); } static int xgpu_nv_set_mailbox_rcv_irq(struct amdgpu_device *adev, @@ -337,8 +337,11 @@ static int xgpu_nv_mailbox_rcv_irq(struct amdgpu_device *adev, switch (event) { case IDH_FLR_NOTIFICATION: - if (amdgpu_sriov_runtime(adev)) - schedule_work(>virt.flr_work); + if (amdgpu_sriov_runtime(adev) && !amdgpu_in_reset(adev)) + WARN_ONCE(!queue_work(adev->reset_domain.wq, + >virt.flr_work), + "Failed to queue work! at %s", + __func__); break; /* READY_TO_ACCESS_GPU is fetched by kernel polling, IRQ can ignore * it byfar since that polling thread will handle it, diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c index aef9d059ae52..02290febfcf4 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c @@ -521,7 +521,7 @@ static void xgpu_vi_mailbox_flr_work(struct work_struct *work) /* Trigger recovery due to world switch failure */ if (amdgpu_device_should_recover_gpu(adev)) - amdgpu_device_gpu_recover(adev, NULL); + amdgpu_device_gpu_recover_imp(adev, NULL); } static int xgpu_vi_set_mailbox_rcv_irq(struct amdgpu_device *adev, @@ -550,8 +550,11 @@ static int xgpu_vi_mailbox_rcv_irq(struct amdgpu_device *adev, r = xgpu_vi_mailbox_rcv_msg(adev, IDH_FLR_NOTIFICATION); /* only handle FLR_NOTIFY now */ - if (!r) - schedule_work(>virt.flr_work); + if (!r && !amdgpu_in_reset(adev)) + WARN_ONCE(!queue_work(adev->reset_domain.wq, +
[RFC v4 08/11] drm/amdgpu: Move reset sem into reset_domain
We want single instance of reset sem across all reset clients because in case of XGMI we should stop access cross device MMIO because any of them could be in a reset in the moment. Signed-off-by: Andrey Grodzovsky --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 23 +-- .../gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c| 18 --- drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h | 1 + drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c| 6 +++-- drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 14 ++- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 4 ++-- 10 files changed, 46 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index cb9764513df8..ddfbcc8fd3d3 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1058,7 +1058,6 @@ struct amdgpu_device { atomic_tin_gpu_reset; enum pp_mp1_state mp1_state; - struct rw_semaphore reset_sem; struct amdgpu_doorbell_index doorbell_index; struct mutexnotifier_lock; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c index 25e2e5bf90eb..c3728061d65a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c @@ -37,6 +37,8 @@ #include "amdgpu_fw_attestation.h" #include "amdgpu_umr.h" +#include "amdgpu_reset.h" + #if defined(CONFIG_DEBUG_FS) /** @@ -1279,7 +1281,7 @@ static int amdgpu_debugfs_test_ib_show(struct seq_file *m, void *unused) } /* Avoid accidently unparking the sched thread during GPU reset */ - r = down_write_killable(>reset_sem); + r = down_write_killable(>reset_domain->sem); if (r) return r; @@ -1308,7 +1310,7 @@ static int amdgpu_debugfs_test_ib_show(struct seq_file *m, void *unused) kthread_unpark(ring->sched.thread); } - up_write(>reset_sem); + up_write(>reset_domain->sem); pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev); @@ -1517,7 +1519,7 @@ static int amdgpu_debugfs_ib_preempt(void *data, u64 val) return -ENOMEM; /* Avoid accidently unparking the sched thread during GPU reset */ - r = down_read_killable(>reset_sem); + r = down_read_killable(>reset_domain->sem); if (r) goto pro_end; @@ -1560,7 +1562,7 @@ static int amdgpu_debugfs_ib_preempt(void *data, u64 val) /* restart the scheduler */ kthread_unpark(ring->sched.thread); - up_read(>reset_sem); + up_read(>reset_domain->sem); ttm_bo_unlock_delayed_workqueue(>mman.bdev, resched); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index d61bc0a0457c..dcbb175d336f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -424,10 +424,10 @@ bool amdgpu_device_skip_hw_access(struct amdgpu_device *adev) * the lock. */ if (in_task()) { - if (down_read_trylock(>reset_sem)) - up_read(>reset_sem); + if (down_read_trylock(>reset_domain->sem)) + up_read(>reset_domain->sem); else - lockdep_assert_held(>reset_sem); + lockdep_assert_held(>reset_domain->sem); } #endif return false; @@ -453,9 +453,9 @@ uint32_t amdgpu_device_rreg(struct amdgpu_device *adev, if ((reg * 4) < adev->rmmio_size) { if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) && - down_read_trylock(>reset_sem)) { + down_read_trylock(>reset_domain->sem)) { ret = amdgpu_kiq_rreg(adev, reg); - up_read(>reset_sem); + up_read(>reset_domain->sem); } else { ret = readl(((void __iomem *)adev->rmmio) + (reg * 4)); } @@ -538,9 +538,9 @@ void amdgpu_device_wreg(struct amdgpu_device *adev, if ((reg * 4) < adev->rmmio_size) { if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev) && - down_read_trylock(>reset_sem)) { + down_read_trylock(>reset_domain->sem)) { amdgpu_kiq_wreg(adev, reg, v); - up_read(>reset_sem); + up_read(>reset_domain->sem); } else { writel(v, ((void
[RFC v4 06/11] drm/amdgpu: Drop concurrent GPU reset protection for device
Since now all GPU resets are serialzied there is no need for this. This patch also reverts 'drm/amdgpu: race issue when jobs on 2 ring timeout' Signed-off-by: Andrey Grodzovsky Reviewed-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 89 ++ 1 file changed, 7 insertions(+), 82 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 7e92f2432087..e3c0ec684a85 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -4817,11 +4817,10 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, return r; } -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, +static void amdgpu_device_lock_adev(struct amdgpu_device *adev, struct amdgpu_hive_info *hive) { - if (atomic_cmpxchg(>in_gpu_reset, 0, 1) != 0) - return false; + atomic_set(>in_gpu_reset, 1); if (hive) { down_write_nest_lock(>reset_sem, >hive_lock); @@ -4840,8 +4839,6 @@ static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, adev->mp1_state = PP_MP1_STATE_NONE; break; } - - return true; } static void amdgpu_device_unlock_adev(struct amdgpu_device *adev) @@ -4852,46 +4849,6 @@ static void amdgpu_device_unlock_adev(struct amdgpu_device *adev) up_write(>reset_sem); } -/* - * to lockup a list of amdgpu devices in a hive safely, if not a hive - * with multiple nodes, it will be similar as amdgpu_device_lock_adev. - * - * unlock won't require roll back. - */ -static int amdgpu_device_lock_hive_adev(struct amdgpu_device *adev, struct amdgpu_hive_info *hive) -{ - struct amdgpu_device *tmp_adev = NULL; - - if (!amdgpu_sriov_vf(adev) && (adev->gmc.xgmi.num_physical_nodes > 1)) { - if (!hive) { - dev_err(adev->dev, "Hive is NULL while device has multiple xgmi nodes"); - return -ENODEV; - } - list_for_each_entry(tmp_adev, >device_list, gmc.xgmi.head) { - if (!amdgpu_device_lock_adev(tmp_adev, hive)) - goto roll_back; - } - } else if (!amdgpu_device_lock_adev(adev, hive)) - return -EAGAIN; - - return 0; -roll_back: - if (!list_is_first(_adev->gmc.xgmi.head, >device_list)) { - /* -* if the lockup iteration break in the middle of a hive, -* it may means there may has a race issue, -* or a hive device locked up independently. -* we may be in trouble and may not, so will try to roll back -* the lock and give out a warnning. -*/ - dev_warn(tmp_adev->dev, "Hive lock iteration broke in the middle. Rolling back to unlock"); - list_for_each_entry_continue_reverse(tmp_adev, >device_list, gmc.xgmi.head) { - amdgpu_device_unlock_adev(tmp_adev); - } - } - return -EAGAIN; -} - static void amdgpu_device_resume_display_audio(struct amdgpu_device *adev) { struct pci_dev *p = NULL; @@ -5078,22 +5035,6 @@ int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, reset_context.hive = hive; clear_bit(AMDGPU_NEED_FULL_RESET, _context.flags); - /* -* lock the device before we try to operate the linked list -* if didn't get the device lock, don't touch the linked list since -* others may iterating it. -*/ - r = amdgpu_device_lock_hive_adev(adev, hive); - if (r) { - dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress", - job ? job->base.id : -1); - - /* even we skipped this reset, still need to set the job to guilty */ - if (job && job->vm) - drm_sched_increase_karma(>base); - goto skip_recovery; - } - /* * Build list of devices to reset. * In case we are in XGMI hive mode, resort the device list @@ -5113,6 +5054,9 @@ int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, /* block all schedulers and reset given job's ring */ list_for_each_entry(tmp_adev, device_list_handle, reset_list) { + + amdgpu_device_lock_adev(tmp_adev, hive); + /* * Try to put the audio codec into suspend state * before gpu reset started. @@ -5264,13 +5208,12 @@ int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, amdgpu_device_unlock_adev(tmp_adev); } -skip_recovery: if (hive) { mutex_unlock(>hive_lock); amdgpu_put_xgmi_hive(hive); } - if (r && r != -EAGAIN)
[RFC v4 07/11] drm/amdgpu: Rework reset domain to be refcounted.
The reset domain contains register access semaphor now and so needs to be present as long as each device in a hive needs it and so it cannot be binded to XGMI hive life cycle. Adress this by making reset domain refcounted and pointed by each member of the hive and the hive itself. v4: Fix crash on boot witrh XGMI hive by adding type to reset_domain. XGMI will only create a new reset_domain if prevoius was of single device type meaning it's first boot. Otherwsie it will take a refocunt to exsiting reset_domain from the amdgou device. Add a wrapper around reset_domain->refcount get/put and a wrapper around send to reset wq (Lijo) Signed-off-by: Andrey Grodzovsky --- drivers/gpu/drm/amd/amdgpu/amdgpu.h| 6 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 44 +- drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 40 drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h | 35 + drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 29 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 2 +- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 6 ++- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 6 ++- drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 6 ++- 9 files changed, 140 insertions(+), 34 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 540a38fe5cd6..cb9764513df8 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -813,9 +813,7 @@ struct amd_powerplay { #define AMDGPU_RESET_MAGIC_NUM 64 #define AMDGPU_MAX_DF_PERFMONS 4 #define AMDGPU_PRODUCT_NAME_LEN 64 -struct amdgpu_reset_domain { - struct workqueue_struct *wq; -}; +struct amdgpu_reset_domain; struct amdgpu_device { struct device *dev; @@ -1104,7 +1102,7 @@ struct amdgpu_device { uint32_t ip_versions[MAX_HWIP][HWIP_MAX_INSTANCE]; boolram_is_direct_mapped; - struct amdgpu_reset_domain reset_domain; + struct amdgpu_reset_domain *reset_domain; }; static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index e3c0ec684a85..d61bc0a0457c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -2316,7 +2316,7 @@ static int amdgpu_device_init_schedulers(struct amdgpu_device *adev) r = drm_sched_init(>sched, _sched_ops, ring->num_hw_submission, amdgpu_job_hang_limit, - timeout, adev->reset_domain.wq, ring->sched_score, ring->name); + timeout, adev->reset_domain->wq, ring->sched_score, ring->name); if (r) { DRM_ERROR("Failed to create scheduler on ring %s.\n", ring->name); @@ -2439,24 +2439,22 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev) if (r) goto init_failed; + /** +* In case of XGMI grab extra reference for reset domain for this device +*/ if (adev->gmc.xgmi.num_physical_nodes > 1) { - struct amdgpu_hive_info *hive; + if (amdgpu_xgmi_add_device(adev) == 0) { + struct amdgpu_hive_info *hive = amdgpu_get_xgmi_hive(adev); - amdgpu_xgmi_add_device(adev); + if (!hive->reset_domain || + !amdgpu_reset_get_reset_domain(hive->reset_domain)) { + r = -ENOENT; + goto init_failed; + } - hive = amdgpu_get_xgmi_hive(adev); - if (!hive || !hive->reset_domain.wq) { - DRM_ERROR("Failed to obtain reset domain info for XGMI hive:%llx", hive->hive_id); - r = -EINVAL; - goto init_failed; - } - - adev->reset_domain.wq = hive->reset_domain.wq; - } else { - adev->reset_domain.wq = alloc_ordered_workqueue("amdgpu-reset-dev", 0); - if (!adev->reset_domain.wq) { - r = -ENOMEM; - goto init_failed; + /* Drop the early temporary reset domain we created for device */ + amdgpu_reset_put_reset_domain(adev->reset_domain); + adev->reset_domain = hive->reset_domain; } } @@ -3640,6 +3638,15 @@ int amdgpu_device_init(struct amdgpu_device *adev, return r; } + /* +* Reset domain needs to be present early, before XGMI hive discovered +* (if any) and intitialized to use reset sem and in_gpu reset flag +* early on during init. +*/ +
[RFC v4 10/11] drm/amdgpu: Rework amdgpu_device_lock_adev
This functions needs to be split into 2 parts where one is called only once for locking single instance of reset_domain's sem and reset flag and the other part which handles MP1 states should still be called for each device in XGMI hive. Signed-off-by: Andrey Grodzovsky --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 48 -- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index e05d7cbefd2c..aaecf0797484 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -4825,16 +4825,20 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, return r; } -static void amdgpu_device_lock_adev(struct amdgpu_device *adev, - struct amdgpu_hive_info *hive) +static void amdgpu_device_lock_reset_domain(struct amdgpu_reset_domain *reset_domain, + struct amdgpu_hive_info *hive) { - atomic_set(>reset_domain->in_gpu_reset, 1); + atomic_set(_domain->in_gpu_reset, 1); if (hive) { - down_write_nest_lock(>reset_domain->sem, >hive_lock); + down_write_nest_lock(_domain->sem, >hive_lock); } else { - down_write(>reset_domain->sem); + down_write(_domain->sem); } +} + +static void amdgpu_device_set_mp1_state(struct amdgpu_device *adev) +{ switch (amdgpu_asic_reset_method(adev)) { case AMD_RESET_METHOD_MODE1: @@ -4849,14 +4853,19 @@ static void amdgpu_device_lock_adev(struct amdgpu_device *adev, } } -static void amdgpu_device_unlock_adev(struct amdgpu_device *adev) +static void amdgpu_device_unset_mp1_state(struct amdgpu_device *adev) { amdgpu_vf_error_trans_all(adev); adev->mp1_state = PP_MP1_STATE_NONE; - atomic_set(>reset_domain->in_gpu_reset, 0); - up_write(>reset_domain->sem); } +static void amdgpu_device_unlock_reset_domain(struct amdgpu_reset_domain *reset_domain) +{ + atomic_set(_domain->in_gpu_reset, 0); + up_write(_domain->sem); +} + + static void amdgpu_device_resume_display_audio(struct amdgpu_device *adev) { struct pci_dev *p = NULL; @@ -5060,10 +5069,15 @@ int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, device_list_handle = _list; } + /* We need to lock reset domain only once both for XGMI and single device */ + tmp_adev = list_first_entry(device_list_handle, struct amdgpu_device, + reset_list); + amdgpu_device_lock_reset_domain(tmp_adev->reset_domain, hive); + /* block all schedulers and reset given job's ring */ list_for_each_entry(tmp_adev, device_list_handle, reset_list) { - amdgpu_device_lock_adev(tmp_adev, hive); + amdgpu_device_set_mp1_state(tmp_adev); /* * Try to put the audio codec into suspend state @@ -5213,9 +5227,14 @@ int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, if (audio_suspended) amdgpu_device_resume_display_audio(tmp_adev); - amdgpu_device_unlock_adev(tmp_adev); + + amdgpu_device_unset_mp1_state(tmp_adev); } + tmp_adev = list_first_entry(device_list_handle, struct amdgpu_device, + reset_list); + amdgpu_device_unlock_reset_domain(tmp_adev->reset_domain); + if (hive) { mutex_unlock(>hive_lock); amdgpu_put_xgmi_hive(hive); @@ -5477,7 +5496,8 @@ pci_ers_result_t amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta * Locking adev->reset_domain->sem will prevent any external access * to GPU during PCI error recovery */ - amdgpu_device_lock_adev(adev, NULL); + amdgpu_device_lock_reset_domain(adev->reset_domain, NULL); + amdgpu_device_set_mp1_state(adev); /* * Block any work scheduling as we do for regular GPU reset @@ -5584,7 +5604,8 @@ pci_ers_result_t amdgpu_pci_slot_reset(struct pci_dev *pdev) DRM_INFO("PCIe error recovery succeeded\n"); } else { DRM_ERROR("PCIe error recovery failed, err:%d", r); - amdgpu_device_unlock_adev(adev); + amdgpu_device_unset_mp1_state(adev); + amdgpu_device_unlock_reset_domain(adev->reset_domain); } return r ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED; @@ -5621,7 +5642,8 @@ void amdgpu_pci_resume(struct pci_dev *pdev) drm_sched_start(>sched, true); } - amdgpu_device_unlock_adev(adev); + amdgpu_device_unset_mp1_state(adev); + amdgpu_device_unlock_reset_domain(adev->reset_domain); }
[RFC v4 11/11] Revert 'drm/amdgpu: annotate a false positive recursive locking'
Since we have a single instance of reset semaphore which we lock only once even for XGMI hive we don't need the nested locking hint anymore. Signed-off-by: Andrey Grodzovsky --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 14 -- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index aaecf0797484..75d0dd289023 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -4825,16 +4825,10 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, return r; } -static void amdgpu_device_lock_reset_domain(struct amdgpu_reset_domain *reset_domain, - struct amdgpu_hive_info *hive) +static void amdgpu_device_lock_reset_domain(struct amdgpu_reset_domain *reset_domain) { atomic_set(_domain->in_gpu_reset, 1); - - if (hive) { - down_write_nest_lock(_domain->sem, >hive_lock); - } else { - down_write(_domain->sem); - } + down_write(_domain->sem); } static void amdgpu_device_set_mp1_state(struct amdgpu_device *adev) @@ -5072,7 +5066,7 @@ int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, /* We need to lock reset domain only once both for XGMI and single device */ tmp_adev = list_first_entry(device_list_handle, struct amdgpu_device, reset_list); - amdgpu_device_lock_reset_domain(tmp_adev->reset_domain, hive); + amdgpu_device_lock_reset_domain(tmp_adev->reset_domain); /* block all schedulers and reset given job's ring */ list_for_each_entry(tmp_adev, device_list_handle, reset_list) { @@ -5496,7 +5490,7 @@ pci_ers_result_t amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta * Locking adev->reset_domain->sem will prevent any external access * to GPU during PCI error recovery */ - amdgpu_device_lock_reset_domain(adev->reset_domain, NULL); + amdgpu_device_lock_reset_domain(adev->reset_domain); amdgpu_device_set_mp1_state(adev); /* -- 2.25.1
[RFC v4 09/11] drm/amdgpu: Move in_gpu_reset into reset_domain
We should have a single instance per entrire reset domain. Signed-off-by: Andrey Grodzovsky Suggested-by: Lijo Lazar --- drivers/gpu/drm/amd/amdgpu/amdgpu.h| 7 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h | 1 + drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 4 ++-- 6 files changed, 15 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index ddfbcc8fd3d3..b89406b01694 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1056,7 +1056,6 @@ struct amdgpu_device { boolin_s4; boolin_s0ix; - atomic_tin_gpu_reset; enum pp_mp1_state mp1_state; struct amdgpu_doorbell_index doorbell_index; @@ -1463,8 +1462,6 @@ static inline bool amdgpu_is_tmz(struct amdgpu_device *adev) return adev->gmc.tmz_enabled; } -static inline int amdgpu_in_reset(struct amdgpu_device *adev) -{ - return atomic_read(>in_gpu_reset); -} +int amdgpu_in_reset(struct amdgpu_device *adev); + #endif diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index dcbb175d336f..e05d7cbefd2c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -3554,7 +3554,6 @@ int amdgpu_device_init(struct amdgpu_device *adev, mutex_init(>mn_lock); mutex_init(>virt.vf_errors.lock); hash_init(adev->mn_hash); - atomic_set(>in_gpu_reset, 0); mutex_init(>psp.mutex); mutex_init(>notifier_lock); @@ -4829,7 +4828,7 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, static void amdgpu_device_lock_adev(struct amdgpu_device *adev, struct amdgpu_hive_info *hive) { - atomic_set(>in_gpu_reset, 1); + atomic_set(>reset_domain->in_gpu_reset, 1); if (hive) { down_write_nest_lock(>reset_domain->sem, >hive_lock); @@ -4854,7 +4853,7 @@ static void amdgpu_device_unlock_adev(struct amdgpu_device *adev) { amdgpu_vf_error_trans_all(adev); adev->mp1_state = PP_MP1_STATE_NONE; - atomic_set(>in_gpu_reset, 0); + atomic_set(>reset_domain->in_gpu_reset, 0); up_write(>reset_domain->sem); } @@ -5699,6 +5698,11 @@ void amdgpu_device_invalidate_hdp(struct amdgpu_device *adev, amdgpu_asic_invalidate_hdp(adev, ring); } +int amdgpu_in_reset(struct amdgpu_device *adev) +{ + return atomic_read(>reset_domain->in_gpu_reset); + } + /** * amdgpu_device_halt() - bring hardware to some kind of halt state * diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c index c0988c804459..5ab72c3bfbda 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c @@ -131,6 +131,7 @@ struct amdgpu_reset_domain *amdgpu_reset_create_reset_domain(enum amdgpu_reset_d } + atomic_set(_domain->in_gpu_reset, 0); init_rwsem(_domain->sem); return reset_domain; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h index 80f918e87d4f..ea6fc98ea927 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h @@ -81,6 +81,7 @@ struct amdgpu_reset_domain { struct workqueue_struct *wq; enum amdgpu_reset_domain_type type; struct rw_semaphore sem; + atomic_t in_gpu_reset; }; diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c index 4e23c29e665c..b81acf59870c 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c @@ -259,7 +259,7 @@ static void xgpu_ai_mailbox_flr_work(struct work_struct *work) * otherwise the mailbox msg will be ruined/reseted by * the VF FLR. */ - if (atomic_cmpxchg(>in_gpu_reset, 0, 1) != 0) + if (atomic_cmpxchg(>reset_domain->in_gpu_reset, 0, 1) != 0) return; down_write(>reset_domain->sem); @@ -277,7 +277,7 @@ static void xgpu_ai_mailbox_flr_work(struct work_struct *work) } while (timeout > 1); flr_done: - atomic_set(>in_gpu_reset, 0); + atomic_set(>reset_domain->in_gpu_reset, 0); up_write(>reset_domain->sem); /* Trigger recovery for world switch failure if no TDR */ diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c index f715780f7d20..22c10b97ea81 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c @@ -283,7 +283,7 @@ static void xgpu_nv_mailbox_flr_work(struct
[RFC v4 05/11] drm/amdgpu: Drop hive->in_reset
Since we serialize all resets no need to protect from concurrent resets. Signed-off-by: Andrey Grodzovsky Reviewed-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 19 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 1 - 3 files changed, 1 insertion(+), 20 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 15e8fde3ac2d..7e92f2432087 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -5067,26 +5067,10 @@ int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, dev_info(adev->dev, "GPU %s begin!\n", need_emergency_restart ? "jobs stop":"reset"); - /* -* Here we trylock to avoid chain of resets executing from -* either trigger by jobs on different adevs in XGMI hive or jobs on -* different schedulers for same device while this TO handler is running. -* We always reset all schedulers for device and all devices for XGMI -* hive so that should take care of them too. -*/ if (!amdgpu_sriov_vf(adev)) hive = amdgpu_get_xgmi_hive(adev); - if (hive) { - if (atomic_cmpxchg(>in_reset, 0, 1) != 0) { - DRM_INFO("Bailing on TDR for s_job:%llx, hive: %llx as another already in progress", - job ? job->base.id : -1, hive->hive_id); - amdgpu_put_xgmi_hive(hive); - if (job && job->vm) - drm_sched_increase_karma(>base); - return 0; - } + if (hive) mutex_lock(>hive_lock); - } reset_context.method = AMD_RESET_METHOD_NONE; reset_context.reset_req_dev = adev; @@ -5282,7 +5266,6 @@ int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, skip_recovery: if (hive) { - atomic_set(>in_reset, 0); mutex_unlock(>hive_lock); amdgpu_put_xgmi_hive(hive); } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c index d406897346d6..89b682afe821 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c @@ -410,7 +410,6 @@ struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev) INIT_LIST_HEAD(>device_list); INIT_LIST_HEAD(>node); mutex_init(>hive_lock); - atomic_set(>in_reset, 0); atomic_set(>number_devices, 0); task_barrier_init(>tb); hive->pstate = AMDGPU_XGMI_PSTATE_UNKNOWN; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h index 6121aaa292cb..2f2ce53645a5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h @@ -33,7 +33,6 @@ struct amdgpu_hive_info { struct list_head node; atomic_t number_devices; struct mutex hive_lock; - atomic_t in_reset; int hi_req_count; struct amdgpu_device *hi_req_gpu; struct task_barrier tb; -- 2.25.1
[RFC v4 04/11] drm/amd/virt: For SRIOV send GPU reset directly to TDR queue.
No need to to trigger another work queue inside the work queue. v3: Problem: Extra reset caused by host side FLR notification following guest side triggered reset. Fix: Preven qeuing flr_work from mailbox irq if guest already executing a reset. Suggested-by: Liu Shaoyun Signed-off-by: Andrey Grodzovsky --- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 9 ++--- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 9 ++--- drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 9 ++--- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c index 56da5ab82987..5869d51d8bee 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c @@ -282,7 +282,7 @@ static void xgpu_ai_mailbox_flr_work(struct work_struct *work) if (amdgpu_device_should_recover_gpu(adev) && (!amdgpu_device_has_job_running(adev) || adev->sdma_timeout == MAX_SCHEDULE_TIMEOUT)) - amdgpu_device_gpu_recover(adev, NULL); + amdgpu_device_gpu_recover_imp(adev, NULL); } static int xgpu_ai_set_mailbox_rcv_irq(struct amdgpu_device *adev, @@ -307,8 +307,11 @@ static int xgpu_ai_mailbox_rcv_irq(struct amdgpu_device *adev, switch (event) { case IDH_FLR_NOTIFICATION: - if (amdgpu_sriov_runtime(adev)) - schedule_work(>virt.flr_work); + if (amdgpu_sriov_runtime(adev) && !amdgpu_in_reset(adev)) + WARN_ONCE(!queue_work(adev->reset_domain.wq, + >virt.flr_work), + "Failed to queue work! at %s", + __func__); break; case IDH_QUERY_ALIVE: xgpu_ai_mailbox_send_ack(adev); diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c index 477d0dde19c5..5728a6401d73 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c @@ -309,7 +309,7 @@ static void xgpu_nv_mailbox_flr_work(struct work_struct *work) adev->gfx_timeout == MAX_SCHEDULE_TIMEOUT || adev->compute_timeout == MAX_SCHEDULE_TIMEOUT || adev->video_timeout == MAX_SCHEDULE_TIMEOUT)) - amdgpu_device_gpu_recover(adev, NULL); + amdgpu_device_gpu_recover_imp(adev, NULL); } static int xgpu_nv_set_mailbox_rcv_irq(struct amdgpu_device *adev, @@ -337,8 +337,11 @@ static int xgpu_nv_mailbox_rcv_irq(struct amdgpu_device *adev, switch (event) { case IDH_FLR_NOTIFICATION: - if (amdgpu_sriov_runtime(adev)) - schedule_work(>virt.flr_work); + if (amdgpu_sriov_runtime(adev) && !amdgpu_in_reset(adev)) + WARN_ONCE(!queue_work(adev->reset_domain.wq, + >virt.flr_work), + "Failed to queue work! at %s", + __func__); break; /* READY_TO_ACCESS_GPU is fetched by kernel polling, IRQ can ignore * it byfar since that polling thread will handle it, diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c index aef9d059ae52..02290febfcf4 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c @@ -521,7 +521,7 @@ static void xgpu_vi_mailbox_flr_work(struct work_struct *work) /* Trigger recovery due to world switch failure */ if (amdgpu_device_should_recover_gpu(adev)) - amdgpu_device_gpu_recover(adev, NULL); + amdgpu_device_gpu_recover_imp(adev, NULL); } static int xgpu_vi_set_mailbox_rcv_irq(struct amdgpu_device *adev, @@ -550,8 +550,11 @@ static int xgpu_vi_mailbox_rcv_irq(struct amdgpu_device *adev, r = xgpu_vi_mailbox_rcv_msg(adev, IDH_FLR_NOTIFICATION); /* only handle FLR_NOTIFY now */ - if (!r) - schedule_work(>virt.flr_work); + if (!r && !amdgpu_in_reset(adev)) + WARN_ONCE(!queue_work(adev->reset_domain.wq, + >virt.flr_work), + "Failed to queue work! at %s", + __func__); } return 0; -- 2.25.1
[RFC v4 02/11] drm/amdgpu: Move scheduler init to after XGMI is ready
Before we initialize schedulers we must know which reset domain are we in - for single device there iis a single domain per device and so single wq per device. For XGMI the reset domain spans the entire XGMI hive and so the reset wq is per hive. Signed-off-by: Andrey Grodzovsky --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 45 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 34 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 + 3 files changed, 51 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 9704b0e1fd82..00123b0013d3 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -2287,6 +2287,47 @@ static int amdgpu_device_fw_loading(struct amdgpu_device *adev) return r; } +static int amdgpu_device_init_schedulers(struct amdgpu_device *adev) +{ + long timeout; + int r, i; + + for (i = 0; i < AMDGPU_MAX_RINGS; ++i) { + struct amdgpu_ring *ring = adev->rings[i]; + + /* No need to setup the GPU scheduler for rings that don't need it */ + if (!ring || ring->no_scheduler) + continue; + + switch (ring->funcs->type) { + case AMDGPU_RING_TYPE_GFX: + timeout = adev->gfx_timeout; + break; + case AMDGPU_RING_TYPE_COMPUTE: + timeout = adev->compute_timeout; + break; + case AMDGPU_RING_TYPE_SDMA: + timeout = adev->sdma_timeout; + break; + default: + timeout = adev->video_timeout; + break; + } + + r = drm_sched_init(>sched, _sched_ops, + ring->num_hw_submission, amdgpu_job_hang_limit, + timeout, adev->reset_domain.wq, ring->sched_score, ring->name); + if (r) { + DRM_ERROR("Failed to create scheduler on ring %s.\n", + ring->name); + return r; + } + } + + return 0; +} + + /** * amdgpu_device_ip_init - run init for hardware IPs * @@ -2419,6 +2460,10 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev) } } + r = amdgpu_device_init_schedulers(adev); + if (r) + goto init_failed; + /* Don't init kfd if whole hive need to be reset during init */ if (!adev->gmc.xgmi.pending_reset) amdgpu_amdkfd_device_init(adev); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index 45977a72b5dd..fa302540c69a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -457,8 +457,6 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring, atomic_t *sched_score) { struct amdgpu_device *adev = ring->adev; - long timeout; - int r; if (!adev) return -EINVAL; @@ -478,36 +476,12 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring, spin_lock_init(>fence_drv.lock); ring->fence_drv.fences = kcalloc(num_hw_submission * 2, sizeof(void *), GFP_KERNEL); - if (!ring->fence_drv.fences) - return -ENOMEM; - /* No need to setup the GPU scheduler for rings that don't need it */ - if (ring->no_scheduler) - return 0; + ring->num_hw_submission = num_hw_submission; + ring->sched_score = sched_score; - switch (ring->funcs->type) { - case AMDGPU_RING_TYPE_GFX: - timeout = adev->gfx_timeout; - break; - case AMDGPU_RING_TYPE_COMPUTE: - timeout = adev->compute_timeout; - break; - case AMDGPU_RING_TYPE_SDMA: - timeout = adev->sdma_timeout; - break; - default: - timeout = adev->video_timeout; - break; - } - - r = drm_sched_init(>sched, _sched_ops, - num_hw_submission, amdgpu_job_hang_limit, - timeout, NULL, sched_score, ring->name); - if (r) { - DRM_ERROR("Failed to create scheduler on ring %s.\n", - ring->name); - return r; - } + if (!ring->fence_drv.fences) + return -ENOMEM; return 0; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h index fae7d185ad0d..7f20ce73a243 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h @@ -251,6 +251,8 @@ struct
[RFC v4 03/11] drm/amdgpu: Serialize non TDR gpu recovery with TDRs
Use reset domain wq also for non TDR gpu recovery trigers such as sysfs and RAS. We must serialize all possible GPU recoveries to gurantee no concurrency there. For TDR call the original recovery function directly since it's already executed from within the wq. For others just use a wrapper to qeueue work and wait on it to finish. v2: Rename to amdgpu_recover_work_struct Signed-off-by: Andrey Grodzovsky Reviewed-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu.h| 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 33 +- drivers/gpu/drm/amd/amdgpu/amdgpu_job.c| 2 +- 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index b76c1cfad7f1..540a38fe5cd6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1298,6 +1298,8 @@ bool amdgpu_device_has_job_running(struct amdgpu_device *adev); bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev); int amdgpu_device_gpu_recover(struct amdgpu_device *adev, struct amdgpu_job* job); +int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, + struct amdgpu_job *job); void amdgpu_device_pci_config_reset(struct amdgpu_device *adev); int amdgpu_device_pci_reset(struct amdgpu_device *adev); bool amdgpu_device_need_post(struct amdgpu_device *adev); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 00123b0013d3..15e8fde3ac2d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -5033,7 +5033,7 @@ static void amdgpu_device_recheck_guilty_jobs( * Returns 0 for success or an error on failure. */ -int amdgpu_device_gpu_recover(struct amdgpu_device *adev, +int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev, struct amdgpu_job *job) { struct list_head device_list, *device_list_handle = NULL; @@ -5292,6 +5292,37 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, return r; } +struct amdgpu_recover_work_struct { + struct work_struct base; + struct amdgpu_device *adev; + struct amdgpu_job *job; + int ret; +}; + +static void amdgpu_device_queue_gpu_recover_work(struct work_struct *work) +{ + struct amdgpu_recover_work_struct *recover_work = container_of(work, struct amdgpu_recover_work_struct, base); + + recover_work->ret = amdgpu_device_gpu_recover_imp(recover_work->adev, recover_work->job); +} +/* + * Serialize gpu recover into reset domain single threaded wq + */ +int amdgpu_device_gpu_recover(struct amdgpu_device *adev, + struct amdgpu_job *job) +{ + struct amdgpu_recover_work_struct work = {.adev = adev, .job = job}; + + INIT_WORK(, amdgpu_device_queue_gpu_recover_work); + + if (!queue_work(adev->reset_domain.wq, )) + return -EAGAIN; + + flush_work(); + + return work.ret; +} + /** * amdgpu_device_get_pcie_info - fence pcie info about the PCIE slot * diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index bfc47bea23db..38c9fd7b7ad4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -63,7 +63,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct drm_sched_job *s_job) ti.process_name, ti.tgid, ti.task_name, ti.pid); if (amdgpu_device_should_recover_gpu(ring->adev)) { - amdgpu_device_gpu_recover(ring->adev, job); + amdgpu_device_gpu_recover_imp(ring->adev, job); } else { drm_sched_suspend_timeout(>sched); if (amdgpu_sriov_vf(adev)) -- 2.25.1
[RFC v4 01/11] drm/amdgpu: Introduce reset domain
Defined a reset_domain struct such that all the entities that go through reset together will be serialized one against another. Do it for both single device and XGMI hive cases. Signed-off-by: Andrey Grodzovsky Suggested-by: Daniel Vetter Suggested-by: Christian König Reviewed-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu.h| 5 + drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 +++- drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 9 + drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 2 ++ 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index d8b854fcbffa..b76c1cfad7f1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -813,6 +813,10 @@ struct amd_powerplay { #define AMDGPU_RESET_MAGIC_NUM 64 #define AMDGPU_MAX_DF_PERFMONS 4 #define AMDGPU_PRODUCT_NAME_LEN 64 +struct amdgpu_reset_domain { + struct workqueue_struct *wq; +}; + struct amdgpu_device { struct device *dev; struct pci_dev *pdev; @@ -1100,6 +1104,7 @@ struct amdgpu_device { uint32_t ip_versions[MAX_HWIP][HWIP_MAX_INSTANCE]; boolram_is_direct_mapped; + struct amdgpu_reset_domain reset_domain; }; static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index ed077de426d9..9704b0e1fd82 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -2398,9 +2398,27 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev) if (r) goto init_failed; - if (adev->gmc.xgmi.num_physical_nodes > 1) + if (adev->gmc.xgmi.num_physical_nodes > 1) { + struct amdgpu_hive_info *hive; + amdgpu_xgmi_add_device(adev); + hive = amdgpu_get_xgmi_hive(adev); + if (!hive || !hive->reset_domain.wq) { + DRM_ERROR("Failed to obtain reset domain info for XGMI hive:%llx", hive->hive_id); + r = -EINVAL; + goto init_failed; + } + + adev->reset_domain.wq = hive->reset_domain.wq; + } else { + adev->reset_domain.wq = alloc_ordered_workqueue("amdgpu-reset-dev", 0); + if (!adev->reset_domain.wq) { + r = -ENOMEM; + goto init_failed; + } + } + /* Don't init kfd if whole hive need to be reset during init */ if (!adev->gmc.xgmi.pending_reset) amdgpu_amdkfd_device_init(adev); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c index e8b8f28c2f72..d406897346d6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c @@ -398,6 +398,14 @@ struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev) goto pro_end; } + hive->reset_domain.wq = alloc_ordered_workqueue("amdgpu-reset-hive", 0); + if (!hive->reset_domain.wq) { + dev_err(adev->dev, "XGMI: failed allocating wq for reset domain!\n"); + kfree(hive); + hive = NULL; + goto pro_end; + } + hive->hive_id = adev->gmc.xgmi.hive_id; INIT_LIST_HEAD(>device_list); INIT_LIST_HEAD(>node); @@ -407,6 +415,7 @@ struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev) task_barrier_init(>tb); hive->pstate = AMDGPU_XGMI_PSTATE_UNKNOWN; hive->hi_req_gpu = NULL; + /* * hive pstate on boot is high in vega20 so we have to go to low * pstate on after boot. diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h index d2189bf7d428..6121aaa292cb 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h @@ -42,6 +42,8 @@ struct amdgpu_hive_info { AMDGPU_XGMI_PSTATE_MAX_VEGA20, AMDGPU_XGMI_PSTATE_UNKNOWN } pstate; + + struct amdgpu_reset_domain reset_domain; }; struct amdgpu_pcs_ras_field { -- 2.25.1
[RFC v4 00/11] Define and use reset domain for GPU recovery in amdgpu
This patchset is based on earlier work by Boris[1] that allowed to have an ordered workqueue at the driver level that will be used by the different schedulers to queue their timeout work. On top of that I also serialized any GPU reset we trigger from within amdgpu code to also go through the same ordered wq and in this way simplify somewhat our GPU reset code so we don't need to protect from concurrency by multiple GPU reset triggeres such as TDR on one hand and sysfs trigger or RAS trigger on the other hand. As advised by Christian and Daniel I defined a reset_domain struct such that all the entities that go through reset together will be serialized one against another. TDR triggered by multiple entities within the same domain due to the same reason will not be triggered as the first such reset will cancel all the pending resets. This is relevant only to TDR timers and not to triggered resets coming from RAS or SYSFS, those will still happen after the in flight resets finishes. v2: Add handling on SRIOV configuration, the reset notify coming from host and driver already trigger a work queue to handle the reset so drop this intermediate wq and send directly to timeout wq. (Shaoyun) v3: Lijo suggested puting 'adev->in_gpu_reset' in amdgpu_reset_domain struct. I followed his advise and also moved adev->reset_sem into same place. This in turn caused to do some follow-up refactor of the original patches where i decoupled amdgpu_reset_domain life cycle frolm XGMI hive because hive is destroyed and reconstructed for the case of reset the devices in the XGMI hive during probe for SRIOV See [2] while we need the reset sem and gpu_reset flag to always be present. This was attained by adding refcount to amdgpu_reset_domain so each device can safely point to it as long as it needs. v4: Just bug fixing of reset_domain refcount on XGMI hive boot and some cosmetic wrappers to reset domain refocunt. [1] https://patchwork.kernel.org/project/dri-devel/patch/20210629073510.2764391-3-boris.brezil...@collabora.com/ [2] https://www.spinics.net/lists/amd-gfx/msg58836.html P.S Going through drm-misc-next and not amd-staging-drm-next as Boris work hasn't landed yet there. P.P.S Patches 8-12 are the refactor on top of the original V2 patchset. Andrey Grodzovsky (11): drm/amdgpu: Introduce reset domain drm/amdgpu: Move scheduler init to after XGMI is ready drm/amdgpu: Serialize non TDR gpu recovery with TDRs drm/amd/virt: For SRIOV send GPU reset directly to TDR queue. drm/amdgpu: Drop hive->in_reset drm/amdgpu: Drop concurrent GPU reset protection for device drm/amdgpu: Rework reset domain to be refcounted. drm/amdgpu: Move reset sem into reset_domain drm/amdgpu: Move in_gpu_reset into reset_domain drm/amdgpu: Rework amdgpu_device_lock_adev Revert 'drm/amdgpu: annotate a false positive recursive locking' drivers/gpu/drm/amd/amdgpu/amdgpu.h | 13 +- drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 +- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 275 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 34 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- .../gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c| 18 +- drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 43 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h | 37 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 + drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 27 +- drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 3 +- drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c| 6 +- drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 14 +- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 19 +- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 19 +- drivers/gpu/drm/amd/amdgpu/mxgpu_vi.c | 11 +- 16 files changed, 335 insertions(+), 198 deletions(-) -- 2.25.1
Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
On 2/8/22 22:08, Daniel Vetter wrote: > This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee. > > With > > commit 27599aacbaefcbf2af7b06b0029459bbf682000d > Author: Thomas Zimmermann > Date: Tue Jan 25 10:12:18 2022 +0100 > > fbdev: Hot-unplug firmware fb devices on forced removal > > this should be fixed properly and we can remove this somewhat hackish > check here (e.g. this won't catch drm drivers if fbdev emulation isn't > enabled). > Unfortunately this hack can't be reverted yet. Thomas' patch solves the issue of platform devices matched with fbdev drivers to be properly unregistered if a DRM driver attempts to remove all the conflicting framebuffers. But the problem that fb561bf9abde ("fbdev: Prevent probing generic drivers if a FB is already registered") worked around is different. It happens when the DRM driver is probed before the {efi,simple}fb and other fbdev drivers, the kicking out of conflicting framebuffers already happened and these drivers will be allowed to probe even when a DRM driver is already present. We need a clearer way to prevent it, but can't revert fb561bf9abde until that. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 06/19] fbcon: Use delayed work for cursor
Hello Daniel, On 2/8/22 22:08, Daniel Vetter wrote: > Allows us to delete a bunch of hand-rolled stuff. Also to simplify the > code we initialize the cursor_work completely when we allocate the > fbcon_ops structure, instead of trying to cope with console > re-initialization. > Maybe also make it more explicit in the commit message that the delayed work is replacing a timer that was used before for the cursor ? > The motiviation here is that fbcon code stops using the fb_info.queue, motivation [snip] > /* > *This is the interface between the low-level console driver and the > @@ -68,7 +68,7 @@ struct fbcon_ops { > int (*update_start)(struct fb_info *info); > int (*rotate_font)(struct fb_info *info, struct vc_data *vc); > struct fb_var_screeninfo var; /* copy of the current fb_var_screeninfo > */ > - struct timer_list cursor_timer; /* Cursor timer */ > + struct delayed_work cursor_work; /* Cursor timer */ A delayed_work uses a timer underneath but I wonder if the comment also needs to be updated since technically isn't a timer anymore but deferred work that gets re-scheduled each time on fb_flashcursor(). The patch looks good to me and makes the logic much simpler than before. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 6/8] mm: don't include in
On Mon, Feb 7, 2022 at 3:49 PM Dan Williams wrote: > > On Sun, Feb 6, 2022 at 10:33 PM Christoph Hellwig wrote: > > > > Move the check for the actual pgmap types that need the free at refcount > > one behavior into the out of line helper, and thus avoid the need to > > pull memremap.h into mm.h. > > Looks good to me assuming the compile bots agree. > > Reviewed-by: Dan Williams Yeah, same as Logan: mm/memcontrol.c: In function ‘get_mctgt_type’: mm/memcontrol.c:5724:29: error: implicit declaration of function ‘is_device_private_page’; did you mean ‘is_device_private_entry’? [-Werror=implicit-function-declaration] 5724 | if (is_device_private_page(page)) | ^~ | is_device_private_entry ...needs: diff --git a/mm/memcontrol.c b/mm/memcontrol.c index d1e97a54ae53..0ac7515c85f9 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -62,6 +62,7 @@ #include #include #include +#include #include "internal.h" #include #include
Re: [RFC][PATCH] Revert "drm/panel-simple: drop use of data-mapping property"
On 2/8/22 22:27, Christoph Niedermaier wrote: From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] Sent: Thursday, February 3, 2022 12:46 AM Hi Christoph, Hi Laurent, On Tue, Feb 01, 2022 at 12:07:17PM +0100, Christoph Niedermaier wrote: Without the data-mapping devicetree property my display won't work properly. It is flickering, because the bus flags won't be assigned without a defined bus format by the imx parallel display driver. There was a discussion about the removal [1] and an agreement that a better solution is needed, but it is missing so far. So what would be the better approach? [1] https://patchwork.freedesktop.org/patch/357659/?series=74705=1 This reverts commit d021d751c14752a0266865700f6f212fab40a18c. Signed-off-by: Christoph Niedermaier Cc: Marek Vasut Cc: Sam Ravnborg Cc: Laurent Pinchart Cc: Maxime Ripard Cc: Philipp Zabel Cc: David Airlie Cc: Daniel Vetter Cc: Shawn Guo Cc: Sascha Hauer Cc: Pengutronix Kernel Team Cc: Fabio Estevam Cc: NXP Linux Team Cc: linux-arm-ker...@lists.infradead.org To: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/panel/panel-simple.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 3c08f9827acf..2c683d94a3f3 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, struct panel_desc *desc; unsigned int bus_flags; struct videomode vm; + const char *mapping; int ret; np = dev->of_node; @@ -477,6 +478,16 @@ static int panel_dpi_probe(struct device *dev, of_property_read_u32(np, "width-mm", >size.width); of_property_read_u32(np, "height-mm", >size.height); + of_property_read_string(np, "data-mapping", ); + if (!strcmp(mapping, "rgb24")) + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; + else if (!strcmp(mapping, "rgb565")) + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; + else if (!strcmp(mapping, "bgr666")) + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; + else if (!strcmp(mapping, "lvds666")) + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; You're right that there's an issue, but a revert isn't the right option. The commit you're reverting never made it in a stable release, because it was deemed to not be a good enough option. First of all, any attempt to fix this should include an update to the DT binding. Second, as this is about DPI panels, the LVDS option should be dropped. Finally, I've shared some initial thoughts in [1], maybe you can reply to that e-mail to continue the discussion there ? According to your thoughts in [1] you mean that the bus format should be build out of the devicetree properties bus-width and data-shift. It would be possible for evenly structured busses like RGB888_1X24 and RGB666_1X18, but what about a bus like RGB565_1X16, where each color has different bus width. Also the order of the colors should be defined to differ between busses like RGB888_1X24 and GBR888_1X24. Are there any ideas how can this be covered? Maybe with props like these ? channel-width -- width of each color channel channel-shift -- shift of each color channel channel-map -- mapping of each color channel So for RGB888 channel-width = <8 8 8>; channel-shift = <0 0 0>; channel-map = "RGB"; // or something ? For BGR565 panel connected to RGB24 scanout channel-width = <5 6 5>; channel-shift = <3 2 3>; channel-map = "BGR"; // or something ? For BGR565 panel connected to RGB565 scanout channel-width = <5 6 5>; channel-shift = <0 0 0>; channel-map = "BGR"; // or something ?
Re: [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD
On Tue 08 Feb 07:18 PST 2022, Sankeerth Billakanti wrote: > Enable the eDP display panel support without HPD on sc7280 platform. > > Signed-off-by: Sankeerth Billakanti > --- > > Changes in v2: > - sort node references alphabetically > - improve readability > - move the pwm pinctrl to pwm node > - move the regulators to root > - define backlight power > - remove dummy regulator node > - cleanup pinctrl definitions > > arch/arm64/boot/dts/qcom/sc7280-crd.dts | 122 > > arch/arm64/boot/dts/qcom/sc7280.dtsi| 2 - > 2 files changed, 122 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > index e2efbdd..bff2707 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > @@ -21,6 +21,34 @@ > chosen { > stdout-path = "serial0:115200n8"; > }; > + > + backlight_power: backlight-power { > + compatible = "regulator-fixed"; > + regulator-name = "backlight_power"; > + > + regulator-min-microvolt = <180>; > + regulator-max-microvolt = <180>; > + > + gpio = <_gpios 7 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_bl_power>; > + }; > + > + edp_power: edp-power { > + compatible = "regulator-fixed"; > + regulator-name = "edp_power"; > + > + regulator-min-microvolt = <330>; > + regulator-max-microvolt = <330>; > + > + gpio = < 80 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_panel_power>; > + }; > }; > > _rsc { > @@ -76,6 +104,42 @@ ap_ts_pen_1v8: { > }; > }; > > +_out { Sorry for missing this while merging changes in sc7280.dtsi. But it would be really nice if this was labeled mdss_edp_out instead (or possibly defined within the _edp node). Now you will have _out and _out floating around away from the edp and dp nodes... > + remote-endpoint = <_panel_in>; > +}; > + > + { > + status = "okay"; > +}; > + > +_edp { > + status = "okay"; > + > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l10c_0p8>; > +}; > + > +_edp_phy { > + status = "okay"; > + > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l10c_0p8>; > +}; > + > +_dp { > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_hot_plug_det>; > + data-lanes = <0 1>; > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l1b_0p8>; > +}; > + > +_mdp { > + status = "okay"; > +}; > + > _3v3_regulator { > gpio = < 51 GPIO_ACTIVE_HIGH>; > }; > @@ -84,7 +148,65 @@ ap_ts_pen_1v8: { > pins = "gpio51"; > }; > > +_pwm { This label doesn't exist, so I won't be able to merge this patch. > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_bl_pwm>; > +}; > + > +_gpios { > + edp_bl_power: edp-bl-power { > + pins = "gpio7"; > + function = "normal"; > + qcom,drive-strength = ; > + bias-disable; > + output-low; > + }; > + > + edp_bl_pwm: edp-bl-pwm { > + pins = "gpio8"; > + function = "func1"; > + qcom,drive-strength = ; > + bias-disable; > + output-low; > + }; > +}; > + > + { > + edp_backlight: edp-backlight { > + compatible = "pwm-backlight"; This is not a device on the mmio bus, so it should not love within the > + > + power-supply = <_power>; > + pwms = <_pwm 3 65535>; > + }; > + > + edp_panel: edp_panel { Ditto. Regards, Bjorn > + compatible = "sharp,lq140m1jw46"; > + > + power-supply = <_power>; > + backlight = <_backlight>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + edp_panel_in: endpoint { > + remote-endpoint = <_out>; > + }; > + }; > + }; > + }; > +}; > + > { > + edp_panel_power: edp-panel-power { > + pins = "gpio80"; > + function = "gpio"; > + bias-pull-down; > + }; > + > tp_int_odl: tp-int-odl { > pins = "gpio7"; > function = "gpio"; > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 3572399..f8fa716 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -3012,8 +3012,6 @@ > > mdss_edp:
Re: [Intel-gfx] [PATCH 0/2] drm/i915/guc: Use temporary memory for regset
It looks like for this series I forgot to Cc dri-devel, although these patches are the same as the ones in https://patchwork.freedesktop.org/series/99711/, just extracted since not dependent on the iosys-map discussion. Lucas De Marchi On Mon, Feb 07, 2022 at 11:01:39PM -0800, Lucas De Marchi wrote: Extract the 2 commits not related to iosys_map from drm/i915/guc: Refactor ADS access to use iosys_map (https://patchwork.freedesktop.org/series/99711/). The conversion of the rest of ADS initializon will take more time to review. So let's take these by themselves as suggested by Daniele. Lucas De Marchi (2): drm/i915/guc: Prepare for error propagation drm/i915/guc: Use a single pass to calculate regset drivers/gpu/drm/i915/gt/uc/intel_guc.h | 7 + drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 159 + 2 files changed, 108 insertions(+), 58 deletions(-) -- 2.35.1
Re: [Intel-gfx] [PATCH v2 03/18] drm/i915/gt: Add helper for shmem copy to iosys_map
On Tue, Feb 08, 2022 at 02:45:09AM -0800, Lucas De Marchi wrote: > Add a variant of shmem_read() that takes a iosys_map pointer rather > than a plain pointer as argument. It's mostly a copy __shmem_rw() but > adapting the api and removing the write support since there's currently > only need to use iosys_map as destination. > > Reworking __shmem_rw() to share the implementation was tempting, but > finding a good balance between reuse and clarity pushed towards a little > code duplication. Since the function is small, just add the similar > function with a copy/paste/adapt approach. > > v2: Add an offset as argument and instead of using a map iterator, use the > offset to keep track of where we are writing data to. > > Cc: Matt Roper > Cc: Joonas Lahtinen > Cc: Tvrtko Ursulin > Cc: David Airlie > Cc: Daniel Vetter > Cc: Matthew Auld > Cc: Thomas Hellström > Cc: Maarten Lankhorst Reviewed-by: Matt Atwood > Signed-off-by: Lucas De Marchi > --- > drivers/gpu/drm/i915/gt/shmem_utils.c | 32 +++ > drivers/gpu/drm/i915/gt/shmem_utils.h | 3 +++ > 2 files changed, 35 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.c > b/drivers/gpu/drm/i915/gt/shmem_utils.c > index 0683b27a3890..402f085f3a02 100644 > --- a/drivers/gpu/drm/i915/gt/shmem_utils.c > +++ b/drivers/gpu/drm/i915/gt/shmem_utils.c > @@ -3,6 +3,7 @@ > * Copyright © 2020 Intel Corporation > */ > > +#include > #include > #include > #include > @@ -123,6 +124,37 @@ static int __shmem_rw(struct file *file, loff_t off, > return 0; > } > > +int shmem_read_to_iosys_map(struct file *file, loff_t off, > + struct iosys_map *map, size_t map_off, size_t len) > +{ > + unsigned long pfn; > + > + for (pfn = off >> PAGE_SHIFT; len; pfn++) { > + unsigned int this = > + min_t(size_t, PAGE_SIZE - offset_in_page(off), len); > + struct page *page; > + void *vaddr; > + > + page = shmem_read_mapping_page_gfp(file->f_mapping, pfn, > +GFP_KERNEL); > + if (IS_ERR(page)) > + return PTR_ERR(page); > + > + vaddr = kmap(page); > + iosys_map_memcpy_to(map, map_off, vaddr + offset_in_page(off), > + this); > + mark_page_accessed(page); > + kunmap(page); > + put_page(page); > + > + len -= this; > + map_off += this; > + off = 0; > + } > + > + return 0; > +} > + > int shmem_read(struct file *file, loff_t off, void *dst, size_t len) > { > return __shmem_rw(file, off, dst, len, false); > diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.h > b/drivers/gpu/drm/i915/gt/shmem_utils.h > index c1669170c351..b2b04d88c6e5 100644 > --- a/drivers/gpu/drm/i915/gt/shmem_utils.h > +++ b/drivers/gpu/drm/i915/gt/shmem_utils.h > @@ -8,6 +8,7 @@ > > #include > > +struct iosys_map; > struct drm_i915_gem_object; > struct file; > > @@ -17,6 +18,8 @@ struct file *shmem_create_from_object(struct > drm_i915_gem_object *obj); > void *shmem_pin_map(struct file *file); > void shmem_unpin_map(struct file *file, void *ptr); > > +int shmem_read_to_iosys_map(struct file *file, loff_t off, > + struct iosys_map *map, size_t map_off, size_t len); > int shmem_read(struct file *file, loff_t off, void *dst, size_t len); > int shmem_write(struct file *file, loff_t off, void *src, size_t len); > > -- > 2.35.1 >
Re: [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions
On 2/8/22 22:08, Daniel Vetter wrote: > Avoids two forward declarations, and more importantly, matches what > I've done in my fbcon scrolling restore patches - so I need this to > avoid a bunch of conflicts in rebasing since we ended up merging > Helge's series instead. > > Signed-off-by: Daniel Vetter Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [Intel-gfx] [PATCH v2 02/18] iosys-map: Add a few more helpers
On Tue, Feb 08, 2022 at 02:45:08AM -0800, Lucas De Marchi wrote: > First the simplest ones: > > - iosys_map_memset(): when abstracting system and I/O memory, > just like the memcpy() use case, memset() also has dedicated > functions to be called for using IO memory. > - iosys_map_memcpy_from(): we may need to copy data from I/O > memory, not only to. > > In certain situations it's useful to be able to read or write to an > offset that is calculated by having the memory layout given by a struct > declaration. Usually we are going to read/write a u8, u16, u32 or u64. > > As a pre-requisite for the implementation, add iosys_map_memcpy_from() > to be the equivalent of iosys_map_memcpy_to(), but in the other > direction. Then add 2 pairs of macros: > > - iosys_map_rd() / iosys_map_wr() > - iosys_map_rd_field() / iosys_map_wr_field() > > The first pair takes the C-type and offset to read/write. The second > pair uses a struct describing the layout of the mapping in order to > calculate the offset and size being read/written. > > We could use readb, readw, readl, readq and the write* counterparts, > however due to alignment issues this may not work on all architectures. > If alignment needs to be checked to call the right function, it's not > possible to decide at compile-time which function to call: so just leave > the decision to the memcpy function that will do exactly that. > > Finally, in order to use the above macros with a map derived from > another, add another initializer: IOSYS_MAP_INIT_OFFSET(). > > v2: > - Rework IOSYS_MAP_INIT_OFFSET() so it doesn't rely on aliasing rules > within the union > - Add offset to both iosys_map_rd_field() and iosys_map_wr_field() to > allow the struct itself to be at an offset from the mapping > - Add documentation to iosys_map_rd_field() with example and expected > memory layout > > Cc: Sumit Semwal > Cc: Christian König > Cc: Thomas Zimmermann > Cc: Mauro Carvalho Chehab > Cc: dri-devel@lists.freedesktop.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Matt Atwood > Signed-off-by: Lucas De Marchi > --- > include/linux/iosys-map.h | 202 ++ > 1 file changed, 202 insertions(+) > > diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h > index edd730b1e899..c6b223534b21 100644 > --- a/include/linux/iosys-map.h > +++ b/include/linux/iosys-map.h > @@ -6,6 +6,7 @@ > #ifndef __IOSYS_MAP_H__ > #define __IOSYS_MAP_H__ > > +#include > #include > #include > > @@ -120,6 +121,45 @@ struct iosys_map { > .is_iomem = false, \ > } > > +/** > + * IOSYS_MAP_INIT_OFFSET - Initializes struct iosys_map from another > iosys_map > + * @map_:The dma-buf mapping structure to copy from > + * @offset_: Offset to add to the other mapping > + * > + * Initializes a new iosys_map struct based on another passed as argument. It > + * does a shallow copy of the struct so it's possible to update the back > storage > + * without changing where the original map points to. It is the equivalent of > + * doing: > + * > + * .. code-block:: c > + * > + * iosys_map map = other_map; > + * iosys_map_incr(, ); > + * > + * Example usage: > + * > + * .. code-block:: c > + * > + * void foo(struct device *dev, struct iosys_map *base_map) > + * { > + * ... > + * struct iosys_map map = IOSYS_MAP_INIT_OFFSET(base_map, > FIELD_OFFSET); > + * ... > + * } > + * > + * The advantage of using the initializer over just increasing the offset > with > + * iosys_map_incr() like above is that the new map will always point to the > + * right place of the buffer during its scope. It reduces the risk of > updating > + * the wrong part of the buffer and having no compiler warning about that. If > + * the assignment to IOSYS_MAP_INIT_OFFSET() is forgotten, the compiler can > warn > + * about the use of uninitialized variable. > + */ > +#define IOSYS_MAP_INIT_OFFSET(map_, offset_) ({ > \ > + struct iosys_map copy = *map_; \ > + iosys_map_incr(, offset_); \ > + copy; \ > +}) > + > /** > * iosys_map_set_vaddr - Sets a iosys mapping structure to an address in > system memory > * @map: The iosys_map structure > @@ -239,6 +279,26 @@ static inline void iosys_map_memcpy_to(struct iosys_map > *dst, size_t dst_offset, > memcpy(dst->vaddr + dst_offset, src, len); > } > > +/** > + * iosys_map_memcpy_from - Memcpy from iosys_map into system memory > + * @dst: Destination in system memory > + * @src: The iosys_map structure > + * @src_offset: The offset from which to copy > + * @len: The number of byte in src > + * > + * Copies data from a iosys_map with an offset. The dest buffer is in > + * system memory. Depending on the
Re: [PATCH v3 4/4] drm/i915/guc: Verify hwconfig blob matches supported format
On 08.02.2022 22:05, Jordan Justen wrote: > i915_drm.h now defines the format of the returned > DRM_I915_QUERY_HWCONFIG_BLOB query item. Since i915 receives this from > the black box GuC software, it should verify that the data matches > that format before sending it to user-space. > > The verification makes a single simple pass through the blob contents, > so this verification step should not add a significant amount of init > time to i915. > > v3: > * Add various changes suggested by Tvrtko > > Signed-off-by: Jordan Justen > --- > .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 56 ++- > 1 file changed, 53 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c > index ce6088f112d4..350a0517b9f0 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c > @@ -71,7 +71,52 @@ static int guc_hwconfig_discover_size(struct > intel_guc_hwconfig *hwconfig) > return 0; > } > > -static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig) > +static int verify_hwconfig_blob(struct drm_device *drm, no need to pass drm as you can use: + struct intel_guc *guc = hwconfig_to_guc(hwconfig); + struct drm_i915_private *i915 = guc_to_gt(guc)->i915; > + const struct intel_guc_hwconfig *hwconfig) > +{ > + struct drm_i915_query_hwconfig_blob_item *pos; > + u32 remaining; > + > + if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL) size alignment could be verified in guc_hwconfig_discover_size() nit: instead of hardcoded 4 you may use 'sizeof(u32)' nit: and IS_ALIGNED and non-null ptr shall be enforced with GEM_BUG_ON as you are calling this function after memcpy > + return -EINVAL; > + > + pos = hwconfig->ptr; add line space and please update below multi-line comments format to /* * blah... > + /* The number of dwords in the blob to validate. Each loop > + * pass will process at least 2 dwords corresponding to the > + * key and length fields of the item. In addition, the length > + * field of the item indicates the length of the data array, > + * and that number of dwords will be processed (skipped) as > + * well. > + */ > + remaining = hwconfig->size / 4; > + > + while (remaining > 0) { > + /* Each item requires at least 2 dwords for the key > + * and length fields. If the length field is 0, then > + * the data array would be of length 0. > + */ > + if (remaining < 2) > + return -EINVAL; > + /* remaining >= 2, so subtracting 2 is ok, whereas > + * adding 2 to pos->length could overflow. > + */ > + if (pos->length > remaining - 2) > + return -EINVAL; > + /* The length check above ensures that the adjustment > + * of the remaining variable will not underflow, and > + * that the adjustment of the pos variable will not > + * pass the end of the blob data. > + */ > + remaining -= 2 + pos->length; > + pos = (void *)>data[pos->length]; > + } btw, if it needs comments then it is too complicated ;) > + > + drm_dbg(drm, "hwconfig blob format is valid\n"); not sure if we need this since we have error message in case of failure maybe better to add dbg message why we claim it is invalid > + return 0; > +} > + > +static int guc_hwconfig_fill_buffer(struct drm_device *drm, no need to pass drm > + struct intel_guc_hwconfig *hwconfig) > { > struct intel_guc *guc = hwconfig_to_guc(hwconfig); > struct i915_vma *vma; > @@ -88,8 +133,13 @@ static int guc_hwconfig_fill_buffer(struct > intel_guc_hwconfig *hwconfig) > ggtt_offset = intel_guc_ggtt_offset(guc, vma); > > ret = __guc_action_get_hwconfig(hwconfig, ggtt_offset, hwconfig->size); > - if (ret >= 0) > + if (ret >= 0) { > memcpy(hwconfig->ptr, vaddr, hwconfig->size); > + if (verify_hwconfig_blob(drm, hwconfig)) { > + drm_err(drm, "Ignoring invalid hwconfig blob received > from GuC!\n"); > + ret = -EINVAL; btw, since we are about to release blob on verification failure, shouldn't we hexdump whole (or part of) blob somewhere for investigations ? or maybe we should expose this blob in debugfs, and do it regardless if it is valid or not, and just fail ioctl if blob is believed to be corrupted. ~Michal > + } > + } > > i915_vma_unpin_and_release(, I915_VMA_RELEASE_MAP); > > @@ -141,7 +191,7 @@ int intel_guc_hwconfig_init(struct intel_guc_hwconfig > *hwconfig) > return -ENOMEM; > } > > - ret =
Re: [Intel-gfx] [PATCH 4/4] drm/i915/guc: Verify hwconfig blob matches supported format
Tvrtko Ursulin writes: > Will GuC folks be reviewing this work? I don't know. If you mean the i915 devs interfacing with the GuC, I know John/Rodrigo seemed a bit resistant writing the patches to give userspace this trivial format guarantee on the blob. So, I hoped they would write the patches (3 & 4 in my series), but now I'm hoping they will at least accept the patches. > Quick sanity check maybe makes sense, given data is being "sent" to > userspace directly, I am just not sure if it is worth having in > non-debug builds of i915. Though I will agree not having it in > production then largely defeats the purpose so dunno. The check seems fairly trivial, and it seems like i915 should provide whatever guidance/guarantee is possible to userspace. (Thus, do it once per boot even on release builds.) See also, the commit message I added. > Effective difference if GuC load fails versus userspace libraries > failing to parse hwconfig? Lots of potential things to consider. Personally, I think for internal Intel builds, if this check fails, then it ought to cause the GuC to always fail to load, which today means the device will be wedged. For external builds, I think it should still load the GuC but not send the blob to userspace. This is what should happen with the patches in this series. (I really hope this never happens, which it why I think the internal builds should be so harsh.) Now ... if i915 ever regains the ability to drive the device without the closed source GuC, well... No reason to go off on unrealistic tangents. :) Also, later down the road for released products, userspace drivers may choose to bypass the hwconfig to limit the dependence on GuC. That is a related, but different topic. -Jordan
Re: [PATCH] drm/msm/dp: Add DisplayPort controller for SM8350
On Wed, 9 Feb 2022 at 00:21, Bjorn Andersson wrote: > > On Wed 19 Jan 15:14 PST 2022, Dmitry Baryshkov wrote: > > > On 28/12/2021 07:59, Bjorn Andersson wrote: > > > The Qualcomm SM8350 platform comes with a single DisplayPort controller, > > > add support for this in the DisplayPort driver. > > > > > > Signed-off-by: Bjorn Andersson > > > > Reviewed-by: Dmitry Baryshkov > > > > I don't see this in linux-next, would it be possible to pick it up now > that we're past the merge window etc? I'll work on my staging tree (and send it to Rob) before the EoW. > > Regards, > Bjorn > > > > --- > > > .../devicetree/bindings/display/msm/dp-controller.yaml| 1 + > > > drivers/gpu/drm/msm/dp/dp_display.c | 8 > > > 2 files changed, 9 insertions(+) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > > > b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > > > index 5457612ab136..cd05cfd76536 100644 > > > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > > > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > > > @@ -21,6 +21,7 @@ properties: > > > - qcom,sc7280-edp > > > - qcom,sc8180x-dp > > > - qcom,sc8180x-edp > > > + - qcom,sm8350-dp > > > reg: > > > items: > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > > > b/drivers/gpu/drm/msm/dp/dp_display.c > > > index 8d9c19dbf33e..fd0fd03f8fed 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > > @@ -143,10 +143,18 @@ static const struct msm_dp_config sc7280_dp_cfg = { > > > .num_descs = 2, > > > }; > > > +static const struct msm_dp_config sm8350_dp_cfg = { > > > + .descs = (const struct msm_dp_desc[]) { > > > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae9, > > > .connector_type = DRM_MODE_CONNECTOR_DisplayPort }, > > > + }, > > > + .num_descs = 1, > > > +}; > > > + > > > static const struct of_device_id dp_dt_match[] = { > > > { .compatible = "qcom,sc7180-dp", .data = _dp_cfg }, > > > { .compatible = "qcom,sc7280-dp", .data = _dp_cfg }, > > > { .compatible = "qcom,sc7280-edp", .data = _dp_cfg }, > > > + { .compatible = "qcom,sm8350-dp", .data = _dp_cfg }, > > > {} > > > }; > > > > > > -- > > With best wishes > > Dmitry -- With best wishes Dmitry
Re: [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD
On Tue, Feb 08, 2022 at 08:48:43PM +0530, Sankeerth Billakanti wrote: > Enable the eDP display panel support without HPD on sc7280 platform. > > Signed-off-by: Sankeerth Billakanti > --- > > Changes in v2: > - sort node references alphabetically > - improve readability > - move the pwm pinctrl to pwm node > - move the regulators to root > - define backlight power > - remove dummy regulator node > - cleanup pinctrl definitions > > arch/arm64/boot/dts/qcom/sc7280-crd.dts | 122 > > arch/arm64/boot/dts/qcom/sc7280.dtsi| 2 - > 2 files changed, 122 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > index e2efbdd..bff2707 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > @@ -21,6 +21,34 @@ > chosen { > stdout-path = "serial0:115200n8"; > }; > + > + backlight_power: backlight-power { nit: the other fixed regulator in sc7280-idp.dtsi is called 'nvme_3v3_regulator', if you wanted to be consistent you could call this backlight_3v3_regulator. > + compatible = "regulator-fixed"; > + regulator-name = "backlight_power"; > + > + regulator-min-microvolt = <180>; > + regulator-max-microvolt = <180>; > + > + gpio = <_gpios 7 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_bl_power>; > + }; > + > + edp_power: edp-power { nit: see above > + compatible = "regulator-fixed"; > + regulator-name = "edp_power"; > + > + regulator-min-microvolt = <330>; > + regulator-max-microvolt = <330>; > + > + gpio = < 80 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_panel_power>; > + }; > }; > > _rsc { > @@ -76,6 +104,42 @@ ap_ts_pen_1v8: { > }; > }; > > +_out { > + remote-endpoint = <_panel_in>; > +}; > + > + { > + status = "okay"; > +}; > + > +_edp { > + status = "okay"; > + > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l10c_0p8>; > +}; > + > +_edp_phy { > + status = "okay"; > + > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l10c_0p8>; > +}; > + > +_dp { should be before 'mdss_edp'. > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_hot_plug_det>; > + data-lanes = <0 1>; > + vdda-1p2-supply = <_l6b_1p2>; > + vdda-0p9-supply = <_l1b_0p8>; > +}; > + > +_mdp { > + status = "okay"; > +}; > + > _3v3_regulator { > gpio = < 51 GPIO_ACTIVE_HIGH>; > }; > @@ -84,7 +148,65 @@ ap_ts_pen_1v8: { > pins = "gpio51"; > }; > > +_pwm { > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <_bl_pwm>; > +}; > + > +_gpios { should be before 'pm8350c_pwm' > + edp_bl_power: edp-bl-power { > + pins = "gpio7"; > + function = "normal"; > + qcom,drive-strength = ; > + bias-disable; > + output-low; > + }; > + > + edp_bl_pwm: edp-bl-pwm { > + pins = "gpio8"; > + function = "func1"; > + qcom,drive-strength = ; > + bias-disable; > + output-low; > + }; > +}; > + > + { > + edp_backlight: edp-backlight { > + compatible = "pwm-backlight"; > + > + power-supply = <_power>; > + pwms = <_pwm 3 65535>; > + }; > + > + edp_panel: edp_panel { in difference to labels node names should use dashes as separator, not underscores (i.e. 'edp-panel') > + compatible = "sharp,lq140m1jw46"; > + > + power-supply = <_power>; > + backlight = <_backlight>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + edp_panel_in: endpoint { > + remote-endpoint = <_out>; > + }; > + }; > + }; > + }; > +}; > + > { > + edp_panel_power: edp-panel-power { > + pins = "gpio80"; > + function = "gpio"; > + bias-pull-down; > + }; > + > tp_int_odl: tp-int-odl { > pins = "gpio7"; > function = "gpio"; > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 3572399..f8fa716 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -3012,8 +3012,6 @@ > > mdss_edp: edp@aea { > compatible = "qcom,sc7280-edp";
Re: [PATCH] drm/i915/psr: Disable PSR2 selective fetch for all TGL steps
Opened the issue at https://gitlab.freedesktop.org/drm/intel/-/issues/5077 , included dmesg + video. Feel free to let me know if you need any more info, or need me to try any patches On Tue, 2022-02-08 at 13:06 +, Souza, Jose wrote: > On Mon, 2022-02-07 at 16:38 -0500, Lyude Paul wrote: > > As we've unfortunately started to come to expect from PSR on Intel > > platforms, PSR2 selective fetch is not at all ready to be enabled on > > Tigerlake as it results in severe flickering issues - at least on this > > ThinkPad X1 Carbon 9th generation. The easiest way I've found of > > reproducing these issues is to just move the cursor around the left border > > of the screen (suspicious…). > > Where is the bug for that? Where is the logs? > We can't go from enabled to disabled without any debug and because of a > single device. > In the mean time you have the option to set the i915 parameter to disable > it. > > > > > So, fix people's displays again and turn PSR2 selective fetch off for all > > steppings of Tigerlake. This can be re-enabled again if someone from Intel > > finds the time to fix this functionality on OEM machines. > > > > Signed-off-by: Lyude Paul > > Fixes: 7f6002e58025 ("drm/i915/display: Enable PSR2 selective fetch by > > default") > > Cc: Gwan-gyeong Mun > > Cc: Ville Syrjälä > > Cc: José Roberto de Souza > > Cc: Jani Nikula > > Cc: Rodrigo Vivi > > Cc: intel-...@lists.freedesktop.org > > Cc: # v5.16+ > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 10 +++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index a1a663f362e7..25c16abcd9cd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -737,10 +737,14 @@ static bool intel_psr2_sel_fetch_config_valid(struct > > intel_dp *intel_dp, > > return false; > > } > > > > - /* Wa_14010254185 Wa_14010103792 */ > > - if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > > + /* > > + * There's two things stopping this from being enabled on TGL: > > + * For steps A0-C0: workarounds Wa_14010254185 Wa_14010103792 are > > missing > > + * For all steps: PSR2 selective fetch causes screen flickering > > + */ > > + if (IS_TIGERLAKE(dev_priv)) { > > drm_dbg_kms(_priv->drm, > > - "PSR2 sel fetch not enabled, missing the > > implementation of WAs\n"); > > + "PSR2 sel fetch not enabled, currently broken > > on TGL\n"); > > return false; > > } > > > -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
Re: [PATCH v3 1/4] drm/i915/guc: Add fetch of hwconfig table
On 08.02.2022 22:05, Jordan Justen wrote: > From: John Harrison > > Implement support for fetching the hardware description table from the > GuC. The call is made twice - once without a destination buffer to > query the size and then a second time to fill in the buffer. > > Note that the table is only available on ADL-P and later platforms. > > Cc: Michal Wajdeczko > Signed-off-by: Rodrigo Vivi > Signed-off-by: John Harrison > Reviewed-by: Matthew Brost > --- > drivers/gpu/drm/i915/Makefile | 1 + > .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 1 + > .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h | 4 + > drivers/gpu/drm/i915/gt/uc/intel_guc.h| 3 + > .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 151 ++ > .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h | 19 +++ > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 + > 7 files changed, 185 insertions(+) > create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c > create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 6836b020a5be..ba9b6557d59d 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -192,6 +192,7 @@ i915-y += gt/uc/intel_uc.o \ > gt/uc/intel_guc_rc.o \ > gt/uc/intel_guc_slpc.o \ > gt/uc/intel_guc_submission.o \ > + gt/uc/intel_guc_hwconfig.o \ nit: I guess ordering of files (by name) is also desired in makefiles > gt/uc/intel_huc.o \ > gt/uc/intel_huc_debugfs.o \ > gt/uc/intel_huc_fw.o > diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h > b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h > index fe5d7d261797..4a61c819f32b 100644 > --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h > +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h > @@ -137,6 +137,7 @@ enum intel_guc_action { > INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009, > INTEL_GUC_ACTION_SETUP_PC_GUCRC = 0x3004, > INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000, > + INTEL_GUC_ACTION_GET_HWCONFIG = 0x4100, > INTEL_GUC_ACTION_REGISTER_CONTEXT = 0x4502, > INTEL_GUC_ACTION_DEREGISTER_CONTEXT = 0x4503, > INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505, > diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h > b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h > index 488b6061ee89..f9e2a6aaef4a 100644 > --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h > +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h > @@ -8,6 +8,10 @@ > > enum intel_guc_response_status { > INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0, > + INTEL_GUC_RESPONSE_NOT_SUPPORTED = 0x20, > + INTEL_GUC_RESPONSE_NO_ATTRIBUTE_TABLE = 0x201, > + INTEL_GUC_RESPONSE_NO_DECRYPTION_KEY = 0x202, > + INTEL_GUC_RESPONSE_DECRYPTION_FAILED = 0x204, > INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000, > }; > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > index f9240d4baa69..ce2ff4bb0fd5 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > @@ -13,6 +13,7 @@ > #include "intel_guc_fw.h" > #include "intel_guc_fwif.h" > #include "intel_guc_ct.h" > +#include "intel_guc_hwconfig.h" > #include "intel_guc_log.h" > #include "intel_guc_reg.h" > #include "intel_guc_slpc_types.h" > @@ -37,6 +38,8 @@ struct intel_guc { > struct intel_guc_ct ct; > /** @slpc: sub-structure containing SLPC related data and objects */ > struct intel_guc_slpc slpc; > + /** @hwconfig: hardware configuration KLV table */ nit: "@hwconfig: data related to hardware configuration KLV blob" > + struct intel_guc_hwconfig hwconfig; > > /** @sched_engine: Global engine used to submit requests to GuC */ > struct i915_sched_engine *sched_engine; > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c > new file mode 100644 > index ..ce6088f112d4 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c > @@ -0,0 +1,151 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2021 Intel Corporation 2022 ? > + */ > + > +#include "gt/intel_gt.h" > +#include "i915_drv.h" > +#include "i915_memcpy.h" > +#include "intel_guc_hwconfig.h" > + > +static inline struct intel_guc *hwconfig_to_guc(struct intel_guc_hwconfig > *hwconfig) no need for explicit "inline" > +{ > + return container_of(hwconfig, struct intel_guc, hwconfig); > +} > + > +/* > + * GuC has a blob containing hardware configuration information (HWConfig). > + * This is formatted as a simple and flexible KLV (Key/Length/Value) table. > + * > + * For example, a minimal version could be: > + * enum device_attr { > + * ATTR_SOME_VALUE = 0, > + * ATTR_SOME_MASK = 1, > + * }; > + * > + * static const
Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
On Tue, 2022-02-08 at 13:40 -0800, Abhinav Kumar wrote: > > > I am checking what usermode sees and will get back ( I didnt see an > error do most likely it was EOF ). I didnt follow the second part. I think probably it got -ENODEV, looking at kernfs_file_read_iter(). > If the file descriptor read returns EOF, even if we consider them > separate how will it resolve this issue? > > My earlier questions were related to fixing it in devcoredump to detect > and fix it there. Are you suggesting to fix in usermode instead? How? > Yeah, no, you cannot fix it in userspace. But I just followed the rabbit hole down kernfs and all, and it looks like indeed the read would be cut short with -ENODEV, sorry. It doesn't look like there's good API for this, but it seems at least from the underlying kernfs POV it should be possible to get_device() in open and put_device() in release, so that the device sticks around while somebody has the file open? It's entirely virtual, so this should be OK? johannes
Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
Hi Johannes On 2/8/2022 1:12 PM, Johannes Berg wrote: On Tue, 2022-02-08 at 13:04 -0800, Abhinav Kumar wrote: It opened the file rightaway but could not finish reading. The device gets deleted so the corresponding /data will disappear too ( as the data node is under devcd*/data) Yeah but even if the file disappears, the open file descriptor is still there, no? Does sysfs somehow make those disappear? I know debugfs does (now, to some extent, it didn't always), but I thought sysfs was refcounting things and didn't do that? What did the userspace actually see? read() returned 0 so EOF? (I guess I could test it, but it's getting late) Your other questions are related - you need to consider the file in sysfs and the open file descriptor separately. johannes I am checking what usermode sees and will get back ( I didnt see an error do most likely it was EOF ). I didnt follow the second part. If the file descriptor read returns EOF, even if we consider them separate how will it resolve this issue? My earlier questions were related to fixing it in devcoredump to detect and fix it there. Are you suggesting to fix in usermode instead? How? Thanks Abhinav
Re: [PATCH] drm/msm/dp: Add DisplayPort controller for SM8350
On Wed 19 Jan 15:14 PST 2022, Dmitry Baryshkov wrote: > On 28/12/2021 07:59, Bjorn Andersson wrote: > > The Qualcomm SM8350 platform comes with a single DisplayPort controller, > > add support for this in the DisplayPort driver. > > > > Signed-off-by: Bjorn Andersson > > Reviewed-by: Dmitry Baryshkov > I don't see this in linux-next, would it be possible to pick it up now that we're past the merge window etc? Regards, Bjorn > > --- > > .../devicetree/bindings/display/msm/dp-controller.yaml| 1 + > > drivers/gpu/drm/msm/dp/dp_display.c | 8 > > 2 files changed, 9 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > > b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > > index 5457612ab136..cd05cfd76536 100644 > > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > > @@ -21,6 +21,7 @@ properties: > > - qcom,sc7280-edp > > - qcom,sc8180x-dp > > - qcom,sc8180x-edp > > + - qcom,sm8350-dp > > reg: > > items: > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > > b/drivers/gpu/drm/msm/dp/dp_display.c > > index 8d9c19dbf33e..fd0fd03f8fed 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > @@ -143,10 +143,18 @@ static const struct msm_dp_config sc7280_dp_cfg = { > > .num_descs = 2, > > }; > > +static const struct msm_dp_config sm8350_dp_cfg = { > > + .descs = (const struct msm_dp_desc[]) { > > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae9, > > .connector_type = DRM_MODE_CONNECTOR_DisplayPort }, > > + }, > > + .num_descs = 1, > > +}; > > + > > static const struct of_device_id dp_dt_match[] = { > > { .compatible = "qcom,sc7180-dp", .data = _dp_cfg }, > > { .compatible = "qcom,sc7280-dp", .data = _dp_cfg }, > > { .compatible = "qcom,sc7280-edp", .data = _dp_cfg }, > > + { .compatible = "qcom,sm8350-dp", .data = _dp_cfg }, > > {} > > }; > > > -- > With best wishes > Dmitry
Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
On Tue, 2022-02-08 at 13:04 -0800, Abhinav Kumar wrote: > > It opened the file rightaway but could not finish reading. > > The device gets deleted so the corresponding /data will disappear too ( > as the data node is under devcd*/data) Yeah but even if the file disappears, the open file descriptor is still there, no? Does sysfs somehow make those disappear? I know debugfs does (now, to some extent, it didn't always), but I thought sysfs was refcounting things and didn't do that? What did the userspace actually see? read() returned 0 so EOF? (I guess I could test it, but it's getting late) Your other questions are related - you need to consider the file in sysfs and the open file descriptor separately. johannes
[PATCH v2 17/19] fbcon: Maintain a private array of fb_info
Accessing the one in fbmem.c without taking the right locks is a bad idea. Instead maintain our own private copy, which is fully protected by console_lock() (like everything else in fbcon.c). That copy is serialized through fbcon_fb_registered/unregistered() calls. Also this means we do not need to hold a full fb_info reference, which is nice because doing so would mean a refcount loop between the console and the fb_info. But it's also not nice since it means console_lock() must be held absolutely everywhere. Well strictly speaking we could still try to do some refcounting games again by calling get_fb_info before we drop the console_lock. But things will get tricky. Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Tetsuo Handa Cc: Claudio Suarez Cc: Du Cheng Cc: Greg Kroah-Hartman --- drivers/video/fbdev/core/fbcon.c | 82 +--- 1 file changed, 43 insertions(+), 39 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 944f514c77ec..6a7d470beec7 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -86,10 +86,6 @@ * - fbcon state itself is protected by the console_lock, and the code does a * pretty good job at making sure that lock is held everywhere it's needed. * - * - access to the registered_fb array is entirely unprotected. This should use - * proper object lifetime handling, i.e. get/put_fb_info. This also means - * switching from indices to proper pointers for fb_info everywhere. - * * - fbcon doesn't bother with fb_lock/unlock at all. This is buggy, since it * means concurrent access to the same fbdev from both fbcon and userspace * will blow up. To fix this all fbcon calls from fbmem.c need to be moved out @@ -107,6 +103,13 @@ enum { static struct fbcon_display fb_display[MAX_NR_CONSOLES]; +struct fb_info *fbcon_registered_fb[FB_MAX]; +int fbcon_num_registered_fb; + +#define fbcon_for_each_registered_fb(i)\ + for (i = 0; WARN_CONSOLE_UNLOCKED(), i < FB_MAX; i++) \ + if (!fbcon_registered_fb[i]) {} else + static signed char con2fb_map[MAX_NR_CONSOLES]; static signed char con2fb_map_boot[MAX_NR_CONSOLES]; @@ -114,12 +117,7 @@ static struct fb_info *fbcon_info_from_console(int console) { WARN_CONSOLE_UNLOCKED(); - /* -* Note that only con2fb_map is protected by the console lock, -* registered_fb is protected by a separate mutex. This lookup can -* therefore race. -*/ - return registered_fb[con2fb_map[console]]; + return fbcon_registered_fb[con2fb_map[console]]; } static int logo_lines; @@ -518,7 +516,7 @@ static int do_fbcon_takeover(int show_logo) { int err, i; - if (!num_registered_fb) + if (!fbcon_num_registered_fb) return -ENODEV; if (!show_logo) @@ -821,7 +819,7 @@ static int set_con2fb_map(int unit, int newidx, int user) { struct vc_data *vc = vc_cons[unit].d; int oldidx = con2fb_map[unit]; - struct fb_info *info = registered_fb[newidx]; + struct fb_info *info = fbcon_registered_fb[newidx]; struct fb_info *oldinfo = NULL; int found, err = 0, show_logo; @@ -839,7 +837,7 @@ static int set_con2fb_map(int unit, int newidx, int user) } if (oldidx != -1) - oldinfo = registered_fb[oldidx]; + oldinfo = fbcon_registered_fb[oldidx]; found = search_fb_in_map(newidx); @@ -931,13 +929,13 @@ static const char *fbcon_startup(void) * If num_registered_fb is zero, this is a call for the dummy part. * The frame buffer devices weren't initialized yet. */ - if (!num_registered_fb || info_idx == -1) + if (!fbcon_num_registered_fb || info_idx == -1) return display_desc; /* * Instead of blindly using registered_fb[0], we use info_idx, set by * fbcon_fb_registered(); */ - info = registered_fb[info_idx]; + info = fbcon_registered_fb[info_idx]; if (!info) return NULL; @@ -1150,9 +1148,9 @@ static void fbcon_release_all(void) struct fb_info *info; int i, j, mapped; - for_each_registered_fb(i) { + fbcon_for_each_registered_fb(i) { mapped = 0; - info = registered_fb[i]; + info = fbcon_registered_fb[i]; for (j = first_fb_vc; j <= last_fb_vc; j++) { if (con2fb_map[j] == i) { @@ -1179,7 +1177,7 @@ static void fbcon_deinit(struct vc_data *vc) if (idx == -1) goto finished; - info = registered_fb[idx]; + info = fbcon_registered_fb[idx]; if (!info) goto finished; @@ -2098,9 +2096,9 @@ static int fbcon_switch(struct vc_data *vc) * * info->currcon = vc->vc_num;
[PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee. With commit 27599aacbaefcbf2af7b06b0029459bbf682000d Author: Thomas Zimmermann Date: Tue Jan 25 10:12:18 2022 +0100 fbdev: Hot-unplug firmware fb devices on forced removal this should be fixed properly and we can remove this somewhat hackish check here (e.g. this won't catch drm drivers if fbdev emulation isn't enabled). Cc: Thomas Zimmermann Cc: Zack Rusin Cc: Javier Martinez Canillas Cc: Zack Rusin Cc: Hans de Goede Cc: Ilya Trukhanov Signed-off-by: Daniel Vetter Cc: Peter Jones Cc: linux-fb...@vger.kernel.org --- drivers/video/fbdev/efifb.c| 11 --- drivers/video/fbdev/simplefb.c | 11 --- 2 files changed, 22 deletions(-) diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index ea42ba6445b2..edca3703b964 100644 --- a/drivers/video/fbdev/efifb.c +++ b/drivers/video/fbdev/efifb.c @@ -351,17 +351,6 @@ static int efifb_probe(struct platform_device *dev) char *option = NULL; efi_memory_desc_t md; - /* -* Generic drivers must not be registered if a framebuffer exists. -* If a native driver was probed, the display hardware was already -* taken and attempting to use the system framebuffer is dangerous. -*/ - if (num_registered_fb > 0) { - dev_err(>dev, - "efifb: a framebuffer is already registered\n"); - return -EINVAL; - } - if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled) return -ENODEV; diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c index 94fc9c6d0411..0ef41173325a 100644 --- a/drivers/video/fbdev/simplefb.c +++ b/drivers/video/fbdev/simplefb.c @@ -413,17 +413,6 @@ static int simplefb_probe(struct platform_device *pdev) struct simplefb_par *par; struct resource *res, *mem; - /* -* Generic drivers must not be registered if a framebuffer exists. -* If a native driver was probed, the display hardware was already -* taken and attempting to use the system framebuffer is dangerous. -*/ - if (num_registered_fb > 0) { - dev_err(>dev, - "simplefb: a framebuffer is already registered\n"); - return -EINVAL; - } - if (fb_get_options("simplefb", NULL)) return -ENODEV; -- 2.34.1
[PATCH v2 14/19] fbcon: Move console_lock for register/unlink/unregister
Ideally console_lock becomes an implementation detail of fbcon.c and doesn't show up anywhere in fbmem.c. We're still pretty far from that, but at least the register/unregister code is there now. With this the do_fb_ioctl() handler is the only code in fbmem.c still calling console_lock(). Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Thomas Zimmermann Cc: Du Cheng Cc: Claudio Suarez Cc: Greg Kroah-Hartman Cc: Tetsuo Handa Cc: Matthew Wilcox Cc: Sam Ravnborg Cc: Zheyu Ma Cc: Guenter Roeck Cc: Alex Deucher Cc: Zhen Lei Cc: Xiyu Yang --- drivers/video/fbdev/core/fbcon.c | 33 ++-- drivers/video/fbdev/core/fbmem.c | 23 ++ 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 4f9752ee9189..abb419a091c6 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -2756,10 +2756,12 @@ void fbcon_fb_unbind(struct fb_info *info) int i, new_idx = -1; int idx = info->node; - WARN_CONSOLE_UNLOCKED(); + console_lock(); - if (!fbcon_has_console_bind) + if (!fbcon_has_console_bind) { + console_unlock(); return; + } for (i = first_fb_vc; i <= last_fb_vc; i++) { if (con2fb_map[i] != idx && @@ -2794,6 +2796,8 @@ void fbcon_fb_unbind(struct fb_info *info) } fbcon_unbind(); } + + console_unlock(); } /* called with console_lock held */ @@ -2801,10 +2805,12 @@ void fbcon_fb_unregistered(struct fb_info *info) { int i, idx; - WARN_CONSOLE_UNLOCKED(); + console_lock(); - if (deferred_takeover) + if (deferred_takeover) { + console_unlock(); return; + } idx = info->node; for (i = first_fb_vc; i <= last_fb_vc; i++) { @@ -2833,6 +2839,7 @@ void fbcon_fb_unregistered(struct fb_info *info) if (!num_registered_fb) do_unregister_con_driver(_con); + console_unlock(); } void fbcon_remap_all(struct fb_info *info) @@ -2890,19 +2897,27 @@ static inline void fbcon_select_primary(struct fb_info *info) } #endif /* CONFIG_FRAMEBUFFER_DETECT_PRIMARY */ +static bool lockless_register_fb; +module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool, 0400); +MODULE_PARM_DESC(lockless_register_fb, + "Lockless framebuffer registration for debugging [default=off]"); + /* called with console_lock held */ int fbcon_fb_registered(struct fb_info *info) { int ret = 0, i, idx; - WARN_CONSOLE_UNLOCKED(); + if (!lockless_register_fb) + console_lock(); + else + atomic_inc(_console_lock_warning); idx = info->node; fbcon_select_primary(info); if (deferred_takeover) { pr_info("fbcon: Deferring console take-over\n"); - return 0; + goto out; } if (info_idx == -1) { @@ -2922,6 +2937,12 @@ int fbcon_fb_registered(struct fb_info *info) } } +out: + if (!lockless_register_fb) + console_unlock(); + else + atomic_dec(_console_lock_warning); + return ret; } diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 37656883e7bd..6f6f7a763969 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1594,14 +1594,9 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, } } -static bool lockless_register_fb; -module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool, 0400); -MODULE_PARM_DESC(lockless_register_fb, - "Lockless framebuffer registration for debugging [default=off]"); - static int do_register_framebuffer(struct fb_info *fb_info) { - int i, ret; + int i; struct fb_videomode mode; if (fb_check_foreignness(fb_info)) @@ -1670,17 +1665,7 @@ static int do_register_framebuffer(struct fb_info *fb_info) } #endif - if (!lockless_register_fb) - console_lock(); - else - atomic_inc(_console_lock_warning); - ret = fbcon_fb_registered(fb_info); - - if (!lockless_register_fb) - console_unlock(); - else - atomic_dec(_console_lock_warning); - return ret; + return fbcon_fb_registered(fb_info); } static void unbind_console(struct fb_info *fb_info) @@ -1690,9 +1675,7 @@ static void unbind_console(struct fb_info *fb_info) if (WARN_ON(i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)) return; - console_lock(); fbcon_fb_unbind(fb_info); - console_unlock(); } static void unlink_framebuffer(struct fb_info *fb_info) @@ -1735,9 +1718,7 @@ static void
[PATCH v2 16/19] fbcon: untangle fbcon_exit
There's a bunch of confusions going on here: - The deferred fbcon setup notifier should only be cleaned up from fb_console_exit(), to be symmetric with fb_console_init() - We also need to make sure we don't race with the work, which means temporarily dropping the console lock (or we can deadlock) - That also means no point in clearing deferred_takeover, we are unloading everything anyway. - Finally rename fbcon_exit to fbcon_release_all and move it, since that's what's it doing when being called from consw->con_deinit through fbcon_deinit. To answer a question from Sam just quoting my own reply: > We loose the call to fbcon_release_all() here [in fb_console_exit()]. > We have part of the old fbcon_exit() above, but miss the release parts. Ah yes that's the entire point of this change. The release_all in the fbcon exit path was only needed when fbcon was a separate module indepedent from core fb.ko. Which means it was possible to unload fbcon while having fbdev drivers registered. But since we've merged them that has become impossible, so by the time the fb.ko module can be unloaded, there's guaranteed to be no fbdev drivers left. And hence removing them is pointless. v2: Explain the why better (Sam) Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Greg Kroah-Hartman Cc: Claudio Suarez Cc: Du Cheng Cc: Tetsuo Handa --- drivers/video/fbdev/core/fbcon.c | 63 1 file changed, 32 insertions(+), 31 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 685b4a9e5546..944f514c77ec 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -187,7 +187,6 @@ static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p, int line, int count, int dy); static void fbcon_modechanged(struct fb_info *info); static void fbcon_set_all_vcs(struct fb_info *info); -static void fbcon_exit(void); static struct device *fbcon_device; @@ -1146,6 +1145,27 @@ static void fbcon_free_font(struct fbcon_display *p, bool freefont) static void set_vc_hi_font(struct vc_data *vc, bool set); +static void fbcon_release_all(void) +{ + struct fb_info *info; + int i, j, mapped; + + for_each_registered_fb(i) { + mapped = 0; + info = registered_fb[i]; + + for (j = first_fb_vc; j <= last_fb_vc; j++) { + if (con2fb_map[j] == i) { + mapped = 1; + con2fb_map[j] = -1; + } + } + + if (mapped) + fbcon_release(info); + } +} + static void fbcon_deinit(struct vc_data *vc) { struct fbcon_display *p = _display[vc->vc_num]; @@ -1185,7 +1205,7 @@ static void fbcon_deinit(struct vc_data *vc) set_vc_hi_font(vc, false); if (!con_is_bound(_con)) - fbcon_exit(); + fbcon_release_all(); if (vc->vc_num == logo_shown) logo_shown = FBCON_LOGO_CANSHOW; @@ -3296,34 +3316,6 @@ static void fbcon_start(void) #endif } -static void fbcon_exit(void) -{ - struct fb_info *info; - int i, j, mapped; - -#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER - if (deferred_takeover) { - dummycon_unregister_output_notifier(_output_nb); - deferred_takeover = false; - } -#endif - - for_each_registered_fb(i) { - mapped = 0; - info = registered_fb[i]; - - for (j = first_fb_vc; j <= last_fb_vc; j++) { - if (con2fb_map[j] == i) { - mapped = 1; - con2fb_map[j] = -1; - } - } - - if (mapped) - fbcon_release(info); - } -} - void __init fb_console_init(void) { int i; @@ -3363,10 +3355,19 @@ static void __exit fbcon_deinit_device(void) void __exit fb_console_exit(void) { +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER + console_lock(); + if (deferred_takeover) + dummycon_unregister_output_notifier(_output_nb); + console_unlock(); + + cancel_work_sync(_deferred_takeover_work); +#endif + console_lock(); fbcon_deinit_device(); device_destroy(fb_class, MKDEV(0, 0)); - fbcon_exit(); + do_unregister_con_driver(_con); console_unlock(); } -- 2.34.1
[PATCH v2 15/19] fbcon: Move more code into fbcon_release
con2fb_release_oldinfo() has a bunch more kfree() calls than fbcon_exit(), but since kfree() on NULL is harmless doing that in both places should be ok. This is also a bit more symmetric now again with fbcon_open also allocating the fbcon_ops structure. Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Tetsuo Handa Cc: Greg Kroah-Hartman Cc: Du Cheng Cc: Claudio Suarez --- drivers/video/fbdev/core/fbcon.c | 33 +--- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index abb419a091c6..685b4a9e5546 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -690,6 +690,18 @@ static void fbcon_release(struct fb_info *info) unlock_fb_info(info); module_put(info->fbops->owner); + + if (info->fbcon_par) { + struct fbcon_ops *ops = info->fbcon_par; + + fbcon_del_cursor_work(info); + kfree(ops->cursor_state.mask); + kfree(ops->cursor_data); + kfree(ops->cursor_src); + kfree(ops->fontbuffer); + kfree(info->fbcon_par); + info->fbcon_par = NULL; + } } static int fbcon_open(struct fb_info *info) @@ -740,18 +752,10 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, static void con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, struct fb_info *newinfo) { - struct fbcon_ops *ops = oldinfo->fbcon_par; int ret; fbcon_release(oldinfo); - fbcon_del_cursor_work(oldinfo); - kfree(ops->cursor_state.mask); - kfree(ops->cursor_data); - kfree(ops->cursor_src); - kfree(ops->fontbuffer); - kfree(oldinfo->fbcon_par); - oldinfo->fbcon_par = NULL; /* If oldinfo and newinfo are driving the same hardware, the fb_release() method of oldinfo may attempt to @@ -3315,19 +3319,8 @@ static void fbcon_exit(void) } } - if (mapped) { - if (info->fbcon_par) { - struct fbcon_ops *ops = info->fbcon_par; - - fbcon_del_cursor_work(info); - kfree(ops->cursor_src); - kfree(ops->cursor_state.mask); - kfree(info->fbcon_par); - info->fbcon_par = NULL; - } - + if (mapped) fbcon_release(info); - } } } -- 2.34.1
[PATCH v2 19/19] fbdev: Make registered_fb[] private to fbmem.c
Well except when the olpc dcon fbdev driver is enabled, that thing digs around in there in rather unfixable ways. Cc oldc_dcon maintainers as fyi. v2: I typoed the config name (0day) Cc: kernel test robot Cc: Jens Frederich Cc: Jon Nettleton Cc: Greg Kroah-Hartman Cc: linux-stag...@lists.linux.dev Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Helge Deller Cc: Matthew Wilcox Cc: Sam Ravnborg Cc: Tetsuo Handa Cc: Zhen Lei Cc: Alex Deucher Cc: Xiyu Yang Cc: linux-fb...@vger.kernel.org Cc: Zheyu Ma Cc: Guenter Roeck --- drivers/video/fbdev/core/fbmem.c | 8 ++-- include/linux/fb.h | 7 +++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 6f6f7a763969..6f0eb596a2cd 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -49,10 +49,14 @@ static DEFINE_MUTEX(registration_lock); struct fb_info *registered_fb[FB_MAX] __read_mostly; -EXPORT_SYMBOL(registered_fb); - int num_registered_fb __read_mostly; +#if IS_ENABLED(CONFIG_FB_OLPC_DCON) +EXPORT_SYMBOL(registered_fb); EXPORT_SYMBOL(num_registered_fb); +#endif +#define for_each_registered_fb(i) \ + for (i = 0; i < FB_MAX; i++)\ + if (!registered_fb[i]) {} else bool fb_center_logo __read_mostly; diff --git a/include/linux/fb.h b/include/linux/fb.h index 23b19cf8bccd..afaa1474a283 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -623,16 +623,15 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var, extern int fb_get_options(const char *name, char **option); extern int fb_new_modelist(struct fb_info *info); +#if IS_ENABLED(CONFIG_FB_OLPC_DCON) extern struct fb_info *registered_fb[FB_MAX]; + extern int num_registered_fb; +#endif extern bool fb_center_logo; extern int fb_logo_count; extern struct class *fb_class; -#define for_each_registered_fb(i) \ - for (i = 0; i < FB_MAX; i++)\ - if (!registered_fb[i]) {} else - static inline void lock_fb_info(struct fb_info *info) { mutex_lock(>lock); -- 2.34.1
[PATCH v2 13/19] fbcon: Consistently protect deferred_takeover with console_lock()
This shouldn't be a problem in practice since until we've actually taken over the console there's nothing we've registered with the console/vt subsystem, so the exit/unbind path that check this can't do the wrong thing. But it's confusing, so fix it by moving it a tad later. Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Du Cheng Cc: Tetsuo Handa Cc: Claudio Suarez Cc: Thomas Zimmermann --- drivers/video/fbdev/core/fbcon.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index cc960bf49991..4f9752ee9189 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -3227,6 +3227,9 @@ static void fbcon_register_existing_fbs(struct work_struct *work) console_lock(); + deferred_takeover = false; + logo_shown = FBCON_LOGO_DONTSHOW; + for_each_registered_fb(i) fbcon_fb_registered(registered_fb[i]); @@ -3244,8 +3247,6 @@ static int fbcon_output_notifier(struct notifier_block *nb, pr_info("fbcon: Taking over console\n"); dummycon_unregister_output_notifier(_output_nb); - deferred_takeover = false; - logo_shown = FBCON_LOGO_DONTSHOW; /* We may get called in atomic context */ schedule_work(_deferred_takeover_work); -- 2.34.1
[PATCH v2 12/19] fbcon: use lock_fb_info in fbcon_open/release
Now we get to the real motiviation, because fbmem.c insists that that's the right lock for these. Ofc fbcon.c has a lot more places where it probably should call lock_fb_info(). But looking at fbmem.c at least most of these seem to be protected by console_lock() too, which is probably what papers over any issues. Note that this means we're shuffling around a bit the locking sections for some of the console takeover and unbind paths, but not all: - console binding/unbinding from the console layer never with lock_fb_info - unbind (as opposed to unlink) never bother with lock_fb_info Also the real serialization against set_par and set_pan are still doing by wrapping the entire ioctl code in console_lock(). So this shuffling shouldn't be worse than what we had from a "can you trigger races?" pov, but it's at least clearer. Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Claudio Suarez Cc: Tetsuo Handa Cc: Thomas Zimmermann Cc: Greg Kroah-Hartman Cc: Du Cheng Cc: Sam Ravnborg Cc: Matthew Wilcox Cc: William Kucharski Cc: Alex Deucher Cc: Zheyu Ma Cc: Zhen Lei Cc: Xiyu Yang --- drivers/video/fbdev/core/fbcon.c | 5 + drivers/video/fbdev/core/fbmem.c | 4 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index f0213a0e3870..cc960bf49991 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -684,8 +684,10 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) static void fbcon_release(struct fb_info *info) { + lock_fb_info(info); if (info->fbops->fb_release) info->fbops->fb_release(info, 0); + unlock_fb_info(info); module_put(info->fbops->owner); } @@ -697,11 +699,14 @@ static int fbcon_open(struct fb_info *info) if (!try_module_get(info->fbops->owner)) return -ENODEV; + lock_fb_info(info); if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) { + unlock_fb_info(info); module_put(info->fbops->owner); return -ENODEV; } + unlock_fb_info(info); ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); if (!ops) { diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index ad9aac06427a..37656883e7bd 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1674,9 +1674,7 @@ static int do_register_framebuffer(struct fb_info *fb_info) console_lock(); else atomic_inc(_console_lock_warning); - lock_fb_info(fb_info); ret = fbcon_fb_registered(fb_info); - unlock_fb_info(fb_info); if (!lockless_register_fb) console_unlock(); @@ -1693,9 +1691,7 @@ static void unbind_console(struct fb_info *fb_info) return; console_lock(); - lock_fb_info(fb_info); fbcon_fb_unbind(fb_info); - unlock_fb_info(fb_info); console_unlock(); } -- 2.34.1
[PATCH v2 08/19] fb: Delete fb_info->queue
It was only used by fbcon, and that now switched to its own, private work. Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Helge Deller Cc: linux-fb...@vger.kernel.org --- include/linux/fb.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/linux/fb.h b/include/linux/fb.h index 3d7306c9a706..23b19cf8bccd 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -449,7 +449,6 @@ struct fb_info { struct fb_var_screeninfo var; /* Current var */ struct fb_fix_screeninfo fix; /* Current fix */ struct fb_monspecs monspecs;/* Current Monitor specs */ - struct work_struct queue; /* Framebuffer event queue */ struct fb_pixmap pixmap;/* Image hardware mapper */ struct fb_pixmap sprite;/* Cursor hardware mapper */ struct fb_cmap cmap;/* Current cmap */ -- 2.34.1
[PATCH v2 11/19] fbcon: move more common code into fb_open()
No idea why con2fb_acquire_newinfo() initializes much less than fbcon_startup(), but so be it. From a quick look most of the un-initialized stuff should be fairly harmless, but who knows. Note that the error handling for the con2fb_acquire_newinfo() failure case was very strange: Callers updated con2fb_map to the new value before calling this function, but upon error con2fb_acquire_newinfo reset it to the old value. Since I removed the call to fbcon_release anyway that strange error path was sticking out like a sore thumb, hence I removed it. Which also allows us to remove the oldidx parameter from that function. v2: Explain what's going on with oldidx and error paths (Sam) v3: Drop unused variable (0day) Acked-by: Sam Ravnborg (v2) Cc: kernel test robot Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Greg Kroah-Hartman Cc: Tetsuo Handa Cc: Thomas Zimmermann Cc: Claudio Suarez Cc: Du Cheng --- drivers/video/fbdev/core/fbcon.c | 75 +--- 1 file changed, 30 insertions(+), 45 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index a60891005d44..f0213a0e3870 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -682,8 +682,18 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) #endif /* CONFIG_MISC_TILEBLITTING */ +static void fbcon_release(struct fb_info *info) +{ + if (info->fbops->fb_release) + info->fbops->fb_release(info, 0); + + module_put(info->fbops->owner); +} + static int fbcon_open(struct fb_info *info) { + struct fbcon_ops *ops; + if (!try_module_get(info->fbops->owner)) return -ENODEV; @@ -693,48 +703,31 @@ static int fbcon_open(struct fb_info *info) return -ENODEV; } - return 0; -} + ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); + if (!ops) { + fbcon_release(info); + return -ENOMEM; + } -static void fbcon_release(struct fb_info *info) -{ - if (info->fbops->fb_release) - info->fbops->fb_release(info, 0); + INIT_DELAYED_WORK(>cursor_work, fb_flashcursor); + ops->info = info; + info->fbcon_par = ops; + ops->cur_blink_jiffies = HZ / 5; - module_put(info->fbops->owner); + return 0; } static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, - int unit, int oldidx) + int unit) { - struct fbcon_ops *ops = NULL; int err; err = fbcon_open(info); if (err) return err; - if (!err) { - ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); - if (!ops) - err = -ENOMEM; - - INIT_DELAYED_WORK(>cursor_work, fb_flashcursor); - } - - if (!err) { - ops->cur_blink_jiffies = HZ / 5; - ops->info = info; - info->fbcon_par = ops; - - if (vc) - set_blitting_type(vc, info); - } - - if (err) { - con2fb_map[unit] = oldidx; - fbcon_release(info); - } + if (vc) + set_blitting_type(vc, info); return err; } @@ -842,9 +835,11 @@ static int set_con2fb_map(int unit, int newidx, int user) found = search_fb_in_map(newidx); - con2fb_map[unit] = newidx; - if (!err && !found) - err = con2fb_acquire_newinfo(vc, info, unit, oldidx); + if (!err && !found) { + err = con2fb_acquire_newinfo(vc, info, unit); + if (!err) + con2fb_map[unit] = newidx; + } /* * If old fb is not mapped to any of the consoles, @@ -941,20 +936,10 @@ static const char *fbcon_startup(void) if (fbcon_open(info)) return NULL; - ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); - if (!ops) { - fbcon_release(info); - return NULL; - } - - INIT_DELAYED_WORK(>cursor_work, fb_flashcursor); - + ops = info->fbcon_par; ops->currcon = -1; ops->graphics = 1; ops->cur_rotate = -1; - ops->cur_blink_jiffies = HZ / 5; - ops->info = info; - info->fbcon_par = ops; p->con_rotate = initial_rotation; if (p->con_rotate == -1) @@ -1024,7 +1009,7 @@ static void fbcon_init(struct vc_data *vc, int init) return; if (!info->fbcon_par) - con2fb_acquire_newinfo(vc, info, vc->vc_num, -1); + con2fb_acquire_newinfo(vc, info, vc->vc_num); /* If we are not the first console on this fb, copy the font from that console */ -- 2.34.1
[PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers
There's two minor behaviour changes in here: - in error paths we now consistently call fb_ops->fb_release - fb_release really can't fail (fbmem.c ignores it too) and there's no reasonable cleanup we can do anyway. Note that everything in fbcon.c is protected by the big console_lock() lock (especially all the global variables), so the minor changes in ordering of setup/cleanup do not matter. v2: Explain a bit better why this is all correct (Sam) Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Claudio Suarez Cc: Greg Kroah-Hartman Cc: Tetsuo Handa Cc: Du Cheng --- drivers/video/fbdev/core/fbcon.c | 107 +++ 1 file changed, 53 insertions(+), 54 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 058e885d24f6..3e1a3e7bf527 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) #endif /* CONFIG_MISC_TILEBLITTING */ +static int fbcon_open(struct fb_info *info) +{ + if (!try_module_get(info->fbops->owner)) + return -ENODEV; + + if (info->fbops->fb_open && + info->fbops->fb_open(info, 0)) { + module_put(info->fbops->owner); + return -ENODEV; + } + + return 0; +} + +static void fbcon_release(struct fb_info *info) +{ + if (info->fbops->fb_release) + info->fbops->fb_release(info, 0); + + module_put(info->fbops->owner); +} static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, int unit, int oldidx) { struct fbcon_ops *ops = NULL; - int err = 0; - - if (!try_module_get(info->fbops->owner)) - err = -ENODEV; + int err; - if (!err && info->fbops->fb_open && - info->fbops->fb_open(info, 0)) - err = -ENODEV; + err = fbcon_open(info); + if (err) + return err; if (!err) { ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); @@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, if (err) { con2fb_map[unit] = oldidx; - module_put(info->fbops->owner); + fbcon_release(info); } return err; @@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, int oldidx, int found) { struct fbcon_ops *ops = oldinfo->fbcon_par; - int err = 0, ret; + int ret; - if (oldinfo->fbops->fb_release && - oldinfo->fbops->fb_release(oldinfo, 0)) { - con2fb_map[unit] = oldidx; - if (!found && newinfo->fbops->fb_release) - newinfo->fbops->fb_release(newinfo, 0); - if (!found) - module_put(newinfo->fbops->owner); - err = -ENODEV; - } + fbcon_release(oldinfo); - if (!err) { - fbcon_del_cursor_work(oldinfo); - kfree(ops->cursor_state.mask); - kfree(ops->cursor_data); - kfree(ops->cursor_src); - kfree(ops->fontbuffer); - kfree(oldinfo->fbcon_par); - oldinfo->fbcon_par = NULL; - module_put(oldinfo->fbops->owner); - /* - If oldinfo and newinfo are driving the same hardware, - the fb_release() method of oldinfo may attempt to - restore the hardware state. This will leave the - newinfo in an undefined state. Thus, a call to - fb_set_par() may be needed for the newinfo. - */ - if (newinfo && newinfo->fbops->fb_set_par) { - ret = newinfo->fbops->fb_set_par(newinfo); + fbcon_del_cursor_work(oldinfo); + kfree(ops->cursor_state.mask); + kfree(ops->cursor_data); + kfree(ops->cursor_src); + kfree(ops->fontbuffer); + kfree(oldinfo->fbcon_par); + oldinfo->fbcon_par = NULL; + /* + If oldinfo and newinfo are driving the same hardware, + the fb_release() method of oldinfo may attempt to + restore the hardware state. This will leave the + newinfo in an undefined state. Thus, a call to + fb_set_par() may be needed for the newinfo. + */ + if (newinfo && newinfo->fbops->fb_set_par) { + ret = newinfo->fbops->fb_set_par(newinfo); - if (ret) - printk(KERN_ERR "con2fb_release_oldinfo: " - "detected unhandled fb_set_par error, " - "error code %d\n", ret); - } + if (ret) +
[PATCH v2 10/19] fbcon: Ditch error handling for con2fb_release_oldinfo
It doesn't ever fail anymore. Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Thomas Zimmermann Cc: Greg Kroah-Hartman Cc: Claudio Suarez Cc: Du Cheng Cc: Tetsuo Handa --- drivers/video/fbdev/core/fbcon.c | 37 +++- 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 3e1a3e7bf527..a60891005d44 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -739,9 +739,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, return err; } -static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, - struct fb_info *newinfo, int unit, - int oldidx, int found) +static void con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, + struct fb_info *newinfo) { struct fbcon_ops *ops = oldinfo->fbcon_par; int ret; @@ -770,8 +769,6 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, "detected unhandled fb_set_par error, " "error code %d\n", ret); } - - return 0; } static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, @@ -825,7 +822,7 @@ static int set_con2fb_map(int unit, int newidx, int user) int oldidx = con2fb_map[unit]; struct fb_info *info = registered_fb[newidx]; struct fb_info *oldinfo = NULL; - int found, err = 0; + int found, err = 0, show_logo; WARN_CONSOLE_UNLOCKED(); @@ -854,18 +851,15 @@ static int set_con2fb_map(int unit, int newidx, int user) * fbcon should release it. */ if (!err && oldinfo && !search_fb_in_map(oldidx)) - err = con2fb_release_oldinfo(vc, oldinfo, info, unit, oldidx, -found); + con2fb_release_oldinfo(vc, oldinfo, info); - if (!err) { - int show_logo = (fg_console == 0 && !user && -logo_shown != FBCON_LOGO_DONTSHOW); + show_logo = (fg_console == 0 && !user && +logo_shown != FBCON_LOGO_DONTSHOW); - if (!found) - fbcon_add_cursor_work(info); - con2fb_map_boot[unit] = newidx; - con2fb_init_display(vc, info, unit, show_logo); - } + if (!found) + fbcon_add_cursor_work(info); + con2fb_map_boot[unit] = newidx; + con2fb_init_display(vc, info, unit, show_logo); if (!search_fb_in_map(info_idx)) info_idx = newidx; @@ -2769,7 +2763,7 @@ static inline void fbcon_unbind(void) {} /* called with console_lock held */ void fbcon_fb_unbind(struct fb_info *info) { - int i, new_idx = -1, ret = 0; + int i, new_idx = -1; int idx = info->node; WARN_CONSOLE_UNLOCKED(); @@ -2803,13 +2797,8 @@ void fbcon_fb_unbind(struct fb_info *info) if (con2fb_map[i] == idx) { con2fb_map[i] = -1; if (!search_fb_in_map(idx)) { - ret = con2fb_release_oldinfo(vc_cons[i].d, -info, NULL, i, -idx, 0); - if (ret) { - con2fb_map[i] = idx; - return; - } + con2fb_release_oldinfo(vc_cons[i].d, + info, NULL); } } } -- 2.34.1
[PATCH v2 07/19] fbcon: Replace FBCON_FLAGS_INIT with a boolean
It's only one flag and slightly tidier code. Acked-by: Thomas Zimmermann Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Tetsuo Handa Cc: Greg Kroah-Hartman Cc: Du Cheng Cc: Thomas Zimmermann Cc: Claudio Suarez --- drivers/video/fbdev/core/fbcon.c | 11 +-- drivers/video/fbdev/core/fbcon.h | 4 +--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index a368ed602e2e..058e885d24f6 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -775,7 +775,7 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, ops->currcon = fg_console; - if (info->fbops->fb_set_par && !(ops->flags & FBCON_FLAGS_INIT)) { + if (info->fbops->fb_set_par && !ops->initialized) { ret = info->fbops->fb_set_par(info); if (ret) @@ -784,7 +784,7 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, "error code %d\n", ret); } - ops->flags |= FBCON_FLAGS_INIT; + ops->initialized = true; ops->graphics = 0; fbcon_set_disp(info, >var, unit); @@ -1103,8 +1103,7 @@ static void fbcon_init(struct vc_data *vc, int init) * We need to do it in fbcon_init() to prevent screen corruption. */ if (con_is_visible(vc) && vc->vc_mode == KD_TEXT) { - if (info->fbops->fb_set_par && - !(ops->flags & FBCON_FLAGS_INIT)) { + if (info->fbops->fb_set_par && !ops->initialized) { ret = info->fbops->fb_set_par(info); if (ret) @@ -1113,7 +1112,7 @@ static void fbcon_init(struct vc_data *vc, int init) "error code %d\n", ret); } - ops->flags |= FBCON_FLAGS_INIT; + ops->initialized = true; } ops->graphics = 0; @@ -1186,7 +1185,7 @@ static void fbcon_deinit(struct vc_data *vc) if (con_is_visible(vc)) fbcon_del_cursor_work(info); - ops->flags &= ~FBCON_FLAGS_INIT; + ops->initialized = false; finished: fbcon_free_font(p, free_font); diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h index 6708ca0048aa..0eaf54a21151 100644 --- a/drivers/video/fbdev/core/fbcon.h +++ b/drivers/video/fbdev/core/fbcon.h @@ -18,8 +18,6 @@ #include -#define FBCON_FLAGS_INIT 1 - /* *This is the interface between the low-level console driver and the *low-level frame buffer device @@ -79,7 +77,7 @@ struct fbcon_ops { intblank_state; intgraphics; intsave_graphics; /* for debug enter/leave */ - intflags; + bool initialized; introtate; intcur_rotate; char *cursor_data; -- 2.34.1
[PATCH v2 06/19] fbcon: Use delayed work for cursor
Allows us to delete a bunch of hand-rolled stuff. Also to simplify the code we initialize the cursor_work completely when we allocate the fbcon_ops structure, instead of trying to cope with console re-initialization. The motiviation here is that fbcon code stops using the fb_info.queue, which helps with locking issues around cleanup and all that in a later patch. Also note that this allows us to ditch the hand-rolled work cleanup in fbcon_exit - we already call fbcon_del_cursor_timer, which takes care of everything. Plus this was racy anyway. Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Claudio Suarez Cc: Du Cheng Cc: Thomas Zimmermann Cc: Greg Kroah-Hartman Cc: Tetsuo Handa --- drivers/video/fbdev/core/fbcon.c | 85 +--- drivers/video/fbdev/core/fbcon.h | 4 +- 2 files changed, 35 insertions(+), 54 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 83f0223f5333..a368ed602e2e 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -350,8 +350,8 @@ static int get_color(struct vc_data *vc, struct fb_info *info, static void fb_flashcursor(struct work_struct *work) { - struct fb_info *info = container_of(work, struct fb_info, queue); - struct fbcon_ops *ops = info->fbcon_par; + struct fbcon_ops *ops = container_of(work, struct fbcon_ops, cursor_work.work); + struct fb_info *info; struct vc_data *vc = NULL; int c; int mode; @@ -364,7 +364,10 @@ static void fb_flashcursor(struct work_struct *work) if (ret == 0) return; - if (ops && ops->currcon != -1) + /* protected by console_lock */ + info = ops->info; + + if (ops->currcon != -1) vc = vc_cons[ops->currcon].d; if (!vc || !con_is_visible(vc) || @@ -380,42 +383,25 @@ static void fb_flashcursor(struct work_struct *work) ops->cursor(vc, info, mode, get_color(vc, info, c, 1), get_color(vc, info, c, 0)); console_unlock(); -} -static void cursor_timer_handler(struct timer_list *t) -{ - struct fbcon_ops *ops = from_timer(ops, t, cursor_timer); - struct fb_info *info = ops->info; - - queue_work(system_power_efficient_wq, >queue); - mod_timer(>cursor_timer, jiffies + ops->cur_blink_jiffies); + queue_delayed_work(system_power_efficient_wq, >cursor_work, + ops->cur_blink_jiffies); } -static void fbcon_add_cursor_timer(struct fb_info *info) +static void fbcon_add_cursor_work(struct fb_info *info) { struct fbcon_ops *ops = info->fbcon_par; - if ((!info->queue.func || info->queue.func == fb_flashcursor) && - !(ops->flags & FBCON_FLAGS_CURSOR_TIMER) && - !fbcon_cursor_noblink) { - if (!info->queue.func) - INIT_WORK(>queue, fb_flashcursor); - - timer_setup(>cursor_timer, cursor_timer_handler, 0); - mod_timer(>cursor_timer, jiffies + ops->cur_blink_jiffies); - ops->flags |= FBCON_FLAGS_CURSOR_TIMER; - } + if (!fbcon_cursor_noblink) + queue_delayed_work(system_power_efficient_wq, >cursor_work, + ops->cur_blink_jiffies); } -static void fbcon_del_cursor_timer(struct fb_info *info) +static void fbcon_del_cursor_work(struct fb_info *info) { struct fbcon_ops *ops = info->fbcon_par; - if (info->queue.func == fb_flashcursor && - ops->flags & FBCON_FLAGS_CURSOR_TIMER) { - del_timer_sync(>cursor_timer); - ops->flags &= ~FBCON_FLAGS_CURSOR_TIMER; - } + cancel_delayed_work_sync(>cursor_work); } #ifndef MODULE @@ -714,6 +700,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); if (!ops) err = -ENOMEM; + + INIT_DELAYED_WORK(>cursor_work, fb_flashcursor); } if (!err) { @@ -751,7 +739,7 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, } if (!err) { - fbcon_del_cursor_timer(oldinfo); + fbcon_del_cursor_work(oldinfo); kfree(ops->cursor_state.mask); kfree(ops->cursor_data); kfree(ops->cursor_src); @@ -867,7 +855,7 @@ static int set_con2fb_map(int unit, int newidx, int user) logo_shown != FBCON_LOGO_DONTSHOW); if (!found) - fbcon_add_cursor_timer(info); + fbcon_add_cursor_work(info); con2fb_map_boot[unit] = newidx; con2fb_init_display(vc, info, unit, show_logo); } @@ -964,6 +952,8 @@ static const char *fbcon_startup(void) return NULL; } +
[PATCH v2 05/19] fbdev/sysfs: Fix locking
fb_set_var requires we hold the fb_info lock. Or at least this now matches what the ioctl does ... Note that ps3fb and sh_mobile_lcdcfb are busted in different ways here, but I will not fix them up. Also in practice this isn't a big deal, because really variable fbdev state is actually protected by console_lock (because fbcon just doesn't bother with lock_fb_info() at all), and lock_fb_info protecting anything is really just a neat lie. But that's a much bigger fish to fry. Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Helge Deller Cc: Daniel Vetter Cc: Qing Wang Cc: Sam Ravnborg --- drivers/video/fbdev/core/fbsysfs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c index 26892940c213..8c1ee9ecec3d 100644 --- a/drivers/video/fbdev/core/fbsysfs.c +++ b/drivers/video/fbdev/core/fbsysfs.c @@ -91,9 +91,11 @@ static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var) var->activate |= FB_ACTIVATE_FORCE; console_lock(); + lock_fb_info(fb_info); err = fb_set_var(fb_info, var); if (!err) fbcon_update_vcs(fb_info, var->activate & FB_ACTIVATE_ALL); + unlock_fb_info(fb_info); console_unlock(); if (err) return err; -- 2.34.1
[PATCH v2 03/19] fbcon: Introduce wrapper for console->fb_info lookup
Half of it is protected by console_lock, but the other half is a lot more awkward: Registration/deregistration of fbdev are serialized, but we don't really clear out anything in con2fb_map and so there's potential for use-after free mixups. First step is to encapsulate the lookup. Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Helge Deller Cc: Daniel Vetter Cc: Greg Kroah-Hartman Cc: Tetsuo Handa Cc: Du Cheng Cc: Claudio Suarez Cc: Thomas Zimmermann --- drivers/video/fbdev/core/fbcon.c | 76 ++-- 1 file changed, 44 insertions(+), 32 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index e925bb608e25..b75e638cb83d 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -110,6 +110,18 @@ static struct fbcon_display fb_display[MAX_NR_CONSOLES]; static signed char con2fb_map[MAX_NR_CONSOLES]; static signed char con2fb_map_boot[MAX_NR_CONSOLES]; +static struct fb_info *fbcon_info_from_console(int console) +{ + WARN_CONSOLE_UNLOCKED(); + + /* +* Note that only con2fb_map is protected by the console lock, +* registered_fb is protected by a separate mutex. This lookup can +* therefore race. +*/ + return registered_fb[con2fb_map[console]]; +} + static int logo_lines; /* logo_shown is an index to vc_cons when >= 0; otherwise follows FBCON_LOGO enums. */ @@ -199,7 +211,7 @@ static void fbcon_rotate(struct fb_info *info, u32 rotate) if (!ops || ops->currcon == -1) return; - fb_info = registered_fb[con2fb_map[ops->currcon]]; + fb_info = fbcon_info_from_console(ops->currcon); if (info == fb_info) { struct fbcon_display *p = _display[ops->currcon]; @@ -226,7 +238,7 @@ static void fbcon_rotate_all(struct fb_info *info, u32 rotate) for (i = first_fb_vc; i <= last_fb_vc; i++) { vc = vc_cons[i].d; if (!vc || vc->vc_mode != KD_TEXT || - registered_fb[con2fb_map[i]] != info) + fbcon_info_from_console(i) != info) continue; p = _display[vc->vc_num]; @@ -356,7 +368,7 @@ static void fb_flashcursor(struct work_struct *work) vc = vc_cons[ops->currcon].d; if (!vc || !con_is_visible(vc) || - registered_fb[con2fb_map[vc->vc_num]] != info || + fbcon_info_from_console(vc->vc_num) != info || vc->vc_deccm != 1) { console_unlock(); return; @@ -791,7 +803,7 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, if (show_logo) { struct vc_data *fg_vc = vc_cons[fg_console].d; struct fb_info *fg_info = - registered_fb[con2fb_map[fg_console]]; + fbcon_info_from_console(fg_console); fbcon_prepare_logo(fg_vc, fg_info, fg_vc->vc_cols, fg_vc->vc_rows, fg_vc->vc_cols, @@ -1014,7 +1026,7 @@ static void fbcon_init(struct vc_data *vc, int init) if (con2fb_map[vc->vc_num] == -1) con2fb_map[vc->vc_num] = info_idx; - info = registered_fb[con2fb_map[vc->vc_num]]; + info = fbcon_info_from_console(vc->vc_num); if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET) logo_shown = FBCON_LOGO_DONTSHOW; @@ -1231,7 +1243,7 @@ static void fbcon_deinit(struct vc_data *vc) static void fbcon_clear(struct vc_data *vc, int sy, int sx, int height, int width) { - struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]]; + struct fb_info *info = fbcon_info_from_console(vc->vc_num); struct fbcon_ops *ops = info->fbcon_par; struct fbcon_display *p = _display[vc->vc_num]; @@ -1269,7 +1281,7 @@ static void fbcon_clear(struct vc_data *vc, int sy, int sx, int height, static void fbcon_putcs(struct vc_data *vc, const unsigned short *s, int count, int ypos, int xpos) { - struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]]; + struct fb_info *info = fbcon_info_from_console(vc->vc_num); struct fbcon_display *p = _display[vc->vc_num]; struct fbcon_ops *ops = info->fbcon_par; @@ -1289,7 +1301,7 @@ static void fbcon_putc(struct vc_data *vc, int c, int ypos, int xpos) static void fbcon_clear_margins(struct vc_data *vc, int bottom_only) { - struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]]; + struct fb_info *info = fbcon_info_from_console(vc->vc_num); struct fbcon_ops *ops = info->fbcon_par; if (!fbcon_is_inactive(vc, info)) @@ -1298,7 +1310,7 @@ static void fbcon_clear_margins(struct vc_data *vc, int bottom_only) static void fbcon_cursor(struct vc_data *vc, int mode) { - struct fb_info *info =
[PATCH v2 04/19] fbcon: delete delayed loading code
Before commit 6104c37094e729f3d4ce65797002112735d49cd1 Author: Daniel Vetter Date: Tue Aug 1 17:32:07 2017 +0200 fbcon: Make fbcon a built-time depency for fbdev it was possible to load fbcon and fbdev drivers in any order, which means that fbcon init had to handle the case where fbdev drivers where already registered. This is no longer possible, hence delete that code. Note that the exit case is a bit more complex and will be done in a separate patch. Since I had to audit the entire fbcon load code I also spotted a wrong function name in a comment in fbcon_startup(), which this patch also fixes. v2: Explain why we also fix the comment (Sam) Acked-by: Sam Ravnborg Signed-off-by: Daniel Vetter Cc: Helge Deller Cc: Daniel Vetter Cc: Claudio Suarez Cc: Greg Kroah-Hartman Cc: Tetsuo Handa Cc: Du Cheng --- drivers/video/fbdev/core/fbcon.c | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index b75e638cb83d..83f0223f5333 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -944,7 +944,7 @@ static const char *fbcon_startup(void) return display_desc; /* * Instead of blindly using registered_fb[0], we use info_idx, set by -* fb_console_init(); +* fbcon_fb_registered(); */ info = registered_fb[info_idx]; if (!info) @@ -3299,17 +3299,6 @@ static void fbcon_start(void) return; } #endif - - if (num_registered_fb) { - int i; - - for_each_registered_fb(i) { - info_idx = i; - break; - } - - do_fbcon_takeover(0); - } } static void fbcon_exit(void) -- 2.34.1
[PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions
Avoids two forward declarations, and more importantly, matches what I've done in my fbcon scrolling restore patches - so I need this to avoid a bunch of conflicts in rebasing since we ended up merging Helge's series instead. Signed-off-by: Daniel Vetter Cc: Helge Deller Cc: Daniel Vetter Cc: Thomas Zimmermann Cc: Du Cheng Cc: Tetsuo Handa Cc: Claudio Suarez Cc: Greg Kroah-Hartman --- drivers/video/fbdev/core/fbcon.c | 134 +++ 1 file changed, 65 insertions(+), 69 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 235eaab37d84..e925bb608e25 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -164,15 +164,11 @@ static int fbcon_cursor_noblink; */ static void fbcon_clear_margins(struct vc_data *vc, int bottom_only); -static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx, - int height, int width); static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table); /* * Internal routines */ -static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx, - int dy, int dx, int height, int width, u_int y_break); static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var, int unit); static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p, @@ -1667,6 +1663,71 @@ static void fbcon_redraw(struct vc_data *vc, struct fbcon_display *p, } } +static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx, + int dy, int dx, int height, int width, u_int y_break) +{ + struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]]; + struct fbcon_ops *ops = info->fbcon_par; + u_int b; + + if (sy < y_break && sy + height > y_break) { + b = y_break - sy; + if (dy < sy) { /* Avoid trashing self */ + fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width, + y_break); + fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx, + height - b, width, y_break); + } else { + fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx, + height - b, width, y_break); + fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width, + y_break); + } + return; + } + + if (dy < y_break && dy + height > y_break) { + b = y_break - dy; + if (dy < sy) { /* Avoid trashing self */ + fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width, + y_break); + fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx, + height - b, width, y_break); + } else { + fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx, + height - b, width, y_break); + fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width, + y_break); + } + return; + } + ops->bmove(vc, info, real_y(p, sy), sx, real_y(p, dy), dx, + height, width); +} + +static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx, + int height, int width) +{ + struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]]; + struct fbcon_display *p = _display[vc->vc_num]; + + if (fbcon_is_inactive(vc, info)) + return; + + if (!width || !height) + return; + + /* Split blits that cross physical y_wrap case. +* Pathological case involves 4 blits, better to use recursive +* code rather than unrolled case +* +* Recursive invocations don't need to erase the cursor over and +* over again, so we use fbcon_bmove_rec() +*/ + fbcon_bmove_rec(vc, p, sy, sx, dy, dx, height, width, + p->vrows - p->yscroll); +} + static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b, enum con_scroll dir, unsigned int count) { @@ -1867,71 +1928,6 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b, } -static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx, - int height, int width) -{ - struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]]; - struct fbcon_display *p = _display[vc->vc_num]; - - if (fbcon_is_inactive(vc, info)) - return; - - if (!width || !height) - return; - - /* Split blits that cross physical y_wrap case. -*
[PATCH v2 01/19] fbcon: delete a few unneeded forward decl
I didn't bother with any code movement to fix the others, these just got a bit in the way. v2: Rebase on top of Helge's reverts. Acked-by: Sam Ravnborg (v1) Reviewed-by: Geert Uytterhoeven (v1) Signed-off-by: Daniel Vetter Cc: Helge Deller Cc: Daniel Vetter Cc: Thomas Zimmermann Cc: Du Cheng Cc: Tetsuo Handa Cc: Claudio Suarez Cc: Greg Kroah-Hartman --- drivers/video/fbdev/core/fbcon.c | 17 + 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 2fc1b80a26ad..235eaab37d84 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -163,29 +163,14 @@ static int fbcon_cursor_noblink; * Interface used by the world */ -static const char *fbcon_startup(void); -static void fbcon_init(struct vc_data *vc, int init); -static void fbcon_deinit(struct vc_data *vc); -static void fbcon_clear(struct vc_data *vc, int sy, int sx, int height, - int width); -static void fbcon_putc(struct vc_data *vc, int c, int ypos, int xpos); -static void fbcon_putcs(struct vc_data *vc, const unsigned short *s, - int count, int ypos, int xpos); static void fbcon_clear_margins(struct vc_data *vc, int bottom_only); -static void fbcon_cursor(struct vc_data *vc, int mode); static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx, int height, int width); -static int fbcon_switch(struct vc_data *vc); -static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch); static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table); /* * Internal routines */ -static __inline__ void ywrap_up(struct vc_data *vc, int count); -static __inline__ void ywrap_down(struct vc_data *vc, int count); -static __inline__ void ypan_up(struct vc_data *vc, int count); -static __inline__ void ypan_down(struct vc_data *vc, int count); static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx, int dy, int dx, int height, int width, u_int y_break); static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var, @@ -194,8 +179,8 @@ static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p, int line, int count, int dy); static void fbcon_modechanged(struct fb_info *info); static void fbcon_set_all_vcs(struct fb_info *info); -static void fbcon_start(void); static void fbcon_exit(void); + static struct device *fbcon_device; #ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION -- 2.34.1
[PATCH v2 00/19] fbcon patches, take two
Hi all, Second round, mostly just compile fixed and some minor polish to commit messages. Also MAINTAINERS patch and fbcon scrolling patches are out because they landed already. There's still a handful here that need review (and somehow intel-gfx-ci just keeled over on this). Cheers, Daniel Daniel Vetter (19): fbcon: delete a few unneeded forward decl fbcon: Move fbcon_bmove(_rec) functions fbcon: Introduce wrapper for console->fb_info lookup fbcon: delete delayed loading code fbdev/sysfs: Fix locking fbcon: Use delayed work for cursor fbcon: Replace FBCON_FLAGS_INIT with a boolean fb: Delete fb_info->queue fbcon: Extract fbcon_open/release helpers fbcon: Ditch error handling for con2fb_release_oldinfo fbcon: move more common code into fb_open() fbcon: use lock_fb_info in fbcon_open/release fbcon: Consistently protect deferred_takeover with console_lock() fbcon: Move console_lock for register/unlink/unregister fbcon: Move more code into fbcon_release fbcon: untangle fbcon_exit fbcon: Maintain a private array of fb_info Revert "fbdev: Prevent probing generic drivers if a FB is already registered" fbdev: Make registered_fb[] private to fbmem.c drivers/video/fbdev/core/fbcon.c | 692 ++--- drivers/video/fbdev/core/fbcon.h | 8 +- drivers/video/fbdev/core/fbmem.c | 35 +- drivers/video/fbdev/core/fbsysfs.c | 2 + drivers/video/fbdev/efifb.c| 11 - drivers/video/fbdev/simplefb.c | 11 - include/linux/fb.h | 8 +- 7 files changed, 342 insertions(+), 425 deletions(-) -- 2.34.1
Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
On Tue, 2022-02-08 at 11:44 -0800, Abhinav Kumar wrote: > There are cases where depending on the size of the devcoredump and the speed > at which the usermode reads the dump, it can take longer than the current 5 > mins > timeout. > > This can lead to incomplete dumps as the device is deleted once the timeout > expires. > > One example is below where it took 6 mins for the devcoredump to be > completely read. > > 04:22:24.668 23916 23994 I HWDeviceDRM::DumpDebugData: Opening > /sys/class/devcoredump/devcd6/data > 04:28:35.377 23916 23994 W HWDeviceDRM::DumpDebugData: Freeing devcoredump > node > > Increase the timeout to 10 mins to accommodate system delays and large > coredump > sizes. > No real objection, I guess, but can the data actually disappear *while* the sysfs file is open?! Or did it take 5 minutes to open the file? If the former, maybe we should fix that too (or instead)? johannes
Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
Hi Johannes Thanks for the response. On 2/8/2022 12:35 PM, Johannes Berg wrote: On Tue, 2022-02-08 at 11:44 -0800, Abhinav Kumar wrote: There are cases where depending on the size of the devcoredump and the speed at which the usermode reads the dump, it can take longer than the current 5 mins timeout. This can lead to incomplete dumps as the device is deleted once the timeout expires. One example is below where it took 6 mins for the devcoredump to be completely read. 04:22:24.668 23916 23994 I HWDeviceDRM::DumpDebugData: Opening /sys/class/devcoredump/devcd6/data 04:28:35.377 23916 23994 W HWDeviceDRM::DumpDebugData: Freeing devcoredump node Increase the timeout to 10 mins to accommodate system delays and large coredump sizes. No real objection, I guess, but can the data actually disappear *while* the sysfs file is open?! Or did it take 5 minutes to open the file? If the former, maybe we should fix that too (or instead)? johannes It opened the file rightaway but could not finish reading. The device gets deleted so the corresponding /data will disappear too ( as the data node is under devcd*/data) 60 static void devcd_del(struct work_struct *wk) 61 { 62 struct devcd_entry *devcd; 63 64 devcd = container_of(wk, struct devcd_entry, del_wk.work); 65 66 device_del(>devcd_dev); 67 put_device(>devcd_dev); 68 } Are you suggesting we implement a logic like : a) if the usermode has started reading the data but has not finished yet ( we can detect the former with something like devcd->data_read_ongoing = 1 and we know it has finished when it acks and we can clear this flag then), in the timeout del_wk then we can delay the the delete timer by another TIMEOUT amount of time to give usermode time to finish the data? b) If usermode acks, we will clear both the flag and delete the device as usual But there is a corner case here: c) If usermode starts the read, but then for some reason crashes, the timer will timeout and try to delete the device but will detect that usermode is still reading and will keep the device. How do we detect this case? Thats why i thought maybe the easier way right now is to try increasing the timeout.
[PATCH v3 3/4] drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item
Also, document DRM_I915_QUERY_HWCONFIG_BLOB with this struct. v3: * Add various changes suggested by Tvrtko Cc: Daniel Vetter Signed-off-by: Jordan Justen --- include/uapi/drm/i915_drm.h | 32 1 file changed, 32 insertions(+) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 069d2fadfbd9..c3faee3b3f70 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -3276,6 +3276,38 @@ struct drm_i915_gem_create_ext_protected_content { __u32 flags; }; +/** + * DOC: GuC HWCONFIG blob uAPI + * + * The GuC produces a blob with information about the current device. + * i915 reads this blob from GuC and makes it available via this uAPI. + * + * The returned blob is a sequence of items of variable length + * described by struct drm_i915_query_hwconfig_blob_item. The + * drm_i915_query_hwconfig_blob_item length field gives the length of + * the drm_i915_query_hwconfig_blob_item data[] array for the item and + * thereby determines the length of that item. The key and length + * fields are required, so the minimum item size is 2 x __u32, or 8 + * bytes. + * + * The overall blob returned by DRM_I915_QUERY_HWCONFIG_BLOB will end + * at the same location as the end of the final + * drm_i915_query_hwconfig_blob_item. In other words, walking through + * the individual items is guaranteed to eventually arrive at the + * exact end of the entire blob. + * + * The meaning of the key field and the data values are documented in + * the Programmer's Reference Manual. + */ +struct drm_i915_query_hwconfig_blob_item { + /** @key: Enum which defines how to interpret @data values. */ + __u32 key; + /** @length: Length of the @data array. */ + __u32 length; + /** @key: Array of values with meaning defined by @key */ + __u32 data[]; +}; + /* ID of the protected content session managed by i915 when PXP is active */ #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf -- 2.34.1
[PATCH v3 4/4] drm/i915/guc: Verify hwconfig blob matches supported format
i915_drm.h now defines the format of the returned DRM_I915_QUERY_HWCONFIG_BLOB query item. Since i915 receives this from the black box GuC software, it should verify that the data matches that format before sending it to user-space. The verification makes a single simple pass through the blob contents, so this verification step should not add a significant amount of init time to i915. v3: * Add various changes suggested by Tvrtko Signed-off-by: Jordan Justen --- .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 56 ++- 1 file changed, 53 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c index ce6088f112d4..350a0517b9f0 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c @@ -71,7 +71,52 @@ static int guc_hwconfig_discover_size(struct intel_guc_hwconfig *hwconfig) return 0; } -static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig) +static int verify_hwconfig_blob(struct drm_device *drm, + const struct intel_guc_hwconfig *hwconfig) +{ + struct drm_i915_query_hwconfig_blob_item *pos; + u32 remaining; + + if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL) + return -EINVAL; + + pos = hwconfig->ptr; + /* The number of dwords in the blob to validate. Each loop +* pass will process at least 2 dwords corresponding to the +* key and length fields of the item. In addition, the length +* field of the item indicates the length of the data array, +* and that number of dwords will be processed (skipped) as +* well. +*/ + remaining = hwconfig->size / 4; + + while (remaining > 0) { + /* Each item requires at least 2 dwords for the key +* and length fields. If the length field is 0, then +* the data array would be of length 0. +*/ + if (remaining < 2) + return -EINVAL; + /* remaining >= 2, so subtracting 2 is ok, whereas +* adding 2 to pos->length could overflow. +*/ + if (pos->length > remaining - 2) + return -EINVAL; + /* The length check above ensures that the adjustment +* of the remaining variable will not underflow, and +* that the adjustment of the pos variable will not +* pass the end of the blob data. +*/ + remaining -= 2 + pos->length; + pos = (void *)>data[pos->length]; + } + + drm_dbg(drm, "hwconfig blob format is valid\n"); + return 0; +} + +static int guc_hwconfig_fill_buffer(struct drm_device *drm, + struct intel_guc_hwconfig *hwconfig) { struct intel_guc *guc = hwconfig_to_guc(hwconfig); struct i915_vma *vma; @@ -88,8 +133,13 @@ static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig) ggtt_offset = intel_guc_ggtt_offset(guc, vma); ret = __guc_action_get_hwconfig(hwconfig, ggtt_offset, hwconfig->size); - if (ret >= 0) + if (ret >= 0) { memcpy(hwconfig->ptr, vaddr, hwconfig->size); + if (verify_hwconfig_blob(drm, hwconfig)) { + drm_err(drm, "Ignoring invalid hwconfig blob received from GuC!\n"); + ret = -EINVAL; + } + } i915_vma_unpin_and_release(, I915_VMA_RELEASE_MAP); @@ -141,7 +191,7 @@ int intel_guc_hwconfig_init(struct intel_guc_hwconfig *hwconfig) return -ENOMEM; } - ret = guc_hwconfig_fill_buffer(hwconfig); + ret = guc_hwconfig_fill_buffer(>drm, hwconfig); if (ret < 0) { intel_guc_hwconfig_fini(hwconfig); return ret; -- 2.34.1
[PATCH v3 2/4] drm/i915/uapi: Add query for hwconfig blob
From: Rodrigo Vivi The DRM_I915_QUERY_HWCONFIG_BLOB query item returns a blob of data which it receives from the GuC software. This blob provides some useful data about the hardware for drivers. Although the blob is not fully documented at this time, the basic format is an array of u32 values. The array is a simple and flexible KLV (Key/Length/Value) formatted table. For example, it could be just: enum device_attr { ATTR_SOME_VALUE = 0, ATTR_SOME_MASK = 1, }; static const u32 hwconfig[] = { ATTR_SOME_VALUE, 1, // Value Length in DWords 8, // Value ATTR_SOME_MASK, 3, 0x00, 0x, 0xFF00, }; The attribute ids and meaning of the values will be documented in the Programmer Reference Manuals when released. Cc: Tvrtko Ursulin Cc: Kenneth Graunke Cc: Michal Wajdeczko Cc: Slawomir Milczarek Cc: Joonas Lahtinen Signed-off-by: Rodrigo Vivi Signed-off-by: John Harrison Reviewed-by: Matthew Brost Acked-by: Jordan Justen Tested-by: Jordan Justen --- drivers/gpu/drm/i915/i915_query.c | 23 +++ include/uapi/drm/i915_drm.h | 1 + 2 files changed, 24 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index 2dfbc22857a3..195524e9a369 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private *i915, return total_length; } +static int query_hwconfig_blob(struct drm_i915_private *i915, + struct drm_i915_query_item *query_item) +{ + struct intel_gt *gt = to_gt(i915); + struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig; + + if (!hwconfig->size || !hwconfig->ptr) + return -ENODEV; + + if (query_item->length == 0) + return hwconfig->size; + + if (query_item->length < hwconfig->size) + return -EINVAL; + + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), +hwconfig->ptr, hwconfig->size)) + return -EFAULT; + + return hwconfig->size; +} + static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv, struct drm_i915_query_item *query_item) = { query_topology_info, query_engine_info, query_perf_config, query_memregion_info, + query_hwconfig_blob, }; int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 914ebd9290e5..069d2fadfbd9 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -2685,6 +2685,7 @@ struct drm_i915_query_item { #define DRM_I915_QUERY_ENGINE_INFO 2 #define DRM_I915_QUERY_PERF_CONFIG 3 #define DRM_I915_QUERY_MEMORY_REGIONS 4 +#define DRM_I915_QUERY_HWCONFIG_BLOB 5 /* Must be kept compact -- no holes and well documented */ /** -- 2.34.1
[PATCH v3 0/4] GuC HWCONFIG with documentation
This is John/Rodrigo's 2 patches with some minor changes, and I added 2 patches. "drm/i915/uapi: Add query for hwconfig blob" was changed: * Rename DRM_I915_QUERY_HWCONFIG_TABLE to DRM_I915_QUERY_HWCONFIG_BLOB as requested by Joonas. * Reword commit message * I added Acked-by to this patch, but this only applies in the context of this version of the patchset. If my changes are rejected, then please *do not* add my Acked-by to the other series. In particular, I do not want my Acked-by on the patch if the patch mentions the HWCONFIG format, but is not willing to add that to the actual uAPI. I also do not want my Acked-by on it if it mentions "consolidation" of this data. Since we are dealing with open source projects (aside from GuC), this doesn't seem appropriate. "drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item" adds a struct to the uAPI and documents the return value for DRM_I915_QUERY_HWCONFIG_BLOB. (Except, keys / values are still deferred to the PRM.) "drm/i915/guc: Verify hwconfig blob matches supported format" does the simple verification of the blob to make sure it matches what the uAPI documents. v2: * Fix -Werror errors. * Rebase to drm-intel/for-linux-next instead of drm-intel/for-linux-next-gt, as this seems to be what CI wants. * Fix u32 -> __u32. (Sorry, I was first testing in Mesa tree.) * Add commit message for "Verify hwconfig blob" patch as requested by Tvrtko. * Reword text added to i915_drm.h as requested by Tvrtko. (Attempting to indicate the overall blob ends right at the last blob item.) v3: * Add several changes suggested by Tvrtko in the "Verify hwconfig blob", along with some tweaks to i915_drm.h from the feedback for the same patch. John Harrison (1): drm/i915/guc: Add fetch of hwconfig table Jordan Justen (2): drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item drm/i915/guc: Verify hwconfig blob matches supported format Rodrigo Vivi (1): drm/i915/uapi: Add query for hwconfig blob drivers/gpu/drm/i915/Makefile | 1 + .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 1 + .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h | 4 + drivers/gpu/drm/i915/gt/uc/intel_guc.h| 3 + .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 201 ++ .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h | 19 ++ drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 + drivers/gpu/drm/i915/i915_query.c | 23 ++ include/uapi/drm/i915_drm.h | 33 +++ 9 files changed, 291 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h -- 2.34.1