Re: [PATCH 1/2] drm/i915/buddy: add some pretty printing
On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote: > Implement the debug hook for the buddy resource manager. For this we > want to print out the status of the memory manager, including how much > memory is still allocatable, what page sizes we have etc. This will be > triggered when TTM is unable to fulfil an allocation request for device > local-memory. > > Signed-off-by: Matthew Auld > Cc: Thomas Hellström > --- > drivers/gpu/drm/i915/i915_buddy.c | 45 +++ > drivers/gpu/drm/i915/i915_buddy.h | 8 > drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 20 - > 3 files changed, 72 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_buddy.c > b/drivers/gpu/drm/i915/i915_buddy.c > index 7b274c51cac0..240e881d9eb0 100644 > --- a/drivers/gpu/drm/i915/i915_buddy.c > +++ b/drivers/gpu/drm/i915/i915_buddy.c > @@ -4,6 +4,7 @@ > */ > > #include > +#include > > #include "i915_buddy.h" > > @@ -82,6 +83,7 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 > size, u64 chunk_size) > size = round_down(size, chunk_size); > > mm->size = size; > + mm->avail = size; > mm->chunk_size = chunk_size; > mm->max_order = ilog2(size) - ilog2(chunk_size); > > @@ -155,6 +157,8 @@ void i915_buddy_fini(struct i915_buddy_mm *mm) > i915_block_free(mm, mm->roots[i]); > } > > + GEM_WARN_ON(mm->avail != mm->size); > + > kfree(mm->roots); > kfree(mm->free_list); > } > @@ -230,6 +234,7 @@ void i915_buddy_free(struct i915_buddy_mm *mm, > struct i915_buddy_block *block) > { > GEM_BUG_ON(!i915_buddy_block_is_allocated(block)); > + mm->avail += i915_buddy_block_size(mm, block); > __i915_buddy_free(mm, block); > } > > @@ -283,6 +288,7 @@ i915_buddy_alloc(struct i915_buddy_mm *mm, unsigned > int order) > } > > mark_allocated(block); > + mm->avail -= i915_buddy_block_size(mm, block); > kmemleak_update_trace(block); > return block; > > @@ -368,6 +374,7 @@ int i915_buddy_alloc_range(struct i915_buddy_mm > *mm, > } > > mark_allocated(block); > + mm->avail -= i915_buddy_block_size(mm, block); > list_add_tail(&block->link, &allocated); > continue; > } > @@ -402,6 +409,44 @@ int i915_buddy_alloc_range(struct i915_buddy_mm > *mm, > return err; > } > > +void i915_buddy_block_print(struct i915_buddy_mm *mm, > + struct i915_buddy_block *block, > + struct drm_printer *p) > +{ > + u64 start = i915_buddy_block_offset(block); > + u64 size = i915_buddy_block_size(mm, block); > + > + drm_printf(p, "%#018llx-%#018llx: %llu\n", start, start + size, > size); > +} > + > +void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer *p) > +{ > + int order; > + > + drm_printf(p, "chunk_size: %lluKB, total: %lluMB, free: > %lluMB\n", > + mm->chunk_size >> 10, mm->size >> 20, mm->avail >> > 20); > + > + for (order = mm->max_order; order >= 0; order--) { > + struct i915_buddy_block *block; > + u64 count = 0, free; > + > + list_for_each_entry(block, &mm->free_list[order], link) > { > + GEM_BUG_ON(!i915_buddy_block_is_free(block)); > + count++; > + } > + > + drm_printf(p, "order-%d ", order); > + > + free = count * (mm->chunk_size << order); > + if (free < SZ_1M) > + drm_printf(p, "free: %lluKB", free >> 10); > + else > + drm_printf(p, "free: %lluMB", free >> 20); Use KiB and MiB instead of KB and MB? Also below. > + > + drm_printf(p, ", pages: %llu\n", count); > + } > +} > + > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > #include "selftests/i915_buddy.c" > #endif > diff --git a/drivers/gpu/drm/i915/i915_buddy.h > b/drivers/gpu/drm/i915/i915_buddy.h > index 3940d632f208..7077742112ac 100644 > --- a/drivers/gpu/drm/i915/i915_buddy.h > +++ b/drivers/gpu/drm/i915/i915_buddy.h > @@ -10,6 +10,8 @@ > #include > #include > > +#include > + > struct i915_buddy_block { > #define I915_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12) > #define I915_BUDDY_HEADER_STATE GENMASK_ULL(11, 10) > @@ -69,6 +71,7 @@ struct i915_buddy_mm { > /* Must be at least PAGE_SIZE */ > u64 chunk_size; > u64 size; > + u64 avail; > }; > > static inline u64 > @@ -129,6 +132,11 @@ void i915_buddy_free(struct i915_buddy_mm *mm, > struct i915_buddy_block *block); > > void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head > *objects); > > +void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer > *p); > +void i9
Re: [PATCH] drm/i915/ttm: ensure we release the intel_memory_region
On Wed, 2021-08-18 at 18:12 +0100, Matthew Auld wrote: > If the ttm_bo_init_reserved() call fails ensure we also release the > region, otherwise we leak the reference, or worse hit some uaf, when > we > start using the objects.list. Also remove the make_unshrinkable call > here, which doesn't do anything. > > Signed-off-by: Matthew Auld > Cc: Thomas Hellström > --- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 771eb2963123..2e8cdcd5e4f7 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -909,7 +909,6 @@ int __i915_gem_ttm_object_init(struct > intel_memory_region *mem, > drm_gem_private_object_init(&i915->drm, &obj->base, size); > i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, > flags); > i915_gem_object_init_memory_region(obj, mem); > - i915_gem_object_make_unshrinkable(obj); > INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL | > __GFP_NOWARN); > mutex_init(&obj->ttm.get_io_page.lock); > bo_type = (obj->flags & I915_BO_ALLOC_USER) ? > ttm_bo_type_device : > @@ -932,7 +931,7 @@ int __i915_gem_ttm_object_init(struct > intel_memory_region *mem, > page_size >> PAGE_SHIFT, > &ctx, NULL, NULL, > i915_ttm_bo_destroy); > if (ret) > - return i915_ttm_err_to_gem(ret); > + goto err_release_mr; IIRC when ttm_object_init_reserved fails, it will call ttm_bo_put() which will eventually end up in i915_ttm_bo_destroy() which will do the right thing? /Thomas
Re: [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug
On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote: > This should give a more complete view of the various bits of internal > resource manager state, for device local-memory. > > Signed-off-by: Matthew Auld > Cc: Thomas Hellström > --- > drivers/gpu/drm/i915/i915_debugfs.c | 12 +--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index eec0d349ea6a..109e6feed6be 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m, > struct drm_i915_gem_object *obj) > static int i915_gem_object_info(struct seq_file *m, void *data) > { > struct drm_i915_private *i915 = node_to_i915(m->private); > + struct drm_printer p = drm_seq_file_printer(m); > struct intel_memory_region *mr; > enum intel_region_id id; > > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct seq_file > *m, void *data) > i915->mm.shrink_count, > atomic_read(&i915->mm.free_count), > i915->mm.shrink_memory); > - for_each_memory_region(mr, i915, id) > - seq_printf(m, "%s: total:%pa, available:%pa bytes\n", > - mr->name, &mr->total, &mr->avail); > + for_each_memory_region(mr, i915, id) { > + seq_printf(m, "%s: ", mr->name); > + if (mr->region_private) > + ttm_resource_manager_debug(mr- > >region_private, &p); > + else > + seq_printf(m, "total:%pa, available:%pa > bytes\n", > + &mr->total, &mr->avail); Hm. Shouldn't we make the above intel_memory_region_debug() or perhaps intel_memory_region_info() to avoid using memory region internals directly here? /Thomas
[PATCH] drm/panel:Adjust sync values for Feixin K101-IM2BYL02 panel
This adjusts sync values according to the datasheet Fixes: 1c243751c095bb95e2795f076ea7a0bcdd60a93a ("drm/panel: ilitek-ili9881c: add support for Feixin K101-IM2BYL02 panel") Co-developed-by: Marius Gripsgard Signed-off-by: Dan Johansen --- drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c index 0145129d7c66..534dd7414d42 100644 --- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c @@ -590,14 +590,14 @@ static const struct drm_display_mode k101_im2byl02_default_mode = { .clock = 69700, .hdisplay = 800, - .hsync_start= 800 + 6, - .hsync_end = 800 + 6 + 15, - .htotal = 800 + 6 + 15 + 16, + .hsync_start= 800 + 52, + .hsync_end = 800 + 52 + 8, + .htotal = 800 + 52 + 8 + 48, .vdisplay = 1280, - .vsync_start= 1280 + 8, - .vsync_end = 1280 + 8 + 48, - .vtotal = 1280 + 8 + 48 + 52, + .vsync_start= 1280 + 16, + .vsync_end = 1280 + 16 + 6, + .vtotal = 1280 + 16 + 6 + 15, .width_mm = 135, .height_mm = 217, -- 2.32.0
Re: [PATCH v5, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel, Thanks for your suggestion. On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote: > +danvet > > Hi, > > On Tue, 10 Aug 2021 at 23:58, Yunfei Dong > wrote: > > > > This series adds support for multi hardware decode into mtk-vcodec, > > by first > > adding component framework to manage each hardware information: > > interrupt, > > clock, register bases and power. Secondly add core thread to deal > > with core > > hardware message, at the same time, add msg queue for different > > hardware > > share messages. Lastly, the architecture of different specs are not > > the same, > > using specs type to separate them. > > > > I don't think it's a good idea to introduce the component API in the > media subsystem. It doesn't seem to be maintained, IRC there's not > even > a maintainer for it, and it has some issues that were never > addressed. > > It would be really important to avoid it. Is it really needed in the > first place? > > Thanks, > Ezequiel For there are many hardware need to use, mt8192 is three and mt8195 is five. Maybe need more to be used in the feature. Each hardware has independent clk/power/iommu port/irq. Use component interface in prob to get each component's information. Just enable the hardware when need to use it, very convenient and simple. I found that there are many modules use component to manage hardware information, such as iommu and drm etc. Do you have any other suggestion for this architecture? Thanks Yunfei Dong
Re: [PATCH v2 27/63] fortify: Move remaining fortify helpers into fortify-string.h
Hi. Le mercredi 18 août 2021, 08:04:57 CEST Kees Cook a écrit : > When commit a28a6e860c6c ("string.h: move fortified functions definitions > in a dedicated header.") moved the fortify-specific code, some helpers > were left behind. Moves the remaining fortify-specific helpers into > fortify-string.h so they're together where they're used. This requires > that any FORTIFY helper function prototypes be conditionally built to > avoid "no prototype" warnings. Additionally removes unused helpers. > > Cc: Andrew Morton > Cc: Francis Laniel > Cc: Daniel Axtens > Cc: Vincenzo Frascino > Cc: Andrey Konovalov > Cc: Dan Williams > Signed-off-by: Kees Cook > --- > include/linux/fortify-string.h | 7 +++ > include/linux/string.h | 9 - > lib/string_helpers.c | 2 ++ > 3 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h > index c1be37437e77..7e67d02764db 100644 > --- a/include/linux/fortify-string.h > +++ b/include/linux/fortify-string.h > @@ -2,6 +2,13 @@ > #ifndef _LINUX_FORTIFY_STRING_H_ > #define _LINUX_FORTIFY_STRING_H_ > > +#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline)) > +#define __RENAME(x) __asm__(#x) > + > +void fortify_panic(const char *name) __noreturn __cold; > +void __read_overflow(void) __compiletime_error("detected read beyond size > of object (1st parameter)"); +void __read_overflow2(void) > __compiletime_error("detected read beyond size of object (2nd parameter)"); > +void __write_overflow(void) __compiletime_error("detected write beyond > size of object (1st parameter)"); > > #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS) > extern void *__underlying_memchr(const void *p, int c, __kernel_size_t > size) __RENAME(memchr); diff --git a/include/linux/string.h > b/include/linux/string.h > index b48d2d28e0b1..9473f81b9db2 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -249,15 +249,6 @@ static inline const char *kbasename(const char *path) > return tail ? tail + 1 : path; > } > > -#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline)) > -#define __RENAME(x) __asm__(#x) > - > -void fortify_panic(const char *name) __noreturn __cold; > -void __read_overflow(void) __compiletime_error("detected read beyond size > of object passed as 1st parameter"); -void __read_overflow2(void) > __compiletime_error("detected read beyond size of object passed as 2nd > parameter"); -void __read_overflow3(void) __compiletime_error("detected > read beyond size of object passed as 3rd parameter"); -void > __write_overflow(void) __compiletime_error("detected write beyond size of > object passed as 1st parameter"); - > #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && > defined(CONFIG_FORTIFY_SOURCE) #include > #endif > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index bde13612c25d..faa9d8e4e2c5 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -883,9 +883,11 @@ char *strreplace(char *s, char old, char new) > } > EXPORT_SYMBOL(strreplace); > > +#ifdef CONFIG_FORTIFY_SOURCE > void fortify_panic(const char *name) > { > pr_emerg("detected buffer overflow in %s\n", name); > BUG(); > } > EXPORT_SYMBOL(fortify_panic); > +#endif /* CONFIG_FORTIFY_SOURCE */ If I remember correctly, I let these helpers in string.h because I thought they could be used by code not related to fortify-string.h. But you are right and I think it is better to have all the code related to one feature in the same place. I am happy to see the kernel is fortifying, and this contribution is good, so here is what I can give: Acked-by: Francis Laniel Best regards.
RE: [PATCH v2] drm/dp: follow DP link CTS spec to read link status back
On Wed, 08 Jul 2021, Lee Shawn C wrote: >On Wed, 07 Jul 2021, 4:14 p.m, Jani Nikula wrote: >>On Wed, 07 Jul 2021, Lee Shawn C wrote: >>> Refer to DP link CTS 1.2/1.4 spec, the following test case request >>> source read DPCD 200h - 205h to get latest link status from sink. >>> >>> (4.3.2.4) Handling of IRQ HPD Pulse with No Error Status Bits Set >>> (400.3.2.1) Successful Link Re-training After IRQ HPD Pulse >>> Due to Loss of Symbol Lock: HBR2 Extension >>> (400.3.2.2) Successful Link Re-training After IRQ HPD Pulse Due >>> to Loss of Clock Recovery Lock: HBR2 Extension >>> (400.3.2.3) Successful Link Re-training After IRQ HPD Pulse Due >>> to Loss of Inter-lane Alignment Lock: HBR2 Extension >>> >>> So far, DRM DP driver just read back the link status from 202h to >>> 207h. DPR-120 would judge source can't pass these cases and shows >>> below error messages. >>> >>> "Test FAILED, Source DUT does not read DPCD registers 200h-205h >>> within >>> 100 ms". >> >>Acked-by: Jani Nikula >> >>for making the test pass iff everything else seems to work. >> >>The underlying question is, though, should we look at 0x200-0x201 for some >>status we don't look at? >> > >Look into 200h. While doing link train with DPRX. In my opinion, sink_count >and cp_ready status should be constant. >And sink would trigger HPD to source to notify 201h value was changed. Seems >source driver don't need this value at link training stage as well. What do >you think? > >Best regards, >Shawn > Hi Simon and all, Please share your recommendations for this patch to pass DP link layer compliance test. Thanks! Best regards, Shawn >> >>> >>> v2: Use sizeof() to retrieve array size. >>> >>> Cc: Jani Nikula >>> Cc: Ville Syrjälä >>> Cc: Lyude Paul >>> Cc: Simon Ser >>> Cc: Cooper Chiou >>> Cc: William Tseng >>> Signed-off-by: Lee Shawn C >>> --- >>> drivers/gpu/drm/drm_dp_helper.c | 10 ++ >>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_dp_helper.c >>> b/drivers/gpu/drm/drm_dp_helper.c index 24bbc710c825..4f03df317d62 >>> 100644 >>> --- a/drivers/gpu/drm/drm_dp_helper.c >>> +++ b/drivers/gpu/drm/drm_dp_helper.c >>> @@ -410,17 +410,19 @@ int drm_dp_dpcd_read_phy_link_status(struct >>> drm_dp_aux *aux, >>> u8 link_status[DP_LINK_STATUS_SIZE]) { >>> int ret; >>> + u8 full_link_stat[DP_LINK_STATUS_SIZE + 2]; >>> >>> if (dp_phy == DP_PHY_DPRX) { >>> ret = drm_dp_dpcd_read(aux, >>> - DP_LANE0_1_STATUS, >>> - link_status, >>> - DP_LINK_STATUS_SIZE); >>> + DP_SINK_COUNT, >>> + full_link_stat, >>> + sizeof(full_link_stat)); >>> >>> if (ret < 0) >>> return ret; >>> >>> - WARN_ON(ret != DP_LINK_STATUS_SIZE); >>> + memcpy(link_status, full_link_stat + 2, DP_LINK_STATUS_SIZE); >>> + WARN_ON(ret != DP_LINK_STATUS_SIZE + 2); >>> >>> return 0; >>> } >> >>-- >>Jani Nikula, Intel Open Source Graphics Center >
Re: [Intel-gfx] [PATCH] drm/i915/ttm: ensure we release the intel_memory_region
On Thu, 19 Aug 2021 at 08:25, Thomas Hellström wrote: > > On Wed, 2021-08-18 at 18:12 +0100, Matthew Auld wrote: > > If the ttm_bo_init_reserved() call fails ensure we also release the > > region, otherwise we leak the reference, or worse hit some uaf, when > > we > > start using the objects.list. Also remove the make_unshrinkable call > > here, which doesn't do anything. > > > > Signed-off-by: Matthew Auld > > Cc: Thomas Hellström > > --- > > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 +-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > index 771eb2963123..2e8cdcd5e4f7 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > @@ -909,7 +909,6 @@ int __i915_gem_ttm_object_init(struct > > intel_memory_region *mem, > > drm_gem_private_object_init(&i915->drm, &obj->base, size); > > i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, > > flags); > > i915_gem_object_init_memory_region(obj, mem); > > - i915_gem_object_make_unshrinkable(obj); > > INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL | > > __GFP_NOWARN); > > mutex_init(&obj->ttm.get_io_page.lock); > > bo_type = (obj->flags & I915_BO_ALLOC_USER) ? > > ttm_bo_type_device : > > @@ -932,7 +931,7 @@ int __i915_gem_ttm_object_init(struct > > intel_memory_region *mem, > >page_size >> PAGE_SHIFT, > >&ctx, NULL, NULL, > > i915_ttm_bo_destroy); > > if (ret) > > - return i915_ttm_err_to_gem(ret); > > + goto err_release_mr; > > IIRC when ttm_object_init_reserved fails, it will call ttm_bo_put() > which will eventually end up in i915_ttm_bo_destroy() which will do the > right thing? Ah right, missed that. > > /Thomas > >
Re: [Intel-gfx] [PATCH 2/4] drm/i915/guc: Print error name on CTB (de)registration failure
On Wed, Aug 18, 2021 at 09:12:32PM +0200, Michal Wajdeczko wrote: > > > On 18.08.2021 18:35, Daniel Vetter wrote: > > On Wed, Aug 18, 2021 at 5:11 PM Michal Wajdeczko > > wrote: > >> > >> > >> > >> On 18.08.2021 16:20, Daniel Vetter wrote: > >>> On Thu, Jul 01, 2021 at 05:55:11PM +0200, Michal Wajdeczko wrote: > Instead of plain error value (%d) print more user friendly error > name (%pe). > > Signed-off-by: Michal Wajdeczko > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > index a26bb55c0898..18d52c39f0c2 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c > @@ -167,8 +167,8 @@ static int ct_register_buffer(struct intel_guc_ct > *ct, u32 type, > err = guc_action_register_ct_buffer(ct_to_guc(ct), type, > desc_addr, buff_addr, size); > if (unlikely(err)) > -CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n", > - guc_ct_buffer_type_to_str(type), err); > +CT_ERROR(ct, "Failed to register %s buffer (%pe)\n", > + guc_ct_buffer_type_to_str(type), ERR_PTR(err)); > >>> > >>> errname() is what you want here, not this convoluted jumping through hoops > >>> to fake an error pointer. > >> > >> nope, I was already trying that shortcut, but errname() is not exported > >> so we fail with > >> > >> ERROR: modpost: "errname" [drivers/gpu/drm/i915/i915.ko] undefined! > >> > >> so unless we add that export we must follow initial approach [1] > > > > Then we export that function. This is all open source, we can actually > > fix things up, there should _never_ be a need to hack around things > > like this. > > simple export might be not sufficient, as this function returns NULL for > unrecognized error codes, and it might be hard to print that code in > plain format, as it %pe does it for us for free. "(%s:%i)", errname(ret), ret Would work, but maybe not so pretty. Otoh %pe for unrecognized integers is also not very pretty. > is that big problem to use ERR_PTR? I'm not the only/first one > > see > drivers/net/can/usb/etas_es58x/es58x_core.c > drivers/net/ethernet/freescale/enetc/enetc_pf.c > drivers/net/phy/phylink.c > ... Ah yeah, looking through grep more than half the users do this pattern. Which still feels extremely silly, because it's not a pointer we're printing, and when it's not an errno we should probably print it as an integer or something. But also meh. On both patches, as-is: Reviewed-by: Daniel Vetter > > And yes I'm keenly aware that there's a pile of i915-gem code which > > outright flaunts this principle, but that doesn't mean we should > > continue with that. scripts/get_maintainers.pl can help with finding > > all the people you need to cc on that export patch. > > I'm perfectly fine with updating/fixing shared code (did that before, > have few more ideas on my todo-list) but in this case I'm not so sure I think an %ie extension or something like that for printk would make some sense. There's absolute enormous amounts of this kind of casting going on, and it just doesn't make sense to me. It would be pretty easy way to get like 100 patches into the kernel to clean it all up :-) -Daniel > > -Michal > > > -Daniel > > > >> > >> -Michal > >> > >> [1] > >> https://cgit.freedesktop.org/drm/drm-tip/commit/?id=57f5677e535ba24b8926a7125be2ef8d7f09323c > >> > >>> > >>> With that: Reviewed-by: Daniel Vetter > return err; > } > > @@ -195,8 +195,8 @@ static int ct_deregister_buffer(struct intel_guc_ct > *ct, u32 type) > int err = guc_action_deregister_ct_buffer(ct_to_guc(ct), type); > > if (unlikely(err)) > -CT_ERROR(ct, "Failed to deregister %s buffer (err=%d)\n", > - guc_ct_buffer_type_to_str(type), err); > +CT_ERROR(ct, "Failed to deregister %s buffer (%pe)\n", > + guc_ct_buffer_type_to_str(type), ERR_PTR(err)); > return err; > } > > -- > 2.25.1 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >>> > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [Intel-gfx] [PATCH] drm/damage_helper: Fix handling of cursor dirty buffers
On Wed, Aug 18, 2021 at 04:44:44PM +, Souza, Jose wrote: > On Wed, 2021-08-18 at 11:54 +0200, Daniel Vetter wrote: > > On Tue, Aug 17, 2021 at 04:26:04PM -0700, José Roberto de Souza wrote: > > > Cursors don't have a framebuffer so the fb comparisson was always > > > failing and atomic state was being committed without any plane state. > > > > > > So here checking if objects match when checking cursors. > > > > This looks extremely backwards ... what exactly is this fixing? If this > > isn't based on a real world compositor usage but some igt, then I'd say > > the igt here is very wrong. > > Yes it is IGT. > Writing to cursor buffer current in the screen and calling > drmModeDirtyFB() causes a empty atomic commit by > drm_atomic_helper_dirtyfb(). Ok if the cursor write is done through legacy cursor uapi then trying to make that work with dirtyfb doesn't make sense. But you've found a bug at least, namely the empty commit. I think that should be filtered out, and our dirtyfb testcases (I hope we have some vendor-agnostic kms_dirtyfb already) should be extended with a testcase where we call dirtyfb on an fb which is not currently used anywhere. Wrt the cursor: The legacy cursor ioctls get remapped onto the cursor plane, which means they come in as full commits. So there's really not dirtyfb required afterwards. The funny thing about the cursor ioctls is that they never supported frontbuffer rendering. You _always_ had to call them to upload data. So the test is invalid from a functional pov too. -Daniel > > > > -Daniel > > > > > Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") > > > Cc: Daniel Vetter > > > Cc: Rob Clark > > > Cc: Deepak Rawat > > > Cc: Gwan-gyeong Mun > > > Signed-off-by: José Roberto de Souza > > > --- > > > drivers/gpu/drm/drm_damage_helper.c | 8 +++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_damage_helper.c > > > b/drivers/gpu/drm/drm_damage_helper.c > > > index 8eeff0c7bdd47..595187d97c131 100644 > > > --- a/drivers/gpu/drm/drm_damage_helper.c > > > +++ b/drivers/gpu/drm/drm_damage_helper.c > > > @@ -157,12 +157,18 @@ int drm_atomic_helper_dirtyfb(struct > > > drm_framebuffer *fb, > > > retry: > > > drm_for_each_plane(plane, fb->dev) { > > > struct drm_plane_state *plane_state; > > > + bool match; > > > > > > ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx); > > > if (ret) > > > goto out; > > > > > > - if (plane->state->fb != fb) { > > > + match = plane->state->fb == fb; > > > + /* Check if objs match to handle dirty buffers of cursors */ > > > + if (plane->type == DRM_PLANE_TYPE_CURSOR && plane->state->fb) > > > + match |= fb->obj[0] == plane->state->fb->obj[0]; > > > + > > > + if (!match) { > > > drm_modeset_unlock(&plane->mutex); > > > continue; > > > } > > > -- > > > 2.32.0 > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: refactor the i915 GVT support
On 2021.08.17 13:22:03 +0800, Zhenyu Wang wrote: > > On 2021.08.16 19:34:58 +0200, Christoph Hellwig wrote: > > > Any updates on this? I'd really hate to miss this merge window. > > > > I'm still waiting for our validation team's report on this. I'm afraid > > it might be missing for next version as i915 merge window is mostly > > till rc5...and for any change outside of gvt, it still needs to be > > acked by i915 maintainers. > > Looks our validation team did have problem against recent i915 change. > If you like to try, we have a gvt-staging branch on > https://github.com/intel/gvt-linux which is generated against drm-tip > with gvt changes for testing, currently it's broken. > > One issue is with i915 export that intel_context_unpin has been > changed into static inline function. Another is that intel_gvt.c > should be part of i915 for gvt interface instead of depending on KVMGT > config. I'm working on below patch to resolve this. But I met a weird issue in case when building i915 as module and also kvmgt module, it caused busy wait on request_module("kvmgt") when boot, it doesn't happen if building i915 into kernel. I'm not sure what could be the reason? > But the problem I see is that after moving gvt device model (gvt/*.c > except kvmgt.c) into kvmgt module, we'll have issue with initial mmio > state which current gvt relies on, that is in design supposed to get > initial HW state before i915 driver has taken any operation. Before > we can ensure that, I think we may only remove MPT part first but > still keep gvt device model as part of i915 with config. I'll try to > split that out. Sorry I misread the code that as we always request kvmgt module when gvt init, so it should still apply original method that this isn't a problem. Our current validation result has shown no regression as well. ---8<--- From 58ff84572f1a0f9d79ca1d7ec0cff5ecbe78d280 Mon Sep 17 00:00:00 2001 From: Zhenyu Wang Date: Thu, 19 Aug 2021 16:36:33 +0800 Subject: [PATCH] TESTONLY:drm/i915/gvt: potential fix for refactor against current tip --- drivers/gpu/drm/i915/Makefile | 4 +++- drivers/gpu/drm/i915/gt/intel_context.c | 5 + drivers/gpu/drm/i915/gt/intel_context.h | 3 ++- drivers/gpu/drm/i915/i915_trace.h | 1 + 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index c4f953837f72..2248574428a1 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -296,7 +296,9 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \ # virtual gpu code i915-y += i915_vgpu.o -i915-$(CONFIG_DRM_I915_GVT_KVMGT) += intel_gvt.o +ifneq ($(CONFIG_DRM_I915_GVT_KVMGT),) +i915-y += intel_gvt.o +endif kvmgt-y += gvt/kvmgt.o \ gvt/gvt.o \ diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index 745e84c72c90..20e7522fed84 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -328,6 +328,11 @@ void __intel_context_do_unpin(struct intel_context *ce, int sub) intel_context_put(ce); } +void intel_context_unpin(struct intel_context *ce) +{ + _intel_context_unpin(ce); +} + static void __intel_context_retire(struct i915_active *active) { struct intel_context *ce = container_of(active, typeof(*ce), active); diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h index c41098950746..f942cbf6300a 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.h +++ b/drivers/gpu/drm/i915/gt/intel_context.h @@ -131,7 +131,7 @@ static inline void intel_context_sched_disable_unpin(struct intel_context *ce) __intel_context_do_unpin(ce, 2); } -static inline void intel_context_unpin(struct intel_context *ce) +static inline void _intel_context_unpin(struct intel_context *ce) { if (!ce->ops->sched_disable) { __intel_context_do_unpin(ce, 1); @@ -150,6 +150,7 @@ static inline void intel_context_unpin(struct intel_context *ce) } } } +void intel_context_unpin(struct intel_context *ce); void intel_context_enter_engine(struct intel_context *ce); void intel_context_exit_engine(struct intel_context *ce); diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 806ad688274b..2c6a8bcef7c1 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -17,6 +17,7 @@ #undef TRACE_SYSTEM #define TRACE_SYSTEM i915 +#undef TRACE_INCLUDE_FILE #define TRACE_INCLUDE_FILE i915_trace /* watermark/fifo updates */ -- 2.32.0 ---8<--- signature.asc Description: PGP signature
Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."
On Thu, Aug 19, 2021 at 03:01:26AM +, Liu, Monk wrote: > [AMD Official Use Only] > > Hi Andrey and Daniel > > We worked for a really long time on this new feature to AMD that finally > can pick up the bad job from all timedout ones, and the change in > scheduler (get/put fence in drm_sched_job_timedout, and remove the bad > job delete and put back) is the last piece for us. > > While we understand and realized that after the "bad job list node > delete logic" being removed from job_timedout, there will be race > issues introduced if vendor's job_timeout calback is accessing the bad > job in parallel of scheduler doing "sched->ops->free_job(leanup_job)". > > And to not introduce impact at all on those vendors I'd like to proposal > a very simple change (which introduced a new bool member for scheduler > to indicate if the del/put-back logic is needed or not) , check patch > here below: If everyone operates like that then the shared code becomes a massive mess of incompatible options and unmaintainable. I don't think that's a good path forward. -Daniel > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > index 47ea468..5e0bdc4 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > @@ -495,6 +495,8 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring > *ring, > return r; > } > > + ring->sched.keep_bad_job = true; > + > return 0; > } > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c > b/drivers/gpu/drm/scheduler/sched_main.c > index 92d8de2..e7ac384 100644 > --- a/drivers/gpu/drm/scheduler/sched_main.c > +++ b/drivers/gpu/drm/scheduler/sched_main.c > @@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct > *work) > { > struct drm_gpu_scheduler *sched; > struct drm_sched_job *job; > + struct dma_fence *f = NULL; > > sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work); > > @@ -328,7 +329,11 @@ static void drm_sched_job_timedout(struct work_struct > *work) >* drm_sched_cleanup_jobs. It will be reinserted back after > sched->thread >* is parked at which point it's safe. >*/ > - list_del_init(&job->list); > + if (sched->keep_bad_job == false) > + list_del_init(&job->list); > + else > + f = dma_fence_get(job->s_fence->parent);//get parent > fence here to prevent hw_fence dropping to zero due to sched-main's > cleanup_jobs, for amdgpu once parent fence drop to zero the sched_job will be > kfree-ed > + > spin_unlock(&sched->job_list_lock); > > job->sched->ops->timedout_job(job); > @@ -341,6 +346,8 @@ static void drm_sched_job_timedout(struct work_struct > *work) > job->sched->ops->free_job(job); > sched->free_guilty = false; > } > + > + dma_fence_put(f); > } else { > spin_unlock(&sched->job_list_lock); > } > @@ -396,7 +403,7 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, > struct drm_sched_job *bad) >* (earlier) cleanups and drm_sched_get_cleanup_job will not be called >* now until the scheduler thread is unparked. >*/ > - if (bad && bad->sched == sched) > + if (bad && bad->sched == sched && sched->keep_bad_job == false) > /* >* Add at the head of the queue to reflect it was the earliest >* job extracted. > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > index 4ea8606..5f9a640 100644 > --- a/include/drm/gpu_scheduler.h > +++ b/include/drm/gpu_scheduler.h > @@ -301,6 +301,7 @@ struct drm_gpu_scheduler { > atomic_t_score; > boolready; > boolfree_guilty; > + bool keep_bad_job; > }; > > int drm_sched_init(struct drm_gpu_scheduler *sched, > > > Thanks > > -- > Monk Liu | Cloud-GPU Core team > -- > > -Original Message- > From: Daniel Vetter > Sent: Wednesday, August 18, 2021 10:43 PM > To: Grodzovsky, Andrey > Cc: Daniel Vetter ; Alex Deucher ; > Chen, JingWen ; Maling list - DRI developers > ; amd-gfx list > ; Liu, Monk ; Koenig, > Christian > Subject: Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job." > > On Wed, Aug 18, 2021 at 10:36:32AM -0400, Andrey Grodzovsky wrote: > > > > On 2021-08-18 10:32 a.m., Daniel Vetter wrote: > > > On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote: > > > > On 2021-08-18 10:02 a.m., Alex Deucher wrote: > > > > > > > > > + dri-devel > > > > > > > > > > Since scheduler is a shared component, please add dri-devel on > > > > > all scheduler patches. > > > > > >
Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."
On Wed, Aug 18, 2021 at 10:51:00AM -0400, Andrey Grodzovsky wrote: > > On 2021-08-18 10:42 a.m., Daniel Vetter wrote: > > On Wed, Aug 18, 2021 at 10:36:32AM -0400, Andrey Grodzovsky wrote: > > > On 2021-08-18 10:32 a.m., Daniel Vetter wrote: > > > > On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote: > > > > > On 2021-08-18 10:02 a.m., Alex Deucher wrote: > > > > > > > > > > > + dri-devel > > > > > > > > > > > > Since scheduler is a shared component, please add dri-devel on all > > > > > > scheduler patches. > > > > > > > > > > > > On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen > > > > > > wrote: > > > > > > > [Why] > > > > > > > for bailing job, this commit will delete it from pending list > > > > > > > thus the > > > > > > > bailing job will never have a chance to be resubmitted even in > > > > > > > advance > > > > > > > tdr mode. > > > > > > > > > > > > > > [How] > > > > > > > after embeded hw_fence into amdgpu_job is done, the race > > > > > > > condition that > > > > > > > this commit tries to work around is completely solved.So revert > > > > > > > this > > > > > > > commit. > > > > > > > This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90. > > > > > > > v2: > > > > > > > add dma_fence_get/put() around timedout_job to avoid concurrent > > > > > > > delete > > > > > > > during processing timedout_job > > > > > > > > > > > > > > Signed-off-by: Jingwen Chen > > > > > > > --- > > > > > > > drivers/gpu/drm/scheduler/sched_main.c | 23 > > > > > > > +-- > > > > > > > 1 file changed, 5 insertions(+), 18 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > index a2a953693b45..f9b9b3aefc4a 100644 > > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > @@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct > > > > > > > work_struct *work) > > > > > > > { > > > > > > >struct drm_gpu_scheduler *sched; > > > > > > >struct drm_sched_job *job; > > > > > > > + struct dma_fence *fence; > > > > > > >enum drm_gpu_sched_stat status = > > > > > > > DRM_GPU_SCHED_STAT_NOMINAL; > > > > > > > > > > > > > >sched = container_of(work, struct drm_gpu_scheduler, > > > > > > > work_tdr.work); > > > > > > > @@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct > > > > > > > work_struct *work) > > > > > > > > > > > > > >if (job) { > > > > > > >/* > > > > > > > -* Remove the bad job so it cannot be freed by > > > > > > > concurrent > > > > > > > -* drm_sched_cleanup_jobs. It will be reinserted > > > > > > > back after sched->thread > > > > > > > -* is parked at which point it's safe. > > > > > > > +* Get job->s_fence->parent here to avoid > > > > > > > concurrent delete during > > > > > > > +* processing timedout_job > > > > > > > */ > > > > > > > - list_del_init(&job->list); > > > > > > > + fence = dma_fence_get(job->s_fence->parent); > > > > > While this is true for amdgpu, it has no meaning for other drivers > > > > > for whom > > > > > we haven't > > > > > done the refactoring of embedding HW fence (parent) into the job > > > > > structure. > > > > > In fact thinking > > > > > about it, unless you do the HW fence embedding for all the drivers > > > > > using the > > > > > scheduler you cannot > > > > > revert this patch or you will just break them. > > > > btw, why did you do that embedding? I do still have my patches with > > > > dma_fence annotations floating around, but my idea at least was to fix > > > > that issue with a mempool, not with embeddeding. What was the motivation > > > > for embedding the wh fence? > > > > -Daniel > > > > > > The motivation was 2 fold, avoid memory allocation during jobs submissions > > > (HW fence allocation) because as Christian explained this leads to > > > deadlock > > > with > > > mm code during evictions due to memory pressure (Christian can clarify if > > > I > > > messed > > Yeah that's the exact same thing I've chased with my dma_fence > > annotations, but thus far zero to none interested in getting it sorted. I > > think it'd be good to have some cross-driver agreement on how this should > > be solved before someone just charges ahead ... > > > > > this explanation). Second is to exactly revert this patch because while it > > > solved the issue > > > described in the patch it created another with drivers who baildc out > > > early > > > during TDR handling > > > for various reason and the job would just leak because it was already > > > removed form pending list. > > Can't we reinsert it before we restart the scheduler thread? It might need > > a separate list for that due to the lockless qu
[PATCH v2 1/2] drm/i915/buddy: add some pretty printing
Implement the debug hook for the buddy resource manager. For this we want to print out the status of the memory manager, including how much memory is still allocatable, what page sizes we have etc. This will be triggered when TTM is unable to fulfil an allocation request for device local-memory. v2(Thomas): - s/MB/MiB - s/KB/KiB Signed-off-by: Matthew Auld Cc: Thomas Hellström Reviewed-by: Thomas Hellström --- drivers/gpu/drm/i915/i915_buddy.c | 45 +++ drivers/gpu/drm/i915/i915_buddy.h | 8 drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 20 - 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_buddy.c b/drivers/gpu/drm/i915/i915_buddy.c index 7b274c51cac0..6e2ad68f8f3f 100644 --- a/drivers/gpu/drm/i915/i915_buddy.c +++ b/drivers/gpu/drm/i915/i915_buddy.c @@ -4,6 +4,7 @@ */ #include +#include #include "i915_buddy.h" @@ -82,6 +83,7 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size) size = round_down(size, chunk_size); mm->size = size; + mm->avail = size; mm->chunk_size = chunk_size; mm->max_order = ilog2(size) - ilog2(chunk_size); @@ -155,6 +157,8 @@ void i915_buddy_fini(struct i915_buddy_mm *mm) i915_block_free(mm, mm->roots[i]); } + GEM_WARN_ON(mm->avail != mm->size); + kfree(mm->roots); kfree(mm->free_list); } @@ -230,6 +234,7 @@ void i915_buddy_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block) { GEM_BUG_ON(!i915_buddy_block_is_allocated(block)); + mm->avail += i915_buddy_block_size(mm, block); __i915_buddy_free(mm, block); } @@ -283,6 +288,7 @@ i915_buddy_alloc(struct i915_buddy_mm *mm, unsigned int order) } mark_allocated(block); + mm->avail -= i915_buddy_block_size(mm, block); kmemleak_update_trace(block); return block; @@ -368,6 +374,7 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm, } mark_allocated(block); + mm->avail -= i915_buddy_block_size(mm, block); list_add_tail(&block->link, &allocated); continue; } @@ -402,6 +409,44 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm, return err; } +void i915_buddy_block_print(struct i915_buddy_mm *mm, + struct i915_buddy_block *block, + struct drm_printer *p) +{ + u64 start = i915_buddy_block_offset(block); + u64 size = i915_buddy_block_size(mm, block); + + drm_printf(p, "%#018llx-%#018llx: %llu\n", start, start + size, size); +} + +void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer *p) +{ + int order; + + drm_printf(p, "chunk_size: %lluKiB, total: %lluMiB, free: %lluMiB\n", + mm->chunk_size >> 10, mm->size >> 20, mm->avail >> 20); + + for (order = mm->max_order; order >= 0; order--) { + struct i915_buddy_block *block; + u64 count = 0, free; + + list_for_each_entry(block, &mm->free_list[order], link) { + GEM_BUG_ON(!i915_buddy_block_is_free(block)); + count++; + } + + drm_printf(p, "order-%d ", order); + + free = count * (mm->chunk_size << order); + if (free < SZ_1M) + drm_printf(p, "free: %lluKiB", free >> 10); + else + drm_printf(p, "free: %lluMiB", free >> 20); + + drm_printf(p, ", pages: %llu\n", count); + } +} + #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftests/i915_buddy.c" #endif diff --git a/drivers/gpu/drm/i915/i915_buddy.h b/drivers/gpu/drm/i915/i915_buddy.h index 3940d632f208..7077742112ac 100644 --- a/drivers/gpu/drm/i915/i915_buddy.h +++ b/drivers/gpu/drm/i915/i915_buddy.h @@ -10,6 +10,8 @@ #include #include +#include + struct i915_buddy_block { #define I915_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12) #define I915_BUDDY_HEADER_STATE GENMASK_ULL(11, 10) @@ -69,6 +71,7 @@ struct i915_buddy_mm { /* Must be at least PAGE_SIZE */ u64 chunk_size; u64 size; + u64 avail; }; static inline u64 @@ -129,6 +132,11 @@ void i915_buddy_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block); void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head *objects); +void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer *p); +void i915_buddy_block_print(struct i915_buddy_mm *mm, + struct i915_buddy_block *block, + struct drm_printer *p); + void i915_buddy_module_exit(void); int i915_buddy_module_init(void); diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_bu
[PATCH v2 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug
This should give a more complete view of the various bits of internal resource manager state, for device local-memory. v2(Thomas): - Move the region printing into a nice helper Signed-off-by: Matthew Auld Cc: Thomas Hellström --- drivers/gpu/drm/i915/i915_debugfs.c| 4 ++-- drivers/gpu/drm/i915/intel_memory_region.c | 12 drivers/gpu/drm/i915/intel_memory_region.h | 4 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index eec0d349ea6a..04351a851586 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) static int i915_gem_object_info(struct seq_file *m, void *data) { struct drm_i915_private *i915 = node_to_i915(m->private); + struct drm_printer p = drm_seq_file_printer(m); struct intel_memory_region *mr; enum intel_region_id id; @@ -246,8 +247,7 @@ static int i915_gem_object_info(struct seq_file *m, void *data) atomic_read(&i915->mm.free_count), i915->mm.shrink_memory); for_each_memory_region(mr, i915, id) - seq_printf(m, "%s: total:%pa, available:%pa bytes\n", - mr->name, &mr->total, &mr->avail); + intel_memory_region_debug(mr, &p); return 0; } diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c index 779eb2fa90b6..e7f7e6627750 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.c +++ b/drivers/gpu/drm/i915/intel_memory_region.c @@ -78,6 +78,18 @@ int intel_memory_region_reserve(struct intel_memory_region *mem, return i915_ttm_buddy_man_reserve(man, offset, size); } +void intel_memory_region_debug(struct intel_memory_region *mr, + struct drm_printer *printer) +{ + drm_printf(printer, "%s: ", mr->name); + + if (mr->region_private) + ttm_resource_manager_debug(mr->region_private, printer); + else + drm_printf(printer, "total:%pa, available:%pa bytes\n", + &mr->total, &mr->avail); +} + struct intel_memory_region * intel_memory_region_create(struct drm_i915_private *i915, resource_size_t start, diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h index 1f2b96efa69d..3feae3353d33 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.h +++ b/drivers/gpu/drm/i915/intel_memory_region.h @@ -15,6 +15,7 @@ struct drm_i915_private; struct drm_i915_gem_object; +struct drm_printer; struct intel_memory_region; struct sg_table; struct ttm_resource; @@ -127,6 +128,9 @@ int intel_memory_region_reserve(struct intel_memory_region *mem, resource_size_t offset, resource_size_t size); +void intel_memory_region_debug(struct intel_memory_region *mr, + struct drm_printer *printer); + struct intel_memory_region * i915_gem_ttm_system_setup(struct drm_i915_private *i915, u16 type, u16 instance); -- 2.26.3
Re: [PATCH v3 8/9] kernel: export task_work_add
On 19/8/21 5:26 pm, Christoph Hellwig wrote: On Wed, Aug 18, 2021 at 03:38:23PM +0800, Desmond Cheong Zhi Xi wrote: +EXPORT_SYMBOL(task_work_add); EXPORT_SYMBOL_GPL for this kinds of functionality, please. Thanks, I wasn't aware of the GPL-only export. I'll update this in a future series if we still need the export.
[PATCH v7 00/10] drm/vc4: hdmi: Support the 4k @ 60Hz modes
Hi, Here is a series that enables the higher resolutions on the HDMI0 Controller found in the BCM2711 (RPi4). In order to work it needs a few adjustments to config.txt, most notably to enable the enable_hdmi_4kp60 option. Let me know what you think, Maxime --- Changes from v6: - Rebased on current drm-misc-next - Removed stale clk_request pointer Changes from v5: - Fixed unused variables warning Changes from v4: - Removed the patches already applied - Added various fixes for the issues that have been discovered on the downstream tree Changes from v3: - Rework the encoder retrieval code that was broken on the RPi3 and older - Fix a scrambling enabling issue on some display Changes from v2: - Gathered the various tags - Added Cc stable when relevant - Split out the check to test whether the scrambler is required into an helper - Fixed a bug where the scrambler state wouldn't be tracked properly if it was enabled at boot Changes from v1: - Dropped the range accessors - Drop the mention of force_turbo - Reordered the SCRAMBLER_CTL register to match the offset - Removed duplicate HDMI_14_MAX_TMDS_CLK define - Warn about enable_hdmi_4kp60 only if there's some modes that can't be reached - Rework the BVB clock computation Maxime Ripard (10): drm/vc4: hdmi: Remove the DDC probing for status detection drm/vc4: hdmi: Fix HPD GPIO detection drm/vc4: Make vc4_crtc_get_encoder public drm/vc4: crtc: Add encoder to vc4_crtc_config_pv prototype drm/vc4: crtc: Rework the encoder retrieval code (again) drm/vc4: crtc: Add some logging drm/vc4: Leverage the load tracker on the BCM2711 drm/vc4: hdmi: Raise the maximum clock rate drm/vc4: hdmi: Enable the scrambler on reconnection drm/vc4: Increase the core clock based on HVS load drivers/gpu/drm/vc4/vc4_crtc.c| 60 -- drivers/gpu/drm/vc4/vc4_debugfs.c | 7 +- drivers/gpu/drm/vc4/vc4_drv.h | 8 +- drivers/gpu/drm/vc4/vc4_hdmi.c| 20 +++-- drivers/gpu/drm/vc4/vc4_kms.c | 126 +- drivers/gpu/drm/vc4/vc4_plane.c | 5 -- 6 files changed, 163 insertions(+), 63 deletions(-) -- 2.31.1
[PATCH v7 01/10] drm/vc4: hdmi: Remove the DDC probing for status detection
Commit 9d44a8d5 ("drm/vc4: Fall back to using an EDID probe in the absence of a GPIO.") added some code to read the EDID through DDC in the HDMI driver detect hook since the Pi3 had no HPD GPIO back then. However, commit b1b8f45b3130 ("ARM: dts: bcm2837: Add missing GPIOs of Expander") changed that a couple of years later. This causes an issue though since some TV (like the LG 55C8) when it comes out of standy will deassert the HPD line, but the EDID will remain readable. It causes an issues nn platforms without an HPD GPIO, like the Pi4, where the DDC probing will be our primary mean to detect a display, and thus we will never detect the HPD pulse. This was fine before since the pulse was small enough that we would never detect it, and we also didn't have anything (like the scrambler) that needed to be set up in the display. However, now that we have both, the display during the HPD pulse will clear its scrambler status, and since we won't detect the disconnect/reconnect cycle we will never enable the scrambler back. As our main reason for that DDC probing is gone, let's just remove it. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b7dc32a0c9bb..f9a672a641ab 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -172,8 +172,6 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) if (vc4_hdmi->hpd_gpio && gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) { connected = true; - } else if (drm_probe_ddc(vc4_hdmi->ddc)) { - connected = true; } else if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) { connected = true; } -- 2.31.1
[PATCH v7 03/10] drm/vc4: Make vc4_crtc_get_encoder public
We'll need that function in vc4_kms to compute the core clock rate requirements. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_crtc.c | 8 drivers/gpu/drm/vc4/vc4_drv.h | 5 + 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 18f5009ce90e..902862a67341 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -279,10 +279,10 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc, * allows drivers to push pixels to more than one encoder from the * same CRTC. */ -static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc, - struct drm_atomic_state *state, - struct drm_connector_state *(*get_state)(struct drm_atomic_state *state, - struct drm_connector *connector)) +struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc, +struct drm_atomic_state *state, +struct drm_connector_state *(*get_state)(struct drm_atomic_state *state, + struct drm_connector *connector)) { struct drm_connector *connector; struct drm_connector_list_iter conn_iter; diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index ef73e0aaf726..0865f05822e0 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -517,6 +517,11 @@ vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc) return container_of(data, struct vc4_pv_data, base); } +struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc, +struct drm_atomic_state *state, +struct drm_connector_state *(*get_state)(struct drm_atomic_state *state, + struct drm_connector *connector)); + struct vc4_crtc_state { struct drm_crtc_state base; /* Dlist area for this CRTC configuration. */ -- 2.31.1
[PATCH v7 02/10] drm/vc4: hdmi: Fix HPD GPIO detection
Prior to commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod"), in the detect hook, if we had an HPD GPIO we would only rely on it and return whatever state it was in. However, that commit changed that by mistake to only consider the case where we have a GPIO and it returns a logical high, and would fall back to the other methods otherwise. Since we can read the EDIDs when the HPD signal is low on some displays, we changed the detection status from disconnected to connected, and we would ignore an HPD pulse. Fixes: 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod") Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index f9a672a641ab..251dfecf1d4c 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -169,9 +169,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) WARN_ON(pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev)); - if (vc4_hdmi->hpd_gpio && - gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) { - connected = true; + if (vc4_hdmi->hpd_gpio) { + if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) + connected = true; } else if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) { connected = true; } -- 2.31.1
[PATCH v7 06/10] drm/vc4: crtc: Add some logging
The encoder retrieval code has been a source of bugs and glitches in the past and the crtc <-> encoder association been wrong in a number of different ways. Add some logging to quickly spot issues if they occur. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index c88ce31ec90f..073b7e528175 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -524,6 +524,9 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, old_state); struct drm_device *dev = crtc->dev; + drm_dbg(dev, "Disabling CRTC %s (%u) connected to Encoder %s (%u)", + crtc->name, crtc->base.id, encoder->name, encoder->base.id); + require_hvs_enabled(dev); /* Disable vblank irq handling before crtc is disabled. */ @@ -555,6 +558,9 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, new_state); struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder); + drm_dbg(dev, "Enabling CRTC %s (%u) connected to Encoder %s (%u)", + crtc->name, crtc->base.id, encoder->name, encoder->base.id); + require_hvs_enabled(dev); /* Enable vblank irq handling before crtc is started otherwise -- 2.31.1
[PATCH v7 05/10] drm/vc4: crtc: Rework the encoder retrieval code (again)
It turns out the encoder retrieval code, in addition to being unnecessarily complicated, has a bug when only the planes and crtcs are affected by a given atomic commit. Indeed, in such a case, either drm_atomic_get_old_connector_state or drm_atomic_get_new_connector_state will return NULL and thus our encoder retrieval code will not match on anything. We can however simplify the code by using drm_for_each_encoder_mask, the drm_crtc_state storing the encoders a given CRTC is connected to directly and without relying on any other state. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_crtc.c | 30 +- drivers/gpu/drm/vc4/vc4_drv.h | 4 +--- 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 93d2413d0842..c88ce31ec90f 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -280,26 +280,14 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc, * same CRTC. */ struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc, -struct drm_atomic_state *state, -struct drm_connector_state *(*get_state)(struct drm_atomic_state *state, - struct drm_connector *connector)) +struct drm_crtc_state *state) { - struct drm_connector *connector; - struct drm_connector_list_iter conn_iter; + struct drm_encoder *encoder; - drm_connector_list_iter_begin(crtc->dev, &conn_iter); - drm_for_each_connector_iter(connector, &conn_iter) { - struct drm_connector_state *conn_state = get_state(state, connector); + WARN_ON(hweight32(state->encoder_mask) > 1); - if (!conn_state) - continue; - - if (conn_state->crtc == crtc) { - drm_connector_list_iter_end(&conn_iter); - return connector->encoder; - } - } - drm_connector_list_iter_end(&conn_iter); + drm_for_each_encoder_mask(encoder, crtc->dev, state->encoder_mask) + return encoder; return NULL; } @@ -533,8 +521,7 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state, crtc); struct vc4_crtc_state *old_vc4_state = to_vc4_crtc_state(old_state); - struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state, - drm_atomic_get_old_connector_state); + struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, old_state); struct drm_device *dev = crtc->dev; require_hvs_enabled(dev); @@ -561,10 +548,11 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc, static void vc4_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state) { + struct drm_crtc_state *new_state = drm_atomic_get_new_crtc_state(state, +crtc); struct drm_device *dev = crtc->dev; struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); - struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state, - drm_atomic_get_new_connector_state); + struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, new_state); struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder); require_hvs_enabled(dev); diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 0865f05822e0..220c5e81ba2c 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -518,9 +518,7 @@ vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc) } struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc, -struct drm_atomic_state *state, -struct drm_connector_state *(*get_state)(struct drm_atomic_state *state, - struct drm_connector *connector)); +struct drm_crtc_state *state); struct vc4_crtc_state { struct drm_crtc_state base; -- 2.31.1
[PATCH v7 09/10] drm/vc4: hdmi: Enable the scrambler on reconnection
If we have a state already and disconnect/reconnect the display, the SCDC messages won't be sent again since we didn't go through a disable / enable cycle. In order to fix this, let's call the vc4_hdmi_enable_scrambling function in the detect callback if there is a mode and it needs the scrambler to be enabled. Fixes: c85695a2016e ("drm/vc4: hdmi: Enable the scrambler") Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index d2a720e05ddd..a3dbd1fdff7d 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -161,6 +161,8 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {} #endif +static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder); + static enum drm_connector_status vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) { @@ -187,7 +189,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) } } + vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base.base); pm_runtime_put(&vc4_hdmi->pdev->dev); + return connector_status_connected; } @@ -543,8 +547,12 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) struct drm_connector *connector = &vc4_hdmi->connector; struct drm_connector_state *cstate = connector->state; struct drm_crtc *crtc = cstate->crtc; - struct drm_display_mode *mode = &crtc->state->adjusted_mode; + struct drm_display_mode *mode; + if (!crtc || !crtc->state) + return; + + mode = &crtc->state->adjusted_mode; if (!vc4_hdmi_supports_scrambling(encoder, mode)) return; -- 2.31.1
[PATCH v7 07/10] drm/vc4: Leverage the load tracker on the BCM2711
The load tracker was initially designed to report and warn about a load too high for the HVS. To do so, it computes for each plane the impact it's going to have on the HVS, and will warn (if it's enabled) if we go over what the hardware can process. While the limits being used are a bit irrelevant to the BCM2711, the algorithm to compute the HVS load will be one component used in order to compute the core clock rate on the BCM2711. Let's remove the hooks to prevent the load tracker to do its computation, but since we don't have the same limits, don't check them against them, and prevent the debugfs file to enable it from being created. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_debugfs.c | 7 +-- drivers/gpu/drm/vc4/vc4_drv.h | 3 --- drivers/gpu/drm/vc4/vc4_kms.c | 16 +--- drivers/gpu/drm/vc4/vc4_plane.c | 5 - 4 files changed, 10 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c index 6da22af4ee91..ba2d8ea562af 100644 --- a/drivers/gpu/drm/vc4/vc4_debugfs.c +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c @@ -7,6 +7,7 @@ #include #include #include +#include #include "vc4_drv.h" #include "vc4_regs.h" @@ -26,8 +27,10 @@ vc4_debugfs_init(struct drm_minor *minor) struct vc4_dev *vc4 = to_vc4_dev(minor->dev); struct vc4_debugfs_info_entry *entry; - debugfs_create_bool("hvs_load_tracker", S_IRUGO | S_IWUSR, - minor->debugfs_root, &vc4->load_tracker_enabled); + if (!of_device_is_compatible(vc4->hvs->pdev->dev.of_node, +"brcm,bcm2711-vc5")) + debugfs_create_bool("hvs_load_tracker", S_IRUGO | S_IWUSR, + minor->debugfs_root, &vc4->load_tracker_enabled); list_for_each_entry(entry, &vc4->debugfs_list, link) { drm_debugfs_create_files(&entry->info, 1, diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 220c5e81ba2c..95a1eb7ebf90 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -202,9 +202,6 @@ struct vc4_dev { int power_refcount; - /* Set to true when the load tracker is supported. */ - bool load_tracker_available; - /* Set to true when the load tracker is active. */ bool load_tracker_enabled; diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index f0b3e4cf5bce..ca688115381e 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -551,9 +551,6 @@ static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state) struct drm_plane *plane; int i; - if (!vc4->load_tracker_available) - return 0; - priv_state = drm_atomic_get_private_obj_state(state, &vc4->load_tracker); if (IS_ERR(priv_state)) @@ -628,9 +625,6 @@ static void vc4_load_tracker_obj_fini(struct drm_device *dev, void *unused) { struct vc4_dev *vc4 = to_vc4_dev(dev); - if (!vc4->load_tracker_available) - return; - drm_atomic_private_obj_fini(&vc4->load_tracker); } @@ -638,9 +632,6 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4) { struct vc4_load_tracker_state *load_state; - if (!vc4->load_tracker_available) - return 0; - load_state = kzalloc(sizeof(*load_state), GFP_KERNEL); if (!load_state) return -ENOMEM; @@ -868,9 +859,12 @@ int vc4_kms_load(struct drm_device *dev) "brcm,bcm2711-vc5"); int ret; + /* +* The limits enforced by the load tracker aren't relevant for +* the BCM2711, but the load tracker computations are used for +* the core clock rate calculation. +*/ if (!is_vc5) { - vc4->load_tracker_available = true; - /* Start with the load tracker enabled. Can be * disabled through the debugfs load_tracker file. */ diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 19161b6ab27f..ac761c683663 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -529,11 +529,6 @@ static void vc4_plane_calc_load(struct drm_plane_state *state) struct vc4_plane_state *vc4_state; struct drm_crtc_state *crtc_state; unsigned int vscale_factor; - struct vc4_dev *vc4; - - vc4 = to_vc4_dev(state->plane->dev); - if (!vc4->load_tracker_available) - return; vc4_state = to_vc4_plane_state(state); crtc_state = drm_atomic_get_existing_crtc_state(state->state, -- 2.31.1
[PATCH v7 08/10] drm/vc4: hdmi: Raise the maximum clock rate
Now that we have the infrastructure in place, we can raise the maximum pixel rate we can reach for HDMI0 on the BCM2711. HDMI1 is left untouched since its pixelvalve has a smaller FIFO and would need a clock faster than what we can provide to support the same modes. Acked-by: Thomas Zimmermann Reviewed-by: Dave Stevenson Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 251dfecf1d4c..d2a720e05ddd 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2330,7 +2330,7 @@ static const struct vc4_hdmi_variant bcm2711_hdmi0_variant = { .encoder_type = VC4_ENCODER_TYPE_HDMI0, .debugfs_name = "hdmi0_regs", .card_name = "vc4-hdmi-0", - .max_pixel_clock= HDMI_14_MAX_TMDS_CLK, + .max_pixel_clock= 6, .registers = vc5_hdmi_hdmi0_fields, .num_registers = ARRAY_SIZE(vc5_hdmi_hdmi0_fields), .phy_lane_mapping = { -- 2.31.1
[PATCH v7 04/10] drm/vc4: crtc: Add encoder to vc4_crtc_config_pv prototype
vc4_crtc_config_pv() retrieves the encoder again, even though its only caller, vc4_crtc_atomic_enable(), already did. Pass the encoder pointer as an argument instead of going through all the connectors to retrieve it again. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_crtc.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 902862a67341..93d2413d0842 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -313,12 +313,11 @@ static void vc4_crtc_pixelvalve_reset(struct drm_crtc *crtc) CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) | PV_CONTROL_FIFO_CLR); } -static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_atomic_state *state) +static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_encoder *encoder, + struct drm_atomic_state *state) { struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); - struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state, - drm_atomic_get_new_connector_state); struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); const struct vc4_pv_data *pv_data = vc4_crtc_to_vc4_pv_data(vc4_crtc); @@ -580,7 +579,7 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc, if (vc4_encoder->pre_crtc_configure) vc4_encoder->pre_crtc_configure(encoder, state); - vc4_crtc_config_pv(crtc, state); + vc4_crtc_config_pv(crtc, encoder, state); CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) | PV_CONTROL_EN); -- 2.31.1
[PATCH v7 10/10] drm/vc4: Increase the core clock based on HVS load
Depending on a given HVS output (HVS to PixelValves) and input (planes attached to a channel) load, the HVS needs for the core clock to be raised above its boot time default. Failing to do so will result in a vblank timeout and a stalled display pipeline. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_crtc.c | 15 + drivers/gpu/drm/vc4/vc4_drv.h | 2 + drivers/gpu/drm/vc4/vc4_kms.c | 110 ++--- 3 files changed, 118 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 073b7e528175..c733b2091d3c 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -642,12 +642,27 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state); struct drm_connector *conn; struct drm_connector_state *conn_state; + struct drm_encoder *encoder; int ret, i; ret = vc4_hvs_atomic_check(crtc, state); if (ret) return ret; + encoder = vc4_get_crtc_encoder(crtc, crtc_state); + if (encoder) { + const struct drm_display_mode *mode = &crtc_state->adjusted_mode; + struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder); + + mode = &crtc_state->adjusted_mode; + if (vc4_encoder->type == VC4_ENCODER_TYPE_HDMI0) { + vc4_state->hvs_load = max(mode->clock * mode->hdisplay / mode->htotal + 1000, + mode->clock * 9 / 10) * 1000; + } else { + vc4_state->hvs_load = mode->clock * 1000; + } + } + for_each_new_connector_in_state(state, conn, conn_state, i) { if (conn_state->crtc != crtc) diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 95a1eb7ebf90..5abb3c090253 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -532,6 +532,8 @@ struct vc4_crtc_state { unsigned int bottom; } margins; + unsigned long hvs_load; + /* Transitional state below, only valid during atomic commits */ bool update_muxing; }; diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index ca688115381e..0a4fca043c65 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -39,9 +39,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv) struct vc4_hvs_state { struct drm_private_state base; + unsigned long core_clock_rate; struct { unsigned in_use: 1; + unsigned long fifo_load; struct drm_crtc_commit *pending_commit; } fifo_state[HVS_NUM_CHANNELS]; }; @@ -339,10 +341,19 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) struct vc4_hvs *hvs = vc4->hvs; struct drm_crtc_state *old_crtc_state; struct drm_crtc_state *new_crtc_state; + struct vc4_hvs_state *new_hvs_state; struct drm_crtc *crtc; struct vc4_hvs_state *old_hvs_state; int i; + old_hvs_state = vc4_hvs_get_old_global_state(state); + if (WARN_ON(!old_hvs_state)) + return; + + new_hvs_state = vc4_hvs_get_new_global_state(state); + if (WARN_ON(!new_hvs_state)) + return; + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { struct vc4_crtc_state *vc4_crtc_state; @@ -353,12 +364,13 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) vc4_hvs_mask_underrun(dev, vc4_crtc_state->assigned_channel); } - if (vc4->hvs->hvs5) - clk_set_min_rate(hvs->core_clk, 5); + if (vc4->hvs->hvs5) { + unsigned long core_rate = max_t(unsigned long, + 5, + new_hvs_state->core_clock_rate); - old_hvs_state = vc4_hvs_get_old_global_state(state); - if (!old_hvs_state) - return; + clk_set_min_rate(hvs->core_clk, core_rate); + } for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { struct vc4_crtc_state *vc4_crtc_state = @@ -398,8 +410,12 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) drm_atomic_helper_cleanup_planes(dev, state); - if (vc4->hvs->hvs5) - clk_set_min_rate(hvs->core_clk, 0); + if (vc4->hvs->hvs5) { + drm_dbg(dev, "Running the core clock at %lu Hz\n", + new_hvs_state->core_clock_rate); + + clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate); + } } static int vc4_atomic_commit_setup(struct drm_atomic_st
Re: [PATCH] backlight: pwm_bl: Improve bootloader/kernel device handover
On Thu, 22 Jul 2021, Daniel Thompson wrote: > Currently there are (at least) two problems in the way pwm_bl starts > managing the enable_gpio pin. Both occur when the backlight is initially > off and the driver finds the pin not already in output mode and, as a > result, unconditionally switches it to output-mode and asserts the signal. > > Problem 1: This could cause the backlight to flicker since, at this stage > in driver initialisation, we have no idea what the PWM and regulator are > doing (an unconfigured PWM could easily "rest" at 100% duty cycle). > > Problem 2: This will cause us not to correctly honour the > post_pwm_on_delay (which also risks flickers). > > Fix this by moving the code to configure the GPIO output mode until after > we have examines the handover state. That allows us to initialize > enable_gpio to off if the backlight is currently off and on if the > backlight is on. > > Reported-by: Marek Vasut > Signed-off-by: Daniel Thompson > Cc: sta...@vger.kernel.org > Acked-by: Marek Vasut > Tested-by: Marek Vasut > --- > drivers/video/backlight/pwm_bl.c | 54 +--- > 1 file changed, 28 insertions(+), 26 deletions(-) Applied, thanks. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
[PATCH 1/2] dt-bindings: panel: ili9341: correct indentation
Correct indentation warning: ilitek,ili9341.yaml:25:9: [warning] wrong indentation: expected 10 but found 8 (indentation) Signed-off-by: Krzysztof Kozlowski --- .../devicetree/bindings/display/panel/ilitek,ili9341.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml index 2ed010f91e2d..20ce88ab4b3a 100644 --- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml @@ -22,7 +22,7 @@ properties: items: - enum: # ili9341 240*320 Color on stm32f429-disco board -- st,sf-tc240t-9370-t + - st,sf-tc240t-9370-t - const: ilitek,ili9341 reg: true -- 2.30.2
[PATCH 2/2] dt-bindings: sound: rt1015p: correct indentation
Use common enum instead of oneOf and correct indentation warning: realtek,rt1015p.yaml:18:7: [warning] wrong indentation: expected 4 but found 6 (indentation) Signed-off-by: Krzysztof Kozlowski --- .../devicetree/bindings/sound/realtek,rt1015p.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/sound/realtek,rt1015p.yaml b/Documentation/devicetree/bindings/sound/realtek,rt1015p.yaml index f31d3c4d0192..fdb7f295ef2d 100644 --- a/Documentation/devicetree/bindings/sound/realtek,rt1015p.yaml +++ b/Documentation/devicetree/bindings/sound/realtek,rt1015p.yaml @@ -15,9 +15,9 @@ description: | properties: compatible: - oneOf: -- const: realtek,rt1015p -- const: realtek,rt1019p +enum: + - realtek,rt1015p + - realtek,rt1019p sdb-gpios: description: -- 2.30.2
RE: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."
[AMD Official Use Only] Hi Daniel >> Why can't we stop the scheduler thread first, so that there's guaranteed no >> race? I've recently had a lot of discussions with panfrost folks about their >> reset that spawns across engines, and without stopping the scheduler thread >> first before you touch anything it's just plain impossible. Yeah we had this though as well in our mind. Our second approach is to call ktrhead_stop() in job_timedout() routine so that the "bad" job is guaranteed to be used without scheduler's touching or freeing, Check this sample patch one as well please: diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index a2a9536..50a49cb 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -319,17 +319,12 @@ static void drm_sched_job_timedout(struct work_struct *work) sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work); /* Protects against concurrent deletion in drm_sched_get_cleanup_job */ + kthread_park(sched->thread); spin_lock(&sched->job_list_lock); job = list_first_entry_or_null(&sched->pending_list, struct drm_sched_job, list); if (job) { - /* -* Remove the bad job so it cannot be freed by concurrent -* drm_sched_cleanup_jobs. It will be reinserted back after sched->thread -* is parked at which point it's safe. -*/ - list_del_init(&job->list); spin_unlock(&sched->job_list_lock); status = job->sched->ops->timedout_job(job); @@ -345,6 +340,7 @@ static void drm_sched_job_timedout(struct work_struct *work) } else { spin_unlock(&sched->job_list_lock); } + kthread_unpark(sched->thread); if (status != DRM_GPU_SCHED_STAT_ENODEV) { spin_lock(&sched->job_list_lock); @@ -393,20 +389,6 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad) kthread_park(sched->thread); /* -* Reinsert back the bad job here - now it's safe as -* drm_sched_get_cleanup_job cannot race against us and release the -* bad job at this point - we parked (waited for) any in progress -* (earlier) cleanups and drm_sched_get_cleanup_job will not be called -* now until the scheduler thread is unparked. -*/ - if (bad && bad->sched == sched) - /* -* Add at the head of the queue to reflect it was the earliest -* job extracted. -*/ - list_add(&bad->list, &sched->pending_list); - - /* * Iterate the job list from later to earlier one and either deactive * their HW callbacks or remove them from pending list if they already * signaled. Thanks -- Monk Liu | Cloud-GPU Core team -- -Original Message- From: Daniel Vetter Sent: Thursday, August 19, 2021 5:31 PM To: Grodzovsky, Andrey Cc: Daniel Vetter ; Alex Deucher ; Chen, JingWen ; Maling list - DRI developers ; amd-gfx list ; Liu, Monk ; Koenig, Christian Subject: Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job." On Wed, Aug 18, 2021 at 10:51:00AM -0400, Andrey Grodzovsky wrote: > > On 2021-08-18 10:42 a.m., Daniel Vetter wrote: > > On Wed, Aug 18, 2021 at 10:36:32AM -0400, Andrey Grodzovsky wrote: > > > On 2021-08-18 10:32 a.m., Daniel Vetter wrote: > > > > On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote: > > > > > On 2021-08-18 10:02 a.m., Alex Deucher wrote: > > > > > > > > > > > + dri-devel > > > > > > > > > > > > Since scheduler is a shared component, please add dri-devel > > > > > > on all scheduler patches. > > > > > > > > > > > > On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen > > > > > > wrote: > > > > > > > [Why] > > > > > > > for bailing job, this commit will delete it from pending > > > > > > > list thus the bailing job will never have a chance to be > > > > > > > resubmitted even in advance tdr mode. > > > > > > > > > > > > > > [How] > > > > > > > after embeded hw_fence into amdgpu_job is done, the race > > > > > > > condition that this commit tries to work around is > > > > > > > completely solved.So revert this commit. > > > > > > > This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90. > > > > > > > v2: > > > > > > > add dma_fence_get/put() around timedout_job to avoid > > > > > > > concurrent delete during processing timedout_job > > > > > > > > > > > > > > Signed-off-by: Jingwen Chen > > > > > > > --- > > > > > > > drivers/gpu/drm/scheduler/sched_main.c | 23 > > > > > > > +-- > > > > > > > 1 file changed, 5 insertions(+), 18 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/dr
Re: [PATCH v3 7/9] drm: update global mutex lock in the ioctl handler
On 18/8/21 7:02 pm, Daniel Vetter wrote: On Wed, Aug 18, 2021 at 03:38:22PM +0800, Desmond Cheong Zhi Xi wrote: In a future patch, a read lock on drm_device.master_rwsem is held in the ioctl handler before the check for ioctl permissions. However, this produces the following lockdep splat: == WARNING: possible circular locking dependency detected 5.14.0-rc6-CI-Patchwork_20831+ #1 Tainted: G U -- kms_lease/1752 is trying to acquire lock: 827bad88 (drm_global_mutex){+.+.}-{3:3}, at: drm_open+0x64/0x280 but task is already holding lock: 88812e350108 (&dev->master_rwsem){}-{3:3}, at: drm_ioctl_kernel+0xfb/0x1a0 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #2 (&dev->master_rwsem){}-{3:3}: lock_acquire+0xd3/0x310 down_read+0x3b/0x140 drm_master_internal_acquire+0x1d/0x60 drm_client_modeset_commit+0x10/0x40 __drm_fb_helper_restore_fbdev_mode_unlocked+0x88/0xb0 drm_fb_helper_set_par+0x34/0x40 intel_fbdev_set_par+0x11/0x40 [i915] fbcon_init+0x270/0x4f0 visual_init+0xc6/0x130 do_bind_con_driver+0x1de/0x2c0 do_take_over_console+0x10e/0x180 do_fbcon_takeover+0x53/0xb0 register_framebuffer+0x22d/0x310 __drm_fb_helper_initial_config_and_unlock+0x36c/0x540 intel_fbdev_initial_config+0xf/0x20 [i915] async_run_entry_fn+0x28/0x130 process_one_work+0x26d/0x5c0 worker_thread+0x37/0x390 kthread+0x13b/0x170 ret_from_fork+0x1f/0x30 -> #1 (&helper->lock){+.+.}-{3:3}: lock_acquire+0xd3/0x310 __mutex_lock+0xa8/0x930 __drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0xb0 intel_fbdev_restore_mode+0x2b/0x50 [i915] drm_lastclose+0x27/0x50 drm_release_noglobal+0x42/0x60 __fput+0x9e/0x250 task_work_run+0x6b/0xb0 exit_to_user_mode_prepare+0x1c5/0x1d0 syscall_exit_to_user_mode+0x19/0x50 do_syscall_64+0x46/0xb0 entry_SYSCALL_64_after_hwframe+0x44/0xae -> #0 (drm_global_mutex){+.+.}-{3:3}: validate_chain+0xb39/0x1e70 __lock_acquire+0x5a1/0xb70 lock_acquire+0xd3/0x310 __mutex_lock+0xa8/0x930 drm_open+0x64/0x280 drm_stub_open+0x9f/0x100 chrdev_open+0x9f/0x1d0 do_dentry_open+0x14a/0x3a0 dentry_open+0x53/0x70 drm_mode_create_lease_ioctl+0x3cb/0x970 drm_ioctl_kernel+0xc9/0x1a0 drm_ioctl+0x201/0x3d0 __x64_sys_ioctl+0x6a/0xa0 do_syscall_64+0x37/0xb0 entry_SYSCALL_64_after_hwframe+0x44/0xae other info that might help us debug this: Chain exists of: drm_global_mutex --> &helper->lock --> &dev->master_rwsem Possible unsafe locking scenario: CPU0CPU1 lock(&dev->master_rwsem); lock(&helper->lock); lock(&dev->master_rwsem); lock(drm_global_mutex); *** DEADLOCK *** The lock hierarchy inversion happens because we grab the drm_global_mutex while already holding on to master_rwsem. To avoid this, we do some prep work to grab the drm_global_mutex before checking for ioctl permissions. At the same time, we update the check for the global mutex to use the drm_dev_needs_global_mutex helper function. This is intentional, essentially we force all non-legacy drivers to have unlocked ioctl (otherwise everyone forgets to set that flag). For non-legacy drivers the global lock only ensures ordering between drm_open and lastclose (I think at least), and between drm_dev_register/unregister and the backwards ->load/unload callbacks (which are called in the wrong place, but we cannot fix that for legacy drivers). ->load/unload should be completely unused (maybe radeon still uses it), and ->lastclose is also on the decline. Ah ok got it, I'll change the check back to drm_core_check_feature(dev, DRIVER_LEGACY) then. Maybe we should update the comment of drm_global_mutex to explain what it protects and why. The comments in drm_dev_needs_global_mutex make sense I think, I just didn't read the code closely enough. I'm also confused how this patch connects to the splat, since for i915 we Right, my bad, this is a separate instance of circular locking. I was too hasty when I saw that for legacy drivers we might grab master_rwsem then drm_global_mutex in the ioctl handler. shouldn't be taking the drm_global_lock here at all. The problem seems to be the drm_open_helper when we create a new lease, which is an entirely different can of worms. I'm honestly not sure how to best do that, but we should be able to create a file and then call drm_open_helper directly, or well a version of that which never takes the drm_global_mutex. Because that is not needed for nested d
Re: [Intel-gfx] [PATCH v3 7/9] drm: update global mutex lock in the ioctl handler
On Thu, Aug 19, 2021 at 12:53 PM Desmond Cheong Zhi Xi wrote: > > On 18/8/21 7:02 pm, Daniel Vetter wrote: > > On Wed, Aug 18, 2021 at 03:38:22PM +0800, Desmond Cheong Zhi Xi wrote: > >> In a future patch, a read lock on drm_device.master_rwsem is > >> held in the ioctl handler before the check for ioctl > >> permissions. However, this produces the following lockdep splat: > >> > >> == > >> WARNING: possible circular locking dependency detected > >> 5.14.0-rc6-CI-Patchwork_20831+ #1 Tainted: G U > >> -- > >> kms_lease/1752 is trying to acquire lock: > >> 827bad88 (drm_global_mutex){+.+.}-{3:3}, at: drm_open+0x64/0x280 > >> > >> but task is already holding lock: > >> 88812e350108 (&dev->master_rwsem){}-{3:3}, at: > >> drm_ioctl_kernel+0xfb/0x1a0 > >> > >> which lock already depends on the new lock. > >> > >> the existing dependency chain (in reverse order) is: > >> > >> -> #2 (&dev->master_rwsem){}-{3:3}: > >> lock_acquire+0xd3/0x310 > >> down_read+0x3b/0x140 > >> drm_master_internal_acquire+0x1d/0x60 > >> drm_client_modeset_commit+0x10/0x40 > >> __drm_fb_helper_restore_fbdev_mode_unlocked+0x88/0xb0 > >> drm_fb_helper_set_par+0x34/0x40 > >> intel_fbdev_set_par+0x11/0x40 [i915] > >> fbcon_init+0x270/0x4f0 > >> visual_init+0xc6/0x130 > >> do_bind_con_driver+0x1de/0x2c0 > >> do_take_over_console+0x10e/0x180 > >> do_fbcon_takeover+0x53/0xb0 > >> register_framebuffer+0x22d/0x310 > >> __drm_fb_helper_initial_config_and_unlock+0x36c/0x540 > >> intel_fbdev_initial_config+0xf/0x20 [i915] > >> async_run_entry_fn+0x28/0x130 > >> process_one_work+0x26d/0x5c0 > >> worker_thread+0x37/0x390 > >> kthread+0x13b/0x170 > >> ret_from_fork+0x1f/0x30 > >> > >> -> #1 (&helper->lock){+.+.}-{3:3}: > >> lock_acquire+0xd3/0x310 > >> __mutex_lock+0xa8/0x930 > >> __drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0xb0 > >> intel_fbdev_restore_mode+0x2b/0x50 [i915] > >> drm_lastclose+0x27/0x50 > >> drm_release_noglobal+0x42/0x60 > >> __fput+0x9e/0x250 > >> task_work_run+0x6b/0xb0 > >> exit_to_user_mode_prepare+0x1c5/0x1d0 > >> syscall_exit_to_user_mode+0x19/0x50 > >> do_syscall_64+0x46/0xb0 > >> entry_SYSCALL_64_after_hwframe+0x44/0xae > >> > >> -> #0 (drm_global_mutex){+.+.}-{3:3}: > >> validate_chain+0xb39/0x1e70 > >> __lock_acquire+0x5a1/0xb70 > >> lock_acquire+0xd3/0x310 > >> __mutex_lock+0xa8/0x930 > >> drm_open+0x64/0x280 > >> drm_stub_open+0x9f/0x100 > >> chrdev_open+0x9f/0x1d0 > >> do_dentry_open+0x14a/0x3a0 > >> dentry_open+0x53/0x70 > >> drm_mode_create_lease_ioctl+0x3cb/0x970 > >> drm_ioctl_kernel+0xc9/0x1a0 > >> drm_ioctl+0x201/0x3d0 > >> __x64_sys_ioctl+0x6a/0xa0 > >> do_syscall_64+0x37/0xb0 > >> entry_SYSCALL_64_after_hwframe+0x44/0xae > >> > >> other info that might help us debug this: > >> Chain exists of: > >>drm_global_mutex --> &helper->lock --> &dev->master_rwsem > >> Possible unsafe locking scenario: > >> CPU0CPU1 > >> > >>lock(&dev->master_rwsem); > >> lock(&helper->lock); > >> lock(&dev->master_rwsem); > >>lock(drm_global_mutex); > >> > >> *** DEADLOCK *** > >> > >> The lock hierarchy inversion happens because we grab the > >> drm_global_mutex while already holding on to master_rwsem. To avoid > >> this, we do some prep work to grab the drm_global_mutex before > >> checking for ioctl permissions. > >> > >> At the same time, we update the check for the global mutex to use the > >> drm_dev_needs_global_mutex helper function. > > > > This is intentional, essentially we force all non-legacy drivers to have > > unlocked ioctl (otherwise everyone forgets to set that flag). > > > > For non-legacy drivers the global lock only ensures ordering between > > drm_open and lastclose (I think at least), and between > > drm_dev_register/unregister and the backwards ->load/unload callbacks > > (which are called in the wrong place, but we cannot fix that for legacy > > drivers). > > > > ->load/unload should be completely unused (maybe radeon still uses it), > > and ->lastclose is also on the decline. > > > > Ah ok got it, I'll change the check back to > drm_core_check_feature(dev, DRIVER_LEGACY) then. > > > Maybe we should update the comment of drm_global_mutex to explain what it > > protects and why. > > > > The comments in drm_dev_needs_global_mutex make sense I think, I just > didn't read the code closely enough. > > > I'm also confused how this patch connects to the splat, since for i915 we >
[PATCH v2 0/5] Kconfig symbol clean-up on gpu
Dear DRM maintainers, The script ./scripts/checkkconfigsymbols.py warns on invalid references to Kconfig symbols (often, minor typos, name confusions or outdated references). This patch series addresses all issues reported by ./scripts/checkkconfigsymbols.py in ./drivers/gpu/ for Kconfig and Makefile files. Issues in the Kconfig and Makefile files indicate some shortcomings in the overall build definitions, and often are true actionable issues to address. These issues can be identified and filtered by: ./scripts/checkkconfigsymbols.py | grep -E "drivers/gpu/.*(Kconfig|Makefile)" -B 1 -A 1 After applying this patch series on linux-next (next-20210817), the command above yields just one further issues to address: DRM_AMD_DC_DCE11_0 Referencing files: drivers/gpu/drm/amd/display/dc/dce100/Makefile Conclusion: No action required. Rationale: drivers/gpu/drm/amd/display/dc/dce100/Makefile refers to DRM_AMD_DC_DCE11_0 in a comment, after an "ifdef 0". Please pick this patch series into your drm-next tree. Note that patch "drm: amdgpu: remove obsolete reference to config CHASH" was already picked and applied by Alex Deucher. Best regards, Lukas v1 -> v2: - adjusted sources in drivers/gpu/drm/Kconfig for drm: zte: remove obsolete DRM Support for ZTE SoCs after report by kernel test robot - removed Tomi from recipient list as email is unreachable. Lukas Bulwahn (5): drm: rockchip: remove reference to non-existing config DRM_RGB drm: amdgpu: remove obsolete reference to config CHASH drm: v3d: correct reference to config ARCH_BRCMSTB drm: zte: remove obsolete DRM Support for ZTE SoCs drm: omap: remove obsolete selection of OMAP2_DSS in config DRM_OMAP drivers/gpu/drm/Kconfig | 3 - drivers/gpu/drm/Makefile | 1 - drivers/gpu/drm/omapdrm/Kconfig | 1 - drivers/gpu/drm/rockchip/Kconfig | 1 - drivers/gpu/drm/v3d/Kconfig | 2 +- drivers/gpu/drm/zte/Kconfig | 10 - drivers/gpu/drm/zte/Makefile | 10 - drivers/gpu/drm/zte/zx_common_regs.h | 28 - drivers/gpu/drm/zte/zx_drm_drv.c | 184 -- drivers/gpu/drm/zte/zx_drm_drv.h | 34 - drivers/gpu/drm/zte/zx_hdmi.c| 760 -- drivers/gpu/drm/zte/zx_hdmi_regs.h | 66 -- drivers/gpu/drm/zte/zx_plane.c | 537 drivers/gpu/drm/zte/zx_plane.h | 26 - drivers/gpu/drm/zte/zx_plane_regs.h | 120 drivers/gpu/drm/zte/zx_tvenc.c | 400 drivers/gpu/drm/zte/zx_tvenc_regs.h | 27 - drivers/gpu/drm/zte/zx_vga.c | 527 --- drivers/gpu/drm/zte/zx_vga_regs.h| 33 - drivers/gpu/drm/zte/zx_vou.c | 921 --- drivers/gpu/drm/zte/zx_vou.h | 64 -- drivers/gpu/drm/zte/zx_vou_regs.h| 212 -- 22 files changed, 1 insertion(+), 3966 deletions(-) delete mode 100644 drivers/gpu/drm/zte/Kconfig delete mode 100644 drivers/gpu/drm/zte/Makefile delete mode 100644 drivers/gpu/drm/zte/zx_common_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_drm_drv.c delete mode 100644 drivers/gpu/drm/zte/zx_drm_drv.h delete mode 100644 drivers/gpu/drm/zte/zx_hdmi.c delete mode 100644 drivers/gpu/drm/zte/zx_hdmi_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_plane.c delete mode 100644 drivers/gpu/drm/zte/zx_plane.h delete mode 100644 drivers/gpu/drm/zte/zx_plane_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_tvenc.c delete mode 100644 drivers/gpu/drm/zte/zx_tvenc_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_vga.c delete mode 100644 drivers/gpu/drm/zte/zx_vga_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_vou.c delete mode 100644 drivers/gpu/drm/zte/zx_vou.h delete mode 100644 drivers/gpu/drm/zte/zx_vou_regs.h -- 2.26.2
[PATCH v2 2/5] drm: amdgpu: remove obsolete reference to config CHASH
Commit 04ed8459f334 ("drm/amdgpu: remove chash") removes the chash architecture and its corresponding config CHASH. There is still a reference to CHASH in the config DRM_AMDGPU in ./drivers/gpu/drm/Kconfig. Remove this obsolete reference to config CHASH. Signed-off-by: Lukas Bulwahn --- drivers/gpu/drm/Kconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index f3bc90baca61..8fc40317f2b7 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -257,7 +257,6 @@ config DRM_AMDGPU select HWMON select BACKLIGHT_CLASS_DEVICE select INTERVAL_TREE - select CHASH help Choose this option if you have a recent AMD Radeon graphics card. -- 2.26.2
[PATCH v2 1/5] drm: rockchip: remove reference to non-existing config DRM_RGB
commit 1f0f01515172 ("drm/rockchip: Add support for Rockchip Soc RGB output interface") accidently adds to select the non-existing config DRM_RGB in ./drivers/gpu/drm/rockchip/Kconfig. Luckily, ./scripts/checkkconfigsymbols.py warns on non-existing configs: DRM_RGB Referencing files: drivers/gpu/drm/rockchip/Kconfig So, remove the reference to the non-existing config DRM_RGB. Signed-off-by: Lukas Bulwahn --- drivers/gpu/drm/rockchip/Kconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig index 558f1b58bd69..9f1ecefc3933 100644 --- a/drivers/gpu/drm/rockchip/Kconfig +++ b/drivers/gpu/drm/rockchip/Kconfig @@ -9,7 +9,6 @@ config DRM_ROCKCHIP select DRM_ANALOGIX_DP if ROCKCHIP_ANALOGIX_DP select DRM_DW_HDMI if ROCKCHIP_DW_HDMI select DRM_DW_MIPI_DSI if ROCKCHIP_DW_MIPI_DSI - select DRM_RGB if ROCKCHIP_RGB select GENERIC_PHY if ROCKCHIP_DW_MIPI_DSI select GENERIC_PHY_MIPI_DPHY if ROCKCHIP_DW_MIPI_DSI select SND_SOC_HDMI_CODEC if ROCKCHIP_CDN_DP && SND_SOC -- 2.26.2
[PATCH v2 4/5] drm: zte: remove obsolete DRM Support for ZTE SoCs
Commit 89d4f98ae90d ("ARM: remove zte zx platform") removes the config ARCH_ZX. So, since then, the DRM Support for ZTE SoCs (config DRM_ZTE) depends on this removed config ARCH_ZX and cannot be selected. Fortunately, ./scripts/checkkconfigsymbols.py detects this and warns: ARCH_ZX Referencing files: drivers/gpu/drm/zte/Kconfig So, remove this obsolete DRM support. Signed-off-by: Lukas Bulwahn --- drivers/gpu/drm/Kconfig | 2 - drivers/gpu/drm/Makefile | 1 - drivers/gpu/drm/zte/Kconfig | 10 - drivers/gpu/drm/zte/Makefile | 10 - drivers/gpu/drm/zte/zx_common_regs.h | 28 - drivers/gpu/drm/zte/zx_drm_drv.c | 184 -- drivers/gpu/drm/zte/zx_drm_drv.h | 34 - drivers/gpu/drm/zte/zx_hdmi.c| 760 -- drivers/gpu/drm/zte/zx_hdmi_regs.h | 66 -- drivers/gpu/drm/zte/zx_plane.c | 537 drivers/gpu/drm/zte/zx_plane.h | 26 - drivers/gpu/drm/zte/zx_plane_regs.h | 120 drivers/gpu/drm/zte/zx_tvenc.c | 400 drivers/gpu/drm/zte/zx_tvenc_regs.h | 27 - drivers/gpu/drm/zte/zx_vga.c | 527 --- drivers/gpu/drm/zte/zx_vga_regs.h| 33 - drivers/gpu/drm/zte/zx_vou.c | 921 --- drivers/gpu/drm/zte/zx_vou.h | 64 -- drivers/gpu/drm/zte/zx_vou_regs.h| 212 -- 19 files changed, 3962 deletions(-) delete mode 100644 drivers/gpu/drm/zte/Kconfig delete mode 100644 drivers/gpu/drm/zte/Makefile delete mode 100644 drivers/gpu/drm/zte/zx_common_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_drm_drv.c delete mode 100644 drivers/gpu/drm/zte/zx_drm_drv.h delete mode 100644 drivers/gpu/drm/zte/zx_hdmi.c delete mode 100644 drivers/gpu/drm/zte/zx_hdmi_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_plane.c delete mode 100644 drivers/gpu/drm/zte/zx_plane.h delete mode 100644 drivers/gpu/drm/zte/zx_plane_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_tvenc.c delete mode 100644 drivers/gpu/drm/zte/zx_tvenc_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_vga.c delete mode 100644 drivers/gpu/drm/zte/zx_vga_regs.h delete mode 100644 drivers/gpu/drm/zte/zx_vou.c delete mode 100644 drivers/gpu/drm/zte/zx_vou.h delete mode 100644 drivers/gpu/drm/zte/zx_vou_regs.h diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 8fc40317f2b7..66105529873c 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -352,8 +352,6 @@ source "drivers/gpu/drm/hisilicon/Kconfig" source "drivers/gpu/drm/mediatek/Kconfig" -source "drivers/gpu/drm/zte/Kconfig" - source "drivers/gpu/drm/mxsfb/Kconfig" source "drivers/gpu/drm/meson/Kconfig" diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index ad1112154898..0dff40bb863c 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -113,7 +113,6 @@ obj-y += bridge/ obj-$(CONFIG_DRM_FSL_DCU) += fsl-dcu/ obj-$(CONFIG_DRM_ETNAVIV) += etnaviv/ obj-y += hisilicon/ -obj-$(CONFIG_DRM_ZTE) += zte/ obj-$(CONFIG_DRM_MXSFB)+= mxsfb/ obj-y += tiny/ obj-$(CONFIG_DRM_PL111) += pl111/ diff --git a/drivers/gpu/drm/zte/Kconfig b/drivers/gpu/drm/zte/Kconfig deleted file mode 100644 index aa8594190b50.. --- a/drivers/gpu/drm/zte/Kconfig +++ /dev/null @@ -1,10 +0,0 @@ -# SPDX-License-Identifier: GPL-2.0-only -config DRM_ZTE - tristate "DRM Support for ZTE SoCs" - depends on DRM && ARCH_ZX - select DRM_KMS_CMA_HELPER - select DRM_KMS_HELPER - select SND_SOC_HDMI_CODEC if SND_SOC - select VIDEOMODE_HELPERS - help - Choose this option to enable DRM on ZTE ZX SoCs. diff --git a/drivers/gpu/drm/zte/Makefile b/drivers/gpu/drm/zte/Makefile deleted file mode 100644 index b6d966d849dd.. --- a/drivers/gpu/drm/zte/Makefile +++ /dev/null @@ -1,10 +0,0 @@ -# SPDX-License-Identifier: GPL-2.0 -zxdrm-y := \ - zx_drm_drv.o \ - zx_hdmi.o \ - zx_plane.o \ - zx_tvenc.o \ - zx_vga.o \ - zx_vou.o - -obj-$(CONFIG_DRM_ZTE) += zxdrm.o diff --git a/drivers/gpu/drm/zte/zx_common_regs.h b/drivers/gpu/drm/zte/zx_common_regs.h deleted file mode 100644 index b7b996db129d.. --- a/drivers/gpu/drm/zte/zx_common_regs.h +++ /dev/null @@ -1,28 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* - * Copyright (C) 2017 Sanechips Technology Co., Ltd. - * Copyright 2017 Linaro Ltd. - */ - -#ifndef __ZX_COMMON_REGS_H__ -#define __ZX_COMMON_REGS_H__ - -/* CSC registers */ -#define CSC_CTRL0 0x30 -#define CSC_COV_MODE_SHIFT 16 -#define CSC_COV_MODE_MASK (0x << CSC_COV_MODE_SHIFT) -#define CSC_BT601_IMAGE_RGB2YCBCR 0 -#define CSC_BT601_IMAGE_YCBCR2RGB 1 -#define CSC_BT601_VIDEO_RGB2YCBCR 2 -#define CSC_BT601_VIDEO_YCBCR2RGB 3 -#define CSC_BT709_IMAGE_RGB2YCBCR 4 -#define CSC_BT709_IMAGE_YCBCR2RGB 5 -#define CS
[PATCH v2 3/5] drm: v3d: correct reference to config ARCH_BRCMSTB
Commit 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+") adds the config DRM_V3D, which depends on "ARCH_BCMSTB". Although, a bit confusing: all Broadcom architectures in ./arch/arm/mach-bcm/Kconfig have the prefix "ARCH_BCM", except for ARCH_BRCMSTB, i.e., the config for Broadcom BCM7XXX based boards. So, correct the reference ARCH_BCMSTB to the intended ARCH_BRCMSTB. Signed-off-by: Lukas Bulwahn --- drivers/gpu/drm/v3d/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/v3d/Kconfig b/drivers/gpu/drm/v3d/Kconfig index 9a5c44606337..e973ec487484 100644 --- a/drivers/gpu/drm/v3d/Kconfig +++ b/drivers/gpu/drm/v3d/Kconfig @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only config DRM_V3D tristate "Broadcom V3D 3.x and newer" - depends on ARCH_BCM || ARCH_BCMSTB || COMPILE_TEST + depends on ARCH_BCM || ARCH_BRCMSTB || COMPILE_TEST depends on DRM depends on COMMON_CLK depends on MMU -- 2.26.2
[PATCH v2 5/5] drm: omap: remove obsolete selection of OMAP2_DSS in config DRM_OMAP
Commit 55b68fb856b5 ("drm/omap: squash omapdrm sub-modules into one") removes the config OMAP2_DSS in ./drivers/gpu/drm/omapdrm/dss/Kconfig, while moving the other configs into./drivers/gpu/drm/omapdrm/Kconfig, but misses to remove an obsolete selection of OMAP2_DSS in config DRM_OMAP. Hence, ./scripts/checkkconfigsymbols.py warns: OMAP2_DSS Referencing files: drivers/gpu/drm/omapdrm/Kconfig Remove this reference in an obsolete selection. Signed-off-by: Lukas Bulwahn --- drivers/gpu/drm/omapdrm/Kconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig index e7281da5bc6a..d6e4df291d6f 100644 --- a/drivers/gpu/drm/omapdrm/Kconfig +++ b/drivers/gpu/drm/omapdrm/Kconfig @@ -3,7 +3,6 @@ config DRM_OMAP tristate "OMAP DRM" depends on DRM depends on ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM - select OMAP2_DSS select DRM_KMS_HELPER select VIDEOMODE_HELPERS select HDMI -- 2.26.2
Re: [PATCH v2 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug
On 8/19/21 11:34 AM, Matthew Auld wrote: This should give a more complete view of the various bits of internal resource manager state, for device local-memory. v2(Thomas): - Move the region printing into a nice helper Signed-off-by: Matthew Auld Cc: Thomas Hellström For the series (2/2) Reviewed-by: Thomas Hellström
Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
On Tue, Aug 17, 2021 at 11:05:26PM -0700, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memset(), avoid intentionally writing across > neighboring fields. > > Add struct_group() to mark region of struct mlx5_ib_mr that should be > initialized to zero. > > Cc: Leon Romanovsky > Cc: Doug Ledford > Cc: Jason Gunthorpe > Cc: linux-r...@vger.kernel.org > Signed-off-by: Kees Cook > --- > drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h > b/drivers/infiniband/hw/mlx5/mlx5_ib.h > index bf20a388eabe..f63bf204a7a1 100644 > --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h > @@ -644,6 +644,7 @@ struct mlx5_ib_mr { > struct ib_umem *umem; > > /* This is zero'd when the MR is allocated */ > + struct_group(cleared, > union { > /* Used only while the MR is in the cache */ > struct { > @@ -691,12 +692,13 @@ struct mlx5_ib_mr { > bool is_odp_implicit; > }; > }; > + ); > }; > > /* Zero the fields in the mr that are variant depending on usage */ > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr) > { > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out)); > + memset(&mr->cleared, 0, sizeof(mr->cleared)); > } Why not use the memset_after(mr->umem) here? Jason
Re: [v1 2/2] dt-bindings: drm/panel: boe-tv101wum-nl6: Support enabling a 3.3V rail
On Thu, 19 Aug 2021 17:29:43 +0800, yangcong wrote: > The auo,b101uan08.3 panel (already supported by this driver) has > a 3.3V rail that needs to be turned on. For previous users of > this panel this voltage was directly output by pmic. On a new > user (the not-yet-upstream sc7180-trogdor-mrbland board) we need > to turn the 3.3V rail on. > > Signed-off-by: yangcong > --- > .../devicetree/bindings/display/panel/boe,tv101wum-nl6.yaml | 4 > 1 file changed, 4 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/boe,tv101wum-nl6.example.dt.yaml: panel@0: 'pp3300-supply' is a required property From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/boe,tv101wum-nl6.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1518552 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On Wed, 18 Aug 2021 at 17:43, Dmitry Osipenko wrote: > > 18.08.2021 13:08, Ulf Hansson пишет: > > On Wed, 18 Aug 2021 at 11:50, Viresh Kumar wrote: > >> > >> On 18-08-21, 11:41, Ulf Hansson wrote: > >>> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar > >>> wrote: > What we need here is just configure. So something like this then: > > - genpd->get_performance_state() > -> dev_pm_opp_get_current_opp() //New API > -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate); > > This can be done just once from probe() then. > >>> > >>> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion? > >> > >> The opp core already has a way of finding current OPP, that's what > >> Dmitry is trying to use here. It finds it using clk_get_rate(), if > >> that is zero, it picks the lowest freq possible. > >> > >>> I am sure I understand the problem. When a device is getting probed, > >>> it needs to consume power, how else can the corresponding driver > >>> successfully probe it? > >> > >> Dmitry can answer that better, but a device doesn't necessarily need > >> to consume energy in probe. It can consume bus clock, like APB we > >> have, but the more energy consuming stuff can be left disabled until > >> the time a user comes up. Probe will just end up registering the > >> driver and initializing it. > > > > That's perfectly fine, as then it's likely that it won't vote for an > > OPP, but can postpone that as well. > > > > Perhaps the problem is rather that the HW may already carry a non-zero > > vote made from a bootloader. If the consumer driver tries to clear > > that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would > > still not lead to any updates of the performance state in genpd, > > because genpd internally has initialized the performance-state to > > zero. > > We don't need to discover internal SoC devices because we use > device-tree on ARM. For most devices power isn't required at a probe > time because probe function doesn't touch h/w at all, thus devices are > left in suspended state after probe. > > We have three components comprising PM on Tegra: > > 1. Power gate > 2. Clock state > 3. Voltage state > > GENPD on/off represents the 'power gate'. > > Clock and reset are controlled by device drivers using clk and rst APIs. > > Voltage state is represented by GENPD's performance level. > > GENPD core assumes that at a first rpm-resume of a consumer device, its > genpd_performance=0. Not true for Tegra because h/w of the device is > preconfigured to a non-zero perf level initially, h/w may not support > zero level at all. I think you may be misunderstanding genpd's behaviour around this, but let me elaborate. In genpd_runtime_resume(), we try to restore the performance state for the device that genpd_runtime_suspend() *may* have dropped earlier. That means, if genpd_runtime_resume() is called prior genpd_runtime_suspend() for the first time, it means that genpd_runtime_resume() will *not* restore a performance state, but instead just leave the performance state as is for the device (see genpd_restore_performance_state()). In other words, a consumer driver may use the following sequence to set an initial performance state for the device during ->probe(): ... rate = clk_get_rate() dev_pm_opp_set_rate(rate) pm_runtime_enable() pm_runtime_resume_and_get() ... Note that, it's the consumer driver's responsibility to manage device specific resources, in its ->runtime_suspend|resume() callbacks. Typically that means dealing with clock gating/ungating, for example. In the other scenario where a consumer driver prefers to *not* call pm_runtime_resume_and_get() in its ->probe(), because it doesn't need to power on the device to complete probing, then we don't want to vote for an OPP at all - and we also want the performance state for the device in genpd to be set to zero. Correct? Is this the main problem you are trying to solve, because I think this doesn't work out of the box as of today? There is another concern though, but perhaps it's not a problem after all. Viresh told us that dev_pm_opp_set_rate() may turn on resources like clock/regulators. That could certainly be problematic, in particular if the device and its genpd have OPP tables associated with it and the consumer driver wants to follow the above sequence in probe. Viresh, can you please chime in here and elaborate on some of the magic happening behind dev_pm_opp_set_rate() API - is there a problem here or not? > > GENPD core assumes that consumer devices can work at any performance > level. Not true for Tegra because voltage needs to be set in accordance > to the clock rate before clock is enabled, otherwise h/w won't work > properly, perhaps clock may be unstable or h/w won't be latching. Correct. Genpd relies on the callers to use the OPP framework if there are constraints like you describe above. That said, it's not forbidden for a consumer driver to call dev_pm_genpd_set
Re: [PATCH v8 19/34] pwm: tegra: Add runtime PM and OPP support
On Tue, Aug 17, 2021 at 04:27:39AM +0300, Dmitry Osipenko wrote: > The PWM on Tegra belongs to the core power domain and we're going to > enable GENPD support for the core domain. Now PWM must be resumed using > runtime PM API in order to initialize the PWM power state. The PWM clock > rate must be changed using OPP API that will reconfigure the power domain > performance state in accordance to the rate. Add runtime PM and OPP > support to the PWM driver. > > Signed-off-by: Dmitry Osipenko > --- > drivers/pwm/pwm-tegra.c | 104 > 1 file changed, 85 insertions(+), 19 deletions(-) Can this be safely applied independently of the rest of the series, or are there any dependencies on earlier patches? Thierry signature.asc Description: PGP signature
[PATCH v1] drm/msm/dpu: Fix address of SM8150 PINGPONG5 IRQ register
Both PINGPONG4 and PINGPONG5 IRQ registers are using the same address, which is incorrect. PINGPONG4 should use the register offset 30, and PINGPONG5 should use the register offset 31 according to the downstream driver. Fixes: 667e9985ee24 ("drm/msm/dpu: replace IRQ lookup with the data in hw catalog") Signed-off-by: Robert Foss --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c index 2e482cdd7b3c5..420d78cfce8af 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c @@ -794,7 +794,7 @@ static const struct dpu_pingpong_cfg sm8150_pp[] = { DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30), -1), PP_BLK("pingpong_5", PINGPONG_5, 0x72800, MERGE_3D_2, sdm845_pp_sblk, - DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30), + DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 31), -1), }; -- 2.30.2
[PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
From: Colin Ian King Don't populate the array ext_div on the stack but instead it static const. Makes the object code smaller by 118 bytes: Before: textdatabss dechex filename 39449 17500128 57077 def5 ./drivers/gpu/drm/bridge/tc358767.o After: textdatabss dechex filename 39235 17596128 56959 de7f ./drivers/gpu/drm/bridge/tc358767.o (gcc version 10.3.0) Signed-off-by: Colin Ian King --- drivers/gpu/drm/bridge/tc358767.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 23a6f90b694b..599c23759400 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock) int div, best_div = 1; int mul, best_mul = 1; int delta, best_delta; - int ext_div[] = {1, 2, 3, 5, 7}; + static const int ext_div[] = {1, 2, 3, 5, 7}; int best_pixelclock = 0; int vco_hi = 0; u32 pxl_pllparam; -- 2.32.0
[PATCH 2/2] drm/vc4: hdmi: Actually check for the connector status in hotplug
The drm_helper_hpd_irq_event() documentation states that this function is "useful for drivers which can't or don't track hotplug interrupts for each connector." and that "Drivers which support hotplug interrupts for each connector individually and which have a more fine-grained detect logic should bypass this code and directly call drm_kms_helper_hotplug_event()". This is thus what we ended-up doing. However, what this actually means, and is further explained in the drm_kms_helper_hotplug_event() documentation, is that drm_kms_helper_hotplug_event() should be called by drivers that can track the connection status change, and if it has changed we should call that function. This underlying expectation we failed to provide is that the caller of drm_kms_helper_hotplug_event() should call drm_helper_probe_detect() to probe the new status of the connector. Since we didn't do it, it meant that even though we were sending the notification to user-space and the DRM clients that something changed we never probed or updated our internal connector status ourselves. This went mostly unnoticed since the detect callback usually doesn't have any side-effect. Also, if we were using the DRM fbdev emulation (which is a DRM client), or any user-space application that can deal with hotplug events, chances are they would react to the hotplug event by probing the connector status eventually. However, now that we have to enable the scrambler in detect() if it was enabled it has a side effect, and an application such as Kodi or modetest doesn't deal with hotplug events. This resulted with a black screen when Kodi or modetest was running when a screen was disconnected and then reconnected, or switched off and on. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index a3dbd1fdff7d..d9e001b9314f 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1578,10 +1578,11 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) static irqreturn_t vc4_hdmi_hpd_irq_thread(int irq, void *priv) { struct vc4_hdmi *vc4_hdmi = priv; - struct drm_device *dev = vc4_hdmi->connector.dev; + struct drm_connector *connector = &vc4_hdmi->connector; + struct drm_device *dev = connector->dev; if (dev && dev->registered) - drm_kms_helper_hotplug_event(dev); + drm_connector_helper_hpd_irq_event(connector); return IRQ_HANDLED; } -- 2.31.1
[PATCH 1/2] drm/probe-helper: Create a HPD IRQ event helper for a single connector
The drm_helper_hpd_irq_event() function is iterating over all the connectors when an hotplug event is detected. During that iteration, it will call each connector detect function and figure out if its status changed. Finally, if any connector changed, it will notify the user-space and the clients that something changed on the DRM device. This is supposed to be used for drivers that don't have a hotplug interrupt for individual connectors. However, drivers that can use an interrupt for a single connector are left in the dust and can either reimplement the logic used during the iteration for each connector or use that helper and iterate over all connectors all the time. Since both are suboptimal, let's create a helper that will only perform the status detection on a single connector. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/drm_probe_helper.c | 105 - include/drm/drm_probe_helper.h | 1 + 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 5606bca3caa8..7e3cbb4333ce 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -795,6 +795,77 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_fini); +static bool +_drm_connector_helper_hpd_irq_event(struct drm_connector *connector, + bool notify) +{ + struct drm_device *dev = connector->dev; + enum drm_connector_status old_status; + u64 old_epoch_counter; + bool changed = false; + + /* Only handle HPD capable connectors. */ + drm_WARN_ON(dev, !(connector->polled & DRM_CONNECTOR_POLL_HPD)); + + drm_WARN_ON(dev, !mutex_is_locked(&dev->mode_config.mutex)); + + old_status = connector->status; + old_epoch_counter = connector->epoch_counter; + + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Old epoch counter %llu\n", + connector->base.id, + connector->name, + old_epoch_counter); + + connector->status = drm_helper_probe_detect(connector, NULL, + false); + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", + connector->base.id, + connector->name, + drm_get_connector_status_name(old_status), + drm_get_connector_status_name(connector->status)); + + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] New epoch counter %llu\n", + connector->base.id, + connector->name, + connector->epoch_counter); + + /* +* Check if epoch counter had changed, meaning that we need +* to send a uevent. +*/ + if (old_epoch_counter != connector->epoch_counter) + changed = true; + + if (changed && notify) { + drm_kms_helper_hotplug_event(dev); + DRM_DEBUG_KMS("Sent hotplug event\n"); + } + + return changed; +} + +/** + * drm_connector_helper_hpd_irq_event - hotplug processing + * @connector: drm_connector + * + * Drivers can use this helper function to run a detect cycle on a connector + * which has the DRM_CONNECTOR_POLL_HPD flag set in its &polled member. + * + * This helper function is useful for drivers which can track hotplug + * interrupts for a single connector. + * + * This function must be called with the mode setting locks held. + * + * Note that a connector can be both polled and probed from the hotplug + * handler, in case the hotplug interrupt is known to be unreliable. + */ +bool drm_connector_helper_hpd_irq_event(struct drm_connector *connector) +{ + return _drm_connector_helper_hpd_irq_event(connector, true); +} +EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); + /** * drm_helper_hpd_irq_event - hotplug processing * @dev: drm_device @@ -822,9 +893,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) { struct drm_connector *connector; struct drm_connector_list_iter conn_iter; - enum drm_connector_status old_status; bool changed = false; - u64 old_epoch_counter; if (!dev->mode_config.poll_enabled) return false; @@ -832,37 +901,9 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) mutex_lock(&dev->mode_config.mutex); drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(connector, &conn_iter) { - /* Only handle HPD capable connectors. */ - if (!(connector->polled & DRM_CONNECTOR_POLL_HPD)) - continue; - - old_status = connector->status; - - old_epoch_counter = connector->epoch_counter; - - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Old epoch counter %llu\n", connector->base.id, - connector->name,
Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote: > From: Colin Ian King > > Don't populate the array ext_div on the stack but instead it > static const. Makes the object code smaller by 118 bytes: > > Before: > textdatabss dechex filename > 39449 17500128 57077 def5 ./drivers/gpu/drm/bridge/tc358767.o > > After: > textdatabss dechex filename > 39235 17596128 56959 de7f ./drivers/gpu/drm/bridge/tc358767.o Why is text smaller and data larger with this change? > > (gcc version 10.3.0) > > Signed-off-by: Colin Ian King > --- > drivers/gpu/drm/bridge/tc358767.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c > b/drivers/gpu/drm/bridge/tc358767.c > index 23a6f90b694b..599c23759400 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, > u32 pixelclock) > int div, best_div = 1; > int mul, best_mul = 1; > int delta, best_delta; > - int ext_div[] = {1, 2, 3, 5, 7}; > + static const int ext_div[] = {1, 2, 3, 5, 7}; > int best_pixelclock = 0; > int vco_hi = 0; > u32 pxl_pllparam;
Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
On 19/08/2021 14:51, Joe Perches wrote: > On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote: >> From: Colin Ian King >> >> Don't populate the array ext_div on the stack but instead it >> static const. Makes the object code smaller by 118 bytes: >> >> Before: >> textdatabss dechex filename >> 39449 17500128 57077 def5 ./drivers/gpu/drm/bridge/tc358767.o >> >> After: >> textdatabss dechex filename >> 39235 17596128 56959 de7f ./drivers/gpu/drm/bridge/tc358767.o > > Why is text smaller and data larger with this change? There are less instructions being used with the change since it's not shoving the array data onto the stack at run time. Instead the array is being stored in the data section and there is less object code required to access the data. Colin > >> >> (gcc version 10.3.0) >> >> Signed-off-by: Colin Ian King >> --- >> drivers/gpu/drm/bridge/tc358767.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c >> b/drivers/gpu/drm/bridge/tc358767.c >> index 23a6f90b694b..599c23759400 100644 >> --- a/drivers/gpu/drm/bridge/tc358767.c >> +++ b/drivers/gpu/drm/bridge/tc358767.c >> @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, >> u32 pixelclock) >> int div, best_div = 1; >> int mul, best_mul = 1; >> int delta, best_delta; >> -int ext_div[] = {1, 2, 3, 5, 7}; >> +static const int ext_div[] = {1, 2, 3, 5, 7}; >> int best_pixelclock = 0; >> int vco_hi = 0; >> u32 pxl_pllparam; > >
[PATCH v3 0/6] drm/vc4: hdmi: Fix CEC access while disabled
Hi, This series aims at fixing a complete and silent hang when one tries to use CEC while the display output is off. This can be tested with: echo off > /sys/class/drm/card0-HDMI-A-1/status cec-ctl --tuner -p 1.0.0.0 cec-compliance This series addresses it by making sure the HDMI controller is powered up as soon as the CEC device is opened by the userspace. Let me know what you think, Maxime Changes from v2: - Rebased on top of drm-misc-fixes - Fixed a build error Changes from v1: - More fixes - Added a big warning if we try to access a register while the device is disabled. - Fixed the pre_crtc_configure error path Maxime Ripard (6): drm/vc4: select PM drm/vc4: hdmi: Make sure the controller is powered up during bind drm/vc4: hdmi: Rework the pre_crtc_configure error handling drm/vc4: hdmi: Split the CEC disable / enable functions in two drm/vc4: hdmi: Make sure the device is powered with CEC drm/vc4: hdmi: Warn if we access the controller while disabled drivers/gpu/drm/vc4/Kconfig | 1 + drivers/gpu/drm/vc4/vc4_hdmi.c | 125 ++-- drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 6 ++ 3 files changed, 90 insertions(+), 42 deletions(-) -- 2.31.1
[PATCH v3 1/6] drm/vc4: select PM
We already depend on runtime PM to get the power domains and clocks for most of the devices supported by the vc4 driver, so let's just select it to make sure it's there, and remove the ifdef. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/Kconfig| 1 + drivers/gpu/drm/vc4/vc4_hdmi.c | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig index 118e8a426b1a..f774ab340863 100644 --- a/drivers/gpu/drm/vc4/Kconfig +++ b/drivers/gpu/drm/vc4/Kconfig @@ -9,6 +9,7 @@ config DRM_VC4 select DRM_KMS_CMA_HELPER select DRM_GEM_CMA_HELPER select DRM_PANEL_BRIDGE + select PM select SND_PCM select SND_PCM_ELD select SND_SOC_GENERIC_DMAENGINE_PCM diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index c2876731ee2d..602203b2d8e1 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2107,7 +2107,6 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) return 0; } -#ifdef CONFIG_PM static int vc4_hdmi_runtime_suspend(struct device *dev) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); @@ -2128,7 +2127,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev) return 0; } -#endif static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) { -- 2.31.1
[PATCH v3 2/6] drm/vc4: hdmi: Make sure the controller is powered up during bind
In the bind hook, we actually need the device to have the HSM clock running during the final part of the display initialisation where we reset the controller and initialise the CEC component. Failing to do so will result in a complete, silent, hang of the CPU. Fixes: 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to runtime_pm") Reviewed-by: Dave Stevenson Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 602203b2d8e1..5dde3e5c1d7f 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2191,6 +2191,18 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi->disable_4kp60 = true; } + /* +* We need to have the device powered up at this point to call +* our reset hook and for the CEC init. +*/ + ret = vc4_hdmi_runtime_resume(dev); + if (ret) + goto err_put_ddc; + + pm_runtime_get_noresume(dev); + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + if (vc4_hdmi->variant->reset) vc4_hdmi->variant->reset(vc4_hdmi); @@ -2202,8 +2214,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) clk_prepare_enable(vc4_hdmi->pixel_bvb_clock); } - pm_runtime_enable(dev); - drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); drm_encoder_helper_add(encoder, &vc4_hdmi_encoder_helper_funcs); @@ -2223,6 +2233,8 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi_debugfs_regs, vc4_hdmi); + pm_runtime_put_sync(dev); + return 0; err_free_cec: @@ -2231,6 +2243,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi_connector_destroy(&vc4_hdmi->connector); err_destroy_encoder: drm_encoder_cleanup(encoder); + pm_runtime_put_sync(dev); pm_runtime_disable(dev); err_put_ddc: put_device(&vc4_hdmi->ddc->dev); -- 2.31.1
[PATCH v3 3/6] drm/vc4: hdmi: Rework the pre_crtc_configure error handling
Since our pre_crtc_configure hook returned void, we didn't implement a goto-based error path handling, leading to errors like failing to put back the device in pm_runtime in all the error paths, but also failing to disable the pixel clock if clk_set_min_rate on the HSM clock fails. Move to a goto-based implementation to have an easier consitency. Fixes: 4f6e3d66ac52 ("drm/vc4: Add runtime PM support to the HDMI encoder driver") Reviewed-by: Dave Stevenson Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 21 ++--- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 5dde3e5c1d7f..8458f38e2883 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -913,13 +913,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate); if (ret) { DRM_ERROR("Failed to set pixel clock rate: %d\n", ret); - return; + goto err_put_runtime_pm; } ret = clk_prepare_enable(vc4_hdmi->pixel_clock); if (ret) { DRM_ERROR("Failed to turn on pixel clock: %d\n", ret); - return; + goto err_put_runtime_pm; } /* @@ -942,7 +942,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate); if (ret) { DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); - return; + goto err_disable_pixel_clock; } vc4_hdmi_cec_update_clk_div(vc4_hdmi); @@ -957,15 +957,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate); if (ret) { DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret); - clk_disable_unprepare(vc4_hdmi->pixel_clock); - return; + goto err_disable_pixel_clock; } ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock); if (ret) { DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret); - clk_disable_unprepare(vc4_hdmi->pixel_clock); - return; + goto err_disable_pixel_clock; } if (vc4_hdmi->variant->phy_init) @@ -978,6 +976,15 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, if (vc4_hdmi->variant->set_timings) vc4_hdmi->variant->set_timings(vc4_hdmi, conn_state, mode); + + return; + +err_disable_pixel_clock: + clk_disable_unprepare(vc4_hdmi->pixel_clock); +err_put_runtime_pm: + pm_runtime_put(&vc4_hdmi->pdev->dev); + + return; } static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, -- 2.31.1
[PATCH v3 4/6] drm/vc4: hdmi: Split the CEC disable / enable functions in two
In order to ease further additions to the CEC enable and disable, let's split the function into two functions, one to enable and the other to disable. Reviewed-by: Dave Stevenson Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 73 -- 1 file changed, 44 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 8458f38e2883..bfa35e32052f 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1740,7 +1740,7 @@ static irqreturn_t vc4_cec_irq_handler(int irq, void *priv) return ret; } -static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable) +static int vc4_hdmi_cec_enable(struct cec_adapter *adap) { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); /* clock period in microseconds */ @@ -1753,38 +1753,53 @@ static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable) val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) | ((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT); - if (enable) { - HDMI_WRITE(HDMI_CEC_CNTRL_5, val | - VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET); - HDMI_WRITE(HDMI_CEC_CNTRL_5, val); - HDMI_WRITE(HDMI_CEC_CNTRL_2, - ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) | - ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) | - ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) | - ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) | - ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT)); - HDMI_WRITE(HDMI_CEC_CNTRL_3, - ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) | - ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) | - ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) | - ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT)); - HDMI_WRITE(HDMI_CEC_CNTRL_4, - ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) | - ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) | - ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) | - ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT)); + HDMI_WRITE(HDMI_CEC_CNTRL_5, val | + VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET); + HDMI_WRITE(HDMI_CEC_CNTRL_5, val); + HDMI_WRITE(HDMI_CEC_CNTRL_2, + ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) | + ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) | + ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) | + ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) | + ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT)); + HDMI_WRITE(HDMI_CEC_CNTRL_3, + ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) | + ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) | + ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) | + ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT)); + HDMI_WRITE(HDMI_CEC_CNTRL_4, + ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) | + ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) | + ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) | + ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT)); + + if (!vc4_hdmi->variant->external_irq_controller) + HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC); - if (!vc4_hdmi->variant->external_irq_controller) - HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC); - } else { - if (!vc4_hdmi->variant->external_irq_controller) - HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC); - HDMI_WRITE(HDMI_CEC_CNTRL_5, val | - VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET); - } return 0; } +static int vc4_hdmi_cec_disable(struct cec_adapter *adap) +{ + struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); + + if (!vc4_hdmi->variant->external_irq_controller) + HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC); + + HDMI_WRITE(HDMI_CEC_CNTRL_5, HDMI_READ(HDMI_CEC_CNTRL_5) | + VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET); + + return 0; +} + +static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable) +{ + if (enable) + return vc4_hdmi_cec_enable(adap); + else + return vc4_hdmi_cec_dis
[PATCH v3 5/6] drm/vc4: hdmi: Make sure the device is powered with CEC
Similarly to what we encountered with the detect hook with DRM, nothing actually prevents any of the CEC callback from being run while the HDMI output is disabled. However, this is an issue since any register access to the controller when it's powered down will result in a silent hang. Let's make sure we run the runtime_pm hooks when the CEC adapter is opened and closed by the userspace to avoid that issue. Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support") Reviewed-by: Dave Stevenson Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index bfa35e32052f..53647ce902ae 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1745,8 +1745,14 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap) struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); /* clock period in microseconds */ const u32 usecs = 100 / CEC_CLOCK_FREQ; - u32 val = HDMI_READ(HDMI_CEC_CNTRL_5); + u32 val; + int ret; + ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev); + if (ret) + return ret; + + val = HDMI_READ(HDMI_CEC_CNTRL_5); val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET | VC4_HDMI_CEC_CNT_TO_4700_US_MASK | VC4_HDMI_CEC_CNT_TO_4500_US_MASK); @@ -1789,6 +1795,8 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap) HDMI_WRITE(HDMI_CEC_CNTRL_5, HDMI_READ(HDMI_CEC_CNTRL_5) | VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET); + pm_runtime_put(&vc4_hdmi->pdev->dev); + return 0; } -- 2.31.1
[PATCH v3 6/6] drm/vc4: hdmi: Warn if we access the controller while disabled
We've had many silent hangs where the kernel would look like it just stalled due to the access to one of the HDMI registers while the controller was disabled. Add a warning if we're about to do that so that it's at least not silent anymore. Reviewed-by: Dave Stevenson Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h index 19d2fdc446bc..99dde6e06a37 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h @@ -1,6 +1,8 @@ #ifndef _VC4_HDMI_REGS_H_ #define _VC4_HDMI_REGS_H_ +#include + #include "vc4_hdmi.h" #define VC4_HDMI_PACKET_STRIDE 0x24 @@ -412,6 +414,8 @@ static inline u32 vc4_hdmi_read(struct vc4_hdmi *hdmi, const struct vc4_hdmi_variant *variant = hdmi->variant; void __iomem *base; + WARN_ON(!pm_runtime_active(&hdmi->pdev->dev)); + if (reg >= variant->num_registers) { dev_warn(&hdmi->pdev->dev, "Invalid register ID %u\n", reg); @@ -438,6 +442,8 @@ static inline void vc4_hdmi_write(struct vc4_hdmi *hdmi, const struct vc4_hdmi_variant *variant = hdmi->variant; void __iomem *base; + WARN_ON(!pm_runtime_active(&hdmi->pdev->dev)); + if (reg >= variant->num_registers) { dev_warn(&hdmi->pdev->dev, "Invalid register ID %u\n", reg); -- 2.31.1
Re: [PATCH v8 19/34] pwm: tegra: Add runtime PM and OPP support
On Thu, 19 Aug 2021 at 15:21, Thierry Reding wrote: > > On Tue, Aug 17, 2021 at 04:27:39AM +0300, Dmitry Osipenko wrote: > > The PWM on Tegra belongs to the core power domain and we're going to > > enable GENPD support for the core domain. Now PWM must be resumed using > > runtime PM API in order to initialize the PWM power state. The PWM clock > > rate must be changed using OPP API that will reconfigure the power domain > > performance state in accordance to the rate. Add runtime PM and OPP > > support to the PWM driver. > > > > Signed-off-by: Dmitry Osipenko > > --- > > drivers/pwm/pwm-tegra.c | 104 > > 1 file changed, 85 insertions(+), 19 deletions(-) > > Can this be safely applied independently of the rest of the series, or > are there any dependencies on earlier patches? Just to make sure we don't rush something in, I would rather withhold all runtime PM related patches in the series, until we have agreed on how to fix the in genpd/opp core parts. Simply, because those may very well affect the deployments in the drivers. > > Thierry Kind regards Uffe
[PATCH] drm/vc4: hdmi: Remove unused struct
Commitc7d30623540b ("drm/vc4: hdmi: Remove unused struct") removed the references to the vc4_hdmi_audio_widgets and vc4_hdmi_audio_routes structures, but not the structures themselves resulting in two warnings. Remove it. Fixes: c7d30623540b ("drm/vc4: hdmi: Remove unused struct") Reported-by: kernel test robot Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 8 1 file changed, 8 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b7dc32a0c9bb..1e2d976e8736 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1403,14 +1403,6 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, return 0; } -static const struct snd_soc_dapm_widget vc4_hdmi_audio_widgets[] = { - SND_SOC_DAPM_OUTPUT("TX"), -}; - -static const struct snd_soc_dapm_route vc4_hdmi_audio_routes[] = { - { "TX", NULL, "Playback" }, -}; - static const struct snd_soc_component_driver vc4_hdmi_audio_cpu_dai_comp = { .name = "vc4-hdmi-cpu-dai-component", }; -- 2.31.1
Re: [PATCH v5, 00/15] Using component framework to support multi hardware decode
On Thu, 19 Aug 2021 at 04:13, yunfei.d...@mediatek.com wrote: > > Hi Ezequiel, > > Thanks for your suggestion. > > On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote: > > +danvet > > > > Hi, > > > > On Tue, 10 Aug 2021 at 23:58, Yunfei Dong > > wrote: > > > > > > This series adds support for multi hardware decode into mtk-vcodec, > > > by first > > > adding component framework to manage each hardware information: > > > interrupt, > > > clock, register bases and power. Secondly add core thread to deal > > > with core > > > hardware message, at the same time, add msg queue for different > > > hardware > > > share messages. Lastly, the architecture of different specs are not > > > the same, > > > using specs type to separate them. > > > > > > > I don't think it's a good idea to introduce the component API in the > > media subsystem. It doesn't seem to be maintained, IRC there's not > > even > > a maintainer for it, and it has some issues that were never > > addressed. > > > > It would be really important to avoid it. Is it really needed in the > > first place? > > > > Thanks, > > Ezequiel > > For there are many hardware need to use, mt8192 is three and mt8195 is > five. Maybe need more to be used in the feature. > > Each hardware has independent clk/power/iommu port/irq. > Use component interface in prob to get each component's information. > Just enable the hardware when need to use it, very convenient and > simple. > > I found that there are many modules use component to manage hardware > information, such as iommu and drm etc. > Many drivers support multiple hardware variants, where each variant has a different number of clocks or interrupts, see for instance struct hantro_variant which allows to expose different codec cores, some having both decoder/encoder, and some having just a decoder. The component API is mostly used by DRM to aggregate independent subdevices (called components) into an aggregated driver. For instance, a DRM driver needs to glue together the HDMI, MIPI, and plany controller, or any other hardware arrangement where devices can be described independently. The component API may look simple but has some issues, it's not easy to debug, and can cause troubles if not used as expected [1]. It's worth making sure you actually need a framework to glue different devices together. > Do you have any other suggestion for this architecture? > Looking at the different patchsets that are posted, it's not clear to me what exactly are the different architectures that you intend to support, can you some documentation which clarifies that? Thanks, Ezequiel [1] https://patchwork.kernel.org/project/linux-rockchip/cover/20200120170602.3832-1-ezequ...@collabora.com/
Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
On Thu, 2021-08-19 at 14:54 +0100, Colin Ian King wrote: > On 19/08/2021 14:51, Joe Perches wrote: > > On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote: > > > From: Colin Ian King > > > > > > Don't populate the array ext_div on the stack but instead it > > > static const. Makes the object code smaller by 118 bytes: > > > > > > Before: > > > textdatabss dechex filename > > > 39449 17500128 57077 def5 ./drivers/gpu/drm/bridge/tc358767.o > > > > > > After: > > > textdatabss dechex filename > > > 39235 17596128 56959 de7f ./drivers/gpu/drm/bridge/tc358767.o > > > > Why is text smaller and data larger with this change? > > There are less instructions being used with the change since it's not > shoving the array data onto the stack at run time. Instead the array is > being stored in the data section and there is less object code required > to access the data. Ah. It's really because it's not a minimal compilation ala defconfig. I think you should really stop making these size comparisons with .config uses that are not based on a defconfig as a whole lot of other things are going on. Please notice that the object sizes are significantly smaller below: So with an x86-64 defconfig and this compilation unit enabled with CONFIG_OF enabled and CONFIG_DRM_TOSHIBA_TC358767=y, with gcc 10.3 and this change the object size actually increases a bit. $ size drivers/gpu/drm/bridge/tc358767.o* textdata bss dec hex filename 13554 268 1 1382335ff drivers/gpu/drm/bridge/tc358767.o.new 13548 268 1 1381735f9 drivers/gpu/drm/bridge/tc358767.o.old objdump -h shows these differences: .old: 0 .text 1e1f 0040 2**4 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE [...] 14 .rodata 05ae 46e0 2**5 CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA .new: 0 .text 1e05 0040 2**4 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE [...] 11 .rodata 05ce 4600 2**5 CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA cheers, Joe
Re: refactor the i915 GVT support
Quoting Zhenyu Wang (2021-08-19 11:29:29) > On 2021.08.17 13:22:03 +0800, Zhenyu Wang wrote: > > > On 2021.08.16 19:34:58 +0200, Christoph Hellwig wrote: > > > > Any updates on this? I'd really hate to miss this merge window. > > > > > > I'm still waiting for our validation team's report on this. I'm afraid > > > it might be missing for next version as i915 merge window is mostly > > > till rc5...and for any change outside of gvt, it still needs to be > > > acked by i915 maintainers. > > > > Looks our validation team did have problem against recent i915 change. > > If you like to try, we have a gvt-staging branch on > > https://github.com/intel/gvt-linux which is generated against drm-tip > > with gvt changes for testing, currently it's broken. > > > > One issue is with i915 export that intel_context_unpin has been > > changed into static inline function. Another is that intel_gvt.c > > should be part of i915 for gvt interface instead of depending on KVMGT > > config. > > I'm working on below patch to resolve this. But I met a weird issue in > case when building i915 as module and also kvmgt module, it caused > busy wait on request_module("kvmgt") when boot, it doesn't happen if > building i915 into kernel. I'm not sure what could be the reason? > > > But the problem I see is that after moving gvt device model (gvt/*.c > > except kvmgt.c) into kvmgt module, we'll have issue with initial mmio > > state which current gvt relies on, that is in design supposed to get > > initial HW state before i915 driver has taken any operation. As mentioned in some past discussions, I think it would be best rely on golden MMIO located in /lib/firmware or elsewhere. This way we will better isolate the guest system from host system updates/changes. This should also hopefully allow enabling kvmgt module after i915 has already loaded, as the initialization would not be conditional to capture the MMIO. Regards, Joonas > > Before > > we can ensure that, I think we may only remove MPT part first but > > still keep gvt device model as part of i915 with config. I'll try to > > split that out. > > Sorry I misread the code that as we always request kvmgt module when > gvt init, so it should still apply original method that this isn't a > problem. Our current validation result has shown no regression as well. > > ---8<--- > From 58ff84572f1a0f9d79ca1d7ec0cff5ecbe78d280 Mon Sep 17 00:00:00 2001 > From: Zhenyu Wang > Date: Thu, 19 Aug 2021 16:36:33 +0800 > Subject: [PATCH] TESTONLY:drm/i915/gvt: potential fix for refactor against > current tip > > --- > drivers/gpu/drm/i915/Makefile | 4 +++- > drivers/gpu/drm/i915/gt/intel_context.c | 5 + > drivers/gpu/drm/i915/gt/intel_context.h | 3 ++- > drivers/gpu/drm/i915/i915_trace.h | 1 + > 4 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index c4f953837f72..2248574428a1 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -296,7 +296,9 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \ > > # virtual gpu code > i915-y += i915_vgpu.o > -i915-$(CONFIG_DRM_I915_GVT_KVMGT) += intel_gvt.o > +ifneq ($(CONFIG_DRM_I915_GVT_KVMGT),) > +i915-y += intel_gvt.o > +endif > > kvmgt-y += gvt/kvmgt.o \ > gvt/gvt.o \ > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c > b/drivers/gpu/drm/i915/gt/intel_context.c > index 745e84c72c90..20e7522fed84 100644 > --- a/drivers/gpu/drm/i915/gt/intel_context.c > +++ b/drivers/gpu/drm/i915/gt/intel_context.c > @@ -328,6 +328,11 @@ void __intel_context_do_unpin(struct intel_context *ce, > int sub) > intel_context_put(ce); > } > > +void intel_context_unpin(struct intel_context *ce) > +{ > + _intel_context_unpin(ce); > +} > + > static void __intel_context_retire(struct i915_active *active) > { > struct intel_context *ce = container_of(active, typeof(*ce), active); > diff --git a/drivers/gpu/drm/i915/gt/intel_context.h > b/drivers/gpu/drm/i915/gt/intel_context.h > index c41098950746..f942cbf6300a 100644 > --- a/drivers/gpu/drm/i915/gt/intel_context.h > +++ b/drivers/gpu/drm/i915/gt/intel_context.h > @@ -131,7 +131,7 @@ static inline void > intel_context_sched_disable_unpin(struct intel_context *ce) > __intel_context_do_unpin(ce, 2); > } > > -static inline void intel_context_unpin(struct intel_context *ce) > +static inline void _intel_context_unpin(struct intel_context *ce) > { > if (!ce->ops->sched_disable) { > __intel_context_do_unpin(ce, 1); > @@ -150,6 +150,7 @@ static inline void intel_context_unpin(struct > intel_context *ce) > } > } > } > +void intel_context_unpin(struct intel_context *ce); > > void intel_context_enter_engine(struct intel_context *ce); > void intel_context_exit_engine(struct intel_context *ce); > diff --git a/drivers/gpu/drm/i915/i915_trace.h > b/drivers/gpu/drm/i915/i915_trace.h > index 806ad688274
Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
On 19/08/2021 15:40, Joe Perches wrote: > On Thu, 2021-08-19 at 14:54 +0100, Colin Ian King wrote: >> On 19/08/2021 14:51, Joe Perches wrote: >>> On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote: From: Colin Ian King Don't populate the array ext_div on the stack but instead it static const. Makes the object code smaller by 118 bytes: Before: textdatabss dechex filename 39449 17500128 57077 def5 ./drivers/gpu/drm/bridge/tc358767.o After: textdatabss dechex filename 39235 17596128 56959 de7f ./drivers/gpu/drm/bridge/tc358767.o >>> >>> Why is text smaller and data larger with this change? >> >> There are less instructions being used with the change since it's not >> shoving the array data onto the stack at run time. Instead the array is >> being stored in the data section and there is less object code required >> to access the data. > > Ah. It's really because it's not a minimal compilation ala defconfig > > I think you should really stop making these size comparisons with > .config uses that are not based on a defconfig as a whole lot of other > things are going on. I'm using allmodconfig, which I believe is a legitimate configuration, especially since distros so build kernels with lots of modules. I'll double check on this though in case I've made a mistake. > > Please notice that the object sizes are significantly smaller below: > > So with an x86-64 defconfig and this compilation unit enabled with > CONFIG_OF enabled and CONFIG_DRM_TOSHIBA_TC358767=y, with gcc 10.3 > and this change the object size actually increases a bit. > > $ size drivers/gpu/drm/bridge/tc358767.o* >text data bss dec hex filename > 13554 268 1 1382335ff > drivers/gpu/drm/bridge/tc358767.o.new > 13548 268 1 1381735f9 > drivers/gpu/drm/bridge/tc358767.o.old> > objdump -h shows these differences: > > .old: > 0 .text 1e1f 0040 2**4 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > [...] > 14 .rodata 05ae 46e0 2**5 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > > .new: > 0 .text 1e05 0040 2**4 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > [...] > 11 .rodata 05ce 4600 2**5 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA ACK. Understood. Even so, it still makes sense for these kind of janitorial changes as it makes sense to constify arrays when they are read-only and making them static is sensible for const data. > > cheers, Joe >
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On Thu, 19 Aug 2021 at 08:17, Viresh Kumar wrote: > > On 18-08-21, 18:55, Dmitry Osipenko wrote: > > 18.08.2021 12:41, Ulf Hansson пишет: > > > > Either way gives the equal result. The new callback allows to remove the > > boilerplate dev_pm_opp_set_rate(clk_get_rate() code from the rpm-resume > > of consumer devices, that's it. > > It may not be equal, as dev_pm_opp_set_rate() may do additional stuff, > now or in a later implementation. Currently it only does > regulator_enable() as a special case, but it can be clk_enable() as > well. Also, this tries to solve the problem in a tricky/hacky way, > while all you wanted was to make the genpd aware of what the > performance state should be. > > Your driver can break tomorrow if we started to do more stuff from > this API at another time. > > > > dev_pm_opp_set_rate() is best called from consumer drivers, as they > > > need to be in control. > > >> What we need here is just configure. So something like this then: > > The intent wasn't to use dev_pm_opp_set_rate() from > > __genpd_dev_pm_attach(), but to set genpd->rpm_pstate in accordance to > > the h/w configuration. > > Right. > > > On Tegra we have a chain of PDs and it's not trivial to convert the > > device's OPP into pstate because only the parent domain can translate > > the required OPP. > > The driver should just be required to make a call, and OPP/genpd core > should return it a value. This is already done today while setting the > pstate for a device. The same frameworks must be able to supply a > value to be used for the device. Right, that sounds reasonable. We already have pm_genpd_opp_to_performance_state() which translates an OPP to a performance state. This function invokes the ->opp_to_performance_state() for a genpd. Maybe we need to allow a genpd to not have ->opp_to_performance_state() callback assigned though, but continue up in the hierarchy to see if the parent has the callback assigned, to make this work for Tegra? Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(), allowing us to pass the device instead of the genpd. But that's a minor thing. Finally, the precondition to use the above, is to first get a handle to an OPP table. This is where I am struggling to find a generic solution, because I guess that would be platform or even consumer driver specific for how to do this. And at what point should we do this? > > > Viresh, please take a look at what I did in [1]. Maybe it could be done > > in another way. > > I looked into this and looked like too much trouble. The > implementation needs to be simple. I am not sure I understand all the > problems you faced while doing that, would be better to start with a > simpler implementation of get_performance_state() kind of API for > genpd, after the domain is attached and its OPP table is initialized. > > Note, that the OPP table isn't required to be fully initialized for > the device at this point, we can parse the DT as well if needed be. Sure, but as I indicated above, you need some kind of input data to figure out what OPP table to pick, before you can translate that into a performance state. Is that always the clock rate, for example? Perhaps, we should start with adding a dev_pm_opp_get_from_rate() or what do you think? Do you have other suggestions? > > -- > viresh Kind regards Uffe
Re: [PATCH v8 01/13] dt-bindings: arm: mediatek: mmsys: add mt8195 SoC binding
Hi, Jason: jason-jh.lin 於 2021年8月19日 週四 上午10:23寫道: > > 1. There are 2 mmsys, namely vdosys0 and vdosys1 in mt8195. >Each of them is bound to a display pipeline, so add their >definition in mtk-mmsys documentation with 2 compatibles. > > 2. Add description for power-domain property. > > Signed-off-by: jason-jh.lin > --- > this patch is base on [1][2] > > [1] dt-bindings: arm: mediatek: mmsys: convert to YAML format > - > https://patchwork.kernel.org/project/linux-mediatek/patch/20210519161847.3747352-1-fpar...@baylibre.com/ > [2] dt-bindings: arm: mediatek: mmsys: add MT8365 SoC binding > - > https://patchwork.kernel.org/project/linux-mediatek/patch/20210519161847.3747352-2-fpar...@baylibre.com/ > --- > .../devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml | 8 > 1 file changed, 8 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml > index 2d4ff0ce387b..68cb330d7595 100644 > --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml > @@ -30,6 +30,8 @@ properties: >- mediatek,mt8173-mmsys >- mediatek,mt8183-mmsys >- mediatek,mt8365-mmsys > + - mediatek,mt8195-vdosys0 > + - mediatek,mt8195-vdosys1 >- const: syscon >- items: >- const: mediatek,mt7623-mmsys > @@ -39,6 +41,12 @@ properties: >reg: > maxItems: 1 > > + power-domains: > +description: > + A phandle and PM domain specifier as defined by bindings > + of the power controller specified by phandle. See > + Documentation/devicetree/bindings/power/power-domain.yaml for details. > + This patch is about mt8195, but mt8173 mmsys also has power domain. So move this part to another patch. Regards, Chun-Kuang. >"#clock-cells": > const: 1 > > -- > 2.18.0 >
Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
On Thu, 2021-08-19 at 15:51 +0100, Colin Ian King wrote: > it still makes sense for these kind of > janitorial changes as it makes sense to constify arrays when they are > read-only and making them static is sensible for const data. I'm not disagreeing. Marking unmodifiable arrays as const is generally useful for readers. Decent compilers though can _mostly_ determine whether or not an array is used as const and whether the array can be placed in a readonly section and is not required to be in a writable one. But the object sizes deltas you show with an allmodconfig are misleading. At a minimum I think you should show the output sizes as allmodconfig.
Re: [PATCH v8 07/13] soc: mediatek: add mtk-mutex support for mt8195 vdosys0
Hi, Jason: jason-jh.lin 於 2021年8月19日 週四 上午10:23寫道: > > Add mtk-mutex support for mt8195 vdosys0. > > Signed-off-by: jason-jh.lin > --- > drivers/soc/mediatek/mtk-mutex.c | 98 +++- > 1 file changed, 95 insertions(+), 3 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-mutex.c > b/drivers/soc/mediatek/mtk-mutex.c > index 2e4bcc300576..c177156ee2fa 100644 > --- a/drivers/soc/mediatek/mtk-mutex.c > +++ b/drivers/soc/mediatek/mtk-mutex.c > @@ -17,6 +17,9 @@ > #define MT8183_MUTEX0_MOD0 0x30 > #define MT8183_MUTEX0_SOF0 0x2c > > +#define MT8195_DISP_MUTEX0_MOD00x30 > +#define MT8195_DISP_MUTEX0_SOF 0x2c > + > #define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * (n)) > #define DISP_REG_MUTEX(n) (0x24 + 0x20 * (n)) > #define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * (n)) > @@ -67,6 +70,36 @@ > #define MT8173_MUTEX_MOD_DISP_PWM1 24 > #define MT8173_MUTEX_MOD_DISP_OD 25 > > +#define MT8195_MUTEX_MOD_DISP_OVL0 0 > +#define MT8195_MUTEX_MOD_DISP_WDMA01 > +#define MT8195_MUTEX_MOD_DISP_RDMA02 > +#define MT8195_MUTEX_MOD_DISP_COLOR0 3 > +#define MT8195_MUTEX_MOD_DISP_CCORR0 4 > +#define MT8195_MUTEX_MOD_DISP_AAL0 5 > +#define MT8195_MUTEX_MOD_DISP_GAMMA0 6 > +#define MT8195_MUTEX_MOD_DISP_DITHER0 7 > +#define MT8195_MUTEX_MOD_DISP_DSI0 8 > +#define MT8195_MUTEX_MOD_DISP_DSC_WRAP0_CORE0 9 > +#define MT8195_MUTEX_MOD_DISP_OVL1 10 > +#define MT8195_MUTEX_MOD_DISP_WDMA111 > +#define MT8195_MUTEX_MOD_DISP_RDMA112 > +#define MT8195_MUTEX_MOD_DISP_COLOR1 13 > +#define MT8195_MUTEX_MOD_DISP_CCORR1 14 > +#define MT8195_MUTEX_MOD_DISP_AAL1 15 > +#define MT8195_MUTEX_MOD_DISP_GAMMA1 16 > +#define MT8195_MUTEX_MOD_DISP_DITHER1 17 > +#define MT8195_MUTEX_MOD_DISP_DSI1 18 > +#define MT8195_MUTEX_MOD_DISP_DSC_WRAP0_CORE1 19 > +#define MT8195_MUTEX_MOD_DISP_VPP_MERGE20 > +#define MT8195_MUTEX_MOD_DISP_DP_INTF0 21 > +#define MT8195_MUTEX_MOD_DISP_VPP1_DL_RELAY0 22 > +#define MT8195_MUTEX_MOD_DISP_VPP1_DL_RELAY1 23 > +#define MT8195_MUTEX_MOD_DISP_VDO1_DL_RELAY2 24 > +#define MT8195_MUTEX_MOD_DISP_VDO0_DL_RELAY3 25 > +#define MT8195_MUTEX_MOD_DISP_VDO0_DL_RELAY4 26 > +#define MT8195_MUTEX_MOD_DISP_PWM0 27 > +#define MT8195_MUTEX_MOD_DISP_PWM1 28 > + > #define MT2712_MUTEX_MOD_DISP_PWM2 10 > #define MT2712_MUTEX_MOD_DISP_OVL0 11 > #define MT2712_MUTEX_MOD_DISP_OVL1 12 > @@ -101,12 +134,27 @@ > #define MT2712_MUTEX_SOF_DSI3 6 > #define MT8167_MUTEX_SOF_DPI0 2 > #define MT8167_MUTEX_SOF_DPI1 3 > + > #define MT8183_MUTEX_SOF_DSI0 1 > #define MT8183_MUTEX_SOF_DPI0 2 > > #define MT8183_MUTEX_EOF_DSI0 (MT8183_MUTEX_SOF_DSI0 << 6) > #define MT8183_MUTEX_EOF_DPI0 (MT8183_MUTEX_SOF_DPI0 << 6) > > +#define MT8195_MUTEX_SOF_DSI0 1 > +#define MT8195_MUTEX_SOF_DSI1 2 > +#define MT8195_MUTEX_SOF_DP_INTF0 3 > +#define MT8195_MUTEX_SOF_DP_INTF1 4 > +#define MT8195_MUTEX_SOF_DPI0 6 /* for HDMI_TX */ > +#define MT8195_MUTEX_SOF_DPI1 5 /* for digital video out */ > + > +#define MT8195_MUTEX_EOF_DSI0 (MT8195_MUTEX_SOF_DSI0 << 7) > +#define MT8195_MUTEX_EOF_DSI1 (MT8195_MUTEX_SOF_DSI1 << 7) > +#define MT8195_MUTEX_EOF_DP_INTF0 (MT8195_MUTEX_SOF_DP_INTF0 << > 7) > +#define MT8195_MUTEX_EOF_DP_INTF1 (MT8195_MUTEX_SOF_DP_INTF1 << > 7) > +#define MT8195_MUTEX_EOF_DPI0 (MT8195_MUTEX_SOF_DPI0 << 7) > +#define MT8195_MUTEX_EOF_DPI1 (MT8195_MUTEX_SOF_DPI1 << 7) > + > struct mtk_mutex { > int id; > bool claimed; > @@ -120,6 +168,9 @@ enum mtk_mutex_sof_id { > MUTEX_SOF_DPI1, > MUTEX_SOF_DSI2, > MUTEX_SOF_DSI3, > + MUTEX_SOF_DP_INTF0, > + MUTEX_SOF_DP_INTF1, > + DDP_MUTEX_SOF_MAX, > }; > > struct mtk_mutex_data { > @@ -214,7 +265,23 @@ static const unsigned int > mt8183_mutex_mod[DDP_COMPONENT_ID_MAX] = { > [DDP_COMPONENT_WDMA0] = MT8183_MUTEX_MOD_DISP_WDMA0, > }; > > -static const unsigned int mt2712_mutex_sof[MUTEX_SOF_DSI3 + 1] = { > +static const unsigned int mt8195_mutex_mod[DDP_COMPONENT_ID_MAX] = { > + [DDP_COMPONENT_OVL0] = MT8195_MUTEX_MOD_DISP_OVL0, > + [DDP_COMPONENT_WDMA0] = MT8195_MUTEX_MOD_DISP_WDMA0, > + [DDP_COMPONENT_RDMA0] = MT8195_MUTEX_MOD_DISP_RDMA0, > + [DDP_COMPONENT_COLOR0] = MT8195_MUTEX_MOD_DISP_COLOR0, > + [DDP_COMPONENT_CCORR] = MT8195_MUTEX_MOD_DISP_
Re: [PATCH v8 08/13] drm/mediatek: remove unused define in mtk_drm_ddp_comp.c
Hi, Jason: jason-jh.lin 於 2021年8月19日 週四 上午10:23寫道: > > Remove the unsed define in mtk_drm_ddp_comp.c Reviewed-by: Chun-Kuang Hu > > Signed-off-by: jason-jh.lin > --- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 10 -- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > index 75bc00e17fc4..aaa7450b3e2b 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > @@ -21,8 +21,6 @@ > #include "mtk_drm_crtc.h" > > #define DISP_OD_EN 0x > -#define DISP_OD_INTEN 0x0008 > -#define DISP_OD_INTSTA 0x000c > #define DISP_OD_CFG0x0020 > #define DISP_OD_SIZE 0x0030 > #define DISP_DITHER_5 0x0114 > @@ -42,8 +40,6 @@ > #define DITHER_ENGINE_EN BIT(1) > #define DISP_DITHER_SIZE 0x0030 > > -#define LUT_10BIT_MASK 0x03ff > - > #define OD_RELAYMODE BIT(0) > > #define UFO_BYPASS BIT(2) > @@ -52,18 +48,12 @@ > > #define DISP_DITHERING BIT(2) > #define DITHER_LSB_ERR_SHIFT_R(x) (((x) & 0x7) << 28) > -#define DITHER_OVFLW_BIT_R(x) (((x) & 0x7) << 24) > #define DITHER_ADD_LSHIFT_R(x) (((x) & 0x7) << 20) > -#define DITHER_ADD_RSHIFT_R(x) (((x) & 0x7) << 16) > #define DITHER_NEW_BIT_MODEBIT(0) > #define DITHER_LSB_ERR_SHIFT_B(x) (((x) & 0x7) << 28) > -#define DITHER_OVFLW_BIT_B(x) (((x) & 0x7) << 24) > #define DITHER_ADD_LSHIFT_B(x) (((x) & 0x7) << 20) > -#define DITHER_ADD_RSHIFT_B(x) (((x) & 0x7) << 16) > #define DITHER_LSB_ERR_SHIFT_G(x) (((x) & 0x7) << 12) > -#define DITHER_OVFLW_BIT_G(x) (((x) & 0x7) << 8) > #define DITHER_ADD_LSHIFT_G(x) (((x) & 0x7) << 4) > -#define DITHER_ADD_RSHIFT_G(x) (((x) & 0x7) << 0) > > struct mtk_ddp_comp_dev { > struct clk *clk; > -- > 2.18.0 >
Re: [PATCH v8 09/13] drm/mediatek: rename the define of register offset
Hi, Jason: jason-jh.lin 於 2021年8月19日 週四 上午10:23寫道: > > Add DISP_REG prefix for the define of register offset to > make the difference from the define of register value. > Reviewed-by: Chun-Kuang Hu > Signed-off-by: jason-jh.lin > --- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 57 +++-- > 1 file changed, 29 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > index aaa7450b3e2b..93beb980414f 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > @@ -20,25 +20,25 @@ > #include "mtk_drm_ddp_comp.h" > #include "mtk_drm_crtc.h" > > -#define DISP_OD_EN 0x > -#define DISP_OD_CFG0x0020 > -#define DISP_OD_SIZE 0x0030 > -#define DISP_DITHER_5 0x0114 > -#define DISP_DITHER_7 0x011c > -#define DISP_DITHER_15 0x013c > -#define DISP_DITHER_16 0x0140 > +#define DISP_REG_OD_EN 0x > +#define DISP_REG_OD_CFG0x0020 > +#define DISP_REG_OD_SIZE 0x0030 > +#define DISP_REG_DITHER_5 0x0114 > +#define DISP_REG_DITHER_7 0x011c > +#define DISP_REG_DITHER_15 0x013c > +#define DISP_REG_DITHER_16 0x0140 > > #define DISP_REG_UFO_START 0x > > -#define DISP_AAL_EN0x > -#define DISP_AAL_SIZE 0x0030 > +#define DISP_REG_AAL_EN0x > +#define DISP_REG_AAL_SIZE 0x0030 > > -#define DISP_DITHER_EN 0x > +#define DISP_REG_DITHER_EN 0x > #define DITHER_EN BIT(0) > -#define DISP_DITHER_CFG0x0020 > +#define DISP_REG_DITHER_CFG0x0020 > #define DITHER_RELAY_MODE BIT(0) > #define DITHER_ENGINE_EN BIT(1) > -#define DISP_DITHER_SIZE 0x0030 > +#define DISP_REG_DITHER_SIZE 0x0030 > > #define OD_RELAYMODE BIT(0) > > @@ -129,19 +129,19 @@ void mtk_dither_set_common(void __iomem *regs, struct > cmdq_client_reg *cmdq_reg, > return; > > if (bpc >= MTK_MIN_BPC) { > - mtk_ddp_write(cmdq_pkt, 0, cmdq_reg, regs, DISP_DITHER_5); > - mtk_ddp_write(cmdq_pkt, 0, cmdq_reg, regs, DISP_DITHER_7); > + mtk_ddp_write(cmdq_pkt, 0, cmdq_reg, regs, DISP_REG_DITHER_5); > + mtk_ddp_write(cmdq_pkt, 0, cmdq_reg, regs, DISP_REG_DITHER_7); > mtk_ddp_write(cmdq_pkt, > DITHER_LSB_ERR_SHIFT_R(MTK_MAX_BPC - bpc) | > DITHER_ADD_LSHIFT_R(MTK_MAX_BPC - bpc) | > DITHER_NEW_BIT_MODE, > - cmdq_reg, regs, DISP_DITHER_15); > + cmdq_reg, regs, DISP_REG_DITHER_15); > mtk_ddp_write(cmdq_pkt, > DITHER_LSB_ERR_SHIFT_B(MTK_MAX_BPC - bpc) | > DITHER_ADD_LSHIFT_B(MTK_MAX_BPC - bpc) | > DITHER_LSB_ERR_SHIFT_G(MTK_MAX_BPC - bpc) | > DITHER_ADD_LSHIFT_G(MTK_MAX_BPC - bpc), > - cmdq_reg, regs, DISP_DITHER_16); > + cmdq_reg, regs, DISP_REG_DITHER_16); > mtk_ddp_write(cmdq_pkt, dither_en, cmdq_reg, regs, cfg); > } > } > @@ -161,16 +161,16 @@ static void mtk_od_config(struct device *dev, unsigned > int w, > { > struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev); > > - mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs, > DISP_OD_SIZE); > - mtk_ddp_write(cmdq_pkt, OD_RELAYMODE, &priv->cmdq_reg, priv->regs, > DISP_OD_CFG); > - mtk_dither_set(dev, bpc, DISP_OD_CFG, cmdq_pkt); > + mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs, > DISP_REG_OD_SIZE); > + mtk_ddp_write(cmdq_pkt, OD_RELAYMODE, &priv->cmdq_reg, priv->regs, > DISP_REG_OD_CFG); > + mtk_dither_set(dev, bpc, DISP_REG_OD_CFG, cmdq_pkt); > } > > static void mtk_od_start(struct device *dev) > { > struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev); > > - writel(1, priv->regs + DISP_OD_EN); > + writel(1, priv->regs + DISP_REG_OD_EN); > } > > static void mtk_ufoe_start(struct device *dev) > @@ -186,7 +186,7 @@ static void mtk_aal_config(struct device *dev, unsigned > int w, > { > struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev); > > - mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv
Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."
On 2021-08-19 5:30 a.m., Daniel Vetter wrote: On Wed, Aug 18, 2021 at 10:51:00AM -0400, Andrey Grodzovsky wrote: On 2021-08-18 10:42 a.m., Daniel Vetter wrote: On Wed, Aug 18, 2021 at 10:36:32AM -0400, Andrey Grodzovsky wrote: On 2021-08-18 10:32 a.m., Daniel Vetter wrote: On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote: On 2021-08-18 10:02 a.m., Alex Deucher wrote: + dri-devel Since scheduler is a shared component, please add dri-devel on all scheduler patches. On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen wrote: [Why] for bailing job, this commit will delete it from pending list thus the bailing job will never have a chance to be resubmitted even in advance tdr mode. [How] after embeded hw_fence into amdgpu_job is done, the race condition that this commit tries to work around is completely solved.So revert this commit. This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90. v2: add dma_fence_get/put() around timedout_job to avoid concurrent delete during processing timedout_job Signed-off-by: Jingwen Chen --- drivers/gpu/drm/scheduler/sched_main.c | 23 +-- 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index a2a953693b45..f9b9b3aefc4a 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct *work) { struct drm_gpu_scheduler *sched; struct drm_sched_job *job; + struct dma_fence *fence; enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL; sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work); @@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct work_struct *work) if (job) { /* -* Remove the bad job so it cannot be freed by concurrent -* drm_sched_cleanup_jobs. It will be reinserted back after sched->thread -* is parked at which point it's safe. +* Get job->s_fence->parent here to avoid concurrent delete during +* processing timedout_job */ - list_del_init(&job->list); + fence = dma_fence_get(job->s_fence->parent); While this is true for amdgpu, it has no meaning for other drivers for whom we haven't done the refactoring of embedding HW fence (parent) into the job structure. In fact thinking about it, unless you do the HW fence embedding for all the drivers using the scheduler you cannot revert this patch or you will just break them. btw, why did you do that embedding? I do still have my patches with dma_fence annotations floating around, but my idea at least was to fix that issue with a mempool, not with embeddeding. What was the motivation for embedding the wh fence? -Daniel The motivation was 2 fold, avoid memory allocation during jobs submissions (HW fence allocation) because as Christian explained this leads to deadlock with mm code during evictions due to memory pressure (Christian can clarify if I messed Yeah that's the exact same thing I've chased with my dma_fence annotations, but thus far zero to none interested in getting it sorted. I think it'd be good to have some cross-driver agreement on how this should be solved before someone just charges ahead ... this explanation). Second is to exactly revert this patch because while it solved the issue described in the patch it created another with drivers who baildc out early during TDR handling for various reason and the job would just leak because it was already removed form pending list. Can't we reinsert it before we restart the scheduler thread? It might need a separate list for that due to the lockless queue tricks. Or am I thinking about the wrong kind of "we lost the job"? -Danile If you look at the original patch it would reinsert it even earlier - right after stopping the SW scheduler thread, and even then it was to late for some drivers as they would decide to return back from their TDR handler even before that. It is solvable but in an ugly way as far as I see, you need to require each driver in his code to put the job back in the list if they do it before reaching the place where scheduler framework does it. Kind of spaghetti code seems to me. Hm yeah I didn't realize this all happens before we stop the scheduler thread. Why can't we stop the scheduler thread first, so that there's guaranteed no race? I've recently had a lot of discussions with panfrost folks about their reset that spawns across engines, and without stopping the scheduler thread first before you touch anything it's just plain impossible. Talked with Christian on that, for each TDR we actually stop all the schedulers for all the rings and not only the hanged ring since ASIC reset will impact all the rings anyway. So we cannot allo
Re: [PATCH v2] drm/i915/dp: Use max params for panels < eDP 1.4
On Thu, Aug 19, 2021 at 01:14:55AM +0800, Kai-Heng Feng wrote: > Users reported that after commit 2bbd6dba84d4 ("drm/i915: Try to use > fast+narrow link on eDP again and fall back to the old max strategy on > failure"), the screen starts to have wobbly effect. > > Commit a5c936add6a2 ("drm/i915/dp: Use slow and wide link training for > everything") doesn't help either, that means the affected eDP 1.2 panels > only work with max params. > > So use max params for panels < eDP 1.4 as Windows does to solve the > issue. > > v2: > - Check eDP 1.4 instead of DPCD 1.1 to apply max params > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3714 > Fixes: 2bbd6dba84d4 ("drm/i915: Try to use fast+narrow link on eDP again and > fall back to the old max strategy on failure") > Fixes: a5c936add6a2 ("drm/i915/dp: Use slow and wide link training for > everything") > Suggested-by: Ville Syrjälä > Signed-off-by: Kai-Heng Feng > --- > drivers/gpu/drm/i915/display/intel_dp.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 75d4ebc669411..f87fad78f1a9f 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -1330,14 +1330,16 @@ intel_dp_compute_link_config(struct intel_encoder > *encoder, > limits.min_bpp = intel_dp_min_bpp(pipe_config->output_format); > limits.max_bpp = intel_dp_max_bpp(intel_dp, pipe_config); > > - if (intel_dp->use_max_params) { > + if (intel_dp->use_max_params || > + intel_dp->edp_dpcd[0] < DP_EDP_14) { I think we're going to need a is_edp check here or else we'll always take this path on extenral DP when edp_dpcd[] is zeroed. Hmm. Maybe just stick intel_dp->use_max_params = intel_dp->edp_dpcd[0] < DP_EDP_14; into intel_edp_init_dpcd()? > /* >* Use the maximum clock and number of lanes the eDP panel >* advertizes being capable of in case the initial fast > - * optimal params failed us. The panels are generally > - * designed to support only a single clock and lane > - * configuration, and typically on older panels these > - * values correspond to the native resolution of the panel. > + * optimal params failed us or the EDP rev is earlier than 1.4. > + * The panels are generally designed to support only a single > + * clock and lane configuration, and typically on older panels > + * these values correspond to the native resolution of the > + * panel. >*/ > limits.min_lane_count = limits.max_lane_count; > limits.min_clock = limits.max_clock; > -- > 2.32.0 -- Ville Syrjälä Intel
Re: [PATCH v8 19/34] pwm: tegra: Add runtime PM and OPP support
On Thu, Aug 19, 2021 at 04:04:50PM +0200, Ulf Hansson wrote: > On Thu, 19 Aug 2021 at 15:21, Thierry Reding wrote: > > > > On Tue, Aug 17, 2021 at 04:27:39AM +0300, Dmitry Osipenko wrote: > > > The PWM on Tegra belongs to the core power domain and we're going to > > > enable GENPD support for the core domain. Now PWM must be resumed using > > > runtime PM API in order to initialize the PWM power state. The PWM clock > > > rate must be changed using OPP API that will reconfigure the power domain > > > performance state in accordance to the rate. Add runtime PM and OPP > > > support to the PWM driver. > > > > > > Signed-off-by: Dmitry Osipenko > > > --- > > > drivers/pwm/pwm-tegra.c | 104 > > > 1 file changed, 85 insertions(+), 19 deletions(-) > > > > Can this be safely applied independently of the rest of the series, or > > are there any dependencies on earlier patches? > > Just to make sure we don't rush something in, I would rather withhold > all runtime PM related patches in the series, until we have agreed on > how to fix the in genpd/opp core parts. Simply, because those may very > well affect the deployments in the drivers. Okay, understood. I didn't realize this may have an impact on how drivers need to cooperate. I'll hold off on applying any of these patches until the discussion has settled, then. Thierry signature.asc Description: PGP signature
Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
On Thu, Aug 19, 2021 at 09:27:16AM -0300, Jason Gunthorpe wrote: > On Tue, Aug 17, 2021 at 11:05:26PM -0700, Kees Cook wrote: > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memset(), avoid intentionally writing across > > neighboring fields. > > > > Add struct_group() to mark region of struct mlx5_ib_mr that should be > > initialized to zero. > > > > Cc: Leon Romanovsky > > Cc: Doug Ledford > > Cc: Jason Gunthorpe > > Cc: linux-r...@vger.kernel.org > > Signed-off-by: Kees Cook > > --- > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h > > b/drivers/infiniband/hw/mlx5/mlx5_ib.h > > index bf20a388eabe..f63bf204a7a1 100644 > > --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h > > @@ -644,6 +644,7 @@ struct mlx5_ib_mr { > > struct ib_umem *umem; > > > > /* This is zero'd when the MR is allocated */ > > + struct_group(cleared, > > union { > > /* Used only while the MR is in the cache */ > > struct { > > @@ -691,12 +692,13 @@ struct mlx5_ib_mr { > > bool is_odp_implicit; > > }; > > }; > > + ); > > }; > > > > /* Zero the fields in the mr that are variant depending on usage */ > > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr) > > { > > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out)); > > + memset(&mr->cleared, 0, sizeof(mr->cleared)); > > } > > Why not use the memset_after(mr->umem) here? I can certainly do that instead. In this series I've tended to opt for groupings so the position of future struct member additions are explicitly chosen. (i.e. reducing the chance that a zeroing of the new member be a surprise.) -Kees -- Kees Cook
Re: [PATCH v1] drm/msm/dpu: Fix address of SM8150 PINGPONG5 IRQ register
On 19/08/2021 16:36, Robert Foss wrote: Both PINGPONG4 and PINGPONG5 IRQ registers are using the same address, which is incorrect. PINGPONG4 should use the register offset 30, and PINGPONG5 should use the register offset 31 according to the downstream driver. Fixes: 667e9985ee24 ("drm/msm/dpu: replace IRQ lookup with the data in hw catalog") Signed-off-by: Robert Foss Reviewed-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c index 2e482cdd7b3c5..420d78cfce8af 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c @@ -794,7 +794,7 @@ static const struct dpu_pingpong_cfg sm8150_pp[] = { DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30), -1), PP_BLK("pingpong_5", PINGPONG_5, 0x72800, MERGE_3D_2, sdm845_pp_sblk, - DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30), + DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 31), -1), }; -- With best wishes Dmitry
Re: [PATCH v2 45/63] ath11k: Use memset_startat() for clearing queue descriptors
On Thu, Aug 19, 2021 at 04:19:37PM +0300, Kalle Valo wrote: > Kees Cook writes: > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memset(), avoid intentionally writing across > > neighboring fields. > > > > Use memset_startat() so memset() doesn't get confused about writing > > beyond the destination member that is intended to be the starting point > > of zeroing through the end of the struct. Additionally split up a later > > field-spanning memset() so that memset() can reason about the size. > > > > Cc: Kalle Valo > > Cc: "David S. Miller" > > Cc: Jakub Kicinski > > Cc: ath...@lists.infradead.org > > Cc: linux-wirel...@vger.kernel.org > > Cc: net...@vger.kernel.org > > Signed-off-by: Kees Cook > > To avoid conflicts I prefer taking this via my ath tree. The memset helpers are introduced as part of this series, so that makes things more difficult. Do you want me to create a branch with the helpers that you can merge? -Kees -- Kees Cook
Re: [PATCH v8 06/34] dt-bindings: clock: tegra-car: Document new tegra-clocks sub-node
On Wed, Aug 18, 2021 at 07:57:04PM +0300, Dmitry Osipenko wrote: > 18.08.2021 19:39, Thierry Reding пишет: > >> We don't have a platform device for CaR. I don't see how it's going to > >> work. We need to create a platform device for each RPM-capable clock > >> because that's how RPM works. The compatible string is required for > >> instantiating OF-devices from a node, otherwise we will have to > >> re-invent the OF core. > > I think we do have a platform device for CAR. It's just not bound > > against by the driver because these clock drivers are "special". But > > from other parts of the series you're already trying to fix that, at > > least partially. > > > > But it doesn't seem right to create a platform device for each RPM- > > capable clock. Why do they need to be devices? They aren't, so why > > pretend? Is it that some API that we want to use here requires the > > struct device? > > The "device" representation is internal to the kernel. It's okay to me > to have PLLs represented by a device, it's a distinct h/w by itself. > > CCF supports managing of clock's RPM and it requires to have clock to be > backed by a device. That's what we are using here. > > Please see > https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109 Looking at the implementation of __clk_register() and where that device pointer typically comes from, I don't think the way this is used here is what was intended. The way I interpret the code is that a clock is registered with a parent device (i.e. its provider) and clk_pm_runtime_get() is then used internally as a way to make sure that when a clock is prepared, it's parent device is runtime resumed. This is presumably to ensure that any registers that the driver might need to access in order to prepare and enable the clock are accessible (i.e. the CAR is not powered off or in reset). So the struct device that is passed to __clk_register() (or its callers) should be that of the CAR rather than virtual struct devices created by the CAR. And it's a bit debatable whether or not PLLs represent distinct hardware. Ultimately every transistor on a chip could be considered distinct hardware. But a platform device is a device on a platform bus, which is really just another way of saying it's a hardware block that's accessible from the CPU via a memory-mapped address. A PLL (just like other clocks) is merely a resource exposed by means of access to these registers. So I don't think they should be platform devices. Even making them struct device:s seems a bit of a stretch. Is there any reason why struct clk can't be used for this? I mean, the whole purpose of that structure is to represent clocks. Why do we need to make them special? > >>> Also, I don't think the tegra- prefix is necessary here. The parent node > >>> is already identified as Tegra via the compatible string. > >>> > >>> In the case of CAR, I'd imagine something like: > >>> > >>> clocks { > >>> sclk { > >>> operating-points-v2 = <&opp_table>; > >>> power-domains = <&domain>; > >>> }; > >>> }; > >>> > >>> Now you've only got the bare minimum in here that you actually add. All > >>> the other data that you used to have is simply derived from the parent. > >> 'clocks' is already a generic keyword in DT. It's probably not okay to > >> redefine it. > > "clocks" is not a generic keyword. It's the name of a property and given > > that we're talking about the clock provider here, it doesn't need a > > clocks property of its own, so it should be fine to use that for the > > node. > > I'm curious what Rob thinks about it. Rob, does this sound okay to you? Another alternative would be to omit that level altogether and just make sclk and siblings direct children of the CAR node. Thierry signature.asc Description: PGP signature
Re: [PATCH v2 02/12] mm: Introduce a function to check for virtualization protection features
On 8/19/21 4:46 AM, Christoph Hellwig wrote: > On Fri, Aug 13, 2021 at 11:59:21AM -0500, Tom Lendacky wrote: >> +#define PATTR_MEM_ENCRYPT 0 /* Encrypted memory */ >> +#define PATTR_HOST_MEM_ENCRYPT 1 /* Host encrypted >> memory */ >> +#define PATTR_GUEST_MEM_ENCRYPT 2 /* Guest encrypted >> memory */ >> +#define PATTR_GUEST_PROT_STATE 3 /* Guest encrypted >> state */ > > Please write an actual detailed explanaton of what these mean, that > is what implications it has on the kernel. Will do. Thanks, Tom >
[PATCH] test upstream
test upstream panel Signed-off-by: yangcong --- drivers/gpu/drm/panel/panel-truly-nt35597.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c index b24b92d93ea5..e3cd37025c12 100644 --- a/drivers/gpu/drm/panel/panel-truly-nt35597.c +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c @@ -2,7 +2,9 @@ /* * Copyright (c) 2018, The Linux Foundation. All rights reserved. */ - +/* + * test upstream. + */ #include #include #include -- 2.25.1
Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
On Thu, Aug 19, 2021 at 09:19:08AM -0700, Kees Cook wrote: > On Thu, Aug 19, 2021 at 09:27:16AM -0300, Jason Gunthorpe wrote: > > On Tue, Aug 17, 2021 at 11:05:26PM -0700, Kees Cook wrote: > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > > field bounds checking for memset(), avoid intentionally writing across > > > neighboring fields. > > > > > > Add struct_group() to mark region of struct mlx5_ib_mr that should be > > > initialized to zero. > > > > > > Cc: Leon Romanovsky > > > Cc: Doug Ledford > > > Cc: Jason Gunthorpe > > > Cc: linux-r...@vger.kernel.org > > > Signed-off-by: Kees Cook > > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h > > > b/drivers/infiniband/hw/mlx5/mlx5_ib.h > > > index bf20a388eabe..f63bf204a7a1 100644 > > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h > > > @@ -644,6 +644,7 @@ struct mlx5_ib_mr { > > > struct ib_umem *umem; > > > > > > /* This is zero'd when the MR is allocated */ > > > + struct_group(cleared, > > > union { > > > /* Used only while the MR is in the cache */ > > > struct { > > > @@ -691,12 +692,13 @@ struct mlx5_ib_mr { > > > bool is_odp_implicit; > > > }; > > > }; > > > + ); > > > }; > > > > > > /* Zero the fields in the mr that are variant depending on usage */ > > > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr) > > > { > > > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out)); > > > + memset(&mr->cleared, 0, sizeof(mr->cleared)); > > > } > > > > Why not use the memset_after(mr->umem) here? > > I can certainly do that instead. In this series I've tended to opt > for groupings so the position of future struct member additions are > explicitly chosen. (i.e. reducing the chance that a zeroing of the new > member be a surprise.) I saw the earlier RDMA patches where using other memset techniques though? Were there flex arrays or something that made groups infeasible? Jason
Re: [PATCH 01/17] drm/dp: add DP 2.0 UHBR link rate and bw code conversions
On Wed, Aug 18, 2021 at 09:10:36PM +0300, Jani Nikula wrote: > The bw code equals link_rate / 0.27 Gbps only for 8b/10b link > rates. Handle DP 2.0 UHBR rates as special cases, though this is not > pretty. Ugh. So if I'm reading the spec right the behaviour of this register now changes dynamically depending on the state of some other bit in another register? > > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/drm_dp_helper.c | 26 ++ > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 6d0f2c447f3b..9b2a2961fca8 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -207,15 +207,33 @@ EXPORT_SYMBOL(drm_dp_lttpr_link_train_channel_eq_delay); > > u8 drm_dp_link_rate_to_bw_code(int link_rate) > { > - /* Spec says link_bw = link_rate / 0.27Gbps */ > - return link_rate / 27000; > + switch (link_rate) { > + case 100: > + return DP_LINK_BW_10; > + case 135: > + return DP_LINK_BW_13_5; > + case 200: > + return DP_LINK_BW_20; > + default: > + /* Spec says link_bw = link_rate / 0.27Gbps */ > + return link_rate / 27000; > + } > } > EXPORT_SYMBOL(drm_dp_link_rate_to_bw_code); > > int drm_dp_bw_code_to_link_rate(u8 link_bw) > { > - /* Spec says link_rate = link_bw * 0.27Gbps */ > - return link_bw * 27000; > + switch (link_bw) { > + case DP_LINK_BW_10: > + return 100; > + case DP_LINK_BW_13_5: > + return 135; > + case DP_LINK_BW_20: > + return 200; > + default: > + /* Spec says link_rate = link_bw * 0.27Gbps */ > + return link_bw * 27000; > + } > } > EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate); > > -- > 2.20.1 -- Ville Syrjälä Intel
Re: [PATCH v8 07/34] clk: tegra: Support runtime PM and power domain
On Wed, Aug 18, 2021 at 08:11:03PM +0300, Dmitry Osipenko wrote: > 18.08.2021 19:42, Thierry Reding пишет: > > On Wed, Aug 18, 2021 at 06:05:21PM +0300, Dmitry Osipenko wrote: > >> 18.08.2021 17:07, Thierry Reding пишет: > >>> On Tue, Aug 17, 2021 at 04:27:27AM +0300, Dmitry Osipenko wrote: > >>> [...] > +struct clk *tegra_clk_register(struct clk_hw *hw) > +{ > +struct platform_device *pdev; > +struct device *dev = NULL; > +struct device_node *np; > +const char *dev_name; > + > +np = tegra_clk_get_of_node(hw); > + > +if (!of_device_is_available(np)) > +goto put_node; > + > +dev_name = kasprintf(GFP_KERNEL, "tegra_clk_%s", > hw->init->name); > +if (!dev_name) > +goto put_node; > + > +pdev = of_platform_device_create(np, dev_name, NULL); > +if (!pdev) { > +pr_err("%s: failed to create device for %pOF\n", > __func__, np); > +kfree(dev_name); > +goto put_node; > +} > + > +dev = &pdev->dev; > +pm_runtime_enable(dev); > +put_node: > +of_node_put(np); > + > +return clk_register(dev, hw); > +} > >>> > >>> This looks wrong. Why do we need struct platform_device objects for each > >>> of these clocks? That's going to be a massive amount of platform devices > >>> and they will completely mess up sysfs. > >> > >> RPM works with a device. It's not a massive amount of devices, it's one > >> device for T20 and four devices for T30. > > > > I'm still not sure I understand why we need to call RPM functions on a > > clock. And even if they are few, it seems wrong to make these platform > > devices. > > Before clock is enabled, we need to raise core voltage. After clock is > disabled, the voltage should be dropped. CCF+RPM takes care of handling > this for us. That's the part that I do understand. What I don't understand is why a clock needs to be runtime suspend/resumed. Typically we suspend/resume devices, and doing so typically involves disabling/enabling clocks. So I don't understand why the clocks themselves now need to be runtime suspended/resumed. > > Perhaps they can be simple struct device:s instead? Ideally they would > > also be parented to the CAR so that they appear in the right place in > > the sysfs hierarchy. > > Could you please clarify what do you mean by 'simple struct device:s'? > These clock devices should be OF devices with a of_node and etc, > otherwise we can't use OPP framework. Perhaps I misunderstand the goal of the OPP framework. My understanding was that this was to attach a table of operating points with a device so that appropriate operating points could be selected and switched to when the workload changes. Typically these operating points would be roughly a clock rate and a corresponding voltage for a regulator, so that when a certain clock rate is requested, the regulator can be set to the matching voltage. Hm... so is it that each of these clocks that you want to create a platform device for has its own regulator? Because the patch series only mentions the CORE domain, so I assumed that we would accumulate all the clock rates for the clocks that are part of that CORE domain and then derive a voltage to be supplied to that CORE domain. But perhaps I just don't understand correctly how this is tied together. > We don't have driver for CAR to bind. I guess we could try to add a > 'dummy' CAR driver that will create sub-devices for the rpm-clocks, is > this what you're wanting? I got confused by the "tegra-clock" driver that this series was adding. This is actually a driver that will bind to the virtual clocks rather than the CAR device itself. For some reason I had assumed that you wanted to create a CAR driver in order to get at the struct device embedded in the CAR's platform device and use that as the parent for all these clocks. So even if we absolutely need some struct device for these clocks, maybe adding that CAR driver and making the clock struct device:s children of the CAR device will help keep a bit of a proper hierarchy in sysfs. Thierry signature.asc Description: PGP signature
Re: [PATCH v8 20/34] mmc: sdhci-tegra: Add runtime PM and OPP support
On Tue, Aug 17, 2021 at 04:27:40AM +0300, Dmitry Osipenko wrote: > The SDHCI on Tegra belongs to the core power domain and we're going to > enable GENPD support for the core domain. Now SDHCI must be resumed using > runtime PM API in order to initialize the SDHCI power state. The SDHCI > clock rate must be changed using OPP API that will reconfigure the power > domain performance state in accordance to the rate. Add runtime PM and OPP > support to the SDHCI driver. > > Signed-off-by: Dmitry Osipenko > --- > drivers/mmc/host/sdhci-tegra.c | 146 - > 1 file changed, 105 insertions(+), 41 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > index 387ce9cdbd7c..a3583359c972 100644 > --- a/drivers/mmc/host/sdhci-tegra.c > +++ b/drivers/mmc/host/sdhci-tegra.c > @@ -15,6 +15,8 @@ > #include > #include > #include > +#include > +#include > #include > #include > #include > @@ -24,6 +26,8 @@ > #include > #include > > +#include > + > #include "sdhci-pltfm.h" > #include "cqhci.h" > > @@ -123,6 +127,12 @@ >SDHCI_TRNS_BLK_CNT_EN | \ >SDHCI_TRNS_DMA) > > +enum { > + TEGRA_CLK_BULK_SDHCI, > + TEGRA_CLK_BULK_TMCLK, > + TEGRA_CLK_BULK_NUM, > +}; > + > struct sdhci_tegra_soc_data { > const struct sdhci_pltfm_data *pdata; > u64 dma_mask; > @@ -171,6 +181,8 @@ struct sdhci_tegra { > bool enable_hwcq; > unsigned long curr_clk_rate; > u8 tuned_tap_delay; > + > + struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM]; This doesn't seem worth it to me. There's a lot of churn in this driver that's only needed to convert this to the clk_bulk API and it makes the code a lot more difficult to read, in my opinion. It looks like the only benefit that this gives us is that runtime suspend and resume become a few lines shorter. Thierry signature.asc Description: PGP signature
Re: [PATCH 1/2] drm/probe-helper: Create a HPD IRQ event helper for a single connector
Hi Maxime, a few comments in the following that I hope you will find useful. Sam On Thu, Aug 19, 2021 at 03:44:53PM +0200, Maxime Ripard wrote: > The drm_helper_hpd_irq_event() function is iterating over all the > connectors when an hotplug event is detected. > > During that iteration, it will call each connector detect function and > figure out if its status changed. > > Finally, if any connector changed, it will notify the user-space and the > clients that something changed on the DRM device. > > This is supposed to be used for drivers that don't have a hotplug > interrupt for individual connectors. However, drivers that can use an > interrupt for a single connector are left in the dust and can either > reimplement the logic used during the iteration for each connector or > use that helper and iterate over all connectors all the time. > > Since both are suboptimal, let's create a helper that will only perform > the status detection on a single connector. > > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/drm_probe_helper.c | 105 - > include/drm/drm_probe_helper.h | 1 + > 2 files changed, 74 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c > b/drivers/gpu/drm/drm_probe_helper.c > index 5606bca3caa8..7e3cbb4333ce 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -795,6 +795,77 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_kms_helper_poll_fini); > > +static bool > +_drm_connector_helper_hpd_irq_event(struct drm_connector *connector, > + bool notify) I hate _names _starting _with '_'. In this case check_connector_changed() would be better IMO. Also the notify bool could be dropped, just move the notify code to the drm_connector_helper_hpd_irq_event(). > +{ > + struct drm_device *dev = connector->dev; > + enum drm_connector_status old_status; > + u64 old_epoch_counter; > + bool changed = false; > + > + /* Only handle HPD capable connectors. */ > + drm_WARN_ON(dev, !(connector->polled & DRM_CONNECTOR_POLL_HPD)); > + > + drm_WARN_ON(dev, !mutex_is_locked(&dev->mode_config.mutex)); > + > + old_status = connector->status; > + old_epoch_counter = connector->epoch_counter; > + > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Old epoch counter %llu\n", > + connector->base.id, > + connector->name, > + old_epoch_counter); As you touch this code please use the new drm_dbg_kms() macro for logging. > + > + connector->status = drm_helper_probe_detect(connector, NULL, > + false); > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", > + connector->base.id, > + connector->name, > + drm_get_connector_status_name(old_status), > + drm_get_connector_status_name(connector->status)); > + > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] New epoch counter %llu\n", > + connector->base.id, > + connector->name, > + connector->epoch_counter); > + > + /* > + * Check if epoch counter had changed, meaning that we need > + * to send a uevent. > + */ > + if (old_epoch_counter != connector->epoch_counter) > + changed = true; > + > + if (changed && notify) { > + drm_kms_helper_hotplug_event(dev); > + DRM_DEBUG_KMS("Sent hotplug event\n"); > + } > + > + return changed; > +} > + > +/** > + * drm_connector_helper_hpd_irq_event - hotplug processing > + * @connector: drm_connector > + * > + * Drivers can use this helper function to run a detect cycle on a connector > + * which has the DRM_CONNECTOR_POLL_HPD flag set in its &polled member. > + * > + * This helper function is useful for drivers which can track hotplug > + * interrupts for a single connector. > + * > + * This function must be called with the mode setting locks held. > + * > + * Note that a connector can be both polled and probed from the hotplug > + * handler, in case the hotplug interrupt is known to be unreliable. > + */ > +bool drm_connector_helper_hpd_irq_event(struct drm_connector *connector) > +{ > + return _drm_connector_helper_hpd_irq_event(connector, true); > +} > +EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); > + > /** > * drm_helper_hpd_irq_event - hotplug processing > * @dev: drm_device > @@ -822,9 +893,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) > { > struct drm_connector *connector; > struct drm_connector_list_iter conn_iter; > - enum drm_connector_status old_status; > bool changed = false; > - u64 old_epoch_counter; > > if (!dev->mode_config.poll_enabled) > return false; > @@ -832,37 +901,9 @@ bool drm_helper_hpd_irq_event(struct drm_device
Re: [PATCH 2/4] drm/dp: use more of the extended receiver cap
On Fri, Aug 13, 2021 at 01:43:20PM +0300, Jani Nikula wrote: > Extend the use of extended receiver cap at 0x2200 to cover > MAIN_LINK_CHANNEL_CODING_CAP in 0x2206, in case an implementation hides > the DP 2.0 128b/132b channel encoding cap. > > Cc: Manasi Navare > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/drm_dp_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 9b2a2961fca8..9389f92cb944 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -608,7 +608,7 @@ static u8 drm_dp_downstream_port_count(const u8 > dpcd[DP_RECEIVER_CAP_SIZE]) > static int drm_dp_read_extended_dpcd_caps(struct drm_dp_aux *aux, > u8 dpcd[DP_RECEIVER_CAP_SIZE]) > { > - u8 dpcd_ext[6]; > + u8 dpcd_ext[DP_MAIN_LINK_CHANNEL_CODING + 1]; Why are we even reading less of this than the normal receiver caps? > int ret; > > /* > -- > 2.20.1 -- Ville Syrjälä Intel
Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
Hi Colin, On Thu, Aug 19, 2021 at 02:38:39PM +0100, Colin King wrote: > From: Colin Ian King > > Don't populate the array ext_div on the stack but instead it > static const. Makes the object code smaller by 118 bytes: > > Before: >textdatabss dechex filename > 39449 17500128 57077 def5 ./drivers/gpu/drm/bridge/tc358767.o > > After: >textdatabss dechex filename > 39235 17596128 56959 de7f ./drivers/gpu/drm/bridge/tc358767.o > > (gcc version 10.3.0) IMO a better argument is that this change prevents any accidental changes and it align with how we define arrays in many other places. The compiler may produce a smaller binary but that is just a side-effect in this case. Sam > > Signed-off-by: Colin Ian King > --- > drivers/gpu/drm/bridge/tc358767.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c > b/drivers/gpu/drm/bridge/tc358767.c > index 23a6f90b694b..599c23759400 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, > u32 pixelclock) > int div, best_div = 1; > int mul, best_mul = 1; > int delta, best_delta; > - int ext_div[] = {1, 2, 3, 5, 7}; > + static const int ext_div[] = {1, 2, 3, 5, 7}; > int best_pixelclock = 0; > int vco_hi = 0; > u32 pxl_pllparam; > -- > 2.32.0
Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
On Thu, Aug 19, 2021 at 10:52:53AM +0100, Christoph Hellwig wrote: > Which suggest that the name is not good to start with. Maybe protected > hardware, system or platform might be a better choice? Yah, coming up with a proper name here hasn't been easy. prot_guest_has() is not the first variant. >From all three things you suggest above, I guess calling it a "platform" is the closest. As in, this is a confidential computing platform which provides host and guest facilities etc. So calling it confidential_computing_platform_has() is obviously too long. ccp_has() clashes with the namespace of drivers/crypto/ccp/ which is used by the technology too. coco_platform_has() is too unserious. So I guess cc_platform_has() ain't all that bad. Unless you have a better idea, ofc. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH 04/17] drm/dp: add helper for extracting adjust 128b/132b TX FFE preset
On Wed, Aug 18, 2021 at 09:10:39PM +0300, Jani Nikula wrote: > The DP 2.0 128b/132b channel coding uses TX FFE presets instead of > vswing and pre-emphasis. > > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Jani Nikula Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/drm_dp_helper.c | 14 ++ > include/drm/drm_dp_helper.h | 2 ++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 9389f92cb944..2843238a78e6 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -130,6 +130,20 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 > link_status[DP_LINK_STATUS_SI > } > EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); > > +/* DP 2.0 128b/132b */ > +u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE], > +int lane) > +{ > + int i = DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1); > + int s = ((lane & 1) ? > + DP_ADJUST_TX_FFE_PRESET_LANE1_SHIFT : > + DP_ADJUST_TX_FFE_PRESET_LANE0_SHIFT); > + u8 l = dp_link_status(link_status, i); > + > + return (l >> s) & 0xf; > +} > +EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset); > + > u8 drm_dp_get_adjust_request_post_cursor(const u8 > link_status[DP_LINK_STATUS_SIZE], >unsigned int lane) > { > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index f3a61341011d..3ee0b3ffb8a5 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -1494,6 +1494,8 @@ u8 drm_dp_get_adjust_request_voltage(const u8 > link_status[DP_LINK_STATUS_SIZE], >int lane); > u8 drm_dp_get_adjust_request_pre_emphasis(const u8 > link_status[DP_LINK_STATUS_SIZE], > int lane); > +u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE], > +int lane); > u8 drm_dp_get_adjust_request_post_cursor(const u8 > link_status[DP_LINK_STATUS_SIZE], >unsigned int lane); > > -- > 2.20.1 -- Ville Syrjälä Intel
Re: [PATCH v6 02/13] mm: remove extra ZONE_DEVICE struct page refcount
On 8/18/2021 2:28 PM, Ralph Campbell wrote: On 8/17/21 5:35 PM, Felix Kuehling wrote: Am 2021-08-17 um 8:01 p.m. schrieb Ralph Campbell: On 8/12/21 11:31 PM, Alex Sierra wrote: From: Ralph Campbell ZONE_DEVICE struct pages have an extra reference count that complicates the code for put_page() and several places in the kernel that need to check the reference count to see that a page is not being used (gup, compaction, migration, etc.). Clean up the code so the reference count doesn't need to be treated specially for ZONE_DEVICE. v2: AS: merged this patch in linux 5.11 version v5: AS: add condition at try_grab_page to check for the zone device type, while page ref counter is checked less/equal to zero. In case of device zone, pages ref counter are initialized to zero. Signed-off-by: Ralph Campbell Signed-off-by: Alex Sierra --- arch/powerpc/kvm/book3s_hv_uvmem.c | 2 +- drivers/gpu/drm/nouveau/nouveau_dmem.c | 2 +- fs/dax.c | 4 +- include/linux/dax.h | 2 +- include/linux/memremap.h | 7 +-- include/linux/mm.h | 13 + lib/test_hmm.c | 2 +- mm/internal.h | 8 +++ mm/memremap.c | 68 +++--- mm/migrate.c | 5 -- mm/page_alloc.c | 3 ++ mm/swap.c | 45 ++--- 12 files changed, 46 insertions(+), 115 deletions(-) I haven't seen a response to the issues I raised back at v3 of this series. https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-mm%2F4f6dd918-d79b-1aa7-3a4c-caa67ddc29bc%40nvidia.com%2F&data=04%7C01%7Calex.sierra%40amd.com%7Cd2bd2d4fbf764528540908d9627e5dcd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649117156919772%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=P7FxYm%2BkJaCkMFa3OHtuKrPOn7SvytFRmYQdIzq7rN4%3D&reserved=0 Did I miss something? I think part of the response was that we did more testing. Alex added support for DEVICE_GENERIC pages to test_hmm and he ran DAX tests recommended by Theodore Tso. In that testing he ran into a WARN_ON_ONCE about a zero page refcount in try_get_page. The fix is in the latest version of patch 2. But it's already obsolete because John Hubbard is about to remove that function altogether. I think the issues you raised were more uncertainty than known bugs. It seems the fact that you can have DAX pages with 0 refcount is a feature more than a bug. Regards, Felix Did you test on a system without CONFIG_ARCH_HAS_PTE_SPECIAL defined? In that case, mmap() of a DAX device will call insert_page() which calls get_page() which would trigger VM_BUG_ON_PAGE(). I can believe it is OK for PTE_SPECIAL page table entries to have no struct page or that MEMORY_DEVICE_GENERIC struct pages be mapped with a zero reference count using insert_pfn(). Hi Ralph, We have tried the DAX tests with and without CONFIG_ARCH_HAS_PTE_SPECIAL defined. Apparently none of the tests touches that condition for a DAX device. Of course, that doesn't mean it could happen. Regards, Alex S. I find it hard to believe that other MM developers don't see an issue with a struct page with refcount == 0 and mapcount == 1. I don't see where init_page_count() is being called for the MEMORY_DEVICE_GENERIC or MEMORY_DEVICE_PRIVATE struct pages the AMD driver allocates and passes to migrate_vma_setup(). Looks like svm_migrate_get_vram_page() needs to call init_page_count() instead of get_page(). (I'm looking at branch origin/alexsierrag/device_generic https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FRadeonOpenCompute%2FROCK-Kernel-Driver.git&data=04%7C01%7Calex.sierra%40amd.com%7Cd2bd2d4fbf764528540908d9627e5dcd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649117156919772%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=IXe8HP2s8x5OdJdERBkGOYJCQk3iqCu5AYkwpDL8zec%3D&reserved=0) Yes, you're right. My bad. Thanks for catching this up. I didn't realize I was missing to define CONFIG_DEBUG_VM on my build. Therefore this BUG was never caught. It worked after I replaced get_pages by init_page_count at svm_migrate_get_vram_page. However, I don't think this is the best way to fix it. Ideally, get_pages call should work for device pages with ref count equal to 0 too. Otherwise, we could overwrite refcounter if someone else is grabbing the page concurrently. I was thinking to add a special condition in get_pages for dev pages. This could also fix the insert_page -> get_page call from a DAX device. Regards, Alex S. Also, what about the other places where is_device_private_page() is called? Don't they need to be updated to call is_device_page() instead? One of my goals for this patch was
Re: [BUG - BISECTED] display not detected anymore
On Wed, Aug 18, 2021 at 01:20:54PM +0200, Heiko Carstens wrote: > On Sat, Aug 14, 2021 at 02:46:27PM +0200, Heiko Carstens wrote: > > Hello, > > > > I have Fedora 33 running, and with the Fedore kernel update from 5.11 > > series to 5.12 my external monitor was not detected anymore. Same is > > true with the Fedora supplied 5.13 kernel version. > > > > So I tried with vanilla kernel 5.11 and latest git head from Linus' > > tree. 5.11 works while latest git head does not. Bisecting the problem > > points to commit 32c3d9b0f51e ("Merge tag 'drm-intel-next-2021-01-27' > > of git://anongit.freedesktop.org/drm/drm-intel into drm-next"). > > > > Unfortunately it is a merge commit, so it looks like conflicting > > changes have been made in the parent branches. > > > > Hardware in use: > > > > - ThinkPad X1 Yoga 4th / Carbon 7th > > - Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz > > > > The Thinkpad is connected to a ThinkPad Thunderbolt 3 Dock with a > > Thunderbolt cable and a monitor (Eizo EV3285) is connected via > > Displayport to the docking station. > > > > The monitor is detected and works without any problems (4k@60HZ) > > before the above mentioned merge commit. With the commit and > > afterwards it is not detected anymore and only the Thinkpad builtin > > display can be used. > > > > Any idea what went wrong? I can provide more information, or test > > debug patches if wanted. Just let me know. > > So looks like I made a mistake when bisecting (it literally took me > two days due to the low power machine, even with a minimized kernel > config). Anyway, looking into it again the first bad commit is > > ef79d62b5ce5 ("drm/i915: Encapsulate dbuf state handling harder") > > With that commit the display is not detected anymore, one commit > before that it still works. So this one seems to be broken. > > Ville, Stanislav, any idea how to fix this? > > commit ef79d62b5ce53851901d6c1d21b74cbb9e27219b > Author: Ville Syrjälä > Date: Fri Jan 22 22:56:32 2021 +0200 > > drm/i915: Encapsulate dbuf state handling harder That has nothing to do with display detection, so very mysterious. Please file a bug at https://gitlab.freedesktop.org/drm/intel/issues/new boot with drm.debug=0xe with both good and bad kernels and attach the dmesg from both to the bug. -- Ville Syrjälä Intel
Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
On Thu, Aug 19, 2021 at 01:47:57PM -0300, Jason Gunthorpe wrote: > On Thu, Aug 19, 2021 at 09:19:08AM -0700, Kees Cook wrote: > > On Thu, Aug 19, 2021 at 09:27:16AM -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 17, 2021 at 11:05:26PM -0700, Kees Cook wrote: > > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > > > field bounds checking for memset(), avoid intentionally writing across > > > > neighboring fields. > > > > > > > > Add struct_group() to mark region of struct mlx5_ib_mr that should be > > > > initialized to zero. > > > > > > > > Cc: Leon Romanovsky > > > > Cc: Doug Ledford > > > > Cc: Jason Gunthorpe > > > > Cc: linux-r...@vger.kernel.org > > > > Signed-off-by: Kees Cook > > > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h > > > > b/drivers/infiniband/hw/mlx5/mlx5_ib.h > > > > index bf20a388eabe..f63bf204a7a1 100644 > > > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h > > > > @@ -644,6 +644,7 @@ struct mlx5_ib_mr { > > > > struct ib_umem *umem; > > > > > > > > /* This is zero'd when the MR is allocated */ > > > > + struct_group(cleared, > > > > union { > > > > /* Used only while the MR is in the cache */ > > > > struct { > > > > @@ -691,12 +692,13 @@ struct mlx5_ib_mr { > > > > bool is_odp_implicit; > > > > }; > > > > }; > > > > + ); > > > > }; > > > > > > > > /* Zero the fields in the mr that are variant depending on usage */ > > > > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr) > > > > { > > > > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, > > > > out)); > > > > + memset(&mr->cleared, 0, sizeof(mr->cleared)); > > > > } > > > > > > Why not use the memset_after(mr->umem) here? > > > > I can certainly do that instead. In this series I've tended to opt > > for groupings so the position of future struct member additions are > > explicitly chosen. (i.e. reducing the chance that a zeroing of the new > > member be a surprise.) > > I saw the earlier RDMA patches where using other memset techniques > though? Were there flex arrays or something that made groups infeasible? Which do you mean? When doing the conversions I tended to opt for struct_group() since it provides more robust "intentionality". Strictly speaking, the new memset helpers are doing field-spanning writes, but the "clear to the end" pattern was so common it made sense to add the helpers, as they're a bit less disruptive. It's totally up to you! :) -- Kees Cook
Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
On 8/19/21 4:52 AM, Christoph Hellwig wrote: > On Fri, Aug 13, 2021 at 11:59:22AM -0500, Tom Lendacky wrote: >> While the name suggests this is intended mainly for guests, it will >> also be used for host memory encryption checks in place of sme_active(). > > Which suggest that the name is not good to start with. Maybe protected > hardware, system or platform might be a better choice? > >> +static inline bool prot_guest_has(unsigned int attr) >> +{ >> +#ifdef CONFIG_AMD_MEM_ENCRYPT >> +if (sme_me_mask) >> +return amd_prot_guest_has(attr); >> +#endif >> + >> +return false; >> +} > > Shouldn't this be entirely out of line? I did it as inline originally because the presence of the function will be decided based on the ARCH_HAS_PROTECTED_GUEST config. For now, that is only selected by the AMD memory encryption support, so if I went out of line I could put in mem_encrypt.c. But with TDX wanting to also use it, it would have to be in an always built file with some #ifdefs or in its own file that is conditionally built based on the ARCH_HAS_PROTECTED_GUEST setting (they've already tried building with ARCH_HAS_PROTECTED_GUEST=y and AMD_MEM_ENCRYPT not set). To take it out of line, I'm leaning towards the latter, creating a new file that is built based on the ARCH_HAS_PROTECTED_GUEST setting. > >> +/* 0x800 - 0x8ff reserved for AMD */ >> +#define PATTR_SME 0x800 >> +#define PATTR_SEV 0x801 >> +#define PATTR_SEV_ES0x802 > > Why do we need reservations for a purely in-kernel namespace? > > And why are you overoading a brand new generic API with weird details > of a specific implementation like this? There was some talk about this on the mailing list where TDX and SEV may need to be differentiated, so we wanted to reserve a range of values per technology. I guess I can remove them until they are actually needed. Thanks, Tom >
Re: [PATCH v2 04/12] powerpc/pseries/svm: Add a powerpc version of prot_guest_has()
On 8/19/21 4:55 AM, Christoph Hellwig wrote: > On Fri, Aug 13, 2021 at 11:59:23AM -0500, Tom Lendacky wrote: >> +static inline bool prot_guest_has(unsigned int attr) > > No reall need to have this inline. In fact I'd suggest we havea the > prototype in a common header so that everyone must implement it out > of line. I'll do the same thing I end up doing for x86. Thanks, Tom >
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
19.08.2021 16:07, Ulf Hansson пишет: > On Wed, 18 Aug 2021 at 17:43, Dmitry Osipenko wrote: >> >> 18.08.2021 13:08, Ulf Hansson пишет: >>> On Wed, 18 Aug 2021 at 11:50, Viresh Kumar wrote: On 18-08-21, 11:41, Ulf Hansson wrote: > On Wed, 18 Aug 2021 at 11:14, Viresh Kumar > wrote: >> What we need here is just configure. So something like this then: >> >> - genpd->get_performance_state() >> -> dev_pm_opp_get_current_opp() //New API >> -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate); >> >> This can be done just once from probe() then. > > How would dev_pm_opp_get_current_opp() work? Do you have a suggestion? The opp core already has a way of finding current OPP, that's what Dmitry is trying to use here. It finds it using clk_get_rate(), if that is zero, it picks the lowest freq possible. > I am sure I understand the problem. When a device is getting probed, > it needs to consume power, how else can the corresponding driver > successfully probe it? Dmitry can answer that better, but a device doesn't necessarily need to consume energy in probe. It can consume bus clock, like APB we have, but the more energy consuming stuff can be left disabled until the time a user comes up. Probe will just end up registering the driver and initializing it. >>> >>> That's perfectly fine, as then it's likely that it won't vote for an >>> OPP, but can postpone that as well. >>> >>> Perhaps the problem is rather that the HW may already carry a non-zero >>> vote made from a bootloader. If the consumer driver tries to clear >>> that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would >>> still not lead to any updates of the performance state in genpd, >>> because genpd internally has initialized the performance-state to >>> zero. >> >> We don't need to discover internal SoC devices because we use >> device-tree on ARM. For most devices power isn't required at a probe >> time because probe function doesn't touch h/w at all, thus devices are >> left in suspended state after probe. >> >> We have three components comprising PM on Tegra: >> >> 1. Power gate >> 2. Clock state >> 3. Voltage state >> >> GENPD on/off represents the 'power gate'. >> >> Clock and reset are controlled by device drivers using clk and rst APIs. >> >> Voltage state is represented by GENPD's performance level. >> >> GENPD core assumes that at a first rpm-resume of a consumer device, its >> genpd_performance=0. Not true for Tegra because h/w of the device is >> preconfigured to a non-zero perf level initially, h/w may not support >> zero level at all. > > I think you may be misunderstanding genpd's behaviour around this, but > let me elaborate. > > In genpd_runtime_resume(), we try to restore the performance state for > the device that genpd_runtime_suspend() *may* have dropped earlier. > That means, if genpd_runtime_resume() is called prior > genpd_runtime_suspend() for the first time, it means that > genpd_runtime_resume() will *not* restore a performance state, but > instead just leave the performance state as is for the device (see > genpd_restore_performance_state()). > > In other words, a consumer driver may use the following sequence to > set an initial performance state for the device during ->probe(): > > ... > rate = clk_get_rate() > dev_pm_opp_set_rate(rate) > > pm_runtime_enable() > pm_runtime_resume_and_get() > ... > > Note that, it's the consumer driver's responsibility to manage device > specific resources, in its ->runtime_suspend|resume() callbacks. > Typically that means dealing with clock gating/ungating, for example. > > In the other scenario where a consumer driver prefers to *not* call > pm_runtime_resume_and_get() in its ->probe(), because it doesn't need > to power on the device to complete probing, then we don't want to vote > for an OPP at all - and we also want the performance state for the > device in genpd to be set to zero. Correct? Yes > Is this the main problem you are trying to solve, because I think this > doesn't work out of the box as of today? The main problem is that the restored performance state is zero for the first genpd_runtime_resume(), while it's not zero from the h/w perspective. > There is another concern though, but perhaps it's not a problem after > all. Viresh told us that dev_pm_opp_set_rate() may turn on resources > like clock/regulators. That could certainly be problematic, in > particular if the device and its genpd have OPP tables associated with > it and the consumer driver wants to follow the above sequence in > probe. dev_pm_opp_set_rate() won't enable clocks and regulators, but it may change the clock rate and voltage. This is also platform/driver specific because it's up to OPP user how to configure OPP table. On Tegra we only assign clock to OPP table, regulators are unused. > Viresh, can you please chime in here and elaborate on some o
Re: [PATCH v2 03/12] x86/sev: Add an x86 version of prot_guest_has()
On 8/19/21 11:33 AM, Tom Lendacky wrote: There was some talk about this on the mailing list where TDX and SEV may need to be differentiated, so we wanted to reserve a range of values per technology. I guess I can remove them until they are actually needed. In TDX also we have similar requirements and we need some flags for TDX specific checks. So I think it is fine to leave some space for vendor flags. -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region
On Thu, Aug 19, 2021 at 10:33:43AM +0530, Lazar, Lijo wrote: > On 8/19/2021 5:29 AM, Kees Cook wrote: > > On Wed, Aug 18, 2021 at 05:12:28PM +0530, Lazar, Lijo wrote: > > > > > > On 8/18/2021 11:34 AM, Kees Cook wrote: > > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > > > field bounds checking for memcpy(), memmove(), and memset(), avoid > > > > intentionally writing across neighboring fields. > > > > > > > > Use struct_group() in structs: > > > > struct atom_smc_dpm_info_v4_5 > > > > struct atom_smc_dpm_info_v4_6 > > > > struct atom_smc_dpm_info_v4_7 > > > > struct atom_smc_dpm_info_v4_10 > > > > PPTable_t > > > > so the grouped members can be referenced together. This will allow > > > > memcpy() and sizeof() to more easily reason about sizes, improve > > > > readability, and avoid future warnings about writing beyond the end of > > > > the first member. > > > > > > > > "pahole" shows no size nor member offset changes to any structs. > > > > "objdump -d" shows no object code changes. > > > > > > > > Cc: "Christian König" > > > > Cc: "Pan, Xinhui" > > > > Cc: David Airlie > > > > Cc: Daniel Vetter > > > > Cc: Hawking Zhang > > > > Cc: Feifei Xu > > > > Cc: Lijo Lazar > > > > Cc: Likun Gao > > > > Cc: Jiawei Gu > > > > Cc: Evan Quan > > > > Cc: amd-...@lists.freedesktop.org > > > > Cc: dri-devel@lists.freedesktop.org > > > > Signed-off-by: Kees Cook > > > > Acked-by: Alex Deucher > > > > Link: > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.com&data=04%7C01%7Clijo.lazar%40amd.com%7C3861f20094074bf7328808d962a433f2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649279701053991%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=386LcfJJGfQfHsXBuK17LMqxJ2nFtGoj%2FUjoN2ZtJd0%3D&reserved=0 > > > > --- > > > >drivers/gpu/drm/amd/include/atomfirmware.h | 9 - > > > >.../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h| 3 ++- > > > >drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++- > > > >.../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++- > > > > > > Hi Kees, > > > > Hi! Thanks for looking into this. > > > > > The headers which define these structs are firmware/VBIOS interfaces and > > > are > > > picked directly from those components. There are difficulties in grouping > > > them to structs at the original source as that involves other component > > > changes. > > > > So, can you help me understand this a bit more? It sounds like these are > > generated headers, yes? I'd like to understand your constraints and > > weight them against various benefits that could be achieved here. > > > > The groupings I made do appear to be roughly documented already, > > for example: > > > > struct atom_common_table_header table_header; > > // SECTION: BOARD PARAMETERS > > + struct_group(dpm_info, > > > > Something emitted the "BOARD PARAMETERS" section heading as a comment, > > so it likely also would know where it ends, yes? The good news here is > > that for the dpm_info groups, they all end at the end of the existing > > structs, see: > > struct atom_smc_dpm_info_v4_5 > > struct atom_smc_dpm_info_v4_6 > > struct atom_smc_dpm_info_v4_7 > > struct atom_smc_dpm_info_v4_10 > > > > The matching regions in the PPTable_t structs are similarly marked with a > > "BOARD PARAMETERS" section heading comment: > > > > --- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h > > +++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h > > @@ -643,6 +643,7 @@ typedef struct { > > // SECTION: BOARD PARAMETERS > > // SVI2 Board Parameters > > + struct_group(v4_6, > > uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU > > will request. Multiple steps are taken if voltage change exceeds this value. > > uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU > > will request. Multiple steps are taken if voltage change exceeds this value. > > @@ -728,10 +729,10 @@ typedef struct { > > uint32_t BoardVoltageCoeffB;// decode by /1000 > > uint32_t BoardReserved[7]; > > + ); > > // Padding for MMHUB - do not modify this > > uint32_t MmHubPadding[8]; // SMU internal use > > - > > } PPTable_t; > > > > Where they end seems known as well (the padding switches from a "Board" > > to "MmHub" prefix at exactly the matching size). > > > > So, given that these regions are already known by the export tool, how > > about just updating the export tool to emit a struct there? I imagine > > the problem with this would be the identifier churn needed, but that's > > entirely mechanical. > > > > However, I'm curious about another aspect of these regions: they are, > > by definition, the same. Why isn't there a single str
Re: [PATCH v6 02/13] mm: remove extra ZONE_DEVICE struct page refcount
Am 2021-08-19 um 2:00 p.m. schrieb Sierra Guiza, Alejandro (Alex): > > On 8/18/2021 2:28 PM, Ralph Campbell wrote: >> On 8/17/21 5:35 PM, Felix Kuehling wrote: >>> Am 2021-08-17 um 8:01 p.m. schrieb Ralph Campbell: On 8/12/21 11:31 PM, Alex Sierra wrote: > From: Ralph Campbell > > ZONE_DEVICE struct pages have an extra reference count that > complicates the > code for put_page() and several places in the kernel that need to > check the > reference count to see that a page is not being used (gup, > compaction, > migration, etc.). Clean up the code so the reference count doesn't > need to > be treated specially for ZONE_DEVICE. > > v2: > AS: merged this patch in linux 5.11 version > > v5: > AS: add condition at try_grab_page to check for the zone device type, > while > page ref counter is checked less/equal to zero. In case of device > zone, pages > ref counter are initialized to zero. > > Signed-off-by: Ralph Campbell > Signed-off-by: Alex Sierra > --- > arch/powerpc/kvm/book3s_hv_uvmem.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_dmem.c | 2 +- > fs/dax.c | 4 +- > include/linux/dax.h | 2 +- > include/linux/memremap.h | 7 +-- > include/linux/mm.h | 13 + > lib/test_hmm.c | 2 +- > mm/internal.h | 8 +++ > mm/memremap.c | 68 > +++--- > mm/migrate.c | 5 -- > mm/page_alloc.c | 3 ++ > mm/swap.c | 45 ++--- > 12 files changed, 46 insertions(+), 115 deletions(-) > I haven't seen a response to the issues I raised back at v3 of this series. https://lore.kernel.org/linux-mm/4f6dd918-d79b-1aa7-3a4c-caa67ddc2...@nvidia.com/ Did I miss something? >>> I think part of the response was that we did more testing. Alex added >>> support for DEVICE_GENERIC pages to test_hmm and he ran DAX tests >>> recommended by Theodore Tso. In that testing he ran into a WARN_ON_ONCE >>> about a zero page refcount in try_get_page. The fix is in the latest >>> version of patch 2. But it's already obsolete because John Hubbard is >>> about to remove that function altogether. >>> >>> I think the issues you raised were more uncertainty than known bugs. It >>> seems the fact that you can have DAX pages with 0 refcount is a feature >>> more than a bug. >>> >>> Regards, >>> Felix >> >> Did you test on a system without CONFIG_ARCH_HAS_PTE_SPECIAL defined? >> In that case, mmap() of a DAX device will call insert_page() which calls >> get_page() which would trigger VM_BUG_ON_PAGE(). >> >> I can believe it is OK for PTE_SPECIAL page table entries to have no >> struct page or that MEMORY_DEVICE_GENERIC struct pages be mapped with >> a zero reference count using insert_pfn(). > Hi Ralph, > We have tried the DAX tests with and without > CONFIG_ARCH_HAS_PTE_SPECIAL defined. > Apparently none of the tests touches that condition for a DAX device. > Of course, > that doesn't mean it could happen. > > Regards, > Alex S. > >> >> >> I find it hard to believe that other MM developers don't see an issue >> with a struct page with refcount == 0 and mapcount == 1. >> >> I don't see where init_page_count() is being called for the >> MEMORY_DEVICE_GENERIC or MEMORY_DEVICE_PRIVATE struct pages the AMD >> driver allocates and passes to migrate_vma_setup(). >> Looks like svm_migrate_get_vram_page() needs to call init_page_count() >> instead of get_page(). (I'm looking at branch >> origin/alexsierrag/device_generic >> https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver.git > Yes, you're right. My bad. Thanks for catching this up. I didn't > realize I was missing > to define CONFIG_DEBUG_VM on my build. Therefore this BUG was never > caught. > It worked after I replaced get_pages by init_page_count at > svm_migrate_get_vram_page. However, I don't think this is the best way > to fix it. > Ideally, get_pages call should work for device pages with ref count > equal to 0 > too. Otherwise, we could overwrite refcounter if someone else is > grabbing the page > concurrently. I think using init_page_count in svm_migrate_get_vram_page is the right answer. This is where the page first gets allocated and initialized (data migrated into it). I think nobody should have or try to take a reference to the page before that. We should probably also add a VM_BUG_ON_PAGE(page_ref_count(page) != 0) before calling init_page_count to make sure of that. > I was thinking to add a special condition in get_pages for dev pages. > This could > also fix the insert_page -> get_page call from a DAX device. [+Theodore] I got lost trying to understand how DAX counts page references an
[PATCH] drm/amd/pm: And destination bounds checking to struct copy
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memcpy(), memmove(), and memset(), avoid intentionally writing across neighboring fields. The "Board Parameters" members of the structs: struct atom_smc_dpm_info_v4_5 struct atom_smc_dpm_info_v4_6 struct atom_smc_dpm_info_v4_7 struct atom_smc_dpm_info_v4_10 are written to the corresponding members of the corresponding PPTable_t variables, but they lack destination size bounds checking, which means the compiler cannot verify at compile time that this is an intended and safe memcpy(). Since the header files are effectively immutable[1] and a struct_group() cannot be used, nor a common struct referenced by both sides of the memcpy() arguments, add a new helper, memcpy_trailing(), to perform the bounds checking at compile time. Replace the open-coded memcpy()s with memcpy_trailing() which includes enough context for the bounds checking. "objdump -d" shows no object code changes. [1] https://lore.kernel.org/lkml/e56aad3c-a06f-da07-f491-a894a570d...@amd.com Cc: Lijo Lazar Cc: "Christian König" Cc: "Pan, Xinhui" Cc: David Airlie Cc: Daniel Vetter Cc: Hawking Zhang Cc: Feifei Xu Cc: Likun Gao Cc: Jiawei Gu Cc: Evan Quan Cc: amd-...@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: Kees Cook Link: https://lore.kernel.org/lkml/cadnq5_npb8uyvd+r4uhgf-w8-cqj3joodjvijr_y9w9wqj7...@mail.gmail.com --- Alex, I dropped your prior Acked-by, since the implementation is very different. If you're still happy with it, I can add it back. :) --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 25 +++ .../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 ++--- .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 8 +++--- .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c| 5 ++-- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 96e895d6be35..4605934a4fb7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1446,4 +1446,29 @@ static inline int amdgpu_in_reset(struct amdgpu_device *adev) { return atomic_read(&adev->in_gpu_reset); } + +/** + * memcpy_trailing - Copy the end of one structure into the middle of another + * + * @dst: Pointer to destination struct + * @first_dst_member: The member name in @dst where the overwrite begins + * @last_dst_member: The member name in @dst where the overwrite ends after + * @src: Pointer to the source struct + * @first_src_member: The member name in @src where the copy begins + * + */ +#define memcpy_trailing(dst, first_dst_member, last_dst_member, \ + src, first_src_member) \ +({\ + size_t __src_offset = offsetof(typeof(*(src)), first_src_member); \ + size_t __src_size = sizeof(*(src)) - __src_offset; \ + size_t __dst_offset = offsetof(typeof(*(dst)), first_dst_member); \ + size_t __dst_size = offsetofend(typeof(*(dst)), last_dst_member) - \ + __dst_offset; \ + BUILD_BUG_ON(__src_size != __dst_size);\ + __builtin_memcpy((u8 *)(dst) + __dst_offset, \ +(u8 *)(src) + __src_offset, \ +__dst_size); \ +}) + #endif diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c index 8ab58781ae13..1918e6232319 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c @@ -465,10 +465,8 @@ static int arcturus_append_powerplay_table(struct smu_context *smu) if ((smc_dpm_table->table_header.format_revision == 4) && (smc_dpm_table->table_header.content_revision == 6)) - memcpy(&smc_pptable->MaxVoltageStepGfx, - &smc_dpm_table->maxvoltagestepgfx, - sizeof(*smc_dpm_table) - offsetof(struct atom_smc_dpm_info_v4_6, maxvoltagestepgfx)); - + memcpy_trailing(smc_pptable, MaxVoltageStepGfx, BoardReserved, + smc_dpm_table, maxvoltagestepgfx); return 0; } diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c index 2e5d3669652b..b738042e064d 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c @@ -431,16 +431,16 @@ static int navi10_append_powerplay_table(struct smu_context *smu) switch (smc_dpm_table->table_header.content_revision) { case 5: /* nv10 and nv14 */ - memcpy(smc_pptable->I2cControllers
Re: [v1 2/2] dt-bindings: drm/panel: boe-tv101wum-nl6: Support enabling a 3.3V rail
On Thu, 19 Aug 2021 20:48:44 +0800, yangcong wrote: > The auo,b101uan08.3 panel (already supported by this driver) has > a 3.3V rail that needs to be turned on. For previous users of > this panel this voltage was directly output by pmic. On a new > user (the not-yet-upstream sc7180-trogdor-mrbland board) we need > to turn the 3.3V rail on. > > Signed-off-by: yangcong > --- > .../devicetree/bindings/display/panel/boe,tv101wum-nl6.yaml | 4 > 1 file changed, 4 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/boe,tv101wum-nl6.example.dt.yaml: panel@0: 'pp3300-supply' is a required property From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/boe,tv101wum-nl6.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1518662 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Re: [Intel-gfx] [PATCH 02/27] drm/i915/guc: Fix outstanding G2H accounting
On 8/18/2021 11:16 PM, Matthew Brost wrote: A small race that could result in incorrect accounting of the number of outstanding G2H. Basically prior to this patch we did not increment the number of outstanding G2H if we encoutered a GT reset while sending a H2G. This was incorrect as the context state had already been updated to anticipate a G2H response thus the counter should be incremented. Also always use helper when decrementing this value. Fixes: f4eb1f3fe946 ("drm/i915/guc: Ensure G2H response has space in buffer") Signed-off-by: Matthew Brost Cc: --- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 24 ++- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 69faa39da178..32c414aa9009 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -352,6 +352,12 @@ static inline void set_lrc_desc_registered(struct intel_guc *guc, u32 id, xa_unlock_irqrestore(&guc->context_lookup, flags); } +static void decr_outstanding_submission_g2h(struct intel_guc *guc) +{ + if (atomic_dec_and_test(&guc->outstanding_submission_g2h)) + wake_up_all(&guc->ct.wq); +} + static int guc_submission_send_busy_loop(struct intel_guc *guc, const u32 *action, u32 len, @@ -360,11 +366,13 @@ static int guc_submission_send_busy_loop(struct intel_guc *guc, { int err; - err = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop); - - if (!err && g2h_len_dw) + if (g2h_len_dw) atomic_inc(&guc->outstanding_submission_g2h); + err = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop); + if (err == -EBUSY && g2h_len_dw) + decr_outstanding_submission_g2h(guc); + here you're special casing -EBUSY, which kind of implies that the caller needs to handle this differently, but most callers seem to ignore the return code. Is the counter handled somewhere else in case of EBUSY? if so, please add a comment about it. Daniele return err; } @@ -616,7 +624,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) init_sched_state(ce); if (pending_enable || destroyed || deregister) { - atomic_dec(&guc->outstanding_submission_g2h); + decr_outstanding_submission_g2h(guc); if (deregister) guc_signal_context_fence(ce); if (destroyed) { @@ -635,7 +643,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) intel_engine_signal_breadcrumbs(ce->engine); } intel_context_sched_disable_unpin(ce); - atomic_dec(&guc->outstanding_submission_g2h); + decr_outstanding_submission_g2h(guc); spin_lock_irqsave(&ce->guc_state.lock, flags); guc_blocked_fence_complete(ce); spin_unlock_irqrestore(&ce->guc_state.lock, flags); @@ -2583,12 +2591,6 @@ g2h_context_lookup(struct intel_guc *guc, u32 desc_idx) return ce; } -static void decr_outstanding_submission_g2h(struct intel_guc *guc) -{ - if (atomic_dec_and_test(&guc->outstanding_submission_g2h)) - wake_up_all(&guc->ct.wq); -} - int intel_guc_deregister_done_process_msg(struct intel_guc *guc, const u32 *msg, u32 len)
Re: [Intel-gfx] [PATCH 02/27] drm/i915/guc: Fix outstanding G2H accounting
On Thu, Aug 19, 2021 at 02:31:51PM -0700, Daniele Ceraolo Spurio wrote: > > > On 8/18/2021 11:16 PM, Matthew Brost wrote: > > A small race that could result in incorrect accounting of the number > > of outstanding G2H. Basically prior to this patch we did not increment > > the number of outstanding G2H if we encoutered a GT reset while sending > > a H2G. This was incorrect as the context state had already been updated > > to anticipate a G2H response thus the counter should be incremented. > > > > Also always use helper when decrementing this value. > > > > Fixes: f4eb1f3fe946 ("drm/i915/guc: Ensure G2H response has space in > > buffer") > > Signed-off-by: Matthew Brost > > Cc: > > --- > > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 24 ++- > > 1 file changed, 13 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > index 69faa39da178..32c414aa9009 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > @@ -352,6 +352,12 @@ static inline void set_lrc_desc_registered(struct > > intel_guc *guc, u32 id, > > xa_unlock_irqrestore(&guc->context_lookup, flags); > > } > > +static void decr_outstanding_submission_g2h(struct intel_guc *guc) > > +{ > > + if (atomic_dec_and_test(&guc->outstanding_submission_g2h)) > > + wake_up_all(&guc->ct.wq); > > +} > > + > > static int guc_submission_send_busy_loop(struct intel_guc *guc, > > const u32 *action, > > u32 len, > > @@ -360,11 +366,13 @@ static int guc_submission_send_busy_loop(struct > > intel_guc *guc, > > { > > int err; > > - err = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop); > > - > > - if (!err && g2h_len_dw) > > + if (g2h_len_dw) > > atomic_inc(&guc->outstanding_submission_g2h); > > + err = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop); > > + if (err == -EBUSY && g2h_len_dw) > > + decr_outstanding_submission_g2h(guc); > > + > > here you're special casing -EBUSY, which kind of implies that the caller > needs to handle this differently, but most callers seem to ignore the return > code. Is the counter handled somewhere else in case of EBUSY? if so, please > add a comment about it. > Good catch, this is a dead code path. Will delete. Matt > Daniele > > > return err; > > } > > @@ -616,7 +624,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct > > intel_guc *guc) > > init_sched_state(ce); > > if (pending_enable || destroyed || deregister) { > > - atomic_dec(&guc->outstanding_submission_g2h); > > + decr_outstanding_submission_g2h(guc); > > if (deregister) > > guc_signal_context_fence(ce); > > if (destroyed) { > > @@ -635,7 +643,7 @@ static void scrub_guc_desc_for_outstanding_g2h(struct > > intel_guc *guc) > > intel_engine_signal_breadcrumbs(ce->engine); > > } > > intel_context_sched_disable_unpin(ce); > > - atomic_dec(&guc->outstanding_submission_g2h); > > + decr_outstanding_submission_g2h(guc); > > spin_lock_irqsave(&ce->guc_state.lock, flags); > > guc_blocked_fence_complete(ce); > > spin_unlock_irqrestore(&ce->guc_state.lock, flags); > > @@ -2583,12 +2591,6 @@ g2h_context_lookup(struct intel_guc *guc, u32 > > desc_idx) > > return ce; > > } > > -static void decr_outstanding_submission_g2h(struct intel_guc *guc) > > -{ > > - if (atomic_dec_and_test(&guc->outstanding_submission_g2h)) > > - wake_up_all(&guc->ct.wq); > > -} > > - > > int intel_guc_deregister_done_process_msg(struct intel_guc *guc, > > const u32 *msg, > > u32 len) >