Re: [PATCH] lib/stackdepot: always do filter_irq_stacks() in stack_depot_save()
On Tue, Nov 30, 2021 at 11:14 AM Marco Elver wrote: > > The non-interrupt portion of interrupt stack traces before interrupt > entry is usually arbitrary. Therefore, saving stack traces of interrupts > (that include entries before interrupt entry) to stack depot leads to > unbounded stackdepot growth. > > As such, use of filter_irq_stacks() is a requirement to ensure > stackdepot can efficiently deduplicate interrupt stacks. > > Looking through all current users of stack_depot_save(), none (except > KASAN) pass the stack trace through filter_irq_stacks() before passing > it on to stack_depot_save(). > > Rather than adding filter_irq_stacks() to all current users of > stack_depot_save(), it became clear that stack_depot_save() should > simply do filter_irq_stacks(). > > Signed-off-by: Marco Elver > --- > lib/stackdepot.c | 13 + > mm/kasan/common.c | 1 - > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/lib/stackdepot.c b/lib/stackdepot.c > index b437ae79aca1..519c7898c7f2 100644 > --- a/lib/stackdepot.c > +++ b/lib/stackdepot.c > @@ -305,6 +305,9 @@ EXPORT_SYMBOL_GPL(stack_depot_fetch); > * (allocates using GFP flags of @alloc_flags). If @can_alloc is %false, > avoids > * any allocations and will fail if no space is left to store the stack > trace. > * > + * If the stack trace in @entries is from an interrupt, only the portion up > to > + * interrupt entry is saved. > + * > * Context: Any context, but setting @can_alloc to %false is required if > * alloc_pages() cannot be used from the current context. Currently > * this is the case from contexts where neither %GFP_ATOMIC nor > @@ -323,6 +326,16 @@ depot_stack_handle_t __stack_depot_save(unsigned long > *entries, > unsigned long flags; > u32 hash; > > + /* > +* If this stack trace is from an interrupt, including anything before > +* interrupt entry usually leads to unbounded stackdepot growth. > +* > +* Because use of filter_irq_stacks() is a requirement to ensure > +* stackdepot can efficiently deduplicate interrupt stacks, always > +* filter_irq_stacks() to simplify all callers' use of stackdepot. > +*/ > + nr_entries = filter_irq_stacks(entries, nr_entries); > + > if (unlikely(nr_entries == 0) || stack_depot_disable) > goto fast_exit; > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 8428da2aaf17..efaa836e5132 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -36,7 +36,6 @@ depot_stack_handle_t kasan_save_stack(gfp_t flags, bool > can_alloc) > unsigned int nr_entries; > > nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0); > - nr_entries = filter_irq_stacks(entries, nr_entries); > return __stack_depot_save(entries, nr_entries, flags, can_alloc); > } > > -- > 2.34.0.rc2.393.gf8c9666880-goog > Reviewed-by: Andrey Konovalov
Re: [Linaro-mm-sig] [RFC PATCH 1/2] dma-fence: Avoid establishing a locking order between fence classes
On 12/1/21 08:05, Christian König wrote: Am 30.11.21 um 20:27 schrieb Thomas Hellström: On 11/30/21 19:12, Thomas Hellström wrote: On Tue, 2021-11-30 at 16:02 +0100, Christian König wrote: Am 30.11.21 um 15:35 schrieb Thomas Hellström: On Tue, 2021-11-30 at 14:26 +0100, Christian König wrote: Am 30.11.21 um 13:56 schrieb Thomas Hellström: On 11/30/21 13:42, Christian König wrote: Am 30.11.21 um 13:31 schrieb Thomas Hellström: [SNIP] Other than that, I didn't investigate the nesting fails enough to say I can accurately review this. :) Basically the problem is that within enable_signaling() which is called with the dma_fence lock held, we take the dma_fence lock of another fence. If that other fence is a dma_fence_array, or a dma_fence_chain which in turn tries to lock a dma_fence_array we hit a splat. Yeah, I already thought that you constructed something like that. You get the splat because what you do here is illegal, you can't mix dma_fence_array and dma_fence_chain like this or you can end up in a stack corruption. Hmm. Ok, so what is the stack corruption, is it that the enable_signaling() will end up with endless recursion? If so, wouldn't it be more usable we break that recursion chain and allow a more general use? The problem is that this is not easily possible for dma_fence_array containers. Just imagine that you drop the last reference to the containing fences during dma_fence_array destruction if any of the contained fences is another container you can easily run into recursion and with that stack corruption. Indeed, that would require some deeper surgery. That's one of the major reasons I came up with the dma_fence_chain container. This one you can chain any number of elements together without running into any recursion. Also what are the mixing rules between these? Never use a dma-fence-chain as one of the array fences and never use a dma-fence-array as a dma-fence-chain fence? You can't add any other container to a dma_fence_array, neither other dma_fence_array instances nor dma_fence_chain instances. IIRC at least technically a dma_fence_chain can contain a dma_fence_array if you absolutely need that, but Daniel, Jason and I already had the same discussion a while back and came to the conclusion to avoid that as well if possible. Yes, this is actually the use-case. But what I can't easily guarantee is that that dma_fence_chain isn't fed into a dma_fence_array somewhere else. How do you typically avoid that? Meanwhile I guess I need to take a different approach in the driver to avoid this altogether. Jason and I came up with a deep dive iterator for his use case, but I think we don't want to use that any more after my dma_resv rework. In other words when you need to create a new dma_fence_array you flatten out the existing construct which is at worst case dma_fence_chain->dma_fence_array->dma_fence. Ok, Are there any cross-driver contract here, Like every driver using a dma_fence_array need to check for dma_fence_chain and flatten like above? So far we only discussed that on the mailing list but haven't made any documentation for that. OK, one other cross-driver pitfall I see is if someone accidently joins two fence chains together by creating a fence chain unknowingly using another fence chain as the @fence argument? The third cross-driver pitfall IMHO is the locking dependency these containers add. Other drivers (read at least i915) may have defined slightly different locking orders and that should also be addressed if needed, but that requires a cross driver agreement what the locking orders really are. Patch 1 actually addresses this, while keeping the container lockdep warnings for deep recursions, so at least I think that could serve as a discussion starter. /Thomas Oh, and a follow up question: If there was a way to break the recursion on final put() (using the same basic approach as patch 2 in this series uses to break recursion in enable_signaling()), so that none of these containers did require any special treatment, would it be worth pursuing? I guess it might be possible by having the callbacks drop the references rather than the loop in the final put. + a couple of changes in code iterating over the fence pointers. That won't really help, you just move the recursion from the final put into the callback. How do we recurse from the callback? The introduced fence_put() of individual fence pointers doesn't recurse anymore (at most 1 level), and any callback recursion is broken by the irq_work? I figure the big amount of work would be to adjust code that iterates over the individual fence pointers to recognize that they are rcu protected. Thanks, /Thomas
Re: [Linaro-mm-sig] [RFC PATCH 1/2] dma-fence: Avoid establishing a locking order between fence classes
Am 01.12.21 um 09:23 schrieb Thomas Hellström (Intel): [SNIP] Jason and I came up with a deep dive iterator for his use case, but I think we don't want to use that any more after my dma_resv rework. In other words when you need to create a new dma_fence_array you flatten out the existing construct which is at worst case dma_fence_chain->dma_fence_array->dma_fence. Ok, Are there any cross-driver contract here, Like every driver using a dma_fence_array need to check for dma_fence_chain and flatten like above? So far we only discussed that on the mailing list but haven't made any documentation for that. OK, one other cross-driver pitfall I see is if someone accidently joins two fence chains together by creating a fence chain unknowingly using another fence chain as the @fence argument? That would indeed be illegal and we should probably add a WARN_ON() for that. The third cross-driver pitfall IMHO is the locking dependency these containers add. Other drivers (read at least i915) may have defined slightly different locking orders and that should also be addressed if needed, but that requires a cross driver agreement what the locking orders really are. Patch 1 actually addresses this, while keeping the container lockdep warnings for deep recursions, so at least I think that could serve as a discussion starter. No, drivers should never make any assumptions on that. E.g. when you need to take a look from a callback you must guarantee that you never have that lock taken when you call any of the dma_fence functions. Your patch breaks the lockdep annotation for that. What we could do is to avoid all this by not calling the callback with the lock held in the first place. /Thomas Oh, and a follow up question: If there was a way to break the recursion on final put() (using the same basic approach as patch 2 in this series uses to break recursion in enable_signaling()), so that none of these containers did require any special treatment, would it be worth pursuing? I guess it might be possible by having the callbacks drop the references rather than the loop in the final put. + a couple of changes in code iterating over the fence pointers. That won't really help, you just move the recursion from the final put into the callback. How do we recurse from the callback? The introduced fence_put() of individual fence pointers doesn't recurse anymore (at most 1 level), and any callback recursion is broken by the irq_work? Yeah, but then you would need to take another lock to avoid racing with dma_fence_array_signaled(). I figure the big amount of work would be to adjust code that iterates over the individual fence pointers to recognize that they are rcu protected. Could be that we could solve this with RCU, but that sounds like a lot of churn for no gain at all. In other words even with the problems solved I think it would be a really bad idea to allow chaining of dma_fence_array objects. Christian. Thanks, /Thomas
Re: [PATCH v3] drm/i915/dp: Perform 30ms delay after source OUI write
On Tue, 30 Nov 2021, Lyude Paul wrote: > While working on supporting the Intel HDR backlight interface, I noticed > that there's a couple of laptops that will very rarely manage to boot up > without detecting Intel HDR backlight support - even though it's supported > on the system. One example of such a laptop is the Lenovo P17 1st > generation. > > Following some investigation Ville Syrjälä did through the docs they have > available to them, they discovered that there's actually supposed to be a > 30ms wait after writing the source OUI before we begin setting up the rest > of the backlight interface. > > This seems to be correct, as adding this 30ms delay seems to have > completely fixed the probing issues I was previously seeing. So - let's > start performing a 30ms wait after writing the OUI, which we do in a manner > similar to how we keep track of PPS delays (e.g. record the timestamp of > the OUI write, and then wait for however many ms are left since that > timestamp right before we interact with the backlight) in order to avoid > waiting any longer then we need to. As well, this also avoids us performing > this delay on systems where we don't end up using the HDR backlight > interface. > > V3: > * Move last_oui_write into intel_dp > V2: > * Move panel delays into intel_pps > > Signed-off-by: Lyude Paul > Reviewed-by: Jani Nikula > Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight interface > (only SDR for now)") > Cc: Ville Syrjälä > Cc: # v5.12+ Thanks, pushed to drm-intel-next. BR, Jani. > --- > drivers/gpu/drm/i915/display/intel_display_types.h| 3 +++ > drivers/gpu/drm/i915/display/intel_dp.c | 11 +++ > drivers/gpu/drm/i915/display/intel_dp.h | 2 ++ > drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c | 5 + > 4 files changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index ea1e8a6e10b0..b9c967837872 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1653,6 +1653,9 @@ struct intel_dp { > struct intel_dp_pcon_frl frl; > > struct intel_psr psr; > + > + /* When we last wrote the OUI for eDP */ > + unsigned long last_oui_write; > }; > > enum lspcon_vendor { > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 0a424bf69396..5a8206298691 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -29,6 +29,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -2010,6 +2011,16 @@ intel_edp_init_source_oui(struct intel_dp *intel_dp, > bool careful) > > if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui)) > < 0) > drm_err(&i915->drm, "Failed to write source OUI\n"); > + > + intel_dp->last_oui_write = jiffies; > +} > + > +void intel_dp_wait_source_oui(struct intel_dp *intel_dp) > +{ > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + > + drm_dbg_kms(&i915->drm, "Performing OUI wait\n"); > + wait_remaining_ms_from_jiffies(intel_dp->last_oui_write, 30); > } > > /* If the device supports it, try to set the power state appropriately */ > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h > b/drivers/gpu/drm/i915/display/intel_dp.h > index ce229026dc91..b64145a3869a 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.h > +++ b/drivers/gpu/drm/i915/display/intel_dp.h > @@ -119,4 +119,6 @@ void intel_dp_pcon_dsc_configure(struct intel_dp > *intel_dp, >const struct intel_crtc_state *crtc_state); > void intel_dp_phy_test(struct intel_encoder *encoder); > > +void intel_dp_wait_source_oui(struct intel_dp *intel_dp); > + > #endif /* __INTEL_DP_H__ */ > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > index 8b9c925c4c16..62c112daacf2 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > @@ -36,6 +36,7 @@ > > #include "intel_backlight.h" > #include "intel_display_types.h" > +#include "intel_dp.h" > #include "intel_dp_aux_backlight.h" > > /* TODO: > @@ -106,6 +107,8 @@ intel_dp_aux_supports_hdr_backlight(struct > intel_connector *connector) > int ret; > u8 tcon_cap[4]; > > + intel_dp_wait_source_oui(intel_dp); > + > ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, > sizeof(tcon_cap)); > if (ret != sizeof(tcon_cap)) > return false; > @@ -204,6 +207,8 @@ intel_dp_aux_hdr_enable_backlight(const struct > intel_crtc_state *crtc_state, > int ret; > u8 old_ctrl, ctrl; > > + intel_dp_wait_source_oui(intel_dp); > + > ret = drm_dp_dpcd_readb(&intel_dp->aux
Re: [Intel-gfx] [PATCH v4 2/2] drm/i915: Use to_root_gt() to refer to the root tile
On 01.12.2021 01:38, Lucas De Marchi wrote: > On Wed, Dec 01, 2021 at 12:41:08AM +0200, Andi Shyti wrote: >> Hi Lucas, >> >> fist of all thanks for taking a look at this, I was eagerly >> waiting for reviewers. >> >> On Tue, Nov 30, 2021 at 01:07:30PM -0800, Lucas De Marchi wrote: >>> On Sun, Nov 28, 2021 at 01:09:26PM +0200, Andi Shyti wrote: >>> > Starting from a patch from Matt to_root_gt() returns the >>> > reference to the root tile in order to abstract the root tile >>> > from th callers. >>> > >>> > Being the root tile identified as tile '0', embed the id in the >>> > name so that i915->gt becomes i915->gt0. >>> > >>> > The renaming has been mostly done with the following command and >>> > some manual fixes. >>> > >>> > sed -i -e sed -i 's/\&i915\->gt\./\&to_root_gt(i915)\->/g' \ >>> > -e sed -i 's/\&dev_priv\->gt\./\&to_root_gt(dev_priv)\->/g' \ >>> > -e 's/\&dev_priv\->gt/to_root_gt(dev_priv)/g' \ >>> > -e 's/\&i915\->gt/to_root_gt(i915)/g' \ >>> > -e 's/dev_priv\->gt\./to_root_gt(dev_priv)\->/g' \ >>> > -e 's/i915\->gt\./to_root_gt(i915)\->/g' \ >>> > `find drivers/gpu/drm/i915/ -name *.[ch]` >>> > >>> > Two small changes have been added to this commit: >>> > >>> > 1. intel_reset_gpu() in intel_display.c retreives the gt from >>> > to_scanout_gt() >>> > 2. in set_scheduler_caps() the gt is taken from the engine and >>> > not from i915. >>> >>> Ideally the non-automatic changes should be in separate patches, before >>> the ones that can be done by automation. Because then it becomes easier >>> to apply the final result without conflicts. >> >> OK >> >>> This is quite a big diff to merge in one go. Looking at the pending >>> patches from Michal however I see he had similar changes, split in >>> sensible chunks.. Could you split your version like that? at least >>> gt/gem and display would be good to have separate. Or sync with Michal >>> on how to proceed with these versions Here are his patches: >>> >>> drm/i915: Remove i915->ggtt >>> drm/i915: Use to_gt() helper for GGTT accesses >>> drm/i915: Use to_gt() helper >>> drm/i915/gvt: Use to_gt() helper >>> drm/i915/gem: Use to_gt() helper >>> drm/i915/gt: Use to_gt() helper >>> drm/i915/display: Use to_gt() helper >>> drm/i915: Introduce to_gt() helper >> >> I understand... will follow this approach. >> >>> This first patch also removed the `struct intel_gt *gt = to_gt(pool)`, >>> that would otherwise be a leftover in >>> drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c >> >> One difference from Michal patch is that I am not using the >> wrapper >> >> to_gt(...) >> >> but >> >> to_root_gt(...) >> >> which was introduced by Matt. To me sounds more meaningful as it >> specifies that we are really looking for the root tile and not >> any tile. > > yes, I think it makes sense, too. Michal, any comment? I think you > also had other plans to get the root gt by another helper... ? The main rationale to use generic "to_gt()" helper name in all existing i915->gt cases in (other) Michal patches was that on some upcoming configs we want to distinguish between "primary" and "root" tile and use "to_root_gt()" helper only when referring to the root tile as described in Bspec:52416. Note that since current code baseline is still "single" tile, you can't tell whether all of these functions really expects special "root" tile or just "any" tile. Thus to avoid confusion or mistakes I would suggest to keep simple name "to_gt()" as in most cases usages of this helper it will likely be replaced with iterator from for_each_gt loop and any remaining usages will just mean "primary" tile or replaced with explicit "to_root_gt()" if really needed. Michal
Re: [PATCH v4] dma-buf: system_heap: Use 'for_each_sgtable_sg' in pages free flow
Hello Guangming, On Mon, 29 Nov 2021 at 23:35, John Stultz wrote: > > On Thu, Nov 25, 2021 at 11:48 PM wrote: > > > > From: Guangming > > > > For previous version, it uses 'sg_table.nent's to traverse sg_table in pages > > free flow. > > However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the > > number of > > created entries in the DMA adderess space. > > So, use 'sg_table.nents' in pages free flow will case some pages can't be > > freed. > > > > Here we should use sg_table.orig_nents to free pages memory, but use the > > sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common > > helper 'for_each_sg' which maybe cause memory leak) is much better. Thanks for catching this and the patch; applied to drm-misc-fixes. > > > > Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if > > available") > > Signed-off-by: Guangming > > Reviewed-by: Robin Murphy > > Cc: # 5.11.* > > Thanks so much for catching this and sending in all the revisions! > > Reviewed-by: John Stultz Best, Sumit. -- Thanks and regards, Sumit Semwal (he / him) Tech Lead - LCG, Vertical Technologies Linaro.org │ Open source software for ARM SoCs
Re: [Linaro-mm-sig] [RFC PATCH 1/2] dma-fence: Avoid establishing a locking order between fence classes
On 12/1/21 09:36, Christian König wrote: Am 01.12.21 um 09:23 schrieb Thomas Hellström (Intel): [SNIP] Jason and I came up with a deep dive iterator for his use case, but I think we don't want to use that any more after my dma_resv rework. In other words when you need to create a new dma_fence_array you flatten out the existing construct which is at worst case dma_fence_chain->dma_fence_array->dma_fence. Ok, Are there any cross-driver contract here, Like every driver using a dma_fence_array need to check for dma_fence_chain and flatten like above? So far we only discussed that on the mailing list but haven't made any documentation for that. OK, one other cross-driver pitfall I see is if someone accidently joins two fence chains together by creating a fence chain unknowingly using another fence chain as the @fence argument? That would indeed be illegal and we should probably add a WARN_ON() for that. The third cross-driver pitfall IMHO is the locking dependency these containers add. Other drivers (read at least i915) may have defined slightly different locking orders and that should also be addressed if needed, but that requires a cross driver agreement what the locking orders really are. Patch 1 actually addresses this, while keeping the container lockdep warnings for deep recursions, so at least I think that could serve as a discussion starter. No, drivers should never make any assumptions on that. Yes that i915 assumption of taking the lock of the last signaled fence first goes back a while in time. We should look at fixing that up, and document any (possibly forbidden) assumptions about fence lock locking orders to avoid it happening again, if there is no common cross-driver locking order that can be agreed. E.g. when you need to take a look from a callback you must guarantee that you never have that lock taken when you call any of the dma_fence functions. Your patch breaks the lockdep annotation for that. I'm pretty sure that could be fixed in a satisfactory way if needed. What we could do is to avoid all this by not calling the callback with the lock held in the first place. If that's possible that might be a good idea, pls also see below. /Thomas Oh, and a follow up question: If there was a way to break the recursion on final put() (using the same basic approach as patch 2 in this series uses to break recursion in enable_signaling()), so that none of these containers did require any special treatment, would it be worth pursuing? I guess it might be possible by having the callbacks drop the references rather than the loop in the final put. + a couple of changes in code iterating over the fence pointers. That won't really help, you just move the recursion from the final put into the callback. How do we recurse from the callback? The introduced fence_put() of individual fence pointers doesn't recurse anymore (at most 1 level), and any callback recursion is broken by the irq_work? Yeah, but then you would need to take another lock to avoid racing with dma_fence_array_signaled(). I figure the big amount of work would be to adjust code that iterates over the individual fence pointers to recognize that they are rcu protected. Could be that we could solve this with RCU, but that sounds like a lot of churn for no gain at all. In other words even with the problems solved I think it would be a really bad idea to allow chaining of dma_fence_array objects. Yes, that was really the question, Is it worth pursuing this? I'm not really suggesting we should allow this as an intentional feature. I'm worried, however, that if we allow these containers to start floating around cross-driver (or even internally) disguised as ordinary dma_fences, they would require a lot of driver special casing, or else completely unexpeced WARN_ON()s and lockdep splats would start to turn up, scaring people off from using them. And that would be a breeding ground for hairy driver-private constructs. /Thomas Christian. Thanks, /Thomas
Re: [Linaro-mm-sig] [RFC PATCH 1/2] dma-fence: Avoid establishing a locking order between fence classes
Am 01.12.21 um 11:15 schrieb Thomas Hellström (Intel): [SNIP] What we could do is to avoid all this by not calling the callback with the lock held in the first place. If that's possible that might be a good idea, pls also see below. The problem with that is dma_fence_signal_locked()/dma_fence_signal_timestamp_locked(). If we could avoid using that or at least allow it to drop the lock then we could call the callback without holding it. Somebody would need to audit the drivers and see if holding the lock is really necessary anywhere. /Thomas Oh, and a follow up question: If there was a way to break the recursion on final put() (using the same basic approach as patch 2 in this series uses to break recursion in enable_signaling()), so that none of these containers did require any special treatment, would it be worth pursuing? I guess it might be possible by having the callbacks drop the references rather than the loop in the final put. + a couple of changes in code iterating over the fence pointers. That won't really help, you just move the recursion from the final put into the callback. How do we recurse from the callback? The introduced fence_put() of individual fence pointers doesn't recurse anymore (at most 1 level), and any callback recursion is broken by the irq_work? Yeah, but then you would need to take another lock to avoid racing with dma_fence_array_signaled(). I figure the big amount of work would be to adjust code that iterates over the individual fence pointers to recognize that they are rcu protected. Could be that we could solve this with RCU, but that sounds like a lot of churn for no gain at all. In other words even with the problems solved I think it would be a really bad idea to allow chaining of dma_fence_array objects. Yes, that was really the question, Is it worth pursuing this? I'm not really suggesting we should allow this as an intentional feature. I'm worried, however, that if we allow these containers to start floating around cross-driver (or even internally) disguised as ordinary dma_fences, they would require a lot of driver special casing, or else completely unexpeced WARN_ON()s and lockdep splats would start to turn up, scaring people off from using them. And that would be a breeding ground for hairy driver-private constructs. Well the question is why we would want to do it? If it's to avoid inter driver lock dependencies by avoiding to call the callback with the spinlock held, then yes please. We had tons of problems with that, resulting in irq_work and work_item delegation all over the place. If it's to allow nesting of dma_fence_array instances, then it's most likely a really bad idea even if we fix all the locking order problems. Christian. /Thomas Christian. Thanks, /Thomas
Re: [PATCH v3 2/2] drm: rcar-du: Add R-Car DSI driver
Quoting Laurent Pinchart (2021-12-01 05:24:06) > From: LUU HOAI > > The driver supports the MIPI DSI/CSI-2 TX encoder found in the R-Car V3U > SoC. It currently supports DSI mode only. > > Signed-off-by: LUU HOAI > Signed-off-by: Laurent Pinchart > Reviewed-by: Kieran Bingham > Tested-by: Kieran Bingham And as there have been some changes to the probe / bridge handling: (Re)Tested-by: Kieran Bingham > Acked-by: Sam Ravnborg > --- > Changes since v2: > > - Support probing of child DSI devices > - Use devm_drm_of_get_bridge() helper > > Changes since v1: > > - Use U suffix for unsigned constants > - Fix indentation in Makefile > - Select DRM_MIPI_DSI > - Report correct fout frequency in debug message > - Move dsi_setup_info.err to local variable > --- > drivers/gpu/drm/rcar-du/Kconfig | 7 + > drivers/gpu/drm/rcar-du/Makefile | 1 + > drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 817 +++ > drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h | 172 > 4 files changed, 997 insertions(+) > create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h > > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig > index 3e588ddba245..1675df21d91f 100644 > --- a/drivers/gpu/drm/rcar-du/Kconfig > +++ b/drivers/gpu/drm/rcar-du/Kconfig > @@ -45,6 +45,13 @@ config DRM_RCAR_LVDS > select OF_FLATTREE > select OF_OVERLAY > > +config DRM_RCAR_MIPI_DSI > + tristate "R-Car DU MIPI DSI Encoder Support" > + depends on DRM && DRM_BRIDGE && OF > + select DRM_MIPI_DSI > + help > + Enable support for the R-Car Display Unit embedded MIPI DSI > encoders. > + > config DRM_RCAR_VSP > bool "R-Car DU VSP Compositor Support" if ARM > default y if ARM64 > diff --git a/drivers/gpu/drm/rcar-du/Makefile > b/drivers/gpu/drm/rcar-du/Makefile > index 4d1187ccc3e5..286bc81b3e7c 100644 > --- a/drivers/gpu/drm/rcar-du/Makefile > +++ b/drivers/gpu/drm/rcar-du/Makefile > @@ -19,6 +19,7 @@ obj-$(CONFIG_DRM_RCAR_CMM)+= rcar_cmm.o > obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o > obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o > obj-$(CONFIG_DRM_RCAR_LVDS)+= rcar_lvds.o > +obj-$(CONFIG_DRM_RCAR_MIPI_DSI)+= rcar_mipi_dsi.o > > # 'remote-endpoint' is fixed up at run-time > DTC_FLAGS_rcar_du_of_lvds_r8a7790 += -Wno-graph_endpoint > diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > new file mode 100644 > index ..fcaec3308d68 > --- /dev/null > +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > @@ -0,0 +1,817 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * rcar_mipi_dsi.c -- R-Car MIPI DSI Encoder > + * > + * Copyright (C) 2020 Renesas Electronics Corporation > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "rcar_mipi_dsi_regs.h" > + > +struct rcar_mipi_dsi { > + struct device *dev; > + const struct rcar_mipi_dsi_device_info *info; > + struct reset_control *rstc; > + > + struct mipi_dsi_host host; > + struct drm_bridge bridge; > + struct drm_bridge *next_bridge; > + struct drm_connector connector; > + > + void __iomem *mmio; > + struct { > + struct clk *mod; > + struct clk *pll; > + struct clk *dsi; > + } clocks; > + > + struct drm_display_mode display_mode; > + enum mipi_dsi_pixel_format format; > + unsigned int num_data_lanes; > + unsigned int lanes; > +}; > + > +static inline struct rcar_mipi_dsi * > +bridge_to_rcar_mipi_dsi(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct rcar_mipi_dsi, bridge); > +} > + > +static inline struct rcar_mipi_dsi * > +host_to_rcar_mipi_dsi(struct mipi_dsi_host *host) > +{ > + return container_of(host, struct rcar_mipi_dsi, host); > +} > + > +static const u32 phtw[] = { > + 0x01020114, 0x01600115, /* General testing */ > + 0x01030116, 0x0102011d, /* General testing */ > + 0x011101a4, 0x018601a4, /* 1Gbps testing */ > + 0x014201a0, 0x010001a3, /* 1Gbps testing */ > + 0x0101011f, /* 1Gbps testing */ > +}; > + > +static const u32 phtw2[] = { > + 0x010c0130, 0x010c0140, /* General testing */ > + 0x010c0150, 0x010c0180, /* General testing */ > + 0x010c0190, > + 0x010a0160, 0x010a0170, > + 0x01800164, 0x01800174, /* 1Gbps testing */ > +}; > + > +static const u32 hsfreqrange_table[][2] = { > + { 8000U, 0x00 }, { 9000U, 0x10 }, { 1U, 0x20 }, > + { 11000U, 0x30 }, { 12000U, 0x01 }, { 13000U, 0x11 }, > +
Re: [PATCH v3 2/2] drm: rcar-du: Add R-Car DSI driver
Hi Laurent, On Wed, Dec 1, 2021 at 10:54 AM Laurent Pinchart wrote: > > From: LUU HOAI > > The driver supports the MIPI DSI/CSI-2 TX encoder found in the R-Car V3U > SoC. It currently supports DSI mode only. > > Signed-off-by: LUU HOAI > Signed-off-by: Laurent Pinchart > Reviewed-by: Kieran Bingham > Tested-by: Kieran Bingham > Acked-by: Sam Ravnborg > --- > Changes since v2: > > - Support probing of child DSI devices > - Use devm_drm_of_get_bridge() helper > > Changes since v1: > > - Use U suffix for unsigned constants > - Fix indentation in Makefile > - Select DRM_MIPI_DSI > - Report correct fout frequency in debug message > - Move dsi_setup_info.err to local variable > --- > drivers/gpu/drm/rcar-du/Kconfig | 7 + > drivers/gpu/drm/rcar-du/Makefile | 1 + > drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 817 +++ > drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h | 172 > 4 files changed, 997 insertions(+) > create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h > > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig > index 3e588ddba245..1675df21d91f 100644 > --- a/drivers/gpu/drm/rcar-du/Kconfig > +++ b/drivers/gpu/drm/rcar-du/Kconfig > @@ -45,6 +45,13 @@ config DRM_RCAR_LVDS > select OF_FLATTREE > select OF_OVERLAY > > +config DRM_RCAR_MIPI_DSI > + tristate "R-Car DU MIPI DSI Encoder Support" > + depends on DRM && DRM_BRIDGE && OF > + select DRM_MIPI_DSI > + help > + Enable support for the R-Car Display Unit embedded MIPI DSI > encoders. > + > config DRM_RCAR_VSP > bool "R-Car DU VSP Compositor Support" if ARM > default y if ARM64 > diff --git a/drivers/gpu/drm/rcar-du/Makefile > b/drivers/gpu/drm/rcar-du/Makefile > index 4d1187ccc3e5..286bc81b3e7c 100644 > --- a/drivers/gpu/drm/rcar-du/Makefile > +++ b/drivers/gpu/drm/rcar-du/Makefile > @@ -19,6 +19,7 @@ obj-$(CONFIG_DRM_RCAR_CMM)+= rcar_cmm.o > obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o > obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o > obj-$(CONFIG_DRM_RCAR_LVDS)+= rcar_lvds.o > +obj-$(CONFIG_DRM_RCAR_MIPI_DSI)+= rcar_mipi_dsi.o > > # 'remote-endpoint' is fixed up at run-time > DTC_FLAGS_rcar_du_of_lvds_r8a7790 += -Wno-graph_endpoint > diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > new file mode 100644 > index ..fcaec3308d68 > --- /dev/null > +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > @@ -0,0 +1,817 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * rcar_mipi_dsi.c -- R-Car MIPI DSI Encoder > + * > + * Copyright (C) 2020 Renesas Electronics Corporation > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "rcar_mipi_dsi_regs.h" > + > +struct rcar_mipi_dsi { > + struct device *dev; > + const struct rcar_mipi_dsi_device_info *info; > + struct reset_control *rstc; > + > + struct mipi_dsi_host host; > + struct drm_bridge bridge; > + struct drm_bridge *next_bridge; > + struct drm_connector connector; > + > + void __iomem *mmio; > + struct { > + struct clk *mod; > + struct clk *pll; > + struct clk *dsi; > + } clocks; > + > + struct drm_display_mode display_mode; > + enum mipi_dsi_pixel_format format; > + unsigned int num_data_lanes; > + unsigned int lanes; > +}; > + > +static inline struct rcar_mipi_dsi * > +bridge_to_rcar_mipi_dsi(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct rcar_mipi_dsi, bridge); > +} > + > +static inline struct rcar_mipi_dsi * > +host_to_rcar_mipi_dsi(struct mipi_dsi_host *host) > +{ > + return container_of(host, struct rcar_mipi_dsi, host); > +} > + > +static const u32 phtw[] = { > + 0x01020114, 0x01600115, /* General testing */ > + 0x01030116, 0x0102011d, /* General testing */ > + 0x011101a4, 0x018601a4, /* 1Gbps testing */ > + 0x014201a0, 0x010001a3, /* 1Gbps testing */ > + 0x0101011f, /* 1Gbps testing */ > +}; > + > +static const u32 phtw2[] = { > + 0x010c0130, 0x010c0140, /* General testing */ > + 0x010c0150, 0x010c0180, /* General testing */ > + 0x010c0190, > + 0x010a0160, 0x010a0170, > + 0x01800164, 0x01800174, /* 1Gbps testing */ > +}; > + > +static const u32 hsfreqrange_table[][2] = { > + { 8000U, 0x00 }, { 9000U, 0x10 }, { 1U, 0x20 }, > + { 11000U, 0x30 }, { 12000U, 0x01 }, { 13000U, 0x11 }, > + { 14000U, 0x21 }, { 15000U, 0x31 }, { 16000U, 0x02 }, > + { 1
Re: [PATCH v2 1/2] drm: rcar-du: mipi-dsi: Support bridge probe ordering
Quoting Laurent Pinchart (2021-12-01 04:38:46) > Hi Kieran, > > Thank you for the patch. > > On Tue, Nov 30, 2021 at 04:25:12PM +, Kieran Bingham wrote: > > The bridge probe ordering for DSI devices has been clarified and further > > documented in > > I've read the document and > Good, I'm glad you've fully got the > > :-) Sorry, not sure how I missed that, I must have failed to paste in the link to the DSI probe documentation. But as you've read that, and now this is squashed, I think this is done anyway. Thanks for handling the integrations. -- Kieran > > > To support connecting with the SN65DSI86 device after commit c3b75d4734cb > > ("drm/bridge: sn65dsi86: Register and attach our DSI device at probe"), > > update to the new probe ordering to remove a perpetual -EPROBE_DEFER > > loop between the two devices. > > > > Signed-off-by: Kieran Bingham > > > > --- > > v2 > > - Remove now unused panel variable from rcar_mipi_dsi_probe() > > > > drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 49 + > > 1 file changed, 26 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > index 50e922328fed..0a9f197ef62c 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > @@ -637,6 +637,8 @@ static int rcar_mipi_dsi_host_attach(struct > > mipi_dsi_host *host, > > struct mipi_dsi_device *device) > > { > > struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host); > > + struct drm_panel *panel; > > + int ret; > > > > if (device->lanes > dsi->num_data_lanes) > > return -EINVAL; > > @@ -644,12 +646,36 @@ static int rcar_mipi_dsi_host_attach(struct > > mipi_dsi_host *host, > > dsi->lanes = device->lanes; > > dsi->format = device->format; > > > > + ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 1, 0, &panel, > > + &dsi->next_bridge); > > + if (ret) { > > + dev_err_probe(dsi->dev, ret, "could not find next bridge\n"); > > dev_err_probe() should only be used in probe(), and this function isn't > guaranteed to be called at probe time only. > > This isn't a big deal as the next patch fixes this, and both will be > squashed. Furthermore, rcar_mipi_dsi_host_attach() should only be called > when the DSI device gets registered, which should happen after it > registers its bridge, so I don't think we can see a probe deferral here. > > Other than that the patch looks fine, I'll squash it with the DSI > driver. > > > + return ret; > > + } > > + > > + if (!dsi->next_bridge) { > > + dsi->next_bridge = devm_drm_panel_bridge_add(dsi->dev, panel); > > + if (IS_ERR(dsi->next_bridge)) { > > + dev_err(dsi->dev, "failed to create panel bridge\n"); > > + return PTR_ERR(dsi->next_bridge); > > + } > > + } > > + > > + /* Initialize the DRM bridge. */ > > + dsi->bridge.funcs = &rcar_mipi_dsi_bridge_ops; > > + dsi->bridge.of_node = dsi->dev->of_node; > > + drm_bridge_add(&dsi->bridge); > > + > > return 0; > > } > > > > static int rcar_mipi_dsi_host_detach(struct mipi_dsi_host *host, > > struct mipi_dsi_device *device) > > { > > + struct rcar_mipi_dsi *dsi = host_to_rcar_mipi_dsi(host); > > + > > + drm_bridge_remove(&dsi->bridge); > > + > > return 0; > > } > > > > @@ -731,7 +757,6 @@ static int rcar_mipi_dsi_get_clocks(struct > > rcar_mipi_dsi *dsi) > > static int rcar_mipi_dsi_probe(struct platform_device *pdev) > > { > > struct rcar_mipi_dsi *dsi; > > - struct drm_panel *panel; > > struct resource *mem; > > int ret; > > > > @@ -764,21 +789,6 @@ static int rcar_mipi_dsi_probe(struct platform_device > > *pdev) > > return PTR_ERR(dsi->rstc); > > } > > > > - ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 1, 0, &panel, > > - &dsi->next_bridge); > > - if (ret) { > > - dev_err_probe(dsi->dev, ret, "could not find next bridge\n"); > > - return ret; > > - } > > - > > - if (!dsi->next_bridge) { > > - dsi->next_bridge = devm_drm_panel_bridge_add(dsi->dev, panel); > > - if (IS_ERR(dsi->next_bridge)) { > > - dev_err(dsi->dev, "failed to create panel bridge\n"); > > - return PTR_ERR(dsi->next_bridge); > > - } > > - } > > - > > /* Initialize the DSI host. */ > > dsi->host.dev = dsi->dev; > > dsi->host.ops = &rcar_mipi_dsi_host_ops; > > @@ -786,11 +796,6 @@ static int rcar_mipi_dsi_probe(struct platform_device > > *pdev) > > if (ret < 0) > > return ret; > > > > - /* Initialize the DRM bridge. */ > > -
[PATCH v3 0/2] drm/msm: Fix dsi/bridge probe
Context, from patch 2/2: Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the DSI host gets initialized earlier, but this caused unability to probe the entire stack of components because they all depend on interrupts coming from the main `mdss` node (mdp5, or dpu1). Changes in v3: - Removed a forgotten (and wrong) kfree() call. Series v2: After a very nice conversation with Dmitry, it turned out that my first approach to solve this issue wasn't great: even though it appeared to actually work, it was introducing a number of issues, one of which was critical as it was not removing down the DRM device when it's supposed to. Instead of actually fixing that patch, I went for "simplifying" the approach by not initializing the entire MDSS, but just the interrupt controller, which still untangles the infinite probe deferrals, but actually doesn't even touch most of the already present logic in place. AngeloGioacchino Del Regno (2): drm/msm: Allocate msm_drm_private early and pass it as driver data drm/msm: Initialize MDSS irq domain at probe time drivers/gpu/drm/msm/adreno/adreno_device.c | 16 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 50 ++-- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 58 -- drivers/gpu/drm/msm/dp/dp_display.c| 10 +--- drivers/gpu/drm/msm/dsi/dsi.c | 6 +- drivers/gpu/drm/msm/edp/edp.c | 6 +- drivers/gpu/drm/msm/hdmi/hdmi.c| 7 +-- drivers/gpu/drm/msm/msm_drv.c | 68 +- drivers/gpu/drm/msm/msm_kms.h | 3 + 11 files changed, 133 insertions(+), 98 deletions(-) -- 2.33.1
[PATCH v3 1/2] drm/msm: Allocate msm_drm_private early and pass it as driver data
In preparation for registering the mdss interrupt controller earlier, move the allocation of msm_drm_private from component bind time to msm_drv probe; this also allows us to use the devm variant of kzalloc. Since it is not right to allocate the drm_device at probe time (as it should exist only when all components are bound, and taken down when components get cleaned up), the only way to make this happen is to pass a pointer to msm_drm_private as driver data (like done in many other DRM drivers), instead of one to drm_device like it's currently done in this driver. This is also simplifying some bind/unbind functions around drm/msm, as some of them are using drm_device just to grab a pointer to the msm_drm_private structure, which we now retrieve in one call. Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/msm/adreno/adreno_device.c | 16 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 4 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +- drivers/gpu/drm/msm/dp/dp_display.c| 10 ++--- drivers/gpu/drm/msm/dsi/dsi.c | 6 +-- drivers/gpu/drm/msm/edp/edp.c | 6 +-- drivers/gpu/drm/msm/hdmi/hdmi.c| 7 ++-- drivers/gpu/drm/msm/msm_drv.c | 46 +- 8 files changed, 38 insertions(+), 60 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index 2a6ce76656aa..6b113881aefb 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -427,13 +427,6 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev) return gpu; } -static void set_gpu_pdev(struct drm_device *dev, - struct platform_device *pdev) -{ - struct msm_drm_private *priv = dev->dev_private; - priv->gpu_pdev = pdev; -} - static int find_chipid(struct device *dev, struct adreno_rev *rev) { struct device_node *node = dev->of_node; @@ -482,8 +475,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data) { static struct adreno_platform_config config = {}; const struct adreno_info *info; - struct drm_device *drm = dev_get_drvdata(master); - struct msm_drm_private *priv = drm->dev_private; + struct msm_drm_private *priv = dev_get_drvdata(master); + struct drm_device *drm = priv->dev; struct msm_gpu *gpu; int ret; @@ -492,7 +485,7 @@ static int adreno_bind(struct device *dev, struct device *master, void *data) return ret; dev->platform_data = &config; - set_gpu_pdev(drm, to_platform_device(dev)); + priv->gpu_pdev = to_platform_device(dev); info = adreno_info(config.rev); @@ -521,12 +514,13 @@ static int adreno_bind(struct device *dev, struct device *master, void *data) static void adreno_unbind(struct device *dev, struct device *master, void *data) { + struct msm_drm_private *priv = dev_get_drvdata(master); struct msm_gpu *gpu = dev_to_gpu(dev); pm_runtime_force_suspend(dev); gpu->funcs->destroy(gpu); - set_gpu_pdev(dev_get_drvdata(master), NULL); + priv->gpu_pdev = NULL; } static const struct component_ops a3xx_ops = { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index a15b26428280..8b038690d9ce 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -1153,9 +1153,9 @@ struct msm_kms *dpu_kms_init(struct drm_device *dev) static int dpu_bind(struct device *dev, struct device *master, void *data) { - struct drm_device *ddev = dev_get_drvdata(master); + struct msm_drm_private *priv = dev_get_drvdata(master); struct platform_device *pdev = to_platform_device(dev); - struct msm_drm_private *priv = ddev->dev_private; + struct drm_device *ddev = priv->dev; struct dpu_kms *dpu_kms; struct dss_module_power *mp; int ret = 0; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index 7b242246d4e7..cab6451661b2 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -936,7 +936,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) static int mdp5_bind(struct device *dev, struct device *master, void *data) { - struct drm_device *ddev = dev_get_drvdata(master); + struct msm_drm_private *priv = dev_get_drvdata(master); + struct drm_device *ddev = priv->dev; struct platform_device *pdev = to_platform_device(dev); DBG(""); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 0eb3c007d503..ac29a8a99450 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -224,13 +224,10 @@ static int dp_display_bind(struct device *dev, struc
[PATCH v3 2/2] drm/msm: Initialize MDSS irq domain at probe time
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the DSI host gets initialized earlier, but this caused unability to probe the entire stack of components because they all depend on interrupts coming from the main `mdss` node (mdp5, or dpu1). To fix this issue, anticipate registering the irq domain from mdp5/dpu1 at msm_mdev_probe() time, as to make sure that the interrupt controller is available before dsi and/or other components try to initialize, finally satisfying the dependency. Moreover, to balance this operation while avoiding to always check if the irq domain is registered everytime we call bind() on msm_pdev, add a new *remove function pointer to msm_mdss_funcs, used to remove the irq domain only at msm_pdev_remove() time. Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order") Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 50 --- drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 58 +++ drivers/gpu/drm/msm/msm_drv.c | 22 - drivers/gpu/drm/msm/msm_kms.h | 3 ++ 4 files changed, 95 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c index b466784d9822..6c2569175633 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c @@ -106,13 +106,10 @@ static const struct irq_domain_ops dpu_mdss_irqdomain_ops = { .xlate = irq_domain_xlate_onecell, }; -static int _dpu_mdss_irq_domain_add(struct dpu_mdss *dpu_mdss) +static int _dpu_mdss_irq_domain_add(struct device *dev, struct dpu_mdss *dpu_mdss) { - struct device *dev; struct irq_domain *domain; - dev = dpu_mdss->base.dev->dev; - domain = irq_domain_add_linear(dev->of_node, 32, &dpu_mdss_irqdomain_ops, dpu_mdss); if (!domain) { @@ -194,7 +191,6 @@ static void dpu_mdss_destroy(struct drm_device *dev) pm_runtime_suspend(dev->dev); pm_runtime_disable(dev->dev); - _dpu_mdss_irq_domain_fini(dpu_mdss); irq = platform_get_irq(pdev, 0); irq_set_chained_handler_and_data(irq, NULL, NULL); msm_dss_put_clk(mp->clk_config, mp->num_clk); @@ -203,15 +199,43 @@ static void dpu_mdss_destroy(struct drm_device *dev) if (dpu_mdss->mmio) devm_iounmap(&pdev->dev, dpu_mdss->mmio); dpu_mdss->mmio = NULL; - priv->mdss = NULL; +} + +static void dpu_mdss_remove(struct msm_mdss *mdss) +{ + _dpu_mdss_irq_domain_fini(to_dpu_mdss(mdss)); } static const struct msm_mdss_funcs mdss_funcs = { .enable = dpu_mdss_enable, .disable = dpu_mdss_disable, .destroy = dpu_mdss_destroy, + .remove = dpu_mdss_remove, }; +int dpu_mdss_early_init(struct device *dev, struct msm_drm_private *priv) +{ + struct dpu_mdss *dpu_mdss; + int ret; + + dpu_mdss = devm_kzalloc(dev, sizeof(*dpu_mdss), GFP_KERNEL); + if (!dpu_mdss) + return -ENOMEM; + + ret = _dpu_mdss_irq_domain_add(dev, dpu_mdss); + if (ret) + return ret; + + /* +* Here we have no drm_device yet, but still do the assignment +* so that we can retrieve our struct dpu_mdss from the main +* init function, since we allocate it here. +*/ + priv->mdss = &dpu_mdss->base; + + return 0; +} + int dpu_mdss_init(struct drm_device *dev) { struct platform_device *pdev = to_platform_device(dev->dev); @@ -221,9 +245,9 @@ int dpu_mdss_init(struct drm_device *dev) int ret; int irq; - dpu_mdss = devm_kzalloc(dev->dev, sizeof(*dpu_mdss), GFP_KERNEL); + dpu_mdss = to_dpu_mdss(priv->mdss); if (!dpu_mdss) - return -ENOMEM; + return -ENODATA; dpu_mdss->mmio = msm_ioremap(pdev, "mdss", "mdss"); if (IS_ERR(dpu_mdss->mmio)) @@ -241,10 +265,6 @@ int dpu_mdss_init(struct drm_device *dev) dpu_mdss->base.dev = dev; dpu_mdss->base.funcs = &mdss_funcs; - ret = _dpu_mdss_irq_domain_add(dpu_mdss); - if (ret) - goto irq_domain_error; - irq = platform_get_irq(pdev, 0); if (irq < 0) { ret = irq; @@ -253,16 +273,10 @@ int dpu_mdss_init(struct drm_device *dev) irq_set_chained_handler_and_data(irq, dpu_mdss_irq, dpu_mdss); - - priv->mdss = &dpu_mdss->base; - pm_runtime_enable(dev->dev); - return 0; irq_error: - _dpu_mdss_irq_domain_fini(dpu_mdss); -irq_domain_error: msm_dss_put_clk(mp->clk_config, mp->num_clk); clk_parse_err: devm_kfree(&pdev->dev, mp->clk_config); diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c index 0ea53420bc40..a99538ae4182 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c +++ b/drivers/gpu/drm/m
Re: [Linaro-mm-sig] [RFC PATCH 1/2] dma-fence: Avoid establishing a locking order between fence classes
On 12/1/21 11:32, Christian König wrote: Am 01.12.21 um 11:15 schrieb Thomas Hellström (Intel): [SNIP] What we could do is to avoid all this by not calling the callback with the lock held in the first place. If that's possible that might be a good idea, pls also see below. The problem with that is dma_fence_signal_locked()/dma_fence_signal_timestamp_locked(). If we could avoid using that or at least allow it to drop the lock then we could call the callback without holding it. Somebody would need to audit the drivers and see if holding the lock is really necessary anywhere. /Thomas Oh, and a follow up question: If there was a way to break the recursion on final put() (using the same basic approach as patch 2 in this series uses to break recursion in enable_signaling()), so that none of these containers did require any special treatment, would it be worth pursuing? I guess it might be possible by having the callbacks drop the references rather than the loop in the final put. + a couple of changes in code iterating over the fence pointers. That won't really help, you just move the recursion from the final put into the callback. How do we recurse from the callback? The introduced fence_put() of individual fence pointers doesn't recurse anymore (at most 1 level), and any callback recursion is broken by the irq_work? Yeah, but then you would need to take another lock to avoid racing with dma_fence_array_signaled(). I figure the big amount of work would be to adjust code that iterates over the individual fence pointers to recognize that they are rcu protected. Could be that we could solve this with RCU, but that sounds like a lot of churn for no gain at all. In other words even with the problems solved I think it would be a really bad idea to allow chaining of dma_fence_array objects. Yes, that was really the question, Is it worth pursuing this? I'm not really suggesting we should allow this as an intentional feature. I'm worried, however, that if we allow these containers to start floating around cross-driver (or even internally) disguised as ordinary dma_fences, they would require a lot of driver special casing, or else completely unexpeced WARN_ON()s and lockdep splats would start to turn up, scaring people off from using them. And that would be a breeding ground for hairy driver-private constructs. Well the question is why we would want to do it? If it's to avoid inter driver lock dependencies by avoiding to call the callback with the spinlock held, then yes please. We had tons of problems with that, resulting in irq_work and work_item delegation all over the place. Yes, that sounds like something desirable, but in these containers, what's causing the lock dependencies is the enable_signaling() callback that is typically called locked. If it's to allow nesting of dma_fence_array instances, then it's most likely a really bad idea even if we fix all the locking order problems. Well I think my use-case where I hit a dead end may illustrate what worries me here: 1) We use a dma-fence-array to coalesce all dependencies for ttm object migration. 2) We use a dma-fence-chain to order the resulting dm_fence into a timeline because the TTM resource manager code requires that. Initially seemingly harmless to me. But after a sequence evict->alloc->clear, the dma-fence-chain feeds into the dma-fence-array for the clearing operation. Code still works fine, and no deep recursion, no warnings. But if I were to add another driver to the system that instead feeds a dma-fence-array into a dma-fence-chain, this would give me a lockdep splat. So then if somebody were to come up with the splendid idea of using a dma-fence-chain to initially coalesce fences, I'd hit the same problem or risk illegaly joining two dma-fence-chains together. To fix this, I would need to look at the incoming fences and iterate over any dma-fence-array or dma-fence-chain that is fed into the dma-fence-array to flatten out the input. In fact all dma-fence-array users would need to do that, and even dma-fence-chain users watching out for not joining chains together or accidently add an array that perhaps came as a disguised dma-fence from antother driver. So the purpose to me would be to allow these containers as input to eachother without a lot of in-driver special-casing, be it by breaking recursion on built-in flattening to avoid a) Hitting issues in the future or with existing interoperating drivers. b) Avoid driver-private containers that also might break the interoperability. (For example the i915 currently driver-private dma_fence_work avoid all these problems, but we're attempting to address issues in common code rather than re-inventing stuff internally). /Thomas Christian. /Thomas Christian. Thanks, /Thomas
Re: [Intel-gfx] [PATCH v2 00/16] drm/i915: Remove short term pins from execbuf.
On 30-11-2021 19:38, Tvrtko Ursulin wrote: > > On 30/11/2021 11:17, Maarten Lankhorst wrote: >> On 30-11-2021 09:54, Tvrtko Ursulin wrote: >>> >>> Hi, >>> >>> On 29/11/2021 13:47, Maarten Lankhorst wrote: New version of the series, with feedback from previous series added. >>> >>> If there was a cover letter sent for this work in the past could you please >>> keep attaching it? Or if there wasn't, could you please write one? >>> >>> I am worried about two things. First is that we need to have a high level >>> overview of the rules/design changes documented so third party people have >>> any hope of getting code right after this lands. (Where we are, where we >>> are going, how we will get there, how far did we get and when we will get >>> to the end.) >>> >>> Second is that when parts of the series land piecemeal (Which they have in >>> this right, right?), it gets very hard to write up a maintainer level >>> changelog. >> >> The preparation part is to ensure we always hold vma->obj->resv when >> unbinding. >> >> The first preparation series ensured vma->obj always existed. This was not >> the case for mock gtt and gen6 aliasing gtt. This allowed us to remove all >> the special handling for those uncommon cases, and actually enforce we can >> always take that lock. This part is merged. > > Sounds good. But also mention the high level motivation for why we always > want to hold vma->obj->resv when unbinding in the introduction as well. > >> >> Patch 2-11 in this series adds the vma->obj->resv to eviction and shrinker. >> Those are the only parts where we don't take the lock yet. >> >> After that, we always hold the lock when required, and we can start >> requiring the obj-> resv lock when unbinding. This is completed in patch 15. >> >> With that fixed, removing short term pins can be done, because for unbind we >> now always take obj->resv, so holding obj->resv during execbuf submission is >> sufficient, and all short term pinning can be removed. > > I'd also like the cover letter to contain a high level description on _why_ > is removing short term pins needed or beneficial. > > What was the flow and object lifetimes so far, and what it will be going > forward etc. Previously, short term pinning in execbuf was required because i915_vma was effectively independent from objects, and has its own refcount, locking, and lifetime rules and pinning. This series removes the separate locking, by requiring vma->obj->resv to be held when pinning and unbinding. This will also be required for VM_BIND work. With pinning required for pinning and unbinding, the lock is enough to prevent unbinding when trying to pin with the lock held, like in execbuf. This makes binding/unbinding similar to ttm_bo_validate()'s use, which just cares that an object is in a certain place, without pinning it in place. Having it part of gem bo removes a lot of the vma refcounting, and makes i915_vma more a part of the bo, instead of its own floating object that just happens to be part of a bo. This is also required to make it more compatible with TTM, and migration in general. For future work, it makes things a lot simpler and clear. We want to end up with i915_vma just being a specific mapping of the BO, just like is the case in other drivers. i915_vma->active removal is the next step there, and makes it when object is destroyed, the bindings are destroyed (after idle), instead of object being destroyed when bindings are idle. >> >> We only pin temporarily when calling i915_gem_evict_vm in execbuf, which >> could also be handled in theory by just marking all objects as unpinned. >> >> As a bonus, using TTM for delayed eviction on all objects becomes easy, just >> need to get rid of i915_active in i915_vma, as it keeps the object refcount >> alive. >> >> Remainder is removing refcount to i915_vma, to make it a real > > Sounds on the right track with maybe a bit more text so the readers can > easily understand it on the higher level. With the obj->resv being the master lock, pinning for execbuf becomes obsolete and can be removed. > >> >>> But in any case, even on the mundane process level, we need to have cover >>> letters for any non trivial work was the conclusion since some time ago. >> >> Here you go! I hope it explains the reasoning. > > It is on the right track. I think just needs to be expanded a bit with high > level direction and plan, pointing out where in the grand scheme this series > is. And then don't forget to add the improved text as cover letter when > sending next time please. > > Regards, > > Tvrtko
Re: [Linaro-mm-sig] [RFC PATCH 1/2] dma-fence: Avoid establishing a locking order between fence classes
Am 01.12.21 um 12:04 schrieb Thomas Hellström (Intel): On 12/1/21 11:32, Christian König wrote: Am 01.12.21 um 11:15 schrieb Thomas Hellström (Intel): [SNIP] What we could do is to avoid all this by not calling the callback with the lock held in the first place. If that's possible that might be a good idea, pls also see below. The problem with that is dma_fence_signal_locked()/dma_fence_signal_timestamp_locked(). If we could avoid using that or at least allow it to drop the lock then we could call the callback without holding it. Somebody would need to audit the drivers and see if holding the lock is really necessary anywhere. /Thomas Oh, and a follow up question: If there was a way to break the recursion on final put() (using the same basic approach as patch 2 in this series uses to break recursion in enable_signaling()), so that none of these containers did require any special treatment, would it be worth pursuing? I guess it might be possible by having the callbacks drop the references rather than the loop in the final put. + a couple of changes in code iterating over the fence pointers. That won't really help, you just move the recursion from the final put into the callback. How do we recurse from the callback? The introduced fence_put() of individual fence pointers doesn't recurse anymore (at most 1 level), and any callback recursion is broken by the irq_work? Yeah, but then you would need to take another lock to avoid racing with dma_fence_array_signaled(). I figure the big amount of work would be to adjust code that iterates over the individual fence pointers to recognize that they are rcu protected. Could be that we could solve this with RCU, but that sounds like a lot of churn for no gain at all. In other words even with the problems solved I think it would be a really bad idea to allow chaining of dma_fence_array objects. Yes, that was really the question, Is it worth pursuing this? I'm not really suggesting we should allow this as an intentional feature. I'm worried, however, that if we allow these containers to start floating around cross-driver (or even internally) disguised as ordinary dma_fences, they would require a lot of driver special casing, or else completely unexpeced WARN_ON()s and lockdep splats would start to turn up, scaring people off from using them. And that would be a breeding ground for hairy driver-private constructs. Well the question is why we would want to do it? If it's to avoid inter driver lock dependencies by avoiding to call the callback with the spinlock held, then yes please. We had tons of problems with that, resulting in irq_work and work_item delegation all over the place. Yes, that sounds like something desirable, but in these containers, what's causing the lock dependencies is the enable_signaling() callback that is typically called locked. If it's to allow nesting of dma_fence_array instances, then it's most likely a really bad idea even if we fix all the locking order problems. Well I think my use-case where I hit a dead end may illustrate what worries me here: 1) We use a dma-fence-array to coalesce all dependencies for ttm object migration. 2) We use a dma-fence-chain to order the resulting dm_fence into a timeline because the TTM resource manager code requires that. Initially seemingly harmless to me. But after a sequence evict->alloc->clear, the dma-fence-chain feeds into the dma-fence-array for the clearing operation. Code still works fine, and no deep recursion, no warnings. But if I were to add another driver to the system that instead feeds a dma-fence-array into a dma-fence-chain, this would give me a lockdep splat. So then if somebody were to come up with the splendid idea of using a dma-fence-chain to initially coalesce fences, I'd hit the same problem or risk illegaly joining two dma-fence-chains together. To fix this, I would need to look at the incoming fences and iterate over any dma-fence-array or dma-fence-chain that is fed into the dma-fence-array to flatten out the input. In fact all dma-fence-array users would need to do that, and even dma-fence-chain users watching out for not joining chains together or accidently add an array that perhaps came as a disguised dma-fence from antother driver. So the purpose to me would be to allow these containers as input to eachother without a lot of in-driver special-casing, be it by breaking recursion on built-in flattening to avoid a) Hitting issues in the future or with existing interoperating drivers. b) Avoid driver-private containers that also might break the interoperability. (For example the i915 currently driver-private dma_fence_work avoid all these problems, but we're attempting to address issues in common code rather than re-inventing stuff internally). I don't think that a dma_fence_array or dma_fence_chain is the right thing to begin with in those use cases. When you wan
[PATCH v2 1/3] dt-bindings: ls2k1000: add gpu device node
From: suijingfeng There is a vivante gpu (GC1000 V5037) in ls2k1000, but it is pci device not platform device. ls2k1000 is dual-core mips64 cpu made by loongson. Signed-off-by: suijingfeng Signed-off-by: Sui Jingfeng <15330273...@189.cn> --- arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi | 11 +++ 1 file changed, 11 insertions(+) diff --git a/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi b/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi index bfc3d3243ee7..f1feffac78a6 100644 --- a/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi +++ b/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi @@ -193,6 +193,17 @@ interrupt-parent = <&liointc0>; }; + gpu@5,0 { + compatible = "pci0014,7a05.0", + "pci0014,7a05", + "pciclass030200", + "pciclass0302"; + + reg = <0x2800 0x0 0x0 0x0 0x0>; + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; + interrupt-parent = <&liointc0>; + }; + pci_bridge@9,0 { compatible = "pci0014,7a19.0", "pci0014,7a19", -- 2.20.1
[PATCH v2 1/3] dt-bindings: ls2k1000: add gpu device node
From: suijingfeng There is a vivante gpu (GC1000 V5037) in ls2k1000, but it is pci device not platform device. ls2k1000 is dual-core mips64 cpu made by loongson. Signed-off-by: suijingfeng Signed-off-by: Sui Jingfeng <15330273...@189.cn> --- arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi | 11 +++ 1 file changed, 11 insertions(+) diff --git a/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi b/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi index bfc3d3243ee7..f1feffac78a6 100644 --- a/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi +++ b/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi @@ -193,6 +193,17 @@ interrupt-parent = <&liointc0>; }; + gpu@5,0 { + compatible = "pci0014,7a05.0", + "pci0014,7a05", + "pciclass030200", + "pciclass0302"; + + reg = <0x2800 0x0 0x0 0x0 0x0>; + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; + interrupt-parent = <&liointc0>; + }; + pci_bridge@9,0 { compatible = "pci0014,7a19.0", "pci0014,7a19", -- 2.20.1
[PATCH v2 2/3] drm/etnaviv: add pci device driver support
From: suijingfeng v2: update commits and provide more material. There is a Vivante GC1000 V5037 in LS2K1000 and LS7A1000, the gpu in those chip is a PCI device not platform one. This GPU have 2D and 3D in the same core, so component framework can be avoid, in fact we find it is diffcult to use it with a pci device driver. Therefore, this patch try to provide PCI device driver wrapper for it by mimic the platform counterpart. LS7A1000 is a bridge chip, brief user manual can be read on line form[1]. This bridge chip typically use with LS3A4000 (4 core 1.8gHz, Mips64r5) and LS3A5000 (4 core loongarch 2.5Ghz)[2]. While LS2K1000[3] is a double core 1.0Ghz Mips64r2 SoC. Loongson CPU's cache coherency is maintained by the hardware, this is tramendous difference from other Mips or ARM, as the cohercecy problems can be ignored when using cached coherent BOs. On the other hand, current Mips kernel's writecombine support is not being implemented correctly under the hardware maintained cache coherency framework. At lease this is true for loongson's cpu. In other words, ETNA_BO_WC is NOT usable on our platform, it suffer from cache coherency problem. Howerver use ETNA_BO_CACHED_COHERENT instead of ETNA_BO_WC, etnaviv driver works well on our platform. Both LS7A1000 and LS2K1000 have a display controller integrated, named lsdc. By using KMS-RO framework, lsdc and gc1000 made a compatible pair. I have write xf86-video-loongson[4] DDX driver which make etnaviv works with lsdc on our custom debian and fedora, glmark2-es2 and glxgears can be hardware accelerated under X11 environment. (Mesa-18.3.6 and Mesa-18.05 are tested and it works) See https://github.com/suijingfeng/xf86-video-loongson. [1] https://loongson.github.io/LoongArch-Documentation/Loongson-7A1000-usermanual-EN.html [2] https://loongson.github.io/LoongArch-Documentation/Loongson-3A5000-usermanual-EN.html [3] https://wiki.debian.org/InstallingDebianOn/Lemote/Loongson2K1000 Signed-off-by: suijingfeng Signed-off-by: Sui Jingfeng <15330273...@189.cn> --- drivers/gpu/drm/etnaviv/Kconfig | 12 ++ drivers/gpu/drm/etnaviv/Makefile | 2 + drivers/gpu/drm/etnaviv/etnaviv_drv.c | 113 --- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 8 + drivers/gpu/drm/etnaviv/etnaviv_gem.c | 28 ++- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 106 +++ drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 6 + drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c | 218 ++ include/uapi/drm/etnaviv_drm.h| 11 +- 9 files changed, 430 insertions(+), 74 deletions(-) create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig index faa7fc68b009..5858eec59025 100644 --- a/drivers/gpu/drm/etnaviv/Kconfig +++ b/drivers/gpu/drm/etnaviv/Kconfig @@ -15,6 +15,18 @@ config DRM_ETNAVIV help DRM driver for Vivante GPUs. +config DRM_ETNAVIV_PCI_DRIVER + bool "Enable PCI device driver support" + depends on DRM_ETNAVIV + depends on PCI + depends on MACH_LOONGSON64 + default y + help + DRM PCI driver for the Vivante GPU in LS7A1000 north bridge + and LS2K1000 SoC. The GC1000 in LS2K1000 and LS7A1000 is a + PCI device. + If in doubt, say "n". + config DRM_ETNAVIV_THERMAL bool "enable ETNAVIV thermal throttling" depends on DRM_ETNAVIV diff --git a/drivers/gpu/drm/etnaviv/Makefile b/drivers/gpu/drm/etnaviv/Makefile index 46e5ffad69a6..6829e1ebf2db 100644 --- a/drivers/gpu/drm/etnaviv/Makefile +++ b/drivers/gpu/drm/etnaviv/Makefile @@ -16,4 +16,6 @@ etnaviv-y := \ etnaviv_perfmon.o \ etnaviv_sched.o +etnaviv-$(CONFIG_DRM_ETNAVIV_PCI_DRIVER) += etnaviv_pci_drv.o + obj-$(CONFIG_DRM_ETNAVIV) += etnaviv.o diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 7dcc6392792d..dc2bb3a6efe6 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -8,6 +8,9 @@ #include #include #include +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER +#include +#endif #include #include @@ -72,7 +75,7 @@ static int etnaviv_open(struct drm_device *dev, struct drm_file *file) drm_sched_entity_init(&ctx->sched_entity[i], DRM_SCHED_PRIORITY_NORMAL, &sched, 1, NULL); - } + } } file->driver_priv = ctx; @@ -470,7 +473,7 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = { DEFINE_DRM_GEM_FOPS(fops); -static const struct drm_driver etnaviv_drm_driver = { +const struct drm_driver etnaviv_drm_driver = { .driver_features= DRIVER_GEM | DRIVER_RENDER, .open = etnaviv_open, .postclose = etnavi
[PATCH v2 3/3] mips: loongson64: enable etnaviv drm driver on ls2k1000 and ls3a4000
From: suijingfeng v2: merge the last two trival patches into one patch. Signed-off-by: suijingfeng Signed-off-by: Sui Jingfeng <15330273...@189.cn> --- arch/mips/configs/loongson2k_defconfig | 1 + arch/mips/configs/loongson3_defconfig | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/mips/configs/loongson2k_defconfig b/arch/mips/configs/loongson2k_defconfig index e948ca487e2d..194f39d07332 100644 --- a/arch/mips/configs/loongson2k_defconfig +++ b/arch/mips/configs/loongson2k_defconfig @@ -243,6 +243,7 @@ CONFIG_MEDIA_USB_SUPPORT=y CONFIG_USB_VIDEO_CLASS=m CONFIG_DRM=y CONFIG_DRM_RADEON=y +CONFIG_DRM_ETNAVIV=m CONFIG_FB_RADEON=y CONFIG_LCD_CLASS_DEVICE=y CONFIG_LCD_PLATFORM=m diff --git a/arch/mips/configs/loongson3_defconfig b/arch/mips/configs/loongson3_defconfig index 25ecd15bc952..65987c5abe83 100644 --- a/arch/mips/configs/loongson3_defconfig +++ b/arch/mips/configs/loongson3_defconfig @@ -280,6 +280,7 @@ CONFIG_MEDIA_USB_SUPPORT=y CONFIG_USB_VIDEO_CLASS=m CONFIG_DRM=y CONFIG_DRM_RADEON=m +CONFIG_DRM_ETNAVIV=m CONFIG_DRM_QXL=y CONFIG_DRM_VIRTIO_GPU=y CONFIG_FB=y -- 2.20.1
Re: [PATCH v3 2/2] drm: rcar-du: Add R-Car DSI driver
Hi Jagan, On Wed, Dec 01, 2021 at 04:12:39PM +0530, Jagan Teki wrote: > On Wed, Dec 1, 2021 at 10:54 AM Laurent Pinchart wrote: > > > > From: LUU HOAI > > > > The driver supports the MIPI DSI/CSI-2 TX encoder found in the R-Car V3U > > SoC. It currently supports DSI mode only. > > > > Signed-off-by: LUU HOAI > > Signed-off-by: Laurent Pinchart > > Reviewed-by: Kieran Bingham > > Tested-by: Kieran Bingham > > Acked-by: Sam Ravnborg > > --- > > Changes since v2: > > > > - Support probing of child DSI devices > > - Use devm_drm_of_get_bridge() helper > > > > Changes since v1: > > > > - Use U suffix for unsigned constants > > - Fix indentation in Makefile > > - Select DRM_MIPI_DSI > > - Report correct fout frequency in debug message > > - Move dsi_setup_info.err to local variable > > --- > > drivers/gpu/drm/rcar-du/Kconfig | 7 + > > drivers/gpu/drm/rcar-du/Makefile | 1 + > > drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 817 +++ > > drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h | 172 > > 4 files changed, 997 insertions(+) > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h > > > > diff --git a/drivers/gpu/drm/rcar-du/Kconfig > > b/drivers/gpu/drm/rcar-du/Kconfig > > index 3e588ddba245..1675df21d91f 100644 > > --- a/drivers/gpu/drm/rcar-du/Kconfig > > +++ b/drivers/gpu/drm/rcar-du/Kconfig > > @@ -45,6 +45,13 @@ config DRM_RCAR_LVDS > > select OF_FLATTREE > > select OF_OVERLAY > > > > +config DRM_RCAR_MIPI_DSI > > + tristate "R-Car DU MIPI DSI Encoder Support" > > + depends on DRM && DRM_BRIDGE && OF > > + select DRM_MIPI_DSI > > + help > > + Enable support for the R-Car Display Unit embedded MIPI DSI > > encoders. > > + > > config DRM_RCAR_VSP > > bool "R-Car DU VSP Compositor Support" if ARM > > default y if ARM64 > > diff --git a/drivers/gpu/drm/rcar-du/Makefile > > b/drivers/gpu/drm/rcar-du/Makefile > > index 4d1187ccc3e5..286bc81b3e7c 100644 > > --- a/drivers/gpu/drm/rcar-du/Makefile > > +++ b/drivers/gpu/drm/rcar-du/Makefile > > @@ -19,6 +19,7 @@ obj-$(CONFIG_DRM_RCAR_CMM)+= rcar_cmm.o > > obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o > > obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o > > obj-$(CONFIG_DRM_RCAR_LVDS)+= rcar_lvds.o > > +obj-$(CONFIG_DRM_RCAR_MIPI_DSI)+= rcar_mipi_dsi.o > > > > # 'remote-endpoint' is fixed up at run-time > > DTC_FLAGS_rcar_du_of_lvds_r8a7790 += -Wno-graph_endpoint > > diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > new file mode 100644 > > index ..fcaec3308d68 > > --- /dev/null > > +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > @@ -0,0 +1,817 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * rcar_mipi_dsi.c -- R-Car MIPI DSI Encoder > > + * > > + * Copyright (C) 2020 Renesas Electronics Corporation > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "rcar_mipi_dsi_regs.h" > > + > > +struct rcar_mipi_dsi { > > + struct device *dev; > > + const struct rcar_mipi_dsi_device_info *info; > > + struct reset_control *rstc; > > + > > + struct mipi_dsi_host host; > > + struct drm_bridge bridge; > > + struct drm_bridge *next_bridge; > > + struct drm_connector connector; > > + > > + void __iomem *mmio; > > + struct { > > + struct clk *mod; > > + struct clk *pll; > > + struct clk *dsi; > > + } clocks; > > + > > + struct drm_display_mode display_mode; > > + enum mipi_dsi_pixel_format format; > > + unsigned int num_data_lanes; > > + unsigned int lanes; > > +}; > > + > > +static inline struct rcar_mipi_dsi * > > +bridge_to_rcar_mipi_dsi(struct drm_bridge *bridge) > > +{ > > + return container_of(bridge, struct rcar_mipi_dsi, bridge); > > +} > > + > > +static inline struct rcar_mipi_dsi * > > +host_to_rcar_mipi_dsi(struct mipi_dsi_host *host) > > +{ > > + return container_of(host, struct rcar_mipi_dsi, host); > > +} > > + > > +static const u32 phtw[] = { > > + 0x01020114, 0x01600115, /* General testing */ > > + 0x01030116, 0x0102011d, /* General testing */ > > + 0x011101a4, 0x018601a4, /* 1Gbps testing */ > > + 0x014201a0, 0x010001a3, /* 1Gbps testing */ > > + 0x0101011f, /* 1Gbps testing */ > > +}; > > + > > +static const u32 phtw2[] = { > > + 0x010c0130, 0x010c0140, /* General testing */ > > + 0x010c0150, 0x010c0180, /* General testing */ > > + 0x010c0190, > > + 0x010a0160, 0x010a0170,
[PATCH v2 2/7] agp: Include "compat_ioctl.h" where necessary
Fix compiler warnings like drivers/char/agp/frontend.c:46:20: warning: no previous prototype for 'agp_find_mem_by_key' [-Wmissing-prototypes] 46 | struct agp_memory *agp_find_mem_by_key(int key) by including the compat_ioctl.h in the source file. Signed-off-by: Thomas Zimmermann Acked-by: Helge Deller --- drivers/char/agp/frontend.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/char/agp/frontend.c b/drivers/char/agp/frontend.c index 6802a6bbf0f2..321118a9cfa5 100644 --- a/drivers/char/agp/frontend.c +++ b/drivers/char/agp/frontend.c @@ -39,7 +39,9 @@ #include #include #include + #include "agp.h" +#include "compat_ioctl.h" struct agp_front_data agp_fe; -- 2.34.0
[PATCH v2 0/7] agp: Various minor fixes
Fix a number of compiler warnings in the AGP drivers. No functional changes. v2: * ati-agp: free page in error branch (Helge) * nvidia-agp: Mark temp as __maybe_unused (Helge) Thomas Zimmermann (7): agp: Remove trailing whitespaces agp: Include "compat_ioctl.h" where necessary agp: Documentation fixes agp/ati: Return error from ati_create_page_map() agp/nvidia: Declare value returned by readl() as unused agp/sworks: Remove unused variable 'current_size' agp/via: Remove unused variable 'current_size' drivers/char/agp/ati-agp.c| 8 ++-- drivers/char/agp/backend.c| 2 ++ drivers/char/agp/frontend.c | 4 +++- drivers/char/agp/nvidia-agp.c | 3 ++- drivers/char/agp/sworks-agp.c | 5 + drivers/char/agp/via-agp.c| 3 --- 6 files changed, 14 insertions(+), 11 deletions(-) base-commit: 6a8f90ec433e2f5de5fc16d7a4839771b7027cc0 -- 2.34.0
[PATCH v2 4/7] agp/ati: Return error from ati_create_page_map()
Fix the compiler warning drivers/char/agp/ati-agp.c: In function 'ati_create_page_map': drivers/char/agp/ati-agp.c:58:16: warning: variable 'err' set but not used [-Wunused-but-set-variable] 58 | int i, err = 0; by returing the error to the caller. v2: * free page in error branch (Helge) Signed-off-by: Thomas Zimmermann --- drivers/char/agp/ati-agp.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/char/agp/ati-agp.c b/drivers/char/agp/ati-agp.c index 857b37141a07..6f5530482d83 100644 --- a/drivers/char/agp/ati-agp.c +++ b/drivers/char/agp/ati-agp.c @@ -55,7 +55,7 @@ static struct _ati_generic_private { static int ati_create_page_map(struct ati_page_map *page_map) { - int i, err = 0; + int i, err; page_map->real = (unsigned long *) __get_free_page(GFP_KERNEL); if (page_map->real == NULL) @@ -63,6 +63,10 @@ static int ati_create_page_map(struct ati_page_map *page_map) set_memory_uc((unsigned long)page_map->real, 1); err = map_page_into_agp(virt_to_page(page_map->real)); + if (err) { + free_page((unsigned long)page_map->real); + return err; + } page_map->remapped = page_map->real; for (i = 0; i < PAGE_SIZE / sizeof(unsigned long); i++) { -- 2.34.0
[PATCH v2 6/7] agp/sworks: Remove unused variable 'current_size'
Fix the compiler warning drivers/char/agp/sworks-agp.c: In function 'serverworks_configure': drivers/char/agp/sworks-agp.c:265:37: warning: variable 'current_size' set but not used [-Wunused-but-set-variable] 265 | struct aper_size_info_lvl2 *current_size; by removing the variable. Signed-off-by: Thomas Zimmermann Acked-by: Helge Deller --- drivers/char/agp/sworks-agp.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/char/agp/sworks-agp.c b/drivers/char/agp/sworks-agp.c index b15d3d4f71d5..b91da5998dd7 100644 --- a/drivers/char/agp/sworks-agp.c +++ b/drivers/char/agp/sworks-agp.c @@ -262,13 +262,10 @@ static void serverworks_tlbflush(struct agp_memory *temp) static int serverworks_configure(void) { - struct aper_size_info_lvl2 *current_size; u32 temp; u8 enable_reg; u16 cap_reg; - current_size = A_SIZE_LVL2(agp_bridge->current_size); - /* Get the memory mapped registers */ pci_read_config_dword(agp_bridge->dev, serverworks_private.mm_addr_ofs, &temp); temp = (temp & PCI_BASE_ADDRESS_MEM_MASK); -- 2.34.0
[PATCH v2 3/7] agp: Documentation fixes
Fix compiler warnings drivers/char/agp/backend.c:68: warning: Function parameter or member 'pdev' not described in 'agp_backend_acquire' drivers/char/agp/backend.c:93: warning: Function parameter or member 'bridge' not described in 'agp_backend_release' by adding the necessary documentation. Signed-off-by: Thomas Zimmermann Acked-by: Helge Deller --- drivers/char/agp/backend.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/char/agp/backend.c b/drivers/char/agp/backend.c index 004a3ce8ba72..0e19c600db53 100644 --- a/drivers/char/agp/backend.c +++ b/drivers/char/agp/backend.c @@ -62,6 +62,7 @@ EXPORT_SYMBOL(agp_find_bridge); /** * agp_backend_acquire - attempt to acquire an agp backend. + * @pdev: the PCI device * */ struct agp_bridge_data *agp_backend_acquire(struct pci_dev *pdev) @@ -83,6 +84,7 @@ EXPORT_SYMBOL(agp_backend_acquire); /** * agp_backend_release - release the lock on the agp backend. + * @bridge: the AGP backend to release * * The caller must insure that the graphics aperture translation table * is read for use by another entity. -- 2.34.0
[PATCH v2 5/7] agp/nvidia: Declare value returned by readl() as unused
Fix the compiler warning drivers/char/agp/nvidia-agp.c: In function 'nvidia_tlbflush': drivers/char/agp/nvidia-agp.c:264:22: warning: variable 'temp' set but not used [-Wunused-but-set-variable] 264 | u32 wbc_reg, temp; by marking the temp variable with __maybe_unused. The affected readl() is only required for flushing caches, but the returned value is not of interest. v2: * declare temp as __maybe_unused (Helge) Signed-off-by: Thomas Zimmermann --- drivers/char/agp/nvidia-agp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/char/agp/nvidia-agp.c b/drivers/char/agp/nvidia-agp.c index f78e756157db..826dbd06f6bb 100644 --- a/drivers/char/agp/nvidia-agp.c +++ b/drivers/char/agp/nvidia-agp.c @@ -261,7 +261,8 @@ static int nvidia_remove_memory(struct agp_memory *mem, off_t pg_start, int type static void nvidia_tlbflush(struct agp_memory *mem) { unsigned long end; - u32 wbc_reg, temp; + u32 wbc_reg; + u32 __maybe_unused temp; int i; /* flush chipset */ -- 2.34.0
[PATCH v2 7/7] agp/via: Remove unused variable 'current_size'
Fix the compiler warning drivers/char/agp/via-agp.c: In function 'via_configure_agp3': drivers/char/agp/via-agp.c:131:35: warning: variable 'current_size' set but not used [-Wunused-but-set-variable] 131 | struct aper_size_info_16 *current_size; by removing the variable. Signed-off-by: Thomas Zimmermann Acked-by: Helge Deller --- drivers/char/agp/via-agp.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/char/agp/via-agp.c b/drivers/char/agp/via-agp.c index 87a92a044570..dc594f4eca38 100644 --- a/drivers/char/agp/via-agp.c +++ b/drivers/char/agp/via-agp.c @@ -128,9 +128,6 @@ static int via_fetch_size_agp3(void) static int via_configure_agp3(void) { u32 temp; - struct aper_size_info_16 *current_size; - - current_size = A_SIZE_16(agp_bridge->current_size); /* address to map to */ agp_bridge->gart_bus_addr = pci_bus_address(agp_bridge->dev, -- 2.34.0
[PATCH v2 1/7] agp: Remove trailing whitespaces
Trivial coding-style fix. Signed-off-by: Thomas Zimmermann Acked-by: Helge Deller --- drivers/char/agp/ati-agp.c| 2 +- drivers/char/agp/frontend.c | 2 +- drivers/char/agp/sworks-agp.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/char/agp/ati-agp.c b/drivers/char/agp/ati-agp.c index 20bf5f78a362..857b37141a07 100644 --- a/drivers/char/agp/ati-agp.c +++ b/drivers/char/agp/ati-agp.c @@ -303,7 +303,7 @@ static int ati_insert_memory(struct agp_memory * mem, for (i = 0, j = pg_start; i < mem->page_count; i++, j++) { addr = (j * PAGE_SIZE) + agp_bridge->gart_bus_addr; cur_gatt = GET_GATT(addr); - writel(agp_bridge->driver->mask_memory(agp_bridge, + writel(agp_bridge->driver->mask_memory(agp_bridge, page_to_phys(mem->pages[i]), mem->type), cur_gatt+GET_GATT_OFF(addr)); diff --git a/drivers/char/agp/frontend.c b/drivers/char/agp/frontend.c index 00ff5fcb808a..6802a6bbf0f2 100644 --- a/drivers/char/agp/frontend.c +++ b/drivers/char/agp/frontend.c @@ -1017,7 +1017,7 @@ static long agp_ioctl(struct file *file, case AGPIOC_UNBIND: ret_val = agpioc_unbind_wrap(curr_priv, (void __user *) arg); break; - + case AGPIOC_CHIPSET_FLUSH: break; } diff --git a/drivers/char/agp/sworks-agp.c b/drivers/char/agp/sworks-agp.c index f875970bda65..b15d3d4f71d5 100644 --- a/drivers/char/agp/sworks-agp.c +++ b/drivers/char/agp/sworks-agp.c @@ -350,7 +350,7 @@ static int serverworks_insert_memory(struct agp_memory *mem, for (i = 0, j = pg_start; i < mem->page_count; i++, j++) { addr = (j * PAGE_SIZE) + agp_bridge->gart_bus_addr; cur_gatt = SVRWRKS_GET_GATT(addr); - writel(agp_bridge->driver->mask_memory(agp_bridge, + writel(agp_bridge->driver->mask_memory(agp_bridge, page_to_phys(mem->pages[i]), mem->type), cur_gatt+GET_GATT_OFF(addr)); } -- 2.34.0
Re: [PATCH v11, 18/19] media: mtk-vcodec: Remove mtk_vcodec_release_dec_pm
Il 29/11/21 04:42, Yunfei Dong ha scritto: There are only two lines in mtk_vcodec_release_dec_pm, using pm_runtime_disable and put_device instead directly. Move pm_runtime_enable outside mtk_vcodec_init_dec_pm to symmetry with pm_runtime_disable, after that, rename mtk_vcodec_init_dec_pm to *_clk since it only has clock operations now. Signed-off-by: Yunfei Dong Co-developed-by: Yong Wu --- .../media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 10 +++--- .../media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c| 7 +-- .../media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c| 12 ++-- .../media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h| 3 +-- 4 files changed, 15 insertions(+), 17 deletions(-) Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH v11, 19/19] media: mtk-vcodec: Remove mtk_vcodec_release_enc_pm
Il 29/11/21 04:42, Yunfei Dong ha scritto: There are only two lines in mtk_vcodec_release_enc_pm, using pm_runtime_disable and put_device instead directly. Move pm_runtime_enable outside mtk_vcodec_release_enc_pm to symmetry with pm_runtime_disable, after that, rename mtk_vcodec_init_enc_pm to *_clk since it only has clock operations now. Signed-off-by: Yunfei Dong Co-developed-by: Yong Wu --- drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 9 ++--- drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 9 + drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.h | 3 +-- 3 files changed, 8 insertions(+), 13 deletions(-) Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH v11,14/19] media: mtk-vcodec: Support 34bits dma address for vdec
Il 29/11/21 04:41, Yunfei Dong ha scritto: Use the dma_set_mask_and_coherent helper to set vdec DMA bit mask to support 34bits iova space(16GB) that the mt8192 iommu HW support. Whole the iova range separate to 0~4G/4G~8G/8G~12G/12G~16G, regarding which iova range VDEC actually locate, it depends on the dma-ranges property of vdec dtsi node. Signed-off-by: Yunfei Dong Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH v11, 10/19] media: mtk-vcodec: Add msg queue feature for lat and core architecture
Il 29/11/21 04:41, Yunfei Dong ha scritto: For lat and core architecture, lat thread will send message to core thread when lat decode done. Core hardware will use the message from lat to decode, then free message to lat thread when decode done. Signed-off-by: Yunfei Dong --- drivers/media/platform/mtk-vcodec/Makefile| 1 + .../platform/mtk-vcodec/mtk_vcodec_drv.h | 9 + .../platform/mtk-vcodec/vdec_msg_queue.c | 257 ++ .../platform/mtk-vcodec/vdec_msg_queue.h | 148 ++ 4 files changed, 415 insertions(+) create mode 100644 drivers/media/platform/mtk-vcodec/vdec_msg_queue.c create mode 100644 drivers/media/platform/mtk-vcodec/vdec_msg_queue.h diff --git a/drivers/media/platform/mtk-vcodec/Makefile b/drivers/media/platform/mtk-vcodec/Makefile index c61bfb179bcc..359619653a0e 100644 --- a/drivers/media/platform/mtk-vcodec/Makefile +++ b/drivers/media/platform/mtk-vcodec/Makefile @@ -12,6 +12,7 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \ mtk_vcodec_dec_drv.o \ vdec_drv_if.o \ vdec_vpu_if.o \ + vdec_msg_queue.o \ mtk_vcodec_dec.o \ mtk_vcodec_dec_stateful.o \ mtk_vcodec_dec_stateless.o \ diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h index 7fc106df039b..610b0af13879 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h @@ -15,7 +15,9 @@ #include #include #include + #include "mtk_vcodec_util.h" +#include "vdec_msg_queue.h" #define MTK_VCODEC_DRV_NAME "mtk_vcodec_drv" #define MTK_VCODEC_DEC_NAME "mtk-vcodec-dec" @@ -282,6 +284,8 @@ struct vdec_pic_info { * @decoded_frame_cnt: number of decoded frames * @lock: protect variables accessed by V4L2 threads and worker thread such as * mtk_video_dec_buf. + * + * @msg_queue: msg queue used to store lat buffer information. */ struct mtk_vcodec_ctx { enum mtk_instance_type type; @@ -325,6 +329,7 @@ struct mtk_vcodec_ctx { int decoded_frame_cnt; struct mutex lock; + struct vdec_msg_queue msg_queue; }; enum mtk_chip { @@ -457,6 +462,8 @@ struct mtk_vcodec_enc_pdata { * @dec_capability: used to identify decode capability, ex: 4k * @enc_capability: used to identify encode capability * + * @msg_queue_core_ctx: msg queue context used for core workqueue + * * @subdev_dev: subdev hardware device * @subdev_bitmap: used to record hardware is ready or not */ @@ -497,6 +504,8 @@ struct mtk_vcodec_dev { unsigned int dec_capability; unsigned int enc_capability; + struct vdec_msg_queue_ctx msg_queue_core_ctx; + void *subdev_dev[MTK_VDEC_HW_MAX]; DECLARE_BITMAP(subdev_bitmap, MTK_VDEC_HW_MAX); }; diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c new file mode 100644 index ..da4d114f7ad0 --- /dev/null +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c @@ -0,0 +1,257 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2021 MediaTek Inc. + * Author: Yunfei Dong + */ + +#include +#include +#include + +#include "mtk_vcodec_dec_pm.h" +#include "mtk_vcodec_drv.h" +#include "vdec_msg_queue.h" + +/* the size used to store lat slice header information */ +#define VDEC_LAT_SLICE_HEADER_SZ(640 * SZ_1K) + +/* the size used to store avc error information */ +#define VDEC_ERR_MAP_SZ_AVC (17 * SZ_1K) + +/* core will read the trans buffer which decoded by lat to decode again. + * The trans buffer size of FHD and 4K bitstreams are different. + */ +static int vde_msg_queue_get_trans_size(int width, int height) +{ + if (width > 1920 || height > 1088) + return 30 * SZ_1M; + else + return 6 * SZ_1M; +} + +void vdec_msg_queue_init_ctx(struct vdec_msg_queue_ctx *ctx, + int hardware_index) +{ + init_waitqueue_head(&ctx->ready_to_use); + INIT_LIST_HEAD(&ctx->ready_queue); + spin_lock_init(&ctx->ready_lock); + ctx->ready_num = 0; + ctx->hardware_index = hardware_index; +} + +static struct list_head *vdec_get_buf_list(int hardware_index, + struct vdec_lat_buf *buf) +{ + switch (hardware_index) { + case MTK_VDEC_CORE: + return &buf->core_list; + case MTK_VDEC_LAT0: + return &buf->lat_list; + default: + return NULL; + } +} + +void vdec_msg_queue_qbuf(struct vdec_msg_queue_ctx *msg_ctx, + struct vdec_lat_buf *buf) This function can fail: it has to return an integer value. +{ + struct list_head *head; + + head = vdec_get_buf_list(msg_ctx->hardware_index, buf); + if (!head) { + mtk_v4l2_err("fail to qbuf: %d",msg_ctx->hardware_index); + return; + } + + spin_
Re: [PATCH v11, 11/19] media: mtk-vcodec: Generalize power and clock on/off interfaces
Il 29/11/21 04:41, Yunfei Dong ha scritto: Generalizes power and clock on/off interfaces to support different hardware. Signed-off-by: Yunfei Dong --- .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 6 +- .../platform/mtk-vcodec/mtk_vcodec_dec_hw.c | 2 +- .../platform/mtk-vcodec/mtk_vcodec_dec_hw.h | 4 + .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 76 +-- .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h | 8 +- .../platform/mtk-vcodec/mtk_vcodec_drv.h | 2 + .../platform/mtk-vcodec/mtk_vcodec_util.c | 60 --- .../platform/mtk-vcodec/mtk_vcodec_util.h | 8 +- .../media/platform/mtk-vcodec/vdec_drv_if.c | 21 ++--- 9 files changed, 147 insertions(+), 40 deletions(-) diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c index ac279c2a3f8a..001cdf447ab8 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c @@ -75,7 +75,7 @@ static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv) void __iomem *vdec_misc_addr = dev->reg_base[VDEC_MISC] + VDEC_IRQ_CFG_REG; - ctx = mtk_vcodec_get_curr_ctx(dev); + ctx = mtk_vcodec_get_curr_ctx(dev, MTK_VDEC_CORE); /* check if HW active or not */ cg_status = readl(dev->reg_base[0]); @@ -224,7 +224,7 @@ static int fops_vcodec_open(struct file *file) mtk_vcodec_dec_set_default_params(ctx); if (v4l2_fh_is_singular(&ctx->fh)) { - ret = mtk_vcodec_dec_pw_on(&dev->pm); + ret = mtk_vcodec_dec_pw_on(dev, MTK_VDEC_LAT0); if (ret < 0) goto err_load_fw; /* @@ -284,7 +284,7 @@ static int fops_vcodec_release(struct file *file) mtk_vcodec_dec_release(ctx); if (v4l2_fh_is_singular(&ctx->fh)) - mtk_vcodec_dec_pw_off(&dev->pm); + mtk_vcodec_dec_pw_off(dev, MTK_VDEC_LAT0); v4l2_fh_del(&ctx->fh); v4l2_fh_exit(&ctx->fh); v4l2_ctrl_handler_free(&ctx->ctrl_hdl); diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c index 8bd23504cf4c..389a17eb4085 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c @@ -42,7 +42,7 @@ static irqreturn_t mtk_vdec_hw_irq_handler(int irq, void *priv) void __iomem *vdec_misc_addr = dev->reg_base[VDEC_HW_MISC] + VDEC_IRQ_CFG_REG; - ctx = mtk_vcodec_get_curr_ctx(dev->main_dev); + ctx = mtk_vcodec_get_curr_ctx(dev->main_dev, dev->hw_idx); /* check if HW active or not */ cg_status = readl(dev->reg_base[VDEC_HW_SYS]); diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h index f7f36790629d..fdf1435fc932 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h @@ -34,6 +34,8 @@ enum mtk_vdec_hw_reg_idx { * @main_dev: main device * @reg_base: Mapped address of MTK Vcodec registers. * + * @curr_ctx: the context that is waiting for codec hardware + * * @dec_irq: decoder irq resource * @pm: power management control * @hw_idx: each hardware index @@ -43,6 +45,8 @@ struct mtk_vdec_hw_dev { struct mtk_vcodec_dev *main_dev; void __iomem *reg_base[VDEC_HW_MAX]; + struct mtk_vcodec_ctx *curr_ctx; + int dec_irq; struct mtk_vcodec_pm pm; int hw_idx; diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c index 221cf60e9fbf..4cf03d38d141 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c @@ -5,11 +5,13 @@ */ #include +#include #include #include #include #include +#include "mtk_vcodec_dec_hw.h" #include "mtk_vcodec_dec_pm.h" #include "mtk_vcodec_util.h" @@ -86,10 +88,23 @@ void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm) } EXPORT_SYMBOL_GPL(mtk_vcodec_release_dec_pm); -int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm) +int mtk_vcodec_dec_pw_on(struct mtk_vcodec_dev *vdec_dev, int hw_idx) { + struct mtk_vdec_hw_dev *subdev_dev; + struct mtk_vcodec_pm *pm; int ret; + if (vdec_dev->vdec_pdata->is_subdev_supported) { + subdev_dev = mtk_vcodec_get_hw_dev(vdec_dev, hw_idx); + if (!subdev_dev) { + mtk_v4l2_err("Failed to get hw dev\n"); + return -EINVAL; + } + pm = &subdev_dev->pm; + } else { + pm = &vdec_dev->pm; + } + ret = pm_runtime_resume_and_get(pm->dev); if (ret)
Re: [PATCH v11, 09/19] media: mtk-vcodec: Add irq interface for multi hardware
Il 29/11/21 04:41, Yunfei Dong ha scritto: Adds irq interface for multi hardware. Signed-off-by: Yunfei Dong --- .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 33 --- .../platform/mtk-vcodec/mtk_vcodec_dec_hw.c | 2 +- .../platform/mtk-vcodec/mtk_vcodec_drv.h | 25 ++ .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 4 +-- .../platform/mtk-vcodec/mtk_vcodec_intr.c | 27 +++ .../platform/mtk-vcodec/mtk_vcodec_intr.h | 4 +-- .../platform/mtk-vcodec/vdec/vdec_h264_if.c | 2 +- .../mtk-vcodec/vdec/vdec_h264_req_if.c| 2 +- .../platform/mtk-vcodec/vdec/vdec_vp8_if.c| 2 +- .../platform/mtk-vcodec/vdec/vdec_vp9_if.c| 2 +- .../platform/mtk-vcodec/venc/venc_h264_if.c | 2 +- .../platform/mtk-vcodec/venc/venc_vp8_if.c| 2 +- 12 files changed, 70 insertions(+), 37 deletions(-) diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c index 95fbe9be3f6d..ac279c2a3f8a 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c @@ -52,6 +52,20 @@ static int mtk_vcodec_subdev_device_check(struct mtk_vcodec_dev *vdec_dev) return 0; } +static int mtk_vcodec_get_hw_count(struct mtk_vcodec_dev *dev) +{ + switch (dev->vdec_pdata->hw_arch) { + case MTK_VDEC_PURE_SINGLE_CORE: +return MTK_VDEC_ONE_CORE; + case MTK_VDEC_LAT_SINGLE_CORE: + return MTK_VDEC_ONE_LAT_ONE_CORE; + default: + mtk_v4l2_err("not support hw arch:%d", + dev->vdec_pdata->hw_arch); There's no need to break this line... also, what about changing the message to "hw arch %d not supported"? Apart from that, Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH v11, 04/19] media: mtk-vcodec: export decoder pm functions
Il 29/11/21 04:41, Yunfei Dong ha scritto: Register each hardware as platform device, need to call pm functions to open/close power and clock from module mtk-vcodec-dec, export these functions. Signed-off-by: Yunfei Dong Reviewed-by: Benjamin Gaignard Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH v11, 05/19] media: mtk-vcodec: Support MT8192
Il 29/11/21 04:41, Yunfei Dong ha scritto: From: Yunfei Dong Adds MT8192's compatible "mediatek,mt8192-vcodec-dec". Adds MT8192's device private data mtk_lat_sig_core_pdata. Signed-off-by: Yunfei Dong Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH] drm/msm/dpu: fix exception in error path
On 01/12/2021 05:53, Bjorn Andersson wrote: On Thu 25 Nov 12:01 CST 2021, Dmitry Baryshkov wrote: In case of DPU probe failure, prevent the following NULL pointer exception: [3.976112] Unable to handle kernel NULL pointer dereference at virtual address 0030 [3.984983] Mem abort info: [3.987800] ESR = 0x9604 [3.990891] EC = 0x25: DABT (current EL), IL = 32 bits [3.996251] SET = 0, FnV = 0 [3.996254] EA = 0, S1PTW = 0 [3.996257] FSC = 0x04: level 0 translation fault [3.996260] Data abort info: [3.996262] ISV = 0, ISS = 0x0004 [4.005229] CM = 0, WnR = 0 [4.028893] [0030] user address but active_mm is swapper [4.035305] Internal error: Oops: 9604 [#1] SMP [4.040223] Modules linked in: [4.043317] CPU: 1 PID: 50 Comm: kworker/u16:2 Not tainted 5.16.0-rc1-00036-g6d4bafcbb015-dirty #166 [4.052518] Hardware name: Thundercomm Dragonboard 845c (DT) [4.058224] Workqueue: events_unbound deferred_probe_work_func [4.064105] pstate: 6045 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [4.071124] pc : dpu_core_irq_uninstall+0x28/0x100 [4.075960] lr : dpu_core_irq_uninstall+0x24/0x100 [4.080793] sp : 80001057b990 [4.084138] x29: 80001057b990 x28: 7653c0a41c00 x27: 7653c0926480 [4.091330] x26: b90d5d262ad0 x25: 7653c4b2e980 x24: 7653c0046080 [4.098520] x23: 7653c099a810 x22: 7653c5a65800 x21: 7653c5a65080 [4.105711] x20: 7653c5a65800 x19: 7653c0046080 x18: 0034 [4.112902] x17: 0038 x16: 0005 x15: 000c [4.120095] x14: 024c x13: 7653c2f90358 x12: [4.127287] x11: 7653c2f903b0 x10: 09c0 x9 : 80001057b180 [4.134477] x8 : 80001057b404 x7 : x6 : 7653c5a5f190 [4.141669] x5 : 80001057b890 x4 : x3 : 7653c5a5f0f4 [4.148859] x2 : 7653c2f5 x1 : x0 : [4.156052] Call trace: [4.158525] dpu_core_irq_uninstall+0x28/0x100 [4.163004] dpu_irq_uninstall+0x10/0x20 [4.166963] msm_drm_uninit.isra.0+0xe0/0x1b0 [4.171353] msm_drm_bind+0x278/0x5f0 [4.175043] try_to_bring_up_master+0x164/0x1d0 [4.179610] __component_add+0xa0/0x170 [4.183482] component_add+0x14/0x20 [4.187086] dsi_dev_probe+0x1c/0x30 [4.190691] platform_probe+0x68/0xe0 [4.194382] really_probe.part.0+0x9c/0x30c [4.198601] __driver_probe_device+0x98/0x144 [4.202990] driver_probe_device+0x44/0x15c [4.207208] __device_attach_driver+0xb4/0x120 [4.211685] bus_for_each_drv+0x78/0xd0 [4.215549] __device_attach+0xdc/0x184 [4.219412] device_initial_probe+0x14/0x20 [4.223630] bus_probe_device+0x9c/0xa4 [4.227503] deferred_probe_work_func+0x88/0xc0 [4.232075] process_one_work+0x1e8/0x380 [4.236126] worker_thread+0x280/0x520 [4.239902] kthread+0x168/0x174 [4.243166] ret_from_fork+0x10/0x20 [4.246778] Code: f9442400 91004000 940188b9 f9430660 (b9403001) [4.252925] ---[ end trace b470a50cd7b5e606 ]--- Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c index d2b6dca487e3..fc1b6c47c93d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c @@ -575,6 +575,9 @@ void dpu_core_irq_uninstall(struct dpu_kms *dpu_kms) { int i; + if (!dpu_kms->hw_intr) + return; I would rather see that we fix msm_drm_init() to nicely unroll things in a more granular fashion instead of handle all types of errors with the big hammer that msm_drm_uninit() provides. It well might be that this issue happened because of earlier version of Angelo's patch (which had other issues too). So I'm dropping this for now. Regards, Bjorn + pm_runtime_get_sync(&dpu_kms->pdev->dev); for (i = 0; i < dpu_kms->hw_intr->total_irqs; i++) if (!list_empty(&dpu_kms->hw_intr->irq_cb_tbl[i])) -- 2.33.0 -- With best wishes Dmitry
Re: [Linaro-mm-sig] [RFC PATCH 1/2] dma-fence: Avoid establishing a locking order between fence classes
On 12/1/21 12:25, Christian König wrote: Am 01.12.21 um 12:04 schrieb Thomas Hellström (Intel): On 12/1/21 11:32, Christian König wrote: Am 01.12.21 um 11:15 schrieb Thomas Hellström (Intel): [SNIP] What we could do is to avoid all this by not calling the callback with the lock held in the first place. If that's possible that might be a good idea, pls also see below. The problem with that is dma_fence_signal_locked()/dma_fence_signal_timestamp_locked(). If we could avoid using that or at least allow it to drop the lock then we could call the callback without holding it. Somebody would need to audit the drivers and see if holding the lock is really necessary anywhere. /Thomas Oh, and a follow up question: If there was a way to break the recursion on final put() (using the same basic approach as patch 2 in this series uses to break recursion in enable_signaling()), so that none of these containers did require any special treatment, would it be worth pursuing? I guess it might be possible by having the callbacks drop the references rather than the loop in the final put. + a couple of changes in code iterating over the fence pointers. That won't really help, you just move the recursion from the final put into the callback. How do we recurse from the callback? The introduced fence_put() of individual fence pointers doesn't recurse anymore (at most 1 level), and any callback recursion is broken by the irq_work? Yeah, but then you would need to take another lock to avoid racing with dma_fence_array_signaled(). I figure the big amount of work would be to adjust code that iterates over the individual fence pointers to recognize that they are rcu protected. Could be that we could solve this with RCU, but that sounds like a lot of churn for no gain at all. In other words even with the problems solved I think it would be a really bad idea to allow chaining of dma_fence_array objects. Yes, that was really the question, Is it worth pursuing this? I'm not really suggesting we should allow this as an intentional feature. I'm worried, however, that if we allow these containers to start floating around cross-driver (or even internally) disguised as ordinary dma_fences, they would require a lot of driver special casing, or else completely unexpeced WARN_ON()s and lockdep splats would start to turn up, scaring people off from using them. And that would be a breeding ground for hairy driver-private constructs. Well the question is why we would want to do it? If it's to avoid inter driver lock dependencies by avoiding to call the callback with the spinlock held, then yes please. We had tons of problems with that, resulting in irq_work and work_item delegation all over the place. Yes, that sounds like something desirable, but in these containers, what's causing the lock dependencies is the enable_signaling() callback that is typically called locked. If it's to allow nesting of dma_fence_array instances, then it's most likely a really bad idea even if we fix all the locking order problems. Well I think my use-case where I hit a dead end may illustrate what worries me here: 1) We use a dma-fence-array to coalesce all dependencies for ttm object migration. 2) We use a dma-fence-chain to order the resulting dm_fence into a timeline because the TTM resource manager code requires that. Initially seemingly harmless to me. But after a sequence evict->alloc->clear, the dma-fence-chain feeds into the dma-fence-array for the clearing operation. Code still works fine, and no deep recursion, no warnings. But if I were to add another driver to the system that instead feeds a dma-fence-array into a dma-fence-chain, this would give me a lockdep splat. So then if somebody were to come up with the splendid idea of using a dma-fence-chain to initially coalesce fences, I'd hit the same problem or risk illegaly joining two dma-fence-chains together. To fix this, I would need to look at the incoming fences and iterate over any dma-fence-array or dma-fence-chain that is fed into the dma-fence-array to flatten out the input. In fact all dma-fence-array users would need to do that, and even dma-fence-chain users watching out for not joining chains together or accidently add an array that perhaps came as a disguised dma-fence from antother driver. So the purpose to me would be to allow these containers as input to eachother without a lot of in-driver special-casing, be it by breaking recursion on built-in flattening to avoid a) Hitting issues in the future or with existing interoperating drivers. b) Avoid driver-private containers that also might break the interoperability. (For example the i915 currently driver-private dma_fence_work avoid all these problems, but we're attempting to address issues in common code rather than re-inventing stuff internally). I don't think that a dma_fence_array or dma_fence_chain is the right thing to
Re: [PATCH v2 0/3] drm/etnaviv: IOMMU related fixes
Hi Michael, Am Dienstag, dem 07.09.2021 um 18:49 +0200 schrieb Michael Walle: > This patch series fixes usage of the etnaviv driver with GPUs behind a > IOMMU. It was tested on a NXP LS1028A SoC. Together with Lucas' MMU patches > [1] there are not more (GPU internal) MMU nor (system) IOMMU faults on the > LS1028A. > > [1] https://lists.freedesktop.org/archives/etnaviv/2021-August/003682.html > > changes since v1: > - don't move the former dma_mask setup code around in patch 2/3. Will >avoid any confusion. > > Michael Walle (3): > drm/etnaviv: use PLATFORM_DEVID_NONE > drm/etnaviv: fix dma configuration of the virtual device > drm/etnaviv: use a 32 bit mask as coherent DMA mask > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 41 --- > 1 file changed, 31 insertions(+), 10 deletions(-) > Thanks for the patches! I applied them to my etnaviv/next branch. Regards, Lucas
Re: [PATCH] drm/etnaviv: constify static struct cooling_ops
Am Sonntag, dem 28.11.2021 um 21:19 +0100 schrieb Rikard Falkeborn: > The only usage of cooling_ops is to pass its address to > thermal_of_cooling_device_register(), which takes a pointer to const > struct thermal_cooling_device_ops as input. Make it const to allow the > compiler to put it in read-only memory. > > Signed-off-by: Rikard Falkeborn Applied to my etnaviv/next branch. Regards, Lucas > --- > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c > b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c > index 06bde46df451..37018bc55810 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c > @@ -1658,7 +1658,7 @@ etnaviv_gpu_cooling_set_cur_state(struct > thermal_cooling_device *cdev, > return 0; > } > > -static struct thermal_cooling_device_ops cooling_ops = { > +static const struct thermal_cooling_device_ops cooling_ops = { > .get_max_state = etnaviv_gpu_cooling_get_max_state, > .get_cur_state = etnaviv_gpu_cooling_get_cur_state, > .set_cur_state = etnaviv_gpu_cooling_set_cur_state,
Re: [PATCH v2 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask
Sorry I missed this earlier... On 2021-09-07 17:49, Michael Walle wrote: The STLB and the first command buffer (which is used to set up the TLBs) has a 32 bit size restriction in hardware. There seems to be no way to specify addresses larger than 32 bit. Keep it simple and restict the addresses to the lower 4 GiB range for all coherent DMA memory allocations. Please note, that platform_device_alloc() will initialize dev->dma_mask to point to pdev->platform_dma_mask, thus dma_set_mask() will work as expected. While at it, move the dma_mask setup code to the of_dma_configure() to keep all the DMA setup code next to each other. Suggested-by: Lucas Stach Signed-off-by: Michael Walle --- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 54eb653ca295..0b756ecb1bc2 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -613,6 +613,24 @@ static int etnaviv_pdev_probe(struct platform_device *pdev) component_match_add(dev, &match, compare_str, names[i]); } + /* +* PTA and MTLB can have 40 bit base addresses, but +* unfortunately, an entry in the MTLB can only point to a +* 32 bit base address of a STLB. Moreover, to initialize the +* MMU we need a command buffer with a 32 bit address because +* without an MMU there is only an indentity mapping between +* the internal 32 bit addresses and the bus addresses. +* +* To make things easy, we set the dma_coherent_mask to 32 +* bit to make sure we are allocating the command buffers and +* TLBs in the lower 4 GiB address space. +*/ + if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(40)) || + dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) { Since AFAICS you're not changing the default dma_mask pointer to point to some storage other than the coherent mask, the dma_set_mask() call effectively does nothing and both masks will end up reading back as 32 bits. Robin. + dev_dbg(&pdev->dev, "No suitable DMA available\n"); + return -ENODEV; + } + /* * Apply the same DMA configuration to the virtual etnaviv * device as the GPU we found. This assumes that all Vivante @@ -671,8 +689,6 @@ static int __init etnaviv_init(void) of_node_put(np); goto unregister_platform_driver; } - pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; ret = platform_device_add(pdev); if (ret) {
Re: [Intel-gfx] [PATCH v4 2/2] drm/i915: Use to_root_gt() to refer to the root tile
Hi Michal, > >> fist of all thanks for taking a look at this, I was eagerly > >> waiting for reviewers. > >> > >> On Tue, Nov 30, 2021 at 01:07:30PM -0800, Lucas De Marchi wrote: > >>> On Sun, Nov 28, 2021 at 01:09:26PM +0200, Andi Shyti wrote: > >>> > Starting from a patch from Matt to_root_gt() returns the > >>> > reference to the root tile in order to abstract the root tile > >>> > from th callers. > >>> > > >>> > Being the root tile identified as tile '0', embed the id in the > >>> > name so that i915->gt becomes i915->gt0. > >>> > > >>> > The renaming has been mostly done with the following command and > >>> > some manual fixes. > >>> > > >>> > sed -i -e sed -i 's/\&i915\->gt\./\&to_root_gt(i915)\->/g' \ > >>> > -e sed -i 's/\&dev_priv\->gt\./\&to_root_gt(dev_priv)\->/g' \ > >>> > -e 's/\&dev_priv\->gt/to_root_gt(dev_priv)/g' \ > >>> > -e 's/\&i915\->gt/to_root_gt(i915)/g' \ > >>> > -e 's/dev_priv\->gt\./to_root_gt(dev_priv)\->/g' \ > >>> > -e 's/i915\->gt\./to_root_gt(i915)\->/g' \ > >>> > `find drivers/gpu/drm/i915/ -name *.[ch]` > >>> > > >>> > Two small changes have been added to this commit: > >>> > > >>> > 1. intel_reset_gpu() in intel_display.c retreives the gt from > >>> > to_scanout_gt() > >>> > 2. in set_scheduler_caps() the gt is taken from the engine and > >>> > not from i915. > >>> > >>> Ideally the non-automatic changes should be in separate patches, before > >>> the ones that can be done by automation. Because then it becomes easier > >>> to apply the final result without conflicts. > >> > >> OK > >> > >>> This is quite a big diff to merge in one go. Looking at the pending > >>> patches from Michal however I see he had similar changes, split in > >>> sensible chunks.. Could you split your version like that? at least > >>> gt/gem and display would be good to have separate. Or sync with Michal > >>> on how to proceed with these versions Here are his patches: > >>> > >>> drm/i915: Remove i915->ggtt > >>> drm/i915: Use to_gt() helper for GGTT accesses > >>> drm/i915: Use to_gt() helper > >>> drm/i915/gvt: Use to_gt() helper > >>> drm/i915/gem: Use to_gt() helper > >>> drm/i915/gt: Use to_gt() helper > >>> drm/i915/display: Use to_gt() helper > >>> drm/i915: Introduce to_gt() helper > >> > >> I understand... will follow this approach. > >> > >>> This first patch also removed the `struct intel_gt *gt = to_gt(pool)`, > >>> that would otherwise be a leftover in > >>> drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c > >> > >> One difference from Michal patch is that I am not using the > >> wrapper > >> > >> to_gt(...) > >> > >> but > >> > >> to_root_gt(...) > >> > >> which was introduced by Matt. To me sounds more meaningful as it > >> specifies that we are really looking for the root tile and not > >> any tile. > > > > yes, I think it makes sense, too. Michal, any comment? I think you > > also had other plans to get the root gt by another helper... ? > > The main rationale to use generic "to_gt()" helper name in all existing > i915->gt cases in (other) Michal patches was that on some upcoming > configs we want to distinguish between "primary" and "root" tile and use > "to_root_gt()" helper only when referring to the root tile as described > in Bspec:52416. > > Note that since current code baseline is still "single" tile, you can't > tell whether all of these functions really expects special "root" tile > or just "any" tile. this series is indeed preparatory for the multitile and making it to_gt() now it will require to replace it with to_root_gt() later. The idea is that a GT is root even if it's alone. The next patch after this will be the actual multitile.[*] In this particular patch I am even renaming i915->gt to i915->gt0 to underline the difference. > Thus to avoid confusion or mistakes I would suggest to keep simple name > "to_gt()" as in most cases usages of this helper it will likely be > replaced with iterator from for_each_gt loop and any remaining usages > will just mean "primary" tile or replaced with explicit "to_root_gt()" > if really needed. Knowing what's about to come, I do not see this as a good reason to have to_gt() as a mid step. Right? Andi [*] https://patchwork.freedesktop.org/patch/464475/?series=97352&rev=1
Re: [PATCH v10 4/8] drm/ingenic: Add dw-hdmi driver for jz4780
Hi Nikolaus, Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller a écrit : 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. Here we add Kconfig DRM_INGENIC_DW_HDMI, Makefile and driver code. 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 | 138 ++ 3 files changed, 148 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 3b57f8be007c4..4efc709d77b0a 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 + tristate "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 d313326bdddbb..f10cc1c5a5f22 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 +obj-$(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 0..199e39c227d29 --- /dev/null +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c @@ -0,0 +1,138 @@ +// 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 void ingenic_dw_hdmi_cleanup(void *data) +{ + struct dw_hdmi *hdmi = (struct dw_hdmi *)data; + + dw_hdmi_remove(hdmi); +} + +static void ingenic_dw_hdmi_disable_regulator(void *data) +{ + struct regulator *regulator = (struct regulator *)data; + + regulator_disable(regulator); +} + +static int ingenic_dw_hdmi_probe(struct platform_device *pdev) +{ + struct dw_hdmi *hdmi; + struct regul
Re: [Intel-gfx] [PATCH v2 00/16] drm/i915: Remove short term pins from execbuf.
On 01/12/2021 11:15, Maarten Lankhorst wrote: On 30-11-2021 19:38, Tvrtko Ursulin wrote: On 30/11/2021 11:17, Maarten Lankhorst wrote: On 30-11-2021 09:54, Tvrtko Ursulin wrote: Hi, On 29/11/2021 13:47, Maarten Lankhorst wrote: New version of the series, with feedback from previous series added. If there was a cover letter sent for this work in the past could you please keep attaching it? Or if there wasn't, could you please write one? I am worried about two things. First is that we need to have a high level overview of the rules/design changes documented so third party people have any hope of getting code right after this lands. (Where we are, where we are going, how we will get there, how far did we get and when we will get to the end.) Second is that when parts of the series land piecemeal (Which they have in this right, right?), it gets very hard to write up a maintainer level changelog. The preparation part is to ensure we always hold vma->obj->resv when unbinding. The first preparation series ensured vma->obj always existed. This was not the case for mock gtt and gen6 aliasing gtt. This allowed us to remove all the special handling for those uncommon cases, and actually enforce we can always take that lock. This part is merged. Sounds good. But also mention the high level motivation for why we always want to hold vma->obj->resv when unbinding in the introduction as well. Patch 2-11 in this series adds the vma->obj->resv to eviction and shrinker. Those are the only parts where we don't take the lock yet. After that, we always hold the lock when required, and we can start requiring the obj-> resv lock when unbinding. This is completed in patch 15. With that fixed, removing short term pins can be done, because for unbind we now always take obj->resv, so holding obj->resv during execbuf submission is sufficient, and all short term pinning can be removed. I'd also like the cover letter to contain a high level description on _why_ is removing short term pins needed or beneficial. What was the flow and object lifetimes so far, and what it will be going forward etc. Previously, short term pinning in execbuf was required because i915_vma was effectively independent from objects, and has its own refcount, locking, and lifetime rules and pinning. This series removes the separate locking, by requiring vma->obj->resv to be held when pinning and unbinding. This will also be required for VM_BIND work. With pinning required for pinning and unbinding, the lock is enough to prevent unbinding when trying to pin with the lock held, like in execbuf. This makes binding/unbinding similar to ttm_bo_validate()'s use, which just cares that an object is in a certain place, without pinning it in place. Having it part of gem bo removes a lot of the vma refcounting, and makes i915_vma more a part of the bo, instead of its own floating object that just happens to be part of a bo. This is also required to make it more compatible with TTM, and migration in general. For future work, it makes things a lot simpler and clear. We want to end up with i915_vma just being a specific mapping of the BO, just like is the case in other drivers. i915_vma->active removal is the next step there, and makes it when object is destroyed, the bindings are destroyed (after idle), instead of object being destroyed when bindings are idle. Excellent, that's exactly the level needed for the cover letter. So that as intro, plus some text about the series details like which part of the overall plan it implements and it will be good. We only pin temporarily when calling i915_gem_evict_vm in execbuf, which could also be handled in theory by just marking all objects as unpinned. As a bonus, using TTM for delayed eviction on all objects becomes easy, just need to get rid of i915_active in i915_vma, as it keeps the object refcount alive. Remainder is removing refcount to i915_vma, to make it a real Sounds on the right track with maybe a bit more text so the readers can easily understand it on the higher level. With the obj->resv being the master lock, pinning for execbuf becomes obsolete and can be removed. Out of personal curiosity about the long term plans - what will be the flow on request retirement in terms of unbinding? Regards, Tvrtko
Re: [PATCH v10 4/8] drm/ingenic: Add dw-hdmi driver for jz4780
Hi Paul, > Am 01.12.2021 um 14:02 schrieb Paul Cercueil : > > Hi Nikolaus, > > Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller > a écrit : >> 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. >> Here we add Kconfig DRM_INGENIC_DW_HDMI, Makefile and driver code. >> 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 | 138 ++ >> 3 files changed, 148 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 3b57f8be007c4..4efc709d77b0a 100644 >> --- a/drivers/gpu/drm/ingenic/Kconfig >> +++ b/drivers/gpu/drm/ingenic/Kconfig >> >> +} >> + >> +if (!regulator) >> +return 0; > > Blank line here. It is one of these cases where checkpatch doesn't complain although it should be improved... > > But I can add it myself when applying. Yes, please. So that we are not wasting mailing list bandwidth... > I'll just wait for Rob's ack first. Indeed. Fortunately he had the right hint how to fix 3/8 quickly. BR and thanks, NIkolaus
Re: [PATCH v2] drm/komeda: Fix an undefined behavior bug in komeda_plane_add()
On Wed, Dec 01, 2021 at 11:37:03AM +0800, Zhou Qingyang wrote: > In komeda_plane_add(), komeda_get_layer_fourcc_list() is assigned to > formats and used in drm_universal_plane_init(). > drm_universal_plane_init() passes formats to > __drm_universal_plane_init(). __drm_universal_plane_init() further > passes formats to memcpy() as src parameter, which could lead to an > undefined behavior bug on failure of komeda_get_layer_fourcc_list(). > > Fix this bug by adding a check of formats. > > This bug was found by a static analyzer. The analysis employs > differential checking to identify inconsistent security operations > (e.g., checks or kfrees) between two code paths and confirms that the > inconsistent operations are not recovered in the current function or > the callers, so they constitute bugs. > > Note that, as a bug found by static analysis, it can be a false > positive or hard to trigger. Multiple researchers have cross-reviewed > the bug. > > Builds with CONFIG_DRM_KOMEDA=m show no new warnings, > and our static analyzer no longer warns about this code. > > Fixes: 61f1c4a8ab75 ("drm/komeda: Attach komeda_dev to DRM-KMS") > Signed-off-by: Zhou Qingyang Thanks for the fix! Reviewed-by: Liviu Dudau Best regards, Liviu > --- > Changes in v2 > - Use kfree and return instead of using 'goto' > > drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > index d63d83800a8a..aa193c58f4bf 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > @@ -265,6 +265,10 @@ static int komeda_plane_add(struct komeda_kms_dev *kms, > > formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl, > layer->layer_type, &n_formats); > + if (!formats) { > + kfree(kplane); > + return -ENOMEM; > + } > > err = drm_universal_plane_init(&kms->base, plane, > get_possible_crtcs(kms, c->pipeline), > -- > 2.25.1 > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯
Re: [PATCH v2 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask
Hi Robin, Am Mittwoch, dem 01.12.2021 um 12:50 + schrieb Robin Murphy: > Sorry I missed this earlier... > > On 2021-09-07 17:49, Michael Walle wrote: > > The STLB and the first command buffer (which is used to set up the TLBs) > > has a 32 bit size restriction in hardware. There seems to be no way to > > specify addresses larger than 32 bit. Keep it simple and restict the > > addresses to the lower 4 GiB range for all coherent DMA memory > > allocations. > > > > Please note, that platform_device_alloc() will initialize dev->dma_mask > > to point to pdev->platform_dma_mask, thus dma_set_mask() will work as > > expected. > > > > While at it, move the dma_mask setup code to the of_dma_configure() to > > keep all the DMA setup code next to each other. > > > > Suggested-by: Lucas Stach > > Signed-off-by: Michael Walle > > --- > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 20 ++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > index 54eb653ca295..0b756ecb1bc2 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > @@ -613,6 +613,24 @@ static int etnaviv_pdev_probe(struct platform_device > > *pdev) > > component_match_add(dev, &match, compare_str, names[i]); > > } > > > > + /* > > +* PTA and MTLB can have 40 bit base addresses, but > > +* unfortunately, an entry in the MTLB can only point to a > > +* 32 bit base address of a STLB. Moreover, to initialize the > > +* MMU we need a command buffer with a 32 bit address because > > +* without an MMU there is only an indentity mapping between > > +* the internal 32 bit addresses and the bus addresses. > > +* > > +* To make things easy, we set the dma_coherent_mask to 32 > > +* bit to make sure we are allocating the command buffers and > > +* TLBs in the lower 4 GiB address space. > > +*/ > > + if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(40)) || > > + dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) { > > Since AFAICS you're not changing the default dma_mask pointer to point > to some storage other than the coherent mask, the dma_set_mask() call > effectively does nothing and both masks will end up reading back as 32 bits. > >From what I can see the dma_mask has allocated storage in the platform device and does not point to the coherent dma mask, see setup_pdev_dma_masks(). Regards, Lucas > Robin. > > > + dev_dbg(&pdev->dev, "No suitable DMA available\n"); > > + return -ENODEV; > > + } > > + > > /* > > * Apply the same DMA configuration to the virtual etnaviv > > * device as the GPU we found. This assumes that all Vivante > > @@ -671,8 +689,6 @@ static int __init etnaviv_init(void) > > of_node_put(np); > > goto unregister_platform_driver; > > } > > - pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40); > > - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > > > > ret = platform_device_add(pdev); > > if (ret) { > >
Re: [PATCH v10 4/8] drm/ingenic: Add dw-hdmi driver for jz4780
On Wed, Dec 01, 2021 at 01:02:45PM +, Paul Cercueil wrote: > Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller > > + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); > > + if (IS_ERR(regulator)) { > > + ret = PTR_ERR(regulator); Why is this using _optional()? This should only be done when the supply can be physically absent (in which case I'd expect to see special handling). signature.asc Description: PGP signature
Re: [PATCH v10 4/8] drm/ingenic: Add dw-hdmi driver for jz4780
Hi, > Am 01.12.2021 um 14:39 schrieb Mark Brown : > > On Wed, Dec 01, 2021 at 01:02:45PM +, Paul Cercueil wrote: >> Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller > >>> + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); >>> + if (IS_ERR(regulator)) { >>> + ret = PTR_ERR(regulator); > > Why is this using _optional()? This should only be done when the supply > can be physically absent There can be +5V for HDMI but without a regulator that is visible to or controllable by the driver. So hdmi-5v can be simply missing in DTS in which case the driver does not need to care about. The driver just can't turn it on or off. > (in which case I'd expect to see special > handling). The special case is to not enable/disable the regulator if it does not exist and assume that there is hardware providing it otherwise (the driver can't know that except by using get_optional). This is done by the code below >>> + if (IS_ERR(regulator)) { ... > + if (!regulator) > + return 0; > > + ret = regulator_enable(regulator); ... BR and thanks, Nikolaus
Re: [PATCH v10 4/8] drm/ingenic: Add dw-hdmi driver for jz4780
Hi Nikolaus, Mark, Le mer., déc. 1 2021 at 14:51:51 +0100, H. Nikolaus Schaller a écrit : Hi, Am 01.12.2021 um 14:39 schrieb Mark Brown : On Wed, Dec 01, 2021 at 01:02:45PM +, Paul Cercueil wrote: Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); + if (IS_ERR(regulator)) { + ret = PTR_ERR(regulator); Why is this using _optional()? This should only be done when the supply can be physically absent There can be +5V for HDMI but without a regulator that is visible to or controllable by the driver. There is always a power supply though. Either a controllable one (through e.g. a GPIO), or it's just connected to the mains +5V; the pin is never left floating. In the second case, in DTS the "hdmi-5v" would be connected to some 5v regulator, even if it's just a dummy VCC-5V regulator. So Mark has a point. So hdmi-5v can be simply missing in DTS in which case the driver does not need to care about. The driver just can't turn it on or off. Please make it mandatory in DTS then, and use devm_regulator_get() in the driver. Cheers, -Paul (in which case I'd expect to see special handling). The special case is to not enable/disable the regulator if it does not exist and assume that there is hardware providing it otherwise (the driver can't know that except by using get_optional). This is done by the code below + if (IS_ERR(regulator)) { ... + if (!regulator) + return 0; + ret = regulator_enable(regulator); ... BR and thanks, Nikolaus
Re: [PATCH v2 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask
On 2021-12-01 13:41, Lucas Stach wrote: Hi Robin, Am Mittwoch, dem 01.12.2021 um 12:50 + schrieb Robin Murphy: Sorry I missed this earlier... On 2021-09-07 17:49, Michael Walle wrote: The STLB and the first command buffer (which is used to set up the TLBs) has a 32 bit size restriction in hardware. There seems to be no way to specify addresses larger than 32 bit. Keep it simple and restict the addresses to the lower 4 GiB range for all coherent DMA memory allocations. Please note, that platform_device_alloc() will initialize dev->dma_mask to point to pdev->platform_dma_mask, thus dma_set_mask() will work as expected. While at it, move the dma_mask setup code to the of_dma_configure() to keep all the DMA setup code next to each other. Suggested-by: Lucas Stach Signed-off-by: Michael Walle --- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 54eb653ca295..0b756ecb1bc2 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -613,6 +613,24 @@ static int etnaviv_pdev_probe(struct platform_device *pdev) component_match_add(dev, &match, compare_str, names[i]); } + /* +* PTA and MTLB can have 40 bit base addresses, but +* unfortunately, an entry in the MTLB can only point to a +* 32 bit base address of a STLB. Moreover, to initialize the +* MMU we need a command buffer with a 32 bit address because +* without an MMU there is only an indentity mapping between +* the internal 32 bit addresses and the bus addresses. +* +* To make things easy, we set the dma_coherent_mask to 32 +* bit to make sure we are allocating the command buffers and +* TLBs in the lower 4 GiB address space. +*/ + if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(40)) || + dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) { Since AFAICS you're not changing the default dma_mask pointer to point to some storage other than the coherent mask, the dma_set_mask() call effectively does nothing and both masks will end up reading back as 32 bits. From what I can see the dma_mask has allocated storage in the platform device and does not point to the coherent dma mask, see setup_pdev_dma_masks(). Urgh, apologies for the confusion - seems I had one of those mental short-circuits and was utterly convinced that that's what the platform device setup did, but of course it's only the fallback case in of_dma_configure(). Sorry for the noise! Cheers, Robin.
Re: [PATCH v10 4/8] drm/ingenic: Add dw-hdmi driver for jz4780
Hi Paul, > Am 01.12.2021 um 15:03 schrieb Paul Cercueil : > > Hi Nikolaus, Mark, > > Le mer., déc. 1 2021 at 14:51:51 +0100, H. Nikolaus Schaller > a écrit : >> Hi, >>> Am 01.12.2021 um 14:39 schrieb Mark Brown : >>> On Wed, Dec 01, 2021 at 01:02:45PM +, Paul Cercueil wrote: Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller > + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); > + if (IS_ERR(regulator)) { > + ret = PTR_ERR(regulator); >>> Why is this using _optional()? This should only be done when the supply >>> can be physically absent >> There can be +5V for HDMI but without a regulator that is visible to or >> controllable >> by the driver. > > There is always a power supply though. Either a controllable one (through > e.g. a GPIO), or it's just connected to the mains +5V; the pin is never left > floating. In the second case, in DTS the "hdmi-5v" would be connected to some > 5v regulator, even if it's just a dummy VCC-5V regulator. So Mark has a point. > >> So hdmi-5v can be simply missing in DTS in which case the driver does not >> need to >> care about. The driver just can't turn it on or off. > > Please make it mandatory in DTS then, and use devm_regulator_get() in the > driver. Well, I just wonder why the elegant devm_regulator_get_optional() exists at all and seems to be used in ca. 80 places. And if it is not allowed, why some DTS should be forced to add not physically existing dummy-regulators. AFAIR drivers should implement functionality defined by DTS but not the other way round: enforce DTS style. BTW: there is no +5 mains dummy regulator defined in ci20.dts. What I fear is that if we always have to define the mains +5V (which is for example not defined in ci20.dts), which rules stops us from asking to add a dummy-regulator from 110/230V to +5V as well. In last consequence, it seems as if we have to describe all dummy regulators from the power plant to our hdmi-5v :) Since I always follow the KISS principle I tend to leave out what is not relevant... Of course adding a dummy regulator to the DTS allows to avoid the NULL pointer test in the driver code. Anyways, you are maintainers :) So should I spin a v11 for the series or just this patch or how should we do it? BR and thanks, Nikolaus > > Cheers, > -Paul > >>> (in which case I'd expect to see special >>> handling). >> The special case is to not enable/disable the regulator if it does not exist >> and assume that there is hardware providing it otherwise (the driver can't >> know >> that except by using get_optional). This is done by the code below > + if (IS_ERR(regulator)) { >> ... >>> + if (!regulator) >>> + return 0; >>> + ret = regulator_enable(regulator); >> ... >> BR and thanks, >> Nikolaus > >
Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver
Hi Noralf, On Tue, Nov 30, 2021 at 03:30:11PM +0100, Noralf Trønnes wrote: > Den 29.11.2021 10.39, skrev Maxime Ripard: > > On Wed, Nov 24, 2021 at 04:03:07PM -0600, David Lechner wrote: > >> On 11/24/21 9:07 AM, Noralf Trønnes wrote: > > I agree that it doesn't really fit in the DT either though. Noralf, what > > kind of data do we need to setup a display in fbtft? The init sequence, > > and maybe some enable/reset GPIO, plus some timing duration maybe? > > > > There's one similar situation I can think of: wifi chips. Those also > > need a few infos from the DT (like what bus it's connected to, enable > > GPIO, etc) and a different sequence (firmware), sometimes different from > > one board to the other. > > > > Could we have a binding that would be something like: > > > > panel@42 { > > compatible = "panel-spi"; > > model = "panel-from-random-place-42"; > > enable-gpios = <&...>; > > } > > > > And then, the driver would request the init sequence through the > > firmware mechanism using a name generated from the model property. > > > > It allows to support multiple devices in a given system, since the > > firmware name wouldn't conflict, it makes a decent binding, and users > > can adjust the init sequence easily (maybe with a bit of tooling) > > > > Would that work? > > I really like this idea. An added benefit is that one driver can handle > all MIPI DBI compatible controllers avoiding the need to do a patchset > like this for all the various MIPI DBI controllers. The firmware will > just contain numeric commands with parameters, so no need for different > controller drivers to handle the controller specific command names. > > The following is a list of the MIPI DBI compatible controllers currently > in staging/fbtft: ili9341, hx8357d, st7735r, ili9163, ili9163, ili9163, > ili9163, ili9486, ili9481, tinylcd, s6d02a1, s6d02a1, hx8340bn, ili9340. > > The compatible needs to be a bit more specific though since there are 2 > major SPI protocols for these display: MIPI DBI and the one used by > ILI9325 and others. > > The full binding would be something like this: > > panel@42 { > compatible = "panel-mipi-dbi-spi"; > model = "panel-from-random-place-42"; > > /* The MIPI DBI spec lists these powers supply pins */ > vdd-supply = <&...>; > vddi-supply = <&...>; > > /* Optional gpio to drive the RESX line */ > reset-gpios = <&...>; > > /* >* D/CX: Data/Command, Command is active low >* Abcense: Interface option 1 (D/C embedded in 9-bit word) >* Precense: Interface option 3 >*/ > dc-gpios = <&...>; > > /* >* If set the driver won't try to read from the controller to see >* if it's already configured by the bootloader or previously by >* the driver. A readable controller avoids flicker and/or delay >* enabling the pipeline. >* >* This property might not be necessary if we are guaranteed to >* always read back all 1's or 0's when MISO is not connected. >* I don't know if all setups can guarantee that. >*/ > write-only; > > /* Optional ref to backlight node */ > backlight = <&...>; > } It looks decent to me. We'll want Rob to give his opinion though, but it looks in a much better shape compared to what we usually have :) > Many of these controllers also have a RGB interface option for the > pixels and only do configuration over SPI. > Maybe the compatible should reflect these 2 options somehow? I think we'll want a "real" panel for RGB, with its own compatible though. We have a few of these drivers in tree already, so it's better to remain consistent. Maxime signature.asc Description: PGP signature
Re: [PATCH] drm: Return error codes from struct drm_driver.gem_create_object
On Tue, Nov 30, 2021 at 10:52:55AM +0100, Thomas Zimmermann wrote: > GEM helper libraries use struct drm_driver.gem_create_object to let > drivers override GEM object allocation. On failure, the call returns > NULL. > > Change the semantics to make the calls return a pointer-encoded error. > This aligns the callback with its callers. Fixes the ingenic driver, > which already returns an error pointer. > > Also update the callers to handle the involved types more strictly. > > Signed-off-by: Thomas Zimmermann > --- > There is an alternative patch at [1] that updates the value returned > by ingenics' gem_create_object to NULL. Fixing the interface to return > an errno code is more consistent with the rest of the GEM functions. > > [1] https://lore.kernel.org/dri-devel/2028111522.GD1147@kili/ Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/i915: Force ww lock for i915_gem_object_ggtt_pin_ww, v2.
On Tue, 30 Nov 2021 at 09:21, Maarten Lankhorst wrote: > > We will need the lock to unbind the vma, and wait for bind to complete. > Remove the special casing for the !ww path, and force ww locking for all. > > Changes since v1: > - Pass err to for_i915_gem_ww handling for -EDEADLK handling. > > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/i915/i915_drv.h | 7 ++- > drivers/gpu/drm/i915/i915_gem.c | 30 ++ > 2 files changed, 28 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1bfadd9127fc..8322abe194da 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1842,13 +1842,10 @@ i915_gem_object_ggtt_pin_ww(struct > drm_i915_gem_object *obj, > const struct i915_ggtt_view *view, > u64 size, u64 alignment, u64 flags); > > -static inline struct i915_vma * __must_check > +struct i915_vma * __must_check > i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj, > const struct i915_ggtt_view *view, > -u64 size, u64 alignment, u64 flags) > -{ > - return i915_gem_object_ggtt_pin_ww(obj, NULL, view, size, alignment, > flags); > -} > +u64 size, u64 alignment, u64 flags); > > int i915_gem_object_unbind(struct drm_i915_gem_object *obj, >unsigned long flags); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 527228d4da7e..6002045bd194 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -877,6 +877,8 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object > *obj, > struct i915_vma *vma; > int ret; > > + GEM_WARN_ON(!ww); Return something here, or GEM_BUG_ON()? I assume it's going to crash and burn anyway? > + > if (flags & PIN_MAPPABLE && > (!view || view->type == I915_GGTT_VIEW_NORMAL)) { > /* > @@ -936,10 +938,7 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object > *obj, > return ERR_PTR(ret); > } > > - if (ww) > - ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | > PIN_GLOBAL); > - else > - ret = i915_vma_pin(vma, size, alignment, flags | PIN_GLOBAL); > + ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | PIN_GLOBAL); > > if (ret) > return ERR_PTR(ret); > @@ -959,6 +958,29 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object > *obj, > return vma; > } > > +struct i915_vma * __must_check > +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj, > +const struct i915_ggtt_view *view, > +u64 size, u64 alignment, u64 flags) > +{ > + struct i915_gem_ww_ctx ww; > + struct i915_vma *ret; > + int err; > + > + for_i915_gem_ww(&ww, err, true) { > + err = i915_gem_object_lock(obj, &ww); > + if (err) > + continue; > + > + ret = i915_gem_object_ggtt_pin_ww(obj, &ww, view, size, > + alignment, flags); > + if (IS_ERR(ret)) > + err = PTR_ERR(ret); Maybe s/ret/vma/ ? Seeing ret makes me think it's an integer. Anyway, Reviewed-by: Matthew Auld > + } > + > + return err ? ERR_PTR(err) : ret; > +} > + > int > i915_gem_madvise_ioctl(struct drm_device *dev, void *data, >struct drm_file *file_priv) > -- > 2.34.1 >
Re: [PATCH v10 4/8] drm/ingenic: Add dw-hdmi driver for jz4780
On Wed, Dec 01, 2021 at 03:33:24PM +0100, H. Nikolaus Schaller wrote: > > Am 01.12.2021 um 15:03 schrieb Paul Cercueil : > > Please make it mandatory in DTS then, and use devm_regulator_get() in the > > driver. > Well, I just wonder why the elegant devm_regulator_get_optional() exists at > all > and seems to be used in ca. 80 places. Frankly because half of them are broken usages like this since people seem determined to have the most fragile error handling they can :/ There are valid use cases for it, with things like SD cards where some supplies are genuinely optional and simply constrain what features are available if they're omitted from the design. You also see some devices with the ability to replace internal regulators with external ones. > And if it is not allowed, why some DTS should be forced to add not physically > existing dummy-regulators. Again, if the supply can be physically absent that is a sensible use case but that means completely absent, not just not software controllable. We can represent fixed voltage regulators just fine. > AFAIR drivers should implement functionality defined by DTS but not the other > way round: enforce DTS style. > BTW: there is no +5 mains dummy regulator defined in ci20.dts. It wouldn't be the first time a DTS were incomplete, and I'm sure it won't be the last. > What I fear is that if we always have to define the mains +5V (which is for > example not > defined in ci20.dts), which rules stops us from asking to add a > dummy-regulator from 110/230V to +5V as well. It is good practice to specify the full tree of supplies all the way to the main supply rail of the board, this ensures that if we need the information for something we've got it (even if that thing is just that we've got to the root of the tree). There's potential applications in battery supplied devices for managing very low power situations. signature.asc Description: PGP signature
Re: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
Am 01.12.21 um 16:13 schrieb Zhou Qingyang: In radeon_driver_open_kms(), radeon_vm_bo_add() is assigned to vm->ib_bo_va and passes and used in radeon_vm_bo_set_addr(). In radeon_vm_bo_set_addr(), there is a dereference of vm->ib_bo_va, which could lead to a NULL pointer dereference on failure of radeon_vm_bo_add(). Fix this bug by adding a check of vm->ib_bo_va. This bug was found by a static analyzer. The analysis employs differential checking to identify inconsistent security operations (e.g., checks or kfrees) between two code paths and confirms that the inconsistent operations are not recovered in the current function or the callers, so they constitute bugs. Note that, as a bug found by static analysis, it can be a false positive or hard to trigger. Multiple researchers have cross-reviewed the bug. Builds with CONFIG_DRM_RADEON=m show no new warnings, and our static analyzer no longer warns about this code. Fixes: cc9e67e3d700 ("drm/radeon: fix VM IB handling") Signed-off-by: Zhou Qingyang --- Changes in v5: - Use conditions to avoid unnecessary initialization Changes in v4: - Initialize the variables to silence warning Changes in v3: - Fix the bug that good case will also be freed - Improve code style Changes in v2: - Improve the error handling into goto style drivers/gpu/drm/radeon/radeon_kms.c | 36 - 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 482fb0ae6cb5..66aee48fd09d 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -648,6 +648,8 @@ void radeon_driver_lastclose_kms(struct drm_device *dev) int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) { struct radeon_device *rdev = dev->dev_private; + struct radeon_fpriv *fpriv; + struct radeon_vm *vm; int r; file_priv->driver_priv = NULL; @@ -660,8 +662,6 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) /* new gpu have virtual address space support */ if (rdev->family >= CHIP_CAYMAN) { - struct radeon_fpriv *fpriv; - struct radeon_vm *vm; fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL); if (unlikely(!fpriv)) { @@ -672,35 +672,39 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) if (rdev->accel_working) { vm = &fpriv->vm; r = radeon_vm_init(rdev, vm); - if (r) { - kfree(fpriv); - goto out_suspend; - } + if (r) + goto out_fpriv; r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false); - if (r) { - radeon_vm_fini(rdev, vm); - kfree(fpriv); - goto out_suspend; - } + if (r) + goto out_vm_fini; /* map the ib pool buffer read only into * virtual address space */ vm->ib_bo_va = radeon_vm_bo_add(rdev, vm, rdev->ring_tmp_bo.bo); + if (!vm->ib_bo_va) { + r = -ENOMEM; + goto out_vm_fini; + } + r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va, RADEON_VA_IB_OFFSET, RADEON_VM_PAGE_READABLE | RADEON_VM_PAGE_SNOOPED); - if (r) { - radeon_vm_fini(rdev, vm); - kfree(fpriv); - goto out_suspend; - } + if (r) + goto out_vm_fini; } file_priv->driver_priv = fpriv; } + if (!r) I think that test is unecessary now, maybe double check. Either way patch Reviewed-by: Christian König . Alex will probably pick it up now. Thanks for the help, Christian. + goto out_suspend; + +out_vm_fini: + radeon_vm_fini(rdev, vm); +out_fpriv: + kfree(fpriv); out_suspend: pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev);
Re: [PATCH] drm: Return error codes from struct drm_driver.gem_create_object
On 30/11/2021 09:52, Thomas Zimmermann wrote: > GEM helper libraries use struct drm_driver.gem_create_object to let > drivers override GEM object allocation. On failure, the call returns > NULL. > > Change the semantics to make the calls return a pointer-encoded error. > This aligns the callback with its callers. Fixes the ingenic driver, > which already returns an error pointer. > > Also update the callers to handle the involved types more strictly. > > Signed-off-by: Thomas Zimmermann Reviewed-by: Steven Price > --- > There is an alternative patch at [1] that updates the value returned > by ingenics' gem_create_object to NULL. Fixing the interface to return > an errno code is more consistent with the rest of the GEM functions. > > [1] https://lore.kernel.org/dri-devel/2028111522.GD1147@kili/ > --- > drivers/gpu/drm/drm_gem_cma_helper.c| 17 ++--- > drivers/gpu/drm/drm_gem_shmem_helper.c | 17 ++--- > drivers/gpu/drm/drm_gem_vram_helper.c | 4 ++-- > drivers/gpu/drm/lima/lima_gem.c | 2 +- > drivers/gpu/drm/panfrost/panfrost_gem.c | 2 +- > drivers/gpu/drm/v3d/v3d_bo.c| 4 ++-- > drivers/gpu/drm/vc4/vc4_bo.c| 2 +- > drivers/gpu/drm/vgem/vgem_drv.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_object.c | 2 +- > include/drm/drm_drv.h | 5 +++-- > 10 files changed, 32 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c > b/drivers/gpu/drm/drm_gem_cma_helper.c > index 7d4895de9e0d..cefd0cbf9deb 100644 > --- a/drivers/gpu/drm/drm_gem_cma_helper.c > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c > @@ -67,18 +67,21 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size, > bool private) > struct drm_gem_object *gem_obj; > int ret = 0; > > - if (drm->driver->gem_create_object) > + if (drm->driver->gem_create_object) { > gem_obj = drm->driver->gem_create_object(drm, size); > - else > - gem_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL); > - if (!gem_obj) > - return ERR_PTR(-ENOMEM); > + if (IS_ERR(gem_obj)) > + return ERR_CAST(gem_obj); > + cma_obj = to_drm_gem_cma_obj(gem_obj); > + } else { > + cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL); > + if (!cma_obj) > + return ERR_PTR(-ENOMEM); > + gem_obj = &cma_obj->base; > + } > > if (!gem_obj->funcs) > gem_obj->funcs = &drm_gem_cma_default_funcs; > > - cma_obj = container_of(gem_obj, struct drm_gem_cma_object, base); > - > if (private) { > drm_gem_private_object_init(drm, gem_obj, size); > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c > b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 0eeda1012364..7915047cb041 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -56,14 +56,17 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t > size, bool private) > > size = PAGE_ALIGN(size); > > - if (dev->driver->gem_create_object) > + if (dev->driver->gem_create_object) { > obj = dev->driver->gem_create_object(dev, size); > - else > - obj = kzalloc(sizeof(*shmem), GFP_KERNEL); > - if (!obj) > - return ERR_PTR(-ENOMEM); > - > - shmem = to_drm_gem_shmem_obj(obj); > + if (IS_ERR(obj)) > + return ERR_CAST(obj); > + shmem = to_drm_gem_shmem_obj(obj); > + } else { > + shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); > + if (!shmem) > + return ERR_PTR(-ENOMEM); > + obj = &shmem->base; > + } > > if (!obj->funcs) > obj->funcs = &drm_gem_shmem_funcs; > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c > b/drivers/gpu/drm/drm_gem_vram_helper.c > index bfa386b98134..3f00192215d1 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -197,8 +197,8 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct > drm_device *dev, > > if (dev->driver->gem_create_object) { > gem = dev->driver->gem_create_object(dev, size); > - if (!gem) > - return ERR_PTR(-ENOMEM); > + if (IS_ERR(gem)) > + return ERR_CAST(gem); > gbo = drm_gem_vram_of_gem(gem); > } else { > gbo = kzalloc(sizeof(*gbo), GFP_KERNEL); > diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c > index 2723d333c608..f9a9198ef198 100644 > --- a/drivers/gpu/drm/lima/lima_gem.c > +++ b/drivers/gpu/drm/lima/lima_gem.c > @@ -221,7 +221,7 @@ struct drm_gem_object *lima_gem_create_object(struct > drm_device *dev, size_t siz > > bo = kzalloc(sizeof(*bo), GFP_KERNEL); > if (!bo) > - return NULL; > + return ERR_
Re: [PATCH v4 4/4] arm64: dts: qcom: sc7280: Add Display Port node
On Mon 22 Nov 05:29 CST 2021, Sankeerth Billakanti wrote: > From: Kuogee Hsieh > > Signed-off-by: Kuogee Hsieh > Reviewed-by: Stephen Boyd > Signed-off-by: Sankeerth Billakanti Can you please update this to make From: and Signed-off-by: match. Also I don't know how you prepared this patch series, because this says patch 4/4, but it's not part of the same series as the other patches. I did pick up the first 3 patches, but then noticed that you are back at using the original labels, so please see below and send all 4 patches in a proper series. > --- > > Changes in v4: > - Add the patch to display DT change series (Bjorn Andersson) > - Remove the trailing whitespaces > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 90 > +++- > 1 file changed, 88 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 5ad500e..0b2ffd5 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -2693,8 +2693,8 @@ ><&gcc GCC_DISP_GPLL0_CLK_SRC>, ><&dsi_phy 0>, ><&dsi_phy 1>, > - <0>, > - <0>, > + <&dp_phy 0>, > + <&dp_phy 1>, ><&edp_phy 0>, ><&edp_phy 1>; > clock-names = "bi_tcxo", > @@ -2791,6 +2791,13 @@ > remote-endpoint = > <&edp_in>; > }; > }; > + > + port@2 { > +reg = <2>; > +dpu_intf0_out: endpoint { > +remote-endpoint = > <&dp_in>; > +}; > +}; > }; > > mdp_opp_table: opp-table { > @@ -3002,6 +3009,79 @@ > > status = "disabled"; > }; > + > + msm_dp: displayport-controller@ae9 { As requested previously, can you please label this mdss_dp, to make it sort nicely in the dts? Thanks, Bjorn > + compatible = "qcom,sc7280-dp"; > + > + reg = <0 0x0ae9 0 0x1400>; > + > + interrupt-parent = <&mdss>; > + interrupts = <12>; > + > + clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_DP_AUX_CLK>, > + <&dispcc DISP_CC_MDSS_DP_LINK_CLK>, > + <&dispcc > DISP_CC_MDSS_DP_LINK_INTF_CLK>, > + <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>; > + clock-names = "core_iface", > + "core_aux", > + "ctrl_link", > + "ctrl_link_iface", > + "stream_pixel"; > + #clock-cells = <1>; > + assigned-clocks = <&dispcc > DISP_CC_MDSS_DP_LINK_CLK_SRC>, > + <&dispcc > DISP_CC_MDSS_DP_PIXEL_CLK_SRC>; > + assigned-clock-parents = <&dp_phy 0>, <&dp_phy > 1>; > + phys = <&dp_phy>; > + phy-names = "dp"; > + > + operating-points-v2 = <&dp_opp_table>; > + power-domains = <&rpmhpd SC7280_CX>; > + > + #sound-dai-cells = <0>; > + > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + dp_in: endpoint { > + remote-endpoint = > <&dpu_intf0_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + dp_out: endpoint { }; > + }; > + }; > + > + dp_opp_table:
Re: [PATCH v4 1/4] arm64: dts: qcom: sc7280: add display dt nodes
On Mon 22 Nov 05:26 CST 2021, Sankeerth Billakanti wrote: > From: Krishna Manikandan > > Add mdss and mdp DT nodes for sc7280. > > Signed-off-by: Krishna Manikandan > Reported-by: kernel test robot > Reviewed-by: Stephen Boyd > Reported-by: kernel test robot > Signed-off-by: Sankeerth Billakanti > --- > > Changes in v4: > None > > Changes in v3: > None > > Changes in v2: > - Rename display dt nodes (Stephen Boyd) > - Add clock names one per line for readability (Stephen Boyd) > > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 90 > > 1 file changed, 90 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 365a2e0..a4536b6 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -2704,6 +2704,96 @@ > #power-domain-cells = <1>; > }; > > + mdss: display-subsystem@ae0 { > + compatible = "qcom,sc7280-mdss"; > + reg = <0 0x0ae0 0 0x1000>; > + reg-names = "mdss"; > + > + power-domains = <&dispcc DISP_CC_MDSS_CORE_GDSC>; > + > + clocks = <&gcc GCC_DISP_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_CLK>; > + clock-names = "iface", > + "ahb", > + "core"; > + > + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>; > + assigned-clock-rates = <3>; > + > + interrupts = ; > + interrupt-controller; > + #interrupt-cells = <1>; > + > + interconnects = <&mmss_noc MASTER_MDP0 0 &mc_virt > SLAVE_EBI1 0>; > + interconnect-names = "mdp0-mem"; > + > + iommus = <&apps_smmu 0x900 0x402>; > + > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + status = "disabled"; > + > + mdp: display-controller@ae01000 { I believe the only reason to give this a label is so that you can enable it in the dts. But I don't see the point of having it status disabled, given that it should always follow the mdss node's status. > + compatible = "qcom,sc7280-dpu"; > + reg = <0 0x0ae01000 0 0x8f030>, > + <0 0x0aeb 0 0x2008>; > + reg-names = "mdp", "vbif"; > + > + clocks = <&gcc GCC_DISP_HF_AXI_CLK>, > + <&gcc GCC_DISP_SF_AXI_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_LUT_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_CLK>, > + <&dispcc DISP_CC_MDSS_VSYNC_CLK>; > + clock-names = "bus", > + "nrt_bus", > + "iface", > + "lut", > + "core", > + "vsync"; > + assigned-clocks = <&dispcc > DISP_CC_MDSS_MDP_CLK>, > + <&dispcc > DISP_CC_MDSS_VSYNC_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>; > + assigned-clock-rates = <3>, > + <1920>, > + <1920>; > + operating-points-v2 = <&mdp_opp_table>; > + power-domains = <&rpmhpd SC7280_CX>; > + > + interrupt-parent = <&mdss>; > + interrupts = <0>; > + > + status = "disabled"; So my suggestion is to drop this and drop the label. If not, please change the label of this node to mdss_mdp, for sorting purposes. Thanks, Bjorn > + > + mdp_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-2 { > + opp-hz = /bits/ 64 <2>; > + required-opps = > <&rpmhpd_opp_low_svs>; > + }; > + > + opp-3 { > + opp-hz = /bits/ 64 <3>; > +
Re: [PATCH v4 1/4] arm64: dts: qcom: sc7280: add display dt nodes
On Mon 22 Nov 05:26 CST 2021, Sankeerth Billakanti wrote: > From: Krishna Manikandan > > Add mdss and mdp DT nodes for sc7280. > > Signed-off-by: Krishna Manikandan > Reported-by: kernel test robot Sorry, missed this one before sending my reply. "kernel test robot" did not report the lack of mdss nodes in your dts. So please drop this as well. > Reviewed-by: Stephen Boyd > Reported-by: kernel test robot And again. Thanks, Bjorn > Signed-off-by: Sankeerth Billakanti > --- > > Changes in v4: > None > > Changes in v3: > None > > Changes in v2: > - Rename display dt nodes (Stephen Boyd) > - Add clock names one per line for readability (Stephen Boyd) > > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 90 > > 1 file changed, 90 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 365a2e0..a4536b6 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -2704,6 +2704,96 @@ > #power-domain-cells = <1>; > }; > > + mdss: display-subsystem@ae0 { > + compatible = "qcom,sc7280-mdss"; > + reg = <0 0x0ae0 0 0x1000>; > + reg-names = "mdss"; > + > + power-domains = <&dispcc DISP_CC_MDSS_CORE_GDSC>; > + > + clocks = <&gcc GCC_DISP_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_CLK>; > + clock-names = "iface", > + "ahb", > + "core"; > + > + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>; > + assigned-clock-rates = <3>; > + > + interrupts = ; > + interrupt-controller; > + #interrupt-cells = <1>; > + > + interconnects = <&mmss_noc MASTER_MDP0 0 &mc_virt > SLAVE_EBI1 0>; > + interconnect-names = "mdp0-mem"; > + > + iommus = <&apps_smmu 0x900 0x402>; > + > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + status = "disabled"; > + > + mdp: display-controller@ae01000 { > + compatible = "qcom,sc7280-dpu"; > + reg = <0 0x0ae01000 0 0x8f030>, > + <0 0x0aeb 0 0x2008>; > + reg-names = "mdp", "vbif"; > + > + clocks = <&gcc GCC_DISP_HF_AXI_CLK>, > + <&gcc GCC_DISP_SF_AXI_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_LUT_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_CLK>, > + <&dispcc DISP_CC_MDSS_VSYNC_CLK>; > + clock-names = "bus", > + "nrt_bus", > + "iface", > + "lut", > + "core", > + "vsync"; > + assigned-clocks = <&dispcc > DISP_CC_MDSS_MDP_CLK>, > + <&dispcc > DISP_CC_MDSS_VSYNC_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>; > + assigned-clock-rates = <3>, > + <1920>, > + <1920>; > + operating-points-v2 = <&mdp_opp_table>; > + power-domains = <&rpmhpd SC7280_CX>; > + > + interrupt-parent = <&mdss>; > + interrupts = <0>; > + > + status = "disabled"; > + > + mdp_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-2 { > + opp-hz = /bits/ 64 <2>; > + required-opps = > <&rpmhpd_opp_low_svs>; > + }; > + > + opp-3 { > + opp-hz = /bits/ 64 <3>; > + required-opps = > <&rpmhpd_opp_svs>; > + }; > + > + opp-38000 { > +
Re: [PATCH v4 2/4] arm64: dts: qcom: sc7280: Add DSI display nodes
On Mon 22 Nov 05:26 CST 2021, Sankeerth Billakanti wrote: > From: Krishna Manikandan > > Add DSI controller and PHY nodes for sc7280. > > Signed-off-by: Rajeev Nandan > Signed-off-by: Krishna Manikandan > Reviewed-by: Matthias Kaehlcke > Reviewed-by: Stephen Boyd > Signed-off-by: Sankeerth Billakanti > --- > > Changes in v4: > None > > Changes in v3: > - Add the dsi_phy clocks (Kuogee Hsieh) > - One clock cell per line (Stephen Boyd) > > Changes in v2: > - Drop flags from interrupts (Stephen Boyd) > - Rename dsi-opp-table (Stephen Boyd) > - Rename dsi phy node (Stephen Boyd) > > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 111 > ++- > 1 file changed, 109 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index a4536b6..12c4d32 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -2691,8 +2691,14 @@ > reg = <0 0xaf0 0 0x2>; > clocks = <&rpmhcc RPMH_CXO_CLK>, ><&gcc GCC_DISP_GPLL0_CLK_SRC>, > - <0>, <0>, <0>, <0>, <0>, <0>; > - clock-names = "bi_tcxo", "gcc_disp_gpll0_clk", > + <&dsi_phy 0>, > + <&dsi_phy 1>, > + <0>, > + <0>, > + <0>, > + <0>; > + clock-names = "bi_tcxo", > + "gcc_disp_gpll0_clk", > "dsi0_phy_pll_out_byteclk", > "dsi0_phy_pll_out_dsiclk", > "dp_phy_pll_link_clk", > @@ -2768,6 +2774,18 @@ > > status = "disabled"; > > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + dpu_intf1_out: endpoint { > + remote-endpoint = > <&dsi0_in>; > + }; > + }; > + }; > + > mdp_opp_table: opp-table { > compatible = "operating-points-v2"; > > @@ -2792,6 +2810,95 @@ > }; > }; > }; > + > + dsi0: dsi@ae94000 { Please label this mdss_dsi0, to make it group nicely when sorted in the dts. > + compatible = "qcom,mdss-dsi-ctrl"; > + reg = <0 0x0ae94000 0 0x400>; > + reg-names = "dsi_ctrl"; > + > + interrupt-parent = <&mdss>; > + interrupts = <4>; > + > + clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>, > + <&dispcc DISP_CC_MDSS_BYTE0_INTF_CLK>, > + <&dispcc DISP_CC_MDSS_PCLK0_CLK>, > + <&dispcc DISP_CC_MDSS_ESC0_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&gcc GCC_DISP_HF_AXI_CLK>; > + clock-names = "byte", > + "byte_intf", > + "pixel", > + "core", > + "iface", > + "bus"; > + > + operating-points-v2 = <&dsi_opp_table>; > + power-domains = <&rpmhpd SC7280_CX>; > + > + phys = <&dsi_phy>; > + phy-names = "dsi"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + dsi0_in: endpoint { > + remote-endpoint = > <&dpu_intf1_out>; > + }; > + }; > + > + port@1 { >
[PATCH 1/3] drm/ast: Handle failed I2C initialization gracefully
I2C initialization is allowed to fail. In this case, create a connector without DDC adapter. The current code would dereference a NULL pointer. Reading the modes from the connector is supposed to work without I2C adapter. Add the respective test. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_mode.c | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 1e30eaeb0e1b..3f0c8c1f9777 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1226,7 +1226,7 @@ static int ast_get_modes(struct drm_connector *connector) else kfree(edid); } - if (!flags) + if (!flags && ast_connector->i2c) edid = drm_get_edid(connector, &ast_connector->i2c->adapter); if (edid) { drm_connector_update_edid_property(&ast_connector->base, edid); @@ -1332,10 +1332,13 @@ static int ast_connector_init(struct drm_device *dev) if (!ast_connector->i2c) drm_err(dev, "failed to add ddc bus for connector\n"); - drm_connector_init_with_ddc(dev, connector, - &ast_connector_funcs, - DRM_MODE_CONNECTOR_VGA, - &ast_connector->i2c->adapter); + if (ast_connector->i2c) + drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs, + DRM_MODE_CONNECTOR_VGA, + &ast_connector->i2c->adapter); + else + drm_connector_init(dev, connector, &ast_connector_funcs, + DRM_MODE_CONNECTOR_VGA); drm_connector_helper_add(connector, &ast_connector_helper_funcs); -- 2.34.0
[PATCH 2/3] drm/ast: Convert I2C code to managed cleanup
Release the I2C adapter as part of the DRM device cleanup. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_mode.c | 36 +++--- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 3f0c8c1f9777..a84dced95440 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -48,7 +49,6 @@ #include "ast_tables.h" static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev); -static void ast_i2c_destroy(struct ast_i2c_chan *i2c); static inline void ast_load_palette_index(struct ast_private *ast, u8 index, u8 red, u8 green, @@ -1300,14 +1300,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, return flags; } -static void ast_connector_destroy(struct drm_connector *connector) -{ - struct ast_connector *ast_connector = to_ast_connector(connector); - - ast_i2c_destroy(ast_connector->i2c); - drm_connector_cleanup(connector); -} - static const struct drm_connector_helper_funcs ast_connector_helper_funcs = { .get_modes = ast_get_modes, .mode_valid = ast_mode_valid, @@ -1316,7 +1308,7 @@ static const struct drm_connector_helper_funcs ast_connector_helper_funcs = { static const struct drm_connector_funcs ast_connector_funcs = { .reset = drm_atomic_helper_connector_reset, .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = ast_connector_destroy, + .destroy = drm_connector_cleanup, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; @@ -1493,6 +1485,14 @@ static void set_data(void *i2c_priv, int data) } } +static void ast_i2c_release(struct drm_device *dev, void *res) +{ + struct ast_i2c_chan *i2c = res; + + i2c_del_adapter(&i2c->adapter); + kfree(i2c); +} + static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) { struct ast_i2c_chan *i2c; @@ -1521,19 +1521,15 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) ret = i2c_bit_add_bus(&i2c->adapter); if (ret) { drm_err(dev, "Failed to register bit i2c\n"); - goto out_free; + goto out_kfree; } + ret = drmm_add_action_or_reset(dev, ast_i2c_release, i2c); + if (ret) + return NULL; return i2c; -out_free: - kfree(i2c); - return NULL; -} -static void ast_i2c_destroy(struct ast_i2c_chan *i2c) -{ - if (!i2c) - return; - i2c_del_adapter(&i2c->adapter); +out_kfree: kfree(i2c); + return NULL; } -- 2.34.0
[PATCH 0/3] ast: Fix I2C corner cases wrt init/cleanup
The VGA connector in ast is supposed to work without I2C. Currently, this isn't correctly implemented in several places. Fix this. Also add managed cleanup of the I2C code, and fail if the connector setup fail. Tested on AST2100 hardware. Thomas Zimmermann (3): drm/ast: Handle failed I2C initialization gracefully drm/ast: Convert I2C code to managed cleanup drm/ast: Fail if connector initialization fails drivers/gpu/drm/ast/ast_mode.c | 52 ++ 1 file changed, 27 insertions(+), 25 deletions(-) base-commit: 6a8f90ec433e2f5de5fc16d7a4839771b7027cc0 -- 2.34.0
[PATCH 3/3] drm/ast: Fail if connector initialization fails
Update the connector code to fail if the connector could not be initialized. The current code just ignored the error and failed later when the connector was supposed to be used. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_mode.c | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index a84dced95440..c988991cad33 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1319,18 +1319,21 @@ static int ast_connector_init(struct drm_device *dev) struct ast_connector *ast_connector = &ast->connector; struct drm_connector *connector = &ast_connector->base; struct drm_encoder *encoder = &ast->encoder; + int ret; ast_connector->i2c = ast_i2c_create(dev); if (!ast_connector->i2c) drm_err(dev, "failed to add ddc bus for connector\n"); if (ast_connector->i2c) - drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs, - DRM_MODE_CONNECTOR_VGA, - &ast_connector->i2c->adapter); + ret = drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs, + DRM_MODE_CONNECTOR_VGA, + &ast_connector->i2c->adapter); else - drm_connector_init(dev, connector, &ast_connector_funcs, - DRM_MODE_CONNECTOR_VGA); + ret = drm_connector_init(dev, connector, &ast_connector_funcs, +DRM_MODE_CONNECTOR_VGA); + if (ret) + return ret; drm_connector_helper_add(connector, &ast_connector_helper_funcs); -- 2.34.0
Re: [PATCH v4 3/4] arm64: dts: qcom: sc7280: add edp display dt nodes
On Mon 22 Nov 05:26 CST 2021, Sankeerth Billakanti wrote: > Add edp controller and phy DT nodes for sc7280. > > Signed-off-by: Krishna Manikandan If Krishna authored the patch (he certified its origin first), then he should be From: as well. > Reviewed-by: Stephen Boyd > Signed-off-by: Sankeerth Billakanti > --- > > Changes in v4: > None > > Changes in v3: > - Add one clock cell per line (Stephen Boyd) > - Unit address should match first reg property (Stephen Boyd) > - Remove new line (Stephen Boyd) > - Add the dsi_phy clocks in dispcc (Kuogee Hsieh) > > Changes in v2: > - Move regulator definitions to board file (Matthias Kaehlcke) > - Move the gpio definitions to board file (Matthias Kaehlcke) > - Move the pinconf to board file (Matthias Kaehlcke) > - Move status property (Stephen Boyd) > - Drop flags from interrupts (Stephen Boyd) > - Add clock names one per line for readability (Stephen Boyd) > - Rename edp-opp-table (Stephen Boyd) > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 107 > ++- > 1 file changed, 105 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 12c4d32..5ad500e 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -2695,8 +2695,8 @@ ><&dsi_phy 1>, ><0>, ><0>, > - <0>, > - <0>; > + <&edp_phy 0>, > + <&edp_phy 1>; > clock-names = "bi_tcxo", > "gcc_disp_gpll0_clk", > "dsi0_phy_pll_out_byteclk", > @@ -2784,6 +2784,13 @@ > remote-endpoint = > <&dsi0_in>; > }; > }; > + > + port@1 { > + reg = <1>; > + dpu_intf5_out: endpoint { > + remote-endpoint = > <&edp_in>; > + }; > + }; > }; > > mdp_opp_table: opp-table { > @@ -2899,6 +2906,102 @@ > > status = "disabled"; > }; > + > + msm_edp: edp@aea { mdss_edp: > + compatible = "qcom,sc7280-edp"; > + > + reg = <0 0xaea 0 0x200>, > + <0 0xaea0200 0 0x200>, > + <0 0xaea0400 0 0xc00>, > + <0 0xaea1000 0 0x400>; > + > + interrupt-parent = <&mdss>; > + interrupts = <14>; > + > + clocks = <&rpmhcc RPMH_CXO_CLK>, > + <&gcc GCC_EDP_CLKREF_EN>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_EDP_AUX_CLK>, > + <&dispcc DISP_CC_MDSS_EDP_LINK_CLK>, > + <&dispcc > DISP_CC_MDSS_EDP_LINK_INTF_CLK>, > + <&dispcc DISP_CC_MDSS_EDP_PIXEL_CLK>; > + clock-names = "core_xo", > + "core_ref", > + "core_iface", > + "core_aux", > + "ctrl_link", > + "ctrl_link_iface", > + "stream_pixel"; > + #clock-cells = <1>; > + assigned-clocks = <&dispcc > DISP_CC_MDSS_EDP_LINK_CLK_SRC>, > + <&dispcc > DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>; > + assigned-clock-parents = <&edp_phy 0>, > <&edp_phy 1>; > + > + phys = <&edp_phy>; > + phy-names = "dp"; > + > + operating-points-v2 = <&edp_opp_table>; > + power-domains = <&rpmhpd SC7280_CX>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > +
Re: [PATCH] drm/komeda: Fix an undefined behavior bug in komeda_plane_add()
On 30/11/2021 14:25, Zhou Qingyang wrote: > In komeda_plane_add(), komeda_get_layer_fourcc_list() is assigned to > formats and used in drm_universal_plane_init(). > drm_universal_plane_init() passes formats to > __drm_universal_plane_init(). __drm_universal_plane_init() further > passes formats to memcpy() as src parameter, which could lead to an > undefined behavior bug on failure of komeda_get_layer_fourcc_list(). > > Fix this bug by adding a check of formats. > > This bug was found by a static analyzer. The analysis employs > differential checking to identify inconsistent security operations > (e.g., checks or kfrees) between two code paths and confirms that the > inconsistent operations are not recovered in the current function or > the callers, so they constitute bugs. > > Note that, as a bug found by static analysis, it can be a false > positive or hard to trigger. Multiple researchers have cross-reviewed > the bug. > > Builds with CONFIG_DRM_KOMEDA=m show no new warnings, > and our static analyzer no longer warns about this code. > > Fixes: 61f1c4a8ab75 ("drm/komeda: Attach komeda_dev to DRM-KMS") > Signed-off-by: Zhou Qingyang > --- > drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > index d63d83800a8a..dd3f17e970dd 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > @@ -265,6 +265,10 @@ static int komeda_plane_add(struct komeda_kms_dev *kms, > > formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl, > layer->layer_type, &n_formats); > + if (!formats) { > + err = -ENOMEM; > + goto cleanup; > + } If this executes it will cause undefined behaviour... The cleanup code calls komeda_plane_destroy() which calls drm_plane_cleanup() which does (amongst other things) a list_del(&plane->head). But the plane hasn't been put on a list yet as that's done in drm_universal_plane_init(). So in this case we simple want to do: if (!formats) { kfree(kplane); return -ENOMEM; } Note that without this 'fix' a NULL return from komeda_get_layer_fourcc_list() would leave n_formats==0, so while the NULL pointer is passed into memcpy() it is also passed a length of 0. Which I believe is safe. However while looking at this function... > > err = drm_universal_plane_init(&kms->base, plane, > get_possible_crtcs(kms, c->pipeline), > This call to drm_universal_plane_init() can fail early before plane->head has been initialised. In which case the following: > komeda_put_fourcc_list(formats); > > if (err) > goto cleanup; commits the exact same sin and would cause a similar NULL dereference in drm_plane_cleanup(). Steve
Re: [PATCH v2] drm/gma500/cdv: Fix a wild pointer dereference in cdv_intel_dp_get_modes()
On Wed, Dec 1, 2021 at 4:29 PM Zhou Qingyang wrote: > > In cdv_intel_dp_get_modes(), the third return value of > drm_mode_duplicate() is assigned to mode and used in > drm_mode_probed_add(). drm_mode_probed_add() passes mode->head to > list_add_tail(). list_add_tail() will further call __list_add() and > there is a dereference of mode->head in __list_add(), which could lead > to a wild pointer dereference on failure of drm_mode_duplicate(). > > Fix this bug by adding a check of mode. > > This bug was found by a static analyzer. The analysis employs > differential checking to identify inconsistent security operations > (e.g., checks or kfrees) between two code paths and confirms that the > inconsistent operations are not recovered in the current function or > the callers, so they constitute bugs. > > Note that, as a bug found by static analysis, it can be a false > positive or hard to trigger. Multiple researchers have cross-reviewed > the bug. > Is it really necessary to explain what the static analyzer does and that it can be faulty in every single patch? "This bug was found by a static analyzer" is enough for me. > Builds with CONFIG_DRM_GMA500=m show no new warnings, > and our static analyzer no longer warns about this code. I assume all patches to be at least compile tested before submitted, so if you didn't actually run this code on hardware it's better to replace the above with: "Only compile tested". -Patrik > > Fixes: d112a8163f83 ("gma500/cdv: Add eDP support") > Signed-off-by: Zhou Qingyang > --- > Changes in V2: > - Instead of returning -ENOMEM, this patch returns 0 > - Use DRM_DEBUG_KMS to report the failure of drm_mode_duplicate() > > drivers/gpu/drm/gma500/cdv_intel_dp.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c > b/drivers/gpu/drm/gma500/cdv_intel_dp.c > index ba6ad1466374..bf47db488b7b 100644 > --- a/drivers/gpu/drm/gma500/cdv_intel_dp.c > +++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c > @@ -1773,6 +1773,11 @@ static int cdv_intel_dp_get_modes(struct drm_connector > *connector) > if (intel_dp->panel_fixed_mode != NULL) { > struct drm_display_mode *mode; > mode = drm_mode_duplicate(dev, > intel_dp->panel_fixed_mode); > + if (!mode) { > + DRM_DEBUG_KMS("Failure in > drm_mode_duplicate()\n"); > + return 0; > + } > + > drm_mode_probed_add(connector, mode); > return 1; > } > -- > 2.25.1 >
Re: [PATCH 09/12] arm64: dts: rockchip: rk356x: Add HDMI nodes
On Wed, Nov 17, 2021 at 09:13:33AM -0600, Rob Herring wrote: > On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer wrote: > > > > Add support for the HDMI port found on RK3568. > > > > Signed-off-by: Sascha Hauer > > --- > > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 65 > > 1 file changed, 65 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > index 6ebf7c14e096a..53be61a7ce595 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > @@ -472,18 +472,36 @@ vp0: port@0 { > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0>; > > + > > + vp0_out_hdmi: endpoint@0 { > > + reg = <0>; > > + remote-endpoint = <&hdmi_in_vp0>; > > + status = "disabled"; > > + }; > > }; > > > > vp1: port@1 { > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <1>; > > + > > + vp1_out_hdmi: endpoint@0 { > > + reg = <0>; > > + remote-endpoint = <&hdmi_in_vp1>; > > + status = "disabled"; > > + }; > > }; > > > > vp2: port@2 { > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <2>; > > + > > + vp2_out_hdmi: endpoint@0 { > > + reg = <0>; > > + remote-endpoint = <&hdmi_in_vp2>; > > + status = "disabled"; > > + }; > > }; > > }; > > }; > > @@ -499,6 +517,53 @@ vop_mmu: iommu@fe043e00 { > > status = "disabled"; > > }; > > > > + hdmi: hdmi@fe0a { > > + compatible = "rockchip,rk3568-dw-hdmi"; > > + reg = <0x0 0xfe0a 0x0 0x2>; > > + interrupts = ; > > + clocks = <&cru PCLK_HDMI_HOST>, > > +<&cru CLK_HDMI_SFR>, > > +<&cru CLK_HDMI_CEC>, > > +<&cru HCLK_VOP>; > > + clock-names = "iahb", "isfr", "cec", "hclk"; > > + power-domains = <&power RK3568_PD_VO>; > > + reg-io-width = <4>; > > + rockchip,grf = <&grf>; > > + #sound-dai-cells = <0>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&hdmitx_scl &hdmitx_sda &hdmitxm0_cec>; > > + status = "disabled"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + hdmi_in: port@0 { > > > The schema says there is only 1 'port' node. Please run schema > validation when making DT changes. > > However, an HDMI bridge should also define an output port to a > connector node (or another bridge). So the fix is to allow 'port@0' > and add an output port. The rockchip bindings traditionally don't have a connector explicitly specified in their device trees. I'll stick to that for the next round. If necessary I'll look later what it takes to add a connector node. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v4 2/6] drm: improve drm_buddy_alloc function
- Make drm_buddy_alloc a single function to handle range allocation and non-range allocation demands - Implemented a new function alloc_range() which allocates the requested power-of-two block comply with range limitations - Moved order computation and memory alignment logic from i915 driver to drm buddy v2: merged below changes to keep the build unbroken - drm_buddy_alloc_range() becomes obsolete and may be removed - enable ttm range allocation (fpfn / lpfn) support in i915 driver - apply enhanced drm_buddy_alloc() function to i915 driver v3(Matthew Auld): - Fix alignment issues and remove unnecessary list_empty check - add more validation checks for input arguments - make alloc_range() block allocations as bottom-up - optimize order computation logic - replace uint64_t with u64, which is preferred in the kernel v4(Matthew Auld): - keep drm_buddy_alloc_range() function implementation for generic actual range allocations - keep alloc_range() implementation for end bias allocations Signed-off-by: Arunpravin --- drivers/gpu/drm/drm_buddy.c | 316 +- drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 67 ++-- drivers/gpu/drm/i915/i915_ttm_buddy_manager.h | 2 + include/drm/drm_buddy.h | 22 +- 4 files changed, 285 insertions(+), 122 deletions(-) diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 9340a4b61c5a..7f47632821f4 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -280,23 +280,97 @@ void drm_buddy_free_list(struct drm_buddy_mm *mm, struct list_head *objects) } EXPORT_SYMBOL(drm_buddy_free_list); -/** - * drm_buddy_alloc - allocate power-of-two blocks - * - * @mm: DRM buddy manager to allocate from - * @order: size of the allocation - * - * The order value here translates to: - * - * 0 = 2^0 * mm->chunk_size - * 1 = 2^1 * mm->chunk_size - * 2 = 2^2 * mm->chunk_size - * - * Returns: - * allocated ptr to the &drm_buddy_block on success - */ -struct drm_buddy_block * -drm_buddy_alloc(struct drm_buddy_mm *mm, unsigned int order) +static inline bool overlaps(u64 s1, u64 e1, u64 s2, u64 e2) +{ + return s1 <= e2 && e1 >= s2; +} + +static inline bool contains(u64 s1, u64 e1, u64 s2, u64 e2) +{ + return s1 <= s2 && e1 >= e2; +} + +static struct drm_buddy_block * +alloc_range_bias(struct drm_buddy_mm *mm, +u64 start, u64 end, +unsigned int order) +{ + struct drm_buddy_block *block; + struct drm_buddy_block *buddy; + LIST_HEAD(dfs); + int err; + int i; + + end = end - 1; + + for (i = 0; i < mm->n_roots; ++i) + list_add_tail(&mm->roots[i]->tmp_link, &dfs); + + do { + u64 block_start; + u64 block_end; + + block = list_first_entry_or_null(&dfs, +struct drm_buddy_block, +tmp_link); + if (!block) + break; + + list_del(&block->tmp_link); + + if (drm_buddy_block_order(block) < order) + continue; + + block_start = drm_buddy_block_offset(block); + block_end = block_start + drm_buddy_block_size(mm, block) - 1; + + if (!overlaps(start, end, block_start, block_end)) + continue; + + if (drm_buddy_block_is_allocated(block)) + continue; + + if (contains(start, end, block_start, block_end) && + order == drm_buddy_block_order(block)) { + /* +* Find the free block within the range. +*/ + if (drm_buddy_block_is_free(block)) + return block; + + continue; + } + + if (!drm_buddy_block_is_split(block)) { + err = split_block(mm, block); + if (unlikely(err)) + goto err_undo; + } + + list_add(&block->right->tmp_link, &dfs); + list_add(&block->left->tmp_link, &dfs); + } while (1); + + return ERR_PTR(-ENOSPC); + +err_undo: + /* +* We really don't want to leave around a bunch of split blocks, since +* bigger is better, so make sure we merge everything back before we +* free the allocated blocks. +*/ + buddy = get_buddy(block); + if (buddy && + (drm_buddy_block_is_free(block) && +drm_buddy_block_is_free(buddy))) + __drm_buddy_free(mm, block); + return ERR_PTR(err); +} + +static struct drm_buddy_block * +alloc_from_freelist(struct drm_buddy_mm *mm, + unsigned int order, + unsigned long flags) { struct drm_
[PATCH v4 3/6] drm: implement top-down allocation method
Implemented a function which walk through the order list, compares the offset and returns the maximum offset block, this method is unpredictable in obtaining the high range address blocks which depends on allocation and deallocation. for instance, if driver requests address at a low specific range, allocator traverses from the root block and splits the larger blocks until it reaches the specific block and in the process of splitting, lower orders in the freelist are occupied with low range address blocks and for the subsequent TOPDOWN memory request we may return the low range blocks.To overcome this issue, we may go with the below approach. The other approach, sorting each order list entries in ascending order and compares the last entry of each order list in the freelist and return the max block. This creates sorting overhead on every drm_buddy_free() request and split up of larger blocks for a single page request. v2: - Fix alignment issues(Matthew Auld) - Remove unnecessary list_empty check(Matthew Auld) - merged the below patch to see the feature in action - add top-down alloc support to i915 driver Signed-off-by: Arunpravin --- drivers/gpu/drm/drm_buddy.c | 36 --- drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 3 ++ include/drm/drm_buddy.h | 1 + 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 7f47632821f4..eddc1eeda02e 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -367,6 +367,26 @@ alloc_range_bias(struct drm_buddy_mm *mm, return ERR_PTR(err); } +static struct drm_buddy_block * +get_maxblock(struct list_head *head) +{ + struct drm_buddy_block *max_block = NULL, *node; + + max_block = list_first_entry_or_null(head, +struct drm_buddy_block, +link); + if (!max_block) + return NULL; + + list_for_each_entry(node, head, link) { + if (drm_buddy_block_offset(node) > + drm_buddy_block_offset(max_block)) + max_block = node; + } + + return max_block; +} + static struct drm_buddy_block * alloc_from_freelist(struct drm_buddy_mm *mm, unsigned int order, @@ -377,11 +397,17 @@ alloc_from_freelist(struct drm_buddy_mm *mm, int err; for (i = order; i <= mm->max_order; ++i) { - block = list_first_entry_or_null(&mm->free_list[i], -struct drm_buddy_block, -link); - if (block) - break; + if (flags & DRM_BUDDY_TOPDOWN_ALLOCATION) { + block = get_maxblock(&mm->free_list[i]); + if (block) + break; + } else { + block = list_first_entry_or_null(&mm->free_list[i], +struct drm_buddy_block, +link); + if (block) + break; + } } if (!block) diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c index 7621d42155e6..7c58efb60dba 100644 --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c @@ -53,6 +53,9 @@ static int i915_ttm_buddy_man_alloc(struct ttm_resource_manager *man, INIT_LIST_HEAD(&bman_res->blocks); bman_res->mm = mm; + if (place->flags & TTM_PL_FLAG_TOPDOWN) + bman_res->flags |= DRM_BUDDY_TOPDOWN_ALLOCATION; + if (place->fpfn || lpfn != man->size) bman_res->flags |= DRM_BUDDY_RANGE_ALLOCATION; diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h index 221de702e909..316ac0d25f08 100644 --- a/include/drm/drm_buddy.h +++ b/include/drm/drm_buddy.h @@ -28,6 +28,7 @@ }) #define DRM_BUDDY_RANGE_ALLOCATION (1 << 0) +#define DRM_BUDDY_TOPDOWN_ALLOCATION (1 << 1) struct drm_buddy_block { #define DRM_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12) -- 2.25.1
[PATCH v4 1/6] drm: move the buddy allocator from i915 into common drm
Move the base i915 buddy allocator code into drm - Move i915_buddy.h to include/drm - Move i915_buddy.c to drm root folder - Rename "i915" string with "drm" string wherever applicable - Rename "I915" string with "DRM" string wherever applicable - Fix header file dependencies - Fix alignment issues - add Makefile support for drm buddy - export functions and write kerneldoc description - Remove i915 selftest config check condition as buddy selftest will be moved to drm selftest folder cleanup i915 buddy references in i915 driver module and replace with drm buddy v2: - include header file in alphabetical order(Thomas) - merged changes listed in the body section into a single patch to keep the build intact(Christian, Jani) v3: - make drm buddy a separate module(Thomas, Christian) Signed-off-by: Arunpravin --- drivers/gpu/drm/Kconfig | 6 + drivers/gpu/drm/Makefile | 2 + drivers/gpu/drm/drm_buddy.c | 516 ++ drivers/gpu/drm/i915/Kconfig | 1 + drivers/gpu/drm/i915/Makefile | 1 - drivers/gpu/drm/i915/i915_buddy.c | 466 drivers/gpu/drm/i915/i915_buddy.h | 143 - drivers/gpu/drm/i915/i915_module.c| 3 - drivers/gpu/drm/i915/i915_scatterlist.c | 11 +- drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 33 +- drivers/gpu/drm/i915/i915_ttm_buddy_manager.h | 4 +- include/drm/drm_buddy.h | 154 ++ 12 files changed, 703 insertions(+), 637 deletions(-) create mode 100644 drivers/gpu/drm/drm_buddy.c delete mode 100644 drivers/gpu/drm/i915/i915_buddy.c delete mode 100644 drivers/gpu/drm/i915/i915_buddy.h create mode 100644 include/drm/drm_buddy.h diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 0039df26854b..7a4a66d54782 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -197,6 +197,12 @@ config DRM_TTM GPU memory types. Will be enabled automatically if a device driver uses it. +config DRM_BUDDY + tristate + depends on DRM + help + A page based buddy allocator + config DRM_VRAM_HELPER tristate depends on DRM diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 0dff40bb863c..e62e432bf1e5 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -35,6 +35,8 @@ drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o +obj-$(CONFIG_DRM_BUDDY) += drm_buddy.o + drm_vram_helper-y := drm_gem_vram_helper.o obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c new file mode 100644 index ..9340a4b61c5a --- /dev/null +++ b/drivers/gpu/drm/drm_buddy.c @@ -0,0 +1,516 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2021 Intel Corporation + */ + +#include +#include +#include + +#include + +static struct drm_buddy_block *drm_block_alloc(struct drm_buddy_mm *mm, + struct drm_buddy_block *parent, + unsigned int order, + u64 offset) +{ + struct drm_buddy_block *block; + + BUG_ON(order > DRM_BUDDY_MAX_ORDER); + + block = kmem_cache_zalloc(mm->slab_blocks, GFP_KERNEL); + if (!block) + return NULL; + + block->header = offset; + block->header |= order; + block->parent = parent; + + BUG_ON(block->header & DRM_BUDDY_HEADER_UNUSED); + return block; +} + +static void drm_block_free(struct drm_buddy_mm *mm, + struct drm_buddy_block *block) +{ + kmem_cache_free(mm->slab_blocks, block); +} + +static void mark_allocated(struct drm_buddy_block *block) +{ + block->header &= ~DRM_BUDDY_HEADER_STATE; + block->header |= DRM_BUDDY_ALLOCATED; + + list_del(&block->link); +} + +static void mark_free(struct drm_buddy_mm *mm, + struct drm_buddy_block *block) +{ + block->header &= ~DRM_BUDDY_HEADER_STATE; + block->header |= DRM_BUDDY_FREE; + + list_add(&block->link, +&mm->free_list[drm_buddy_block_order(block)]); +} + +static void mark_split(struct drm_buddy_block *block) +{ + block->header &= ~DRM_BUDDY_HEADER_STATE; + block->header |= DRM_BUDDY_SPLIT; + + list_del(&block->link); +} + +/** + * drm_buddy_init - init memory manager + * + * @mm: DRM buddy manager to initialize + * @size: size in bytes to manage + * @chunk_size: minimum page size in bytes for our allocations + * + * Initializes the memory manager and its resources. + * + * Returns: + * 0 on success, error code on failure. + */ +int drm_buddy_init(struct drm_buddy_mm *mm, u64 size, u64 chunk_size) +{ + unsigned int i; + u64 offset; + + if (size < chunk_si
[PATCH v4 4/6] drm: implement a method to free unused pages
On contiguous allocation, we round up the size to the *next* power of 2, implement a function to free the unused pages after the newly allocate block. v2(Matthew Auld): - replace function name 'drm_buddy_free_unused_pages' with drm_buddy_block_trim - replace input argument name 'actual_size' with 'new_size' - add more validation checks for input arguments - add overlaps check to avoid needless searching and splitting - merged the below patch to see the feature in action - add free unused pages support to i915 driver - lock drm_buddy_block_trim() function as it calls mark_free/mark_split are all globally visible v3: - remove drm_buddy_block_trim() error handling and print a warn message if it fails Signed-off-by: Arunpravin --- drivers/gpu/drm/drm_buddy.c | 72 ++- drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 10 +++ include/drm/drm_buddy.h | 4 ++ 3 files changed, 83 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index eddc1eeda02e..707efc82216d 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -434,7 +434,8 @@ alloc_from_freelist(struct drm_buddy_mm *mm, static int __alloc_range(struct drm_buddy_mm *mm, struct list_head *dfs, u64 start, u64 size, -struct list_head *blocks) +struct list_head *blocks, +bool trim_path) { struct drm_buddy_block *block; struct drm_buddy_block *buddy; @@ -480,8 +481,20 @@ static int __alloc_range(struct drm_buddy_mm *mm, if (!drm_buddy_block_is_split(block)) { err = split_block(mm, block); - if (unlikely(err)) + if (unlikely(err)) { + if (trim_path) + /* +* Here in case of trim, we return and dont goto +* split failure path as it removes from the +* original list and potentially also freeing +* the block. so we could leave as it is, +* worse case we get some internal fragmentation +* and leave the decision to the user +*/ + return err; + goto err_undo; + } } list_add(&block->right->tmp_link, dfs); @@ -535,8 +548,61 @@ static int __drm_buddy_alloc_range(struct drm_buddy_mm *mm, for (i = 0; i < mm->n_roots; ++i) list_add_tail(&mm->roots[i]->tmp_link, &dfs); - return __alloc_range(mm, &dfs, start, size, blocks); + return __alloc_range(mm, &dfs, start, size, blocks, 0); +} + +/** + * drm_buddy_block_trim - free unused pages + * + * @mm: DRM buddy manager + * @new_size: original size requested + * @blocks: output list head to add allocated blocks + * + * For contiguous allocation, we round up the size to the nearest + * power of two value, drivers consume *actual* size, so remaining + * portions are unused and it can be freed. + * + * Returns: + * 0 on success, error code on failure. + */ +int drm_buddy_block_trim(struct drm_buddy_mm *mm, +u64 new_size, +struct list_head *blocks) +{ + struct drm_buddy_block *block; + u64 new_start; + LIST_HEAD(dfs); + + if (!list_is_singular(blocks)) + return -EINVAL; + + block = list_first_entry(blocks, +struct drm_buddy_block, +link); + + if (!drm_buddy_block_is_allocated(block)) + return -EINVAL; + + if (new_size > drm_buddy_block_size(mm, block)) + return -EINVAL; + + if (!new_size && !IS_ALIGNED(new_size, mm->chunk_size)) + return -EINVAL; + + if (new_size == drm_buddy_block_size(mm, block)) + return 0; + + list_del(&block->link); + + new_start = drm_buddy_block_offset(block); + + mark_free(mm, block); + + list_add(&block->tmp_link, &dfs); + + return __alloc_range(mm, &dfs, new_start, new_size, blocks, 1); } +EXPORT_SYMBOL(drm_buddy_block_trim); /** * drm_buddy_alloc - allocate power-of-two blocks diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c index 7c58efb60dba..c5831c27fe82 100644 --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c @@ -97,6 +97,16 @@ static int i915_ttm_buddy_man_alloc(struct ttm_resource_manager *man, if (unlikely(err))
[PATCH v4 6/6] drm/amdgpu: add drm buddy support to amdgpu
- Remove drm_mm references and replace with drm buddy functionalities - Add res cursor support for drm buddy v2(Matthew Auld): - replace spinlock with mutex as we call kmem_cache_zalloc (..., GFP_KERNEL) in drm_buddy_alloc() function - lock drm_buddy_block_trim() function as it calls mark_free/mark_split are all globally visible v3: - remove drm_buddy_block_trim() error handling and print a warn message if it fails Signed-off-by: Arunpravin --- drivers/gpu/drm/Kconfig | 1 + .../gpu/drm/amd/amdgpu/amdgpu_res_cursor.h| 97 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 6 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 261 ++ 4 files changed, 232 insertions(+), 133 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 7a4a66d54782..dd880910282b 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -276,6 +276,7 @@ config DRM_AMDGPU select HWMON select BACKLIGHT_CLASS_DEVICE select INTERVAL_TREE + select DRM_BUDDY help Choose this option if you have a recent AMD Radeon graphics card. diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h index acfa207cf970..da12b4ff2e45 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h @@ -30,12 +30,15 @@ #include #include +#include "amdgpu_vram_mgr.h" + /* state back for walking over vram_mgr and gtt_mgr allocations */ struct amdgpu_res_cursor { uint64_tstart; uint64_tsize; uint64_tremaining; - struct drm_mm_node *node; + void*node; + uint32_tmem_type; }; /** @@ -52,27 +55,63 @@ static inline void amdgpu_res_first(struct ttm_resource *res, uint64_t start, uint64_t size, struct amdgpu_res_cursor *cur) { + struct drm_buddy_block *block; + struct list_head *head, *next; struct drm_mm_node *node; - if (!res || res->mem_type == TTM_PL_SYSTEM) { - cur->start = start; - cur->size = size; - cur->remaining = size; - cur->node = NULL; - WARN_ON(res && start + size > res->num_pages << PAGE_SHIFT); - return; - } + if (!res) + goto err_out; BUG_ON(start + size > res->num_pages << PAGE_SHIFT); - node = to_ttm_range_mgr_node(res)->mm_nodes; - while (start >= node->size << PAGE_SHIFT) - start -= node++->size << PAGE_SHIFT; + cur->mem_type = res->mem_type; + + switch (cur->mem_type) { + case TTM_PL_VRAM: + head = &to_amdgpu_vram_mgr_node(res)->blocks; + + block = list_first_entry_or_null(head, +struct drm_buddy_block, +link); + if (!block) + goto err_out; + + while (start >= amdgpu_node_size(block)) { + start -= amdgpu_node_size(block); + + next = block->link.next; + if (next != head) + block = list_entry(next, struct drm_buddy_block, link); + } + + cur->start = amdgpu_node_start(block) + start; + cur->size = min(amdgpu_node_size(block) - start, size); + cur->remaining = size; + cur->node = block; + break; + case TTM_PL_TT: + node = to_ttm_range_mgr_node(res)->mm_nodes; + while (start >= node->size << PAGE_SHIFT) + start -= node++->size << PAGE_SHIFT; + + cur->start = (node->start << PAGE_SHIFT) + start; + cur->size = min((node->size << PAGE_SHIFT) - start, size); + cur->remaining = size; + cur->node = node; + break; + default: + goto err_out; + } - cur->start = (node->start << PAGE_SHIFT) + start; - cur->size = min((node->size << PAGE_SHIFT) - start, size); + return; + +err_out: + cur->start = start; + cur->size = size; cur->remaining = size; - cur->node = node; + cur->node = NULL; + WARN_ON(res && start + size > res->num_pages << PAGE_SHIFT); + return; } /** @@ -85,7 +124,9 @@ static inline void amdgpu_res_first(struct ttm_resource *res, */ static inline void amdgpu_res_next(struct amdgpu_res_cursor *cur, uint64_t size) { - struct drm_mm_node *node = cur->node; + struct drm_buddy_block *block; + struct drm_mm_node *node; + struct list_head *next; BUG_ON(size > cur->remaining); @@ -99,9 +140
[PATCH v4 5/6] drm/amdgpu: move vram inline functions into a header
Move shared vram inline functions and structs into a header file Signed-off-by: Arunpravin --- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h | 51 1 file changed, 51 insertions(+) create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h new file mode 100644 index ..59983464cce5 --- /dev/null +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: MIT + * Copyright 2021 Advanced Micro Devices, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + */ + +#ifndef __AMDGPU_VRAM_MGR_H__ +#define __AMDGPU_VRAM_MGR_H__ + +#include + +struct amdgpu_vram_mgr_node { + struct ttm_resource base; + struct list_head blocks; + unsigned long flags; +}; + +static inline u64 amdgpu_node_start(struct drm_buddy_block *block) +{ + return drm_buddy_block_offset(block); +} + +static inline u64 amdgpu_node_size(struct drm_buddy_block *block) +{ + return PAGE_SIZE << drm_buddy_block_order(block); +} + +static inline struct amdgpu_vram_mgr_node * +to_amdgpu_vram_mgr_node(struct ttm_resource *res) +{ + return container_of(res, struct amdgpu_vram_mgr_node, base); +} + +#endif -- 2.25.1
Re: [PATCH v10 4/8] drm/ingenic: Add dw-hdmi driver for jz4780
Hi Mark, > Am 01.12.2021 um 16:10 schrieb Mark Brown : > > On Wed, Dec 01, 2021 at 03:33:24PM +0100, H. Nikolaus Schaller wrote: >>> Am 01.12.2021 um 15:03 schrieb Paul Cercueil : > >>> Please make it mandatory in DTS then, and use devm_regulator_get() in the >>> driver. > >> Well, I just wonder why the elegant devm_regulator_get_optional() exists at >> all >> and seems to be used in ca. 80 places. > > Frankly because half of them are broken usages like this since people > seem determined to have the most fragile error handling they can :/ I see. I had made the mistake myself to not check for NULL pointer on regulator_disable here... > There are valid use cases for it, with things like SD cards where some > supplies are genuinely optional and simply constrain what features are > available if they're omitted from the design. You also see some devices > with the ability to replace internal regulators with external ones. > >> And if it is not allowed, why some DTS should be forced to add not >> physically existing dummy-regulators. > > Again, if the supply can be physically absent that is a sensible use > case but that means completely absent, not just not software > controllable. We can represent fixed voltage regulators just fine. The question may be how we can know for a more generic driver that there is always a regulator. In the present case we know the schematics but it is just one example. > >> AFAIR drivers should implement functionality defined by DTS but not the >> other way round: enforce DTS style. >> BTW: there is no +5 mains dummy regulator defined in ci20.dts. > > It wouldn't be the first time a DTS were incomplete, and I'm sure it > won't be the last. > >> What I fear is that if we always have to define the mains +5V (which is for >> example not >> defined in ci20.dts), which rules stops us from asking to add a >> dummy-regulator from 110/230V to +5V as well. > > It is good practice to specify the full tree of supplies all the way to > the main supply rail of the board, this ensures that if we need the > information for something we've got it (even if that thing is just that > we've got to the root of the tree). There's potential applications in > battery supplied devices for managing very low power situations. Indeed. So let's modify it as you have suggested. BR and thanks, Nikolaus
Re: [PATCH v10 4/8] drm/ingenic: Add dw-hdmi driver for jz4780
On Wed, Dec 01, 2021 at 05:53:03PM +0100, H. Nikolaus Schaller wrote: > > Am 01.12.2021 um 16:10 schrieb Mark Brown : > > Again, if the supply can be physically absent that is a sensible use > > case but that means completely absent, not just not software > > controllable. We can represent fixed voltage regulators just fine. > The question may be how we can know for a more generic driver that there is > always a regulator. > In the present case we know the schematics but it is just one example. The datasheet will generally explicitly call out if a supply can be disconnected. In general it is astonishingly rare for this to be the case, supporting that case will tend to make designing the chip harder (you have to cope with what happens where the power domains meet) and results in whatever functionality the supplies power not working. If not otherwise specified it's safer to assume that the supplies must be connected. signature.asc Description: PGP signature
Re: [PATCH v2 2/3] drm/etnaviv: add pci device driver support
Hi Sui, Am Mittwoch, dem 01.12.2021 um 19:35 +0800 schrieb Sui Jingfeng: > From: suijingfeng > > v2: update commits and provide more material. > > There is a Vivante GC1000 V5037 in LS2K1000 and LS7A1000, > the gpu in those chip is a PCI device not platform one. > This GPU have 2D and 3D in the same core, so component > framework can be avoid, in fact we find it is diffcult to > use it with a pci device driver. > > Therefore, this patch try to provide PCI device driver wrapper > for it by mimic the platform counterpart. > > LS7A1000 is a bridge chip, brief user manual can be read on line > form[1]. This bridge chip typically use with LS3A4000 (4 core 1.8gHz, > Mips64r5) and LS3A5000 (4 core loongarch 2.5Ghz)[2]. > > While LS2K1000[3] is a double core 1.0Ghz Mips64r2 SoC. > > Loongson CPU's cache coherency is maintained by the hardware, > this is tramendous difference from other Mips or ARM, as the > cohercecy problems can be ignored when using cached coherent > BOs. > > On the other hand, current Mips kernel's writecombine support > is not being implemented correctly under the hardware maintained > cache coherency framework. At lease this is true for loongson's cpu. Can you please explain in some more detail what is the issue with writecombined memory on your platform? I'm asking because we use WC not just to get around missing hardware cache coherence on other platforms, but also for performance reasons. Graphics buffers can be quite huge and in a lot of cases it makes no sense to cache them on the CPU side an sacrifice other valuable cached data for it. WC allows us to efficiently write graphics buffers without polluting the cache. Is the coherency issue that you don't have a proper way to invalidate the cached alias? Do you hit speculatively loaded cache lines even if you access the writecombined alias? Regards, Lucas > > In other words, ETNA_BO_WC is NOT usable on our platform, it suffer > from cache coherency problem. Howerver use ETNA_BO_CACHED_COHERENT > instead of ETNA_BO_WC, etnaviv driver works well on our platform. > > Both LS7A1000 and LS2K1000 have a display controller integrated, > named lsdc. By using KMS-RO framework, lsdc and gc1000 made a > compatible pair. > > I have write xf86-video-loongson[4] DDX driver which make etnaviv works > with lsdc on our custom debian and fedora, glmark2-es2 and glxgears > can be hardware accelerated under X11 environment. (Mesa-18.3.6 and > Mesa-18.05 are tested and it works) > > See https://github.com/suijingfeng/xf86-video-loongson. > > [1] > https://loongson.github.io/LoongArch-Documentation/Loongson-7A1000-usermanual-EN.html > [2] > https://loongson.github.io/LoongArch-Documentation/Loongson-3A5000-usermanual-EN.html > [3] https://wiki.debian.org/InstallingDebianOn/Lemote/Loongson2K1000 > > Signed-off-by: suijingfeng > Signed-off-by: Sui Jingfeng <15330273...@189.cn> > --- > drivers/gpu/drm/etnaviv/Kconfig | 12 ++ > drivers/gpu/drm/etnaviv/Makefile | 2 + > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 113 --- > drivers/gpu/drm/etnaviv/etnaviv_drv.h | 8 + > drivers/gpu/drm/etnaviv/etnaviv_gem.c | 28 ++- > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 106 +++ > drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 6 + > drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c | 218 ++ > include/uapi/drm/etnaviv_drm.h| 11 +- > 9 files changed, 430 insertions(+), 74 deletions(-) > create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c > > diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig > index faa7fc68b009..5858eec59025 100644 > --- a/drivers/gpu/drm/etnaviv/Kconfig > +++ b/drivers/gpu/drm/etnaviv/Kconfig > @@ -15,6 +15,18 @@ config DRM_ETNAVIV > help > DRM driver for Vivante GPUs. > > +config DRM_ETNAVIV_PCI_DRIVER > + bool "Enable PCI device driver support" > + depends on DRM_ETNAVIV > + depends on PCI > + depends on MACH_LOONGSON64 > + default y > + help > + DRM PCI driver for the Vivante GPU in LS7A1000 north bridge > + and LS2K1000 SoC. The GC1000 in LS2K1000 and LS7A1000 is a > + PCI device. > + If in doubt, say "n". > + > config DRM_ETNAVIV_THERMAL > bool "enable ETNAVIV thermal throttling" > depends on DRM_ETNAVIV > diff --git a/drivers/gpu/drm/etnaviv/Makefile > b/drivers/gpu/drm/etnaviv/Makefile > index 46e5ffad69a6..6829e1ebf2db 100644 > --- a/drivers/gpu/drm/etnaviv/Makefile > +++ b/drivers/gpu/drm/etnaviv/Makefile > @@ -16,4 +16,6 @@ etnaviv-y := \ > etnaviv_perfmon.o \ > etnaviv_sched.o > > +etnaviv-$(CONFIG_DRM_ETNAVIV_PCI_DRIVER) += etnaviv_pci_drv.o > + > obj-$(CONFIG_DRM_ETNAVIV)+= etnaviv.o > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > index 7dcc6392792d..dc2bb3a6efe6 100644 > --- a/drivers/gpu/drm/etnaviv/etna
Re: [PATCH] fix a NULL pointer dereference in amdgpu_connector_lcd_native_mode()
On Tue, Nov 30, 2021 at 6:24 AM Zhou Qingyang wrote: > > In amdgpu_connector_lcd_native_mode(), the return value of > drm_mode_duplicate() is assigned to mode, and there is a dereference > of it in amdgpu_connector_lcd_native_mode(), which will lead to a NULL > pointer dereference on failure of drm_mode_duplicate(). > > Fix this bug add a check of mode. > > This bug was found by a static analyzer. The analysis employs > differential checking to identify inconsistent security operations > (e.g., checks or kfrees) between two code paths and confirms that the > inconsistent operations are not recovered in the current function or > the callers, so they constitute bugs. > > Note that, as a bug found by static analysis, it can be a false > positive or hard to trigger. Multiple researchers have cross-reviewed > the bug. > > Builds with CONFIG_DRM_AMDGPU=m show no new warnings, and > our static analyzer no longer warns about this code. > > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") > Signed-off-by: Zhou Qingyang > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > index 0de66f59adb8..0170aa84c5e6 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > @@ -387,6 +387,9 @@ amdgpu_connector_lcd_native_mode(struct drm_encoder > *encoder) > native_mode->vdisplay != 0 && > native_mode->clock != 0) { > mode = drm_mode_duplicate(dev, native_mode); > + if (!mode) > + return NULL; > + The else if clause needs a similar check. Care to fix that up as well? Alex > mode->type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER; > drm_mode_set_name(mode); > > -- > 2.25.1 >
Re: [PATCH] drm/radeon/radeon_connectors: Fix a NULL pointer dereference in radeon_fp_native_mode()
On Tue, Nov 30, 2021 at 9:49 AM Zhou Qingyang wrote: > > In radeon_fp_native_mode(), the return value of drm_mode_duplicate() is > assigned to mode and there is a dereference of it in > radeon_fp_native_mode(), which could lead to a NULL pointer > dereference on failure of drm_mode_duplicate(). > > Fix this bug by adding a check of mode. > > This bug was found by a static analyzer. The analysis employs > differential checking to identify inconsistent security operations > (e.g., checks or kfrees) between two code paths and confirms that the > inconsistent operations are not recovered in the current function or > the callers, so they constitute bugs. > > Note that, as a bug found by static analysis, it can be a false > positive or hard to trigger. Multiple researchers have cross-reviewed > the bug. > > Builds with CONFIG_DRM_RADEON=m show no new warnings, > and our static analyzer no longer warns about this code. > > Fixes: d2efdf6d6f42 ("drm/radeon/kms: add cvt mode if we only have lvds w/h > and no edid (v4)") > Signed-off-by: Zhou Qingyang > --- > drivers/gpu/drm/radeon/radeon_connectors.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c > b/drivers/gpu/drm/radeon/radeon_connectors.c > index 607ad5620bd9..49f187614f96 100644 > --- a/drivers/gpu/drm/radeon/radeon_connectors.c > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c > @@ -473,6 +473,9 @@ static struct drm_display_mode > *radeon_fp_native_mode(struct drm_encoder *encode > native_mode->vdisplay != 0 && > native_mode->clock != 0) { > mode = drm_mode_duplicate(dev, native_mode); > + if (!mode) > + return NULL; > + The else if clause needs a similar check. Care to fix that up as well? Alex > mode->type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER; > drm_mode_set_name(mode); > > -- > 2.25.1 >
[PATCH] drm/msm: Initialize MDSS irq domain at probe time
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the DSI host gets initialized earlier, but this caused unability to probe the entire stack of components because they all depend on interrupts coming from the main `mdss` node (mdp5, or dpu1). To fix this issue, move mdss device initialization (which include irq domain setup) to msm_mdev_probe() time, as to make sure that the interrupt controller is available before dsi and/or other components try to initialize, finally satisfying the dependency. Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order") Co-Developed-By: AngeloGioacchino Del Regno Signed-off-by: Dmitry Baryshkov --- When checking your patch, I noticed that IRQ domain is created before respective MDSS clocks are enabled. This does not look like causing any issues at this time, but it did not look good. So I started moving clocks parsing to early_init() callbacks. And at some point it looked like we can drop the init()/destroy() callbacks in favour of early_init() and remove(). Which promted me to move init()/destroy() in place of early_init()/remove() with few minor fixes here and there. --- drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 25 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 32 ++--- drivers/gpu/drm/msm/msm_drv.c | 56 --- drivers/gpu/drm/msm/msm_kms.h | 8 ++-- 4 files changed, 59 insertions(+), 62 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c index b466784d9822..131c1f1a869c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c @@ -111,7 +111,7 @@ static int _dpu_mdss_irq_domain_add(struct dpu_mdss *dpu_mdss) struct device *dev; struct irq_domain *domain; - dev = dpu_mdss->base.dev->dev; + dev = dpu_mdss->base.dev; domain = irq_domain_add_linear(dev->of_node, 32, &dpu_mdss_irqdomain_ops, dpu_mdss); @@ -184,16 +184,15 @@ static int dpu_mdss_disable(struct msm_mdss *mdss) return ret; } -static void dpu_mdss_destroy(struct drm_device *dev) +static void dpu_mdss_destroy(struct msm_mdss *mdss) { - struct platform_device *pdev = to_platform_device(dev->dev); - struct msm_drm_private *priv = dev->dev_private; - struct dpu_mdss *dpu_mdss = to_dpu_mdss(priv->mdss); + struct platform_device *pdev = to_platform_device(mdss->dev); + struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss); struct dss_module_power *mp = &dpu_mdss->mp; int irq; - pm_runtime_suspend(dev->dev); - pm_runtime_disable(dev->dev); + pm_runtime_suspend(mdss->dev); + pm_runtime_disable(mdss->dev); _dpu_mdss_irq_domain_fini(dpu_mdss); irq = platform_get_irq(pdev, 0); irq_set_chained_handler_and_data(irq, NULL, NULL); @@ -203,7 +202,6 @@ static void dpu_mdss_destroy(struct drm_device *dev) if (dpu_mdss->mmio) devm_iounmap(&pdev->dev, dpu_mdss->mmio); dpu_mdss->mmio = NULL; - priv->mdss = NULL; } static const struct msm_mdss_funcs mdss_funcs = { @@ -212,16 +210,15 @@ static const struct msm_mdss_funcs mdss_funcs = { .destroy = dpu_mdss_destroy, }; -int dpu_mdss_init(struct drm_device *dev) +int dpu_mdss_init(struct platform_device *pdev) { - struct platform_device *pdev = to_platform_device(dev->dev); - struct msm_drm_private *priv = dev->dev_private; + struct msm_drm_private *priv = platform_get_drvdata(pdev); struct dpu_mdss *dpu_mdss; struct dss_module_power *mp; int ret; int irq; - dpu_mdss = devm_kzalloc(dev->dev, sizeof(*dpu_mdss), GFP_KERNEL); + dpu_mdss = devm_kzalloc(&pdev->dev, sizeof(*dpu_mdss), GFP_KERNEL); if (!dpu_mdss) return -ENOMEM; @@ -238,7 +235,7 @@ int dpu_mdss_init(struct drm_device *dev) goto clk_parse_err; } - dpu_mdss->base.dev = dev; + dpu_mdss->base.dev = &pdev->dev; dpu_mdss->base.funcs = &mdss_funcs; ret = _dpu_mdss_irq_domain_add(dpu_mdss); @@ -256,7 +253,7 @@ int dpu_mdss_init(struct drm_device *dev) priv->mdss = &dpu_mdss->base; - pm_runtime_enable(dev->dev); + pm_runtime_enable(&pdev->dev); return 0; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c index c34760d981b8..b3f79c2277e9 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c @@ -112,7 +112,7 @@ static const struct irq_domain_ops mdss_hw_irqdomain_ops = { static int mdss_irq_domain_init(struct mdp5_mdss *mdp5_mdss) { - struct device *dev = mdp5_mdss->base.dev->dev; + struct device *dev = mdp5_mdss->base.dev; struct irq_domain *d; d = irq_domain_add_linear(dev->of_node, 32, &mdss_hw_irqdomain_ops, @@ -155,7 +155,7 @@ stat
Re: [PATCH v4 4/4] drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed
On 2021-11-24 23:32, Dmitry Baryshkov wrote: On 04/05/2021 07:35, Stephen Boyd wrote: Quoting Kuogee Hsieh (2021-04-21 16:37:38) Add checking aux read/write status at both dp_link_parse_sink_count() and dp_link_parse_sink_status_filed() to avoid long timeout delay if s/filed/field/ dp aux read/write failed at timeout due to cable unplugged. Changes in V4: -- split this patch as stand alone patch Signed-off-by: Kuogee Hsieh Can this patch come before the one previously? And then some fixes tag be added? Otherwise looks good to me. Reviewed-by: Stephen Boyd Tested-by: Stephen Boyd Is this something that we still need to pursue/merge? There were changes requested for this and for the previous patch, but no new versions were posted. It is my fault to miss this one. The first two patches of this serial are merged. I will rebase and re submit this one to V5.10.
[PATCH v2 0/2] staging: fbtft: add and use macro FBTFT_REGISTER_SPI_DRIVER
After 5fa6863ba692 ("spi: Check we have a spi_device_id for each DT compatible") we need to add spi id_tables. Changing existing macro FBTFT_REGISTER_DRIVER would have meant to change arguments and therefore adjust all fbtft drivers. This series adds a new and simplified macro FBTFT_REGISTER_SPI_DRIVER that includes a spi id_table, and in addition to that: - does not define a platform driver - uses macro module_spi_driver() Also the MODULE_ALIASes can be removed. Works for me with a SH1106-based OLED display incl. module autoload. For now I changed this driver only because I have hw to test it. v2: - consider that spi id_table name consists of device part of compatible string only - instead of changing the existing macro, add a new one and make fb_sh1106 the first user Heiner Kallweit (2): staging: fbtft: add macro FBTFT_REGISTER_SPI_DRIVER staging: fbtft: sh1106: use new macro FBTFT_REGISTER_SPI_DRIVER drivers/staging/fbtft/fb_sh1106.c | 7 +- drivers/staging/fbtft/fbtft.h | 41 +++ 2 files changed, 42 insertions(+), 6 deletions(-) -- 2.34.1
[PATCH v2 1/2] staging: fbtft: add macro FBTFT_REGISTER_SPI_DRIVER
After 5fa6863ba692 ("spi: Check we have a spi_device_id for each DT compatible") we need to add spi id_tables. Changing existing macro FBTFT_REGISTER_DRIVER would have meant to change arguments and therefore adjust all fbtft drivers. This patch adds a new and simplified macro FBTFT_REGISTER_SPI_DRIVER that includes a spi id_table, and in addition to that: - does not define a platform driver - uses macro module_spi_driver() Signed-off-by: Heiner Kallweit --- drivers/staging/fbtft/fbtft.h | 41 +++ 1 file changed, 41 insertions(+) diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h index 6869f3603..4cdec34e2 100644 --- a/drivers/staging/fbtft/fbtft.h +++ b/drivers/staging/fbtft/fbtft.h @@ -346,6 +346,47 @@ static void __exit fbtft_driver_module_exit(void) \ module_init(fbtft_driver_module_init); \ module_exit(fbtft_driver_module_exit); +#define FBTFT_REGISTER_SPI_DRIVER(_name, _comp_vend, _comp_dev, _display) \ + \ +static int fbtft_driver_probe_spi(struct spi_device *spi) \ +{ \ + return fbtft_probe_common(_display, spi, NULL); \ +} \ + \ +static int fbtft_driver_remove_spi(struct spi_device *spi) \ +{ \ + struct fb_info *info = spi_get_drvdata(spi); \ + \ + fbtft_remove_common(&spi->dev, info); \ + return 0; \ +} \ + \ +static const struct of_device_id dt_ids[] = { \ + { .compatible = _comp_vend "," _comp_dev }, \ + {}, \ +}; \ + \ +MODULE_DEVICE_TABLE(of, dt_ids); \ + \ +static const struct spi_device_id spi_ids[] = { \ + { .name = _comp_dev }, \ + {}, \ +}; \ + \ +MODULE_DEVICE_TABLE(spi, spi_ids); \ + \ +static struct spi_driver fbtft_driver_spi_driver = { \ + .driver = { \ + .name = _name, \ + .of_match_table = dt_ids, \ + }, \ + .id_table = spi_ids, \ + .probe = fbtft_driver_probe_spi, \ + .remove = fbtft_driver_remove_spi, \ +}; \ + \ +module_spi_driver(fbtft_driver_spi_driver); + /* Debug macros */ /* shorthand debug levels */ -- 2.34.1
[PATCH v2 2/2] staging: fbtft: sh1106: use new macro FBTFT_REGISTER_SPI_DRIVER
Make fb_sh1106 the first user of new macro FBTFT_REGISTER_SPI_DRIVER. In addition the MODULE_ALIASes can be removed. Module auto-loading was successfully tested with a SH1106-based OLED module connected to an Odroid C2. Signed-off-by: Heiner Kallweit --- drivers/staging/fbtft/fb_sh1106.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/staging/fbtft/fb_sh1106.c b/drivers/staging/fbtft/fb_sh1106.c index 7b9ab39e1..9685ca516 100644 --- a/drivers/staging/fbtft/fb_sh1106.c +++ b/drivers/staging/fbtft/fb_sh1106.c @@ -173,12 +173,7 @@ static struct fbtft_display display = { }, }; -FBTFT_REGISTER_DRIVER(DRVNAME, "sinowealth,sh1106", &display); - -MODULE_ALIAS("spi:" DRVNAME); -MODULE_ALIAS("platform:" DRVNAME); -MODULE_ALIAS("spi:sh1106"); -MODULE_ALIAS("platform:sh1106"); +FBTFT_REGISTER_SPI_DRIVER(DRVNAME, "sinowealth", "sh1106", &display); MODULE_DESCRIPTION("SH1106 OLED Driver"); MODULE_AUTHOR("Heiner Kallweit"); -- 2.34.1
Re: [PATCH] drm/komeda: Fix an undefined behavior bug in komeda_plane_add()
On Wed, Dec 01, 2021 at 03:44:03PM +, Steven Price wrote: > On 30/11/2021 14:25, Zhou Qingyang wrote: > > In komeda_plane_add(), komeda_get_layer_fourcc_list() is assigned to > > formats and used in drm_universal_plane_init(). > > drm_universal_plane_init() passes formats to > > __drm_universal_plane_init(). __drm_universal_plane_init() further > > passes formats to memcpy() as src parameter, which could lead to an > > undefined behavior bug on failure of komeda_get_layer_fourcc_list(). > > > > Fix this bug by adding a check of formats. > > > > This bug was found by a static analyzer. The analysis employs > > differential checking to identify inconsistent security operations > > (e.g., checks or kfrees) between two code paths and confirms that the > > inconsistent operations are not recovered in the current function or > > the callers, so they constitute bugs. > > > > Note that, as a bug found by static analysis, it can be a false > > positive or hard to trigger. Multiple researchers have cross-reviewed > > the bug. > > > > Builds with CONFIG_DRM_KOMEDA=m show no new warnings, > > and our static analyzer no longer warns about this code. > > > > Fixes: 61f1c4a8ab75 ("drm/komeda: Attach komeda_dev to DRM-KMS") > > Signed-off-by: Zhou Qingyang > > --- > > drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > > b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > > index d63d83800a8a..dd3f17e970dd 100644 > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c > > @@ -265,6 +265,10 @@ static int komeda_plane_add(struct komeda_kms_dev *kms, > > > > formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl, > >layer->layer_type, &n_formats); > > + if (!formats) { > > + err = -ENOMEM; > > + goto cleanup; > > + } > > If this executes it will cause undefined behaviour... > > The cleanup code calls komeda_plane_destroy() which calls > drm_plane_cleanup() which does (amongst other things) a > list_del(&plane->head). But the plane hasn't been put on a list yet as > that's done in drm_universal_plane_init(). > > So in this case we simple want to do: > > if (!formats) { > kfree(kplane); > return -ENOMEM; > } Zhou has already posted v2 that contains this fix. > > Note that without this 'fix' a NULL return from > komeda_get_layer_fourcc_list() would leave n_formats==0, so while the > NULL pointer is passed into memcpy() it is also passed a length of 0. > Which I believe is safe. > > However while looking at this function... > > > > > err = drm_universal_plane_init(&kms->base, plane, > > get_possible_crtcs(kms, c->pipeline), > > > > This call to drm_universal_plane_init() can fail early before > plane->head has been initialised. In which case the following: > > > komeda_put_fourcc_list(formats); > > > > if (err) > > goto cleanup; > > commits the exact same sin and would cause a similar NULL dereference in > drm_plane_cleanup(). I will come up with a patch for this case and post it to the list tomorrow. Best regards, Liviu > > Steve -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯
Re: [PATCH 1/6] dt-bindings: display: sitronix,st7735r: Fix backlight in example
On Wed, 24 Nov 2021 16:07:52 +0100, Noralf Trønnes wrote: > The backlight property was lost during conversion to yaml in commit > abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT schema"). > Put it back. > > Fixes: abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT > schema") > Signed-off-by: Noralf Trønnes > --- > Documentation/devicetree/bindings/display/sitronix,st7735r.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring
Re: [PATCH 2/6] dt-bindings: display: sitronix,st7735r: Make reset-gpios optional
On Wed, 24 Nov 2021 16:07:53 +0100, Noralf Trønnes wrote: > There are other ways than using a gpio to reset the controller so make > this property optional. > > Signed-off-by: Noralf Trønnes > --- > Documentation/devicetree/bindings/display/sitronix,st7735r.yaml | 1 - > 1 file changed, 1 deletion(-) > Acked-by: Rob Herring
Re: [PATCH 3/6] dt-bindings: display: sitronix,st7735r: Remove spi-max-frequency limit
On Wed, 24 Nov 2021 16:07:54 +0100, Noralf Trønnes wrote: > The datasheet lists the minimum Serial clock cycle (Write) as 66ns which is > 15MHz. Mostly it can do much better than that and is in fact often run at > 32MHz. With a clever driver that runs configuration commands at a low speed > and only the pixel data at the maximum speed the configuration can't be > messed up by transfer errors and the speed is only limited by the amount of > pixel glitches that one is able to tolerate. > > Signed-off-by: Noralf Trønnes > --- > .../devicetree/bindings/display/sitronix,st7735r.yaml | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > Acked-by: Rob Herring
[PATCH v3.1 2/2] drm: rcar-du: Add R-Car DSI driver
From: LUU HOAI The driver supports the MIPI DSI/CSI-2 TX encoder found in the R-Car V3U SoC. It currently supports DSI mode only. Signed-off-by: LUU HOAI Signed-off-by: Laurent Pinchart Reviewed-by: Kieran Bingham Tested-by: Kieran Bingham Acked-by: Sam Ravnborg --- Changes since v3: - Use bridge atomic ops Changes since v2: - Support probing of child DSI devices - Use devm_drm_of_get_bridge() helper Changes since v1: - Use U suffix for unsigned constants - Fix indentation in Makefile - Select DRM_MIPI_DSI - Report correct fout frequency in debug message - Move dsi_setup_info.err to local variable --- drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 820 +++ drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h | 172 4 files changed, 1000 insertions(+) create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig index 3e588ddba245..1675df21d91f 100644 --- a/drivers/gpu/drm/rcar-du/Kconfig +++ b/drivers/gpu/drm/rcar-du/Kconfig @@ -45,6 +45,13 @@ config DRM_RCAR_LVDS select OF_FLATTREE select OF_OVERLAY +config DRM_RCAR_MIPI_DSI + tristate "R-Car DU MIPI DSI Encoder Support" + depends on DRM && DRM_BRIDGE && OF + select DRM_MIPI_DSI + help + Enable support for the R-Car Display Unit embedded MIPI DSI encoders. + config DRM_RCAR_VSP bool "R-Car DU VSP Compositor Support" if ARM default y if ARM64 diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile index 4d1187ccc3e5..286bc81b3e7c 100644 --- a/drivers/gpu/drm/rcar-du/Makefile +++ b/drivers/gpu/drm/rcar-du/Makefile @@ -19,6 +19,7 @@ obj-$(CONFIG_DRM_RCAR_CMM)+= rcar_cmm.o obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o obj-$(CONFIG_DRM_RCAR_LVDS)+= rcar_lvds.o +obj-$(CONFIG_DRM_RCAR_MIPI_DSI)+= rcar_mipi_dsi.o # 'remote-endpoint' is fixed up at run-time DTC_FLAGS_rcar_du_of_lvds_r8a7790 += -Wno-graph_endpoint diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c new file mode 100644 index ..faf993eae564 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c @@ -0,0 +1,820 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * rcar_mipi_dsi.c -- R-Car MIPI DSI Encoder + * + * Copyright (C) 2020 Renesas Electronics Corporation + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +#include "rcar_mipi_dsi_regs.h" + +struct rcar_mipi_dsi { + struct device *dev; + const struct rcar_mipi_dsi_device_info *info; + struct reset_control *rstc; + + struct mipi_dsi_host host; + struct drm_bridge bridge; + struct drm_bridge *next_bridge; + struct drm_connector connector; + + void __iomem *mmio; + struct { + struct clk *mod; + struct clk *pll; + struct clk *dsi; + } clocks; + + enum mipi_dsi_pixel_format format; + unsigned int num_data_lanes; + unsigned int lanes; +}; + +static inline struct rcar_mipi_dsi * +bridge_to_rcar_mipi_dsi(struct drm_bridge *bridge) +{ + return container_of(bridge, struct rcar_mipi_dsi, bridge); +} + +static inline struct rcar_mipi_dsi * +host_to_rcar_mipi_dsi(struct mipi_dsi_host *host) +{ + return container_of(host, struct rcar_mipi_dsi, host); +} + +static const u32 phtw[] = { + 0x01020114, 0x01600115, /* General testing */ + 0x01030116, 0x0102011d, /* General testing */ + 0x011101a4, 0x018601a4, /* 1Gbps testing */ + 0x014201a0, 0x010001a3, /* 1Gbps testing */ + 0x0101011f, /* 1Gbps testing */ +}; + +static const u32 phtw2[] = { + 0x010c0130, 0x010c0140, /* General testing */ + 0x010c0150, 0x010c0180, /* General testing */ + 0x010c0190, + 0x010a0160, 0x010a0170, + 0x01800164, 0x01800174, /* 1Gbps testing */ +}; + +static const u32 hsfreqrange_table[][2] = { + { 8000U, 0x00 }, { 9000U, 0x10 }, { 1U, 0x20 }, + { 11000U, 0x30 }, { 12000U, 0x01 }, { 13000U, 0x11 }, + { 14000U, 0x21 }, { 15000U, 0x31 }, { 16000U, 0x02 }, + { 17000U, 0x12 }, { 18000U, 0x22 }, { 19000U, 0x32 }, + { 20500U, 0x03 }, { 22000U, 0x13 }, { 23500U, 0x23 }, + { 25000U, 0x33 }, { 27500U, 0x04 }, { 3U, 0x14 }, + { 32500U, 0x25 }, { 35000U, 0x35 }, { 4U, 0x05 }, + { 45000U, 0x16 }, { 5U, 0x26 }, { 55000U, 0x37 }, +
Re: [PATCH v3 2/2] drm: rcar-du: Add R-Car DSI driver
Hi Kieran, On Wed, Dec 01, 2021 at 10:40:07AM +, Kieran Bingham wrote: > Quoting Laurent Pinchart (2021-12-01 05:24:06) > > From: LUU HOAI > > > > The driver supports the MIPI DSI/CSI-2 TX encoder found in the R-Car V3U > > SoC. It currently supports DSI mode only. > > > > Signed-off-by: LUU HOAI > > Signed-off-by: Laurent Pinchart > > Reviewed-by: Kieran Bingham > > Tested-by: Kieran Bingham > > And as there have been some changes to the probe / bridge handling: > > (Re)Tested-by: Kieran Bingham I'm afraid I've posted a v3.1 that needs testing too :-) > > Acked-by: Sam Ravnborg > > --- > > Changes since v2: > > > > - Support probing of child DSI devices > > - Use devm_drm_of_get_bridge() helper > > > > Changes since v1: > > > > - Use U suffix for unsigned constants > > - Fix indentation in Makefile > > - Select DRM_MIPI_DSI > > - Report correct fout frequency in debug message > > - Move dsi_setup_info.err to local variable > > --- > > drivers/gpu/drm/rcar-du/Kconfig | 7 + > > drivers/gpu/drm/rcar-du/Makefile | 1 + > > drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 817 +++ > > drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h | 172 > > 4 files changed, 997 insertions(+) > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h > > > > diff --git a/drivers/gpu/drm/rcar-du/Kconfig > > b/drivers/gpu/drm/rcar-du/Kconfig > > index 3e588ddba245..1675df21d91f 100644 > > --- a/drivers/gpu/drm/rcar-du/Kconfig > > +++ b/drivers/gpu/drm/rcar-du/Kconfig > > @@ -45,6 +45,13 @@ config DRM_RCAR_LVDS > > select OF_FLATTREE > > select OF_OVERLAY > > > > +config DRM_RCAR_MIPI_DSI > > + tristate "R-Car DU MIPI DSI Encoder Support" > > + depends on DRM && DRM_BRIDGE && OF > > + select DRM_MIPI_DSI > > + help > > + Enable support for the R-Car Display Unit embedded MIPI DSI > > encoders. > > + > > config DRM_RCAR_VSP > > bool "R-Car DU VSP Compositor Support" if ARM > > default y if ARM64 > > diff --git a/drivers/gpu/drm/rcar-du/Makefile > > b/drivers/gpu/drm/rcar-du/Makefile > > index 4d1187ccc3e5..286bc81b3e7c 100644 > > --- a/drivers/gpu/drm/rcar-du/Makefile > > +++ b/drivers/gpu/drm/rcar-du/Makefile > > @@ -19,6 +19,7 @@ obj-$(CONFIG_DRM_RCAR_CMM)+= rcar_cmm.o > > obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o > > obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o > > obj-$(CONFIG_DRM_RCAR_LVDS)+= rcar_lvds.o > > +obj-$(CONFIG_DRM_RCAR_MIPI_DSI)+= rcar_mipi_dsi.o > > > > # 'remote-endpoint' is fixed up at run-time > > DTC_FLAGS_rcar_du_of_lvds_r8a7790 += -Wno-graph_endpoint > > diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > new file mode 100644 > > index ..fcaec3308d68 > > --- /dev/null > > +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > > @@ -0,0 +1,817 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * rcar_mipi_dsi.c -- R-Car MIPI DSI Encoder > > + * > > + * Copyright (C) 2020 Renesas Electronics Corporation > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "rcar_mipi_dsi_regs.h" > > + > > +struct rcar_mipi_dsi { > > + struct device *dev; > > + const struct rcar_mipi_dsi_device_info *info; > > + struct reset_control *rstc; > > + > > + struct mipi_dsi_host host; > > + struct drm_bridge bridge; > > + struct drm_bridge *next_bridge; > > + struct drm_connector connector; > > + > > + void __iomem *mmio; > > + struct { > > + struct clk *mod; > > + struct clk *pll; > > + struct clk *dsi; > > + } clocks; > > + > > + struct drm_display_mode display_mode; > > + enum mipi_dsi_pixel_format format; > > + unsigned int num_data_lanes; > > + unsigned int lanes; > > +}; > > + > > +static inline struct rcar_mipi_dsi * > > +bridge_to_rcar_mipi_dsi(struct drm_bridge *bridge) > > +{ > > + return container_of(bridge, struct rcar_mipi_dsi, bridge); > > +} > > + > > +static inline struct rcar_mipi_dsi * > > +host_to_rcar_mipi_dsi(struct mipi_dsi_host *host) > > +{ > > + return container_of(host, struct rcar_mipi_dsi, host); > > +} > > + > > +static const u32 phtw[] = { > > + 0x01020114, 0x01600115, /* General testing */ > > + 0x01030116, 0x0102011d, /* General testing */ > > + 0x011101a4, 0x018601a4, /* 1Gbps testing */ > > + 0x014201a0, 0x010001a3, /* 1Gbps testing */ > > + 0x0101011f, /* 1Gbps testing */ > > +}; > > + > > +static const u32 phtw2[] = { > > +
Re: [PATCH 4/6] dt-bindings: display: sitronix,st7735r: Add initialization properties
On Wed, Nov 24, 2021 at 04:07:55PM +0100, Noralf Trønnes wrote: > Add initialization properties that are commonly used to initialize the > controller for a specific display panel. It is common for displays to have > a datasheet listing the necessary controller settings or some example code > doing the same. These settings can be matched directly to the DT > properties. > > The commands FRMCTR2, FRMCTR3, PWCTR4 and PWCTR5 are usually part of the > setup examples but they are skipped here since they deal with partial and > idle mode which are powersaving modes for very special use cases. > > dc-gpios is made optional because its absence indicates 3-line mode. > > Signed-off-by: Noralf Trønnes > --- > .../bindings/display/sitronix,st7735r.yaml| 118 +- > 1 file changed, 116 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml > b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml > index 157b1a7b18f9..2db1cfe6ae30 100644 > --- a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml > +++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml > @@ -19,6 +19,10 @@ allOf: > properties: >compatible: > oneOf: > + - description: > + Sitronix ST7735R 262K Color Single-Chip TFT Controller/Driver > +items: > + - const: sitronix,st7735r >- description: >Adafruit 1.8" 160x128 Color TFT LCD (Product ID 358 or 618) > items: > @@ -32,20 +36,99 @@ properties: >- okaya,rh128128t >- const: sitronix,st7715r > > + width: > +description: > + Width of display panel in pixels > + > + height: > +description: > + Height of display panel in pixels We already have width-mm and height-mm for physical size so this might be a bit confusing. There's also panel-timing 'vactive' and 'hactive' which is effectively the same thing you are defining. > + > + frmctr1: Are all these standardized by MIPI or otherwise common? If not, they need vendor prefixes. > +$ref: /schemas/types.yaml#definitions/uint8-array > +description: > + Frame Rate Control (In normal mode/Full colors) (B1h) > +minItems: 3 > +maxItems: 3 > + > + invctr: > +$ref: /schemas/types.yaml#definitions/uint8-array > +description: > + Display Inversion Control (B4h) > +minItems: 1 > +maxItems: 1 > + > + pwctr1: > +$ref: /schemas/types.yaml#definitions/uint8-array > +description: > + Power Control 1 (C0h) > +minItems: 3 > +maxItems: 3 > + > + pwctr2: > +$ref: /schemas/types.yaml#definitions/uint8-array > +description: > + Power Control 2 (C1h) > +minItems: 1 > +maxItems: 1 > + > + pwctr3: > +$ref: /schemas/types.yaml#definitions/uint8-array > +description: > + Power Control 3 (in Normal mode/Full colors) (C2h) > +minItems: 2 > +maxItems: 2 > + > + vmctr1: > +$ref: /schemas/types.yaml#definitions/uint8-array > +description: > + VCOM Control 1 (C5h) > +minItems: 1 > +maxItems: 1 > + > + madctl: > +$ref: /schemas/types.yaml#definitions/uint8-array > +description: > + Memory Data Access Control (36h) > +minItems: 1 > +maxItems: 1 > + > + gamctrp1: > +$ref: /schemas/types.yaml#definitions/uint8-array > +description: > + Gamma Positive Polarity Correction Characteristics Setting (E0h) > +minItems: 16 > +maxItems: 16 > + > + gamctrn1: > +$ref: /schemas/types.yaml#definitions/uint8-array > +description: > + Gamma Negative Polarity Correction Characteristics Setting (E1h) > +minItems: 16 > +maxItems: 16 > + > + write-only: > +type: boolean > +description: > + Controller is not readable (ie. MISO is not wired up). > + >dc-gpios: > maxItems: 1 > -description: Display data/command selection (D/CX) > +description: | > + Controller data/command selection (D/CX) in 4-line SPI mode. > + If not set, the controller is in 3-line SPI mode. > >backlight: true >reg: true >spi-max-frequency: true >reset-gpios: true >rotation: true > + width-mm: true > + height-mm: true > > required: >- compatible >- reg > - - dc-gpios > > additionalProperties: false > > @@ -72,5 +155,36 @@ examples: > backlight = <&backlight>; > }; > }; > + - | > +#include > + > +spi { > +#address-cells = <1>; > +#size-cells = <0>; > + > +sainsmart18@0{ > +compatible = "sitronix,st7735r"; > +reg = <0>; > +spi-max-frequency = <4000>; > + > +width = <160>; > +height = <128>; > + > +frmctr1 = [ 01 2C 2D ]; > +invctr = [ 07 ]; > +pwctr1 = [ A2 02 84 ]; > +pwctr2
[PATCH v1 1/8] drm/msm/dpu: move disable_danger out of plane subdir
The disable_danger debugfs file is not related to a single plane. Instead it is used by all registered planes. Move it from plane subtree to the global subtree next to danger_status and safe_status files, so that the new file supplements them. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 70 + drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 74 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 6 ++ 3 files changed, 77 insertions(+), 73 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 6c457c419412..259d438bc6e8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -101,6 +101,73 @@ static int dpu_debugfs_safe_stats_show(struct seq_file *s, void *v) } DEFINE_SHOW_ATTRIBUTE(dpu_debugfs_safe_stats); +static ssize_t _dpu_plane_danger_read(struct file *file, + char __user *buff, size_t count, loff_t *ppos) +{ + struct dpu_kms *kms = file->private_data; + int len; + char buf[40]; + + len = scnprintf(buf, sizeof(buf), "%d\n", !kms->has_danger_ctrl); + + return simple_read_from_buffer(buff, count, ppos, buf, len); +} + +static void _dpu_plane_set_danger_state(struct dpu_kms *kms, bool enable) +{ + struct drm_plane *plane; + + drm_for_each_plane(plane, kms->dev) { + if (plane->fb && plane->state) { + dpu_plane_danger_signal_ctrl(plane, enable); + DPU_DEBUG("plane:%d img:%dx%d ", + plane->base.id, plane->fb->width, + plane->fb->height); + DPU_DEBUG("src[%d,%d,%d,%d] dst[%d,%d,%d,%d]\n", + plane->state->src_x >> 16, + plane->state->src_y >> 16, + plane->state->src_w >> 16, + plane->state->src_h >> 16, + plane->state->crtc_x, plane->state->crtc_y, + plane->state->crtc_w, plane->state->crtc_h); + } else { + DPU_DEBUG("Inactive plane:%d\n", plane->base.id); + } + } +} + +static ssize_t _dpu_plane_danger_write(struct file *file, + const char __user *user_buf, size_t count, loff_t *ppos) +{ + struct dpu_kms *kms = file->private_data; + int disable_panic; + int ret; + + ret = kstrtouint_from_user(user_buf, count, 0, &disable_panic); + if (ret) + return ret; + + if (disable_panic) { + /* Disable panic signal for all active pipes */ + DPU_DEBUG("Disabling danger:\n"); + _dpu_plane_set_danger_state(kms, false); + kms->has_danger_ctrl = false; + } else { + /* Enable panic signal for all active pipes */ + DPU_DEBUG("Enabling danger:\n"); + kms->has_danger_ctrl = true; + _dpu_plane_set_danger_state(kms, true); + } + + return count; +} + +static const struct file_operations dpu_plane_danger_enable = { + .open = simple_open, + .read = _dpu_plane_danger_read, + .write = _dpu_plane_danger_write, +}; + static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms, struct dentry *parent) { @@ -110,6 +177,9 @@ static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms, dpu_kms, &dpu_debugfs_danger_stats_fops); debugfs_create_file("safe_status", 0600, entry, dpu_kms, &dpu_debugfs_safe_stats_fops); + debugfs_create_file("disable_danger", 0600, entry, + dpu_kms, &dpu_plane_danger_enable); + } static int _dpu_debugfs_show_regset32(struct seq_file *s, void *data) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index ca190d92f0d5..6ea4db061c9f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -1350,7 +1350,7 @@ static void dpu_plane_reset(struct drm_plane *plane) } #ifdef CONFIG_DEBUG_FS -static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable) +void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable) { struct dpu_plane *pdpu = to_dpu_plane(plane); struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); @@ -1363,73 +1363,6 @@ static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable) pm_runtime_put_sync(&dpu_kms->pdev->dev); } -static ssize_t _dpu_plane_danger_read(struct file *file, - char __user *buff, size_t count, loff_t *ppos) -{ - struct dpu_kms *kms = file->private_data; - int len; - char buf[40]; - - len = scnprintf(buf, sizeof(buf), "%d\n", !kms->has_danger_ctr
[PATCH v1 0/8] drm/msm/dpu: cleanup debugfs code
This patchset provides a partial rework/cleanup/fixup of debugfs code in DPU driver. It started as a single patch removing (and then just moving) SSPP debugfs code from the plane (as planes are going to be less connected with SSPP blocks soon). However the more I touched this code, the more patches were generated as more and more issues arrive on the surface. The following changes since commit fee32807633395e666f0951d6b7b6546e9b76c3d: mailmap: add and update email addresses (2021-11-29 16:19:58 -0800) are available in the Git repository at: https://git.linaro.org/people/dmitry.baryshkov/kernel.git dpu-cleanup-debugfs for you to fetch changes up to 7f3598ee9ea525920cd6fa65b498604a9ff8b36a: drm/msm/dpu: move SSPP debugfs support from plane to SSPP code (2021-12-02 01:03:49 +0300) Dmitry Baryshkov (8): drm/msm/dpu: move disable_danger out of plane subdir drm/msm/dpu: fix safe status debugfs file drm/msm/dpu: make danger_status/safe_status readable drm/msm/dpu: drop plane's default_scaling debugfs file drm/msm/dpu: stop manually removing debugfs files for the DPU plane drm/msm/dpu: stop manually removing debugfs files for the DPU CRTC drm/msm/dpu: simplify DPU's regset32 code drm/msm/dpu: move SSPP debugfs support from plane to SSPP code drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 15 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 3 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 4 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 109 +++--- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 38 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 172 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 6 + 8 files changed, 189 insertions(+), 225 deletions(-)
[PATCH v1 2/8] drm/msm/dpu: fix safe status debugfs file
Make safe_status debugfs fs file actually return safe status rather than danger status data. Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 259d438bc6e8..e7f0cded2c6b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -73,8 +73,8 @@ static int _dpu_danger_signal_status(struct seq_file *s, &status); } else { seq_puts(s, "\nSafe signal status:\n"); - if (kms->hw_mdp->ops.get_danger_status) - kms->hw_mdp->ops.get_danger_status(kms->hw_mdp, + if (kms->hw_mdp->ops.get_safe_status) + kms->hw_mdp->ops.get_safe_status(kms->hw_mdp, &status); } pm_runtime_put_sync(&kms->pdev->dev); -- 2.33.0
[PATCH v1 3/8] drm/msm/dpu: make danger_status/safe_status readable
Change \t to \n in the print format to stop putting all SSPP status in a single line. Splitting it to one SSPP per line is much more readable. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index e7f0cded2c6b..4c04982c71b2 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -82,7 +82,7 @@ static int _dpu_danger_signal_status(struct seq_file *s, seq_printf(s, "MDP : 0x%x\n", status.mdp); for (i = SSPP_VIG0; i < SSPP_MAX; i++) - seq_printf(s, "SSPP%d : 0x%x \t", i - SSPP_VIG0, + seq_printf(s, "SSPP%d : 0x%x \n", i - SSPP_VIG0, status.sspp[i]); seq_puts(s, "\n"); -- 2.33.0
[PATCH v1 4/8] drm/msm/dpu: drop plane's default_scaling debugfs file
Proper support for the 'default_scaling' debugfs file was removed during DPU driver pre-merge cleanup. Remove leftover file. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 5 - 1 file changed, 5 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 6ea4db061c9f..f80ee3ba9a8a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -114,7 +114,6 @@ struct dpu_plane { struct dpu_debugfs_regset32 debugfs_src; struct dpu_debugfs_regset32 debugfs_scaler; struct dpu_debugfs_regset32 debugfs_csc; - bool debugfs_default_scale; }; static const uint64_t supported_format_modifiers[] = { @@ -1398,10 +1397,6 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane) dpu_debugfs_create_regset32("scaler_blk", 0400, pdpu->debugfs_root, &pdpu->debugfs_scaler); - debugfs_create_bool("default_scaling", - 0600, - pdpu->debugfs_root, - &pdpu->debugfs_default_scale); } if (cfg->features & BIT(DPU_SSPP_CSC) || -- 2.33.0