This is v2 for a previous patch series being discussed at https://lore.kernel.org/dri-devel/20220517174216.381287-1-adrian.larumbe@col....
Mesa MR under review can be found at: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14034
Changes with respect to v1 of the same patch:
- Change the list of dumped registers to those of greatest debugging value. - Fix bug where an invalid bomap header wouldn't be dumped - Changed the way that a decision is made whether to dump a BOMAP header so that no compiler warning supression has to be done. - Changed magic number for header fields to something more representative. - Changed the dump header format to a union whose members are the different header types.
Adrián Larumbe (1): drm/panfrost: Add support for devcoredump
drivers/gpu/drm/panfrost/Kconfig | 1 + drivers/gpu/drm/panfrost/Makefile | 3 +- drivers/gpu/drm/panfrost/panfrost_dump.c | 233 +++++++++++++++++++++++ drivers/gpu/drm/panfrost/panfrost_dump.h | 12 ++ drivers/gpu/drm/panfrost/panfrost_job.c | 3 + include/uapi/drm/panfrost_drm.h | 44 +++++ 6 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/panfrost/panfrost_dump.c create mode 100644 drivers/gpu/drm/panfrost/panfrost_dump.h
In the event of a job timeout, debug dump information will be written into /sys/class/devcoredump.
Inspired by etnaviv's similar feature.
Signed-off-by: Adrián Larumbe adrian.larumbe@collabora.com --- drivers/gpu/drm/panfrost/Kconfig | 1 + drivers/gpu/drm/panfrost/Makefile | 3 +- drivers/gpu/drm/panfrost/panfrost_dump.c | 233 +++++++++++++++++++++++ drivers/gpu/drm/panfrost/panfrost_dump.h | 12 ++ drivers/gpu/drm/panfrost/panfrost_job.c | 3 + include/uapi/drm/panfrost_drm.h | 44 +++++ 6 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/panfrost/panfrost_dump.c create mode 100644 drivers/gpu/drm/panfrost/panfrost_dump.h
diff --git a/drivers/gpu/drm/panfrost/Kconfig b/drivers/gpu/drm/panfrost/Kconfig index 86cdc0ce79e6..079600328be1 100644 --- a/drivers/gpu/drm/panfrost/Kconfig +++ b/drivers/gpu/drm/panfrost/Kconfig @@ -11,6 +11,7 @@ config DRM_PANFROST select DRM_GEM_SHMEM_HELPER select PM_DEVFREQ select DEVFREQ_GOV_SIMPLE_ONDEMAND + select WANT_DEV_COREDUMP help DRM driver for ARM Mali Midgard (T6xx, T7xx, T8xx) and Bifrost (G3x, G5x, G7x) GPUs. diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile index b71935862417..7da2b3f02ed9 100644 --- a/drivers/gpu/drm/panfrost/Makefile +++ b/drivers/gpu/drm/panfrost/Makefile @@ -9,6 +9,7 @@ panfrost-y := \ panfrost_gpu.o \ panfrost_job.o \ panfrost_mmu.o \ - panfrost_perfcnt.o + panfrost_perfcnt.o \ + panfrost_dump.o
obj-$(CONFIG_DRM_PANFROST) += panfrost.o diff --git a/drivers/gpu/drm/panfrost/panfrost_dump.c b/drivers/gpu/drm/panfrost/panfrost_dump.c new file mode 100644 index 000000000000..53ecc9b43666 --- /dev/null +++ b/drivers/gpu/drm/panfrost/panfrost_dump.c @@ -0,0 +1,233 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright 2021 Collabora ltd. */ + +#include <linux/err.h> +#include <linux/device.h> +#include <linux/devcoredump.h> +#include <linux/moduleparam.h> +#include <linux/iosys-map.h> +#include <drm/panfrost_drm.h> +#include <drm/drm_device.h> + +#include "panfrost_job.h" +#include "panfrost_gem.h" +#include "panfrost_regs.h" +#include "panfrost_dump.h" +#include "panfrost_device.h" + +static bool panfrost_dump_core = true; +module_param_named(dump_core, panfrost_dump_core, bool, 0600); + +struct panfrost_dump_iterator { + void *start; + struct panfrost_dump_object_header *hdr; + void *data; +}; + +static const unsigned short panfrost_dump_registers[] = { + SHADER_READY_LO, + SHADER_READY_HI, + TILER_READY_LO, + TILER_READY_HI, + L2_READY_LO, + L2_READY_HI, + JOB_INT_MASK, + JOB_INT_STAT, + JS_HEAD_LO(0), + JS_HEAD_HI(0), + JS_TAIL_LO(0), + JS_TAIL_HI(0), + JS_AFFINITY_LO(0), + JS_AFFINITY_HI(0), + JS_CONFIG(0), + JS_STATUS(0), + JS_HEAD_NEXT_LO(0), + JS_HEAD_NEXT_HI(0), + JS_AFFINITY_NEXT_LO(0), + JS_AFFINITY_NEXT_HI(0), + JS_CONFIG_NEXT(0), + MMU_INT_MASK, + MMU_INT_STAT, + AS_TRANSTAB_LO(0), + AS_TRANSTAB_HI(0), + AS_MEMATTR_LO(0), + AS_MEMATTR_HI(0), + AS_FAULTSTATUS(0), + AS_FAULTADDRESS_LO(0), + AS_FAULTADDRESS_HI(0), + AS_STATUS(0), +}; + +static void panfrost_core_dump_header(struct panfrost_dump_iterator *iter, + u32 type, void *data_end) +{ + struct panfrost_dump_object_header *hdr = iter->hdr; + + hdr->magic = cpu_to_le32(PANFROSTDUMP_MAGIC); + hdr->type = cpu_to_le32(type); + hdr->file_offset = cpu_to_le32(iter->data - iter->start); + hdr->file_size = cpu_to_le32(data_end - iter->data); + + iter->hdr++; + iter->data += le32_to_cpu(hdr->file_size); +} + +static void +panfrost_core_dump_registers(struct panfrost_dump_iterator *iter, + struct panfrost_device *pfdev, + u32 as_nr, int slot) +{ + struct panfrost_dump_registers *dumpreg = iter->data; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(panfrost_dump_registers); i++, dumpreg++) { + unsigned int js_as_offset = 0; + unsigned int reg; + + if (panfrost_dump_registers[i] >= JS_HEAD_LO(0) && + panfrost_dump_registers[i] <= JS_CONFIG_NEXT(0)) + js_as_offset = slot * 0x80; + else if (panfrost_dump_registers[i] >= AS_TRANSTAB_LO(0) && + panfrost_dump_registers[i] <= AS_STATUS(0)) + js_as_offset = ((as_nr) << 6); + + reg = panfrost_dump_registers[i] + js_as_offset; + + dumpreg->reg = cpu_to_le32(reg); + dumpreg->value = cpu_to_le32(gpu_read(pfdev, reg)); + } + + panfrost_core_dump_header(iter, PANFROSTDUMP_BUF_REG, dumpreg); +} + +void panfrost_core_dump(struct panfrost_job *job) +{ + struct panfrost_device *pfdev = job->pfdev; + struct panfrost_dump_iterator iter; + struct drm_gem_object *dbo; + unsigned int n_obj, n_bomap_pages; + __le64 *bomap, *bomap_start; + size_t file_size; + u32 as_nr; + int slot; + int ret, i; + + as_nr = job->file_priv->mmu->as; + slot = panfrost_job_get_slot(job); + slot = slot ? slot : 0; + + /* Only catch the first event, or when manually re-armed */ + if (!panfrost_dump_core) + return; + panfrost_dump_core = false; + + /* At least, we dump registers and end marker */ + n_obj = 2; + n_bomap_pages = 0; + file_size = ARRAY_SIZE(panfrost_dump_registers) * + sizeof(struct panfrost_dump_registers); + + /* Add in the active buffer objects */ + for (i = 0; i < job->bo_count; i++) { + dbo = job->bos[i]; + file_size += dbo->size; + n_bomap_pages += dbo->size >> PAGE_SHIFT; + n_obj++; + } + + /* If we have any buffer objects, add a bomap object */ + if (n_bomap_pages) { + file_size += n_bomap_pages * sizeof(*bomap); + n_obj++; + } + + /* Add the size of the headers */ + file_size += sizeof(*iter.hdr) * n_obj; + + /* Allocate the file in vmalloc memory, it's likely to be big */ + iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN | + __GFP_NORETRY); + if (!iter.start) { + dev_warn(pfdev->dev, "failed to allocate devcoredump file\n"); + return; + } + + /* Point the data member after the headers */ + iter.hdr = iter.start; + iter.data = &iter.hdr[n_obj]; + + memset(iter.hdr, 0, iter.data - iter.start); + + /* + * For now, we write the job identifier in the register dump header, + * so that we can decode the entire dump later with pandecode + */ + iter.hdr->reghdr.jc = cpu_to_le64(job->jc); + iter.hdr->reghdr.version = cpu_to_le32(PANFROSTDUMP_VERSION_1); + iter.hdr->reghdr.gpu_id = cpu_to_le32(pfdev->features.id); + iter.hdr->reghdr.nbos = cpu_to_le64(job->bo_count); + + panfrost_core_dump_registers(&iter, pfdev, as_nr, slot); + + /* Reserve space for the bomap */ + if (job->bo_count) { + bomap_start = bomap = iter.data; + memset(bomap, 0, sizeof(*bomap) * n_bomap_pages); + panfrost_core_dump_header(&iter, PANFROSTDUMP_BUF_BOMAP, + bomap + n_bomap_pages); + } + + for (i = 0; i < job->bo_count; i++) { + struct iosys_map map; + struct panfrost_gem_mapping *mapping; + struct panfrost_gem_object *bo; + struct sg_page_iter page_iter; + void *vaddr; + + bo = to_panfrost_bo(job->bos[i]); + mapping = job->mappings[i]; + + if (!bo->base.sgt) { + dev_err(pfdev->dev, "Panfrost Dump: BO has no sgt, cannot dump\n"); + iter.hdr->bomap.valid = 0; + goto dump_header; + } + + ret = drm_gem_shmem_vmap(&bo->base, &map); + if (ret) { + dev_err(pfdev->dev, "Panfrost Dump: couldn't map Buffer Object\n"); + iter.hdr->bomap.valid = 0; + goto dump_header; + } + + WARN_ON(!mapping->active); + + iter.hdr->bomap.data[0] = cpu_to_le32((bomap - bomap_start)); + + for_each_sgtable_page(bo->base.sgt, &page_iter, 0) { + struct page *page = sg_page_iter_page(&page_iter); + + if (!IS_ERR(page)) + *bomap++ = cpu_to_le64(page_to_phys(page)); + else { + dev_err(pfdev->dev, "Panfrost Dump: wrong page\n"); + *bomap++ = ~cpu_to_le64(0); + } + } + + iter.hdr->bomap.iova = cpu_to_le64(mapping->mmnode.start << PAGE_SHIFT); + + vaddr = map.vaddr; + memcpy(iter.data, vaddr, bo->base.base.size); + + drm_gem_shmem_vunmap(&bo->base, &map); + + iter.hdr->bomap.valid = cpu_to_le64(1); + +dump_header: panfrost_core_dump_header(&iter, PANFROSTDUMP_BUF_BO, iter.data + + bo->base.base.size); + } + panfrost_core_dump_header(&iter, PANFROSTDUMP_BUF_TRAILER, iter.data); + + dev_coredumpv(pfdev->dev, iter.start, iter.data - iter.start, GFP_KERNEL); +} diff --git a/drivers/gpu/drm/panfrost/panfrost_dump.h b/drivers/gpu/drm/panfrost/panfrost_dump.h new file mode 100644 index 000000000000..7d9bcefa5346 --- /dev/null +++ b/drivers/gpu/drm/panfrost/panfrost_dump.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright 2021 Collabora ltd. + */ + +#ifndef PANFROST_DUMP_H +#define PANFROST_DUMP_H + +struct panfrost_job; +void panfrost_core_dump(struct panfrost_job *job); + +#endif diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index fda5871aebe3..f506d0ea067c 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -20,6 +20,7 @@ #include "panfrost_regs.h" #include "panfrost_gpu.h" #include "panfrost_mmu.h" +#include "panfrost_dump.h"
#define JOB_TIMEOUT_MS 500
@@ -727,6 +728,8 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job job_read(pfdev, JS_TAIL_LO(js)), sched_job);
+ panfrost_core_dump(job); + atomic_set(&pfdev->reset.pending, 1); panfrost_reset(pfdev, sched_job);
diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h index 9e40277d8185..9a1d3a686aeb 100644 --- a/include/uapi/drm/panfrost_drm.h +++ b/include/uapi/drm/panfrost_drm.h @@ -224,6 +224,50 @@ struct drm_panfrost_madvise { __u32 retained; /* out, whether backing store still exists */ };
+/* Definitions for coredump decoding in user space */ +#define PANFROSTDUMP_VERSION_1 1 +#define PANFROSTDUMP_MAGIC 0x464E4150 /* PANF */ + +#define PANFROSTDUMP_BUF_REG 0 +#define PANFROSTDUMP_BUF_BOMAP (PANFROSTDUMP_BUF_REG + 1) +#define PANFROSTDUMP_BUF_BO (PANFROSTDUMP_BUF_BOMAP + 1) +#define PANFROSTDUMP_BUF_TRAILER (PANFROSTDUMP_BUF_BO + 1) + +struct panfrost_dump_object_header { + __le32 magic; + __le32 type; + __le32 file_size; + __le32 file_offset; + + union { + struct pan_reg_hdr { + __le64 jc; + __le32 gpu_id; + __le64 version; + __le64 nbos; + } reghdr; + + struct pan_bomap_hdr { + __le32 valid; + __le64 iova; + __le32 data[2]; + } bomap; + + /* + * Force same size in case we want to expand the header + * with new fields and also keep it 512-byte aligned + */ + + __le32 sizer[496]; + }; +}; + +/* Registers object, an array of these */ +struct panfrost_dump_registers { + __le32 reg; + __le32 value; +}; + #if defined(__cplusplus) } #endif
Hi "Adrián,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm/drm-next] [also build test ERROR on linus/master v5.19-rc2 next-20220617] [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/intel-lab-lkp/linux/commits/Adri-n-Larumbe/devcoredump-su... base: git://anongit.freedesktop.org/drm/drm drm-next config: alpha-buildonly-randconfig-r003-20220619 (https://download.01.org/0day-ci/archive/20220621/202206211114.PJcD2pJh-lkp@i...) compiler: alpha-linux-gcc (GCC) 11.3.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/intel-lab-lkp/linux/commit/297bd4948ab1f4eeb78389d57adc1e... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Adri-n-Larumbe/devcoredump-support-for-Panfrost-GPU-driver/20220621-103431 git checkout 297bd4948ab1f4eeb78389d57adc1edc819cb6f2 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/gpu/drm/panfrost/
If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/gpu/drm/panfrost/panfrost_dump.c: In function 'panfrost_core_dump':
drivers/gpu/drm/panfrost/panfrost_dump.c:115:20: error: 'struct panfrost_job' has no member named 'file_priv'
115 | as_nr = job->file_priv->mmu->as; | ^~
vim +115 drivers/gpu/drm/panfrost/panfrost_dump.c
102 103 void panfrost_core_dump(struct panfrost_job *job) 104 { 105 struct panfrost_device *pfdev = job->pfdev; 106 struct panfrost_dump_iterator iter; 107 struct drm_gem_object *dbo; 108 unsigned int n_obj, n_bomap_pages; 109 __le64 *bomap, *bomap_start; 110 size_t file_size; 111 u32 as_nr; 112 int slot; 113 int ret, i; 114
115 as_nr = job->file_priv->mmu->as;
116 slot = panfrost_job_get_slot(job); 117 slot = slot ? slot : 0; 118 119 /* Only catch the first event, or when manually re-armed */ 120 if (!panfrost_dump_core) 121 return; 122 panfrost_dump_core = false; 123 124 /* At least, we dump registers and end marker */ 125 n_obj = 2; 126 n_bomap_pages = 0; 127 file_size = ARRAY_SIZE(panfrost_dump_registers) * 128 sizeof(struct panfrost_dump_registers); 129 130 /* Add in the active buffer objects */ 131 for (i = 0; i < job->bo_count; i++) { 132 dbo = job->bos[i]; 133 file_size += dbo->size; 134 n_bomap_pages += dbo->size >> PAGE_SHIFT; 135 n_obj++; 136 } 137 138 /* If we have any buffer objects, add a bomap object */ 139 if (n_bomap_pages) { 140 file_size += n_bomap_pages * sizeof(*bomap); 141 n_obj++; 142 } 143 144 /* Add the size of the headers */ 145 file_size += sizeof(*iter.hdr) * n_obj; 146 147 /* Allocate the file in vmalloc memory, it's likely to be big */ 148 iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN | 149 __GFP_NORETRY); 150 if (!iter.start) { 151 dev_warn(pfdev->dev, "failed to allocate devcoredump file\n"); 152 return; 153 } 154 155 /* Point the data member after the headers */ 156 iter.hdr = iter.start; 157 iter.data = &iter.hdr[n_obj]; 158 159 memset(iter.hdr, 0, iter.data - iter.start); 160 161 /* 162 * For now, we write the job identifier in the register dump header, 163 * so that we can decode the entire dump later with pandecode 164 */ 165 iter.hdr->reghdr.jc = cpu_to_le64(job->jc); 166 iter.hdr->reghdr.version = cpu_to_le32(PANFROSTDUMP_VERSION_1); 167 iter.hdr->reghdr.gpu_id = cpu_to_le32(pfdev->features.id); 168 iter.hdr->reghdr.nbos = cpu_to_le64(job->bo_count); 169 170 panfrost_core_dump_registers(&iter, pfdev, as_nr, slot); 171 172 /* Reserve space for the bomap */ 173 if (job->bo_count) { 174 bomap_start = bomap = iter.data; 175 memset(bomap, 0, sizeof(*bomap) * n_bomap_pages); 176 panfrost_core_dump_header(&iter, PANFROSTDUMP_BUF_BOMAP, 177 bomap + n_bomap_pages); 178 } 179 180 for (i = 0; i < job->bo_count; i++) { 181 struct iosys_map map; 182 struct panfrost_gem_mapping *mapping; 183 struct panfrost_gem_object *bo; 184 struct sg_page_iter page_iter; 185 void *vaddr; 186 187 bo = to_panfrost_bo(job->bos[i]); 188 mapping = job->mappings[i]; 189 190 if (!bo->base.sgt) { 191 dev_err(pfdev->dev, "Panfrost Dump: BO has no sgt, cannot dump\n"); 192 iter.hdr->bomap.valid = 0; 193 goto dump_header; 194 } 195 196 ret = drm_gem_shmem_vmap(&bo->base, &map); 197 if (ret) { 198 dev_err(pfdev->dev, "Panfrost Dump: couldn't map Buffer Object\n"); 199 iter.hdr->bomap.valid = 0; 200 goto dump_header; 201 } 202 203 WARN_ON(!mapping->active); 204 205 iter.hdr->bomap.data[0] = cpu_to_le32((bomap - bomap_start)); 206 207 for_each_sgtable_page(bo->base.sgt, &page_iter, 0) { 208 struct page *page = sg_page_iter_page(&page_iter); 209 210 if (!IS_ERR(page)) 211 *bomap++ = cpu_to_le64(page_to_phys(page)); 212 else { 213 dev_err(pfdev->dev, "Panfrost Dump: wrong page\n"); 214 *bomap++ = ~cpu_to_le64(0); 215 } 216 } 217 218 iter.hdr->bomap.iova = cpu_to_le64(mapping->mmnode.start << PAGE_SHIFT); 219 220 vaddr = map.vaddr; 221 memcpy(iter.data, vaddr, bo->base.base.size); 222 223 drm_gem_shmem_vunmap(&bo->base, &map); 224 225 iter.hdr->bomap.valid = cpu_to_le64(1); 226
drivers/gpu/drm/panfrost/panfrost_dump.c: In function 'panfrost_core_dump':
drivers/gpu/drm/panfrost/panfrost_dump.c:115:20: error: 'struct panfrost_job' has no member named 'file_priv'
115 | as_nr = job->file_priv->mmu->as; | ^~
FWIW -- this is due to recent changes in panfrost, you should rebase on the latest drm-misc-next which is where the patch will be applied anyway.
On 6/21/2022 10:32 PM, Alyssa Rosenzweig wrote:
drivers/gpu/drm/panfrost/panfrost_dump.c: In function 'panfrost_core_dump':drivers/gpu/drm/panfrost/panfrost_dump.c:115:20: error: 'struct panfrost_job' has no member named 'file_priv'
115 | as_nr = job->file_priv->mmu->as; | ^~FWIW -- this is due to recent changes in panfrost, you should rebase on the latest drm-misc-next which is where the patch will be applied anyway.
Hi Alyssa,
Thanks for your help, we'll try drm-misc-next next time.
Best Regards, Rong Chen
Hi Rong Chen,
Sorry for the noise -- I think that was meant for Adrian!
Apologies,
Alyssa
On Wed, Jun 22, 2022 at 10:30:00AM +0800, Chen, Rong A wrote:
On 6/21/2022 10:32 PM, Alyssa Rosenzweig wrote:
drivers/gpu/drm/panfrost/panfrost_dump.c: In function 'panfrost_core_dump':drivers/gpu/drm/panfrost/panfrost_dump.c:115:20: error: 'struct panfrost_job' has no member named 'file_priv'
115 | as_nr = job->file_priv->mmu->as; | ^~FWIW -- this is due to recent changes in panfrost, you should rebase on the latest drm-misc-next which is where the patch will be applied anyway.
Hi Alyssa,
Thanks for your help, we'll try drm-misc-next next time.
Best Regards, Rong Chen
Hi "Adrián,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next] [also build test WARNING on linus/master v5.19-rc2 next-20220617] [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/intel-lab-lkp/linux/commits/Adri-n-Larumbe/devcoredump-su... base: git://anongit.freedesktop.org/drm/drm drm-next config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220621/202206211320.GPk3Mfi3-lkp@i...) compiler: arceb-elf-gcc (GCC) 11.3.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/intel-lab-lkp/linux/commit/297bd4948ab1f4eeb78389d57adc1e... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Adri-n-Larumbe/devcoredump-support-for-Panfrost-GPU-driver/20220621-103431 git checkout 297bd4948ab1f4eeb78389d57adc1edc819cb6f2 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/gpu/drm/panfrost/
If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/gpu/drm/panfrost/panfrost_dump.c: In function 'panfrost_core_dump': drivers/gpu/drm/panfrost/panfrost_dump.c:115:20: error: 'struct panfrost_job' has no member named 'file_priv' 115 | as_nr = job->file_priv->mmu->as; | ^~ In file included from include/linux/byteorder/big_endian.h:5, from arch/arc/include/uapi/asm/byteorder.h:14, from include/asm-generic/bitops/le.h:6, from arch/arc/include/asm/bitops.h:192, from include/linux/bitops.h:33, from include/linux/log2.h:12, from include/asm-generic/div64.h:55, from ./arch/arc/include/generated/asm/div64.h:1, from include/linux/math.h:6, from include/linux/math64.h:6, from include/linux/time64.h:5, from include/linux/restart_block.h:10, from include/linux/thread_info.h:14, from include/asm-generic/preempt.h:5, from ./arch/arc/include/generated/asm/preempt.h:1, from include/linux/preempt.h:78, from include/linux/rcupdate.h:27, from include/linux/rculist.h:11, from include/linux/pid.h:5, from include/linux/sched.h:14, from include/linux/ratelimit.h:6, from include/linux/dev_printk.h:16, from include/linux/device.h:15, from drivers/gpu/drm/panfrost/panfrost_dump.c:5:
include/uapi/linux/byteorder/big_endian.h:32:26: warning: conversion from 'long long unsigned int' to '__le32' {aka 'unsigned int'} changes value from '72057594037927936' to '0' [-Woverflow]
32 | #define __cpu_to_le64(x) ((__force __le64)__swab64((x))) | ^ include/linux/byteorder/generic.h:86:21: note: in expansion of macro '__cpu_to_le64' 86 | #define cpu_to_le64 __cpu_to_le64 | ^~~~~~~~~~~~~ drivers/gpu/drm/panfrost/panfrost_dump.c:225:41: note: in expansion of macro 'cpu_to_le64' 225 | iter.hdr->bomap.valid = cpu_to_le64(1); | ^~~~~~~~~~~
vim +32 include/uapi/linux/byteorder/big_endian.h
5921e6f8809b16 David Howells 2012-10-13 15 5921e6f8809b16 David Howells 2012-10-13 16 #define __constant_htonl(x) ((__force __be32)(__u32)(x)) 5921e6f8809b16 David Howells 2012-10-13 17 #define __constant_ntohl(x) ((__force __u32)(__be32)(x)) 5921e6f8809b16 David Howells 2012-10-13 18 #define __constant_htons(x) ((__force __be16)(__u16)(x)) 5921e6f8809b16 David Howells 2012-10-13 19 #define __constant_ntohs(x) ((__force __u16)(__be16)(x)) 5921e6f8809b16 David Howells 2012-10-13 20 #define __constant_cpu_to_le64(x) ((__force __le64)___constant_swab64((x))) 5921e6f8809b16 David Howells 2012-10-13 21 #define __constant_le64_to_cpu(x) ___constant_swab64((__force __u64)(__le64)(x)) 5921e6f8809b16 David Howells 2012-10-13 22 #define __constant_cpu_to_le32(x) ((__force __le32)___constant_swab32((x))) 5921e6f8809b16 David Howells 2012-10-13 23 #define __constant_le32_to_cpu(x) ___constant_swab32((__force __u32)(__le32)(x)) 5921e6f8809b16 David Howells 2012-10-13 24 #define __constant_cpu_to_le16(x) ((__force __le16)___constant_swab16((x))) 5921e6f8809b16 David Howells 2012-10-13 25 #define __constant_le16_to_cpu(x) ___constant_swab16((__force __u16)(__le16)(x)) 5921e6f8809b16 David Howells 2012-10-13 26 #define __constant_cpu_to_be64(x) ((__force __be64)(__u64)(x)) 5921e6f8809b16 David Howells 2012-10-13 27 #define __constant_be64_to_cpu(x) ((__force __u64)(__be64)(x)) 5921e6f8809b16 David Howells 2012-10-13 28 #define __constant_cpu_to_be32(x) ((__force __be32)(__u32)(x)) 5921e6f8809b16 David Howells 2012-10-13 29 #define __constant_be32_to_cpu(x) ((__force __u32)(__be32)(x)) 5921e6f8809b16 David Howells 2012-10-13 30 #define __constant_cpu_to_be16(x) ((__force __be16)(__u16)(x)) 5921e6f8809b16 David Howells 2012-10-13 31 #define __constant_be16_to_cpu(x) ((__force __u16)(__be16)(x)) 5921e6f8809b16 David Howells 2012-10-13 @32 #define __cpu_to_le64(x) ((__force __le64)__swab64((x))) 5921e6f8809b16 David Howells 2012-10-13 33 #define __le64_to_cpu(x) __swab64((__force __u64)(__le64)(x)) 5921e6f8809b16 David Howells 2012-10-13 34 #define __cpu_to_le32(x) ((__force __le32)__swab32((x))) 5921e6f8809b16 David Howells 2012-10-13 35 #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) 5921e6f8809b16 David Howells 2012-10-13 36 #define __cpu_to_le16(x) ((__force __le16)__swab16((x))) 5921e6f8809b16 David Howells 2012-10-13 37 #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) 5921e6f8809b16 David Howells 2012-10-13 38 #define __cpu_to_be64(x) ((__force __be64)(__u64)(x)) 5921e6f8809b16 David Howells 2012-10-13 39 #define __be64_to_cpu(x) ((__force __u64)(__be64)(x)) 5921e6f8809b16 David Howells 2012-10-13 40 #define __cpu_to_be32(x) ((__force __be32)(__u32)(x)) 5921e6f8809b16 David Howells 2012-10-13 41 #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) 5921e6f8809b16 David Howells 2012-10-13 42 #define __cpu_to_be16(x) ((__force __be16)(__u16)(x)) 5921e6f8809b16 David Howells 2012-10-13 43 #define __be16_to_cpu(x) ((__force __u16)(__be16)(x)) 5921e6f8809b16 David Howells 2012-10-13 44
Hi "Adrián,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm/drm-next] [also build test ERROR on linus/master v5.19-rc3 next-20220620] [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/intel-lab-lkp/linux/commits/Adri-n-Larumbe/devcoredump-su... base: git://anongit.freedesktop.org/drm/drm drm-next config: s390-buildonly-randconfig-r006-20220620 (https://download.01.org/0day-ci/archive/20220621/202206211507.zzo0qSdL-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project af6d2a0b6825e71965f3e2701a63c239fa0ad70f) 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 # install s390 cross compiling tool for clang build # apt-get install binutils-s390x-linux-gnu # https://github.com/intel-lab-lkp/linux/commit/297bd4948ab1f4eeb78389d57adc1e... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Adri-n-Larumbe/devcoredump-support-for-Panfrost-GPU-driver/20220621-103431 git checkout 297bd4948ab1f4eeb78389d57adc1edc819cb6f2 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/gpu/drm/panfrost/
If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com
All error/warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/panfrost/panfrost_dump.c:6: In file included from include/linux/devcoredump.h:12: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) ^ In file included from drivers/gpu/drm/panfrost/panfrost_dump.c:6: In file included from include/linux/devcoredump.h:12: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) ^ In file included from drivers/gpu/drm/panfrost/panfrost_dump.c:6: In file included from include/linux/devcoredump.h:12: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^
drivers/gpu/drm/panfrost/panfrost_dump.c:115:15: error: no member named 'file_priv' in 'struct panfrost_job'
as_nr = job->file_priv->mmu->as; ~~~ ^
drivers/gpu/drm/panfrost/panfrost_dump.c:225:27: warning: implicit conversion from '__le64' (aka 'unsigned long long') to '__le32' (aka 'unsigned int') changes value from 72057594037927936 to 0 [-Wconstant-conversion]
iter.hdr->bomap.valid = cpu_to_le64(1); ~ ^~~~~~~~~~~~~~ include/linux/byteorder/generic.h:86:21: note: expanded from macro 'cpu_to_le64' #define cpu_to_le64 __cpu_to_le64 ^ include/uapi/linux/byteorder/big_endian.h:32:27: note: expanded from macro '__cpu_to_le64' #define __cpu_to_le64(x) ((__force __le64)__swab64((x))) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 13 warnings and 1 error generated.
vim +115 drivers/gpu/drm/panfrost/panfrost_dump.c
102 103 void panfrost_core_dump(struct panfrost_job *job) 104 { 105 struct panfrost_device *pfdev = job->pfdev; 106 struct panfrost_dump_iterator iter; 107 struct drm_gem_object *dbo; 108 unsigned int n_obj, n_bomap_pages; 109 __le64 *bomap, *bomap_start; 110 size_t file_size; 111 u32 as_nr; 112 int slot; 113 int ret, i; 114
115 as_nr = job->file_priv->mmu->as;
116 slot = panfrost_job_get_slot(job); 117 slot = slot ? slot : 0; 118 119 /* Only catch the first event, or when manually re-armed */ 120 if (!panfrost_dump_core) 121 return; 122 panfrost_dump_core = false; 123 124 /* At least, we dump registers and end marker */ 125 n_obj = 2; 126 n_bomap_pages = 0; 127 file_size = ARRAY_SIZE(panfrost_dump_registers) * 128 sizeof(struct panfrost_dump_registers); 129 130 /* Add in the active buffer objects */ 131 for (i = 0; i < job->bo_count; i++) { 132 dbo = job->bos[i]; 133 file_size += dbo->size; 134 n_bomap_pages += dbo->size >> PAGE_SHIFT; 135 n_obj++; 136 } 137 138 /* If we have any buffer objects, add a bomap object */ 139 if (n_bomap_pages) { 140 file_size += n_bomap_pages * sizeof(*bomap); 141 n_obj++; 142 } 143 144 /* Add the size of the headers */ 145 file_size += sizeof(*iter.hdr) * n_obj; 146 147 /* Allocate the file in vmalloc memory, it's likely to be big */ 148 iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN | 149 __GFP_NORETRY); 150 if (!iter.start) { 151 dev_warn(pfdev->dev, "failed to allocate devcoredump file\n"); 152 return; 153 } 154 155 /* Point the data member after the headers */ 156 iter.hdr = iter.start; 157 iter.data = &iter.hdr[n_obj]; 158 159 memset(iter.hdr, 0, iter.data - iter.start); 160 161 /* 162 * For now, we write the job identifier in the register dump header, 163 * so that we can decode the entire dump later with pandecode 164 */ 165 iter.hdr->reghdr.jc = cpu_to_le64(job->jc); 166 iter.hdr->reghdr.version = cpu_to_le32(PANFROSTDUMP_VERSION_1); 167 iter.hdr->reghdr.gpu_id = cpu_to_le32(pfdev->features.id); 168 iter.hdr->reghdr.nbos = cpu_to_le64(job->bo_count); 169 170 panfrost_core_dump_registers(&iter, pfdev, as_nr, slot); 171 172 /* Reserve space for the bomap */ 173 if (job->bo_count) { 174 bomap_start = bomap = iter.data; 175 memset(bomap, 0, sizeof(*bomap) * n_bomap_pages); 176 panfrost_core_dump_header(&iter, PANFROSTDUMP_BUF_BOMAP, 177 bomap + n_bomap_pages); 178 } 179 180 for (i = 0; i < job->bo_count; i++) { 181 struct iosys_map map; 182 struct panfrost_gem_mapping *mapping; 183 struct panfrost_gem_object *bo; 184 struct sg_page_iter page_iter; 185 void *vaddr; 186 187 bo = to_panfrost_bo(job->bos[i]); 188 mapping = job->mappings[i]; 189 190 if (!bo->base.sgt) { 191 dev_err(pfdev->dev, "Panfrost Dump: BO has no sgt, cannot dump\n"); 192 iter.hdr->bomap.valid = 0; 193 goto dump_header; 194 } 195 196 ret = drm_gem_shmem_vmap(&bo->base, &map); 197 if (ret) { 198 dev_err(pfdev->dev, "Panfrost Dump: couldn't map Buffer Object\n"); 199 iter.hdr->bomap.valid = 0; 200 goto dump_header; 201 } 202 203 WARN_ON(!mapping->active); 204 205 iter.hdr->bomap.data[0] = cpu_to_le32((bomap - bomap_start)); 206 207 for_each_sgtable_page(bo->base.sgt, &page_iter, 0) { 208 struct page *page = sg_page_iter_page(&page_iter); 209 210 if (!IS_ERR(page)) 211 *bomap++ = cpu_to_le64(page_to_phys(page)); 212 else { 213 dev_err(pfdev->dev, "Panfrost Dump: wrong page\n"); 214 *bomap++ = ~cpu_to_le64(0); 215 } 216 } 217 218 iter.hdr->bomap.iova = cpu_to_le64(mapping->mmnode.start << PAGE_SHIFT); 219 220 vaddr = map.vaddr; 221 memcpy(iter.data, vaddr, bo->base.base.size); 222 223 drm_gem_shmem_vunmap(&bo->base, &map); 224
225 iter.hdr->bomap.valid = cpu_to_le64(1);
226
Hi Adrian,
Great work on the devcoredump support! This is really cool to see coming along, thank you! I've left a few notes below:
if (panfrost_dump_registers[i] >= JS_HEAD_LO(0) &&panfrost_dump_registers[i] <= JS_CONFIG_NEXT(0))js_as_offset = slot * 0x80;else if (panfrost_dump_registers[i] >= AS_TRANSTAB_LO(0) &&panfrost_dump_registers[i] <= AS_STATUS(0))js_as_offset = ((as_nr) << 6);
I'm not a fan of the magic numbers. Do you think it makes sense to add
#define JS_SLOT_STRIDE 0x80 #define MMU_AS_SHIFT 0x6
in the appropriate places in panfrost_regs.h, reexpress the existing #defines in terms of those
#define JS_HEAD_LO(n) (JS_BASE + ((n) * JS_SLOT_STRIDE) + 0x00) ... #define JS_FLUSH_ID_NEXT(n) (JS_BASE + ((n) * JS_SLOT_STRIDE) + 0x70) ... #define MM_AS(as) (0x2400 + ((as) << MMU_AS_SHIFT)
and then use those here?
Also, drop the parans around (as_nr), this isn't a macro.
- /* Add in the active buffer objects */
- for (i = 0; i < job->bo_count; i++) {
dbo = job->bos[i];file_size += dbo->size;n_bomap_pages += dbo->size >> PAGE_SHIFT;n_obj++;- }
Strictly, I don't think this is right -- what happens if the CPU is configured to use 16K or 64K pages? -- however, that mistake is pretty well entrenched in panfrost.ko right now and it doesn't seem to bother anyone (non-4K pages on arm64 are pretty rare outside of fruit computers).
That said, out-of-context there looks like an alignment question. Could we add an assert for that, documenting the invariant:
WARN_ON(!IS_ALIGNED(dbo->size, PAGE_SIZE));
- iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN |
__GFP_NORETRY);- if (!iter.start) {
dev_warn(pfdev->dev, "failed to allocate devcoredump file\n");return;- }
...
- memset(iter.hdr, 0, iter.data - iter.start);
Why are we using __GFP_NOWARN and __GFP_NORETRY? Why not plain vmalloc?
Also, why vmalloc instead of vzalloc? (Or adding __GFP_ZERO to the list of __vmalloc flags if __GFP_NOWARN/__GFP_NORETRY are really needed?) Are there relevant performance or security considerations?
+/* Definitions for coredump decoding in user space */ +#define PANFROSTDUMP_VERSION_1 1
I'm not a fan of an enum that just represents a number. Using the numbers directly means we can compare them in a natural way. Also, using a major/minor split like Steven suggested can help with semantic versioning.
Cheers, Alyssa
Hi Alyssa, thanks for the feedback.
If I don't answer directly to any of your concerns in this message, it's because I applied your suggestions in v3 of the patch straight away.
On 21.06.2022 09:03, Alyssa Rosenzweig wrote:
Hi Adrian,
Great work on the devcoredump support! This is really cool to see coming along, thank you! I've left a few notes below:
if (panfrost_dump_registers[i] >= JS_HEAD_LO(0) &&panfrost_dump_registers[i] <= JS_CONFIG_NEXT(0))js_as_offset = slot * 0x80;else if (panfrost_dump_registers[i] >= AS_TRANSTAB_LO(0) &&panfrost_dump_registers[i] <= AS_STATUS(0))js_as_offset = ((as_nr) << 6);I'm not a fan of the magic numbers. Do you think it makes sense to add
#define JS_SLOT_STRIDE 0x80 #define MMU_AS_SHIFT 0x6
in the appropriate places in panfrost_regs.h, reexpress the existing #defines in terms of those
#define JS_HEAD_LO(n) (JS_BASE + ((n) * JS_SLOT_STRIDE) + 0x00) ... #define JS_FLUSH_ID_NEXT(n) (JS_BASE + ((n) * JS_SLOT_STRIDE) + 0x70) ... #define MM_AS(as) (0x2400 + ((as) << MMU_AS_SHIFT)
and then use those here?
Also, drop the parans around (as_nr), this isn't a macro.
- /* Add in the active buffer objects */
- for (i = 0; i < job->bo_count; i++) {
dbo = job->bos[i];file_size += dbo->size;n_bomap_pages += dbo->size >> PAGE_SHIFT;n_obj++;- }
Strictly, I don't think this is right -- what happens if the CPU is configured to use 16K or 64K pages? -- however, that mistake is pretty well entrenched in panfrost.ko right now and it doesn't seem to bother anyone (non-4K pages on arm64 are pretty rare outside of fruit computers).
That said, out-of-context there looks like an alignment question. Could we add an assert for that, documenting the invariant:
WARN_ON(!IS_ALIGNED(dbo->size, PAGE_SIZE));
- iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN |
__GFP_NORETRY);- if (!iter.start) {
dev_warn(pfdev->dev, "failed to allocate devcoredump file\n");return;- }
...
- memset(iter.hdr, 0, iter.data - iter.start);
Why are we using __GFP_NOWARN and __GFP_NORETRY? Why not plain vmalloc?
Also, why vmalloc instead of vzalloc? (Or adding __GFP_ZERO to the list of __vmalloc flags if __GFP_NOWARN/__GFP_NORETRY are really needed?) Are there relevant performance or security considerations?
I borrowed this code from Etnaviv a while ago and the same doubt struck me then. My understanding of its intended behaviour is that because the dump file might be huge, we don't want the memory manager to trigger the OOM killer and annoy quite a few running processes because of a debug feature. Also since the code already handles the situation when an allocation fails by refusing to generate a dump, there's no need for the allocator to generate specific error messages.
So I guess it boils down to 'if there's quite enough memory to allocate a huge dump file, go ahead, otherwise don't reclaim any processes' pages for something that isn't essential'.
I don't see much use for __GFP_ZERO in this case, because the dump file gets memcpy'd with the contents of every single bo so whatever the original contents of the memory were at the time of the allocation, they're overwritten immediately.
+/* Definitions for coredump decoding in user space */ +#define PANFROSTDUMP_VERSION_1 1
I'm not a fan of an enum that just represents a number. Using the numbers directly means we can compare them in a natural way. Also, using a major/minor split like Steven suggested can help with semantic versioning.
Cheers, Alyssa
I've also rebased v3 on top of drm-misc-next and the compiler error because of the removed panfrost_job structure member is gone.
Adrian Larumbe
- iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN |
__GFP_NORETRY);- if (!iter.start) {
dev_warn(pfdev->dev, "failed to allocate devcoredump file\n");return;- }
...
- memset(iter.hdr, 0, iter.data - iter.start);
Why are we using __GFP_NOWARN and __GFP_NORETRY? Why not plain vmalloc?
Also, why vmalloc instead of vzalloc? (Or adding __GFP_ZERO to the list of __vmalloc flags if __GFP_NOWARN/__GFP_NORETRY are really needed?) Are there relevant performance or security considerations?
I borrowed this code from Etnaviv a while ago and the same doubt struck me then. My understanding of its intended behaviour is that because the dump file might be huge, we don't want the memory manager to trigger the OOM killer and annoy quite a few running processes because of a debug feature. Also since the code already handles the situation when an allocation fails by refusing to generate a dump, there's no need for the allocator to generate specific error messages.
So I guess it boils down to 'if there's quite enough memory to allocate a huge dump file, go ahead, otherwise don't reclaim any processes' pages for something that isn't essential'.
I don't see much use for __GFP_ZERO in this case, because the dump file gets memcpy'd with the contents of every single bo so whatever the original contents of the memory were at the time of the allocation, they're overwritten immediately.
I think that's a reasonable explanation, bearing in mind I'm firmly a userspace person ;-)
Please add a comment explaining the assumptions here, though, because the code will live longer than this ML thread.
I've also rebased v3 on top of drm-misc-next and the compiler error because of the removed panfrost_job structure member is gone.
Excellent
Am Mittwoch, dem 22.06.2022 um 02:54 +0100 schrieb Adri??n Larumbe:
Hi Alyssa, thanks for the feedback.
If I don't answer directly to any of your concerns in this message, it's because I applied your suggestions in v3 of the patch straight away.
On 21.06.2022 09:03, Alyssa Rosenzweig wrote:
Hi Adrian,
Great work on the devcoredump support! This is really cool to see coming along, thank you! I've left a few notes below:
if (panfrost_dump_registers[i] >= JS_HEAD_LO(0) &&panfrost_dump_registers[i] <= JS_CONFIG_NEXT(0))js_as_offset = slot * 0x80;else if (panfrost_dump_registers[i] >= AS_TRANSTAB_LO(0) &&panfrost_dump_registers[i] <= AS_STATUS(0))js_as_offset = ((as_nr) << 6);I'm not a fan of the magic numbers. Do you think it makes sense to add
#define JS_SLOT_STRIDE 0x80 #define MMU_AS_SHIFT 0x6
in the appropriate places in panfrost_regs.h, reexpress the existing #defines in terms of those
#define JS_HEAD_LO(n) (JS_BASE + ((n) * JS_SLOT_STRIDE) + 0x00) ... #define JS_FLUSH_ID_NEXT(n) (JS_BASE + ((n) * JS_SLOT_STRIDE) + 0x70) ... #define MM_AS(as) (0x2400 + ((as) << MMU_AS_SHIFT)
and then use those here?
Also, drop the parans around (as_nr), this isn't a macro.
- /* Add in the active buffer objects */
- for (i = 0; i < job->bo_count; i++) {
dbo = job->bos[i];file_size += dbo->size;n_bomap_pages += dbo->size >> PAGE_SHIFT;n_obj++;- }
Strictly, I don't think this is right -- what happens if the CPU is configured to use 16K or 64K pages? -- however, that mistake is pretty well entrenched in panfrost.ko right now and it doesn't seem to bother anyone (non-4K pages on arm64 are pretty rare outside of fruit computers).
That said, out-of-context there looks like an alignment question. Could we add an assert for that, documenting the invariant:
WARN_ON(!IS_ALIGNED(dbo->size, PAGE_SIZE));
- iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN |
__GFP_NORETRY);- if (!iter.start) {
dev_warn(pfdev->dev, "failed to allocate devcoredump file\n");return;- }
...
- memset(iter.hdr, 0, iter.data - iter.start);
Why are we using __GFP_NOWARN and __GFP_NORETRY? Why not plain vmalloc?
Also, why vmalloc instead of vzalloc? (Or adding __GFP_ZERO to the list of __vmalloc flags if __GFP_NOWARN/__GFP_NORETRY are really needed?) Are there relevant performance or security considerations?
I borrowed this code from Etnaviv a while ago and the same doubt struck me then. My understanding of its intended behaviour is that because the dump file might be huge, we don't want the memory manager to trigger the OOM killer and annoy quite a few running processes because of a debug feature. Also since the code already handles the situation when an allocation fails by refusing to generate a dump, there's no need for the allocator to generate specific error messages.
Yes, that's exactly the reasoning behind those flags. Without the __GFP_NORETRY the devcoredump might not only trigger reclaim, but also the OOM killer. People might see it as suboptimal user experience when their favorite app gets killed just to make space for a devcoredump, which they might not even be interested in. As the code is dealing properly with allocation failure, we also don't want to print a warning in the kernel log.
Regards, Lucas
dri-devel@lists.freedesktop.org
-
Adri??n Larumbe -
Adrián Larumbe
-
Alyssa Rosenzweig -
Chen, Rong A -
kernel test robot -
Lucas Stach