Re: [Intel-gfx] [RFC PATCH v2 0/1] Replace shmem memory region and object backend
On Fri, 27 May 2022 at 16:37, Adrian Larumbe wrote: > > On 18.05.2022 16:00, Matthew Auld wrote: > > On Tue, 17 May 2022 at 21:45, Adrian Larumbe > > wrote: > > > > > > This patch is a second attempt at eliminating the old shmem memory region > > > and GEM object backend, in favour of a TTM-based one that is able to > > > manage > > > objects placed on both system and local memory. > > > > > > Questions addressed since previous revision: > > > > > > * Creating an anonymous vfs mount for shmem files in TTM > > > * Fixing LLC caching properties and bit 17 swizzling before setting a TTM > > > bo's pages when calling get_pages > > > * Added handling of phys backend from TTM functions > > > * Added pread callback to TTM gem object backend > > > * In shmem_create_from_object, ensuring an shmem object we just got a filp > > > for has its pages marked dirty and accessed. Otherwise, the engine won't > > > be > > > able to read the initial state and a GPU hung will ensue > > > > > > However, one of the issues persists: > > > > > > Many GPU hungs in machines of GEN <= 5. My assumption is this has > > > something > > > to do with a caching pitfall, but everywhere across the TTM backend code > > > I've tried to handle object creation and getting its pages with the same > > > set of caching and coherency properties as in the old shmem backend. > > > > Some thoughts in case it's helpful: > > > > - We still look to be trampling the cache_level etc after object > > creation. AFAICT i915_ttm_adjust_gem_after_move can be called in > > various places after creation. > > I traced this function so that I could see everywhere it was being called when > running some IGT tests and kmscube, and the only place it was setting a > caching > coherence value other than none was in init_status_page, where I915_CACHE_LLC > is > picked as the cache coherency mode even for machines that do not have > it. However this code was already present before my changes and didn't seem to > cause any issues, so I don't think it's involved. Yeah, I guess it's a different issue, but we still need to somehow ensure we never trample the cache_level etc on integrated platforms after object creation. On non-LLC platforms(not discrete) it's normal to set CACHE_LLC for certain types of buffers. And even on LLC platforms it's normal to set CACHE_NONE, like for display buffers, since the display engine is not coherent. For reference we can have something like: bb = gem_create() <-- assume non-llc so must be CACHE_NONE gem_set_caching(bb, CACHED) <-- should now be CACHE_LLC/L3 ptr = mmap_wb(bb); *ptr = BATCH_BUFFER_END <-- gem_after_move() resets to CACHE_NONE execbuf(bb) <-- doesn't see the BATCH_BUFFER_END > > > - The i915_ttm_pwrite hook won't play nice on non-llc platforms, since > > it doesn't force a clflush or keep track of the writes with > > cache_dirty. The existing ->shmem_pwrite hook only works because we > > are guaranteed to have not yet populated the mm.pages, and on non-llc > > platforms we always force a clflush in __set_pages(). In > > i915_ttm_pwrite we are now just calling pin_pages() and then writing > > through the page-cache without forcing a clflush, or ensuring that we > > leave cache_dirty set. Also AFAIK the whole point of shmem_pwrite was > > to avoid needing to populate the entire object like when calling > > pin_pages(). Would it make sense to just fallback to using > > i915_gem_shmem_pwrite, which should already take care of the required > > flushing? > > > > For reference a common usage pattern is something like: > > > > bb = gem_create() <-- assume non-llc so must be CACHE_NONE > > gem_write(bb, BATCH_BUFFER_END) <-- might use cached pwrite internally > > execbuf(bb) <-- doesn't see BATCH_BUFFER_END if we don't clflush > > It turns out this was the underlying issue causing machines GEN <= 5 to break > in > pretty much every single test. It seems that for older hardware, IGT tests > would > pick pwrite as the preferred method for filling BO's from UM, so my code > wasn't > calling clfush after getting the pages and writing the UM pointer data into > the > shmem file. > > The way I fixed it was creating an shmem file for this and other cases when > it's > required by the existing code, but without getting the pages. In a way, I > just > cut through the usual TTM populate path and instance an shmem object so that I > can avoid caching issues. > > Thanks a lot for catching this one! > > > > > > > Adrian Larumbe (1): > > > drm/i915: Replace shmem memory region and object backend with TTM > > > > > > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 12 +- > > > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 32 +- > > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 +- > > > drivers/gpu/drm/i915/gem/i915_gem_phys.c | 32 +- > > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c| 390 +-- > > > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 267 - > > >
Re: [Intel-gfx] [PATCH 03/10] drm/i915/uapi: expose the avail tracking
On 26/05/2022 09:33, Tvrtko Ursulin wrote:
On 26/05/2022 09:10, Matthew Auld wrote:
On 26/05/2022 08:58, Tvrtko Ursulin wrote:
On 25/05/2022 19:43, Matthew Auld wrote:
Vulkan would like to have a rough measure of how much device memory can
in theory be allocated. Also add unallocated_cpu_visible_size to track
the visible portion, in case the device is using small BAR.
I have concerns here that it isn't useful and could even be
counter-productive. If we give unprivileged access it may be
considered a side channel, but if we "lie" (report total region size)
to unprivileged clients (like in this patch), then they don't
co-operate well and end trashing.
Is Vulkan really sure it wants this and why?
Lionel pointed out:
https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VK_EXT_memory_budget.html
So this query would provide
VkPhysicalDeviceMemoryBudgetPropertiesEXT::heapBudget. Apart that it
wouldn't since we thought to lie. And apart that it's text says:
"""
...how much total memory from each heap the current process can use at
any given time, before allocations may start failing or causing
performance degradation. The values may change based on other activity
in the system that is outside the scope and control of the Vulkan
implementation.
"""
It acknowledges itself in the second sentence that the first sentence is
questionable.
And VkPhysicalDeviceMemoryBudgetPropertiesEXT::heapUsage would be still
missing and would maybe come via fdinfo? Or you plan to add it to this
same query later?
IIUC the heapUsage is something like per app usage, which already looks
to be tracked in anv, although I don't think it knows if stuff is
actually resident or not. The heapBudget looks to then be roughly the
heapUsage + info.unallocated.
I like to source knowledge of best practices from the long established
world of CPU scheduling and process memory management. Is anyone aware
of this kind of techniques there - applications actively looking at free
memory data from /proc/meminfo and dynamically adjusting their runtime
behaviour based on it? And that they are not single application on a
dedicated system type of things.
Or perhaps this Vk extension is envisaged for exactly those kind of
scenarios? However if so then userspace can know all this data from
probed size and the data set it created.
Also note that the existing behaviour was to lie. I'm not sure what's
the best option here.
Uapi reserved -1 for unknown so we could do that?
AFAICT we can do that for the info.unallocated_cpu_visible, but not for
the existing info.unallocated without maybe breaking something?
Regards,
Tvrtko
Regards,
Tvrtko
Testcase: igt@i915_query@query-regions-unallocated
Testcase: igt@i915_query@query-regions-sanity-check
Signed-off-by: Matthew Auld
Cc: Thomas Hellström
Cc: Lionel Landwerlin
Cc: Tvrtko Ursulin
Cc: Jon Bloomfield
Cc: Daniel Vetter
Cc: Jordan Justen
Cc: Kenneth Graunke
Cc: Akeem G Abodunrin
---
drivers/gpu/drm/i915/i915_query.c | 10 +-
drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 20
drivers/gpu/drm/i915/i915_ttm_buddy_manager.h | 3 ++
drivers/gpu/drm/i915/intel_memory_region.c | 14 +
drivers/gpu/drm/i915/intel_memory_region.h | 3 ++
include/uapi/drm/i915_drm.h | 31
++-
6 files changed, 79 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 9aa0b28aa6ee..e095c55f4d4b 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -502,7 +502,15 @@ static int query_memregion_info(struct
drm_i915_private *i915,
else
info.probed_cpu_visible_size = mr->total;
- info.unallocated_size = mr->avail;
+ if (perfmon_capable()) {
+ intel_memory_region_avail(mr,
+ _size,
+ _cpu_visible_size);
+ } else {
+ info.unallocated_size = info.probed_size;
+ info.unallocated_cpu_visible_size =
+ info.probed_cpu_visible_size;
+ }
if (__copy_to_user(info_ptr, , sizeof(info)))
return -EFAULT;
diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
index a5109548abc0..aa5c91e44438 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
@@ -365,6 +365,26 @@ u64 i915_ttm_buddy_man_visible_size(struct
ttm_resource_manager *man)
return bman->visible_size;
}
+/**
+ * i915_ttm_buddy_man_visible_size - Query the avail tracking for
the manager.
+ *
+ * @man: The buddy allocator ttm manager
+ * @avail: The total available memory in pages for the entire manager.
+ * @visible_avail: The total available memory in pages for the CPU
visible
+ * portion. Note that this will always give the same value
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Treat DMRRS as static DRRS
On Mon, 30 May 2022, Jani Nikula wrote:
> On Fri, 27 May 2022, Ville Syrjala wrote:
>> From: Ville Syrjälä
>>
>> Some machines declare DRRS type = seamless, DRRS = no, DMRRS = yes.
>> I *think* DMRRS stands for "dynamcic media refresh rate", and
>> I suspect the way it's meant to work is that it lets the driver
>> switch refresh rates to match the frame rate for media playback.
>> Obviously for us all that kind of policy stuff is entirely up to
>> userspace, so the only thing we may do is make the extra refresh
>> rate(s) available.
>>
>> So let's treat this case as just static DRRS for now. In the
>> future We might want to differentiate the "seamless w/ downclocking"
>> vs. "seamless w/o downclocking" cases so that we could do seamless
>> refresh rate changes for systems that only claim to support DMRRS.
>>
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/125
>> Signed-off-by: Ville Syrjälä
>
> Acked-by: Jani Nikula
Oh, the reasoning for some Acked-bys instead of Reviewed-bys today in
this and another series:
They could all be r-b in the sense that they do what they say on the
box. But I don't really have the information to confirm they are the
right thing to do. I'm acking "let's go with this, and see where it gets
us".
Make sense?
BR,
Jani.
>
>> ---
>> drivers/gpu/drm/i915/display/intel_bios.c | 24 +++
>> 1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
>> b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 0774238e429b..c42b9e7d0dce 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1279,8 +1279,16 @@ parse_panel_driver_features(struct drm_i915_private
>> *i915,
>> * static DRRS is 0 and DRRS not supported is represented by
>> * driver->drrs_enabled=false
>> */
>> -if (!driver->drrs_enabled)
>> -panel->vbt.drrs_type = DRRS_TYPE_NONE;
>> +if (!driver->drrs_enabled && panel->vbt.drrs_type !=
>> DRRS_TYPE_NONE) {
>> +/*
>> + * FIXME Should DMRRS perhaps be treated as seamless
>> + * but without the automatic downclocking?
>> + */
>> +if (driver->dmrrs_enabled)
>> +panel->vbt.drrs_type = DRRS_TYPE_STATIC;
>> +else
>> +panel->vbt.drrs_type = DRRS_TYPE_NONE;
>> +}
>>
>> panel->vbt.psr.enable = driver->psr_enabled;
>> }
>> @@ -1310,8 +1318,16 @@ parse_power_conservation_features(struct
>> drm_i915_private *i915,
>> * static DRRS is 0 and DRRS not supported is represented by
>> * power->drrs & BIT(panel_type)=false
>> */
>> -if (!(power->drrs & BIT(panel_type)))
>> -panel->vbt.drrs_type = DRRS_TYPE_NONE;
>> +if (!(power->drrs & BIT(panel_type)) && panel->vbt.drrs_type !=
>> DRRS_TYPE_NONE) {
>> +/*
>> + * FIXME Should DMRRS perhaps be treated as seamless
>> + * but without the automatic downclocking?
>> + */
>> +if (power->dmrrs & BIT(panel_type))
>> +panel->vbt.drrs_type = DRRS_TYPE_STATIC;
>> +else
>> +panel->vbt.drrs_type = DRRS_TYPE_NONE;
>> +}
>>
>> if (i915->vbt.version >= 232)
>> panel->vbt.edp.hobl = power->hobl & BIT(panel_type);
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/3] lib/igt_device_scan: Free filtered devices in igt_devices_free
On Mon, May 30, 2022 at 04:40:06AM +0200, Zbigniew Kempczyński wrote:
> On Fri, May 27, 2022 at 11:50:40AM +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin
> >
> > Fix a possible oversight.
>
> Yes, properly coded in igt_device_scan() only. Thanks for spotting this.
>
> >
> > Signed-off-by: Tvrtko Ursulin
> > ---
> > lib/igt_device_scan.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> > index 3c23fe0eb520..a30433ae2cff 100644
> > --- a/lib/igt_device_scan.c
> > +++ b/lib/igt_device_scan.c
> > @@ -814,6 +814,11 @@ void igt_devices_free(void)
> > igt_device_free(dev);
> > free(dev);
> > }
> > +
> > + igt_list_for_each_entry_safe(dev, tmp, _devs.filtered, link) {
> > + igt_list_del(>link);
> > + free(dev);
> > + }
>
> Small nit - I would change the order (filtered list I would remove first).
> igt_device_free() also frees dev->devnode, ... so if we would change the
> code to be more "parallel" it would be better to avoid use-after-free.
>
> With this:
>
> Reviewed-by: Zbigniew Kempczyński
Tvrtko is away this week so I made this change and merged.
--
Petri Latvala
>
> --
> Zbigniew
>
> > }
> >
> > /**
> > --
> > 2.32.0
> >
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Treat DMRRS as static DRRS
On Fri, 27 May 2022, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Some machines declare DRRS type = seamless, DRRS = no, DMRRS = yes.
> I *think* DMRRS stands for "dynamcic media refresh rate", and
> I suspect the way it's meant to work is that it lets the driver
> switch refresh rates to match the frame rate for media playback.
> Obviously for us all that kind of policy stuff is entirely up to
> userspace, so the only thing we may do is make the extra refresh
> rate(s) available.
>
> So let's treat this case as just static DRRS for now. In the
> future We might want to differentiate the "seamless w/ downclocking"
> vs. "seamless w/o downclocking" cases so that we could do seamless
> refresh rate changes for systems that only claim to support DMRRS.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/125
> Signed-off-by: Ville Syrjälä
Acked-by: Jani Nikula
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 24 +++
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> b/drivers/gpu/drm/i915/display/intel_bios.c
> index 0774238e429b..c42b9e7d0dce 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1279,8 +1279,16 @@ parse_panel_driver_features(struct drm_i915_private
> *i915,
>* static DRRS is 0 and DRRS not supported is represented by
>* driver->drrs_enabled=false
>*/
> - if (!driver->drrs_enabled)
> - panel->vbt.drrs_type = DRRS_TYPE_NONE;
> + if (!driver->drrs_enabled && panel->vbt.drrs_type !=
> DRRS_TYPE_NONE) {
> + /*
> + * FIXME Should DMRRS perhaps be treated as seamless
> + * but without the automatic downclocking?
> + */
> + if (driver->dmrrs_enabled)
> + panel->vbt.drrs_type = DRRS_TYPE_STATIC;
> + else
> + panel->vbt.drrs_type = DRRS_TYPE_NONE;
> + }
>
> panel->vbt.psr.enable = driver->psr_enabled;
> }
> @@ -1310,8 +1318,16 @@ parse_power_conservation_features(struct
> drm_i915_private *i915,
>* static DRRS is 0 and DRRS not supported is represented by
>* power->drrs & BIT(panel_type)=false
>*/
> - if (!(power->drrs & BIT(panel_type)))
> - panel->vbt.drrs_type = DRRS_TYPE_NONE;
> + if (!(power->drrs & BIT(panel_type)) && panel->vbt.drrs_type !=
> DRRS_TYPE_NONE) {
> + /*
> + * FIXME Should DMRRS perhaps be treated as seamless
> + * but without the automatic downclocking?
> + */
> + if (power->dmrrs & BIT(panel_type))
> + panel->vbt.drrs_type = DRRS_TYPE_STATIC;
> + else
> + panel->vbt.drrs_type = DRRS_TYPE_NONE;
> + }
>
> if (i915->vbt.version >= 232)
> panel->vbt.edp.hobl = power->hobl & BIT(panel_type);
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH i-g-t 3/3] intel_gpu_top: Free all memory on exit
On Fri, May 27, 2022 at 11:50:42AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Be nice and explicitly free all memory on exit.
>
> Also fix a Valgrind reported unitilised conditional jump.
>
> Signed-off-by: Tvrtko Ursulin
> Cc: Petri Latvala
Reviewed-by: Petri Latvala
> ---
> tools/intel_gpu_top.c | 51 +++
> 1 file changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 26986a822bb7..997aff582ff7 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -437,6 +437,36 @@ static struct engines *discover_engines(char *device)
> return engines;
> }
>
> +static void free_engines(struct engines *engines)
> +{
> + struct pmu_counter **pmu, *free_list[] = {
> + >r_gpu,
> + >r_pkg,
> + >imc_reads,
> + >imc_writes,
> + NULL
> + };
> + unsigned int i;
> +
> + for (pmu = _list[0]; *pmu; pmu++) {
> + if ((*pmu)->present)
> + free((char *)(*pmu)->units);
> + }
> +
> + for (i = 0; i < engines->num_engines; i++) {
> + struct engine *engine = engine_ptr(engines, i);
> +
> + free((char *)engine->name);
> + free((char *)engine->short_name);
> + free((char *)engine->display_name);
> + }
> +
> + closedir(engines->root);
> +
> + free(engines->class);
> + free(engines);
> +}
> +
> #define _open_pmu(type, cnt, pmu, fd) \
> ({ \
> int fd__; \
> @@ -1073,7 +1103,7 @@ static size_t freadat2buf(char *buf, const size_t sz,
> DIR *at, const char *name)
> return count;
> }
>
> -static struct clients *scan_clients(struct clients *clients)
> +static struct clients *scan_clients(struct clients *clients, bool display)
> {
> struct dirent *proc_dent;
> struct client *c;
> @@ -1181,7 +1211,7 @@ next:
> break;
> }
>
> - return display_clients(clients);
> + return display ? display_clients(clients) : clients;
> }
>
> static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
> @@ -2391,7 +2421,7 @@ static void process_stdin(unsigned int timeout_us)
>
> static bool has_drm_fdinfo(const struct igt_device_card *card)
> {
> - struct drm_client_fdinfo info;
> + struct drm_client_fdinfo info = { };
> unsigned int cnt;
> int fd;
>
> @@ -2572,7 +2602,7 @@ int main(int argc, char **argv)
> }
>
> pmu_sample(engines);
> - scan_clients(clients);
> + scan_clients(clients, false);
> codename = igt_device_get_pretty_name(, false);
>
> while (!stop_top) {
> @@ -2599,7 +2629,7 @@ int main(int argc, char **argv)
> pmu_sample(engines);
> t = (double)(engines->ts.cur - engines->ts.prev) / 1e9;
>
> - disp_clients = scan_clients(clients);
> + disp_clients = scan_clients(clients, true);
>
> if (stop_top)
> break;
> @@ -2649,21 +2679,24 @@ int main(int argc, char **argv)
> pops->close_struct();
> }
>
> - if (stop_top)
> - break;
> -
> if (disp_clients != clients)
> free_clients(disp_clients);
>
> + if (stop_top)
> + break;
> +
> if (output_mode == INTERACTIVE)
> process_stdin(period_us);
> else
> usleep(period_us);
> }
>
> + if (clients)
> + free_clients(clients);
> +
> free(codename);
> err:
> - free(engines);
> + free_engines(engines);
> free(pmu_device);
> exit:
> igt_devices_free();
> --
> 2.32.0
>
Re: [Intel-gfx] [PATCH i-g-t 2/3] lib/drm_fdinfo: Ensure buffer is null terminated
On Fri, May 27, 2022 at 11:50:41AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Ensure buffer is null terminated at the point where the read ended and not
> at the end of the whole buffer. Otherwise string parsing can stray into
> un-initialised memory.
>
> Signed-off-by: Tvrtko Ursulin
Reviewed-by: Petri Latvala
> ---
> lib/igt_drm_fdinfo.c | 8
> lib/igt_drm_fdinfo.h | 4 ++--
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
> index b422f67a4ace..250d9e8917f2 100644
> --- a/lib/igt_drm_fdinfo.c
> +++ b/lib/igt_drm_fdinfo.c
> @@ -44,12 +44,12 @@ static size_t read_fdinfo(char *buf, const size_t sz, int
> at, const char *name)
> if (fd < 0)
> return 0;
>
> - buf[sz - 1] = 0;
> - count = read(fd, buf, sz);
> - buf[sz - 1] = 0;
> + count = read(fd, buf, sz - 1);
> + if (count > 0)
> + buf[count - 1] = 0;
> close(fd);
>
> - return count;
> + return count > 0 ? count : 0;
> }
>
> static int parse_engine(char *line, struct drm_client_fdinfo *info,
> diff --git a/lib/igt_drm_fdinfo.h b/lib/igt_drm_fdinfo.h
> index 5db63e28b07e..8759471615bd 100644
> --- a/lib/igt_drm_fdinfo.h
> +++ b/lib/igt_drm_fdinfo.h
> @@ -46,7 +46,7 @@ struct drm_client_fdinfo {
> * igt_parse_drm_fdinfo: Parses the drm fdinfo file
> *
> * @drm_fd: DRM file descriptor
> - * @info: Structure to populate with read data
> + * @info: Structure to populate with read data. Must be zeroed.
> *
> * Returns the number of valid drm fdinfo keys found or zero if not all
> * mandatory keys were present or no engines found.
> @@ -58,7 +58,7 @@ unsigned int igt_parse_drm_fdinfo(int drm_fd, struct
> drm_client_fdinfo *info);
> *
> * @dir: File descriptor pointing to /proc/pid/fdinfo directory
> * @fd: String representation of the file descriptor number to parse.
> - * @info: Structure to populate with read data
> + * @info: Structure to populate with read data. Must be zeroed.
> *
> * Returns the number of valid drm fdinfo keys found or zero if not all
> * mandatory keys were present or no engines found.
> --
> 2.32.0
>
Re: [Intel-gfx] [PATCH 5/6] drm/i915/bios: Define more BDB contents
On Fri, 27 May 2022, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Add a bunch of new struff we're missing in various BDB blocks.
>
> TODO: Bunch of these might actually need to be taken
> into use...
Cc: Jouni, Lyude for some HDR backlight stuff below.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 50 ---
> 1 file changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 39109f204c6d..be99f585b1d0 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -564,7 +564,9 @@ struct bdb_driver_features {
> u16 tbt_enabled:1;
> u16 psr_enabled:1;
> u16 ips_enabled:1;
> - u16 reserved3:4;
> + u16 reserved3:1;
> + u16 dmrrs_enabled:1;
Should we start logging the version ranges here too, since it's obsolete
from 228. Kinda duplicating the info though. *shrug*.
> + u16 reserved4:2;
> u16 pc_feature_valid:1;
> } __packed;
>
> @@ -666,6 +668,16 @@ struct edp_full_link_params {
> u8 vswing:4;
> } __packed;
>
> +struct edp_apical_params {
> + u32 panel_oui;
> + u32 dpcd_base_address;
> + u32 dpcd_idridix_control_0;
> + u32 dpcd_option_select;
> + u32 dpcd_backlight;
> + u32 ambient_light;
> + u32 backlight_scale;
> +} __packed;
> +
> struct bdb_edp {
> struct edp_power_seq power_seqs[16];
> u32 color_depth;
> @@ -681,6 +693,9 @@ struct bdb_edp {
> struct edp_pwm_delays pwm_delays[16]; /* 186 */
> u16 full_link_params_provided; /* 199 */
> struct edp_full_link_params full_link_params[16]; /* 199 */
> + u16 apical_enable; /* 203 */
> + struct edp_apical_params apical_params[16]; /* 203 */
> + u16 edp_fast_link_training_rate[16];/* 224 */
Another eDP port link rate param would go here? Could be added in
another patch.
> } __packed;
>
> /*
> @@ -717,6 +732,7 @@ struct bdb_lvds_options {
>
> u16 lcdvcc_s0_enable; /* 200 */
> u32 rotation; /* 228 */
> + u32 position; /* 240 */
> } __packed;
>
> /*
> @@ -843,13 +859,22 @@ struct bdb_lfp_backlight_data {
> u8 level[16]; /* Obsolete from 234+ */
> struct lfp_backlight_control_method backlight_control[16];
> struct lfp_brightness_level brightness_level[16]; /* 234+
> */
> - struct lfp_brightness_level brightness_min_level[16]; /* 234+ */
> - u8 brightness_precision_bits[16];
> /* 236+ */
> + struct lfp_brightness_level brightness_min_level[16]; /* 234+
> */
> + u8 brightness_precision_bits[16]; /* 236+
> */
> + u16 hdr_dpcd_refresh_timeout[16]; /* 239+
> */
Jouni, Lyude, this is probably interesting to you:
"""
This table of values (for 16 panels, 1 value per panel) is used to
specify the time required by the TCON (with Intel HDR Aux Interface
Support) to refresh the DPCD set with Intel HDR CAPS (DPCD offset:
340h-344h).
The value is in units of 10 us(microseconds).
"""
> } __packed;
>
> /*
> * Block 44 - LFP Power Conservation Features Block
> */
> +struct lfp_features {
Nit, maybe lfp_power_features.
Anyway,
Reviewed-by: Jani Nikula
> + u8 reserved1:1;
> + u8 power_conservation_pref:3;
> + u8 reserved2:1;
> + u8 lace_enabled_status:1;
> + u8 lace_support:1;
> + u8 als_enable:1;
> +} __packed;
>
> struct als_data_entry {
> u16 backlight_adjust;
> @@ -861,10 +886,16 @@ struct aggressiveness_profile_entry {
> u8 lace_aggressiveness : 4;
> } __packed;
>
> +struct aggressiveness_profile2_entry {
> + u8 opst_aggressiveness : 4;
> + u8 elp_aggressiveness : 4;
> +} __packed;
> +
> struct bdb_lfp_power {
> - u8 lfp_feature_bits;
> + struct lfp_features features;
> struct als_data_entry als[5];
> - u8 lace_aggressiveness_profile;
> + u8 lace_aggressiveness_profile:3;
> + u8 reserved1:5;
> u16 dpst;
> u16 psr;
> u16 drrs;
> @@ -876,6 +907,9 @@ struct bdb_lfp_power {
> struct aggressiveness_profile_entry aggressiveness[16];
> u16 hobl; /* 232+ */
> u16 vrr_feature_enabled; /* 233+ */
> + u16 elp; /* 247+ */
> + u16 opst; /* 247+ */
> + struct aggressiveness_profile2_entry aggressiveness2[16]; /* 247+ */
> } __packed;
>
> /*
> @@ -885,8 +919,10 @@ struct bdb_lfp_power {
> #define MAX_MIPI_CONFIGURATIONS 6
>
> struct bdb_mipi_config {
> - struct mipi_config config[MAX_MIPI_CONFIGURATIONS];
> - struct mipi_pps_data pps[MAX_MIPI_CONFIGURATIONS];
>
Re: [Intel-gfx] [PATCH 4/6] drm/i915/bios: Fix aggressiveness typos
On Fri, 27 May 2022, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Fix various typos around "aggressiveness". Note that
> the VBT spec also sometimes missspells it as
> "agressiveness" so I guess that's where some of the typos
> came from.
Could nitpick unrelated things like space before ":" in bitfields or the
struct naming, but this patch does what it says on the box.
Reviewed-by: Jani Nikula
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 4b98bab3b890..39109f204c6d 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -856,9 +856,9 @@ struct als_data_entry {
> u16 lux;
> } __packed;
>
> -struct agressiveness_profile_entry {
> - u8 dpst_agressiveness : 4;
> - u8 lace_agressiveness : 4;
> +struct aggressiveness_profile_entry {
> + u8 dpst_aggressiveness : 4;
> + u8 lace_aggressiveness : 4;
> } __packed;
>
> struct bdb_lfp_power {
> @@ -873,7 +873,7 @@ struct bdb_lfp_power {
> u16 dmrrs;
> u16 adb;
> u16 lace_enabled_status;
> - struct agressiveness_profile_entry aggressivenes[16];
> + struct aggressiveness_profile_entry aggressiveness[16];
> u16 hobl; /* 232+ */
> u16 vrr_feature_enabled; /* 233+ */
> } __packed;
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 3/6] drm/i915: Accept more fixed modes with VRR panels
On Fri, 27 May 2022, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> It seem that when dealing with VRR capable eDP panels we need
> to accept fixed modes with variable vblank length (which is what
> VRR varies dynamically). Typically the preferred mode seems to be
> a non-VRR more (lowish dotclock/refresh rate + short vblank).
>
> We also have examples where it looks like even the hblank length
> is a bit different between the preferred mode vs. VRR mode(s).
> So let's just accept anything that has matching hdisp+vdisp+flags.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/125
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_dp.c| 3 +-
> drivers/gpu/drm/i915/display/intel_lvds.c | 3 +-
> drivers/gpu/drm/i915/display/intel_panel.c | 48 --
> drivers/gpu/drm/i915/display/intel_panel.h | 3 +-
> drivers/gpu/drm/i915/display/intel_sdvo.c | 2 +-
> 5 files changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 1bc1f6458e81..b8e2d3cd4d68 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5217,7 +5217,8 @@ static bool intel_edp_init_connector(struct intel_dp
> *intel_dp,
> IS_ERR(edid) ? NULL : edid);
>
> intel_panel_add_edid_fixed_modes(intel_connector,
> - intel_connector->panel.vbt.drrs_type
> != DRRS_TYPE_NONE);
> + intel_connector->panel.vbt.drrs_type
> != DRRS_TYPE_NONE,
> + intel_vrr_is_capable(intel_connector));
>
> /* MSO requires information from the EDID */
> intel_edp_mso_init(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c
> b/drivers/gpu/drm/i915/display/intel_lvds.c
> index 595f03343939..e55802b45461 100644
> --- a/drivers/gpu/drm/i915/display/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/display/intel_lvds.c
> @@ -972,7 +972,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>
> /* Try EDID first */
> intel_panel_add_edid_fixed_modes(intel_connector,
> - intel_connector->panel.vbt.drrs_type
> != DRRS_TYPE_NONE);
> + intel_connector->panel.vbt.drrs_type
> != DRRS_TYPE_NONE,
> + false);
>
> /* Failed to get EDID, what about VBT? */
> if (!intel_panel_preferred_fixed_mode(intel_connector))
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> b/drivers/gpu/drm/i915/display/intel_panel.c
> index f55e4eafd74e..963b24293b50 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -71,6 +71,27 @@ intel_panel_fixed_mode(struct intel_connector *connector,
> return best_mode;
> }
>
> +static bool is_alt_drrs_mode(const struct drm_display_mode *mode,
> + const struct drm_display_mode *preferred_mode)
> +{
> + return drm_mode_match(mode, preferred_mode,
> + DRM_MODE_MATCH_TIMINGS |
> + DRM_MODE_MATCH_FLAGS |
> + DRM_MODE_MATCH_3D_FLAGS) &&
> + mode->clock != preferred_mode->clock;
> +}
> +
> +static bool is_alt_vrr_mode(const struct drm_display_mode *mode,
> + const struct drm_display_mode *preferred_mode)
> +{
> + return drm_mode_match(mode, preferred_mode,
> + DRM_MODE_MATCH_FLAGS |
> + DRM_MODE_MATCH_3D_FLAGS) &&
> + mode->hdisplay == preferred_mode->hdisplay &&
> + mode->vdisplay == preferred_mode->vdisplay &&
> + mode->clock != preferred_mode->clock;
> +}
> +
> const struct drm_display_mode *
> intel_panel_downclock_mode(struct intel_connector *connector,
> const struct drm_display_mode *adjusted_mode)
> @@ -83,7 +104,8 @@ intel_panel_downclock_mode(struct intel_connector
> *connector,
> list_for_each_entry(fixed_mode, >panel.fixed_modes, head) {
> int vrefresh = drm_mode_vrefresh(fixed_mode);
>
> - if (vrefresh >= min_vrefresh && vrefresh < max_vrefresh) {
> + if (is_alt_drrs_mode(fixed_mode, adjusted_mode) &&
> + vrefresh >= min_vrefresh && vrefresh < max_vrefresh) {
> max_vrefresh = vrefresh;
> best_mode = fixed_mode;
> }
> @@ -151,16 +173,17 @@ int intel_panel_compute_config(struct intel_connector
> *connector,
> }
>
> static bool is_alt_fixed_mode(const struct drm_display_mode *mode,
> - const struct drm_display_mode *preferred_mode)
> + const struct drm_display_mode *preferred_mode,
> + bool has_vrr)
> {
> -
Re: [Intel-gfx] [PATCH 2/6] drm/i915: Print out rejected fixed modes
On Fri, 27 May 2022, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> To help with debugging DRRS/VRR panel init let's dump out all
> the fixed modes we rejected for whatever reason.
>
> Signed-off-by: Ville Syrjälä
Reviewed-by: Jani Nikula
> ---
> drivers/gpu/drm/i915/display/intel_panel.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> b/drivers/gpu/drm/i915/display/intel_panel.c
> index d055e4118558..f55e4eafd74e 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -217,6 +217,10 @@ static void intel_panel_destroy_probed_modes(struct
> intel_connector *connector)
> struct drm_display_mode *mode, *next;
>
> list_for_each_entry_safe(mode, next, >base.probed_modes,
> head) {
> + drm_dbg_kms(>drm,
> + "[CONNECTOR:%d:%s] not using EDID mode: "
> DRM_MODE_FMT "\n",
> + connector->base.base.id, connector->base.name,
> + DRM_MODE_ARG(mode));
> list_del(>head);
> drm_mode_destroy(>drm, mode);
> }
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/syncobj: flatten dma_fence_chains on transfer
On 30/05/2022 14:40, Christian König wrote:
Am 30.05.22 um 12:09 schrieb Lionel Landwerlin:
On 30/05/2022 12:52, Christian König wrote:
Am 25.05.22 um 23:59 schrieb Lucas De Marchi:
On Wed, May 25, 2022 at 12:38:51PM +0200, Christian König wrote:
Am 25.05.22 um 11:35 schrieb Lionel Landwerlin:
[SNIP]
Err... Let's double check with my colleagues.
It seems we're running into a test failure in IGT with this
patch, but now I have doubts that it's where the problem lies.
Yeah, exactly that's what I couldn't understand as well.
What you describe above should still work fine.
Thanks for taking a look into this,
Christian.
With some additional prints:
[ 210.742634] Console: switching to colour dummy device 80x25
[ 210.742686] [IGT] syncobj_timeline: executing
[ 210.756988] [IGT] syncobj_timeline: starting subtest
transfer-timeline-point
[ 210.757364] [drm:drm_syncobj_transfer_ioctl] *ERROR* adding
fence0 signaled=1
[ 210.764543] [drm:drm_syncobj_transfer_ioctl] *ERROR* resulting
array fence signaled=0
[ 210.800469] [IGT] syncobj_timeline: exiting, ret=98
[ 210.825426] Console: switching to colour frame buffer device 240x67
still learning this part of the code but AFAICS the problem is because
when we are creating the array, the 'signaled' doesn't propagate to
the
array.
Yeah, but that is intentionally. The array should only signal when
requested.
I still don't get what the test case here is checking.
There must be something I don't know about fence arrays.
You seem to say that creating an array of signaled fences will not
make the array signaled.
Exactly that, yes. The array delays it's signaling until somebody asks
for it.
In other words the fences inside the array are check only after
someone calls dma_fence_enable_sw_signaling() which in turn calls
dma_fence_array_enable_signaling().
It is certainly possible that nobody does that in the drm_syncobj and
because of this the array never signals.
Regards,
Christian.
Thanks,
Yeah I guess dma_fence_enable_sw_signaling() is never called for sw_sync.
Don't we also want to call it right at the end of
drm_syncobj_flatten_chain() ?
-Lionel
This is the situation with this IGT test.
We started with a syncobj with point 1 & 2 signaled.
We take point 2 and import it as a new point 3 on the same syncobj.
We expect point 3 to be signaled as well and it's not.
Thanks,
-Lionel
Regards,
Christian.
dma_fence_array_create() {
...
atomic_set(>num_pending, signal_on_any ? 1 : num_fences);
...
}
This is not considering the fact that some of the fences could already
have been signaled as is the case in the
igt@syncobj_timeline@transfer-timeline-point
test. See
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11693/shard-dg1-12/igt@syncobj_timel...@transfer-timeline-point.html
Quick patch on this function fixes it for me:
-8<
Subject: [PATCH] dma-buf: Honor already signaled fences on array
creation
When creating an array, array->num_pending is marked with the
number of
fences. However the fences could alredy have been signaled. Propagate
num_pending to the array by looking at each individual fence the array
contains.
Signed-off-by: Lucas De Marchi
---
drivers/dma-buf/dma-fence-array.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-fence-array.c
b/drivers/dma-buf/dma-fence-array.c
index 5c8a7084577b..32f491c32fa0 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -158,6 +158,8 @@ struct dma_fence_array
*dma_fence_array_create(int num_fences,
{
struct dma_fence_array *array;
size_t size = sizeof(*array);
+ unsigned num_pending = 0;
+ struct dma_fence **f;
WARN_ON(!num_fences || !fences);
@@ -173,7 +175,14 @@ struct dma_fence_array
*dma_fence_array_create(int num_fences,
init_irq_work(>work, irq_dma_fence_array_work);
array->num_fences = num_fences;
- atomic_set(>num_pending, signal_on_any ? 1 : num_fences);
+
+ for (f = fences; f < fences + num_fences; f++)
+ num_pending += !dma_fence_is_signaled(*f);
+
+ if (signal_on_any)
+ num_pending = !!num_pending;
+
+ atomic_set(>num_pending, num_pending);
array->fences = fences;
array->base.error = PENDING_ERROR;
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Parse VRR capability from VBT
On Fri, 27 May 2022, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> VBT seems to have an extra flag for VRR vs. not. Let's consult
> that for eDP panels.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 5 +
> .../drm/i915/display/intel_display_types.h| 2 ++
> drivers/gpu/drm/i915/display/intel_vrr.c | 22 ++-
> 3 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> b/drivers/gpu/drm/i915/display/intel_bios.c
> index 337277ae3dae..0774238e429b 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1293,6 +1293,8 @@ parse_power_conservation_features(struct
> drm_i915_private *i915,
> const struct bdb_lfp_power *power;
> u8 panel_type = panel->vbt.panel_type;
>
> + panel->vbt.vrr = true; /* matches Windows behaviour */
> +
> if (i915->vbt.version < 228)
> return;
>
> @@ -1313,6 +1315,9 @@ parse_power_conservation_features(struct
> drm_i915_private *i915,
>
> if (i915->vbt.version >= 232)
> panel->vbt.edp.hobl = power->hobl & BIT(panel_type);
> +
> + if (i915->vbt.version >= 233)
> + panel->vbt.vrr = power->vrr_feature_enabled & BIT(panel_type);
> }
>
> static void
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index a27d66fd4383..7a76ba1a3b47 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -294,6 +294,8 @@ struct intel_vbt_panel_data {
> unsigned int lvds_dither:1;
> unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>
> + bool vrr;
> +
> u8 seamless_drrs_min_refresh_rate;
> enum drrs_type drrs_type;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 081e52dd6c4e..04250a0fec3c 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -15,19 +15,29 @@ bool intel_vrr_is_capable(struct intel_connector
> *connector)
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> struct intel_dp *intel_dp;
>
> - if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP &&
> - connector->base.connector_type != DRM_MODE_CONNECTOR_DisplayPort)
> - return false;
> -
> - intel_dp = intel_attached_dp(connector);
> /*
>* DP Sink is capable of VRR video timings if
>* Ignore MSA bit is set in DPCD.
>* EDID monitor range also should be atleast 10 for reasonable
>* Adaptive Sync or Variable Refresh Rate end user experience.
>*/
> + switch (connector->base.connector_type) {
> + case DRM_MODE_CONNECTOR_eDP:
> + if (!connector->panel.vbt.vrr)
> + return false;
> + fallthrough;
> + case DRM_MODE_CONNECTOR_DisplayPort:
> + intel_dp = intel_attached_dp(connector);
> +
> + if
> (!drm_dp_sink_can_do_video_without_timing_msa(intel_dp->dpcd))
> + return false;
> +
> + break;
> + default:
> + return false;
> + }
> +
> return HAS_VRR(i915) &&
Feels like !HAS_VRR() should be an early return at the top. But that's
not part of this patch.
Reviewed-by: Jani Nikula
> - drm_dp_sink_can_do_video_without_timing_msa(intel_dp->dpcd) &&
> info->monitor_range.max_vfreq - info->monitor_range.min_vfreq >
> 10;
> }
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 26/26] drm/i915: Round TMDS clock to nearest
On Tue, 03 May 2022, Ville Syrjala wrote: > From: Ville Syrjälä > > Use round-to-nearest behavour when calculating the TMDS clock. > Matches what we co for most other clock related things. *do Acked-by: Jani Nikula > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 3 ++- > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > b/drivers/gpu/drm/i915/display/intel_ddi.c > index 0cf2d4fba6a8..8b3e6ae85a08 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -330,7 +330,8 @@ int intel_crtc_dotclock(const struct intel_crtc_state > *pipe_config) > dotclock = intel_dotclock_calculate(pipe_config->port_clock, > _config->dp_m_n); > else if (pipe_config->has_hdmi_sink && pipe_config->pipe_bpp > 24) > - dotclock = pipe_config->port_clock * 24 / pipe_config->pipe_bpp; > + dotclock = DIV_ROUND_CLOSEST(pipe_config->port_clock * 24, > + pipe_config->pipe_bpp); > else > dotclock = pipe_config->port_clock; > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c > b/drivers/gpu/drm/i915/display/intel_hdmi.c > index 1ae09431f53a..0b04b3800cd4 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > @@ -1891,7 +1891,7 @@ int intel_hdmi_tmds_clock(int clock, int bpc, bool > ycbcr420_output) >* 1.5x for 12bpc >* 1.25x for 10bpc >*/ > - return clock * bpc / 8; > + return DIV_ROUND_CLOSEST(clock * bpc, 8); > } > > static bool intel_hdmi_source_bpc_possible(struct drm_i915_private *i915, > int bpc) -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 25/26] drm/i915: Round to closest in M/N calculations
On Tue, 03 May 2022, Ville Syrjala wrote: > From: Ville Syrjälä > > Rounding to nearest is what we do for most other clock calculations > so should probably do that for M/N too. > > TODO: GOP seems to truncate instead so fastboot is going to be > a PITA to get right. Not sure what to do about it yet. Meh. Damned if you do, damned if you don't. Acked-by: Jani Nikula > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/display/intel_display.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 89a7c8c1be28..c4257630a3fe 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -2856,7 +2856,7 @@ static void compute_m_n(u32 *ret_m, u32 *ret_n, > else > *ret_n = min_t(unsigned int, roundup_pow_of_two(n), > DATA_LINK_N_MAX); > > - *ret_m = div_u64(mul_u32_u32(m, *ret_n), n); > + *ret_m = DIV_ROUND_CLOSEST_ULL(mul_u32_u32(m, *ret_n), n); > intel_reduce_m_n_ratio(ret_m, ret_n); > } > > @@ -4602,7 +4602,8 @@ int intel_dotclock_calculate(int link_freq, > if (!m_n->link_n) > return 0; > > - return div_u64(mul_u32_u32(m_n->link_m, link_freq), m_n->link_n); > + return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(m_n->link_m, link_freq), > + m_n->link_n); > } > > /* Returns the currently programmed mode of the given encoder. */ -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 24/26] drm/i915: Use a fixed N value always
On Tue, 03 May 2022, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Windows/BIOS always uses fixed N values. Let's match that
> behaviour.
>
> Allows us to also get rid of that constant_n quirk stuff.
>
> Signed-off-by: Ville Syrjälä
Reviewed-by: Jani Nikula
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 36 +---
> drivers/gpu/drm/i915/display/intel_display.h | 2 +-
> drivers/gpu/drm/i915/display/intel_dp.c | 10 +++---
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +-
> drivers/gpu/drm/i915/display/intel_fdi.c | 2 +-
> 5 files changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index f30bdcdd4c84..89a7c8c1be28 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2848,19 +2848,11 @@ intel_reduce_m_n_ratio(u32 *num, u32 *den)
> }
> }
>
> -static void compute_m_n(unsigned int m, unsigned int n,
> - u32 *ret_m, u32 *ret_n,
> - bool constant_n)
> +static void compute_m_n(u32 *ret_m, u32 *ret_n,
> + u32 m, u32 n, u32 constant_n)
> {
> - /*
> - * Several DP dongles in particular seem to be fussy about
> - * too large link M/N values. Give N value as 0x8000 that
> - * should be acceptable by specific devices. 0x8000 is the
> - * specified fixed N value for asynchronous clock mode,
> - * which the devices expect also in synchronous clock mode.
> - */
> if (constant_n)
> - *ret_n = DP_LINK_CONSTANT_N_VALUE;
> + *ret_n = constant_n;
> else
> *ret_n = min_t(unsigned int, roundup_pow_of_two(n),
> DATA_LINK_N_MAX);
>
> @@ -2872,22 +2864,28 @@ void
> intel_link_compute_m_n(u16 bits_per_pixel, int nlanes,
> int pixel_clock, int link_clock,
> struct intel_link_m_n *m_n,
> -bool constant_n, bool fec_enable)
> +bool fec_enable)
> {
> u32 data_clock = bits_per_pixel * pixel_clock;
>
> if (fec_enable)
> data_clock = intel_dp_mode_to_fec_clock(data_clock);
>
> + /*
> + * Windows/BIOS uses fixed M/N values always. Follow suit.
> + *
> + * Also several DP dongles in particular seem to be fussy
> + * about too large link M/N values. Presumably the 20bit
> + * value used by Windows/BIOS is acceptable to everyone.
> + */
> m_n->tu = 64;
> - compute_m_n(data_clock,
> - link_clock * nlanes * 8,
> - _n->data_m, _n->data_n,
> - constant_n);
> + compute_m_n(_n->data_m, _n->data_n,
> + data_clock, link_clock * nlanes * 8,
> + 0x800);
>
> - compute_m_n(pixel_clock, link_clock,
> - _n->link_m, _n->link_n,
> - constant_n);
> + compute_m_n(_n->link_m, _n->link_n,
> + pixel_clock, link_clock,
> + 0x8);
> }
>
> static void intel_panel_sanitize_ssc(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> b/drivers/gpu/drm/i915/display/intel_display.h
> index 187910d94ec6..862338b6c4fa 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -546,7 +546,7 @@ u8 intel_calc_active_pipes(struct intel_atomic_state
> *state,
> void intel_link_compute_m_n(u16 bpp, int nlanes,
> int pixel_clock, int link_clock,
> struct intel_link_m_n *m_n,
> - bool constant_n, bool fec_enable);
> + bool fec_enable);
> u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
> u32 pixel_format, u64 modifier);
> enum drm_mode_status
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 9385178c7fd6..d10f05d40360 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1878,7 +1878,7 @@ static bool can_enable_drrs(struct intel_connector
> *connector,
> static void
> intel_dp_drrs_compute_config(struct intel_connector *connector,
>struct intel_crtc_state *pipe_config,
> - int output_bpp, bool constant_n)
> + int output_bpp)
> {
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> const struct drm_display_mode *downclock_mode =
> @@ -1906,7 +1906,7 @@ intel_dp_drrs_compute_config(struct intel_connector
> *connector,
>
> intel_link_compute_m_n(output_bpp, pipe_config->lane_count, pixel_clock,
> pipe_config->port_clock, _config->dp_m2_n2,
> -constant_n,
Re: [Intel-gfx] [PATCH v10 0/4] Separate panel orientation property creating and value setting
On Mon, May 30, 2022 at 4:53 PM Hans de Goede wrote: > > Hi, > > On 5/30/22 10:19, Hsin-Yi Wang wrote: > > Some drivers, eg. mtk_drm and msm_drm, rely on the panel to set the > > orientation. Panel calls drm_connector_set_panel_orientation() to create > > orientation property and sets the value. However, connector properties > > can't be created after drm_dev_register() is called. The goal is to > > separate the orientation property creation, so drm drivers can create it > > earlier before drm_dev_register(). > > Sorry for jumping in pretty late in the discussion (based on the v10 > I seem to have missed this before). > > This sounds to me like the real issue here is that drm_dev_register() > is getting called too early? > Right. > To me it seems sensible to delay calling drm_dev_register() and > thus allowing userspace to start detecting available displays + > features until after the panel has been probed. > Most panels set this value very late, in .get_modes callback (since it is when the connector is known), though the value was known during panel probe. I think we can also let drm check if they have remote panel nodes: If there is a panel and the panel sets the orientation, let the drm read this value and set the property. Does this workflow sound reasonable? The corresponding patch to implement this: https://patchwork.kernel.org/project/linux-mediatek/patch/20220530113033.124072-1-hsi...@chromium.org/ Thanks > I see a devicetree patch in this series, so I guess that the panel > is described in devicetree. Especially in the case of devicetree > I would expect the kernel to have enough info to do the right > thing and make sure the panel is probed before calling > drm_dev_register() ? > > Regards, > > Hans > > > > > > > > After this series, drm_connector_set_panel_orientation() works like > > before. It won't affect existing callers of > > drm_connector_set_panel_orientation(). The only difference is that > > some drm drivers can call drm_connector_init_panel_orientation_property() > > earlier. > > > > Hsin-Yi Wang (4): > > gpu: drm: separate panel orientation property creating and value > > setting > > drm/mediatek: init panel orientation property > > drm/msm: init panel orientation property > > arm64: dts: mt8183: Add panel rotation > > > > .../arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 + > > drivers/gpu/drm/drm_connector.c | 58 ++- > > drivers/gpu/drm/mediatek/mtk_dsi.c| 7 +++ > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++ > > include/drm/drm_connector.h | 2 + > > 5 files changed, 59 insertions(+), 13 deletions(-) > > >
Re: [Intel-gfx] [PATCH] drm/syncobj: flatten dma_fence_chains on transfer
On 30/05/2022 12:52, Christian König wrote:
Am 25.05.22 um 23:59 schrieb Lucas De Marchi:
On Wed, May 25, 2022 at 12:38:51PM +0200, Christian König wrote:
Am 25.05.22 um 11:35 schrieb Lionel Landwerlin:
[SNIP]
Err... Let's double check with my colleagues.
It seems we're running into a test failure in IGT with this patch,
but now I have doubts that it's where the problem lies.
Yeah, exactly that's what I couldn't understand as well.
What you describe above should still work fine.
Thanks for taking a look into this,
Christian.
With some additional prints:
[ 210.742634] Console: switching to colour dummy device 80x25
[ 210.742686] [IGT] syncobj_timeline: executing
[ 210.756988] [IGT] syncobj_timeline: starting subtest
transfer-timeline-point
[ 210.757364] [drm:drm_syncobj_transfer_ioctl] *ERROR* adding fence0
signaled=1
[ 210.764543] [drm:drm_syncobj_transfer_ioctl] *ERROR* resulting
array fence signaled=0
[ 210.800469] [IGT] syncobj_timeline: exiting, ret=98
[ 210.825426] Console: switching to colour frame buffer device 240x67
still learning this part of the code but AFAICS the problem is because
when we are creating the array, the 'signaled' doesn't propagate to the
array.
Yeah, but that is intentionally. The array should only signal when
requested.
I still don't get what the test case here is checking.
There must be something I don't know about fence arrays.
You seem to say that creating an array of signaled fences will not make
the array signaled.
This is the situation with this IGT test.
We started with a syncobj with point 1 & 2 signaled.
We take point 2 and import it as a new point 3 on the same syncobj.
We expect point 3 to be signaled as well and it's not.
Thanks,
-Lionel
Regards,
Christian.
dma_fence_array_create() {
...
atomic_set(>num_pending, signal_on_any ? 1 : num_fences);
...
}
This is not considering the fact that some of the fences could already
have been signaled as is the case in the
igt@syncobj_timeline@transfer-timeline-point
test. See
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11693/shard-dg1-12/igt@syncobj_timel...@transfer-timeline-point.html
Quick patch on this function fixes it for me:
-8<
Subject: [PATCH] dma-buf: Honor already signaled fences on array
creation
When creating an array, array->num_pending is marked with the number of
fences. However the fences could alredy have been signaled. Propagate
num_pending to the array by looking at each individual fence the array
contains.
Signed-off-by: Lucas De Marchi
---
drivers/dma-buf/dma-fence-array.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-fence-array.c
b/drivers/dma-buf/dma-fence-array.c
index 5c8a7084577b..32f491c32fa0 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -158,6 +158,8 @@ struct dma_fence_array
*dma_fence_array_create(int num_fences,
{
struct dma_fence_array *array;
size_t size = sizeof(*array);
+ unsigned num_pending = 0;
+ struct dma_fence **f;
WARN_ON(!num_fences || !fences);
@@ -173,7 +175,14 @@ struct dma_fence_array
*dma_fence_array_create(int num_fences,
init_irq_work(>work, irq_dma_fence_array_work);
array->num_fences = num_fences;
- atomic_set(>num_pending, signal_on_any ? 1 : num_fences);
+
+ for (f = fences; f < fences + num_fences; f++)
+ num_pending += !dma_fence_is_signaled(*f);
+
+ if (signal_on_any)
+ num_pending = !!num_pending;
+
+ atomic_set(>num_pending, num_pending);
array->fences = fences;
array->base.error = PENDING_ERROR;
Re: [Intel-gfx] [PATCH v6 04/22] drm/panfrost: Fix shrinker list corruption by madvise IOCTL
On 27/05/2022 00:50, Dmitry Osipenko wrote:
> Calling madvise IOCTL twice on BO causes memory shrinker list corruption
> and crashes kernel because BO is already on the list and it's added to
> the list again, while BO should be removed from from the list before it's
> re-added. Fix it.
>
> Cc: sta...@vger.kernel.org
> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Signed-off-by: Dmitry Osipenko
Reviewed-by: Steven Price
> ---
> drivers/gpu/drm/panfrost/panfrost_drv.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
> b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 087e69b98d06..b1e6d238674f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -433,8 +433,8 @@ static int panfrost_ioctl_madvise(struct drm_device *dev,
> void *data,
>
> if (args->retained) {
> if (args->madv == PANFROST_MADV_DONTNEED)
> - list_add_tail(>base.madv_list,
> - >shrinker_list);
> + list_move_tail(>base.madv_list,
> +>shrinker_list);
> else if (args->madv == PANFROST_MADV_WILLNEED)
> list_del_init(>base.madv_list);
> }
[Intel-gfx] ✗ Fi.CI.BAT: failure for Separate panel orientation property creating and value setting (rev2)
== Series Details == Series: Separate panel orientation property creating and value setting (rev2) URL : https://patchwork.freedesktop.org/series/101530/ State : failure == Summary == CI Bug Log - changes from CI_DRM_11705 -> Patchwork_101530v2 Summary --- **FAILURE** Serious unknown changes coming with Patchwork_101530v2 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_101530v2, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/index.html Participating hosts (42 -> 46) -- Additional (4): bat-dg2-8 fi-rkl-11600 fi-bsw-nick fi-tgl-u2 Possible new issues --- Here are the unknown changes that may have been introduced in Patchwork_101530v2: ### IGT changes ### Possible regressions * igt@prime_self_import@basic-with_one_bo_two_files: - fi-kbl-soraka: [PASS][1] -> [INCOMPLETE][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11705/fi-kbl-soraka/igt@prime_self_import@basic-with_one_bo_two_files.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-kbl-soraka/igt@prime_self_import@basic-with_one_bo_two_files.html Known issues Here are the changes found in Patchwork_101530v2 that come from known issues: ### IGT changes ### Issues hit * igt@gem_huc_copy@huc-copy: - fi-tgl-u2: NOTRUN -> [SKIP][3] ([i915#2190]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-tgl-u2/igt@gem_huc_c...@huc-copy.html - fi-rkl-11600: NOTRUN -> [SKIP][4] ([i915#2190]) [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-rkl-11600/igt@gem_huc_c...@huc-copy.html * igt@gem_lmem_swapping@basic: - fi-rkl-11600: NOTRUN -> [SKIP][5] ([i915#4613]) +3 similar issues [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-rkl-11600/igt@gem_lmem_swapp...@basic.html * igt@gem_lmem_swapping@parallel-random-engines: - fi-bsw-nick:NOTRUN -> [SKIP][6] ([fdo#109271]) +48 similar issues [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-bsw-nick/igt@gem_lmem_swapp...@parallel-random-engines.html * igt@gem_tiled_pread_basic: - fi-rkl-11600: NOTRUN -> [SKIP][7] ([i915#3282]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-rkl-11600/igt@gem_tiled_pread_basic.html * igt@i915_pm_backlight@basic-brightness: - fi-rkl-11600: NOTRUN -> [SKIP][8] ([i915#3012]) [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-rkl-11600/igt@i915_pm_backli...@basic-brightness.html * igt@i915_selftest@live@gem: - fi-blb-e6850: NOTRUN -> [DMESG-FAIL][9] ([i915#4528]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-blb-e6850/igt@i915_selftest@l...@gem.html * igt@i915_selftest@live@hangcheck: - bat-dg1-6: [PASS][10] -> [DMESG-FAIL][11] ([i915#4494] / [i915#4957]) [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11705/bat-dg1-6/igt@i915_selftest@l...@hangcheck.html [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/bat-dg1-6/igt@i915_selftest@l...@hangcheck.html * igt@i915_suspend@basic-s2idle-without-i915: - fi-bdw-gvtdvm: NOTRUN -> [INCOMPLETE][12] ([i915#4817]) [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-bdw-gvtdvm/igt@i915_susp...@basic-s2idle-without-i915.html * igt@i915_suspend@basic-s3-without-i915: - fi-rkl-11600: NOTRUN -> [INCOMPLETE][13] ([i915#5982]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-rkl-11600/igt@i915_susp...@basic-s3-without-i915.html * igt@kms_chamelium@hdmi-edid-read: - fi-tgl-u2: NOTRUN -> [SKIP][14] ([fdo#109284] / [fdo#111827]) +7 similar issues [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-tgl-u2/igt@kms_chamel...@hdmi-edid-read.html - fi-rkl-11600: NOTRUN -> [SKIP][15] ([fdo#111827]) +7 similar issues [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-rkl-11600/igt@kms_chamel...@hdmi-edid-read.html * igt@kms_chamelium@hdmi-hpd-fast: - fi-bsw-nick:NOTRUN -> [SKIP][16] ([fdo#109271] / [fdo#111827]) +8 similar issues [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-bsw-nick/igt@kms_chamel...@hdmi-hpd-fast.html * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy: - fi-tgl-u2: NOTRUN -> [SKIP][17] ([i915#4103]) +1 similar issue [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101530v2/fi-tgl-u2/igt@kms_cursor_leg...@basic-busy-flip-before-cursor-legacy.html - fi-rkl-11600: NOTRUN -> [SKIP][18]
Re: [Intel-gfx] [PATCH v10 1/4] gpu: drm: separate panel orientation property creating and value setting
Hi,
On 5/30/22 10:57, Hans de Goede wrote:
> Hi,
>
> On 5/30/22 10:19, Hsin-Yi Wang wrote:
>> drm_dev_register() sets connector->registration_state to
>> DRM_CONNECTOR_REGISTERED and dev->registered to true. If
>> drm_connector_set_panel_orientation() is first called after
>> drm_dev_register(), it will fail several checks and results in following
>> warning.
>>
>> Add a function to create panel orientation property and set default value
>> to UNKNOWN, so drivers can call this function to init the property earlier
>> , and let the panel set the real value later.
>>
>> [4.480976] [ cut here ]
>> [4.485603] WARNING: CPU: 5 PID: 369 at
>> drivers/gpu/drm/drm_mode_object.c:45 __drm_mode_object_add+0xb4/0xbc
>>
>> [4.609772] Call trace:
>> [4.612208] __drm_mode_object_add+0xb4/0xbc
>> [4.616466] drm_mode_object_add+0x20/0x2c
>> [4.620552] drm_property_create+0xdc/0x174
>> [4.624723] drm_property_create_enum+0x34/0x98
>> [4.629241] drm_connector_set_panel_orientation+0x64/0xa0
>> [4.634716] boe_panel_get_modes+0x88/0xd8
>> [4.638802] drm_panel_get_modes+0x2c/0x48
>> [4.642887] panel_bridge_get_modes+0x1c/0x28
>> [4.647233] drm_bridge_connector_get_modes+0xa0/0xd4
>> [4.652273] drm_helper_probe_single_connector_modes+0x218/0x700
>> [4.658266] drm_mode_getconnector+0x1b4/0x45c
>> [4.662699] drm_ioctl_kernel+0xac/0x128
>> [4.11] drm_ioctl+0x268/0x410
>> [4.670002] drm_compat_ioctl+0xdc/0xf0
>> [4.673829] __arm64_compat_sys_ioctl+0xc8/0x100
>> [4.678436] el0_svc_common+0xf4/0x1c0
>> [4.682174] do_el0_svc_compat+0x28/0x3c
>> [4.686088] el0_svc_compat+0x10/0x1c
>> [4.689738] el0_sync_compat_handler+0xa8/0xcc
>> [4.694171] el0_sync_compat+0x178/0x180
>> [4.698082] ---[ end trace b4f2db9d9c88610b ]---
>> [4.702721] [ cut here ]
>> [4.707329] WARNING: CPU: 5 PID: 369 at
>> drivers/gpu/drm/drm_mode_object.c:243 drm_object_attach_property+0x48/0xb8
>>
>> [4.833830] Call trace:
>> [4.836266] drm_object_attach_property+0x48/0xb8
>> [4.840958] drm_connector_set_panel_orientation+0x84/0xa0
>> [4.846432] boe_panel_get_modes+0x88/0xd8
>> [4.850516] drm_panel_get_modes+0x2c/0x48
>> [4.854600] panel_bridge_get_modes+0x1c/0x28
>> [4.858946] drm_bridge_connector_get_modes+0xa0/0xd4
>> [4.863984] drm_helper_probe_single_connector_modes+0x218/0x700
>> [4.869978] drm_mode_getconnector+0x1b4/0x45c
>> [4.874410] drm_ioctl_kernel+0xac/0x128
>> [4.878320] drm_ioctl+0x268/0x410
>> [4.881711] drm_compat_ioctl+0xdc/0xf0
>> [4.885536] __arm64_compat_sys_ioctl+0xc8/0x100
>> [4.890142] el0_svc_common+0xf4/0x1c0
>> [4.893879] do_el0_svc_compat+0x28/0x3c
>> [4.897791] el0_svc_compat+0x10/0x1c
>> [4.901441] el0_sync_compat_handler+0xa8/0xcc
>> [4.905873] el0_sync_compat+0x178/0x180
>> [4.909783] ---[ end trace b4f2db9d9c88610c ]---
>>
>> Signed-off-by: Hsin-Yi Wang
>> Reviewed-by: Sean Paul
>> ---
>> v9->v10: rebase to latest linux-next.
>> v9:
>> https://patchwork.kernel.org/project/linux-mediatek/patch/20220318074825.3359978-2-hsi...@chromium.org/
>> v8:
>> https://patchwork.kernel.org/project/linux-mediatek/patch/20220208084234.1684930-1-hsi...@chromium.org/
>> v7:
>> https://patchwork.kernel.org/project/linux-mediatek/patch/20220208073714.1540390-1-hsi...@chromium.org/
>> ---
>> drivers/gpu/drm/drm_connector.c | 58 +
>> include/drm/drm_connector.h | 2 ++
>> 2 files changed, 47 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c
>> index 1c48d162c77e..d68cc78f6684 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1252,7 +1252,7 @@ static const struct drm_prop_enum_list
>> dp_colorspaces[] = {
>> * INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
>> * coordinates, so if userspace rotates the picture to adjust for
>> * the orientation it must also apply the same transformation to the
>> - * touchscreen input coordinates. This property is initialized by calling
>> + * touchscreen input coordinates. This property value is set by calling
>> * drm_connector_set_panel_orientation() or
>> * drm_connector_set_panel_orientation_with_quirk()
>> *
>> @@ -2310,8 +2310,8 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
>> * @connector: connector for which to set the panel-orientation property.
>> * @panel_orientation: drm_panel_orientation value to set
>> *
>> - * This function sets the connector's panel_orientation and attaches
>> - * a "panel orientation" property to the connector.
>> + * This function sets the connector's panel_orientation value. If the
>> property
>> + * doesn't exist, it will try to create one.
>> *
>> * Calling this function on a connector where the
Re: [Intel-gfx] [PATCH v10 1/4] gpu: drm: separate panel orientation property creating and value setting
Hi,
On 5/30/22 10:19, Hsin-Yi Wang wrote:
> drm_dev_register() sets connector->registration_state to
> DRM_CONNECTOR_REGISTERED and dev->registered to true. If
> drm_connector_set_panel_orientation() is first called after
> drm_dev_register(), it will fail several checks and results in following
> warning.
>
> Add a function to create panel orientation property and set default value
> to UNKNOWN, so drivers can call this function to init the property earlier
> , and let the panel set the real value later.
>
> [4.480976] [ cut here ]
> [4.485603] WARNING: CPU: 5 PID: 369 at
> drivers/gpu/drm/drm_mode_object.c:45 __drm_mode_object_add+0xb4/0xbc
>
> [4.609772] Call trace:
> [4.612208] __drm_mode_object_add+0xb4/0xbc
> [4.616466] drm_mode_object_add+0x20/0x2c
> [4.620552] drm_property_create+0xdc/0x174
> [4.624723] drm_property_create_enum+0x34/0x98
> [4.629241] drm_connector_set_panel_orientation+0x64/0xa0
> [4.634716] boe_panel_get_modes+0x88/0xd8
> [4.638802] drm_panel_get_modes+0x2c/0x48
> [4.642887] panel_bridge_get_modes+0x1c/0x28
> [4.647233] drm_bridge_connector_get_modes+0xa0/0xd4
> [4.652273] drm_helper_probe_single_connector_modes+0x218/0x700
> [4.658266] drm_mode_getconnector+0x1b4/0x45c
> [4.662699] drm_ioctl_kernel+0xac/0x128
> [4.11] drm_ioctl+0x268/0x410
> [4.670002] drm_compat_ioctl+0xdc/0xf0
> [4.673829] __arm64_compat_sys_ioctl+0xc8/0x100
> [4.678436] el0_svc_common+0xf4/0x1c0
> [4.682174] do_el0_svc_compat+0x28/0x3c
> [4.686088] el0_svc_compat+0x10/0x1c
> [4.689738] el0_sync_compat_handler+0xa8/0xcc
> [4.694171] el0_sync_compat+0x178/0x180
> [4.698082] ---[ end trace b4f2db9d9c88610b ]---
> [4.702721] [ cut here ]
> [4.707329] WARNING: CPU: 5 PID: 369 at
> drivers/gpu/drm/drm_mode_object.c:243 drm_object_attach_property+0x48/0xb8
>
> [4.833830] Call trace:
> [4.836266] drm_object_attach_property+0x48/0xb8
> [4.840958] drm_connector_set_panel_orientation+0x84/0xa0
> [4.846432] boe_panel_get_modes+0x88/0xd8
> [4.850516] drm_panel_get_modes+0x2c/0x48
> [4.854600] panel_bridge_get_modes+0x1c/0x28
> [4.858946] drm_bridge_connector_get_modes+0xa0/0xd4
> [4.863984] drm_helper_probe_single_connector_modes+0x218/0x700
> [4.869978] drm_mode_getconnector+0x1b4/0x45c
> [4.874410] drm_ioctl_kernel+0xac/0x128
> [4.878320] drm_ioctl+0x268/0x410
> [4.881711] drm_compat_ioctl+0xdc/0xf0
> [4.885536] __arm64_compat_sys_ioctl+0xc8/0x100
> [4.890142] el0_svc_common+0xf4/0x1c0
> [4.893879] do_el0_svc_compat+0x28/0x3c
> [4.897791] el0_svc_compat+0x10/0x1c
> [4.901441] el0_sync_compat_handler+0xa8/0xcc
> [4.905873] el0_sync_compat+0x178/0x180
> [4.909783] ---[ end trace b4f2db9d9c88610c ]---
>
> Signed-off-by: Hsin-Yi Wang
> Reviewed-by: Sean Paul
> ---
> v9->v10: rebase to latest linux-next.
> v9:
> https://patchwork.kernel.org/project/linux-mediatek/patch/20220318074825.3359978-2-hsi...@chromium.org/
> v8:
> https://patchwork.kernel.org/project/linux-mediatek/patch/20220208084234.1684930-1-hsi...@chromium.org/
> v7:
> https://patchwork.kernel.org/project/linux-mediatek/patch/20220208073714.1540390-1-hsi...@chromium.org/
> ---
> drivers/gpu/drm/drm_connector.c | 58 +
> include/drm/drm_connector.h | 2 ++
> 2 files changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..d68cc78f6684 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1252,7 +1252,7 @@ static const struct drm_prop_enum_list dp_colorspaces[]
> = {
> * INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
> * coordinates, so if userspace rotates the picture to adjust for
> * the orientation it must also apply the same transformation to the
> - * touchscreen input coordinates. This property is initialized by calling
> + * touchscreen input coordinates. This property value is set by calling
> * drm_connector_set_panel_orientation() or
> * drm_connector_set_panel_orientation_with_quirk()
> *
> @@ -2310,8 +2310,8 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
> * @connector: connector for which to set the panel-orientation property.
> * @panel_orientation: drm_panel_orientation value to set
> *
> - * This function sets the connector's panel_orientation and attaches
> - * a "panel orientation" property to the connector.
> + * This function sets the connector's panel_orientation value. If the
> property
> + * doesn't exist, it will try to create one.
> *
> * Calling this function on a connector where the panel_orientation has
> * already been set is a no-op (e.g. the orientation has been overridden with
> @@ -2343,18 +2343,13 @@ int
Re: [Intel-gfx] [PATCH v10 0/4] Separate panel orientation property creating and value setting
Hi, On 5/30/22 10:19, Hsin-Yi Wang wrote: > Some drivers, eg. mtk_drm and msm_drm, rely on the panel to set the > orientation. Panel calls drm_connector_set_panel_orientation() to create > orientation property and sets the value. However, connector properties > can't be created after drm_dev_register() is called. The goal is to > separate the orientation property creation, so drm drivers can create it > earlier before drm_dev_register(). Sorry for jumping in pretty late in the discussion (based on the v10 I seem to have missed this before). This sounds to me like the real issue here is that drm_dev_register() is getting called too early? To me it seems sensible to delay calling drm_dev_register() and thus allowing userspace to start detecting available displays + features until after the panel has been probed. I see a devicetree patch in this series, so I guess that the panel is described in devicetree. Especially in the case of devicetree I would expect the kernel to have enough info to do the right thing and make sure the panel is probed before calling drm_dev_register() ? Regards, Hans > > After this series, drm_connector_set_panel_orientation() works like > before. It won't affect existing callers of > drm_connector_set_panel_orientation(). The only difference is that > some drm drivers can call drm_connector_init_panel_orientation_property() > earlier. > > Hsin-Yi Wang (4): > gpu: drm: separate panel orientation property creating and value > setting > drm/mediatek: init panel orientation property > drm/msm: init panel orientation property > arm64: dts: mt8183: Add panel rotation > > .../arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 + > drivers/gpu/drm/drm_connector.c | 58 ++- > drivers/gpu/drm/mediatek/mtk_dsi.c| 7 +++ > drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++ > include/drm/drm_connector.h | 2 + > 5 files changed, 59 insertions(+), 13 deletions(-) >
[Intel-gfx] ✗ Fi.CI.SPARSE: warning for Separate panel orientation property creating and value setting (rev2)
== Series Details == Series: Separate panel orientation property creating and value setting (rev2) URL : https://patchwork.freedesktop.org/series/101530/ State : warning == Summary == Error: dim sparse failed Sparse version: v0.6.2 Fast mode used, each commit won't be checked separately.
[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Separate panel orientation property creating and value setting (rev2)
== Series Details ==
Series: Separate panel orientation property creating and value setting (rev2)
URL : https://patchwork.freedesktop.org/series/101530/
State : warning
== Summary ==
Error: dim checkpatch failed
bf8654ee2da6 gpu: drm: separate panel orientation property creating and value
setting
-:130: WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#130: FILE: drivers/gpu/drm/drm_connector.c:2389:
+ * ^Icreate the connector's panel orientation property$
-:141: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#141: FILE: drivers/gpu/drm/drm_connector.c:2400:
+int drm_connector_init_panel_orientation_property(
-:147: ERROR:SPACING: space required before the open parenthesis '('
#147: FILE: drivers/gpu/drm/drm_connector.c:2406:
+ if(dev->mode_config.panel_orientation_property)
-:151: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#151: FILE: drivers/gpu/drm/drm_connector.c:2410:
+ prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
+ "panel orientation",
-:176: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#176: FILE: include/drm/drm_connector.h:1805:
+int drm_connector_init_panel_orientation_property(
total: 1 errors, 1 warnings, 3 checks, 99 lines checked
fb63fbb19c0d drm/mediatek: init panel orientation property
24e07972d2f3 drm/msm: init panel orientation property
205b521cb9de arm64: dts: mt8183: Add panel rotation
[Intel-gfx] [PATCH v10 4/4] arm64: dts: mt8183: Add panel rotation
krane, kakadu, and kodama boards have a default panel rotation.
Signed-off-by: Hsin-Yi Wang
Reviewed-by: Enric Balletbo i Serra
Tested-by: Enric Balletbo i Serra
---
arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
index 8d5bf73a9099..f0dd5a06629d 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
@@ -276,6 +276,7 @@ panel: panel@0 {
avee-supply = <_lcd>;
pp1800-supply = <_lcd>;
backlight = <_lcd0>;
+ rotation = <270>;
port {
panel_in: endpoint {
remote-endpoint = <_out>;
--
2.36.1.124.g0e6072fb45-goog
[Intel-gfx] [PATCH v10 3/4] drm/msm: init panel orientation property
Init panel orientation property after connector is initialized. Let the panel driver decides the orientation value later. Signed-off-by: Hsin-Yi Wang --- drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index cb84d185d73a..16ad3d8fab4d 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -669,6 +669,10 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id) connector->interlace_allowed = 0; connector->doublescan_allowed = 0; + ret = drm_connector_init_panel_orientation_property(connector); + if (ret) + goto fail; + drm_connector_attach_encoder(connector, msm_dsi->encoder); ret = msm_dsi_manager_panel_init(connector, id); -- 2.36.1.124.g0e6072fb45-goog
[Intel-gfx] [PATCH v10 2/4] drm/mediatek: init panel orientation property
Init panel orientation property after connector is initialized. Let the
panel driver decides the orientation value later.
Signed-off-by: Hsin-Yi Wang
Acked-by: Chun-Kuang Hu
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
b/drivers/gpu/drm/mediatek/mtk_dsi.c
index d9f10a33e6fa..3db44d9b161a 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -822,6 +822,13 @@ static int mtk_dsi_encoder_init(struct drm_device *drm,
struct mtk_dsi *dsi)
ret = PTR_ERR(dsi->connector);
goto err_cleanup_encoder;
}
+
+ ret = drm_connector_init_panel_orientation_property(dsi->connector);
+ if (ret) {
+ DRM_ERROR("Unable to init panel orientation\n");
+ goto err_cleanup_encoder;
+ }
+
drm_connector_attach_encoder(dsi->connector, >encoder);
return 0;
--
2.36.1.124.g0e6072fb45-goog
[Intel-gfx] [PATCH v10 1/4] gpu: drm: separate panel orientation property creating and value setting
drm_dev_register() sets connector->registration_state to
DRM_CONNECTOR_REGISTERED and dev->registered to true. If
drm_connector_set_panel_orientation() is first called after
drm_dev_register(), it will fail several checks and results in following
warning.
Add a function to create panel orientation property and set default value
to UNKNOWN, so drivers can call this function to init the property earlier
, and let the panel set the real value later.
[4.480976] [ cut here ]
[4.485603] WARNING: CPU: 5 PID: 369 at drivers/gpu/drm/drm_mode_object.c:45
__drm_mode_object_add+0xb4/0xbc
[4.609772] Call trace:
[4.612208] __drm_mode_object_add+0xb4/0xbc
[4.616466] drm_mode_object_add+0x20/0x2c
[4.620552] drm_property_create+0xdc/0x174
[4.624723] drm_property_create_enum+0x34/0x98
[4.629241] drm_connector_set_panel_orientation+0x64/0xa0
[4.634716] boe_panel_get_modes+0x88/0xd8
[4.638802] drm_panel_get_modes+0x2c/0x48
[4.642887] panel_bridge_get_modes+0x1c/0x28
[4.647233] drm_bridge_connector_get_modes+0xa0/0xd4
[4.652273] drm_helper_probe_single_connector_modes+0x218/0x700
[4.658266] drm_mode_getconnector+0x1b4/0x45c
[4.662699] drm_ioctl_kernel+0xac/0x128
[4.11] drm_ioctl+0x268/0x410
[4.670002] drm_compat_ioctl+0xdc/0xf0
[4.673829] __arm64_compat_sys_ioctl+0xc8/0x100
[4.678436] el0_svc_common+0xf4/0x1c0
[4.682174] do_el0_svc_compat+0x28/0x3c
[4.686088] el0_svc_compat+0x10/0x1c
[4.689738] el0_sync_compat_handler+0xa8/0xcc
[4.694171] el0_sync_compat+0x178/0x180
[4.698082] ---[ end trace b4f2db9d9c88610b ]---
[4.702721] [ cut here ]
[4.707329] WARNING: CPU: 5 PID: 369 at
drivers/gpu/drm/drm_mode_object.c:243 drm_object_attach_property+0x48/0xb8
[4.833830] Call trace:
[4.836266] drm_object_attach_property+0x48/0xb8
[4.840958] drm_connector_set_panel_orientation+0x84/0xa0
[4.846432] boe_panel_get_modes+0x88/0xd8
[4.850516] drm_panel_get_modes+0x2c/0x48
[4.854600] panel_bridge_get_modes+0x1c/0x28
[4.858946] drm_bridge_connector_get_modes+0xa0/0xd4
[4.863984] drm_helper_probe_single_connector_modes+0x218/0x700
[4.869978] drm_mode_getconnector+0x1b4/0x45c
[4.874410] drm_ioctl_kernel+0xac/0x128
[4.878320] drm_ioctl+0x268/0x410
[4.881711] drm_compat_ioctl+0xdc/0xf0
[4.885536] __arm64_compat_sys_ioctl+0xc8/0x100
[4.890142] el0_svc_common+0xf4/0x1c0
[4.893879] do_el0_svc_compat+0x28/0x3c
[4.897791] el0_svc_compat+0x10/0x1c
[4.901441] el0_sync_compat_handler+0xa8/0xcc
[4.905873] el0_sync_compat+0x178/0x180
[4.909783] ---[ end trace b4f2db9d9c88610c ]---
Signed-off-by: Hsin-Yi Wang
Reviewed-by: Sean Paul
---
v9->v10: rebase to latest linux-next.
v9:
https://patchwork.kernel.org/project/linux-mediatek/patch/20220318074825.3359978-2-hsi...@chromium.org/
v8:
https://patchwork.kernel.org/project/linux-mediatek/patch/20220208084234.1684930-1-hsi...@chromium.org/
v7:
https://patchwork.kernel.org/project/linux-mediatek/patch/20220208073714.1540390-1-hsi...@chromium.org/
---
drivers/gpu/drm/drm_connector.c | 58 +
include/drm/drm_connector.h | 2 ++
2 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..d68cc78f6684 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1252,7 +1252,7 @@ static const struct drm_prop_enum_list dp_colorspaces[] =
{
* INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
* coordinates, so if userspace rotates the picture to adjust for
* the orientation it must also apply the same transformation to the
- * touchscreen input coordinates. This property is initialized by calling
+ * touchscreen input coordinates. This property value is set by calling
* drm_connector_set_panel_orientation() or
* drm_connector_set_panel_orientation_with_quirk()
*
@@ -2310,8 +2310,8 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
* @connector: connector for which to set the panel-orientation property.
* @panel_orientation: drm_panel_orientation value to set
*
- * This function sets the connector's panel_orientation and attaches
- * a "panel orientation" property to the connector.
+ * This function sets the connector's panel_orientation value. If the property
+ * doesn't exist, it will try to create one.
*
* Calling this function on a connector where the panel_orientation has
* already been set is a no-op (e.g. the orientation has been overridden with
@@ -2343,18 +2343,13 @@ int drm_connector_set_panel_orientation(
prop = dev->mode_config.panel_orientation_property;
if (!prop) {
- prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
- "panel orientation",
-
[Intel-gfx] [PATCH v10 0/4] Separate panel orientation property creating and value setting
Some drivers, eg. mtk_drm and msm_drm, rely on the panel to set the orientation. Panel calls drm_connector_set_panel_orientation() to create orientation property and sets the value. However, connector properties can't be created after drm_dev_register() is called. The goal is to separate the orientation property creation, so drm drivers can create it earlier before drm_dev_register(). After this series, drm_connector_set_panel_orientation() works like before. It won't affect existing callers of drm_connector_set_panel_orientation(). The only difference is that some drm drivers can call drm_connector_init_panel_orientation_property() earlier. Hsin-Yi Wang (4): gpu: drm: separate panel orientation property creating and value setting drm/mediatek: init panel orientation property drm/msm: init panel orientation property arm64: dts: mt8183: Add panel rotation .../arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 + drivers/gpu/drm/drm_connector.c | 58 ++- drivers/gpu/drm/mediatek/mtk_dsi.c| 7 +++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++ include/drm/drm_connector.h | 2 + 5 files changed, 59 insertions(+), 13 deletions(-) -- 2.36.1.124.g0e6072fb45-goog
Re: [Intel-gfx] [PATCH RESEND v6] drm/i915/display: disable HPD workers before display driver unregister
On Fri, 2022-05-20 at 00:35 +0200, Andrzej Hajda wrote: > Handling HPD during driver removal is pointless, and can cause different > use-after-free/concurrency issues: > 1. Setup of deferred fbdev after fbdev unregistration. > 2. Access to DP-AUX after DP-AUX removal. Thanks. Reviewed-by: Vinod Govindapillai > > Below stacktraces of both cases observed on CI: > > [272.634530] general protection fault, probably for non-canonical address > 0x6b6b6b6b6b6b6b6b: > [#1] PREEMPT SMP NOPTI > [272.634536] CPU: 0 PID: 6030 Comm: i915_selftest Tainted: G U > 5.18.0-rc5- > CI_DRM_11603-g12dccf4f5eef+ #1 > [272.634541] Hardware name: Intel Corporation Raptor Lake Client > Platform/RPL-S ADP-S DDR5 UDIMM > CRB, BIOS RPLSFWI1.R00.2397.A01.2109300731 09/30/2021 > [272.634545] RIP: 0010:fb_do_apertures_overlap.part.14+0x26/0x60 > ... > [272.634582] Call Trace: > [272.634583] > [272.634585] do_remove_conflicting_framebuffers+0x59/0xa0 > [272.634589] remove_conflicting_framebuffers+0x2d/0xc0 > [272.634592] remove_conflicting_pci_framebuffers+0xc8/0x110 > [272.634595] drm_aperture_remove_conflicting_pci_framebuffers+0x52/0x70 > [272.634604] i915_driver_probe+0x63a/0xdd0 [i915] > > [283.405824] cpu_latency_qos_update_request called for unknown object > [283.405866] WARNING: CPU: 2 PID: 240 at kernel/power/qos.c:296 > cpu_latency_qos_update_request+0x2d/0x100 > [283.405912] CPU: 2 PID: 240 Comm: kworker/u64:9 Not tainted > 5.18.0-rc6-Patchwork_103738v3- > g1672d1c43e43+ #1 > [283.405915] Hardware name: Intel Corporation Raptor Lake Client > Platform/RPL-S ADP-S DDR5 UDIMM > CRB, BIOS RPLSFWI1.R00.2397.A01.2109300731 09/30/2021 > [283.405916] Workqueue: i915-dp i915_digport_work_func [i915] > [283.406020] RIP: 0010:cpu_latency_qos_update_request+0x2d/0x100 > ... > [283.406040] Call Trace: > [283.406041] > [283.406044] intel_dp_aux_xfer+0x60e/0x8e0 [i915] > [283.406131] ? finish_swait+0x80/0x80 > [283.406139] intel_dp_aux_transfer+0xc5/0x2b0 [i915] > [283.406218] drm_dp_dpcd_access+0x79/0x130 [drm_display_helper] > [283.406227] drm_dp_dpcd_read+0xe2/0xf0 [drm_display_helper] > [283.406233] intel_dp_hpd_pulse+0x134/0x570 [i915] > [283.406308] ? __down_killable+0x70/0x140 > [283.406313] i915_digport_work_func+0xba/0x150 [i915] > > Signed-off-by: Andrzej Hajda > --- > Hi all, > > This is resend of [1]. For unknown reason CC-ing ppl did not work, > so I've decided to resend. I hope this time it will work. > The patch was already succesfully tested by CI (rev6, rev7 of [1]). > > [1]: https://patchwork.freedesktop.org/series/103811/ > > Regards > Andrzej > --- > drivers/gpu/drm/i915/display/intel_display.c | 11 --- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 806d50b302ab92..007bc6daef7d31 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -10486,13 +10486,6 @@ void intel_modeset_driver_remove_noirq(struct > drm_i915_private *i915) >*/ > intel_hpd_poll_fini(i915); > > - /* > - * MST topology needs to be suspended so we don't have any calls to > - * fbdev after it's finalized. MST will be destroyed later as part of > - * drm_mode_config_cleanup() > - */ > - intel_dp_mst_suspend(i915); > - > /* poll work can call into fbdev, hence clean that up afterwards */ > intel_fbdev_fini(i915); > > @@ -10584,6 +10577,10 @@ void intel_display_driver_unregister(struct > drm_i915_private *i915) > if (!HAS_DISPLAY(i915)) > return; > > + intel_dp_mst_suspend(i915); > + intel_hpd_cancel_work(i915); > + drm_kms_helper_poll_disable(>drm); > + > intel_fbdev_unregister(i915); > intel_audio_deinit(i915); >
