[PATCH v2] drm/fbdev: Clamp fbdev surface size if too large
Clamp the fbdev surface size of the available maximumi height to avoid failing to init console emulation. An example error is shown below. bad framebuffer height 2304, should be >= 768 && <= 768 [drm] Initialized simpledrm 1.0.0 20200625 for simple-framebuffer.0 on minor 0 simple-framebuffer simple-framebuffer.0: [drm] *ERROR* fbdev: Failed to setup generic emulation (ret=-22) This is especially a problem with drivers that have very small screen sizes and cannot over-allocate at all. v2: * reduce warning level (Ville) Signed-off-by: Thomas Zimmermann Fixes: 11e8f5fd223b ("drm: Add simpledrm driver") Reported-by: Amanoel Dawod Reported-by: Zoltán Kővágó Reported-by: Michael Stapelberg Cc: Daniel Vetter Cc: Maxime Ripard Cc: dri-devel@lists.freedesktop.org Cc: # v5.14+ --- drivers/gpu/drm/drm_fb_helper.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 6860223f0068..3b5661cf6c2b 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1508,6 +1508,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, { struct drm_client_dev *client = &fb_helper->client; struct drm_device *dev = fb_helper->dev; + struct drm_mode_config *config = &dev->mode_config; int ret = 0; int crtc_count = 0; struct drm_connector_list_iter conn_iter; @@ -1665,6 +1666,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, /* Handle our overallocation */ sizes.surface_height *= drm_fbdev_overalloc; sizes.surface_height /= 100; + if (sizes.surface_height > config->max_height) { + drm_dbg_kms(dev, "Fbdev over-allocation too large; clamping height to %d\n", + config->max_height); + sizes.surface_height = config->max_height; + } /* push down into drivers */ ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); -- 2.33.0
Re: [PATCH 25/33] drm/i915/guc: Support request cancellation
On 2021-07-27 12:15:59 [-0700], Daniele Ceraolo Spurio wrote: > On 7/26/2021 5:23 PM, Matthew Brost wrote: > > This adds GuC backend support for i915_request_cancel(), which in turn > > makes CONFIG_DRM_I915_REQUEST_TIMEOUT work. > > > Reviewed-by: Daniele Ceraolo Spurio I have a few instances of ODEBUG warnings since this commit 62eaf0ae217d4 ("drm/i915/guc: Support request cancellation") like: | [ cut here ] | ODEBUG: init destroyed (active state 0) object type: i915_sw_fence hint: sw_fence_dummy_notify+0x0/0x10 | WARNING: CPU: 0 PID: 987 at lib/debugobjects.c:505 debug_print_object+0x6e/0x90 | Modules linked in: | CPU: 0 PID: 987 Comm: Xorg Not tainted 5.15.0-rc4+ #67 | Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.10 04/24/2012 | RIP: 0010:debug_print_object+0x6e/0x90 … | Call Trace: | i915_sw_fence_reinit+0x10/0x40 | intel_context_init+0x185/0x1e0 | intel_context_create+0x2e/0x100 | default_engines+0x9d/0x120 | i915_gem_create_context+0x40a/0x5d0 | ? trace_kmalloc+0x29/0xd0 | ? kmem_cache_alloc_trace+0xdd/0x190 | i915_gem_context_open+0x140/0x1c0 | i915_gem_open+0x70/0xa0 | drm_file_alloc+0x1af/0x270 | drm_open+0xdc/0x270 | drm_stub_open+0xa6/0x130 | chrdev_open+0xbe/0x250 | ? cdev_device_add+0x80/0x80 | do_dentry_open+0x15e/0x390 | path_openat+0x76b/0xa60 | do_filp_open+0xa4/0x150 | ? lock_release+0x149/0x2f0 | ? _raw_spin_unlock+0x24/0x40 | do_sys_openat2+0x92/0x160 | __x64_sys_openat+0x4f/0x90 | do_syscall_64+0x3b/0xc0 | entry_SYSCALL_64_after_hwframe+0x44/0xae | RIP: 0033:0x7f91b5cfdf07 and: | ODEBUG: activate destroyed (active state 0) object type: i915_sw_fence hint: sw_fence_dummy_notify+0x0/0x10 | WARNING: CPU: 0 PID: 987 at lib/debugobjects.c:505 debug_print_object+0x6e/0x90 | | Call Trace: | debug_object_activate+0x174/0x200 | i915_sw_fence_commit+0x10/0x20 | intel_context_init+0x18d/0x1e0 | intel_context_create+0x2e/0x100 | default_engines+0x9d/0x120 --- | ODEBUG: active_state destroyed (active state 0) object type: i915_sw_fence hint: sw_fence_dummy_notify+0x0/0x10 | WARNING: CPU: 0 PID: 987 at lib/debugobjects.c:505 debug_print_object+0x6e/0x90 | Call Trace: | __i915_sw_fence_complete+0x6f/0x280 | intel_context_init+0x18d/0x1e0 | intel_context_create+0x2e/0x100 | default_engines+0x9d/0x120 Is this known? This is yesterday's -rc4, I first noticed it in -rc3. > Daniele Sebastian
Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE
On Tue, Oct 05, 2021 at 09:19:39AM +0300, Dan Carpenter wrote: On Mon, Oct 04, 2021 at 01:52:27PM -0700, Lucas De Marchi wrote: Cc'ing Dan Carpenter On Fri, Oct 01, 2021 at 12:57:13PM +0300, Jani Nikula wrote: > 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. I would really prefer that instead of ensuring that code doesn't generate Smatch warnings, people just look over the warnings and then mass mark them all as false positives and never look at them again. It let's us warn about more complicated things without worrying so much about being perfect. When code is fresh in your head then warnings are not a big deal to review and you want to warn about every possible issue After a year then they take forever and so you really want them to be correct or it's a huge waste of time. I'd prefer Smatch live in the space where people run it when the code is fresh. You would have received some automated emails about this Smatch warning but I look over the zero day output and filter the results. clang gives a warning only in drivers/gpu/drm/i915/i915_config.c and the warning is gone if the condition swapped: - if (context && CONFIG_DRM_I915_FENCE_TIMEOUT) + if (CONFIG_DRM_I915_FENCE_TIMEOUT && context) I like this rule that when the constant is on the left it's not a mask. That makes sense. I will add that. thanks, that would be great, so we can really get rid of the macro by sticking this rule since it works for smatch and clang (and gcc doesn't give this warning). thanks Lucas De Marchi which would make sense if we think about shortcutting the if condition. However smatch still reports the warning and an additional one in drivers/gpu/drm/i915/i915_request.c. The ways I found to stop the false positives with smatch are: if (context && CONFIG_DRM_I915_FENCE_TIMEOUT != 0) or if (context && !!CONFIG_DRM_I915_FENCE_TIMEOUT) or if (context && CONFIG_DRM_I915_FENCE_TIMEOUT > 0) I guess I prefer the first and third but I'll add the rule that Clang uses to silence the warning. regards, dan carpenter
Re: [PATCH v3 6/6] drm/mediatek: Add mt8195 DisplayPort driver
Hi Chun-Kuang, On Sat, Oct 02, 2021 at 12:16:26AM +0800, Chun-Kuang Hu wrote: > Hi, Markus: > [...] > > > > 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 ++ > > Separate the phy driver to another patch because phy driver would go > into different maintainer's tree. Oh of course. Thank you. I fixed it for the next version. Best, Markus
Re: refactor the i915 GVT support
Hi folks: It seems we haven't reached a possible solution of this refactor patch series. The current patch series needs to be re-worked because of the module/symbol dependency(The root cause has been discussed in another email). I have to get them off from our gvt-next repo so that we can continue our development and pull-request to upstream. Thanks so much for the patch and the discussion. Thanks, Zhi. On 10/1/21 1:01 PM, Wang, Zhi A wrote: > 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 08/28] dma-buf: use the new iterator in dma_buf_debug_show
On 01/10/2021 11:05, Christian König wrote: 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"); Reviewed-by: Tvrtko Ursulin Regards, Tvrtko
Re: [PATCH] drm/connector: refer to CTA-861-G in the "content type" prop docs
On Mon, 04 Oct 2021 09:12:50 + Simon Ser wrote: > The KMS documentation doesn't say much about the meaning of each > content type. Add a reference to the specification defining them. > > Signed-off-by: Simon Ser > Cc: Emmanuel Gil Peyrot > Cc: Daniel Vetter > Cc: Pekka Paalanen > Cc: Ville Syrjala > Cc: Jani Nikula > --- > drivers/gpu/drm/drm_connector.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 3bc782b630b9..79d8163686cd 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1397,6 +1397,8 @@ > EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); > * Game: > * Content type is game > * > + * The meaning of each content type is defined in CTA-861-G table 15. > + * > * Drivers can set up this property by calling > * drm_connector_attach_content_type_property(). Decoding to > * infoframe values is done through drm_hdmi_avi_infoframe_content_type(). Reviewed-by: Pekka Paalanen Thanks! pq pgpIKQ0nLFKvR.pgp Description: OpenPGP digital signature
Re: [PATCH 09/28] dma-buf: use the new iterator in dma_resv_poll
On 01/10/2021 11:05, Christian König wrote: 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) It is unchanged with this patch, but are the semantics supposed to be like this? Signal poll event if _any_ of the shared fences has been signaled? Regards, Tvrtko @@ -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
Re: [PATCH 23/28] drm: use new iterator in drm_gem_fence_array_add_implicit v3
On 01/10/2021 11:06, Christian König wrote: 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); Reviewed-by: Tvrtko Ursulin Regards, Tvrtko
Re: [PATCH v2] drm/fbdev: Clamp fbdev surface size if too large
On Tue, Oct 05, 2021 at 09:03:55AM +0200, Thomas Zimmermann wrote: > Clamp the fbdev surface size of the available maximumi height to avoid > failing to init console emulation. An example error is shown below. > > bad framebuffer height 2304, should be >= 768 && <= 768 > [drm] Initialized simpledrm 1.0.0 20200625 for simple-framebuffer.0 on > minor 0 > simple-framebuffer simple-framebuffer.0: [drm] *ERROR* fbdev: Failed to > setup generic emulation (ret=-22) > > This is especially a problem with drivers that have very small screen > sizes and cannot over-allocate at all. > > v2: > * reduce warning level (Ville) > > Signed-off-by: Thomas Zimmermann > Fixes: 11e8f5fd223b ("drm: Add simpledrm driver") > Reported-by: Amanoel Dawod > Reported-by: Zoltán Kővágó > Reported-by: Michael Stapelberg > Cc: Daniel Vetter > Cc: Maxime Ripard > Cc: dri-devel@lists.freedesktop.org > Cc: # v5.14+ Looks sane. Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/drm_fb_helper.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 6860223f0068..3b5661cf6c2b 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1508,6 +1508,7 @@ static int drm_fb_helper_single_fb_probe(struct > drm_fb_helper *fb_helper, > { > struct drm_client_dev *client = &fb_helper->client; > struct drm_device *dev = fb_helper->dev; > + struct drm_mode_config *config = &dev->mode_config; > int ret = 0; > int crtc_count = 0; > struct drm_connector_list_iter conn_iter; > @@ -1665,6 +1666,11 @@ static int drm_fb_helper_single_fb_probe(struct > drm_fb_helper *fb_helper, > /* Handle our overallocation */ > sizes.surface_height *= drm_fbdev_overalloc; > sizes.surface_height /= 100; > + if (sizes.surface_height > config->max_height) { > + drm_dbg_kms(dev, "Fbdev over-allocation too large; clamping > height to %d\n", > + config->max_height); > + sizes.surface_height = config->max_height; > + } > > /* push down into drivers */ > ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); > -- > 2.33.0 -- Ville Syrjälä Intel
Re: [PATCH 24/28] drm: use new iterator in drm_gem_plane_helper_prepare_fb
On 01/10/2021 11:06, Christian König wrote: 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 */ What is the TODO? NB instead? + drm_atomic_set_fence_for_plane(state, fence); + return 0; + } + dma_resv_iter_end(&cursor); + drm_atomic_set_fence_for_plane(state, NULL); dma_resv_iter_begin(&cursor, obj->resv, false); dma_resv_for_each_fence_unlocked(&cursor, fence) { dma_fence_get(fence); break; } dma_resv_iter_end(&cursor); drm_atomic_set_fence_for_plane(state, fence); Does this work? But overall I am not sure this is nicer. Is the goal to remove dma_resv_get_excl_unlocked as API it just does not happen in this series? Regards, Tvrtko return 0; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
Re: [Intel-gfx] [PATCH 12/26] drm/i915/guc: Implement multi-lrc submission
Hi Matthew, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on drm-tip/drm-tip] [cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master airlied/drm-next 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/Matthew-Brost/Parallel-submission-aka-multi-bb-execbuf/20211005-061424 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: i386-randconfig-a004-20211004 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c0039de2953d15815448b4b3c3bafb45607781e0) 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/3bea3cc438df1105d0d8c1bcc01b96559d4bb78c git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Matthew-Brost/Parallel-submission-aka-multi-bb-execbuf/20211005-061424 git checkout 3bea3cc438df1105d0d8c1bcc01b96559d4bb78c # 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/gt/uc/intel_guc_submission.c:1486:7: warning: variable >> 'ret' is used uninitialized whenever 'if' condition is false >> [-Wsometimes-uninitialized] if (multi_lrc_submit(rq)) { ^~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1499:9: note: uninitialized use occurs here return ret; ^~~ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1486:3: note: remove the 'if' if its condition is always true if (multi_lrc_submit(rq)) { ^~ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1479:9: note: initialize the variable 'ret' to silence this warning int ret; ^ = 0 1 warning generated. vim +1486 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 1475 1476 static int guc_bypass_tasklet_submit(struct intel_guc *guc, 1477 struct i915_request *rq) 1478 { 1479 int ret; 1480 1481 __i915_request_submit(rq); 1482 1483 trace_i915_request_in(rq, 0); 1484 1485 if (is_multi_lrc_rq(rq)) { > 1486 if (multi_lrc_submit(rq)) { 1487 ret = guc_wq_item_append(guc, rq); 1488 if (!ret) 1489 ret = guc_add_request(guc, rq); 1490 } 1491 } else { 1492 guc_set_lrc_tail(rq); 1493 ret = guc_add_request(guc, rq); 1494 } 1495 1496 if (unlikely(ret == -EPIPE)) 1497 disable_submission(guc); 1498 1499 return ret; 1500 } 1501 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
linux-next: build warning after merge of the drm-misc tree
Hi all, After merging the drm-misc tree, today's linux-next build (htmldocs) produced this warning: include/linux/dma-buf.h:456: warning: Function parameter or member 'cb_in' not described in 'dma_buf' include/linux/dma-buf.h:456: warning: Function parameter or member 'cb_out' not described in 'dma_buf' Introduced by commit 6b51b02a3a0a ("dma-buf: fix and rework dma_buf_poll v7") -- Cheers, Stephen Rothwell pgpQ8loPRVsHO.pgp Description: OpenPGP digital signature
[PATCH 1/2] drm/dp: add drm_dp_phy_name() for getting DP PHY name
Add a helper for getting the DP PHY name. In the interest of caller simplicity and to avoid allocations and passing in of buffers, duplicate the const strings to return. It's a minor penalty to pay for simplicity in all the call sites. Cc: Ville Syrjälä Signed-off-by: Jani Nikula --- drivers/gpu/drm/drm_dp_helper.c | 21 + include/drm/drm_dp_helper.h | 2 ++ 2 files changed, 23 insertions(+) diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 4d0d1e8e51fa..f1d03b5a4bab 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -197,6 +197,27 @@ void drm_dp_link_train_channel_eq_delay(const struct drm_dp_aux *aux, } EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); +const char *drm_dp_phy_name(enum drm_dp_phy dp_phy) +{ + static const char * const phy_names[] = { + [DP_PHY_DPRX] = "DPRX", + [DP_PHY_LTTPR1] = "LTTPR 1", + [DP_PHY_LTTPR2] = "LTTPR 2", + [DP_PHY_LTTPR3] = "LTTPR 3", + [DP_PHY_LTTPR4] = "LTTPR 4", + [DP_PHY_LTTPR5] = "LTTPR 5", + [DP_PHY_LTTPR6] = "LTTPR 6", + [DP_PHY_LTTPR7] = "LTTPR 7", + [DP_PHY_LTTPR8] = "LTTPR 8", + }; + + if (dp_phy < 0 || dp_phy >= ARRAY_SIZE(phy_names)) + return ""; + + return phy_names[dp_phy]; +} +EXPORT_SYMBOL(drm_dp_phy_name); + void drm_dp_lttpr_link_train_clock_recovery_delay(void) { usleep_range(100, 200); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index b52df4db3e8f..c873e6349b41 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -2115,6 +2115,8 @@ bool drm_dp_read_sink_count_cap(struct drm_connector *connector, const struct drm_dp_desc *desc); int drm_dp_read_sink_count(struct drm_dp_aux *aux); +const char *drm_dp_phy_name(enum drm_dp_phy dp_phy); + int drm_dp_read_lttpr_common_caps(struct drm_dp_aux *aux, u8 caps[DP_LTTPR_COMMON_CAP_SIZE]); int drm_dp_read_lttpr_phy_caps(struct drm_dp_aux *aux, -- 2.30.2
[PATCH 2/2] drm/i915/dp: use drm_dp_phy_name() for logging
Drop the local intel_dp_phy_name() function, and replace with drm_dp_phy_name(). This lets us drop a number of local buffers. Cc: Ville Syrjälä Signed-off-by: Jani Nikula --- .../drm/i915/display/intel_dp_link_training.c | 24 --- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c index c052ce14894d..219568304c08 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -46,17 +46,6 @@ static void intel_dp_reset_lttpr_count(struct intel_dp *intel_dp) DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV] = 0; } -static const char *intel_dp_phy_name(enum drm_dp_phy dp_phy, -char *buf, size_t buf_size) -{ - if (dp_phy == DP_PHY_DPRX) - snprintf(buf, buf_size, "DPRX"); - else - snprintf(buf, buf_size, "LTTPR %d", dp_phy - DP_PHY_LTTPR1 + 1); - - return buf; -} - static u8 *intel_dp_lttpr_phy_caps(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy) { @@ -67,20 +56,17 @@ static void intel_dp_read_lttpr_phy_caps(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy) { u8 *phy_caps = intel_dp_lttpr_phy_caps(intel_dp, dp_phy); - char phy_name[10]; - - intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name)); if (drm_dp_read_lttpr_phy_caps(&intel_dp->aux, dp_phy, phy_caps) < 0) { drm_dbg_kms(&dp_to_i915(intel_dp)->drm, "failed to read the PHY caps for %s\n", - phy_name); + drm_dp_phy_name(dp_phy)); return; } drm_dbg_kms(&dp_to_i915(intel_dp)->drm, "%s PHY capabilities: %*ph\n", - phy_name, + drm_dp_phy_name(dp_phy), (int)sizeof(intel_dp->lttpr_phy_caps[0]), phy_caps); } @@ -443,7 +429,6 @@ void intel_dp_set_signal_levels(struct intel_dp *intel_dp, { struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); - char phy_name[10]; drm_dbg_kms(&dev_priv->drm, "[ENCODER:%d:%s] lanes: %d, " "vswing levels: " TRAIN_SET_FMT ", " @@ -452,7 +437,7 @@ void intel_dp_set_signal_levels(struct intel_dp *intel_dp, crtc_state->lane_count, TRAIN_SET_VSWING_ARGS(intel_dp->train_set), TRAIN_SET_PREEMPH_ARGS(intel_dp->train_set), - intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name))); + drm_dp_phy_name(dp_phy)); if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy)) encoder->set_signal_levels(encoder, crtc_state); @@ -849,7 +834,6 @@ intel_dp_link_train_phy(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy) { struct intel_connector *intel_connector = intel_dp->attached_connector; - char phy_name[10]; bool ret = false; if (!intel_dp_link_training_clock_recovery(intel_dp, crtc_state, dp_phy)) @@ -867,7 +851,7 @@ intel_dp_link_train_phy(struct intel_dp *intel_dp, intel_connector->base.name, ret ? "passed" : "failed", crtc_state->port_clock, crtc_state->lane_count, - intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name))); + drm_dp_phy_name(dp_phy)); return ret; } -- 2.30.2
[PATCH v2 00/10] drm/gma500: Refactor GEM code
Bring GEM code up to current standards and untangle the connection to GTT helpers. The allocation and pinning helpers for struct gtt_range are located in the GTT code, but actually part of the GEM implementation. The patchset moves them to GEM code and refactors much of the implementation. Most of the GTT code is then independend from the struct gtt_range, while the GEM code does not contain GTT management. In addition to internal refiactoring, patches 2 to 4 update the rest of the driver to use the GEM interfaces for object allocation and release. Finally, rename struct gtt_range to struct psb_gem_object to designate it as a 'real' GEM object. Future work: with the GEM and GTT code separated, future patchsets can implement on-demand release of GTT entries, or remove the perma-mapping of stolen memory. Dma-buf support might also be added. Tested on Atom N2800 hardware. v2: * keep docs about GTT locking * release mutex in pin error path Thomas Zimmermann (10): drm/gma500: Move helpers for struct gtt_range from gtt.c to gem.c drm/gma500: Use to_gtt_range() everywhere drm/gma500: Reimplement psb_gem_create() drm/gma500: Allocate GTT ranges in stolen memory with psb_gem_create() drm/gma500: Rename psb_gtt_{pin,unpin}() to psb_gem_{pin,unpin}() drm/gma500: Inline psb_gtt_attach_pages() and psb_gtt_detach_pages() drm/gma500: Inline psb_gtt_{alloc,free}_range() into rsp callers drm/gma500: Set page-caching flags in GEM pin/unpin drm/gma500: Rewrite GTT page insert/remove without struct gtt_range drm/gma500: Rename struct gtt_range to struct psb_gem_object drivers/gpu/drm/gma500/framebuffer.c | 52 +--- drivers/gpu/drm/gma500/gem.c | 234 +++ drivers/gpu/drm/gma500/gem.h | 28 +- drivers/gpu/drm/gma500/gma_display.c | 51 ++-- drivers/gpu/drm/gma500/gtt.c | 320 - drivers/gpu/drm/gma500/gtt.h | 29 +- drivers/gpu/drm/gma500/oaktrail_crtc.c | 3 +- drivers/gpu/drm/gma500/psb_intel_display.c | 17 +- drivers/gpu/drm/gma500/psb_intel_drv.h | 2 +- 9 files changed, 317 insertions(+), 419 deletions(-) -- 2.33.0
[PATCH v2 02/10] drm/gma500: Use to_gtt_range() everywhere
Convert upcasts from struct drm_gem_object to struct gtt_range to to_gtt_range(). Some places used container_of() directly. Signed-off-by: Thomas Zimmermann Acked-by: Patrik Jakobsson --- drivers/gpu/drm/gma500/gem.c | 4 ++-- drivers/gpu/drm/gma500/gma_display.c | 7 +++ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index a41990dbcc0e..b52908313ee5 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -117,7 +117,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf); static void psb_gem_free_object(struct drm_gem_object *obj) { - struct gtt_range *gtt = container_of(obj, struct gtt_range, gem); + struct gtt_range *gtt = to_gtt_range(obj); /* Remove the list map if one is present */ drm_gem_free_mmap_offset(obj); @@ -267,7 +267,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf) dev = obj->dev; dev_priv = to_drm_psb_private(dev); - r = container_of(obj, struct gtt_range, gem); /* Get the gtt range */ + r = to_gtt_range(obj); /* Make sure we don't parallel update on a fault, nor move or remove something from beneath our feet */ diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index ecf8153416ac..8285358fac01 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -349,8 +349,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, /* Unpin the old GEM object */ if (gma_crtc->cursor_obj) { - gt = container_of(gma_crtc->cursor_obj, - struct gtt_range, gem); + gt = to_gtt_range(gma_crtc->cursor_obj); psb_gtt_unpin(gt); drm_gem_object_put(gma_crtc->cursor_obj); gma_crtc->cursor_obj = NULL; @@ -376,7 +375,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, goto unref_cursor; } - gt = container_of(obj, struct gtt_range, gem); + gt = to_gtt_range(obj); /* Pin the memory into the GTT */ ret = psb_gtt_pin(gt); @@ -426,7 +425,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, /* unpin the old bo */ if (gma_crtc->cursor_obj) { - gt = container_of(gma_crtc->cursor_obj, struct gtt_range, gem); + gt = to_gtt_range(gma_crtc->cursor_obj); psb_gtt_unpin(gt); drm_gem_object_put(gma_crtc->cursor_obj); } -- 2.33.0
[PATCH v2 01/10] drm/gma500: Move helpers for struct gtt_range from gtt.c to gem.c
Allocation and pinning helpers for struct gtt_range are GEM functions, so move them to gem.c. No functional changes. v2: * keep docs for psb_gtt_{attach,detach}_pages() (Patrik) Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/framebuffer.c | 1 - drivers/gpu/drm/gma500/gem.c | 144 ++-- drivers/gpu/drm/gma500/gem.h | 8 + drivers/gpu/drm/gma500/gma_display.c | 1 + drivers/gpu/drm/gma500/gtt.c | 190 + drivers/gpu/drm/gma500/gtt.h | 11 +- drivers/gpu/drm/gma500/psb_intel_display.c | 1 + 7 files changed, 147 insertions(+), 209 deletions(-) diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 321e416489a9..ce92d11bd20f 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -25,7 +25,6 @@ #include "framebuffer.h" #include "gem.h" -#include "gtt.h" #include "psb_drv.h" #include "psb_intel_drv.h" #include "psb_intel_reg.h" diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 5ae54c9d2819..a41990dbcc0e 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -19,6 +19,100 @@ #include "gem.h" #include "psb_drv.h" +/* + * Pin and build an in-kernel list of the pages that back our GEM object. + * While we hold this the pages cannot be swapped out. This is protected + * via the gtt mutex which the caller must hold. + */ +static int psb_gtt_attach_pages(struct gtt_range *gt) +{ + struct page **pages; + + WARN_ON(gt->pages); + + pages = drm_gem_get_pages(>->gem); + if (IS_ERR(pages)) + return PTR_ERR(pages); + + gt->npage = gt->gem.size / PAGE_SIZE; + gt->pages = pages; + + return 0; +} + +/* + * Undo the effect of psb_gtt_attach_pages. At this point the pages + * must have been removed from the GTT as they could now be paged out + * and move bus address. This is protected via the gtt mutex which the + * caller must hold. + */ +static void psb_gtt_detach_pages(struct gtt_range *gt) +{ + drm_gem_put_pages(>->gem, gt->pages, true, false); + gt->pages = NULL; +} + +int psb_gtt_pin(struct gtt_range *gt) +{ + int ret = 0; + struct drm_device *dev = gt->gem.dev; + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + u32 gpu_base = dev_priv->gtt.gatt_start; + + mutex_lock(&dev_priv->gtt_mutex); + + if (gt->in_gart == 0 && gt->stolen == 0) { + ret = psb_gtt_attach_pages(gt); + if (ret < 0) + goto out; + ret = psb_gtt_insert(dev, gt, 0); + if (ret < 0) { + psb_gtt_detach_pages(gt); + goto out; + } + psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), +gt->pages, (gpu_base + gt->offset), +gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY); + } + gt->in_gart++; +out: + mutex_unlock(&dev_priv->gtt_mutex); + return ret; +} + +void psb_gtt_unpin(struct gtt_range *gt) +{ + struct drm_device *dev = gt->gem.dev; + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + u32 gpu_base = dev_priv->gtt.gatt_start; + + mutex_lock(&dev_priv->gtt_mutex); + + WARN_ON(!gt->in_gart); + + gt->in_gart--; + if (gt->in_gart == 0 && gt->stolen == 0) { + psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu), +(gpu_base + gt->offset), gt->npage, 0, 0); + psb_gtt_remove(dev, gt); + psb_gtt_detach_pages(gt); + } + + mutex_unlock(&dev_priv->gtt_mutex); +} + +void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) +{ + /* Undo the mmap pin if we are destroying the object */ + if (gt->mmapping) { + psb_gtt_unpin(gt); + gt->mmapping = 0; + } + WARN_ON(gt->in_gart && !gt->stolen); + release_resource(>->resource); + kfree(gt); +} + static vm_fault_t psb_gem_fault(struct vm_fault *vmf); static void psb_gem_free_object(struct drm_gem_object *obj) @@ -44,19 +138,43 @@ const struct drm_gem_object_funcs psb_gem_object_funcs = { .vm_ops = &psb_gem_vm_ops, }; -/** - * psb_gem_create - create a mappable object - * @file: the DRM file of the client - * @dev: our device - * @size: the size requested - * @handlep: returned handle (opaque number) - * @stolen: unused - * @align: unused - * - * Create a GEM object, fill in the boilerplate and attach a handle to - * it so that userspace can speak about it. This does the core work - * for the various methods that do/will create GEM objects for things - */ +struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev,
[PATCH v2 05/10] drm/gma500: Rename psb_gtt_{pin, unpin}() to psb_gem_{pin, unpin}()
Rename psb_gtt_pin() to psb_gem_pin() to reflect the semantics of the function. Same for psb_gtt_unpin(). No functional changes. Signed-off-by: Thomas Zimmermann Acked-by: Patrik Jakobsson --- drivers/gpu/drm/gma500/gem.c | 8 drivers/gpu/drm/gma500/gem.h | 4 ++-- drivers/gpu/drm/gma500/gma_display.c | 12 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 97b7f3bdbdc3..1905468924ca 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -52,7 +52,7 @@ static void psb_gtt_detach_pages(struct gtt_range *gt) gt->pages = NULL; } -int psb_gtt_pin(struct gtt_range *gt) +int psb_gem_pin(struct gtt_range *gt) { int ret = 0; struct drm_device *dev = gt->gem.dev; @@ -80,7 +80,7 @@ int psb_gtt_pin(struct gtt_range *gt) return ret; } -void psb_gtt_unpin(struct gtt_range *gt) +void psb_gem_unpin(struct gtt_range *gt) { struct drm_device *dev = gt->gem.dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); @@ -105,7 +105,7 @@ static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) { /* Undo the mmap pin if we are destroying the object */ if (gt->mmapping) { - psb_gtt_unpin(gt); + psb_gem_unpin(gt); gt->mmapping = 0; } WARN_ON(gt->in_gart && !gt->stolen); @@ -301,7 +301,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf) /* For now the mmap pins the object and it stays pinned. As things stand that will do us no harm */ if (r->mmapping == 0) { - err = psb_gtt_pin(r); + err = psb_gem_pin(r); if (err < 0) { dev_err(dev->dev, "gma500: pin failed: %d\n", err); ret = vmf_error(err); diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h index 6b67c58cbed5..21c86df482a6 100644 --- a/drivers/gpu/drm/gma500/gem.h +++ b/drivers/gpu/drm/gma500/gem.h @@ -15,7 +15,7 @@ struct drm_device; struct gtt_range * psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align); -int psb_gtt_pin(struct gtt_range *gt); -void psb_gtt_unpin(struct gtt_range *gt); +int psb_gem_pin(struct gtt_range *gt); +void psb_gem_unpin(struct gtt_range *gt); #endif diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index 8c95b50034a5..6d0470b27bc5 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -75,7 +75,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y, /* We are displaying this buffer, make sure it is actually loaded into the GTT */ - ret = psb_gtt_pin(gtt); + ret = psb_gem_pin(gtt); if (ret < 0) goto gma_pipe_set_base_exit; start = gtt->offset; @@ -126,7 +126,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y, gma_pipe_cleaner: /* If there was a previous display we can now unpin it */ if (old_fb) - psb_gtt_unpin(to_gtt_range(old_fb->obj[0])); + psb_gem_unpin(to_gtt_range(old_fb->obj[0])); gma_pipe_set_base_exit: gma_power_end(dev); @@ -350,7 +350,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, /* Unpin the old GEM object */ if (gma_crtc->cursor_obj) { gt = to_gtt_range(gma_crtc->cursor_obj); - psb_gtt_unpin(gt); + psb_gem_unpin(gt); drm_gem_object_put(gma_crtc->cursor_obj); gma_crtc->cursor_obj = NULL; } @@ -378,7 +378,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, gt = to_gtt_range(obj); /* Pin the memory into the GTT */ - ret = psb_gtt_pin(gt); + ret = psb_gem_pin(gt); if (ret) { dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle); goto unref_cursor; @@ -426,7 +426,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, /* unpin the old bo */ if (gma_crtc->cursor_obj) { gt = to_gtt_range(gma_crtc->cursor_obj); - psb_gtt_unpin(gt); + psb_gem_unpin(gt); drm_gem_object_put(gma_crtc->cursor_obj); } @@ -490,7 +490,7 @@ void gma_crtc_disable(struct drm_crtc *crtc) if (crtc->primary->fb) { gt = to_gtt_range(crtc->primary->fb->obj[0]); - psb_gtt_unpin(gt); + psb_gem_unpin(gt); } } -- 2.33.0
[PATCH v2 04/10] drm/gma500: Allocate GTT ranges in stolen memory with psb_gem_create()
Support private objects for stolen memory in psb_gem_create() and convert users to psb_gem_create(). For stolen memory, psb_gem_create() now initializes the GEM object via drm_gem_private_object_init(). In the fbdev setup, replace the open-coded initialization of struct gtt_range with a call to psb_gem_create(). Use drm_gem_object_put() for release. In the cursor setup, use psb_gem_create() and get a real GEM object. Previously the allocated instance of struct gtt_range was only partially initialized. Release the cursor GEM object in gma_crtc_destroy(). The release was missing from the original code. With the conversion of all callers to psb_gem_create(), the extern declarations of psb_gtt_alloc_range, psb_gtt_free_range and psb_gem_object_func are not required any longer. Declare them as static inline. Signed-off-by: Thomas Zimmermann Acked-by: Patrik Jakobsson --- drivers/gpu/drm/gma500/framebuffer.c | 44 ++ drivers/gpu/drm/gma500/gem.c | 22 ++- drivers/gpu/drm/gma500/gem.h | 5 --- drivers/gpu/drm/gma500/gma_display.c | 3 ++ drivers/gpu/drm/gma500/psb_intel_display.c | 5 +-- 5 files changed, 29 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index ce92d11bd20f..3ea6679ccd38 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -224,31 +224,6 @@ static struct drm_framebuffer *psb_framebuffer_create return fb; } -/** - * psbfb_alloc - allocate frame buffer memory - * @dev: the DRM device - * @aligned_size: space needed - * - * Allocate the frame buffer. In the usual case we get a GTT range that - * is stolen memory backed and life is simple. If there isn't sufficient - * we fail as we don't have the virtual mapping space to really vmap it - * and the kernel console code can't handle non linear framebuffers. - * - * Re-address this as and if the framebuffer layer grows this ability. - */ -static struct gtt_range *psbfb_alloc(struct drm_device *dev, int aligned_size) -{ - struct gtt_range *backing; - /* Begin by trying to use stolen memory backing */ - backing = psb_gtt_alloc_range(dev, aligned_size, "fb", 1, PAGE_SIZE); - if (backing) { - backing->gem.funcs = &psb_gem_object_funcs; - drm_gem_private_object_init(dev, &backing->gem, aligned_size); - return backing; - } - return NULL; -} - /** * psbfb_create- create a framebuffer * @fb_helper: the framebuffer helper @@ -268,6 +243,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, int size; int ret; struct gtt_range *backing; + struct drm_gem_object *obj; u32 bpp, depth; mode_cmd.width = sizes->surface_width; @@ -285,24 +261,25 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, size = ALIGN(size, PAGE_SIZE); /* Allocate the framebuffer in the GTT with stolen page backing */ - backing = psbfb_alloc(dev, size); - if (backing == NULL) - return -ENOMEM; + backing = psb_gem_create(dev, size, "fb", true, PAGE_SIZE); + if (IS_ERR(backing)) + return PTR_ERR(backing); + obj = &backing->gem; memset(dev_priv->vram_addr + backing->offset, 0, size); info = drm_fb_helper_alloc_fbi(fb_helper); if (IS_ERR(info)) { ret = PTR_ERR(info); - goto out; + goto err_drm_gem_object_put; } mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth); - fb = psb_framebuffer_create(dev, &mode_cmd, &backing->gem); + fb = psb_framebuffer_create(dev, &mode_cmd, obj); if (IS_ERR(fb)) { ret = PTR_ERR(fb); - goto out; + goto err_drm_gem_object_put; } fb_helper->fb = fb; @@ -333,8 +310,9 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, dev_dbg(dev->dev, "allocated %dx%d fb\n", fb->width, fb->height); return 0; -out: - psb_gtt_free_range(dev, backing); + +err_drm_gem_object_put: + drm_gem_object_put(obj); return ret; } diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 0c094d4e3f1c..97b7f3bdbdc3 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -101,7 +101,7 @@ void psb_gtt_unpin(struct gtt_range *gt) mutex_unlock(&dev_priv->gtt_mutex); } -void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) +static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) { /* Undo the mmap pin if we are destroying the object */ if (gt->mmapping) { @@ -133,13 +133,13 @@ static const struct vm_operations_struct psb_gem_vm_ops = { .close = drm_gem_vm_close, }; -co
[PATCH v2 08/10] drm/gma500: Set page-caching flags in GEM pin/unpin
Caching of the GEM object's backing pages are unrelated to GTT management. Move the respective calls from GTT code to GEM code. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 9 - drivers/gpu/drm/gma500/gtt.c | 17 ++--- drivers/gpu/drm/gma500/gtt.h | 2 +- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 425d183c76ca..def26d9ce89d 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -13,6 +13,8 @@ #include +#include + #include #include @@ -41,7 +43,9 @@ int psb_gem_pin(struct gtt_range *gt) npages = gt->gem.size / PAGE_SIZE; - ret = psb_gtt_insert(dev, gt, 0); + set_pages_array_wc(pages, npages); + + ret = psb_gtt_insert(dev, gt); if (ret) goto err_drm_gem_put_pages; @@ -84,6 +88,9 @@ void psb_gem_unpin(struct gtt_range *gt) (gpu_base + gt->offset), gt->npage, 0, 0); psb_gtt_remove(dev, gt); + /* Reset caching flags */ + set_pages_array_wb(gt->pages, gt->npage); + drm_gem_put_pages(>->gem, gt->pages, true, false); gt->pages = NULL; diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c index 5d940fdbe6b8..244de618e612 100644 --- a/drivers/gpu/drm/gma500/gtt.c +++ b/drivers/gpu/drm/gma500/gtt.c @@ -7,10 +7,6 @@ * Alan Cox */ -#include - -#include - #include "psb_drv.h" @@ -92,17 +88,15 @@ static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r) * psb_gtt_insert - put an object into the GTT * @dev: our DRM device * @r: our GTT range - * @resume: on resume * * Take our preallocated GTT range and insert the GEM object into * the GTT. This is protected via the gtt mutex which the caller * must hold. */ -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume) +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r) { u32 __iomem *gtt_slot; u32 pte; - struct page **pages; int i; if (r->pages == NULL) { @@ -113,12 +107,6 @@ int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume) WARN_ON(r->stolen); /* refcount these maybe ? */ gtt_slot = psb_gtt_entry(dev, r); - pages = r->pages; - - if (!resume) { - /* Make sure changes are visible to the GPU */ - set_pages_array_wc(pages, r->npage); - } /* Write our page entries into the GTT itself */ for (i = 0; i < r->npage; i++) { @@ -158,7 +146,6 @@ void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r) for (i = 0; i < r->npage; i++) iowrite32(pte, gtt_slot++); ioread32(gtt_slot - 1); - set_pages_array_wb(r->pages, r->npage); } static void psb_gtt_alloc(struct drm_device *dev) @@ -349,7 +336,7 @@ int psb_gtt_restore(struct drm_device *dev) while (r != NULL) { range = container_of(r, struct gtt_range, resource); if (range->pages) { - psb_gtt_insert(dev, range, 1); + psb_gtt_insert(dev, range); size += range->resource.end - range->resource.start; restored++; } diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h index 459a03141e8b..7af71617e0c5 100644 --- a/drivers/gpu/drm/gma500/gtt.h +++ b/drivers/gpu/drm/gma500/gtt.h @@ -49,7 +49,7 @@ int psb_gtt_allocate_resource(struct drm_psb_private *pdev, struct resource *res const char *name, resource_size_t size, resource_size_t align, bool stolen, u32 offset[static 1]); -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume); +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r); void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r); #endif -- 2.33.0
[PATCH v2 09/10] drm/gma500: Rewrite GTT page insert/remove without struct gtt_range
struct gtt_range represents a GEM object and should not be used for GTT setup. Change psb_gtt_insert() and psb_gtt_remove() to receive all necessary parameters from their caller. This also eliminates possible failure from psb_gtt_insert(). There's one exception in psb_gtt_restore(), which requires an upcast from struct resource to struct gtt_range when restoring the GTT after hibernation. A possible solution would track the GEM objects that need restoration separately from the GTT resource. Rename the functions to psb_gtt_insert_pages() and psb_gtt_remove_pages() to reflect their similarity to MMU interfaces. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 12 ++--- drivers/gpu/drm/gma500/gtt.c | 87 drivers/gpu/drm/gma500/gtt.h | 5 ++- 3 files changed, 36 insertions(+), 68 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index def26d9ce89d..c93d09e1921e 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -23,12 +23,12 @@ int psb_gem_pin(struct gtt_range *gt) { - int ret = 0; struct drm_device *dev = gt->gem.dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); u32 gpu_base = dev_priv->gtt.gatt_start; struct page **pages; unsigned int npages; + int ret; mutex_lock(&dev_priv->gtt_mutex); @@ -45,10 +45,7 @@ int psb_gem_pin(struct gtt_range *gt) set_pages_array_wc(pages, npages); - ret = psb_gtt_insert(dev, gt); - if (ret) - goto err_drm_gem_put_pages; - + psb_gtt_insert_pages(dev_priv, >->resource, pages); psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages, (gpu_base + gt->offset), npages, 0, 0, PSB_MMU_CACHED_MEMORY); @@ -62,8 +59,6 @@ int psb_gem_pin(struct gtt_range *gt) return 0; -err_drm_gem_put_pages: - drm_gem_put_pages(>->gem, pages, true, false); err_mutex_unlock: mutex_unlock(&dev_priv->gtt_mutex); return ret; @@ -86,13 +81,14 @@ void psb_gem_unpin(struct gtt_range *gt) psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu), (gpu_base + gt->offset), gt->npage, 0, 0); - psb_gtt_remove(dev, gt); + psb_gtt_remove_pages(dev_priv, >->resource); /* Reset caching flags */ set_pages_array_wb(gt->pages, gt->npage); drm_gem_put_pages(>->gem, gt->pages, true, false); gt->pages = NULL; + gt->npage = 0; out: mutex_unlock(&dev_priv->gtt_mutex); diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c index 244de618e612..cf71a2396c16 100644 --- a/drivers/gpu/drm/gma500/gtt.c +++ b/drivers/gpu/drm/gma500/gtt.c @@ -66,85 +66,51 @@ static inline uint32_t psb_gtt_mask_pte(uint32_t pfn, int type) return (pfn << PAGE_SHIFT) | mask; } -/** - * psb_gtt_entry - find the GTT entries for a gtt_range - * @dev: our DRM device - * @r: our GTT range - * - * Given a gtt_range object return the GTT offset of the page table - * entries for this gtt_range - */ -static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r) +static u32 __iomem *psb_gtt_entry(struct drm_psb_private *pdev, const struct resource *res) { - struct drm_psb_private *dev_priv = to_drm_psb_private(dev); - unsigned long offset; - - offset = r->resource.start - dev_priv->gtt_mem->start; + unsigned long offset = res->start - pdev->gtt_mem->start; - return dev_priv->gtt_map + (offset >> PAGE_SHIFT); + return pdev->gtt_map + (offset >> PAGE_SHIFT); } -/** - * psb_gtt_insert - put an object into the GTT - * @dev: our DRM device - * @r: our GTT range - * - * Take our preallocated GTT range and insert the GEM object into - * the GTT. This is protected via the gtt mutex which the caller - * must hold. - */ -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r) +void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *res, + struct page **pages) { + resource_size_t npages, i; u32 __iomem *gtt_slot; u32 pte; - int i; - if (r->pages == NULL) { - WARN_ON(1); - return -EINVAL; - } - - WARN_ON(r->stolen); /* refcount these maybe ? */ + /* Write our page entries into the GTT itself */ - gtt_slot = psb_gtt_entry(dev, r); + npages = resource_size(res) >> PAGE_SHIFT; + gtt_slot = psb_gtt_entry(pdev, res); - /* Write our page entries into the GTT itself */ - for (i = 0; i < r->npage; i++) { - pte = psb_gtt_mask_pte(page_to_pfn(r->pages[i]), - PSB_MMU_CACHED_MEMORY); - iowrite32(pte, gtt_slot++); + for (i =
[PATCH v2 07/10] drm/gma500: Inline psb_gtt_{alloc, free}_range() into rsp callers
psb_gtt_alloc_range() allocates struct gtt_range, create the GTT resource and performs some half-baked initialization. Inline the function into its only caller psb_gem_create(). For creating the GTT resource, introduce a new helper, psb_gtt_alloc_resource() that hides the details of the GTT. For psb_gtt_free_range(), inline the function into its only caller psb_gem_free_object(). While at it, remove the explicit invocation of drm_gem_free_mmap_offset(). The mmap offset is already released by drm_gem_object_release(). Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 94 drivers/gpu/drm/gma500/gtt.c | 27 +++ drivers/gpu/drm/gma500/gtt.h | 6 +++ 3 files changed, 65 insertions(+), 62 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 37b61334ade2..425d183c76ca 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -91,30 +91,22 @@ void psb_gem_unpin(struct gtt_range *gt) mutex_unlock(&dev_priv->gtt_mutex); } -static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) -{ - /* Undo the mmap pin if we are destroying the object */ - if (gt->mmapping) { - psb_gem_unpin(gt); - gt->mmapping = 0; - } - WARN_ON(gt->in_gart && !gt->stolen); - release_resource(>->resource); - kfree(gt); -} - static vm_fault_t psb_gem_fault(struct vm_fault *vmf); static void psb_gem_free_object(struct drm_gem_object *obj) { - struct gtt_range *gtt = to_gtt_range(obj); + struct gtt_range *gt = to_gtt_range(obj); - /* Remove the list map if one is present */ - drm_gem_free_mmap_offset(obj); drm_gem_object_release(obj); - /* This must occur last as it frees up the memory of the GEM object */ - psb_gtt_free_range(obj->dev, gtt); + /* Undo the mmap pin if we are destroying the object */ + if (gt->mmapping) + psb_gem_unpin(gt); + + WARN_ON(gt->in_gart && !gt->stolen); + + release_resource(>->resource); + kfree(gt); } static const struct vm_operations_struct psb_gem_vm_ops = { @@ -128,59 +120,35 @@ static const struct drm_gem_object_funcs psb_gem_object_funcs = { .vm_ops = &psb_gem_vm_ops, }; -static struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, -const char *name, int backed, u32 align) -{ - struct drm_psb_private *dev_priv = to_drm_psb_private(dev); - struct gtt_range *gt; - struct resource *r = dev_priv->gtt_mem; - int ret; - unsigned long start, end; - - if (backed) { - /* The start of the GTT is the stolen pages */ - start = r->start; - end = r->start + dev_priv->gtt.stolen_size - 1; - } else { - /* The rest we will use for GEM backed objects */ - start = r->start + dev_priv->gtt.stolen_size; - end = r->end; - } - - gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL); - if (gt == NULL) - return NULL; - gt->resource.name = name; - gt->stolen = backed; - gt->in_gart = backed; - /* Ensure this is set for non GEM objects */ - gt->gem.dev = dev; - ret = allocate_resource(dev_priv->gtt_mem, >->resource, - len, start, end, align, NULL, NULL); - if (ret == 0) { - gt->offset = gt->resource.start - r->start; - return gt; - } - kfree(gt); - return NULL; -} - struct gtt_range * psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align) { + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); struct gtt_range *gt; struct drm_gem_object *obj; int ret; size = roundup(size, PAGE_SIZE); - gt = psb_gtt_alloc_range(dev, size, name, stolen, align); - if (!gt) { - dev_err(dev->dev, "no memory for %lld byte GEM object\n", size); - return ERR_PTR(-ENOSPC); - } + gt = kzalloc(sizeof(*gt), GFP_KERNEL); + if (!gt) + return ERR_PTR(-ENOMEM); obj = >->gem; + /* GTT resource */ + + ret = psb_gtt_allocate_resource(dev_priv, >->resource, name, size, align, stolen, + >->offset); + if (ret) + goto err_kfree; + + if (stolen) { + gt->stolen = true; + gt->in_gart = 1; + } + + /* GEM object */ + obj->funcs = &psb_gem_object_funcs; if (stolen) { @@ -188,7 +156,7 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, } else { ret = drm_gem_object_init(dev, obj, size); if (ret) - goto err_psb_gtt_free_range; +
[PATCH v2 06/10] drm/gma500: Inline psb_gtt_attach_pages() and psb_gtt_detach_pages()
psb_gtt_attach_pages() are not GTT functions but deal with the GEM object's SHMEM pages. The only callers of psb_gtt_attach_pages() and psb_gtt_detach_pages() are the GEM pin helpers. Inline the calls and cleanup the resulting code. v2: * unlock gtt_mutex in pin-error handling (Patrik) Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 94 1 file changed, 42 insertions(+), 52 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 1905468924ca..37b61334ade2 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -19,63 +19,48 @@ #include "gem.h" #include "psb_drv.h" -/* - * Pin and build an in-kernel list of the pages that back our GEM object. - * While we hold this the pages cannot be swapped out. This is protected - * via the gtt mutex which the caller must hold. - */ -static int psb_gtt_attach_pages(struct gtt_range *gt) -{ - struct page **pages; - - WARN_ON(gt->pages); - - pages = drm_gem_get_pages(>->gem); - if (IS_ERR(pages)) - return PTR_ERR(pages); - - gt->npage = gt->gem.size / PAGE_SIZE; - gt->pages = pages; - - return 0; -} - -/* - * Undo the effect of psb_gtt_attach_pages. At this point the pages - * must have been removed from the GTT as they could now be paged out - * and move bus address. This is protected via the gtt mutex which the - * caller must hold. - */ -static void psb_gtt_detach_pages(struct gtt_range *gt) -{ - drm_gem_put_pages(>->gem, gt->pages, true, false); - gt->pages = NULL; -} - int psb_gem_pin(struct gtt_range *gt) { int ret = 0; struct drm_device *dev = gt->gem.dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); u32 gpu_base = dev_priv->gtt.gatt_start; + struct page **pages; + unsigned int npages; mutex_lock(&dev_priv->gtt_mutex); - if (gt->in_gart == 0 && gt->stolen == 0) { - ret = psb_gtt_attach_pages(gt); - if (ret < 0) - goto out; - ret = psb_gtt_insert(dev, gt, 0); - if (ret < 0) { - psb_gtt_detach_pages(gt); - goto out; - } - psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), -gt->pages, (gpu_base + gt->offset), -gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY); + if (gt->in_gart || gt->stolen) + goto out; /* already mapped */ + + pages = drm_gem_get_pages(>->gem); + if (IS_ERR(pages)) { + ret = PTR_ERR(pages); + goto err_mutex_unlock; } - gt->in_gart++; + + npages = gt->gem.size / PAGE_SIZE; + + ret = psb_gtt_insert(dev, gt, 0); + if (ret) + goto err_drm_gem_put_pages; + + psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages, +(gpu_base + gt->offset), npages, 0, 0, +PSB_MMU_CACHED_MEMORY); + + gt->npage = npages; + gt->pages = pages; + out: + ++gt->in_gart; + mutex_unlock(&dev_priv->gtt_mutex); + + return 0; + +err_drm_gem_put_pages: + drm_gem_put_pages(>->gem, pages, true, false); +err_mutex_unlock: mutex_unlock(&dev_priv->gtt_mutex); return ret; } @@ -90,14 +75,19 @@ void psb_gem_unpin(struct gtt_range *gt) WARN_ON(!gt->in_gart); - gt->in_gart--; - if (gt->in_gart == 0 && gt->stolen == 0) { - psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu), + --gt->in_gart; + + if (gt->in_gart || gt->stolen) + goto out; + + psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu), (gpu_base + gt->offset), gt->npage, 0, 0); - psb_gtt_remove(dev, gt); - psb_gtt_detach_pages(gt); - } + psb_gtt_remove(dev, gt); + drm_gem_put_pages(>->gem, gt->pages, true, false); + gt->pages = NULL; + +out: mutex_unlock(&dev_priv->gtt_mutex); } -- 2.33.0
[PATCH v2 03/10] drm/gma500: Reimplement psb_gem_create()
Implement psb_gem_create() for general use. Create the GEM handle in psb_gem_create_dumb(). Allows to use psb_gem_create() for creating all of the GEM objects. While at it, clean-up drm_gem_dumb_create() to make it more readable. Signed-off-by: Thomas Zimmermann Acked-by: Patrik Jakobsson --- drivers/gpu/drm/gma500/gem.c | 93 ++-- drivers/gpu/drm/gma500/gem.h | 4 +- 2 files changed, 59 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index b52908313ee5..0c094d4e3f1c 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -175,45 +175,36 @@ struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, return NULL; } -int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size, - u32 *handlep, int stolen, u32 align) +struct gtt_range * +psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align) { - struct gtt_range *r; + struct gtt_range *gt; + struct drm_gem_object *obj; int ret; - u32 handle; size = roundup(size, PAGE_SIZE); - /* Allocate our object - for now a direct gtt range which is not - stolen memory backed */ - r = psb_gtt_alloc_range(dev, size, "gem", 0, PAGE_SIZE); - if (r == NULL) { + gt = psb_gtt_alloc_range(dev, size, name, stolen, align); + if (!gt) { dev_err(dev->dev, "no memory for %lld byte GEM object\n", size); - return -ENOSPC; + return ERR_PTR(-ENOSPC); } - r->gem.funcs = &psb_gem_object_funcs; - /* Initialize the extra goodies GEM needs to do all the hard work */ - if (drm_gem_object_init(dev, &r->gem, size) != 0) { - psb_gtt_free_range(dev, r); - /* GEM doesn't give an error code so use -ENOMEM */ - dev_err(dev->dev, "GEM init failed for %lld\n", size); - return -ENOMEM; - } - /* Limit the object to 32bit mappings */ - mapping_set_gfp_mask(r->gem.filp->f_mapping, GFP_KERNEL | __GFP_DMA32); - /* Give the object a handle so we can carry it more easily */ - ret = drm_gem_handle_create(file, &r->gem, &handle); - if (ret) { - dev_err(dev->dev, "GEM handle failed for %p, %lld\n", - &r->gem, size); - drm_gem_object_release(&r->gem); - psb_gtt_free_range(dev, r); - return ret; - } - /* We have the initial and handle reference but need only one now */ - drm_gem_object_put(&r->gem); - *handlep = handle; - return 0; + obj = >->gem; + + obj->funcs = &psb_gem_object_funcs; + + ret = drm_gem_object_init(dev, obj, size); + if (ret) + goto err_psb_gtt_free_range; + + /* Limit the object to 32-bit mappings */ + mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32); + + return gt; + +err_psb_gtt_free_range: + psb_gtt_free_range(dev, gt); + return ERR_PTR(ret); } /** @@ -229,10 +220,40 @@ int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size, int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args) { - args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 64); - args->size = args->pitch * args->height; - return psb_gem_create(file, dev, args->size, &args->handle, 0, - PAGE_SIZE); + size_t pitch, size; + struct gtt_range *gt; + struct drm_gem_object *obj; + u32 handle; + int ret; + + pitch = args->width * DIV_ROUND_UP(args->bpp, 8); + pitch = ALIGN(pitch, 64); + + size = pitch * args->height; + size = roundup(size, PAGE_SIZE); + if (!size) + return -EINVAL; + + gt = psb_gem_create(dev, size, "gem", false, PAGE_SIZE); + if (IS_ERR(gt)) + return PTR_ERR(gt); + obj = >->gem; + + ret = drm_gem_handle_create(file, obj, &handle); + if (ret) + goto err_drm_gem_object_put; + + drm_gem_object_put(obj); + + args->pitch = pitch; + args->size = size; + args->handle = handle; + + return 0; + +err_drm_gem_object_put: + drm_gem_object_put(obj); + return ret; } /** diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h index 275494aedd4c..ad76127dc719 100644 --- a/drivers/gpu/drm/gma500/gem.h +++ b/drivers/gpu/drm/gma500/gem.h @@ -14,8 +14,8 @@ struct drm_device; extern const struct drm_gem_object_funcs psb_gem_object_funcs; -extern int psb_gem_create(struct drm_file *file, struct drm_device *dev, - u64 size, u32 *handlep, int stolen, u32 align); +struct gtt_range * +psb_gem_create(struct dr
[PATCH v2 10/10] drm/gma500: Rename struct gtt_range to struct psb_gem_object
struct gtt_range represents a GEM object. Rename the structure to struct psb_gem_object and update all users. No functional changes. Signed-off-by: Thomas Zimmermann Acked-by: Patrik Jakobsson --- drivers/gpu/drm/gma500/framebuffer.c | 9 +- drivers/gpu/drm/gma500/gem.c | 106 +++-- drivers/gpu/drm/gma500/gem.h | 25 - drivers/gpu/drm/gma500/gma_display.c | 50 +- drivers/gpu/drm/gma500/gtt.c | 15 +-- drivers/gpu/drm/gma500/gtt.h | 15 --- drivers/gpu/drm/gma500/oaktrail_crtc.c | 3 +- drivers/gpu/drm/gma500/psb_intel_display.c | 15 ++- drivers/gpu/drm/gma500/psb_intel_drv.h | 2 +- 9 files changed, 123 insertions(+), 117 deletions(-) diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 3ea6679ccd38..45df9de22007 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -81,14 +81,13 @@ static vm_fault_t psbfb_vm_fault(struct vm_fault *vmf) struct drm_framebuffer *fb = vma->vm_private_data; struct drm_device *dev = fb->dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); - struct gtt_range *gtt = to_gtt_range(fb->obj[0]); + struct psb_gem_object *pobj = to_psb_gem_object(fb->obj[0]); int page_num; int i; unsigned long address; vm_fault_t ret = VM_FAULT_SIGBUS; unsigned long pfn; - unsigned long phys_addr = (unsigned long)dev_priv->stolen_base + - gtt->offset; + unsigned long phys_addr = (unsigned long)dev_priv->stolen_base + pobj->offset; page_num = vma_pages(vma); address = vmf->address - (vmf->pgoff << PAGE_SHIFT); @@ -242,7 +241,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, struct drm_mode_fb_cmd2 mode_cmd; int size; int ret; - struct gtt_range *backing; + struct psb_gem_object *backing; struct drm_gem_object *obj; u32 bpp, depth; @@ -264,7 +263,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, backing = psb_gem_create(dev, size, "fb", true, PAGE_SIZE); if (IS_ERR(backing)) return PTR_ERR(backing); - obj = &backing->gem; + obj = &backing->base; memset(dev_priv->vram_addr + backing->offset, 0, size); diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index c93d09e1921e..8d65af80bb08 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -21,9 +21,10 @@ #include "gem.h" #include "psb_drv.h" -int psb_gem_pin(struct gtt_range *gt) +int psb_gem_pin(struct psb_gem_object *pobj) { - struct drm_device *dev = gt->gem.dev; + struct drm_gem_object *obj = &pobj->base; + struct drm_device *dev = obj->dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); u32 gpu_base = dev_priv->gtt.gatt_start; struct page **pages; @@ -32,29 +33,29 @@ int psb_gem_pin(struct gtt_range *gt) mutex_lock(&dev_priv->gtt_mutex); - if (gt->in_gart || gt->stolen) + if (pobj->in_gart || pobj->stolen) goto out; /* already mapped */ - pages = drm_gem_get_pages(>->gem); + pages = drm_gem_get_pages(obj); if (IS_ERR(pages)) { ret = PTR_ERR(pages); goto err_mutex_unlock; } - npages = gt->gem.size / PAGE_SIZE; + npages = obj->size / PAGE_SIZE; set_pages_array_wc(pages, npages); - psb_gtt_insert_pages(dev_priv, >->resource, pages); + psb_gtt_insert_pages(dev_priv, &pobj->resource, pages); psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages, -(gpu_base + gt->offset), npages, 0, 0, +(gpu_base + pobj->offset), npages, 0, 0, PSB_MMU_CACHED_MEMORY); - gt->npage = npages; - gt->pages = pages; + pobj->npage = npages; + pobj->pages = pages; out: - ++gt->in_gart; + ++pobj->in_gart; mutex_unlock(&dev_priv->gtt_mutex); return 0; @@ -64,31 +65,32 @@ int psb_gem_pin(struct gtt_range *gt) return ret; } -void psb_gem_unpin(struct gtt_range *gt) +void psb_gem_unpin(struct psb_gem_object *pobj) { - struct drm_device *dev = gt->gem.dev; + struct drm_gem_object *obj = &pobj->base; + struct drm_device *dev = obj->dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); u32 gpu_base = dev_priv->gtt.gatt_start; mutex_lock(&dev_priv->gtt_mutex); - WARN_ON(!gt->in_gart); + WARN_ON(!pobj->in_gart); - --gt->in_gart; + --pobj->in_gart; - if (gt->in_gart || gt->stolen) + if (pobj->in_gart || pobj->stolen) goto out; psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_p
Re: [PATCH 09/28] dma-buf: use the new iterator in dma_resv_poll
Am 05.10.21 um 09:44 schrieb Tvrtko Ursulin: On 01/10/2021 11:05, Christian König wrote: 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) It is unchanged with this patch, but are the semantics supposed to be like this? Signal poll event if _any_ of the shared fences has been signaled? That had Daniel and me confused for a moment as well. We don't signal the poll when any of the shared fences has signaled, but rather install a callback on the first not-signaled fence. This callback then issues a re-test of the poll and only if we can't find any more fence the poll is considered signaled (at least that's the idea, the coding could as well be broken). Christian. Regards, Tvrtko @@ -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
Re: linux-next: build warning after merge of the drm-misc tree
Thanks for the notice, going to fix this. Christian. Am 05.10.21 um 09:59 schrieb Stephen Rothwell: Hi all, After merging the drm-misc tree, today's linux-next build (htmldocs) produced this warning: include/linux/dma-buf.h:456: warning: Function parameter or member 'cb_in' not described in 'dma_buf' include/linux/dma-buf.h:456: warning: Function parameter or member 'cb_out' not described in 'dma_buf' Introduced by commit 6b51b02a3a0a ("dma-buf: fix and rework dma_buf_poll v7")
Re: [PATCH 1/2] drm/dp: add drm_dp_phy_name() for getting DP PHY name
On Tue, Oct 05, 2021 at 11:10:52AM +0300, Jani Nikula wrote: > Add a helper for getting the DP PHY name. In the interest of caller > simplicity and to avoid allocations and passing in of buffers, duplicate > the const strings to return. It's a minor penalty to pay for simplicity > in all the call sites. Yeah, the on stack extra buffer is getting a bit annoying, especially with the calls multiplying like rabbits in my recent patches. Series is Reviewed-by: Ville Syrjälä > > Cc: Ville Syrjälä > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/drm_dp_helper.c | 21 + > include/drm/drm_dp_helper.h | 2 ++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 4d0d1e8e51fa..f1d03b5a4bab 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -197,6 +197,27 @@ void drm_dp_link_train_channel_eq_delay(const struct > drm_dp_aux *aux, > } > EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); > > +const char *drm_dp_phy_name(enum drm_dp_phy dp_phy) > +{ > + static const char * const phy_names[] = { > + [DP_PHY_DPRX] = "DPRX", > + [DP_PHY_LTTPR1] = "LTTPR 1", > + [DP_PHY_LTTPR2] = "LTTPR 2", > + [DP_PHY_LTTPR3] = "LTTPR 3", > + [DP_PHY_LTTPR4] = "LTTPR 4", > + [DP_PHY_LTTPR5] = "LTTPR 5", > + [DP_PHY_LTTPR6] = "LTTPR 6", > + [DP_PHY_LTTPR7] = "LTTPR 7", > + [DP_PHY_LTTPR8] = "LTTPR 8", > + }; > + > + if (dp_phy < 0 || dp_phy >= ARRAY_SIZE(phy_names)) > + return ""; > + > + return phy_names[dp_phy]; > +} > +EXPORT_SYMBOL(drm_dp_phy_name); > + > void drm_dp_lttpr_link_train_clock_recovery_delay(void) > { > usleep_range(100, 200); > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index b52df4db3e8f..c873e6349b41 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -2115,6 +2115,8 @@ bool drm_dp_read_sink_count_cap(struct drm_connector > *connector, > const struct drm_dp_desc *desc); > int drm_dp_read_sink_count(struct drm_dp_aux *aux); > > +const char *drm_dp_phy_name(enum drm_dp_phy dp_phy); > + > int drm_dp_read_lttpr_common_caps(struct drm_dp_aux *aux, > u8 caps[DP_LTTPR_COMMON_CAP_SIZE]); > int drm_dp_read_lttpr_phy_caps(struct drm_dp_aux *aux, > -- > 2.30.2 -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH 21/26] drm/i915: Multi-BB execbuf
Hi Matthew, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on drm-tip/drm-tip] [cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master airlied/drm-next 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/Matthew-Brost/Parallel-submission-aka-multi-bb-execbuf/20211005-061424 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: i386-randconfig-a004-20211004 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c0039de2953d15815448b4b3c3bafb45607781e0) 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/758202922dad66c1b302eb34a141961acbefe417 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Matthew-Brost/Parallel-submission-aka-multi-bb-execbuf/20211005-061424 git checkout 758202922dad66c1b302eb34a141961acbefe417 # 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/gem/i915_gem_execbuffer.c:2361:6: warning: variable >> 'rq' is used uninitialized whenever 'if' condition is false >> [-Wsometimes-uninitialized] if (throttle) ^~~~ drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:2365:6: note: uninitialized use occurs here if (rq) { ^~ drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:2361:2: note: remove the 'if' if its condition is always true if (throttle) ^ drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:2346:25: note: initialize the variable 'rq' to silence this warning struct i915_request *rq; ^ = NULL 1 warning generated. vim +2361 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c e5dadff4b09376 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-15 2341 758202922dad66 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Matthew Brost 2021-10-04 2342 static int eb_pin_timeline(struct i915_execbuffer *eb, struct intel_context *ce, 758202922dad66 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Matthew Brost 2021-10-04 2343 bool throttle) 8f2a1057d6ec21 drivers/gpu/drm/i915/i915_gem_execbuffer.c Chris Wilson 2019-04-25 2344 { e5dadff4b09376 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-15 2345struct intel_timeline *tl; 758202922dad66 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Matthew Brost 2021-10-04 2346struct i915_request *rq; 8f2a1057d6ec21 drivers/gpu/drm/i915/i915_gem_execbuffer.c Chris Wilson 2019-04-25 2347 a4e57f9031ccd5 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-04 2348/* a4e57f9031ccd5 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-04 2349 * Take a local wakeref for preparing to dispatch the execbuf as a4e57f9031ccd5 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-04 2350 * we expect to access the hardware fairly frequently in the a4e57f9031ccd5 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-04 2351 * process, and require the engine to be kept awake between accesses. a4e57f9031ccd5 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-04 2352 * Upon dispatch, we acquire another prolonged wakeref that we hold a4e57f9031ccd5 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-04 2353 * until the timeline is idle, which in turn releases the wakeref a4e57f9031ccd5 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-04 2354 * taken on the engine, and the parent device. a4e57f9031ccd5 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-04 2355 */ e5dadff4b09376 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Chris Wilson 2019-08-15 2356tl = intel_context_timeline_lock(ce); 758202922dad66 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Matthew Brost 2021-10-04 2357if (IS_ERR(tl)) 758202922dad66 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c Matthew Brost 2021-10-04 2358
Re: [PATCH 1/2] drm/dp: add drm_dp_phy_name() for getting DP PHY name
On Tue, 05 Oct 2021, Ville Syrjälä wrote: > On Tue, Oct 05, 2021 at 11:10:52AM +0300, Jani Nikula wrote: >> Add a helper for getting the DP PHY name. In the interest of caller >> simplicity and to avoid allocations and passing in of buffers, duplicate >> the const strings to return. It's a minor penalty to pay for simplicity >> in all the call sites. > > Yeah, the on stack extra buffer is getting a bit annoying, especially > with the calls multiplying like rabbits in my recent patches. > > Series is > Reviewed-by: Ville Syrjälä Thanks. I don't want to block your patches with this, so I can wait and rebase. BR, Jani. > >> >> Cc: Ville Syrjälä >> Signed-off-by: Jani Nikula >> --- >> drivers/gpu/drm/drm_dp_helper.c | 21 + >> include/drm/drm_dp_helper.h | 2 ++ >> 2 files changed, 23 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_dp_helper.c >> b/drivers/gpu/drm/drm_dp_helper.c >> index 4d0d1e8e51fa..f1d03b5a4bab 100644 >> --- a/drivers/gpu/drm/drm_dp_helper.c >> +++ b/drivers/gpu/drm/drm_dp_helper.c >> @@ -197,6 +197,27 @@ void drm_dp_link_train_channel_eq_delay(const struct >> drm_dp_aux *aux, >> } >> EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); >> >> +const char *drm_dp_phy_name(enum drm_dp_phy dp_phy) >> +{ >> +static const char * const phy_names[] = { >> +[DP_PHY_DPRX] = "DPRX", >> +[DP_PHY_LTTPR1] = "LTTPR 1", >> +[DP_PHY_LTTPR2] = "LTTPR 2", >> +[DP_PHY_LTTPR3] = "LTTPR 3", >> +[DP_PHY_LTTPR4] = "LTTPR 4", >> +[DP_PHY_LTTPR5] = "LTTPR 5", >> +[DP_PHY_LTTPR6] = "LTTPR 6", >> +[DP_PHY_LTTPR7] = "LTTPR 7", >> +[DP_PHY_LTTPR8] = "LTTPR 8", >> +}; >> + >> +if (dp_phy < 0 || dp_phy >= ARRAY_SIZE(phy_names)) >> +return ""; >> + >> +return phy_names[dp_phy]; >> +} >> +EXPORT_SYMBOL(drm_dp_phy_name); >> + >> void drm_dp_lttpr_link_train_clock_recovery_delay(void) >> { >> usleep_range(100, 200); >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h >> index b52df4db3e8f..c873e6349b41 100644 >> --- a/include/drm/drm_dp_helper.h >> +++ b/include/drm/drm_dp_helper.h >> @@ -2115,6 +2115,8 @@ bool drm_dp_read_sink_count_cap(struct drm_connector >> *connector, >> const struct drm_dp_desc *desc); >> int drm_dp_read_sink_count(struct drm_dp_aux *aux); >> >> +const char *drm_dp_phy_name(enum drm_dp_phy dp_phy); >> + >> int drm_dp_read_lttpr_common_caps(struct drm_dp_aux *aux, >>u8 caps[DP_LTTPR_COMMON_CAP_SIZE]); >> int drm_dp_read_lttpr_phy_caps(struct drm_dp_aux *aux, >> -- >> 2.30.2 -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 09/28] dma-buf: use the new iterator in dma_resv_poll
On 05/10/2021 09:16, Christian König wrote: Am 05.10.21 um 09:44 schrieb Tvrtko Ursulin: On 01/10/2021 11:05, Christian König wrote: 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) It is unchanged with this patch, but are the semantics supposed to be like this? Signal poll event if _any_ of the shared fences has been signaled? That had Daniel and me confused for a moment as well. We don't signal the poll when any of the shared fences has signaled, but rather install a callback on the first not-signaled fence. This callback then issues a re-test of the poll and only if we can't find any more fence the poll is considered signaled (at least that's the idea, the coding could as well be broken). You are right, one too many boolean inversions for me not to get confused. Reviewed-by: Tvrtko Ursulin Regards, Tvrtko Christian. Regards, Tvrtko @@ -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
Re: [PATCH v13 13/35] drm/tegra: gr2d: Support generic power domain and runtime PM
On Mon, 4 Oct 2021 at 17:57, Dmitry Osipenko wrote: > > 04.10.2021 14:01, Ulf Hansson пишет: > > On Fri, 1 Oct 2021 at 21:00, Dmitry Osipenko wrote: > >> > >> 01.10.2021 17:55, Ulf Hansson пишет: > >>> On Fri, 1 Oct 2021 at 16:29, Dmitry Osipenko wrote: > > 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 > >>> > >>> pm_runtime_disable() end up calling _pm_runtime_barrier(), which calls > >>> cancel_work_sync() if dev->power.request_pending has been set. > >>> > >>> If the work that was punted to the pm_wq in rpm_idle() has not been > >>> started yet, we end up just canceling it. In other words, there are no > >>> guarantees it runs to completion. > >> > >> You're right. Although, in a case of this particular patch, the syncing > >> is actually implicitly done by pm_runtime_dont_use_autosuspend(). > >> > >> But for drivers which don't use auto-suspend, there is no sync. This > >> looks like a disaster, it's a very common pattern for drivers to > >> 'put+disable'. > >> > >>> Moreover, use space may have bumped the usage count via sysfs for the > >>> device (pm_runtime_forbid()) to keep the device runtime resumed. > >> > >> Right, this is also a disaster in a case of driver removal. > >> > Calling pm_runtime_force_suspend() isn't correct because each 'enable' > must have the corresponding 'disable'. Hence there is no problem here. > >>> > >>> pm_runtime_force_suspend() calls pm_runtime_disable(), so I think that > >>> should be fine. No? > >> > >> [adding Rafael] > >> > >> Rafael, could you please explain how drivers are supposed to properly > >> suspend and disable RPM to cut off power and reset state that was > >> altered by the driver's resume callback? What we're missing? Is Ulf's > >> suggestion acceptable? > >> > >> The RPM state of a device is getting reset on driver's removal, hence > >> all refcounts that were bumped by the rpm-resume callback of the device > >> driver will be screwed up if device is kept resumed after removal. I > >> just verified that it's true in practice. > > > > Note that, what makes the Tegra drivers a bit special is that they are > > always built with CONFIG_PM being set (selected from the "SoC" > > Kconfig). > > > > Therefore, pm_runtime_force_suspend() can work for some of these > > cases. Using this, would potentially avoid the driver from having to > > runtime resume the device in ->remove(), according to the below > > generic sequence, which is used in many drivers. > > > > pm_runtime_get_sync() > > clk_disable_unprepare() (+ additional things to turn off the device) > > pm_runtime_disable() > > pm_runtime_put_noidle() > > It's not a problem to change this patchset. The problem is that if > you'll grep mainline for 'pm_runtime_disable', you will find that there > are a lot of drivers in a potential trouble. Let's start by fixing this patchset, please - then we can consider what to do with the other cases separately. > > I'm proposing that we should change pm_runtime_disable() to perform the > syncing with this oneliner: > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index ec94049442b9..5c9f28165824 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -1380,6 +1380,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_barrier); > */ > void __pm_runtime_disable(struct device *dev, bool check_resume) > { > + flush_work(&dev->power.work); > + What about the latency this may introduce? I am not sure that is acceptable here!? > spin_lock_irq(&dev->power.lock); > > if (dev->power.disable_depth > 0) { > > Objections? > > The sysfs rpm-forbid is a separat
Re: [PATCH 01/10] backlight: qcom-wled: Pass number of elements to read to read_u32_array
On Mon, Oct 04, 2021 at 09:27:32PM +0200, Marijn Suijten wrote: > of_property_read_u32_array takes the number of elements to read as last > argument. This does not always need to be 4 (sizeof(u32)) but should > instead be the size of the array in DT as read just above with > of_property_count_elems_of_size. > > To not make such an error go unnoticed again the driver now bails > accordingly when of_property_read_u32_array returns an error. > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") > Signed-off-by: Marijn Suijten > Reviewed-by: AngeloGioacchino Del Regno > Reviewed-by: Daniel Thompson > --- > drivers/video/backlight/qcom-wled.c | 13 ++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index d094299c2a48..6af808af2328 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -1528,11 +1528,18 @@ static int wled_configure(struct wled *wled) > string_len = of_property_count_elems_of_size(dev->of_node, >"qcom,enabled-strings", >sizeof(u32)); > - if (string_len > 0) > - of_property_read_u32_array(dev->of_node, > + if (string_len > 0) { > + rc = of_property_read_u32_array(dev->of_node, > "qcom,enabled-strings", > wled->cfg.enabled_strings, > - sizeof(u32)); > + string_len); > + if (rc) { > + dev_err(dev, "Failed to read %d elements from " > + "qcom,enabled-strings: %d\n", > + string_len, rc); > + return -EINVAL; > + } > + } > > return 0; > } > -- > 2.33.0 >
Re: [PATCH 02/10] backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion
On Mon, Oct 04, 2021 at 09:27:33PM +0200, Marijn Suijten wrote: > The kernel already provides appropriate primitives to perform endianness > conversion which should be used in favour of manual bit-wrangling. > > Signed-off-by: Marijn Suijten > Reviewed-by: AngeloGioacchino Del Regno > Reviewed-by: Daniel Thompson > --- > drivers/video/backlight/qcom-wled.c | 25 +++-- > 1 file changed, 11 insertions(+), 14 deletions(-) > > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index 6af808af2328..9927ed98944a 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -231,14 +231,14 @@ struct wled { > static int wled3_set_brightness(struct wled *wled, u16 brightness) > { > int rc, i; > - u8 v[2]; > + u16 v; > > - v[0] = brightness & 0xff; > - v[1] = (brightness >> 8) & 0xf; > + v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); > > for (i = 0; i < wled->cfg.num_strings; ++i) { > rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr + > -WLED3_SINK_REG_BRIGHT(i), v, 2); > +WLED3_SINK_REG_BRIGHT(i), > +&v, sizeof(v)); > if (rc < 0) > return rc; > } > @@ -249,19 +249,18 @@ static int wled3_set_brightness(struct wled *wled, u16 > brightness) > static int wled4_set_brightness(struct wled *wled, u16 brightness) > { > int rc, i; > - u16 low_limit = wled->max_brightness * 4 / 1000; > - u8 v[2]; > + u16 v, low_limit = wled->max_brightness * 4 / 1000; > > /* WLED4's lower limit of operation is 0.4% */ > if (brightness > 0 && brightness < low_limit) > brightness = low_limit; > > - v[0] = brightness & 0xff; > - v[1] = (brightness >> 8) & 0xf; > + v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); > > for (i = 0; i < wled->cfg.num_strings; ++i) { > rc = regmap_bulk_write(wled->regmap, wled->sink_addr + > -WLED4_SINK_REG_BRIGHT(i), v, 2); > +WLED4_SINK_REG_BRIGHT(i), > +&v, sizeof(v)); > if (rc < 0) > return rc; > } > @@ -272,22 +271,20 @@ static int wled4_set_brightness(struct wled *wled, u16 > brightness) > static int wled5_set_brightness(struct wled *wled, u16 brightness) > { > int rc, offset; > - u16 low_limit = wled->max_brightness * 1 / 1000; > - u8 v[2]; > + u16 v, low_limit = wled->max_brightness * 1 / 1000; > > /* WLED5's lower limit is 0.1% */ > if (brightness < low_limit) > brightness = low_limit; > > - v[0] = brightness & 0xff; > - v[1] = (brightness >> 8) & 0x7f; > + v = cpu_to_le16(brightness & WLED5_SINK_REG_BRIGHT_MAX_15B); > > offset = (wled->cfg.mod_sel == MOD_A) ? > WLED5_SINK_REG_MOD_A_BRIGHTNESS_LSB : > WLED5_SINK_REG_MOD_B_BRIGHTNESS_LSB; > > rc = regmap_bulk_write(wled->regmap, wled->sink_addr + offset, > -v, 2); > +&v, sizeof(v)); > return rc; > } > > -- > 2.33.0 >
Re: [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT
On Mon, Oct 04, 2021 at 09:27:35PM +0200, Marijn Suijten wrote: > The strings passed in DT may possibly cause out-of-bounds register > accesses and should be validated before use. > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") The first half of this patch actually fixes patch 1 from this patch set. It would be better to move that code there. Daniel. > Signed-off-by: Marijn Suijten > Reviewed-by: AngeloGioacchino Del Regno > > --- > drivers/video/backlight/qcom-wled.c | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index 29910e603c42..27e8949c7922 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled) >"qcom,enabled-strings", >sizeof(u32)); > if (string_len > 0) { > + if (string_len > wled->max_string_count) { > + dev_err(dev, "Cannot have more than %d strings\n", > + wled->max_string_count); > + return -EINVAL; > + } > + > rc = of_property_read_u32_array(dev->of_node, > "qcom,enabled-strings", > wled->cfg.enabled_strings, > @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled) > return -EINVAL; > } > > + for (i = 0; i < string_len; ++i) { > + if (wled->cfg.enabled_strings[i] >= > wled->max_string_count) { > + dev_err(dev, "qcom,enabled-strings index %d at > %d is out of bounds\n", > + wled->cfg.enabled_strings[i], i); > + return -EINVAL; > + } > + } > + > cfg->num_strings = string_len; > } > > -- > 2.33.0 >
Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote: > When not specifying num-strings in the DT the default is used, but +1 is > added to it which turns wled3 into 4 and wled4/5 into 5 strings instead > of 3 and 4 respectively, causing out of bounds reads and register > read/writes. This +1 exists for a deficiency in the DT parsing code, > and is simply omitted entirely - solving this oob issue - by allowing > one extra iteration of the wled_var_cfg function parsing this particular > property. > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver") > Signed-off-by: Marijn Suijten > Reviewed-by: AngeloGioacchino Del Regno > > --- > drivers/video/backlight/qcom-wled.c | 8 +++- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index 27e8949c7922..66ce77ee3099 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = { > > static u32 wled3_num_strings_values_fn(u32 idx) > { > - return idx + 1; > + return idx; > } > > static const struct wled_var_cfg wled3_num_strings_cfg = { > .fn = wled3_num_strings_values_fn, > - .size = 3, > + .size = 4, /* [0, 3] */ 0 is not a valid value for this property. > }; > > static const struct wled_var_cfg wled4_num_strings_cfg = { > .fn = wled3_num_strings_values_fn, > - .size = 4, > + .size = 5, /* [0, 4] */ Ditto. > }; > > static u32 wled3_switch_freq_values_fn(u32 idx) > @@ -1520,8 +1520,6 @@ static int wled_configure(struct wled *wled) > *bool_opts[i].val_ptr = true; > } > > - cfg->num_strings = cfg->num_strings + 1; > - > string_len = of_property_count_elems_of_size(dev->of_node, >"qcom,enabled-strings", >sizeof(u32)); > -- > 2.33.0 >
Re: [PATCH 06/10] backlight: qcom-wled: Remove unnecessary 4th default string in wled3
On Mon, Oct 04, 2021 at 09:27:37PM +0200, Marijn Suijten wrote: > The previous commit improves num_strings parsing to not go over the > maximum of 3 strings for wled3 anymore. Likewise this default index for > a hypothetical 4th string is invalid and could access registers that are > not mapped to the desired purpose. > Removing this value gets rid of undesired confusion and avoids the > possibility of accessing registers at this offset even if the 4th array > element is used by accident. > > Signed-off-by: Marijn Suijten > Reviewed-by: AngeloGioacchino Del Regno > Reviewed-by: Daniel Thompson > --- > drivers/video/backlight/qcom-wled.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index 66ce77ee3099..9ec1bdd374d2 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -946,7 +946,7 @@ static const struct wled_config wled3_config_defaults = { > .cs_out_en = false, > .ext_gen = false, > .cabc = false, > - .enabled_strings = {0, 1, 2, 3}, > + .enabled_strings = {0, 1, 2}, > }; > > static int wled4_setup(struct wled *wled) > -- > 2.33.0 >
Re: [PATCH 07/10] backlight: qcom-wled: Provide enabled_strings default for wled 4 and 5
On Mon, Oct 04, 2021 at 09:27:38PM +0200, Marijn Suijten wrote: > Only wled 3 sets a sensible default that allows operating this driver > with just qcom,num-strings in the DT; wled 4 and 5 require > qcom,enabled-strings to be provided otherwise enabled_strings remains > zero-initialized, resuling in every string-specific register write > (currently only the setup and config functions, brightness follows in a > future patch) to only configure the zero'th string multiple times. > > Signed-off-by: Marijn Suijten > Reviewed-by: AngeloGioacchino Del Regno > Reviewed-by: Daniel Thompson
Re: [PATCH 08/10] backlight: qcom-wled: Remove unnecessary double whitespace
On Mon, Oct 04, 2021 at 09:27:39PM +0200, Marijn Suijten wrote: > Remove redundant spaces inside for loop conditions. No other double > spaces were found that are not part of indentation with `[^\s] `. > > Signed-off-by: Marijn Suijten > Reviewed-by: AngeloGioacchino Del Regno > Reviewed-by: Daniel Thompson
Re: [Intel-gfx] [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts
On 01/10/2021 16:48, Matthew Brost wrote: On Fri, Oct 01, 2021 at 09:40:19AM +0100, Tvrtko Ursulin wrote: + Daniel as reviewer and maybe merge, avoid falling through cracks at least. Ty, working on push rights myself. I ended up pushing it myself to avoid having a potential crash in the driver for too long. Hope people will not mind. Regards, Tvrtko 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. Yep, checkpatch didn't like this. Will 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. I like a single assignment, let's not overthink this. + 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: Again, ty. Matt Reviewed-by: Tvrtko Ursulin Regards, Tvrtko
Re: [PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness
On Mon, Oct 04, 2021 at 09:27:40PM +0200, Marijn Suijten wrote: > The hardware is capable of controlling any non-contiguous sequence of > LEDs specified in the DT using qcom,enabled-strings as u32 > array, and this also follows from the DT-bindings documentation. The > numbers specified in this array represent indices of the LED strings > that are to be enabled and disabled. > > Its value is appropriately used to setup and enable string modules, but > completely disregarded in the set_brightness paths which only iterate > over the number of strings linearly. > Take an example where only string 2 is enabled with > qcom,enabled_strings=<2>: this string is appropriately enabled but > subsequent brightness changes would have only touched the zero'th > brightness register because num_strings is 1 here. This is simply > addressed by looking up the string for this index in the enabled_strings > array just like the other codepaths that iterate over num_strings. This isn't true until patch 10 is applied! Given both patches fix the same issue in different functions I'd prefer these to be squashed together (and doubly so because the autodetect code uses set_brightness() as a helper function). Daniel.
Re: [PATCH 03/10] backlight: qcom-wled: Override num-strings when enabled-strings is set
On Mon, Oct 04, 2021 at 09:27:34PM +0200, Marijn Suijten wrote: > DT-bindings do not specify num-strings as mandatory property, yet it is > required to be specified even if enabled-strings is used. The length of > that property-array should already be enough to determine exactly which > and how many strings to enable. > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") > Signed-off-by: Marijn Suijten > Reviewed-by: AngeloGioacchino Del Regno > Reviewed-by: Daniel Thompson
[PATCH v1] drm/msm: use compatible string to find mdp node
In the current implementation, substring comparison using device node name is used to find mdp node during driver probe. Use compatible string instead of node name to get mdp node from the parent mdss node. Signed-off-by: Krishna Manikandan --- drivers/gpu/drm/msm/msm_drv.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 2e6fc18..50a23cf 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -1241,9 +1241,16 @@ static int add_components_mdp(struct device *mdp_dev, return 0; } -static int compare_name_mdp(struct device *dev, void *data) +static int find_mdp_node(struct device *dev, void *data) { - return (strstr(dev_name(dev), "mdp") != NULL); + return of_device_is_compatible(dev->of_node, "qcom,mdp4") || + of_device_is_compatible(dev->of_node, "qcom,mdp5") || + of_device_is_compatible(dev->of_node, "qcom,mdss_mdp") || + of_device_is_compatible(dev->of_node, "qcom,sdm845-dpu") || + of_device_is_compatible(dev->of_node, "qcom,sm8150-dpu") || + of_device_is_compatible(dev->of_node, "qcom,sm8250-dpu") || + of_device_is_compatible(dev->of_node, "qcom,sc7180-dpu") || + of_device_is_compatible(dev->of_node, "qcom,sc7280-dpu"); } static int add_display_components(struct platform_device *pdev, @@ -1268,7 +1275,7 @@ static int add_display_components(struct platform_device *pdev, return ret; } - mdp_dev = device_find_child(dev, NULL, compare_name_mdp); + mdp_dev = device_find_child(dev, NULL, find_mdp_node); if (!mdp_dev) { DRM_DEV_ERROR(dev, "failed to find MDSS MDP node\n"); of_platform_depopulate(dev); -- 2.7.4
Re: [PATCH v1 2/4] arm64: dts: qcom: sc7280: add display dt nodes
On 2021-10-05 07:21, Stephen Boyd wrote: Quoting mkri...@codeaurora.org (2021-09-30 23:39:07) On 2021-09-30 23:28, Stephen Boyd wrote: > Quoting mkri...@codeaurora.org (2021-09-30 04:56:59) >> On 2021-08-19 01:27, Stephen Boyd wrote: >> > Quoting Krishna Manikandan (2021-08-18 03:27:02) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> index 53a21d0..fd7ff1c 100644 >> >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> + >> >> + status = "disabled"; >> >> + >> >> + mdp: mdp@ae01000 { >> > >> > display-controller@ae01000 >> >> Stephen, >> In the current driver code, there is a substring comparison for >> "mdp" >> in device node name as part of probe sequence. If "mdp" is not present >> in the node name, it will >> return an error resulting in probe failure. Can we continue using >> mdp >> as nodename instead of display controller? >> > > Can we fix the driver to not look for node names and look for > compatible > strings instead? It took me a minute to find compare_name_mdp() in > drivers/gpu/drm/msm/msm_drv.c to understand what you're talking about. > Perhaps looking for qcom,mdp5 in there will be sufficient instead of > looking at the node name. Sure Stephen. I will make the necessary changes in msm_drv.c to look for compatible string instead of node name. Can I include these two changes (changing mdp--> display controller and msm_drv.c changes) in a separate series ? Sure. So you'll send the drm driver change now and we'll get the DT change after that with the more generic node name? Yes Stephen.I have raised the change to fix the driver issue. https://patchwork.kernel.org/project/linux-arm-msm/patch/1633427071-19523-1-git-send-email-mkri...@codeaurora.org/ Regards, Krishna
Re: [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT
On 2021-10-05 10:14:52, Daniel Thompson wrote: > On Mon, Oct 04, 2021 at 09:27:35PM +0200, Marijn Suijten wrote: > > The strings passed in DT may possibly cause out-of-bounds register > > accesses and should be validated before use. > > > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for > > WLED3") > > The first half of this patch actually fixes patch 1 from this patch set. > It would be better to move that code there. It only helps guarding against a maximum of 3 leds for WLED3, while using string_len instead of an unintentional sizeof(u32) (resulting in a fixed size of 4) is a different issue requiring a separate patch to fix. Would it help to reorder this patch before 1/10, and mention in patch 1/10 (then 2/10) that, besides properly using string_len instead of hardcoded 4 (which causes wrong reads from DT on top of this), it relies on the previous patch to prevent against an array longer than 3 for WLED3? - Marijn > Daniel. > > > > Signed-off-by: Marijn Suijten > > Reviewed-by: AngeloGioacchino Del Regno > > > > --- > > drivers/video/backlight/qcom-wled.c | 14 ++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/video/backlight/qcom-wled.c > > b/drivers/video/backlight/qcom-wled.c > > index 29910e603c42..27e8949c7922 100644 > > --- a/drivers/video/backlight/qcom-wled.c > > +++ b/drivers/video/backlight/qcom-wled.c > > @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled) > > "qcom,enabled-strings", > > sizeof(u32)); > > if (string_len > 0) { > > + if (string_len > wled->max_string_count) { > > + dev_err(dev, "Cannot have more than %d strings\n", > > + wled->max_string_count); > > + return -EINVAL; > > + } > > + > > rc = of_property_read_u32_array(dev->of_node, > > "qcom,enabled-strings", > > wled->cfg.enabled_strings, > > @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled) > > return -EINVAL; > > } > > > > + for (i = 0; i < string_len; ++i) { > > + if (wled->cfg.enabled_strings[i] >= > > wled->max_string_count) { > > + dev_err(dev, "qcom,enabled-strings index %d at > > %d is out of bounds\n", > > + wled->cfg.enabled_strings[i], i); > > + return -EINVAL; > > + } > > + } > > + > > cfg->num_strings = string_len; > > } > > > > -- > > 2.33.0 > >
Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On 2021-10-05 10:19:47, Daniel Thompson wrote: > On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote: > > When not specifying num-strings in the DT the default is used, but +1 is > > added to it which turns wled3 into 4 and wled4/5 into 5 strings instead > > of 3 and 4 respectively, causing out of bounds reads and register > > read/writes. This +1 exists for a deficiency in the DT parsing code, > > and is simply omitted entirely - solving this oob issue - by allowing > > one extra iteration of the wled_var_cfg function parsing this particular > > property. > > > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver") > > Signed-off-by: Marijn Suijten > > Reviewed-by: AngeloGioacchino Del Regno > > > > --- > > drivers/video/backlight/qcom-wled.c | 8 +++- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/video/backlight/qcom-wled.c > > b/drivers/video/backlight/qcom-wled.c > > index 27e8949c7922..66ce77ee3099 100644 > > --- a/drivers/video/backlight/qcom-wled.c > > +++ b/drivers/video/backlight/qcom-wled.c > > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = { > > > > static u32 wled3_num_strings_values_fn(u32 idx) > > { > > - return idx + 1; > > + return idx; > > } > > > > static const struct wled_var_cfg wled3_num_strings_cfg = { > > .fn = wled3_num_strings_values_fn, > > - .size = 3, > > + .size = 4, /* [0, 3] */ > > 0 is not a valid value for this property. These comments represent the possible loop iterations the DT "cfg parser" runs through, starting at j=0 and running up until and including j=3. Should I make that more clear or omit these comments entirely? - Marijn > > }; > > > > static const struct wled_var_cfg wled4_num_strings_cfg = { > > .fn = wled3_num_strings_values_fn, > > - .size = 4, > > + .size = 5, /* [0, 4] */ > > Ditto. > > > > }; > > > > static u32 wled3_switch_freq_values_fn(u32 idx) > > @@ -1520,8 +1520,6 @@ static int wled_configure(struct wled *wled) > > *bool_opts[i].val_ptr = true; > > } > > > > - cfg->num_strings = cfg->num_strings + 1; > > - > > string_len = of_property_count_elems_of_size(dev->of_node, > > "qcom,enabled-strings", > > sizeof(u32)); > > -- > > 2.33.0 > >
Re: [PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness
On 2021-10-05 10:33:31, Daniel Thompson wrote: > On Mon, Oct 04, 2021 at 09:27:40PM +0200, Marijn Suijten wrote: > > The hardware is capable of controlling any non-contiguous sequence of > > LEDs specified in the DT using qcom,enabled-strings as u32 > > array, and this also follows from the DT-bindings documentation. The > > numbers specified in this array represent indices of the LED strings > > that are to be enabled and disabled. > > > > Its value is appropriately used to setup and enable string modules, but > > completely disregarded in the set_brightness paths which only iterate > > over the number of strings linearly. > > Take an example where only string 2 is enabled with > > qcom,enabled_strings=<2>: this string is appropriately enabled but > > subsequent brightness changes would have only touched the zero'th > > brightness register because num_strings is 1 here. This is simply > > addressed by looking up the string for this index in the enabled_strings > > array just like the other codepaths that iterate over num_strings. > > This isn't true until patch 10 is applied! Patch 9 and 10 were split up at a last resort to prevent a clash in the title, apologies for that. > Given both patches fix the same issue in different functions I'd prefer > these to be squashed together (and doubly so because the autodetect code > uses set_brightness() as a helper function). That's a fair reason, and solution I agree on. I'll figure out how to generify the title and re-spin this patchset except if there are other reviewers/maintainers I should wait for. - Marijn > Daniel.
Re: [Intel-gfx] [PATCH 25/33] drm/i915/guc: Support request cancellation
On 05/10/2021 08:06, Sebastian Andrzej Siewior wrote: On 2021-07-27 12:15:59 [-0700], Daniele Ceraolo Spurio wrote: On 7/26/2021 5:23 PM, Matthew Brost wrote: This adds GuC backend support for i915_request_cancel(), which in turn makes CONFIG_DRM_I915_REQUEST_TIMEOUT work. Reviewed-by: Daniele Ceraolo Spurio I have a few instances of ODEBUG warnings since this commit 62eaf0ae217d4 ("drm/i915/guc: Support request cancellation") like: | [ cut here ] | ODEBUG: init destroyed (active state 0) object type: i915_sw_fence hint: sw_fence_dummy_notify+0x0/0x10 | WARNING: CPU: 0 PID: 987 at lib/debugobjects.c:505 debug_print_object+0x6e/0x90 | Modules linked in: | CPU: 0 PID: 987 Comm: Xorg Not tainted 5.15.0-rc4+ #67 | Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.10 04/24/2012 | RIP: 0010:debug_print_object+0x6e/0x90 … | Call Trace: | i915_sw_fence_reinit+0x10/0x40 | intel_context_init+0x185/0x1e0 | intel_context_create+0x2e/0x100 | default_engines+0x9d/0x120 | i915_gem_create_context+0x40a/0x5d0 | ? trace_kmalloc+0x29/0xd0 | ? kmem_cache_alloc_trace+0xdd/0x190 | i915_gem_context_open+0x140/0x1c0 | i915_gem_open+0x70/0xa0 | drm_file_alloc+0x1af/0x270 | drm_open+0xdc/0x270 | drm_stub_open+0xa6/0x130 | chrdev_open+0xbe/0x250 | ? cdev_device_add+0x80/0x80 | do_dentry_open+0x15e/0x390 | path_openat+0x76b/0xa60 | do_filp_open+0xa4/0x150 | ? lock_release+0x149/0x2f0 | ? _raw_spin_unlock+0x24/0x40 | do_sys_openat2+0x92/0x160 | __x64_sys_openat+0x4f/0x90 | do_syscall_64+0x3b/0xc0 | entry_SYSCALL_64_after_hwframe+0x44/0xae | RIP: 0033:0x7f91b5cfdf07 and: | ODEBUG: activate destroyed (active state 0) object type: i915_sw_fence hint: sw_fence_dummy_notify+0x0/0x10 | WARNING: CPU: 0 PID: 987 at lib/debugobjects.c:505 debug_print_object+0x6e/0x90 | | Call Trace: | debug_object_activate+0x174/0x200 | i915_sw_fence_commit+0x10/0x20 | intel_context_init+0x18d/0x1e0 | intel_context_create+0x2e/0x100 | default_engines+0x9d/0x120 --- | ODEBUG: active_state destroyed (active state 0) object type: i915_sw_fence hint: sw_fence_dummy_notify+0x0/0x10 | WARNING: CPU: 0 PID: 987 at lib/debugobjects.c:505 debug_print_object+0x6e/0x90 | Call Trace: | __i915_sw_fence_complete+0x6f/0x280 | intel_context_init+0x18d/0x1e0 | intel_context_create+0x2e/0x100 | default_engines+0x9d/0x120 Is this known? This is yesterday's -rc4, I first noticed it in -rc3. Needs this fix: commit d576b31bdece7b5034047cbe21170e948198d32f Author: Matthew Auld Date: Fri Sep 24 15:46:46 2021 +0100 drm/i915: remember to call i915_sw_fence_fini But in the fix we forgot to add: Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation") So not sure if it will appear on it's own. Adding Joonas and Rodrigo for maintainer level help. Regards, Tvrtko
Re: [RFC PATCH v2 2/2] RDMA/rxe: Add dma-buf support
ping 2021年10月1日(金) 12:56 Shunsuke Mie : > > 2021年9月30日(木) 23:41 Daniel Vetter : > > > > On Wed, Sep 29, 2021 at 01:19:05PM +0900, Shunsuke Mie wrote: > > > Implement a ib device operation ‘reg_user_mr_dmabuf’. Generate a > > > rxe_map from the memory space linked the passed dma-buf. > > > > > > Signed-off-by: Shunsuke Mie > > > --- > > > drivers/infiniband/sw/rxe/rxe_loc.h | 2 + > > > drivers/infiniband/sw/rxe/rxe_mr.c| 118 ++ > > > drivers/infiniband/sw/rxe/rxe_verbs.c | 34 > > > drivers/infiniband/sw/rxe/rxe_verbs.h | 2 + > > > 4 files changed, 156 insertions(+) > > > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h > > > b/drivers/infiniband/sw/rxe/rxe_loc.h > > > index 1ca43b859d80..8bc19ea1a376 100644 > > > --- a/drivers/infiniband/sw/rxe/rxe_loc.h > > > +++ b/drivers/infiniband/sw/rxe/rxe_loc.h > > > @@ -75,6 +75,8 @@ u8 rxe_get_next_key(u32 last_key); > > > void rxe_mr_init_dma(struct rxe_pd *pd, int access, struct rxe_mr *mr); > > > int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > > >int access, struct rxe_mr *mr); > > > +int rxe_mr_dmabuf_init_user(struct rxe_pd *pd, int fd, u64 start, u64 > > > length, > > > + u64 iova, int access, struct rxe_mr *mr); > > > int rxe_mr_init_fast(struct rxe_pd *pd, int max_pages, struct rxe_mr > > > *mr); > > > int rxe_mr_copy(struct rxe_mr *mr, u64 iova, void *addr, int length, > > > enum rxe_mr_copy_dir dir); > > > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c > > > b/drivers/infiniband/sw/rxe/rxe_mr.c > > > index 53271df10e47..af6ef671c3a5 100644 > > > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > > > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > > > @@ -4,6 +4,7 @@ > > > * Copyright (c) 2015 System Fabric Works, Inc. All rights reserved. > > > */ > > > > > > +#include > > > #include "rxe.h" > > > #include "rxe_loc.h" > > > > > > @@ -245,6 +246,120 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, > > > u64 length, u64 iova, > > > return err; > > > } > > > > > > +static int rxe_map_dmabuf_mr(struct rxe_mr *mr, > > > + struct ib_umem_dmabuf *umem_dmabuf) > > > +{ > > > + struct rxe_map_set *set; > > > + struct rxe_phys_buf *buf = NULL; > > > + struct rxe_map **map; > > > + void *vaddr, *vaddr_end; > > > + int num_buf = 0; > > > + int err; > > > + size_t remain; > > > + > > > + mr->dmabuf_map = kzalloc(sizeof &mr->dmabuf_map, GFP_KERNEL); > > > > dmabuf_maps are just tagged pointers (and we could shrink them to actually > > just a tagged pointer if anyone cares about the overhead of the separate > > bool), allocating them seperately is overkill. > > I agree with you. However, I think it is needed to unmap by > dma_buf_vunmap(). If there is another simple way to unmap it. It is not > needed I think. What do you think about it? > > > > + if (!mr->dmabuf_map) { > > > + err = -ENOMEM; > > > + goto err_out; > > > + } > > > + > > > + err = dma_buf_vmap(umem_dmabuf->dmabuf, mr->dmabuf_map); > > > + if (err) > > > + goto err_free_dmabuf_map; > > > + > > > + set = mr->cur_map_set; > > > + set->page_shift = PAGE_SHIFT; > > > + set->page_mask = PAGE_SIZE - 1; > > > + > > > + map = set->map; > > > + buf = map[0]->buf; > > > + > > > + vaddr = mr->dmabuf_map->vaddr; > > > > dma_buf_map can be an __iomem too, you shouldn't dig around in this, but > > use the dma-buf-map.h helpers instead. On x86 (and I think also on most > > arm) it doesn't matter, but it's kinda not very nice in a pure software > > driver. > > > > If anything is missing in dma-buf-map.h wrappers just add more. > > > > Or alternatively you need to fail the import if you can't handle __iomem. > > > > Aside from these I think the dma-buf side here for cpu access looks > > reasonable now. > > -Daniel > I'll see the dma-buf-map.h and consider the error handling that you suggested. > I appreciate your support. > > Thanks a lot, > Shunsuke. > > > > + vaddr_end = vaddr + umem_dmabuf->dmabuf->size; > > > + remain = umem_dmabuf->dmabuf->size; > > > + > > > + for (; remain; vaddr += PAGE_SIZE) { > > > + if (num_buf >= RXE_BUF_PER_MAP) { > > > + map++; > > > + buf = map[0]->buf; > > > + num_buf = 0; > > > + } > > > + > > > + buf->addr = (uintptr_t)vaddr; > > > + if (remain >= PAGE_SIZE) > > > + buf->size = PAGE_SIZE; > > > + else > > > + buf->size = remain; > > > + remain -= buf->size; > > > + > > > + num_buf++; > > > + buf++; > > > + } > > > + > > > + return 0; > > > + > > > +err_free_dmabuf_map: > > > + kfree(mr->dmabuf_map); > > > +err_out: > > > + return err; > > > +} > > > + > > > +static void rxe_unmap_dma
Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge
Hi Andrzej Sorry, I'm just coming back to this. I'd started this reply a while back, but got sidetracked onto other priorities and not sent it. On Wed, 8 Sept 2021 at 22:14, Andrzej Hajda wrote: > > > W dniu 08.09.2021 o 13:11, Dave Stevenson pisze: > > Hi Marek and Andrzej > > > > On Tue, 7 Sept 2021 at 22:24, Marek Vasut wrote: > >> On 9/7/21 7:29 PM, Andrzej Hajda wrote: > >>> W dniu 07.09.2021 o 16:25, Marek Vasut pisze: > On 9/7/21 9:31 AM, Andrzej Hajda wrote: > > On 07.09.2021 04:39, Marek Vasut wrote: > >> In rare cases, the bridge may not start up correctly, which usually > >> leads to no display output. In case this happens, warn about it in > >> the kernel log. > >> > >> Signed-off-by: Marek Vasut > >> Cc: Jagan Teki > >> Cc: Laurent Pinchart > >> Cc: Linus Walleij > >> Cc: Robert Foss > >> Cc: Sam Ravnborg > >> Cc: dri-devel@lists.freedesktop.org > >> --- > >> NOTE: See the following: > >> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video > >> > >> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533 > >> > >> --- > >> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 + > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > >> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > >> index a32f70bc68ea4..4ea71d7f0bfbc 100644 > >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > >> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct > >> drm_bridge *bridge, > >> /* Clear all errors that got asserted during initialization. > >> */ > >> regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > >> regmap_write(ctx->regmap, REG_IRQ_STAT, pval); > > > > It does not look as correct error handling, maybe it would be good to > > analyze and optionally report 'unexpected' errors here as well. > The above is correct -- it clears the status register because the > setup might've set random bits in that register. Then we wait a bit, > let the link run, and read them again to get the real link status in > this new piece of code below, hence the usleep_range there. And then > if the link indicates a problem, we know it is a problem. > >>> > >>> Usually such registers are cleared on very beginning of the > >>> initialization, and tested (via irq handler, or via reading), during > >>> initalization, if initialization phase goes well. If it is not the case > >>> forgive me. > >> The init just flips the bit at random in the IRQ_STAT register, so no, > >> that's not really viable here. That's why we clear them at the end, and > >> then wait a bit, and then check whether something new appeared in them. > >> > >> If not, all is great. > >> > >> Sure, we could generate an IRQ, but then IRQ line is not always > >> connected to this chip on all hardware I have available. So this gives > >> the user at least some indication that something is wrong with their HW. > >> > >> + > >> +usleep_range(1, 12000); > >> +regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > >> +if (pval) > >> +dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); > > > > I am not sure what is the case here but it looks like 'we do not know > > what is going on, so let's add some diagnostic messages to gather info > > and figure it out later'. > That's pretty much the case, see the two links above in the NOTE > section. If something goes wrong, we print the value for the user > (usually developer) so they can fix their problems. We cannot do much > better in the attach callback. > > The issue I ran into (and where this would be helpful information to > me during debugging, since the issue happened real seldom, see also > the NOTE links above) is that the DSI controller driver started > streaming video on the data lanes before the DSI83 had a chance to > initialize. This worked most of the time, except for a few exceptions > here and there, where the video didn't start. This does set link > status bits consistently. In the meantime, I fixed the controller > driver (so far downstream, due to ongoing discussion). > >>> > >>> Maybe drm_connector_set_link_status_property(conn, > >>> DRM_MODE_LINK_STATUS_BAD) would be usefule here. > >> Hmm, this works on connector, the dsi83 is a bridge and it can be stuck > >> between two other bridges. That doesn't seem like the right tool, no ? > >> > > Whole driver lacks IRQ handler which IMO could perform better diagnosis, > > and I guess it could also help in recovery, but this is just my guess. > > So if this patch is enough for now you can add
Re: [PATCH 24/28] drm: use new iterator in drm_gem_plane_helper_prepare_fb
Am 05.10.21 um 09:53 schrieb Tvrtko Ursulin: On 01/10/2021 11:06, Christian König wrote: 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 */ What is the TODO? NB instead? The drm atomic API can unfortunately handle only one fence and we can certainly have more than that. + drm_atomic_set_fence_for_plane(state, fence); + return 0; + } + dma_resv_iter_end(&cursor); + drm_atomic_set_fence_for_plane(state, NULL); dma_resv_iter_begin(&cursor, obj->resv, false); dma_resv_for_each_fence_unlocked(&cursor, fence) { dma_fence_get(fence); break; } dma_resv_iter_end(&cursor); drm_atomic_set_fence_for_plane(state, fence); Does this work? Yeah that should work as well. But overall I am not sure this is nicer. Is the goal to remove dma_resv_get_excl_unlocked as API it just does not happen in this series? Yes, the only user left is the i915_gem_object_last_write_engine() function and that one you already removed in i915. Regards, Christian. Regards, Tvrtko return 0; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
Re: [Intel-gfx] [PATCH 12/26] drm/i915/guc: Implement multi-lrc submission
Hi Matthew, Thank you for the patch! Yet something to improve: [auto build test ERROR on drm-tip/drm-tip] [cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master airlied/drm-next 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/Matthew-Brost/Parallel-submission-aka-multi-bb-execbuf/20211005-061424 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: x86_64-randconfig-a003-20211004 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c0039de2953d15815448b4b3c3bafb45607781e0) 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/3bea3cc438df1105d0d8c1bcc01b96559d4bb78c git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Matthew-Brost/Parallel-submission-aka-multi-bb-execbuf/20211005-061424 git checkout 3bea3cc438df1105d0d8c1bcc01b96559d4bb78c # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 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/i915/gt/uc/intel_guc_submission.c:1486:7: error: variable >> 'ret' is used uninitialized whenever 'if' condition is false >> [-Werror,-Wsometimes-uninitialized] if (multi_lrc_submit(rq)) { ^~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1499:9: note: uninitialized use occurs here return ret; ^~~ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1486:3: note: remove the 'if' if its condition is always true if (multi_lrc_submit(rq)) { ^~ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1479:9: note: initialize the variable 'ret' to silence this warning int ret; ^ = 0 1 error generated. vim +1486 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 1475 1476 static int guc_bypass_tasklet_submit(struct intel_guc *guc, 1477 struct i915_request *rq) 1478 { 1479 int ret; 1480 1481 __i915_request_submit(rq); 1482 1483 trace_i915_request_in(rq, 0); 1484 1485 if (is_multi_lrc_rq(rq)) { > 1486 if (multi_lrc_submit(rq)) { 1487 ret = guc_wq_item_append(guc, rq); 1488 if (!ret) 1489 ret = guc_add_request(guc, rq); 1490 } 1491 } else { 1492 guc_set_lrc_tail(rq); 1493 ret = guc_add_request(guc, rq); 1494 } 1495 1496 if (unlikely(ret == -EPIPE)) 1497 disable_submission(guc); 1498 1499 return ret; 1500 } 1501 --- 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 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On Tue, Oct 05, 2021 at 12:06:06PM +0200, Marijn Suijten wrote: > On 2021-10-05 10:19:47, Daniel Thompson wrote: > > On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote: > > > When not specifying num-strings in the DT the default is used, but +1 is > > > added to it which turns wled3 into 4 and wled4/5 into 5 strings instead > > > of 3 and 4 respectively, causing out of bounds reads and register > > > read/writes. This +1 exists for a deficiency in the DT parsing code, > > > and is simply omitted entirely - solving this oob issue - by allowing > > > one extra iteration of the wled_var_cfg function parsing this particular > > > property. > > > > > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver") > > > Signed-off-by: Marijn Suijten > > > Reviewed-by: AngeloGioacchino Del Regno > > > > > > --- > > > drivers/video/backlight/qcom-wled.c | 8 +++- > > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/video/backlight/qcom-wled.c > > > b/drivers/video/backlight/qcom-wled.c > > > index 27e8949c7922..66ce77ee3099 100644 > > > --- a/drivers/video/backlight/qcom-wled.c > > > +++ b/drivers/video/backlight/qcom-wled.c > > > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = { > > > > > > static u32 wled3_num_strings_values_fn(u32 idx) > > > { > > > - return idx + 1; > > > + return idx; > > > } > > > > > > static const struct wled_var_cfg wled3_num_strings_cfg = { > > > .fn = wled3_num_strings_values_fn, > > > - .size = 3, > > > + .size = 4, /* [0, 3] */ > > > > 0 is not a valid value for this property. > > These comments represent the possible loop iterations the DT "cfg > parser" runs through, starting at j=0 and running up until and including > j=3. Should I make that more clear or omit these comments entirely? The role of wled3_num_strings_values_fn() is to enumerate the list of legal values for the property [ 1, 2, 3 ]. Your changes cause the enumeration to include a non-legal value so that you can have an identity mapping between the symbol and the enumerate value. An alternative approach would be to leave the enumeration logic alone but set the num_string default to UINT_MAX in all cases: - cfg->num_strings = cfg->num_strings + 1; + if (cfg->num_strings == UINT_MAX) + cfg->num_strings = + else + /* Convert from enumerated to numeric form */ + cfg->num_strings = wled3_num_strings_values_fn( + cfg->num_strings); Daniel.
Re: [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT
On Tue, Oct 05, 2021 at 12:03:50PM +0200, Marijn Suijten wrote: > On 2021-10-05 10:14:52, Daniel Thompson wrote: > > On Mon, Oct 04, 2021 at 09:27:35PM +0200, Marijn Suijten wrote: > > > The strings passed in DT may possibly cause out-of-bounds register > > > accesses and should be validated before use. > > > > > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for > > > WLED3") > > > > The first half of this patch actually fixes patch 1 from this patch set. > > It would be better to move that code there. > > It only helps guarding against a maximum of 3 leds for WLED3, while > using string_len instead of an unintentional sizeof(u32) (resulting in > a fixed size of 4) is a different issue requiring a separate patch to > fix. > > Would it help to reorder this patch before 1/10, and mention in patch > 1/10 (then 2/10) that, besides properly using string_len instead of > hardcoded 4 (which causes wrong reads from DT on top of this), it relies > on the previous patch to prevent against an array longer than 3 for > WLED3? Reordering is OK for me. Daniel. > > > Signed-off-by: Marijn Suijten > > > Reviewed-by: AngeloGioacchino Del Regno > > > > > > --- > > > drivers/video/backlight/qcom-wled.c | 14 ++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/video/backlight/qcom-wled.c > > > b/drivers/video/backlight/qcom-wled.c > > > index 29910e603c42..27e8949c7922 100644 > > > --- a/drivers/video/backlight/qcom-wled.c > > > +++ b/drivers/video/backlight/qcom-wled.c > > > @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled) > > >"qcom,enabled-strings", > > >sizeof(u32)); > > > if (string_len > 0) { > > > + if (string_len > wled->max_string_count) { > > > + dev_err(dev, "Cannot have more than %d strings\n", > > > + wled->max_string_count); > > > + return -EINVAL; > > > + } > > > + > > > rc = of_property_read_u32_array(dev->of_node, > > > "qcom,enabled-strings", > > > wled->cfg.enabled_strings, > > > @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled) > > > return -EINVAL; > > > } > > > > > > + for (i = 0; i < string_len; ++i) { > > > + if (wled->cfg.enabled_strings[i] >= > > > wled->max_string_count) { > > > + dev_err(dev, "qcom,enabled-strings index %d at > > > %d is out of bounds\n", > > > + wled->cfg.enabled_strings[i], i); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > cfg->num_strings = string_len; > > > } > > > > > > -- > > > 2.33.0 > > >
Re: [PATCH 24/28] drm: use new iterator in drm_gem_plane_helper_prepare_fb
On 05/10/2021 11:27, Christian König wrote: Am 05.10.21 um 09:53 schrieb Tvrtko Ursulin: On 01/10/2021 11:06, Christian König wrote: 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 */ What is the TODO? NB instead? The drm atomic API can unfortunately handle only one fence and we can certainly have more than that. + drm_atomic_set_fence_for_plane(state, fence); + return 0; + } + dma_resv_iter_end(&cursor); + drm_atomic_set_fence_for_plane(state, NULL); dma_resv_iter_begin(&cursor, obj->resv, false); dma_resv_for_each_fence_unlocked(&cursor, fence) { dma_fence_get(fence); break; } dma_resv_iter_end(&cursor); drm_atomic_set_fence_for_plane(state, fence); Does this work? Yeah that should work as well. But overall I am not sure this is nicer. Is the goal to remove dma_resv_get_excl_unlocked as API it just does not happen in this series? Yes, the only user left is the i915_gem_object_last_write_engine() function and that one you already removed in i915. To me the above feels clumsier than dma_resv_get_excl_unlocked and you can even view it as open coding that helper. So don't know, someone else can have a casting vote. I guess if support for more than one fence is coming soon(-ish) do drm atomic api then I could be convinced the iterator here makes sense today. Regards, Tvrtko Regards, Christian. Regards, Tvrtko return 0; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On Tue, Oct 05, 2021 at 11:38:43AM +0100, Daniel Thompson wrote: > On Tue, Oct 05, 2021 at 12:06:06PM +0200, Marijn Suijten wrote: > > On 2021-10-05 10:19:47, Daniel Thompson wrote: > > > On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote: > > > > When not specifying num-strings in the DT the default is used, but +1 is > > > > added to it which turns wled3 into 4 and wled4/5 into 5 strings instead > > > > of 3 and 4 respectively, causing out of bounds reads and register > > > > read/writes. This +1 exists for a deficiency in the DT parsing code, > > > > and is simply omitted entirely - solving this oob issue - by allowing > > > > one extra iteration of the wled_var_cfg function parsing this particular > > > > property. > > > > > > > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver") > > > > Signed-off-by: Marijn Suijten > > > > Reviewed-by: AngeloGioacchino Del Regno > > > > > > > > --- > > > > drivers/video/backlight/qcom-wled.c | 8 +++- > > > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/video/backlight/qcom-wled.c > > > > b/drivers/video/backlight/qcom-wled.c > > > > index 27e8949c7922..66ce77ee3099 100644 > > > > --- a/drivers/video/backlight/qcom-wled.c > > > > +++ b/drivers/video/backlight/qcom-wled.c > > > > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg > > > > = { > > > > > > > > static u32 wled3_num_strings_values_fn(u32 idx) > > > > { > > > > - return idx + 1; > > > > + return idx; > > > > > > } > > > > > > > > static const struct wled_var_cfg wled3_num_strings_cfg = { > > > > .fn = wled3_num_strings_values_fn, > > > > - .size = 3, > > > > + .size = 4, /* [0, 3] */ > > > > > > 0 is not a valid value for this property. > > > > These comments represent the possible loop iterations the DT "cfg > > parser" runs through, starting at j=0 and running up until and including > > j=3. Should I make that more clear or omit these comments entirely? > > The role of wled3_num_strings_values_fn() is to enumerate the list of > legal values for the property [ 1, 2, 3 ]. Your changes cause the > enumeration to include a non-legal value so that you can have an > identity mapping between the symbol and the enumerate value. > > An alternative approach would be to leave the enumeration logic > alone but set the num_string default to UINT_MAX in all cases: > > - cfg->num_strings = cfg->num_strings + 1; > + if (cfg->num_strings == UINT_MAX) > + cfg->num_strings = Oops... looks like I missed the cfg->max_string_count here. > + else > + /* Convert from enumerated to numeric form */ > + cfg->num_strings = wled3_num_strings_values_fn( > + cfg->num_strings); PS the alternative option is not to treat num-strings as an enumerated value at all and just read it directly without using wled_values()...
Re: [Intel-gfx] [PATCH 25/33] drm/i915/guc: Support request cancellation
On 2021-10-05 11:13:16 [+0100], Tvrtko Ursulin wrote: > Needs this fix: > > commit d576b31bdece7b5034047cbe21170e948198d32f > Author: Matthew Auld > Date: Fri Sep 24 15:46:46 2021 +0100 > > drm/i915: remember to call i915_sw_fence_fini Thanks, works. Needed a tweak since it does not apply as-is. > But in the fix we forgot to add: > > Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation") > > So not sure if it will appear on it's own. Adding Joonas and Rodrigo for > maintainer level help. > > Regards, > > Tvrtko Sebastian
Re: Questions over DSI within DRM.
Hi Laurent On Sun, 3 Oct 2021 at 15:16, Laurent Pinchart wrote: > > Hello, > > Reviving a bit of an old thread. I'd been looking at reviving this conversation too as I've moved further on with DSI on the Pi, and converting from an encoder to a bridge > On Thu, Jul 15, 2021 at 11:50:22AM +0200, Maxime Ripard wrote: > > On Tue, Jul 06, 2021 at 05:44:58PM +0100, Dave Stevenson wrote: > > > On Tue, 6 Jul 2021 at 16:13, Maxime Ripard wrote: > > > > > > > On a similar theme, some devices want the clock lane in HS mode > > > > > > > early > > > > > > > so they can use it in place of an external oscillator, but the > > > > > > > data > > > > > > > lanes still in LP-11. There appears to be no way for the > > > > > > > display/bridge to signal this requirement or it be achieved. > > > > > > > > > > > > You're right. A lng time ago, the omapdrm driver had an internal > > > > > > infrastructure that didn't use drm_bridge or drm_panel and instead > > > > > > required omapdrm-specific drivers for those components. It used to > > > > > > model > > > > > > the display pipeline in a different way than drm_bridge, with the > > > > > > sync > > > > > > explicitly setting the source state. A DSI sink could thus control > > > > > > its > > > > > > enable sequence, interleaving programming of the sink with control > > > > > > of > > > > > > the source. > > > > > > > > > > > > Migrating omapdrm to the drm_bridge model took a really large > > > > > > effort, > > > > > > which makes me believe that transitioning the whole subsystem to > > > > > > sink-controlled sources would be close to impossible. We could add > > > > > > DSI-specific operations, or add another enable bridge operation > > > > > > (post_pre_enable ? :-D). Neither would scale, but it may be enough. > > > > > > > > > > I haven't thought it through for all generic cases, but I suspect it's > > > > > more a pre_pre_enable that is needed to initialise the PHY etc, > > > > > probably from source to sink. > > I believe it could be implemented as a pre-pre-enable indeed. It feels > like a bit of a hack, as the next time we need more fine-grained control > over the startup sequence, we'll have to add a pre-pre-pre-enable. Given > that the startup sequence requirements come from the sink device, it > would make sense to let it explicitly control the initialization, > instead of driving it from the source. I don't think we'll be able to > rework the bridge API in that direction though, so I'm fine with a hack. There are pros and cons to both approaches. You're in a much better place to make that sort of call than I am, so I'll take your advice. Implementing a DSI host op function may mean an update to a number of existing DSI host drivers, but it would be cleaner. It's also what Andrzej has suggested. Thinking it through, a function that requests clock lane frequency and state (ULPS or LP-11 predominantly), and data lane state (again ULPS or LP-11) should allow the required behaviour for most of the bridges I'm aware of. Most want either LP-11, or HS clock at a known frequency. Giving the option for setting back into ULPS also allows for power saving/standby mechanisms should the need arise. Does it need a way to pass back the actual DSI frequency being used, in a similar vein to mode_fixup? That allows for the bridge to request the display clock, but the burst mode link frequency to be returned (I'm assuming that's a property of the DSI host only, and not the bridge). I'm having a discussion with someone who wants to run SN65DSI85 in the two independent LVDS display mode. That requires a DSI HS clock on DSI-A even if only panel B is active, so with this extra function that would be achievable as well. Thoughts? > > > > > If the panel/bridge can set a flag that can be checked at this point > > > > > for whether an early clock is required or not, I think that allows us > > > > > to comply with the requirements for a large number of panels/bridges > > > > > (LP-11 vs HS config for clock and or data lanes before pre_enable is > > > > > called). > > > > > > > > > > pre_enable retains the current behaviour (initialise the chain from > > > > > sink to source). > > > > > enable then actually starts sending video and enabling outputs. > > > > > > > > Flags indeed seem like a more contained option. Another one could be to > > > > have a mipi_dsi_host to (for example) power up the clock lane that would > > > > be called by default before the bridge's enable, or at the downstream > > > > bridge driver discretion before that. > > > > > > Which driver will that call? > > > > The parent DSI Host > > > > > An extreme example perhaps, but Toshiba make the TC358860 eDP to DSI > > > bridge chip[1]. So the encoder will be eDP, but the DSI config needs > > > to go to that bridge. Does that happen automatically within the > > > framework? I guess so as the bridge will have called > > > mipi_dsi_host_register for the DSI sink to attach to. > > > > In that case, whatever sink
Re: [PATCH 24/28] drm: use new iterator in drm_gem_plane_helper_prepare_fb
Am 05.10.21 um 12:47 schrieb Tvrtko Ursulin: On 05/10/2021 11:27, Christian König wrote: Am 05.10.21 um 09:53 schrieb Tvrtko Ursulin: On 01/10/2021 11:06, Christian König wrote: 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 */ What is the TODO? NB instead? The drm atomic API can unfortunately handle only one fence and we can certainly have more than that. + drm_atomic_set_fence_for_plane(state, fence); + return 0; + } + dma_resv_iter_end(&cursor); + drm_atomic_set_fence_for_plane(state, NULL); dma_resv_iter_begin(&cursor, obj->resv, false); dma_resv_for_each_fence_unlocked(&cursor, fence) { dma_fence_get(fence); break; } dma_resv_iter_end(&cursor); drm_atomic_set_fence_for_plane(state, fence); Does this work? Yeah that should work as well. But overall I am not sure this is nicer. Is the goal to remove dma_resv_get_excl_unlocked as API it just does not happen in this series? Yes, the only user left is the i915_gem_object_last_write_engine() function and that one you already removed in i915. To me the above feels clumsier than dma_resv_get_excl_unlocked and you can even view it as open coding that helper. So don't know, someone else can have a casting vote. I guess if support for more than one fence is coming soon(-ish) do drm atomic api then I could be convinced the iterator here makes sense today. Well Daniel noted that the drm atomic API needs some more work here because we don't handle different fences for different planes correctly either. We could as well postpone this change and fix the atomic functions first. Regards, Christian. Regards, Tvrtko Regards, Christian. Regards, Tvrtko return 0; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
[PATCH] drm/i915: Handle Intel igfx + Intel dgfx hybrid graphics setup
From: Tvrtko Ursulin In short this makes i915 work for hybrid setups (DRI_PRIME=1 with Mesa) when rendering is done on Intel dgfx and scanout/composition on Intel igfx. Before this patch the driver was not quite ready for that setup, mainly because it was able to emit a semaphore wait between the two GPUs, which results in deadlocks because semaphore target location in HWSP is neither shared between the two, nor mapped in both GGTT spaces. To fix it the patch adds an additional check to a couple of relevant code paths in order to prevent using semaphores for inter-engine synchronisation when relevant objects are not in the same GGTT space. v2: * Avoid adding rq->i915. (Chris) v3: * Use GGTT which describes the limit more precisely. Signed-off-by: Tvrtko Ursulin Cc: Daniel Vetter Cc: Matthew Auld Cc: Thomas Hellström --- drivers/gpu/drm/i915/i915_request.c | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 79da5eca60af..4f189982f67e 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -1145,6 +1145,12 @@ __emit_semaphore_wait(struct i915_request *to, return 0; } +static bool +can_use_semaphore_wait(struct i915_request *to, struct i915_request *from) +{ + return to->engine->gt->ggtt == from->engine->gt->ggtt; +} + static int emit_semaphore_wait(struct i915_request *to, struct i915_request *from, @@ -1153,6 +1159,9 @@ emit_semaphore_wait(struct i915_request *to, const intel_engine_mask_t mask = READ_ONCE(from->engine)->mask; struct i915_sw_fence *wait = &to->submit; + if (!can_use_semaphore_wait(to, from)) + goto await_fence; + if (!intel_context_use_semaphores(to->context)) goto await_fence; @@ -1256,7 +1265,8 @@ __i915_request_await_execution(struct i915_request *to, * immediate execution, and so we must wait until it reaches the * active slot. */ - if (intel_engine_has_semaphores(to->engine) && + if (can_use_semaphore_wait(to, from) && + intel_engine_has_semaphores(to->engine) && !i915_request_has_initial_breadcrumb(to)) { err = __emit_semaphore_wait(to, from, from->fence.seqno - 1); if (err < 0) -- 2.30.2
[PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v8
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 v8: fix index check in dma_resv_iter_is_exclusive() Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin (v7) --- 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..
Deploying new iterator interface for dma-buf
Hi guys, a few more bug fixes, looks like the more selftests I add the more odies I find. Assuming the CI tests now pass I will start pushing patches I've already got an rb for to drm-misc-next. Please review and/or comment, Christian.
[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 1a43bef03af3..220c40dc5c11 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -614,74 +614,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 02/28] dma-buf: add dma_resv_for_each_fence v2
A simpler version of the iterator to be used when the dma_resv object is locked. v2: fix index check here as well Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 49 ++ include/linux/dma-resv.h | 19 +++ 2 files changed, 68 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 3cbcf66a137e..231bae173ef1 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -423,6 +423,55 @@ 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 = 0; + 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) +{ + unsigned int idx; + + dma_resv_assert_held(cursor->obj); + + cursor->is_restarted = false; + if (!cursor->all_fences || !cursor->fences || + cursor->index >= cursor->fences->shared_count) + return NULL; + + idx = cursor->index++; + return rcu_dereference_protected(cursor->fences->shared[idx], +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 764138ad8583..3df7ef23712d 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 03/28] dma-buf: add dma_resv selftest v3
Just exercising a very minor subset of the functionality, but already proven useful. v2: add missing locking v3: some more cleanup and consolidation, add unlocked test as well Signed-off-by: Christian König --- drivers/dma-buf/Makefile | 3 +- drivers/dma-buf/selftests.h | 1 + drivers/dma-buf/st-dma-resv.c | 282 ++ 3 files changed, 285 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 ..50d3791ccb8c --- /dev/null +++ b/drivers/dma-buf/st-dma-resv.c @@ -0,0 +1,282 @@ +/* SPDX-License-Identifier: MIT */ + +/* +* Copyright © 2019 Intel Corporation +* Copyright © 2021 Advanced Micro Devices, Inc. +*/ + +#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_resv resv; + struct dma_fence *f; + int r; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_fence_signal(f); + dma_fence_put(f); + + dma_resv_init(&resv); + r = dma_resv_lock(&resv, NULL); + if (r) + pr_err("Resv locking failed\n"); + else + dma_resv_unlock(&resv); + dma_resv_fini(&resv); + return r; +} + +static int test_signaling(void *arg, bool shared) +{ + struct dma_resv resv; + struct dma_fence *f; + int r; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_resv_init(&resv); + r = dma_resv_lock(&resv, NULL); + if (r) { + pr_err("Resv locking failed\n"); + goto err_free; + } + + if (shared) { + r = dma_resv_reserve_shared(&resv, 1); + if (r) { + pr_err("Resv shared slot allocation failed\n"); + goto err_unlock; + } + + dma_resv_add_shared_fence(&resv, f); + } else { + dma_resv_add_excl_fence(&resv, f); + } + + if (dma_resv_test_signaled(&resv, shared)) { + pr_err("Resv unexpectedly signaled\n"); + r = -EINVAL; + goto err_unlock; + } + dma_fence_signal(f); + if (!dma_resv_test_signaled(&resv, shared)) { + pr_err("Resv not reporting signaled\n"); + r = -EINVAL; + goto err_unlock; + } +err_unlock: + dma_resv_unlock(&resv); +err_free: + dma_resv_fini(&resv); + dma_fence_put(f); + return r; +} + +static int test_excl_signaling(void *arg) +{ + return test_signaling(arg, false); +} + +static int test_shared_signaling(void *arg) +{ + return test_signaling(arg, true); +} + +static int test_for_each(void *arg, bool shared) +{ + struct dma_resv_iter cursor; + struct dma_fence *f, *fence; + struct dma_resv resv; + int r; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_resv_init(&resv); + r = dma_resv_lock(&resv, NULL); + if (r) { + pr_err("Resv locking failed\n"); + goto err_free; + } + + if (shared) { + r = dma_resv_reserve_shared(&resv, 1); + if (r) { + pr_err("Resv shared slot allocation failed\n"); + goto err_unlock; + } + + dma_resv_add_shared_fence(&resv, f); + } else { + dma_resv_add_excl_fence(&resv, f); + } + + r = -ENOENT; + dma_resv_for_ea
[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 e5ea42df0c6b..1a43bef03af3 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -538,99 +538,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 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 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 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 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 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 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 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 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 09/28] dma-buf: use the new iterator in dma_resv_poll
Simplify the code a bit. Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin --- 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 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 220c40dc5c11..41c2e951213c 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -633,22 +633,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. @@ -665,43 +649,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 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 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 08/28] dma-buf: use the new iterator in dma_buf_debug_show
Simplifying the code a bit. Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin --- 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 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 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 231bae173ef1..e5ea42df0c6b 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -481,74 +481,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 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 Reviewed-by: Tvrtko Ursulin --- 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
[PATCH 24/28] drm: use new iterator in drm_gem_plane_helper_prepare_fb v2
Makes the handling a bit more complex, but avoids the use of dma_resv_get_excl_unlocked(). v2: improve coding and documentation 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..8534f78d4d6d 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) { + /* TODO: We only use the first write fence here and need to fix +* the drm_atomic_set_fence_for_plane() API to accept more than +* one. */ + dma_fence_get(fence); + break; + } + dma_resv_iter_end(&cursor); + drm_atomic_set_fence_for_plane(state, fence); return 0; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); -- 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 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 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 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 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
Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On 2021-10-05 11:53:12, Daniel Thompson wrote: > On Tue, Oct 05, 2021 at 11:38:43AM +0100, Daniel Thompson wrote: > > On Tue, Oct 05, 2021 at 12:06:06PM +0200, Marijn Suijten wrote: > > > On 2021-10-05 10:19:47, Daniel Thompson wrote: > > > > On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote: > > > > > When not specifying num-strings in the DT the default is used, but +1 > > > > > is > > > > > added to it which turns wled3 into 4 and wled4/5 into 5 strings > > > > > instead > > > > > of 3 and 4 respectively, causing out of bounds reads and register > > > > > read/writes. This +1 exists for a deficiency in the DT parsing code, > > > > > and is simply omitted entirely - solving this oob issue - by allowing > > > > > one extra iteration of the wled_var_cfg function parsing this > > > > > particular > > > > > property. > > > > > > > > > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver") > > > > > Signed-off-by: Marijn Suijten > > > > > Reviewed-by: AngeloGioacchino Del Regno > > > > > > > > > > --- > > > > > drivers/video/backlight/qcom-wled.c | 8 +++- > > > > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/video/backlight/qcom-wled.c > > > > > b/drivers/video/backlight/qcom-wled.c > > > > > index 27e8949c7922..66ce77ee3099 100644 > > > > > --- a/drivers/video/backlight/qcom-wled.c > > > > > +++ b/drivers/video/backlight/qcom-wled.c > > > > > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg > > > > > wled5_ovp_cfg = { > > > > > > > > > > static u32 wled3_num_strings_values_fn(u32 idx) > > > > > { > > > > > - return idx + 1; > > > > > + return idx; > > > > > } > > > > > > > > > > static const struct wled_var_cfg wled3_num_strings_cfg = { > > > > > .fn = wled3_num_strings_values_fn, > > > > > - .size = 3, > > > > > + .size = 4, /* [0, 3] */ > > > > > > > > 0 is not a valid value for this property. > > > > > > These comments represent the possible loop iterations the DT "cfg > > > parser" runs through, starting at j=0 and running up until and including > > > j=3. Should I make that more clear or omit these comments entirely? > > > > The role of wled3_num_strings_values_fn() is to enumerate the list of > > legal values for the property [ 1, 2, 3 ]. Your changes cause the > > enumeration to include a non-legal value so that you can have an > > identity mapping between the symbol and the enumerate value. > > > > An alternative approach would be to leave the enumeration logic > > alone but set the num_string default to UINT_MAX in all cases: > > > > - cfg->num_strings = cfg->num_strings + 1; > > + if (cfg->num_strings == UINT_MAX) > > + cfg->num_strings = > > Oops... looks like I missed the cfg->max_string_count here. > > > > + else > > + /* Convert from enumerated to numeric form */ > > + cfg->num_strings = wled3_num_strings_values_fn( > > + cfg->num_strings); > > > PS the alternative option is not to treat num-strings as an enumerated >value at all and just read it directly without using wled_values()... I much prefer doing that instead of trying to wrangle enumeration parsing around integer values that are supposed to be used as-is. After all this variable is already named to set the `+ 1` override currently, and `qcom,enabled_strings` has "custom" handling as well. I'll extend the validation to ensure num_strings>=1 too. In addition, and this needs some investigation on the dt-bindings side too, it might be beneficial to make both properties mutually exclusive. When specifying qcom,enabled_strings it makes little sense to also provide qcom,num_strings and we want the former to take precedence. At that point one might ask why qcom,num_strings remains at all when DT can use qcom,enabled_strings instead. We will supposedly have to keep backwards compatibility with DTs in mind so none of this can be removed or made mutually exclusive from a driver standpoint, that all has to be done in dt-bindings yaml to be enforced on checked-in DTs. - Marijn
[PATCH v5 0/7] MIPS: JZ4780 and CI20 HDMI
PATCH V5 2021-10-05 14:28:44: - dropped mode_fixup and timings support in dw-hdmi as it is no longer needed in this V5 (by h...@goldelico.com) - dropped "drm/ingenic: add some jz4780 specific features" (stimulated by p...@crapouillou.net) - fixed typo in commit subject: "synopsis" -> "synopsys" (by h...@goldelico.com) - swapped clocks in jz4780.dtsi to match synopsys,dw-hdmi.yaml (by h...@goldelico.com) - improved, simplified, fixed, dtbschecked ingenic-jz4780-hdmi.yaml and made dependent of bridge/synopsys,dw-hdmi.yaml (based on suggestions by max...@cerno.tech) - fixed binding vs. driver&DTS use of hdmi-5v regulator (suggested by max...@cerno.tech) - dropped "drm/bridge: synopsis: Fix to properly handle HPD" - was a no longer needed workaround for a previous version (suggested by max...@cerno.tech) PATCH V4 2021-09-27 18:44:38: - fix setting output_port = 1 (issue found by p...@crapouillou.net) - ci20.dts: convert to use hdmi-connector (by h...@goldelico.com) - add a hdmi-regulator to control +5V power (by h...@goldelico.com) - added a fix to dw-hdmi to call drm_kms_helper_hotplug_event on plugin event detection (by h...@goldelico.com) - always allocate extended descriptor but initialize only for jz4780 (by h...@goldelico.com) - updated to work on top of "[PATCH v3 0/6] drm/ingenic: Various improvements v3" (by p...@crapouillou.net) - rebased to v5.13-rc3 PATCH V3 2021-08-08 07:10:50: This series adds HDMI support for JZ4780 and CI20 board (and fixes one IPU related issue in registration error path) - [patch 1/8] switched from mode_fixup to atomic_check (suggested by robert.f...@linaro.org) - the call to the dw-hdmi specialization is still called mode_fixup - [patch 3/8] diverse fixes for ingenic-drm-drv (suggested by p...@crapouillou.net) - factor out some non-HDMI features of the jz4780 into a separate patch - multiple fixes around max height - do not change regmap config but a copy on stack - define some constants - factor out fixing of drm_init error path for IPU into separate patch - use FIELD_PREP() - [patch 8/8] conversion to component framework dropped (suggested by laurent.pinch...@ideasonboard.com and p...@crapouillou.net) PATCH V2 2021-08-05 16:08:05: This series adds HDMI support for JZ4780 and CI20 board V2: - code and commit messages revisited for checkpatch warnings - rebased on v5.14-rc4 - include (failed, hence RFC 8/8) attempt to convert to component framework (was suggested by Paul Cercueil a while ago) H. Nikolaus Schaller (1): MIPS: defconfig: CI20: configure for DRM_DW_HDMI_JZ4780 Paul Boddie (5): drm/ingenic: Fix drm_init error path if IPU was registered drm/ingenic: Add support for JZ4780 and HDMI output drm/ingenic: Add dw-hdmi driver for jz4780 MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers MIPS: DTS: CI20: Add DT nodes for HDMI setup Sam Ravnborg (1): dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema .../bindings/display/ingenic-jz4780-hdmi.yaml | 79 +++ arch/mips/boot/dts/ingenic/ci20.dts | 67 ++ arch/mips/boot/dts/ingenic/jz4780.dtsi| 45 +++ arch/mips/configs/ci20_defconfig | 6 + drivers/gpu/drm/ingenic/Kconfig | 9 ++ drivers/gpu/drm/ingenic/Makefile | 1 + drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 96 +- drivers/gpu/drm/ingenic/ingenic-drm.h | 42 ++ drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 125 ++ 9 files changed, 464 insertions(+), 6 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c -- 2.33.0
[PATCH v5 1/7] drm/ingenic: Fix drm_init error path if IPU was registered
From: Paul Boddie If ingenic drm driver can not be registered, the IPU driver won't be deregistered. Code structure is chosen in preparation to add hdmi unregistration in error case following the same pattern by a later patch. Signed-off-by: Paul Boddie Signed-off-by: H. Nikolaus Schaller --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 35b61657d9f6..f73522bdacaa 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -1498,7 +1498,16 @@ static int ingenic_drm_init(void) return err; } - return platform_driver_register(&ingenic_drm_driver); + err = platform_driver_register(&ingenic_drm_driver); + if (err) + goto err_ipu_unreg; + + return 0; + +err_ipu_unreg: + if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) + platform_driver_unregister(ingenic_ipu_driver_ptr); + return err; } module_init(ingenic_drm_init); -- 2.33.0
[PATCH v5 4/7] drm/ingenic: Add dw-hdmi driver for jz4780
From: Paul Boddie A specialisation of the generic Synopsys HDMI driver is employed for JZ4780 HDMI support. This requires a new driver, plus device tree and configuration modifications. Signed-off-by: Paul Boddie Signed-off-by: Ezequiel Garcia Signed-off-by: H. Nikolaus Schaller --- drivers/gpu/drm/ingenic/Kconfig | 9 ++ drivers/gpu/drm/ingenic/Makefile | 1 + drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 125 ++ 3 files changed, 135 insertions(+) create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig index 3b57f8be007c..4c7d311fbeff 100644 --- a/drivers/gpu/drm/ingenic/Kconfig +++ b/drivers/gpu/drm/ingenic/Kconfig @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU The Image Processing Unit (IPU) will appear as a second primary plane. +config DRM_INGENIC_DW_HDMI + bool "Ingenic specific support for Synopsys DW HDMI" + depends on MACH_JZ4780 + select DRM_DW_HDMI + help + Choose this option to enable Synopsys DesignWare HDMI based driver. + If you want to enable HDMI on Ingenic JZ4780 based SoC, you should + select this option.. + endif diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile index d313326bdddb..3db9888a6c04 100644 --- a/drivers/gpu/drm/ingenic/Makefile +++ b/drivers/gpu/drm/ingenic/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o ingenic-drm-y = ingenic-drm-drv.o ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o +ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file mode 100644 index ..17bd3d351546 --- /dev/null +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc. + * Copyright (C) 2019, 2020 Paul Boddie + * + * Derived from dw_hdmi-imx.c with i.MX portions removed. + * Probe and remove operations derived from rcar_dw_hdmi.c. + */ + +#include +#include +#include + +#include +#include +#include + +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = { + { 4525, { { 0x01e0, 0x }, { 0x21e1, 0x }, { 0x41e2, 0x } } }, + { 9250, { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 } } }, + { 14850, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a } } }, + { 21600, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f } } }, + { ~0UL, { { 0x, 0x }, { 0x, 0x }, { 0x, 0x } } } +}; + +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = { + /*pixelclk bpp8bpp10 bpp12 */ + { 5400, { 0x091c, 0x091c, 0x06dc } }, + { 5840, { 0x091c, 0x06dc, 0x06dc } }, + { 7200, { 0x06dc, 0x06dc, 0x091c } }, + { 7425, { 0x06dc, 0x0b5c, 0x091c } }, + { 11880, { 0x091c, 0x091c, 0x06dc } }, + { 21600, { 0x06dc, 0x0b5c, 0x091c } }, + { ~0UL, { 0x, 0x, 0x } }, +}; + +/* + * Resistance term 133Ohm Cfg + * PREEMP config 0.00 + * TX/CK level 10 + */ +static const struct dw_hdmi_phy_config ingenic_phy_config[] = { + /*pixelclk symbol term vlev */ + { 21600, 0x800d, 0x0005, 0x01ad}, + { ~0UL, 0x, 0x, 0x} +}; + +static enum drm_mode_status +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data, + const struct drm_display_info *info, + const struct drm_display_mode *mode) +{ + if (mode->clock < 13500) + return MODE_CLOCK_LOW; + /* FIXME: Hardware is capable of 270MHz, but setup data is missing. */ + if (mode->clock > 216000) + return MODE_CLOCK_HIGH; + + return MODE_OK; +} + +static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = { + .mpll_cfg = ingenic_mpll_cfg, + .cur_ctr= ingenic_cur_ctr, + .phy_config = ingenic_phy_config, + .mode_valid = ingenic_dw_hdmi_mode_valid, + .output_port= 1, +}; + +static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = { + { .compatible = "ingenic,jz4780-dw-hdmi" }, + { /* Sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids); + +static int ingenic_dw_hdmi_probe(struct platform_device *pdev) +{ + struct dw_hdmi *hdmi; + struct regulator *regulator; + int ret; + + hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data); + if (IS_ERR(hdmi)) + return PTR_ERR(hdmi); + + platform_set_drvdata(pdev, hdmi); + + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); + + if (IS_ERR(regulator)) { + ret = PTR_ERR(regulator); + + DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %s (%
[PATCH v5 7/7] MIPS: defconfig: CI20: configure for DRM_DW_HDMI_JZ4780
Enable CONFIG options as modules. Signed-off-by: Ezequiel Garcia Signed-off-by: H. Nikolaus Schaller --- arch/mips/configs/ci20_defconfig | 6 ++ 1 file changed, 6 insertions(+) diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig index ab7ebb066834..9c9c649d385b 100644 --- a/arch/mips/configs/ci20_defconfig +++ b/arch/mips/configs/ci20_defconfig @@ -98,7 +98,13 @@ CONFIG_RC_DEVICES=y CONFIG_IR_GPIO_CIR=m CONFIG_IR_GPIO_TX=m CONFIG_MEDIA_SUPPORT=m +CONFIG_DRM=m +CONFIG_DRM_INGENIC=m +CONFIG_DRM_INGENIC_DW_HDMI=y +CONFIG_DRM_DISPLAY_CONNECTOR=m # CONFIG_VGA_CONSOLE is not set +CONFIG_FB=y +CONFIG_FRAMEBUFFER_CONSOLE=y # CONFIG_HID is not set CONFIG_USB=y CONFIG_USB_STORAGE=y -- 2.33.0
[PATCH v5 5/7] MIPS: DTS: jz4780: Account for Synopsys HDMI driver and LCD controllers
From: Paul Boddie A specialisation of the generic Synopsys HDMI driver is employed for JZ4780 HDMI support. This requires a new driver, plus device tree and configuration modifications. Signed-off-by: Paul Boddie Signed-off-by: H. Nikolaus Schaller --- arch/mips/boot/dts/ingenic/jz4780.dtsi | 45 ++ 1 file changed, 45 insertions(+) diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi index 9e34f433b9b5..c3c18a59c377 100644 --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi @@ -424,6 +424,51 @@ i2c4: i2c@10054000 { status = "disabled"; }; + hdmi: hdmi@1018 { + compatible = "ingenic,jz4780-dw-hdmi"; + reg = <0x1018 0x8000>; + reg-io-width = <4>; + + clocks = <&cgu JZ4780_CLK_AHB0>, <&cgu JZ4780_CLK_HDMI>; + clock-names = "iahb", "isfr"; + + assigned-clocks = <&cgu JZ4780_CLK_HDMI>; + assigned-clock-rates = <2700>; + + interrupt-parent = <&intc>; + interrupts = <3>; + + /* ddc-i2c-bus = <&i2c4>; */ + + status = "disabled"; + }; + + lcdc0: lcdc0@1305 { + compatible = "ingenic,jz4780-lcd"; + reg = <0x1305 0x1800>; + + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>; + clock-names = "lcd", "lcd_pclk"; + + interrupt-parent = <&intc>; + interrupts = <31>; + + status = "disabled"; + }; + + lcdc1: lcdc1@130a { + compatible = "ingenic,jz4780-lcd"; + reg = <0x130a 0x1800>; + + clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>; + clock-names = "lcd", "lcd_pclk"; + + interrupt-parent = <&intc>; + interrupts = <31>; + + status = "disabled"; + }; + nemc: nemc@1341 { compatible = "ingenic,jz4780-nemc", "simple-mfd"; reg = <0x1341 0x1>; -- 2.33.0
[PATCH v5 3/7] dt-bindings: display: Add ingenic, jz4780-dw-hdmi DT Schema
From: Sam Ravnborg Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC. Based on .txt binding from Zubair Lutfullah Kakakhel Signed-off-by: Sam Ravnborg Signed-off-by: H. Nikolaus Schaller Cc: Rob Herring Cc: devicet...@vger.kernel.org --- .../bindings/display/ingenic-jz4780-hdmi.yaml | 79 +++ 1 file changed, 79 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file mode 100644 index ..5bcb342da86f --- /dev/null +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml @@ -0,0 +1,79 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Bindings for Ingenic JZ4780 HDMI Transmitter + +maintainers: + - H. Nikolaus Schaller + +description: | + The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4 + TX controller IP with accompanying PHY IP. + +allOf: + - $ref: bridge/synopsys,dw-hdmi.yaml# + +properties: + compatible: +const: ingenic,jz4780-dw-hdmi + + reg-io-width: +const: 4 + + clocks: +maxItems: 2 + + hdmi-5v-supply: +description: Optional regulator to provide +5V at the connector + + ddc-i2c-bus: +description: An I2C interface if the internal DDC I2C driver is not to be used + + ports: +$ref: /schemas/graph.yaml#/properties/ports + +required: +- compatible +- clocks +- clock-names +- ports +- reg-io-width + +additionalProperties: false + +examples: + - | +#include + +hdmi: hdmi@1018 { +compatible = "ingenic,jz4780-dw-hdmi"; +reg = <0x1018 0x8000>; +reg-io-width = <4>; +ddc-i2c-bus = <&i2c4>; +interrupt-parent = <&intc>; +interrupts = <3>; +clocks = <&cgu JZ4780_CLK_AHB0>, <&cgu JZ4780_CLK_HDMI>; +clock-names = "iahb", "isfr"; + +ports { +#address-cells = <1>; +#size-cells = <0>; +hdmi_in: port@0 { +reg = <0>; +dw_hdmi_in: endpoint { +remote-endpoint = <&jz4780_lcd_out>; +}; +}; +hdmi_out: port@1 { +reg = <1>; +dw_hdmi_out: endpoint { +remote-endpoint = <&hdmi_con>; +}; +}; +}; +}; + +... -- 2.33.0
[PATCH v5 6/7] MIPS: DTS: CI20: Add DT nodes for HDMI setup
From: Paul Boddie We need to hook up * HDMI connector * HDMI power regulator * DDC pinmux * HDMI and LCDC endpoint connections Signed-off-by: Paul Boddie Signed-off-by: H. Nikolaus Schaller --- arch/mips/boot/dts/ingenic/ci20.dts | 67 + 1 file changed, 67 insertions(+) diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts index a688809beebc..4776be35b14d 100644 --- a/arch/mips/boot/dts/ingenic/ci20.dts +++ b/arch/mips/boot/dts/ingenic/ci20.dts @@ -78,6 +78,18 @@ eth0_power: fixedregulator@0 { enable-active-high; }; + hdmi_out: connector { + compatible = "hdmi-connector"; + label = "HDMI OUT"; + type = "a"; + + port { + hdmi_con: endpoint { + remote-endpoint = <&dw_hdmi_out>; + }; + }; + }; + ir: ir { compatible = "gpio-ir-receiver"; gpios = <&gpe 3 GPIO_ACTIVE_LOW>; @@ -102,6 +114,17 @@ otg_power: fixedregulator@2 { gpio = <&gpf 14 GPIO_ACTIVE_LOW>; enable-active-high; }; + + hdmi_power: fixedregulator@3 { + compatible = "regulator-fixed"; + + regulator-name = "hdmi_power"; + regulator-min-microvolt = <500>; + regulator-max-microvolt = <500>; + + gpio = <&gpa 25 GPIO_ACTIVE_LOW>; + enable-active-high; + }; }; &ext { @@ -506,6 +529,12 @@ pins_i2c4: i2c4 { bias-disable; }; + pins_hdmi_ddc: hdmi_ddc { + function = "hdmi-ddc"; + groups = "hdmi-ddc"; + bias-disable; + }; + pins_nemc: nemc { function = "nemc"; groups = "nemc-data", "nemc-cle-ale", "nemc-rd-we", "nemc-frd-fwe"; @@ -536,3 +565,41 @@ pins_mmc1: mmc1 { bias-disable; }; }; + +&hdmi { + status = "okay"; + + pinctrl-names = "default"; + pinctrl-0 = <&pins_hdmi_ddc>; + + hdmi-5v-supply = <&hdmi_power>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + dw_hdmi_in: endpoint { + remote-endpoint = <&lcd_out>; + }; + }; + + port@1 { + reg = <1>; + dw_hdmi_out: endpoint { + remote-endpoint = <&hdmi_con>; + }; + }; + }; +}; + +&lcdc0 { + status = "okay"; + + port { + lcd_out: endpoint { + remote-endpoint = <&dw_hdmi_in>; + }; + }; +}; -- 2.33.0
[PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output
From: Paul Boddie Add support for the LCD controller present on JZ4780 SoCs. This SoC uses 8-byte descriptors which extend the current 4-byte descriptors used for other Ingenic SoCs. Tested on MIPS Creator CI20 board. Signed-off-by: Paul Boddie Signed-off-by: Ezequiel Garcia Signed-off-by: H. Nikolaus Schaller --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +-- drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++ 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index f73522bdacaa..e2df4b085905 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -6,6 +6,7 @@ #include "ingenic-drm.h" +#include #include #include #include @@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc { u32 addr; u32 id; u32 cmd; + /* extended hw descriptor for jz4780 */ + u32 offsize; + u32 pagewidth; + u32 cpos; + u32 dessize; } __aligned(16); struct ingenic_dma_hwdescs { @@ -60,9 +66,11 @@ struct jz_soc_info { bool needs_dev_clk; bool has_osd; bool map_noncoherent; + bool use_extended_hwdesc; unsigned int max_width, max_height; const u32 *formats_f0, *formats_f1; unsigned int num_formats_f0, num_formats_f1; + unsigned int max_reg; }; struct ingenic_drm_private_state { @@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg) } } -static const struct regmap_config ingenic_drm_regmap_config = { +static struct regmap_config ingenic_drm_regmap_config = { .reg_bits = 32, .val_bits = 32, .reg_stride = 4, - .max_register = JZ_REG_LCD_SIZE1, .writeable_reg = ingenic_drm_writeable_reg, }; @@ -663,6 +670,37 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4); hwdesc->next = dma_hwdesc_addr(priv, next_id); + if (priv->soc_info->use_extended_hwdesc) { + hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE; + + /* Extended 8-byte descriptor */ + hwdesc->cpos = 0; + hwdesc->offsize = 0; + hwdesc->pagewidth = 0; + + switch (newstate->fb->format->format) { + case DRM_FORMAT_XRGB1555: + hwdesc->cpos |= JZ_LCD_CPOS_RGB555; + fallthrough; + case DRM_FORMAT_RGB565: + hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16; + break; + case DRM_FORMAT_XRGB: + hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24; + break; + } + hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD | + (JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 << +JZ_LCD_CPOS_COEFFICIENT_OFFSET); + + hwdesc->dessize = + (0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) | + FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK << + JZ_LCD_DESSIZE_HEIGHT_OFFSET, height - 1) | + FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK << + JZ_LCD_DESSIZE_WIDTH_OFFSET, width - 1); + } + if (drm_atomic_crtc_needs_modeset(crtc_state)) { fourcc = newstate->fb->format->format; @@ -694,6 +732,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE; } + /* set use of the 8-word descriptor and OSD foreground usage. */ + if (priv->soc_info->use_extended_hwdesc) + cfg |= JZ_LCD_CFG_DESCRIPTOR_8; + if (mode->flags & DRM_MODE_FLAG_NHSYNC) cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; if (mode->flags & DRM_MODE_FLAG_NVSYNC) @@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) struct drm_encoder *encoder; struct ingenic_drm_bridge *ib; struct drm_device *drm; + struct regmap_config regmap_config; void __iomem *base; long parent_rate; unsigned int i, clone_mask = 0; @@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) return PTR_ERR(base); } + regmap_config = ingenic_drm_regmap_config; + regmap_config.max_register = soc_info->max_reg; priv->map = devm_regmap_init_mmio(dev, base, -
Re: [PATCH] drm: fsl-dcu: enable PIXCLK on LS1021A
On Thu, 2021-09-16 at 14:50 +0200, Matthias Schiffer wrote: > On Fri, 2020-08-21 at 15:41 +0200, Stefan Agner wrote: > > Hi Matthias, > > > > On 2020-08-20 12:58, Matthias Schiffer wrote: > > > The PIXCLK needs to be enabled in SCFG before accessing the DCU on > > > LS1021A, > > > or the access will hang. > > > > Hm, this seems a rather ad-hoc access to SCFG from the DCU. We do > > support a pixel clock in the device tree bindings of fsl-dcu, so ideally > > we should enable the pixel clock through the clock framework. > > > > On the other hand, I guess that would mean adding a clock driver to flip > > a single bit, which seems a bit excessive too. > > > > I'd like a second opinion on that. Adding clk framework maintainers. > > It's been a while, and nobody else has given their opinion. How should > we proceed with this patch? > > Matthias This patch is still blocked on a maintainer decision. Should I send a rebased version of the current solution, or do we want to have a proper clk driver to flip this bit? > > > > > > -- > > Stefan > > > > > > > > Signed-off-by: Matthias Schiffer > > > --- > > > drivers/gpu/drm/fsl-dcu/Kconfig | 1 + > > > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 25 +++ > > > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 3 +++ > > > 3 files changed, 29 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig > > > b/drivers/gpu/drm/fsl-dcu/Kconfig > > > index d7dd8ba90e3a..9e5a35e7c00c 100644 > > > --- a/drivers/gpu/drm/fsl-dcu/Kconfig > > > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig > > > @@ -8,6 +8,7 @@ config DRM_FSL_DCU > > > select DRM_PANEL > > > select REGMAP_MMIO > > > select VIDEOMODE_HELPERS > > > + select MFD_SYSCON if SOC_LS1021A > > > help > > > Choose this option if you have an Freescale DCU chipset. > > > If M is selected the module will be called fsl-dcu-drm. > > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > > > index abbc1ddbf27f..8a7556655581 100644 > > > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > > > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > > > @@ -51,6 +51,23 @@ static const struct regmap_config > > > fsl_dcu_regmap_config = { > > > .volatile_reg = fsl_dcu_drm_is_volatile_reg, > > > }; > > > > > > +static int fsl_dcu_scfg_config_ls1021a(struct device_node *np) > > > +{ > > > + struct regmap *scfg; > > > + > > > + scfg = syscon_regmap_lookup_by_compatible("fsl,ls1021a-scfg"); > > > + if (IS_ERR(scfg)) > > > + return PTR_ERR(scfg); > > > + > > > + /* > > > + * For simplicity, enable the PIXCLK unconditionally. It might > > > + * be possible to disable the clock in PM or on unload as a future > > > + * improvement. > > > + */ > > > + return regmap_update_bits(scfg, SCFG_PIXCLKCR, SCFG_PIXCLKCR_PXCEN, > > > + SCFG_PIXCLKCR_PXCEN); > > > +} > > > + > > > static void fsl_dcu_irq_uninstall(struct drm_device *dev) > > > { > > > struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > > > @@ -70,6 +87,14 @@ static int fsl_dcu_load(struct drm_device *dev, > > > unsigned long flags) > > > return ret; > > > } > > > > > > + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) { > > > + ret = fsl_dcu_scfg_config_ls1021a(fsl_dev->np); > > > + if (ret < 0) { > > > + dev_err(dev->dev, "failed to enable pixclk\n"); > > > + goto done; > > > + } > > > + } > > > + > > > ret = drm_vblank_init(dev, dev->mode_config.num_crtc); > > > if (ret < 0) { > > > dev_err(dev->dev, "failed to initialize vblank\n"); > > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > > index e2049a0e8a92..566396013c04 100644 > > > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > > @@ -160,6 +160,9 @@ > > > #define FSL_DCU_ARGB 12 > > > #define FSL_DCU_YUV422 14 > > > > > > +#define SCFG_PIXCLKCR0x28 > > > +#define SCFG_PIXCLKCR_PXCEN BIT(31) > > > + > > > #define VF610_LAYER_REG_NUM 9 > > > #define LS1021A_LAYER_REG_NUM10