Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags
Hi Robin, On Thu, 30 Sep 2021 21:44:24 +0200 Boris Brezillon wrote: > On Thu, 30 Sep 2021 20:47:23 +0200 > Boris Brezillon wrote: > > > So we can create GPU mappings without R/W permissions. Particularly > > useful to debug corruptions caused by out-of-bound writes. > > Oops, I forgot to add the PANFROST_BO_PRIVATE flag suggested by Robin > here [1]. I'll send a v2. When you're talking about a PANFROST_BO_GPU_PRIVATE flag (or PANFROST_BO_NO_CPU_ACCESS), you mean something that can set ARM_LPAE_PTE_SH_IS instead of the unconditional ARM_LPAE_PTE_SH_OS we have right now [1], right? In this case, how would you pass this info to the iommu? Looks like we have an IOMMU_CACHE, but I don't think it reflects what we're trying to do. IOMMU_PRIV is about privileged mappings, so definitely not what we want. Should we add a new IOMMU_NO_{EXTERNAL,HOST,CPU}_ACCESS flag for that? Regards, Boris [1]https://elixir.bootlin.com/linux/v5.15-rc3/source/drivers/iommu/io-pgtable-arm.c#L453
Re: [PATCH v3 4/4] drm/v3d: add multiple syncobjs support
On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote: > Using the generic extension from the previous patch, a specific > multisync > extension enables more than one in/out binary syncobj per job > submission. > Arrays of syncobjs are set in struct drm_v3d_multisync, that also > cares > of determining the stage for sync (wait deps) according to the job > queue. > > v2: > - subclass the generic extension struct (Daniel) > - simplify adding dependency conditions to make understandable (Iago) > > v3: > - fix conditions to consider single or multiples in/out_syncs (Iago) > - remove irrelevant comment (Iago) > > Signed-off-by: Melissa Wen > --- > drivers/gpu/drm/v3d/v3d_drv.c | 6 +- > drivers/gpu/drm/v3d/v3d_drv.h | 24 +++-- > drivers/gpu/drm/v3d/v3d_gem.c | 185 ++ > > include/uapi/drm/v3d_drm.h| 49 - > 4 files changed, 232 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c > b/drivers/gpu/drm/v3d/v3d_drv.c > index 3d6b9bcce2f7..bd46396a1ae0 100644 > --- a/drivers/gpu/drm/v3d/v3d_drv.c > +++ b/drivers/gpu/drm/v3d/v3d_drv.c > @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device (...) > > @@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct > drm_file *file_priv, >struct v3d_job *job, >struct ww_acquire_ctx > *acquire_ctx, >u32 out_sync, > + struct v3d_submit_ext *se, >struct dma_fence *done_fence) > { > struct drm_syncobj *sync_out; > + bool has_multisync = se && (se->flags & We always pass the 'se' pointer from a local variable allocated in the stack of the caller so it is never going to be NULL. I am happy to keep the NULL check if you want to protect against future changes just in case, but if we do that then... > DRM_V3D_EXT_ID_MULTI_SYNC); > int i; > > for (i = 0; i < job->bo_count; i++) { (...) > +static void > +v3d_put_multisync_post_deps(struct v3d_submit_ext *se) > +{ > + unsigned int i; > + > + if (!se->out_sync_count) ...we should also check for NULL here for consistency. Also, I think there is another problem in the code: we always call v3d_job_cleanup for failed jobs, but only call v3d_job_put for successful jobs. However, reading the docs for drm_sched_job_init: "Drivers must make sure drm_sched_job_cleanup() if this function returns successfully, even when @job is aborted before drm_sched_job_arm() is called." So my understanding is that we should call v3d_job_cleanup instead of v3d_job_put for successful jobs or we would be leaking sched resources on every successful job, no? Iago > + return; > + > + for (i = 0; i < se->out_sync_count; i++) > + drm_syncobj_put(se->out_syncs[i].syncobj); > + kvfree(se->out_syncs); > +} > + > +static int > +v3d_get_multisync_post_deps(struct drm_file *file_priv, > + struct v3d_submit_ext *se, > + u32 count, u64 handles) > +{ > + struct drm_v3d_sem __user *post_deps; > + int i, ret; > + > + if (!count) > + return 0; > + > + se->out_syncs = (struct v3d_submit_outsync *) > + kvmalloc_array(count, > +sizeof(struct > v3d_submit_outsync), > +GFP_KERNEL); > + if (!se->out_syncs) > + return -ENOMEM; > + > + post_deps = u64_to_user_ptr(handles); > + > + for (i = 0; i < count; i++) { > + struct drm_v3d_sem out; > + > + ret = copy_from_user(&out, post_deps++, sizeof(out)); > + if (ret) { > + DRM_DEBUG("Failed to copy post dep handles\n"); > + goto fail; > + } > + > + se->out_syncs[i].syncobj = drm_syncobj_find(file_priv, > + out.handle) > ; > + if (!se->out_syncs[i].syncobj) { > + ret = -EINVAL; > + goto fail; > + } > } > + se->out_sync_count = count; > + > + return 0; > + > +fail: > + for (i--; i >= 0; i--) > + drm_syncobj_put(se->out_syncs[i].syncobj); > + kvfree(se->out_syncs); > + > + return ret; > +} > + > +/* Get data for multiple binary semaphores synchronization. Parse > syncobj > + * to be signaled when job completes (out_sync). > + */ > +static int > +v3d_get_multisync_submit_deps(struct drm_file *file_priv, > + struct drm_v3d_extension __user *ext, > + void *data) > +{ > + struct drm_v3d_multi_sync multisync; > + struct v3d_submit_ext *se = data; > + int ret; > + > + ret = copy_from_user(&multisync, ext, sizeof(multisync)); > + if (ret) > + return ret; >
Re: [PATCH] drm/amdgpu: fix some repeated includings
It means that you should modify your patch, yes. Regards, Christian. Am 01.10.21 um 05:02 schrieb 郭正奎: So, it means I need to make another commit? Zhengkui *From:*guozheng...@vivo.com *On Behalf Of *Christian K?nig *Sent:* Thursday, September 30, 2021 7:56 PM *To:* Guo Zhengkui ; Simon Ser *Cc:* Deucher, Alexander ; Pan, Xinhui ; David Airlie ; Daniel Vetter ; Chen, Guchun ; Zhou, Peng Ju ; Zhang, Bokun ; Gao, Likun ; amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org; kernel *Subject:* Re: [PATCH] drm/amdgpu: fix some repeated includings Ah, that makes more sense. Then please remove the duplicates in lines 46 and 47 instead since the other ones are more correctly grouped together with their blocks. Christian. Am 30.09.21 um 13:54 schrieb 郭正奎: Actually the duplicates take place in line 46, 47 and 62, 63. diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c index 291a47f7992a..94fca56583a0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c @@ -46,34 +46,32 @@ #include "vcn_v2_0.h" #include "jpeg_v2_0.h" #include "vcn_v2_5.h" #include "jpeg_v2_5.h" #include "smuio_v9_0.h" #include "gmc_v10_0.h" #include "gfxhub_v2_0.h" #include "mmhub_v2_0.h" #include "nbio_v2_3.h" #include "nbio_v7_2.h" #include "hdp_v5_0.h" #include "nv.h" #include "navi10_ih.h" #include "gfx_v10_0.h" #include "sdma_v5_0.h" #include "sdma_v5_2.h" -#include "vcn_v2_0.h" -#include "jpeg_v2_0.h" #include "vcn_v3_0.h" #include "jpeg_v3_0.h" #include "amdgpu_vkms.h" #include "mes_v10_1.h" #include "smuio_v11_0.h" #include "smuio_v11_0_6.h" #include "smuio_v13_0.h" MODULE_FIRMWARE("amdgpu/ip_discovery.bin"); #define mmRCC_CONFIG_MEMSIZE 0xde3 #define mmMM_INDEX 0x0 #define mmMM_INDEX_HI 0x6 #define mmMM_DATA 0x1 static const char *hw_id_names[HW_ID_MAX] = {
[PATCH] drm/i915: remove IS_ACTIVE
When trying to bring IS_ACTIVE to linux/kconfig.h I thought it wouldn't provide much value just encapsulating it in a boolean context. So I also added the support for handling undefined macros as the IS_ENABLED() counterpart. However the feedback received from Masahiro Yamada was that it is too ugly, not providing much value. And just wrapping in a boolean context is too dumb - we could simply open code it. As detailed in commit babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates"), the IS_ACTIVE macro was added to workaround a compilation warning. However after checking again our current uses of IS_ACTIVE it turned out there is only 1 case in which it would potentially trigger a warning. All the others can simply use the shorter version, without wrapping it in any macro. And even that single one didn't trigger any warning in gcc 10.3. So here I'm dialing all the way back to simply removing the macro. If it triggers warnings in future we may change the few cases to check for > 0 or != 0. Another possibility would be to use the great "not not operator" for all positive checks, which would allow us to maintain consistency. However let's try first the simplest form though, hopefully we don't hit broken compilers spitting a warning: Signed-off-by: Lucas De Marchi --- drivers/gpu/drm/i915/gem/i915_gem_context.c| 2 +- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- drivers/gpu/drm/i915/gt/intel_engine.h | 4 ++-- drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 2 +- drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 +- .../gpu/drm/i915/gt/intel_execlists_submission.c | 2 +- .../gpu/drm/i915/gt/selftest_engine_heartbeat.c| 4 ++-- drivers/gpu/drm/i915/gt/selftest_execlists.c | 14 +++--- drivers/gpu/drm/i915/i915_config.c | 2 +- drivers/gpu/drm/i915/i915_request.c| 2 +- drivers/gpu/drm/i915/i915_utils.h | 13 - 11 files changed, 18 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 8208fd5b72c3..be60bcf8069c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -761,7 +761,7 @@ static int intel_context_set_gem(struct intel_context *ce, intel_engine_has_semaphores(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); - if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) && + if (CONFIG_DRM_I915_REQUEST_TIMEOUT && ctx->i915->params.request_timeout_ms) { unsigned int timeout_ms = ctx->i915->params.request_timeout_ms; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 5130e8ed9564..65fc6ff5f59d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -395,7 +395,7 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) /* Track the mmo associated with the fenced vma */ vma->mmo = mmo; - if (IS_ACTIVE(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)) + if (CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) intel_wakeref_auto(&i915->ggtt.userfault_wakeref, msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)); diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index 87579affb952..6aba239a10e8 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -273,7 +273,7 @@ static inline bool intel_engine_uses_guc(const struct intel_engine_cs *engine) static inline bool intel_engine_has_preempt_reset(const struct intel_engine_cs *engine) { - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) return false; return intel_engine_has_preemption(engine); @@ -300,7 +300,7 @@ intel_virtual_engine_has_heartbeat(const struct intel_engine_cs *engine) static inline bool intel_engine_has_heartbeat(const struct intel_engine_cs *engine) { - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) return false; if (intel_engine_is_virtual(engine)) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c index 74775ae961b2..a3698f611f45 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c @@ -207,7 +207,7 @@ static void heartbeat(struct work_struct *wrk) void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine) { - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) return; next_heartbeat(engine); diff --git a/drivers/gpu/drm/i915/g
Re: [PATCH] drm/i915: remove IS_ACTIVE
On Fri, 01 Oct 2021, Lucas De Marchi wrote: > When trying to bring IS_ACTIVE to linux/kconfig.h I thought it wouldn't > provide much value just encapsulating it in a boolean context. So I also > added the support for handling undefined macros as the IS_ENABLED() > counterpart. However the feedback received from Masahiro Yamada was that > it is too ugly, not providing much value. And just wrapping in a boolean > context is too dumb - we could simply open code it. > > As detailed in commit babaab2f4738 ("drm/i915: Encapsulate kconfig > constant values inside boolean predicates"), the IS_ACTIVE macro was > added to workaround a compilation warning. However after checking again > our current uses of IS_ACTIVE it turned out there is only > 1 case in which it would potentially trigger a warning. All the others > can simply use the shorter version, without wrapping it in any macro. > And even that single one didn't trigger any warning in gcc 10.3. > > So here I'm dialing all the way back to simply removing the macro. If it > triggers warnings in future we may change the few cases to check for > 0 > or != 0. Another possibility would be to use the great "not not > operator" for all positive checks, which would allow us to maintain > consistency. However let's try first the simplest form though, hopefully > we don't hit broken compilers spitting a warning: > > Signed-off-by: Lucas De Marchi Acked-by: Jani Nikula > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c| 2 +- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- > drivers/gpu/drm/i915/gt/intel_engine.h | 4 ++-- > drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 2 +- > drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 +- > .../gpu/drm/i915/gt/intel_execlists_submission.c | 2 +- > .../gpu/drm/i915/gt/selftest_engine_heartbeat.c| 4 ++-- > drivers/gpu/drm/i915/gt/selftest_execlists.c | 14 +++--- > drivers/gpu/drm/i915/i915_config.c | 2 +- > drivers/gpu/drm/i915/i915_request.c| 2 +- > drivers/gpu/drm/i915/i915_utils.h | 13 - > 11 files changed, 18 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c > b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 8208fd5b72c3..be60bcf8069c 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -761,7 +761,7 @@ static int intel_context_set_gem(struct intel_context *ce, > intel_engine_has_semaphores(ce->engine)) > __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); > > - if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) && > + if (CONFIG_DRM_I915_REQUEST_TIMEOUT && > ctx->i915->params.request_timeout_ms) { > unsigned int timeout_ms = ctx->i915->params.request_timeout_ms; > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > index 5130e8ed9564..65fc6ff5f59d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > @@ -395,7 +395,7 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) > /* Track the mmo associated with the fenced vma */ > vma->mmo = mmo; > > - if (IS_ACTIVE(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)) > + if (CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) > intel_wakeref_auto(&i915->ggtt.userfault_wakeref, > > msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)); > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h > b/drivers/gpu/drm/i915/gt/intel_engine.h > index 87579affb952..6aba239a10e8 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h > @@ -273,7 +273,7 @@ static inline bool intel_engine_uses_guc(const struct > intel_engine_cs *engine) > static inline bool > intel_engine_has_preempt_reset(const struct intel_engine_cs *engine) > { > - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) > + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) > return false; > > return intel_engine_has_preemption(engine); > @@ -300,7 +300,7 @@ intel_virtual_engine_has_heartbeat(const struct > intel_engine_cs *engine) > static inline bool > intel_engine_has_heartbeat(const struct intel_engine_cs *engine) > { > - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) > + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) > return false; > > if (intel_engine_is_virtual(engine)) > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > index 74775ae961b2..a3698f611f45 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > @@ -207,7 +207,7 @@ static void heartbeat(struct work_struct *wrk) > > void intel_engine_unpark_heartbeat(struct intel_
Re: [PATCH] fbdev: Garbage collect fbdev scrolling acceleration, part 1 (from TODO list)
Hi Am 30.09.21 um 17:10 schrieb Claudio: Scroll acceleration is disabled in fbcon by hard-wiring p->scrollmode = SCROLL_REDRAW. Remove the obsolete code in fbcon.c and fbdev/core/ Signed-off-by: Claudio Suarez --- - This is a task in the TODO list Documentation/gpu/todo.rst - The contact in the task is Daniel Vetter. He is/you are in copy. - To ease the things and saving time, I did a patch. It is included in this message. I can redo it if there is something wrong. - I tested it in some configurations. My plan for new patches in this task: - A buch of patches to remove code from drivers: fb_copyarea and related. - Simplify the code around fbcon_ops as much as possible to remove the hooks as the TODO suggests. - Remove fb_copyarea in headers and exported symbols: cfb_copyarea, etc. This must be done when all the drivers are changed. I think that the correct list to ask questions about this is linux-fb...@vger.kernel.org . Is it correct ? My question: I can develop the new changes. I can test in two computers/two drivers. Is there a way to test the rest of the patches ? I have not hardware to test them. Is anyone helping with this? Only regression tests are needed. I can test other patches in return. Thank you. Claudio Suarez. Patch follows: Documentation/gpu/todo.rst | 13 +- drivers/video/fbdev/core/bitblit.c | 16 - drivers/video/fbdev/core/fbcon.c| 509 ++-- drivers/video/fbdev/core/fbcon.h| 59 drivers/video/fbdev/core/fbcon_ccw.c| 28 +- drivers/video/fbdev/core/fbcon_cw.c | 28 +- drivers/video/fbdev/core/fbcon_rotate.h | 9 - drivers/video/fbdev/core/fbcon_ud.c | 37 +-- drivers/video/fbdev/core/tileblit.c | 16 - drivers/video/fbdev/skeletonfb.c| 12 +- include/linux/fb.h | 2 +- 11 files changed, 51 insertions(+), 678 deletions(-) Nice stats :) I looked through it and it looks good. Maybe double-check that everything still builds. Acked-by: Thomas Zimmermann Best regards Thomas diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 12e61869939e..bb1e04bbf4fb 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -314,16 +314,19 @@ Level: Advanced Garbage collect fbdev scrolling acceleration -Scroll acceleration is disabled in fbcon by hard-wiring p->scrollmode = -SCROLL_REDRAW. There's a ton of code this will allow us to remove: +Scroll acceleration has been disabled in fbcon. Now it works as the old +SCROLL_REDRAW mode. A ton of code was removed in fbcon.c and the hook bmove was +removed from fbcon_ops. +Remaining tasks: -- lots of code in fbcon.c - -- a bunch of the hooks in fbcon_ops, maybe the remaining hooks could be called +- a bunch of the hooks in fbcon_ops could be removed or simplified by calling directly instead of the function table (with a switch on p->rotate) - fb_copyarea is unused after this, and can be deleted from all drivers +- after that, fb_copyarea can be deleted from fb_ops in include/linux/fb.h as + well as cfb_copyarea + Note that not all acceleration code can be deleted, since clearing and cursor support is still accelerated, which might be good candidates for further deletion projects. diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c index f98e8f298bc1..01fae2c96965 100644 --- a/drivers/video/fbdev/core/bitblit.c +++ b/drivers/video/fbdev/core/bitblit.c @@ -43,21 +43,6 @@ static void update_attr(u8 *dst, u8 *src, int attribute, } } -static void bit_bmove(struct vc_data *vc, struct fb_info *info, int sy, - int sx, int dy, int dx, int height, int width) -{ - struct fb_copyarea area; - - area.sx = sx * vc->vc_font.width; - area.sy = sy * vc->vc_font.height; - area.dx = dx * vc->vc_font.width; - area.dy = dy * vc->vc_font.height; - area.height = height * vc->vc_font.height; - area.width = width * vc->vc_font.width; - - info->fbops->fb_copyarea(info, &area); -} - static void bit_clear(struct vc_data *vc, struct fb_info *info, int sy, int sx, int height, int width) { @@ -393,7 +378,6 @@ static int bit_update_start(struct fb_info *info) void fbcon_set_bitops(struct fbcon_ops *ops) { - ops->bmove = bit_bmove; ops->clear = bit_clear; ops->putcs = bit_putcs; ops->clear_margins = bit_clear_margins; diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 22bb3892f6bd..99ecd9a6d844 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -173,8 +173,6 @@ 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 f
Re: [PATCH v3] drm/i915/ttm: Rework object initialization slightly
On 30/09/2021 12:32, Thomas Hellström wrote: We may end up in i915_ttm_bo_destroy() in an error path before the object is fully initialized. In that case it's not correct to call __i915_gem_free_object(), because that function a) Assumes the gem object refcount is 0, which it isn't. b) frees the placements which are owned by the caller until the init_object() region ops returns successfully. Fix this by providing a lightweight cleanup function __i915_gem_object_fini() which is also called by __i915_gem_free_object(). While doing this, also make sure we call dma_resv_fini() as part of ordinary object destruction and not from the RCU callback that frees the object. This will help track down bugs where the object is incorrectly locked from an RCU lookup. Finally, make sure the object isn't put on the region list until it's either locked or fully initialized in order to block list processing of partially initialized objects. v2: - The TTM object backend memory was freed before the gem pages were put. Separate this functionality into __i915_gem_object_pages_fini() and call it from the TTM delete_mem_notify() callback. v3: - Include i915_gem_object_free_mmaps() in __i915_gem_object_pages_fini() to make sure we don't inadvertedly introduce a race. Signed-off-by: Thomas Hellström Reviewed-by: Matthew Auld #v1 R-b still stands. --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 43 +++--- drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++ drivers/gpu/drm/i915/gem/i915_gem_ttm.c| 36 +++--- 3 files changed, 64 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 6fb9afb65034..b88b121e244a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -89,6 +89,22 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, mutex_init(&obj->mm.get_dma_page.lock); } +/** + * i915_gem_object_fini - Clean up a GEM object initialization + * @obj: The gem object to cleanup + * + * This function cleans up gem object fields that are set up by + * drm_gem_private_object_init() and i915_gem_object_init(). + * It's primarily intended as a helper for backends that need to + * clean up the gem object in separate steps. + */ +void __i915_gem_object_fini(struct drm_i915_gem_object *obj) +{ + mutex_destroy(&obj->mm.get_page.lock); + mutex_destroy(&obj->mm.get_dma_page.lock); + dma_resv_fini(&obj->base._resv); +} + /** * Mark up the object's coherency levels for a given cache_level * @obj: #drm_i915_gem_object @@ -174,7 +190,6 @@ void __i915_gem_free_object_rcu(struct rcu_head *head) container_of(head, typeof(*obj), rcu); struct drm_i915_private *i915 = to_i915(obj->base.dev); - dma_resv_fini(&obj->base._resv); i915_gem_object_free(obj); GEM_BUG_ON(!atomic_read(&i915->mm.free_count)); @@ -204,10 +219,17 @@ static void __i915_gem_object_free_mmaps(struct drm_i915_gem_object *obj) } } -void __i915_gem_free_object(struct drm_i915_gem_object *obj) +/** + * __i915_gem_object_pages_fini - Clean up pages use of a gem object + * @obj: The gem object to clean up + * + * This function cleans up usage of the object mm.pages member. It + * is intended for backends that need to clean up a gem object in + * separate steps and needs to be called when the object is idle before + * the object's backing memory is freed. + */ +void __i915_gem_object_pages_fini(struct drm_i915_gem_object *obj) { - trace_i915_gem_object_destroy(obj); - if (!list_empty(&obj->vma.list)) { struct i915_vma *vma; @@ -233,11 +255,17 @@ void __i915_gem_free_object(struct drm_i915_gem_object *obj) __i915_gem_object_free_mmaps(obj); - GEM_BUG_ON(!list_empty(&obj->lut_list)); - atomic_set(&obj->mm.pages_pin_count, 0); __i915_gem_object_put_pages(obj); GEM_BUG_ON(i915_gem_object_has_pages(obj)); +} + +void __i915_gem_free_object(struct drm_i915_gem_object *obj) +{ + trace_i915_gem_object_destroy(obj); + + GEM_BUG_ON(!list_empty(&obj->lut_list)); + bitmap_free(obj->bit_17); if (obj->base.import_attach) @@ -253,6 +281,8 @@ void __i915_gem_free_object(struct drm_i915_gem_object *obj) if (obj->shares_resv_from) i915_vm_resv_put(obj->shares_resv_from); + + __i915_gem_object_fini(obj); } static void __i915_gem_free_objects(struct drm_i915_private *i915, @@ -266,6 +296,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, obj->ops->delayed_free(obj); continue; } + __i915_gem_object_pages_fini(obj); __i915_gem_free_object(obj); /* But keep the pointer alive for RCU-protected lookups */ diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/
Re: [PATCH v3 4/4] drm/v3d: add multiple syncobjs support
On 10/01, Iago Toral wrote: > On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote: > > Using the generic extension from the previous patch, a specific > > multisync > > extension enables more than one in/out binary syncobj per job > > submission. > > Arrays of syncobjs are set in struct drm_v3d_multisync, that also > > cares > > of determining the stage for sync (wait deps) according to the job > > queue. > > > > v2: > > - subclass the generic extension struct (Daniel) > > - simplify adding dependency conditions to make understandable (Iago) > > > > v3: > > - fix conditions to consider single or multiples in/out_syncs (Iago) > > - remove irrelevant comment (Iago) > > > > Signed-off-by: Melissa Wen > > --- > > drivers/gpu/drm/v3d/v3d_drv.c | 6 +- > > drivers/gpu/drm/v3d/v3d_drv.h | 24 +++-- > > drivers/gpu/drm/v3d/v3d_gem.c | 185 ++ > > > > include/uapi/drm/v3d_drm.h| 49 - > > 4 files changed, 232 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c > > b/drivers/gpu/drm/v3d/v3d_drv.c > > index 3d6b9bcce2f7..bd46396a1ae0 100644 > > --- a/drivers/gpu/drm/v3d/v3d_drv.c > > +++ b/drivers/gpu/drm/v3d/v3d_drv.c > > @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device > > (...) > > > > > @@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct > > drm_file *file_priv, > > struct v3d_job *job, > > struct ww_acquire_ctx > > *acquire_ctx, > > u32 out_sync, > > +struct v3d_submit_ext *se, > > struct dma_fence *done_fence) > > { > > struct drm_syncobj *sync_out; > > + bool has_multisync = se && (se->flags & > > We always pass the 'se' pointer from a local variable allocated in the > stack of the caller so it is never going to be NULL. > > I am happy to keep the NULL check if you want to protect against future > changes just in case, but if we do that then... > > > DRM_V3D_EXT_ID_MULTI_SYNC); > > int i; > > > > for (i = 0; i < job->bo_count; i++) { > > (...) > > > +static void > > +v3d_put_multisync_post_deps(struct v3d_submit_ext *se) > > +{ > > + unsigned int i; > > + > > + if (!se->out_sync_count) > > ...we should also check for NULL here for consistency. yes, consistency makes sense here. > > Also, I think there is another problem in the code: we always call > v3d_job_cleanup for failed jobs, but only call v3d_job_put for > successful jobs. However, reading the docs for drm_sched_job_init: > > "Drivers must make sure drm_sched_job_cleanup() if this function > returns successfully, even when @job is aborted before > drm_sched_job_arm() is called." > > So my understanding is that we should call v3d_job_cleanup instead of > v3d_job_put for successful jobs or we would be leaking sched resources > on every successful job, no? When job_init is successful, v3d_job_cleanup is called by scheduler when job is completed. drm_sched_job_cleanup describes how it works after drm_sched_job_arm: " After that point of no return @job is committed to be executed by the scheduler, and this function should be called from the &drm_sched_backend_ops.free_job callback." On v3d_sched.c, .free_job points to v3d_sched_job_free(), which in turn calls v3d_job_cleanup() (and then drm_sched_job_cleanup). So, it looks ok. Also, we can say that the very last v3d_job_put() is in charge to decrement refcount initialized (set 1) in v3d_job_init(); while the v3d_job_cleanup() from v3d_sched_job_free() callback decrements refcount that was incremented when v3d_job_push() pushed the job to the scheduler. So, refcount pairs seem consistent, I mean, get and put. And about drm_sched_job_cleanup, it is explicitly called when job_init fails or after that by the scheduler. Thanks, Melissa > > Iago > > > + return; > > + > > + for (i = 0; i < se->out_sync_count; i++) > > + drm_syncobj_put(se->out_syncs[i].syncobj); > > + kvfree(se->out_syncs); > > +} > > + > > +static int > > +v3d_get_multisync_post_deps(struct drm_file *file_priv, > > + struct v3d_submit_ext *se, > > + u32 count, u64 handles) > > +{ > > + struct drm_v3d_sem __user *post_deps; > > + int i, ret; > > + > > + if (!count) > > + return 0; > > + > > + se->out_syncs = (struct v3d_submit_outsync *) > > + kvmalloc_array(count, > > + sizeof(struct > > v3d_submit_outsync), > > + GFP_KERNEL); > > + if (!se->out_syncs) > > + return -ENOMEM; > > + > > + post_deps = u64_to_user_ptr(handles); > > + > > + for (i = 0; i < count; i++) { > > + struct drm_v3d_sem out; > > + > > + ret = copy_from_user(&out, post_deps++, sizeof(out)); > > + if (ret) { > > +
Re: [Intel-gfx] [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts
+ Daniel as reviewer and maybe merge, avoid falling through cracks at least. On 22/09/2021 20:43, Matthew Brost wrote: Set number of engines before attempting to create contexts so the function free_engines can clean up properly. Also check return of alloc_engines for NULL. v2: (Tvrtko) - Send as stand alone patch (John Harrison) - Check for alloc_engines returning NULL Cc: Jason Ekstrand Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create parameters (v5)") Signed-off-by: Matthew Brost Cc: --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index c2ab0e22db0a..9627c7aac6a3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -898,6 +898,11 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx, unsigned int n; e = alloc_engines(num_engines); + if (!e) { + return ERR_PTR(-ENOMEM); + } Ideally remove the braces and respin. + e->num_engines = num_engines; Theoretically you could have put it next to "e->engines[n] = ce" assignment so the pattern is the same as in default_engines(). Kind of makes more sense that the number is not set before anything is created, but as it doesn't really matter since free_engines handles sparse arrays so there is argument to have a simpler single assignment as well. + for (n = 0; n < num_engines; n++) { struct intel_context *ce; int ret; @@ -931,7 +936,6 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx, goto free_engines; } } - e->num_engines = num_engines; return e; Fix looks good to me. I did not want to butt in but since more than a week has passed without it getting noticed: Reviewed-by: Tvrtko Ursulin Regards, Tvrtko
Re: [PATCH v3 4/4] drm/v3d: add multiple syncobjs support
On Fri, 2021-10-01 at 09:37 +0100, Melissa Wen wrote: > On 10/01, Iago Toral wrote: > > On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote: > > > Using the generic extension from the previous patch, a specific > > > multisync > > > extension enables more than one in/out binary syncobj per job > > > submission. > > > Arrays of syncobjs are set in struct drm_v3d_multisync, that also > > > cares > > > of determining the stage for sync (wait deps) according to the > > > job > > > queue. > > > > > > v2: > > > - subclass the generic extension struct (Daniel) > > > - simplify adding dependency conditions to make understandable > > > (Iago) > > > > > > v3: > > > - fix conditions to consider single or multiples in/out_syncs > > > (Iago) > > > - remove irrelevant comment (Iago) > > > > > > Signed-off-by: Melissa Wen > > > --- > > > drivers/gpu/drm/v3d/v3d_drv.c | 6 +- > > > drivers/gpu/drm/v3d/v3d_drv.h | 24 +++-- > > > drivers/gpu/drm/v3d/v3d_gem.c | 185 > > > ++ > > > > > > include/uapi/drm/v3d_drm.h| 49 - > > > 4 files changed, 232 insertions(+), 32 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c > > > b/drivers/gpu/drm/v3d/v3d_drv.c > > > index 3d6b9bcce2f7..bd46396a1ae0 100644 > > > --- a/drivers/gpu/drm/v3d/v3d_drv.c > > > +++ b/drivers/gpu/drm/v3d/v3d_drv.c > > > @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct > > > drm_device > > > > (...) > > > > > @@ -516,9 +536,11 @@ > > > v3d_attach_fences_and_unlock_reservation(struct > > > drm_file *file_priv, > > >struct v3d_job *job, > > >struct ww_acquire_ctx > > > *acquire_ctx, > > >u32 out_sync, > > > + struct v3d_submit_ext *se, > > >struct dma_fence *done_fence) > > > { > > > struct drm_syncobj *sync_out; > > > + bool has_multisync = se && (se->flags & > > > > We always pass the 'se' pointer from a local variable allocated in > > the > > stack of the caller so it is never going to be NULL. > > > > I am happy to keep the NULL check if you want to protect against > > future > > changes just in case, but if we do that then... > > > > > DRM_V3D_EXT_ID_MULTI_SYNC); > > > int i; > > > > > > for (i = 0; i < job->bo_count; i++) { > > > > (...) > > > > > +static void > > > +v3d_put_multisync_post_deps(struct v3d_submit_ext *se) > > > +{ > > > + unsigned int i; > > > + > > > + if (!se->out_sync_count) > > > > ...we should also check for NULL here for consistency. > yes, consistency makes sense here. > > Also, I think there is another problem in the code: we always call > > v3d_job_cleanup for failed jobs, but only call v3d_job_put for > > successful jobs. However, reading the docs for drm_sched_job_init: > > > > "Drivers must make sure drm_sched_job_cleanup() if this function > > returns successfully, even when @job is aborted before > > drm_sched_job_arm() is called." > > > > So my understanding is that we should call v3d_job_cleanup instead > > of > > v3d_job_put for successful jobs or we would be leaking sched > > resources > > on every successful job, no? > > When job_init is successful, v3d_job_cleanup is called by scheduler > when > job is completed. drm_sched_job_cleanup describes how it works after > drm_sched_job_arm: > > " After that point of no return @job is committed to be executed by > the > scheduler, and this function should be called from the > &drm_sched_backend_ops.free_job callback." > > On v3d_sched.c, .free_job points to v3d_sched_job_free(), which in > turn > calls v3d_job_cleanup() (and then drm_sched_job_cleanup). So, it > looks > ok. > > Also, we can say that the very last v3d_job_put() is in charge to > decrement refcount initialized (set 1) in v3d_job_init(); while the > v3d_job_cleanup() from v3d_sched_job_free() callback decrements > refcount > that was incremented when v3d_job_push() pushed the job to the > scheduler. > > So, refcount pairs seem consistent, I mean, get and put. And about > drm_sched_job_cleanup, it is explicitly called when job_init fails or > after that by the scheduler. > A. Ah ok, thanks for explaining this! With the consistency issue discussed above fixed, for the series: Reviewed-by: Iago Toral Quiroga Iago
Re: [PATCH v3 4/4] drm/v3d: add multiple syncobjs support
On 10/01, Iago Toral wrote: > On Fri, 2021-10-01 at 09:37 +0100, Melissa Wen wrote: > > On 10/01, Iago Toral wrote: > > > On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote: > > > > Using the generic extension from the previous patch, a specific > > > > multisync > > > > extension enables more than one in/out binary syncobj per job > > > > submission. > > > > Arrays of syncobjs are set in struct drm_v3d_multisync, that also > > > > cares > > > > of determining the stage for sync (wait deps) according to the > > > > job > > > > queue. > > > > > > > > v2: > > > > - subclass the generic extension struct (Daniel) > > > > - simplify adding dependency conditions to make understandable > > > > (Iago) > > > > > > > > v3: > > > > - fix conditions to consider single or multiples in/out_syncs > > > > (Iago) > > > > - remove irrelevant comment (Iago) > > > > > > > > Signed-off-by: Melissa Wen > > > > --- > > > > drivers/gpu/drm/v3d/v3d_drv.c | 6 +- > > > > drivers/gpu/drm/v3d/v3d_drv.h | 24 +++-- > > > > drivers/gpu/drm/v3d/v3d_gem.c | 185 > > > > ++ > > > > > > > > include/uapi/drm/v3d_drm.h| 49 - > > > > 4 files changed, 232 insertions(+), 32 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c > > > > b/drivers/gpu/drm/v3d/v3d_drv.c > > > > index 3d6b9bcce2f7..bd46396a1ae0 100644 > > > > --- a/drivers/gpu/drm/v3d/v3d_drv.c > > > > +++ b/drivers/gpu/drm/v3d/v3d_drv.c > > > > @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct > > > > drm_device > > > > > > (...) > > > > > > > @@ -516,9 +536,11 @@ > > > > v3d_attach_fences_and_unlock_reservation(struct > > > > drm_file *file_priv, > > > > struct v3d_job *job, > > > > struct ww_acquire_ctx > > > > *acquire_ctx, > > > > u32 out_sync, > > > > +struct v3d_submit_ext *se, > > > > struct dma_fence *done_fence) > > > > { > > > > struct drm_syncobj *sync_out; > > > > + bool has_multisync = se && (se->flags & > > > > > > We always pass the 'se' pointer from a local variable allocated in > > > the > > > stack of the caller so it is never going to be NULL. > > > > > > I am happy to keep the NULL check if you want to protect against > > > future > > > changes just in case, but if we do that then... > > > > > > > DRM_V3D_EXT_ID_MULTI_SYNC); > > > > int i; > > > > > > > > for (i = 0; i < job->bo_count; i++) { > > > > > > (...) > > > > > > > +static void > > > > +v3d_put_multisync_post_deps(struct v3d_submit_ext *se) > > > > +{ > > > > + unsigned int i; > > > > + > > > > + if (!se->out_sync_count) > > > > > > ...we should also check for NULL here for consistency. > > yes, consistency makes sense here. > > > Also, I think there is another problem in the code: we always call > > > v3d_job_cleanup for failed jobs, but only call v3d_job_put for > > > successful jobs. However, reading the docs for drm_sched_job_init: > > > > > > "Drivers must make sure drm_sched_job_cleanup() if this function > > > returns successfully, even when @job is aborted before > > > drm_sched_job_arm() is called." > > > > > > So my understanding is that we should call v3d_job_cleanup instead > > > of > > > v3d_job_put for successful jobs or we would be leaking sched > > > resources > > > on every successful job, no? > > > > When job_init is successful, v3d_job_cleanup is called by scheduler > > when > > job is completed. drm_sched_job_cleanup describes how it works after > > drm_sched_job_arm: > > > > " After that point of no return @job is committed to be executed by > > the > > scheduler, and this function should be called from the > > &drm_sched_backend_ops.free_job callback." > > > > On v3d_sched.c, .free_job points to v3d_sched_job_free(), which in > > turn > > calls v3d_job_cleanup() (and then drm_sched_job_cleanup). So, it > > looks > > ok. > > > > Also, we can say that the very last v3d_job_put() is in charge to > > decrement refcount initialized (set 1) in v3d_job_init(); while the > > v3d_job_cleanup() from v3d_sched_job_free() callback decrements > > refcount > > that was incremented when v3d_job_push() pushed the job to the > > scheduler. > > > > So, refcount pairs seem consistent, I mean, get and put. And about > > drm_sched_job_cleanup, it is explicitly called when job_init fails or > > after that by the scheduler. > > > >A. Ah ok, thanks for explaining this! nice! > > With the consistency issue discussed above fixed, for the series: > > Reviewed-by: Iago Toral Quiroga ok, thanks for reviewing and all improvement suggestions, Melissa > > Iago > signature.asc Description: PGP signature
Re: [RFC 1/6] sched: Add nice value change notifier
Hi Peter, On 30/09/2021 19:33, Peter Zijlstra wrote: On Thu, Sep 30, 2021 at 06:15:47PM +0100, Tvrtko Ursulin wrote: void set_user_nice(struct task_struct *p, long nice) { bool queued, running; - int old_prio; + int old_prio, ret; struct rq_flags rf; struct rq *rq; @@ -6913,6 +6945,9 @@ void set_user_nice(struct task_struct *p, long nice) */ p->sched_class->prio_changed(rq, p, old_prio); + ret = atomic_notifier_call_chain(&user_nice_notifier_list, nice, p); + WARN_ON_ONCE(ret != NOTIFY_DONE); + out_unlock: task_rq_unlock(rq, p, &rf); } No, we're not going to call out to exported, and potentially unbounded, functions under scheduler locks. Agreed, that's another good point why it is even more hairy, as I have generally alluded in the cover letter. Do you have any immediate thoughts on possible alternatives? Like for instance if I did a queue_work from set_user_nice and then ran a notifier chain async from a worker? I haven't looked at yet what repercussion would that have in terms of having to cancel the pending workers when tasks exit. I can try and prototype that and see how it would look. There is of course an example ioprio which solves the runtime adjustments via a dedicated system call. But I don't currently feel that a third one would be a good solution. At least I don't see a case for being able to decouple the priority of CPU and GPU and computations. Have I opened a large can of worms? :) Regards, Tvrtko
[PATCH v2] drm/locking: add backtrace for locking contended locks without backoff
If drm_modeset_lock() returns -EDEADLK, the caller is supposed to drop all currently held locks using drm_modeset_backoff(). Failing to do so will result in warnings and backtraces on the paths trying to lock a contended lock. Add support for optionally printing the backtrace on the path that hit the deadlock and didn't gracefully handle the situation. For example, the patch [1] inadvertently dropped the return value check and error return on replacing calc_watermark_data() with intel_compute_global_watermarks(). The backtraces on the subsequent locking paths hitting WARN_ON(ctx->contended) were unhelpful, but adding the backtrace to the deadlock path produced this helpful printout: <7> [98.002465] drm_modeset_lock attempting to lock a contended lock without backoff: drm_modeset_lock+0x107/0x130 drm_atomic_get_plane_state+0x76/0x150 skl_compute_wm+0x251d/0x2b20 [i915] intel_atomic_check+0x1942/0x29e0 [i915] drm_atomic_check_only+0x554/0x910 drm_atomic_nonblocking_commit+0xe/0x50 drm_mode_atomic_ioctl+0x8c2/0xab0 drm_ioctl_kernel+0xac/0x140 Add new CONFIG_DRM_DEBUG_MODESET_LOCK to enable modeset lock debugging with stack depot and trace. [1] https://lore.kernel.org/r/20210924114741.15940-4-jani.nik...@intel.com v2: - default y if DEBUG_WW_MUTEX_SLOWPATH (Daniel) - depends on DEBUG_KERNEL Cc: Daniel Vetter Cc: Dave Airlie Reviewed-by: Daniel Vetter Signed-off-by: Jani Nikula --- drivers/gpu/drm/Kconfig| 15 + drivers/gpu/drm/drm_modeset_lock.c | 49 -- include/drm/drm_modeset_lock.h | 8 + 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 2a926d0de423..a4c020a9a0eb 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -100,6 +100,21 @@ config DRM_DEBUG_DP_MST_TOPOLOGY_REFS This has the potential to use a lot of memory and print some very large kernel messages. If in doubt, say "N". +config DRM_DEBUG_MODESET_LOCK + bool "Enable backtrace history for lock contention" + depends on STACKTRACE_SUPPORT + depends on DEBUG_KERNEL + depends on EXPERT + select STACKDEPOT + default y if DEBUG_WW_MUTEX_SLOWPATH + help + Enable debug tracing of failures to gracefully handle drm modeset lock + contention. A history of each drm modeset lock path hitting -EDEADLK + will be saved until gracefully handled, and the backtrace will be + printed when attempting to lock a contended lock. + + If in doubt, say "N". + config DRM_FBDEV_EMULATION bool "Enable legacy fbdev support for your modesetting driver" depends on DRM diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index bf8a6e823a15..4d32b61fa1fd 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -25,6 +25,7 @@ #include #include #include +#include /** * DOC: kms locking @@ -77,6 +78,45 @@ static DEFINE_WW_CLASS(crtc_ww_class); +#if IS_ENABLED(CONFIG_DRM_DEBUG_MODESET_LOCK) +static noinline depot_stack_handle_t __stack_depot_save(void) +{ + unsigned long entries[8]; + unsigned int n; + + n = stack_trace_save(entries, ARRAY_SIZE(entries), 1); + + return stack_depot_save(entries, n, GFP_NOWAIT | __GFP_NOWARN); +} + +static void __stack_depot_print(depot_stack_handle_t stack_depot) +{ + struct drm_printer p = drm_debug_printer("drm_modeset_lock"); + unsigned long *entries; + unsigned int nr_entries; + char *buf; + + buf = kmalloc(PAGE_SIZE, GFP_NOWAIT | __GFP_NOWARN); + if (!buf) + return; + + nr_entries = stack_depot_fetch(stack_depot, &entries); + stack_trace_snprint(buf, PAGE_SIZE, entries, nr_entries, 2); + + drm_printf(&p, "attempting to lock a contended lock without backoff:\n%s", buf); + + kfree(buf); +} +#else /* CONFIG_DRM_DEBUG_MODESET_LOCK */ +static depot_stack_handle_t __stack_depot_save(void) +{ + return 0; +} +static void __stack_depot_print(depot_stack_handle_t stack_depot) +{ +} +#endif /* CONFIG_DRM_DEBUG_MODESET_LOCK */ + /** * drm_modeset_lock_all - take all modeset locks * @dev: DRM device @@ -225,7 +265,9 @@ EXPORT_SYMBOL(drm_modeset_acquire_fini); */ void drm_modeset_drop_locks(struct drm_modeset_acquire_ctx *ctx) { - WARN_ON(ctx->contended); + if (WARN_ON(ctx->contended)) + __stack_depot_print(ctx->stack_depot); + while (!list_empty(&ctx->locked)) { struct drm_modeset_lock *lock; @@ -243,7 +285,8 @@ static inline int modeset_lock(struct drm_modeset_lock *lock, { int ret; - WARN_ON(ctx->contended); + if (WARN_ON(ctx->contended)) + __stack_depot_print(ctx->stack_depot); if (ctx->trylock_only) { lockdep_assert_held(&ctx->ww_ctx); @@ -274,6 +317,7
Re: [PATCH] drm/locking: add backtrace for locking contended locks without backoff
On Thu, 30 Sep 2021, Daniel Vetter wrote: > On Wed, Sep 29, 2021 at 01:32:41AM +0300, Jani Nikula wrote: >> If drm_modeset_lock() returns -EDEADLK, the caller is supposed to drop >> all currently held locks using drm_modeset_backoff(). Failing to do so >> will result in warnings and backtraces on the paths trying to lock a >> contended lock. Add support for optionally printing the backtrace on the >> path that hit the deadlock and didn't gracefully handle the situation. >> >> For example, the patch [1] inadvertently dropped the return value check >> and error return on replacing calc_watermark_data() with >> intel_compute_global_watermarks(). The backtraces on the subsequent >> locking paths hitting WARN_ON(ctx->contended) were unhelpful, but adding >> the backtrace to the deadlock path produced this helpful printout: >> >> <7> [98.002465] drm_modeset_lock attempting to lock a contended lock without >> backoff: >>drm_modeset_lock+0x107/0x130 >>drm_atomic_get_plane_state+0x76/0x150 >>skl_compute_wm+0x251d/0x2b20 [i915] >>intel_atomic_check+0x1942/0x29e0 [i915] >>drm_atomic_check_only+0x554/0x910 >>drm_atomic_nonblocking_commit+0xe/0x50 >>drm_mode_atomic_ioctl+0x8c2/0xab0 >>drm_ioctl_kernel+0xac/0x140 >> >> Add new CONFIG_DRM_DEBUG_MODESET_LOCK to enable modeset lock debugging >> with stack depot and trace. >> >> [1] https://lore.kernel.org/r/20210924114741.15940-4-jani.nik...@intel.com >> >> Cc: Daniel Vetter >> Cc: Dave Airlie >> Signed-off-by: Jani Nikula > > I wonder whether we shouldn't just enable this when lock debugging is > enabled? Otherwise we need to make sure CI have this set or it's not very > useful. Or at least a default y if CONFIG_DEBUG_WW_MUTEX_SLOWPATH or > something like that. Added the conditional default y, as well as depends on DEBUG_KERNEL. > > Either way: > > Reviewed-by: Daniel Vetter Thanks, Jani. > >> --- >> drivers/gpu/drm/Kconfig| 13 >> drivers/gpu/drm/drm_modeset_lock.c | 49 -- >> include/drm/drm_modeset_lock.h | 8 + >> 3 files changed, 68 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >> index b17e231ca6f7..7334975c788b 100644 >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -100,6 +100,19 @@ config DRM_DEBUG_DP_MST_TOPOLOGY_REFS >>This has the potential to use a lot of memory and print some very >>large kernel messages. If in doubt, say "N". >> >> +config DRM_DEBUG_MODESET_LOCK >> +bool "Enable backtrace history for lock contention" >> +depends on STACKTRACE_SUPPORT >> +select STACKDEPOT >> +depends on EXPERT >> +help >> + Enable debug tracing of failures to gracefully handle drm modeset lock >> + contention. A history of each drm modeset lock path hitting -EDEADLK >> + will be saved until gracefully handled, and the backtrace will be >> + printed when attempting to lock a contended lock. >> + >> + If in doubt, say "N". >> + >> config DRM_FBDEV_EMULATION >> bool "Enable legacy fbdev support for your modesetting driver" >> depends on DRM >> diff --git a/drivers/gpu/drm/drm_modeset_lock.c >> b/drivers/gpu/drm/drm_modeset_lock.c >> index bf8a6e823a15..4d32b61fa1fd 100644 >> --- a/drivers/gpu/drm/drm_modeset_lock.c >> +++ b/drivers/gpu/drm/drm_modeset_lock.c >> @@ -25,6 +25,7 @@ >> #include >> #include >> #include >> +#include >> >> /** >> * DOC: kms locking >> @@ -77,6 +78,45 @@ >> >> static DEFINE_WW_CLASS(crtc_ww_class); >> >> +#if IS_ENABLED(CONFIG_DRM_DEBUG_MODESET_LOCK) >> +static noinline depot_stack_handle_t __stack_depot_save(void) >> +{ >> +unsigned long entries[8]; >> +unsigned int n; >> + >> +n = stack_trace_save(entries, ARRAY_SIZE(entries), 1); >> + >> +return stack_depot_save(entries, n, GFP_NOWAIT | __GFP_NOWARN); >> +} >> + >> +static void __stack_depot_print(depot_stack_handle_t stack_depot) >> +{ >> +struct drm_printer p = drm_debug_printer("drm_modeset_lock"); >> +unsigned long *entries; >> +unsigned int nr_entries; >> +char *buf; >> + >> +buf = kmalloc(PAGE_SIZE, GFP_NOWAIT | __GFP_NOWARN); >> +if (!buf) >> +return; >> + >> +nr_entries = stack_depot_fetch(stack_depot, &entries); >> +stack_trace_snprint(buf, PAGE_SIZE, entries, nr_entries, 2); >> + >> +drm_printf(&p, "attempting to lock a contended lock without >> backoff:\n%s", buf); >> + >> +kfree(buf); >> +} >> +#else /* CONFIG_DRM_DEBUG_MODESET_LOCK */ >> +static depot_stack_handle_t __stack_depot_save(void) >> +{ >> +return 0; >> +} >> +static void __stack_depot_print(depot_stack_handle_t stack_depot) >> +{ >> +} >> +#endif /* CONFIG_DRM_DEBUG_MODESET_LOCK */ >> + >> /** >> * drm_modeset_lock_all - take all modeset locks >> * @dev: DRM device >> @@ -225,7 +265,9 @@ EXPORT_SYMBOL(drm_modeset_acquire_fini); >> */ >> void drm_modeset_drop_locks(s
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock()
On 2021-09-16 11:38:55 [+0200], Maarten Lankhorst wrote: > Patches look good. Thank you for looking. > For both patches: > > Reviewed-by: Maarten Lankhorst > > I've been looking at running i915 with the -rt patch series, and > noticed i915_request_submit fails with GEM_BUG_ON(!irqs_disabled()); > presumably same failure exists for i915_request_unsubmit(). > > Might be worth removing those checks as well? Seems double with > lockdep_assert_held on an irq lock anyway. yes, let me prepare something in a few. > I've also noticed the local_irq_disable/enable is removed from > intel_pipe_update_(start/end) in the rt series. It might make sense > from a -rt point of view, but that code needs to run without > interruptions, or i915 may show visual glitches or even locks up the > system. > > It should just be a set of registers hammered in, but the code might > needs to be fixed to take the mmio lock as outer lock, and become a > strict set of register read/writes only. Let me see. So Anton Lundin (Cc:) reported glitches due to _this_ patch on -RT. I have just a Sandybridge around with a i915 and it does not get near that code here. > ~Maarten Sebastian
[PATCH v3 0/6] drm/mediatek: Add mt8195 DisplayPort driver
Hi everyone, this series is built around the DisplayPort driver. The dpi/dpintf driver and the added helper functions are required for the DisplayPort driver to work. For v3 I fixed/removed obsolete TODOs in the driver code and fixed one feedback comment regarding a 'DRM_' prefix for the CEA_SAD defines. The series is still based on v5.15-rc1 but also applies cleanly on linux-next at the moment. There still is a functional dependency on many different patches pulled in through the main two dependencies, vdosys0 and vdosys1, but I still don't have a stable and clean base with these. Note: This patch series is currently tested on v5.10 and I am still working on testing it on v5.15. Dependencies: - Add Mediatek Soc DRM (vdosys0) support for mt8195 https://lore.kernel.org/linux-mediatek/20210825144833.7757-1-jason-jh@mediatek.com/ - Add MediaTek SoC DRM (vdosys1) support for mt8195 https://lore.kernel.org/linux-mediatek/20210825100531.5653-1-nancy@mediatek.com/ Older revisions: RFC - https://lore.kernel.org/linux-mediatek/20210816192523.1739365-1-...@baylibre.com/ v1 - https://lore.kernel.org/linux-mediatek/20210906193529.718845-1-...@baylibre.com/ v2 - https://lore.kernel.org/linux-mediatek/20210920084424.231825-1-...@baylibre.com/ Thanks in advance for any feedback and comments. Best, Markus Markus Schneider-Pargmann (6): dt-bindings: mediatek,dpintf: Add DP_INTF binding dt-bindings: mediatek,dp: Add Display Port binding drm/edid: Add cea_sad helpers for freq/length video/hdmi: Add audio_infoframe packing for DP drm/mediatek: dpi: Add dpintf support drm/mediatek: Add mt8195 DisplayPort driver .../display/mediatek/mediatek,dp.yaml | 89 + .../display/mediatek/mediatek,dpintf.yaml | 78 + drivers/gpu/drm/drm_edid.c| 74 + drivers/gpu/drm/mediatek/Kconfig |7 + drivers/gpu/drm/mediatek/Makefile |2 + drivers/gpu/drm/mediatek/mtk_dp.c | 2825 + drivers/gpu/drm/mediatek/mtk_dp_reg.h | 535 drivers/gpu/drm/mediatek/mtk_dpi.c| 248 +- drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 12 + drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |4 + drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |1 + drivers/gpu/drm/mediatek/mtk_drm_drv.c|6 +- drivers/gpu/drm/mediatek/mtk_drm_drv.h|1 + drivers/phy/mediatek/Kconfig |8 + drivers/phy/mediatek/Makefile |1 + drivers/phy/mediatek/phy-mtk-dp.c | 218 ++ drivers/video/hdmi.c | 83 +- include/drm/drm_dp_helper.h |2 + include/drm/drm_edid.h| 18 +- include/linux/hdmi.h |7 +- include/linux/soc/mediatek/mtk-mmsys.h|2 + 21 files changed, 4147 insertions(+), 74 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,dpintf.yaml create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h create mode 100644 drivers/phy/mediatek/phy-mtk-dp.c -- 2.33.0
[PATCH v3 1/6] dt-bindings: mediatek,dpintf: Add DP_INTF binding
DP_INTF is a similar functional block to mediatek,dpi but is different in that it serves the DisplayPort controller on mediatek SoCs and uses different clocks. Therefore this patch creates a new binding file for this functional block. Signed-off-by: Markus Schneider-Pargmann --- Notes: Changes v1 -> v2: - Move the devicetree binding from mediatek,dpi into its own binding file. .../display/mediatek/mediatek,dpintf.yaml | 78 +++ 1 file changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,dpintf.yaml diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpintf.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpintf.yaml new file mode 100644 index ..ac1fd93327e6 --- /dev/null +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpintf.yaml @@ -0,0 +1,78 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/mediatek/mediatek,dpintf.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Mediatek DP_INTF Controller Device Tree Bindings + +maintainers: + - CK Hu + - Jitao shi + +description: | + The Mediatek DP_INTF function block is a sink of the display subsystem + connected to the display port controller. + +properties: + compatible: +enum: + - mediatek,mt8195-dpintf + + reg: +maxItems: 1 + + interrupts: +maxItems: 1 + + clocks: +items: + - description: hf_fmm Clock + - description: hf_fdp Clock + - description: Pixel Clock + - description: DP_INTF PLL + + clock-names: +items: + - const: hf_fmm + - const: hf_fdp + - const: pixel + - const: pll + + port: +$ref: /schemas/graph.yaml#/properties/port +description: + Output port node. This port should be connected to the input port of an + attached display port controller. + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - port + +additionalProperties: false + +examples: + - | +#include +#include +#include + + dp_intf1: dp_intf1@1c113000 { + compatible = "mediatek,mt8195-dpintf"; + reg = <0 0x1c113000 0 0x1000>; + interrupts = ; + clocks = <&vdosys1 CLK_VDO1_DP_INTF0_MM>, +<&vdosys1 CLK_VDO1_DPINTF>, +<&topckgen CLK_TOP_DP_SEL>, +<&topckgen CLK_TOP_TVDPLL2>; + clock-names = "hf_fmm", + "hf_fdp", + "pixel", + "pll"; + }; + +... -- 2.33.0
[PATCH v3 2/6] dt-bindings: mediatek,dp: Add Display Port binding
This controller is present on several mediatek hardware. Currently mt8195 and mt8395 have this controller without a functional difference, so only one compatible field is added. The controller can have two forms, as a normal display port and as an embedded display port. Signed-off-by: Markus Schneider-Pargmann --- .../display/mediatek/mediatek,dp.yaml | 89 +++ 1 file changed, 89 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml new file mode 100644 index ..f7a35962c23b --- /dev/null +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml @@ -0,0 +1,89 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/mediatek/mediatek,dp.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Mediatek Display Port Controller + +maintainers: + - CK Hu + - Jitao shi + +description: | + Device tree bindings for the Mediatek (embedded) Display Port controller + present on some Mediatek SoCs. + +properties: + compatible: +enum: + - mediatek,mt8195-edp_tx + - mediatek,mt8195-dp_tx + + reg: +maxItems: 1 + + interrupts: +maxItems: 1 + + clocks: +items: + - description: faxi clock + + clock-names: +items: + - const: faxi + + power-domains: +maxItems: 1 + + ports: +$ref: /schemas/graph.yaml#/properties/ports +properties: + port@0: +$ref: /schemas/graph.yaml#/properties/port +description: Input endpoint of the controller, usually dp_intf + + port@1: +$ref: /schemas/graph.yaml#/properties/port +description: Output endpoint of the controller + +required: + - compatible + - reg + - interrupts + - ports + +additionalProperties: false + +examples: + - | +#include +#include +dp_tx: edp_tx@1c50 { +compatible = "mediatek,mt8195-edp_tx"; +reg = <0 0x1c50 0 0x8000>; +interrupts = ; +power-domains = <&spm MT8195_POWER_DOMAIN_EPD_TX>; +pinctrl-names = "default"; +pinctrl-0 = <&edp_pin>; +status = "okay"; + +ports { +#address-cells = <1>; +#size-cells = <0>; + +port@0 { +reg = <0>; +edp_in: endpoint { +remote-endpoint = <&dp_intf0_out>; +}; +}; +port@1 { +reg = <1>; +edp_out: endpoint { + remote-endpoint = <&panel_in>; +}; +}; +}; +}; -- 2.33.0
[PATCH v3 3/6] drm/edid: Add cea_sad helpers for freq/length
This patch adds two helper functions that extract the frequency and word length from a struct cea_sad. For these helper functions new defines are added that help translate the 'freq' and 'byte2' fields into real numbers. Signed-off-by: Markus Schneider-Pargmann --- Notes: Changes v2 -> v3: - Add DRM_ prefix to the CEA_SAD defines. Changes v1 -> v2: - Use const struct pointers. - Add a check whether the format is actually uncompressed or not. drivers/gpu/drm/drm_edid.c | 74 ++ include/drm/drm_edid.h | 18 -- 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 6325877c5fd6..c134803e18db 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4666,6 +4666,80 @@ int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb) } EXPORT_SYMBOL(drm_edid_to_speaker_allocation); +/** + * drm_cea_sad_get_sample_rate - Extract the sample rate from cea_sad + * @sad: Pointer to the cea_sad struct + * + * Extracts the cea_sad frequency field and returns the sample rate in Hz. + * + * Return: Sample rate in Hz or a negative errno if parsing failed. + */ +int drm_cea_sad_get_sample_rate(const struct cea_sad *sad) +{ + switch (sad->freq) { + case DRM_CEA_SAD_FREQ_32KHZ: + return 32000; + case DRM_CEA_SAD_FREQ_44KHZ: + return 44100; + case DRM_CEA_SAD_FREQ_48KHZ: + return 48000; + case DRM_CEA_SAD_FREQ_88KHZ: + return 88200; + case DRM_CEA_SAD_FREQ_96KHZ: + return 96000; + case DRM_CEA_SAD_FREQ_176KHZ: + return 176400; + case DRM_CEA_SAD_FREQ_192KHZ: + return 192000; + default: + return -EINVAL; + } +} +EXPORT_SYMBOL(drm_cea_sad_get_sample_rate); + +static bool drm_cea_sad_is_uncompressed(const struct cea_sad *sad) +{ + switch (sad->format) { + case HDMI_AUDIO_CODING_TYPE_STREAM: + case HDMI_AUDIO_CODING_TYPE_PCM: + return true; + default: + return false; + } +} + +/** + * drm_cea_sad_get_uncompressed_word_length - Extract word length + * @sad: Pointer to the cea_sad struct + * + * Extracts the cea_sad byte2 field and returns the word length for an + * uncompressed stream. + * + * Note: This function may only be called for uncompressed audio. + * + * Return: Word length in bits or a negative errno if parsing failed. + */ +int drm_cea_sad_get_uncompressed_word_length(const struct cea_sad *sad) +{ + if (!drm_cea_sad_is_uncompressed(sad)) { + DRM_WARN("Unable to get the uncompressed word length for a compressed format: %u\n", +sad->format); + return -EINVAL; + } + + switch (sad->byte2) { + case DRM_CEA_SAD_UNCOMPRESSED_WORD_16BIT: + return 16; + case DRM_CEA_SAD_UNCOMPRESSED_WORD_20BIT: + return 20; + case DRM_CEA_SAD_UNCOMPRESSED_WORD_24BIT: + return 24; + default: + return -EINVAL; + } +} +EXPORT_SYMBOL(drm_cea_sad_get_uncompressed_word_length); + /** * drm_av_sync_delay - compute the HDMI/DP sink audio-video sync delay * @connector: connector associated with the HDMI/DP sink diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index deccfd39e6db..9d75df652b17 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -361,12 +361,24 @@ struct edid { /* Short Audio Descriptor */ struct cea_sad { - u8 format; + u8 format; /* See HDMI_AUDIO_CODING_TYPE_* */ u8 channels; /* max number of channels - 1 */ - u8 freq; + u8 freq; /* See CEA_SAD_FREQ_* */ u8 byte2; /* meaning depends on format */ }; +#define DRM_CEA_SAD_FREQ_32KHZ BIT(0) +#define DRM_CEA_SAD_FREQ_44KHZ BIT(1) +#define DRM_CEA_SAD_FREQ_48KHZ BIT(2) +#define DRM_CEA_SAD_FREQ_88KHZ BIT(3) +#define DRM_CEA_SAD_FREQ_96KHZ BIT(4) +#define DRM_CEA_SAD_FREQ_176KHZ BIT(5) +#define DRM_CEA_SAD_FREQ_192KHZ BIT(6) + +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_16BIT BIT(0) +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_20BIT BIT(1) +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_24BIT BIT(2) + struct drm_encoder; struct drm_connector; struct drm_connector_state; @@ -374,6 +386,8 @@ struct drm_display_mode; int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads); int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb); +int drm_cea_sad_get_sample_rate(const struct cea_sad *sad); +int drm_cea_sad_get_uncompressed_word_length(const struct cea_sad *sad); int drm_av_sync_delay(struct drm_connector *connector, const struct drm_display_mode *mode); -- 2.33.0
[PATCH v3 4/6] video/hdmi: Add audio_infoframe packing for DP
Similar to HDMI, DP uses audio infoframes as well which are structured very similar to the HDMI ones. This patch adds a helper function to pack the HDMI audio infoframe for DP, called hdmi_audio_infoframe_pack_for_dp(). hdmi_audio_infoframe_pack_only() is split into two parts. One of them packs the payload only and can be used for HDMI and DP. Signed-off-by: Markus Schneider-Pargmann --- Notes: Changes v1 -> v2: - Create a define for HB2. - Use struct dp_sdp to pass data in a better way. drivers/video/hdmi.c| 83 - include/drm/drm_dp_helper.h | 2 + include/linux/hdmi.h| 7 +++- 3 files changed, 72 insertions(+), 20 deletions(-) diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index 947be761dfa4..63e74d9fd210 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -21,6 +21,7 @@ * DEALINGS IN THE SOFTWARE. */ +#include #include #include #include @@ -381,12 +382,34 @@ static int hdmi_audio_infoframe_check_only(const struct hdmi_audio_infoframe *fr * * Returns 0 on success or a negative error code on failure. */ -int hdmi_audio_infoframe_check(struct hdmi_audio_infoframe *frame) +int hdmi_audio_infoframe_check(const struct hdmi_audio_infoframe *frame) { return hdmi_audio_infoframe_check_only(frame); } EXPORT_SYMBOL(hdmi_audio_infoframe_check); +static void +hdmi_audio_infoframe_pack_payload(const struct hdmi_audio_infoframe *frame, + u8 *buffer) +{ + u8 channels; + + if (frame->channels >= 2) + channels = frame->channels - 1; + else + channels = 0; + + buffer[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7); + buffer[1] = ((frame->sample_frequency & 0x7) << 2) | +(frame->sample_size & 0x3); + buffer[2] = frame->coding_type_ext & 0x1f; + buffer[3] = frame->channel_allocation; + buffer[4] = (frame->level_shift_value & 0xf) << 3; + + if (frame->downmix_inhibit) + buffer[4] |= BIT(7); +} + /** * hdmi_audio_infoframe_pack_only() - write HDMI audio infoframe to binary buffer * @frame: HDMI audio infoframe @@ -404,7 +427,6 @@ EXPORT_SYMBOL(hdmi_audio_infoframe_check); ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame, void *buffer, size_t size) { - unsigned char channels; u8 *ptr = buffer; size_t length; int ret; @@ -420,28 +442,13 @@ ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame, memset(buffer, 0, size); - if (frame->channels >= 2) - channels = frame->channels - 1; - else - channels = 0; - ptr[0] = frame->type; ptr[1] = frame->version; ptr[2] = frame->length; ptr[3] = 0; /* checksum */ - /* start infoframe payload */ - ptr += HDMI_INFOFRAME_HEADER_SIZE; - - ptr[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7); - ptr[1] = ((frame->sample_frequency & 0x7) << 2) | -(frame->sample_size & 0x3); - ptr[2] = frame->coding_type_ext & 0x1f; - ptr[3] = frame->channel_allocation; - ptr[4] = (frame->level_shift_value & 0xf) << 3; - - if (frame->downmix_inhibit) - ptr[4] |= BIT(7); + hdmi_audio_infoframe_pack_payload(frame, + ptr + HDMI_INFOFRAME_HEADER_SIZE); hdmi_infoframe_set_checksum(buffer, length); @@ -479,6 +486,44 @@ ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame, } EXPORT_SYMBOL(hdmi_audio_infoframe_pack); +/** + * hdmi_audio_infoframe_pack_for_dp - Pack a HDMI Audio infoframe for + *displayport + * + * @frame HDMI Audio infoframe + * @sdp secondary data packet for display port. This is filled with the + * appropriate data + * @dp_version Display Port version to be encoded in the header + * + * Packs a HDMI Audio Infoframe to be sent over Display Port. This function + * fills the secondary data packet to be used for Display Port. + * + * Return: Number of total written bytes or a negative errno on failure. + */ +ssize_t +hdmi_audio_infoframe_pack_for_dp(const struct hdmi_audio_infoframe *frame, +struct dp_sdp *sdp, u8 dp_version) +{ + int ret; + + ret = hdmi_audio_infoframe_check(frame); + if (ret) + return ret; + + memset(sdp->db, 0, sizeof(sdp->db)); + + // Secondary-data packet header + sdp->sdp_header.HB0 = 0; + sdp->sdp_header.HB1 = frame->type; + sdp->sdp_header.HB2 = DP_SDP_AUDIO_INFOFRAME_HB2; + sdp->sdp_header.HB3 = (dp_version & 0x3f) << 2; + + hdmi_audio_infoframe_pack_payload(frame, sdp->db); + + return frame->length + 4; +} +EXPORT_SYMBOL(hdmi_audio_infoframe_pack_for_dp); + /** * hdmi_vendor_in
[PATCH v3 5/6] drm/mediatek: dpi: Add dpintf support
dpintf is the displayport interface hardware unit. This unit is similar to dpi and can reuse most of the code. This patch adds support for mt8195-dpintf to this dpi driver. Main differences are: - Some features/functional components are not available for dpintf which are now excluded from code execution once is_dpintf is set - dpintf can and needs to choose between different clockdividers based on the clockspeed. This is done by choosing a different clock parent. - There are two additional clocks that need to be managed. These are only set for dpintf and will be set to NULL if not supplied. The clk_* calls handle these as normal clocks then. - Some register contents differ slightly between the two components. To work around this I added register bits/masks with a DPINTF_ prefix and use them where different. Based on a separate driver for dpintf created by Jason-JH.Lin . Signed-off-by: Markus Schneider-Pargmann --- Notes: Changes RFC -> v1: - Remove setting parents and fully rely on the clock tree instead which already models a mux at the important place. - Integrated mtk_dpi dpintf changes into the mediatek drm driver. drivers/gpu/drm/mediatek/mtk_dpi.c | 248 drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 12 + drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 4 + drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + drivers/gpu/drm/mediatek/mtk_drm_drv.c | 5 +- 5 files changed, 218 insertions(+), 52 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c index 4554e2de1430..87961ebf5d35 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi.c +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c @@ -71,6 +71,8 @@ struct mtk_dpi { void __iomem *regs; struct device *dev; struct clk *engine_clk; + struct clk *hf_fmm_clk; + struct clk *hf_fdp_clk; struct clk *pixel_clk; struct clk *tvd_clk; int irq; @@ -125,6 +127,7 @@ struct mtk_dpi_conf { bool edge_sel_en; const u32 *output_fmts; u32 num_output_fmts; + bool is_dpintf; }; static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask) @@ -153,30 +156,52 @@ static void mtk_dpi_disable(struct mtk_dpi *dpi) static void mtk_dpi_config_hsync(struct mtk_dpi *dpi, struct mtk_dpi_sync_param *sync) { - mtk_dpi_mask(dpi, DPI_TGEN_HWIDTH, -sync->sync_width << HPW, HPW_MASK); - mtk_dpi_mask(dpi, DPI_TGEN_HPORCH, -sync->back_porch << HBP, HBP_MASK); - mtk_dpi_mask(dpi, DPI_TGEN_HPORCH, sync->front_porch << HFP, -HFP_MASK); + if (dpi->conf->is_dpintf) { + mtk_dpi_mask(dpi, DPI_TGEN_HWIDTH, +sync->sync_width << HPW, DPINTF_HPW_MASK); + mtk_dpi_mask(dpi, DPI_TGEN_HPORCH, +sync->back_porch << HBP, DPINTF_HBP_MASK); + mtk_dpi_mask(dpi, DPI_TGEN_HPORCH, sync->front_porch << HFP, +DPINTF_HFP_MASK); + } else { + mtk_dpi_mask(dpi, DPI_TGEN_HWIDTH, +sync->sync_width << HPW, HPW_MASK); + mtk_dpi_mask(dpi, DPI_TGEN_HPORCH, +sync->back_porch << HBP, HBP_MASK); + mtk_dpi_mask(dpi, DPI_TGEN_HPORCH, sync->front_porch << HFP, +HFP_MASK); + } } static void mtk_dpi_config_vsync(struct mtk_dpi *dpi, struct mtk_dpi_sync_param *sync, u32 width_addr, u32 porch_addr) { - mtk_dpi_mask(dpi, width_addr, -sync->sync_width << VSYNC_WIDTH_SHIFT, -VSYNC_WIDTH_MASK); mtk_dpi_mask(dpi, width_addr, sync->shift_half_line << VSYNC_HALF_LINE_SHIFT, VSYNC_HALF_LINE_MASK); - mtk_dpi_mask(dpi, porch_addr, -sync->back_porch << VSYNC_BACK_PORCH_SHIFT, -VSYNC_BACK_PORCH_MASK); - mtk_dpi_mask(dpi, porch_addr, -sync->front_porch << VSYNC_FRONT_PORCH_SHIFT, -VSYNC_FRONT_PORCH_MASK); + + if (dpi->conf->is_dpintf) { + mtk_dpi_mask(dpi, width_addr, +sync->sync_width << VSYNC_WIDTH_SHIFT, +DPINTF_VSYNC_WIDTH_MASK); + mtk_dpi_mask(dpi, porch_addr, +sync->back_porch << VSYNC_BACK_PORCH_SHIFT, +DPINTF_VSYNC_BACK_PORCH_MASK); + mtk_dpi_mask(dpi, porch_addr, +sync->front_porch << VSYNC_FRONT_PORCH_SHIFT, +DPINTF_VSYNC_FRONT_PORCH_MASK); + } else { + mtk_dpi_mask(dpi, width_addr, +sync->sync_width << VSYNC_WIDTH_SHIFT, +
[PATCH v3 6/6] drm/mediatek: Add mt8195 DisplayPort driver
This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a according phy driver mediatek-dp-phy. It supports both functional units on the mt8195, the embedded DisplayPort as well as the external DisplayPort units. It offers hot-plug-detection, audio up to 8 channels, and DisplayPort 1.4 with up to 4 lanes. The driver creates a child device for the phy. The child device will never exist without the parent being active. As they are sharing a register range, the parent passes a regmap pointer to the child so that both can work with the same register range. The phy driver sets device data that is read by the parent to get the phy device that can be used to control the phy properties. This driver is based on an initial version by Jason-JH.Lin . Signed-off-by: Markus Schneider-Pargmann --- Notes: Changes v2 -> v3: - Solve TODOs and add defines for undescribed registers - Remove TODOs that were irrelevant Changes v1 -> v2: - Fix checkpatch --strict suggestions - General cleanups of the code. - Remove all remaining non-atomic functions. - Remove unused includes and sort them. - Remove unused select GENERIC_PHY - Rename phy registers DP_PHY -> MTK_DP_PHY - Replace usage of delays with usleep_range. - Split the phy register accesses into a separate phy driver. - Use a lock to guard access to mtk_dp->edid as it can be allocated/used/freed in different threads - use struct dp_sdp for sdp packets. Changes RFC -> v1: - Removed unused register definitions. - Replaced workqueue with threaded irq. - Removed connector code. - Move to atomic_* drm functions. - General cleanups of the code. - Remove unused select GENERIC_PHY. drivers/gpu/drm/mediatek/Kconfig |7 + drivers/gpu/drm/mediatek/Makefile |2 + drivers/gpu/drm/mediatek/mtk_dp.c | 2825 drivers/gpu/drm/mediatek/mtk_dp_reg.h | 535 + drivers/gpu/drm/mediatek/mtk_drm_drv.c |1 + drivers/gpu/drm/mediatek/mtk_drm_drv.h |1 + drivers/phy/mediatek/Kconfig |8 + drivers/phy/mediatek/Makefile |1 + drivers/phy/mediatek/phy-mtk-dp.c | 218 ++ include/linux/soc/mediatek/mtk-mmsys.h |2 + 10 files changed, 3600 insertions(+) create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h create mode 100644 drivers/phy/mediatek/phy-mtk-dp.c diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig index 2976d21e9a34..029b94c71613 100644 --- a/drivers/gpu/drm/mediatek/Kconfig +++ b/drivers/gpu/drm/mediatek/Kconfig @@ -28,3 +28,10 @@ config DRM_MEDIATEK_HDMI select PHY_MTK_HDMI help DRM/KMS HDMI driver for Mediatek SoCs + +config MTK_DPTX_SUPPORT + tristate "DRM DPTX Support for Mediatek SoCs" + depends on DRM_MEDIATEK + select PHY_MTK_DP + help + DRM/KMS Display Port driver for Mediatek SoCs. diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile index 29098d7c8307..d86a6406055e 100644 --- a/drivers/gpu/drm/mediatek/Makefile +++ b/drivers/gpu/drm/mediatek/Makefile @@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \ mtk_hdmi_ddc.o obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o + +obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c new file mode 100644 index ..8a5d03b8c5ff --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_dp.c @@ -0,0 +1,2825 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2019 MediaTek Inc. + * Copyright (c) 2021 BayLibre + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "mtk_dp_reg.h" + +#define MTK_DP_AUX_WAIT_REPLY_COUNT2 +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT3 + +#define MTK_DP_MAX_LANES 4 +#define MTK_DP_MAX_LINK_RATE MTK_DP_LINKRATE_HBR3 + +#define MTK_DP_TBC_BUF_READ_START_ADDR 0x08 + +#define MTK_DP_TRAIN_RETRY_LIMIT 8 +#define MTK_DP_TRAIN_MAX_ITERATIONS5 + +#define MTK_DP_AUX_WRITE_READ_WAIT_TIME_US 20 + +#define MTK_DP_DP_VERSION_11 0x11 + +enum mtk_dp_state { + MTK_DP_STATE_INITIAL, + MTK_DP_STATE_IDLE, + MTK_DP_STATE_PREPARE, + MTK_DP_STATE_NORMAL, +}; + +enum mtk_dp_train_state { + MTK_DP_TRAIN_STATE_STARTUP = 0, + MTK_DP_TRAIN_STATE_CHECKCAP, + MTK_DP_TRAIN_STATE_CHECKEDID, + MTK_DP_TRAIN_STATE_TRAINING_PRE, + MTK_DP_TRAIN_STATE_TRAINING, + MTK_DP_TRAIN_STATE_CHECKTIMING, + MTK_DP_TRAIN_STATE_NORMAL, + MTK_DP_TRAIN_STATE_POWERSAVE
Re: [PULL] drm-misc-fixes
On Thu, Sep 30, 2021 at 12:06:21PM +0200, Maarten Lankhorst wrote: > drm-misc-fixes-2021-09-30: > drm-misc-fixes for v5.15: > - Not sure if drm-misc-fixes-2021-09-08 tag was pulled, assuming it is. > - Power management fixes for vc4. > - Compiler fix for vc4. > - Cursor fix for nouveau. > - Fix ttm buffer moves for ampere gpu's by adding minimal acceleration > support. > - Small rockchip fixes. > - Fix DT bindings indent for ili9341. > - Fix y030xx067a init sequence to not get a yellow tint. > - Kconfig fix for fb_simple vs simpledrm. > The following changes since commit 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f: > > Linux 5.15-rc1 (2021-09-12 16:28:37 -0700) > > are available in the Git repository at: > > git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-fixes-2021-09-30 > > for you to fetch changes up to fd09961dbb9ca6558d8ad318a3967c1048bdb090: > > fbdev: simplefb: fix Kconfig dependencies (2021-09-29 09:26:58 +0200) > > > drm-misc-fixes for v5.15: > - Not sure if drm-misc-fixes-2021-09-08 tag was pulled, assuming it is. > - Power management fixes for vc4. > - Compiler fix for vc4. > - Cursor fix for nouveau. > - Fix ttm buffer moves for ampere gpu's by adding minimal acceleration > support. > - Small rockchip fixes. > - Fix DT bindings indent for ili9341. > - Fix y030xx067a init sequence to not get a yellow tint. > - Kconfig fix for fb_simple vs simpledrm. I can't pull this, because it conflicts with vc4 reverts in -rc2. There's a completely busted merge resolution in drm-tip, which doesn't even compile. Please - drop all vc4 patches - rebase onto -rc3 or -rc4 if it's too late I'll do the pull to Linus this afternoon, would be good to get the other fixes in. -Daniel > > > Arnd Bergmann (1): > fbdev: simplefb: fix Kconfig dependencies > > Ben Skeggs (3): > drm/nouveau/kms/tu102-: delay enabling cursor until after assign_windows > drm/nouveau/ga102-: support ttm buffer moves via copy engine > drm/nouveau/fifo/ga102: initialise chid on return from channel creation > > Chris Morgan (1): > drm/rockchip: Update crtc fixup to account for fractional clk change > > Christophe Branchereau (1): > drm/panel: abt-y030xx067a: yellow tint fix > > Edmund Dea (1): > drm/kmb: Enable alpha blended second plane > > Jernej Skrabec (1): > drm/sun4i: dw-hdmi: Fix HDMI PHY clock setup > > Krzysztof Kozlowski (1): > dt-bindings: panel: ili9341: correct indentation > > Maarten Lankhorst (1): > Merge drm/drm-fixes into drm-misc-fixes > > Maxime Ripard (7): > drm/vc4: select PM > drm/vc4: hdmi: Make sure the controller is powered up during bind > drm/vc4: hdmi: Rework the pre_crtc_configure error handling > drm/vc4: hdmi: Split the CEC disable / enable functions in two > drm/vc4: hdmi: Make sure the device is powered with CEC > drm/vc4: hdmi: Warn if we access the controller while disabled > drm/vc4: hdmi: Remove unused struct > > Palmer Dabbelt (1): > drm/rockchip: cdn-dp-core: Fix cdn_dp_resume unused warning > > xinhui pan (1): > drm/ttm: Fix a deadlock if the target BO is not idle during swap > > .../bindings/display/panel/ilitek,ili9341.yaml | 2 +- > drivers/gpu/drm/kmb/kmb_drv.c | 8 +- > drivers/gpu/drm/kmb/kmb_drv.h | 5 + > drivers/gpu/drm/kmb/kmb_plane.c| 81 +- > drivers/gpu/drm/kmb/kmb_plane.h| 5 +- > drivers/gpu/drm/kmb/kmb_regs.h | 3 + > drivers/gpu/drm/nouveau/dispnv50/head.c| 2 +- > drivers/gpu/drm/nouveau/include/nvif/class.h | 2 + > drivers/gpu/drm/nouveau/include/nvkm/engine/fifo.h | 1 + > drivers/gpu/drm/nouveau/nouveau_bo.c | 1 + > drivers/gpu/drm/nouveau/nouveau_chan.c | 6 +- > drivers/gpu/drm/nouveau/nouveau_drm.c | 4 + > drivers/gpu/drm/nouveau/nv84_fence.c | 2 +- > drivers/gpu/drm/nouveau/nvkm/engine/device/base.c | 3 + > drivers/gpu/drm/nouveau/nvkm/engine/fifo/Kbuild| 1 + > drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga102.c | 311 > + > drivers/gpu/drm/nouveau/nvkm/subdev/top/ga100.c| 7 +- > drivers/gpu/drm/panel/panel-abt-y030xx067a.c | 4 +- > drivers/gpu/drm/rockchip/cdn-dp-core.c | 2 +- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c| 26 +- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 7 +- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 4 +- > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 97 --- > drivers/gpu/drm/vc4/Kconfig| 1 + > drivers/gpu/drm/vc4/vc4_hdmi.c | 133 + > drivers/gpu/drm/vc4/vc4_hdmi_regs.h| 6 + > drivers/of/base.c
Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE
On Fri, 01 Oct 2021, Chris Wilson wrote: > Quoting Lucas De Marchi (2021-10-01 08:40:41) >> When trying to bring IS_ACTIVE to linux/kconfig.h I thought it wouldn't >> provide much value just encapsulating it in a boolean context. So I also >> added the support for handling undefined macros as the IS_ENABLED() >> counterpart. However the feedback received from Masahiro Yamada was that >> it is too ugly, not providing much value. And just wrapping in a boolean >> context is too dumb - we could simply open code it. >> >> As detailed in commit babaab2f4738 ("drm/i915: Encapsulate kconfig >> constant values inside boolean predicates"), the IS_ACTIVE macro was >> added to workaround a compilation warning. However after checking again >> our current uses of IS_ACTIVE it turned out there is only >> 1 case in which it would potentially trigger a warning. All the others >> can simply use the shorter version, without wrapping it in any macro. >> And even that single one didn't trigger any warning in gcc 10.3. >> >> So here I'm dialing all the way back to simply removing the macro. If it >> triggers warnings in future we may change the few cases to check for > 0 >> or != 0. Another possibility would be to use the great "not not >> operator" for all positive checks, which would allow us to maintain >> consistency. However let's try first the simplest form though, hopefully >> we don't hit broken compilers spitting a warning: > > You didn't prevent the compilation warning this re-introduces. > > drivers/gpu/drm/i915/i915_config.c:11 i915_fence_context_timeout() warn: > should this be a bitwise op? > drivers/gpu/drm/i915/i915_request.c:1679 i915_request_wait() warn: should > this be a bitwise op? Looks like that's a Smatch warning. The immediate fix would be to just add the != 0 in the relevant places. But this is stuff that's just going to get broken again unless we add Smatch to CI. Most people aren't running it on a regular basis. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Deploying new iterator interface for dma-buf
Hi guys, I've fixed up the lockdep splat in the new selftests found by the CI systems and added another path for dma_resv_poll. I know you guys are flooded, but can we get at least the first few patches committed? The patches to change the individual drivers could also be pushed later on I think. Thanks, Christian.
[PATCH 02/28] dma-buf: add dma_resv_for_each_fence
A simpler version of the iterator to be used when the dma_resv object is locked. Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 46 ++ include/linux/dma-resv.h | 19 2 files changed, 65 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 3cbcf66a137e..a104197d12b5 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -423,6 +423,52 @@ struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) } EXPORT_SYMBOL(dma_resv_iter_next_unlocked); +/** + * dma_resv_iter_first - first fence from a locked dma_resv object + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object while holding the + * &dma_resv.lock. + */ +struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor) +{ + struct dma_fence *fence; + + dma_resv_assert_held(cursor->obj); + + cursor->index = -1; + cursor->fences = dma_resv_shared_list(cursor->obj); + + fence = dma_resv_excl_fence(cursor->obj); + if (!fence) + fence = dma_resv_iter_next(cursor); + + cursor->is_restarted = true; + return fence; +} +EXPORT_SYMBOL_GPL(dma_resv_iter_first); + +/** + * dma_resv_iter_next - next fence from a locked dma_resv object + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object while holding the + * &dma_resv.lock. + */ +struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor) +{ + dma_resv_assert_held(cursor->obj); + + cursor->is_restarted = false; + if (!cursor->all_fences || !cursor->fences || + ++cursor->index >= cursor->fences->shared_count) + return NULL; + + return rcu_dereference_protected(cursor->fences->shared[cursor->index], +dma_resv_held(cursor->obj)); +} +EXPORT_SYMBOL_GPL(dma_resv_iter_next); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 5d7d28cb9008..d4b4cd43f0f1 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -179,6 +179,8 @@ struct dma_resv_iter { struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor); struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor); +struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor); +struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor); /** * dma_resv_iter_begin - initialize a dma_resv_iter object @@ -244,6 +246,23 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) for (fence = dma_resv_iter_first_unlocked(cursor); \ fence; fence = dma_resv_iter_next_unlocked(cursor)) +/** + * dma_resv_for_each_fence - fence iterator + * @cursor: a struct dma_resv_iter pointer + * @obj: a dma_resv object pointer + * @all_fences: true if all fences should be returned + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object while holding the + * &dma_resv.lock. @all_fences controls if the shared fences are returned as + * well. The cursor initialisation is part of the iterator and the fence stays + * valid as long as the lock is held. + */ +#define dma_resv_for_each_fence(cursor, obj, all_fences, fence)\ + for (dma_resv_iter_begin(cursor, obj, all_fences), \ +fence = dma_resv_iter_first(cursor); fence;\ +fence = dma_resv_iter_next(cursor)) + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) -- 2.25.1
[PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. v5: fix typos in the documentation v6: fix coding error when excl fence is NULL v7: one more logic fix Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 100 + include/linux/dma-resv.h | 95 +++ 2 files changed, 195 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..3cbcf66a137e 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iteration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->index++; + if (!cursor->fence) + continue; + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + break; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index++; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + cursor->fence = dma_fence_get_rcu(cursor->fence); + if (!cursor->fence || !dma_fence_is_signaled(cursor->fence)) + break; + } while (true); +} + +/** + * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj. + * @cursor: the cursor with the current position + * + * Returns the first fence from an unlocked dma_resv obj. + */ +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor) +{ + rcu_read_lock(); + do { + dma_resv_iter_restart_unlocked(cursor); + dma_resv_iter_walk_unlocked(cursor); + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); + rcu_read_unlock(); + + return cursor->fence; +} +EXPORT_SYMBOL(dma_resv_iter_first_unlocked); + +/** + * dma_resv_iter_next_unlocked - next fence in an unlocked dma_resv obj. + * @cursor: the cursor with the current position + * + * Returns the next fence from an unlocked dma_resv obj. + */ +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) +{ + bool restart; + + rcu_read_lock(); + cursor->is_restarted = false; + restart = read_seqcount_retry(&cursor->obj->seq, cursor->seq); + do { + if (restart) + dma_resv_iter_restart_unlocked(cursor); + dma_resv_iter_walk_unlocked(cursor); + restart = true; + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); + rcu_read_unlock(); + + return cursor->fence; +} +EXPORT_SYMBOL(dma_resv_iter_next_unlocked); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 9100dd3dc21f..5d7d28cb9008 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -1
[PATCH 03/28] dma-buf: add dma_resv selftest
Just exercising a very minor subset of the functionality, but already proven useful. Signed-off-by: Christian König --- drivers/dma-buf/Makefile | 3 +- drivers/dma-buf/selftests.h | 1 + drivers/dma-buf/st-dma-resv.c | 164 ++ 3 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 drivers/dma-buf/st-dma-resv.c diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 1ef021273a06..511805dbeb75 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_DMABUF_SYSFS_STATS) += dma-buf-sysfs-stats.o dmabuf_selftests-y := \ selftest.o \ st-dma-fence.o \ - st-dma-fence-chain.o + st-dma-fence-chain.o \ + st-dma-resv.o obj-$(CONFIG_DMABUF_SELFTESTS) += dmabuf_selftests.o diff --git a/drivers/dma-buf/selftests.h b/drivers/dma-buf/selftests.h index bc8cea67bf1e..97d73aaa31da 100644 --- a/drivers/dma-buf/selftests.h +++ b/drivers/dma-buf/selftests.h @@ -12,3 +12,4 @@ selftest(sanitycheck, __sanitycheck__) /* keep first (igt selfcheck) */ selftest(dma_fence, dma_fence) selftest(dma_fence_chain, dma_fence_chain) +selftest(dma_resv, dma_resv) diff --git a/drivers/dma-buf/st-dma-resv.c b/drivers/dma-buf/st-dma-resv.c new file mode 100644 index ..ea44769d058d --- /dev/null +++ b/drivers/dma-buf/st-dma-resv.c @@ -0,0 +1,164 @@ +/* SPDX-License-Identifier: MIT */ + +/* +* Copyright © 2019 Intel Corporation +*/ + +//#include +//#include +//#include +//#include +//#include + +#include +#include +#include + +#include "selftest.h" + +static struct spinlock fence_lock; + +static const char *fence_name(struct dma_fence *f) +{ + return "selftest"; +} + +static const struct dma_fence_ops fence_ops = { + .get_driver_name = fence_name, + .get_timeline_name = fence_name, +}; + +static struct dma_fence *alloc_fence(void) +{ + struct dma_fence *f; + + f = kmalloc(sizeof(*f), GFP_KERNEL); + if (!f) + return NULL; + + dma_fence_init(f, &fence_ops, &fence_lock, 0, 0); + return f; +} + +static int sanitycheck(void *arg) +{ + struct dma_fence *f; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_fence_signal(f); + dma_fence_put(f); + return 0; +} + +static int test_excl_signaling(void *arg) +{ + struct dma_resv resv; + struct dma_fence *f; + int err = -EINVAL; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_resv_init(&resv); + dma_resv_add_excl_fence(&resv, f); + if (dma_resv_test_signaled(&resv, false)) { + pr_err("Resv unexpectedly signaled\n"); + goto err_free; + } + dma_fence_signal(f); + if (!dma_resv_test_signaled(&resv, false)) { + pr_err("Resv not reporting signaled\n"); + goto err_free; + } + err = 0; +err_free: + dma_resv_fini(&resv); + dma_fence_put(f); + return err; +} + +static int test_shared_signaling(void *arg) +{ + struct dma_resv resv; + struct dma_fence *f; + int err; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_resv_init(&resv); + err = dma_resv_reserve_shared(&resv, 1); + if (err) { + pr_err("Resv shared slot allocation failed\n"); + goto err_free; + } + + err = -EINVAL; + dma_resv_add_shared_fence(&resv, f); + if (dma_resv_test_signaled(&resv, true)) { + pr_err("Resv unexpectedly signaled\n"); + goto err_free; + } + dma_fence_signal(f); + if (!dma_resv_test_signaled(&resv, true)) { + pr_err("Resv not reporting signaled\n"); + goto err_free; + } + err = 0; +err_free: + dma_resv_fini(&resv); + dma_fence_put(f); + return err; +} + +static int test_excl_for_each(void *arg) +{ + struct dma_resv_iter cursor; + struct dma_fence *f, *fence; + struct dma_resv resv; + int err; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_resv_init(&resv); + dma_resv_add_excl_fence(&resv, f); + + err = -EINVAL; + dma_resv_for_each_fence(&cursor, &resv, false, fence) { + if (f != fence) { + pr_err("Unexpected fence\n"); + goto err_free; + } + err = 0; + } + if (err) { + pr_err("No fence found\n"); + goto err_free; + } + dma_fence_signal(f); + err = 0; +err_free: + dma_resv_fini(&resv); + dma_fence_put(f); + return err; +} + +int dma_resv(void) +{ + static const struct subtest tests[] = { + SUBTEST(sanitycheck), + SUBTEST(test_excl_signaling), + SUBTEST(test_shar
[PATCH 07/28] dma-buf: use new iterator in dma_resv_test_signaled
This makes the function much simpler since the complex retry logic is now handled elsewhere. Signed-off-by: Christian König Reviewed-by: Daniel Vetter --- drivers/dma-buf/dma-resv.c | 57 +- 1 file changed, 7 insertions(+), 50 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 510e15f805bb..324f243cb56b 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -630,22 +630,6 @@ long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr, EXPORT_SYMBOL_GPL(dma_resv_wait_timeout); -static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) -{ - struct dma_fence *fence, *lfence = passed_fence; - int ret = 1; - - if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &lfence->flags)) { - fence = dma_fence_get_rcu(lfence); - if (!fence) - return -1; - - ret = !!dma_fence_is_signaled(fence); - dma_fence_put(fence); - } - return ret; -} - /** * dma_resv_test_signaled - Test if a reservation object's fences have been * signaled. @@ -662,43 +646,16 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { + struct dma_resv_iter cursor; struct dma_fence *fence; - unsigned int seq; - int ret; - - rcu_read_lock(); -retry: - ret = true; - seq = read_seqcount_begin(&obj->seq); - - if (test_all) { - struct dma_resv_list *fobj = dma_resv_shared_list(obj); - unsigned int i, shared_count; - - shared_count = fobj ? fobj->shared_count : 0; - for (i = 0; i < shared_count; ++i) { - fence = rcu_dereference(fobj->shared[i]); - ret = dma_resv_test_signaled_single(fence); - if (ret < 0) - goto retry; - else if (!ret) - break; - } - } - - fence = dma_resv_excl_fence(obj); - if (ret && fence) { - ret = dma_resv_test_signaled_single(fence); - if (ret < 0) - goto retry; + dma_resv_iter_begin(&cursor, obj, test_all); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + dma_resv_iter_end(&cursor); + return false; } - - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; - - rcu_read_unlock(); - return ret; + dma_resv_iter_end(&cursor); + return true; } EXPORT_SYMBOL_GPL(dma_resv_test_signaled); -- 2.25.1
[PATCH 09/28] dma-buf: use the new iterator in dma_resv_poll
Simplify the code a bit. Signed-off-by: Christian König --- drivers/dma-buf/dma-buf.c | 36 ++-- 1 file changed, 6 insertions(+), 30 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 8242b5d9baeb..beb504a92d60 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -209,19 +209,14 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb) dma_fence_put(fence); } -static bool dma_buf_poll_shared(struct dma_resv *resv, +static bool dma_buf_poll_add_cb(struct dma_resv *resv, bool write, struct dma_buf_poll_cb_t *dcb) { - struct dma_resv_list *fobj = dma_resv_shared_list(resv); + struct dma_resv_iter cursor; struct dma_fence *fence; - int i, r; - - if (!fobj) - return false; + int r; - for (i = 0; i < fobj->shared_count; ++i) { - fence = rcu_dereference_protected(fobj->shared[i], - dma_resv_held(resv)); + dma_resv_for_each_fence(&cursor, resv, write, fence) { dma_fence_get(fence); r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb); if (!r) @@ -232,24 +227,6 @@ static bool dma_buf_poll_shared(struct dma_resv *resv, return false; } -static bool dma_buf_poll_excl(struct dma_resv *resv, - struct dma_buf_poll_cb_t *dcb) -{ - struct dma_fence *fence = dma_resv_excl_fence(resv); - int r; - - if (!fence) - return false; - - dma_fence_get(fence); - r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb); - if (!r) - return true; - dma_fence_put(fence); - - return false; -} - static __poll_t dma_buf_poll(struct file *file, poll_table *poll) { struct dma_buf *dmabuf; @@ -282,8 +259,7 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLOUT) { - if (!dma_buf_poll_shared(resv, dcb) && - !dma_buf_poll_excl(resv, dcb)) + if (!dma_buf_poll_add_cb(resv, true, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); else @@ -303,7 +279,7 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLIN) { - if (!dma_buf_poll_excl(resv, dcb)) + if (!dma_buf_poll_add_cb(resv, false, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); else -- 2.25.1
[PATCH 04/28] dma-buf: use new iterator in dma_resv_copy_fences
This makes the function much simpler since the complex retry logic is now handled else where. Signed-off-by: Christian König Reviewed-by: Daniel Vetter --- drivers/dma-buf/dma-resv.c | 84 +++--- 1 file changed, 32 insertions(+), 52 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index a104197d12b5..064972c6bde2 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -478,74 +478,54 @@ EXPORT_SYMBOL_GPL(dma_resv_iter_next); */ int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) { - struct dma_resv_list *src_list, *dst_list; - struct dma_fence *old, *new; - unsigned int i; + struct dma_resv_iter cursor; + struct dma_resv_list *list; + struct dma_fence *f, *excl; dma_resv_assert_held(dst); - rcu_read_lock(); - src_list = dma_resv_shared_list(src); + list = NULL; + excl = NULL; -retry: - if (src_list) { - unsigned int shared_count = src_list->shared_count; + dma_resv_iter_begin(&cursor, src, true); + dma_resv_for_each_fence_unlocked(&cursor, f) { - rcu_read_unlock(); + if (dma_resv_iter_is_restarted(&cursor)) { + dma_resv_list_free(list); + dma_fence_put(excl); - dst_list = dma_resv_list_alloc(shared_count); - if (!dst_list) - return -ENOMEM; + if (cursor.fences) { + unsigned int cnt = cursor.fences->shared_count; - rcu_read_lock(); - src_list = dma_resv_shared_list(src); - if (!src_list || src_list->shared_count > shared_count) { - kfree(dst_list); - goto retry; - } - - dst_list->shared_count = 0; - for (i = 0; i < src_list->shared_count; ++i) { - struct dma_fence __rcu **dst; - struct dma_fence *fence; + list = dma_resv_list_alloc(cnt); + if (!list) { + dma_resv_iter_end(&cursor); + return -ENOMEM; + } - fence = rcu_dereference(src_list->shared[i]); - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, -&fence->flags)) - continue; + list->shared_count = 0; - if (!dma_fence_get_rcu(fence)) { - dma_resv_list_free(dst_list); - src_list = dma_resv_shared_list(src); - goto retry; + } else { + list = NULL; } - - if (dma_fence_is_signaled(fence)) { - dma_fence_put(fence); - continue; - } - - dst = &dst_list->shared[dst_list->shared_count++]; - rcu_assign_pointer(*dst, fence); + excl = NULL; } - } else { - dst_list = NULL; - } - new = dma_fence_get_rcu_safe(&src->fence_excl); - rcu_read_unlock(); - - src_list = dma_resv_shared_list(dst); - old = dma_resv_excl_fence(dst); + dma_fence_get(f); + if (dma_resv_iter_is_exclusive(&cursor)) + excl = f; + else + RCU_INIT_POINTER(list->shared[list->shared_count++], f); + } + dma_resv_iter_end(&cursor); write_seqcount_begin(&dst->seq); - /* write_seqcount_begin provides the necessary memory barrier */ - RCU_INIT_POINTER(dst->fence_excl, new); - RCU_INIT_POINTER(dst->fence, dst_list); + excl = rcu_replace_pointer(dst->fence_excl, excl, dma_resv_held(dst)); + list = rcu_replace_pointer(dst->fence, list, dma_resv_held(dst)); write_seqcount_end(&dst->seq); - dma_resv_list_free(src_list); - dma_fence_put(old); + dma_resv_list_free(list); + dma_fence_put(excl); return 0; } -- 2.25.1
[PATCH 08/28] dma-buf: use the new iterator in dma_buf_debug_show
Simplifying the code a bit. Signed-off-by: Christian König --- drivers/dma-buf/dma-buf.c | 24 ++-- 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 61e20ae7b08b..8242b5d9baeb 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1356,10 +1356,9 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) { struct dma_buf *buf_obj; struct dma_buf_attachment *attach_obj; - struct dma_resv *robj; - struct dma_resv_list *fobj; + struct dma_resv_iter cursor; struct dma_fence *fence; - int count = 0, attach_count, shared_count, i; + int count = 0, attach_count; size_t size = 0; int ret; @@ -1386,21 +1385,10 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) file_inode(buf_obj->file)->i_ino, buf_obj->name ?: ""); - robj = buf_obj->resv; - fence = dma_resv_excl_fence(robj); - if (fence) - seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n", - fence->ops->get_driver_name(fence), - fence->ops->get_timeline_name(fence), - dma_fence_is_signaled(fence) ? "" : "un"); - - fobj = rcu_dereference_protected(robj->fence, -dma_resv_held(robj)); - shared_count = fobj ? fobj->shared_count : 0; - for (i = 0; i < shared_count; i++) { - fence = rcu_dereference_protected(fobj->shared[i], - dma_resv_held(robj)); - seq_printf(s, "\tShared fence: %s %s %ssignalled\n", + dma_resv_for_each_fence(&cursor, buf_obj->resv, true, fence) { + seq_printf(s, "\t%s fence: %s %s %ssignalled\n", + dma_resv_iter_is_exclusive(&cursor) ? + "Exclusive" : "Shared", fence->ops->get_driver_name(fence), fence->ops->get_timeline_name(fence), dma_fence_is_signaled(fence) ? "" : "un"); -- 2.25.1
[PATCH 10/28] drm/ttm: use the new iterator in ttm_bo_flush_all_fences
This is probably a fix since we didn't even grabed a reference to the fences. Signed-off-by: Christian König Reviewed-by: Daniel Vetter --- drivers/gpu/drm/ttm/ttm_bo.c | 16 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index d62b2013c367..3934ee225c78 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -269,23 +269,15 @@ static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo) static void ttm_bo_flush_all_fences(struct ttm_buffer_object *bo) { struct dma_resv *resv = &bo->base._resv; - struct dma_resv_list *fobj; + struct dma_resv_iter cursor; struct dma_fence *fence; - int i; - - rcu_read_lock(); - fobj = dma_resv_shared_list(resv); - fence = dma_resv_excl_fence(resv); - if (fence && !fence->ops->signaled) - dma_fence_enable_sw_signaling(fence); - - for (i = 0; fobj && i < fobj->shared_count; ++i) { - fence = rcu_dereference(fobj->shared[i]); + dma_resv_iter_begin(&cursor, resv, true); + dma_resv_for_each_fence_unlocked(&cursor, fence) { if (!fence->ops->signaled) dma_fence_enable_sw_signaling(fence); } - rcu_read_unlock(); + dma_resv_iter_end(&cursor); } /** -- 2.25.1
[PATCH 05/28] dma-buf: use new iterator in dma_resv_get_fences v3
This makes the function much simpler since the complex retry logic is now handled elsewhere. v2: use sizeof(void*) instead v3: fix rebase bug Signed-off-by: Christian König Reviewed-by: Daniel Vetter --- drivers/dma-buf/dma-resv.c | 108 - 1 file changed, 35 insertions(+), 73 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 064972c6bde2..9b494828e7ca 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -535,99 +535,61 @@ EXPORT_SYMBOL(dma_resv_copy_fences); * dma_resv_get_fences - Get an object's shared and exclusive * fences without update side lock held * @obj: the reservation object - * @pfence_excl: the returned exclusive fence (or NULL) - * @pshared_count: the number of shared fences returned - * @pshared: the array of shared fence ptrs returned (array is krealloc'd to + * @fence_excl: the returned exclusive fence (or NULL) + * @shared_count: the number of shared fences returned + * @shared: the array of shared fence ptrs returned (array is krealloc'd to * the required size, and must be freed by caller) * * Retrieve all fences from the reservation object. If the pointer for the * exclusive fence is not specified the fence is put into the array of the * shared fences as well. Returns either zero or -ENOMEM. */ -int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, - unsigned int *pshared_count, - struct dma_fence ***pshared) +int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **fence_excl, + unsigned int *shared_count, struct dma_fence ***shared) { - struct dma_fence **shared = NULL; - struct dma_fence *fence_excl; - unsigned int shared_count; - int ret = 1; - - do { - struct dma_resv_list *fobj; - unsigned int i, seq; - size_t sz = 0; - - shared_count = i = 0; - - rcu_read_lock(); - seq = read_seqcount_begin(&obj->seq); + struct dma_resv_iter cursor; + struct dma_fence *fence; - fence_excl = dma_resv_excl_fence(obj); - if (fence_excl && !dma_fence_get_rcu(fence_excl)) - goto unlock; + *shared_count = 0; + *shared = NULL; - fobj = dma_resv_shared_list(obj); - if (fobj) - sz += sizeof(*shared) * fobj->shared_max; + if (fence_excl) + *fence_excl = NULL; - if (!pfence_excl && fence_excl) - sz += sizeof(*shared); + dma_resv_iter_begin(&cursor, obj, true); + dma_resv_for_each_fence_unlocked(&cursor, fence) { - if (sz) { - struct dma_fence **nshared; + if (dma_resv_iter_is_restarted(&cursor)) { + unsigned int count; - nshared = krealloc(shared, sz, - GFP_NOWAIT | __GFP_NOWARN); - if (!nshared) { - rcu_read_unlock(); + while (*shared_count) + dma_fence_put((*shared)[--(*shared_count)]); - dma_fence_put(fence_excl); - fence_excl = NULL; + if (fence_excl) + dma_fence_put(*fence_excl); - nshared = krealloc(shared, sz, GFP_KERNEL); - if (nshared) { - shared = nshared; - continue; - } + count = cursor.fences ? cursor.fences->shared_count : 0; + count += fence_excl ? 0 : 1; - ret = -ENOMEM; - break; + /* Eventually re-allocate the array */ + *shared = krealloc_array(*shared, count, +sizeof(void *), +GFP_KERNEL); + if (count && !*shared) { + dma_resv_iter_end(&cursor); + return -ENOMEM; } - shared = nshared; - shared_count = fobj ? fobj->shared_count : 0; - for (i = 0; i < shared_count; ++i) { - shared[i] = rcu_dereference(fobj->shared[i]); - if (!dma_fence_get_rcu(shared[i])) - break; - } - } - - if (i != shared_count || read_seqcount_retry(&obj->seq, seq)) { - while (i--) -
[PATCH 06/28] dma-buf: use new iterator in dma_resv_wait_timeout
This makes the function much simpler since the complex retry logic is now handled elsewhere. Signed-off-by: Christian König Reviewed-by: Daniel Vetter --- drivers/dma-buf/dma-resv.c | 69 +- 1 file changed, 8 insertions(+), 61 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 9b494828e7ca..510e15f805bb 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -611,74 +611,21 @@ long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr, unsigned long timeout) { long ret = timeout ? timeout : 1; - unsigned int seq, shared_count; + struct dma_resv_iter cursor; struct dma_fence *fence; - int i; - -retry: - shared_count = 0; - seq = read_seqcount_begin(&obj->seq); - rcu_read_lock(); - i = -1; - - fence = dma_resv_excl_fence(obj); - if (fence && !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { - if (!dma_fence_get_rcu(fence)) - goto unlock_retry; - - if (dma_fence_is_signaled(fence)) { - dma_fence_put(fence); - fence = NULL; - } - - } else { - fence = NULL; - } - - if (wait_all) { - struct dma_resv_list *fobj = dma_resv_shared_list(obj); - - if (fobj) - shared_count = fobj->shared_count; - - for (i = 0; !fence && i < shared_count; ++i) { - struct dma_fence *lfence; - - lfence = rcu_dereference(fobj->shared[i]); - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, -&lfence->flags)) - continue; - if (!dma_fence_get_rcu(lfence)) - goto unlock_retry; - - if (dma_fence_is_signaled(lfence)) { - dma_fence_put(lfence); - continue; - } + dma_resv_iter_begin(&cursor, obj, wait_all); + dma_resv_for_each_fence_unlocked(&cursor, fence) { - fence = lfence; - break; + ret = dma_fence_wait_timeout(fence, intr, ret); + if (ret <= 0) { + dma_resv_iter_end(&cursor); + return ret; } } + dma_resv_iter_end(&cursor); - rcu_read_unlock(); - if (fence) { - if (read_seqcount_retry(&obj->seq, seq)) { - dma_fence_put(fence); - goto retry; - } - - ret = dma_fence_wait_timeout(fence, intr, ret); - dma_fence_put(fence); - if (ret > 0 && wait_all && (i + 1 < shared_count)) - goto retry; - } return ret; - -unlock_retry: - rcu_read_unlock(); - goto retry; } EXPORT_SYMBOL_GPL(dma_resv_wait_timeout); -- 2.25.1
[PATCH 11/28] drm/amdgpu: use the new iterator in amdgpu_sync_resv
Simplifying the code a bit. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 44 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c index 862eb3c1c4c5..f7d8487799b2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c @@ -252,41 +252,25 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync, struct dma_resv *resv, enum amdgpu_sync_mode mode, void *owner) { - struct dma_resv_list *flist; + struct dma_resv_iter cursor; struct dma_fence *f; - unsigned i; - int r = 0; + int r; if (resv == NULL) return -EINVAL; - /* always sync to the exclusive fence */ - f = dma_resv_excl_fence(resv); - dma_fence_chain_for_each(f, f) { - struct dma_fence_chain *chain = to_dma_fence_chain(f); - - if (amdgpu_sync_test_fence(adev, mode, owner, chain ? - chain->fence : f)) { - r = amdgpu_sync_fence(sync, f); - dma_fence_put(f); - if (r) - return r; - break; - } - } - - flist = dma_resv_shared_list(resv); - if (!flist) - return 0; - - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(resv)); - - if (amdgpu_sync_test_fence(adev, mode, owner, f)) { - r = amdgpu_sync_fence(sync, f); - if (r) - return r; + dma_resv_for_each_fence(&cursor, resv, true, f) { + dma_fence_chain_for_each(f, f) { + struct dma_fence_chain *chain = to_dma_fence_chain(f); + + if (amdgpu_sync_test_fence(adev, mode, owner, chain ? + chain->fence : f)) { + r = amdgpu_sync_fence(sync, f); + dma_fence_put(f); + if (r) + return r; + break; + } } } return 0; -- 2.25.1
[PATCH 12/28] drm/amdgpu: use new iterator in amdgpu_ttm_bo_eviction_valuable
Simplifying the code a bit. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 -- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index e8d70b6e6737..722e3c9e8882 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1345,10 +1345,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, const struct ttm_place *place) { unsigned long num_pages = bo->resource->num_pages; + struct dma_resv_iter resv_cursor; struct amdgpu_res_cursor cursor; - struct dma_resv_list *flist; struct dma_fence *f; - int i; /* Swapout? */ if (bo->resource->mem_type == TTM_PL_SYSTEM) @@ -1362,14 +1361,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, * If true, then return false as any KFD process needs all its BOs to * be resident to run successfully */ - flist = dma_resv_shared_list(bo->base.resv); - if (flist) { - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(bo->base.resv)); - if (amdkfd_fence_check_mm(f, current->mm)) - return false; - } + dma_resv_for_each_fence(&resv_cursor, bo->base.resv, true, f) { + if (amdkfd_fence_check_mm(f, current->mm)) + return false; } switch (bo->resource->mem_type) { -- 2.25.1
[PATCH 14/28] drm/msm: use new iterator in msm_gem_describe
Simplifying the code a bit. Also drop the RCU read side lock since the object is locked anyway. Untested since I can't get the driver to compile on !ARM. Signed-off-by: Christian König --- drivers/gpu/drm/msm/msm_gem.c | 19 +-- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index 40a9863f5951..5bd511f07c07 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -880,7 +880,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m, { struct msm_gem_object *msm_obj = to_msm_bo(obj); struct dma_resv *robj = obj->resv; - struct dma_resv_list *fobj; + struct dma_resv_iter cursor; struct dma_fence *fence; struct msm_gem_vma *vma; uint64_t off = drm_vma_node_start(&obj->vma_node); @@ -955,22 +955,13 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m, seq_puts(m, "\n"); } - rcu_read_lock(); - fobj = dma_resv_shared_list(robj); - if (fobj) { - unsigned int i, shared_count = fobj->shared_count; - - for (i = 0; i < shared_count; i++) { - fence = rcu_dereference(fobj->shared[i]); + dma_resv_for_each_fence(&cursor, robj, true, fence) { + if (dma_resv_iter_is_exclusive(&cursor)) + describe_fence(fence, "Exclusive", m); + else describe_fence(fence, "Shared", m); - } } - fence = dma_resv_excl_fence(robj); - if (fence) - describe_fence(fence, "Exclusive", m); - rcu_read_unlock(); - msm_gem_unlock(obj); } -- 2.25.1
[PATCH 16/28] drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2
Simplifying the code a bit. v2: use dma_resv_for_each_fence Signed-off-by: Christian König Reviewed-by: Daniel Vetter --- drivers/gpu/drm/scheduler/sched_main.c | 26 ++ 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 042c16b5d54a..5bc5f775abe1 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -699,30 +699,16 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job, struct drm_gem_object *obj, bool write) { + struct dma_resv_iter cursor; + struct dma_fence *fence; int ret; - struct dma_fence **fences; - unsigned int i, fence_count; - - if (!write) { - struct dma_fence *fence = dma_resv_get_excl_unlocked(obj->resv); - - return drm_sched_job_add_dependency(job, fence); - } - - ret = dma_resv_get_fences(obj->resv, NULL, &fence_count, &fences); - if (ret || !fence_count) - return ret; - for (i = 0; i < fence_count; i++) { - ret = drm_sched_job_add_dependency(job, fences[i]); + dma_resv_for_each_fence(&cursor, obj->resv, write, fence) { + ret = drm_sched_job_add_dependency(job, fence); if (ret) - break; + return ret; } - - for (; i < fence_count; i++) - dma_fence_put(fences[i]); - kfree(fences); - return ret; + return 0; } EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies); -- 2.25.1
[PATCH 18/28] drm/i915: use the new iterator in i915_sw_fence_await_reservation v3
Simplifying the code a bit. v2: use dma_resv_for_each_fence instead, according to Tvrtko the lock is held here anyway. v3: back to using dma_resv_for_each_fence_unlocked. Signed-off-by: Christian König --- drivers/gpu/drm/i915/i915_sw_fence.c | 53 ++-- 1 file changed, 11 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index c589a681da77..7ea0dbf81530 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -572,56 +572,25 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence, unsigned long timeout, gfp_t gfp) { - struct dma_fence *excl; + struct dma_resv_iter cursor; + struct dma_fence *f; int ret = 0, pending; debug_fence_assert(fence); might_sleep_if(gfpflags_allow_blocking(gfp)); - if (write) { - struct dma_fence **shared; - unsigned int count, i; - - ret = dma_resv_get_fences(resv, &excl, &count, &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - if (shared[i]->ops == exclude) - continue; - - pending = i915_sw_fence_await_dma_fence(fence, - shared[i], - timeout, - gfp); - if (pending < 0) { - ret = pending; - break; - } - - ret |= pending; - } - - for (i = 0; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); - } else { - excl = dma_resv_get_excl_unlocked(resv); - } - - if (ret >= 0 && excl && excl->ops != exclude) { - pending = i915_sw_fence_await_dma_fence(fence, - excl, - timeout, + dma_resv_iter_begin(&cursor, resv, write); + dma_resv_for_each_fence_unlocked(&cursor, f) { + pending = i915_sw_fence_await_dma_fence(fence, f, timeout, gfp); - if (pending < 0) + if (pending < 0) { ret = pending; - else - ret |= pending; - } - - dma_fence_put(excl); + break; + } + ret |= pending; + } + dma_resv_iter_end(&cursor); return ret; } -- 2.25.1
[PATCH 20/28] drm/i915: use new iterator in i915_gem_object_wait_reservation
Simplifying the code a bit. Signed-off-by: Christian König --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 51 +--- 1 file changed, 9 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index f909aaa09d9c..a13193db1dba 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -37,55 +37,22 @@ i915_gem_object_wait_reservation(struct dma_resv *resv, unsigned int flags, long timeout) { - struct dma_fence *excl; - bool prune_fences = false; - - if (flags & I915_WAIT_ALL) { - struct dma_fence **shared; - unsigned int count, i; - int ret; + struct dma_resv_iter cursor; + struct dma_fence *fence; - ret = dma_resv_get_fences(resv, &excl, &count, &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - timeout = i915_gem_object_wait_fence(shared[i], -flags, timeout); - if (timeout < 0) - break; - - dma_fence_put(shared[i]); - } - - for (; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); - - /* -* If both shared fences and an exclusive fence exist, -* then by construction the shared fences must be later -* than the exclusive fence. If we successfully wait for -* all the shared fences, we know that the exclusive fence -* must all be signaled. If all the shared fences are -* signaled, we can prune the array and recover the -* floating references on the fences/requests. -*/ - prune_fences = count && timeout >= 0; - } else { - excl = dma_resv_get_excl_unlocked(resv); + dma_resv_iter_begin(&cursor, resv, flags & I915_WAIT_ALL); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + timeout = i915_gem_object_wait_fence(fence, flags, timeout); + if (timeout < 0) + break; } - - if (excl && timeout >= 0) - timeout = i915_gem_object_wait_fence(excl, flags, timeout); - - dma_fence_put(excl); + dma_resv_iter_end(&cursor); /* * Opportunistically prune the fences iff we know they have *all* been * signaled. */ - if (prune_fences) + if (timeout > 0) dma_resv_prune(resv); return timeout; -- 2.25.1
[PATCH 17/28] drm/i915: use the new iterator in i915_gem_busy_ioctl v2
This makes the function much simpler since the complex retry logic is now handled else where. Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/gem/i915_gem_busy.c | 35 ++-- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index 6234e17259c1..dc72b36dae54 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, { struct drm_i915_gem_busy *args = data; struct drm_i915_gem_object *obj; - struct dma_resv_list *list; - unsigned int seq; + struct dma_resv_iter cursor; + struct dma_fence *fence; int err; err = -ENOENT; @@ -109,27 +109,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ -retry: - seq = raw_read_seqcount(&obj->base.resv->seq); - - /* Translate the exclusive fence to the READ *and* WRITE engine */ - args->busy = busy_check_writer(dma_resv_excl_fence(obj->base.resv)); - - /* Translate shared fences to READ set of engines */ - list = dma_resv_shared_list(obj->base.resv); - if (list) { - unsigned int shared_count = list->shared_count, i; - - for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence = - rcu_dereference(list->shared[i]); - + args->busy = 0; + dma_resv_iter_begin(&cursor, obj->base.resv, true); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + if (dma_resv_iter_is_restarted(&cursor)) + args->busy = 0; + + if (dma_resv_iter_is_exclusive(&cursor)) + /* Translate the exclusive fence to the READ *and* WRITE engine */ + args->busy |= busy_check_writer(fence); + else + /* Translate shared fences to READ set of engines */ args->busy |= busy_check_reader(fence); - } } - - if (args->busy && read_seqcount_retry(&obj->base.resv->seq, seq)) - goto retry; + dma_resv_iter_end(&cursor); err = 0; out: -- 2.25.1
[PATCH 13/28] drm/amdgpu: use new iterator in amdgpu_vm_prt_fini
No need to actually allocate an array of fences here. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 26 +- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 6b15cad78de9..e42dd79ed6f4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -2090,30 +2090,14 @@ static void amdgpu_vm_free_mapping(struct amdgpu_device *adev, static void amdgpu_vm_prt_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm) { struct dma_resv *resv = vm->root.bo->tbo.base.resv; - struct dma_fence *excl, **shared; - unsigned i, shared_count; - int r; + struct dma_resv_iter cursor; + struct dma_fence *fence; - r = dma_resv_get_fences(resv, &excl, &shared_count, &shared); - if (r) { - /* Not enough memory to grab the fence list, as last resort -* block for all the fences to complete. -*/ - dma_resv_wait_timeout(resv, true, false, - MAX_SCHEDULE_TIMEOUT); - return; - } - - /* Add a callback for each fence in the reservation object */ - amdgpu_vm_prt_get(adev); - amdgpu_vm_add_prt_cb(adev, excl); - - for (i = 0; i < shared_count; ++i) { + dma_resv_for_each_fence(&cursor, resv, true, fence) { + /* Add a callback for each fence in the reservation object */ amdgpu_vm_prt_get(adev); - amdgpu_vm_add_prt_cb(adev, shared[i]); + amdgpu_vm_add_prt_cb(adev, fence); } - - kfree(shared); } /** -- 2.25.1
[PATCH 19/28] drm/i915: use the new iterator in i915_request_await_object v2
Simplifying the code a bit. v2: add missing rcu_read_lock()/rcu_read_unlock() v3: use dma_resv_for_each_fence instead Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_request.c | 34 + 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index ce446716d092..3839712ebd23 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -1509,38 +1509,14 @@ i915_request_await_object(struct i915_request *to, struct drm_i915_gem_object *obj, bool write) { - struct dma_fence *excl; + struct dma_resv_iter cursor; + struct dma_fence *fence; int ret = 0; - if (write) { - struct dma_fence **shared; - unsigned int count, i; - - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, - &shared); + dma_resv_for_each_fence(&cursor, obj->base.resv, write, fence) { + ret = i915_request_await_dma_fence(to, fence); if (ret) - return ret; - - for (i = 0; i < count; i++) { - ret = i915_request_await_dma_fence(to, shared[i]); - if (ret) - break; - - dma_fence_put(shared[i]); - } - - for (; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); - } else { - excl = dma_resv_get_excl_unlocked(obj->base.resv); - } - - if (excl) { - if (ret == 0) - ret = i915_request_await_dma_fence(to, excl); - - dma_fence_put(excl); + break; } return ret; -- 2.25.1
[PATCH 21/28] drm/i915: use new iterator in i915_gem_object_wait_priority
Simplifying the code a bit. Signed-off-by: Christian König --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 31 +--- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index a13193db1dba..569658c7859c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -118,32 +118,13 @@ i915_gem_object_wait_priority(struct drm_i915_gem_object *obj, unsigned int flags, const struct i915_sched_attr *attr) { - struct dma_fence *excl; - - if (flags & I915_WAIT_ALL) { - struct dma_fence **shared; - unsigned int count, i; - int ret; - - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, - &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - i915_gem_fence_wait_priority(shared[i], attr); - dma_fence_put(shared[i]); - } - - kfree(shared); - } else { - excl = dma_resv_get_excl_unlocked(obj->base.resv); - } + struct dma_resv_iter cursor; + struct dma_fence *fence; - if (excl) { - i915_gem_fence_wait_priority(excl, attr); - dma_fence_put(excl); - } + dma_resv_iter_begin(&cursor, obj->base.resv, flags & I915_WAIT_ALL); + dma_resv_for_each_fence_unlocked(&cursor, fence) + i915_gem_fence_wait_priority(fence, attr); + dma_resv_iter_end(&cursor); return 0; } -- 2.25.1
[PATCH 22/28] drm/i915: use new cursor in intel_prepare_plane_fb
Simplifying the code a bit. Signed-off-by: Christian König --- drivers/gpu/drm/i915/display/intel_display.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 134a6acbd8fb..d32137a84694 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -11290,6 +11290,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, i915_gem_object_flush_frontbuffer(obj, ORIGIN_DIRTYFB); if (!new_plane_state->uapi.fence) { /* implicit fencing */ + struct dma_resv_iter cursor; struct dma_fence *fence; ret = i915_sw_fence_await_reservation(&state->commit_ready, @@ -11300,12 +11301,12 @@ intel_prepare_plane_fb(struct drm_plane *_plane, if (ret < 0) goto unpin_fb; - fence = dma_resv_get_excl_unlocked(obj->base.resv); - if (fence) { + dma_resv_iter_begin(&cursor, obj->base.resv, false); + dma_resv_for_each_fence_unlocked(&cursor, fence) { add_rps_boost_after_vblank(new_plane_state->hw.crtc, fence); - dma_fence_put(fence); } + dma_resv_iter_end(&cursor); } else { add_rps_boost_after_vblank(new_plane_state->hw.crtc, new_plane_state->uapi.fence); -- 2.25.1
[PATCH 24/28] drm: use new iterator in drm_gem_plane_helper_prepare_fb
Makes the handling a bit more complex, but avoids the use of dma_resv_get_excl_unlocked(). Signed-off-by: Christian König --- drivers/gpu/drm/drm_gem_atomic_helper.c | 13 +++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index e570398abd78..21ed930042b8 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -143,6 +143,7 @@ */ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) { + struct dma_resv_iter cursor; struct drm_gem_object *obj; struct dma_fence *fence; @@ -150,9 +151,17 @@ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_st return 0; obj = drm_gem_fb_get_obj(state->fb, 0); - fence = dma_resv_get_excl_unlocked(obj->resv); - drm_atomic_set_fence_for_plane(state, fence); + dma_resv_iter_begin(&cursor, obj->resv, false); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + dma_fence_get(fence); + dma_resv_iter_end(&cursor); + /* TODO: We only use the first write fence here */ + drm_atomic_set_fence_for_plane(state, fence); + return 0; + } + dma_resv_iter_end(&cursor); + drm_atomic_set_fence_for_plane(state, NULL); return 0; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); -- 2.25.1
[PATCH 15/28] drm/radeon: use new iterator in radeon_sync_resv
Simplifying the code a bit. Signed-off-by: Christian König --- drivers/gpu/drm/radeon/radeon_sync.c | 22 +++--- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_sync.c b/drivers/gpu/drm/radeon/radeon_sync.c index 9257b60144c4..b991ba1bcd51 100644 --- a/drivers/gpu/drm/radeon/radeon_sync.c +++ b/drivers/gpu/drm/radeon/radeon_sync.c @@ -91,33 +91,17 @@ int radeon_sync_resv(struct radeon_device *rdev, struct dma_resv *resv, bool shared) { - struct dma_resv_list *flist; - struct dma_fence *f; + struct dma_resv_iter cursor; struct radeon_fence *fence; - unsigned i; + struct dma_fence *f; int r = 0; - /* always sync to the exclusive fence */ - f = dma_resv_excl_fence(resv); - fence = f ? to_radeon_fence(f) : NULL; - if (fence && fence->rdev == rdev) - radeon_sync_fence(sync, fence); - else if (f) - r = dma_fence_wait(f, true); - - flist = dma_resv_shared_list(resv); - if (shared || !flist || r) - return r; - - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(resv)); + dma_resv_for_each_fence(&cursor, resv, shared, f) { fence = to_radeon_fence(f); if (fence && fence->rdev == rdev) radeon_sync_fence(sync, fence); else r = dma_fence_wait(f, true); - if (r) break; } -- 2.25.1
[PATCH 26/28] drm/nouveau: use the new interator in nv50_wndw_prepare_fb
Makes the handling a bit more complex, but avoids the use of dma_resv_get_excl_unlocked(). Signed-off-by: Christian König --- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c index 8d048bacd6f0..30712a681e2a 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c @@ -539,6 +539,8 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) struct nouveau_bo *nvbo; struct nv50_head_atom *asyh; struct nv50_wndw_ctxdma *ctxdma; + struct dma_resv_iter cursor; + struct dma_fence *fence; int ret; NV_ATOMIC(drm, "%s prepare: %p\n", plane->name, fb); @@ -561,7 +563,13 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) asyw->image.handle[0] = ctxdma->object.handle; } - asyw->state.fence = dma_resv_get_excl_unlocked(nvbo->bo.base.resv); + dma_resv_iter_begin(&cursor, nvbo->bo.base.resv, false); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + /* TODO: We only use the first writer here */ + asyw->state.fence = dma_fence_get(fence); + break; + } + dma_resv_iter_end(&cursor); asyw->image.offset[0] = nvbo->offset; if (wndw->func->prepare) { -- 2.25.1
[PATCH 25/28] drm/nouveau: use the new iterator in nouveau_fence_sync
Simplifying the code a bit. Signed-off-by: Christian König --- drivers/gpu/drm/nouveau/nouveau_fence.c | 48 +++-- 1 file changed, 12 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index 05d0b3eb3690..26f9299df881 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -339,14 +339,15 @@ nouveau_fence_wait(struct nouveau_fence *fence, bool lazy, bool intr) } int -nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool exclusive, bool intr) +nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, + bool exclusive, bool intr) { struct nouveau_fence_chan *fctx = chan->fence; - struct dma_fence *fence; struct dma_resv *resv = nvbo->bo.base.resv; - struct dma_resv_list *fobj; + struct dma_resv_iter cursor; + struct dma_fence *fence; struct nouveau_fence *f; - int ret = 0, i; + int ret; if (!exclusive) { ret = dma_resv_reserve_shared(resv, 1); @@ -355,10 +356,7 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool e return ret; } - fobj = dma_resv_shared_list(resv); - fence = dma_resv_excl_fence(resv); - - if (fence) { + dma_resv_for_each_fence(&cursor, resv, exclusive, fence) { struct nouveau_channel *prev = NULL; bool must_wait = true; @@ -366,41 +364,19 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool e if (f) { rcu_read_lock(); prev = rcu_dereference(f->channel); - if (prev && (prev == chan || fctx->sync(f, prev, chan) == 0)) + if (prev && (prev == chan || +fctx->sync(f, prev, chan) == 0)) must_wait = false; rcu_read_unlock(); } - if (must_wait) + if (must_wait) { ret = dma_fence_wait(fence, intr); - - return ret; - } - - if (!exclusive || !fobj) - return ret; - - for (i = 0; i < fobj->shared_count && !ret; ++i) { - struct nouveau_channel *prev = NULL; - bool must_wait = true; - - fence = rcu_dereference_protected(fobj->shared[i], - dma_resv_held(resv)); - - f = nouveau_local_fence(fence, chan->drm); - if (f) { - rcu_read_lock(); - prev = rcu_dereference(f->channel); - if (prev && (prev == chan || fctx->sync(f, prev, chan) == 0)) - must_wait = false; - rcu_read_unlock(); + if (ret) + return ret; } - - if (must_wait) - ret = dma_fence_wait(fence, intr); } - - return ret; + return 0; } void -- 2.25.1
[PATCH 27/28] drm/etnaviv: use new iterator in etnaviv_gem_describe
Instead of hand rolling the logic. Signed-off-by: Christian König --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 31 ++- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 8f1b5af47dd6..0eeb33de2ff4 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -428,19 +428,17 @@ int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj, static void etnaviv_gem_describe_fence(struct dma_fence *fence, const char *type, struct seq_file *m) { - if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) - seq_printf(m, "\t%9s: %s %s seq %llu\n", - type, - fence->ops->get_driver_name(fence), - fence->ops->get_timeline_name(fence), - fence->seqno); + seq_printf(m, "\t%9s: %s %s seq %llu\n", type, + fence->ops->get_driver_name(fence), + fence->ops->get_timeline_name(fence), + fence->seqno); } static void etnaviv_gem_describe(struct drm_gem_object *obj, struct seq_file *m) { struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); struct dma_resv *robj = obj->resv; - struct dma_resv_list *fobj; + struct dma_resv_iter cursor; struct dma_fence *fence; unsigned long off = drm_vma_node_start(&obj->vma_node); @@ -449,21 +447,14 @@ static void etnaviv_gem_describe(struct drm_gem_object *obj, struct seq_file *m) obj->name, kref_read(&obj->refcount), off, etnaviv_obj->vaddr, obj->size); - rcu_read_lock(); - fobj = dma_resv_shared_list(robj); - if (fobj) { - unsigned int i, shared_count = fobj->shared_count; - - for (i = 0; i < shared_count; i++) { - fence = rcu_dereference(fobj->shared[i]); + dma_resv_iter_begin(&cursor, robj, true); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + if (dma_resv_iter_is_exclusive(&cursor)) + etnaviv_gem_describe_fence(fence, "Exclusive", m); + else etnaviv_gem_describe_fence(fence, "Shared", m); - } } - - fence = dma_resv_excl_fence(robj); - if (fence) - etnaviv_gem_describe_fence(fence, "Exclusive", m); - rcu_read_unlock(); + dma_resv_iter_end(&cursor); } void etnaviv_gem_describe_objects(struct etnaviv_drm_private *priv, -- 2.25.1
[PATCH 28/28] drm/etnaviv: replace dma_resv_get_excl_unlocked
We certainly hold the reservation lock here, no need for the RCU dance. Signed-off-by: Christian König --- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 4dd7d9d541c0..7e17bc2b5df1 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -195,7 +195,7 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) if (ret) return ret; } else { - bo->excl = dma_resv_get_excl_unlocked(robj); + bo->excl = dma_fence_get(dma_resv_excl_fence(robj)); } } -- 2.25.1
[PATCH 23/28] drm: use new iterator in drm_gem_fence_array_add_implicit v3
Simplifying the code a bit. v2: add missing rcu_read_lock()/unlock() v3: switch to locked version Signed-off-by: Christian König --- drivers/gpu/drm/drm_gem.c | 26 +- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 09c820045859..4dcdec6487bb 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1340,31 +1340,15 @@ int drm_gem_fence_array_add_implicit(struct xarray *fence_array, struct drm_gem_object *obj, bool write) { - int ret; - struct dma_fence **fences; - unsigned int i, fence_count; - - if (!write) { - struct dma_fence *fence = - dma_resv_get_excl_unlocked(obj->resv); - - return drm_gem_fence_array_add(fence_array, fence); - } + struct dma_resv_iter cursor; + struct dma_fence *fence; + int ret = 0; - ret = dma_resv_get_fences(obj->resv, NULL, - &fence_count, &fences); - if (ret || !fence_count) - return ret; - - for (i = 0; i < fence_count; i++) { - ret = drm_gem_fence_array_add(fence_array, fences[i]); + dma_resv_for_each_fence(&cursor, obj->resv, write, fence) { + ret = drm_gem_fence_array_add(fence_array, fence); if (ret) break; } - - for (; i < fence_count; i++) - dma_fence_put(fences[i]); - kfree(fences); return ret; } EXPORT_SYMBOL(drm_gem_fence_array_add_implicit); -- 2.25.1
Re: [PATCH v9 0/4] drm: LogiCVC display controller support
Hi, On Tue 14 Sep 21, 22:05, Paul Kocialkowski wrote: > This series introduces support for the LogiCVC display controller. > The controller is a bit unusual since it is usually loaded as > programmable logic on Xilinx FPGAs or Zynq-7000 SoCs. > More details are presented on the main commit for the driver. A gentle ping here, I think this series is pretty much good to go and unless there are objections, I'd be very happy to see it merged soon rather than having to post another version to follow internal API changes. Thanks! Paul > More information about the controller is available on the dedicated > web page: https://www.logicbricks.com/Products/logiCVC-ML.aspx > > Note that this driver has rather simple connector management, which was > not converted to drm_panel_bridge to keep the ability to enable the panel > at first vblank but also to support DVI. > > Changes since v8: > - Rebased on top of the latest drm-misc-next; > - Dropped useless phandle-based syscon regmap support; > - Switched to a single-port graph description; > - Updated the device-tree schema to the port schema and added a > description for the port. > > Change since v7: > - Replaced DRM_INFO/DRM_ERROR/DRM_DEBUG_DRIVER with fashions using drm_device; > - Fixed yaml binding alignment issue; > - Renamed logicvc-display name to the generic "display" name; > - Added patternProperties match for display in the parent mfd binding; > - Used drm_atomic_get_new_crtc_state when needed; > - Checked mode in mode_valid instead of atomic_check; > - Switched to drmm_mode_config_init; > - Removed useless logicvc_connector_destroy wrapper; > - Removed useless drm_dev_put calls; > - Removed atomic_commit_tail that enables the panel and streamlined the logic; > - Reworked Makefile cosmetics; > - Fixed checkpatch issues. > > Changes since v6: > - Updated to the latest DRM internal API changes; > - Used an enum to index dt properties instead of the name string. > > Changes since v5: > - Subclass DRM device and use devm_drm_dev_alloc for allocation; > - Removed call to drm_mode_config_cleanup (done automatically with devm); > - Some related code cleanups; > - Bring back not-for-merge patch adding colorkey support. > > Changes since v4: > - Updated to internal DRM API changes (rebased on drm-misc-next); > - Added Kconfig dependency on OF; > - Added MAINTAINERS entry; > - Used drm_err and dev_err instead of DRM_ERROR where possible; > - Various cosmetic changes. > > Changes since v3: > - Rebased on latest drm-misc; > - Improved event lock wrapping; > - Added collect tag; > - Added color-key support patch (not for merge, for reference only). > > Changes since v2: > - Fixed and slightly improved dt schema. > > Changes since v1: > - Switched dt bindings documentation to dt schema; > - Described more possible dt parameters; > - Added support for the lvds-3bit interface; > - Added support for grabbing syscon regmap from parent node; > - Removed layers count property and count layers child nodes instead. > > Paul Kocialkowski (4): > dt-bindings: display: Document the Xylon LogiCVC display controller > dt-bindings: mfd: logicvc: Add patternProperties for the display > drm: Add support for the LogiCVC display controller > NOTFORMERGE: drm/logicvc: Add plane colorkey support > > .../display/xylon,logicvc-display.yaml| 302 +++ > .../bindings/mfd/xylon,logicvc.yaml | 3 + > MAINTAINERS | 6 + > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/logicvc/Kconfig | 9 + > drivers/gpu/drm/logicvc/Makefile | 9 + > drivers/gpu/drm/logicvc/logicvc_crtc.c| 280 +++ > drivers/gpu/drm/logicvc/logicvc_crtc.h| 21 + > drivers/gpu/drm/logicvc/logicvc_drm.c | 471 +++ > drivers/gpu/drm/logicvc/logicvc_drm.h | 67 ++ > drivers/gpu/drm/logicvc/logicvc_interface.c | 214 + > drivers/gpu/drm/logicvc/logicvc_interface.h | 28 + > drivers/gpu/drm/logicvc/logicvc_layer.c | 767 ++ > drivers/gpu/drm/logicvc/logicvc_layer.h | 71 ++ > drivers/gpu/drm/logicvc/logicvc_mode.c| 80 ++ > drivers/gpu/drm/logicvc/logicvc_mode.h| 15 + > drivers/gpu/drm/logicvc/logicvc_of.c | 185 + > drivers/gpu/drm/logicvc/logicvc_of.h | 46 ++ > drivers/gpu/drm/logicvc/logicvc_regs.h| 88 ++ > 20 files changed, 2665 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/xylon,logicvc-display.yaml > create mode 100644 drivers/gpu/drm/logicvc/Kconfig > create mode 100644 drivers/gpu/drm/logicvc/Makefile > create mode 100644 drivers/gpu/drm/logicvc/logicvc_crtc.c > create mode 100644 drivers/gpu/drm/logicvc/logicvc_crtc.h > create mode 100644 drivers/gpu/drm/logicvc/logicvc_drm.c > create mode 100644 drivers/gpu/drm/logicvc/logicvc_drm.h > create mode 100644 dri
[PATCH] drm/amdgpu: remove some repeated includings
Remove two repeated includings in line 46 and 47. Signed-off-by: Guo Zhengkui --- drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c index 291a47f7992a..daa798c5b882 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c @@ -30,34 +30,32 @@ #include "soc15.h" #include "gfx_v9_0.h" #include "gmc_v9_0.h" #include "df_v1_7.h" #include "df_v3_6.h" #include "nbio_v6_1.h" #include "nbio_v7_0.h" #include "nbio_v7_4.h" #include "hdp_v4_0.h" #include "vega10_ih.h" #include "vega20_ih.h" #include "sdma_v4_0.h" #include "uvd_v7_0.h" #include "vce_v4_0.h" #include "vcn_v1_0.h" -#include "vcn_v2_0.h" -#include "jpeg_v2_0.h" #include "vcn_v2_5.h" #include "jpeg_v2_5.h" #include "smuio_v9_0.h" #include "gmc_v10_0.h" #include "gfxhub_v2_0.h" #include "mmhub_v2_0.h" #include "nbio_v2_3.h" #include "nbio_v7_2.h" #include "hdp_v5_0.h" #include "nv.h" #include "navi10_ih.h" #include "gfx_v10_0.h" #include "sdma_v5_0.h" #include "sdma_v5_2.h" #include "vcn_v2_0.h" #include "jpeg_v2_0.h" -- 2.20.1
Re: [PATCH 1/4] phy: mediatek: add support for phy-mtk-hdmi-mt8195
On 07-09-21, 10:37, Guillaume Ranquet wrote: > Add basic support for the mediatek hdmi phy on MT8195 SoC > > Signed-off-by: Guillaume Ranquet > --- > drivers/phy/mediatek/Makefile | 1 + > drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 777 + > drivers/phy/mediatek/phy-mtk-hdmi-mt8195.h | 179 + > 3 files changed, 957 insertions(+) > create mode 100644 drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > create mode 100644 drivers/phy/mediatek/phy-mtk-hdmi-mt8195.h > > diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile > index ace660fbed3a..8024961160ad 100644 > --- a/drivers/phy/mediatek/Makefile > +++ b/drivers/phy/mediatek/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_PHY_MTK_XSPHY) += phy-mtk-xsphy.o > phy-mtk-hdmi-drv-y := phy-mtk-hdmi.o > phy-mtk-hdmi-drv-y += phy-mtk-hdmi-mt2701.o > phy-mtk-hdmi-drv-y += phy-mtk-hdmi-mt8173.o > +phy-mtk-hdmi-drv-y += phy-mtk-hdmi-mt8195.o > obj-$(CONFIG_PHY_MTK_HDMI) += phy-mtk-hdmi-drv.o > > phy-mtk-mipi-dsi-drv-y := phy-mtk-mipi-dsi.o > diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > new file mode 100644 > index ..0cb46ab29257 > --- /dev/null > +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > @@ -0,0 +1,777 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019 MediaTek Inc. 2021 > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "phy-mtk-hdmi-mt8195.h" > +#include "phy-mtk-hdmi.h" > + > +static inline bool mtk_hdmi_phy_readbit(struct mtk_hdmi_phy *hdmi_phy, > + unsigned short reg, unsigned int offset) > +{ > + return (readl(hdmi_phy->regs + reg) & offset); > +} > + > +/*Analog API export to HDMI Digital start*/ > +static void mtk_hdmi_ana_fifo_en(struct mtk_hdmi_phy *hdmi_phy) > +{ > + /*make data fifo writable for hdmi2.0*/ pls run checkpatch --strict to check for code style issues > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_ANA_CTL, reg_ana_hdmi20_fifo_en, > + reg_ana_hdmi20_fifo_en); > +} > + > +void mtk_tmds_high_bit_clk_ratio(struct mtk_hdmi_phy *hdmi_phy, bool enable) > +{ > + mtk_hdmi_ana_fifo_en(hdmi_phy); > + > + /* HDMI 2.0 specification, 3.4Gbps <= TMDS Bit Rate <= 6G, > + * clock bit ratio 1:40, under 3.4Gbps, clock bit ratio 1:10 > + */ /* * I prefer this generic * kernel style, pls use this! */ > + if (enable) > + mtk_hdmi_phy_mask(hdmi_phy, HDMI20_CLK_CFG, > + (0x2 << reg_txc_div_SHIFT), reg_txc_div); > + else > + mtk_hdmi_phy_mask(hdmi_phy, HDMI20_CLK_CFG, 0, reg_txc_div); > +} > + > +/*Analog API export to HDMI Digital end***/ > + > +static int mtk_hdmi_pll_select_source(struct clk_hw *hw) > +{ > + struct mtk_hdmi_phy *hdmi_phy = to_mtk_hdmi_phy(hw); > + > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_CTL_3, > + 0x0 << REG_HDMITX_REF_XTAL_SEL_SHIFT, > + REG_HDMITX_REF_XTAL_SEL); > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_CTL_3, > + 0x0 << REG_HDMITX_REF_RESPLL_SEL_SHIFT, > + REG_HDMITX_REF_RESPLL_SEL); Have you looked into helpers in bitfield.h, they help in avoiding using these shift defines > + > + /*DA_HDMITX21_REF_CK for TXPLL input source*/ > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_1_CFG_10, > + 0x0 << RG_HDMITXPLL_REF_CK_SEL_SHIFT, > + RG_HDMITXPLL_REF_CK_SEL); > + > + return 0; > +} > + > +static int mtk_hdmi_pll_performance_setting(struct clk_hw *hw) > +{ > + struct mtk_hdmi_phy *hdmi_phy = to_mtk_hdmi_phy(hw); > + > + /* no matter pll input source is HDMIRX_REF_CK, xTal26M or TVD PLL, > + * the performance configuration is the same. > + * RG_HDMITXPLL_BP2 always 1'b1 = 0x1 > + * RG_HDMITXPLL_BC[1:0] always 2'b11 = 0x3 > + * RG_HDMITXPLL_IC[4:0] always 5'b1 = 0x1 > + * RG_HDMITXPLL_BR[2:0] stage treatment: > + * 24bit or 48bit->3'b001 = 0x1 > + * 30bit or 36bit->3'b011 = 0x3 > + * RG_HDMITXPLL_IR[4:0] stage treatment: > + * 24bit,30bit,48bit ->5'b00010 = 0x2 > + * 36bit ->5'b00011 = 0x3 > + * RG_HDMITXPLL_BP[3:0] always 4'b = 0xf > + * RG_HDMITXPLL_IBAND_FIX_EN, RG_HDMITXPLL_RESERVE[14] always 2'b00 = > 0x0 > + * RG_HDMITXPLL_HIKVCO always 1'b1 = 0x1 > + */ > + > + /* BP2 */ > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_1_PLL_CFG_0, > + 0x1 << RG_HDMITXPLL_BP2_SHIFT, RG_HDMITXPLL_BP2); > + > + /* BC */ > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_1_PLL_CFG_2, > + 0x3 << RG_HDMITXPLL_BC_SHIFT, RG_HDMITXPLL_BC); > + > + /
Re: [PATCH] drm/amdgpu: remove some repeated includings
Am 01.10.21 um 12:13 schrieb Guo Zhengkui: Remove two repeated includings in line 46 and 47. Signed-off-by: Guo Zhengkui Acked-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c index 291a47f7992a..daa798c5b882 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c @@ -30,34 +30,32 @@ #include "soc15.h" #include "gfx_v9_0.h" #include "gmc_v9_0.h" #include "df_v1_7.h" #include "df_v3_6.h" #include "nbio_v6_1.h" #include "nbio_v7_0.h" #include "nbio_v7_4.h" #include "hdp_v4_0.h" #include "vega10_ih.h" #include "vega20_ih.h" #include "sdma_v4_0.h" #include "uvd_v7_0.h" #include "vce_v4_0.h" #include "vcn_v1_0.h" -#include "vcn_v2_0.h" -#include "jpeg_v2_0.h" #include "vcn_v2_5.h" #include "jpeg_v2_5.h" #include "smuio_v9_0.h" #include "gmc_v10_0.h" #include "gfxhub_v2_0.h" #include "mmhub_v2_0.h" #include "nbio_v2_3.h" #include "nbio_v7_2.h" #include "hdp_v5_0.h" #include "nv.h" #include "navi10_ih.h" #include "gfx_v10_0.h" #include "sdma_v5_0.h" #include "sdma_v5_2.h" #include "vcn_v2_0.h" #include "jpeg_v2_0.h"
Re: [RFC 1/6] sched: Add nice value change notifier
On 01/10/2021 10:04, Tvrtko Ursulin wrote: Hi Peter, On 30/09/2021 19:33, Peter Zijlstra wrote: On Thu, Sep 30, 2021 at 06:15:47PM +0100, Tvrtko Ursulin wrote: void set_user_nice(struct task_struct *p, long nice) { bool queued, running; - int old_prio; + int old_prio, ret; struct rq_flags rf; struct rq *rq; @@ -6913,6 +6945,9 @@ void set_user_nice(struct task_struct *p, long nice) */ p->sched_class->prio_changed(rq, p, old_prio); + ret = atomic_notifier_call_chain(&user_nice_notifier_list, nice, p); + WARN_ON_ONCE(ret != NOTIFY_DONE); + out_unlock: task_rq_unlock(rq, p, &rf); } No, we're not going to call out to exported, and potentially unbounded, functions under scheduler locks. Agreed, that's another good point why it is even more hairy, as I have generally alluded in the cover letter. Do you have any immediate thoughts on possible alternatives? Like for instance if I did a queue_work from set_user_nice and then ran a notifier chain async from a worker? I haven't looked at yet what repercussion would that have in terms of having to cancel the pending workers when tasks exit. I can try and prototype that and see how it would look. Hm or I simply move calling the notifier chain to after task_rq_unlock? That would leave it run under the tasklist lock so probably still quite bad. Or another option - I stash aside the tasks on a private list (adding new list_head to trask_struct), with elevated task ref count, and run the notifier chain outside any locked sections, at the end of the setpriority syscall. This way only the sycall caller pays the cost of any misbehaving notifiers in the chain. Further improvement could be per task notifiers but that would grow the task_struct more. Regards, Tvrtko There is of course an example ioprio which solves the runtime adjustments via a dedicated system call. But I don't currently feel that a third one would be a good solution. At least I don't see a case for being able to decouple the priority of CPU and GPU and computations. Have I opened a large can of worms? :) Regards, Tvrtko
Re: [PATCH 17/28] drm/i915: use the new iterator in i915_gem_busy_ioctl v2
On 01/10/2021 11:05, Christian König wrote: This makes the function much simpler since the complex retry logic is now handled else where. Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin Sorry I retract until you add the text about the increased cost of the added atomics. I think the point is important to discuss given proposal goes from zero atomics to num_fences * 2 (fence get/put unless I am mistaken) atomics per busy ioctl. That makes me lean towards just leaving this as is since it is not that complex. Regards, Tvrtko --- drivers/gpu/drm/i915/gem/i915_gem_busy.c | 35 ++-- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index 6234e17259c1..dc72b36dae54 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, { struct drm_i915_gem_busy *args = data; struct drm_i915_gem_object *obj; - struct dma_resv_list *list; - unsigned int seq; + struct dma_resv_iter cursor; + struct dma_fence *fence; int err; err = -ENOENT; @@ -109,27 +109,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ -retry: - seq = raw_read_seqcount(&obj->base.resv->seq); - - /* Translate the exclusive fence to the READ *and* WRITE engine */ - args->busy = busy_check_writer(dma_resv_excl_fence(obj->base.resv)); - - /* Translate shared fences to READ set of engines */ - list = dma_resv_shared_list(obj->base.resv); - if (list) { - unsigned int shared_count = list->shared_count, i; - - for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence = - rcu_dereference(list->shared[i]); - + args->busy = 0; + dma_resv_iter_begin(&cursor, obj->base.resv, true); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + if (dma_resv_iter_is_restarted(&cursor)) + args->busy = 0; + + if (dma_resv_iter_is_exclusive(&cursor)) + /* Translate the exclusive fence to the READ *and* WRITE engine */ + args->busy |= busy_check_writer(fence); + else + /* Translate shared fences to READ set of engines */ args->busy |= busy_check_reader(fence); - } } - - if (args->busy && read_seqcount_retry(&obj->base.resv->seq, seq)) - goto retry; + dma_resv_iter_end(&cursor); err = 0; out:
Re: [PATCH 17/28] drm/i915: use the new iterator in i915_gem_busy_ioctl v2
Am 01.10.21 um 12:37 schrieb Tvrtko Ursulin: On 01/10/2021 11:05, Christian König wrote: This makes the function much simpler since the complex retry logic is now handled else where. Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin Sorry I retract until you add the text about the increased cost of the added atomics. I think the point is important to discuss given proposal goes from zero atomics to num_fences * 2 (fence get/put unless I am mistaken) atomics per busy ioctl. That makes me lean towards just leaving this as is since it is not that complex. I'm certainly pushing hard to remove all manual RCU dance from the drivers, including this one. The only option is to either have the atomics overhead (which is indeed num_fences*2) or the locking overhead. Regards, Christian. Regards, Tvrtko --- drivers/gpu/drm/i915/gem/i915_gem_busy.c | 35 ++-- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index 6234e17259c1..dc72b36dae54 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, { struct drm_i915_gem_busy *args = data; struct drm_i915_gem_object *obj; - struct dma_resv_list *list; - unsigned int seq; + struct dma_resv_iter cursor; + struct dma_fence *fence; int err; err = -ENOENT; @@ -109,27 +109,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ -retry: - seq = raw_read_seqcount(&obj->base.resv->seq); - - /* Translate the exclusive fence to the READ *and* WRITE engine */ - args->busy = busy_check_writer(dma_resv_excl_fence(obj->base.resv)); - - /* Translate shared fences to READ set of engines */ - list = dma_resv_shared_list(obj->base.resv); - if (list) { - unsigned int shared_count = list->shared_count, i; - - for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence = - rcu_dereference(list->shared[i]); - + args->busy = 0; + dma_resv_iter_begin(&cursor, obj->base.resv, true); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + if (dma_resv_iter_is_restarted(&cursor)) + args->busy = 0; + + if (dma_resv_iter_is_exclusive(&cursor)) + /* Translate the exclusive fence to the READ *and* WRITE engine */ + args->busy |= busy_check_writer(fence); + else + /* Translate shared fences to READ set of engines */ args->busy |= busy_check_reader(fence); - } } - - if (args->busy && read_seqcount_retry(&obj->base.resv->seq, seq)) - goto retry; + dma_resv_iter_end(&cursor); err = 0; out:
Lockdep spalt on killing a processes
Hey, Andrey. while investigating some memory management problems I've got the logdep splat below. Looks like something is wrong with drm_sched_entity_kill_jobs_cb(), can you investigate? Thanks, Christian. [11176.741052] [11176.741056] WARNING: possible recursive locking detected [11176.741060] 5.15.0-rc1-00031-g9d546d600800 #171 Not tainted [11176.741066] [11176.741070] swapper/12/0 is trying to acquire lock: [11176.741074] 9c337ed175a8 (&fence->lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741088] but task is already holding lock: [11176.741092] 9c337ed172a8 (&fence->lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741100] other info that might help us debug this: [11176.741104] Possible unsafe locking scenario: [11176.741108] CPU0 [11176.741110] [11176.741113] lock(&fence->lock); [11176.741118] lock(&fence->lock); [11176.741122] *** DEADLOCK *** [11176.741125] May be due to missing lock nesting notation [11176.741128] 2 locks held by swapper/12/0: [11176.741133] #0: 9c339c30f768 (&ring->fence_drv.lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741142] #1: 9c337ed172a8 (&fence->lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741151] stack backtrace: [11176.741155] CPU: 12 PID: 0 Comm: swapper/12 Not tainted 5.15.0-rc1-00031-g9d546d600800 #171 [11176.741160] Hardware name: System manufacturer System Product Name/PRIME X399-A, BIOS 0808 10/12/2018 [11176.741165] Call Trace: [11176.741169] [11176.741173] dump_stack_lvl+0x5b/0x74 [11176.741181] dump_stack+0x10/0x12 [11176.741186] __lock_acquire.cold+0x208/0x2df [11176.741197] lock_acquire+0xc6/0x2d0 [11176.741204] ? dma_fence_signal+0x28/0x80 [11176.741212] _raw_spin_lock_irqsave+0x4d/0x70 [11176.741219] ? dma_fence_signal+0x28/0x80 [11176.741225] dma_fence_signal+0x28/0x80 [11176.741230] drm_sched_fence_finished+0x12/0x20 [gpu_sched] [11176.741240] drm_sched_entity_kill_jobs_cb+0x1c/0x50 [gpu_sched] [11176.741248] dma_fence_signal_timestamp_locked+0xac/0x1a0 [11176.741254] dma_fence_signal+0x3b/0x80 [11176.741260] drm_sched_fence_finished+0x12/0x20 [gpu_sched] [11176.741268] drm_sched_job_done.isra.0+0x7f/0x1a0 [gpu_sched] [11176.741277] drm_sched_job_done_cb+0x12/0x20 [gpu_sched] [11176.741284] dma_fence_signal_timestamp_locked+0xac/0x1a0 [11176.741290] dma_fence_signal+0x3b/0x80 [11176.741296] amdgpu_fence_process+0xd1/0x140 [amdgpu] [11176.741504] sdma_v4_0_process_trap_irq+0x8c/0xb0 [amdgpu] [11176.741731] amdgpu_irq_dispatch+0xce/0x250 [amdgpu] [11176.741954] amdgpu_ih_process+0x81/0x100 [amdgpu] [11176.742174] amdgpu_irq_handler+0x26/0xa0 [amdgpu] [11176.742393] __handle_irq_event_percpu+0x4f/0x2c0 [11176.742402] handle_irq_event_percpu+0x33/0x80 [11176.742408] handle_irq_event+0x39/0x60 [11176.742414] handle_edge_irq+0x93/0x1d0 [11176.742419] __common_interrupt+0x50/0xe0 [11176.742426] common_interrupt+0x80/0x90 [11176.742431] [11176.742436] asm_common_interrupt+0x1e/0x40 [11176.742442] RIP: 0010:cpuidle_enter_state+0xff/0x470 [11176.742449] Code: 0f a3 05 04 54 24 01 0f 82 70 02 00 00 31 ff e8 37 5d 6f ff 80 7d d7 00 0f 85 e9 01 00 00 e8 58 a2 7f ff fb 66 0f 1f 44 00 00 <45> 85 ff 0f 88 01 01 00 00 49 63 c7 4c 2b 75 c8 48 8d 14 40 48 8d [11176.742455] RSP: 0018:b6970021fe48 EFLAGS: 0202 [11176.742461] RAX: 0059be25 RBX: 0002 RCX: [11176.742465] RDX: RSI: RDI: 9efeed78 [11176.742470] RBP: b6970021fe80 R08: 0001 R09: 0001 [11176.742473] R10: 0001 R11: 0001 R12: 9c3350b0e800 [11176.742477] R13: a00e9680 R14: 0a2a49ada060 R15: 0002 [11176.742483] ? cpuidle_enter_state+0xf8/0x470 [11176.742489] ? cpuidle_enter_state+0xf8/0x470 [11176.742495] cpuidle_enter+0x2e/0x40 [11176.742500] call_cpuidle+0x23/0x40 [11176.742506] do_idle+0x201/0x280 [11176.742512] cpu_startup_entry+0x20/0x30 [11176.742517] start_secondary+0x11f/0x160 [11176.742523] secondary_startup_64_no_verify+0xb0/0xbb
Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE
Quoting Lucas De Marchi (2021-10-01 08:40:41) > When trying to bring IS_ACTIVE to linux/kconfig.h I thought it wouldn't > provide much value just encapsulating it in a boolean context. So I also > added the support for handling undefined macros as the IS_ENABLED() > counterpart. However the feedback received from Masahiro Yamada was that > it is too ugly, not providing much value. And just wrapping in a boolean > context is too dumb - we could simply open code it. > > As detailed in commit babaab2f4738 ("drm/i915: Encapsulate kconfig > constant values inside boolean predicates"), the IS_ACTIVE macro was > added to workaround a compilation warning. However after checking again > our current uses of IS_ACTIVE it turned out there is only > 1 case in which it would potentially trigger a warning. All the others > can simply use the shorter version, without wrapping it in any macro. > And even that single one didn't trigger any warning in gcc 10.3. > > So here I'm dialing all the way back to simply removing the macro. If it > triggers warnings in future we may change the few cases to check for > 0 > or != 0. Another possibility would be to use the great "not not > operator" for all positive checks, which would allow us to maintain > consistency. However let's try first the simplest form though, hopefully > we don't hit broken compilers spitting a warning: You didn't prevent the compilation warning this re-introduces. drivers/gpu/drm/i915/i915_config.c:11 i915_fence_context_timeout() warn: should this be a bitwise op? drivers/gpu/drm/i915/i915_request.c:1679 i915_request_wait() warn: should this be a bitwise op? -Chris
Re: [PATCH] drm/amdkfd: match the signatures of the real and stub kgd2kfd_probe()
Hi, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.15-rc3 next-20210922] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/trix-redhat-com/drm-amdkfd-match-the-signatures-of-the-real-and-stub-kgd2kfd_probe/20211001-043648 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 02d5e016800d082058b3d3b7c3ede136cdc6ddcb config: riscv-randconfig-r042-20211001 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 962e503cc8bc411f7523cc393acae8aae425b1c4) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/0day-ci/linux/commit/f110a72ee42592cc2f841f5c345ffe7e0b831124 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review trix-redhat-com/drm-amdkfd-match-the-signatures-of-the-real-and-stub-kgd2kfd_probe/20211001-043648 git checkout f110a72ee42592cc2f841f5c345ffe7e0b831124 # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/gpu/drm/amd/amdgpu/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): >> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c:74:23: error: too many arguments >> to function call, expected 2, have 4 adev->pdev, adev->asic_type, vf); ^~~ drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu_amdkfd.h:349:17: note: 'kgd2kfd_probe' declared here struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd, bool vf) ^ 1 error generated. vim +74 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 5c33f214db87b3 Felix Kuehling 2017-07-28 65 5c33f214db87b3 Felix Kuehling 2017-07-28 66 void amdgpu_amdkfd_device_probe(struct amdgpu_device *adev) 5c33f214db87b3 Felix Kuehling 2017-07-28 67 { 050091ab6e832f Yong Zhao 2019-09-03 68bool vf = amdgpu_sriov_vf(adev); 5c33f214db87b3 Felix Kuehling 2017-07-28 69 c7651b73586600 Felix Kuehling 2020-09-16 70if (!kfd_initialized) c7651b73586600 Felix Kuehling 2020-09-16 71return; c7651b73586600 Felix Kuehling 2020-09-16 72 8e07e2676a42e7 Amber Lin 2018-12-13 73adev->kfd.dev = kgd2kfd_probe((struct kgd_dev *)adev, e392c887df9791 Yong Zhao 2019-09-27 @74 adev->pdev, adev->asic_type, vf); 611736d8447c0c Felix Kuehling 2018-11-19 75 611736d8447c0c Felix Kuehling 2018-11-19 76if (adev->kfd.dev) 611736d8447c0c Felix Kuehling 2018-11-19 77 amdgpu_amdkfd_total_mem_size += adev->gmc.real_vram_size; 130e0371b7d454 Oded Gabbay2015-06-12 78 } 130e0371b7d454 Oded Gabbay2015-06-12 79 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH v10, 2/5] drm/mediatek: add component POSTMASK
On 30.09.21 17:52, Yongqiang Niu wrote: This patch add component POSTMASK. Signed-off-by: Yongqiang Niu Signed-off-by: Hsin-Yi Wang Reviewed-by: CK Hu --- drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 102 ++-- drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + 2 files changed, 73 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c index 4a2abcf3e5f9..89170ad825fd 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c @@ -62,6 +62,12 @@ #define DITHER_ADD_LSHIFT_G(x)(((x) & 0x7) << 4) #define DITHER_ADD_RSHIFT_G(x)(((x) & 0x7) << 0) +#define DISP_POSTMASK_EN 0x +#define POSTMASK_ENBIT(0) +#define DISP_POSTMASK_CFG 0x0020 +#define POSTMASK_RELAY_MODEBIT(0) +#define DISP_POSTMASK_SIZE 0x0030 + struct mtk_ddp_comp_dev { struct clk *clk; void __iomem *regs; @@ -214,6 +220,32 @@ static void mtk_dither_stop(struct device *dev) writel_relaxed(0x0, priv->regs + DISP_DITHER_EN); } +static void mtk_postmask_config(struct device *dev, unsigned int w, + unsigned int h, unsigned int vrefresh, + unsigned int bpc, struct cmdq_pkt *cmdq_pkt) +{ + struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev); + + mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs, + DISP_POSTMASK_SIZE); + mtk_ddp_write(cmdq_pkt, POSTMASK_RELAY_MODE, &priv->cmdq_reg, + priv->regs, DISP_POSTMASK_CFG); +} + +static void mtk_postmask_start(struct device *dev) +{ + struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev); + + writel(POSTMASK_EN, priv->regs + DISP_POSTMASK_EN); +} + +static void mtk_postmask_stop(struct device *dev) +{ + struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev); + + writel_relaxed(0x0, priv->regs + DISP_POSTMASK_EN); +} + static const struct mtk_ddp_comp_funcs ddp_aal = { .clk_enable = mtk_aal_clk_enable, .clk_disable = mtk_aal_clk_disable, @@ -289,6 +321,14 @@ static const struct mtk_ddp_comp_funcs ddp_ovl = { .bgclr_in_off = mtk_ovl_bgclr_in_off, }; +static const struct mtk_ddp_comp_funcs ddp_postmask = { + .clk_enable = mtk_ddp_clk_enable, + .clk_disable = mtk_ddp_clk_disable, + .config = mtk_postmask_config, + .start = mtk_postmask_start, + .stop = mtk_postmask_stop, +}; + static const struct mtk_ddp_comp_funcs ddp_rdma = { .clk_enable = mtk_rdma_clk_enable, .clk_disable = mtk_rdma_clk_disable, @@ -324,6 +364,7 @@ static const char * const mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX] = { [MTK_DISP_MUTEX] = "mutex", [MTK_DISP_OD] = "od", [MTK_DISP_BLS] = "bls", + [MTK_DISP_POSTMASK] = "postmask", }; struct mtk_ddp_comp_match { @@ -333,36 +374,37 @@ struct mtk_ddp_comp_match { }; static const struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = { - [DDP_COMPONENT_AAL0]= { MTK_DISP_AAL, 0, &ddp_aal }, - [DDP_COMPONENT_AAL1]= { MTK_DISP_AAL, 1, &ddp_aal }, - [DDP_COMPONENT_BLS] = { MTK_DISP_BLS, 0, NULL }, - [DDP_COMPONENT_CCORR] = { MTK_DISP_CCORR, 0, &ddp_ccorr }, - [DDP_COMPONENT_COLOR0] = { MTK_DISP_COLOR, 0, &ddp_color }, - [DDP_COMPONENT_COLOR1] = { MTK_DISP_COLOR, 1, &ddp_color }, - [DDP_COMPONENT_DITHER] = { MTK_DISP_DITHER,0, &ddp_dither }, - [DDP_COMPONENT_DPI0]= { MTK_DPI,0, &ddp_dpi }, - [DDP_COMPONENT_DPI1]= { MTK_DPI,1, &ddp_dpi }, - [DDP_COMPONENT_DSI0]= { MTK_DSI,0, &ddp_dsi }, - [DDP_COMPONENT_DSI1]= { MTK_DSI,1, &ddp_dsi }, - [DDP_COMPONENT_DSI2]= { MTK_DSI,2, &ddp_dsi }, - [DDP_COMPONENT_DSI3]= { MTK_DSI,3, &ddp_dsi }, - [DDP_COMPONENT_GAMMA] = { MTK_DISP_GAMMA, 0, &ddp_gamma }, - [DDP_COMPONENT_OD0] = { MTK_DISP_OD,0, &ddp_od }, - [DDP_COMPONENT_OD1] = { MTK_DISP_OD,1, &ddp_od }, - [DDP_COMPONENT_OVL0]= { MTK_DISP_OVL, 0, &ddp_ovl }, - [DDP_COMPONENT_OVL1]= { MTK_DISP_OVL, 1, &ddp_ovl }, - [DDP_COMPONENT_OVL_2L0] = { MTK_DISP_OVL_2L,0, &ddp_ovl }, - [DDP_COMPONENT_OVL_2L1] = { MTK_DISP_OVL_2L,1, &ddp_ovl }, - [DDP_COMPONENT_OVL_2L2] = { MTK_DISP_OVL_2L,2, &ddp_ovl }, - [DDP_COMPONENT_PWM0]= { MTK_DISP_PWM, 0, NULL }, - [DDP_COMPONENT_PWM1]= { MTK_DISP_PWM, 1, NULL }, - [DDP_COMPONENT_PWM2]= { MTK_DISP_PWM, 2, NULL }, - [DDP_COMPONENT_RDMA0] = { MTK_DISP_RDMA, 0, &ddp_rdma }, -
Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags
> > > > > + /* Executable implies readable */ > > > > > + if ((args->flags & PANFROST_BO_NOREAD) && > > > > > + !(args->flags & PANFROST_BO_NOEXEC)) > > > > > + return -EINVAL; > > > > > > > > Generally, executable also implies not-writeable. Should we check that? > > > > > > > > > > We were allowing it until now, so doing that would break the backward > > > compat, unfortunately. > > > > Not a problem if you only enforce this starting with the appropriate > > UABI version, but... > > I still don't see how that solves the > situation, since old-userspace doesn't know about the new UABI, and > there's no version field on the CREATE_BO ioctl() to let the kernel > know about the UABI used by this userspace program. I mean, we could > add one, or add a new PANFROST_BO_EXTENDED_FLAGS flag to enforce this > 'noexec implies nowrite' behavior, but is it really simpler than > explicitly passing the NOWRITE flag when NOEXEC is passed? For some reason I thought the ABI version was negotiated (it is in kbase). Don't worry about it. That commit is Reviewed-by: Alyssa Rosenzweig
Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags
On 30/09/2021 23:12, Alyssa Rosenzweig wrote: + /* Executable implies readable */ + if ((args->flags & PANFROST_BO_NOREAD) && + !(args->flags & PANFROST_BO_NOEXEC)) + return -EINVAL; >>> >>> Generally, executable also implies not-writeable. Should we check that? >> >> We were allowing it until now, so doing that would break the backward >> compat, unfortunately. > > Not a problem if you only enforce this starting with the appropriate > UABI version, but... > >> Steve also mentioned that the DDK might use shaders modifying other >> shaders here [1] > > What? I believe it, but what? *might* ;) I've not looked in detail and a quick look through the code just now I can't actually find anything which does. > For the case of pilot shaders, that shouldn't require self-modifying > code. As I understand, the DDK binds the push uniform (FAU / RMU) buffer > as global shader memory (SSBO) and uses regular STORE instructions on > it. That requires writability on that BO but that should be fine. Yeah it looks like you're correct here - I've never looked closely into exactly how pilot shaders work. It would appear that the DDK checks (in user space) for the GPU-executable + GPU-writable condition and moans. So this obviously isn't used by the DDK as it stands. For the actual patch: Reviewed-by: Steven Price Although if you can figure out how to add the "private mapping" flag at the same time we can avoid bumping the version number too many times. And today I'm actually in the office so (perversely) I haven't got the hardware to actually test it at the moment - I really need to get remote access set up! Thanks, Steve
[bug report] drm/msm: Add SDM845 DPU support
Hello Jeykumar Sankaran, The patch 25fdd5933e4c: "drm/msm: Add SDM845 DPU support" from Jun 27, 2018, leads to the following Smatch static checker warning: drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c:1679 dpu_plane_init() warn: '&pdpu->mplane_list' not removed from list drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 1567 struct drm_plane *dpu_plane_init(struct drm_device *dev, 1568 uint32_t pipe, enum drm_plane_type type, 1569 unsigned long possible_crtcs, u32 master_plane_id) 1570 { 1571 struct drm_plane *plane = NULL, *master_plane = NULL; 1572 const uint32_t *format_list; 1573 struct dpu_plane *pdpu; 1574 struct msm_drm_private *priv = dev->dev_private; 1575 struct dpu_kms *kms = to_dpu_kms(priv->kms); 1576 int zpos_max = DPU_ZPOS_MAX; 1577 uint32_t num_formats; 1578 int ret = -EINVAL; 1579 1580 /* create and zero local structure */ 1581 pdpu = kzalloc(sizeof(*pdpu), GFP_KERNEL); 1582 if (!pdpu) { 1583 DPU_ERROR("[%u]failed to allocate local plane struct\n", pipe); 1584 ret = -ENOMEM; 1585 return ERR_PTR(ret); 1586 } 1587 1588 /* cache local stuff for later */ 1589 plane = &pdpu->base; 1590 pdpu->pipe = pipe; 1591 pdpu->is_virtual = (master_plane_id != 0); 1592 INIT_LIST_HEAD(&pdpu->mplane_list); 1593 master_plane = drm_plane_find(dev, NULL, master_plane_id); 1594 if (master_plane) { 1595 struct dpu_plane *mpdpu = to_dpu_plane(master_plane); 1596 1597 list_add_tail(&pdpu->mplane_list, &mpdpu->mplane_list); ^ This is not removed from the list in the error handling code so it will lead to a Use After Free. 1598 } 1599 1600 /* initialize underlying h/w driver */ 1601 pdpu->pipe_hw = dpu_hw_sspp_init(pipe, kms->mmio, kms->catalog, 1602 master_plane_id != 0); 1603 if (IS_ERR(pdpu->pipe_hw)) { 1604 DPU_ERROR("[%u]SSPP init failed\n", pipe); 1605 ret = PTR_ERR(pdpu->pipe_hw); 1606 goto clean_plane; 1607 } else if (!pdpu->pipe_hw->cap || !pdpu->pipe_hw->cap->sblk) { 1608 DPU_ERROR("[%u]SSPP init returned invalid cfg\n", pipe); 1609 goto clean_sspp; 1610 } 1611 1612 /* cache features mask for later */ 1613 pdpu->features = pdpu->pipe_hw->cap->features; 1614 pdpu->pipe_sblk = pdpu->pipe_hw->cap->sblk; 1615 if (!pdpu->pipe_sblk) { 1616 DPU_ERROR("[%u]invalid sblk\n", pipe); 1617 goto clean_sspp; 1618 } 1619 1620 if (pdpu->is_virtual) { 1621 format_list = pdpu->pipe_sblk->virt_format_list; 1622 num_formats = pdpu->pipe_sblk->virt_num_formats; 1623 } 1624 else { 1625 format_list = pdpu->pipe_sblk->format_list; 1626 num_formats = pdpu->pipe_sblk->num_formats; 1627 } 1628 1629 ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs, 1630 format_list, num_formats, 1631 supported_format_modifiers, type, NULL); 1632 if (ret) 1633 goto clean_sspp; 1634 1635 pdpu->catalog = kms->catalog; 1636 1637 if (kms->catalog->mixer_count && 1638 kms->catalog->mixer[0].sblk->maxblendstages) { 1639 zpos_max = kms->catalog->mixer[0].sblk->maxblendstages - 1; 1640 if (zpos_max > DPU_STAGE_MAX - DPU_STAGE_0 - 1) 1641 zpos_max = DPU_STAGE_MAX - DPU_STAGE_0 - 1; 1642 } 1643 1644 ret = drm_plane_create_zpos_property(plane, 0, 0, zpos_max); 1645 if (ret) 1646 DPU_ERROR("failed to install zpos property, rc = %d\n", ret); 1647 1648 drm_plane_create_alpha_property(plane); 1649 drm_plane_create_blend_mode_property(plane, 1650 BIT(DRM_MODE_BLEND_PIXEL_NONE) | 1651 BIT(DRM_MODE_BLEND_PREMULTI) | 1652 BIT(DRM_MODE_BLEND_COVERAGE)); 1653 1654 drm_plane_create_rotation_property(plane, 1655 DRM_MODE_ROTATE_0, 1656 DRM_MODE_ROTATE_0 | 1657 DRM_MODE_ROTATE_180 | 1658 DRM_MODE_REFLECT_X
[bug report] drm/msm: dsi: Handle dual-channel for 6G as well
Hello Sean Paul, The patch a6bcddbc2ee1: "drm/msm: dsi: Handle dual-channel for 6G as well" from Jul 25, 2018, leads to the following Smatch static checker warning: drivers/gpu/drm/msm/dsi/dsi_host.c:729 dsi_calc_clk_rate_6g() warn: wrong type for 'msm_host->esc_clk_rate' (should be 'ulong') drivers/gpu/drm/msm/dsi/dsi_host.c 721 int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi) 722 { 723 if (!msm_host->mode) { 724 pr_err("%s: mode not set\n", __func__); 725 return -EINVAL; 726 } 727 728 dsi_calc_pclk(msm_host, is_bonded_dsi); --> 729 msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk); ^^ I don't know why Smatch is suddenly warning about ancient msm code, but clock rates should be unsigned long. (I don't remember why). 730 return 0; 731 } regards, dan carpenter
Re: [PATCH v13 06/35] clk: tegra: Support runtime PM and power domain
On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: > > The Clock-and-Reset controller resides in a core power domain on NVIDIA > Tegra SoCs. In order to support voltage scaling of the core power domain, > we hook up DVFS-capable clocks to the core GENPD for managing of the > GENPD's performance state based on the clock changes. > > Some clocks don't have any specific physical hardware unit that backs > them, like root PLLs and system clock and they have theirs own voltage > requirements. This patch adds new clk-device driver that backs the clocks > and provides runtime PM functionality for them. A virtual clk-device is > created for each such DVFS-capable clock at the clock's registration time > by the new tegra_clk_register() helper. Driver changes clock's device > GENPD performance state based on clk-rate notifications. > > In result we have this sequence of events: > > 1. Clock driver creates virtual device for selective clocks, enables > runtime PM for the created device and registers the clock. > 2. Clk-device driver starts to listen to clock rate changes. > 3. Something changes clk rate or enables/disables clk. > 4. CCF core propagates the change through the clk tree. > 5. Clk-device driver gets clock rate-change notification or GENPD core > handles prepare/unprepare of the clock. > 6. Clk-device driver changes GENPD performance state on clock rate > change. > 7. GENPD driver changes voltage regulator state change. > 8. The regulator state is committed to hardware via I2C. > > We rely on fact that DVFS is not needed for Tegra I2C and that Tegra I2C > driver already keeps clock always-prepared. Hence I2C subsystem stays > independent from the clk power management and there are no deadlock spots > in the sequence. > > Currently all clocks are registered very early during kernel boot when the > device driver core isn't available yet. The clk-device can't be created > at that time. This patch splits the registration of the clocks in two > phases: > > 1. Register all essential clocks which don't use RPM and are needed > during early boot. > > 2. Register at a later boot time the rest of clocks. > > This patch adds power management support for Tegra20 and Tegra30 clocks. > > Tested-by: Peter Geis # Ouya T30 > Tested-by: Paul Fertser # PAZ00 T20 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko > --- > drivers/clk/tegra/Makefile | 1 + > drivers/clk/tegra/clk-device.c | 230 > drivers/clk/tegra/clk-pll.c | 2 +- > drivers/clk/tegra/clk-super.c | 2 +- > drivers/clk/tegra/clk-tegra20.c | 77 --- > drivers/clk/tegra/clk-tegra30.c | 116 +++- > drivers/clk/tegra/clk.c | 75 ++- > drivers/clk/tegra/clk.h | 2 + > 8 files changed, 451 insertions(+), 54 deletions(-) > create mode 100644 drivers/clk/tegra/clk-device.c > > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile > index 7b1816856eb5..a0715cdfc1a4 100644 > --- a/drivers/clk/tegra/Makefile > +++ b/drivers/clk/tegra/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-y += clk.o > obj-y += clk-audio-sync.o > +obj-y += clk-device.o > obj-y += clk-dfll.o > obj-y += clk-divider.o > obj-y += clk-periph.o > diff --git a/drivers/clk/tegra/clk-device.c b/drivers/clk/tegra/clk-device.c > new file mode 100644 > index ..830bc0ba25d3 > --- /dev/null > +++ b/drivers/clk/tegra/clk-device.c > @@ -0,0 +1,230 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include "clk.h" > + > +/* > + * This driver manages performance state of the core power domain for the > + * independent PLLs and system clocks. We created a virtual clock device > + * for such clocks, see tegra_clk_dev_register(). > + */ > + > +struct tegra_clk_device { > + struct notifier_block clk_nb; > + struct device *dev; > + struct clk_hw *hw; > + struct mutex lock; > +}; > + > +static int tegra_clock_set_pd_state(struct tegra_clk_device *clk_dev, > + unsigned long rate) > +{ > + struct device *dev = clk_dev->dev; > + struct dev_pm_opp *opp; > + unsigned int pstate; > + > + opp = dev_pm_opp_find_freq_ceil(dev, &rate); > + if (opp == ERR_PTR(-ERANGE)) { > + dev_dbg(dev, "failed to find ceil OPP for %luHz\n", rate); > + opp = dev_pm_opp_find_freq_floor(dev, &rate); > + } > + > + if (IS_ERR(opp)) { > + dev_err(dev, "failed to find OPP for %luHz: %pe\n", rate, >
[PATCH 1/3] drm/msm/dsi: Fix an error code in msm_dsi_modeset_init()
Return an error code if msm_dsi_manager_validate_current_config(). Don't return success. Fixes: 8b03ad30e314 ("drm/msm/dsi: Use one connector for dual DSI mode") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/msm/dsi/dsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c index 614dc7f26f2c..75ae3008b68f 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.c +++ b/drivers/gpu/drm/msm/dsi/dsi.c @@ -215,8 +215,10 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev, goto fail; } - if (!msm_dsi_manager_validate_current_config(msm_dsi->id)) + if (!msm_dsi_manager_validate_current_config(msm_dsi->id)) { + ret = -EINVAL; goto fail; + } msm_dsi->encoder = encoder; -- 2.20.1
[PATCH 2/3] drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling
This disables a lock which wasn't enabled and it does not disable the first lock in the array. Fixes: 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index e269df285136..c86b5090fae6 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -451,7 +451,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host) return 0; err: - for (; i > 0; i--) + while (--i >= 0) clk_disable_unprepare(msm_host->bus_clks[i]); return ret; -- 2.20.1
[PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()
The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx() returns a negative error code that it type promoted to a high positive value and treat as a success. The second problem with this code is that it can return meaningless positive values on error. Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/msm/dsi/dsi_host.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index c86b5090fae6..42073a562072 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -2133,8 +2133,10 @@ int msm_dsi_host_cmd_rx(struct mipi_dsi_host *host, } ret = dsi_cmds2buf_tx(msm_host, msg); - if (ret < msg->tx_len) { + if (ret < 0 || ret < msg->tx_len) { pr_err("%s: Read cmd Tx failed, %d\n", __func__, ret); + if (ret >= 0) + ret = -EIO; return ret; } -- 2.20.1
Re: [PATCH] fbdev: Garbage collect fbdev scrolling acceleration, part 1 (from TODO list)
On Fri, Oct 01, 2021 at 10:21:44AM +0200, Thomas Zimmermann wrote: > Hi > > Am 30.09.21 um 17:10 schrieb Claudio: > > Scroll acceleration is disabled in fbcon by hard-wiring > > p->scrollmode = SCROLL_REDRAW. Remove the obsolete code in fbcon.c > > and fbdev/core/ > > > > Signed-off-by: Claudio Suarez > > --- > > > > - This is a task in the TODO list Documentation/gpu/todo.rst > > - The contact in the task is Daniel Vetter. He is/you are in copy. > > - To ease the things and saving time, I did a patch. It is included in this > >message. I can redo it if there is something wrong. > > - I tested it in some configurations. > > > > My plan for new patches in this task: > > - A buch of patches to remove code from drivers: fb_copyarea and related. > > - Simplify the code around fbcon_ops as much as possible to remove the hooks > >as the TODO suggests. > > - Remove fb_copyarea in headers and exported symbols: cfb_copyarea, etc. > > This > >must be done when all the drivers are changed. > > > > I think that the correct list to ask questions about this > > is linux-fb...@vger.kernel.org . Is it correct ? > > My question: I can develop the new changes. I can test in two computers/two > > drivers. Is there a way to test the rest of the patches ? I have not > > hardware > > to test them. Is anyone helping with this? Only regression tests are needed. > > I can test other patches in return. > > > > Thank you. > > Claudio Suarez. > > > > Patch follows: > > > > Documentation/gpu/todo.rst | 13 +- > > drivers/video/fbdev/core/bitblit.c | 16 - > > drivers/video/fbdev/core/fbcon.c| 509 > > ++-- > > drivers/video/fbdev/core/fbcon.h| 59 > > drivers/video/fbdev/core/fbcon_ccw.c| 28 +- > > drivers/video/fbdev/core/fbcon_cw.c | 28 +- > > drivers/video/fbdev/core/fbcon_rotate.h | 9 - > > drivers/video/fbdev/core/fbcon_ud.c | 37 +-- > > drivers/video/fbdev/core/tileblit.c | 16 - > > drivers/video/fbdev/skeletonfb.c| 12 +- > > include/linux/fb.h | 2 +- > > 11 files changed, 51 insertions(+), 678 deletions(-) > > Nice stats :) > > I looked through it and it looks good. Maybe double-check that everything > still builds. > > Acked-by: Thomas Zimmermann > Yes, it still builds :) I had built with some different .config options, including allyesconfig, allno, some randoms and debian default config. I tested some .config options related to fbdev. I spent time running some kernels with different parameters and everything was ok. Today, I've just applied the patch to source from two gits: Linus rc and drm. Both have built ok. I think that I did enough tests to ensure it works fine. This code is going to run in many computers, mine included! Of course, if you or anyone is worried about something specific, please, tell me and I can check and re-check it. I don't want to miss something important. Thank you! Best regards Claudio Suarez > Best regards > Thomas > > > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > > index 12e61869939e..bb1e04bbf4fb 100644 > > --- a/Documentation/gpu/todo.rst > > +++ b/Documentation/gpu/todo.rst > > @@ -314,16 +314,19 @@ Level: Advanced > > Garbage collect fbdev scrolling acceleration > > > > -Scroll acceleration is disabled in fbcon by hard-wiring p->scrollmode = > > -SCROLL_REDRAW. There's a ton of code this will allow us to remove: > > +Scroll acceleration has been disabled in fbcon. Now it works as the old > > +SCROLL_REDRAW mode. A ton of code was removed in fbcon.c and the hook > > bmove was > > +removed from fbcon_ops. > > +Remaining tasks: > > -- lots of code in fbcon.c > > - > > -- a bunch of the hooks in fbcon_ops, maybe the remaining hooks could be > > called > > +- a bunch of the hooks in fbcon_ops could be removed or simplified by > > calling > > directly instead of the function table (with a switch on p->rotate) > > - fb_copyarea is unused after this, and can be deleted from all drivers > > +- after that, fb_copyarea can be deleted from fb_ops in include/linux/fb.h > > as > > + well as cfb_copyarea > > + > > Note that not all acceleration code can be deleted, since clearing and > > cursor > > support is still accelerated, which might be good candidates for further > > deletion projects. > > diff --git a/drivers/video/fbdev/core/bitblit.c > > b/drivers/video/fbdev/core/bitblit.c > > index f98e8f298bc1..01fae2c96965 100644 > > --- a/drivers/video/fbdev/core/bitblit.c > > +++ b/drivers/video/fbdev/core/bitblit.c > > @@ -43,21 +43,6 @@ static void update_attr(u8 *dst, u8 *src, int attribute, > > } > > } > > -static void bit_bmove(struct vc_data *vc, struct fb_info *info, int sy, > > - int sx, int dy, int dx, int height, int width) > > -{ > > - struct fb_copyarea area; > > - > > - area.sx = sx * vc->vc_font.w
Re: [PATCH] drm/i915: remove IS_ACTIVE
Hi Lucas, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.15-rc3 next-20210922] [cannot apply to airlied/drm-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Lucas-De-Marchi/drm-i915-remove-IS_ACTIVE/20211001-154226 base: git://anongit.freedesktop.org/drm-intel for-linux-next config: i386-randconfig-a015-20211001 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 962e503cc8bc411f7523cc393acae8aae425b1c4) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/50006042f1d264599bd1be1942f9958112e15c01 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Lucas-De-Marchi/drm-i915-remove-IS_ACTIVE/20211001-154226 git checkout 50006042f1d264599bd1be1942f9958112e15c01 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): >> drivers/gpu/drm/i915/i915_config.c:11:14: warning: use of logical '&&' with >> constant operand [-Wconstant-logical-operand] if (context && CONFIG_DRM_I915_FENCE_TIMEOUT) ^ ~ drivers/gpu/drm/i915/i915_config.c:11:14: note: use '&' for a bitwise operation if (context && CONFIG_DRM_I915_FENCE_TIMEOUT) ^~ & drivers/gpu/drm/i915/i915_config.c:11:14: note: remove constant to silence this warning if (context && CONFIG_DRM_I915_FENCE_TIMEOUT) ~^~~~ 1 warning generated. vim +11 drivers/gpu/drm/i915/i915_config.c 7 8 unsigned long 9 i915_fence_context_timeout(const struct drm_i915_private *i915, u64 context) 10 { > 11 if (context && CONFIG_DRM_I915_FENCE_TIMEOUT) --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH v13 02/35] soc/tegra: Add devm_tegra_core_dev_init_opp_table_common()
On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: > > Only couple drivers need to get the -ENODEV error code and majority of > drivers need to explicitly initialize the performance state. Add new > common helper which sets up OPP table for these drivers. > > Signed-off-by: Dmitry Osipenko > --- > include/soc/tegra/common.h | 24 > 1 file changed, 24 insertions(+) > > diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h > index af41ad80ec21..5b4a042f60fb 100644 > --- a/include/soc/tegra/common.h > +++ b/include/soc/tegra/common.h > @@ -39,4 +39,28 @@ devm_tegra_core_dev_init_opp_table(struct device *dev, > } > #endif > > +/* > + * This function should be invoked with the enabled runtime PM of the device > + * in order to initialize performance state properly. Most of Tegra devices > + * are assumed to be suspended at a probe time and GENPD require RPM to be > + * enabled to set up the rpm-resume state, otherwise device is active and > + * performance state is applied immediately. Note that it will initialize > + * OPP bandwidth if it's wired in a device-tree for this device, which is > + * undesirable for a suspended device. > + */ > +static inline int > +devm_tegra_core_dev_init_opp_table_common(struct device *dev) > +{ > + struct tegra_core_opp_params opp_params = {}; > + int err; > + > + opp_params.init_state = true; > + > + err = devm_tegra_core_dev_init_opp_table(dev, &opp_params); > + if (err != -ENODEV) > + return err; > + > + return 0; > +} Just want to share a few thoughts around these functions. So, I assume it's fine to call devm_tegra_core_dev_init_opp_table_common() or devm_tegra_core_dev_init_opp_table() from consumer drivers during ->probe(), as long as those drivers are tegra specific, which I assume all are in the series!? My point is, a cross SoC consumer driver that needs to initiate OPP tables can get rather messy, if it would need to make one specific function call per SoC. That said, I hope we can tackle this as a separate/future problem, so the series can get merged as is. > + > #endif /* __SOC_TEGRA_COMMON_H__ */ > -- > 2.32.0 > Kind regards Uffe
[PATCH] drm/msm/a4xx: fix error handling in a4xx_gpu_init()
This code returns 1 on error instead of a negative error. It leads to an Oops in the caller. A second problem is that the check for "if (ret != -ENODATA)" cannot be true because "ret" is set to 1. Fixes: 5785dd7a8ef0 ("drm/msm: Fix duplicate gpu node in icc summary") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c index 82bebb40234d..a96ee79cc5e0 100644 --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c @@ -699,13 +699,14 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev) } icc_path = devm_of_icc_get(&pdev->dev, "gfx-mem"); - ret = IS_ERR(icc_path); - if (ret) + if (IS_ERR(icc_path)) { + ret = PTR_ERR(icc_path); goto fail; + } ocmem_icc_path = devm_of_icc_get(&pdev->dev, "ocmem"); - ret = IS_ERR(ocmem_icc_path); - if (ret) { + if (IS_ERR(ocmem_icc_path)) { + ret = PTR_ERR(ocmem_icc_path); /* allow -ENODATA, ocmem icc is optional */ if (ret != -ENODATA) goto fail; -- 2.20.1
[PATCH] drm/msm/a3xx: fix error handling in a3xx_gpu_init()
These error paths returned 1 on failure, instead of a negative error code. This would lead to an Oops in the caller. A second problem is that the check for "if (ret != -ENODATA)" did not work because "ret" was set to 1. Fixes: 5785dd7a8ef0 ("drm/msm: Fix duplicate gpu node in icc summary") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c index 4534633fe7cd..8fb847c174ff 100644 --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c @@ -571,13 +571,14 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev) } icc_path = devm_of_icc_get(&pdev->dev, "gfx-mem"); - ret = IS_ERR(icc_path); - if (ret) + if (IS_ERR(icc_path)) { + ret = PTR_ERR(icc_path); goto fail; + } ocmem_icc_path = devm_of_icc_get(&pdev->dev, "ocmem"); - ret = IS_ERR(ocmem_icc_path); - if (ret) { + if (IS_ERR(ocmem_icc_path)) { + ret = PTR_ERR(ocmem_icc_path); /* allow -ENODATA, ocmem icc is optional */ if (ret != -ENODATA) goto fail; -- 2.20.1
Re: [PATCH v3 0/6] drm/gud: Add some more pixel formats
Den 30.09.2021 11.10, skrev Thomas Zimmermann: > Hi > > Am 29.09.21 um 21:11 schrieb Noralf Trønnes: >> Hi, >> >> I'd appreciate if someone could review the 3 small driver patches. > > Series is > > Acked-by: Thomas Zimmermann > Many thanks Thomas! Noralf. > Best regards > Thomas > >> >> Changes since version 2: >> - Drop the patch adding module parameter 'xrgb'. Both Daniel and >> Thomas had some comments that eventually led me to to drop this for now. >> >> Noralf. >> >> >> Noralf Trønnes (6): >> drm/fourcc: Add R8 to drm_format_info >> drm/format-helper: Add drm_fb_xrgb_to_rgb332() >> drm/format-helper: Add drm_fb_xrgb_to_rgb888() >> drm/gud: Add GUD_PIXEL_FORMAT_R8 >> drm/gud: Add GUD_PIXEL_FORMAT_RGB332 >> drm/gud: Add GUD_PIXEL_FORMAT_RGB888 >> >> drivers/gpu/drm/drm_format_helper.c | 88 + >> drivers/gpu/drm/drm_fourcc.c | 1 + >> drivers/gpu/drm/gud/gud_drv.c | 6 ++ >> drivers/gpu/drm/gud/gud_internal.h | 12 >> drivers/gpu/drm/gud/gud_pipe.c | 6 ++ >> include/drm/drm_format_helper.h | 4 ++ >> include/drm/gud.h | 6 +- >> 7 files changed, 121 insertions(+), 2 deletions(-) >> >
Re: refactor the i915 GVT support
On 9/29/21 6:55 PM, Jason Gunthorpe wrote: > On Wed, Sep 29, 2021 at 06:27:16PM +, Wang, Zhi A wrote: >> On 9/28/21 3:05 PM, Jason Gunthorpe wrote: >>> On Tue, Sep 28, 2021 at 02:35:06PM +, Wang, Zhi A wrote: >>> Yes. I was thinking of the possibility of putting off some work later so that we don't need to make a lot of changes. GVT-g needs to take a snapshot of GPU registers as the initial virtual states for other vGPUs, which requires the initialization happens at a certain early time of initialization of i915. I was thinking maybe we can take other patches from Christoph like "de-virtualize*" except this one because currently we have to maintain a TEST-ONLY patch on our tree to prevent i915 built as kernel module. >>> How about just capture these registers in the main module/device and >>> not try so hard to isolate it to the gvt stuff? >> Hi Jason: >> >> Thanks for the idea. I am not sure i915 guys would take this idea since >> that it's only for GVT-g, i915 doesn't use this at all. We need to take >> a snapshot of both PCI configuration space and MMIO registers before >> i915 driver starts to touch the HW. > Given the code is already linked into i915 I don't see there is much > to object to here. It can remain conditional on the kernel parameter > as today. > > As a general philosophy this would all be much less strange if the > mdev .ko is truely optional. It should be cleanly seperate from its > base device and never request_module'd.. > > In this case auxiliary device might be a good option, have i915 create > one and the mdev module be loaded against it. > > In the mean time is there some shortcut to get this series to move > ahead? Is patch 4 essential to the rest of the series? > > A really awful hack would be to push the pci_driver_register into a > WQ so that the request_module is guarenteed to not be part of the > module_init callchain. Hi Jason and folks: Thanks so much for the ideas. That sounds great and I was keeping thinking how to make progress on this. How about we do like this: We don't do request_module("kvmgt") in i915.ko, which resolves the circular module dependency. We keep the code of doing snapshot of registers in intel_gvt.c. When i915.enable_gvt=1, we do the snapshot. Then we export functions for kvmgt.ko in intel_gvt.c to check if gvt in i915 is enabled or not and get the snapshots. How does that sounds? I just need to write another patch and put it on top of Christoph's series. Thanks, Zhi. >> Also I was thinking if moving gvt into kvmgt.ko is the right direction. >> It seems the module loading system in kernel is not designed for "module >> A loading module B, which needs symbols from module A, in the >> initialization path of module A". > Of course not, that is a circular module dependency, it should not be > that way. The SW layers need to be clean and orderly - meaning the > i915 module needs to have the minimal amount of code to support the > mdev module. > > Jason
Re: [PATCH 1/4] phy: mediatek: add support for phy-mtk-hdmi-mt8195
Quoting Vinod Koul (2021-10-01 12:15:43) > On 07-09-21, 10:37, Guillaume Ranquet wrote: > > Add basic support for the mediatek hdmi phy on MT8195 SoC > > > > Signed-off-by: Guillaume Ranquet > > --- > > drivers/phy/mediatek/Makefile | 1 + > > drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 777 + > > drivers/phy/mediatek/phy-mtk-hdmi-mt8195.h | 179 + > > 3 files changed, 957 insertions(+) > > create mode 100644 drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > > create mode 100644 drivers/phy/mediatek/phy-mtk-hdmi-mt8195.h > > > > diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile > > index ace660fbed3a..8024961160ad 100644 > > --- a/drivers/phy/mediatek/Makefile > > +++ b/drivers/phy/mediatek/Makefile > > @@ -10,6 +10,7 @@ obj-$(CONFIG_PHY_MTK_XSPHY) += phy-mtk-xsphy.o > > phy-mtk-hdmi-drv-y := phy-mtk-hdmi.o > > phy-mtk-hdmi-drv-y += phy-mtk-hdmi-mt2701.o > > phy-mtk-hdmi-drv-y += phy-mtk-hdmi-mt8173.o > > +phy-mtk-hdmi-drv-y += phy-mtk-hdmi-mt8195.o > > obj-$(CONFIG_PHY_MTK_HDMI) += phy-mtk-hdmi-drv.o > > > > phy-mtk-mipi-dsi-drv-y := phy-mtk-mipi-dsi.o > > diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > > b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > > new file mode 100644 > > index ..0cb46ab29257 > > --- /dev/null > > +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > > @@ -0,0 +1,777 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2019 MediaTek Inc. > > 2021 > > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "phy-mtk-hdmi-mt8195.h" > > +#include "phy-mtk-hdmi.h" > > + > > +static inline bool mtk_hdmi_phy_readbit(struct mtk_hdmi_phy *hdmi_phy, > > + unsigned short reg, unsigned int > > offset) > > +{ > > + return (readl(hdmi_phy->regs + reg) & offset); > > +} > > + > > +/*Analog API export to HDMI Digital start*/ > > +static void mtk_hdmi_ana_fifo_en(struct mtk_hdmi_phy *hdmi_phy) > > +{ > > + /*make data fifo writable for hdmi2.0*/ > > pls run checkpatch --strict to check for code style issues > > > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_ANA_CTL, reg_ana_hdmi20_fifo_en, > > + reg_ana_hdmi20_fifo_en); > > +} > > + > > +void mtk_tmds_high_bit_clk_ratio(struct mtk_hdmi_phy *hdmi_phy, bool > > enable) > > +{ > > + mtk_hdmi_ana_fifo_en(hdmi_phy); > > + > > + /* HDMI 2.0 specification, 3.4Gbps <= TMDS Bit Rate <= 6G, > > + * clock bit ratio 1:40, under 3.4Gbps, clock bit ratio 1:10 > > + */ > > /* > * I prefer this generic > * kernel style, pls use this! > */ > > > + if (enable) > > + mtk_hdmi_phy_mask(hdmi_phy, HDMI20_CLK_CFG, > > + (0x2 << reg_txc_div_SHIFT), reg_txc_div); > > + else > > + mtk_hdmi_phy_mask(hdmi_phy, HDMI20_CLK_CFG, 0, reg_txc_div); > > +} > > + > > +/*Analog API export to HDMI Digital end***/ > > + > > +static int mtk_hdmi_pll_select_source(struct clk_hw *hw) > > +{ > > + struct mtk_hdmi_phy *hdmi_phy = to_mtk_hdmi_phy(hw); > > + > > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_CTL_3, > > + 0x0 << REG_HDMITX_REF_XTAL_SEL_SHIFT, > > + REG_HDMITX_REF_XTAL_SEL); > > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_CTL_3, > > + 0x0 << REG_HDMITX_REF_RESPLL_SEL_SHIFT, > > + REG_HDMITX_REF_RESPLL_SEL); > > Have you looked into helpers in bitfield.h, they help in avoiding using > these shift defines > > > + > > + /*DA_HDMITX21_REF_CK for TXPLL input source*/ > > + mtk_hdmi_phy_mask(hdmi_phy, HDMI_1_CFG_10, > > + 0x0 << RG_HDMITXPLL_REF_CK_SEL_SHIFT, > > + RG_HDMITXPLL_REF_CK_SEL); > > + > > + return 0; > > +} > > + > > +static int mtk_hdmi_pll_performance_setting(struct clk_hw *hw) > > +{ > > + struct mtk_hdmi_phy *hdmi_phy = to_mtk_hdmi_phy(hw); > > + > > + /* no matter pll input source is HDMIRX_REF_CK, xTal26M or TVD PLL, > > + * the performance configuration is the same. > > + * RG_HDMITXPLL_BP2 always 1'b1 = 0x1 > > + * RG_HDMITXPLL_BC[1:0] always 2'b11 = 0x3 > > + * RG_HDMITXPLL_IC[4:0] always 5'b1 = 0x1 > > + * RG_HDMITXPLL_BR[2:0] stage treatment: > > + * 24bit or 48bit->3'b001 = 0x1 > > + * 30bit or 36bit->3'b011 = 0x3 > > + * RG_HDMITXPLL_IR[4:0] stage treatment: > > + * 24bit,30bit,48bit ->5'b00010 = 0x2 > > + * 36bit ->5'b00011 = 0x3 > > + * RG_HDMITXPLL_BP[3:0] always 4'b = 0xf > > + * RG_HDMITXPLL_IBAND_FIX_EN, RG_HDMITXPLL_RESERVE[14] always 2'b00 = > > 0x0 > > + * RG_HDMITXPLL_HIKVCO always 1'b1 = 0x1 > > + */ > > + > >
Re: [PATCH v13 09/35] gpu: host1x: Add runtime PM and OPP support
On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: > > Add runtime PM and OPP support to the Host1x driver. For the starter we > will keep host1x always-on because dynamic power management require a major > refactoring of the driver code since lot's of code paths are missing the > RPM handling and we're going to remove some of these paths in the future. > > Tested-by: Peter Geis # Ouya T30 > Tested-by: Paul Fertser # PAZ00 T20 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko > --- [...] > --- a/drivers/gpu/host1x/dev.c > +++ b/drivers/gpu/host1x/dev.c > @@ -6,14 +6,18 @@ > */ > > #include > +#include > #include > #include > #include > #include > #include > #include > +#include > #include > > +#include > + > #define CREATE_TRACE_POINTS > #include > #undef CREATE_TRACE_POINTS > @@ -190,6 +194,9 @@ static void host1x_setup_sid_table(struct host1x *host) > const struct host1x_info *info = host->info; > unsigned int i; > > + if (!info->has_hypervisor) > + return; > + > for (i = 0; i < info->num_sid_entries; i++) { > const struct host1x_sid_entry *entry = &info->sid_table[i]; > > @@ -347,6 +354,27 @@ static void host1x_iommu_exit(struct host1x *host) > } > } > > +static int host1x_get_resets(struct host1x *host) > +{ > + int err; > + > + host->resets[0].id = "mc"; > + host->resets[1].id = "host1x"; > + host->nresets = ARRAY_SIZE(host->resets); > + > + err = devm_reset_control_bulk_get_optional_exclusive_released( > + host->dev, host->nresets, host->resets); > + if (err) { > + dev_err(host->dev, "failed to get reset: %d\n", err); > + return err; > + } > + > + if (WARN_ON(!host->resets[1].rstc)) > + return -ENOENT; > + > + return 0; > +} > + > static int host1x_probe(struct platform_device *pdev) > { > struct host1x *host; > @@ -423,12 +451,9 @@ static int host1x_probe(struct platform_device *pdev) > return err; > } > > - host->rst = devm_reset_control_get(&pdev->dev, "host1x"); > - if (IS_ERR(host->rst)) { > - err = PTR_ERR(host->rst); > - dev_err(&pdev->dev, "failed to get reset: %d\n", err); > + err = host1x_get_resets(host); > + if (err) > return err; > - } > > err = host1x_iommu_init(host); > if (err < 0) { > @@ -443,22 +468,10 @@ static int host1x_probe(struct platform_device *pdev) > goto iommu_exit; > } > > - err = clk_prepare_enable(host->clk); > - if (err < 0) { > - dev_err(&pdev->dev, "failed to enable clock\n"); > - goto free_channels; > - } > - > - err = reset_control_deassert(host->rst); > - if (err < 0) { > - dev_err(&pdev->dev, "failed to deassert reset: %d\n", err); > - goto unprepare_disable; > - } > - > err = host1x_syncpt_init(host); > if (err) { > dev_err(&pdev->dev, "failed to initialize syncpts\n"); > - goto reset_assert; > + goto free_channels; > } > > err = host1x_intr_init(host, syncpt_irq); > @@ -467,10 +480,18 @@ static int host1x_probe(struct platform_device *pdev) > goto deinit_syncpt; > } > > - host1x_debug_init(host); > + pm_runtime_enable(&pdev->dev); > + > + err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > + if (err) > + goto pm_disable; > > - if (host->info->has_hypervisor) > - host1x_setup_sid_table(host); > + /* the driver's code isn't ready yet for the dynamic RPM */ > + err = pm_runtime_resume_and_get(&pdev->dev); > + if (err) > + goto pm_disable; > + > + host1x_debug_init(host); > > err = host1x_register(host); > if (err < 0) > @@ -486,13 +507,14 @@ static int host1x_probe(struct platform_device *pdev) > host1x_unregister(host); > deinit_debugfs: > host1x_debug_deinit(host); > + > + pm_runtime_put(&pdev->dev); pm_runtime_put() is asynchronous, so it may not actually succeed to trigger the ->runtime_suspend() callback to be invoked. Thus, this could end up that we leave clocks prepared/enabled when ->probe() fails, for example. I guess pm_runtime_put_sync_suspend() is slightly better. Another option is to call pm_runtime_force_suspend(), but then you must skip the call pm_runtime_disable() afterwards, as that has already been done inside that function. > +pm_disable: > + pm_runtime_disable(&pdev->dev); > + > host1x_intr_deinit(host); > deinit_syncpt: > host1x_syncpt_deinit(host); > -reset_assert: > - reset_control_assert(host->rst); > -unprepare_disable: > -
Re: [PATCH v13 11/35] drm/tegra: dc: Support OPP and SoC core voltage scaling
On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: > > Add OPP and SoC core voltage scaling support to the display controller > driver. This is required for enabling system-wide DVFS on pre-Tegra186 > SoCs. > > Tested-by: Peter Geis # Ouya T30 > Tested-by: Paul Fertser # PAZ00 T20 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko > --- > drivers/gpu/drm/tegra/dc.c | 74 ++ > drivers/gpu/drm/tegra/dc.h | 2 ++ > 2 files changed, 76 insertions(+) > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > index a29d64f87563..d4047a14e2b6 100644 > --- a/drivers/gpu/drm/tegra/dc.c > +++ b/drivers/gpu/drm/tegra/dc.c > @@ -11,9 +11,12 @@ > #include > #include > #include > +#include > +#include > #include > #include > > +#include > #include > > #include > @@ -1762,6 +1765,47 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc, > return 0; > } > > +static void tegra_dc_update_voltage_state(struct tegra_dc *dc, > + struct tegra_dc_state *state) > +{ > + unsigned long rate, pstate; > + struct dev_pm_opp *opp; > + int err; > + > + if (!dc->has_opp_table) > + return; > + > + /* calculate actual pixel clock rate which depends on internal > divider */ > + rate = DIV_ROUND_UP(clk_get_rate(dc->clk) * 2, state->div + 2); > + > + /* find suitable OPP for the rate */ > + opp = dev_pm_opp_find_freq_ceil(dc->dev, &rate); > + > + if (opp == ERR_PTR(-ERANGE)) > + opp = dev_pm_opp_find_freq_floor(dc->dev, &rate); > + > + if (IS_ERR(opp)) { > + dev_err(dc->dev, "failed to find OPP for %luHz: %pe\n", > + rate, opp); > + return; > + } > + > + pstate = dev_pm_opp_get_required_pstate(opp, 0); > + dev_pm_opp_put(opp); > + > + /* > +* The minimum core voltage depends on the pixel clock rate (which > +* depends on internal clock divider of the CRTC) and not on the > +* rate of the display controller clock. This is why we're not using > +* dev_pm_opp_set_rate() API and instead controlling the power domain > +* directly. > +*/ > + err = dev_pm_genpd_set_performance_state(dc->dev, pstate); > + if (err) > + dev_err(dc->dev, "failed to set power domain state to %lu: > %d\n", > + pstate, err); Yeah, the above code looks very similar to the code I pointed to in patch6. Perhaps we need to discuss with Viresh, whether it makes sense to fold in a patch adding an opp helper function after all, to avoid the open coding. Viresh? [...] Kind regards Uffe
Re: [PATCH v13 13/35] drm/tegra: gr2d: Support generic power domain and runtime PM
On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: > > Add runtime power management and support generic power domains. > > Tested-by: Peter Geis # Ouya T30 > Tested-by: Paul Fertser # PAZ00 T20 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko > --- > drivers/gpu/drm/tegra/gr2d.c | 155 +-- [...] > static int gr2d_remove(struct platform_device *pdev) > @@ -259,15 +312,101 @@ static int gr2d_remove(struct platform_device *pdev) > return err; > } > > + pm_runtime_dont_use_autosuspend(&pdev->dev); > + pm_runtime_disable(&pdev->dev); There is no guarantee that the ->runtime_suspend() has been invoked here, which means that clock may be left prepared/enabled beyond this point. I suggest you call pm_runtime_force_suspend(), instead of pm_runtime_disable(), to make sure that gets done. [...] Kind regards Uffe
[bug report] drm/msm: Add SDM845 DPU support
Hello Jeykumar Sankaran, This is a semi-automatic email about new static checker warnings. The patch 25fdd5933e4c: "drm/msm: Add SDM845 DPU support" from Jun 27, 2018, leads to the following Smatch complaint: drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c:422 _dpu_hw_sspp_setup_scaler3() warn: variable dereferenced before check 'ctx->cap->sblk' (see line 421) drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c 420 (void)pe; ^ You should file a bug report with your compiler devs instead of adding these sorts of statements to your code. This function is used as a function pointer so unused parameters are normal. 421 if (_sspp_subblk_offset(ctx, DPU_SSPP_SCALER_QSEED3, &idx) || !sspp The _sspp_subblk_offset() dereferenced "ctx" before it is checked and then it also derefereces "ctx->cap->sblk" without checking. 422 || !scaler3_cfg || !ctx || !ctx->cap || !ctx->cap->sblk) So these will have already crashed before we reach this point. 423 return; 424 regards, dan carpenter
Re: [bug report] drm/msm: Add SDM845 DPU support
On Fri, Oct 01, 2021 at 04:49:12PM +0300, Dan Carpenter wrote: > Hello Jeykumar Sankaran, > > This is a semi-automatic email about new static checker warnings. > > The patch 25fdd5933e4c: "drm/msm: Add SDM845 DPU support" from Jun > 27, 2018, leads to the following Smatch complaint: > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c:422 > _dpu_hw_sspp_setup_scaler3() > warn: variable dereferenced before check 'ctx->cap->sblk' (see line 421) > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >420(void)pe; > ^ > You should file a bug report with your compiler devs instead of adding > these sorts of statements to your code. This function is used as a > function pointer so unused parameters are normal. > >421if (_sspp_subblk_offset(ctx, DPU_SSPP_SCALER_QSEED3, > &idx) || !sspp > > The _sspp_subblk_offset() dereferenced "ctx" before it is checked and > then it also derefereces "ctx->cap->sblk" without checking. > >422|| !scaler3_cfg || !ctx || !ctx->cap || > !ctx->cap->sblk) > > > So these will have already crashed before we reach this point. > Same thing later as well. drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c:591 dpu_hw_sspp_setup_creq_lut() warn: variable dereferenced before check 'ctx->cap' (see line 588) regards, dan carpenter
[bug report] drm/msm/dp: add debugfs support to DP driver
Hello Abhinav Kumar, The patch d11a93690df7: "drm/msm/dp: add debugfs support to DP driver" from Sep 12, 2020, leads to the following Smatch static checker warning: drivers/gpu/drm/msm/dp/dp_debug.c:194 dp_debug_read_info() warn: userbuf overflow? is 'len' <= 'count' drivers/gpu/drm/msm/dp/dp_debug.c 46 static ssize_t dp_debug_read_info(struct file *file, char __user *user_buff, 47 size_t count, loff_t *ppos) 48 { 49 struct dp_debug_private *debug = file->private_data; 50 char *buf; 51 u32 len = 0, rc = 0; 52 u64 lclk = 0; 53 u32 max_size = SZ_4K; 54 u32 link_params_rate; 55 struct drm_display_mode *drm_mode; 56 57 if (!debug) 58 return -ENODEV; 59 60 if (*ppos) 61 return 0; 62 63 buf = kzalloc(SZ_4K, GFP_KERNEL); 64 if (!buf) 65 return -ENOMEM; 66 67 drm_mode = &debug->panel->dp_mode.drm_mode; 68 69 rc = snprintf(buf + len, max_size, "\tname = %s\n", DEBUG_NAME); 70 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 71 goto error; 72 73 rc = snprintf(buf + len, max_size, 74 "\tdp_panel\n\t\tmax_pclk_khz = %d\n", 75 debug->panel->max_pclk_khz); 76 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 77 goto error; 78 79 rc = snprintf(buf + len, max_size, 80 "\tdrm_dp_link\n\t\trate = %u\n", 81 debug->panel->link_info.rate); 82 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 83 goto error; 84 85 rc = snprintf(buf + len, max_size, 86 "\t\tnum_lanes = %u\n", 87 debug->panel->link_info.num_lanes); 88 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 89 goto error; 90 91 rc = snprintf(buf + len, max_size, 92 "\t\tcapabilities = %lu\n", 93 debug->panel->link_info.capabilities); 94 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 95 goto error; 96 97 rc = snprintf(buf + len, max_size, 98 "\tdp_panel_info:\n\t\tactive = %dx%d\n", 99 drm_mode->hdisplay, 100 drm_mode->vdisplay); 101 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 102 goto error; 103 104 rc = snprintf(buf + len, max_size, 105 "\t\tback_porch = %dx%d\n", 106 drm_mode->htotal - drm_mode->hsync_end, 107 drm_mode->vtotal - drm_mode->vsync_end); 108 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 109 goto error; 110 111 rc = snprintf(buf + len, max_size, 112 "\t\tfront_porch = %dx%d\n", 113 drm_mode->hsync_start - drm_mode->hdisplay, 114 drm_mode->vsync_start - drm_mode->vdisplay); 115 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 116 goto error; 117 118 rc = snprintf(buf + len, max_size, 119 "\t\tsync_width = %dx%d\n", 120 drm_mode->hsync_end - drm_mode->hsync_start, 121 drm_mode->vsync_end - drm_mode->vsync_start); 122 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 123 goto error; 124 125 rc = snprintf(buf + len, max_size, 126 "\t\tactive_low = %dx%d\n", 127 debug->panel->dp_mode.h_active_low, 128 debug->panel->dp_mode.v_active_low); 129 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 130 goto error; 131 132 rc = snprintf(buf + len, max_size, 133 "\t\th_skew = %d\n", 134 drm_mode->hskew); 135 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 136 goto error; 137 138 rc = snprintf(buf + len, max_size, 139 "\t\trefresh rate = %d\n", 140 drm_mode_vrefresh(drm_mode)); 141 if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) 142 goto error; 143 144 rc = snprintf(buf + len, max_size, 145 "\t\tpixel clock khz = %d\n", 146
Re: [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers
Hi, On Thu, Aug 26, 2021 at 10:20 PM Stephen Boyd wrote: > > Quoting Bjorn Andersson (2021-08-25 16:42:31) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > > b/drivers/gpu/drm/msm/dp/dp_display.c > > index 2c7de43f655a..4a6132c18e57 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > @@ -78,6 +78,8 @@ struct dp_display_private { > > char *name; > > int irq; > > > > + int id; > > + > > /* state variables */ > > bool core_initialized; > > bool hpd_irq_on; > > @@ -115,8 +117,19 @@ struct dp_display_private { > > struct dp_audio *audio; > > }; > > > > + > > +struct msm_dp_config { > > + phys_addr_t io_start[3]; > > Can this be made into another struct, like msm_dp_desc, that also > indicates what type of DP connector it is, i.e. eDP vs DP? That would > help me understand in modetest and /sys/class/drm what sort of connector > is probing. dp_drm_connector_init() would need to pass the type of > connector appropriately. Right now, eDP connectors still show up as DP > instead of eDP in sysfs. I'm not convinced that's the right way to do it. I think the right way forward here is to look at whether drm_of_find_panel_or_bridge() finds a panel. If it finds a panel then we're eDP. If it doesn't then we're DP. That matches roughly what Laurent was planning to do for ti-sn65dsi86: https://lore.kernel.org/all/20210322030128.2283-11-laurent.pinchart+rene...@ideasonboard.com/ I know technically an eDP and DP controller can have different sets of features but I think what we really are trying to communicate to modetest is whether an internal panel or external display is connected, right? -Doug
Re: [PATCH v13 14/35] drm/tegra: gr3d: Support generic power domain and runtime PM
On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: > > Add runtime power management and support generic power domains. > > Tested-by: Peter Geis # Ouya T30 > Tested-by: Paul Fertser # PAZ00 T20 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko > --- > drivers/gpu/drm/tegra/gr3d.c | 388 ++- [...] > + > +static int gr3d_probe(struct platform_device *pdev) > +{ > + struct host1x_syncpt **syncpts; > + struct gr3d *gr3d; > + unsigned int i; > + int err; > + > + gr3d = devm_kzalloc(&pdev->dev, sizeof(*gr3d), GFP_KERNEL); > + if (!gr3d) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, gr3d); > + > + gr3d->soc = of_device_get_match_data(&pdev->dev); > + > + syncpts = devm_kzalloc(&pdev->dev, sizeof(*syncpts), GFP_KERNEL); > + if (!syncpts) > + return -ENOMEM; > + > + err = gr3d_get_clocks(&pdev->dev, gr3d); > + if (err) > + return err; > + > + err = gr3d_get_resets(&pdev->dev, gr3d); > + if (err) > + return err; > + > + err = gr3d_init_power(&pdev->dev, gr3d); > + if (err) > + return err; > + > INIT_LIST_HEAD(&gr3d->client.base.list); > gr3d->client.base.ops = &gr3d_client_ops; > gr3d->client.base.dev = &pdev->dev; > @@ -352,20 +552,36 @@ static int gr3d_probe(struct platform_device *pdev) > gr3d->client.version = gr3d->soc->version; > gr3d->client.ops = &gr3d_ops; > > + pm_runtime_enable(&pdev->dev); > + pm_runtime_use_autosuspend(&pdev->dev); > + pm_runtime_set_autosuspend_delay(&pdev->dev, 200); > + > + err = devm_pm_opp_register_set_opp_helper(&pdev->dev, gr3d_set_opp); > + if (err) > + goto disable_rpm; > + > + err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > + if (err) > + goto disable_rpm; > + > err = host1x_client_register(&gr3d->client.base); > if (err < 0) { > dev_err(&pdev->dev, "failed to register host1x client: %d\n", > err); > - return err; > + goto disable_rpm; > } > > /* initialize address register map */ > for (i = 0; i < ARRAY_SIZE(gr3d_addr_regs); i++) > set_bit(gr3d_addr_regs[i], gr3d->addr_regs); > > - platform_set_drvdata(pdev, gr3d); > - > return 0; > + > +disable_rpm: > + pm_runtime_dont_use_autosuspend(&pdev->dev); > + pm_runtime_disable(&pdev->dev); Similar comment as for patch13. > + > + return err; > } > > static int gr3d_remove(struct platform_device *pdev) > @@ -380,23 +596,83 @@ static int gr3d_remove(struct platform_device *pdev) > return err; > } > > - if (gr3d->clk_secondary) { > - reset_control_assert(gr3d->rst_secondary); > - tegra_powergate_power_off(TEGRA_POWERGATE_3D1); > - clk_disable_unprepare(gr3d->clk_secondary); > + pm_runtime_dont_use_autosuspend(&pdev->dev); > + pm_runtime_disable(&pdev->dev); Similar comment as for patch13. You may want to use pm_runtime_force_suspend() in favor of pm_runtime_disable(). > + > + return 0; > +} [...] I was looking for a call to dev_pm_opp_set_rate(), but couldn't find it. Isn't that needed when changing the rate of the clock? Kind regards Uffe
Re: [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers
On Fri 01 Oct 06:58 PDT 2021, Doug Anderson wrote: > Hi, > > On Thu, Aug 26, 2021 at 10:20 PM Stephen Boyd wrote: > > > > Quoting Bjorn Andersson (2021-08-25 16:42:31) > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > > > b/drivers/gpu/drm/msm/dp/dp_display.c > > > index 2c7de43f655a..4a6132c18e57 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > > @@ -78,6 +78,8 @@ struct dp_display_private { > > > char *name; > > > int irq; > > > > > > + int id; > > > + > > > /* state variables */ > > > bool core_initialized; > > > bool hpd_irq_on; > > > @@ -115,8 +117,19 @@ struct dp_display_private { > > > struct dp_audio *audio; > > > }; > > > > > > + > > > +struct msm_dp_config { > > > + phys_addr_t io_start[3]; > > > > Can this be made into another struct, like msm_dp_desc, that also > > indicates what type of DP connector it is, i.e. eDP vs DP? That would > > help me understand in modetest and /sys/class/drm what sort of connector > > is probing. dp_drm_connector_init() would need to pass the type of > > connector appropriately. Right now, eDP connectors still show up as DP > > instead of eDP in sysfs. > > I'm not convinced that's the right way to do it. I think the right way > forward here is to look at whether drm_of_find_panel_or_bridge() finds > a panel. If it finds a panel then we're eDP. If it doesn't then we're > DP. That matches roughly what Laurent was planning to do for > ti-sn65dsi86: > > https://lore.kernel.org/all/20210322030128.2283-11-laurent.pinchart+rene...@ideasonboard.com/ > When we spoke about this earlier I got the impression that there was interest in representing the DP display as a panel at some point in the future. Did I misunderstand you or would we simply update the scheme when that day comes? I updated the patch based on Stephen's input and it looks nice, but I could certainly respin it again to simply rely on us having an explicit panel or not. > I know technically an eDP and DP controller can have different sets of > features but I think what we really are trying to communicate to > modetest is whether an internal panel or external display is > connected, right? > For me Stephen's suggestion resulted in the monitor names in Wayland suddenly making more sense, i.e. it makes more sense to say that the lid on my laptop should control eDP-1, rather than DP-3 on this machine... Regards, Bjorn
Re: [PATCH v13 17/35] bus: tegra-gmi: Add runtime PM and OPP support
On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: > > The GMI bus on Tegra belongs to the core power domain and we're going to > enable GENPD support for the core domain. Now GMI must be resumed using > runtime PM API in order to initialize the GMI power state. Add runtime PM > and OPP support to the GMI driver. > > Signed-off-by: Dmitry Osipenko > --- > drivers/bus/tegra-gmi.c | 52 - > 1 file changed, 46 insertions(+), 6 deletions(-) > > diff --git a/drivers/bus/tegra-gmi.c b/drivers/bus/tegra-gmi.c > index a6570789f7af..72ef8a8c236b 100644 > --- a/drivers/bus/tegra-gmi.c > +++ b/drivers/bus/tegra-gmi.c > @@ -13,8 +13,11 @@ > #include > #include > #include > +#include > #include > > +#include > + > #define TEGRA_GMI_CONFIG 0x00 > #define TEGRA_GMI_CONFIG_GOBIT(31) > #define TEGRA_GMI_BUS_WIDTH_32BIT BIT(30) > @@ -54,9 +57,9 @@ static int tegra_gmi_enable(struct tegra_gmi *gmi) > { > int err; > > - err = clk_prepare_enable(gmi->clk); > - if (err < 0) { > - dev_err(gmi->dev, "failed to enable clock: %d\n", err); > + err = pm_runtime_resume_and_get(gmi->dev); > + if (err) { > + pm_runtime_disable(gmi->dev); > return err; > } > > @@ -83,7 +86,8 @@ static void tegra_gmi_disable(struct tegra_gmi *gmi) > writel(config, gmi->base + TEGRA_GMI_CONFIG); > > reset_control_assert(gmi->rst); > - clk_disable_unprepare(gmi->clk); > + > + pm_runtime_put(gmi->dev); > } > > static int tegra_gmi_parse_dt(struct tegra_gmi *gmi) > @@ -213,6 +217,7 @@ static int tegra_gmi_probe(struct platform_device *pdev) > if (!gmi) > return -ENOMEM; > > + platform_set_drvdata(pdev, gmi); > gmi->dev = dev; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ -232,6 +237,14 @@ static int tegra_gmi_probe(struct platform_device *pdev) > return PTR_ERR(gmi->rst); > } > > + err = devm_pm_runtime_enable(gmi->dev); > + if (err) > + return err; > + > + err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > + if (err) > + return err; > + > err = tegra_gmi_parse_dt(gmi); > if (err) > return err; > @@ -247,8 +260,6 @@ static int tegra_gmi_probe(struct platform_device *pdev) > return err; > } > > - platform_set_drvdata(pdev, gmi); > - > return 0; > } > > @@ -262,6 +273,34 @@ static int tegra_gmi_remove(struct platform_device *pdev) Similar comment as for patch13, for the ->remove() callback. This problem, which sometimes also exists in the error path in ->probe() (according to my comments in patch13), seems to be a common issue in the series. I will therefore not continue to repeat my comment on this for the remaining patches in the series, I think I have made my point. :-) Kind regards Uffe
Re: [PATCH] backlight: hx8357: Add SPI device ID table
On Mon, Sep 27, 2021 at 02:30:56PM +0100, Mark Brown wrote: > On Mon, Sep 27, 2021 at 02:24:17PM +0100, Daniel Thompson wrote: > > > In that case what is the point of including unconsumed driver data? Most > > DT centric drivers that included this for modalias reasons leave it > > NULL. > > It's mainly there because it's generated fairly mechanically from the OF > ID table - there's no great reason for it to be there while all > instantiation is done via DT. Ok. If there's no plan to change the driver to pick it up from the table then let's remove the redundant values. They just make it look like somebody forgot something in the probe method (instead of it being a deliberate choice). Daniel.
[bug report] drm/msm: Add SDM845 DPU support
Hello Jeykumar Sankaran, The patch 25fdd5933e4c: "drm/msm: Add SDM845 DPU support" from Jun 27, 2018, leads to the following Smatch static checker warning: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c:359 dpu_encoder_phys_cmd_tearcheck_config() warn: 'vsync_hz' unsigned <= 0 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 322 static void dpu_encoder_phys_cmd_tearcheck_config( 323 struct dpu_encoder_phys *phys_enc) 324 { 325 struct dpu_encoder_phys_cmd *cmd_enc = 326 to_dpu_encoder_phys_cmd(phys_enc); 327 struct dpu_hw_tear_check tc_cfg = { 0 }; 328 struct drm_display_mode *mode; 329 bool tc_enable = true; 330 u32 vsync_hz; 331 struct dpu_kms *dpu_kms; 332 333 if (!phys_enc->hw_pp) { 334 DPU_ERROR("invalid encoder\n"); 335 return; 336 } 337 mode = &phys_enc->cached_mode; 338 339 DPU_DEBUG_CMDENC(cmd_enc, "pp %d\n", phys_enc->hw_pp->idx - PINGPONG_0); 340 341 if (!phys_enc->hw_pp->ops.setup_tearcheck || 342 !phys_enc->hw_pp->ops.enable_tearcheck) { 343 DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n"); 344 return; 345 } 346 347 dpu_kms = phys_enc->dpu_kms; 348 349 /* 350 * TE default: dsi byte clock calculated base on 70 fps; 351 * around 14 ms to complete a kickoff cycle if te disabled; 352 * vclk_line base on 60 fps; write is faster than read; 353 * init == start == rdptr; 354 * 355 * vsync_count is ratio of MDP VSYNC clock frequency to LCD panel 356 * frequency divided by the no. of rows (lines) in the LCDpanel. 357 */ 358 vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync"); --> 359 if (vsync_hz <= 0) { dpu_kms_get_clk_rate() returns -EINVAL (but cast to u64). The "vsync_hz" variable is a u32 so it can't be less than zero and the -EINVAL is treated as a success. 360 DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n", 361 vsync_hz); 362 return; 363 } 364 365 tc_cfg.vsync_count = vsync_hz / 366 (mode->vtotal * drm_mode_vrefresh(mode)); regards, dan carpenter regards, dan carpenter
Re: [PATCH v13 20/35] mtd: rawnand: tegra: Add runtime PM and OPP support
On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: > > The NAND on Tegra belongs to the core power domain and we're going to > enable GENPD support for the core domain. Now NAND must be resumed using > runtime PM API in order to initialize the NAND power state. Add runtime PM > and OPP support to the NAND driver. > > Acked-by: Miquel Raynal > Signed-off-by: Dmitry Osipenko > --- > drivers/mtd/nand/raw/tegra_nand.c | 55 ++- > 1 file changed, 47 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/nand/raw/tegra_nand.c > b/drivers/mtd/nand/raw/tegra_nand.c > index 32431bbe69b8..098fcc9cb9df 100644 > --- a/drivers/mtd/nand/raw/tegra_nand.c > +++ b/drivers/mtd/nand/raw/tegra_nand.c > @@ -17,8 +17,11 @@ > #include > #include > #include > +#include > #include > > +#include > + > #define COMMAND0x00 > #define COMMAND_GO BIT(31) > #define COMMAND_CLE BIT(30) > @@ -1151,6 +1154,7 @@ static int tegra_nand_probe(struct platform_device > *pdev) > return -ENOMEM; > > ctrl->dev = &pdev->dev; > + platform_set_drvdata(pdev, ctrl); > nand_controller_init(&ctrl->controller); > ctrl->controller.ops = &tegra_nand_controller_ops; > > @@ -1166,14 +1170,22 @@ static int tegra_nand_probe(struct platform_device > *pdev) > if (IS_ERR(ctrl->clk)) > return PTR_ERR(ctrl->clk); > > - err = clk_prepare_enable(ctrl->clk); > + err = devm_pm_runtime_enable(&pdev->dev); > + if (err) > + return err; > + > + err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > + if (err) > + return err; > + > + err = pm_runtime_resume_and_get(&pdev->dev); > if (err) > return err; > > err = reset_control_reset(rst); > if (err) { > dev_err(ctrl->dev, "Failed to reset HW: %d\n", err); > - goto err_disable_clk; > + goto err_put_pm; > } > > writel_relaxed(HWSTATUS_CMD_DEFAULT, ctrl->regs + HWSTATUS_CMD); > @@ -1188,21 +1200,19 @@ static int tegra_nand_probe(struct platform_device > *pdev) >dev_name(&pdev->dev), ctrl); > if (err) { > dev_err(ctrl->dev, "Failed to get IRQ: %d\n", err); > - goto err_disable_clk; > + goto err_put_pm; > } > > writel_relaxed(DMA_MST_CTRL_IS_DONE, ctrl->regs + DMA_MST_CTRL); > > err = tegra_nand_chips_init(ctrl->dev, ctrl); > if (err) > - goto err_disable_clk; > - > - platform_set_drvdata(pdev, ctrl); > + goto err_put_pm; > There is no corresponding call pm_runtime_put() here. Is it intentional to always leave the device runtime resumed after ->probe() has succeeded? I noticed you included some comments about this for some other drivers, as those needed more tweaks. Is that also the case for this driver? > return 0; > > -err_disable_clk: > - clk_disable_unprepare(ctrl->clk); > +err_put_pm: > + pm_runtime_put(ctrl->dev); > return err; > } > [...] Kind regards Uffe
[bug report] drm/msm/submit: Move copy_from_user ahead of locking bos
Hello Rob Clark, The patch 20224d715a88: "drm/msm/submit: Move copy_from_user ahead of locking bos" from Oct 23, 2020, leads to the following Smatch static checker warning: drivers/gpu/drm/msm/msm_gem_submit.c:207 submit_lookup_cmds() warn: impossible condition '(sz == (~0)) => (0-u32max == u64max)' drivers/gpu/drm/msm/msm_gem_submit.c 161 static int submit_lookup_cmds(struct msm_gem_submit *submit, 162 struct drm_msm_gem_submit *args, struct drm_file *file) 163 { 164 unsigned i, sz; 165 int ret = 0; 166 167 for (i = 0; i < args->nr_cmds; i++) { 168 struct drm_msm_gem_submit_cmd submit_cmd; 169 void __user *userptr = 170 u64_to_user_ptr(args->cmds + (i * sizeof(submit_cmd))); 171 172 ret = copy_from_user(&submit_cmd, userptr, sizeof(submit_cmd)); 173 if (ret) { 174 ret = -EFAULT; 175 goto out; 176 } 177 178 /* validate input from userspace: */ 179 switch (submit_cmd.type) { 180 case MSM_SUBMIT_CMD_BUF: 181 case MSM_SUBMIT_CMD_IB_TARGET_BUF: 182 case MSM_SUBMIT_CMD_CTX_RESTORE_BUF: 183 break; 184 default: 185 DRM_ERROR("invalid type: %08x\n", submit_cmd.type); 186 return -EINVAL; 187 } 188 189 if (submit_cmd.size % 4) { 190 DRM_ERROR("non-aligned cmdstream buffer size: %u\n", 191 submit_cmd.size); 192 ret = -EINVAL; 193 goto out; 194 } 195 196 submit->cmd[i].type = submit_cmd.type; 197 submit->cmd[i].size = submit_cmd.size / 4; 198 submit->cmd[i].offset = submit_cmd.submit_offset / 4; 199 submit->cmd[i].idx = submit_cmd.submit_idx; 200 submit->cmd[i].nr_relocs = submit_cmd.nr_relocs; 201 202 userptr = u64_to_user_ptr(submit_cmd.relocs); 203 204 sz = array_size(submit_cmd.nr_relocs, 205 sizeof(struct drm_msm_gem_submit_reloc)); 206 /* check for overflow: */ --> 207 if (sz == SIZE_MAX) { "sz" is u32 (not size_t) so this check is impossible on 64 bit systems. May as well just remove it and let the kmalloc() fail. Or use a smaller limit so that users can't trigger the kmalloc() failure. 208 ret = -ENOMEM; 209 goto out; 210 } 211 submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL); 212 ret = copy_from_user(submit->cmd[i].relocs, userptr, sz); 213 if (ret) { 214 ret = -EFAULT; 215 goto out; 216 } 217 } 218 219 out: 220 return ret; 221 } regards, dan carpenter
Re: [PATCH v13 13/35] drm/tegra: gr2d: Support generic power domain and runtime PM
01.10.2021 16:39, Ulf Hansson пишет: > On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: >> >> Add runtime power management and support generic power domains. >> >> Tested-by: Peter Geis # Ouya T30 >> Tested-by: Paul Fertser # PAZ00 T20 >> Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 >> Tested-by: Matt Merhar # Ouya T30 >> Signed-off-by: Dmitry Osipenko >> --- >> drivers/gpu/drm/tegra/gr2d.c | 155 +-- > > [...] > >> static int gr2d_remove(struct platform_device *pdev) >> @@ -259,15 +312,101 @@ static int gr2d_remove(struct platform_device *pdev) >> return err; >> } >> >> + pm_runtime_dont_use_autosuspend(&pdev->dev); >> + pm_runtime_disable(&pdev->dev); > > There is no guarantee that the ->runtime_suspend() has been invoked > here, which means that clock may be left prepared/enabled beyond this > point. > > I suggest you call pm_runtime_force_suspend(), instead of > pm_runtime_disable(), to make sure that gets done. The pm_runtime_disable() performs the final synchronization, please see [1]. [1] https://elixir.bootlin.com/linux/v5.15-rc3/source/drivers/base/power/runtime.c#L1412 Calling pm_runtime_force_suspend() isn't correct because each 'enable' must have the corresponding 'disable'. Hence there is no problem here.
[PATCH v2 1/5] [RFC]iommu: Add a IOMMU_DEVONLY protection flag
The IOMMU_DEVONLY flag allows the caller to flag a mappings backed by device-private buffers. That means other devices or CPUs are not expected to access the physical memory region pointed by the mapping, and the MMU driver can safely restrict the shareability domain to the device itself. Will be used by the ARM MMU driver to flag Mali mappings accessed only by the GPU as Inner-shareable. Signed-off-by: Boris Brezillon --- include/linux/iommu.h | 7 +++ 1 file changed, 7 insertions(+) diff --git a/include/linux/iommu.h b/include/linux/iommu.h index d2f3435e7d17..db14781b522f 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -31,6 +31,13 @@ * if the IOMMU page table format is equivalent. */ #define IOMMU_PRIV (1 << 5) +/* + * Mapping is only accessed by the device behind the iommu. That means other + * devices or CPUs are not expected to access this physical memory region, + * and the MMU driver can safely restrict the shareability domain to the + * device itself. + */ +#define IOMMU_DEVONLY (1 << 6) struct iommu_ops; struct iommu_group; -- 2.31.1
[PATCH v2 0/5] drm/panfrost: Add extra GPU-usage flags
Hello, This is a follow-up of [1], which was adding the read/write restrictions on GPU buffers. Robin and Steven suggested that I add a flag to restrict the shareability domain on GPU-private buffers, so here it is. As you can see, the first patch is flagges RFC, since I'm not sure adding a new IOMMU_ flag is the right solution, but IOMMU_CACHE doesn't feel like a good fit either. Please let me know if you have better ideas. Regards, Boris [1]https://patchwork.kernel.org/project/dri-devel/patch/20210930184723.1482426-1-boris.brezil...@collabora.com/ Boris Brezillon (5): [RFC]iommu: Add a IOMMU_DEVONLY protection flag [RFC]iommu/io-pgtable-arm: Take the DEVONLY flag into account on ARM_MALI_LPAE drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags drm/panfrost: Add a PANFROST_BO_GPUONLY flag drm/panfrost: Bump the driver version to 1.3 drivers/gpu/drm/panfrost/panfrost_drv.c | 15 +-- drivers/gpu/drm/panfrost/panfrost_gem.c | 3 +++ drivers/gpu/drm/panfrost/panfrost_gem.h | 3 +++ drivers/gpu/drm/panfrost/panfrost_mmu.c | 11 ++- drivers/iommu/io-pgtable-arm.c | 25 + include/linux/iommu.h | 7 +++ include/uapi/drm/panfrost_drm.h | 3 +++ 7 files changed, 56 insertions(+), 11 deletions(-) -- 2.31.1
[PATCH v2 2/5] [RFC]iommu/io-pgtable-arm: Take the DEVONLY flag into account on ARM_MALI_LPAE
Restrict the shareability domain when mapping buffers that are GPU-visible only. Signed-off-by: Boris Brezillon --- Flagged RFC because I'm not sure adding a new flag is the right way to convey the 'dev-private buffer' information. --- drivers/iommu/io-pgtable-arm.c | 25 + 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c index dd9e47189d0d..6ac3defb9ae1 100644 --- a/drivers/iommu/io-pgtable-arm.c +++ b/drivers/iommu/io-pgtable-arm.c @@ -450,16 +450,25 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data, << ARM_LPAE_PTE_ATTRINDX_SHIFT); } - /* -* Also Mali has its own notions of shareability wherein its Inner -* domain covers the cores within the GPU, and its Outer domain is -* "outside the GPU" (i.e. either the Inner or System domain in CPU -* terms, depending on coherency). -*/ - if (prot & IOMMU_CACHE && data->iop.fmt != ARM_MALI_LPAE) + if (data->iop.fmt == ARM_MALI_LPAE) { + /* +* Mali has its own notions of shareability wherein its Inner +* domain covers the cores within the GPU, and its Outer domain +* is "outside the GPU" (i.e. either the Inner or System domain +* in CPU terms, depending on coherency). +* If the mapping is only device-visible, we can use the Inner +* domain, otherwise we need to stick to Outer domain +* shareability. +*/ + if (prot & IOMMU_DEVONLY) + pte |= ARM_LPAE_PTE_SH_IS; + else + pte |= ARM_LPAE_PTE_SH_OS; + } else if (prot & IOMMU_CACHE) { pte |= ARM_LPAE_PTE_SH_IS; - else + } else { pte |= ARM_LPAE_PTE_SH_OS; + } if (prot & IOMMU_NOEXEC) pte |= ARM_LPAE_PTE_XN; -- 2.31.1
[PATCH v2 5/5] drm/panfrost: Bump the driver version to 1.3
Bump the driver version to 1.3 to account for the PANFROST_BO_NO{READ,WRITE,GPUONLY} flags addition. Signed-off-by: Boris Brezillon --- drivers/gpu/drm/panfrost/panfrost_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index b176921b9392..5afcec5ab4db 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -530,6 +530,7 @@ DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops); * - 1.0 - initial interface * - 1.1 - adds HEAP and NOEXEC flags for CREATE_BO * - 1.2 - adds AFBC_FEATURES query + * - 1.3 - adds PANFROST_BO_NO{READ,WRITE,GPUONLY} flags */ static const struct drm_driver panfrost_drm_driver = { .driver_features= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ, @@ -542,7 +543,7 @@ static const struct drm_driver panfrost_drm_driver = { .desc = "panfrost DRM", .date = "20180908", .major = 1, - .minor = 2, + .minor = 3, .gem_create_object = panfrost_gem_create_object, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, -- 2.31.1
[PATCH v2 4/5] drm/panfrost: Add a PANFROST_BO_GPUONLY flag
This lets the driver reduce shareability domain of the MMU mapping, which can in turn reduce access time and save power on cache-coherent systems. Signed-off-by: Boris Brezillon --- drivers/gpu/drm/panfrost/panfrost_drv.c | 3 ++- drivers/gpu/drm/panfrost/panfrost_gem.c | 1 + drivers/gpu/drm/panfrost/panfrost_gem.h | 1 + drivers/gpu/drm/panfrost/panfrost_mmu.c | 3 +++ include/uapi/drm/panfrost_drm.h | 1 + 5 files changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index b29ac942ae2d..b176921b9392 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -77,7 +77,8 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct #define PANFROST_BO_FLAGS \ (PANFROST_BO_NOEXEC | PANFROST_BO_HEAP | \ -PANFROST_BO_NOREAD | PANFROST_BO_NOWRITE) +PANFROST_BO_NOREAD | PANFROST_BO_NOWRITE | \ +PANFROST_BO_GPUONLY) static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *file) diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index d6c1bb1445f2..4b1f85c0b98f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -254,6 +254,7 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv, bo->noread = !!(flags & PANFROST_BO_NOREAD); bo->nowrite = !!(flags & PANFROST_BO_NOWRITE); bo->is_heap = !!(flags & PANFROST_BO_HEAP); + bo->gpuonly = !!(flags & PANFROST_BO_GPUONLY); /* * Allocate an id of idr table where the obj is registered diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h index 6246b5fef446..e332d5a4c24f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.h +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h @@ -40,6 +40,7 @@ struct panfrost_gem_object { bool noread :1; bool nowrite:1; bool is_heap:1; + bool gpuonly:1; }; struct panfrost_gem_mapping { diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 6a5c9d94d6f2..89eee8e80aa5 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -321,6 +321,9 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) if (!bo->noread) prot |= IOMMU_READ; + if (bo->gpuonly) + prot |= IOMMU_DEVONLY; + sgt = drm_gem_shmem_get_pages_sgt(obj); if (WARN_ON(IS_ERR(sgt))) return PTR_ERR(sgt); diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h index a2de81225125..538b58b2d095 100644 --- a/include/uapi/drm/panfrost_drm.h +++ b/include/uapi/drm/panfrost_drm.h @@ -88,6 +88,7 @@ struct drm_panfrost_wait_bo { #define PANFROST_BO_HEAP 2 #define PANFROST_BO_NOREAD 4 #define PANFROST_BO_NOWRITE8 +#define PANFROST_BO_GPUONLY0x10 /** * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs. -- 2.31.1