Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
Hi Greg On 2/11/2022 11:04 PM, Greg KH wrote: On Fri, Feb 11, 2022 at 10:59:39AM -0800, Abhinav Kumar wrote: Hi Greg Thanks for the response. On 2/11/2022 3:09 AM, Greg KH wrote: On Tue, Feb 08, 2022 at 11:44:32AM -0800, Abhinav Kumar wrote: There are cases where depending on the size of the devcoredump and the speed at which the usermode reads the dump, it can take longer than the current 5 mins timeout. This can lead to incomplete dumps as the device is deleted once the timeout expires. One example is below where it took 6 mins for the devcoredump to be completely read. 04:22:24.668 23916 23994 I HWDeviceDRM::DumpDebugData: Opening /sys/class/devcoredump/devcd6/data 04:28:35.377 23916 23994 W HWDeviceDRM::DumpDebugData: Freeing devcoredump node What makes this so slow? Reading from the kernel shouldn't be the limit, is it where the data is being sent to? We are still checking this. We are seeing better read times when we bump up the thread priority of the thread which was reading this. Where is the thread sending the data to? The thread is writing the data to a file in local storage. From our profiling, the read is the one taking the time not the write. We are also trying to check if bumping up CPU speed is helping. But, results have not been consistently good enough. So we thought we should also increase the timeout to be safe. Why would 10 minutes be better than 30? What should the limit be? :) Again, this is from our profiling. We are seeing a worst case time of 7 mins to finish the read for our data. Thats where the 10mins came from. Just doubling what we have currently. I am not sure how the current 5 mins timeout came from. thanks, greg k-h
Re: [RFC v2 6/6] android: binder: Add a buffer flag to relinquish ownership of fds
On Fri, Feb 11, 2022 at 04:18:29PM +, T.J. Mercier wrote:
> This patch introduces a buffer flag BINDER_BUFFER_FLAG_SENDER_NO_NEED
> that a process sending an fd array to another process over binder IPC
> can set to relinquish ownership of the fds being sent for memory
> accounting purposes. If the flag is found to be set during the fd array
> translation and the fd is for a DMA-BUF, the buffer is uncharged from
> the sender's cgroup and charged to the receiving process's cgroup
> instead.
>
> It is up to the sending process to ensure that it closes the fds
> regardless of whether the transfer failed or succeeded.
>
> Most graphics shared memory allocations in Android are done by the
> graphics allocator HAL process. On requests from clients, the HAL process
> allocates memory and sends the fds to the clients over binder IPC.
> The graphics allocator HAL will not retain any references to the
> buffers. When the HAL sets the BINDER_BUFFER_FLAG_SENDER_NO_NEED for fd
> arrays holding DMA-BUF fds, the gpu cgroup controller will be able to
> correctly charge the buffers to the client processes instead of the
> graphics allocator HAL.
>
> From: Hridya Valsaraju
> Signed-off-by: Hridya Valsaraju
> Co-developed-by: T.J. Mercier
> Signed-off-by: T.J. Mercier
> ---
> changes in v2
> - Move dma-buf cgroup charge transfer from a dma_buf_op defined by every
> heap to a single dma-buf function for all heaps per Daniel Vetter and
> Christian König.
>
> drivers/android/binder.c| 26 ++
> include/uapi/linux/android/binder.h | 1 +
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 8351c5638880..f50d88ded188 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -42,6 +42,7 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> +#include
> #include
> #include
> #include
> @@ -2482,8 +2483,10 @@ static int binder_translate_fd_array(struct list_head
> *pf_head,
> {
> binder_size_t fdi, fd_buf_size;
> binder_size_t fda_offset;
> + bool transfer_gpu_charge = false;
> const void __user *sender_ufda_base;
> struct binder_proc *proc = thread->proc;
> + struct binder_proc *target_proc = t->to_proc;
> int ret;
>
> fd_buf_size = sizeof(u32) * fda->num_fds;
> @@ -2521,8 +2524,15 @@ static int binder_translate_fd_array(struct list_head
> *pf_head,
> if (ret)
> return ret;
>
> + if (IS_ENABLED(CONFIG_CGROUP_GPU) &&
> + parent->flags & BINDER_BUFFER_FLAG_SENDER_NO_NEED)
> + transfer_gpu_charge = true;
> +
> for (fdi = 0; fdi < fda->num_fds; fdi++) {
> u32 fd;
> + struct dma_buf *dmabuf;
> + struct gpucg *gpucg;
> +
> binder_size_t offset = fda_offset + fdi * sizeof(fd);
> binder_size_t sender_uoffset = fdi * sizeof(fd);
>
> @@ -2532,6 +2542,22 @@ static int binder_translate_fd_array(struct list_head
> *pf_head,
> in_reply_to);
> if (ret)
> return ret > 0 ? -EINVAL : ret;
> +
> + if (!transfer_gpu_charge)
> + continue;
> +
> + dmabuf = dma_buf_get(fd);
> + if (IS_ERR(dmabuf))
> + continue;
> +
> + gpucg = gpucg_get(target_proc->tsk);
> + ret = dma_buf_charge_transfer(dmabuf, gpucg);
> + if (ret) {
> + pr_warn("%d:%d Unable to transfer DMA-BUF fd charge to
> %d",
> + proc->pid, thread->pid, target_proc->pid);
> + gpucg_put(gpucg);
> + }
> + dma_buf_put(dmabuf);
> }
> return 0;
> }
> diff --git a/include/uapi/linux/android/binder.h
> b/include/uapi/linux/android/binder.h
> index 3246f2c74696..169fd5069a1a 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -137,6 +137,7 @@ struct binder_buffer_object {
>
> enum {
> BINDER_BUFFER_FLAG_HAS_PARENT = 0x01,
> + BINDER_BUFFER_FLAG_SENDER_NO_NEED = 0x02,
> };
>
> /* struct binder_fd_array_object - object describing an array of fds in a
> buffer
> --
> 2.35.1.265.g69c8d7142f-goog
>
How does userspace know that binder supports this new flag? And where
is the userspace test for this new feature? Isn't there a binder test
framework somewhere?
thanks,
greg k-h
Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
On Fri, Feb 11, 2022 at 10:59:39AM -0800, Abhinav Kumar wrote: > Hi Greg > > Thanks for the response. > > On 2/11/2022 3:09 AM, Greg KH wrote: > > On Tue, Feb 08, 2022 at 11:44:32AM -0800, Abhinav Kumar wrote: > > > There are cases where depending on the size of the devcoredump and the > > > speed > > > at which the usermode reads the dump, it can take longer than the current > > > 5 mins > > > timeout. > > > > > > This can lead to incomplete dumps as the device is deleted once the > > > timeout expires. > > > > > > One example is below where it took 6 mins for the devcoredump to be > > > completely read. > > > > > > 04:22:24.668 23916 23994 I HWDeviceDRM::DumpDebugData: Opening > > > /sys/class/devcoredump/devcd6/data > > > 04:28:35.377 23916 23994 W HWDeviceDRM::DumpDebugData: Freeing > > > devcoredump node > > > > What makes this so slow? Reading from the kernel shouldn't be the > > limit, is it where the data is being sent to? > > We are still checking this. We are seeing better read times when we bump up > the thread priority of the thread which was reading this. Where is the thread sending the data to? > We are also trying to check if bumping up CPU speed is helping. > But, results have not been consistently good enough. So we thought we should > also increase the timeout to be safe. Why would 10 minutes be better than 30? What should the limit be? :) thanks, greg k-h
Re: [PATCH v2] drm/lima: avoid error task dump attempt when not enabled
Applied to drm-misc-next. On Wed, Feb 9, 2022 at 5:37 PM Erico Nunes wrote: > > Currently when users try to run an application with lima and that hits > an issue such as a timeout, a message saying "fail to save task state" > and "error task list is full" is shown in dmesg. > > The error task dump is a debug feature disabled by default, so the > error task list is usually not going to be available at all. > The message can be misleading and creates confusion in bug reports. > > We can avoid that code path and that particular message when the user > has not explicitly set the max_error_tasks parameter to enable the > feature. > > Signed-off-by: Erico Nunes > Reviewed-by: Qiang Yu > Reviewed-by: Javier Martinez Canillas > --- > v2: > - collect review tags > - update summary line to "drm/lima:" > --- > drivers/gpu/drm/lima/lima_sched.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/lima/lima_sched.c > b/drivers/gpu/drm/lima/lima_sched.c > index 5612d73f238f..12437e42cc76 100644 > --- a/drivers/gpu/drm/lima/lima_sched.c > +++ b/drivers/gpu/drm/lima/lima_sched.c > @@ -409,7 +409,8 @@ static enum drm_gpu_sched_stat > lima_sched_timedout_job(struct drm_sched_job *job > > drm_sched_increase_karma(>base); > > - lima_sched_build_error_task_list(task); > + if (lima_max_error_tasks) > + lima_sched_build_error_task_list(task); > > pipe->task_error(pipe); > > -- > 2.34.1 >
Re: [PATCH v2 2/3] mm/gup.c: Migrate device coherent pages when pinning instead of failing
On 2/11/22 18:51, Alistair Popple wrote:
...
@@ -1888,15 +1942,40 @@ static long check_and_migrate_movable_pages(unsigned
long nr_pages,
continue;
prev_head = head;
/*
-* If we get a movable page, since we are going to be pinning
-* these entries, try to move them out if possible.
+* Device coherent pages are managed by a driver and should not
+* be pinned indefinitely as it prevents the driver moving the
+* page. So when trying to pin with FOLL_LONGTERM instead try
+* migrating page out of device memory.
*/
if (is_dev_private_or_coherent_page(head)) {
+ /*
+* device private pages will get faulted in during gup
+* so it shouldn't be possible to see one here.
+*/
WARN_ON_ONCE(is_device_private_page(head));
- ret = -EFAULT;
- goto unpin_pages;
+ WARN_ON_ONCE(PageCompound(head));
+
+ /*
+* migration will fail if the page is pinned, so convert
+* the pin on the source page to a normal reference.
+*/
+ if (gup_flags & FOLL_PIN) {
+ get_page(head);
+ unpin_user_page(head);
OK...but now gup_flags can no longer be used as a guide for how to
release these pages, right? In other words, up until this point,
FOLL_PIN meant "call unpin_user_page() in order to release". However,
now this page must be released via put_page().
This is the source page (head). We are unpinning it because we can't migrate a
pinned page, however we still need a reference on it for migrate_vma hence the
get_page followed by unpin. In the non-FOLL_PIN case we already have a
reference from gup.
See below...
+ }
+
+ pages[i] = migrate_device_page(head, gup_flags);
migrate_device_page() will return a new page that has been correctly pinned
with gup_flags by try_grab_page(). Therefore this page can still be released
with unpin_user_page() or put_page() as appropriate for the given gup_flags.
The reference we had on the source page (head) always gets dropped in
migrate_vma_finalize().
OK. Good.
The above would be good to have in a comment, right around here, imho.
Because we have this marvelous mix of references for migration (get_page())
and other, and it's a bit hard to see that it's all correct without a
hint or two.
...
Which unless I've missed something is still the correct thing to do.
This reminds me: out of the many things to monitor, the FOLL_PIN counts
in /proc/vmstat are especially helpful, whenever making changes to code
that deals with this:
nr_foll_pin_acquired
nr_foll_pin_released
...and those should normally be equal to each other when "at rest".
I hope this is/was run, just to be sure?
thanks,
--
John Hubbard
NVIDIA
Re: [PATCH v2 2/3] mm/gup.c: Migrate device coherent pages when pinning instead of failing
On Saturday, 12 February 2022 1:10:29 PM AEDT John Hubbard wrote:
> On 2/6/22 20:26, Alistair Popple wrote:
> > Currently any attempts to pin a device coherent page will fail. This is
> > because device coherent pages need to be managed by a device driver, and
> > pinning them would prevent a driver from migrating them off the device.
> >
> > However this is no reason to fail pinning of these pages. These are
> > coherent and accessible from the CPU so can be migrated just like
> > pinning ZONE_MOVABLE pages. So instead of failing all attempts to pin
> > them first try migrating them out of ZONE_DEVICE.
> >
>
> Hi Alistair and all,
>
> Here's a possible issue (below) that I really should have spotted the
> first time around, sorry for this late-breaking review. And maybe it's
> actually just my misunderstanding, anyway.
I think it might be a misunderstanding, see below.
> > Signed-off-by: Alistair Popple
> > Acked-by: Felix Kuehling
> > ---
> >
> > Changes for v2:
> >
> > - Added Felix's Acked-by
> > - Fixed missing check for dpage == NULL
> >
> > mm/gup.c | 105 ++--
> > 1 file changed, 95 insertions(+), 10 deletions(-)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 56d9577..5e826db 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1861,6 +1861,60 @@ struct page *get_dump_page(unsigned long addr)
> >
> > #ifdef CONFIG_MIGRATION
> > /*
> > + * Migrates a device coherent page back to normal memory. Caller should
> > have a
> > + * reference on page which will be copied to the new page if migration is
> > + * successful or dropped on failure.
> > + */
> > +static struct page *migrate_device_page(struct page *page,
> > + unsigned int gup_flags)
> > +{
> > + struct page *dpage;
> > + struct migrate_vma args;
> > + unsigned long src_pfn, dst_pfn = 0;
> > +
> > + lock_page(page);
> > + src_pfn = migrate_pfn(page_to_pfn(page)) | MIGRATE_PFN_MIGRATE;
> > + args.src = _pfn;
> > + args.dst = _pfn;
> > + args.cpages = 1;
> > + args.npages = 1;
> > + args.vma = NULL;
> > + migrate_vma_setup();
> > + if (!(src_pfn & MIGRATE_PFN_MIGRATE))
> > + return NULL;
> > +
> > + dpage = alloc_pages(GFP_USER | __GFP_NOWARN, 0);
> > +
> > + /*
> > +* get/pin the new page now so we don't have to retry gup after
> > +* migrating. We already have a reference so this should never fail.
> > +*/
> > + if (dpage && WARN_ON_ONCE(!try_grab_page(dpage, gup_flags))) {
> > + __free_pages(dpage, 0);
> > + dpage = NULL;
> > + }
> > +
> > + if (dpage) {
> > + lock_page(dpage);
> > + dst_pfn = migrate_pfn(page_to_pfn(dpage));
> > + }
> > +
> > + migrate_vma_pages();
> > + if (src_pfn & MIGRATE_PFN_MIGRATE)
> > + copy_highpage(dpage, page);
> > + migrate_vma_finalize();
> > + if (dpage && !(src_pfn & MIGRATE_PFN_MIGRATE)) {
> > + if (gup_flags & FOLL_PIN)
> > + unpin_user_page(dpage);
> > + else
> > + put_page(dpage);
> > + dpage = NULL;
> > + }
> > +
> > + return dpage;
> > +}
> > +
> > +/*
> >* Check whether all pages are pinnable, if so return number of pages.
> > If some
> >* pages are not pinnable, migrate them, and unpin all pages. Return zero
> > if
> >* pages were migrated, or if some pages were not successfully isolated.
> > @@ -1888,15 +1942,40 @@ static long
> > check_and_migrate_movable_pages(unsigned long nr_pages,
> > continue;
> > prev_head = head;
> > /*
> > -* If we get a movable page, since we are going to be pinning
> > -* these entries, try to move them out if possible.
> > +* Device coherent pages are managed by a driver and should not
> > +* be pinned indefinitely as it prevents the driver moving the
> > +* page. So when trying to pin with FOLL_LONGTERM instead try
> > +* migrating page out of device memory.
> > */
> > if (is_dev_private_or_coherent_page(head)) {
> > + /*
> > +* device private pages will get faulted in during gup
> > +* so it shouldn't be possible to see one here.
> > +*/
> > WARN_ON_ONCE(is_device_private_page(head));
> > - ret = -EFAULT;
> > - goto unpin_pages;
> > + WARN_ON_ONCE(PageCompound(head));
> > +
> > + /*
> > +* migration will fail if the page is pinned, so convert
> > +* the pin on the source page to a normal reference.
> > +*/
> > + if (gup_flags & FOLL_PIN) {
> > + get_page(head);
> > + unpin_user_page(head);
>
> OK...but now gup_flags can no
Re: [PATCH v2 2/3] mm/gup.c: Migrate device coherent pages when pinning instead of failing
On 2/6/22 20:26, Alistair Popple wrote:
Currently any attempts to pin a device coherent page will fail. This is
because device coherent pages need to be managed by a device driver, and
pinning them would prevent a driver from migrating them off the device.
However this is no reason to fail pinning of these pages. These are
coherent and accessible from the CPU so can be migrated just like
pinning ZONE_MOVABLE pages. So instead of failing all attempts to pin
them first try migrating them out of ZONE_DEVICE.
Hi Alistair and all,
Here's a possible issue (below) that I really should have spotted the
first time around, sorry for this late-breaking review. And maybe it's
actually just my misunderstanding, anyway.
Signed-off-by: Alistair Popple
Acked-by: Felix Kuehling
---
Changes for v2:
- Added Felix's Acked-by
- Fixed missing check for dpage == NULL
mm/gup.c | 105 ++--
1 file changed, 95 insertions(+), 10 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 56d9577..5e826db 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1861,6 +1861,60 @@ struct page *get_dump_page(unsigned long addr)
#ifdef CONFIG_MIGRATION
/*
+ * Migrates a device coherent page back to normal memory. Caller should have a
+ * reference on page which will be copied to the new page if migration is
+ * successful or dropped on failure.
+ */
+static struct page *migrate_device_page(struct page *page,
+ unsigned int gup_flags)
+{
+ struct page *dpage;
+ struct migrate_vma args;
+ unsigned long src_pfn, dst_pfn = 0;
+
+ lock_page(page);
+ src_pfn = migrate_pfn(page_to_pfn(page)) | MIGRATE_PFN_MIGRATE;
+ args.src = _pfn;
+ args.dst = _pfn;
+ args.cpages = 1;
+ args.npages = 1;
+ args.vma = NULL;
+ migrate_vma_setup();
+ if (!(src_pfn & MIGRATE_PFN_MIGRATE))
+ return NULL;
+
+ dpage = alloc_pages(GFP_USER | __GFP_NOWARN, 0);
+
+ /*
+* get/pin the new page now so we don't have to retry gup after
+* migrating. We already have a reference so this should never fail.
+*/
+ if (dpage && WARN_ON_ONCE(!try_grab_page(dpage, gup_flags))) {
+ __free_pages(dpage, 0);
+ dpage = NULL;
+ }
+
+ if (dpage) {
+ lock_page(dpage);
+ dst_pfn = migrate_pfn(page_to_pfn(dpage));
+ }
+
+ migrate_vma_pages();
+ if (src_pfn & MIGRATE_PFN_MIGRATE)
+ copy_highpage(dpage, page);
+ migrate_vma_finalize();
+ if (dpage && !(src_pfn & MIGRATE_PFN_MIGRATE)) {
+ if (gup_flags & FOLL_PIN)
+ unpin_user_page(dpage);
+ else
+ put_page(dpage);
+ dpage = NULL;
+ }
+
+ return dpage;
+}
+
+/*
* Check whether all pages are pinnable, if so return number of pages. If
some
* pages are not pinnable, migrate them, and unpin all pages. Return zero if
* pages were migrated, or if some pages were not successfully isolated.
@@ -1888,15 +1942,40 @@ static long check_and_migrate_movable_pages(unsigned
long nr_pages,
continue;
prev_head = head;
/*
-* If we get a movable page, since we are going to be pinning
-* these entries, try to move them out if possible.
+* Device coherent pages are managed by a driver and should not
+* be pinned indefinitely as it prevents the driver moving the
+* page. So when trying to pin with FOLL_LONGTERM instead try
+* migrating page out of device memory.
*/
if (is_dev_private_or_coherent_page(head)) {
+ /*
+* device private pages will get faulted in during gup
+* so it shouldn't be possible to see one here.
+*/
WARN_ON_ONCE(is_device_private_page(head));
- ret = -EFAULT;
- goto unpin_pages;
+ WARN_ON_ONCE(PageCompound(head));
+
+ /*
+* migration will fail if the page is pinned, so convert
+* the pin on the source page to a normal reference.
+*/
+ if (gup_flags & FOLL_PIN) {
+ get_page(head);
+ unpin_user_page(head);
OK...but now gup_flags can no longer be used as a guide for how to
release these pages, right? In other words, up until this point,
FOLL_PIN meant "call unpin_user_page() in order to release". However,
now this page must be released via put_page().
See below...
+ }
+
+ pages[i] = migrate_device_page(head, gup_flags);
+
Re: [Intel-gfx] [PATCH v5 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color
On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C wrote:
>
> From: Mika Kahola
>
> DG2 clear color render compression uses Tile4 layout. Therefore, we need
> to define a new format modifier for uAPI to support clear color rendering.
>
> v2:
> Display version is fixed. [Imre]
> KDoc is enhanced for cc modifier. [Nanley & Lionel]
>
> Signed-off-by: Mika Kahola
> cc: Anshuman Gupta
> Signed-off-by: Juha-Pekka Heikkilä
> Signed-off-by: Ramalingam C
> ---
> drivers/gpu/drm/i915/display/intel_fb.c| 8
> drivers/gpu/drm/i915/display/skl_universal_plane.c | 9 -
> include/uapi/drm/drm_fourcc.h | 10 ++
> 3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> b/drivers/gpu/drm/i915/display/intel_fb.c
> index 4d4d01963f15..3df6ef5ffec5 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -144,6 +144,12 @@ static const struct intel_modifier_desc
> intel_modifiers[] = {
> .modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS,
> .display_ver = { 13, 13 },
> .plane_caps = INTEL_PLANE_CAP_TILING_4 |
> INTEL_PLANE_CAP_CCS_MC,
> + }, {
> + .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC,
> + .display_ver = { 13, 13 },
> + .plane_caps = INTEL_PLANE_CAP_TILING_4 |
> INTEL_PLANE_CAP_CCS_RC_CC,
> +
> + .ccs.cc_planes = BIT(1),
> }, {
> .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
> .display_ver = { 13, 13 },
> @@ -559,6 +565,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb,
> int color_plane)
> else
> return 512;
> case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> case I915_FORMAT_MOD_4_TILED:
> /*
> @@ -763,6 +770,7 @@ unsigned int intel_surf_alignment(const struct
> drm_framebuffer *fb,
> case I915_FORMAT_MOD_Yf_TILED:
> return 1 * 1024 * 1024;
> case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> return 16 * 1024;
> default:
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index c38ae0876c15..b4dced1907c5 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -772,6 +772,8 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> return PLANE_CTL_TILED_4 |
> PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE |
> PLANE_CTL_CLEAR_COLOR_DISABLE;
> + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> + return PLANE_CTL_TILED_4 |
> PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
> case I915_FORMAT_MOD_Y_TILED_CCS:
> case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> return PLANE_CTL_TILED_Y |
> PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
> @@ -2358,10 +2360,15 @@ skl_get_initial_plane_config(struct intel_crtc *crtc,
> break;
> case PLANE_CTL_TILED_YF: /* aka PLANE_CTL_TILED_4 on XE_LPD+ */
> if (HAS_4TILE(dev_priv)) {
> - if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> + u32 rc_mask = PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |
> + PLANE_CTL_CLEAR_COLOR_DISABLE;
> +
> + if ((val & rc_mask) == rc_mask)
> fb->modifier =
> I915_FORMAT_MOD_4_TILED_DG2_RC_CCS;
> else if (val & PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE)
> fb->modifier =
> I915_FORMAT_MOD_4_TILED_DG2_MC_CCS;
> + else if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> + fb->modifier =
> I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC;
> else
> fb->modifier = I915_FORMAT_MOD_4_TILED;
> } else {
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index b8fb7b44c03c..697614ea4b84 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -605,6 +605,16 @@ extern "C" {
> */
> #define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS fourcc_mod_code(INTEL, 11)
>
> +/*
> + * Intel color control surfaces (CCS) for DG2 clear color render compression.
> + *
> + * DG2 uses a unified compression format for clear color render compression.
What's unified about DG2's compression format? If this doesn't affect
the layout, maybe we should drop this sentence.
> + * The general layout is a tiled layout using 4Kb tiles i.e. Tile4
Re: [Intel-gfx] [PATCH v5 15/19] drm/i915/dg2: Add DG2 unified compression
On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C wrote:
>
> From: Matt Roper
>
> DG2 unifies render compression and media compression into a single
> format for the first time. The programming and buffer layout is
> supposed to match compression on older gen12 platforms, but the actual
> compression algorithm is different from any previous platform; as such,
> we need a new framebuffer modifier to represent buffers in this format,
> but otherwise we can re-use the existing gen12 compression driver logic.
>
> v2:
> Display version fix [Imre]
>
> Signed-off-by: Matt Roper
> cc: Radhakrishna Sripada
> Signed-off-by: Mika Kahola (v2)
> cc: Anshuman Gupta
> Signed-off-by: Juha-Pekka Heikkilä
> Signed-off-by: Ramalingam C
> ---
> drivers/gpu/drm/i915/display/intel_fb.c | 13 ++
> .../drm/i915/display/skl_universal_plane.c| 26 ---
> include/uapi/drm/drm_fourcc.h | 22
> 3 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> b/drivers/gpu/drm/i915/display/intel_fb.c
> index 94c57facbb46..4d4d01963f15 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -141,6 +141,14 @@ struct intel_modifier_desc {
>
> static const struct intel_modifier_desc intel_modifiers[] = {
> {
> + .modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS,
> + .display_ver = { 13, 13 },
> + .plane_caps = INTEL_PLANE_CAP_TILING_4 |
> INTEL_PLANE_CAP_CCS_MC,
> + }, {
> + .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
> + .display_ver = { 13, 13 },
> + .plane_caps = INTEL_PLANE_CAP_TILING_4 |
> INTEL_PLANE_CAP_CCS_RC,
> + }, {
> .modifier = I915_FORMAT_MOD_4_TILED,
> .display_ver = { 13, 13 },
> .plane_caps = INTEL_PLANE_CAP_TILING_4,
> @@ -550,6 +558,8 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb,
> int color_plane)
> return 128;
> else
> return 512;
> + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> + case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> case I915_FORMAT_MOD_4_TILED:
> /*
> * Each 4K tile consists of 64B(8*8) subtiles, with
> @@ -752,6 +762,9 @@ unsigned int intel_surf_alignment(const struct
> drm_framebuffer *fb,
> case I915_FORMAT_MOD_4_TILED:
> case I915_FORMAT_MOD_Yf_TILED:
> return 1 * 1024 * 1024;
> + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> + case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> + return 16 * 1024;
> default:
> MISSING_CASE(fb->modifier);
> return 0;
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 5299dfe68802..c38ae0876c15 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -764,6 +764,14 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> return PLANE_CTL_TILED_Y;
> case I915_FORMAT_MOD_4_TILED:
> return PLANE_CTL_TILED_4;
> + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> + return PLANE_CTL_TILED_4 |
> + PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |
> + PLANE_CTL_CLEAR_COLOR_DISABLE;
> + case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> + return PLANE_CTL_TILED_4 |
> + PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE |
> + PLANE_CTL_CLEAR_COLOR_DISABLE;
> case I915_FORMAT_MOD_Y_TILED_CCS:
> case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> return PLANE_CTL_TILED_Y |
> PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
> @@ -2094,6 +2102,10 @@ static bool gen12_plane_has_mc_ccs(struct
> drm_i915_private *i915,
> if (IS_ADLP_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> return false;
>
> + /* Wa_14013215631 */
> + if (IS_DG2_DISPLAY_STEP(i915, STEP_A0, STEP_C0))
> + return false;
> +
> return plane_id < PLANE_SPRITE4;
> }
>
> @@ -2335,9 +2347,10 @@ skl_get_initial_plane_config(struct intel_crtc *crtc,
> case PLANE_CTL_TILED_Y:
> plane_config->tiling = I915_TILING_Y;
> if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> - fb->modifier = DISPLAY_VER(dev_priv) >= 12 ?
> - I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS :
> - I915_FORMAT_MOD_Y_TILED_CCS;
> + if (DISPLAY_VER(dev_priv) >= 12)
> + fb->modifier =
> I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS;
> + else
> + fb->modifier =
Re: [Freedreno] [PATCH v5 3/6] drm/msm/dpu: get INTF blocks directly rather than through RM
On 2/11/2022 5:47 AM, Dmitry Baryshkov wrote: On 11/02/2022 02:31, Abhinav Kumar wrote: On 2/10/2022 1:32 AM, Dmitry Baryshkov wrote: On 10/02/2022 03:25, Abhinav Kumar wrote: On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote: INTF blocks are not really handled by resource manager, they are assigned at dpu_encoder_setup_display using dpu_encoder_get_intf(). Then this allocation is passed to RM and then returned to then dpu_encoder. So allocate them outside of RM and use them directly. Signed-off-by: Dmitry Baryshkov Reviewed-by: Stephen Boyd I have some questions about this approach. Agreed, that when there is one encoder tied to one interface, we dont need to go through RM because RM just gives what the encoder asks. Instead we can directly get the interface from encoder. But what happens when multiple displays are requesting the same interface? There are use-cases which we have handled especially in mid-tier chips which do not have external display where between mode-switches OR between some user triggered events DSI0 was shared between two displays. So lets say display 1 (with encoder X) requests DSI0 and display 2 (with encoder Y) also requests DSI0, All the encoders are allocated in the loop over possible DSI/DP interfaces. Thus it's not possible to have two encoders being driven by the same DSI entity. Moreover, the MSM DSI manager would not be able to cope with such cases. In my opinion, the proper way to handle such cases would be a bridge which would generate hotplug events and toggle display pipeline inbetween. Like I wrote in the previous comment, today there is always one encoder requesting for one interface (or two for split DSI ) and there is no chance of a conflict ( the loop that you are referring to ). And yes I am aware that DSI does not support this today. Myself and a few others internally are looking ahead and thinking of what can come in the future and something which we already support downstream, which is to support sharing the interface across encoders. So what happens downstream is the controller ID comes from the device tree: 6266 info->num_of_h_tiles = display->ctrl_count; 6267 for (i = 0; i < info->num_of_h_tiles; i++) 6268 info->h_tile_instance[i] = display->ctrl[i].ctrl->cell_index; 6269 There is a concept of a dsi_display quite similar to the dsi_manager we have upstream. There are also implementations already in place of a shared display, where like I was describing in IRC, the DSI0 can be shared across two encoders between a mode_set(). Hotplug is not necessarily the only trigger which happens, it can just be a simple mode_set() in between. Even having a modeset, you will have to trigger the electrical switch between the DSI sinks. It should be a separate device sitting in both DSI pipelines, so you'll have a question of having it in sync. In that case, this encoder to intf mapping in the RM will protect against concurrent hardware use. Ofcourse, all this is not present today but will someday :) That time this will have to be brought back. So I thought I must mention the use-cases which will get potentially affected with this change of removing INTF from RM. If for the sake of code simplicity, if we want to ignore the possibility of this coming back later, please let me know what you think and we can take this further by acking it. As I wrote on the IRC, I suppose that neither you nor me can imagine the way such usecases might be actually implemented when the need arises. You have an idea of using multiple encoders. Fine idea, nothing particularly wrong with it. I'd use single pipeline and multiple panels being switched. Or the panel driver providing modes for both small and larger panels (or internal and external) at the same time and guarding the switch on its own. The engineer that would implement this feature might come with another approach (and might e.g. implement dynamic drm_bridge pipelines inside the core). And if we bring in DSI split link into the equation, the situation might become even more complex (with one of encoders owning a half of the DSI and and enother encoder owning another half). We should always think about future use cases, but we should not over-predict the future. Unnecessary abstractions complicate the code, making the driver harder to comprehend, harder to modify and more error-prone. That said I'd suggest/ask to ack and accept this patch. Alright, since we have discussed pros and cons of this approach, in the absence of any current code/features which will get impacted due to this, Reviewed-by: Abhinav Kumar I will make an equivalent change in the WB series. with this change, the code will allow that as there is no interface to encoder mapping. --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 36 +- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 16 -
[PATCH v2] drm/i915/guc: Do not complain about stale reset notifications
From: John Harrison
It is possible for reset notifications to arrive for a context that is
in the process of being banned. So don't flag these as an error, just
report it as informational (because it is still useful to know that
resets are happening even if they are being ignored).
v2: Better wording for the message (review feedback from Tvrtko).
Signed-off-by: John Harrison
---
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8
1 file changed, 4 insertions(+), 4 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 b3a429a92c0d..3afff24b8f24 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -4022,10 +4022,10 @@ static void guc_handle_context_reset(struct intel_guc
*guc,
capture_error_state(guc, ce);
guc_context_replay(ce);
} else {
- drm_err(_to_gt(guc)->i915->drm,
- "Invalid GuC engine reset notificaion for 0x%04X on %s:
banned = %d, blocked = %d",
- ce->guc_id.id, ce->engine->name,
intel_context_is_banned(ce),
- context_blocked(ce));
+ drm_info(_to_gt(guc)->i915->drm,
+"Ignoring context reset notification for 0x%04X on %s:
banned = %d, blocked = %d",
+ce->guc_id.id, ce->engine->name,
intel_context_is_banned(ce),
+context_blocked(ce));
}
}
--
2.25.1
Re: [PATCH 7/7] drm/msm/dpu: pull connector from dpu_encoder_phys to dpu_encoder_virt
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
All physical encoders used by virtual encoder share the same connector,
so pull the connector field from dpu_encoder_phys into dpu_encoder_virt
structure.
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 2 --
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1462c426c14c..afafdaf48aea 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -143,6 +143,7 @@ enum dpu_enc_rc_states {
*link between encoder/crtc. However in this case we need
*to track crtc in the disable() hook which is called
*_after_ encoder_mask is cleared.
+ * @connector: If a mode is set, cached pointer to the active connector
* @crtc_kickoff_cb: Callback into CRTC that will flush & start
*all CTL paths
* @crtc_kickoff_cb_data: Opaque user data given to crtc_kickoff_cb
@@ -183,6 +184,7 @@ struct dpu_encoder_virt {
bool intfs_swapped;
struct drm_crtc *crtc;
+ struct drm_connector *connector;
struct dentry *debugfs_root;
struct mutex enc_lock;
@@ -993,6 +995,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct
drm_encoder *drm_enc,
cstate->num_mixers = num_lm;
+ dpu_enc->connector = conn_state->connector;
+
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
int num_blk;
struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
@@ -1030,7 +1034,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct
drm_encoder *drm_enc,
return;
}
- phys->connector = conn_state->connector;
phys->cached_mode = crtc_state->adjusted_mode;
if (phys->ops.atomic_mode_set)
phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
@@ -1064,7 +1067,7 @@ static void _dpu_encoder_virt_enable_helper(struct
drm_encoder *drm_enc)
if (dpu_enc->disp_info.intf_type == INTF_DSI &&
!WARN_ON(dpu_enc->num_phys_encs == 0)) {
- unsigned bpc =
dpu_enc->phys_encs[0]->connector->display_info.bpc;
+ unsigned bpc = dpu_enc->connector->display_info.bpc;
for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
if (!dpu_enc->hw_pp[i])
continue;
@@ -1168,9 +1171,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_STOP);
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- dpu_enc->phys_encs[i]->connector = NULL;
- }
+ dpu_enc->connector = NULL;
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 6e80321b13c5..5093810f6663 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -174,7 +174,6 @@ struct dpu_encoder_irq {
*tied to a specific panel / sub-panel. Abstract type, sub-classed by
*phys_vid or phys_cmd for video mode or command mode encs respectively.
* @parent: Pointer to the containing virtual encoder
- * @connector: If a mode is set, cached pointer to the active connector
* @ops: Operations exposed to the virtual encoder
* @parent_ops: Callbacks exposed by the parent to the phys_enc
* @hw_mdptop:Hardware interface to the top registers
@@ -203,7 +202,6 @@ struct dpu_encoder_irq {
*/
struct dpu_encoder_phys {
struct drm_encoder *parent;
- struct drm_connector *connector;
struct dpu_encoder_phys_ops ops;
const struct dpu_encoder_virt_ops *parent_ops;
struct dpu_hw_mdp *hw_mdptop;
Re: [PATCH] drm/msm: populate intf_audio_select() base on hardware capability
On 2/11/2022 4:08 PM, Dmitry Baryshkov wrote:
On 12/02/2022 02:23, Kuogee Hsieh wrote:
intf_audio_select() callback function use to configure
HDMI_DP_CORE_SELECT to decide audio output routes to HDMI or DP
interface. HDMI is obsoleted at newer chipset. To keep supporting
legacy hdmi application, intf_audio_select call back function have
to be populated base on hardware chip capability where legacy
chipsets have has_audio_select flag set to true.
So, after thinking more about the patch, I have a bunch of questions:
You are enabling this callback only for sdm845 and sm8150.
Does this register exist on other (newer) platforms (but just defaults
to DP)?
The register itself exists but there is no logic associated with it. Its
a no-op.
Neither sdm845 nor sm8150 support INTF_HDMI. What's the purpose of the
register on these platforms?
Yes we also had a similar thought earlier that this register has meaning
only on chipsets which have HDMI and DP but our hardware team suggested
sm8250 and its derivatives should be the cut-off point to stop using
this register. So we are just following that.
Does that mean that we should program the register for HDMI (e.g. on 8998)?
Yes, we should program this for HDMI 8998 ( although the default value
of the register is 0 for HDMI ).
And, as you are touching this piece of code, how do we control audio
routing on newer platforms which have several hardware DP interfaces?
Thats unrelated to this register because on newer chipsets which have
two DPs there is no HDMI and hence this register remains a no-op.
But coming to the overall question on multi-DP audio.
This is not a new question. I had first asked about this to Bjorn for
sc8180x. The current hdmi-codec interface which is used for single DP
audio will have to be extended to support this to support which stream
to pass the audio on. This is an open item which was left to be done
later on because the only chipset which has multi-DP in upstream is
sc8180x. We dont have that hardware with us for development. When we
start working on that, we will have to implement what I just mentioned.
Thanks
Abhinav
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 9 ++---
3 files changed, 9 insertions(+), 3 deletions(-)
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 272b14b..23680e7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -201,6 +201,7 @@ static const struct dpu_caps sdm845_dpu_caps = {
.has_dim_layer = true,
.has_idle_pc = true,
.has_3d_merge = true,
+ .has_audio_select = true,
.max_linewidth = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
.max_hdeci_exp = MAX_HORZ_DECIMATION,
@@ -229,6 +230,7 @@ static const struct dpu_caps sm8150_dpu_caps = {
.has_dim_layer = true,
.has_idle_pc = true,
.has_3d_merge = true,
+ .has_audio_select = true,
.max_linewidth = 4096,
.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
.max_hdeci_exp = MAX_HORZ_DECIMATION,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index e5a96d6..b33f91b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -357,6 +357,7 @@ struct dpu_caps {
bool has_dim_layer;
bool has_idle_pc;
bool has_3d_merge;
+ bool has_audio_select;
/* SSPP limits */
u32 max_linewidth;
u32 pixel_ram_size;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
index 282e3c6..e608f4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
@@ -261,14 +261,17 @@ static void dpu_hw_intf_audio_select(struct
dpu_hw_mdp *mdp)
}
static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
- unsigned long cap)
+ unsigned long cap,
+ const struct dpu_mdss_cfg *m)
{
ops->setup_split_pipe = dpu_hw_setup_split_pipe;
ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl;
ops->get_danger_status = dpu_hw_get_danger_status;
ops->setup_vsync_source = dpu_hw_setup_vsync_source;
ops->get_safe_status = dpu_hw_get_safe_status;
- ops->intf_audio_select = dpu_hw_intf_audio_select;
+
+ if (m->caps->has_audio_select)
+ ops->intf_audio_select = dpu_hw_intf_audio_select;
}
static const struct dpu_mdp_cfg *_top_offset(enum dpu_mdp mdp,
@@ -320,7 +323,7 @@ struct dpu_hw_mdp *dpu_hw_mdptop_init(enum dpu_mdp
idx,
*/
mdp->idx = idx;
mdp->caps = cfg;
- _setup_mdp_ops(>ops, mdp->caps->features);
+ _setup_mdp_ops(>ops, mdp->caps->features, m);
return
[PATCH v3] drm/msm/dpu: Only create debugfs for PRIMARY minor
From: Bjorn Andersson dpu_kms_debugfs_init() is invoked for each minor being registered. Most of the files created are unrelated to the minor, so there's no reason to present them per minor. The exception to this is the DisplayPort code, which ends up invoking dp_debug_get() for each minor, each time associate the allocated object with dp->debug. As such dp_debug will create debugfs files in both the PRIMARY and the RENDER minor's debugfs directory, but only the last reference will be remembered. The only use of this reference today is in the cleanup path in dp_display_deinit_sub_modules() and the dp_debug_private object does outlive the debugfs entries in either case, so there doesn't seem to be any adverse effects of this, but per the code the current behavior is unexpected, so change it to only create debugfs files for the PRIMARY minor. Signed-off-by: Bjorn Andersson [DB: slightly change description and in-patch comment] Signed-off-by: Dmitry Baryshkov --- This is a replacement for https://patchwork.freedesktop.org/patch/467273/ with the patch subject and comment being fixed. --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 5f0dc44119c9..c394bd6b2e5d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -271,6 +271,10 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor) if (!p) return -EINVAL; + /* Only create a set of debugfs for the primary node, ignore render nodes */ + if (minor->type != DRM_MINOR_PRIMARY) + return 0; + dev = dpu_kms->dev; priv = dev->dev_private; -- 2.34.1
Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code
On 12/02/2022 03:17, Abhinav Kumar wrote:
On 2/11/2022 4:05 PM, Dmitry Baryshkov wrote:
On 12/02/2022 02:38, Abhinav Kumar wrote:
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to
guess,
which intf type we mean, pass INTF_DSI/INTF_DP directly.
Typically, I am only seeing a 1:1 mapping like
DRM_MODE_ENCODER_DSI means DSI
DRM_MODE_ENCODER_VIRTUAL means WB
So I am not seeing any guessing for the encoder.
s/guessing/deriving/
Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison,
then I noticed that there is a misnaming, we were talking about the
intf_type (which clearly belongs to INTF_foo namespace), while passing
DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type
actually contain INTF_type and let DRM_ENCODER live in encoder's code.
The only conflict I am seeing is between DP and EDP as both use
DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
But that has been marked as a "FIXME" below.
I am suggesting an approach to handle that as well below.
Let me know if you agree with that.
Actually this brings a question to me. Do we need to distinguish
between INTF_DP and INTF_EDP from the DPU driver point of view? Are
there any differences? Or we'd better always use INTF_DP here and make
INTF_EDP a memorial of old eDP IP found in 8x74/8x84?
So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but
declares it as INTF_DP. Most probably we should stick to this idea and
drop INTF_EDP from dpu1?
I believe you are referring to this part:
static const struct dpu_intf_cfg sc7280_intf[] = {
INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, MSM_DP_CONTROLLER_0,
24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, MSM_DP_CONTROLLER_1,
24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
};
Yep, this one.
This is interesting ... this should have been marked as INTF_eDP unless
I am missing some reason why.
My take on this is that, since are re-using DP driver now for DP and
eDP, less confusion the better in terms of naming of DP/eDP.
I think we should fix that in the catalog to INTF_eDP. And in case some
place is missed out due to this change fix that too.
Well, we need to fix that if there is a difference from the DPU point of
view (not from the DP IP block). If there is no difference between
INTF_0 and INTF_5, it might be easier to just use INTF_DP for both of them.
In case we change it, your suggestion seems fine to me, I'll include it
in v2.
Meanwhile I can check with sankeerth if there was any specific reason
for not using INTF_eDP in the original commit:
commit ef7837ff091c8805cfa18d2ad06c2e5f4d820a7e
Author: Sankeerth Billakanti
Date: Tue Nov 2 13:18:42 2021 +0530
drm/msm/dp: Add DP controllers for sc7280
The eDP controller on SC7280 is similar to the eDP/DP controllers
supported by the current driver implementation.
SC7280 supports one EDP and one DP controller which can operate
concurrently.
This change adds the support for eDP and DP controller on sc7280.
Signed-off-by: Sankeerth Billakanti
While we are at it, fix the DP audio enablement code which was
comparing
intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
DRM_MODE_CONNECTOR_DisplayPort (= 10).
Which would never succeed.
This is a surprising catch for me and left me thinking for a while
about how DP audio is working with this bug because that piece of
code was done to program a register which is needed for DP audio.
So did I!
This bug happened due to difference in the meaning of intf_type between
upstream and downstream.
After checking more, we found that the register in question has been
deprecated on newer chipsets so I have asked Kuogee to selectively
program it. Here is the change for that:
https://patchwork.freedesktop.org/patch/473869/
I'll further comment on it in the respective thread.
Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port
on MSM")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28
+++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +--
3 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1e648db439f9..e8fc029ad607 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
hw_mdptop = phys_enc->hw_mdptop;
disp_info = _enc->disp_info;
- if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
+ if (disp_info->intf_type != INTF_DSI)
return;
/**
@@ -555,7 +555,7 @@
Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code
On 2/11/2022 4:05 PM, Dmitry Baryshkov wrote:
On 12/02/2022 02:38, Abhinav Kumar wrote:
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
which intf type we mean, pass INTF_DSI/INTF_DP directly.
Typically, I am only seeing a 1:1 mapping like
DRM_MODE_ENCODER_DSI means DSI
DRM_MODE_ENCODER_VIRTUAL means WB
So I am not seeing any guessing for the encoder.
s/guessing/deriving/
Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison, then
I noticed that there is a misnaming, we were talking about the intf_type
(which clearly belongs to INTF_foo namespace), while passing
DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type
actually contain INTF_type and let DRM_ENCODER live in encoder's code.
The only conflict I am seeing is between DP and EDP as both use
DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
But that has been marked as a "FIXME" below.
I am suggesting an approach to handle that as well below.
Let me know if you agree with that.
Actually this brings a question to me. Do we need to distinguish between
INTF_DP and INTF_EDP from the DPU driver point of view? Are there any
differences? Or we'd better always use INTF_DP here and make INTF_EDP a
memorial of old eDP IP found in 8x74/8x84?
So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but
declares it as INTF_DP. Most probably we should stick to this idea and
drop INTF_EDP from dpu1?
I believe you are referring to this part:
static const struct dpu_intf_cfg sc7280_intf[] = {
INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, MSM_DP_CONTROLLER_0,
24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, MSM_DP_CONTROLLER_1,
24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
};
This is interesting ... this should have been marked as INTF_eDP unless
I am missing some reason why.
My take on this is that, since are re-using DP driver now for DP and
eDP, less confusion the better in terms of naming of DP/eDP.
I think we should fix that in the catalog to INTF_eDP. And in case some
place is missed out due to this change fix that too.
Meanwhile I can check with sankeerth if there was any specific reason
for not using INTF_eDP in the original commit:
commit ef7837ff091c8805cfa18d2ad06c2e5f4d820a7e
Author: Sankeerth Billakanti
Date: Tue Nov 2 13:18:42 2021 +0530
drm/msm/dp: Add DP controllers for sc7280
The eDP controller on SC7280 is similar to the eDP/DP controllers
supported by the current driver implementation.
SC7280 supports one EDP and one DP controller which can operate
concurrently.
This change adds the support for eDP and DP controller on sc7280.
Signed-off-by: Sankeerth Billakanti
While we are at it, fix the DP audio enablement code which was comparing
intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
DRM_MODE_CONNECTOR_DisplayPort (= 10).
Which would never succeed.
This is a surprising catch for me and left me thinking for a while
about how DP audio is working with this bug because that piece of code
was done to program a register which is needed for DP audio.
So did I!
This bug happened due to difference in the meaning of intf_type between
upstream and downstream.
After checking more, we found that the register in question has been
deprecated on newer chipsets so I have asked Kuogee to selectively
program it. Here is the change for that:
https://patchwork.freedesktop.org/patch/473869/
I'll further comment on it in the respective thread.
Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port
on MSM")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +--
3 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1e648db439f9..e8fc029ad607 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
hw_mdptop = phys_enc->hw_mdptop;
disp_info = _enc->disp_info;
- if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
+ if (disp_info->intf_type != INTF_DSI)
return;
/**
@@ -555,7 +555,7 @@ static struct msm_display_topology
dpu_encoder_get_topology(
else
topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2
: 1;
- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
+ if (dpu_enc->disp_info.intf_type == INTF_DSI) {
if (dpu_kms->catalog->dspp &&
(dpu_kms->catalog->dspp_count >= topology.num_lm))
Re: [PATCH] drm/msm: populate intf_audio_select() base on hardware capability
On 12/02/2022 02:23, Kuogee Hsieh wrote:
intf_audio_select() callback function use to configure
HDMI_DP_CORE_SELECT to decide audio output routes to HDMI or DP
interface. HDMI is obsoleted at newer chipset. To keep supporting
legacy hdmi application, intf_audio_select call back function have
to be populated base on hardware chip capability where legacy
chipsets have has_audio_select flag set to true.
So, after thinking more about the patch, I have a bunch of questions:
You are enabling this callback only for sdm845 and sm8150.
Does this register exist on other (newer) platforms (but just defaults
to DP)?
Neither sdm845 nor sm8150 support INTF_HDMI. What's the purpose of the
register on these platforms?
Does that mean that we should program the register for HDMI (e.g. on 8998)?
And, as you are touching this piece of code, how do we control audio
routing on newer platforms which have several hardware DP interfaces?
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 9 ++---
3 files changed, 9 insertions(+), 3 deletions(-)
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 272b14b..23680e7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -201,6 +201,7 @@ static const struct dpu_caps sdm845_dpu_caps = {
.has_dim_layer = true,
.has_idle_pc = true,
.has_3d_merge = true,
+ .has_audio_select = true,
.max_linewidth = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
.max_hdeci_exp = MAX_HORZ_DECIMATION,
@@ -229,6 +230,7 @@ static const struct dpu_caps sm8150_dpu_caps = {
.has_dim_layer = true,
.has_idle_pc = true,
.has_3d_merge = true,
+ .has_audio_select = true,
.max_linewidth = 4096,
.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
.max_hdeci_exp = MAX_HORZ_DECIMATION,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index e5a96d6..b33f91b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -357,6 +357,7 @@ struct dpu_caps {
bool has_dim_layer;
bool has_idle_pc;
bool has_3d_merge;
+ bool has_audio_select;
/* SSPP limits */
u32 max_linewidth;
u32 pixel_ram_size;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
index 282e3c6..e608f4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
@@ -261,14 +261,17 @@ static void dpu_hw_intf_audio_select(struct dpu_hw_mdp
*mdp)
}
static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
- unsigned long cap)
+ unsigned long cap,
+ const struct dpu_mdss_cfg *m)
{
ops->setup_split_pipe = dpu_hw_setup_split_pipe;
ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl;
ops->get_danger_status = dpu_hw_get_danger_status;
ops->setup_vsync_source = dpu_hw_setup_vsync_source;
ops->get_safe_status = dpu_hw_get_safe_status;
- ops->intf_audio_select = dpu_hw_intf_audio_select;
+
+ if (m->caps->has_audio_select)
+ ops->intf_audio_select = dpu_hw_intf_audio_select;
}
static const struct dpu_mdp_cfg *_top_offset(enum dpu_mdp mdp,
@@ -320,7 +323,7 @@ struct dpu_hw_mdp *dpu_hw_mdptop_init(enum dpu_mdp idx,
*/
mdp->idx = idx;
mdp->caps = cfg;
- _setup_mdp_ops(>ops, mdp->caps->features);
+ _setup_mdp_ops(>ops, mdp->caps->features, m);
return mdp;
}
--
With best wishes
Dmitry
Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code
On 12/02/2022 02:38, Abhinav Kumar wrote:
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
which intf type we mean, pass INTF_DSI/INTF_DP directly.
Typically, I am only seeing a 1:1 mapping like
DRM_MODE_ENCODER_DSI means DSI
DRM_MODE_ENCODER_VIRTUAL means WB
So I am not seeing any guessing for the encoder.
s/guessing/deriving/
Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison, then
I noticed that there is a misnaming, we were talking about the intf_type
(which clearly belongs to INTF_foo namespace), while passing
DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type
actually contain INTF_type and let DRM_ENCODER live in encoder's code.
The only conflict I am seeing is between DP and EDP as both use
DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
But that has been marked as a "FIXME" below.
I am suggesting an approach to handle that as well below.
Let me know if you agree with that.
Actually this brings a question to me. Do we need to distinguish between
INTF_DP and INTF_EDP from the DPU driver point of view? Are there any
differences? Or we'd better always use INTF_DP here and make INTF_EDP a
memorial of old eDP IP found in 8x74/8x84?
So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but
declares it as INTF_DP. Most probably we should stick to this idea and
drop INTF_EDP from dpu1?
While we are at it, fix the DP audio enablement code which was comparing
intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
DRM_MODE_CONNECTOR_DisplayPort (= 10).
Which would never succeed.
This is a surprising catch for me and left me thinking for a while about
how DP audio is working with this bug because that piece of code was
done to program a register which is needed for DP audio.
So did I!
This bug happened due to difference in the meaning of intf_type between
upstream and downstream.
After checking more, we found that the register in question has been
deprecated on newer chipsets so I have asked Kuogee to selectively
program it. Here is the change for that:
https://patchwork.freedesktop.org/patch/473869/
I'll further comment on it in the respective thread.
Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port
on MSM")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +--
3 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1e648db439f9..e8fc029ad607 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
hw_mdptop = phys_enc->hw_mdptop;
disp_info = _enc->disp_info;
- if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
+ if (disp_info->intf_type != INTF_DSI)
return;
/**
@@ -555,7 +555,7 @@ static struct msm_display_topology
dpu_encoder_get_topology(
else
topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2
: 1;
- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
+ if (dpu_enc->disp_info.intf_type == INTF_DSI) {
if (dpu_kms->catalog->dspp &&
(dpu_kms->catalog->dspp_count >= topology.num_lm))
topology.num_dspp = topology.num_lm;
@@ -1099,7 +1099,7 @@ static void
_dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
}
- if (dpu_enc->disp_info.intf_type ==
DRM_MODE_CONNECTOR_DisplayPort &&
+ if (dpu_enc->disp_info.intf_type == INTF_DP &&
dpu_enc->cur_master->hw_mdptop &&
dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
@@ -1107,7 +1107,7 @@ static void
_dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
_dpu_encoder_update_vsync_source(dpu_enc, _enc->disp_info);
- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
+ if (dpu_enc->disp_info.intf_type == INTF_DSI &&
!WARN_ON(dpu_enc->num_phys_encs == 0)) {
unsigned bpc =
dpu_enc->phys_encs[0]->connector->display_info.bpc;
for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
@@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
{
int ret = 0;
int i = 0;
- enum dpu_intf_type intf_type = INTF_NONE;
struct dpu_enc_phys_init_params phys_params;
if (!dpu_enc) {
@@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
phys_params.parent_ops = _encoder_parent_ops;
phys_params.enc_spinlock = _enc->enc_spinlock;
- switch (disp_info->intf_type) {
- case DRM_MODE_ENCODER_DSI:
- intf_type =
Re: [PATCH 6/7] drm/msm/dpu: switch dpu_encoder to use atomic_mode_set
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
Make dpu_encoder use atomic_mode_set to receive connector and CRTC
states as arguments rather than finding connector and CRTC by manually
looping through the respective lists.
Signed-off-by: Dmitry Baryshkov
Nice cleanup !!
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 37 +--
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 8 ++--
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 18 ++---
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 ++-
4 files changed, 21 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e1c43a0c0643..1462c426c14c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -930,16 +930,13 @@ static int dpu_encoder_resource_control(struct
drm_encoder *drm_enc,
return 0;
}
-static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
- struct drm_display_mode *mode,
- struct drm_display_mode *adj_mode)
+static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
+struct drm_crtc_state *crtc_state,
+struct drm_connector_state
*conn_state)
{
struct dpu_encoder_virt *dpu_enc;
struct msm_drm_private *priv;
struct dpu_kms *dpu_kms;
- struct list_head *connector_list;
- struct drm_connector *conn = NULL, *conn_iter;
- struct drm_crtc *drm_crtc;
struct dpu_crtc_state *cstate;
struct dpu_global_state *global_state;
struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
@@ -959,7 +956,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
*drm_enc,
priv = drm_enc->dev->dev_private;
dpu_kms = to_dpu_kms(priv->kms);
- connector_list = _kms->dev->mode_config.connector_list;
global_state = dpu_kms_get_existing_global_state(dpu_kms);
if (IS_ERR_OR_NULL(global_state)) {
@@ -969,22 +965,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
*drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
- list_for_each_entry(conn_iter, connector_list, head)
- if (conn_iter->encoder == drm_enc)
- conn = conn_iter;
-
- if (!conn) {
- DPU_ERROR_ENC(dpu_enc, "failed to find attached connector\n");
- return;
- } else if (!conn->state) {
- DPU_ERROR_ENC(dpu_enc, "invalid connector state\n");
- return;
- }
-
- drm_for_each_crtc(drm_crtc, drm_enc->dev)
- if (drm_crtc->state->encoder_mask & drm_encoder_mask(drm_enc))
- break;
-
/* Query resource that have been reserved in atomic check step. */
num_pp = dpu_rm_get_assigned_resources(_kms->rm, global_state,
drm_enc->base.id, DPU_HW_BLK_PINGPONG, hw_pp,
@@ -1001,7 +981,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
*drm_enc,
dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
: NULL;
- cstate = to_dpu_crtc_state(drm_crtc->state);
+ cstate = to_dpu_crtc_state(crtc_state);
for (i = 0; i < num_lm; i++) {
int ctl_idx = (i < num_ctl) ? i : (num_ctl-1);
@@ -1050,9 +1030,10 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
*drm_enc,
return;
}
- phys->connector = conn->state->connector;
- if (phys->ops.mode_set)
- phys->ops.mode_set(phys, mode, adj_mode);
+ phys->connector = conn_state->connector;
+ phys->cached_mode = crtc_state->adjusted_mode;
+ if (phys->ops.atomic_mode_set)
+ phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
}
}
@@ -2057,7 +2038,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
}
static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
- .mode_set = dpu_encoder_virt_mode_set,
+ .atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
.disable = dpu_encoder_virt_disable,
.enable = dpu_encoder_virt_enable,
.atomic_check = dpu_encoder_virt_atomic_check,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 159deb8ed7fb..6e80321b13c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -84,7 +84,7 @@ struct dpu_encoder_virt_ops {
* @is_master:Whether this phys_enc is the current
master
*encoder. Can be switched at enable time. Based
*
Re: [PATCH 5/7] drm/msm/dpu: encoder: drop unused callbacks
On 12/02/2022 02:53, Abhinav Kumar wrote:
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
Both cmd and vid backends provide no atomic_check() callbacks and
useless mode_fixup() callbacks. Drop support for both of them.
Signed-off-by: Dmitry Baryshkov
We can drop mode_fixup() but I am using atomic_check() for WB to
validate whether the programmed FB exceeds WB limits OR
the programmed mode. Hence without an alternative for that we cannot
drop that
Ack, will change in v2, thanks for catching this!
Just as a remark: as I think more and more about the WB/CMD/VID, I have
an impression that at some point we should turn drm_encoder inside out.
Let drm_encoder_phys/vid/wb be a first class citizens and call helpers
from dpu_encoder.c rather than making dpu_encoder.c calling out
functions from respective backend. For now it's quick rough idea, but
your thoughts are welcomed.
Please refer to dpu_encoder_phys_wb_atomic_check in
https://patchwork.freedesktop.org/patch/472324/
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 39 +--
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 8
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 10 -
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 ---
4 files changed, 9 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4530549850f0..e1c43a0c0643 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -573,7 +573,6 @@ static int dpu_encoder_virt_atomic_check(
struct drm_display_mode *adj_mode;
struct msm_display_topology topology;
struct dpu_global_state *global_state;
- int i = 0;
int ret = 0;
if (!drm_enc || !crtc_state || !conn_state) {
@@ -595,39 +594,19 @@ static int dpu_encoder_virt_atomic_check(
trace_dpu_enc_atomic_check(DRMID(drm_enc));
- /* perform atomic check on the first physical encoder (master) */
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
- if (phys->ops.atomic_check)
- ret = phys->ops.atomic_check(phys, crtc_state,
- conn_state);
- else if (phys->ops.mode_fixup)
- if (!phys->ops.mode_fixup(phys, mode, adj_mode))
- ret = -EINVAL;
-
- if (ret) {
- DPU_ERROR_ENC(dpu_enc,
- "mode unsupported, phys idx %d\n", i);
- break;
- }
- }
-
topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
/* Reserve dynamic resources now. */
- if (!ret) {
- /*
- * Release and Allocate resources on every modeset
- * Dont allocate when active is false.
- */
- if (drm_atomic_crtc_needs_modeset(crtc_state)) {
- dpu_rm_release(global_state, drm_enc);
+ /*
+ * Release and Allocate resources on every modeset
+ * Dont allocate when active is false.
+ */
+ if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+ dpu_rm_release(global_state, drm_enc);
- if (!crtc_state->active_changed || crtc_state->active)
- ret = dpu_rm_reserve(_kms->rm, global_state,
- drm_enc, crtc_state, topology);
- }
+ if (!crtc_state->active_changed || crtc_state->active)
+ ret = dpu_rm_reserve(_kms->rm, global_state,
+ drm_enc, crtc_state, topology);
}
trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index e7270eb6b84b..159deb8ed7fb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -84,12 +84,10 @@ struct dpu_encoder_virt_ops {
* @is_master: Whether this phys_enc is the current master
* encoder. Can be switched at enable time. Based
* on split_role and current mode (CMD/VID).
- * @mode_fixup: DRM Call. Fixup a DRM mode.
* @mode_set: DRM Call. Set a DRM mode.
* This likely caches the mode, for use at enable.
* @enable: DRM Call. Enable a DRM mode.
* @disable: DRM Call. Disable mode.
- * @atomic_check: DRM Call. Atomic check new DRM state.
* @destroy: DRM Call. Destroy and release resources.
* @get_hw_resources: Populate the structure with the hardware
* resources that this phys_enc is using.
@@ -117,17 +115,11 @@ struct dpu_encoder_phys_ops {
struct dentry *debugfs_root);
void (*prepare_commit)(struct dpu_encoder_phys *encoder);
bool (*is_master)(struct dpu_encoder_phys *encoder);
- bool (*mode_fixup)(struct dpu_encoder_phys *encoder,
- const struct drm_display_mode *mode,
-
Re: [PATCH 5/7] drm/msm/dpu: encoder: drop unused callbacks
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
Both cmd and vid backends provide no atomic_check() callbacks and
useless mode_fixup() callbacks. Drop support for both of them.
Signed-off-by: Dmitry Baryshkov
We can drop mode_fixup() but I am using atomic_check() for WB to
validate whether the programmed FB exceeds WB limits OR
the programmed mode. Hence without an alternative for that we cannot
drop that
Please refer to dpu_encoder_phys_wb_atomic_check in
https://patchwork.freedesktop.org/patch/472324/
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 39 +--
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 8
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 10 -
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 ---
4 files changed, 9 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4530549850f0..e1c43a0c0643 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -573,7 +573,6 @@ static int dpu_encoder_virt_atomic_check(
struct drm_display_mode *adj_mode;
struct msm_display_topology topology;
struct dpu_global_state *global_state;
- int i = 0;
int ret = 0;
if (!drm_enc || !crtc_state || !conn_state) {
@@ -595,39 +594,19 @@ static int dpu_encoder_virt_atomic_check(
trace_dpu_enc_atomic_check(DRMID(drm_enc));
- /* perform atomic check on the first physical encoder (master) */
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
- if (phys->ops.atomic_check)
- ret = phys->ops.atomic_check(phys, crtc_state,
- conn_state);
- else if (phys->ops.mode_fixup)
- if (!phys->ops.mode_fixup(phys, mode, adj_mode))
- ret = -EINVAL;
-
- if (ret) {
- DPU_ERROR_ENC(dpu_enc,
- "mode unsupported, phys idx %d\n", i);
- break;
- }
- }
-
topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
/* Reserve dynamic resources now. */
- if (!ret) {
- /*
-* Release and Allocate resources on every modeset
-* Dont allocate when active is false.
-*/
- if (drm_atomic_crtc_needs_modeset(crtc_state)) {
- dpu_rm_release(global_state, drm_enc);
+ /*
+* Release and Allocate resources on every modeset
+* Dont allocate when active is false.
+*/
+ if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+ dpu_rm_release(global_state, drm_enc);
- if (!crtc_state->active_changed || crtc_state->active)
- ret = dpu_rm_reserve(_kms->rm, global_state,
- drm_enc, crtc_state, topology);
- }
+ if (!crtc_state->active_changed || crtc_state->active)
+ ret = dpu_rm_reserve(_kms->rm, global_state,
+ drm_enc, crtc_state, topology);
}
trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index e7270eb6b84b..159deb8ed7fb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -84,12 +84,10 @@ struct dpu_encoder_virt_ops {
* @is_master:Whether this phys_enc is the current
master
*encoder. Can be switched at enable time. Based
*on split_role and current mode (CMD/VID).
- * @mode_fixup:DRM Call. Fixup a DRM mode.
* @mode_set: DRM Call. Set a DRM mode.
*This likely caches the mode, for use at enable.
* @enable: DRM Call. Enable a DRM mode.
* @disable: DRM Call. Disable mode.
- * @atomic_check: DRM Call. Atomic check new DRM state.
* @destroy: DRM Call. Destroy and release resources.
* @get_hw_resources: Populate the structure with the hardware
*resources that this phys_enc is using.
@@ -117,17 +115,11 @@ struct dpu_encoder_phys_ops {
struct dentry *debugfs_root);
void (*prepare_commit)(struct dpu_encoder_phys *encoder);
bool (*is_master)(struct dpu_encoder_phys *encoder);
- bool (*mode_fixup)(struct dpu_encoder_phys *encoder,
- const struct drm_display_mode *mode,
- struct drm_display_mode
Re: [PATCH 4/7] drm/msm/dpu: drop bus_scaling_client field
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
We do not use MSM bus client, so drop bus_scaling_client field from
dpu_encoder_virt.
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 6c1a19ffae38..4530549850f0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -127,7 +127,6 @@ enum dpu_enc_rc_states {
*Virtual encoder registers itself with the DRM Framework as the encoder.
* @base: drm_encoder base class for registration with DRM
* @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes
- * @bus_scaling_client:Client handle to the bus scaling interface
* @enabled: True if the encoder is active, protected by enc_lock
* @num_phys_encs:Actual number of physical encoders contained.
* @phys_encs:Container of physical encoders managed.
@@ -172,7 +171,6 @@ enum dpu_enc_rc_states {
struct dpu_encoder_virt {
struct drm_encoder base;
spinlock_t enc_spinlock;
- uint32_t bus_scaling_client;
bool enabled;
Re: [PATCH 3/7] drm/msm/dpu: remove msm_dp cached in dpu_encoder_virt
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
Stop caching msm_dp instance in dpu_encoder_virt since it's not used
now.
Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and
disable")
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e8fc029ad607..6c1a19ffae38 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -168,7 +168,6 @@ enum dpu_enc_rc_states {
* @vsync_event_work: worker to handle vsync event for autorefresh
* @topology: topology of the display
* @idle_timeout: idle timeout duration in milliseconds
- * @dp:msm_dp pointer, for DP encoders
*/
struct dpu_encoder_virt {
struct drm_encoder base;
@@ -207,8 +206,6 @@ struct dpu_encoder_virt {
struct msm_display_topology topology;
u32 idle_timeout;
-
- struct msm_dp *dp;
};
#define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
@@ -2118,8 +2115,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct
drm_encoder *enc,
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
- else if (disp_info->intf_type == INTF_DP || disp_info->intf_type ==
INTF_EDP)
- dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code
On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
which intf type we mean, pass INTF_DSI/INTF_DP directly.
Typically, I am only seeing a 1:1 mapping like
DRM_MODE_ENCODER_DSI means DSI
DRM_MODE_ENCODER_VIRTUAL means WB
So I am not seeing any guessing for the encoder.
The only conflict I am seeing is between DP and EDP as both use
DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
But that has been marked as a "FIXME" below.
I am suggesting an approach to handle that as well below.
Let me know if you agree with that.
While we are at it, fix the DP audio enablement code which was comparing
intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
DRM_MODE_CONNECTOR_DisplayPort (= 10).
Which would never succeed.
This is a surprising catch for me and left me thinking for a while about
how DP audio is working with this bug because that piece of code was
done to program a register which is needed for DP audio.
This bug happened due to difference in the meaning of intf_type between
upstream and downstream.
After checking more, we found that the register in question has been
deprecated on newer chipsets so I have asked Kuogee to selectively
program it. Here is the change for that:
https://patchwork.freedesktop.org/patch/473869/
Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port on MSM")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +--
3 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1e648db439f9..e8fc029ad607 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
hw_mdptop = phys_enc->hw_mdptop;
disp_info = _enc->disp_info;
- if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
+ if (disp_info->intf_type != INTF_DSI)
return;
/**
@@ -555,7 +555,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
else
topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
+ if (dpu_enc->disp_info.intf_type == INTF_DSI) {
if (dpu_kms->catalog->dspp &&
(dpu_kms->catalog->dspp_count >= topology.num_lm))
topology.num_dspp = topology.num_lm;
@@ -1099,7 +1099,7 @@ static void _dpu_encoder_virt_enable_helper(struct
drm_encoder *drm_enc)
}
- if (dpu_enc->disp_info.intf_type == DRM_MODE_CONNECTOR_DisplayPort &&
+ if (dpu_enc->disp_info.intf_type == INTF_DP &&
dpu_enc->cur_master->hw_mdptop &&
dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
@@ -1107,7 +1107,7 @@ static void _dpu_encoder_virt_enable_helper(struct
drm_encoder *drm_enc)
_dpu_encoder_update_vsync_source(dpu_enc, _enc->disp_info);
- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
+ if (dpu_enc->disp_info.intf_type == INTF_DSI &&
!WARN_ON(dpu_enc->num_phys_encs == 0)) {
unsigned bpc =
dpu_enc->phys_encs[0]->connector->display_info.bpc;
for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
@@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
{
int ret = 0;
int i = 0;
- enum dpu_intf_type intf_type = INTF_NONE;
struct dpu_enc_phys_init_params phys_params;
if (!dpu_enc) {
@@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
phys_params.parent_ops = _encoder_parent_ops;
phys_params.enc_spinlock = _enc->enc_spinlock;
- switch (disp_info->intf_type) {
- case DRM_MODE_ENCODER_DSI:
- intf_type = INTF_DSI;
- break;
- case DRM_MODE_ENCODER_TMDS:
- intf_type = INTF_DP;
- break;
- }
-
WARN_ON(disp_info->num_of_h_tiles < 1);
DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", disp_info->num_of_h_tiles);
@@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
i, controller_id, phys_params.split_role);
phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
-
intf_type,
-
controller_id);
+ disp_info->intf_type,
+
Re: [PATCH] drm/msm: populate intf_audio_select() base on hardware capability
On Sat, 12 Feb 2022 at 02:23, Kuogee Hsieh wrote:
>
> intf_audio_select() callback function use to configure
> HDMI_DP_CORE_SELECT to decide audio output routes to HDMI or DP
> interface. HDMI is obsoleted at newer chipset. To keep supporting
> legacy hdmi application, intf_audio_select call back function have
> to be populated base on hardware chip capability where legacy
> chipsets have has_audio_select flag set to true.
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 1 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 9 ++---
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
> 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 272b14b..23680e7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -201,6 +201,7 @@ static const struct dpu_caps sdm845_dpu_caps = {
> .has_dim_layer = true,
> .has_idle_pc = true,
> .has_3d_merge = true,
> + .has_audio_select = true,
> .max_linewidth = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
> .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> .max_hdeci_exp = MAX_HORZ_DECIMATION,
> @@ -229,6 +230,7 @@ static const struct dpu_caps sm8150_dpu_caps = {
> .has_dim_layer = true,
> .has_idle_pc = true,
> .has_3d_merge = true,
> + .has_audio_select = true,
> .max_linewidth = 4096,
> .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> .max_hdeci_exp = MAX_HORZ_DECIMATION,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index e5a96d6..b33f91b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -357,6 +357,7 @@ struct dpu_caps {
> bool has_dim_layer;
> bool has_idle_pc;
> bool has_3d_merge;
> + bool has_audio_select;
I'd suggest adding a bit to dpu_mdp_cfg's features instead, following
the example of other hardware blocks.
> /* SSPP limits */
> u32 max_linewidth;
> u32 pixel_ram_size;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> index 282e3c6..e608f4d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> @@ -261,14 +261,17 @@ static void dpu_hw_intf_audio_select(struct dpu_hw_mdp
> *mdp)
> }
>
> static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
> - unsigned long cap)
> + unsigned long cap,
> + const struct dpu_mdss_cfg *m)
> {
> ops->setup_split_pipe = dpu_hw_setup_split_pipe;
> ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl;
> ops->get_danger_status = dpu_hw_get_danger_status;
> ops->setup_vsync_source = dpu_hw_setup_vsync_source;
> ops->get_safe_status = dpu_hw_get_safe_status;
> - ops->intf_audio_select = dpu_hw_intf_audio_select;
> +
> + if (m->caps->has_audio_select)
> + ops->intf_audio_select = dpu_hw_intf_audio_select;
> }
>
> static const struct dpu_mdp_cfg *_top_offset(enum dpu_mdp mdp,
> @@ -320,7 +323,7 @@ struct dpu_hw_mdp *dpu_hw_mdptop_init(enum dpu_mdp idx,
> */
> mdp->idx = idx;
> mdp->caps = cfg;
> - _setup_mdp_ops(>ops, mdp->caps->features);
> + _setup_mdp_ops(>ops, mdp->caps->features, m);
>
> return mdp;
> }
--
With best wishes
Dmitry
[PATCH] drm/msm: populate intf_audio_select() base on hardware capability
intf_audio_select() callback function use to configure
HDMI_DP_CORE_SELECT to decide audio output routes to HDMI or DP
interface. HDMI is obsoleted at newer chipset. To keep supporting
legacy hdmi application, intf_audio_select call back function have
to be populated base on hardware chip capability where legacy
chipsets have has_audio_select flag set to true.
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 9 ++---
3 files changed, 9 insertions(+), 3 deletions(-)
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 272b14b..23680e7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -201,6 +201,7 @@ static const struct dpu_caps sdm845_dpu_caps = {
.has_dim_layer = true,
.has_idle_pc = true,
.has_3d_merge = true,
+ .has_audio_select = true,
.max_linewidth = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
.max_hdeci_exp = MAX_HORZ_DECIMATION,
@@ -229,6 +230,7 @@ static const struct dpu_caps sm8150_dpu_caps = {
.has_dim_layer = true,
.has_idle_pc = true,
.has_3d_merge = true,
+ .has_audio_select = true,
.max_linewidth = 4096,
.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
.max_hdeci_exp = MAX_HORZ_DECIMATION,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index e5a96d6..b33f91b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -357,6 +357,7 @@ struct dpu_caps {
bool has_dim_layer;
bool has_idle_pc;
bool has_3d_merge;
+ bool has_audio_select;
/* SSPP limits */
u32 max_linewidth;
u32 pixel_ram_size;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
index 282e3c6..e608f4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
@@ -261,14 +261,17 @@ static void dpu_hw_intf_audio_select(struct dpu_hw_mdp
*mdp)
}
static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
- unsigned long cap)
+ unsigned long cap,
+ const struct dpu_mdss_cfg *m)
{
ops->setup_split_pipe = dpu_hw_setup_split_pipe;
ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl;
ops->get_danger_status = dpu_hw_get_danger_status;
ops->setup_vsync_source = dpu_hw_setup_vsync_source;
ops->get_safe_status = dpu_hw_get_safe_status;
- ops->intf_audio_select = dpu_hw_intf_audio_select;
+
+ if (m->caps->has_audio_select)
+ ops->intf_audio_select = dpu_hw_intf_audio_select;
}
static const struct dpu_mdp_cfg *_top_offset(enum dpu_mdp mdp,
@@ -320,7 +323,7 @@ struct dpu_hw_mdp *dpu_hw_mdptop_init(enum dpu_mdp idx,
*/
mdp->idx = idx;
mdp->caps = cfg;
- _setup_mdp_ops(>ops, mdp->caps->features);
+ _setup_mdp_ops(>ops, mdp->caps->features, m);
return mdp;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[RFC PATCH v2 2/5] drm/msm/dp: support attaching bridges to the DP encoder
Currently DP driver will allocate panel bridge for eDP panels. This
supports only the following topology:
- eDP encoder ⇒ eDP panel (wrapped using panel-bridge)
Simplify this code to just check if there is any next bridge in the
chain (be it a panel bridge or regular bridge). Rename panel_bridge
field to next_bridge accordingly.
This allows one to use e.g. one of the following display topologies:
- eDP encoder ⇒ ptn3460 ⇒ fixed LVDS panel
- eDP encoder ⇒ ptn3460 ⇒ LVDS connector with EDID lines for panel autodetect
- eDP encoder ⇒ ptn3460 ⇒ THC63LVD1024 ⇒ DPI panel.
- eDP encoder ⇒ LT8912 ⇒ DSI panel
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
drivers/gpu/drm/msm/dp/dp_display.h | 2 +-
drivers/gpu/drm/msm/dp/dp_drm.c | 4 ++--
drivers/gpu/drm/msm/dp/dp_parser.c | 31 +++--
drivers/gpu/drm/msm/dp/dp_parser.h | 2 +-
5 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 44d42c76c2a3..45f9a912ecc5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -266,7 +266,7 @@ static int dp_display_bind(struct device *dev, struct
device *master,
goto end;
}
- dp->dp_display.panel_bridge = dp->parser->panel_bridge;
+ dp->dp_display.next_bridge = dp->parser->next_bridge;
dp->aux->drm_dev = drm;
rc = dp_aux_register(dp->aux);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index e3adcd578a90..7af2b186d2d9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -16,7 +16,7 @@ struct msm_dp {
struct drm_bridge *bridge;
struct drm_connector *connector;
struct drm_encoder *encoder;
- struct drm_bridge *panel_bridge;
+ struct drm_bridge *next_bridge;
bool is_connected;
bool audio_enabled;
bool power_on;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 26ef41a4c1b6..80f59cf99089 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -236,9 +236,9 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp
*dp_display, struct drm_devi
return ERR_PTR(rc);
}
- if (dp_display->panel_bridge) {
+ if (dp_display->next_bridge) {
rc = drm_bridge_attach(dp_display->encoder,
- dp_display->panel_bridge, bridge,
+ dp_display->next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
if (rc < 0) {
DRM_ERROR("failed to attach panel bridge: %d\n", rc);
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index a7acc23f742b..901d7967370f 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -265,23 +265,16 @@ static int dp_parser_clock(struct dp_parser *parser)
return 0;
}
-static int dp_parser_find_panel(struct dp_parser *parser)
+static int dp_parser_find_next_bridge(struct dp_parser *parser)
{
struct device *dev = >pdev->dev;
- struct drm_panel *panel;
- int rc;
+ struct drm_bridge *bridge;
- rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
- if (rc) {
- DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
- return rc;
- }
+ bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
+ if (IS_ERR(bridge))
+ return PTR_ERR(bridge);
- parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
- if (IS_ERR(parser->panel_bridge)) {
- DRM_ERROR("failed to create panel bridge\n");
- return PTR_ERR(parser->panel_bridge);
- }
+ parser->next_bridge = bridge;
return 0;
}
@@ -307,10 +300,18 @@ static int dp_parser_parse(struct dp_parser *parser, int
connector_type)
if (rc)
return rc;
+ /*
+* Currently we support external bridges only for eDP connectors.
+*
+* No external bridges are expected for the DisplayPort connector,
+* it is physically present in a form of a DP or USB-C connector.
+*/
if (connector_type == DRM_MODE_CONNECTOR_eDP) {
- rc = dp_parser_find_panel(parser);
- if (rc)
+ rc = dp_parser_find_next_bridge(parser);
+ if (rc) {
+ DRM_ERROR("DP: failed to find next bridge\n");
return rc;
+ }
}
/* Map the corresponding regulator information according to
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
b/drivers/gpu/drm/msm/dp/dp_parser.h
index 3172da089421..4cec851e38d9 100644
---
[RFC PATCH v2 3/5] drm/msm/dp: support finding next bridge even for DP interfaces
It is possible to supply display-connector (bridge) to the DP interface,
add support for parsing it too.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_parser.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index 901d7967370f..1056b8d5755b 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -301,17 +301,22 @@ static int dp_parser_parse(struct dp_parser *parser, int
connector_type)
return rc;
/*
-* Currently we support external bridges only for eDP connectors.
+* External bridges are mandatory for eDP interfaces: one has to
+* provide at least an eDP panel (which gets wrapped into panel-bridge).
*
-* No external bridges are expected for the DisplayPort connector,
-* it is physically present in a form of a DP or USB-C connector.
+* For DisplayPort interfaces external bridges are optional, so
+* silently ignore an error if one is not present (-ENODEV).
*/
- if (connector_type == DRM_MODE_CONNECTOR_eDP) {
- rc = dp_parser_find_next_bridge(parser);
- if (rc) {
- DRM_ERROR("DP: failed to find next bridge\n");
+ rc = dp_parser_find_next_bridge(parser);
+ if (rc == -ENODEV) {
+ if (connector_type == DRM_MODE_CONNECTOR_eDP) {
+ DRM_ERROR("eDP: next bridge is not present\n");
return rc;
}
+ } else if (rc) {
+ if (rc != -EPROBE_DEFER)
+ DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
+ return rc;
}
/* Map the corresponding regulator information according to
--
2.34.1
[RFC PATCH v2 5/5] drm/msm/dp: remove extra wrappers and public functions
dp_bridge's functions are thin wrappers around the msm_dp_display_*
family. Squash dp_bridge callbacks into respective msm_dp_display
functions, removing the latter functions from public space.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 54 +++---
drivers/gpu/drm/msm/dp/dp_display.h | 1 -
drivers/gpu/drm/msm/dp/dp_drm.c | 72 ++---
drivers/gpu/drm/msm/dp/dp_drm.h | 22 -
drivers/gpu/drm/msm/msm_drv.h | 31 -
5 files changed, 60 insertions(+), 120 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 59e5e5b8e5b4..a9b641a68bff 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,7 +10,6 @@
#include
#include
#include
-#include
#include "msm_drv.h"
#include "msm_kms.h"
@@ -945,18 +944,36 @@ int dp_display_set_plugged_cb(struct msm_dp *dp_display,
return 0;
}
-int dp_display_validate_mode(struct msm_dp *dp, u32 mode_pclk_khz)
+/**
+ * dp_bridge_mode_valid - callback to determine if specified mode is valid
+ * @bridge: Pointer to drm bridge structure
+ * @info: display info
+ * @mode: Pointer to drm mode structure
+ * Returns: Validity status for specified mode
+ */
+enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
+ const struct drm_display_info *info,
+ const struct drm_display_mode *mode)
{
const u32 num_components = 3, default_bpp = 24;
struct dp_display_private *dp_display;
struct dp_link_info *link_info;
u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0;
+ struct msm_dp *dp;
+ int mode_pclk_khz = mode->clock;
+
+ dp = to_dp_bridge(bridge)->dp_display;
if (!dp || !mode_pclk_khz || !dp->connector) {
DRM_ERROR("invalid params\n");
return -EINVAL;
}
+ if ((dp->max_pclk_khz <= 0) ||
+ (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
+ (mode->clock > dp->max_pclk_khz))
+ return MODE_BAD;
+
dp_display = container_of(dp, struct dp_display_private, dp_display);
link_info = _display->panel->link_info;
@@ -1501,7 +1518,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct
drm_device *dev,
dp_display->encoder = encoder;
- dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
+ dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
if (IS_ERR(dp_display->bridge)) {
ret = PTR_ERR(dp_display->bridge);
DRM_DEV_ERROR(dev->dev,
@@ -1528,8 +1545,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
return 0;
}
-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
+void dp_bridge_enable(struct drm_bridge *drm_bridge)
{
+ struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
+ struct msm_dp *dp = dp_bridge->dp_display;
int rc = 0;
struct dp_display_private *dp_display;
u32 state;
@@ -1537,7 +1556,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct
drm_encoder *encoder)
dp_display = container_of(dp, struct dp_display_private, dp_display);
if (!dp_display->dp_mode.drm_mode.clock) {
DRM_ERROR("invalid params\n");
- return -EINVAL;
+ return;
}
mutex_lock(_display->event_mutex);
@@ -1549,14 +1568,14 @@ int msm_dp_display_enable(struct msm_dp *dp, struct
drm_encoder *encoder)
if (rc) {
DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
mutex_unlock(_display->event_mutex);
- return rc;
+ return;
}
rc = dp_display_prepare(dp);
if (rc) {
DRM_ERROR("DP display prepare failed, rc=%d\n", rc);
mutex_unlock(_display->event_mutex);
- return rc;
+ return;
}
state = dp_display->hpd_state;
@@ -1581,23 +1600,23 @@ int msm_dp_display_enable(struct msm_dp *dp, struct
drm_encoder *encoder)
dp_display->hpd_state = ST_CONNECTED;
mutex_unlock(_display->event_mutex);
-
- return rc;
}
-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
+void dp_bridge_disable(struct drm_bridge *drm_bridge)
{
+ struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
+ struct msm_dp *dp = dp_bridge->dp_display;
struct dp_display_private *dp_display;
dp_display = container_of(dp, struct dp_display_private, dp_display);
dp_ctrl_push_idle(dp_display->ctrl);
-
- return 0;
}
-int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
+void dp_bridge_post_disable(struct drm_bridge
[RFC PATCH v2 1/5] drm/msm/dp: fix panel bridge attachment
In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display
enable and disable") the DP driver received a drm_bridge instance, which
is always attached to the encoder as a root bridge. However it conflicts
with the panel_bridge support for eDP panels. The panel bridge attaches
to the encoder before the "dp" bridge has a chace to do so. Change
panel_bridge attachment to come after dp_bridge attachment.
Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable
and disable")
Cc: Kuogee Hsieh
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index d4d360d19eba..26ef41a4c1b6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -169,16 +169,6 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display)
drm_connector_attach_encoder(connector, dp_display->encoder);
- if (dp_display->panel_bridge) {
- ret = drm_bridge_attach(dp_display->encoder,
- dp_display->panel_bridge, NULL,
- DRM_BRIDGE_ATTACH_NO_CONNECTOR);
- if (ret < 0) {
- DRM_ERROR("failed to attach panel bridge: %d\n", ret);
- return ERR_PTR(ret);
- }
- }
-
return connector;
}
@@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp
*dp_display, struct drm_devi
return ERR_PTR(rc);
}
+ if (dp_display->panel_bridge) {
+ rc = drm_bridge_attach(dp_display->encoder,
+ dp_display->panel_bridge, bridge,
+ DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+ if (rc < 0) {
+ DRM_ERROR("failed to attach panel bridge: %d\n", rc);
+ drm_bridge_remove(bridge);
+ return ERR_PTR(rc);
+ }
+ }
+
return bridge;
}
--
2.34.1
[RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
There is little point in having both connector and root bridge
implementation in the same driver. Move connector's functionality to the
bridge to let next bridge in chain to override it.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 22 +++---
drivers/gpu/drm/msm/dp/dp_drm.c | 113 ++--
2 files changed, 52 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 45f9a912ecc5..59e5e5b8e5b4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1501,6 +1501,17 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
dp_display->encoder = encoder;
+ dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
+ if (IS_ERR(dp_display->bridge)) {
+ ret = PTR_ERR(dp_display->bridge);
+ DRM_DEV_ERROR(dev->dev,
+ "failed to create dp bridge: %d\n", ret);
+ dp_display->bridge = NULL;
+ return ret;
+ }
+
+ priv->bridges[priv->num_bridges++] = dp_display->bridge;
+
dp_display->connector = dp_drm_connector_init(dp_display);
if (IS_ERR(dp_display->connector)) {
ret = PTR_ERR(dp_display->connector);
@@ -1514,17 +1525,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
priv->connectors[priv->num_connectors++] = dp_display->connector;
- dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
- if (IS_ERR(dp_display->bridge)) {
- ret = PTR_ERR(dp_display->bridge);
- DRM_DEV_ERROR(dev->dev,
- "failed to create dp bridge: %d\n", ret);
- dp_display->bridge = NULL;
- return ret;
- }
-
- priv->bridges[priv->num_bridges++] = dp_display->bridge;
-
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 80f59cf99089..57800b865fe6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include "msm_drv.h"
@@ -20,24 +21,16 @@ struct msm_dp_bridge {
#define to_dp_display(x) container_of((x), struct msm_dp_bridge, bridge)
-struct dp_connector {
- struct drm_connector base;
- struct msm_dp *dp_display;
-};
-#define to_dp_connector(x) container_of(x, struct dp_connector, base)
-
/**
- * dp_connector_detect - callback to determine if connector is connected
- * @conn: Pointer to drm connector structure
- * @force: Force detect setting from drm framework
- * Returns: Connector 'is connected' status
+ * dp_bridge_detect - callback to determine if connector is connected
+ * @bridge: Pointer to drm bridge structure
+ * Returns: Bridge's 'is connected' status
*/
-static enum drm_connector_status dp_connector_detect(struct drm_connector
*conn,
- bool force)
+static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge)
{
struct msm_dp *dp;
- dp = to_dp_connector(conn)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
DRM_DEBUG_DP("is_connected = %s\n",
(dp->is_connected) ? "true" : "false");
@@ -47,11 +40,12 @@ static enum drm_connector_status dp_connector_detect(struct
drm_connector *conn,
}
/**
- * dp_connector_get_modes - callback to add drm modes via drm_mode_probed_add()
+ * dp_bridge_get_modes - callback to add drm modes via drm_mode_probed_add()
+ * @bridge: Poiner to drm bridge
* @connector: Pointer to drm connector structure
* Returns: Number of modes added
*/
-static int dp_connector_get_modes(struct drm_connector *connector)
+static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector
*connector)
{
int rc = 0;
struct msm_dp *dp;
@@ -61,7 +55,7 @@ static int dp_connector_get_modes(struct drm_connector
*connector)
if (!connector)
return 0;
- dp = to_dp_connector(connector)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL);
if (!dp_mode)
@@ -102,18 +96,20 @@ static int dp_connector_get_modes(struct drm_connector
*connector)
}
/**
- * dp_connector_mode_valid - callback to determine if specified mode is valid
- * @connector: Pointer to drm connector structure
+ * dp_bridge_mode_valid - callback to determine if specified mode is valid
+ * @bridge: Pointer to drm bridge structure
+ * @info: display info
* @mode: Pointer to drm mode structure
* Returns: Validity status for specified mode
*/
-static enum drm_mode_status dp_connector_mode_valid(
- struct drm_connector *connector,
- struct drm_display_mode *mode)
+static enum drm_mode_status dp_bridge_mode_valid(
+ struct drm_bridge *bridge,
[RFC PATCH v2 0/5] Simplify and correct msm/dp bridge implementation
I noticed that commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for
display
enable and disable") conflicts with the panel-edp (panel bridge)
support. Both bridges will try to attach directly to the drm encoder
itself without forming a proper bridge chain.
Initially I started writing lengthy letter describing what is broken and
how it should be fixed. Then at some point I stopped and quickly coded
this RFC (which is compile-tested only).
Comments and tests (on both DP and eDP setups) are more than welcome.
Changes since RFC v1:
- Expanded commit messages to reference possible setups
Added details about possible bridges, usage, etc
- Changed handling of errors for devm_drm_of_get_bridge(). Made the
-ENODEV fatal for the eDP connectors only, all other errors should be
fatal for both eDP and DP.
Dmitry Baryshkov (5):
drm/msm/dp: fix panel bridge attachment
drm/msm/dp: support attaching bridges to the DP encoder
drm/msm/dp: support finding next bridge even for DP interfaces
drm/msm/dp: replace dp_connector with drm_bridge_connector
drm/msm/dp: remove extra wrappers and public functions
drivers/gpu/drm/msm/dp/dp_display.c | 76 +++-
drivers/gpu/drm/msm/dp/dp_display.h | 3 +-
drivers/gpu/drm/msm/dp/dp_drm.c | 186 +++-
drivers/gpu/drm/msm/dp/dp_drm.h | 22 +++-
drivers/gpu/drm/msm/dp/dp_parser.c | 38 +++---
drivers/gpu/drm/msm/dp/dp_parser.h | 2 +-
drivers/gpu/drm/msm/msm_drv.h | 31 -
7 files changed, 137 insertions(+), 221 deletions(-)
base-commit: 6aa89ae1fb049614b7e03e24485bbfb96754a02b
prerequisite-patch-id: 89e012b5b7da1a90cc243cc4c305400a4fafdf0b
prerequisite-patch-id: 0de618d54d5fea5b48c2b540c8731a1a7e2f4c15
prerequisite-patch-id: a9b1a27e9800626cc0ebc73291d65c2790670583
prerequisite-patch-id: 2067135baa2995fbcbfd6793b61e39027e6b7516
prerequisite-patch-id: 0591114f3c59f9376ba25e77e7a48daf90cf7aa6
prerequisite-patch-id: 684cf6c7a177cb7c6c9d83a859eec0acef5c132c
prerequisite-patch-id: 083313bc9b19fcf7fed78f63a3cb0752f54cec4f
prerequisite-patch-id: 6e46e24cd7471ba38679b3d6f99a1132fa1154b3
--
2.34.1
Re: [PATCH v2 3/9] PCI: drop `is_thunderbolt` attribute
Hi Mario,
On Thu, Feb 10, 2022 at 04:43:23PM -0600, Mario Limonciello wrote:
> The `is_thunderbolt` attribute is currently a dumping ground for a
> variety of things.
>
> Instead use the driver core removable attribute to indicate the
> detail a device is attached to a thunderbolt or USB4 chain.
>
> Signed-off-by: Mario Limonciello
> ---
> drivers/pci/pci.c | 2 +-
> drivers/pci/probe.c | 20 +++-
> drivers/platform/x86/apple-gmux.c | 2 +-
> include/linux/pci.h | 5 ++---
> 4 files changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f..1264984d5e6d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2955,7 +2955,7 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> return true;
>
> /* Even the oldest 2010 Thunderbolt controller supports D3. */
> - if (bridge->is_thunderbolt)
> + if (dev_is_removable(>dev))
For this, I'm not entirely sure this is what we want. The purpose of
this check is to enable port power management of Apple systems with
Intel Thunderbolt controller and therefore checking for "removable" here
is kind of misleading IMHO.
I wonder if we could instead remove the check completely here and rely
on the below:
if (platform_pci_bridge_d3(bridge))
return true;
and that would then look like:
static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
{
if (pci_use_mid_pm())
return false;
if (acpi_pci_bridge_d3(dev))
return true;
if (device_property_read_bool(>dev, "HotPlugSupportInD3"))
return true;
return false;
}
and then make a quirk in quirks.c that adds the software node property
for the Apple systems? Or something along those lines.
@Lukas, what do you think?
Re: [PATCH v2 4/9] PCI: mark USB4 devices as removable
Hi Mario, On Thu, Feb 10, 2022 at 04:43:24PM -0600, Mario Limonciello wrote: > USB4 class devices are also removable like Intel Thunderbolt devices. > > Drivers of downstream devices use this information to declare functional > differences in how the drivers perform by knowing that they are connected > to an upstream TBT/USB4 port. This may not be covering the integrated controllers. For discrete, yes but if it is the PCIe root ports that start the PCIe topology (over the PCIe tunnels) this does not work. For integrated we have the "usb4-host-interface" ACPI property that tells this for each port: https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers and for discrete there is the PCIe DVSEC that can be used (see the USB4 spec archive it includes the "USB4 DVSEC Version 1.0.pdf" that has more information). I would expect AMD controller (assuming it is discrete) implements this too. So I'm proposing that we mark the devices that are below PCIe ports (root, downstream) that fall in the above categories as "removable". This is then not dependent on checking the USB4 controller and how it is setup in a particular system.
Re: [PATCH v2 1/9] thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4
On Thu, Feb 10, 2022 at 04:43:21PM -0600, Mario Limonciello wrote: > This PCI class definition of the USB4 device is currently located only in > the thunderbolt driver. > > It will be needed by a few other drivers for upcoming changes. Move it into > the common include file. > > Acked-by: Alex Deucher > Signed-off-by: Mario Limonciello Acked-by: Mika Westerberg
Re: [PATCH v3 07/12] PCI: Set ports for discrete USB4 controllers appropriately
Make the subject specific, not just "appropriately." I think you're
marking something *removable*, so include that.
On Fri, Feb 11, 2022 at 01:32:45PM -0600, Mario Limonciello wrote:
> Discrete USB4 controllers won't have ACPI nodes specifying which
> PCIe or XHCI port they are linked with.
>
> In order to set the removable attribute appropriately, use the
> USB4 DVSEC extended capabability set on these root ports to determine
> if they are located on a discrete USB4 controller.
s/capabability/capability/
> Suggested-by: Mika Westerberg
> Link: https://usb.org/sites/default/files/USB4%20Specification%202026.zip
> Signed-off-by: Mario Limonciello
> ---
> drivers/pci/probe.c | 33 +
> include/linux/pci_ids.h | 2 ++
> 2 files changed, 35 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 67ca33188cba..1ed3e24db11e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -25,6 +25,8 @@
> #define CARDBUS_LATENCY_TIMER176 /* secondary latency timer */
> #define CARDBUS_RESERVE_BUSNR3
>
> +#define PCI_DVSEC_ID_USB40x23
> +
> static struct resource busn_resource = {
> .name = "PCI busn",
> .start = 0,
> @@ -1590,6 +1592,36 @@ static void set_pcie_untrusted(struct pci_dev *dev)
> dev->untrusted = true;
> }
>
> +static bool pci_is_discrete_usb4(struct pci_dev *dev)
> +{
> + int dvsec_val = 0, pos;
> + u32 hdr;
> +
> + /* USB4 spec says vendors can use either */
> + pos = pci_find_dvsec_capability(dev,
> + PCI_VENDOR_ID_INTEL,
> + PCI_DVSEC_ID_USB4);
> + if (pos) {
> + dvsec_val = 0x06;
> + } else {
> + pos = pci_find_dvsec_capability(dev,
> + PCI_VENDOR_ID_USB_IF,
> + PCI_DVSEC_ID_USB4);
> + if (pos)
> + dvsec_val = 0x01;
> + }
> + if (!dvsec_val)
> + return false;
> +
> + pci_read_config_dword(dev, pos + PCI_DVSEC_HEADER2, );
> + if ((hdr & GENMASK(15, 0)) != dvsec_val)
> + return false;
> + /* this port is used for either NHI/PCIe tunnel/USB tunnel */
Capitalize comment like others in this file.
Spec reference would be helpful, too. I don't know how to verify
any of these values. The Link: above is great, but name, revision,
section number would be even better.
> + if (hdr & GENMASK(18, 16))
> + return true;
> + return false;
> +}
> +
> static void pci_set_removable(struct pci_dev *dev)
> {
> struct pci_dev *parent = pci_upstream_bridge(dev);
> @@ -1612,6 +1644,7 @@ static void pci_set_removable(struct pci_dev *dev)
> if (vsec ||
> dev->class == PCI_CLASS_SERIAL_USB_USB4 ||
> pci_acpi_is_usb4(dev) ||
> + pci_is_discrete_usb4(dev) ||
> (parent &&
> (parent->external_facing || dev_is_removable(>dev
> dev_set_removable(>dev, DEVICE_REMOVABLE);
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 61b161d914f0..271326e058b9 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -3097,4 +3097,6 @@
>
> #define PCI_VENDOR_ID_NCUBE 0x10ff
>
> +#define PCI_VENDOR_ID_USB_IF 0x1EC0
This file is supposed to be sorted by Vendor ID. PCI_VENDOR_ID_XEN,
PCI_VENDOR_ID_OCZ, and PCI_VENDOR_ID_NCUBE screwed up, but you can put
USB_IF in the correct spot.
It's not 100%, but most entries use lower-case hex.
[pull] amdgpu, amdkfd, radeon drm-next-5.18
Hi Dave, Daniel, New stuff for 5.18. The following changes since commit 4efdddbce7c1329f00c458e85dcaf105aebdc0ed: Merge tag 'amd-drm-next-5.17-2022-01-12' of https://gitlab.freedesktop.org/agd5f/linux into drm-next (2022-01-14 15:42:28 +0100) are available in the Git repository at: https://gitlab.freedesktop.org/agd5f/linux.git tags/amd-drm-next-5.18-2022-02-11-1 for you to fetch changes up to 7f161df1a513e2961f4e3c96a8355c8ce93ad175: drm/amdkfd: replace err by dbg print at svm vram migration (2022-02-11 16:20:24 -0500) amd-drm-next-5.18-2022-02-11-1: amdgpu: - Clean up of power management code - Enable freesync video mode by default - Clean up of RAS code - Improve VRAM access for debug using SDMA - Coding style cleanups - SR-IOV fixes - More display FP reorg - TLB flush fixes for Arcuturus, Vega20 - Misc display fixes - Rework special register access methods for SR-IOV - DP2 fixes - DP tunneling fixes - DSC fixes - More IP discovery cleanups - Misc RAS fixes - Enable both SMU i2c buses where applicable - s2idle improvements - DPCS header cleanup - Add new CAP firmware support for SR-IOV amdkfd: - Misc cleanups - SVM fixes - CRIU support - Clean up MQD manager UAPI: - Add interface to amdgpu CTX ioctl to request a stable power state for profiling https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/207 - Add amdkfd support for CRIU https://github.com/checkpoint-restore/criu/pull/1709 - Remove old unused amdkfd debugger interface Was only implemented for Kaveri and was only ever used by an old HSA tool that was never open sourced radeon: - Fix error handling in radeon_driver_open_kms - UVD suspend fix - Misc fixes Aaron Liu (4): drm/amdgpu: convert code name to ip version for athub drm/amdgpu: add 1.3.1/2.4.0 athub CG support drm/amdgpu: add utcl2_harvest to gc 10.3.1 drm/amdgpu: check the GART table before invalidating TLB Agustin Gutierrez (1): drm/amd/display: Update watermark values for DCN301 Alex Deucher (22): drm/amdgpu/swsmu: make sienna cichlid function static drm/amdgpu/pm: move additional logic into amdgpu_dpm_force_performance_level drm/amdgpu: invert the logic in amdgpu_device_should_recover_gpu() drm/amdgpu: don't do resets on APUs which don't support it drm/amdgpu: drop flags check for CHIP_IP_DISCOVERY drm/amdgpu: filter out radeon secondary ids as well drm/amdgpu/display: adjust msleep limit in dp_wait_for_training_aux_rd_interval drm/amdgpu/display: use msleep rather than udelay for long delays drm/amdgpu/pm/smu7: drop message about VI performance levels drm/amdgpu: set APU flag based on IP discovery table drm/amdgpu: move PX checking into amdgpu_device_ip_early_init drm/amdgpu: move runtime pm init after drm and fbdev init drm/amdgpu: handle BACO synchronization with secondary funcs drm/amdgpu: convert amdgpu_display_supported_domains() to IP versions drm/amdgpu/swsmu/i2c: return an error if the SMU is not running drm/amdgpu/UAPI: add new CTX OP to get/set stable pstates drm/amdgpu: bump driver version for new CTX OP to set/get stable pstates drm/amdgpu: drop experimental flag on aldebaran drm/amdgpu/display: change pipe policy for DCN 2.0 drm/amdgpu: add missing license to dpcs_3_0_0 headers drm/amdgpu: move dpcs_3_0_0 headers from dcn to dpcs drm/amdgpu: move dpcs_3_0_3 headers from dcn to dpcs Alex Sierra (1): drm/amdkfd: replace err by dbg print at svm vram migration Alvin Lee (1): drm/amd/display: Driver support for MCLK query tool Anthony Koo (4): drm/amd/display: [FW Promotion] Release 0.0.100.0 drm/amd/display: [FW Promotion] Release 0.0.101.0 drm/amd/display: [FW Promotion] Release 0.0.102.0 drm/amd/display: [FW Promotion] Release 0.0.103.0 Aric Cyr (6): drm/amd/display: 3.2.168 drm/amd/display: 3.2.169 drm/amd/display: 3.2.170 drm/amd/display: Remove unnecessary function definition drm/amd/display: 3.2.171 drm/amd/display: 3.2.172 Aun-Ali Zaidi (1): drm/amd/display: Force link_rate as LINK_RATE_RBR2 for 2018 15" Apple Retina panels Bas Nieuwenhuizen (3): drm/amd/display: Fix FP start/end for dcn30_internal_validate_bw. drm/amd/display: Wrap dcn301_calculate_wm_and_dlg for FPU. drm/amdgpu/display: Remove t_srx_delay_us. Bing Guo (1): drm/amdgpu/display/dc: do blocked MST topology discovery at resume from S3/S4 Bokun Zhang (1): drm/amdgpu: Add interface to load SRIOV cap FW CHANDAN VURDIGERE NATARAJ (1): drm/amdgpu: Enable recovery on yellow carp Changcheng Deng (3): drm/amd/pm: Replace one-element array with flexible-array member drm/amdgpu: remove duplicate include in 'amdgpu_device.c' drm/amd/pm: remove
Re: [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk
On 2/11/2022 15:35, Bjorn Helgaas wrote:
On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote:
`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt
controller to indicate that D3 is possible. As this is used solely
for older Apple systems, move it into a quirk that enumerates across
all Intel TBT controllers.
Suggested-by: Mika Westerberg
Signed-off-by: Mario Limonciello
---
drivers/pci/pci.c| 12 +-
drivers/pci/quirks.c | 53
2 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f..5002e214c9a6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1064,7 +1064,13 @@ static inline bool platform_pci_bridge_d3(struct pci_dev
*dev)
if (pci_use_mid_pm())
return false;
- return acpi_pci_bridge_d3(dev);
+ if (acpi_pci_bridge_d3(dev))
+ return true;
+
+ if (device_property_read_bool(>dev, "HotPlugSupportInD3"))
+ return true;
Why do we need this? acpi_pci_bridge_d3() already looks for
"HotPlugSupportInD3".
The Apple machines don't have ACPI companion devices that specify this
property.
I guess this probes a different question; can
`device_property_read_bool` be used in `acpi_pci_bridge_d3` instead of:
if (acpi_dev_get_property(adev, "HotPlugSupportInD3",
ACPI_TYPE_INTEGER, ) < 0)
return false;
return obj->integer.value == 1;
If so, then yeah this can probably be simplified.
+ return false;
}
/**
@@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
if (pci_bridge_d3_force)
return true;
- /* Even the oldest 2010 Thunderbolt controller supports D3. */
- if (bridge->is_thunderbolt)
- return true;
-
/* Platform might know better if the bridge supports D3 */
if (platform_pci_bridge_d3(bridge))
return true;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 6d3c88edde00..aaf098ca7d54 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
quirk_apple_poweroff_thunderbolt);
#endif
+/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but don't specify
+ * it in the ACPI tables
Wrap to fit in 80 columns like the rest of the file. Also use the:
/*
* comment ...
*/
style if it's more than one line.
I don't think "as old as 2010" is helpful here -- I assume 2010 is
there because there *were* no Thunderbolt controllers before 2010, but
the code doesn't check any dates, so we basically assume all Apple
machines of any age with the listed controllers can do this.
The old comment was saying that, which is where I got it from. Yeah,
I'll update it.
+ */
+static void quirk_apple_d3_thunderbolt(struct pci_dev *dev)
+{
+ struct property_entry properties[] = {
+ PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),
+ {},
+ };
+
+ if (!x86_apple_machine)
+ return;
The current code doesn't check x86_apple_machine, so this needs some
justification. How do I know this works the same as before?
Mika and Lucas were saying the only reason for this codepath was Apple
machines in the first place, which is where this idea came from.
Something specifically relevant is that the Apple machines use a SW
connection manager, whereas everyone else up until USB4 devices use a
firmware based connection manager with varying behaviors on generation
(ICM).
+
+ if (device_create_managed_software_node(>dev, properties, NULL))
+ pci_warn(dev, "could not add HotPlugSupportInD3 property");
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
+ quirk_apple_d3_thunderbolt);
The current code assumes *all* Thunderbolt controllers support D3, so
it would assume a controller released next year would support D3, but
this code would assume the opposite. Are we supposed to add
everything to this list, or do newer machines supply
HotPlugSupportInD3, or ...?
This quirk is intended specifically for Apple, which has stopped making
Intel machines with Intel TBT controllers.
So I don't believe the list should be growing any more, if anything it
might need to shrink if I got too many models that weren't actually in
Apple products. Lucas probably needs to confirm that.
How did you derive this list? (Question for the commit log and/or
comments here.)
I went to pci_ids.h and got all the Thunderbolt controllers listed there.
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EAGLE_RIDGE,
+ quirk_apple_d3_thunderbolt);
Re: [PATCH v3 06/12] PCI: Explicitly mark USB4 NHI devices as removable
On Fri, Feb 11, 2022 at 01:32:44PM -0600, Mario Limonciello wrote: > USB4 class devices are also removable like Intel Thunderbolt devices. Spec reference, please. > Drivers of downstream devices use this information to declare functional > differences in how the drivers perform by knowing that they are connected > to an upstream TBT/USB4 port. > > Reviewed-by: Macpaul Lin > Signed-off-by: Mario Limonciello > --- > drivers/pci/probe.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 2693211d31cf..67ca33188cba 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1610,6 +1610,7 @@ static void pci_set_removable(struct pci_dev *dev) >* exposed as "removable" to userspace. >*/ > if (vsec || > + dev->class == PCI_CLASS_SERIAL_USB_USB4 || > pci_acpi_is_usb4(dev) || > (parent && > (parent->external_facing || dev_is_removable(>dev > -- > 2.34.1 >
Re: [PATCH v3 05/12] PCI: Detect root port of internal USB4 devices by `usb4-host-interface`
On Fri, Feb 11, 2022 at 01:32:43PM -0600, Mario Limonciello wrote:
> The root port used for PCIe tunneling should be marked as removable to
> ensure that the entire chain is marked removable.
>
> This can be done by looking for the device property specified in
> the ACPI tables `usb4-host-interface`.
>
> Suggested-by: Mika Westerberg
> Link:
> https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers
> Signed-off-by: Mario Limonciello
> ---
> drivers/pci/pci-acpi.c | 10 ++
> drivers/pci/pci.h | 5 +
> drivers/pci/probe.c| 1 +
> 3 files changed, 16 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a42dbf448860..6368e5633b1b 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1391,6 +1391,16 @@ void pci_acpi_cleanup(struct device *dev, struct
> acpi_device *adev)
> }
> }
>
> +bool pci_acpi_is_usb4(struct pci_dev *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(>dev);
> +
> + if (!adev)
> + return false;
> + return fwnode_property_present(acpi_fwnode_handle(adev),
> +"usb4-host-interface");
Maybe it's obvious to everybody but me that "USB4" means this device
is removable. The Microsoft reference above doesn't say anything
about removability.
My expectation is that "USB" (like "PCI" and "PCIe") tells me
something about how a device is electrically connected and how
software can operate it. It doesn't really tell me anything about
whether those electrical connections are permanent, made through an
internal slot, or made through an external connector and cable.
> +}
> +
> static struct fwnode_handle *(*pci_msi_get_fwnode_cb)(struct device *dev);
>
> /**
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3d60cabde1a1..359607c0542d 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -695,6 +695,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev);
> int acpi_pci_wakeup(struct pci_dev *dev, bool enable);
> bool acpi_pci_need_resume(struct pci_dev *dev);
> pci_power_t acpi_pci_choose_state(struct pci_dev *pdev);
> +bool pci_acpi_is_usb4(struct pci_dev *dev);
> #else
> static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)
> {
> @@ -734,6 +735,10 @@ static inline pci_power_t acpi_pci_choose_state(struct
> pci_dev *pdev)
> {
> return PCI_POWER_ERROR;
> }
> +static inline bool pci_acpi_is_usb4(struct pci_dev *dev)
> +{
> + return false;
> +}
> #endif
>
> #ifdef CONFIG_PCIEASPM
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index e41656cdd8f0..2693211d31cf 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1610,6 +1610,7 @@ static void pci_set_removable(struct pci_dev *dev)
>* exposed as "removable" to userspace.
>*/
> if (vsec ||
> + pci_acpi_is_usb4(dev) ||
> (parent &&
> (parent->external_facing || dev_is_removable(>dev
> dev_set_removable(>dev, DEVICE_REMOVABLE);
> --
> 2.34.1
>
Re: [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk
On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote:
> `pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt
> controller to indicate that D3 is possible. As this is used solely
> for older Apple systems, move it into a quirk that enumerates across
> all Intel TBT controllers.
>
> Suggested-by: Mika Westerberg
> Signed-off-by: Mario Limonciello
> ---
> drivers/pci/pci.c| 12 +-
> drivers/pci/quirks.c | 53
> 2 files changed, 60 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f..5002e214c9a6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1064,7 +1064,13 @@ static inline bool platform_pci_bridge_d3(struct
> pci_dev *dev)
> if (pci_use_mid_pm())
> return false;
>
> - return acpi_pci_bridge_d3(dev);
> + if (acpi_pci_bridge_d3(dev))
> + return true;
> +
> + if (device_property_read_bool(>dev, "HotPlugSupportInD3"))
> + return true;
Why do we need this? acpi_pci_bridge_d3() already looks for
"HotPlugSupportInD3".
> + return false;
> }
>
> /**
> @@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> if (pci_bridge_d3_force)
> return true;
>
> - /* Even the oldest 2010 Thunderbolt controller supports D3. */
> - if (bridge->is_thunderbolt)
> - return true;
> -
> /* Platform might know better if the bridge supports D3 */
> if (platform_pci_bridge_d3(bridge))
> return true;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 6d3c88edde00..aaf098ca7d54 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
> quirk_apple_poweroff_thunderbolt);
> #endif
>
> +/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but
> don't specify
> + * it in the ACPI tables
Wrap to fit in 80 columns like the rest of the file. Also use the:
/*
* comment ...
*/
style if it's more than one line.
I don't think "as old as 2010" is helpful here -- I assume 2010 is
there because there *were* no Thunderbolt controllers before 2010, but
the code doesn't check any dates, so we basically assume all Apple
machines of any age with the listed controllers can do this.
> + */
> +static void quirk_apple_d3_thunderbolt(struct pci_dev *dev)
> +{
> + struct property_entry properties[] = {
> + PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),
> + {},
> + };
> +
> + if (!x86_apple_machine)
> + return;
The current code doesn't check x86_apple_machine, so this needs some
justification. How do I know this works the same as before?
> +
> + if (device_create_managed_software_node(>dev, properties, NULL))
> + pci_warn(dev, "could not add HotPlugSupportInD3 property");
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
> + quirk_apple_d3_thunderbolt);
The current code assumes *all* Thunderbolt controllers support D3, so
it would assume a controller released next year would support D3, but
this code would assume the opposite. Are we supposed to add
everything to this list, or do newer machines supply
HotPlugSupportInD3, or ...?
How did you derive this list? (Question for the commit log and/or
comments here.)
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EAGLE_RIDGE,
> + quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_PEAK,
> + quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
> + quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C,
> + quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
> + quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_NHI,
> + quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_BRIDGE,
> + quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_NHI,
> + quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_BRIDGE,
> + quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI,
> + quirk_apple_d3_thunderbolt);
Re: [PATCH v3 02/12] PCI: Move `is_thunderbolt` check for lack of command completed to a quirk
Update subject to something like:
PCI: pciehp: Quirk broken Command Completed support on Intel Thunderbolt
controllers
On Fri, Feb 11, 2022 at 01:32:40PM -0600, Mario Limonciello wrote:
> The `is_thunderbolt` check is currently used to indicate the lack of
> command completed support for a number of older Thunderbolt devices.
>
> This however is heavy handed and should have been done via a quirk. Move
> the affected devices outlined in commit 493fb50e958c ("PCI: pciehp: Assume
> NoCompl+ for Thunderbolt ports") into pci quirks.
>
> Suggested-by: Lukas Wunner
> Signed-off-by: Mario Limonciello
> ---
> drivers/pci/hotplug/pciehp_hpc.c | 6 +-
> drivers/pci/quirks.c | 17 +
> include/linux/pci.h | 2 ++
> 3 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> b/drivers/pci/hotplug/pciehp_hpc.c
> index 1c1ebf3dad43..e4c42b24aba8 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -996,11 +996,7 @@ struct controller *pcie_init(struct pcie_device *dev)
> if (pdev->hotplug_user_indicators)
> slot_cap &= ~(PCI_EXP_SLTCAP_AIP | PCI_EXP_SLTCAP_PIP);
>
> - /*
> - * We assume no Thunderbolt controllers support Command Complete events,
> - * but some controllers falsely claim they do.
> - */
> - if (pdev->is_thunderbolt)
> + if (pdev->no_cmd_complete)
> slot_cap |= PCI_EXP_SLTCAP_NCCS;
>
> ctrl->slot_cap = slot_cap;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index d2dd6a6cda60..6d3c88edde00 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3675,6 +3675,23 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
> quirk_thunderbolt_hotplug_msi);
Please add a comment above to the effect that PCI_EXP_SLTCAP_NCCS
being clear means the controller generates a Command Completed software
notification when it completes a command, and these controllers don't
generate those notifications even though PCI_EXP_SLTCAP_NCCS is clear
(PCIe r6.0, sec 7.5.3.9).
> +static void quirk_thunderbolt_command_completed(struct pci_dev *pdev)
Rename to quirk_no_command_completed(). This doesn't have anything to
do with Thunderbolt; it's just that the affected devices happen to be
Thunderbolt controllers.
> +{
> + pdev->no_cmd_complete = 1;
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
> + quirk_thunderbolt_command_completed);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EAGLE_RIDGE,
> + quirk_thunderbolt_command_completed);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_PEAK,
> + quirk_thunderbolt_command_completed);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
> + quirk_thunderbolt_command_completed);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C,
> + quirk_thunderbolt_command_completed);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
> + quirk_thunderbolt_command_completed);
Can we put these in drivers/pci/hotplug/pciehp_hpc.c? We already
have a few similar quirks there.
> #ifdef CONFIG_ACPI
> /*
> * Apple: Shutdown Cactus Ridge Thunderbolt controller.
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8253a5413d7c..1e5b769e42fc 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -443,6 +443,8 @@ struct pci_dev {
> unsigned intis_hotplug_bridge:1;
> unsigned intshpc_managed:1; /* SHPC owned by shpchp */
> unsigned intis_thunderbolt:1; /* Thunderbolt controller */
> + unsigned intno_cmd_complete:1; /* Lies about command completed
> events */
> +
> /*
>* Devices marked being untrusted are the ones that can potentially
>* execute DMA attacks and similar. They are typically connected
> --
> 2.34.1
>
Re: [PATCH v3 01/12] thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4
On Fri, Feb 11, 2022 at 01:32:39PM -0600, Mario Limonciello wrote: > This PCI class definition of the USB4 device is currently located only in > the thunderbolt driver. > > It will be needed by a few other drivers for upcoming changes. Move it into > the common include file. > > Acked-by: Alex Deucher > Acked-by: Mika Westerberg > Signed-off-by: Mario Limonciello I would change the subject to: PCI: Add USB4 class definition because this seems like more of a PCI thing than a Thunderbolt thing, but either way: Acked-by: Bjorn Helgaas > --- > drivers/thunderbolt/nhi.h | 2 -- > include/linux/pci_ids.h | 1 + > 2 files changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h > index 69083aab2736..79e980b51f94 100644 > --- a/drivers/thunderbolt/nhi.h > +++ b/drivers/thunderbolt/nhi.h > @@ -81,6 +81,4 @@ extern const struct tb_nhi_ops icl_nhi_ops; > #define PCI_DEVICE_ID_INTEL_TGL_H_NHI0 0x9a1f > #define PCI_DEVICE_ID_INTEL_TGL_H_NHI1 0x9a21 > > -#define PCI_CLASS_SERIAL_USB_USB40x0c0340 > - > #endif > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index aad54c666407..61b161d914f0 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -116,6 +116,7 @@ > #define PCI_CLASS_SERIAL_USB_OHCI0x0c0310 > #define PCI_CLASS_SERIAL_USB_EHCI0x0c0320 > #define PCI_CLASS_SERIAL_USB_XHCI0x0c0330 > +#define PCI_CLASS_SERIAL_USB_USB40x0c0340 > #define PCI_CLASS_SERIAL_USB_DEVICE 0x0c03fe > #define PCI_CLASS_SERIAL_FIBER 0x0c04 > #define PCI_CLASS_SERIAL_SMBUS 0x0c05 > -- > 2.34.1 >
Re: [PATCH] drm/amdgpu: Fix htmldoc warning
On Fri, Feb 11, 2022 at 3:55 PM Andrey Grodzovsky wrote: > > Update function name. > > Signed-off-by: Andrey Grodzovsky > Reported-by: kernel test robot Reviewed-by: Alex Deucher > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 54f8e1fa4cae..d78141e2c509 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -4978,7 +4978,7 @@ static void amdgpu_device_recheck_guilty_jobs( > } > > /** > - * amdgpu_device_gpu_recover - reset the asic and recover scheduler > + * amdgpu_device_gpu_recover_imp - reset the asic and recover scheduler > * > * @adev: amdgpu_device pointer > * @job: which job trigger hang > -- > 2.25.1 >
[PATCH] drm/amdgpu: Fix htmldoc warning
Update function name. Signed-off-by: Andrey Grodzovsky Reported-by: kernel test robot --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 54f8e1fa4cae..d78141e2c509 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -4978,7 +4978,7 @@ static void amdgpu_device_recheck_guilty_jobs( } /** - * amdgpu_device_gpu_recover - reset the asic and recover scheduler + * amdgpu_device_gpu_recover_imp - reset the asic and recover scheduler * * @adev: amdgpu_device pointer * @job: which job trigger hang -- 2.25.1
[PATCH 9/9] drm/panfrost: Handle arm,mali-valhall compatible
From: Alyssa Rosenzweig
The most important Valhall-specific quirks have been handled, so add the
Valhall compatible and probe.
Signed-off-by: Alyssa Rosenzweig
---
drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 96bb5a465627..12977454af75 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -663,6 +663,7 @@ static const struct of_device_id dt_match[] = {
{ .compatible = "arm,mali-t860", .data = _data, },
{ .compatible = "arm,mali-t880", .data = _data, },
{ .compatible = "arm,mali-bifrost", .data = _data, },
+ { .compatible = "arm,mali-valhall", .data = _data, },
{ .compatible = "mediatek,mt8183-mali", .data = _mt8183_data },
{}
};
--
2.34.1
[PATCH 8/9] drm/panfrost: Add Mali-G57 "Natt" support
From: Alyssa Rosenzweig
Add the features, issues, and GPU ID for Mali-G57, a first-generation
Valhall GPU. Other first- and second-generation Valhall GPUs should be
similar.
Signed-off-by: Alyssa Rosenzweig
---
drivers/gpu/drm/panfrost/panfrost_features.h | 12
drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 ++
drivers/gpu/drm/panfrost/panfrost_issues.h | 5 +
3 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h
b/drivers/gpu/drm/panfrost/panfrost_features.h
index 1a8bdebc86a3..7ed0cd3ea2d4 100644
--- a/drivers/gpu/drm/panfrost/panfrost_features.h
+++ b/drivers/gpu/drm/panfrost/panfrost_features.h
@@ -106,6 +106,18 @@ enum panfrost_hw_feature {
BIT_ULL(HW_FEATURE_TLS_HASHING) | \
BIT_ULL(HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG))
+#define hw_features_g57 (\
+ BIT_ULL(HW_FEATURE_JOBCHAIN_DISAMBIGUATION) | \
+ BIT_ULL(HW_FEATURE_PWRON_DURING_PWROFF_TRANS) | \
+ BIT_ULL(HW_FEATURE_XAFFINITY) | \
+ BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
+ BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
+ BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
+ BIT_ULL(HW_FEATURE_COHERENCY_REG) | \
+ BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
+ BIT_ULL(HW_FEATURE_IDVS_GROUP_SIZE) | \
+ BIT_ULL(HW_FEATURE_CLEAN_ONLY_SAFE))
+
static inline bool panfrost_has_hw_feature(struct panfrost_device *pfdev,
enum panfrost_hw_feature feat)
{
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c
b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 73e5774f01c1..08d657527099 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -201,6 +201,8 @@ static const struct panfrost_model gpu_models[] = {
GPU_MODEL(g52, 0x7002),
GPU_MODEL(g31, 0x7003,
GPU_REV(g31, 1, 0)),
+
+ GPU_MODEL(g57, 0x9001),
};
static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h
b/drivers/gpu/drm/panfrost/panfrost_issues.h
index b8865fc9efce..1a0dc7f7f857 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -258,6 +258,11 @@ enum panfrost_hw_issue {
#define hw_issues_g76 0
+#define hw_issues_g57 (\
+ BIT_ULL(HW_ISSUE_TTRX_2968_TTRX_3162) | \
+ BIT_ULL(HW_ISSUE_TTRX_3076) | \
+ BIT_ULL(HW_ISSUE_TTRX_3485))
+
static inline bool panfrost_has_hw_issue(const struct panfrost_device *pfdev,
enum panfrost_hw_issue issue)
{
--
2.34.1
[PATCH 6/9] drm/panfrost: Add "clean only safe" feature bit
From: Alyssa Rosenzweig
Add the HW_FEATURE_CLEAN_ONLY_SAFE bit based on kbase. When I actually
tried to port the logic from kbase, trivial jobs raised Data Invalid
Faults, so this may depend on other coherency details. It's still useful
to have the bit to record the feature bit when adding new models.
Signed-off-by: Alyssa Rosenzweig
---
drivers/gpu/drm/panfrost/panfrost_features.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h
b/drivers/gpu/drm/panfrost/panfrost_features.h
index 36fadcf9634e..1a8bdebc86a3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_features.h
+++ b/drivers/gpu/drm/panfrost/panfrost_features.h
@@ -21,6 +21,7 @@ enum panfrost_hw_feature {
HW_FEATURE_TLS_HASHING,
HW_FEATURE_THREAD_GROUP_SPLIT,
HW_FEATURE_IDVS_GROUP_SIZE,
+ HW_FEATURE_CLEAN_ONLY_SAFE,
HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG,
};
--
2.34.1
[PATCH 7/9] drm/panfrost: Don't set L2_MMU_CONFIG quirks
From: Alyssa Rosenzweig L2_MMU_CONFIG is an implementation-defined register. Different Mali GPUs define slightly different MAX_READS and MAX_WRITES fields, which throttle outstanding reads and writes when set to non-zero values. When left as zero, reads and writes are not throttled. Both kbase and panfrost always zero these registers. Per discussion with Steven Price, there are two reasons these quirks may be used: 1. Simulating slower memory subsystems. This use case is only of interest to system-on-chip designers; it is not relevant to mainline. 2. Working around broken memory subsystems. Hopefully we never see this case in mainline. If we do, we'll need to set this register based on an SoC-compatible, rather than generally matching on the GPU model. To the best of our knowledge, these fields are zero at reset, so the write is not necessary. Let's remove the write to aid porting to new Mali GPUs, which have different layouts for the L2_MMU_CONFIG register. Signed-off-by: Alyssa Rosenzweig Suggested-by: Steven Price --- drivers/gpu/drm/panfrost/panfrost_gpu.c | 12 1 file changed, 12 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c index 1c1e2017aa80..73e5774f01c1 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c @@ -127,18 +127,6 @@ static void panfrost_gpu_init_quirks(struct panfrost_device *pfdev) gpu_write(pfdev, GPU_TILER_CONFIG, quirks); - quirks = gpu_read(pfdev, GPU_L2_MMU_CONFIG); - - /* Limit read & write ID width for AXI */ - if (panfrost_has_hw_feature(pfdev, HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG)) - quirks &= ~(L2_MMU_CONFIG_3BIT_LIMIT_EXTERNAL_READS | - L2_MMU_CONFIG_3BIT_LIMIT_EXTERNAL_WRITES); - else - quirks &= ~(L2_MMU_CONFIG_LIMIT_EXTERNAL_READS | - L2_MMU_CONFIG_LIMIT_EXTERNAL_WRITES); - - gpu_write(pfdev, GPU_L2_MMU_CONFIG, quirks); - quirks = 0; if ((panfrost_model_eq(pfdev, 0x860) || panfrost_model_eq(pfdev, 0x880)) && pfdev->features.revision >= 0x2000) -- 2.34.1
[PATCH 2/9] drm/panfrost: Handle HW_ISSUE_TTRX_2968_TTRX_3162
From: Alyssa Rosenzweig
Add handling for the HW_ISSUE_TTRX_2968_TTRX_3162 quirk. Logic ported
from kbase. kbase lists this workaround as used on Mali-G57.
Signed-off-by: Alyssa Rosenzweig
---
drivers/gpu/drm/panfrost/panfrost_gpu.c| 3 +++
drivers/gpu/drm/panfrost/panfrost_issues.h | 3 +++
drivers/gpu/drm/panfrost/panfrost_regs.h | 1 +
3 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c
b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 50c8922694d7..1c1e2017aa80 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -108,6 +108,9 @@ static void panfrost_gpu_init_quirks(struct panfrost_device
*pfdev)
quirks |= SC_LS_ALLOW_ATTR_TYPES;
}
+ if (panfrost_has_hw_issue(pfdev, HW_ISSUE_TTRX_2968_TTRX_3162))
+ quirks |= SC_VAR_ALGORITHM;
+
if (panfrost_has_hw_feature(pfdev, HW_FEATURE_TLS_HASHING))
quirks |= SC_TLS_HASH_ENABLE;
diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h
b/drivers/gpu/drm/panfrost/panfrost_issues.h
index 8e59d765bf19..3af7d723377e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -125,6 +125,9 @@ enum panfrost_hw_issue {
* kernel must fiddle with L2 caches to prevent data leakage */
HW_ISSUE_TGOX_R1_1234,
+ /* Must set SC_VAR_ALGORITHM */
+ HW_ISSUE_TTRX_2968_TTRX_3162,
+
HW_ISSUE_END
};
diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h
b/drivers/gpu/drm/panfrost/panfrost_regs.h
index 16e776cc82ea..fa1e1af56e17 100644
--- a/drivers/gpu/drm/panfrost/panfrost_regs.h
+++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
@@ -195,6 +195,7 @@
#define SC_TLS_HASH_ENABLE BIT(17)
#define SC_LS_ATTR_CHECK_DISABLE BIT(18)
#define SC_ENABLE_TEXGRD_FLAGS BIT(25)
+#define SC_VAR_ALGORITHM BIT(29)
/* End SHADER_CONFIG register */
/* TILER_CONFIG register */
--
2.34.1
[PATCH 5/9] drm/panfrost: Add HW_ISSUE_TTRX_3485 quirk
From: Alyssa Rosenzweig
TTRX_3485 requires the infamous "dummy job" workaround. I have this
workaround implemented in a local branch, but I have not yet hit a case
that requires it so I cannot test whether the implementation is correct.
In the mean time, add the quirk bit so we can document which platforms
may need it in the future.
Signed-off-by: Alyssa Rosenzweig
---
drivers/gpu/drm/panfrost/panfrost_issues.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h
b/drivers/gpu/drm/panfrost/panfrost_issues.h
index 058f6a4c8435..b8865fc9efce 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -132,6 +132,9 @@ enum panfrost_hw_issue {
* to hang */
HW_ISSUE_TTRX_3076,
+ /* Must issue a dummy job before starting real work to prevent hangs */
+ HW_ISSUE_TTRX_3485,
+
HW_ISSUE_END
};
--
2.34.1
[PATCH 4/9] drm/panfrost: Handle HW_ISSUE_TTRX_3076
From: Alyssa Rosenzweig
Some Valhall GPUs require resets when encountering bus faults due to
occlusion query writes. Add the issue bit for this and handle it.
Signed-off-by: Alyssa Rosenzweig
---
drivers/gpu/drm/panfrost/panfrost_device.c | 9 +++--
drivers/gpu/drm/panfrost/panfrost_issues.h | 4
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c
b/drivers/gpu/drm/panfrost/panfrost_device.c
index 7f51a4682ccb..ee612303f076 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -11,6 +11,7 @@
#include "panfrost_device.h"
#include "panfrost_devfreq.h"
#include "panfrost_features.h"
+#include "panfrost_issues.h"
#include "panfrost_gpu.h"
#include "panfrost_job.h"
#include "panfrost_mmu.h"
@@ -380,9 +381,13 @@ const char *panfrost_exception_name(u32 exception_code)
bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
u32 exception_code)
{
- /* Right now, none of the GPU we support need a reset, but this
-* might change.
+ /* If an occlusion query write causes a bus fault on affected GPUs,
+* future fragment jobs may hang. Reset to workaround.
*/
+ if (exception_code == DRM_PANFROST_EXCEPTION_JOB_BUS_FAULT)
+ return panfrost_has_hw_issue(pfdev, HW_ISSUE_TTRX_3076);
+
+ /* No other GPUs we support need a reset */
return false;
}
diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h
b/drivers/gpu/drm/panfrost/panfrost_issues.h
index a66692663833..058f6a4c8435 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -128,6 +128,10 @@ enum panfrost_hw_issue {
/* Must set SC_VAR_ALGORITHM */
HW_ISSUE_TTRX_2968_TTRX_3162,
+ /* Bus fault from occlusion query write may cause future fragment jobs
+* to hang */
+ HW_ISSUE_TTRX_3076,
+
HW_ISSUE_END
};
--
2.34.1
[PATCH 3/9] drm/panfrost: Constify argument to has_hw_issue
From: Alyssa Rosenzweig
Logically, this function is free of side effects, so any pointers it
takes should be const. Needed to avoid a warning in the next patch.
Signed-off-by: Alyssa Rosenzweig
---
drivers/gpu/drm/panfrost/panfrost_issues.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h
b/drivers/gpu/drm/panfrost/panfrost_issues.h
index 3af7d723377e..a66692663833 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -251,7 +251,7 @@ enum panfrost_hw_issue {
#define hw_issues_g76 0
-static inline bool panfrost_has_hw_issue(struct panfrost_device *pfdev,
+static inline bool panfrost_has_hw_issue(const struct panfrost_device *pfdev,
enum panfrost_hw_issue issue)
{
return test_bit(issue, pfdev->features.hw_issues);
--
2.34.1
[PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible
From: Alyssa Rosenzweig >From the kernel's perspective, pre-CSF Valhall is more or less compatible with Bifrost, although they differ to userspace. Add a compatible for Valhall to the existing Bifrost bindings documentation. Signed-off-by: Alyssa Rosenzweig Cc: devicet...@vger.kernel.org --- Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 63a08f3f321d..48aeabd2ed68 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -23,6 +23,7 @@ properties: - rockchip,px30-mali - rockchip,rk3568-mali - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable + - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable reg: maxItems: 1 -- 2.34.1
[PATCH 0/9] drm/panfrost: Initial Valhall support
From: Alyssa Rosenzweig This patch series adds preliminary support for Mali "Valhall" GPUs into the Panfrost kernel driver. The series has been tested on the Mali-G57 on a MediaTek MT8192 system. However, that system requires additional MediaTek-specific patches [1] as well as core mainlining for MediaTek. I'll post the MT8192-specific Panfrost patches soon; they depend on this core series. On the userspace side, pre-CSF Valhall (what is supported here) uses an identical UABI as Bifrost. Mesa support for Valhall is being worked on in parallel [2]. I'm hoping basic support for Valhall will be available in Mesa 22.1. [1] https://gitlab.freedesktop.org/panfrost/linux/-/tree/mt8192-branch [2] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14795 Alyssa Rosenzweig (9): dt-bindings: Add arm,mali-valhall compatible drm/panfrost: Handle HW_ISSUE_TTRX_2968_TTRX_3162 drm/panfrost: Constify argument to has_hw_issue drm/panfrost: Handle HW_ISSUE_TTRX_3076 drm/panfrost: Add HW_ISSUE_TTRX_3485 quirk drm/panfrost: Add "clean only safe" feature bit drm/panfrost: Don't set L2_MMU_CONFIG quirks drm/panfrost: Add Mali-G57 "Natt" support drm/panfrost: Handle arm,mali-valhall compatible .../bindings/gpu/arm,mali-bifrost.yaml | 1 + drivers/gpu/drm/panfrost/panfrost_device.c | 9 +++-- drivers/gpu/drm/panfrost/panfrost_drv.c | 1 + drivers/gpu/drm/panfrost/panfrost_features.h| 13 + drivers/gpu/drm/panfrost/panfrost_gpu.c | 17 + drivers/gpu/drm/panfrost/panfrost_issues.h | 17 - drivers/gpu/drm/panfrost/panfrost_regs.h| 1 + 7 files changed, 44 insertions(+), 15 deletions(-) -- 2.34.1
Re: [Freedreno] [PATCH 1/2] drm/msm/dsi: move DSI host powerup to modeset time
On 2/11/2022 11:37 AM, Dmitry Baryshkov wrote:
On 18/01/2022 23:03, Dmitry Baryshkov wrote:
On Tue, 18 Jan 2022 at 22:29, Abhinav Kumar
wrote:
On 12/7/2021 2:29 PM, Dmitry Baryshkov wrote:
The DSI subsystem does not fully fall into the pre-enable/enable system
of callbacks, since typically DSI device bridge drivers expect to be
able to communicate with DSI devices at the pre-enable() callback. The
reason is that for some DSI hosts enabling the video stream would
prevent other drivers from sending DSI commands. For example see the
panel-bridge driver, which does drm_panel_prepare() from the
pre_enable() callback (which would be called before our pre_enable()
callback, resulting in panel preparation failures as the link is not
yet
ready).
Therewere several attempts to solve this issue, but currently the best
approach is to power up the DSI link from the mode_set() callback,
allowing next bridge/panel to use DSI transfers in the pre_enable()
time. Follow this approach.
Change looks okay. As per the programming guideline, we should set the
VIDEO_MODE_EN register in the DSI controller followed by enabling the
timing engine which will still happen even now because we will do it in
modeset instead of the pre_enable().
But, this can potentially increase the delay between VIDEO_MODE_EN
and TIMING_ENGINE_EN. I dont see anything in the programming guide
against this but since this is a change from the original flow, I would
like to do one test before acking this. Can you please try adding a huge
delay like 200-300ms between VIDEO_MODE_EN and timing engine enable to
make sure there are no issues? You can do that here:
Fine, I'll do the test as the time permits.
I did the tests, the display pipeline works as expected.
Let's get this in, it allows using other DSI-controlled bridges.
Alright, sounds good,
Reviewed-by: Abhinav Kumar
int msm_dsi_host_enable(struct mipi_dsi_host *host)
{
struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
dsi_op_mode_config(msm_host,
!!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO), true);
msleep(300);
}
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 43
+++
1 file changed, 31 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 681ca74fe410..497719efb9e9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -336,13 +336,12 @@ dsi_mgr_connector_best_encoder(struct
drm_connector *connector)
return msm_dsi_get_encoder(msm_dsi);
}
-static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
+static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
{
int id = dsi_mgr_bridge_get_id(bridge);
struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
struct mipi_dsi_host *host = msm_dsi->host;
- struct drm_panel *panel = msm_dsi->panel;
struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX];
bool is_bonded_dsi = IS_BONDED_DSI();
int ret;
@@ -383,6 +382,34 @@ static void dsi_mgr_bridge_pre_enable(struct
drm_bridge *bridge)
if (is_bonded_dsi && msm_dsi1)
msm_dsi_host_enable_irq(msm_dsi1->host);
+ return;
+
+host1_on_fail:
+ msm_dsi_host_power_off(host);
+host_on_fail:
+ dsi_mgr_phy_disable(id);
+phy_en_fail:
+ return;
+}
+
+static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
+{
+ int id = dsi_mgr_bridge_get_id(bridge);
+ struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
+ struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
+ struct mipi_dsi_host *host = msm_dsi->host;
+ struct drm_panel *panel = msm_dsi->panel;
+ bool is_bonded_dsi = IS_BONDED_DSI();
+ int ret;
+
+ DBG("id=%d", id);
+ if (!msm_dsi_device_connected(msm_dsi))
+ return;
+
+ /* Do nothing with the host if it is slave-DSI in case of
bonded DSI */
+ if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
+ return;
+
/* Always call panel functions once, because even for dual
panels,
* there is only one drm_panel instance.
*/
@@ -417,17 +444,7 @@ static void dsi_mgr_bridge_pre_enable(struct
drm_bridge *bridge)
if (panel)
drm_panel_unprepare(panel);
panel_prep_fail:
- msm_dsi_host_disable_irq(host);
- if (is_bonded_dsi && msm_dsi1)
- msm_dsi_host_disable_irq(msm_dsi1->host);
- if (is_bonded_dsi && msm_dsi1)
- msm_dsi_host_power_off(msm_dsi1->host);
-host1_on_fail:
- msm_dsi_host_power_off(host);
-host_on_fail:
- dsi_mgr_phy_disable(id);
-phy_en_fail:
return;
}
@@ -573,6 +590,8 @@ static void dsi_mgr_bridge_mode_set(struct
drm_bridge *bridge,
msm_dsi_host_set_display_mode(host, adjusted_mode);
if (is_bonded_dsi && other_dsi)
Re: [git pull] drm fixes for 5.17-rc4
The pull request you sent on Fri, 11 Feb 2022 13:46:47 +1000: > git://anongit.freedesktop.org/drm/drm tags/drm-fixes-2022-02-11 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/c3ee3a9e4fa6b1d249b5abff2d4c7dab5a47d522 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
Re: [PATCH 1/2] drm/msm/dsi: move DSI host powerup to modeset time
On 18/01/2022 23:03, Dmitry Baryshkov wrote:
On Tue, 18 Jan 2022 at 22:29, Abhinav Kumar wrote:
On 12/7/2021 2:29 PM, Dmitry Baryshkov wrote:
The DSI subsystem does not fully fall into the pre-enable/enable system
of callbacks, since typically DSI device bridge drivers expect to be
able to communicate with DSI devices at the pre-enable() callback. The
reason is that for some DSI hosts enabling the video stream would
prevent other drivers from sending DSI commands. For example see the
panel-bridge driver, which does drm_panel_prepare() from the
pre_enable() callback (which would be called before our pre_enable()
callback, resulting in panel preparation failures as the link is not yet
ready).
Therewere several attempts to solve this issue, but currently the best
approach is to power up the DSI link from the mode_set() callback,
allowing next bridge/panel to use DSI transfers in the pre_enable()
time. Follow this approach.
Change looks okay. As per the programming guideline, we should set the
VIDEO_MODE_EN register in the DSI controller followed by enabling the
timing engine which will still happen even now because we will do it in
modeset instead of the pre_enable().
But, this can potentially increase the delay between VIDEO_MODE_EN
and TIMING_ENGINE_EN. I dont see anything in the programming guide
against this but since this is a change from the original flow, I would
like to do one test before acking this. Can you please try adding a huge
delay like 200-300ms between VIDEO_MODE_EN and timing engine enable to
make sure there are no issues? You can do that here:
Fine, I'll do the test as the time permits.
I did the tests, the display pipeline works as expected.
Let's get this in, it allows using other DSI-controlled bridges.
int msm_dsi_host_enable(struct mipi_dsi_host *host)
{
struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
dsi_op_mode_config(msm_host,
!!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO), true);
msleep(300);
}
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++
1 file changed, 31 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 681ca74fe410..497719efb9e9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -336,13 +336,12 @@ dsi_mgr_connector_best_encoder(struct drm_connector
*connector)
return msm_dsi_get_encoder(msm_dsi);
}
-static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
+static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
{
int id = dsi_mgr_bridge_get_id(bridge);
struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
struct mipi_dsi_host *host = msm_dsi->host;
- struct drm_panel *panel = msm_dsi->panel;
struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX];
bool is_bonded_dsi = IS_BONDED_DSI();
int ret;
@@ -383,6 +382,34 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge
*bridge)
if (is_bonded_dsi && msm_dsi1)
msm_dsi_host_enable_irq(msm_dsi1->host);
+ return;
+
+host1_on_fail:
+ msm_dsi_host_power_off(host);
+host_on_fail:
+ dsi_mgr_phy_disable(id);
+phy_en_fail:
+ return;
+}
+
+static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
+{
+ int id = dsi_mgr_bridge_get_id(bridge);
+ struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
+ struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
+ struct mipi_dsi_host *host = msm_dsi->host;
+ struct drm_panel *panel = msm_dsi->panel;
+ bool is_bonded_dsi = IS_BONDED_DSI();
+ int ret;
+
+ DBG("id=%d", id);
+ if (!msm_dsi_device_connected(msm_dsi))
+ return;
+
+ /* Do nothing with the host if it is slave-DSI in case of bonded DSI */
+ if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
+ return;
+
/* Always call panel functions once, because even for dual panels,
* there is only one drm_panel instance.
*/
@@ -417,17 +444,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge
*bridge)
if (panel)
drm_panel_unprepare(panel);
panel_prep_fail:
- msm_dsi_host_disable_irq(host);
- if (is_bonded_dsi && msm_dsi1)
- msm_dsi_host_disable_irq(msm_dsi1->host);
- if (is_bonded_dsi && msm_dsi1)
- msm_dsi_host_power_off(msm_dsi1->host);
-host1_on_fail:
- msm_dsi_host_power_off(host);
-host_on_fail:
- dsi_mgr_phy_disable(id);
-phy_en_fail:
return;
}
@@ -573,6 +590,8 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge
*bridge,
msm_dsi_host_set_display_mode(host, adjusted_mode);
if (is_bonded_dsi && other_dsi)
msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
+
+ dsi_mgr_bridge_power_on(bridge);
}
static
RE: [PATCH v2 3/9] PCI: drop `is_thunderbolt` attribute
[Public]
> -Original Message-
> From: Mika Westerberg
> Sent: Friday, February 11, 2022 04:24
> To: Limonciello, Mario
> Cc: Bjorn Helgaas ; Andreas Noever
> ; open list:PCI SUBSYSTEM p...@vger.kernel.org>; open list:THUNDERBOLT DRIVER u...@vger.kernel.org>; open list:RADEON and AMDGPU DRM DRIVERS g...@lists.freedesktop.org>; open list:DRM DRIVERS de...@lists.freedesktop.org>; open list:DRM DRIVER FOR NVIDIA
> GEFORCE/QUADRO GPUS ; open list:X86
> PLATFORM DRIVERS ; Michael Jamet
> ; Yehezkel Bernat ;
> Lukas Wunner ; Deucher, Alexander
>
> Subject: Re: [PATCH v2 3/9] PCI: drop `is_thunderbolt` attribute
>
> Hi Mario,
>
> On Thu, Feb 10, 2022 at 04:43:23PM -0600, Mario Limonciello wrote:
> > The `is_thunderbolt` attribute is currently a dumping ground for a
> > variety of things.
> >
> > Instead use the driver core removable attribute to indicate the
> > detail a device is attached to a thunderbolt or USB4 chain.
> >
> > Signed-off-by: Mario Limonciello
> > ---
> > drivers/pci/pci.c | 2 +-
> > drivers/pci/probe.c | 20 +++-
> > drivers/platform/x86/apple-gmux.c | 2 +-
> > include/linux/pci.h | 5 ++---
> > 4 files changed, 11 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 9ecce435fb3f..1264984d5e6d 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2955,7 +2955,7 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> > return true;
> >
> > /* Even the oldest 2010 Thunderbolt controller supports D3. */
> > - if (bridge->is_thunderbolt)
> > + if (dev_is_removable(>dev))
>
> For this, I'm not entirely sure this is what we want. The purpose of
> this check is to enable port power management of Apple systems with
> Intel Thunderbolt controller and therefore checking for "removable" here
> is kind of misleading IMHO.
>
> I wonder if we could instead remove the check completely here and rely
> on the below:
>
> if (platform_pci_bridge_d3(bridge))
> return true;
>
> and that would then look like:
>
> static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> {
> if (pci_use_mid_pm())
> return false;
>
> if (acpi_pci_bridge_d3(dev))
> return true;
>
> if (device_property_read_bool(>dev, "HotPlugSupportInD3"))
> return true;
>
> return false;
> }
>
> and then make a quirk in quirks.c that adds the software node property
> for the Apple systems? Or something along those lines.
>
> @Lukas, what do you think?
I took a stab at doing this for V3, but I'm unsure whether ALL of the TBT
controllers
in pci_ids.h have been used in Apple laptops, so it might be a bit wasteful of
a quirk
list. If there is a known list somewhere that is shorter than that, it may be
possible
to pare down. Lukas, if you can please look closely at patch 3 of v3.
RE: [PATCH v2 4/9] PCI: mark USB4 devices as removable
[Public] > -Original Message- > From: Mika Westerberg > Sent: Friday, February 11, 2022 04:35 > To: Limonciello, Mario > Cc: Bjorn Helgaas ; Andreas Noever > ; open list:PCI SUBSYSTEM p...@vger.kernel.org>; open list:THUNDERBOLT DRIVER u...@vger.kernel.org>; open list:RADEON and AMDGPU DRM DRIVERS g...@lists.freedesktop.org>; open list:DRM DRIVERS de...@lists.freedesktop.org>; open list:DRM DRIVER FOR NVIDIA > GEFORCE/QUADRO GPUS ; open list:X86 > PLATFORM DRIVERS ; Michael Jamet > ; Yehezkel Bernat ; > Lukas Wunner ; Deucher, Alexander > > Subject: Re: [PATCH v2 4/9] PCI: mark USB4 devices as removable > > Hi Mario, > > On Thu, Feb 10, 2022 at 04:43:24PM -0600, Mario Limonciello wrote: > > USB4 class devices are also removable like Intel Thunderbolt devices. > > > > Drivers of downstream devices use this information to declare functional > > differences in how the drivers perform by knowing that they are connected > > to an upstream TBT/USB4 port. > > This may not be covering the integrated controllers. For discrete, yes > but if it is the PCIe root ports that start the PCIe topology (over the > PCIe tunnels) this does not work. > > For integrated we have the "usb4-host-interface" ACPI property that > tells this for each port: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.mi > crosoft.com%2Fen-us%2Fwindows-hardware%2Fdrivers%2Fpci%2Fdsd-for-pcie- > root-ports%23mapping-native-protocols-pcie-displayport-tunneled-through- > usb4-to-usb4-host- > routersdata=04%7C01%7Cmario.limonciello%40amd.com%7C64e5b663f > 97b40f4035a08d9ed4a3162%7C3dd8961fe4884e608e11a82d994e183d%7C0%7 > C0%7C637801725176496963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sd > ata=7BvPgExVP8Upvi25EEbqH9TacFDZ4zpCEKOfoBJWcxs%3Dreserved=0 > > and for discrete there is the PCIe DVSEC that can be used (see the USB4 > spec archive it includes the "USB4 DVSEC Version 1.0.pdf" that has more > information). I would expect AMD controller (assuming it is discrete) > implements this too. > > So I'm proposing that we mark the devices that are below PCIe ports > (root, downstream) that fall in the above categories as "removable". > This is then not dependent on checking the USB4 controller and how it is > setup in a particular system. Thanks for all of the great suggestions! I've incorporated them in v3.
[PATCH v3 06/12] PCI: Explicitly mark USB4 NHI devices as removable
USB4 class devices are also removable like Intel Thunderbolt devices. Drivers of downstream devices use this information to declare functional differences in how the drivers perform by knowing that they are connected to an upstream TBT/USB4 port. Reviewed-by: Macpaul Lin Signed-off-by: Mario Limonciello --- drivers/pci/probe.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 2693211d31cf..67ca33188cba 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1610,6 +1610,7 @@ static void pci_set_removable(struct pci_dev *dev) * exposed as "removable" to userspace. */ if (vsec || + dev->class == PCI_CLASS_SERIAL_USB_USB4 || pci_acpi_is_usb4(dev) || (parent && (parent->external_facing || dev_is_removable(>dev -- 2.34.1
[PATCH v3 09/12] drm/nouveau: drop the use of `pci_is_thunderbolt_attached`
Currently `pci_is_thunderbolt_attached` is used to indicate a device is connected externally. The PCI core now marks such devices as removable and downstream drivers can use this instead. Signed-off-by: Mario Limonciello --- drivers/gpu/drm/nouveau/nouveau_vga.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c index 60cd8c0463df..2c8008cb38e0 100644 --- a/drivers/gpu/drm/nouveau/nouveau_vga.c +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c @@ -97,7 +97,7 @@ nouveau_vga_init(struct nouveau_drm *drm) vga_client_register(pdev, nouveau_vga_set_decode); /* don't register Thunderbolt eGPU with vga_switcheroo */ - if (pci_is_thunderbolt_attached(pdev)) + if (dev_is_removable(>dev)) return; vga_switcheroo_register_client(pdev, _switcheroo_ops, runtime); @@ -120,7 +120,7 @@ nouveau_vga_fini(struct nouveau_drm *drm) vga_client_unregister(pdev); - if (pci_is_thunderbolt_attached(pdev)) + if (dev_is_removable(>dev)) return; vga_switcheroo_unregister_client(pdev); -- 2.34.1
[PATCH v3 12/12] PCI: drop `pci_is_thunderbolt_attached`
Currently `pci_is_thunderbolt_attached` is used to indicate a device
is connected externally.
As all drivers now look at the removable attribute, drop this function.
Signed-off-by: Mario Limonciello
---
include/linux/pci.h | 22 --
1 file changed, 22 deletions(-)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d9719eb14654..089e7e36a0d9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2434,28 +2434,6 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
return bus->self && bus->self->ari_enabled;
}
-/**
- * pci_is_thunderbolt_attached - whether device is on a Thunderbolt daisy chain
- * @pdev: PCI device to check
- *
- * Walk upwards from @pdev and check for each encountered bridge if it's part
- * of a Thunderbolt controller. Reaching the host bridge means @pdev is not
- * Thunderbolt-attached. (But rather soldered to the mainboard usually.)
- */
-static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev)
-{
- struct pci_dev *parent = pdev;
-
- if (dev_is_removable(>dev))
- return true;
-
- while ((parent = pci_upstream_bridge(parent)))
- if (dev_is_removable(>dev))
- return true;
-
- return false;
-}
-
#if defined(CONFIG_PCIEPORTBUS) || defined(CONFIG_EEH)
void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type);
#endif
--
2.34.1
[PATCH v3 10/12] drm/radeon: drop the use of `pci_is_thunderbolt_attached`
Currently `pci_is_thunderbolt_attached` is used to indicate a device is connected externally. The PCI core now marks such devices as removable and downstream drivers can use this instead. Signed-off-by: Mario Limonciello --- drivers/gpu/drm/radeon/radeon_device.c | 4 ++-- drivers/gpu/drm/radeon/radeon_kms.c| 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 4f0fbf667431..5117fce23b3f 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -1439,7 +1439,7 @@ int radeon_device_init(struct radeon_device *rdev, if (rdev->flags & RADEON_IS_PX) runtime = true; - if (!pci_is_thunderbolt_attached(rdev->pdev)) + if (!dev_is_removable(>pdev->dev)) vga_switcheroo_register_client(rdev->pdev, _switcheroo_ops, runtime); if (runtime) @@ -1527,7 +1527,7 @@ void radeon_device_fini(struct radeon_device *rdev) /* evict vram memory */ radeon_bo_evict_vram(rdev); radeon_fini(rdev); - if (!pci_is_thunderbolt_attached(rdev->pdev)) + if (!dev_is_removable(>pdev->dev)) vga_switcheroo_unregister_client(rdev->pdev); if (rdev->flags & RADEON_IS_PX) vga_switcheroo_fini_domain_pm_ops(rdev->dev); diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 11ad210919c8..e01ee7a5cf5d 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -139,7 +139,7 @@ int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags) if ((radeon_runtime_pm != 0) && radeon_has_atpx() && ((flags & RADEON_IS_IGP) == 0) && - !pci_is_thunderbolt_attached(pdev)) + !dev_is_removable(>dev)) flags |= RADEON_IS_PX; /* radeon_device_init should report only fatal error -- 2.34.1
[PATCH v3 11/12] platform/x86: amd-gmux: drop the use of `pci_is_thunderbolt_attached`
Currently `pci_is_thunderbolt_attached` is used to indicate a device
is connected externally.
The PCI core now marks such devices as removable and downstream drivers
can use this instead.
Acked-by: Hans de Goede
Signed-off-by: Mario Limonciello
---
drivers/platform/x86/apple-gmux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/x86/apple-gmux.c
b/drivers/platform/x86/apple-gmux.c
index 04232fbc7d56..ffac15b9befd 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -596,7 +596,7 @@ static int gmux_resume(struct device *dev)
static int is_thunderbolt(struct device *dev, void *data)
{
- return pci_is_thunderbolt_attached(to_pci_dev(dev));
+ return dev_is_removable(dev);
}
static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
--
2.34.1
[PATCH v3 07/12] PCI: Set ports for discrete USB4 controllers appropriately
Discrete USB4 controllers won't have ACPI nodes specifying which
PCIe or XHCI port they are linked with.
In order to set the removable attribute appropriately, use the
USB4 DVSEC extended capabability set on these root ports to determine
if they are located on a discrete USB4 controller.
Suggested-by: Mika Westerberg
Link: https://usb.org/sites/default/files/USB4%20Specification%202026.zip
Signed-off-by: Mario Limonciello
---
drivers/pci/probe.c | 33 +
include/linux/pci_ids.h | 2 ++
2 files changed, 35 insertions(+)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 67ca33188cba..1ed3e24db11e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -25,6 +25,8 @@
#define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
#define CARDBUS_RESERVE_BUSNR 3
+#define PCI_DVSEC_ID_USB4 0x23
+
static struct resource busn_resource = {
.name = "PCI busn",
.start = 0,
@@ -1590,6 +1592,36 @@ static void set_pcie_untrusted(struct pci_dev *dev)
dev->untrusted = true;
}
+static bool pci_is_discrete_usb4(struct pci_dev *dev)
+{
+ int dvsec_val = 0, pos;
+ u32 hdr;
+
+ /* USB4 spec says vendors can use either */
+ pos = pci_find_dvsec_capability(dev,
+ PCI_VENDOR_ID_INTEL,
+ PCI_DVSEC_ID_USB4);
+ if (pos) {
+ dvsec_val = 0x06;
+ } else {
+ pos = pci_find_dvsec_capability(dev,
+ PCI_VENDOR_ID_USB_IF,
+ PCI_DVSEC_ID_USB4);
+ if (pos)
+ dvsec_val = 0x01;
+ }
+ if (!dvsec_val)
+ return false;
+
+ pci_read_config_dword(dev, pos + PCI_DVSEC_HEADER2, );
+ if ((hdr & GENMASK(15, 0)) != dvsec_val)
+ return false;
+ /* this port is used for either NHI/PCIe tunnel/USB tunnel */
+ if (hdr & GENMASK(18, 16))
+ return true;
+ return false;
+}
+
static void pci_set_removable(struct pci_dev *dev)
{
struct pci_dev *parent = pci_upstream_bridge(dev);
@@ -1612,6 +1644,7 @@ static void pci_set_removable(struct pci_dev *dev)
if (vsec ||
dev->class == PCI_CLASS_SERIAL_USB_USB4 ||
pci_acpi_is_usb4(dev) ||
+ pci_is_discrete_usb4(dev) ||
(parent &&
(parent->external_facing || dev_is_removable(>dev
dev_set_removable(>dev, DEVICE_REMOVABLE);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 61b161d914f0..271326e058b9 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -3097,4 +3097,6 @@
#define PCI_VENDOR_ID_NCUBE0x10ff
+#define PCI_VENDOR_ID_USB_IF 0x1EC0
+
#endif /* _LINUX_PCI_IDS_H */
--
2.34.1
[PATCH v3 04/12] PCI: Drop the `is_thunderbolt` attribute from PCI core
The `is_thunderbolt` attribute is currently a dumping ground for a
variety of things.
Instead use the driver core removable attribute to indicate the
detail a device is attached to a thunderbolt or USB4 chain.
Signed-off-by: Mario Limonciello
---
drivers/pci/probe.c | 20 +++-
drivers/platform/x86/apple-gmux.c | 2 +-
include/linux/pci.h | 5 ++---
3 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..e41656cdd8f0 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1577,16 +1577,6 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev)
pdev->is_hotplug_bridge = 1;
}
-static void set_pcie_thunderbolt(struct pci_dev *dev)
-{
- u16 vsec;
-
- /* Is the device part of a Thunderbolt controller? */
- vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL,
PCI_VSEC_ID_INTEL_TBT);
- if (vsec)
- dev->is_thunderbolt = 1;
-}
-
static void set_pcie_untrusted(struct pci_dev *dev)
{
struct pci_dev *parent;
@@ -1603,6 +1593,10 @@ static void set_pcie_untrusted(struct pci_dev *dev)
static void pci_set_removable(struct pci_dev *dev)
{
struct pci_dev *parent = pci_upstream_bridge(dev);
+ u16 vsec;
+
+ /* Is the device a Thunderbolt controller? */
+ vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL,
PCI_VSEC_ID_INTEL_TBT);
/*
* We (only) consider everything downstream from an external_facing
@@ -1615,8 +1609,9 @@ static void pci_set_removable(struct pci_dev *dev)
* accessible to user / may not be removed by end user, and thus not
* exposed as "removable" to userspace.
*/
- if (parent &&
- (parent->external_facing || dev_is_removable(>dev)))
+ if (vsec ||
+ (parent &&
+ (parent->external_facing || dev_is_removable(>dev
dev_set_removable(>dev, DEVICE_REMOVABLE);
}
@@ -1860,7 +1855,6 @@ int pci_setup_device(struct pci_dev *dev)
dev->cfg_size = pci_cfg_space_size(dev);
/* Need to have dev->cfg_size ready */
- set_pcie_thunderbolt(dev);
set_pcie_untrusted(dev);
diff --git a/drivers/platform/x86/apple-gmux.c
b/drivers/platform/x86/apple-gmux.c
index 57553f9b4d1d..04232fbc7d56 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -596,7 +596,7 @@ static int gmux_resume(struct device *dev)
static int is_thunderbolt(struct device *dev, void *data)
{
- return to_pci_dev(dev)->is_thunderbolt;
+ return pci_is_thunderbolt_attached(to_pci_dev(dev));
}
static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1e5b769e42fc..d9719eb14654 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -442,7 +442,6 @@ struct pci_dev {
unsigned intis_virtfn:1;
unsigned intis_hotplug_bridge:1;
unsigned intshpc_managed:1; /* SHPC owned by shpchp */
- unsigned intis_thunderbolt:1; /* Thunderbolt controller */
unsigned intno_cmd_complete:1; /* Lies about command completed
events */
/*
@@ -2447,11 +2446,11 @@ static inline bool pci_is_thunderbolt_attached(struct
pci_dev *pdev)
{
struct pci_dev *parent = pdev;
- if (pdev->is_thunderbolt)
+ if (dev_is_removable(>dev))
return true;
while ((parent = pci_upstream_bridge(parent)))
- if (parent->is_thunderbolt)
+ if (dev_is_removable(>dev))
return true;
return false;
--
2.34.1
[PATCH v3 08/12] drm/amd: drop the use of `pci_is_thunderbolt_attached`
Currently `pci_is_thunderbolt_attached` is used to indicate a device is connected externally. The PCI core now marks such devices as removable and downstream drivers can use this instead. Reviewed-by: Macpaul Lin Signed-off-by: Mario Limonciello --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index 1ebb91db2274..6dbf5753b5be 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -161,7 +161,7 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags) (amdgpu_is_atpx_hybrid() || amdgpu_has_atpx_dgpu_power_cntl()) && ((flags & AMD_IS_APU) == 0) && - !pci_is_thunderbolt_attached(to_pci_dev(dev->dev))) + !dev_is_removable(>pdev->dev)) flags |= AMD_IS_PX; parent = pci_upstream_bridge(adev->pdev); diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c index ee7cab37dfd5..2c5d74d836f0 100644 --- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c @@ -382,7 +382,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev, data |= NAVI10_PCIE__LC_L0S_INACTIVITY_DEFAULT << PCIE_LC_CNTL__LC_L0S_INACTIVITY__SHIFT; - if (pci_is_thunderbolt_attached(adev->pdev)) + if (dev_is_removable(>pdev->dev)) data |= NAVI10_PCIE__LC_L1_INACTIVITY_TBT_DEFAULT << PCIE_LC_CNTL__LC_L1_INACTIVITY__SHIFT; else data |= NAVI10_PCIE__LC_L1_INACTIVITY_DEFAULT << PCIE_LC_CNTL__LC_L1_INACTIVITY__SHIFT; -- 2.34.1
[PATCH v3 05/12] PCI: Detect root port of internal USB4 devices by `usb4-host-interface`
The root port used for PCIe tunneling should be marked as removable to
ensure that the entire chain is marked removable.
This can be done by looking for the device property specified in
the ACPI tables `usb4-host-interface`.
Suggested-by: Mika Westerberg
Link:
https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers
Signed-off-by: Mario Limonciello
---
drivers/pci/pci-acpi.c | 10 ++
drivers/pci/pci.h | 5 +
drivers/pci/probe.c| 1 +
3 files changed, 16 insertions(+)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a42dbf448860..6368e5633b1b 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1391,6 +1391,16 @@ void pci_acpi_cleanup(struct device *dev, struct
acpi_device *adev)
}
}
+bool pci_acpi_is_usb4(struct pci_dev *dev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(>dev);
+
+ if (!adev)
+ return false;
+ return fwnode_property_present(acpi_fwnode_handle(adev),
+ "usb4-host-interface");
+}
+
static struct fwnode_handle *(*pci_msi_get_fwnode_cb)(struct device *dev);
/**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3d60cabde1a1..359607c0542d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -695,6 +695,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev);
int acpi_pci_wakeup(struct pci_dev *dev, bool enable);
bool acpi_pci_need_resume(struct pci_dev *dev);
pci_power_t acpi_pci_choose_state(struct pci_dev *pdev);
+bool pci_acpi_is_usb4(struct pci_dev *dev);
#else
static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)
{
@@ -734,6 +735,10 @@ static inline pci_power_t acpi_pci_choose_state(struct
pci_dev *pdev)
{
return PCI_POWER_ERROR;
}
+static inline bool pci_acpi_is_usb4(struct pci_dev *dev)
+{
+ return false;
+}
#endif
#ifdef CONFIG_PCIEASPM
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index e41656cdd8f0..2693211d31cf 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1610,6 +1610,7 @@ static void pci_set_removable(struct pci_dev *dev)
* exposed as "removable" to userspace.
*/
if (vsec ||
+ pci_acpi_is_usb4(dev) ||
(parent &&
(parent->external_facing || dev_is_removable(>dev
dev_set_removable(>dev, DEVICE_REMOVABLE);
--
2.34.1
[PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk
`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt
controller to indicate that D3 is possible. As this is used solely
for older Apple systems, move it into a quirk that enumerates across
all Intel TBT controllers.
Suggested-by: Mika Westerberg
Signed-off-by: Mario Limonciello
---
drivers/pci/pci.c| 12 +-
drivers/pci/quirks.c | 53
2 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f..5002e214c9a6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1064,7 +1064,13 @@ static inline bool platform_pci_bridge_d3(struct pci_dev
*dev)
if (pci_use_mid_pm())
return false;
- return acpi_pci_bridge_d3(dev);
+ if (acpi_pci_bridge_d3(dev))
+ return true;
+
+ if (device_property_read_bool(>dev, "HotPlugSupportInD3"))
+ return true;
+
+ return false;
}
/**
@@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
if (pci_bridge_d3_force)
return true;
- /* Even the oldest 2010 Thunderbolt controller supports D3. */
- if (bridge->is_thunderbolt)
- return true;
-
/* Platform might know better if the bridge supports D3 */
if (platform_pci_bridge_d3(bridge))
return true;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 6d3c88edde00..aaf098ca7d54 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
quirk_apple_poweroff_thunderbolt);
#endif
+/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but
don't specify
+ * it in the ACPI tables
+ */
+static void quirk_apple_d3_thunderbolt(struct pci_dev *dev)
+{
+ struct property_entry properties[] = {
+ PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),
+ {},
+ };
+
+ if (!x86_apple_machine)
+ return;
+
+ if (device_create_managed_software_node(>dev, properties, NULL))
+ pci_warn(dev, "could not add HotPlugSupportInD3 property");
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EAGLE_RIDGE,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_PEAK,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_NHI,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_BRIDGE,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_NHI,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_BRIDGE,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI,
+ quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE,
+ quirk_apple_d3_thunderbolt);
+
/*
* Following are device-specific reset methods which can be used to
* reset a single function if other methods (e.g. FLR, PM D0->D3) are
--
2.34.1
[PATCH v3 01/12] thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4
This PCI class definition of the USB4 device is currently located only in the thunderbolt driver. It will be needed by a few other drivers for upcoming changes. Move it into the common include file. Acked-by: Alex Deucher Acked-by: Mika Westerberg Signed-off-by: Mario Limonciello --- drivers/thunderbolt/nhi.h | 2 -- include/linux/pci_ids.h | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h index 69083aab2736..79e980b51f94 100644 --- a/drivers/thunderbolt/nhi.h +++ b/drivers/thunderbolt/nhi.h @@ -81,6 +81,4 @@ extern const struct tb_nhi_ops icl_nhi_ops; #define PCI_DEVICE_ID_INTEL_TGL_H_NHI0 0x9a1f #define PCI_DEVICE_ID_INTEL_TGL_H_NHI1 0x9a21 -#define PCI_CLASS_SERIAL_USB_USB4 0x0c0340 - #endif diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index aad54c666407..61b161d914f0 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -116,6 +116,7 @@ #define PCI_CLASS_SERIAL_USB_OHCI 0x0c0310 #define PCI_CLASS_SERIAL_USB_EHCI 0x0c0320 #define PCI_CLASS_SERIAL_USB_XHCI 0x0c0330 +#define PCI_CLASS_SERIAL_USB_USB4 0x0c0340 #define PCI_CLASS_SERIAL_USB_DEVICE0x0c03fe #define PCI_CLASS_SERIAL_FIBER 0x0c04 #define PCI_CLASS_SERIAL_SMBUS 0x0c05 -- 2.34.1
[PATCH v3 02/12] PCI: Move `is_thunderbolt` check for lack of command completed to a quirk
The `is_thunderbolt` check is currently used to indicate the lack of
command completed support for a number of older Thunderbolt devices.
This however is heavy handed and should have been done via a quirk. Move
the affected devices outlined in commit 493fb50e958c ("PCI: pciehp: Assume
NoCompl+ for Thunderbolt ports") into pci quirks.
Suggested-by: Lukas Wunner
Signed-off-by: Mario Limonciello
---
drivers/pci/hotplug/pciehp_hpc.c | 6 +-
drivers/pci/quirks.c | 17 +
include/linux/pci.h | 2 ++
3 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 1c1ebf3dad43..e4c42b24aba8 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -996,11 +996,7 @@ struct controller *pcie_init(struct pcie_device *dev)
if (pdev->hotplug_user_indicators)
slot_cap &= ~(PCI_EXP_SLTCAP_AIP | PCI_EXP_SLTCAP_PIP);
- /*
-* We assume no Thunderbolt controllers support Command Complete events,
-* but some controllers falsely claim they do.
-*/
- if (pdev->is_thunderbolt)
+ if (pdev->no_cmd_complete)
slot_cap |= PCI_EXP_SLTCAP_NCCS;
ctrl->slot_cap = slot_cap;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index d2dd6a6cda60..6d3c88edde00 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3675,6 +3675,23 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
quirk_thunderbolt_hotplug_msi);
+static void quirk_thunderbolt_command_completed(struct pci_dev *pdev)
+{
+ pdev->no_cmd_complete = 1;
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
+ quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EAGLE_RIDGE,
+ quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_PEAK,
+ quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
+ quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C,
+ quirk_thunderbolt_command_completed);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
+ quirk_thunderbolt_command_completed);
+
#ifdef CONFIG_ACPI
/*
* Apple: Shutdown Cactus Ridge Thunderbolt controller.
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8253a5413d7c..1e5b769e42fc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -443,6 +443,8 @@ struct pci_dev {
unsigned intis_hotplug_bridge:1;
unsigned intshpc_managed:1; /* SHPC owned by shpchp */
unsigned intis_thunderbolt:1; /* Thunderbolt controller */
+ unsigned intno_cmd_complete:1; /* Lies about command completed
events */
+
/*
* Devices marked being untrusted are the ones that can potentially
* execute DMA attacks and similar. They are typically connected
--
2.34.1
[PATCH v3 00/12] Overhaul `is_thunderbolt`
Various drivers in the kernel use `is_thunderbolt` or `pci_is_thunderbolt_attached` to designate behaving differently from a device that is internally in the machine. This relies upon checks for a specific capability only set on Intel controllers. Non-Intel USB4 designs should also match this designation so that they can be treated the same regardless of the host they're connected to. As part of adding the generic USB4 controller code, it was realized that `is_thunderbolt` and `pcie_is_thunderbolt_attached` have been overloaded. Instead migrate to using removable attribute from device core. Changes from v2->v3: - Add various tags for patches that haven't changed from v2->v3 - Add new patches for Mika's suggestions: * Moving Apple Thunderbolt D3 declaration into quirks * Detect PCIe root port used for PCIe tunneling on integrated controllers using `usb4-host-interface` * Detect PCIe root port used for PCIe tunneling on discrete controllers using the USB4 DVSEC specification Changes from v1->v2: - Add Alex's tag to first patch - Move lack of command completion into a quirk (Lukas) - Drop `is_thunderbolt` attribute and `pci_is_thunderbolt_attached` and use device core removable attribute instead - Adjust all consumers of old attribute to use removable Note: this spans USB/DRM/platform-x86/PCI trees. As a majority of the changes are in PCI, it should probably come through that tree if possible. Mario Limonciello (12): thunderbolt: move definition of PCI_CLASS_SERIAL_USB_USB4 PCI: Move `is_thunderbolt` check for lack of command completed to a quirk PCI: Move check for old Apple Thunderbolt controllers into a quirk PCI: Drop the `is_thunderbolt` attribute from PCI core PCI: Detect root port of internal USB4 devices by `usb4-host-interface` PCI: Explicitly mark USB4 NHI devices as removable PCI: Set ports for discrete USB4 controllers appropriately drm/amd: drop the use of `pci_is_thunderbolt_attached` drm/nouveau: drop the use of `pci_is_thunderbolt_attached` drm/radeon: drop the use of `pci_is_thunderbolt_attached` platform/x86: amd-gmux: drop the use of `pci_is_thunderbolt_attached` PCI: drop `pci_is_thunderbolt_attached` drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 2 +- drivers/gpu/drm/nouveau/nouveau_vga.c | 4 +- drivers/gpu/drm/radeon/radeon_device.c | 4 +- drivers/gpu/drm/radeon/radeon_kms.c | 2 +- drivers/pci/hotplug/pciehp_hpc.c| 6 +-- drivers/pci/pci-acpi.c | 10 drivers/pci/pci.c | 12 +++-- drivers/pci/pci.h | 5 ++ drivers/pci/probe.c | 55 ++- drivers/pci/quirks.c| 70 + drivers/platform/x86/apple-gmux.c | 2 +- drivers/thunderbolt/nhi.h | 2 - include/linux/pci.h | 25 + include/linux/pci_ids.h | 3 ++ 15 files changed, 148 insertions(+), 56 deletions(-) -- 2.34.1
Re: (subset) [PATCH v6 0/5] Add GPU for RK356x SoCs
On Wed, 9 Feb 2022 22:55:44 +0100, Michael Riesch wrote: > This series aims to bring the GPU support for the RK356x mainline. In > conjunction with the VOP2/HDMI TX patches v4 [0]) it has been tested > successfully on a RK3568 EVB1 with weston and glmark2-es2-wayland. > > It should be noted that on the RK3568 EVB1 the supply of the GPU power > domain needs to be set to "always-on" in the device tree. There is an > ongoing discussion to provide a clean solution [1], in the meantime one > has to apply a hack. > > [...] Applied, thanks! [2/5] arm64: dts: rockchip: add gpu node to rk356x commit: 810028668c6d9da25664195d6b906c98a8169f72 [3/5] arm64: dts: rockchip: add cooling map and trip points for gpu to rk356x commit: c0a7259fad2df72469b602418083516c2cb3a7af [4/5] arm64: dts: rockchip: enable the gpu on quartz64-a commit: 6ac383456452378de07e55fc687069d1898a567d [5/5] arm64: dts: rockchip: enable the gpu on rk3568-evb1-v10 commit: 679f048a10d8322d79010e0e52f18a6c17bdf306 [6] arm64: dts: rockchip: enable the tsadc on rk3568-evb1-v10 Best regards, -- Heiko Stuebner
Re: (subset) [PATCH v6 0/5] Add GPU for RK356x SoCs
On Wed, 9 Feb 2022 22:55:44 +0100, Michael Riesch wrote: > This series aims to bring the GPU support for the RK356x mainline. In > conjunction with the VOP2/HDMI TX patches v4 [0]) it has been tested > successfully on a RK3568 EVB1 with weston and glmark2-es2-wayland. > > It should be noted that on the RK3568 EVB1 the supply of the GPU power > domain needs to be set to "always-on" in the device tree. There is an > ongoing discussion to provide a clean solution [1], in the meantime one > has to apply a hack. > > [...] Applied to drm-misc-next, thanks! [1/5] dt-bindings: gpu: mali-bifrost: describe clocks for the rk356x gpu commit: f1775c26e8b8809d922a29bb5e3df6ea503d2fa0 Best regards, -- Heiko Stuebner
Re: [PATCH v5 3/6] drm: Add driver for Solomon SSD130x OLED displays
Hello Andy,
On 2/11/22 17:13, Andy Shevchenko wrote:
[snip]
>
>> +#define SSD130X_SET_COM_PINS_CONFIG1_MASK GENMASK(4, 4)
>
> BIT(4)
>
>> +#define SSD130X_SET_COM_PINS_CONFIG1_SET(val)
>> FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (!val))
>> +#define SSD130X_SET_COM_PINS_CONFIG2_MASK GENMASK(5, 5)
>
> BIT(5)
>
I actually thought about that when using these macros and considered
just using BIT(N) instead but at the end decided to do GENMASK(n, n)
for two reasons:
1) It better documents what this is about, that's bitmask of 1 -bit.
One of the main advantages of using these macros for me is to better
express what these are, otherwise could just use 1 << n or whatever.
2) It's consistent with the other definitions for bitmasks that have
more than one bit.
Looked at other drivers using these macros and noticed that is not
uncommon to have GENMASK(n, n), so I went for that.
>> +#define SSD130X_SET_COM_PINS_CONFIG2_SET(val)
>> FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG2_MASK, (val))
>
> I would put GENMASK() directly into FIELD(), but it's up to you
> (and I haven't checked the use of *_MASK anyway).
>
Same. I also considered just using GENMASK() directly, but since I was
already reworking these, I thought that having the _MASK constant macros
would make the code more explicit about these being masks and what for.
>
> ...
>
>> +static int ssd130x_write_data(struct ssd130x_device *ssd130x, u8 *values,
>> int count)
>> +{
>> +int ret;
>> +
>> +ret = regmap_bulk_write(ssd130x->regmap, SSD130X_DATA, values, count);
>> +if (ret)
>> +return ret;
>> +
>> +return 0;
>
> return regmap_bulk_write(...);
>
Sure.
>> +}
>
> ...
>
>> +/*
>> + * Helper to write command (SSD130X_COMMAND). The fist variadic argument
>> + * is the command to write and the following are the command options.
>> + *
>> + * Note that the ssd130x protocol requires each command and option to be
>> + * written as a SSD130X_COMMAND device register value. That is why a call
>> + * to regmap_write(..., SSD130X_COMMAND, ...) is done for each argument.
>> + */
>
> Thanks!
>
You are welcome.
>> +static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
>> +/* u8 cmd, u8 option, ... */...)
>> +{
>> +va_list ap;
>> +u8 value;
>> +int ret;
>> +
>> +va_start(ap, count);
>> +
>> +do {
>> +value = va_arg(ap, int);
>> +ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
>
> Wondering if you really need this casting. value is u8 by definition.
>
Yeah, I'll drop it too.
[snip]
>> +ssd130x = devm_drm_dev_alloc(dev, _drm_driver,
>> + struct ssd130x_device, drm);
>> +if (IS_ERR(ssd130x)) {
>
>> +dev_err_probe(dev, PTR_ERR(ssd130x),
>> + "Failed to allocate DRM device\n");
>> +return ssd130x;
>
> This...
>
>> +}
>
> ...
>
>> +bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
>> +_bl_ops, NULL);
>> +if (IS_ERR(bl))
>> +return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
>> + "Unable to register backlight
>> device\n"));
>
> Can be consistent with this then.
>
Yes. I meant to change it everywhere but seems that one slipped it through.
It's not worth to send a v6 just for the changes you mentioned but I can do
them before pushing the patches to drm-misc (once I get ack for this patch).
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH] devcoredump: increase the device delete timeout to 10 mins
Hi Greg Thanks for the response. On 2/11/2022 3:09 AM, Greg KH wrote: On Tue, Feb 08, 2022 at 11:44:32AM -0800, Abhinav Kumar wrote: There are cases where depending on the size of the devcoredump and the speed at which the usermode reads the dump, it can take longer than the current 5 mins timeout. This can lead to incomplete dumps as the device is deleted once the timeout expires. One example is below where it took 6 mins for the devcoredump to be completely read. 04:22:24.668 23916 23994 I HWDeviceDRM::DumpDebugData: Opening /sys/class/devcoredump/devcd6/data 04:28:35.377 23916 23994 W HWDeviceDRM::DumpDebugData: Freeing devcoredump node What makes this so slow? Reading from the kernel shouldn't be the limit, is it where the data is being sent to? We are still checking this. We are seeing better read times when we bump up the thread priority of the thread which was reading this. We are also trying to check if bumping up CPU speed is helping. But, results have not been consistently good enough. So we thought we should also increase the timeout to be safe. Increase the timeout to 10 mins to accommodate system delays and large coredump sizes. Nit, please wrap your changelog texts at 72 columns. Yes, i will fix this when I re-post. And what is "large"? We are seeing devcoredumps in the range of 2.5MB-3MB. I can also mention this in the commit text in the next post. Thanks Abhinav thanks, greg k-h
Re: [PATCH v4 1/6] drm/format-helper: Add drm_fb_xrgb8888_to_gray8_line()
On Fri, Feb 11, 2022 at 06:25:17PM +0200, Jani Nikula wrote:
> On Fri, 11 Feb 2022, Andy Shevchenko
> wrote:
> > On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote:
> >> On Fri, 11 Feb 2022, Thomas Zimmermann wrote:
> >> > Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
> >> >> On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas
> >> >> wrote:
> >> >>> On 2/11/22 11:28, Andy Shevchenko wrote:
> >> On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas
> >> wrote:
> >
> > ...
> >
> >> > +static void drm_fb_xrgb_to_gray8_line(u8 *dst, const u32 *src,
> >> > unsigned int pixels)
> >> > +{
> >> > +unsigned int x;
> >> > +
> >> > +for (x = 0; x < pixels; x++) {
> >> > +u8 r = (*src & 0x00ff) >> 16;
> >> > +u8 g = (*src & 0xff00) >> 8;
> >> > +u8 b = *src & 0x00ff;
> >> > +
> >> > +/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> >> > +*dst++ = (3 * r + 6 * g + b) / 10;
> >> > +src++;
> >> > +}
> >>
> >> Can be done as
> >>
> >> while (pixels--) {
> >> ...
> >> }
> >>
> >> or
> >>
> >> do {
> >> ...
> >> } while (--pixels);
> >>
> >> >>>
> >> >>> I don't see why a while loop would be an improvement here TBH.
> >> >>
> >> >> Less letters to parse when reading the code.
> >> >
> >> > It's a simple refactoring of code that has worked well so far. Let's
> >> > leave it as-is for now.
> >>
> >> IMO *always* prefer a for loop over while or do-while.
> >>
> >> The for (i = 0; i < N; i++) is such a strong paradigm in C. You
> >> instantly know how many times you're going to loop, at a glance. Not so
> >> with with the alternatives, which should be used sparingly.
> >
> > while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
>
> And while() is just syntax sugar for goto. :p
>
> The for loop written as for (i = 0; i < N; i++) is hands down the most
> obvious counting loop pattern there is in C.
>
> >> And yes, the do-while suggested above is buggy, and you actually need to
> >> stop and think to see why.
> >
> > It depends if pixels can be 0 or not and if it's not, then does it contain
> > last
> > or number.
> >
> > The do {} while (--pixels); might be buggy iff pixels may be 0.
>
> Yeah. And how long does it take to figure that out?
Okay, I made a mistake to drop the explanation. So, I (mistakenly) assumed
that people know this difference between post-decrement and pre-decrement
(note, while-loop here is not what is problematic).
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 1/5] dt-bindings: gpu: mali-bifrost: describe clocks for the rk356x gpu
On Wed, 09 Feb 2022 22:55:45 +0100, Michael Riesch wrote: > From: Alex Bee > > The Bifrost GPU in Rockchip RK356x SoCs has a core and a bus clock. > Reflect this in the SoC specific part of the binding. > > Signed-off-by: Alex Bee > [move the changes to the SoC section] > Signed-off-by: Michael Riesch > --- > .../devicetree/bindings/gpu/arm,mali-bifrost.yaml | 15 +++ > 1 file changed, 15 insertions(+) > Reviewed-by: Rob Herring
Re: [PATCH v6 01/10] mm: add zone device coherent type memory support
Am 2022-02-11 um 11:39 schrieb David Hildenbrand: On 11.02.22 17:15, David Hildenbrand wrote: On 01.02.22 16:48, Alex Sierra wrote: Device memory that is cache coherent from device and CPU point of view. This is used on platforms that have an advanced system bus (like CAPI or CXL). Any page of a process can be migrated to such memory. However, no one should be allowed to pin such memory so that it can always be evicted. Signed-off-by: Alex Sierra Acked-by: Felix Kuehling Reviewed-by: Alistair Popple So, I'm currently messing with PageAnon() pages and CoW semantics ... all these PageAnon() ZONE_DEVICE variants don't necessarily make my life easier but I'm not sure yet if they make my life harder. I hope you can help me understand some of that stuff. 1) What are expected CoW semantics for DEVICE_COHERENT? I assume we'll share them just like other PageAnon() pages during fork() readable, and the first sharer writing to them receives an "ordinary" !ZONE_DEVICE copy. So this would be just like DEVICE_EXCLUSIVE CoW handling I assume, just that we don't have to go through the loop of restoring a device exclusive entry? 2) How are these pages freed to clear/invalidate PageAnon() ? I assume for PageAnon() ZONE_DEVICE pages we'll always for via free_devmap_managed_page(), correct? 3) FOLL_PIN While you write "no one should be allowed to pin such memory", patch #2 only blocks FOLL_LONGTERM. So I assume we allow ordinary FOLL_PIN and you might want to be a bit more precise? ... I'm pretty sure we cannot FOLL_PIN DEVICE_PRIVATE pages, but can we FILL_PIN DEVICE_EXCLUSIVE pages? I strongly assume so? Thanks for any information. (digging a bit more, I realized that device exclusive pages are not actually/necessarily ZONE_DEVICE pages -- so I assume DEVICE_COHERENT will be the actual first PageAnon() ZONE_DEVICE pages that can be present in a page table.) I think DEVICE_GENERIC pages can also be mapped in the page table. In fact, the first version of our patches attempted to add migration support to DEVICE_GENERIC. But we were convinced to create a new ZONE_DEVICE page type for our use case instead. Regards, Felix
Re: [PATCH v6 01/10] mm: add zone device coherent type memory support
Am 2022-02-11 um 11:15 schrieb David Hildenbrand: On 01.02.22 16:48, Alex Sierra wrote: Device memory that is cache coherent from device and CPU point of view. This is used on platforms that have an advanced system bus (like CAPI or CXL). Any page of a process can be migrated to such memory. However, no one should be allowed to pin such memory so that it can always be evicted. Signed-off-by: Alex Sierra Acked-by: Felix Kuehling Reviewed-by: Alistair Popple So, I'm currently messing with PageAnon() pages and CoW semantics ... all these PageAnon() ZONE_DEVICE variants don't necessarily make my life easier but I'm not sure yet if they make my life harder. I hope you can help me understand some of that stuff. 1) What are expected CoW semantics for DEVICE_COHERENT? I assume we'll share them just like other PageAnon() pages during fork() readable, and the first sharer writing to them receives an "ordinary" !ZONE_DEVICE copy. Yes. So this would be just like DEVICE_EXCLUSIVE CoW handling I assume, just that we don't have to go through the loop of restoring a device exclusive entry? I'm not sure how DEVICE_EXCLUSIVE pages are handled under CoW. As I understand it, they're not really in a special memory zone like DEVICE_COHERENT. Just a special way of mapping an ordinary page in order to allow device-exclusive access for some time. I suspect there may even be a possibility that a page can be both DEVICE_EXCLUSIVE and DEVICE_COHERENT. That said, your statement sounds correct. There is no requirement to do anything with the new "ordinary" page after copying. What actually happens to DEVICE_COHERENT pages on CoW is a bit convoluted: When the page is marked as CoW, it is marked R/O in the CPU page table. This causes an MMU notifier that invalidates the device PTE. The next device access in the parent process causes a page fault. If that's a write fault (usually is in our current driver), it will trigger CoW, which means the parent process now gets a new system memory copy of the page, while the child process keeps the DEVICE_COHERENT page. The driver could decide to migrate the page back to a new DEVICE_COHERENT allocation. In practice that means, "fork" basically causes all DEVICE_COHERENT memory in the parent process to be migrated to ordinary system memory, which is quite disruptive. What we have today results in correct behaviour, but the performance is far from ideal. We could probably mitigate it by making the driver better at mapping pages R/O in the device on read faults, at the potential cost of having to handle a second (write) fault later. 2) How are these pages freed to clear/invalidate PageAnon() ? I assume for PageAnon() ZONE_DEVICE pages we'll always for via free_devmap_managed_page(), correct? Yes. The driver depends on the the page->pgmap->ops->page_free callback to free the device memory allocation backing the page. 3) FOLL_PIN While you write "no one should be allowed to pin such memory", patch #2 only blocks FOLL_LONGTERM. So I assume we allow ordinary FOLL_PIN and you might want to be a bit more precise? I agree. I think the paragraph was written before we fully fleshed out the interaction with GUP, and the forgotten. ... I'm pretty sure we cannot FOLL_PIN DEVICE_PRIVATE pages, Right. Trying to GUP a DEVICE_PRIVATE page causes a page fault that migrates the page back to normal system memory (using the page->pgmap->ops->migrate_to_ram callback). Then you pin the system memory page. but can we FILL_PIN DEVICE_EXCLUSIVE pages? I strongly assume so? I assume you mean DEVICE_COHERENT, not DEVICE_EXCLUSIVE? In that case the answer is "Yes". Regards, Felix Thanks for any information.
Re: [PATCH v6 01/10] mm: add zone device coherent type memory support
On Fri, Feb 11, 2022 at 05:49:08PM +0100, David Hildenbrand wrote: > On 11.02.22 17:45, Jason Gunthorpe wrote: > > On Fri, Feb 11, 2022 at 05:15:25PM +0100, David Hildenbrand wrote: > > > >> ... I'm pretty sure we cannot FOLL_PIN DEVICE_PRIVATE pages > > > > Currently the only way to get a DEVICE_PRIVATE page out of the page > > tables is via hmm_range_fault() and that doesn't manipulate any ref > > counts. > > Thanks for clarifying Jason! ... and AFAIU, device exclusive entries are > essentially just pointers at ordinary PageAnon() pages. So with DEVICE > COHERENT we'll have the first PageAnon() ZONE_DEVICE pages mapped as > present in the page tables where GUP could FOLL_PIN them. This is my understanding Though you probably understand what PageAnon means alot better than I do.. I wonder if it really makes sense to talk about that together with ZONE_DEVICE which has alot in common with filesystem originated pages too. I'm not sure what AMDs plan is here, is there an expecation that a GPU driver will somehow stuff these pages into an existing anonymous memory VMA or do they always come from a driver originated VMA? Jason
Re: [PATCH v6 01/10] mm: add zone device coherent type memory support
On 2/11/2022 10:39 AM, David Hildenbrand wrote: On 11.02.22 17:15, David Hildenbrand wrote: On 01.02.22 16:48, Alex Sierra wrote: Device memory that is cache coherent from device and CPU point of view. This is used on platforms that have an advanced system bus (like CAPI or CXL). Any page of a process can be migrated to such memory. However, no one should be allowed to pin such memory so that it can always be evicted. Signed-off-by: Alex Sierra Acked-by: Felix Kuehling Reviewed-by: Alistair Popple So, I'm currently messing with PageAnon() pages and CoW semantics ... all these PageAnon() ZONE_DEVICE variants don't necessarily make my life easier but I'm not sure yet if they make my life harder. I hope you can help me understand some of that stuff. 1) What are expected CoW semantics for DEVICE_COHERENT? I assume we'll share them just like other PageAnon() pages during fork() readable, and the first sharer writing to them receives an "ordinary" !ZONE_DEVICE copy. So this would be just like DEVICE_EXCLUSIVE CoW handling I assume, just that we don't have to go through the loop of restoring a device exclusive entry? 2) How are these pages freed to clear/invalidate PageAnon() ? I assume for PageAnon() ZONE_DEVICE pages we'll always for via free_devmap_managed_page(), correct? 3) FOLL_PIN While you write "no one should be allowed to pin such memory", patch #2 only blocks FOLL_LONGTERM. So I assume we allow ordinary FOLL_PIN and you might want to be a bit more precise? Device coherent pages can be FOLL_PIN. However, we want to avoid FOLL_LONGTERM with these pages because that would affect our memory manager's ability to evict device memory. Regards, Alex Sierra ... I'm pretty sure we cannot FOLL_PIN DEVICE_PRIVATE pages, but can we FILL_PIN DEVICE_EXCLUSIVE pages? I strongly assume so? Thanks for any information. (digging a bit more, I realized that device exclusive pages are not actually/necessarily ZONE_DEVICE pages -- so I assume DEVICE_COHERENT will be the actual first PageAnon() ZONE_DEVICE pages that can be present in a page table.) Yes, that's correct. Device coherent pages are pte present. Regards, Alex Sierra
Re: [PATCH v6 01/10] mm: add zone device coherent type memory support
On 11.02.22 17:45, Jason Gunthorpe wrote: > On Fri, Feb 11, 2022 at 05:15:25PM +0100, David Hildenbrand wrote: > >> ... I'm pretty sure we cannot FOLL_PIN DEVICE_PRIVATE pages > > Currently the only way to get a DEVICE_PRIVATE page out of the page > tables is via hmm_range_fault() and that doesn't manipulate any ref > counts. Thanks for clarifying Jason! ... and AFAIU, device exclusive entries are essentially just pointers at ordinary PageAnon() pages. So with DEVICE COHERENT we'll have the first PageAnon() ZONE_DEVICE pages mapped as present in the page tables where GUP could FOLL_PIN them. -- Thanks, David / dhildenb
Re: [PATCH v6 01/10] mm: add zone device coherent type memory support
On Fri, Feb 11, 2022 at 05:15:25PM +0100, David Hildenbrand wrote: > ... I'm pretty sure we cannot FOLL_PIN DEVICE_PRIVATE pages Currently the only way to get a DEVICE_PRIVATE page out of the page tables is via hmm_range_fault() and that doesn't manipulate any ref counts. Jason
Re: [PATCH v6 01/10] mm: add zone device coherent type memory support
On 11.02.22 17:15, David Hildenbrand wrote: > On 01.02.22 16:48, Alex Sierra wrote: >> Device memory that is cache coherent from device and CPU point of view. >> This is used on platforms that have an advanced system bus (like CAPI >> or CXL). Any page of a process can be migrated to such memory. However, >> no one should be allowed to pin such memory so that it can always be >> evicted. >> >> Signed-off-by: Alex Sierra >> Acked-by: Felix Kuehling >> Reviewed-by: Alistair Popple > > So, I'm currently messing with PageAnon() pages and CoW semantics ... > all these PageAnon() ZONE_DEVICE variants don't necessarily make my life > easier but I'm not sure yet if they make my life harder. I hope you can > help me understand some of that stuff. > > 1) What are expected CoW semantics for DEVICE_COHERENT? > > I assume we'll share them just like other PageAnon() pages during fork() > readable, and the first sharer writing to them receives an "ordinary" > !ZONE_DEVICE copy. > > So this would be just like DEVICE_EXCLUSIVE CoW handling I assume, just > that we don't have to go through the loop of restoring a device > exclusive entry? > > 2) How are these pages freed to clear/invalidate PageAnon() ? > > I assume for PageAnon() ZONE_DEVICE pages we'll always for via > free_devmap_managed_page(), correct? > > > 3) FOLL_PIN > > While you write "no one should be allowed to pin such memory", patch #2 > only blocks FOLL_LONGTERM. So I assume we allow ordinary FOLL_PIN and > you might want to be a bit more precise? > > > ... I'm pretty sure we cannot FOLL_PIN DEVICE_PRIVATE pages, but can we > FILL_PIN DEVICE_EXCLUSIVE pages? I strongly assume so? > > > Thanks for any information. > (digging a bit more, I realized that device exclusive pages are not actually/necessarily ZONE_DEVICE pages -- so I assume DEVICE_COHERENT will be the actual first PageAnon() ZONE_DEVICE pages that can be present in a page table.) -- Thanks, David / dhildenb
Re: [PATCH v2] dt-bindings: display: bridge: document Toshiba TC358768 cells and panel node
On Mon, 07 Feb 2022 23:39:11 +0100, David Heidelberg wrote: > Inherit valid properties from the dsi-controller. > > Reviewed-by: Dmitry Osipenko > Signed-off-by: David Heidelberg > --- > v2: > - added $ref ../dsi-controller.yaml# instead copying properties (Dmitry) > - additionalProperties -> unevaluatedProperties (Dmitry) > - example dsi-bridge@ -> dsi@ (Dmitry) > > .../bindings/display/bridge/toshiba,tc358768.yaml | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > Applied, thanks!
Re: [PATCH v4 1/6] drm/format-helper: Add drm_fb_xrgb8888_to_gray8_line()
On Fri, 11 Feb 2022, Andy Shevchenko wrote:
> On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote:
>> On Fri, 11 Feb 2022, Thomas Zimmermann wrote:
>> > Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
>> >> On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
>> >>> On 2/11/22 11:28, Andy Shevchenko wrote:
>> On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas
>> wrote:
>
> ...
>
>> > +static void drm_fb_xrgb_to_gray8_line(u8 *dst, const u32 *src,
>> > unsigned int pixels)
>> > +{
>> > + unsigned int x;
>> > +
>> > + for (x = 0; x < pixels; x++) {
>> > + u8 r = (*src & 0x00ff) >> 16;
>> > + u8 g = (*src & 0xff00) >> 8;
>> > + u8 b = *src & 0x00ff;
>> > +
>> > + /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
>> > + *dst++ = (3 * r + 6 * g + b) / 10;
>> > + src++;
>> > + }
>>
>> Can be done as
>>
>> while (pixels--) {
>> ...
>> }
>>
>> or
>>
>> do {
>> ...
>> } while (--pixels);
>>
>> >>>
>> >>> I don't see why a while loop would be an improvement here TBH.
>> >>
>> >> Less letters to parse when reading the code.
>> >
>> > It's a simple refactoring of code that has worked well so far. Let's
>> > leave it as-is for now.
>>
>> IMO *always* prefer a for loop over while or do-while.
>>
>> The for (i = 0; i < N; i++) is such a strong paradigm in C. You
>> instantly know how many times you're going to loop, at a glance. Not so
>> with with the alternatives, which should be used sparingly.
>
> while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
And while() is just syntax sugar for goto. :p
The for loop written as for (i = 0; i < N; i++) is hands down the most
obvious counting loop pattern there is in C.
>> And yes, the do-while suggested above is buggy, and you actually need to
>> stop and think to see why.
>
> It depends if pixels can be 0 or not and if it's not, then does it contain
> last
> or number.
>
> The do {} while (--pixels); might be buggy iff pixels may be 0.
Yeah. And how long does it take to figure that out?
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v1 1/1] drm/i915/gt: Move wbvind_on_all_cpus #define
On 11/02/2022 13:33, Jani Nikula wrote:
On Thu, 10 Feb 2022, Michael Cheng wrote:
Move wbvind_on_all_cpus to intel_gt.h. This will allow other wbind_on_all_cpus
calls to benefit from the #define logic, and prevent compiler errors
when building for non-x86 architectures.
Signed-off-by: Michael Cheng
---
drivers/gpu/drm/i915/gem/i915_gem_pm.c | 7 ---
drivers/gpu/drm/i915/gt/intel_gt.h | 7 +++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
index 6da68b38f00f..ff7340ae5ac8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
@@ -12,13 +12,6 @@
#include "i915_drv.h"
-#if defined(CONFIG_X86)
-#include
-#else
-#define wbinvd_on_all_cpus() \
- pr_warn(DRIVER_NAME ": Missing cache flush in %s\n", __func__)
-#endif
-
void i915_gem_suspend(struct drm_i915_private *i915)
{
GEM_TRACE("%s\n", dev_name(i915->drm.dev));
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
b/drivers/gpu/drm/i915/gt/intel_gt.h
index 2dad46c3eff2..149e8c13e402 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -10,6 +10,13 @@
#include "intel_gt_types.h"
#include "intel_reset.h"
+#if defined(CONFIG_X86)
+#include
+#else
+#define wbinvd_on_all_cpus() \
+ pr_warn(DRIVER_NAME ": Missing cache flush in %s\n", __func__)
+#endif
Don't include headers from headers if it can be avoided.
gt/intel_gt.h is included from 79 files. We don't want all of them to
include when only 3 files actually need
wbinvd_on_all_cpus().
Also, gt/intel_gt.h has absolutely nothing to do with
wbinvd_on_all_cpus() or asm/smp.h. Please don't use topical headers as
dumping grounds for random things.
Maybe a better idea is to add a local wrapper for wbinvd_on_all_cpus()
that does the right thing. Or add the above in a dedicated header.
+1, noting the naming angle:
WBINVD — Write Back and Invalidate Cache
Is an x86 instruction. Also interesting comment:
* XXX: Consider doing a vmap flush or something, where possible.
* Currently we just do a heavy handed wbinvd_on_all_cpus() here since
* the underlying sg_table might not even point to struct pages, so we
* can't just call drm_clflush_sg or similar, like we do elsewhere in
* the driver.
*/
if (i915_gem_object_can_bypass_llc(obj) ||
(!HAS_LLC(i915) && !IS_DG1(i915)))
wbinvd_on_all_cpus();
The two together to me sound like the fix is to either find an equivalent
existing platform agnostic API in the kernel, or if it does not exist create
one and name it generically.
Either per-platform i915, if we go for my proposal, or I guess drm_cache.c if
we don't.
Regards,
Tvrtko
Re: [PATCH v6 01/10] mm: add zone device coherent type memory support
On 01.02.22 16:48, Alex Sierra wrote: > Device memory that is cache coherent from device and CPU point of view. > This is used on platforms that have an advanced system bus (like CAPI > or CXL). Any page of a process can be migrated to such memory. However, > no one should be allowed to pin such memory so that it can always be > evicted. > > Signed-off-by: Alex Sierra > Acked-by: Felix Kuehling > Reviewed-by: Alistair Popple So, I'm currently messing with PageAnon() pages and CoW semantics ... all these PageAnon() ZONE_DEVICE variants don't necessarily make my life easier but I'm not sure yet if they make my life harder. I hope you can help me understand some of that stuff. 1) What are expected CoW semantics for DEVICE_COHERENT? I assume we'll share them just like other PageAnon() pages during fork() readable, and the first sharer writing to them receives an "ordinary" !ZONE_DEVICE copy. So this would be just like DEVICE_EXCLUSIVE CoW handling I assume, just that we don't have to go through the loop of restoring a device exclusive entry? 2) How are these pages freed to clear/invalidate PageAnon() ? I assume for PageAnon() ZONE_DEVICE pages we'll always for via free_devmap_managed_page(), correct? 3) FOLL_PIN While you write "no one should be allowed to pin such memory", patch #2 only blocks FOLL_LONGTERM. So I assume we allow ordinary FOLL_PIN and you might want to be a bit more precise? ... I'm pretty sure we cannot FOLL_PIN DEVICE_PRIVATE pages, but can we FILL_PIN DEVICE_EXCLUSIVE pages? I strongly assume so? Thanks for any information. -- Thanks, David / dhildenb
Re: [PATCH v5 3/6] drm: Add driver for Solomon SSD130x OLED displays
On Fri, Feb 11, 2022 at 03:33:55PM +0100, Javier Martinez Canillas wrote:
> This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
> OLED display controllers.
>
> It's only the core part of the driver and a bus specific driver is needed
> for each transport interface supported by the display controllers.
...
> +#define SSD130X_SET_CLOCK_DIV_MASK GENMASK(3, 0)
> +#define SSD130X_SET_CLOCK_DIV_SET(val)
> FIELD_PREP(SSD130X_SET_CLOCK_DIV_MASK, (val))
> +#define SSD130X_SET_CLOCK_FREQ_MASK GENMASK(7, 4)
> +#define SSD130X_SET_CLOCK_FREQ_SET(val)
> FIELD_PREP(SSD130X_SET_CLOCK_FREQ_MASK, (val))
> +#define SSD130X_SET_PRECHARGE_PERIOD1_MASK GENMASK(3, 0)
> +#define SSD130X_SET_PRECHARGE_PERIOD1_SET(val)
> FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD1_MASK, (val))
> +#define SSD130X_SET_PRECHARGE_PERIOD2_MASK GENMASK(7, 4)
> +#define SSD130X_SET_PRECHARGE_PERIOD2_SET(val)
> FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD2_MASK, (val))
> +#define SSD130X_SET_COM_PINS_CONFIG1_MASKGENMASK(4, 4)
BIT(4)
> +#define SSD130X_SET_COM_PINS_CONFIG1_SET(val)
> FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (!val))
> +#define SSD130X_SET_COM_PINS_CONFIG2_MASKGENMASK(5, 5)
BIT(5)
> +#define SSD130X_SET_COM_PINS_CONFIG2_SET(val)
> FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG2_MASK, (val))
I would put GENMASK() directly into FIELD(), but it's up to you
(and I haven't checked the use of *_MASK anyway).
...
> +static int ssd130x_write_data(struct ssd130x_device *ssd130x, u8 *values,
> int count)
> +{
> + int ret;
> +
> + ret = regmap_bulk_write(ssd130x->regmap, SSD130X_DATA, values, count);
> + if (ret)
> + return ret;
> +
> + return 0;
return regmap_bulk_write(...);
> +}
...
> +/*
> + * Helper to write command (SSD130X_COMMAND). The fist variadic argument
> + * is the command to write and the following are the command options.
> + *
> + * Note that the ssd130x protocol requires each command and option to be
> + * written as a SSD130X_COMMAND device register value. That is why a call
> + * to regmap_write(..., SSD130X_COMMAND, ...) is done for each argument.
> + */
Thanks!
> +static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
> + /* u8 cmd, u8 option, ... */...)
> +{
> + va_list ap;
> + u8 value;
> + int ret;
> +
> + va_start(ap, count);
> +
> + do {
> + value = va_arg(ap, int);
> + ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
Wondering if you really need this casting. value is u8 by definition.
> + if (ret)
> + goto out_end;
> + } while (--count);
> +
> +out_end:
> + va_end(ap);
> +
> + return ret;
> +}
...
> + ssd130x = devm_drm_dev_alloc(dev, _drm_driver,
> + struct ssd130x_device, drm);
> + if (IS_ERR(ssd130x)) {
> + dev_err_probe(dev, PTR_ERR(ssd130x),
> + "Failed to allocate DRM device\n");
> + return ssd130x;
This...
> + }
...
> + bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
> + _bl_ops, NULL);
> + if (IS_ERR(bl))
> + return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
> + "Unable to register backlight
> device\n"));
Can be consistent with this then.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v5 2/6] drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()
On Fri, Feb 11, 2022 at 03:33:54PM +0100, Javier Martinez Canillas wrote:
> Add support to convert from XR24 to reversed monochrome for drivers that
> control monochromatic display panels, that only have 1 bit per pixel.
>
> The function does a line-by-line conversion doing an intermediate step
> first from XR24 to 8-bit grayscale and then to reversed monochrome.
>
> The drm_fb_gray8_to_mono_reversed_line() helper was based on code from
> drivers/gpu/drm/tiny/repaper.c driver.
I believe there is enough room for (micro-)optimizations (like moving out
invariant conditionals from the loop), but since it's almost a direct copy
of the existing code let's improve it later on.
Reviewed-by: Andy Shevchenko
> Signed-off-by: Javier Martinez Canillas
> Reviewed-by: Thomas Zimmermann
> ---
>
> Changes in v5:
> - Use drm_WARN_ON* macros instead of deprecated ones (Thomas Zimmermann)
>
> Changes in v4:
> - Rename end_offset to end_len (Thomas Zimmermann)
> - Warn once if dst_pitch is not a multiple of 8 (Thomas Zimmermann)
> - Drop drm_fb_gray8_to_mono_reversed() that's not used (Thomas Zimmermann)
> - Allocate single buffer for both copy cma memory and gray8 (Thomas
> Zimmermann)
> - Add Thomas Zimmermann Reviewed-by tag to patch adding XR24 -> mono helper.
>
> Changes in v3:
> - Also add a drm_fb_xrgb_to_mono_reversed() helper (Thomas Zimmermann)
> - Split lines copy to drm_fb_gray8_to_mono_reversed_line() (Thomas Zimmermann)
> - Handle case where the source buffer is not aligned to 8 (Thomas Zimmermann)
>
> drivers/gpu/drm/drm_format_helper.c | 110
> include/drm/drm_format_helper.h | 4 +
> 2 files changed, 114 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c
> b/drivers/gpu/drm/drm_format_helper.c
> index b981712623d3..bc0f49773868 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -12,9 +12,11 @@
> #include
> #include
>
> +#include
> #include
> #include
> #include
> +#include
> #include
>
> static unsigned int clip_offset(const struct drm_rect *clip, unsigned int
> pitch, unsigned int cpp)
> @@ -591,3 +593,111 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int
> dst_pitch, uint32_t dst_for
> return -EINVAL;
> }
> EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src,
> unsigned int pixels,
> +unsigned int start_offset,
> unsigned int end_len)
> +{
> + unsigned int xb, i;
> +
> + for (xb = 0; xb < pixels; xb++) {
> + unsigned int start = 0, end = 8;
> + u8 byte = 0x00;
> + if (xb == 0 && start_offset)
> + start = start_offset;
Just to point out again, this is 100% invariant and may be moved out.
> + if (xb == pixels - 1 && end_len)
> + end = end_len;
This one almost, can be moved out after refactoring.
> + for (i = start; i < end; i++) {
> + unsigned int x = xb * 8 + i;
> +
> + byte >>= 1;
> + if (src[x] >> 7)
> + byte |= BIT(7);
> + }
> + *dst++ = byte;
> + }
> +}
> +
> +/**
> + * drm_fb_xrgb_to_mono_reversed - Convert XRGB to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @src: XRGB source buffer
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use this function to convert to the native format.
> + *
> + * This function uses drm_fb_xrgb_to_gray8() to convert to grayscale and
> + * then the result is converted from grayscale to reversed monohrome.
> + */
> +void drm_fb_xrgb_to_mono_reversed(void *dst, unsigned int dst_pitch,
> const void *vaddr,
> + const struct drm_framebuffer *fb, const
> struct drm_rect *clip)
> +{
> + unsigned int linepixels = drm_rect_width(clip);
> + unsigned int lines = clip->y2 - clip->y1;
> + unsigned int cpp = fb->format->cpp[0];
> + unsigned int len_src32 = linepixels * cpp;
> + struct drm_device *dev = fb->dev;
> + unsigned int start_offset, end_len;
> + unsigned int y;
> + u8 *mono = dst, *gray8;
> + u32 *src32;
> +
> + if (drm_WARN_ON(dev, fb->format->format != DRM_FORMAT_XRGB))
> + return;
> +
> + /*
> + * The reversed mono destination buffer contains 1 bit per pixel
> + * and destination scanlines have to be in multiple of 8 pixels.
> + */
> + if (!dst_pitch)
> + dst_pitch = DIV_ROUND_UP(linepixels, 8);
> +
> + drm_WARN_ONCE(dev, dst_pitch % 8 != 0, "dst_pitch is not a multiple of
>
Re: [PATCH v1 1/1] drm/i915/gt: Move wbvind_on_all_cpus #define
Thanks for the feedback! Another idea I had that kind of align to what you are
suggesting is adding a wrapper for wbinvd_on_all_cpus within drm_cache.h, then
having it be included in the three files that calls this function. Thoughts on
that?
From: Jani Nikula
Sent: Friday, February 11, 2022 5:33:37 AM
To: Cheng, Michael ; intel-...@lists.freedesktop.org
Cc: tvrtko.ursu...@linux.intel.com ; Cheng,
Michael ; Vivekanandan, Balasubramani
; Boyer, Wayne ;
Bowman, Casey G ; De Marchi, Lucas
; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v1 1/1] drm/i915/gt: Move wbvind_on_all_cpus #define
On Thu, 10 Feb 2022, Michael Cheng wrote:
> Move wbvind_on_all_cpus to intel_gt.h. This will allow other wbind_on_all_cpus
> calls to benefit from the #define logic, and prevent compiler errors
> when building for non-x86 architectures.
>
> Signed-off-by: Michael Cheng
> ---
> drivers/gpu/drm/i915/gem/i915_gem_pm.c | 7 ---
> drivers/gpu/drm/i915/gt/intel_gt.h | 7 +++
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> index 6da68b38f00f..ff7340ae5ac8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> @@ -12,13 +12,6 @@
>
> #include "i915_drv.h"
>
> -#if defined(CONFIG_X86)
> -#include
> -#else
> -#define wbinvd_on_all_cpus() \
> - pr_warn(DRIVER_NAME ": Missing cache flush in %s\n", __func__)
> -#endif
> -
> void i915_gem_suspend(struct drm_i915_private *i915)
> {
>GEM_TRACE("%s\n", dev_name(i915->drm.dev));
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
> b/drivers/gpu/drm/i915/gt/intel_gt.h
> index 2dad46c3eff2..149e8c13e402 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> @@ -10,6 +10,13 @@
> #include "intel_gt_types.h"
> #include "intel_reset.h"
>
> +#if defined(CONFIG_X86)
> +#include
> +#else
> +#define wbinvd_on_all_cpus() \
> + pr_warn(DRIVER_NAME ": Missing cache flush in %s\n", __func__)
> +#endif
Don't include headers from headers if it can be avoided.
gt/intel_gt.h is included from 79 files. We don't want all of them to
include when only 3 files actually need
wbinvd_on_all_cpus().
Also, gt/intel_gt.h has absolutely nothing to do with
wbinvd_on_all_cpus() or asm/smp.h. Please don't use topical headers as
dumping grounds for random things.
Maybe a better idea is to add a local wrapper for wbinvd_on_all_cpus()
that does the right thing. Or add the above in a dedicated header.
BR,
Jani.
> +
> struct drm_i915_private;
> struct drm_printer;
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v5 1/6] drm/format-helper: Add drm_fb_xrgb8888_to_gray8_line()
On Fri, Feb 11, 2022 at 03:33:53PM +0100, Javier Martinez Canillas wrote:
> Pull the per-line conversion logic into a separate helper function.
>
> This will allow to do line-by-line conversion in other helpers that
> convert to a gray8 format.
for-loop vs. while-loop is not critical, so
Reviewed-by: Andy Shevchenko
> Suggested-by: Thomas Zimmermann
> Signed-off-by: Javier Martinez Canillas
> Reviewed-by: Thomas Zimmermann
> ---
>
> Changes in v5:
> - Add Thomas Zimmermann's Reviewed-by to patch #1.
>
> Changes in v3:
> - Add a drm_fb_xrgb_to_gray8_line() helper function (Thomas Zimmermann)
>
> drivers/gpu/drm/drm_format_helper.c | 31 ++---
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c
> b/drivers/gpu/drm/drm_format_helper.c
> index 0f28dd2bdd72..b981712623d3 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -464,6 +464,21 @@ void drm_fb_xrgb_to_xrgb2101010_toio(void __iomem
> *dst,
> }
> EXPORT_SYMBOL(drm_fb_xrgb_to_xrgb2101010_toio);
>
> +static void drm_fb_xrgb_to_gray8_line(u8 *dst, const u32 *src, unsigned
> int pixels)
> +{
> + unsigned int x;
> +
> + for (x = 0; x < pixels; x++) {
> + u8 r = (*src & 0x00ff) >> 16;
> + u8 g = (*src & 0xff00) >> 8;
> + u8 b = *src & 0x00ff;
> +
> + /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> + *dst++ = (3 * r + 6 * g + b) / 10;
> + src++;
> + }
> +}
> +
> /**
> * drm_fb_xrgb_to_gray8 - Convert XRGB to grayscale
> * @dst: 8-bit grayscale destination buffer
> @@ -484,8 +499,9 @@ EXPORT_SYMBOL(drm_fb_xrgb_to_xrgb2101010_toio);
> void drm_fb_xrgb_to_gray8(void *dst, unsigned int dst_pitch, const void
> *vaddr,
> const struct drm_framebuffer *fb, const struct
> drm_rect *clip)
> {
> - unsigned int len = (clip->x2 - clip->x1) * sizeof(u32);
> - unsigned int x, y;
> + unsigned int linepixels = clip->x2 - clip->x1;
> + unsigned int len = linepixels * sizeof(u32);
> + unsigned int y;
> void *buf;
> u8 *dst8;
> u32 *src32;
> @@ -508,16 +524,7 @@ void drm_fb_xrgb_to_gray8(void *dst, unsigned int
> dst_pitch, const void *vad
> for (y = clip->y1; y < clip->y2; y++) {
> dst8 = dst;
> src32 = memcpy(buf, vaddr, len);
> - for (x = clip->x1; x < clip->x2; x++) {
> - u8 r = (*src32 & 0x00ff) >> 16;
> - u8 g = (*src32 & 0xff00) >> 8;
> - u8 b = *src32 & 0x00ff;
> -
> - /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> - *dst8++ = (3 * r + 6 * g + b) / 10;
> - src32++;
> - }
> -
> + drm_fb_xrgb_to_gray8_line(dst8, src32, linepixels);
> vaddr += fb->pitches[0];
> dst += dst_pitch;
> }
> --
> 2.34.1
>
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 2/6] drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()
On Fri, Feb 11, 2022 at 12:50:04PM +0100, Javier Martinez Canillas wrote:
> On 2/11/22 12:10, Andy Shevchenko wrote:
...
> >> + for (xb = 0; xb < pixels; xb++) {
> >> + unsigned int start = 0, end = 8;
> >> + u8 byte = 0x00;
> >
> >> + if (xb == 0 && start_offset)
> >> + start = start_offset;
> >
> > This is invariant to the loop, can be moved out.
> >
> >> + if (xb == pixels - 1 && end_len)
> >> + end = end_len;
> >
> > Ditto. However it may require to factor out the following loop to a helper.
>
> Not sure I'm following, it's not invariant since it depends on the
> loop iterator value. It only applies to the first and last pixels.
It's. You simply does it at the last iteration which may be perfectly done
outside of the main (aligned) loop.
...
> >> + dst_pitch = DIV_ROUND_UP(linepixels, 8);
> >
> > round_up() ?
>
> But it's not a round up operation but a div and round up.
Indeed.
...
> >> + WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
> >
> >
> > I would move this to the if conditional, i.e.
> >
> > if (dst_pitch)
> > WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of
> > 8\n");
> > else
> > dst_pitch = round_up(linepixels, 8);
>
> No, because we always need to div and round up. The warning is just printed to
> let know that the dst pitch is not a multiple of 8 as it should be. So callers
> could be fixed.
Okay, you expect that linepixels to be multiple of 64? Otherwise I didn't get
what's going on with this warning.
...
> >> + start_offset = clip->x1 % 8;
> >> + end_len = clip->x2 % 8;
> >
> > ALIGN() ?
>
> But we don't want to align here but to know what's the start and end if is
> not aligned since that would mean converting to mono in the middle of a byte.
Indeed. Somehow I missed that it's a complimentary to ALIGN().
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 3/6] drm: Add driver for Solomon SSD130x OLED displays
On Fri, Feb 11, 2022 at 01:05:57PM +0100, Javier Martinez Canillas wrote:
> On 2/11/22 12:33, Andy Shevchenko wrote:
> > On Fri, Feb 11, 2022 at 10:19:24AM +0100, Javier Martinez Canillas wrote:
...
> >> + * Helper to write command (SSD130X_COMMAND). The fist variadic argument
> >> + * is the command to write and the following are the command options.
> >
> > This is not correct explanation. Please, rephrase to show that _each_ of the
> > options is sent with a preceding command.
> >
>
> It's a correct explanation IMO from the caller point of view. The first
> argument
> is the command sent (i.e: SSD130X_SET_ADDRESS_MODE) and the next ones are the
> the command options (i.e: SSD130X_SET_ADDRESS_MODE_HORIZONTAL).
>
> The fact that each command and options are preceding with a SSD130X_COMMAND
> value is part of the protocol of the device and a detail that's abstracted
> away by this helper function to the callers.
My previous suggestion about bulk transaction was purely based on this
(misinterpreted) description. Can we make sure somehow that another reader
don't trap into the same?
...
> >> + bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
> >> + _bl_ops, NULL);
> >> + if (IS_ERR(bl)) {
> >
> >> + ret = PTR_ERR(bl);
> >> + dev_err_probe(dev, ret, "Unable to register backlight
> >> device\n");
> >> + return ERR_PTR(ret);
> >
> > dev_err_probe(dev, PTR_ERR(bl), "Unable to register backlight
> > device\n");
> > return bl;
> >
> > ?
>
> No, because this function's return value is a struct ssd130x_device pointer,
> not a struct backlight_device pointer.
return ERR_CAST(bl);
--
With Best Regards,
Andy Shevchenko
Re: [RFC PATCH 1/6] dt-bindings: display: imx: Add EPDC
On Sun, Feb 06, 2022 at 09:00:11AM +0100, Andreas Kemnade wrote:
> Add a binding for the Electrophoretic Display Controller found at least
> in the i.MX6.
The first version was in i.MX50 (I helped design the register
interface). Is that version compatible?
> The timing subnode is directly here to avoid having display parameters
> spread all over the plate.
>
> Supplies are organized the same way as in the fbdev driver in the
> NXP/Freescale kernel forks. The regulators used for that purpose,
> like the TPS65185, the SY7636A and MAX17135 have typically a single bit to
> start a bunch of regulators of higher or negative voltage with a
> well-defined timing. VCOM can be handled separately, but can also be
> incorporated into that single bit.
>
> Signed-off-by: Andreas Kemnade
> ---
> .../bindings/display/imx/fsl,mxc-epdc.yaml| 159 ++
> 1 file changed, 159 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml
> b/Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml
> new file mode 100644
> index ..7e0795cc3f70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml
> @@ -0,0 +1,159 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/imx/fsl,mxc-epdc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX6 EPDC
> +
> +maintainers:
> + - Andreas Kemnade
> +
> +description: |
> + The EPDC is a controller for handling electronic paper displays found in
> + i.MX6 SoCs.
> +
> +properties:
> + compatible:
> +enum:
> + - fsl,imx6sl-epdc
> + - fsl,imx6sll-epdc
Not compatible with each other?
> +
> + reg:
> +maxItems: 1
> +
> + clocks:
> +items:
> + - description: Bus clock
> + - description: Pixel clock
> +
> + clock-names:
> +items:
> + - const: axi
> + - const: pix
> +
> + interrupts:
> +maxItems: 1
> +
> + vscan-holdoff:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
> +
> + sdoed-width:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
> +
> + sdoed-delay:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
> +
> + sdoez-width:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
> +
> + sdoez-delay:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
> +
> + gdclk-hp-offs:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
> +
> + gdsp-offs:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
> +
> + gdoe-offs:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
> +
> + gdclk-offs:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
> +
> + num-ce:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +maxItems: 1
All these need a vendor prefix and descriptions.
> +
> + timing:
> +$ref: /display/panel/panel-timing.yaml#
> +
> + DISPLAY-supply:
> +description:
> + A couple of +/- voltages automatically powered on in a defintive order
> +
> + VCOM-supply:
> +description: compensation voltage
> +
> + V3P3-supply:
> +description: V3P3 supply
> +
> + epd-thermal-zone:
> +description:
> + Zone to get temperature of the EPD from, practically ambient
> temperature.
> +
> +
> +
1 blank line.
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - interrupts
> + - vscan-holdoff
> + - sdoed-width
> + - sdoed-delay
> + - sdoez-width
> + - sdoez-delay
> + - gdclk-hp-offs
> + - gdsp-offs
> + - gdoe-offs
> + - gdclk-offs
> + - num-ce
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +#include
> +
> +epdc: epdc@20f4000 {
> +compatible = "fsl,imx6sl-epdc";
> +reg = <0x020f4000 0x4000>;
> +interrupts = <0 97 IRQ_TYPE_LEVEL_HIGH>;
> +clocks = < IMX6SL_CLK_EPDC_AXI>, < IMX6SL_CLK_EPDC_PIX>;
> +clock-names = "axi", "pix";
> +
> +pinctrl-names = "default";
> +pinctrl-0 = <_epdc0>;
> +V3P3-supply = <_reg>;
> +VCOM-supply = <_reg>;
> +DISPLAY-supply = <_reg>;
> +epd-thermal-zone = "epd-thermal";
> +
> +vscan-holdoff = <4>;
> +sdoed-width = <10>;
> +sdoed-delay = <20>;
> +sdoez-width = <10>;
> +sdoez-delay = <20>;
> +gdclk-hp-offs = <562>;
> +gdsp-offs = <662>;
> +gdoe-offs = <0>;
> +gdclk-offs = <225>;
> +num-ce = <3>;
> +status = "okay";
Don't need status in examples.
> +
> +timing {
> +clock-frequency = <8000>;
> +hactive = <1448>;
> +hback-porch = <16>;
> +hfront-porch = <102>;
> +
Re: [PATCH V2 5/13] hid: use time_is_after_jiffies() instead of jiffies judgment
On Thu, 2022-02-10 at 18:30 -0800, Qing Wang wrote: > From: Wang Qing > > It is better to use time_xxx() directly instead of jiffies judgment > for understanding. > > Signed-off-by: Wang Qing Acked-by: Srinivas Pandruvada > --- > drivers/hid/intel-ish-hid/ipc/ipc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel- > ish-hid/ipc/ipc.c > index 8ccb246..15e1423 > --- a/drivers/hid/intel-ish-hid/ipc/ipc.c > +++ b/drivers/hid/intel-ish-hid/ipc/ipc.c > @@ -578,7 +578,7 @@ static void _ish_sync_fw_clock(struct > ishtp_device *dev) > static unsigned longprev_sync; > uint64_tusec; > > - if (prev_sync && jiffies - prev_sync < 20 * HZ) > + if (prev_sync && time_is_after_jiffies(prev_sync + 20 * HZ)) > return; > > prev_sync = jiffies;
Re: [PATCH v4 1/6] drm/format-helper: Add drm_fb_xrgb8888_to_gray8_line()
On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote:
> On Fri, 11 Feb 2022, Thomas Zimmermann wrote:
> > Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
> >> On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
> >>> On 2/11/22 11:28, Andy Shevchenko wrote:
> On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
...
> > +static void drm_fb_xrgb_to_gray8_line(u8 *dst, const u32 *src,
> > unsigned int pixels)
> > +{
> > + unsigned int x;
> > +
> > + for (x = 0; x < pixels; x++) {
> > + u8 r = (*src & 0x00ff) >> 16;
> > + u8 g = (*src & 0xff00) >> 8;
> > + u8 b = *src & 0x00ff;
> > +
> > + /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> > + *dst++ = (3 * r + 6 * g + b) / 10;
> > + src++;
> > + }
>
> Can be done as
>
> while (pixels--) {
> ...
> }
>
> or
>
> do {
> ...
> } while (--pixels);
>
> >>>
> >>> I don't see why a while loop would be an improvement here TBH.
> >>
> >> Less letters to parse when reading the code.
> >
> > It's a simple refactoring of code that has worked well so far. Let's
> > leave it as-is for now.
>
> IMO *always* prefer a for loop over while or do-while.
>
> The for (i = 0; i < N; i++) is such a strong paradigm in C. You
> instantly know how many times you're going to loop, at a glance. Not so
> with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
> And yes, the do-while suggested above is buggy, and you actually need to
> stop and think to see why.
It depends if pixels can be 0 or not and if it's not, then does it contain last
or number.
The do {} while (--pixels); might be buggy iff pixels may be 0.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2] drm/panfrost: Handle IDVS_GROUP_SIZE feature
On 11/02/2022 14:58, alyssa.rosenzw...@collabora.com wrote:
> From: Alyssa Rosenzweig
>
> The IDVS group size feature was missing. It is used on some Bifrost and
> Valhall GPUs, and is the last kernel-relevant Bifrost feature we're
> missing.
>
> This feature adds an extra IDVS group size field to the JM_CONFIG
> register. In kbase, the value is configurable via the device tree; kbase
> uses 0xF as a default if no value is specified. Until we find a device
> demanding otherwise, let's always set the 0xF default on devices which
> support this feature mimicking kbase's behaviour.
>
> Tuning this register slightly improves performance of index-driven vertex
> shading. On Mali-G52 (with Mesa), overall glmark2 score is improved from 1026
> to
> 1037. Geometry-heavy scenes like -bshading are improved from 1068 to 1098.
>
> Signed-off-by: Alyssa Rosenzweig
Reviewed-by: Steven Price
> ---
> drivers/gpu/drm/panfrost/panfrost_features.h | 3 +++
> drivers/gpu/drm/panfrost/panfrost_gpu.c | 3 +++
> drivers/gpu/drm/panfrost/panfrost_regs.h | 1 +
> 3 files changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h
> b/drivers/gpu/drm/panfrost/panfrost_features.h
> index 34f2bae1ec8c..36fadcf9634e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_features.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_features.h
> @@ -20,6 +20,7 @@ enum panfrost_hw_feature {
> HW_FEATURE_AARCH64_MMU,
> HW_FEATURE_TLS_HASHING,
> HW_FEATURE_THREAD_GROUP_SPLIT,
> + HW_FEATURE_IDVS_GROUP_SIZE,
> HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG,
> };
>
> @@ -74,6 +75,7 @@ enum panfrost_hw_feature {
> BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
> BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
> BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
> + BIT_ULL(HW_FEATURE_IDVS_GROUP_SIZE) | \
> BIT_ULL(HW_FEATURE_COHERENCY_REG))
>
> #define hw_features_g76 (\
> @@ -87,6 +89,7 @@ enum panfrost_hw_feature {
> BIT_ULL(HW_FEATURE_COHERENCY_REG) | \
> BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
> BIT_ULL(HW_FEATURE_TLS_HASHING) | \
> + BIT_ULL(HW_FEATURE_IDVS_GROUP_SIZE) | \
> BIT_ULL(HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG))
>
> #define hw_features_g31 (\
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index bbe628b306ee..50c8922694d7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -145,6 +145,9 @@ static void panfrost_gpu_init_quirks(struct
> panfrost_device *pfdev)
> quirks |= (COHERENCY_ACE_LITE | COHERENCY_ACE) <<
> JM_FORCE_COHERENCY_FEATURES_SHIFT;
>
> + if (panfrost_has_hw_feature(pfdev, HW_FEATURE_IDVS_GROUP_SIZE))
> + quirks |= JM_DEFAULT_IDVS_GROUP_SIZE <<
> JM_IDVS_GROUP_SIZE_SHIFT;
> +
> if (quirks)
> gpu_write(pfdev, GPU_JM_CONFIG, quirks);
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h
> b/drivers/gpu/drm/panfrost/panfrost_regs.h
> index 6c5a11ef1ee8..16e776cc82ea 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_regs.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
> @@ -208,6 +208,7 @@
> #define JM_MAX_JOB_THROTTLE_LIMIT0x3F
> #define JM_FORCE_COHERENCY_FEATURES_SHIFT 2
> #define JM_IDVS_GROUP_SIZE_SHIFT 16
> +#define JM_DEFAULT_IDVS_GROUP_SIZE 0xF
> #define JM_MAX_IDVS_GROUP_SIZE 0x3F
>
>
