[PATCH v1, 1/1] soc: mediatek: cmdq: add cmdq_pkt_poll_addr function
From: mtk18742
add cmdq_pkt_poll_addr function in cmdq helper functions
Signed-off-by: Yongqiang Niu
---
drivers/soc/mediatek/mtk-cmdq-helper.c | 39
include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
include/linux/soc/mediatek/mtk-cmdq.h| 2 ++
3 files changed, 42 insertions(+)
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 3c8e4212d941..6c5cfb284140 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -344,6 +344,45 @@ int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
}
EXPORT_SYMBOL(cmdq_pkt_set_event);
+s32 cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, u32 value, u32 addr, u32 mask, u8
reg_gpr)
+{
+ struct cmdq_instruction inst = { {0} };
+
+ s32 err;
+
+ if (mask != 0x) {
+ inst.op = CMDQ_CODE_MASK;
+ inst.mask = ~mask;
+ err = cmdq_pkt_append_command(pkt, inst);
+ if (err != 0)
+ return err;
+
+ addr = addr | 0x1;
+ }
+
+ /* Move extra handle APB address to GPR */
+ inst.op = CMDQ_CODE_MOVE;
+ inst.value = addr;
+ inst.sop = reg_gpr;
+ inst.dst_t = 1;
+ err = cmdq_pkt_append_command(pkt, inst);
+ if (err != 0)
+ pr_err("%s fail append command move addr to reg err:%d",
+ __func__, err);
+
+ inst.op = CMDQ_CODE_POLL;
+ inst.value = value;
+ inst.sop = reg_gpr;
+ inst.dst_t = 1;
+ err = cmdq_pkt_append_command(pkt, inst);
+ if (err != 0)
+ pr_err("%s fail append command poll err:%d",
+ __func__, err);
+
+ return err;
+}
+EXPORT_SYMBOL(cmdq_pkt_poll_addr);
+
int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
u16 offset, u32 value)
{
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h
b/include/linux/mailbox/mtk-cmdq-mailbox.h
index 44365aab043c..a27329fd7c7f 100644
--- a/include/linux/mailbox/mtk-cmdq-mailbox.h
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -54,6 +54,7 @@
*/
enum cmdq_code {
CMDQ_CODE_MASK = 0x02,
+ CMDQ_CODE_MOVE = 0x02,
CMDQ_CODE_WRITE = 0x04,
CMDQ_CODE_POLL = 0x08,
CMDQ_CODE_JUMP = 0x10,
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
b/include/linux/soc/mediatek/mtk-cmdq.h
index ac6b5f3cba95..28dc5ce0ff03 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -280,4 +280,6 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
void *data);
+s32 cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, u32 value, u32 addr, u32 mask, u8
reg_gpr);
+
#endif /* __MTK_CMDQ_H__ */
--
2.25.1
[PATCH v1, 0/1] soc: mediatek: cmdq: add cmdq_pkt_poll_addr function
add cmdq_pkt_poll_addr function mtk18742 (1): soc: mediatek: cmdq: add cmdq_pkt_poll_addr function drivers/soc/mediatek/mtk-cmdq-helper.c | 39 include/linux/mailbox/mtk-cmdq-mailbox.h | 1 + include/linux/soc/mediatek/mtk-cmdq.h| 2 ++ 3 files changed, 42 insertions(+) -- 2.25.1
[PATCH v5] dma-buf: system_heap: Add a size check for allocation
From: Guangming Add a size check for allocation since the allocation size should be always less than the total DRAM size on system heap. Adding this check can prevent comsuming too much time for invalid allocations. Signed-off-by: Guangming --- drivers/dma-buf/heaps/system_heap.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..459dc18bc4a2 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -347,6 +347,14 @@ static struct dma_buf *system_heap_allocate(struct dma_heap *heap, struct page *page, *tmp_page; int i, ret = -ENOMEM; + /* +* Size check. The "len" should be less than totalram since system_heap +* memory is comes from system. Adding check here can prevent comsuming +* too much time for invalid allocations. +*/ + if (len >> PAGE_SHIFT > totalram_pages()) + return ERR_PTR(-EINVAL); + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); if (!buffer) return ERR_PTR(-ENOMEM); -- 2.17.1
Re: [PATCH v3 00/10] Add MEMORY_DEVICE_COHERENT for coherent device memory mapping
On Wednesday, 12 January 2022 10:06:03 PM AEDT Alistair Popple wrote:
> I have been looking at this in relation to the migration code and noticed we
> have the following in try_to_migrate():
>
> if (is_zone_device_page(page) && !is_device_private_page(page))
> return;
>
> Which if I'm understanding correctly means that migration of device coherent
> pages will always fail. Given that I do wonder how hmm-tests are passing, but
> I assume you must always be hitting this fast path in
> migrate_vma_collect_pmd():
>
> /*
> * Optimize for the common case where page is only mapped once
> * in one process. If we can lock the page, then we can safely
> * set up a special migration page table entry now.
> */
>
> Meaning that try_to_migrate() never gets called from migrate_vma_unmap(). So
> you will also need some changes to try_to_migrate() and possibly
> try_to_migrate_one() to make this reliable.
I have been running the hmm tests with the changes below. I'm pretty sure these
are correct because the only zone device pages try_to_migrate_one() should be
called on are device coherent/private, and coherent pages can be treated just
the same as a normal pages for migration. However it would be worth checking I
haven't missed anything.
- Alistair
---
diff --git a/mm/rmap.c b/mm/rmap.c
index 163ac4e6bcee..15f56c27daab 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1806,7 +1806,7 @@ static bool try_to_migrate_one(struct page *page, struct
vm_area_struct *vma,
/* Update high watermark before we lower rss */
update_hiwater_rss(mm);
- if (is_zone_device_page(page)) {
+ if (is_device_private_page(page)) {
unsigned long pfn = page_to_pfn(page);
swp_entry_t entry;
pte_t swp_pte;
@@ -1947,7 +1947,7 @@ void try_to_migrate(struct page *page, enum ttu_flags
flags)
TTU_SYNC)))
return;
- if (is_zone_device_page(page) && !is_device_private_page(page))
+ if (is_zone_device_page(page) && !is_device_page(page))
return;
/*
Re: [PATCH v3 10/10] tools: update test_hmm script to support SP config
Looks good,
Reviewed-by: Alistair Popple
On Tuesday, 11 January 2022 9:32:01 AM AEDT Alex Sierra wrote:
> Add two more parameters to set spm_addr_dev0 & spm_addr_dev1
> addresses. These two parameters configure the start SP
> addresses for each device in test_hmm driver.
> Consequently, this configures zone device type as coherent.
>
> Signed-off-by: Alex Sierra
> ---
> v2:
> Add more mknods for device coherent type. These are represented under
> /dev/hmm_mirror2 and /dev/hmm_mirror3, only in case they have created
> at probing the hmm-test driver.
> ---
> tools/testing/selftests/vm/test_hmm.sh | 24 +---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/test_hmm.sh
> b/tools/testing/selftests/vm/test_hmm.sh
> index 0647b525a625..539c9371e592 100755
> --- a/tools/testing/selftests/vm/test_hmm.sh
> +++ b/tools/testing/selftests/vm/test_hmm.sh
> @@ -40,11 +40,26 @@ check_test_requirements()
>
> load_driver()
> {
> - modprobe $DRIVER > /dev/null 2>&1
> + if [ $# -eq 0 ]; then
> + modprobe $DRIVER > /dev/null 2>&1
> + else
> + if [ $# -eq 2 ]; then
> + modprobe $DRIVER spm_addr_dev0=$1 spm_addr_dev1=$2
> + > /dev/null 2>&1
> + else
> + echo "Missing module parameters. Make sure pass"\
> + "spm_addr_dev0 and spm_addr_dev1"
> + usage
> + fi
> + fi
> if [ $? == 0 ]; then
> major=$(awk "\$2==\"HMM_DMIRROR\" {print \$1}" /proc/devices)
> mknod /dev/hmm_dmirror0 c $major 0
> mknod /dev/hmm_dmirror1 c $major 1
> + if [ $# -eq 2 ]; then
> + mknod /dev/hmm_dmirror2 c $major 2
> + mknod /dev/hmm_dmirror3 c $major 3
> + fi
> fi
> }
>
> @@ -58,7 +73,7 @@ run_smoke()
> {
> echo "Running smoke test. Note, this test provides basic coverage."
>
> - load_driver
> + load_driver $1 $2
> $(dirname "${BASH_SOURCE[0]}")/hmm-tests
> unload_driver
> }
> @@ -75,6 +90,9 @@ usage()
> echo "# Smoke testing"
> echo "./${TEST_NAME}.sh smoke"
> echo
> + echo "# Smoke testing with SPM enabled"
> + echo "./${TEST_NAME}.sh smoke "
> + echo
> exit 0
> }
>
> @@ -84,7 +102,7 @@ function run_test()
> usage
> else
> if [ "$1" = "smoke" ]; then
> - run_smoke
> + run_smoke $2 $3
> else
> usage
> fi
>
Re: [PATCH v3 09/10] tools: update hmm-test to support device coherent type
On Tuesday, 11 January 2022 9:32:00 AM AEDT Alex Sierra wrote:
> Test cases such as migrate_fault and migrate_multiple, were modified to
> explicit migrate from device to sys memory without the need of page
> faults, when using device coherent type.
>
> Snapshot test case updated to read memory device type first and based
> on that, get the proper returned results migrate_ping_pong test case
Where is the migrate_ping_pong test? Did you perhaps forget to add it? :-)
> added to test explicit migration from device to sys memory for both
> private and coherent zone types.
>
> Helpers to migrate from device to sys memory and vicerversa
> were also added.
>
> Signed-off-by: Alex Sierra
> ---
> v2:
> Set FIXTURE_VARIANT to add multiple device types to the FIXTURE. This
> will run all the tests for each device type (private and coherent) in
> case both existed during hmm-test driver probed.
> ---
> tools/testing/selftests/vm/hmm-tests.c | 122 -
> 1 file changed, 101 insertions(+), 21 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/hmm-tests.c
> b/tools/testing/selftests/vm/hmm-tests.c
> index 864f126ffd78..8eb81dfba4b3 100644
> --- a/tools/testing/selftests/vm/hmm-tests.c
> +++ b/tools/testing/selftests/vm/hmm-tests.c
> @@ -44,6 +44,14 @@ struct hmm_buffer {
> int fd;
> uint64_tcpages;
> uint64_tfaults;
> + int zone_device_type;
> +};
> +
> +enum {
> + HMM_PRIVATE_DEVICE_ONE,
> + HMM_PRIVATE_DEVICE_TWO,
> + HMM_COHERENCE_DEVICE_ONE,
> + HMM_COHERENCE_DEVICE_TWO,
> };
>
> #define TWOMEG (1 << 21)
> @@ -60,6 +68,21 @@ FIXTURE(hmm)
> unsigned intpage_shift;
> };
>
> +FIXTURE_VARIANT(hmm)
> +{
> + int device_number;
> +};
> +
> +FIXTURE_VARIANT_ADD(hmm, hmm_device_private)
> +{
> + .device_number = HMM_PRIVATE_DEVICE_ONE,
> +};
> +
> +FIXTURE_VARIANT_ADD(hmm, hmm_device_coherent)
> +{
> + .device_number = HMM_COHERENCE_DEVICE_ONE,
> +};
> +
> FIXTURE(hmm2)
> {
> int fd0;
> @@ -68,6 +91,24 @@ FIXTURE(hmm2)
> unsigned intpage_shift;
> };
>
> +FIXTURE_VARIANT(hmm2)
> +{
> + int device_number0;
> + int device_number1;
> +};
> +
> +FIXTURE_VARIANT_ADD(hmm2, hmm2_device_private)
> +{
> + .device_number0 = HMM_PRIVATE_DEVICE_ONE,
> + .device_number1 = HMM_PRIVATE_DEVICE_TWO,
> +};
> +
> +FIXTURE_VARIANT_ADD(hmm2, hmm2_device_coherent)
> +{
> + .device_number0 = HMM_COHERENCE_DEVICE_ONE,
> + .device_number1 = HMM_COHERENCE_DEVICE_TWO,
> +};
> +
> static int hmm_open(int unit)
> {
> char pathname[HMM_PATH_MAX];
> @@ -81,12 +122,19 @@ static int hmm_open(int unit)
> return fd;
> }
>
> +static bool hmm_is_coherent_type(int dev_num)
> +{
> + return (dev_num >= HMM_COHERENCE_DEVICE_ONE);
> +}
> +
> FIXTURE_SETUP(hmm)
> {
> self->page_size = sysconf(_SC_PAGE_SIZE);
> self->page_shift = ffs(self->page_size) - 1;
>
> - self->fd = hmm_open(0);
> + self->fd = hmm_open(variant->device_number);
> + if (self->fd < 0 && hmm_is_coherent_type(variant->device_number))
> + SKIP(exit(0), "DEVICE_COHERENT not available");
> ASSERT_GE(self->fd, 0);
> }
>
> @@ -95,9 +143,11 @@ FIXTURE_SETUP(hmm2)
> self->page_size = sysconf(_SC_PAGE_SIZE);
> self->page_shift = ffs(self->page_size) - 1;
>
> - self->fd0 = hmm_open(0);
> + self->fd0 = hmm_open(variant->device_number0);
> + if (self->fd0 < 0 && hmm_is_coherent_type(variant->device_number0))
> + SKIP(exit(0), "DEVICE_COHERENT not available");
> ASSERT_GE(self->fd0, 0);
> - self->fd1 = hmm_open(1);
> + self->fd1 = hmm_open(variant->device_number1);
> ASSERT_GE(self->fd1, 0);
> }
>
> @@ -144,6 +194,7 @@ static int hmm_dmirror_cmd(int fd,
> }
> buffer->cpages = cmd.cpages;
> buffer->faults = cmd.faults;
> + buffer->zone_device_type = cmd.zone_device_type;
>
> return 0;
> }
> @@ -211,6 +262,20 @@ static void hmm_nanosleep(unsigned int n)
> nanosleep(, NULL);
> }
>
> +static int hmm_migrate_sys_to_dev(int fd,
> +struct hmm_buffer *buffer,
> +unsigned long npages)
> +{
> + return hmm_dmirror_cmd(fd, HMM_DMIRROR_MIGRATE_TO_DEV, buffer, npages);
> +}
> +
> +static int hmm_migrate_dev_to_sys(int fd,
> +struct hmm_buffer *buffer,
> +unsigned long npages)
> +{
> + return hmm_dmirror_cmd(fd, HMM_DMIRROR_MIGRATE_TO_SYS, buffer, npages);
> +}
> +
> /*
> * Simple NULL test of device open/close.
> */
> @@ -875,7 +940,7 @@ TEST_F(hmm, migrate)
> ptr[i] = i;
>
> /* Migrate memory to device. */
> - ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_MIGRATE, buffer, npages);
> + ret = hmm_migrate_sys_to_dev(self->fd, buffer, npages);
> ASSERT_EQ(ret, 0);
>
Re: [RFC] amdgpu MST questions, and future plans
On Wed, 2022-01-19 at 17:56 -0500, Lyude Paul wrote: > Hi! I'm writing this email because I'm currently finishing up removing > pretty > much all of the non-atomic MST code in drm_dp_mst_topology_mgr.c as it's > really made it difficult to maintain MST over time. As well, once that's > complete it's likely I'm going to start on the (extremely overdue) task of > moving as much of amdgpu's MST code for DSC out of amdgpu and into the DRM > code where it's supposed to live. > > This brings me to two questions. The first major one being: is anyone > capable > of testing the MST support in radeon.ko to figure out whether it works or > not? > I've already talked with hwentlan and ag5df about this and they haven't been > able to find anyone to help with testing this. The reason I ask is because > radeon isn't an atomic driver, and is basically the only user of any of the > non-atomic parts of the MST helpers. If anyone want to prevent this from > breaking in the future, I would definitely recommend they step up to try and > help with testing it - otherwise I'm probably going to be pushing for us > just > to drop the code entirely. > > The second question is: is anyone willing to help me figure out how much of > the code in amdgpu related to DSC is definitely not amdgpu specific and can > be > moved out? I'm honestly having a lot of trouble wrapping my head around how > some of this works, and how much of this code is even needed. As well, with > the amount of issues I've already found in it (there's numerous spots where > we're storing MST state outside of atomic state for instance, lots of > duplicates of DP helper functions that should not be here, etc.) it's quite > likely I'm going to be rewriting rather large chunks of it. If anyone would mhhh - on second thought I think I'm starting to wrap my head around this and it's not actually too bad :), still could use some help with the radeon testing though! > like to volunteer please let me know, it'd be super appreciated and likely > will make reviewing the patches that will come out of this easier. -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
Re: [PATCH v3 08/10] lib: add support for device coherent type in test_hmm
On Tuesday, 11 January 2022 9:31:59 AM AEDT Alex Sierra wrote:
> Device Coherent type uses device memory that is coherently accesible by
> the CPU. This could be shown as SP (special purpose) memory range
> at the BIOS-e820 memory enumeration. If no SP memory is supported in
> system, this could be faked by setting CONFIG_EFI_FAKE_MEMMAP.
>
> Currently, test_hmm only supports two different SP ranges of at least
> 256MB size. This could be specified in the kernel parameter variable
> efi_fake_mem. Ex. Two SP ranges of 1GB starting at 0x1 &
> 0x14000 physical address. Ex.
> efi_fake_mem=1G@0x1:0x4,1G@0x14000:0x4
>
> Private and coherent device mirror instances can be created in the same
> probed. This is done by passing the module parameters spm_addr_dev0 &
> spm_addr_dev1. In this case, it will create four instances of
> device_mirror. The first two correspond to private device type, the
> last two to coherent type. Then, they can be easily accessed from user
> space through /dev/hmm_mirror. Usually num_device 0 and 1
> are for private, and 2 and 3 for coherent types. If no module
> parameters are passed, two instances of private type device_mirror will
> be created only.
>
> Signed-off-by: Alex Sierra
> ---
> lib/test_hmm.c | 247
> lib/test_hmm_uapi.h | 15 ++-
> 2 files changed, 193 insertions(+), 69 deletions(-)
>
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 9edeff52302e..7c641c5a9cfa 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -29,11 +29,22 @@
>
> #include "test_hmm_uapi.h"
>
> -#define DMIRROR_NDEVICES 2
> +#define DMIRROR_NDEVICES 4
> #define DMIRROR_RANGE_FAULT_TIMEOUT 1000
> #define DEVMEM_CHUNK_SIZE(256 * 1024 * 1024U)
> #define DEVMEM_CHUNKS_RESERVE16
>
> +/*
> + * For device_private pages, dpage is just a dummy struct page
> + * representing a piece of device memory. dmirror_devmem_alloc_page
> + * allocates a real system memory page as backing storage to fake a
> + * real device. zone_device_data points to that backing page. But
> + * for device_coherent memory, the struct page represents real
> + * physical CPU-accessible memory that we can use directly.
> + */
> +#define BACKING_PAGE(page) (is_device_private_page((page)) ? \
> +(page)->zone_device_data : (page))
> +
> static unsigned long spm_addr_dev0;
> module_param(spm_addr_dev0, long, 0644);
> MODULE_PARM_DESC(spm_addr_dev0,
> @@ -122,6 +133,21 @@ static int dmirror_bounce_init(struct dmirror_bounce
> *bounce,
> return 0;
> }
>
> +static bool dmirror_is_private_zone(struct dmirror_device *mdevice)
> +{
> + return (mdevice->zone_device_type ==
> + HMM_DMIRROR_MEMORY_DEVICE_PRIVATE) ? true : false;
> +}
> +
> +static enum migrate_vma_direction
> + dmirror_select_device(struct dmirror *dmirror)
> +{
> + return (dmirror->mdevice->zone_device_type ==
> + HMM_DMIRROR_MEMORY_DEVICE_PRIVATE) ?
> + MIGRATE_VMA_SELECT_DEVICE_PRIVATE :
> + MIGRATE_VMA_SELECT_DEVICE_COHERENT;
> +}
> +
> static void dmirror_bounce_fini(struct dmirror_bounce *bounce)
> {
> vfree(bounce->ptr);
> @@ -572,16 +598,19 @@ static int dmirror_allocate_chunk(struct dmirror_device
> *mdevice,
> static struct page *dmirror_devmem_alloc_page(struct dmirror_device *mdevice)
> {
> struct page *dpage = NULL;
> - struct page *rpage;
> + struct page *rpage = NULL;
>
> /*
> - * This is a fake device so we alloc real system memory to store
> - * our device memory.
> + * For ZONE_DEVICE private type, this is a fake device so we alloc real
> + * system memory to store our device memory.
> + * For ZONE_DEVICE coherent type we use the actual dpage to store the
> data
> + * and ignore rpage.
>*/
> - rpage = alloc_page(GFP_HIGHUSER);
> - if (!rpage)
> - return NULL;
> -
> + if (dmirror_is_private_zone(mdevice)) {
> + rpage = alloc_page(GFP_HIGHUSER);
> + if (!rpage)
> + return NULL;
> + }
> spin_lock(>lock);
>
> if (mdevice->free_pages) {
> @@ -601,7 +630,8 @@ static struct page *dmirror_devmem_alloc_page(struct
> dmirror_device *mdevice)
> return dpage;
>
> error:
> - __free_page(rpage);
> + if (rpage)
> + __free_page(rpage);
> return NULL;
> }
>
> @@ -627,12 +657,15 @@ static void dmirror_migrate_alloc_and_copy(struct
> migrate_vma *args,
>* unallocated pte_none() or read-only zero page.
>*/
> spage = migrate_pfn_to_page(*src);
> + WARN(spage && is_zone_device_page(spage),
> + "page already in device spage pfn: 0x%lx\n",
> + page_to_pfn(spage));
This should also lead to test failure because we are only supposed to be
selecting system
[Bug 213561] [bisected][regression] GFX10 AMDGPUs can no longer enter idle state after commit. Commit has been pushed to stable branches too.
https://bugzilla.kernel.org/show_bug.cgi?id=213561 sosh...@gmail.com changed: What|Removed |Added CC||sosh...@gmail.com --- Comment #15 from sosh...@gmail.com --- I have exactly the same problem on my Dell XPS 9575 laptop. GPU: AMD Polaris 22 XL [Radeon RX Vega M GL]. Kernel: 5.15.12 Several months ago there was no such problem. The amdgpu is always in D0 state and fans are spinning all the time. How may I help to fix the problem? -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v3 07/10] lib: test_hmm add module param for zone device type
Thanks for splitting the coherent devices into separate device nodes. Couple of
comments below.
On Tuesday, 11 January 2022 9:31:58 AM AEDT Alex Sierra wrote:
> In order to configure device coherent in test_hmm, two module parameters
> should be passed, which correspond to the SP start address of each
> device (2) spm_addr_dev0 & spm_addr_dev1. If no parameters are passed,
> private device type is configured.
>
> Signed-off-by: Alex Sierra
> ---
> lib/test_hmm.c | 74 +++--
> lib/test_hmm_uapi.h | 1 +
> 2 files changed, 53 insertions(+), 22 deletions(-)
>
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 97e48164d56a..9edeff52302e 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -34,6 +34,16 @@
> #define DEVMEM_CHUNK_SIZE(256 * 1024 * 1024U)
> #define DEVMEM_CHUNKS_RESERVE16
>
> +static unsigned long spm_addr_dev0;
> +module_param(spm_addr_dev0, long, 0644);
> +MODULE_PARM_DESC(spm_addr_dev0,
> + "Specify start address for SPM (special purpose memory) used
> for device 0. By setting this Coherent device type will be used. Make sure
> spm_addr_dev1 is set too");
It would be useful if you could mention the required size for this region
(ie. DEVMEM_CHUNK_SIZE).
> +
> +static unsigned long spm_addr_dev1;
> +module_param(spm_addr_dev1, long, 0644);
> +MODULE_PARM_DESC(spm_addr_dev1,
> + "Specify start address for SPM (special purpose memory) used
> for device 1. By setting this Coherent device type will be used. Make sure
> spm_addr_dev0 is set too");
> +
> static const struct dev_pagemap_ops dmirror_devmem_ops;
> static const struct mmu_interval_notifier_ops dmirror_min_ops;
> static dev_t dmirror_dev;
> @@ -452,29 +462,44 @@ static int dmirror_write(struct dmirror *dmirror,
> struct hmm_dmirror_cmd *cmd)
> return ret;
> }
>
> -static bool dmirror_allocate_chunk(struct dmirror_device *mdevice,
> +static int dmirror_allocate_chunk(struct dmirror_device *mdevice,
> struct page **ppage)
> {
> struct dmirror_chunk *devmem;
> - struct resource *res;
> + struct resource *res = NULL;
> unsigned long pfn;
> unsigned long pfn_first;
> unsigned long pfn_last;
> void *ptr;
> + int ret = -ENOMEM;
>
> devmem = kzalloc(sizeof(*devmem), GFP_KERNEL);
> if (!devmem)
> - return false;
> + return ret;
>
> - res = request_free_mem_region(_resource, DEVMEM_CHUNK_SIZE,
> - "hmm_dmirror");
> - if (IS_ERR(res))
> + switch (mdevice->zone_device_type) {
> + case HMM_DMIRROR_MEMORY_DEVICE_PRIVATE:
> + res = request_free_mem_region(_resource,
> DEVMEM_CHUNK_SIZE,
> + "hmm_dmirror");
> + if (IS_ERR_OR_NULL(res))
> + goto err_devmem;
> + devmem->pagemap.range.start = res->start;
> + devmem->pagemap.range.end = res->end;
> + devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
> + break;
> + case HMM_DMIRROR_MEMORY_DEVICE_COHERENT:
> + devmem->pagemap.range.start = (MINOR(mdevice->cdevice.dev) - 2)
> ?
> + spm_addr_dev0 :
> + spm_addr_dev1;
> + devmem->pagemap.range.end = devmem->pagemap.range.start +
> + DEVMEM_CHUNK_SIZE - 1;
> + devmem->pagemap.type = MEMORY_DEVICE_COHERENT;
> + break;
> + default:
> + ret = -EINVAL;
> goto err_devmem;
> + }
>
> - mdevice->zone_device_type = HMM_DMIRROR_MEMORY_DEVICE_PRIVATE;
What initialises mdevice->zone_device_type now? It looks like it needs to get
initialised in hmm_dmirror_init(), which would be easier to do in the previous
patch rather than adding it here in the first place.
> - devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
> - devmem->pagemap.range.start = res->start;
> - devmem->pagemap.range.end = res->end;
> devmem->pagemap.nr_range = 1;
> devmem->pagemap.ops = _devmem_ops;
> devmem->pagemap.owner = mdevice;
> @@ -495,10 +520,14 @@ static bool dmirror_allocate_chunk(struct
> dmirror_device *mdevice,
> mdevice->devmem_capacity = new_capacity;
> mdevice->devmem_chunks = new_chunks;
> }
> -
> ptr = memremap_pages(>pagemap, numa_node_id());
> - if (IS_ERR(ptr))
> + if (IS_ERR_OR_NULL(ptr)) {
> + if (ptr)
> + ret = PTR_ERR(ptr);
> + else
> + ret = -EFAULT;
> goto err_release;
> + }
>
> devmem->mdevice = mdevice;
> pfn_first = devmem->pagemap.range.start >> PAGE_SHIFT;
> @@ -527,15 +556,17 @@ static bool dmirror_allocate_chunk(struct
> dmirror_device *mdevice,
>
Re: [PATCH v3 06/10] lib: test_hmm add ioctl to get zone device type
On Tuesday, 11 January 2022 9:31:57 AM AEDT Alex Sierra wrote:
[...]
> +enum {
> + /* 0 is reserved to catch uninitialized type fields */
This seems unnecessary and can be dropped to start at zero.
Reviewed-by: Alistair Popple
> + HMM_DMIRROR_MEMORY_DEVICE_PRIVATE = 1,
> +};
> +
> #endif /* _LIB_TEST_HMM_UAPI_H */
>
Re: [PATCH] dt-bindings: Improve phandle-array schemas
On 18-01-22, 19:50, Rob Herring wrote: > The 'phandle-array' type is a bit ambiguous. It can be either just an > array of phandles or an array of phandles plus args. Many schemas for > phandle-array properties aren't clear in the schema which case applies > though the description usually describes it. > > The array of phandles case boils down to needing: > > items: > maxItems: 1 > > The phandle plus args cases should typically take this form: > > items: > - items: > - description: A phandle > - description: 1st arg cell > - description: 2nd arg cell > > With this change, some examples need updating so that the bracketing of > property values matches the schema. Acked-By: Vinod Koul -- ~Vinod
Re: [PATCH v5 3/4] drm/bridge: anx7625: Support reading edid through aux channel
Hi Hsin-Yi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-tip/drm-tip]
[cannot apply to drm/drm-next robh/for-next drm-intel/for-linux-next v5.16
next-20220118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Hsin-Yi-Wang/drm-bridge-anx7625-send-DPCD-command-to-downstream/20220119-231952
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: i386-randconfig-r024-20220117
(https://download.01.org/0day-ci/archive/20220120/202201201223.sts9atbc-...@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/61809df270082584886188b067ee19744f4b35e1
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Hsin-Yi-Wang/drm-bridge-anx7625-send-DPCD-command-to-downstream/20220119-231952
git checkout 61809df270082584886188b067ee19744f4b35e1
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
ld: drivers/gpu/drm/bridge/analogix/anx7625.o: in function
`anx7625_i2c_probe':
>> drivers/gpu/drm/bridge/analogix/anx7625.c:2648: undefined reference to
>> `devm_of_dp_aux_populate_ep_devices'
vim +2648 drivers/gpu/drm/bridge/analogix/anx7625.c
2574
2575 static int anx7625_i2c_probe(struct i2c_client *client,
2576 const struct i2c_device_id *id)
2577 {
2578 struct anx7625_data *platform;
2579 struct anx7625_platform_data *pdata;
2580 int ret = 0;
2581 struct device *dev = >dev;
2582
2583 if (!i2c_check_functionality(client->adapter,
2584 I2C_FUNC_SMBUS_I2C_BLOCK)) {
2585 DRM_DEV_ERROR(dev, "anx7625's i2c bus doesn't
support\n");
2586 return -ENODEV;
2587 }
2588
2589 platform = devm_kzalloc(dev, sizeof(*platform), GFP_KERNEL);
2590 if (!platform) {
2591 DRM_DEV_ERROR(dev, "fail to allocate driver data\n");
2592 return -ENOMEM;
2593 }
2594
2595 pdata = >pdata;
2596
2597 platform->client = client;
2598 i2c_set_clientdata(client, platform);
2599
2600 pdata->supplies[0].supply = "vdd10";
2601 pdata->supplies[1].supply = "vdd18";
2602 pdata->supplies[2].supply = "vdd33";
2603 ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(pdata->supplies),
2604pdata->supplies);
2605 if (ret) {
2606 DRM_DEV_ERROR(dev, "fail to get power supplies: %d\n",
ret);
2607 return ret;
2608 }
2609 anx7625_init_gpio(platform);
2610
2611 mutex_init(>lock);
2612 mutex_init(>hdcp_wq_lock);
2613
2614 INIT_DELAYED_WORK(>hdcp_work, hdcp_check_work_func);
2615 platform->hdcp_workqueue = create_workqueue("hdcp workqueue");
2616 if (!platform->hdcp_workqueue) {
2617 dev_err(dev, "fail to create work queue\n");
2618 ret = -ENOMEM;
2619 return ret;
2620 }
2621
2622 platform->pdata.intp_irq = client->irq;
2623 if (platform->pdata.intp_irq) {
2624 INIT_WORK(>work, anx7625_work_func);
2625 platform->workqueue = alloc_workqueue("anx7625_work",
2626WQ_FREEZABLE |
WQ_MEM_RECLAIM, 1);
2627 if (!platform->workqueue) {
2628 DRM_DEV_ERROR(dev, "fail to create work
queue\n");
2629 ret = -ENOMEM;
2630 goto free_hdcp_wq;
2631 }
2632
2633 ret = devm_request_threaded_irq(dev,
platform->pdata.intp_irq,
2634 NULL,
anx7625_intr_hpd_isr,
2635 IRQF_TRIGGER_FALLING |
2636 IRQF_ONESHOT,
2637 "anx7625-intp",
platform);
2638 if (ret) {
2639 DRM_DEV_ERROR(dev, "fail to request irq\n");
2640
[PATCH] Revert "drm/ast: Support 1600x900 with 108MHz PCLK"
From: Dave Airlie
This reverts commit 9bb7b689274b67ecb3641e399e76f84adc627df1.
This caused a regression reported to Red Hat.
Fixes: 9bb7b689274b ("drm/ast: Support 1600x900 with 108MHz PCLK")
Signed-off-by: Dave Airlie
---
drivers/gpu/drm/ast/ast_tables.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_tables.h b/drivers/gpu/drm/ast/ast_tables.h
index d9eb353a4bf0..dbe1cc620f6e 100644
--- a/drivers/gpu/drm/ast/ast_tables.h
+++ b/drivers/gpu/drm/ast/ast_tables.h
@@ -282,8 +282,6 @@ static const struct ast_vbios_enhtable res_1360x768[] = {
};
static const struct ast_vbios_enhtable res_1600x900[] = {
- {1800, 1600, 24, 80, 1000, 900, 1, 3, VCLK108, /* 60Hz */
-(SyncPP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo),
60, 3, 0x3A },
{1760, 1600, 48, 32, 926, 900, 3, 5, VCLK97_75, /* 60Hz CVT RB
*/
(SyncNP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo |
AST2500PreCatchCRT), 60, 1, 0x3A },
--
2.25.4
Re: [PATCH v3 01/10] mm: add zone device coherent type memory support
On Tuesday, 11 January 2022 9:31:52 AM AEDT 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
> ---
> include/linux/memremap.h | 8
> include/linux/mm.h | 16
> mm/memcontrol.c | 6 +++---
> mm/memory-failure.c | 8 ++--
> mm/memremap.c| 5 -
> mm/migrate.c | 21 +
> 6 files changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index c0e9d35889e8..ff4d398edf35 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -39,6 +39,13 @@ struct vmem_altmap {
> * A more complete discussion of unaddressable memory may be found in
> * include/linux/hmm.h and Documentation/vm/hmm.rst.
> *
> + * MEMORY_DEVICE_COHERENT:
> + * 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).
> A
> + * driver can hotplug the device memory using ZONE_DEVICE and with that
> memory
> + * type. 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.
> + *
> * MEMORY_DEVICE_FS_DAX:
> * Host memory that has similar access semantics as System RAM i.e. DMA
> * coherent and supports page pinning. In support of coordinating page
> @@ -59,6 +66,7 @@ struct vmem_altmap {
> enum memory_type {
> /* 0 is reserved to catch uninitialized type fields */
> MEMORY_DEVICE_PRIVATE = 1,
> + MEMORY_DEVICE_COHERENT,
> MEMORY_DEVICE_FS_DAX,
> MEMORY_DEVICE_GENERIC,
> MEMORY_DEVICE_PCI_P2PDMA,
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 73a52aba448f..fcf96c0fc918 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1162,6 +1162,7 @@ static inline bool page_is_devmap_managed(struct page
> *page)
> return false;
> switch (page->pgmap->type) {
> case MEMORY_DEVICE_PRIVATE:
> + case MEMORY_DEVICE_COHERENT:
> case MEMORY_DEVICE_FS_DAX:
> return true;
> default:
> @@ -1191,6 +1192,21 @@ static inline bool is_device_private_page(const struct
> page *page)
> page->pgmap->type == MEMORY_DEVICE_PRIVATE;
> }
>
> +static inline bool is_device_coherent_page(const struct page *page)
> +{
> + return IS_ENABLED(CONFIG_DEV_PAGEMAP_OPS) &&
> + is_zone_device_page(page) &&
> + page->pgmap->type == MEMORY_DEVICE_COHERENT;
> +}
> +
> +static inline bool is_device_page(const struct page *page)
I wish we could think of a better name for this - it's too similar to
is_zone_device_page() so I can never remember if it includes FS_DAX pages or
not. Unfortunately I don't have any better suggestions though.
> +{
> + return IS_ENABLED(CONFIG_DEV_PAGEMAP_OPS) &&
> + is_zone_device_page(page) &&
> + (page->pgmap->type == MEMORY_DEVICE_PRIVATE ||
> + page->pgmap->type == MEMORY_DEVICE_COHERENT);
> +}
> +
> static inline bool is_pci_p2pdma_page(const struct page *page)
> {
> return IS_ENABLED(CONFIG_DEV_PAGEMAP_OPS) &&
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 6da5020a8656..d0bab0747c73 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5695,8 +5695,8 @@ static int mem_cgroup_move_account(struct page *page,
> * 2(MC_TARGET_SWAP): if the swap entry corresponding to this pte is a
> * target for charge migration. if @target is not NULL, the entry is
> stored
> * in target->ent.
> - * 3(MC_TARGET_DEVICE): like MC_TARGET_PAGE but page is
> MEMORY_DEVICE_PRIVATE
> - * (so ZONE_DEVICE page and thus not on the lru).
> + * 3(MC_TARGET_DEVICE): like MC_TARGET_PAGE but page is device memory and
> + * thus not on the lru.
> * For now we such page is charge like a regular page would be as for all
> * intent and purposes it is just special memory taking the place of a
> * regular page.
> @@ -5730,7 +5730,7 @@ static enum mc_target_type get_mctgt_type(struct
> vm_area_struct *vma,
>*/
> if (page_memcg(page) == mc.from) {
> ret = MC_TARGET_PAGE;
> - if (is_device_private_page(page))
> + if (is_device_page(page))
> ret = MC_TARGET_DEVICE;
> if (target)
> target->page = page;
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 3e6449f2102a..4cf212e5f432 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1554,12 +1554,16 @@ static int
Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions
On Wed, 2022-01-19 at 17:36 +0100, Javier Martinez Canillas wrote: > On 1/19/22 16:50, Thomas Zimmermann wrote: > > [snip] > > > > > > IMHO the best solution is to drop IORESOURCE_BUSY from sysfb > > > > > and have > > > > > drivers register/release the range with _BUSY. That would > > > > > signal the > > > > > memory belongs to the sysfb device but is not busy unless a > > > > > driver > > > > > has > > > > > been bound. After simplefb released the range, it should be > > > > > 'non- > > > > > busy' > > > > > again and available for vmwgfx. Simpledrm does a hot-unplug of > > > > > the > > > > > sysfb > > > > > device, so the memory range gets released entirely. If you > > > > > want, I'll > > > > > prepare some patches for this scenario. > > > > > > > > Attached is a patch that implements this. Doing > > > > > > > > cat /proc/iomem > > > > ... > > > > e000-efff : :00:02.0 > > > > > > > > e000-e07e8fff : BOOTFB > > > > > > > > e000-e07e8fff : simplefb > > > > > > > > ... > > > > > > > > shows the memory. 'BOOTFB' is the simple-framebuffer device and > > > > 'simplefb' is the driver. Only the latter uses _BUSY. Same for > > > > and the memory canbe acquired by vmwgfx. > > > > > > > > Zack, please test this patch. If it works, I'll send out the real > > > > patchset. > > > > > > Hmm, the patch looks good but it doesn't work. After boot: > > > /proc/iomem > > > 5000-7fff : pcie@0x4000 > > > 7800-7fff : :00:0f.0 > > > 7800-782f : BOOTFB > > > > > > and vmwgfx fails on pci_request_regions: > > > > > > kernel: fb0: switching to vmwgfx from simple > > > kernel: Console: switching to colour dummy device 80x25 > > > kernel: vmwgfx :00:0f.0: BAR 2: can't reserve [mem 0x7800- > > > 0x7fff 64bit pref] > > > kernel: vmwgfx: probe of :00:0f.0 failed with error -16 > > > > > > leaving the system without a fb driver. > > > > OK, I suspect that it would work if you use simpledrm instead of > > simplefb. Could you try please? You'd have to build DRM and simpledrm > > into the kernel binary. > > > > Yes, I believe that should work. > Zack, could you please try if just the following [0] make it works ? Unfortunately that still fails with the same: kernel: fb0: switching to vmwgfx from simple kernel: Console: switching to colour dummy device 80x25 kernel: vmwgfx :00:0f.0: BAR 2: can't reserve [mem 0x7800- 0x7fff 64bit pref] kernel: vmwgfx: probe of :00:0f.0 failed with error -16 z
Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions
On Wed, 2022-01-19 at 16:50 +0100, Thomas Zimmermann wrote: > Hi > > Am 19.01.22 um 16:12 schrieb Zack Rusin: > > On Wed, 2022-01-19 at 15:00 +0100, Thomas Zimmermann wrote: > > > Hi Zack > > > > > > Am 19.01.22 um 10:13 schrieb Thomas Zimmermann: > > > > Hi > > > > > > > > Am 19.01.22 um 03:15 schrieb Zack Rusin: > > > > > On Tue, 2022-01-18 at 20:00 +0100, Javier Martinez Canillas > > > > > wrote: > > > > > > Hello Zack, > > > > > > > > > > > > On 1/17/22 19:03, Zack Rusin wrote: > > > > > > > From: Zack Rusin > > > > > > > > > > > > > > When sysfb_simple is enabled loading vmwgfx fails because > > > > > > > the > > > > > > > regions > > > > > > > are held by the platform. In that case > > > > > > > remove_conflicting*_framebuffers > > > > > > > only removes the simplefb but not the regions held by > > > > > > > sysfb. > > > > > > > > > > > > > > > > > > > Indeed, that's an issue. I wonder if we should drop the > > > > > > IORESOURCE_BUSY > > > > > > flag from the memory resource added to the "simple- > > > > > > framebuffer" > > > > > > device > > > > > > ? > > > > > > > > > > I think this is one of those cases where it depends on what > > > > > we plan > > > > > to > > > > > do after that. Sementically it makes sense to have it in > > > > > there - > > > > > the > > > > > framebuffer memory is claimed by the "simple-framebuffer" and > > > > > it's > > > > > busy, it's just that it creates issues for drivers after > > > > > unloading. > > > > > I > > > > > think removing it, while making things easier for drivers, > > > > > would be > > > > > confusing for people reading the code later, unless there's > > > > > some > > > > > kind > > > > > of cleanup that would happen with it (e.g. removing > > > > > IORESOURCE_BUSY > > > > > altogether and making the drm drivers properly request their > > > > > resources). At least by itself it doesn't seem to be much > > > > > better > > > > > solution than having the drm drivers not call > > > > > pci_request_region[s], > > > > > which apart from hyperv and cirrus (iirc bochs does it for > > > > > resources > > > > > other than fb which wouldn't have been claimed by "simple- > > > > > framebuffer") > > > > > is already the case. > > > > > > > > > > I do think we should do one of them to make the codebase > > > > > coherent: > > > > > either remove IORESOURCE_BUSY from "simple-framebuffer" or > > > > > remove > > > > > pci_request_region[s] from hyperv and cirrus. > > > > > > > > I just discussed this a bit with Javier. It's a problem with > > > > the > > > > simple-framebuffer code, rather then vmwgfx. > > > > > > > > IMHO the best solution is to drop IORESOURCE_BUSY from sysfb > > > > and have > > > > drivers register/release the range with _BUSY. That would > > > > signal the > > > > memory belongs to the sysfb device but is not busy unless a > > > > driver > > > > has > > > > been bound. After simplefb released the range, it should be > > > > 'non- > > > > busy' > > > > again and available for vmwgfx. Simpledrm does a hot-unplug of > > > > the > > > > sysfb > > > > device, so the memory range gets released entirely. If you > > > > want, I'll > > > > prepare some patches for this scenario. > > > > > > Attached is a patch that implements this. Doing > > > > > > cat /proc/iomem > > > ... > > > e000-efff : :00:02.0 > > > > > > e000-e07e8fff : BOOTFB > > > > > > e000-e07e8fff : simplefb > > > > > > ... > > > > > > shows the memory. 'BOOTFB' is the simple-framebuffer device and > > > 'simplefb' is the driver. Only the latter uses _BUSY. Same for > > > and the memory canbe acquired by vmwgfx. > > > > > > Zack, please test this patch. If it works, I'll send out the real > > > patchset. > > > > Hmm, the patch looks good but it doesn't work. After boot: > > /proc/iomem > > 5000-7fff : pcie@0x4000 > > 7800-7fff : :00:0f.0 > > 7800-782f : BOOTFB > > > > and vmwgfx fails on pci_request_regions: > > > > kernel: fb0: switching to vmwgfx from simple > > kernel: Console: switching to colour dummy device 80x25 > > kernel: vmwgfx :00:0f.0: BAR 2: can't reserve [mem 0x7800- > > 0x7fff 64bit pref] > > kernel: vmwgfx: probe of :00:0f.0 failed with error -16 > > > > leaving the system without a fb driver. > > OK, I suspect that it would work if you use simpledrm instead of > into the kernel binary. Yes, simpledrm works fine. BTW, is there any remaining work before distros can enable it by default? > > If that works, would you consider protecting pci_request_region() > with > #if not defined(CONFIG_FB_SIMPLE) > #endif > > with a FIXME comment? Yes, I think that's a good compromise. I'll respin the patch with that. z
Re: [PATCH v4] dma-buf: system_heap: Add a size check for allocation
On Wed, Jan 19, 2022 at 7:34 PM wrote: > > From: Guangming > > Add a size check for allocation since the allocation size should be > always less than the total DRAM size on system heap. > And it can prevent consuming too much time for invalid allocations. > > Signed-off-by: Guangming > --- > drivers/dma-buf/heaps/system_heap.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/dma-buf/heaps/system_heap.c > b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..bd6f255620e2 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -347,6 +347,13 @@ static struct dma_buf *system_heap_allocate(struct > dma_heap *heap, > struct page *page, *tmp_page; > int i, ret = -ENOMEM; > > + /* > +* Size check. The "len" should be less than totalram since > system_heap > +* memory is comes from system. Adding check here can prevent > consuming > +* too much time for invalid allocations. > +*/ > + if (len >> PAGE_SHIFT > totalram_pages()) > + return -EINVAL; Thanks so much for revising and sending this along! It looks good to me. Acked-by: John Stultz thanks again -john
[PATCH v4] dma-buf: system_heap: Add a size check for allocation
From: Guangming Add a size check for allocation since the allocation size should be always less than the total DRAM size on system heap. And it can prevent consuming too much time for invalid allocations. Signed-off-by: Guangming --- drivers/dma-buf/heaps/system_heap.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..bd6f255620e2 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -347,6 +347,13 @@ static struct dma_buf *system_heap_allocate(struct dma_heap *heap, struct page *page, *tmp_page; int i, ret = -ENOMEM; + /* +* Size check. The "len" should be less than totalram since system_heap +* memory is comes from system. Adding check here can prevent consuming +* too much time for invalid allocations. +*/ + if (len >> PAGE_SHIFT > totalram_pages()) + return -EINVAL; buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); if (!buffer) return ERR_PTR(-ENOMEM); -- 2.17.1
Re: [PATCH v5 3/4] drm/bridge: anx7625: Support reading edid through aux channel
Hi Hsin-Yi, Thank you for the patch! Yet something to improve: [auto build test ERROR on drm-tip/drm-tip] [cannot apply to drm/drm-next robh/for-next drm-intel/for-linux-next v5.16 next-20220118] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Hsin-Yi-Wang/drm-bridge-anx7625-send-DPCD-command-to-downstream/20220119-231952 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: arm-randconfig-c002-20220118 (https://download.01.org/0day-ci/archive/20220120/202201200932.s1r1twum-...@intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/61809df270082584886188b067ee19744f4b35e1 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Hsin-Yi-Wang/drm-bridge-anx7625-send-DPCD-command-to-downstream/20220119-231952 git checkout 61809df270082584886188b067ee19744f4b35e1 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/analogix/anx7625.o: in function `anx7625_i2c_probe': >> anx7625.c:(.text+0x1c10): undefined reference to >> `devm_of_dp_aux_populate_ep_devices' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: linux-next: build warning after merge of the drm-misc tree
Hi all,
On Wed, 17 Nov 2021 13:49:26 +1100 Stephen Rothwell
wrote:
>
> After merging the drm-misc tree, today's linux-next build (htmldocs)
> produced this warning:
>
> include/drm/gpu_scheduler.h:316: warning: Function parameter or member 'work'
> not described in 'drm_sched_job'
>
> Introduced by commit
>
> 542cff7893a3 ("drm/sched: Avoid lockdep spalt on killing a processes")
I am still seeing this warning.
--
Cheers,
Stephen Rothwell
pgpdTNL5jQkoF.pgp
Description: OpenPGP digital signature
Re: linux-next: build warning after merge of the drm-misc tree
Hi all,
On Fri, 15 Oct 2021 21:01:04 +1100 Stephen Rothwell
wrote:
>
> After merging the drm-misc tree, today's linux-next build (htmldocs)
> produced this warning:
>
> include/drm/drm_modeset_lock.h:74: warning: Function parameter or member
> 'stack_depot' not described in 'drm_modeset_acquire_ctx'
>
> Introduced by commit
>
> cd06ab2fd48f ("drm/locking: add backtrace for locking contended locks
> without backoff")
I am still getting this warning.
--
Cheers,
Stephen Rothwell
pgp1WXuktxO87.pgp
Description: OpenPGP digital signature
Re: linux-next: build warning after merge of the drm-misc tree
Hi all,
On Fri, 15 Oct 2021 20:54:22 +1100 Stephen Rothwell
wrote:
>
> After merging the drm-misc tree, today's linux-next build (htmldocs)
> produced this warning:
>
> Documentation/gpu/drm-kms-helpers:451:
> /home/sfr/next/next/drivers/gpu/drm/drm_privacy_screen.c:270: WARNING: Inline
> emphasis start-string without end-string.
>
> Introduced by commit
>
> 8a12b170558a ("drm/privacy-screen: Add notifier support (v2)")
I am still getting this warning.
--
Cheers,
Stephen Rothwell
pgp4pmWFdj3WJ.pgp
Description: OpenPGP digital signature
Re: [PATCH v2] phy: dphy: Correct clk_pre parameter
Hi Heiko, Wyon,
On Wed, 2022-01-19 at 10:37 +0800, Liu Ying wrote:
> The D-PHY specification (v1.2) explicitly mentions that the T-CLK-PRE
> parameter's unit is Unit Interval(UI) and the minimum value is 8. Also,
> kernel doc of the 'clk_pre' member of struct phy_configure_opts_mipi_dphy
> mentions that it should be in UI. However, the dphy core driver wrongly
> sets 'clk_pre' to 8000, which seems to hint that it's in picoseconds.
> And, the kernel doc of the 'clk_pre' member wrongly says the minimum value
> is '8 UI', instead of 8.
>
> So, let's fix both the dphy core driver and the kernel doc of the 'clk_pre'
> member to correctly reflect the T-CLK-PRE parameter's unit and the minimum
> value according to the D-PHY specification.
>
> I'm assuming that all impacted custom drivers shall program values in
> TxByteClkHS cycles into hardware for the T-CLK-PRE parameter. The D-PHY
> specification mentions that the frequency of TxByteClkHS is exactly 1/8
> the High-Speed(HS) bit rate(each HS bit consumes one UI). So, relevant
> custom driver code is changed to program those values as
> DIV_ROUND_UP(cfg->clk_pre, BITS_PER_BYTE), then.
>
> Note that I've only tested the patch with RM67191 DSI panel on i.MX8mq EVK.
> Help is needed to test with other i.MX8mq, Meson and Rockchip platforms,
> as I don't have the hardwares.
>
> Fixes: 2ed869990e14 ("phy: Add MIPI D-PHY configuration options")
> Cc: Andrzej Hajda
> Cc: Neil Armstrong
> Cc: Robert Foss
> Cc: Laurent Pinchart
> Cc: Jonas Karlman
> Cc: Jernej Skrabec
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Kishon Vijay Abraham I
> Cc: Vinod Koul
> Cc: Kevin Hilman
> Cc: Jerome Brunet
> Cc: Martin Blumenstingl
> Cc: Heiko Stuebner
> Cc: Maxime Ripard
> Cc: Guido Günther
> Tested-by: Liu Ying # RM67191 DSI panel on i.MX8mq EVK
> Signed-off-by: Liu Ying
> ---
> v1->v2:
> * Use BITS_PER_BYTE macro. (Andrzej)
> * Drop dsi argument from ui2bc() in nwl-dsi.c.
>
> drivers/gpu/drm/bridge/nwl-dsi.c | 12 +---
> drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c| 3 ++-
> drivers/phy/phy-core-mipi-dphy.c | 4 ++--
> drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c | 3 ++-
> include/linux/phy/phy-mipi-dphy.h| 2 +-
> 5 files changed, 12 insertions(+), 12 deletions(-)
[...]
> diff --git a/drivers/phy/phy-core-mipi-dphy.c
> b/drivers/phy/phy-core-mipi-dphy.c
> index 288c9c67aa74..ccb4045685cd 100644
> --- a/drivers/phy/phy-core-mipi-dphy.c
> +++ b/drivers/phy/phy-core-mipi-dphy.c
> @@ -36,7 +36,7 @@ int phy_mipi_dphy_get_default_config(unsigned long
> pixel_clock,
>
> cfg->clk_miss = 0;
> cfg->clk_post = 6 + 52 * ui;
> - cfg->clk_pre = 8000;
> + cfg->clk_pre = 8;
> cfg->clk_prepare = 38000;
> cfg->clk_settle = 95000;
> cfg->clk_term_en = 0;
> @@ -97,7 +97,7 @@ int phy_mipi_dphy_config_validate(struct
> phy_configure_opts_mipi_dphy *cfg)
> if (cfg->clk_post < (6 + 52 * ui))
> return -EINVAL;
>
> - if (cfg->clk_pre < 8000)
> + if (cfg->clk_pre < 8)
> return -EINVAL;
>
> if (cfg->clk_prepare < 38000 || cfg->clk_prepare > 95000)
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> index 347dc79a18c1..630e01b5c19b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> @@ -5,6 +5,7 @@
> * Author: Wyon Bi
> */
>
> +#include
> #include
> #include
> #include
> @@ -364,7 +365,7 @@ static void inno_dsidphy_mipi_mode_enable(struct
> inno_dsidphy *inno)
>* The value of counter for HS Tclk-pre
>* Tclk-pre = Tpin_txbyteclkhs * value
>*/
> - clk_pre = DIV_ROUND_UP(cfg->clk_pre, t_txbyteclkhs);
> + clk_pre = DIV_ROUND_UP(cfg->clk_pre, BITS_PER_BYTE);
For the Rockchip part, can you please give a test? Any comments?
We already have T-b tags on i.MX8mq and Meson platforms.
Regards,
Liu Ying
>
> /*
>* The value of counter for HS Tlpx Time
> diff --git a/include/linux/phy/phy-mipi-dphy.h
> b/include/linux/phy/phy-mipi-dphy.h
> index a877ffee845d..59a5e77ab409 100644
> --- a/include/linux/phy/phy-mipi-dphy.h
> +++ b/include/linux/phy/phy-mipi-dphy.h
> @@ -42,7 +42,7 @@ struct phy_configure_opts_mipi_dphy {
>* the transmitter prior to any associated Data Lane beginning
>* the transition from LP to HS mode.
>*
> - * Minimum value: 8 UI
> + * Minimum value: 8
>*/
> unsigned intclk_pre;
>
Re: [PATCH v3 2/3] drm/msm/dpu: simplify clocks handling
On 1/19/2022 2:16 PM, Dmitry Baryshkov wrote:
DPU driver contains code to parse clock items from device tree into
special data struct and then enable/disable/set rate for the clocks
using that data struct. However the DPU driver itself uses only parsing
and enabling/disabling part (the rate setting is used by DP driver).
Move this implementation to the DP driver (which actually uses rate
setting) and replace hand-coded enable/disable/get loops in the DPU
with the respective clk_bulk operations. Put operation is removed
completely because, it is handled using devres instead.
DP implementation is unchanged for now.
Signed-off-by: Dmitry Baryshkov
Tested on: Qualcomm RB3 (debian, sdm845), Qualcomm RB5 (debian, qrb5165)
Reviewed-by: Jessica Zhang
---
drivers/gpu/drm/msm/Makefile | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 24 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 46 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 26 +++
.../dpu1/dpu_io_util.c => dp/dp_clk_util.c} | 69 +--
.../dpu1/dpu_io_util.h => dp/dp_clk_util.h} | 8 +--
drivers/gpu/drm/msm/dp/dp_parser.h| 2 +-
drivers/gpu/drm/msm/msm_drv.c | 1 +
drivers/gpu/drm/msm/msm_drv.h | 1 +
drivers/gpu/drm/msm/msm_io_utils.c| 50 ++
12 files changed, 89 insertions(+), 150 deletions(-)
rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.c => dp/dp_clk_util.c} (61%)
rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.h => dp/dp_clk_util.h} (85%)
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 5b6e37477079..e9cc7d8ac301 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -66,7 +66,6 @@ msm-y := \
disp/dpu1/dpu_hw_top.o \
disp/dpu1/dpu_hw_util.o \
disp/dpu1/dpu_hw_vbif.o \
- disp/dpu1/dpu_io_util.o \
disp/dpu1/dpu_kms.o \
disp/dpu1/dpu_mdss.o \
disp/dpu1/dpu_plane.o \
@@ -103,6 +102,7 @@ msm-$(CONFIG_DRM_MSM_GPU_STATE) +=
adreno/a6xx_gpu_state.o
msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
dp/dp_catalog.o \
+ dp/dp_clk_util.o \
dp/dp_ctrl.o \
dp/dp_display.o \
dp/dp_drm.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 60fe06018581..4d184122d63e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -284,17 +284,6 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
}
}
-static int _dpu_core_perf_set_core_clk_rate(struct dpu_kms *kms, u64 rate)
-{
- struct dss_clk *core_clk = kms->perf.core_clk;
-
- if (core_clk->max_rate && (rate > core_clk->max_rate))
- rate = core_clk->max_rate;
-
- core_clk->rate = rate;
- return dev_pm_opp_set_rate(>pdev->dev, core_clk->rate);
-}
-
static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
{
u64 clk_rate = kms->perf.perf_tune.min_core_clk;
@@ -306,7 +295,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms
*kms)
dpu_cstate = to_dpu_crtc_state(crtc->state);
clk_rate = max(dpu_cstate->new_perf.core_clk_rate,
clk_rate);
- clk_rate = clk_round_rate(kms->perf.core_clk->clk,
+ clk_rate = clk_round_rate(kms->perf.core_clk,
clk_rate);
}
}
@@ -405,10 +394,11 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
- ret = _dpu_core_perf_set_core_clk_rate(kms, clk_rate);
+ if (clk_rate > kms->perf.max_core_clk_rate)
+ clk_rate = kms->perf.max_core_clk_rate;
+ ret = dev_pm_opp_set_rate(>pdev->dev, clk_rate);
if (ret) {
- DPU_ERROR("failed to set %s clock rate %llu\n",
- kms->perf.core_clk->clk_name, clk_rate);
+ DPU_ERROR("failed to set core clock rate %llu\n",
clk_rate);
return ret;
}
@@ -529,13 +519,13 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
int dpu_core_perf_init(struct dpu_core_perf *perf,
struct drm_device *dev,
struct dpu_mdss_cfg *catalog,
- struct dss_clk *core_clk)
+ struct clk *core_clk)
{
perf->dev = dev;
perf->catalog = catalog;
perf->core_clk = core_clk;
- perf->max_core_clk_rate = core_clk->max_rate;
+ perf->max_core_clk_rate = clk_get_rate(core_clk);
if
Re: [Freedreno] [PATCH v3 1/3] drm/msm: move utility functions from msm_drv.c
On 1/19/2022 2:16 PM, Dmitry Baryshkov wrote:
Move clock/IO/hrtimer utility functions from msm_drv.c to new
msm_io_utils.c file.
Signed-off-by: Dmitry Baryshkov
Tested on: Qualcomm RB3 (debian, sdm845), Qualcomm RB5 (debian, qrb5165)
Reviewed-by: Jessica Zhang
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/msm_drv.c | 118 ---
drivers/gpu/drm/msm/msm_io_utils.c | 126 +
3 files changed, 127 insertions(+), 118 deletions(-)
create mode 100644 drivers/gpu/drm/msm/msm_io_utils.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 03ab55c37beb..5b6e37477079 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -87,6 +87,7 @@ msm-y := \
msm_gem_vma.o \
msm_gpu.o \
msm_gpu_devfreq.o \
+ msm_io_utils.o \
msm_iommu.o \
msm_perf.o \
msm_rd.o \
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index fd62a4da14a1..30c44c395a24 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -75,124 +75,6 @@ static bool modeset = true;
MODULE_PARM_DESC(modeset, "Use kernel modesetting [KMS] (1=on (default),
0=disable)");
module_param(modeset, bool, 0600);
-/*
- * Util/helpers:
- */
-
-struct clk *msm_clk_bulk_get_clock(struct clk_bulk_data *bulk, int count,
- const char *name)
-{
- int i;
- char n[32];
-
- snprintf(n, sizeof(n), "%s_clk", name);
-
- for (i = 0; bulk && i < count; i++) {
- if (!strcmp(bulk[i].id, name) || !strcmp(bulk[i].id, n))
- return bulk[i].clk;
- }
-
-
- return NULL;
-}
-
-struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
-{
- struct clk *clk;
- char name2[32];
-
- clk = devm_clk_get(>dev, name);
- if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
- return clk;
-
- snprintf(name2, sizeof(name2), "%s_clk", name);
-
- clk = devm_clk_get(>dev, name2);
- if (!IS_ERR(clk))
- dev_warn(>dev, "Using legacy clk name binding. Use "
- "\"%s\" instead of \"%s\"\n", name, name2);
-
- return clk;
-}
-
-static void __iomem *_msm_ioremap(struct platform_device *pdev, const char
*name,
- bool quiet, phys_addr_t *psize)
-{
- struct resource *res;
- unsigned long size;
- void __iomem *ptr;
-
- if (name)
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
- else
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
- if (!res) {
- if (!quiet)
- DRM_DEV_ERROR(>dev, "failed to get memory resource:
%s\n", name);
- return ERR_PTR(-EINVAL);
- }
-
- size = resource_size(res);
-
- ptr = devm_ioremap(>dev, res->start, size);
- if (!ptr) {
- if (!quiet)
- DRM_DEV_ERROR(>dev, "failed to ioremap: %s\n",
name);
- return ERR_PTR(-ENOMEM);
- }
-
- if (psize)
- *psize = size;
-
- return ptr;
-}
-
-void __iomem *msm_ioremap(struct platform_device *pdev, const char *name)
-{
- return _msm_ioremap(pdev, name, false, NULL);
-}
-
-void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name)
-{
- return _msm_ioremap(pdev, name, true, NULL);
-}
-
-void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
- phys_addr_t *psize)
-{
- return _msm_ioremap(pdev, name, false, psize);
-}
-
-static enum hrtimer_restart msm_hrtimer_worktimer(struct hrtimer *t)
-{
- struct msm_hrtimer_work *work = container_of(t,
- struct msm_hrtimer_work, timer);
-
- kthread_queue_work(work->worker, >work);
-
- return HRTIMER_NORESTART;
-}
-
-void msm_hrtimer_queue_work(struct msm_hrtimer_work *work,
- ktime_t wakeup_time,
- enum hrtimer_mode mode)
-{
- hrtimer_start(>timer, wakeup_time, mode);
-}
-
-void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
- struct kthread_worker *worker,
- kthread_work_func_t fn,
- clockid_t clock_id,
- enum hrtimer_mode mode)
-{
- hrtimer_init(>timer, clock_id, mode);
- work->timer.function = msm_hrtimer_worktimer;
- work->worker = worker;
- kthread_init_work(>work, fn);
-}
-
static irqreturn_t msm_irq(int irq, void *arg)
{
struct drm_device *dev = arg;
diff --git a/drivers/gpu/drm/msm/msm_io_utils.c
b/drivers/gpu/drm/msm/msm_io_utils.c
new file mode 100644
index ..7b504617833a
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_io_utils.c
@@ -0,0 +1,126 @@
+//
[pull] amdgpu, radeon drm-fixes-5.17
Hi Dave, Daniel, Fixes for 5.17. This does not yet include the proper fix for the patch Linux reverted when he pulled in the original drm merge windows PR. I was waiting for rc1. At that point do you plan to merge rc1 into drm-fixes? If so, I can just drop the fix on top. 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-fixes-5.17-2022-01-19 for you to fetch changes up to 4722f463896cc0ef1a6f1c3cb2e171e949831249: drm/radeon: fix error handling in radeon_driver_open_kms (2022-01-18 18:00:58 -0500) amd-drm-fixes-5.17-2022-01-19: amdgpu: - SR-IOV fix - VCN harvest fix - Suspend/resume fixes - Tahiti fix - Enable GPU recovery on yellow carp radeon: - Fix error handling regression in radeon_driver_open_kms Alex Deucher (3): 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 CHANDAN VURDIGERE NATARAJ (1): drm/amdgpu: Enable recovery on yellow carp Christian König (1): drm/radeon: fix error handling in radeon_driver_open_kms Guchun Chen (1): drm/amdgpu: apply vcn harvest quirk Jingwen Chen (1): drm/amd/amdgpu: fixing read wrong pf2vf data in SRIOV Lukas Fink (1): drm/amdgpu: Fix rejecting Tahiti GPUs Mario Limonciello (1): drm/amd/display: Revert W/A for hard hangs on DCN20/DCN21 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 45 +- drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 32 --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 5 --- drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 20 -- drivers/gpu/drm/amd/amdgpu/cik.c | 4 ++ drivers/gpu/drm/amd/amdgpu/vi.c| 4 ++ .../amd/display/dc/clk_mgr/dcn20/dcn20_clk_mgr.c | 11 +- .../drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c | 11 +- .../amd/display/dc/irq/dcn20/irq_service_dcn20.c | 25 .../amd/display/dc/irq/dcn20/irq_service_dcn20.h | 2 - .../amd/display/dc/irq/dcn21/irq_service_dcn21.c | 25 .../amd/display/dc/irq/dcn21/irq_service_dcn21.h | 2 - drivers/gpu/drm/amd/display/dc/irq/irq_service.c | 2 +- drivers/gpu/drm/amd/display/dc/irq/irq_service.h | 4 -- drivers/gpu/drm/radeon/radeon_kms.c| 22 ++- 15 files changed, 75 insertions(+), 139 deletions(-)
Re: [PATCH 3/3] drm: fix the warnning of string style for scheduler trace.
Ping~
Seems this patch is missed.
Thanks,
Ray
On Mon, Dec 13, 2021 at 02:34:22PM +0800, Huang, Ray wrote:
> Use __string(), __assign_str() and __get_str() helpers in the TRACE_EVENT()
> instead of string definitions in gpu scheduler trace.
>
> [ 158.890890] [ cut here ]
> [ 158.890899] fmt: 'entity=%p, id=%llu, fence=%p, ring=%s, job count:%u, hw
> job count:%d
>' current_buffer: 'Xorg-1588[001] .
> 149.391136: drm_sched_job: entity=76f0d517, id=1,
> fence=8dd56028, ring='
> [ 158.890910] WARNING: CPU: 6 PID: 1617 at kernel/trace/trace.c:3830
> trace_check_vprintf+0x481/0x4a0
>
> Signed-off-by: Huang Rui
> ---
> drivers/gpu/drm/scheduler/gpu_scheduler_trace.h | 17 +
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> index 877ce9b127f1..4e397790c195 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> @@ -38,6 +38,7 @@ TRACE_EVENT(drm_sched_job,
> TP_STRUCT__entry(
>__field(struct drm_sched_entity *, entity)
>__field(struct dma_fence *, fence)
> + __string(name, sched_job->sched->name)
>__field(const char *, name)
>__field(uint64_t, id)
>__field(u32, job_count)
> @@ -48,14 +49,14 @@ TRACE_EVENT(drm_sched_job,
> __entry->entity = entity;
> __entry->id = sched_job->id;
> __entry->fence = _job->s_fence->finished;
> -__entry->name = sched_job->sched->name;
> +__assign_str(name, sched_job->sched->name);
> __entry->job_count =
> spsc_queue_count(>job_queue);
> __entry->hw_job_count = atomic_read(
> _job->sched->hw_rq_count);
> ),
> TP_printk("entity=%p, id=%llu, fence=%p, ring=%s, job count:%u, hw
> job count:%d",
> __entry->entity, __entry->id,
> - __entry->fence, __entry->name,
> + __entry->fence, __get_str(name),
> __entry->job_count, __entry->hw_job_count)
> );
>
> @@ -65,7 +66,7 @@ TRACE_EVENT(drm_run_job,
> TP_STRUCT__entry(
>__field(struct drm_sched_entity *, entity)
>__field(struct dma_fence *, fence)
> - __field(const char *, name)
> + __string(name, sched_job->sched->name)
>__field(uint64_t, id)
>__field(u32, job_count)
>__field(int, hw_job_count)
> @@ -75,14 +76,14 @@ TRACE_EVENT(drm_run_job,
> __entry->entity = entity;
> __entry->id = sched_job->id;
> __entry->fence = _job->s_fence->finished;
> -__entry->name = sched_job->sched->name;
> +__assign_str(name, sched_job->sched->name);
> __entry->job_count =
> spsc_queue_count(>job_queue);
> __entry->hw_job_count = atomic_read(
> _job->sched->hw_rq_count);
> ),
> TP_printk("entity=%p, id=%llu, fence=%p, ring=%s, job count:%u, hw
> job count:%d",
> __entry->entity, __entry->id,
> - __entry->fence, __entry->name,
> + __entry->fence, __get_str(name),
> __entry->job_count, __entry->hw_job_count)
> );
>
> @@ -103,7 +104,7 @@ TRACE_EVENT(drm_sched_job_wait_dep,
> TP_PROTO(struct drm_sched_job *sched_job, struct dma_fence *fence),
> TP_ARGS(sched_job, fence),
> TP_STRUCT__entry(
> - __field(const char *,name)
> + __string(name, sched_job->sched->name)
>__field(uint64_t, id)
>__field(struct dma_fence *, fence)
>__field(uint64_t, ctx)
> @@ -111,14 +112,14 @@ TRACE_EVENT(drm_sched_job_wait_dep,
>),
>
> TP_fast_assign(
> -__entry->name = sched_job->sched->name;
> +__assign_str(name, sched_job->sched->name);
> __entry->id = sched_job->id;
> __entry->fence = fence;
> __entry->ctx = fence->context;
> __entry->seqno = fence->seqno;
> ),
> TP_printk("job ring=%s, id=%llu, depends fence=%p,
[PATCH] drm/rockchip: vop: Correct RK3399 VOP register fields
Commit 7707f7227f09 ("drm/rockchip: Add support for afbc") switched up
the rk3399_vop_big[] register windows, but it did so incorrectly.
The biggest problem is in rk3288_win23_data[] vs.
rk3368_win23_data[] .format field:
RK3288's format: VOP_REG(RK3288_WIN2_CTRL0, 0x7, 1)
RK3368's format: VOP_REG(RK3368_WIN2_CTRL0, 0x3, 5)
Bits 5:6 (i.e., shift 5, mask 0x3) are correct for RK3399, according to
the TRM.
There are a few other small differences between the 3288 and 3368
definitions that were swapped in commit 7707f7227f09. I reviewed them to
the best of my ability according to the RK3399 TRM and fixed them up.
This fixes IOMMU issues (and display errors) when testing with BG24
color formats.
Fixes: 7707f7227f09 ("drm/rockchip: Add support for afbc")
Cc: Andrzej Pietrasiewicz
Cc:
Signed-off-by: Brian Norris
---
I'd appreciate notes or testing from Andrzej, since I'm not sure how he
tested his original AFBC work.
drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
index 1f7353f0684a..798b542e5916 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -902,6 +902,7 @@ static const struct vop_win_phy rk3399_win01_data = {
.enable = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 0),
.format = VOP_REG(RK3288_WIN0_CTRL0, 0x7, 1),
.rb_swap = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 12),
+ .x_mir_en = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 21),
.y_mir_en = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 22),
.act_info = VOP_REG(RK3288_WIN0_ACT_INFO, 0x1fff1fff, 0),
.dsp_info = VOP_REG(RK3288_WIN0_DSP_INFO, 0x0fff0fff, 0),
@@ -912,6 +913,7 @@ static const struct vop_win_phy rk3399_win01_data = {
.uv_vir = VOP_REG(RK3288_WIN0_VIR, 0x3fff, 16),
.src_alpha_ctl = VOP_REG(RK3288_WIN0_SRC_ALPHA_CTRL, 0xff, 0),
.dst_alpha_ctl = VOP_REG(RK3288_WIN0_DST_ALPHA_CTRL, 0xff, 0),
+ .channel = VOP_REG(RK3288_WIN0_CTRL2, 0xff, 0),
};
/*
@@ -922,11 +924,11 @@ static const struct vop_win_phy rk3399_win01_data = {
static const struct vop_win_data rk3399_vop_win_data[] = {
{ .base = 0x00, .phy = _win01_data,
.type = DRM_PLANE_TYPE_PRIMARY },
- { .base = 0x40, .phy = _win01_data,
+ { .base = 0x40, .phy = _win01_data,
.type = DRM_PLANE_TYPE_OVERLAY },
- { .base = 0x00, .phy = _win23_data,
+ { .base = 0x00, .phy = _win23_data,
.type = DRM_PLANE_TYPE_OVERLAY },
- { .base = 0x50, .phy = _win23_data,
+ { .base = 0x50, .phy = _win23_data,
.type = DRM_PLANE_TYPE_CURSOR },
};
--
2.34.1.703.g22d0c6ccf7-goog
Re: [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions
On Wed, Jan 19, 2022 at 06:24:22PM +0100, Daniel Vetter wrote: > On Wed, Jan 19, 2022 at 08:53:56AM -0800, Ira Weiny wrote: > > On Fri, Dec 10, 2021 at 03:23:57PM -0800, 'Ira Weiny' wrote: > > > From: Ira Weiny > > > > > > This series starts by converting the last easy kmap() uses to > > > kmap_local_page(). > > > > > > There is one more call to kmap() wrapped in ttm_bo_kmap_ttm(). > > > Unfortunately, > > > ttm_bo_kmap_ttm() is called in a number of different ways including some > > > which > > > are not thread local. I have a patch to convert that call. However, it > > > is not > > > straight forward so it is not included in this series. > > > > > > The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm() > > > conversion. > > > > Gentile ping on this series? Will it make this merge window? > > I think this fell through the cracks and so no. Note that generally we > feature-freeze drm tree around -rc6 anyway for the upcoming merge window, > so you were cutting this all a bit close anyway. Ok, No problem. I just had not heard if this was picked up or not. > Also looks like the ttm > kmap caching question didn't get resolved? I'm sorry I thought it was resolve for this series. Christian said the patches in this series were "a good bug fix" even if not strictly necessary.[1] Beyond this series I was discussing where to go from here, and is it possible to go further with more changes.[2] At the moment I don't think I will. Christian did I misunderstand? I can drop patch 6 and 7 if they are not proper bug fixes or at least clarifications to the code. Ira [1] https://lore.kernel.org/lkml/c3b173ea-6509-ebbe-b5f9-eeb29f1ce...@amd.com/ [2] https://lore.kernel.org/lkml/20211215210949.gw3538...@iweiny-desk2.sc.intel.com/ > > Anyway if patches are stuck resend with RESEND and if people still don't > pick them up poke me and I'll apply as fallback. > > Cheers, Daniel > > > > > Thanks, > > Ira > > > > > > > > > > > Ira Weiny (7): > > > drm/i915: Replace kmap() with kmap_local_page() > > > drm/amd: Replace kmap() with kmap_local_page() > > > drm/gma: Remove calls to kmap() > > > drm/radeon: Replace kmap() with kmap_local_page() > > > drm/msm: Alter comment to use kmap_local_page() > > > drm/amdgpu: Ensure kunmap is called on error > > > drm/radeon: Ensure kunmap is called on error > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 > > > drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 + > > > drivers/gpu/drm/gma500/gma_display.c | 6 ++ > > > drivers/gpu/drm/gma500/mmu.c | 8 > > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++-- > > > drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8 > > > drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++-- > > > drivers/gpu/drm/i915/gt/shmem_utils.c | 4 ++-- > > > drivers/gpu/drm/i915/i915_gem.c | 8 > > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > > > drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++-- > > > drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++-- > > > drivers/gpu/drm/radeon/radeon_uvd.c | 1 + > > > 13 files changed, 32 insertions(+), 32 deletions(-) > > > > > > -- > > > 2.31.1 > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Re: [PATCH] drm/msm/hdmi: Fix missing put_device() call in msm_hdmi_get_phy
On 07/01/2022 11:50, Miaoqian Lin wrote:
The reference taken by 'of_find_device_by_node()' must be released when
not needed anymore.
Add the corresponding 'put_device()' in the error handling path.
Fixes: e00012b256d4 ("drm/msm/hdmi: Make HDMI core get its PHY")
Signed-off-by: Miaoqian Lin
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 75b64e6ae035..a439794a32e8 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -95,10 +95,15 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
of_node_put(phy_node);
- if (!phy_pdev || !hdmi->phy) {
+ if (!phy_pdev) {
DRM_DEV_ERROR(>dev, "phy driver is not ready\n");
return -EPROBE_DEFER;
}
+ if (!hdmi->phy) {
+ DRM_DEV_ERROR(>dev, "phy driver is not ready\n");
+ put_device(_pdev->dev);
+ return -EPROBE_DEFER;
+ }
hdmi->phy_dev = get_device(_pdev->dev);
--
With best wishes
Dmitry
Re: [PATCH] drm/msm/dsi: Fix missing put_device() call in dsi_get_phy
On 30/12/2021 10:09, Miaoqian Lin wrote:
If of_find_device_by_node() succeeds, dsi_get_phy() doesn't
a corresponding put_device(). Thus add put_device() to fix the exception
handling.
Fixes: ec31abf ("drm/msm/dsi: Separate PHY to another platform device")
Signed-off-by: Miaoqian Lin
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 75ae3008b68f..35be526e907a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -40,7 +40,12 @@ static int dsi_get_phy(struct msm_dsi *msm_dsi)
of_node_put(phy_node);
- if (!phy_pdev || !msm_dsi->phy) {
+ if (!phy_pdev) {
+ DRM_DEV_ERROR(>dev, "%s: phy driver is not ready\n",
__func__);
+ return -EPROBE_DEFER;
+ }
+ if (!msm_dsi->phy) {
+ put_device(_pdev->dev);
DRM_DEV_ERROR(>dev, "%s: phy driver is not ready\n",
__func__);
return -EPROBE_DEFER;
}
--
With best wishes
Dmitry
Re: [PATCH] drm/msm/dp: Add DisplayPort controller for SM8350
On 28/12/2021 07:59, Bjorn Andersson wrote:
The Qualcomm SM8350 platform comes with a single DisplayPort controller,
add support for this in the DisplayPort driver.
Signed-off-by: Bjorn Andersson
Reviewed-by: Dmitry Baryshkov
---
.../devicetree/bindings/display/msm/dp-controller.yaml| 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 8
2 files changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index 5457612ab136..cd05cfd76536 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -21,6 +21,7 @@ properties:
- qcom,sc7280-edp
- qcom,sc8180x-dp
- qcom,sc8180x-edp
+ - qcom,sm8350-dp
reg:
items:
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 8d9c19dbf33e..fd0fd03f8fed 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -143,10 +143,18 @@ static const struct msm_dp_config sc7280_dp_cfg = {
.num_descs = 2,
};
+static const struct msm_dp_config sm8350_dp_cfg = {
+ .descs = (const struct msm_dp_desc[]) {
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae9,
.connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+ },
+ .num_descs = 1,
+};
+
static const struct of_device_id dp_dt_match[] = {
{ .compatible = "qcom,sc7180-dp", .data = _dp_cfg },
{ .compatible = "qcom,sc7280-dp", .data = _dp_cfg },
{ .compatible = "qcom,sc7280-edp", .data = _dp_cfg },
+ { .compatible = "qcom,sm8350-dp", .data = _dp_cfg },
{}
};
--
With best wishes
Dmitry
Re: [PATCH] dt-bindings: Improve phandle-array schemas
Quoting Rob Herring (2022-01-18 17:50:38) > The 'phandle-array' type is a bit ambiguous. It can be either just an > array of phandles or an array of phandles plus args. Many schemas for > phandle-array properties aren't clear in the schema which case applies > though the description usually describes it. > > The array of phandles case boils down to needing: > > items: > maxItems: 1 > > The phandle plus args cases should typically take this form: > > items: > - items: > - description: A phandle > - description: 1st arg cell > - description: 2nd arg cell > > With this change, some examples need updating so that the bracketing of > property values matches the schema. [..] > Signed-off-by: Rob Herring > --- Acked-by: Stephen Boyd
Re: [PATCH] drm/msm/dpu: Bind pingpong block to intf on active ctls in cmd encoder
On 22/12/2021 13:55, Marijn Suijten wrote:
As per the specification of DPU_CTL_ACTIVE_CFG the configuration of
active blocks should be proactively specified, and the pingpong block is
no different.
The downstream display driver [1] confirms this by also calling
bind_pingpong_blk on CTL_ACTIVE_CFG. Note that this else-if is always
entered, as setup_intf_cfg - unlike this mainline dpu driver that
combines both behind the same function pointer - is left NULL in favour
of using setup_intf_cfg_v1 when CTL_ACTIVE_CFG is set.
This solves continuous timeouts on at least the Qualcomm sm6125 SoC:
[drm:dpu_encoder_frame_done_timeout:2091] [dpu error]enc31 frame done
timeout
[drm:_dpu_encoder_phys_cmd_handle_ppdone_timeout.isra.0] *ERROR* id:31
pp:0 kickoff timeout 0 cnt 1 koff_cnt 1
[drm:dpu_encoder_phys_cmd_prepare_for_kickoff] *ERROR* failed
wait_for_idle: id:31 ret:-110 pp:0
In the same way this pingpong block should also be unbound followed by
an interface flush when the encoder is disabled, according to the
downstream display driver [2].
[1]:
https://source.codeaurora.org/quic/la/platform/vendor/opensource/display-drivers/tree/msm/sde/sde_encoder_phys_cmd.c?h=LA.UM.9.16.r1-08500-MANNAR.0#n167
[2]:
https://source.codeaurora.org/quic/la/platform/vendor/opensource/display-drivers/tree/msm/sde/sde_encoder.c?h=LA.UM.9.16.r1-08500-MANNAR.0#n2986
Signed-off-by: Marijn Suijten
Reviewed-by: AngeloGioacchino Del Regno
Reviewed-by: Dmitry Baryshkov
---
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 21 +++
1 file changed, 21 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 8e433af7aea4..e0e08a874f07 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -71,6 +71,13 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
ctl->ops.setup_intf_cfg(ctl, _cfg);
+
+ /* setup which pp blk will connect to this intf */
+ if (test_bit(DPU_CTL_ACTIVE_CFG, >caps->features) &&
phys_enc->hw_intf->ops.bind_pingpong_blk)
Nit: here we bind all interfaces, but later we unbind only master. Is
this correct?
+ phys_enc->hw_intf->ops.bind_pingpong_blk(
+ phys_enc->hw_intf,
+ true,
+ phys_enc->hw_pp->idx);
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
@@ -507,6 +514,7 @@ static void dpu_encoder_phys_cmd_disable(struct
dpu_encoder_phys *phys_enc)
{
struct dpu_encoder_phys_cmd *cmd_enc =
to_dpu_encoder_phys_cmd(phys_enc);
+ struct dpu_hw_ctl *ctl;
if (!phys_enc->hw_pp) {
DPU_ERROR("invalid encoder\n");
@@ -523,6 +531,19 @@ static void dpu_encoder_phys_cmd_disable(struct
dpu_encoder_phys *phys_enc)
if (phys_enc->hw_pp->ops.enable_tearcheck)
phys_enc->hw_pp->ops.enable_tearcheck(phys_enc->hw_pp, false);
+
+ if (dpu_encoder_phys_cmd_is_master(phys_enc)) {
+ if (phys_enc->hw_intf->ops.bind_pingpong_blk) {
+ phys_enc->hw_intf->ops.bind_pingpong_blk(
+ phys_enc->hw_intf,
+ false,
+ phys_enc->hw_pp->idx);
+
+ ctl = phys_enc->hw_ctl;
+ ctl->ops.update_pending_flush_intf(ctl,
phys_enc->intf_idx);
+ }
+ }
+
phys_enc->enable_state = DPU_ENC_DISABLED;
}
--
With best wishes
Dmitry
[RFC] amdgpu MST questions, and future plans
Hi! I'm writing this email because I'm currently finishing up removing pretty much all of the non-atomic MST code in drm_dp_mst_topology_mgr.c as it's really made it difficult to maintain MST over time. As well, once that's complete it's likely I'm going to start on the (extremely overdue) task of moving as much of amdgpu's MST code for DSC out of amdgpu and into the DRM code where it's supposed to live. This brings me to two questions. The first major one being: is anyone capable of testing the MST support in radeon.ko to figure out whether it works or not? I've already talked with hwentlan and ag5df about this and they haven't been able to find anyone to help with testing this. The reason I ask is because radeon isn't an atomic driver, and is basically the only user of any of the non-atomic parts of the MST helpers. If anyone want to prevent this from breaking in the future, I would definitely recommend they step up to try and help with testing it - otherwise I'm probably going to be pushing for us just to drop the code entirely. The second question is: is anyone willing to help me figure out how much of the code in amdgpu related to DSC is definitely not amdgpu specific and can be moved out? I'm honestly having a lot of trouble wrapping my head around how some of this works, and how much of this code is even needed. As well, with the amount of issues I've already found in it (there's numerous spots where we're storing MST state outside of atomic state for instance, lots of duplicates of DP helper functions that should not be here, etc.) it's quite likely I'm going to be rewriting rather large chunks of it. If anyone would like to volunteer please let me know, it'd be super appreciated and likely will make reviewing the patches that will come out of this easier. -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
Re: [PATCH -next] drm/msm: remove variable set but not used
On 16/12/2021 06:11, Yang Li wrote:
The code that uses variable mdss has been removed, So the declaration
and assignment of the variable can be removed.
Eliminate the following clang warning:
drivers/gpu/drm/msm/msm_drv.c:513:19: warning: variable 'mdss' set but
not used [-Wunused-but-set-variable]
Reported-by: Abaci Robot
Fixes: 2027e5b3413d ("drm/msm: Initialize MDSS irq domain at probe time")
Signed-off-by: Yang Li
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/msm_drv.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index ad35a5d94053..59e30192cdf6 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -510,7 +510,6 @@ static int msm_drm_init(struct device *dev, const struct
drm_driver *drv)
struct msm_drm_private *priv = dev_get_drvdata(dev);
struct drm_device *ddev;
struct msm_kms *kms;
- struct msm_mdss *mdss;
int ret, i;
ddev = drm_dev_alloc(drv, dev);
@@ -521,8 +520,6 @@ static int msm_drm_init(struct device *dev, const struct
drm_driver *drv)
ddev->dev_private = priv;
priv->dev = ddev;
- mdss = priv->mdss;
-
priv->wq = alloc_ordered_workqueue("msm", 0);
priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
--
With best wishes
Dmitry
[PATCH v2 3/4] drm/msm: split the main platform driver
Currently the msm platform driver is a multiplex handling several cases:
- headless GPU-only driver,
- MDP4 with flat device nodes,
- MDP5/DPU MDSS with all the nodes being children of MDSS node.
This results in not-so-perfect code, checking the hardware version
(MDP4/MDP5/DPU) in several places, checking for mdss even when it can
not exist, etc. Split the code into three handling subdrivers (mdp4,
mdss and headless msm).
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 56 ++
drivers/gpu/drm/msm/msm_drv.c| 236 ---
drivers/gpu/drm/msm/msm_drv.h| 19 ++
drivers/gpu/drm/msm/msm_kms.h| 7 -
drivers/gpu/drm/msm/msm_mdss.c | 178 -
5 files changed, 287 insertions(+), 209 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 3cf476c55158..c5c0650414c5 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -569,3 +569,59 @@ static struct mdp4_platform_config *mdp4_get_config(struct
platform_device *dev)
return
}
+
+static const struct dev_pm_ops mdp4_pm_ops = {
+ .prepare = msm_pm_prepare,
+ .complete = msm_pm_complete,
+};
+
+static int mdp4_probe(struct platform_device *pdev)
+{
+ struct msm_drm_private *priv;
+
+ priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
+ /*
+* on MDP4 based platforms, the MDP platform device is the component
+* master that adds other display interface components to itself.
+*/
+ return msm_drv_probe(>dev, >dev);
+}
+
+static int mdp4_remove(struct platform_device *pdev)
+{
+ component_master_del(>dev, _drm_ops);
+
+ return 0;
+}
+
+static const struct of_device_id mdp4_dt_match[] = {
+ { .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mdp4_dt_match);
+
+static struct platform_driver mdp4_platform_driver = {
+ .probe = mdp4_probe,
+ .remove = mdp4_remove,
+ .shutdown = msm_drv_shutdown,
+ .driver = {
+ .name = "mdp4",
+ .of_match_table = mdp4_dt_match,
+ .pm = _pm_ops,
+ },
+};
+
+void __init msm_mdp4_register(void)
+{
+ platform_driver_register(_platform_driver);
+}
+
+void __exit msm_mdp4_unregister(void)
+{
+ platform_driver_unregister(_platform_driver);
+}
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f18dfbb614f0..629f3ac7a88c 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -256,10 +256,6 @@ static int msm_drm_uninit(struct device *dev)
return 0;
}
-#define KMS_MDP4 4
-#define KMS_MDP5 5
-#define KMS_DPU 3
-
static int get_mdp_ver(struct platform_device *pdev)
{
struct device *dev = >dev;
@@ -944,50 +940,7 @@ static const struct drm_driver msm_driver = {
.patchlevel = MSM_VERSION_PATCHLEVEL,
};
-static int __maybe_unused msm_runtime_suspend(struct device *dev)
-{
- struct msm_drm_private *priv = dev_get_drvdata(dev);
- struct msm_mdss *mdss = priv->mdss;
-
- DBG("");
-
- if (mdss)
- return msm_mdss_disable(mdss);
-
- return 0;
-}
-
-static int __maybe_unused msm_runtime_resume(struct device *dev)
-{
- struct msm_drm_private *priv = dev_get_drvdata(dev);
- struct msm_mdss *mdss = priv->mdss;
-
- DBG("");
-
- if (mdss)
- return msm_mdss_enable(mdss);
-
- return 0;
-}
-
-static int __maybe_unused msm_pm_suspend(struct device *dev)
-{
-
- if (pm_runtime_suspended(dev))
- return 0;
-
- return msm_runtime_suspend(dev);
-}
-
-static int __maybe_unused msm_pm_resume(struct device *dev)
-{
- if (pm_runtime_suspended(dev))
- return 0;
-
- return msm_runtime_resume(dev);
-}
-
-static int __maybe_unused msm_pm_prepare(struct device *dev)
+int msm_pm_prepare(struct device *dev)
{
struct msm_drm_private *priv = dev_get_drvdata(dev);
struct drm_device *ddev = priv ? priv->dev : NULL;
@@ -998,7 +951,7 @@ static int __maybe_unused msm_pm_prepare(struct device *dev)
return drm_mode_config_helper_suspend(ddev);
}
-static void __maybe_unused msm_pm_complete(struct device *dev)
+void msm_pm_complete(struct device *dev)
{
struct msm_drm_private *priv = dev_get_drvdata(dev);
struct drm_device *ddev = priv ? priv->dev : NULL;
@@ -1010,8 +963,6 @@ static void __maybe_unused msm_pm_complete(struct device
*dev)
}
static const struct dev_pm_ops msm_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(msm_pm_suspend, msm_pm_resume)
- SET_RUNTIME_PM_OPS(msm_runtime_suspend, msm_runtime_resume, NULL)
.prepare =
[PATCH v2 4/4] drm/msm: stop using device's match data pointer
Let's make the match's data pointer a (sub-)driver's private data. The
only user currently is the msm_drm_init() function, using this data to
select kms_init callback. Pass this callback through the driver's
private data instead.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 ---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 14 +
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 11 ---
drivers/gpu/drm/msm/msm_drv.c| 38 ++--
drivers/gpu/drm/msm/msm_drv.h| 5 +---
drivers/gpu/drm/msm/msm_kms.h| 4 ---
drivers/gpu/drm/msm/msm_mdss.c | 23 +++---
7 files changed, 41 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index a46aa36bc5db..c466cbc56e16 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1175,7 +1175,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
return rc;
}
-struct msm_kms *dpu_kms_init(struct drm_device *dev)
+static int dpu_kms_init(struct drm_device *dev)
{
struct msm_drm_private *priv;
struct dpu_kms *dpu_kms;
@@ -1183,7 +1183,7 @@ struct msm_kms *dpu_kms_init(struct drm_device *dev)
if (!dev) {
DPU_ERROR("drm device node invalid\n");
- return ERR_PTR(-EINVAL);
+ return -EINVAL;
}
priv = dev->dev_private;
@@ -1192,11 +1192,11 @@ struct msm_kms *dpu_kms_init(struct drm_device *dev)
irq = irq_of_parse_and_map(dpu_kms->pdev->dev.of_node, 0);
if (irq < 0) {
DPU_ERROR("failed to get irq: %d\n", irq);
- return ERR_PTR(irq);
+ return irq;
}
dpu_kms->base.irq = irq;
- return _kms->base;
+ return 0;
}
static int dpu_bind(struct device *dev, struct device *master, void *data)
@@ -1207,6 +1207,8 @@ static int dpu_bind(struct device *dev, struct device
*master, void *data)
struct dpu_kms *dpu_kms;
int ret = 0;
+ priv->kms_init = dpu_kms_init;
+
dpu_kms = devm_kzalloc(>dev, sizeof(*dpu_kms), GFP_KERNEL);
if (!dpu_kms)
return -ENOMEM;
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index c5c0650414c5..2e5f6b6fd3c3 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -389,7 +389,7 @@ static void read_mdp_hw_revision(struct mdp4_kms *mdp4_kms,
DRM_DEV_INFO(dev->dev, "MDP4 version v%d.%d", *major, *minor);
}
-struct msm_kms *mdp4_kms_init(struct drm_device *dev)
+static int mdp4_kms_init(struct drm_device *dev)
{
struct platform_device *pdev = to_platform_device(dev->dev);
struct mdp4_platform_config *config = mdp4_get_config(pdev);
@@ -403,8 +403,7 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL);
if (!mdp4_kms) {
DRM_DEV_ERROR(dev->dev, "failed to allocate kms\n");
- ret = -ENOMEM;
- goto fail;
+ return -ENOMEM;
}
ret = mdp_kms_init(_kms->base, _funcs);
@@ -551,12 +550,13 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
dev->mode_config.max_width = 2048;
dev->mode_config.max_height = 2048;
- return kms;
+ return 0;
fail:
if (kms)
mdp4_destroy(kms);
- return ERR_PTR(ret);
+
+ return ret;
}
static struct mdp4_platform_config *mdp4_get_config(struct platform_device
*dev)
@@ -583,6 +583,8 @@ static int mdp4_probe(struct platform_device *pdev)
if (!priv)
return -ENOMEM;
+ priv->kms_init = mdp4_kms_init;
+
platform_set_drvdata(pdev, priv);
/*
@@ -600,7 +602,7 @@ static int mdp4_remove(struct platform_device *pdev)
}
static const struct of_device_id mdp4_dt_match[] = {
- { .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
+ { .compatible = "qcom,mdp4" },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, mdp4_dt_match);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 81bd434980cf..3b7d7c77c503 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -551,7 +551,7 @@ static int get_clk(struct platform_device *pdev, struct clk
**clkp,
return 0;
}
-struct msm_kms *mdp5_kms_init(struct drm_device *dev)
+static int mdp5_kms_init(struct drm_device *dev)
{
struct msm_drm_private *priv = dev->dev_private;
struct platform_device *pdev;
@@ -565,7 +565,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
/* priv->kms would have been populated by the MDP5 driver */
kms = priv->kms;
if (!kms)
- return NULL;
+
[PATCH v2 1/4] drm/msm: unify MDSS drivers
MDP5 and DPU1 both provide the driver handling the MDSS region, which
handles the irq domain and (incase of DPU1) adds some init for the UBWC
controller. Unify those two pieces of code into a common driver.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/Makefile | 3 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 252 --
drivers/gpu/drm/msm/msm_drv.c | 4 +-
drivers/gpu/drm/msm/msm_kms.h | 3 +-
.../msm/{disp/dpu1/dpu_mdss.c => msm_mdss.c} | 160 ++-
5 files changed, 98 insertions(+), 324 deletions(-)
delete mode 100644 drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
rename drivers/gpu/drm/msm/{disp/dpu1/dpu_mdss.c => msm_mdss.c} (58%)
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index e9cc7d8ac301..e76927b42033 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -42,7 +42,6 @@ msm-y := \
disp/mdp5/mdp5_crtc.o \
disp/mdp5/mdp5_encoder.o \
disp/mdp5/mdp5_irq.o \
- disp/mdp5/mdp5_mdss.o \
disp/mdp5/mdp5_kms.o \
disp/mdp5/mdp5_pipe.o \
disp/mdp5/mdp5_mixer.o \
@@ -67,7 +66,6 @@ msm-y := \
disp/dpu1/dpu_hw_util.o \
disp/dpu1/dpu_hw_vbif.o \
disp/dpu1/dpu_kms.o \
- disp/dpu1/dpu_mdss.o \
disp/dpu1/dpu_plane.o \
disp/dpu1/dpu_rm.o \
disp/dpu1/dpu_vbif.o \
@@ -88,6 +86,7 @@ msm-y := \
msm_gpu_devfreq.o \
msm_io_utils.o \
msm_iommu.o \
+ msm_mdss.o \
msm_perf.o \
msm_rd.o \
msm_ringbuffer.o \
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
deleted file mode 100644
index 049c6784a531..
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
+++ /dev/null
@@ -1,252 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (c) 2016, The Linux Foundation. All rights reserved.
- */
-
-#include
-#include
-
-#include "msm_drv.h"
-#include "mdp5_kms.h"
-
-#define to_mdp5_mdss(x) container_of(x, struct mdp5_mdss, base)
-
-struct mdp5_mdss {
- struct msm_mdss base;
-
- void __iomem *mmio, *vbif;
-
- struct clk *ahb_clk;
- struct clk *axi_clk;
- struct clk *vsync_clk;
-
- struct {
- volatile unsigned long enabled_mask;
- struct irq_domain *domain;
- } irqcontroller;
-};
-
-static inline void mdss_write(struct mdp5_mdss *mdp5_mdss, u32 reg, u32 data)
-{
- msm_writel(data, mdp5_mdss->mmio + reg);
-}
-
-static inline u32 mdss_read(struct mdp5_mdss *mdp5_mdss, u32 reg)
-{
- return msm_readl(mdp5_mdss->mmio + reg);
-}
-
-static irqreturn_t mdss_irq(int irq, void *arg)
-{
- struct mdp5_mdss *mdp5_mdss = arg;
- u32 intr;
-
- intr = mdss_read(mdp5_mdss, REG_MDSS_HW_INTR_STATUS);
-
- VERB("intr=%08x", intr);
-
- while (intr) {
- irq_hw_number_t hwirq = fls(intr) - 1;
-
- generic_handle_domain_irq(mdp5_mdss->irqcontroller.domain,
hwirq);
- intr &= ~(1 << hwirq);
- }
-
- return IRQ_HANDLED;
-}
-
-/*
- * interrupt-controller implementation, so sub-blocks (MDP/HDMI/eDP/DSI/etc)
- * can register to get their irq's delivered
- */
-
-#define VALID_IRQS (MDSS_HW_INTR_STATUS_INTR_MDP | \
- MDSS_HW_INTR_STATUS_INTR_DSI0 | \
- MDSS_HW_INTR_STATUS_INTR_DSI1 | \
- MDSS_HW_INTR_STATUS_INTR_HDMI | \
- MDSS_HW_INTR_STATUS_INTR_EDP)
-
-static void mdss_hw_mask_irq(struct irq_data *irqd)
-{
- struct mdp5_mdss *mdp5_mdss = irq_data_get_irq_chip_data(irqd);
-
- smp_mb__before_atomic();
- clear_bit(irqd->hwirq, _mdss->irqcontroller.enabled_mask);
- smp_mb__after_atomic();
-}
-
-static void mdss_hw_unmask_irq(struct irq_data *irqd)
-{
- struct mdp5_mdss *mdp5_mdss = irq_data_get_irq_chip_data(irqd);
-
- smp_mb__before_atomic();
- set_bit(irqd->hwirq, _mdss->irqcontroller.enabled_mask);
- smp_mb__after_atomic();
-}
-
-static struct irq_chip mdss_hw_irq_chip = {
- .name = "mdss",
- .irq_mask = mdss_hw_mask_irq,
- .irq_unmask = mdss_hw_unmask_irq,
-};
-
-static int mdss_hw_irqdomain_map(struct irq_domain *d, unsigned int irq,
-irq_hw_number_t hwirq)
-{
- struct mdp5_mdss *mdp5_mdss = d->host_data;
-
- if (!(VALID_IRQS & (1 << hwirq)))
- return -EPERM;
-
- irq_set_chip_and_handler(irq, _hw_irq_chip, handle_level_irq);
- irq_set_chip_data(irq, mdp5_mdss);
-
- return 0;
-}
-
-static const struct irq_domain_ops mdss_hw_irqdomain_ops = {
- .map = mdss_hw_irqdomain_map,
- .xlate = irq_domain_xlate_onecell,
-};
-
-
-static int mdss_irq_domain_init(struct mdp5_mdss *mdp5_mdss)
-{
- struct device *dev = mdp5_mdss->base.dev;
- struct irq_domain *d;
-
- d =
[PATCH v2 2/4] drm/msm: remove extra indirection for msm_mdss
Since now there is just one mdss subdriver, drop all the indirection,
make msm_mdss struct completely opaque (and defined inside msm_mdss.c)
and call mdss functions directly.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/msm_drv.c | 44
drivers/gpu/drm/msm/msm_kms.h | 16 ++---
drivers/gpu/drm/msm/msm_mdss.c | 125 ++---
3 files changed, 88 insertions(+), 97 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index be06a62d7ccb..f18dfbb614f0 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -951,8 +951,8 @@ static int __maybe_unused msm_runtime_suspend(struct device
*dev)
DBG("");
- if (mdss && mdss->funcs)
- return mdss->funcs->disable(mdss);
+ if (mdss)
+ return msm_mdss_disable(mdss);
return 0;
}
@@ -964,8 +964,8 @@ static int __maybe_unused msm_runtime_resume(struct device
*dev)
DBG("");
- if (mdss && mdss->funcs)
- return mdss->funcs->enable(mdss);
+ if (mdss)
+ return msm_mdss_enable(mdss);
return 0;
}
@@ -1200,6 +1200,7 @@ static const struct component_master_ops msm_drm_ops = {
static int msm_pdev_probe(struct platform_device *pdev)
{
struct component_match *match = NULL;
+ struct msm_mdss *mdss;
struct msm_drm_private *priv;
int ret;
@@ -1211,19 +1212,32 @@ static int msm_pdev_probe(struct platform_device *pdev)
switch (get_mdp_ver(pdev)) {
case KMS_MDP5:
- ret = msm_mdss_init(pdev, true);
+ mdss = msm_mdss_init(pdev, true);
+ if (IS_ERR(mdss)) {
+ ret = PTR_ERR(mdss);
+ platform_set_drvdata(pdev, NULL);
+
+ return ret;
+ } else {
+ priv->mdss = mdss;
+ pm_runtime_enable(>dev);
+ }
break;
case KMS_DPU:
- ret = msm_mdss_init(pdev, false);
+ mdss = msm_mdss_init(pdev, false);
+ if (IS_ERR(mdss)) {
+ ret = PTR_ERR(mdss);
+ platform_set_drvdata(pdev, NULL);
+
+ return ret;
+ } else {
+ priv->mdss = mdss;
+ pm_runtime_enable(>dev);
+ }
break;
default:
- ret = 0;
break;
}
- if (ret) {
- platform_set_drvdata(pdev, NULL);
- return ret;
- }
if (get_mdp_ver(pdev)) {
ret = add_display_components(pdev, );
@@ -1251,8 +1265,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
fail:
of_platform_depopulate(>dev);
- if (priv->mdss && priv->mdss->funcs)
- priv->mdss->funcs->destroy(priv->mdss);
+ if (priv->mdss)
+ msm_mdss_destroy(priv->mdss);
return ret;
}
@@ -1265,8 +1279,8 @@ static int msm_pdev_remove(struct platform_device *pdev)
component_master_del(>dev, _drm_ops);
of_platform_depopulate(>dev);
- if (mdss && mdss->funcs)
- mdss->funcs->destroy(mdss);
+ if (mdss)
+ msm_mdss_destroy(mdss);
return 0;
}
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 2459ba479caf..0c341660941a 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -201,18 +201,12 @@ struct msm_kms *dpu_kms_init(struct drm_device *dev);
extern const struct of_device_id dpu_dt_match[];
extern const struct of_device_id mdp5_dt_match[];
-struct msm_mdss_funcs {
- int (*enable)(struct msm_mdss *mdss);
- int (*disable)(struct msm_mdss *mdss);
- void (*destroy)(struct msm_mdss *mdss);
-};
-
-struct msm_mdss {
- struct device *dev;
- const struct msm_mdss_funcs *funcs;
-};
+struct msm_mdss;
-int msm_mdss_init(struct platform_device *pdev, bool mdp5);
+struct msm_mdss *msm_mdss_init(struct platform_device *pdev, bool mdp5);
+int msm_mdss_enable(struct msm_mdss *mdss);
+int msm_mdss_disable(struct msm_mdss *mdss);
+void msm_mdss_destroy(struct msm_mdss *mdss);
#define for_each_crtc_mask(dev, crtc, crtc_mask) \
drm_for_each_crtc(crtc, dev) \
diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index f5429eb0ae52..92562221b517 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -14,8 +14,6 @@
/* for DPU_HW_* defines */
#include "disp/dpu1/dpu_hw_catalog.h"
-#define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base)
-
#define HW_REV 0x0
#define HW_INTR_STATUS 0x0010
@@ -23,8 +21,9 @@
#define UBWC_CTRL_20x150
#define UBWC_PREDICTION_MODE 0x154
-struct dpu_mdss {
-
[PATCH v2 0/4] drm/msm: rework MDSS drivers
These patches coninue work started by AngeloGioacchino Del Regno in the previous cycle by further decoupling and dissecting MDSS and MDP drivers probe/binding paths. This removes code duplication between MDP5 and DPU1 MDSS drivers, by merging them and moving to the top level. This patchset depends on the patches 1 and 2 from [1] Changes since v1: - Rebased on top of [2] and [1] [1] https://patchwork.freedesktop.org/series/99066/ [2] https://patchwork.freedesktop.org/series/98521/ Dmitry Baryshkov (4): drm/msm: unify MDSS drivers drm/msm: remove extra indirection for msm_mdss drm/msm: split the main platform driver drm/msm: stop using device's match data pointer drivers/gpu/drm/msm/Makefile | 3 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +- drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 260 - drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 68 +++- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 11 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 252 - drivers/gpu/drm/msm/msm_drv.c | 260 +++-- drivers/gpu/drm/msm/msm_drv.h | 16 + drivers/gpu/drm/msm/msm_kms.h | 18 - drivers/gpu/drm/msm/msm_mdss.c| 438 ++ 10 files changed, 578 insertions(+), 758 deletions(-) delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c delete mode 100644 drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c create mode 100644 drivers/gpu/drm/msm/msm_mdss.c base-commit: 6ed95285382d6f90a3c3a11d5806a5eb7db715c3 prerequisite-patch-id: b5572c353efa8ebefb32fc70e54bc537f78ff2bd prerequisite-patch-id: 40a854aee69b6ddf5b1c33cac3647efae3e3ebf1 prerequisite-patch-id: 8b687a19047983e26262a1bb2feda8a9fd5bc97f prerequisite-patch-id: e2d7f768b4b287c80797635f3ed5b6353b12adb5 prerequisite-patch-id: cac9fb235e3510b5bcb713f602658e6233cd308b -- 2.34.1
RE: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
> > except '"no\0\0yes" + v * 4' works a bit better. > > Is it a C code obfuscation contest? That would be: return &(v * 3)["no\0yes"]; :-) - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
[PATCH v3 2/3] drm/msm/dpu: simplify clocks handling
DPU driver contains code to parse clock items from device tree into
special data struct and then enable/disable/set rate for the clocks
using that data struct. However the DPU driver itself uses only parsing
and enabling/disabling part (the rate setting is used by DP driver).
Move this implementation to the DP driver (which actually uses rate
setting) and replace hand-coded enable/disable/get loops in the DPU
with the respective clk_bulk operations. Put operation is removed
completely because, it is handled using devres instead.
DP implementation is unchanged for now.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/Makefile | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 24 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 46 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 26 +++
.../dpu1/dpu_io_util.c => dp/dp_clk_util.c} | 69 +--
.../dpu1/dpu_io_util.h => dp/dp_clk_util.h} | 8 +--
drivers/gpu/drm/msm/dp/dp_parser.h| 2 +-
drivers/gpu/drm/msm/msm_drv.c | 1 +
drivers/gpu/drm/msm/msm_drv.h | 1 +
drivers/gpu/drm/msm/msm_io_utils.c| 50 ++
12 files changed, 89 insertions(+), 150 deletions(-)
rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.c => dp/dp_clk_util.c} (61%)
rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.h => dp/dp_clk_util.h} (85%)
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 5b6e37477079..e9cc7d8ac301 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -66,7 +66,6 @@ msm-y := \
disp/dpu1/dpu_hw_top.o \
disp/dpu1/dpu_hw_util.o \
disp/dpu1/dpu_hw_vbif.o \
- disp/dpu1/dpu_io_util.o \
disp/dpu1/dpu_kms.o \
disp/dpu1/dpu_mdss.o \
disp/dpu1/dpu_plane.o \
@@ -103,6 +102,7 @@ msm-$(CONFIG_DRM_MSM_GPU_STATE) +=
adreno/a6xx_gpu_state.o
msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
dp/dp_catalog.o \
+ dp/dp_clk_util.o \
dp/dp_ctrl.o \
dp/dp_display.o \
dp/dp_drm.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 60fe06018581..4d184122d63e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -284,17 +284,6 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
}
}
-static int _dpu_core_perf_set_core_clk_rate(struct dpu_kms *kms, u64 rate)
-{
- struct dss_clk *core_clk = kms->perf.core_clk;
-
- if (core_clk->max_rate && (rate > core_clk->max_rate))
- rate = core_clk->max_rate;
-
- core_clk->rate = rate;
- return dev_pm_opp_set_rate(>pdev->dev, core_clk->rate);
-}
-
static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
{
u64 clk_rate = kms->perf.perf_tune.min_core_clk;
@@ -306,7 +295,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms
*kms)
dpu_cstate = to_dpu_crtc_state(crtc->state);
clk_rate = max(dpu_cstate->new_perf.core_clk_rate,
clk_rate);
- clk_rate = clk_round_rate(kms->perf.core_clk->clk,
+ clk_rate = clk_round_rate(kms->perf.core_clk,
clk_rate);
}
}
@@ -405,10 +394,11 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
- ret = _dpu_core_perf_set_core_clk_rate(kms, clk_rate);
+ if (clk_rate > kms->perf.max_core_clk_rate)
+ clk_rate = kms->perf.max_core_clk_rate;
+ ret = dev_pm_opp_set_rate(>pdev->dev, clk_rate);
if (ret) {
- DPU_ERROR("failed to set %s clock rate %llu\n",
- kms->perf.core_clk->clk_name, clk_rate);
+ DPU_ERROR("failed to set core clock rate %llu\n",
clk_rate);
return ret;
}
@@ -529,13 +519,13 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
int dpu_core_perf_init(struct dpu_core_perf *perf,
struct drm_device *dev,
struct dpu_mdss_cfg *catalog,
- struct dss_clk *core_clk)
+ struct clk *core_clk)
{
perf->dev = dev;
perf->catalog = catalog;
perf->core_clk = core_clk;
- perf->max_core_clk_rate = core_clk->max_rate;
+ perf->max_core_clk_rate = clk_get_rate(core_clk);
if (!perf->max_core_clk_rate) {
DPU_DEBUG("optional max core clk rate, use default\n");
perf->max_core_clk_rate =
[PATCH v3 1/3] drm/msm: move utility functions from msm_drv.c
Move clock/IO/hrtimer utility functions from msm_drv.c to new
msm_io_utils.c file.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/msm_drv.c | 118 ---
drivers/gpu/drm/msm/msm_io_utils.c | 126 +
3 files changed, 127 insertions(+), 118 deletions(-)
create mode 100644 drivers/gpu/drm/msm/msm_io_utils.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 03ab55c37beb..5b6e37477079 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -87,6 +87,7 @@ msm-y := \
msm_gem_vma.o \
msm_gpu.o \
msm_gpu_devfreq.o \
+ msm_io_utils.o \
msm_iommu.o \
msm_perf.o \
msm_rd.o \
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index fd62a4da14a1..30c44c395a24 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -75,124 +75,6 @@ static bool modeset = true;
MODULE_PARM_DESC(modeset, "Use kernel modesetting [KMS] (1=on (default),
0=disable)");
module_param(modeset, bool, 0600);
-/*
- * Util/helpers:
- */
-
-struct clk *msm_clk_bulk_get_clock(struct clk_bulk_data *bulk, int count,
- const char *name)
-{
- int i;
- char n[32];
-
- snprintf(n, sizeof(n), "%s_clk", name);
-
- for (i = 0; bulk && i < count; i++) {
- if (!strcmp(bulk[i].id, name) || !strcmp(bulk[i].id, n))
- return bulk[i].clk;
- }
-
-
- return NULL;
-}
-
-struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
-{
- struct clk *clk;
- char name2[32];
-
- clk = devm_clk_get(>dev, name);
- if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
- return clk;
-
- snprintf(name2, sizeof(name2), "%s_clk", name);
-
- clk = devm_clk_get(>dev, name2);
- if (!IS_ERR(clk))
- dev_warn(>dev, "Using legacy clk name binding. Use "
- "\"%s\" instead of \"%s\"\n", name, name2);
-
- return clk;
-}
-
-static void __iomem *_msm_ioremap(struct platform_device *pdev, const char
*name,
- bool quiet, phys_addr_t *psize)
-{
- struct resource *res;
- unsigned long size;
- void __iomem *ptr;
-
- if (name)
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
- else
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
- if (!res) {
- if (!quiet)
- DRM_DEV_ERROR(>dev, "failed to get memory
resource: %s\n", name);
- return ERR_PTR(-EINVAL);
- }
-
- size = resource_size(res);
-
- ptr = devm_ioremap(>dev, res->start, size);
- if (!ptr) {
- if (!quiet)
- DRM_DEV_ERROR(>dev, "failed to ioremap: %s\n",
name);
- return ERR_PTR(-ENOMEM);
- }
-
- if (psize)
- *psize = size;
-
- return ptr;
-}
-
-void __iomem *msm_ioremap(struct platform_device *pdev, const char *name)
-{
- return _msm_ioremap(pdev, name, false, NULL);
-}
-
-void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name)
-{
- return _msm_ioremap(pdev, name, true, NULL);
-}
-
-void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
- phys_addr_t *psize)
-{
- return _msm_ioremap(pdev, name, false, psize);
-}
-
-static enum hrtimer_restart msm_hrtimer_worktimer(struct hrtimer *t)
-{
- struct msm_hrtimer_work *work = container_of(t,
- struct msm_hrtimer_work, timer);
-
- kthread_queue_work(work->worker, >work);
-
- return HRTIMER_NORESTART;
-}
-
-void msm_hrtimer_queue_work(struct msm_hrtimer_work *work,
- ktime_t wakeup_time,
- enum hrtimer_mode mode)
-{
- hrtimer_start(>timer, wakeup_time, mode);
-}
-
-void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
- struct kthread_worker *worker,
- kthread_work_func_t fn,
- clockid_t clock_id,
- enum hrtimer_mode mode)
-{
- hrtimer_init(>timer, clock_id, mode);
- work->timer.function = msm_hrtimer_worktimer;
- work->worker = worker;
- kthread_init_work(>work, fn);
-}
-
static irqreturn_t msm_irq(int irq, void *arg)
{
struct drm_device *dev = arg;
diff --git a/drivers/gpu/drm/msm/msm_io_utils.c
b/drivers/gpu/drm/msm/msm_io_utils.c
new file mode 100644
index ..7b504617833a
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_io_utils.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All rights
reserved.
+ * Copyright (C) 2013 Red Hat
+ * Author: Rob Clark
+ */
+
[PATCH v3 3/3] drm/msm/dp: rewrite dss_module_power to use bulk clock functions
In order to simplify DP code, drop hand-coded loops over clock arrays,
replacing them with clk_bulk_* functions.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/Makefile | 1 -
drivers/gpu/drm/msm/dp/dp_clk_util.c | 120 ---
drivers/gpu/drm/msm/dp/dp_clk_util.h | 38 -
drivers/gpu/drm/msm/dp/dp_ctrl.c | 19 ++---
drivers/gpu/drm/msm/dp/dp_parser.c | 21 -
drivers/gpu/drm/msm/dp/dp_parser.h | 17 +++-
drivers/gpu/drm/msm/dp/dp_power.c| 82 +++---
7 files changed, 95 insertions(+), 203 deletions(-)
delete mode 100644 drivers/gpu/drm/msm/dp/dp_clk_util.c
delete mode 100644 drivers/gpu/drm/msm/dp/dp_clk_util.h
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index e9cc7d8ac301..ac31f85e0699 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -102,7 +102,6 @@ msm-$(CONFIG_DRM_MSM_GPU_STATE) +=
adreno/a6xx_gpu_state.o
msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
dp/dp_catalog.o \
- dp/dp_clk_util.o \
dp/dp_ctrl.o \
dp/dp_display.o \
dp/dp_drm.o \
diff --git a/drivers/gpu/drm/msm/dp/dp_clk_util.c
b/drivers/gpu/drm/msm/dp/dp_clk_util.c
deleted file mode 100644
index 44a4fc59ff31..
--- a/drivers/gpu/drm/msm/dp/dp_clk_util.c
+++ /dev/null
@@ -1,120 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2012-2015, 2017-2018, The Linux Foundation.
- * All rights reserved.
- */
-
-#include
-#include
-#include
-#include
-#include
-
-#include
-
-#include "dp_clk_util.h"
-
-void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk)
-{
- int i;
-
- for (i = num_clk - 1; i >= 0; i--) {
- if (clk_arry[i].clk)
- clk_put(clk_arry[i].clk);
- clk_arry[i].clk = NULL;
- }
-}
-
-int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, int num_clk)
-{
- int i, rc = 0;
-
- for (i = 0; i < num_clk; i++) {
- clk_arry[i].clk = clk_get(dev, clk_arry[i].clk_name);
- rc = PTR_ERR_OR_ZERO(clk_arry[i].clk);
- if (rc) {
- DEV_ERR("%pS->%s: '%s' get failed. rc=%d\n",
- __builtin_return_address(0), __func__,
- clk_arry[i].clk_name, rc);
- goto error;
- }
- }
-
- return rc;
-
-error:
- for (i--; i >= 0; i--) {
- if (clk_arry[i].clk)
- clk_put(clk_arry[i].clk);
- clk_arry[i].clk = NULL;
- }
-
- return rc;
-}
-
-int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk)
-{
- int i, rc = 0;
-
- for (i = 0; i < num_clk; i++) {
- if (clk_arry[i].clk) {
- if (clk_arry[i].type != DSS_CLK_AHB) {
- DEV_DBG("%pS->%s: '%s' rate %ld\n",
- __builtin_return_address(0), __func__,
- clk_arry[i].clk_name,
- clk_arry[i].rate);
- rc = clk_set_rate(clk_arry[i].clk,
- clk_arry[i].rate);
- if (rc) {
- DEV_ERR("%pS->%s: %s failed. rc=%d\n",
- __builtin_return_address(0),
- __func__,
- clk_arry[i].clk_name, rc);
- break;
- }
- }
- } else {
- DEV_ERR("%pS->%s: '%s' is not available\n",
- __builtin_return_address(0), __func__,
- clk_arry[i].clk_name);
- rc = -EPERM;
- break;
- }
- }
-
- return rc;
-}
-
-int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable)
-{
- int i, rc = 0;
-
- if (enable) {
- for (i = 0; i < num_clk; i++) {
- DEV_DBG("%pS->%s: enable '%s'\n",
- __builtin_return_address(0), __func__,
- clk_arry[i].clk_name);
- rc = clk_prepare_enable(clk_arry[i].clk);
- if (rc)
- DEV_ERR("%pS->%s: %s en fail. rc=%d\n",
- __builtin_return_address(0),
- __func__,
- clk_arry[i].clk_name, rc);
-
- if (rc && i) {
- msm_dss_enable_clk(_arry[i - 1],
- i - 1, false);
- break;
- }
[PATCH v3 0/3] drm/msm: rework clock handling
msm_dss_clk_*() functions significantly duplicate clk_bulk_* family of functions. Drop custom code and use bulk clocks directly. This also removes dependency of DP driver on the DPU driver internals. Prerequisites: [1] Changes since v2: - Retain conditional code/prints in DP code to ease debugging - Rebase on top of msm-next and [1] - Split helper functions to msm_io_utils.c as suggested by Jessica Changes since v1: - Rebase on top of current tree to fix conflicts Dmitry Baryshkov (3): drm/msm: move utility functions from msm_drv.c drm/msm/dpu: simplify clocks handling drm/msm/dp: rewrite dss_module_power to use bulk clock functions [1] https://patchwork.freedesktop.org/series/98521/ drivers/gpu/drm/msm/Makefile | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 24 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 6 +- drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 187 -- drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h | 40 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 46 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 26 +-- drivers/gpu/drm/msm/dp/dp_ctrl.c | 19 +- drivers/gpu/drm/msm/dp/dp_parser.c| 21 +- drivers/gpu/drm/msm/dp/dp_parser.h| 17 +- drivers/gpu/drm/msm/dp/dp_power.c | 82 +--- drivers/gpu/drm/msm/msm_drv.c | 119 +-- drivers/gpu/drm/msm/msm_drv.h | 1 + drivers/gpu/drm/msm/msm_io_utils.c| 176 + 15 files changed, 305 insertions(+), 465 deletions(-) delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h create mode 100644 drivers/gpu/drm/msm/msm_io_utils.c base-commit: 6ed95285382d6f90a3c3a11d5806a5eb7db715c3 prerequisite-patch-id: b5572c353efa8ebefb32fc70e54bc537f78ff2bd prerequisite-patch-id: 40a854aee69b6ddf5b1c33cac3647efae3e3ebf1 prerequisite-patch-id: 8b687a19047983e26262a1bb2feda8a9fd5bc97f -- 2.34.1
[PATCH 3/3] drm/i915/guc: Flush G2H handler during a GT reset
Now that the error capture is fully decoupled from fence signalling
(request retirement to free memory, which in turn depends on resets) we
can safely flush the G2H handler during a GT reset. This is eliminates
corner cases where GuC generated G2H (e.g. engine resets) race with a GT
reset.
v2:
(John Harrison)
- Fix typo in commit message (s/is/in)
Signed-off-by: Matthew Brost
Reviewed-by: John Harrison
---
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 18 +-
1 file changed, 1 insertion(+), 17 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 514b3060b141..406dd2e3f5a9 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1396,8 +1396,6 @@ static void guc_flush_destroyed_contexts(struct intel_guc
*guc);
void intel_guc_submission_reset_prepare(struct intel_guc *guc)
{
- int i;
-
if (unlikely(!guc_submission_initialized(guc))) {
/* Reset called during driver load? GuC not yet initialised! */
return;
@@ -1414,21 +1412,7 @@ void intel_guc_submission_reset_prepare(struct intel_guc
*guc)
guc_flush_submissions(guc);
guc_flush_destroyed_contexts(guc);
-
- /*
-* Handle any outstanding G2Hs before reset. Call IRQ handler directly
-* each pass as interrupt have been disabled. We always scrub for
-* outstanding G2H as it is possible for outstanding_submission_g2h to
-* be incremented after the context state update.
-*/
- for (i = 0; i < 4 && atomic_read(>outstanding_submission_g2h);
++i) {
- intel_guc_to_host_event_handler(guc);
-#define wait_for_reset(guc, wait_var) \
- intel_guc_wait_for_pending_msg(guc, wait_var, false, (HZ / 20))
- do {
- wait_for_reset(guc, >outstanding_submission_g2h);
- } while (!list_empty(>ct.requests.incoming));
- }
+ flush_work(>ct.requests.worker);
scrub_guc_desc_for_outstanding_g2h(guc);
}
--
2.34.1
[PATCH 2/3] drm/i915/guc: Add work queue to trigger a GT reset
The G2H handler needs to be flushed during a GT reset but a G2H
indicating engine reset failure can trigger a GT reset. Add a worker to
trigger the GT when an engine reset failure is received to break this
circular dependency.
v2:
(John Harrison)
- Store engine reset mask
- Fix typo in commit message
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 9 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 37 +--
2 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 9d26a86fe557..c4a9fc7dd246 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -119,6 +119,15 @@ struct intel_guc {
* function as it might be in an atomic context (no sleeping)
*/
struct work_struct destroyed_worker;
+ /**
+* @reset_worker: worker to trigger a GT reset after an engine
+* reset fails
+*/
+ struct work_struct reset_worker;
+ /**
+* @reset_mask: mask of engines that failed to reset
+*/
+ intel_engine_mask_t reset_mask;
} submission_state;
/**
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 3918f1be114f..514b3060b141 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1731,6 +1731,7 @@ void intel_guc_submission_reset_finish(struct intel_guc
*guc)
}
static void destroyed_worker_func(struct work_struct *w);
+static void reset_worker_func(struct work_struct *w);
/*
* Set up the memory resources to be shared with the GuC (via the GGTT)
@@ -1761,6 +1762,8 @@ int intel_guc_submission_init(struct intel_guc *guc)
INIT_LIST_HEAD(>submission_state.destroyed_contexts);
INIT_WORK(>submission_state.destroyed_worker,
destroyed_worker_func);
+ INIT_WORK(>submission_state.reset_worker,
+ reset_worker_func);
guc->submission_state.guc_ids_bitmap =
bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID(guc), GFP_KERNEL);
@@ -4026,6 +4029,26 @@ guc_lookup_engine(struct intel_guc *guc, u8 guc_class,
u8 instance)
return gt->engine_class[engine_class][instance];
}
+static void reset_worker_func(struct work_struct *w)
+{
+ struct intel_guc *guc = container_of(w, struct intel_guc,
+submission_state.reset_worker);
+ struct intel_gt *gt = guc_to_gt(guc);
+ intel_engine_mask_t reset_mask;
+ unsigned long flags;
+
+ spin_lock_irqsave(>submission_state.lock, flags);
+ reset_mask = guc->submission_state.reset_mask;
+ guc->submission_state.reset_mask = 0;
+ spin_unlock_irqrestore(>submission_state.lock, flags);
+
+ if (likely(reset_mask))
+ intel_gt_handle_error(gt, reset_mask,
+ I915_ERROR_CAPTURE,
+ "GuC failed to reset engine mask=0x%x\n",
+ reset_mask);
+}
+
int intel_guc_engine_failure_process_msg(struct intel_guc *guc,
const u32 *msg, u32 len)
{
@@ -4033,6 +4056,7 @@ int intel_guc_engine_failure_process_msg(struct intel_guc
*guc,
struct intel_gt *gt = guc_to_gt(guc);
u8 guc_class, instance;
u32 reason;
+ unsigned long flags;
if (unlikely(len != 3)) {
drm_err(>i915->drm, "Invalid length %u", len);
@@ -4057,10 +4081,15 @@ int intel_guc_engine_failure_process_msg(struct
intel_guc *guc,
drm_err(>i915->drm, "GuC engine reset request failed on %d:%d (%s)
because 0x%08X",
guc_class, instance, engine->name, reason);
- intel_gt_handle_error(gt, engine->mask,
- I915_ERROR_CAPTURE,
- "GuC failed to reset %s (reason=0x%08x)\n",
- engine->name, reason);
+ spin_lock_irqsave(>submission_state.lock, flags);
+ guc->submission_state.reset_mask |= engine->mask;
+ spin_unlock_irqrestore(>submission_state.lock, flags);
+
+ /*
+* A GT reset flushes this worker queue (G2H handler) so we must use
+* another worker to trigger a GT reset.
+*/
+ queue_work(system_unbound_wq, >submission_state.reset_worker);
return 0;
}
--
2.34.1
[PATCH 0/3] Flush G2H handler during a GT reset
After a small fix to error capture code, we now can flush G2H during a GT reset which simplifies code and seals some extreme corner case races. v2: (CI) - Don't trigger GT reset from G2H handler v3: - Address John Harrison's comments Signed-off-by: Matthew Brost Matthew Brost (3): drm/i915: Allocate intel_engine_coredump_alloc with ALLOW_FAIL drm/i915/guc: Add work queue to trigger a GT reset drm/i915/guc: Flush G2H handler during a GT reset drivers/gpu/drm/i915/gt/uc/intel_guc.h| 9 +++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 55 --- drivers/gpu/drm/i915/i915_gpu_error.c | 2 +- 3 files changed, 44 insertions(+), 22 deletions(-) -- 2.34.1
[PATCH 1/3] drm/i915: Allocate intel_engine_coredump_alloc with ALLOW_FAIL
Allocate intel_engine_coredump_alloc with ALLOW_FAIL rather than
GFP_KERNEL to fully decouple the error capture from fence signalling.
v2:
(John Harrison)
- Fix typo in commit message (s/do/to)
Fixes: 8b91cdd4f8649 ("drm/i915: Use __GFP_KSWAPD_RECLAIM in the capture code")
Signed-off-by: Matthew Brost
Reviewed-by: John Harrison
---
drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 67f3515f07e7..aee42eae4729 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1516,7 +1516,7 @@ capture_engine(struct intel_engine_cs *engine,
struct i915_request *rq = NULL;
unsigned long flags;
- ee = intel_engine_coredump_alloc(engine, GFP_KERNEL);
+ ee = intel_engine_coredump_alloc(engine, ALLOW_FAIL);
if (!ee)
return NULL;
--
2.34.1
[PATCH] drm/i915/guc: Ensure multi-lrc fini breadcrumb math is correct
Realized that the GuC multi-lrc fini breadcrumb emit code is very
delicate as the math this code does relies on functions it calls to emit
a certain number of DWs. Add a few GEM_BUG_ONs to assert the math is
correct.
v2:
- Rebase + resend for CI
(Checkpatch)
- Fix blank line warning
Signed-off-by: Matthew Brost
Reviewed-by: John Harrison
---
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 33 +++
1 file changed, 27 insertions(+), 6 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 3918f1be114f..650efd3d3a48 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -4429,27 +4429,31 @@ static inline bool skip_handshake(struct i915_request
*rq)
return test_bit(I915_FENCE_FLAG_SKIP_PARALLEL, >fence.flags);
}
+#define NON_SKIP_LEN 6
static u32 *
emit_fini_breadcrumb_parent_no_preempt_mid_batch(struct i915_request *rq,
u32 *cs)
{
struct intel_context *ce = rq->context;
+ __maybe_unused u32 *before_fini_breadcrumb_user_interrupt_cs;
+ __maybe_unused u32 *start_fini_breadcrumb_cs = cs;
GEM_BUG_ON(!intel_context_is_parent(ce));
if (unlikely(skip_handshake(rq))) {
/*
* NOP everything in
__emit_fini_breadcrumb_parent_no_preempt_mid_batch,
-* the -6 comes from the length of the emits below.
+* the NON_SKIP_LEN comes from the length of the emits below.
*/
memset(cs, 0, sizeof(u32) *
- (ce->engine->emit_fini_breadcrumb_dw - 6));
- cs += ce->engine->emit_fini_breadcrumb_dw - 6;
+ (ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN));
+ cs += ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN;
} else {
cs = __emit_fini_breadcrumb_parent_no_preempt_mid_batch(rq, cs);
}
/* Emit fini breadcrumb */
+ before_fini_breadcrumb_user_interrupt_cs = cs;
cs = gen8_emit_ggtt_write(cs,
rq->fence.seqno,
i915_request_active_timeline(rq)->hwsp_offset,
@@ -4459,6 +4463,12 @@ emit_fini_breadcrumb_parent_no_preempt_mid_batch(struct
i915_request *rq,
*cs++ = MI_USER_INTERRUPT;
*cs++ = MI_NOOP;
+ /* Ensure our math for skip + emit is correct */
+ GEM_BUG_ON(before_fini_breadcrumb_user_interrupt_cs + NON_SKIP_LEN !=
+ cs);
+ GEM_BUG_ON(start_fini_breadcrumb_cs +
+ ce->engine->emit_fini_breadcrumb_dw != cs);
+
rq->tail = intel_ring_offset(rq, cs);
return cs;
@@ -4501,22 +4511,25 @@ emit_fini_breadcrumb_child_no_preempt_mid_batch(struct
i915_request *rq,
u32 *cs)
{
struct intel_context *ce = rq->context;
+ __maybe_unused u32 *before_fini_breadcrumb_user_interrupt_cs;
+ __maybe_unused u32 *start_fini_breadcrumb_cs = cs;
GEM_BUG_ON(!intel_context_is_child(ce));
if (unlikely(skip_handshake(rq))) {
/*
* NOP everything in
__emit_fini_breadcrumb_child_no_preempt_mid_batch,
-* the -6 comes from the length of the emits below.
+* the NON_SKIP_LEN comes from the length of the emits below.
*/
memset(cs, 0, sizeof(u32) *
- (ce->engine->emit_fini_breadcrumb_dw - 6));
- cs += ce->engine->emit_fini_breadcrumb_dw - 6;
+ (ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN));
+ cs += ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN;
} else {
cs = __emit_fini_breadcrumb_child_no_preempt_mid_batch(rq, cs);
}
/* Emit fini breadcrumb */
+ before_fini_breadcrumb_user_interrupt_cs = cs;
cs = gen8_emit_ggtt_write(cs,
rq->fence.seqno,
i915_request_active_timeline(rq)->hwsp_offset,
@@ -4526,11 +4539,19 @@ emit_fini_breadcrumb_child_no_preempt_mid_batch(struct
i915_request *rq,
*cs++ = MI_USER_INTERRUPT;
*cs++ = MI_NOOP;
+ /* Ensure our math for skip + emit is correct */
+ GEM_BUG_ON(before_fini_breadcrumb_user_interrupt_cs + NON_SKIP_LEN !=
+ cs);
+ GEM_BUG_ON(start_fini_breadcrumb_cs +
+ ce->engine->emit_fini_breadcrumb_dw != cs);
+
rq->tail = intel_ring_offset(rq, cs);
return cs;
}
+#undef NON_SKIP_LEN
+
static struct intel_context *
guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count,
unsigned long flags)
--
2.34.1
Re: [PATCH 2/3] drm/i915/guc: Add work queue to trigger a GT reset
On Wed, Jan 19, 2022 at 01:07:22PM -0800, John Harrison wrote:
> On 1/19/2022 12:54, Matthew Brost wrote:
> > On Tue, Jan 18, 2022 at 05:37:01PM -0800, John Harrison wrote:
> > > On 1/18/2022 13:43, Matthew Brost wrote:
> > > > The G2H handler needs to be flushed during a GT reset but a G2H
> > > > indicating engine reset failure can trigger a GT reset. Add a worker to
> > > > trigger the GT when a engine reset failure is received to break this
> > > s/a/an/
> > >
> > Yep.
> >
> > > > circular dependency.
> > > >
> > > > Signed-off-by: Matthew Brost
> > > > ---
> > > >drivers/gpu/drm/i915/gt/uc/intel_guc.h| 5
> > > >.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 23
> > > > +++
> > > >2 files changed, 24 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > > index 9d26a86fe557a..60ea8deef5392 100644
> > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > > @@ -119,6 +119,11 @@ struct intel_guc {
> > > > * function as it might be in an atomic context (no
> > > > sleeping)
> > > > */
> > > > struct work_struct destroyed_worker;
> > > > + /**
> > > > +* @reset_worker: worker to trigger a GT reset after an
> > > > engine
> > > > +* reset fails
> > > > +*/
> > > > + struct work_struct reset_worker;
> > > > } submission_state;
> > > > /**
> > > > 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 23a40f10d376d..cdd8d691251ff 100644
> > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > > @@ -1746,6 +1746,7 @@ void intel_guc_submission_reset_finish(struct
> > > > intel_guc *guc)
> > > >}
> > > >static void destroyed_worker_func(struct work_struct *w);
> > > > +static void reset_worker_func(struct work_struct *w);
> > > >/*
> > > > * Set up the memory resources to be shared with the GuC (via the
> > > > GGTT)
> > > > @@ -1776,6 +1777,8 @@ int intel_guc_submission_init(struct intel_guc
> > > > *guc)
> > > > INIT_LIST_HEAD(>submission_state.destroyed_contexts);
> > > > INIT_WORK(>submission_state.destroyed_worker,
> > > > destroyed_worker_func);
> > > > + INIT_WORK(>submission_state.reset_worker,
> > > > + reset_worker_func);
> > > > guc->submission_state.guc_ids_bitmap =
> > > > bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID(guc), GFP_KERNEL);
> > > > @@ -4052,6 +4055,17 @@ guc_lookup_engine(struct intel_guc *guc, u8
> > > > guc_class, u8 instance)
> > > > return gt->engine_class[engine_class][instance];
> > > >}
> > > > +static void reset_worker_func(struct work_struct *w)
> > > > +{
> > > > + struct intel_guc *guc = container_of(w, struct intel_guc,
> > > > +
> > > > submission_state.reset_worker);
> > > > + struct intel_gt *gt = guc_to_gt(guc);
> > > > +
> > > > + intel_gt_handle_error(gt, ALL_ENGINES,
> > > > + I915_ERROR_CAPTURE,
> > > > + "GuC failed to reset a engine\n");
> > > s/a/an/
> > >
> > Yep.
> >
> > > > +}
> > > > +
> > > >int intel_guc_engine_failure_process_msg(struct intel_guc *guc,
> > > > const u32 *msg, u32 len)
> > > >{
> > > > @@ -4083,10 +4097,11 @@ int intel_guc_engine_failure_process_msg(struct
> > > > intel_guc *guc,
> > > > drm_err(>i915->drm, "GuC engine reset request failed on
> > > > %d:%d (%s) because 0x%08X",
> > > > guc_class, instance, engine->name, reason);
> > > > - intel_gt_handle_error(gt, engine->mask,
> > > > - I915_ERROR_CAPTURE,
> > > > - "GuC failed to reset %s
> > > > (reason=0x%08x)\n",
> > > > - engine->name, reason);
> > > The engine name and reason code are lost from the error capture? I guess
> > > we
> > > still get it in the drm_err above, though. So probably not an issue. We
> > > shouldn't be getting these from end users and any internal CI run is only
> > > likely to give us the dmesg, not the error capture anyway! However, still
> > That was my reasoning on the msg too.
> >
> > > seems like it is work saving engine->mask in the submission_state
> > > structure
> > > (ORing in, in case there are multiple resets). Clearing it should be safe
> > > because once a GT reset has happened, we aren't getting any more G2Hs. And
> > > we can't have multiple message handlers running concurrently, right? So no
> > > need to protect the OR either.
> > >
> > I could do that but the
Re: [Intel-gfx] [PATCH 0/3] lib/string_helpers: Add a few string helpers
On Wed, Jan 19, 2022 at 12:53:43PM -0800, Lucas De Marchi wrote: > On Wed, Jan 19, 2022 at 05:15:02PM +0100, Daniel Vetter wrote: > > On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote: ... > > Yeah we can sed this anytime later we want to, but we need to get the foot > > in the door. There's also a pile more of these all over. > > > > Acked-by: Daniel Vetter > > > > on the series, maybe it helps? And yes let's merge this through drm-misc. > > Ok, it seems we are reaching some agreement here then: > - Change it to use str_ prefix Not sure about this (*), but have no strong argument against. Up to you. Ah, yes, Jani said about additional churn this change will make if goes together with this series. Perhaps it can be done separately? > - Wait -rc1 to avoid conflict > - Merge through drm-misc *) E.g. yesno() to me doesn't sound too bad to misunderstand its meaning, esp.when it's used as an argument to the printf() functions. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/3] drm/i915/guc: Add work queue to trigger a GT reset
On 1/19/2022 12:54, Matthew Brost wrote:
On Tue, Jan 18, 2022 at 05:37:01PM -0800, John Harrison wrote:
On 1/18/2022 13:43, Matthew Brost wrote:
The G2H handler needs to be flushed during a GT reset but a G2H
indicating engine reset failure can trigger a GT reset. Add a worker to
trigger the GT when a engine reset failure is received to break this
s/a/an/
Yep.
circular dependency.
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 5
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 23 +++
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 9d26a86fe557a..60ea8deef5392 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -119,6 +119,11 @@ struct intel_guc {
* function as it might be in an atomic context (no sleeping)
*/
struct work_struct destroyed_worker;
+ /**
+* @reset_worker: worker to trigger a GT reset after an engine
+* reset fails
+*/
+ struct work_struct reset_worker;
} submission_state;
/**
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 23a40f10d376d..cdd8d691251ff 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1746,6 +1746,7 @@ void intel_guc_submission_reset_finish(struct intel_guc
*guc)
}
static void destroyed_worker_func(struct work_struct *w);
+static void reset_worker_func(struct work_struct *w);
/*
* Set up the memory resources to be shared with the GuC (via the GGTT)
@@ -1776,6 +1777,8 @@ int intel_guc_submission_init(struct intel_guc *guc)
INIT_LIST_HEAD(>submission_state.destroyed_contexts);
INIT_WORK(>submission_state.destroyed_worker,
destroyed_worker_func);
+ INIT_WORK(>submission_state.reset_worker,
+ reset_worker_func);
guc->submission_state.guc_ids_bitmap =
bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID(guc), GFP_KERNEL);
@@ -4052,6 +4055,17 @@ guc_lookup_engine(struct intel_guc *guc, u8 guc_class,
u8 instance)
return gt->engine_class[engine_class][instance];
}
+static void reset_worker_func(struct work_struct *w)
+{
+ struct intel_guc *guc = container_of(w, struct intel_guc,
+submission_state.reset_worker);
+ struct intel_gt *gt = guc_to_gt(guc);
+
+ intel_gt_handle_error(gt, ALL_ENGINES,
+ I915_ERROR_CAPTURE,
+ "GuC failed to reset a engine\n");
s/a/an/
Yep.
+}
+
int intel_guc_engine_failure_process_msg(struct intel_guc *guc,
const u32 *msg, u32 len)
{
@@ -4083,10 +4097,11 @@ int intel_guc_engine_failure_process_msg(struct
intel_guc *guc,
drm_err(>i915->drm, "GuC engine reset request failed on %d:%d (%s)
because 0x%08X",
guc_class, instance, engine->name, reason);
- intel_gt_handle_error(gt, engine->mask,
- I915_ERROR_CAPTURE,
- "GuC failed to reset %s (reason=0x%08x)\n",
- engine->name, reason);
The engine name and reason code are lost from the error capture? I guess we
still get it in the drm_err above, though. So probably not an issue. We
shouldn't be getting these from end users and any internal CI run is only
likely to give us the dmesg, not the error capture anyway! However, still
That was my reasoning on the msg too.
seems like it is work saving engine->mask in the submission_state structure
(ORing in, in case there are multiple resets). Clearing it should be safe
because once a GT reset has happened, we aren't getting any more G2Hs. And
we can't have multiple message handlers running concurrently, right? So no
need to protect the OR either.
I could do that but the engine->mask is really only used for the error
capture with GuC submission as any i915 based reset with GuC submission
is a GT reset. Going from engine->mask to ALL_ENGINES will just capture
all engine state before doing a GT reset which probably isn't a bad
thing, right?
I can update the commit message explaining this if that helps.
Except that a failure to reset is notionally a hardware bug. As recently
demonstrated, it could be a software bug due to timeouts being broken.
But officially, it is something that should never happen. So in the rare
case where one does show up, we would want to know as much as possible
about the issue. Most especially - which engine it was that failed. And
if all we get is a customer bug report with an error capture but no
dmesg then we will have no idea which. It just seems wrong to be
Re: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, Jan 19, 2022 at 04:00:17PM -0500, Steven Rostedt wrote: > On Wed, 19 Jan 2022 21:25:08 +0200 > Andy Shevchenko wrote: > > > > I say keep it one line! > > > > > > Reviewed-by: Steven Rostedt (Google) > > > > I believe Sakari strongly follows the 80 rule, which means... > > Checkpatch says "100" I think we need to simply update the docs (the > documentation always lags the code ;-) The idea of checkpatch change is for old code to avoid tons of patches to satisfy 80 rule in (mostly) staging code. Some maintainers started / have been using relaxed approach. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, 19 Jan 2022 21:25:08 +0200 Andy Shevchenko wrote: > > I say keep it one line! > > > > Reviewed-by: Steven Rostedt (Google) > > I believe Sakari strongly follows the 80 rule, which means... Checkpatch says "100" I think we need to simply update the docs (the documentation always lags the code ;-) bdc48fa11e46f -- Steve
Re: [PATCH 2/3] drm/i915/guc: Add work queue to trigger a GT reset
On Tue, Jan 18, 2022 at 05:37:01PM -0800, John Harrison wrote:
> On 1/18/2022 13:43, Matthew Brost wrote:
> > The G2H handler needs to be flushed during a GT reset but a G2H
> > indicating engine reset failure can trigger a GT reset. Add a worker to
> > trigger the GT when a engine reset failure is received to break this
> s/a/an/
>
Yep.
> > circular dependency.
> >
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_guc.h| 5
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 23 +++
> > 2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index 9d26a86fe557a..60ea8deef5392 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -119,6 +119,11 @@ struct intel_guc {
> > * function as it might be in an atomic context (no sleeping)
> > */
> > struct work_struct destroyed_worker;
> > + /**
> > +* @reset_worker: worker to trigger a GT reset after an engine
> > +* reset fails
> > +*/
> > + struct work_struct reset_worker;
> > } submission_state;
> > /**
> > 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 23a40f10d376d..cdd8d691251ff 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -1746,6 +1746,7 @@ void intel_guc_submission_reset_finish(struct
> > intel_guc *guc)
> > }
> > static void destroyed_worker_func(struct work_struct *w);
> > +static void reset_worker_func(struct work_struct *w);
> > /*
> >* Set up the memory resources to be shared with the GuC (via the GGTT)
> > @@ -1776,6 +1777,8 @@ int intel_guc_submission_init(struct intel_guc *guc)
> > INIT_LIST_HEAD(>submission_state.destroyed_contexts);
> > INIT_WORK(>submission_state.destroyed_worker,
> > destroyed_worker_func);
> > + INIT_WORK(>submission_state.reset_worker,
> > + reset_worker_func);
> > guc->submission_state.guc_ids_bitmap =
> > bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID(guc), GFP_KERNEL);
> > @@ -4052,6 +4055,17 @@ guc_lookup_engine(struct intel_guc *guc, u8
> > guc_class, u8 instance)
> > return gt->engine_class[engine_class][instance];
> > }
> > +static void reset_worker_func(struct work_struct *w)
> > +{
> > + struct intel_guc *guc = container_of(w, struct intel_guc,
> > +submission_state.reset_worker);
> > + struct intel_gt *gt = guc_to_gt(guc);
> > +
> > + intel_gt_handle_error(gt, ALL_ENGINES,
> > + I915_ERROR_CAPTURE,
> > + "GuC failed to reset a engine\n");
> s/a/an/
>
Yep.
> > +}
> > +
> > int intel_guc_engine_failure_process_msg(struct intel_guc *guc,
> > const u32 *msg, u32 len)
> > {
> > @@ -4083,10 +4097,11 @@ int intel_guc_engine_failure_process_msg(struct
> > intel_guc *guc,
> > drm_err(>i915->drm, "GuC engine reset request failed on %d:%d (%s)
> > because 0x%08X",
> > guc_class, instance, engine->name, reason);
> > - intel_gt_handle_error(gt, engine->mask,
> > - I915_ERROR_CAPTURE,
> > - "GuC failed to reset %s (reason=0x%08x)\n",
> > - engine->name, reason);
> The engine name and reason code are lost from the error capture? I guess we
> still get it in the drm_err above, though. So probably not an issue. We
> shouldn't be getting these from end users and any internal CI run is only
> likely to give us the dmesg, not the error capture anyway! However, still
That was my reasoning on the msg too.
> seems like it is work saving engine->mask in the submission_state structure
> (ORing in, in case there are multiple resets). Clearing it should be safe
> because once a GT reset has happened, we aren't getting any more G2Hs. And
> we can't have multiple message handlers running concurrently, right? So no
> need to protect the OR either.
>
I could do that but the engine->mask is really only used for the error
capture with GuC submission as any i915 based reset with GuC submission
is a GT reset. Going from engine->mask to ALL_ENGINES will just capture
all engine state before doing a GT reset which probably isn't a bad
thing, right?
I can update the commit message explaining this if that helps.
Matt
> John.
>
>
> > + /*
> > +* A GT reset flushes this worker queue (G2H handler) so we must use
> > +* another worker to trigger a GT reset.
> > +*/
> > + queue_work(system_unbound_wq, >submission_state.reset_worker);
> > return 0;
> > }
>
Re: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, 19 Jan 2022 21:22:57 +0200
Andy Shevchenko wrote:
> On Wed, Jan 19, 2022 at 04:38:26PM +, David Laight wrote:
> > > > > > +static inline const char *yesno(bool v) { return v ? "yes" : "no";
> > > > > > }
> > >
> > > return "yes\0no" + v * 4;
> > >
> > > :-)
> >
> > except '"no\0\0yes" + v * 4' works a bit better.
>
> Is it a C code obfuscation contest?
>
return '/'/'/';
-- Steve
Re: [PATCH 1/3] drm/i915: Allocate intel_engine_coredump_alloc with ALLOW_FAIL
On 1/19/2022 12:47, Matthew Brost wrote:
On Tue, Jan 18, 2022 at 05:29:54PM -0800, John Harrison wrote:
On 1/18/2022 13:43, Matthew Brost wrote:
Allocate intel_engine_coredump_alloc with ALLOW_FAIL rather than
GFP_KERNEL do fully decouple the error capture from fence signalling.
s/do/to/
Yep.
Fixes: 8b91cdd4f8649 ("drm/i915: Use __GFP_KSWAPD_RECLAIM in the capture code")
Does this really count as a bug fix over that patch? Isn't it more of a
changing in policy now that we do DRM fence signalling and that other
changes related to error capture behaviour have been implemented.
That patch was supposed to allow signalling annotations to be added,
without this change I think these annotations would be broken. So I
think the Fixes is correct.
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 67f3515f07e7a..aee42eae4729f 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1516,7 +1516,7 @@ capture_engine(struct intel_engine_cs *engine,
struct i915_request *rq = NULL;
unsigned long flags;
- ee = intel_engine_coredump_alloc(engine, GFP_KERNEL);
+ ee = intel_engine_coredump_alloc(engine, ALLOW_FAIL);
This still makes me nervous that we will fail to allocate engine captures in
stress test scenarios, which are exactly the kind of situations where we
need valid error captures.
Me too, but this whole file has been changed to the ALLOW_FAIL. Thomas
and Daniel seem to think this is correct. For what it's worth this
allocation is less than a page, so it should be pretty safe to do with
ALLOW_FAIL.
There is also still a GFP_KERNEL in __i915_error_grow(). Doesn't that need
updating as well?
Probably just should be deleted. If look it tries with ALLOW_FAIL first,
then falls back to GFP_KERNEL. I didn't want to make that update in this
series yet but that is something to keep an eye on.
Matt
Okay. Makes sense. With the description typo fixed:
Reviewed-by: John Harrison
John.
if (!ee)
return NULL;
Re: [Intel-gfx] [PATCH 0/3] lib/string_helpers: Add a few string helpers
On Wed, Jan 19, 2022 at 05:15:02PM +0100, Daniel Vetter wrote: On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote: On Wed, 19 Jan 2022, Petr Mladek wrote: > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote: >> Add some helpers under lib/string_helpers.h so they can be used >> throughout the kernel. When I started doing this there were 2 other >> previous attempts I know of, not counting the iterations each of them >> had: >> >> 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nik...@intel.com/ >> 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevche...@linux.intel.com/#t >> >> Going through the comments I tried to find some common ground and >> justification for what is in here, addressing some of the concerns >> raised. >> >> d. This doesn't bring onoff() helper as there are some places in the >>kernel with onoff as variable - another name is probably needed for >>this function in order not to shadow the variable, or those variables >>could be renamed. Or if people wanting >>try to find a short one > > I would call it str_on_off(). > > And I would actually suggest to use the same style also for > the other helpers. > > The "str_" prefix would make it clear that it is something with > string. There are other _on_off() that affect some > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off(). > > The dash '_' would significantly help to parse the name. yesno() and > onoff() are nicely short and kind of acceptable. But "enabledisable()" > is a puzzle. > > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good > compromise. > > The main motivation should be code readability. You write the > code once. But many people will read it many times. Open coding > is sometimes better than misleading macro names. > > That said, I do not want to block this patchset. If others like > it... ;-) I don't mind the names either way. Adding the prefix and dashes is helpful in that it's possible to add the functions first and convert users at leisure, though with a bunch of churn, while using names that collide with existing ones requires the changes to happen in one go. What I do mind is grinding this series to a halt once again. I sent a handful of versions of this three years ago, with inconclusive bikeshedding back and forth, eventually threw my hands up in disgust, and walked away. Yeah we can sed this anytime later we want to, but we need to get the foot in the door. There's also a pile more of these all over. Acked-by: Daniel Vetter on the series, maybe it helps? And yes let's merge this through drm-misc. Ok, it seems we are reaching some agreement here then: - Change it to use str_ prefix - Wait -rc1 to avoid conflict - Merge through drm-misc I will re-send the series again soon. Lucas De Marchi
Re: [PATCH 1/3] drm/i915: Allocate intel_engine_coredump_alloc with ALLOW_FAIL
On Tue, Jan 18, 2022 at 05:29:54PM -0800, John Harrison wrote:
> On 1/18/2022 13:43, Matthew Brost wrote:
> > Allocate intel_engine_coredump_alloc with ALLOW_FAIL rather than
> > GFP_KERNEL do fully decouple the error capture from fence signalling.
> s/do/to/
>
Yep.
> >
> > Fixes: 8b91cdd4f8649 ("drm/i915: Use __GFP_KSWAPD_RECLAIM in the capture
> > code")
> Does this really count as a bug fix over that patch? Isn't it more of a
> changing in policy now that we do DRM fence signalling and that other
> changes related to error capture behaviour have been implemented.
>
That patch was supposed to allow signalling annotations to be added,
without this change I think these annotations would be broken. So I
think the Fixes is correct.
> >
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 67f3515f07e7a..aee42eae4729f 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1516,7 +1516,7 @@ capture_engine(struct intel_engine_cs *engine,
> > struct i915_request *rq = NULL;
> > unsigned long flags;
> > - ee = intel_engine_coredump_alloc(engine, GFP_KERNEL);
> > + ee = intel_engine_coredump_alloc(engine, ALLOW_FAIL);
> This still makes me nervous that we will fail to allocate engine captures in
> stress test scenarios, which are exactly the kind of situations where we
> need valid error captures.
>
Me too, but this whole file has been changed to the ALLOW_FAIL. Thomas
and Daniel seem to think this is correct. For what it's worth this
allocation is less than a page, so it should be pretty safe to do with
ALLOW_FAIL.
> There is also still a GFP_KERNEL in __i915_error_grow(). Doesn't that need
> updating as well?
>
Probably just should be deleted. If look it tries with ALLOW_FAIL first,
then falls back to GFP_KERNEL. I didn't want to make that update in this
series yet but that is something to keep an eye on.
Matt
> John.
>
> > if (!ee)
> > return NULL;
>
Re: [Intel-gfx] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, Jan 19, 2022 at 10:06:35AM -0500, Steven Rostedt wrote:
On Wed, 19 Jan 2022 11:18:59 +0200
Sakari Ailus wrote:
On Tue, Jan 18, 2022 at 11:24:48PM -0800, Lucas De Marchi wrote:
> @@ -1354,8 +1345,7 @@ static bool tomoyo_print_condition(struct
tomoyo_io_buffer *head,
>case 3:
>if (cond->grant_log != TOMOYO_GRANTLOG_AUTO)
>tomoyo_io_printf(head, " grant_log=%s",
> - tomoyo_yesno(cond->grant_log ==
> -TOMOYO_GRANTLOG_YES));
> + yesno(cond->grant_log ==
TOMOYO_GRANTLOG_YES));
This would be better split on two lines.
Really? Yuck!
I thought the "max line size" guideline was going to grow to a 100, but I
still see it as 80. But anyway...
Checking that: docs still say 80, but checkpatch was changed to warn
only on 100. Commit bdc48fa11e46 ("checkpatch/coding-style: deprecate
80-column warning") is clear why the discrepancy.
Lucas De Marchi
cond->grant_log ==
TOMOYO_GRANTLOG_YES
is not readable at all. Not compared to
cond->grant_log == TOMOYO_GRANTLOG_YES
I say keep it one line!
Reviewed-by: Steven Rostedt (Google)
-- Steve
Then,
Reviewed-by: Sakari Ailus
Re: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation
On Wed, Jan 19, 2022 at 1:58 AM Guangming.Cao wrote: > On Fri, 2022-01-14 at 17:17 -0800, John Stultz wrote: > > If the max value is per-heap, why not enforce that value in the > > per-heap allocation function? > > > > Moving the check to the heap alloc to me seems simpler to me than > > adding complexity to the infrastructure to add a heap max_size > > callback. Is there some other use for the callback that you envision? > > > > If you think max the value is per-heap, why not add an optional > callback for dma-heap to solve this issue(prevent consuming too much > time for a doomed to fail allocation), if the dma-heap doesn't have a > special size check, just use the default value(totalram) in dma-heap > framework to do the size check. As the totalram default isn't correct for all heaps (or necessarily even most heaps), so those heaps would need to implement the callback. I'm just not sure adding complexity to the framework to address this is useful. Instead of an additional check in the allocation function, heap implementers will need to assess if the default logic in a framework is correct, and then possibly implement the callback. > Yes, for linux dma-heaps, only system-heap needs it, so adding it in > system heap is the simplest. However, there are many vendor dma-heaps > like system-heap which won't be uploaded to linux codebase, and maybe > have same limitation, all these heaps need to add the same limitation. My worry is that without seeing these vendor heaps, this is a bit of a theoretical concern. We don't have the data on how common this is. I very much hope that vendors can start submitting their heaps upstream (along with drivers that benefit from the heaps). Then we can really assess what makes the most sense for the community maintained code. > I just think it's boring. However, If you think discussing these absent > cases based on current linux code is meaningless, I also agree to it. So, as a rule, the upstream kernel doesn't create/maintain logic to accommodate out of tree code. Now, I agree there is the potential for some duplication in the checks in the allocation logic, but until it affects the upstream kernel, community maintainers can't really make an appropriate evaluation. As a contra-example, if the allocation is some extreme hotpath, adding an extra un-inlinable function pointer traversal for the size callback may actually have a negative impact. This isn't likely but again, if we cannot demonstrate it one way or the other against the upstream tree, we can't figure out what the best solution might be. > So, to summarize, if you still think adding it in system_heap.c is > better, I also agree and I will update the patch to add it in > system_heap.c I think this is the best solution for now. As this is not part of an userland ABI, we can always change it in the future once we see the need. thanks -john
[PATCH v3 0/2] Add support for querying hw info that UMDs need
From: John Harrison Various UMDs require hardware configuration information about the current platform. A bunch of static information is available in a fixed table that can be retrieved from the GuC. v2: Rebased to newer baseline and added a kerneldoc comment. v3: Rebased to newer baseline and newer GuC interface. Test-with: 20220119200137.2364940-2-john.c.harri...@intel.com UMD: https://github.com/intel/compute-runtime/pull/432/files UMD: https://github.com/intel/media-driver/pull/1239/files CC: Katarzyna Cencelewska CC: Tony Ye CC: Jason Ekstrand Signed-off-by: John Harrison Reviewed-by: Matthew Brost John Harrison (1): drm/i915/guc: Add fetch of hwconfig table Rodrigo Vivi (1): drm/i915/uapi: Add query for hwconfig table drivers/gpu/drm/i915/Makefile | 1 + .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 1 + .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h | 4 + drivers/gpu/drm/i915/gt/uc/intel_guc.h| 3 + .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 151 ++ .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h | 19 +++ drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 + drivers/gpu/drm/i915/i915_query.c | 23 +++ include/uapi/drm/i915_drm.h | 1 + 9 files changed, 209 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h -- 2.25.1
[PATCH v3 2/2] drm/i915/uapi: Add query for hwconfig table
From: Rodrigo Vivi
GuC contains a consolidated table with a bunch of information about the
current device.
Previously, this information was spread and hardcoded to all the components
including GuC, i915 and various UMDs. The goal here is to consolidate
the data into GuC in a way that all interested components can grab the
very latest and synchronized information using a simple query.
As per most of the other queries, this one can be called twice.
Once with item.length=0 to determine the exact buffer size, then
allocate the user memory and call it again for to retrieve the
table data. For example:
struct drm_i915_query_item item = {
.query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
};
query.items_ptr = (int64_t)
query.num_items = 1;
ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
if (item.length <= 0)
return -ENOENT;
data = malloc(item.length);
item.data_ptr = (int64_t)
ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
// Parse the data as appropriate...
The returned array is a simple and flexible KLV (Key/Length/Value)
formatted table. For example, it could be just:
enum device_attr {
ATTR_SOME_VALUE = 0,
ATTR_SOME_MASK = 1,
};
static const u32 hwconfig[] = {
ATTR_SOME_VALUE,
1, // Value Length in DWords
8, // Value
ATTR_SOME_MASK,
3,
0x00, 0x, 0xFF00,
};
The attribute ids are defined in a hardware spec.
Cc: Tvrtko Ursulin
Cc: Kenneth Graunke
Cc: Michal Wajdeczko
Cc: Slawomir Milczarek
Signed-off-by: Rodrigo Vivi
Signed-off-by: John Harrison
Reviewed-by: Matthew Brost
---
drivers/gpu/drm/i915/i915_query.c | 23 +++
include/uapi/drm/i915_drm.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..609e64d5f395 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private
*i915,
return total_length;
}
+static int query_hwconfig_table(struct drm_i915_private *i915,
+ struct drm_i915_query_item *query_item)
+{
+ struct intel_gt *gt = to_gt(i915);
+ struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig;
+
+ if (!hwconfig->size || !hwconfig->ptr)
+ return -ENODEV;
+
+ if (query_item->length == 0)
+ return hwconfig->size;
+
+ if (query_item->length < hwconfig->size)
+ return -EINVAL;
+
+ if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+hwconfig->ptr, hwconfig->size))
+ return -EFAULT;
+
+ return hwconfig->size;
+}
+
static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item)
= {
query_topology_info,
query_engine_info,
query_perf_config,
query_memregion_info,
+ query_hwconfig_table,
};
int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 914ebd9290e5..132515199f27 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
#define DRM_I915_QUERY_ENGINE_INFO 2
#define DRM_I915_QUERY_PERF_CONFIG 3
#define DRM_I915_QUERY_MEMORY_REGIONS 4
+#define DRM_I915_QUERY_HWCONFIG_TABLE 5
/* Must be kept compact -- no holes and well documented */
/**
--
2.25.1
[PATCH v3 1/2] drm/i915/guc: Add fetch of hwconfig table
From: John Harrison
Implement support for fetching the hardware description table from the
GuC. The call is made twice - once without a destination buffer to
query the size and then a second time to fill in the buffer.
Note that the table is only available on ADL-P and later platforms.
Cc: Michal Wajdeczko
Signed-off-by: Rodrigo Vivi
Signed-off-by: John Harrison
Reviewed-by: Matthew Brost
---
drivers/gpu/drm/i915/Makefile | 1 +
.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 1 +
.../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h | 4 +
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 3 +
.../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c | 151 ++
.../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h | 19 +++
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 +
7 files changed, 185 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index f1a9a648ce09..23f6b264d260 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -193,6 +193,7 @@ i915-y += gt/uc/intel_uc.o \
gt/uc/intel_guc_rc.o \
gt/uc/intel_guc_slpc.o \
gt/uc/intel_guc_submission.o \
+ gt/uc/intel_guc_hwconfig.o \
gt/uc/intel_huc.o \
gt/uc/intel_huc_debugfs.o \
gt/uc/intel_huc_fw.o
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
index 7afdadc7656f..a9a329e53c35 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
@@ -129,6 +129,7 @@ enum intel_guc_action {
INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009,
INTEL_GUC_ACTION_SETUP_PC_GUCRC = 0x3004,
INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
+ INTEL_GUC_ACTION_GET_HWCONFIG = 0x4100,
INTEL_GUC_ACTION_REGISTER_CONTEXT = 0x4502,
INTEL_GUC_ACTION_DEREGISTER_CONTEXT = 0x4503,
INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
index c20658ee85a5..8085fb181274 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
@@ -8,6 +8,10 @@
enum intel_guc_response_status {
INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0,
+ INTEL_GUC_RESPONSE_NOT_SUPPORTED = 0x20,
+ INTEL_GUC_RESPONSE_NO_ATTRIBUTE_TABLE = 0x201,
+ INTEL_GUC_RESPONSE_NO_DECRYPTION_KEY = 0x202,
+ INTEL_GUC_RESPONSE_DECRYPTION_FAILED = 0x204,
INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000,
};
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 9d26a86fe557..bc785403097f 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -13,6 +13,7 @@
#include "intel_guc_fw.h"
#include "intel_guc_fwif.h"
#include "intel_guc_ct.h"
+#include "intel_guc_hwconfig.h"
#include "intel_guc_log.h"
#include "intel_guc_reg.h"
#include "intel_guc_slpc_types.h"
@@ -37,6 +38,8 @@ struct intel_guc {
struct intel_guc_ct ct;
/** @slpc: sub-structure containing SLPC related data and objects */
struct intel_guc_slpc slpc;
+ /** @hwconfig: hardware configuration KLV table */
+ struct intel_guc_hwconfig hwconfig;
/** @sched_engine: Global engine used to submit requests to GuC */
struct i915_sched_engine *sched_engine;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
new file mode 100644
index ..ce6088f112d4
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "gt/intel_gt.h"
+#include "i915_drv.h"
+#include "i915_memcpy.h"
+#include "intel_guc_hwconfig.h"
+
+static inline struct intel_guc *hwconfig_to_guc(struct intel_guc_hwconfig
*hwconfig)
+{
+ return container_of(hwconfig, struct intel_guc, hwconfig);
+}
+
+/*
+ * GuC has a blob containing hardware configuration information (HWConfig).
+ * This is formatted as a simple and flexible KLV (Key/Length/Value) table.
+ *
+ * For example, a minimal version could be:
+ * enum device_attr {
+ * ATTR_SOME_VALUE = 0,
+ * ATTR_SOME_MASK = 1,
+ * };
+ *
+ * static const u32 hwconfig[] = {
+ * ATTR_SOME_VALUE,
+ * 1, // Value Length in DWords
+ * 8, // Value
+ *
+ * ATTR_SOME_MASK,
+ * 3,
+ * 0x00, 0x, 0xFF00,
+ * };
+ *
+ * The attribute ids are defined in a hardware spec.
+ */
+
+static int __guc_action_get_hwconfig(struct intel_guc_hwconfig *hwconfig,
+u32
Re: [Freedreno] [PATCH v2 1/2] drm/msm/dpu: simplify clocks handling
On 19/01/2022 05:32, Jessica Zhang wrote:
On 11/25/2021 6:35 PM, Dmitry Baryshkov wrote:
DPU driver contains code to parse clock items from device tree into
special data struct and then enable/disable/set rate for the clocks
using that data struct. However the DPU driver itself uses only parsing
and enabling/disabling part (the rate setting is used by DP driver).
Move this implementation to the DP driver (which actually uses rate
setting) and replace hand-coded enable/disable/get loops in the DPU
with the respective clk_bulk operations. Put operation is removed
completely because, it is handled using devres instead.
DP implementation is unchanged for now.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/Makefile | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 24 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 46 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 26 +++
.../dpu1/dpu_io_util.c => dp/dp_clk_util.c} | 69 +--
.../dpu1/dpu_io_util.h => dp/dp_clk_util.h} | 2 -
drivers/gpu/drm/msm/dp/dp_parser.h | 2 +-
drivers/gpu/drm/msm/msm_drv.c | 49 +
drivers/gpu/drm/msm/msm_drv.h | 1 +
11 files changed, 84 insertions(+), 147 deletions(-)
rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.c =>
dp/dp_clk_util.c} (61%)
rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.h =>
dp/dp_clk_util.h} (92%)
[skipped]
@@ -174,14 +174,10 @@ static int dpu_mdss_enable(struct msm_mdss *mdss)
static int dpu_mdss_disable(struct msm_mdss *mdss)
{
struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
- struct dss_module_power *mp = _mdss->mp;
- int ret;
- ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
- if (ret)
- DPU_ERROR("clock disable failed, ret:%d\n", ret);
+ clk_bulk_disable_unprepare(dpu_mdss->num_clocks, dpu_mdss->clocks);
- return ret;
+ return 0;
}
static void dpu_mdss_destroy(struct drm_device *dev)
Hi Dmitry,
Looks like this is based on some outdated code:
2027e5b3 (drm/msm: Initialize MDSS irq domain at probe time) changes
`*dev` to `*mdss`
I want to test this patch on some boards (namely RB3 and RB5). Can you
release a version with your changes rebased on top of the tip of msm-next?
Sure, I'll post v3.
@@ -189,7 +185,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
struct platform_device *pdev = to_platform_device(dev->dev);
struct msm_drm_private *priv = dev->dev_private;
struct dpu_mdss *dpu_mdss = to_dpu_mdss(priv->mdss);
- struct dss_module_power *mp = _mdss->mp;
int irq;
pm_runtime_suspend(dev->dev);
@@ -197,8 +192,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
_dpu_mdss_irq_domain_fini(dpu_mdss);
irq = platform_get_irq(pdev, 0);
irq_set_chained_handler_and_data(irq, NULL, NULL);
- msm_dss_put_clk(mp->clk_config, mp->num_clk);
- devm_kfree(>dev, mp->clk_config);
if (dpu_mdss->mmio)
devm_iounmap(>dev, dpu_mdss->mmio);
@@ -217,7 +210,6 @@ int dpu_mdss_init(struct drm_device *dev)
struct platform_device *pdev = to_platform_device(dev->dev);
struct msm_drm_private *priv = dev->dev_private;
struct dpu_mdss *dpu_mdss;
- struct dss_module_power *mp;
int ret;
int irq;
@@ -231,12 +223,12 @@ int dpu_mdss_init(struct drm_device *dev)
DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
- mp = _mdss->mp;
- ret = msm_dss_parse_clock(pdev, mp);
- if (ret) {
+ ret = msm_parse_clock(pdev, _mdss->clocks);
+ if (ret < 0) {
DPU_ERROR("failed to parse clocks, ret=%d\n", ret);
goto clk_parse_err;
}
+ dpu_mdss->num_clocks = ret;
dpu_mdss->base.dev = dev;
dpu_mdss->base.funcs = _funcs;
@@ -263,9 +255,7 @@ int dpu_mdss_init(struct drm_device *dev)
irq_error:
_dpu_mdss_irq_domain_fini(dpu_mdss);
irq_domain_error:
- msm_dss_put_clk(mp->clk_config, mp->num_clk);
clk_parse_err:
- devm_kfree(>dev, mp->clk_config);
if (dpu_mdss->mmio)
devm_iounmap(>dev, dpu_mdss->mmio);
dpu_mdss->mmio = NULL;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
b/drivers/gpu/drm/msm/dp/dp_clk_util.c
similarity index 61%
rename from drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
rename to drivers/gpu/drm/msm/dp/dp_clk_util.c
index 078afc5f5882..44a4fc59ff31 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
+++ b/drivers/gpu/drm/msm/dp/dp_clk_util.c
@@ -11,7 +11,7 @@
#include
-#include "dpu_io_util.h"
+#include "dp_clk_util.h"
void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk)
{
@@ -118,70 +118,3 @@ int msm_dss_enable_clk(struct dss_clk *clk_arry,
int num_clk, int enable)
return rc;
}
-
-int msm_dss_parse_clock(struct platform_device *pdev,
-
Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output
On Wednesday, 19 January 2022 07:40:22 CET H. Nikolaus Schaller wrote: > Hi Paul, > > > Am 18.01.2022 um 23:59 schrieb Paul Boddie : > > > > On Tuesday, 18 January 2022 17:58:58 CET Paul Cercueil wrote: > >> > >> Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working > >> fine last time I tried, and now I indeed get a black screen unless this > >> bit is set. The PM doesn't make it obvious that the bit is required, > >> but that wouldn't be surprising. > > > > It isn't actually needed. If the DMA descriptors are set up appropriately, > > the OSD alpha bit seems to be set as a consequence. In my non-Linux > > testing environment I don't even set any OSD registers explicitly, but > > the OSD alpha and enable flags become set when the display is active. > > Is it set by DMA descriptors or by explicit code? The descriptors will cause it to be set when the peripheral is enabled, as far as I can tell. > We did have an explicit setting of JZ_LCD_OSDC_ALPHAEN > > https://www.spinics.net/lists/devicetree/msg438447.html > > but that was postponed for further discussion. And now if we > add it (from basic functionality) back, it is fine again. It may be set in various versions of the Linux driver, but my observation was that in a non-Linux environment where nothing else is setting anything in the register concerned, initialising the descriptors seems to enable OSD and the OSD alpha enable bit. Yesterday, I did consider what might be done to avoid the alpha bit being set, but I didn't immediately see anything in the descriptor fields that would offer such an alternative. The bit in question seems to be a global alpha enable setting, and so choosing per-pixel alpha would probably also result in it being set, although I didn't fire up the CI20 to check. Paul
Re: [PATCH v2 4/4] drm/i915/uapi: document behaviour for DG2 64K support
On 19/01/2022 18:36, Jordan Justen wrote:
Robert Beckett writes:
From: Matthew Auld
On discrete platforms like DG2, we need to support a minimum page size
of 64K when dealing with device local-memory. This is quite tricky for
various reasons, so try to document the new implicit uapi for this.
v2: Fixed suggestions on formatting [Daniel]
Signed-off-by: Matthew Auld
Signed-off-by: Ramalingam C
Signed-off-by: Robert Beckett
cc: Simon Ser
cc: Pekka Paalanen
Cc: Jordan Justen
Cc: Kenneth Graunke
Cc: mesa-...@lists.freedesktop.org
Cc: Tony Ye
Cc: Slawomir Milczarek
---
include/uapi/drm/i915_drm.h | 44 -
1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 5e678917da70..486b7b96291e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1118,10 +1118,16 @@ struct drm_i915_gem_exec_object2 {
/**
* When the EXEC_OBJECT_PINNED flag is specified this is populated by
* the user with the GTT offset at which this object will be pinned.
+*
* When the I915_EXEC_NO_RELOC flag is specified this must contain the
* presumed_offset of the object.
+*
* During execbuffer2 the kernel populates it with the value of the
* current GTT offset of the object, for future presumed_offset writes.
+*
+* See struct drm_i915_gem_create_ext for the rules when dealing with
+* alignment restrictions with I915_MEMORY_CLASS_DEVICE, on devices with
+* minimum page sizes, like DG2.
*/
__u64 offset;
@@ -3145,11 +3151,39 @@ struct drm_i915_gem_create_ext {
*
* The (page-aligned) allocated size for the object will be returned.
*
-* Note that for some devices we have might have further minimum
-* page-size restrictions(larger than 4K), like for device local-memory.
-* However in general the final size here should always reflect any
-* rounding up, if for example using the
I915_GEM_CREATE_EXT_MEMORY_REGIONS
-* extension to place the object in device local-memory.
+*
+* **DG2 64K min page size implications:**
Long term, I'm not sure that the "**" (for emphasis) is needed here or
below. It's interesting at the moment, but will be just another thing
baked into the kernel/user code in a month from now. :)
fair point, I'll make it less shouty
+*
+* On discrete platforms, starting from DG2, we have to contend with GTT
+* page size restrictions when dealing with I915_MEMORY_CLASS_DEVICE
+* objects. Specifically the hardware only supports 64K or larger GTT
+* page sizes for such memory. The kernel will already ensure that all
+* I915_MEMORY_CLASS_DEVICE memory is allocated using 64K or larger page
+* sizes underneath.
+*
+* Note that the returned size here will always reflect any required
+* rounding up done by the kernel, i.e 4K will now become 64K on devices
+* such as DG2.
+*
+* **Special DG2 GTT address alignment requirement:**
+*
+* The GTT alignment will also need be at least 2M for such objects.
+*
+* Note that due to how the hardware implements 64K GTT page support, we
+* have some further complications:
+*
+* 1) The entire PDE(which covers a 2MB virtual address range), must
+* contain only 64K PTEs, i.e mixing 4K and 64K PTEs in the same
+* PDE is forbidden by the hardware.
+*
+* 2) We still need to support 4K PTEs for I915_MEMORY_CLASS_SYSTEM
+* objects.
+*
+* To keep things simple for userland, we mandate that any GTT mappings
+* must be aligned to and rounded up to 2MB. As this only wastes virtual
+* address space and avoids userland having to copy any needlessly
+* complicated PDE sharing scheme (coloring) and only affects GD2, this
+* id deemed to be a good compromise.
typos: GD2, id
thanks
Isn't much of this more relavent to the vma offset at exec time? Is
there actually any new restriction on the size field during buffer
creation?
No new restriction on size, just placement, which mesa is already doing.
The request for ack was just to get an ack from mesa folks that they are
happy with the mandatory 2MB alignment for DG2 vma.
I see Matthew references these notes from the offset comments, so if the
kernel devs prefer it here, then you can add my Acked-by on this patch.
thanks!
-Jordan
*/
__u64 size;
/**
--
2.25.1
Re: [PATCH 3/3] drm: Convert open yes/no strings to yesno()
On Tue, Jan 18, 2022 at 11:24:50PM -0800, Lucas De Marchi wrote: > linux/string_helpers.h provides a helper to return "yes"/"no" > strings. Replace the open coded versions with yesno(). The places were > identified with the following semantic patch: > > @@ > expression b; > @@ > > - b ? "yes" : "no" > + yesno(b) > > Then the includes were added, so we include-what-we-use, and parenthesis > adjusted in drivers/gpu/drm/v3d/v3d_debugfs.c. After the conversion we > still see the same binary sizes: > >textdata bss dec hex filename > 1442171 60344 800 1503315 16f053 ./drivers/gpu/drm/radeon/radeon.ko > 1442171 60344 800 1503315 16f053 ./drivers/gpu/drm/radeon/radeon.ko.old > 5985991 324439 33808 6344238 60ce2e ./drivers/gpu/drm/amd/amdgpu/amdgpu.ko > 5985991 324439 33808 6344238 60ce2e > ./drivers/gpu/drm/amd/amdgpu/amdgpu.ko.old > 411986 104906176 428652 68a6c ./drivers/gpu/drm/drm.ko > 411986 104906176 428652 68a6c ./drivers/gpu/drm/drm.ko.old > 1970292 1095152352 2082159 1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko > 1970292 1095152352 2082159 1fc56f > ./drivers/gpu/drm/nouveau/nouveau.ko.old ... > #include > #include > #include > +#include + blank line? > +#include ... > seq_printf(m, "\tDP branch device present: %s\n", > -branch_device ? "yes" : "no"); > +yesno(branch_device)); Now it's possible to keep this on one line. ... > drm_printf_indent(p, indent, "imported=%s\n", > - obj->import_attach ? "yes" : "no"); > + yesno(obj->import_attach)); 81 here, but anyway, ditto! ... > */ +blank line here? > +#include > + > #include "aux.h" > #include "pad.h" ... > seq_printf(m, "MMU:%s\n", > -(ident2 & V3D_HUB_IDENT2_WITH_MMU) ? "yes" : "no"); > +yesno(ident2 & V3D_HUB_IDENT2_WITH_MMU)); > seq_printf(m, "TFU:%s\n", > -(ident1 & V3D_HUB_IDENT1_WITH_TFU) ? "yes" : "no"); > +yesno(ident1 & V3D_HUB_IDENT1_WITH_TFU)); > seq_printf(m, "TSY:%s\n", > -(ident1 & V3D_HUB_IDENT1_WITH_TSY) ? "yes" : "no"); > +yesno(ident1 & V3D_HUB_IDENT1_WITH_TSY)); > seq_printf(m, "MSO:%s\n", > -(ident1 & V3D_HUB_IDENT1_WITH_MSO) ? "yes" : "no"); > +yesno(ident1 & V3D_HUB_IDENT1_WITH_MSO)); > seq_printf(m, "L3C:%s (%dkb)\n", > -(ident1 & V3D_HUB_IDENT1_WITH_L3C) ? "yes" : "no", > +yesno(ident1 & V3D_HUB_IDENT1_WITH_L3C), > V3D_GET_FIELD(ident2, V3D_HUB_IDENT2_L3C_NKB)); I believe it's fine to join back to have less LOCs (yes, it will be 83 or so, but I believe in these cases it's very much okay). -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, Jan 19, 2022 at 04:38:26PM +, David Laight wrote:
> > > > > +static inline const char *yesno(bool v) { return v ? "yes" : "no"; }
> >
> > return "yes\0no" + v * 4;
> >
> > :-)
>
> except '"no\0\0yes" + v * 4' works a bit better.
Is it a C code obfuscation contest?
--
With Best Regards,
Andy Shevchenko
Re: [RFC PATCH v3 2/3] drm: add support modifiers for drivers whose planes only support linear layout
On Wed, Jan 19, 2022 at 11:35:22AM +0900, Esaki Tomohito wrote:
> On 2022/01/18 18:53, Andy Shevchenko wrote:
> > On Mon, Jan 17, 2022 at 02:15:48PM +0900, Esaki Tomohito wrote:
> > > On 2022/01/14 23:16, Andy Shevchenko wrote:
> > > > On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote:
> > > > > The LINEAR modifier is advertised as default if a driver doesn't
> > > > > specify
> > > > > modifiers.
> > > >
> > > > ...
> > > >
> > > > > + const uint64_t default_modifiers[] = {
> > > > > + DRM_FORMAT_MOD_LINEAR,
> > > > > + DRM_FORMAT_MOD_INVALID
> > > >
> > > > + Comma?
> > >
> > > There is no mention in the coding style about adding/removing a comma to
> > > the
> > > last element of an array. Is there a policy in drm driver?
> > >
> > > I think the advantage of adding a comma to the last element of an array is
> > > that diff is only one line when an element is added to the end.
> > > However since INVALID is always the last element in the modifiers array, I
> > > think it can be either in this case.
> > > If there is a policy, I will match it.
> >
> > Indeed, but there is a common sense. The idea behind (multi-line)
> > definitions
> > that when next time somebody will add an element in the array, there are
> > will
> > be:
> >
> > a) no additional churn (like in case of this patch, if the item will be
> > added
> > at the bottom;
> >
> > b) an element that may not be added behind the terminator, which will look
> > weird.
> >
> > That said, the question is if the element is terminator one or not, if not,
> > comma is better than no comma and vise versa.
>
> Ah I see. In this case, DRM_FORMAT_MOD_INVALID is terminator, so it
> should not have a comma.
Thanks for pointing this out. In this case we are good and any new item, AFAIU,
must be added before _INVALID one.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, Jan 19, 2022 at 10:06:35AM -0500, Steven Rostedt wrote: > On Wed, 19 Jan 2022 11:18:59 +0200 > Sakari Ailus wrote: > > On Tue, Jan 18, 2022 at 11:24:48PM -0800, Lucas De Marchi wrote: > > > @@ -1354,8 +1345,7 @@ static bool tomoyo_print_condition(struct > > > tomoyo_io_buffer *head, > > > case 3: > > > if (cond->grant_log != TOMOYO_GRANTLOG_AUTO) > > > tomoyo_io_printf(head, " grant_log=%s", > > > - tomoyo_yesno(cond->grant_log == > > > - TOMOYO_GRANTLOG_YES)); > > > + yesno(cond->grant_log == > > > TOMOYO_GRANTLOG_YES)); > > > > This would be better split on two lines. > > Really? Yuck! > > I thought the "max line size" guideline was going to grow to a 100, but I > still see it as 80. But anyway... > > cond->grant_log == > TOMOYO_GRANTLOG_YES > > is not readable at all. Not compared to > > cond->grant_log == TOMOYO_GRANTLOG_YES > > I say keep it one line! > > Reviewed-by: Steven Rostedt (Google) I believe Sakari strongly follows the 80 rule, which means... > > Reviewed-by: Sakari Ailus ...chose either of these tags and be happy with :-) -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 4/4] drm/i915/uapi: document behaviour for DG2 64K support
Robert Beckett writes:
> From: Matthew Auld
>
> On discrete platforms like DG2, we need to support a minimum page size
> of 64K when dealing with device local-memory. This is quite tricky for
> various reasons, so try to document the new implicit uapi for this.
>
> v2: Fixed suggestions on formatting [Daniel]
>
> Signed-off-by: Matthew Auld
> Signed-off-by: Ramalingam C
> Signed-off-by: Robert Beckett
> cc: Simon Ser
> cc: Pekka Paalanen
> Cc: Jordan Justen
> Cc: Kenneth Graunke
> Cc: mesa-...@lists.freedesktop.org
> Cc: Tony Ye
> Cc: Slawomir Milczarek
> ---
> include/uapi/drm/i915_drm.h | 44 -
> 1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 5e678917da70..486b7b96291e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1118,10 +1118,16 @@ struct drm_i915_gem_exec_object2 {
> /**
>* When the EXEC_OBJECT_PINNED flag is specified this is populated by
>* the user with the GTT offset at which this object will be pinned.
> + *
>* When the I915_EXEC_NO_RELOC flag is specified this must contain the
>* presumed_offset of the object.
> + *
>* During execbuffer2 the kernel populates it with the value of the
>* current GTT offset of the object, for future presumed_offset writes.
> + *
> + * See struct drm_i915_gem_create_ext for the rules when dealing with
> + * alignment restrictions with I915_MEMORY_CLASS_DEVICE, on devices with
> + * minimum page sizes, like DG2.
>*/
> __u64 offset;
>
> @@ -3145,11 +3151,39 @@ struct drm_i915_gem_create_ext {
>*
>* The (page-aligned) allocated size for the object will be returned.
>*
> - * Note that for some devices we have might have further minimum
> - * page-size restrictions(larger than 4K), like for device local-memory.
> - * However in general the final size here should always reflect any
> - * rounding up, if for example using the
> I915_GEM_CREATE_EXT_MEMORY_REGIONS
> - * extension to place the object in device local-memory.
> + *
> + * **DG2 64K min page size implications:**
Long term, I'm not sure that the "**" (for emphasis) is needed here or
below. It's interesting at the moment, but will be just another thing
baked into the kernel/user code in a month from now. :)
> + *
> + * On discrete platforms, starting from DG2, we have to contend with GTT
> + * page size restrictions when dealing with I915_MEMORY_CLASS_DEVICE
> + * objects. Specifically the hardware only supports 64K or larger GTT
> + * page sizes for such memory. The kernel will already ensure that all
> + * I915_MEMORY_CLASS_DEVICE memory is allocated using 64K or larger page
> + * sizes underneath.
> + *
> + * Note that the returned size here will always reflect any required
> + * rounding up done by the kernel, i.e 4K will now become 64K on devices
> + * such as DG2.
> + *
> + * **Special DG2 GTT address alignment requirement:**
> + *
> + * The GTT alignment will also need be at least 2M for such objects.
> + *
> + * Note that due to how the hardware implements 64K GTT page support, we
> + * have some further complications:
> + *
> + * 1) The entire PDE(which covers a 2MB virtual address range), must
> + * contain only 64K PTEs, i.e mixing 4K and 64K PTEs in the same
> + * PDE is forbidden by the hardware.
> + *
> + * 2) We still need to support 4K PTEs for I915_MEMORY_CLASS_SYSTEM
> + * objects.
> + *
> + * To keep things simple for userland, we mandate that any GTT mappings
> + * must be aligned to and rounded up to 2MB. As this only wastes virtual
> + * address space and avoids userland having to copy any needlessly
> + * complicated PDE sharing scheme (coloring) and only affects GD2, this
> + * id deemed to be a good compromise.
typos: GD2, id
Isn't much of this more relavent to the vma offset at exec time? Is
there actually any new restriction on the size field during buffer
creation?
I see Matthew references these notes from the offset comments, so if the
kernel devs prefer it here, then you can add my Acked-by on this patch.
-Jordan
>*/
> __u64 size;
> /**
> --
> 2.25.1
Re: [PATCH v3 00/10] drm: Make drivers to honour the nomodeset parameter
On 12/22/21 09:28, Javier Martinez Canillas wrote: > The nomodeset kernel command line parameter is used to prevent the KMS/DRM > drivers to be registered/probed. But only a few drivers implement support > for this and most DRM drivers just ignore it. > > This patch series is a v3 to make DRM drivers to honour nomodeset. It is > posted as separate patches to make easier for drivers maintainers to ack > or pick them independently at their own pace. > I've pushed this series to drm-misc-next. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
RE: [PATCH v2] hv: account for packet descriptor in maximum packet size
From: Wei Liu Sent: Friday, January 14, 2022 11:13 AM
>
> On Mon, Jan 10, 2022 at 01:44:19AM +0100, Andrea Parri wrote:
> > (Extending Cc: list,)
> >
> > On Sun, Jan 09, 2022 at 05:55:16PM +0800, Yanming Liu wrote:
> > > Commit adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V
> > > out of the ring buffer") introduced a notion of maximum packet size in
> > > vmbus channel and used that size to initialize a buffer holding all
> > > incoming packet along with their vmbus packet header. Currently, some
> > > vmbus drivers set max_pkt_size to the size of their receive buffer
> > > passed to vmbus_recvpacket, however vmbus_open expects this size to also
> > > include vmbus packet header. This leads to corruption of the ring buffer
> > > state when receiving a maximum sized packet.
> > >
> > > Specifically, in hv_balloon I have observed of a dm_unballoon_request
> > > message of 4096 bytes being truncated to 4080 bytes. When the driver
> > > tries to read next packet it starts from a wrong read_index, receives
> > > garbage and prints a lot of "Unhandled message: type: " in
> > > dmesg.
> > >
> > > The same mismatch also happens in hv_fcopy, hv_kvp, hv_snapshot,
> > > hv_util, hyperv_drm and hyperv_fb, though bad cases are not observed
> > > yet.
> > >
> > > Allocate the buffer with HV_HYP_PAGE_SIZE more bytes to make room for
> > > the descriptor, assuming the vmbus packet header will never be larger
> > > than HV_HYP_PAGE_SIZE. This is essentially free compared to just adding
> > > 'sizeof(struct vmpacket_descriptor)' because these buffers are all more
> > > than HV_HYP_PAGE_SIZE bytes so kmalloc rounds them up anyway.
> > >
> > > Fixes: adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V
> > > out of the ring buffer")
> > > Suggested-by: Andrea Parri (Microsoft)
> > > Signed-off-by: Yanming Liu
> >
> > Thanks for sorting this out; the patch looks good to me:
> >
> > Reviewed-by: Andrea Parri (Microsoft)
> >
>
> Thanks. I will pick this up after 5.17-rc1 is out.
>
> Wei.
I'm NACK'ing this set of changes. I've spent some further time investigating,
so let me explain.
I'm good with the overall approach of fixing individual drivers to set the
max_pkt_size to account for the VMbus packet header, as this is an
important aspect that was missed in the original coding. But interestingly,
all but one of the miscellaneous VMbus drivers allocate significantly more
receive buffer space than is actually needed, and the max_pkt_size matching
that receive buffer size is already bigger than needed. In all these
cases, there is already plenty of space for the VMbus packet header.
These hv-util.c drivers allocate a receive buffer 4 Kbytes in size, and all
receive only small fixed-size packets: heartbeat, shutdown, timesync.
I don't think any changes are needed for these drivers because the default
max_pkt_size value of 4 Kbytes bytes is plenty of space even when
accounting for the VMbus packet header.
The VSS driver in hv_snapshot.c allocates a receive buffer of 8 Kbytes
and sets max_pkt_size to 8 Kbytes. But the received messages are
all fixed size and small. I don't know why the driver uses an 8 Kbyte
receive buffer instead of 4 Kbytes, but the current settings are
more than sufficient.
The FCOPY driver in hv_fcopy.c allocates a receive buffer of 8 Kbytes
and sets max_pkt_size to 8 Kbytes. The received messages have
some header overhead plus up to 6 Kbytes of data, so the 8 Kbyte
receive buffer is definitely needed. And while this one is a little
closer to filling up the available receive space than the previous
ones, there's still plenty of room for the VMbus packet header. I
don't think any changes are needed.
The KVP driver in hv_kvp.c allocates a receive buffer of 16 Kbytes
and sets max_pkt_size to 16 Kbytes. From what I can tell, the
received messages max out at close to 4 Kbytes. Key exchange
messages have 512 bytes of key name and 2048 bytes of key
value, plus some header overhead. ipaddr_value messages
are the largest, with 3 IP addresses @ 1024 bytes each, plus
a gateway with 512 bytes, and an adapter ID with 128 bytes.
But altogether, that is still less than 4096. I don't know why
the receive buffer is 16 Kbytes, but it is plenty big and no
changes are needed.
The two frame buffer drivers also use 16 Kbyte receive buffers
and set max_pkt_size to 16 Kbytes. Again, this looks to be overkill
as the messages received are mostly fixed size. One message
returns a variable size list of supported screen resolutions, but
each entry in the list is only 4 bytes, and we're looking at a few
tens of resolutions at the very most. Again, no changes are
needed.
After all this analysis, the balloon driver is the only one that
needs changing. It uses a 4 Kbyte receive buffer, and indeed
Hyper-V may fill that receive buffer in the case of unballoon
messages. And that where the original problem was observed.
Two other aspects for completeness. First, all these drivers
[PATCH] drm/i915: Fix vma resource freeing
In some cases we use leftover kfree() instead of i915_vma_resource_free().
Fix this.
Fixes: Fixes: 2f6b90da9192 ("drm/i915: Use vma resources for async unbinding")
Reported-by: Robert Beckett
Cc: Matthew Auld
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/i915_vma.c | 4 ++--
drivers/gpu/drm/i915/i915_vma_resource.c | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index b959e904c4d3..b1816a835abf 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -464,7 +464,7 @@ int i915_vma_bind(struct i915_vma *vma,
if (vma->resource || !vma_res) {
/* Rebinding with an additional I915_VMA_*_BIND */
GEM_WARN_ON(!vma_flags);
- kfree(vma_res);
+ i915_vma_resource_free(vma_res);
} else {
i915_vma_resource_init_from_vma(vma_res, vma);
vma->resource = vma_res;
@@ -1462,7 +1462,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct
i915_gem_ww_ctx *ww,
err_unlock:
mutex_unlock(>vm->mutex);
err_vma_res:
- kfree(vma_res);
+ i915_vma_resource_free(vma_res);
err_fence:
if (work)
dma_fence_work_commit_imm(>base);
diff --git a/drivers/gpu/drm/i915/i915_vma_resource.c
b/drivers/gpu/drm/i915/i915_vma_resource.c
index 1f41c0c699eb..6426c2f8a3b4 100644
--- a/drivers/gpu/drm/i915/i915_vma_resource.c
+++ b/drivers/gpu/drm/i915/i915_vma_resource.c
@@ -62,7 +62,8 @@ struct i915_vma_resource *i915_vma_resource_alloc(void)
*/
void i915_vma_resource_free(struct i915_vma_resource *vma_res)
{
- kmem_cache_free(slab_vma_resources, vma_res);
+ if (vma_res)
+ kmem_cache_free(slab_vma_resources, vma_res);
}
static const char *get_driver_name(struct dma_fence *fence)
--
2.31.1
Re: [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions
On Wed, Jan 19, 2022 at 08:53:56AM -0800, Ira Weiny wrote: > On Fri, Dec 10, 2021 at 03:23:57PM -0800, 'Ira Weiny' wrote: > > From: Ira Weiny > > > > This series starts by converting the last easy kmap() uses to > > kmap_local_page(). > > > > There is one more call to kmap() wrapped in ttm_bo_kmap_ttm(). > > Unfortunately, > > ttm_bo_kmap_ttm() is called in a number of different ways including some > > which > > are not thread local. I have a patch to convert that call. However, it is > > not > > straight forward so it is not included in this series. > > > > The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm() > > conversion. > > Gentile ping on this series? Will it make this merge window? I think this fell through the cracks and so no. Note that generally we feature-freeze drm tree around -rc6 anyway for the upcoming merge window, so you were cutting this all a bit close anyway. Also looks like the ttm kmap caching question didn't get resolved? Anyway if patches are stuck resend with RESEND and if people still don't pick them up poke me and I'll apply as fallback. Cheers, Daniel > > Thanks, > Ira > > > > > > > Ira Weiny (7): > > drm/i915: Replace kmap() with kmap_local_page() > > drm/amd: Replace kmap() with kmap_local_page() > > drm/gma: Remove calls to kmap() > > drm/radeon: Replace kmap() with kmap_local_page() > > drm/msm: Alter comment to use kmap_local_page() > > drm/amdgpu: Ensure kunmap is called on error > > drm/radeon: Ensure kunmap is called on error > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 > > drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 + > > drivers/gpu/drm/gma500/gma_display.c | 6 ++ > > drivers/gpu/drm/gma500/mmu.c | 8 > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++-- > > drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8 > > drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++-- > > drivers/gpu/drm/i915/gt/shmem_utils.c | 4 ++-- > > drivers/gpu/drm/i915/i915_gem.c | 8 > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > > drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++-- > > drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++-- > > drivers/gpu/drm/radeon/radeon_uvd.c | 1 + > > 13 files changed, 32 insertions(+), 32 deletions(-) > > > > -- > > 2.31.1 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 4/4] dma-buf: warn about containers in dma_resv object
On Wed, Jan 19, 2022 at 02:43:39PM +0100, Christian König wrote: > Drivers should not add containers as shared fences to the dma_resv > object, instead each fence should be added individually. > > Signed-off-by: Christian König > --- > drivers/dma-buf/dma-resv.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 6dd9a40b55d4..e8a0c1d51da2 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -256,6 +256,11 @@ void dma_resv_add_shared_fence(struct dma_resv *obj, > struct dma_fence *fence) > > dma_resv_assert_held(obj); > > + /* Drivers should not add containers here, instead add each fence > + * individually. > + */ > + WARN_ON(dma_fence_is_container(fence)); I'm honestly not sure whether this could go boom, so good if we push this asap and let it soak in linux-next for the entire release cycle. Reviewed-by: Daniel Vetter On the change itself, we'll see what it brings. -Daniel > + > fobj = dma_resv_shared_list(obj); > count = fobj->shared_count; > > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 3/4] dma-buf: Warn about dma_fence_chain container rules
On Wed, Jan 19, 2022 at 02:43:38PM +0100, Christian König wrote: > Chaining of dma_fence_chain objects is only allowed through the prev > fence and not through the contained fence. > > Warn about that when we create a dma_fence_chain. > > Signed-off-by: Christian König > --- > drivers/dma-buf/dma-fence-chain.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/dma-buf/dma-fence-chain.c > b/drivers/dma-buf/dma-fence-chain.c > index 1b4cb3e5cec9..fa33f6b7f77b 100644 > --- a/drivers/dma-buf/dma-fence-chain.c > +++ b/drivers/dma-buf/dma-fence-chain.c > @@ -254,5 +254,13 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, > > dma_fence_init(>base, _fence_chain_ops, > >lock, context, seqno); > + > + /* Chaining dma_fence_chain container together is only allowed through > + * the prev fence and not through the contained fence. > + * > + * The correct way of handling this is to flatten out the fence > + * structure into a dma_fence_array by the caller instead. > + */ > + WARN_ON(dma_fence_is_chain(fence)); At first I was worried that you'd leave a chain fence in the array fence as an option, but we exclude that with the previous patch. Reviewed-by: Daniel Vetter > } > EXPORT_SYMBOL(dma_fence_chain_init); > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 2/4] dma-buf: warn about dma_fence_array container rules
On Wed, Jan 19, 2022 at 02:43:37PM +0100, Christian König wrote: > It's not allowed to nest another dma_fence container into a dma_fence_array > or otherwise we can run into recursion. > > Warn about that when we create a dma_fence_array. > > Signed-off-by: Christian König > --- > drivers/dma-buf/dma-fence-array.c | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/drivers/dma-buf/dma-fence-array.c > b/drivers/dma-buf/dma-fence-array.c > index 3e07f961e2f3..4bfbcb885bbc 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -176,6 +176,19 @@ struct dma_fence_array *dma_fence_array_create(int > num_fences, > > array->base.error = PENDING_ERROR; > > + /* dma_fence_array objects should never contain any other fence > + * containers or otherwise we run into recursion and potential kernel > + * stack overflow on operations on the dma_fence_array. > + * > + * The correct way of handling this is to flatten out the array by the > + * caller instead. > + * > + * Enforce this here by checking that we don't create a dma_fence_array > + * with any container inside. > + */ > + while (seqno--) > + WARN_ON(dma_fence_is_container(fences[seqno])); Reviewed-by: Daniel Vetter > + > return array; > } > EXPORT_SYMBOL(dma_fence_array_create); > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 1/4] dma-buf: consolidate dma_fence subclass checking
On Wed, Jan 19, 2022 at 02:43:36PM +0100, Christian König wrote:
> Consolidate the wrapper functions to check for dma_fence
> subclasses in the dma_fence header.
>
> This makes it easier to document and also check the different
> requirements for fence containers in the subclasses.
>
> Signed-off-by: Christian König
> ---
> include/linux/dma-fence-array.h | 15 +
> include/linux/dma-fence-chain.h | 3 +--
> include/linux/dma-fence.h | 38 +
> 3 files changed, 40 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h
> index 303dd712220f..fec374f69e12 100644
> --- a/include/linux/dma-fence-array.h
> +++ b/include/linux/dma-fence-array.h
> @@ -45,19 +45,6 @@ struct dma_fence_array {
> struct irq_work work;
> };
>
> -extern const struct dma_fence_ops dma_fence_array_ops;
> -
> -/**
> - * dma_fence_is_array - check if a fence is from the array subsclass
> - * @fence: fence to test
> - *
> - * Return true if it is a dma_fence_array and false otherwise.
> - */
> -static inline bool dma_fence_is_array(struct dma_fence *fence)
> -{
> - return fence->ops == _fence_array_ops;
> -}
> -
> /**
> * to_dma_fence_array - cast a fence to a dma_fence_array
> * @fence: fence to cast to a dma_fence_array
> @@ -68,7 +55,7 @@ static inline bool dma_fence_is_array(struct dma_fence
> *fence)
> static inline struct dma_fence_array *
> to_dma_fence_array(struct dma_fence *fence)
> {
> - if (fence->ops != _fence_array_ops)
> + if (!fence || !dma_fence_is_array(fence))
> return NULL;
>
> return container_of(fence, struct dma_fence_array, base);
> diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h
> index 54fe3443fd2c..ee906b659694 100644
> --- a/include/linux/dma-fence-chain.h
> +++ b/include/linux/dma-fence-chain.h
> @@ -49,7 +49,6 @@ struct dma_fence_chain {
> spinlock_t lock;
> };
>
> -extern const struct dma_fence_ops dma_fence_chain_ops;
>
> /**
> * to_dma_fence_chain - cast a fence to a dma_fence_chain
> @@ -61,7 +60,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops;
> static inline struct dma_fence_chain *
> to_dma_fence_chain(struct dma_fence *fence)
> {
> - if (!fence || fence->ops != _fence_chain_ops)
> + if (!fence || !dma_fence_is_chain(fence))
> return NULL;
>
> return container_of(fence, struct dma_fence_chain, base);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 1ea691753bd3..775cdc0b4f24 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -587,4 +587,42 @@ struct dma_fence *dma_fence_get_stub(void);
> struct dma_fence *dma_fence_allocate_private_stub(void);
> u64 dma_fence_context_alloc(unsigned num);
>
> +extern const struct dma_fence_ops dma_fence_array_ops;
> +extern const struct dma_fence_ops dma_fence_chain_ops;
> +
> +/**
> + * dma_fence_is_array - check if a fence is from the array subclass
> + * @fence: the fence to test
> + *
> + * Return true if it is a dma_fence_array and false otherwise.
> + */
> +static inline bool dma_fence_is_array(struct dma_fence *fence)
> +{
> + return fence->ops == _fence_array_ops;
> +}
> +
> +/**
> + * dma_fence_is_chain - check if a fence is from the chain subclass
> + * @fence: the fence to test
> + *
> + * Return true if it is a dma_fence_chain and false otherwise.
> + */
> +static inline bool dma_fence_is_chain(struct dma_fence *fence)
> +{
> + return fence->ops == _fence_chain_ops;
> +}
> +
> +/**
> + * dma_fence_is_container - check if a fence is a container for other fences
> + * @fence: the fence to test
> + *
> + * Return true if this fence is a container for other fences, false
> otherwise.
> + * This is important since we can't build up large fence structure or
> otherwise
> + * we run into recursion during operation on those fences.
> + */
> +static inline bool dma_fence_is_container(struct dma_fence *fence)
Code looks all good, but I'm not super enthusiastic about exporting the
ops to drivers and letting them do random nonsense. At least i915 does
pretty enormous amounts of stuff with that instead of having pushed
priority boosting into dma-fence as a proper concept. And maybe a few
other things.
Now i915-gem team having gone off the rails of good upstream conduct is
another thing maybe, but I'd like to not encourage that.
So could we perhaps do this all in header which is entirely private to
drivers/dma-buf, like dma-fence-internal or so? And maybe whack a big
fixme onto the current abuse in drivers (of which __dma_fence_is_chain()
gets a special price for "not how upstream should be done" *sigh*).
Cheers, Daniel
> +{
> + return dma_fence_is_array(fence) || dma_fence_is_chain(fence);
> +}
> +
> #endif /* __LINUX_DMA_FENCE_H */
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH v5 1/4] drm/bridge: anx7625: send DPCD command to downstream
On Wed, Jan 19, 2022 at 11:38 PM Robert Foss wrote:
>
> On Wed, 19 Jan 2022 at 16:17, Hsin-Yi Wang wrote:
> >
> > From: Xin Ji
> >
> > Send DPCD command to downstream before anx7625 power down,
> > let downstream monitor enter into standby mode.
> >
> > Signed-off-by: Xin Ji
> > Signed-off-by: Hsin-Yi Wang
Reviewed-by: Hsin-Yi Wang
>
> Hsin-Yi: Can you supply a r-b tag to this patch if it looks good to you?
>
> > ---
> > v3->v4:
> > Use common DP_AUX_NATIVE_READ/WRITE
> >
> > Previously in:
> > https://patchwork.kernel.org/project/dri-devel/patch/1f36f8bf0a48fb2bba17bacec23700e58c1d407d.1641891874.git@analogixsemi.com/
> > ---
> > drivers/gpu/drm/bridge/analogix/anx7625.c | 42 +++
> > drivers/gpu/drm/bridge/analogix/anx7625.h | 2 --
> > 2 files changed, 35 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 76662fce4ce61d..17b23940549a42 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -129,6 +129,23 @@ static int anx7625_reg_write(struct anx7625_data *ctx,
> > return ret;
> > }
> >
> > +static int anx7625_reg_block_write(struct anx7625_data *ctx,
> > + struct i2c_client *client,
> > + u8 reg_addr, u8 len, u8 *buf)
> > +{
> > + int ret;
> > + struct device *dev = >dev;
> > +
> > + i2c_access_workaround(ctx, client);
> > +
> > + ret = i2c_smbus_write_i2c_block_data(client, reg_addr, len, buf);
> > + if (ret < 0)
> > + dev_err(dev, "write i2c block failed id=%x\n:%x",
> > + client->addr, reg_addr);
> > +
> > + return ret;
> > +}
> > +
> > static int anx7625_write_or(struct anx7625_data *ctx,
> > struct i2c_client *client,
> > u8 offset, u8 mask)
> > @@ -214,8 +231,8 @@ static int wait_aux_op_finish(struct anx7625_data *ctx)
> > return 0;
> > }
> >
> > -static int anx7625_aux_dpcd_read(struct anx7625_data *ctx,
> > -u32 address, u8 len, u8 *buf)
> > +static int anx7625_aux_dpcd_trans(struct anx7625_data *ctx, u8 op,
> > + u32 address, u8 len, u8 *buf)
> > {
> > struct device *dev = >client->dev;
> > int ret;
> > @@ -231,8 +248,7 @@ static int anx7625_aux_dpcd_read(struct anx7625_data
> > *ctx,
> > addrm = (address >> 8) & 0xFF;
> > addrh = (address >> 16) & 0xFF;
> >
> > - cmd = DPCD_CMD(len, DPCD_READ);
> > - cmd = ((len - 1) << 4) | 0x09;
> > + cmd = DPCD_CMD(len, op);
> >
> > /* Set command and length */
> > ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > @@ -246,6 +262,9 @@ static int anx7625_aux_dpcd_read(struct anx7625_data
> > *ctx,
> > ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > AP_AUX_ADDR_19_16, addrh);
> >
> > + if (op == DP_AUX_NATIVE_WRITE)
> > + ret |= anx7625_reg_block_write(ctx, ctx->i2c.rx_p0_client,
> > + AP_AUX_BUFF_START, len, buf);
> > /* Enable aux access */
> > ret |= anx7625_write_or(ctx, ctx->i2c.rx_p0_client,
> > AP_AUX_CTRL_STATUS, AP_AUX_CTRL_OP_EN);
> > @@ -255,14 +274,17 @@ static int anx7625_aux_dpcd_read(struct anx7625_data
> > *ctx,
> > return -EIO;
> > }
> >
> > - usleep_range(2000, 2100);
> > -
> > ret = wait_aux_op_finish(ctx);
> > if (ret) {
> > dev_err(dev, "aux IO error: wait aux op finish.\n");
> > return ret;
> > }
> >
> > + /* Write done */
> > + if (op == DP_AUX_NATIVE_WRITE)
> > + return 0;
> > +
> > + /* Read done, read out dpcd data */
> > ret = anx7625_reg_block_read(ctx, ctx->i2c.rx_p0_client,
> > AP_AUX_BUFF_START, len, buf);
> > if (ret < 0) {
> > @@ -845,7 +867,7 @@ static int anx7625_hdcp_enable(struct anx7625_data *ctx)
> > }
> >
> > /* Read downstream capability */
> > - anx7625_aux_dpcd_read(ctx, 0x68028, 1, );
> > + anx7625_aux_dpcd_trans(ctx, DP_AUX_NATIVE_READ, 0x68028, 1, );
> > if (!(bcap & 0x01)) {
> > pr_warn("downstream not support HDCP 1.4, cap(%x).\n",
> > bcap);
> > return 0;
> > @@ -918,6 +940,7 @@ static void anx7625_dp_stop(struct anx7625_data *ctx)
> > {
> > struct device *dev = >client->dev;
> > int ret;
> > + u8 data;
> >
> > DRM_DEV_DEBUG_DRIVER(dev, "stop dp output\n");
> >
> > @@ -929,6 +952,11 @@ static void anx7625_dp_stop(struct anx7625_data *ctx)
> > ret |= anx7625_write_and(ctx, ctx->i2c.tx_p2_client, 0x08, 0x7f);
> >
> > ret |=
Re: [PATCH 2/2] Revert "fbcon: Disable accelerated scrolling"
Hi Thomas, On 1/19/22 16:37, Thomas Zimmermann wrote: > Am 19.01.22 um 16:05 schrieb Sven Schnelle: >> Daniel Vetter writes: >> >>> On Wed, Jan 19, 2022 at 3:01 PM Linus Torvalds >>> wrote: >>> Irrespective of this code being buggy or not buggy I think the bigger >>> pictures, and really the reason I want to see as much code ditched >>> from the fbdev/fbcon stack as we possible can, are very clear: >>> >>> - it's full of bugs >>> - there's no test coverage/CI to speak of >>> - it's very arcane code which is damn hard to understand and fix issues >>> within >>> - the locking is busted (largely thanks to console_lock, and the >>> effort to make that reasonable from -rt folks has been slowly creeping >>> forward for years). >>> >>> Iow this subsystem is firmly stuck in the 90s, and I think it's best >>> to just leave it there. There's also not been anyone actually capable >>> and willing to put in the work to change this (pretty much all actual >>> changes/fixes have been done by drm folks anyway, like me having a >>> small pet project to make the fbdev vs fbcon locking slightly less >>> busted). >> >> Saying it's stuck in the 90ies, and actively trying to prevent >> Helge from taking over maintainership at the same time looks odd. > > The issues are in the design itself. It's impossible to model today's > hardware and constraints with fbdev. It's impossible to change > configuration in a reliable way (i.e., what DRM calls atomic). Fbdev > mmaps plain video ram to userspace, which is one of the reasons why > DRM's fbdev support is hard to improve. That's fully understood, but I think you are mixing up things here... The fbdev userspace api is most likely not the best way forward. I'm sure that drm can and will provide better solutions for userspace. And userspace will surely pick up those new interfaces. DRM folks will drive it in the right direction, I'm sure! But in addition fbdev/fbcon is the kernel framework for nearly all existing graphic cards which are not (yet) supported by DRM. They need fbdev/fbcon to show their text console and maybe a simple X server. If you break fbdev for those cards, they are completely stuck. Hopefully those drivers will be ported to DRM, but that's currently not easily possible (or they would be so slow that they are unuseable). So, I don't think you should try to improve DRM's /dev/fb0 support further, but instead work forward for a new interface which perfectly suits DRM. That's Ok, and my goal is not to prevent that. >> I think Helge should at least get a chance to fix the issues. If the >> state is still the same in a year or so it should be discussed again. > > You cannot fix that in 10yrs. > >> >>> The other side is that being a maintainer is about collaboration, and >>> this entire fbdev maintainership takeover has been a demonstration of >>> anything but that. MAINTAINERS entry was a bit confusing since defacto >>> drm has been maintaining it for years. >> >> It was marked as 'Orphaned'. Anyone is free to send a Patch/PR to take >> over maintainership. If you have strong opinions about that code (And you >> obviously have reading your mail, set it to 'maintained' and care about >> it. Everything else is just wrong in my opinion. > > No, it's not wrong. Helge takes fbdev over the weekend, without noteworthy > experience, and ignores advice from the people that have kept it alive over > the past years. This isn't going to work in the long term. > > Best regards > Thomas > >> >> /Sven >
Re: [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions
On Fri, Dec 10, 2021 at 03:23:57PM -0800, 'Ira Weiny' wrote: > From: Ira Weiny > > This series starts by converting the last easy kmap() uses to > kmap_local_page(). > > There is one more call to kmap() wrapped in ttm_bo_kmap_ttm(). Unfortunately, > ttm_bo_kmap_ttm() is called in a number of different ways including some which > are not thread local. I have a patch to convert that call. However, it is > not > straight forward so it is not included in this series. > > The final 2 patches fix bugs found while working on the ttm_bo_kmap_ttm() > conversion. Gentile ping on this series? Will it make this merge window? Thanks, Ira > > > Ira Weiny (7): > drm/i915: Replace kmap() with kmap_local_page() > drm/amd: Replace kmap() with kmap_local_page() > drm/gma: Remove calls to kmap() > drm/radeon: Replace kmap() with kmap_local_page() > drm/msm: Alter comment to use kmap_local_page() > drm/amdgpu: Ensure kunmap is called on error > drm/radeon: Ensure kunmap is called on error > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 > drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 + > drivers/gpu/drm/gma500/gma_display.c | 6 ++ > drivers/gpu/drm/gma500/mmu.c | 8 > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++-- > drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8 > drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++-- > drivers/gpu/drm/i915/gt/shmem_utils.c | 4 ++-- > drivers/gpu/drm/i915/i915_gem.c | 8 > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++-- > drivers/gpu/drm/radeon/radeon_ttm.c | 4 ++-- > drivers/gpu/drm/radeon/radeon_uvd.c | 1 + > 13 files changed, 32 insertions(+), 32 deletions(-) > > -- > 2.31.1 >
RE: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
> > > > +static inline const char *yesno(bool v) { return v ? "yes" : "no"; }
>
> return "yes\0no" + v * 4;
>
> :-)
except '"no\0\0yes" + v * 4' works a bit better.
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
RE: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
> > > +static inline const char *yesno(bool v) { return v ? "yes" : "no"; }
return "yes\0no" + v * 4;
:-)
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions
On 1/19/22 16:50, Thomas Zimmermann wrote: [snip] IMHO the best solution is to drop IORESOURCE_BUSY from sysfb and have drivers register/release the range with _BUSY. That would signal the memory belongs to the sysfb device but is not busy unless a driver has been bound. After simplefb released the range, it should be 'non- busy' again and available for vmwgfx. Simpledrm does a hot-unplug of the sysfb device, so the memory range gets released entirely. If you want, I'll prepare some patches for this scenario. >>> >>> Attached is a patch that implements this. Doing >>> >>> cat /proc/iomem >>> ... >>> e000-efff : :00:02.0 >>> >>> e000-e07e8fff : BOOTFB >>> >>> e000-e07e8fff : simplefb >>> >>> ... >>> >>> shows the memory. 'BOOTFB' is the simple-framebuffer device and >>> 'simplefb' is the driver. Only the latter uses _BUSY. Same for >>> and the memory canbe acquired by vmwgfx. >>> >>> Zack, please test this patch. If it works, I'll send out the real >>> patchset. >> >> Hmm, the patch looks good but it doesn't work. After boot: /proc/iomem >> 5000-7fff : pcie@0x4000 >>7800-7fff : :00:0f.0 >> 7800-782f : BOOTFB >> >> and vmwgfx fails on pci_request_regions: >> >> kernel: fb0: switching to vmwgfx from simple >> kernel: Console: switching to colour dummy device 80x25 >> kernel: vmwgfx :00:0f.0: BAR 2: can't reserve [mem 0x7800- >> 0x7fff 64bit pref] >> kernel: vmwgfx: probe of :00:0f.0 failed with error -16 >> >> leaving the system without a fb driver. > > OK, I suspect that it would work if you use simpledrm instead of > simplefb. Could you try please? You'd have to build DRM and simpledrm > into the kernel binary. > Yes, I believe that should work. Zack, could you please try if just the following [0] make it works ? That is, dropping the IORESOURCE_BUSY but not doing the memory region request / release in simplefb and keeping it in the vmwgfx driver. [0]: >From da6de1430b9dc252eccf2d6fee0446d33375fa6d Mon Sep 17 00:00:00 2001 From: Javier Martinez Canillas Date: Wed, 19 Jan 2022 14:41:25 +0100 Subject: [PATCH] drivers/firmware: Don't mark as busy the simple-framebuffer IO resource The sysfb_create_simplefb() function requests a IO memory resource for the simple-framebuffer platform device, but it also marks it as busy which led to drivers requesting the same memory resource to fail. Let's drop the IORESOURCE_BUSY flag and let drivers to request it as busy instead. Signed-off-by: Javier Martinez Canillas --- drivers/firmware/sysfb_simplefb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c index 303a491e520d..76c4abc42a30 100644 --- a/drivers/firmware/sysfb_simplefb.c +++ b/drivers/firmware/sysfb_simplefb.c @@ -99,7 +99,7 @@ __init int sysfb_create_simplefb(const struct screen_info *si, /* setup IORESOURCE_MEM as framebuffer memory */ memset(, 0, sizeof(res)); - res.flags = IORESOURCE_MEM | IORESOURCE_BUSY; + res.flags = IORESOURCE_MEM; res.name = simplefb_resname; res.start = base; res.end = res.start + length - 1; -- 2.33.1
Re: fbdev: Garbage collect fbdev scrolling acceleration
Hi Daniel, Daniel Vetter writes: > On Wed, Jan 19, 2022 at 05:15:44PM +0100, Sven Schnelle wrote: >> Hi Daniel, >> >> Daniel Vetter writes: >> >> > On Thu, Jan 13, 2022 at 10:46:03PM +0100, Sven Schnelle wrote: >> >> Helge Deller writes: >> >> > Maybe on fast new x86 boxes the performance difference isn't huge, >> >> > but for all old systems, or when emulated in qemu, this makes >> >> > a big difference. >> >> > >> >> > Helge >> >> >> >> I second that. For most people, the framebuffer isn't important as >> >> they're mostly interested in getting to X11/wayland as fast as possible. >> >> But for systems like servers without X11 it's nice to have a fast >> >> console. >> > >> > Fast console howto: >> > - shadow buffer in cached memory >> > - timer based upload of changed areas to the real framebuffer >> > >> > This one is actually fast, instead of trying to use hw bltcopy and having >> > the most terrible fallback path if that's gone. Yes drm fbdev helpers has >> > this (but not enabled on most drivers because very, very few people care). >> >> Hmm Take my Laptop with a 4k (3180x2160) screen as an example: >> >> Lets say on average the half of every line is filled with text. >> >> So 3840/2*2160 pixels that change = 4147200 pixels. Every pixel takes 4 >> bytes = 16,588800 bytes per timer interrupt. In another Mail updating on >> vsync was mentioned, so multiply that by 60 and get ~927MB. And even if >> you only update the screen ony 4 times per second, that would be ~64MB >> of data. I'm likely missing something here. > > Since you say 4k it's a modern box, so you have on the order of 10GB/s of > write bandwidth. > > And around 100MB/s of read bandwidth. Both from the cpu. It all adds up. > It's that uncached read which kills you and means dmesg takes seconds to > display. > > Also since this is 4k looking at sales volume we're talking integrated, so > whether it's the gpu or the cpu that's doing the memcpy, it's the same > memory bw budget you're burning down. That might be true for integrated graphics, as said, i don't know the architecture. But saying it's good just because it's good on one architecture doesn't mean it's good for everyone. If you have an external GPU, than the memory/system bus BW would be different whether it's memcpy or the GPU doing the scrolling. And whether internal or external graphics - the CPU could do other stuff while the GPU scrolls stuff. Quite a lot of discussion for a revert of a patch that was already in the kernel for more than 20(?) years. /Sven
Re: [PATCH 2/2] Revert "fbcon: Disable accelerated scrolling"
On Wed, Jan 19, 2022 at 06:13:39PM +0200, Jani Nikula wrote: > On Wed, 19 Jan 2022, Helge Deller wrote: > > On 1/19/22 16:42, Daniel Vetter wrote: > >> If otoh you get the maintainership first to be able to cram in reverts > >> without discussions, then it's very backwards. > > I'm working on the Linux kernel since at least 23 years and am a maintainer > > of parts of it. > > I know that and would never push something which is controversal without > > discussions. > > I think the entire MAINTAINERS change was controversial and rushed to > Linus without discussion over a weekend. Yeah just looking at the size of the thread is pretty clear indiciation that this went wrong real good. And I said that you need to clean this up in my very first reply. None of this should have been a surprise, but somehow it all happened. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] dt-bindings: Drop unnecessary pinctrl properties
On Tue, Jan 18, 2022 at 07:53:25PM -0600, Rob Herring wrote: > For a single pinctrl mode, it is not necessary to define pinctrl > properties as the tools always allow pinctrl properties. Acked-by: Mark Brown signature.asc Description: PGP signature
Re: [RFC] How to add hardware rotation, scaling etc to a DRM/KMS driver
On Wed, Jan 19, 2022 at 05:55:22PM +0900, Daniel Palmer wrote: > Hi all, > > I've copied and pasted my way to mostly working DRM/KMS driver for a > low cost ARM SoC (Sigmastar SSD202D). The hardware is 2D only. > > One of the devices that uses this SoC has the screen upside down so it > needs the screen rotated. > The hardware doesn't have bits that change the scan out direction from > what I can tell (so it can't mirror/flip while feeding it to the > screen) but it does have a 2D blitter style block that can take a > framebuffer and flip/mirror/scale/convert the colour space into > another buffer. > > My idea was to create a buffer for the rotated image when allocating > the framebuffer and trigger the hardware to do the conversion each > vblank or something. > > While reading the discussion about maintaining fbdev I realised maybe > I should ask instead of wasting too much time on something that's > wrong. > > I got the feeling that maybe I should just provide an interface to the > blitter from userspace and userspace should be doing the rotation. I'd > like to do it in the kernel so stuff like SDL1 apps just work but > maybe that isn't possible? panel orientation property is for that stuff: https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties You need to scroll down to the "panel orientation:" property. And here's how to quirk this for at least some cases (for dt panels it should be in the dt): https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html?highlight=drm_get_panel_orientation_quirk#c.drm_get_panel_orientation_quirk; fbcon will head this and rotate in sw, as should any competent compositor in userspace (but some might not, it depends). Also ping Hans de Geode if you have any questions, he's done this. -Daniel > > Cheers, > > Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: fbdev: Garbage collect fbdev scrolling acceleration
On Wed, Jan 19, 2022 at 05:15:44PM +0100, Sven Schnelle wrote: > Hi Daniel, > > Daniel Vetter writes: > > > On Thu, Jan 13, 2022 at 10:46:03PM +0100, Sven Schnelle wrote: > >> Helge Deller writes: > >> > Maybe on fast new x86 boxes the performance difference isn't huge, > >> > but for all old systems, or when emulated in qemu, this makes > >> > a big difference. > >> > > >> > Helge > >> > >> I second that. For most people, the framebuffer isn't important as > >> they're mostly interested in getting to X11/wayland as fast as possible. > >> But for systems like servers without X11 it's nice to have a fast > >> console. > > > > Fast console howto: > > - shadow buffer in cached memory > > - timer based upload of changed areas to the real framebuffer > > > > This one is actually fast, instead of trying to use hw bltcopy and having > > the most terrible fallback path if that's gone. Yes drm fbdev helpers has > > this (but not enabled on most drivers because very, very few people care). > > Hmm Take my Laptop with a 4k (3180x2160) screen as an example: > > Lets say on average the half of every line is filled with text. > > So 3840/2*2160 pixels that change = 4147200 pixels. Every pixel takes 4 > bytes = 16,588800 bytes per timer interrupt. In another Mail updating on > vsync was mentioned, so multiply that by 60 and get ~927MB. And even if > you only update the screen ony 4 times per second, that would be ~64MB > of data. I'm likely missing something here. Since you say 4k it's a modern box, so you have on the order of 10GB/s of write bandwidth. And around 100MB/s of read bandwidth. Both from the cpu. It all adds up. It's that uncached read which kills you and means dmesg takes seconds to display. Also since this is 4k looking at sales volume we're talking integrated, so whether it's the gpu or the cpu that's doing the memcpy, it's the same memory bw budget you're burning down. And at that point doing less copying (which the shadow buffer thing will do compared to fbcon accelerated scrolling for every line) is the win. And since max resolutions pretty much scales down with pcie or memory bandwidth for roughly the last 2-3 decades, this all works as well on old stuff. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: fbdev: Garbage collect fbdev scrolling acceleration
Hi Daniel, Daniel Vetter writes: > On Thu, Jan 13, 2022 at 10:46:03PM +0100, Sven Schnelle wrote: >> Helge Deller writes: >> > Maybe on fast new x86 boxes the performance difference isn't huge, >> > but for all old systems, or when emulated in qemu, this makes >> > a big difference. >> > >> > Helge >> >> I second that. For most people, the framebuffer isn't important as >> they're mostly interested in getting to X11/wayland as fast as possible. >> But for systems like servers without X11 it's nice to have a fast >> console. > > Fast console howto: > - shadow buffer in cached memory > - timer based upload of changed areas to the real framebuffer > > This one is actually fast, instead of trying to use hw bltcopy and having > the most terrible fallback path if that's gone. Yes drm fbdev helpers has > this (but not enabled on most drivers because very, very few people care). Hmm Take my Laptop with a 4k (3180x2160) screen as an example: Lets say on average the half of every line is filled with text. So 3840/2*2160 pixels that change = 4147200 pixels. Every pixel takes 4 bytes = 16,588800 bytes per timer interrupt. In another Mail updating on vsync was mentioned, so multiply that by 60 and get ~927MB. And even if you only update the screen ony 4 times per second, that would be ~64MB of data. I'm likely missing something here.
Re: [PATCH 0/3] lib/string_helpers: Add a few string helpers
On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote: > On Wed, 19 Jan 2022, Petr Mladek wrote: > > On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote: > >> Add some helpers under lib/string_helpers.h so they can be used > >> throughout the kernel. When I started doing this there were 2 other > >> previous attempts I know of, not counting the iterations each of them > >> had: > >> > >> 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nik...@intel.com/ > >> 2) > >> https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevche...@linux.intel.com/#t > >> > >> Going through the comments I tried to find some common ground and > >> justification for what is in here, addressing some of the concerns > >> raised. > >> > >> d. This doesn't bring onoff() helper as there are some places in the > >>kernel with onoff as variable - another name is probably needed for > >>this function in order not to shadow the variable, or those variables > >>could be renamed. Or if people wanting > >>try to find a short one > > > > I would call it str_on_off(). > > > > And I would actually suggest to use the same style also for > > the other helpers. > > > > The "str_" prefix would make it clear that it is something with > > string. There are other _on_off() that affect some > > functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off(). > > > > The dash '_' would significantly help to parse the name. yesno() and > > onoff() are nicely short and kind of acceptable. But "enabledisable()" > > is a puzzle. > > > > IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good > > compromise. > > > > The main motivation should be code readability. You write the > > code once. But many people will read it many times. Open coding > > is sometimes better than misleading macro names. > > > > That said, I do not want to block this patchset. If others like > > it... ;-) > > I don't mind the names either way. Adding the prefix and dashes is > helpful in that it's possible to add the functions first and convert > users at leisure, though with a bunch of churn, while using names that > collide with existing ones requires the changes to happen in one go. > > What I do mind is grinding this series to a halt once again. I sent a > handful of versions of this three years ago, with inconclusive > bikeshedding back and forth, eventually threw my hands up in disgust, > and walked away. Yeah we can sed this anytime later we want to, but we need to get the foot in the door. There's also a pile more of these all over. Acked-by: Daniel Vetter on the series, maybe it helps? And yes let's merge this through drm-misc. -Daniel > > > > > > >> e. One alternative to all of this suggested by Christian König > >>(43456ba7-c372-84cc-4949-dcb817188...@amd.com) would be to add a > >>printk format. But besides the comment, he also seemed to like > >>the common function. This brought the argument from others that the > >>simple yesno()/enabledisable() already used in the code is easier to > >>remember and use than e.g. %py[DOY] > > > > Thanks for not going this way :-) > > > >> Last patch also has some additional conversion of open coded cases. I > >> preferred starting with drm/ since this is "closer to home". > >> > >> I hope this is a good summary of the previous attempts and a way we can > >> move forward. > >> > >> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my > >> proposal is to take first 2 patches either through mm tree or maybe > >> vsprintf. Last patch can be taken later through drm. > > > > I agree with Andy that it should go via drm tree. It would make it > > easier to handle potential conflicts. > > > > Just in case, you decide to go with str_yes_no() or something similar. > > Mass changes are typically done at the end on the merge window. > > The best solution is when it can be done by a script. > > > > Best Regards, > > Petr > > -- > Jani Nikula, Intel Open Source Graphics Center -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 2/2] Revert "fbcon: Disable accelerated scrolling"
On Wed, 19 Jan 2022, Helge Deller wrote: > On 1/19/22 16:42, Daniel Vetter wrote: >> If otoh you get the maintainership first to be able to cram in reverts >> without discussions, then it's very backwards. > I'm working on the Linux kernel since at least 23 years and am a maintainer > of parts of it. > I know that and would never push something which is controversal without > discussions. I think the entire MAINTAINERS change was controversial and rushed to Linus without discussion over a weekend. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [RFC 00/10] drm: selftests: Convert to KUnit
On Tue, Jan 18, 2022 at 12:22:49AM +0100, Michał Winiarski wrote:
> KUnit unifies the test structure and provides helper tools that simplify
> the development. Basic use case allows running tests as regular processes,
> leveraging User Mode Linux.
> For example, to execute all DRM unit tests:
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm
> (the tool also allows using QEMU instead of UML by adding e.g. --arch=x86_64)
>
> For developers - it means that it's easier to run unit tests on the
> development
> machine, tightening the feedback loop. When using UML, it also simplifies
> using
> gdb for debug (since the kernel is just a regular process).
> For CI systems - DRM tests can be moved from being executed on device under
> test (that's also running IGTs and so on) to being executed on buildsystem
> during build (just like checkpatch.pl).
>
> All tests were renamed - IGT prefix is no longer used.
>
> Compared to selftests executed by CI using IGT, there's one functional
> regression - KUnit test runner is not catching WARNs.
> To solve this, we could either go in the similar direction that UBSAN went in:
> 1195505 ("kunit: ubsan integration")
> Or we could expand the test runner to catch WARN signature in dmesg.
I think catching WARN signature in dmesg makes sense, assuming Kunit has a
standard WARN format. It would be nice to be consistent here.
I haven't looked at any details, but yes so much I want to see this done.
I'll cc a bunch of people I've talked with about this conversion,
hopefully I got them all. Maybe they can help with review and getting this
all landed!
Cheers, Daniel
> Pastebin to preview the output and execution times:
> https://gitlab.freedesktop.org/-/snippets/4139
>
> -Michał
>
> Michał Winiarski (10):
> drm: test-drm_cmdline_parser: Convert to KUnit
> drm: test-drm_plane_helper: Convert to KUnit
> drm: test-drm_format: Convert to KUnit
> drm: test-drm_framebuffer: Convert to KUnit
> drm: test-drm_damage_helper: Convert to KUnit
> drm: test-drm_dp_mst_helper: Convert to KUnit
> drm: test-drm_rect: Convert to KUnit
> drm: test-drm_mm: Convert to KUnit
> drm: selftests: Convert to KUnit
> drm: test: Simplify testing on UML with kunit.py
>
> drivers/gpu/drm/.kunitconfig |3 +
> drivers/gpu/drm/Kconfig | 22 +-
> drivers/gpu/drm/Makefile |2 +-
> drivers/gpu/drm/i915/Kconfig.debug|1 -
> drivers/gpu/drm/selftests/Makefile|7 -
> .../gpu/drm/selftests/drm_cmdline_selftests.h | 68 -
> drivers/gpu/drm/selftests/drm_mm_selftests.h | 28 -
> .../gpu/drm/selftests/drm_modeset_selftests.h | 40 -
> drivers/gpu/drm/selftests/drm_selftest.c | 109 -
> drivers/gpu/drm/selftests/drm_selftest.h | 41 -
> .../drm/selftests/test-drm_cmdline_parser.c | 1141
> .../drm/selftests/test-drm_damage_helper.c| 667 -
> .../drm/selftests/test-drm_dp_mst_helper.c| 273 --
> drivers/gpu/drm/selftests/test-drm_format.c | 280 --
> drivers/gpu/drm/selftests/test-drm_mm.c | 2487 -
> .../drm/selftests/test-drm_modeset_common.c | 32 -
> .../drm/selftests/test-drm_modeset_common.h | 52 -
> .../gpu/drm/selftests/test-drm_plane_helper.c | 223 --
> drivers/gpu/drm/selftests/test-drm_rect.c | 223 --
> drivers/gpu/drm/test/Makefile |7 +
> .../gpu/drm/test/test-drm_cmdline_parser.c| 1027 +++
> drivers/gpu/drm/test/test-drm_damage_helper.c | 667 +
> drivers/gpu/drm/test/test-drm_dp_mst_helper.c | 429 +++
> drivers/gpu/drm/test/test-drm_format.c| 356 +++
> .../test-drm_framebuffer.c| 109 +-
> drivers/gpu/drm/test/test-drm_mm.c| 2426
> drivers/gpu/drm/test/test-drm_plane_helper.c | 312 +++
> drivers/gpu/drm/test/test-drm_rect.c | 249 ++
> drivers/video/Kconfig |4 +
> 29 files changed, 5558 insertions(+), 5727 deletions(-)
> create mode 100644 drivers/gpu/drm/.kunitconfig
> delete mode 100644 drivers/gpu/drm/selftests/Makefile
> delete mode 100644 drivers/gpu/drm/selftests/drm_cmdline_selftests.h
> delete mode 100644 drivers/gpu/drm/selftests/drm_mm_selftests.h
> delete mode 100644 drivers/gpu/drm/selftests/drm_modeset_selftests.h
> delete mode 100644 drivers/gpu/drm/selftests/drm_selftest.c
> delete mode 100644 drivers/gpu/drm/selftests/drm_selftest.h
> delete mode 100644 drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
> delete mode 100644 drivers/gpu/drm/selftests/test-drm_damage_helper.c
> delete mode 100644 drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> delete mode 100644 drivers/gpu/drm/selftests/test-drm_format.c
> delete mode 100644 drivers/gpu/drm/selftests/test-drm_mm.c
> delete mode 100644 drivers/gpu/drm/selftests/test-drm_modeset_common.c
> delete mode 100644 drivers/gpu/drm/selftests/test-drm_modeset_common.h
>
Re: [PATCH] dt-bindings: display: Add SPI peripheral schema to SPI based displays
On Tue, 21 Dec 2021 08:52:09 -0400, Rob Herring wrote:
> With 'unevaluatedProperties' support enabled, several SPI based display
> binding examples have warnings:
>
> Documentation/devicetree/bindings/display/panel/samsung,ld9040.example.dt.yaml:
> lcd@0: Unevaluated properties are not allowed ('#address-cells',
> '#size-cells', 'spi-max-frequency', 'spi-cpol', 'spi-cpha' were unexpected)
> Documentation/devicetree/bindings/display/panel/kingdisplay,kd035g6-54nt.example.dt.yaml:
> panel@0: Unevaluated properties are not allowed ('spi-max-frequency',
> 'spi-3wire' were unexpected)
> Documentation/devicetree/bindings/display/panel/ilitek,ili9322.example.dt.yaml:
> display@0: Unevaluated properties are not allowed ('reg' was unexpected)
> Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.example.dt.yaml:
> display@0: Unevaluated properties are not allowed ('spi-max-frequency' was
> unexpected)
> Documentation/devicetree/bindings/display/panel/abt,y030xx067a.example.dt.yaml:
> panel@0: Unevaluated properties are not allowed ('spi-max-frequency' was
> unexpected)
> Documentation/devicetree/bindings/display/panel/sony,acx565akm.example.dt.yaml:
> panel@2: Unevaluated properties are not allowed ('spi-max-frequency', 'reg'
> were unexpected)
> Documentation/devicetree/bindings/display/panel/tpo,td.example.dt.yaml:
> panel@0: Unevaluated properties are not allowed ('spi-max-frequency',
> 'spi-cpol', 'spi-cpha' were unexpected)
> Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.example.dt.yaml:
> panel@0: Unevaluated properties are not allowed ('reg', 'spi-max-frequency',
> 'spi-cpol', 'spi-cpha' were unexpected)
> Documentation/devicetree/bindings/display/panel/innolux,ej030na.example.dt.yaml:
> panel@0: Unevaluated properties are not allowed ('spi-max-frequency' was
> unexpected)
> Documentation/devicetree/bindings/display/panel/sitronix,st7789v.example.dt.yaml:
> panel@0: Unevaluated properties are not allowed ('spi-max-frequency',
> 'spi-cpol', 'spi-cpha' were unexpected)
>
> Fix all of these by adding a reference to spi-peripheral-props.yaml.
> With this, the description that the binding must follow
> spi-controller.yaml is both a bit out of date and redundant, so remove
> it.
>
> Signed-off-by: Rob Herring
> ---
> This is dependent on spi-peripheral-props.yaml landing in 5.17-rc1.
> ---
> .../devicetree/bindings/display/panel/abt,y030xx067a.yaml | 5 +
> .../devicetree/bindings/display/panel/ilitek,ili9322.yaml | 4 +---
> .../devicetree/bindings/display/panel/innolux,ej030na.yaml | 5 +
> .../bindings/display/panel/kingdisplay,kd035g6-54nt.yaml | 5 +
> .../bindings/display/panel/lgphilips,lb035q02.yaml | 5 +
> .../devicetree/bindings/display/panel/samsung,ld9040.yaml | 7 +--
> .../devicetree/bindings/display/panel/samsung,s6e63m0.yaml | 1 +
> .../bindings/display/panel/sitronix,st7789v.yaml | 5 +
> .../devicetree/bindings/display/panel/sony,acx565akm.yaml | 5 +
> .../devicetree/bindings/display/panel/tpo,td.yaml | 5 +
> 10 files changed, 10 insertions(+), 37 deletions(-)
>
Applied, thanks!
Re: [RFC 2/6] cgroup: gpu: Add a cgroup controller for allocator attribution of GPU memory
Hi-- On 1/14/22 17:06, Hridya Valsaraju wrote: > diff --git a/init/Kconfig b/init/Kconfig > index cd23faa163d1..408910b21387 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -990,6 +990,13 @@ config BLK_CGROUP > > See Documentation/admin-guide/cgroup-v1/blkio-controller.rst for more > information. > > +config CGROUP_GPU > + bool "gpu cgroup controller (EXPERIMENTAL)" > + select PAGE_COUNTER > + help > + Provides accounting and limit setting for memory allocations by the GPU > + and GPU-related subsystems. Please follow coding-style for Kconfig files: (from Documentation/process/coding-style.rst, section 10): For all of the Kconfig* configuration files throughout the source tree, the indentation is somewhat different. Lines under a ``config`` definition are indented with one tab, while help text is indented an additional two spaces. thanks. -- ~Randy
