Re: [PATCH v2] drm/gem-shmem: When drm_gem_object_init failed, should release object
Hi Thomas, Can I discard the first two patchs, and pull the new code, then modify and git send-email this patch? ?? Thu, 17 Nov 2022 14:42:36 +0100 Thomas Zimmermann : > Hi > > Am 11.11.22 um 04:38 schrieb ChunyouTang: > > when goto err_free, the object had init, so it should be release > > when fail. > > > > Signed-off-by: ChunyouTang > > --- > > drivers/gpu/drm/drm_gem.c | 19 --- > > drivers/gpu/drm/drm_gem_shmem_helper.c | 4 +++- > > include/drm/drm_gem.h | 1 + > > 3 files changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 8b68a3c1e6ab..cba32c46bb05 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -169,6 +169,21 @@ void drm_gem_private_object_init(struct > > drm_device *dev, } > > EXPORT_SYMBOL(drm_gem_private_object_init); > > > > +/** > > + * drm_gem_private_object_fini - Finalize a failed drm_gem_object > > + * @obj: drm_gem_object > > + * > > + * Uninitialize an already allocated GEM object when it > > initialized failed > > + */ > > +void drm_gem_private_object_fini(struct drm_gem_object *obj) > > +{ > > + WARN_ON(obj->dma_buf); > > Rather lease this in its original place. > > > + > > + dma_resv_fini(&obj->_resv); > > + drm_gem_lru_remove(obj); > > AFAICT drm_gem_lru_remove() doesn't belong into this function. > > > +} > > +EXPORT_SYMBOL(drm_gem_private_object_fini); > > + > > /** > >* drm_gem_object_handle_free - release resources bound to > > userspace handles > >* @obj: GEM object to clean up. > > @@ -930,14 +945,12 @@ drm_gem_release(struct drm_device *dev, > > struct drm_file *file_private) void > > drm_gem_object_release(struct drm_gem_object *obj) > > { > > - WARN_ON(obj->dma_buf); > > + drm_gem_private_object_fini(obj); > > > > if (obj->filp) > > fput(obj->filp); > > > > - dma_resv_fini(&obj->_resv); > > Please call drm_gem_private_object_fini() here. > > > drm_gem_free_mmap_offset(obj); > > - drm_gem_lru_remove(obj); > > Please keep this line here. > > Best regards > Thomas > > > } > > EXPORT_SYMBOL(drm_gem_object_release); > > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c > > b/drivers/gpu/drm/drm_gem_shmem_helper.c index > > 35138f8a375c..845e3d5d71eb 100644 --- > > a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ > > b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -79,8 +79,10 @@ > > __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool > > private) } else { ret = drm_gem_object_init(dev, obj, size); > > } > > - if (ret) > > + if (ret) { > > + drm_gem_private_object_fini(obj) > > goto err_free; > > + } > > > > ret = drm_gem_create_mmap_offset(obj); > > if (ret) > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > > index bd42f25e449c..9b1feb03069d 100644 > > --- a/include/drm/drm_gem.h > > +++ b/include/drm/drm_gem.h > > @@ -405,6 +405,7 @@ int drm_gem_object_init(struct drm_device *dev, > > struct drm_gem_object *obj, size_t size); > > void drm_gem_private_object_init(struct drm_device *dev, > > struct drm_gem_object *obj, > > size_t size); +void drm_gem_private_object_fini(struct > > drm_gem_object *obj); void drm_gem_vm_open(struct vm_area_struct > > *vma); void drm_gem_vm_close(struct vm_area_struct *vma); > > int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long > > obj_size, >
Re: [PATCH] drm/gem-shmem: When drm_gem_object_init failed, should release object
Hi, drm_gem_object_init() will do these before failed: void drm_gem_private_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size) { BUG_ON((size & (PAGE_SIZE - 1)) != 0); obj->dev = dev; obj->filp = NULL; kref_init(&obj->refcount); obj->handle_count = 0; obj->size = size; dma_resv_init(&obj->_resv); if (!obj->resv) obj->resv = &obj->_resv; drm_vma_node_reset(&obj->vma_node); INIT_LIST_HEAD(&obj->lru_node); } so I think when it failed, should use drm_gem_object_release() to do dma_resv_fini() and others. another, if drm_gem_object_init() fails, the drm_gem_handle_create() can not be called. ?? Tue, 8 Nov 2022 09:28:34 +0100 Thomas Zimmermann : > Hi > > Am 08.11.22 um 03:03 schrieb ChunyouTang: > > when goto err_free, the object had init, so it should be release > > when fail. > > If the call to drm_gem_object_init() fails, the object is still > uninitialized. Admittedly, the call to gem_create_object could need > additional cleanup, but it appears as if no one has had a need for > this so far. > > Is there anything that might leak? > > > > > Signed-off-by: ChunyouTang > > --- > > drivers/gpu/drm/drm_gem_shmem_helper.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c > > b/drivers/gpu/drm/drm_gem_shmem_helper.c index > > 35138f8a375c..2e5e3207355f 100644 --- > > a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ > > b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -104,10 +104,10 @@ > > __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool > > private) return shmem; > > > > -err_release: > > - drm_gem_object_release(obj); > > err_free: > > kfree(obj); > > +err_release: > > + drm_gem_object_release(obj); > > You have now freed the object's memory before releasing it. Not going > to work. > > Best regards > Thomas > > > > > return ERR_PTR(ret); > > } >
Re: [PATCH v2] drm/panfrost:report the full raw fault information instead
Hi Steve, > You didn't answer my previous question: > > > Is this device working with the kbase/DDK proprietary driver? I don't know whether I used kbase/DDK,I only know I used the driver of panfrost in linux 5.11. > What you are describing sounds like a hardware integration issue, so > it would be good to check that the hardware is working with the > proprietary driver to rule that out. And perhaps there is something > in the kbase for this device that is setting a chicken bit to 'fix' > the coherency? I don't have the proprietary driver,I only used driver in linux 5.11. Thinks very much! Chunyou. ?? Thu, 1 Jul 2021 11:15:14 +0100 Steven Price : > On 29/06/2021 04:04, Chunyou Tang wrote: > > Hi Steve, > > thinks for your reply. > > I set the pte in arm_lpae_prot_to_pte(), > > *** > > /* > > * Also Mali has its own notions of shareability wherein its > > Inner > > * domain covers the cores within the GPU, and its Outer > > domain is > > * "outside the GPU" (i.e. either the Inner or System > > domain in CPU > > * terms, depending on coherency). > > */ > > if (prot & IOMMU_CACHE && data->iop.fmt != ARM_MALI_LPAE) > > pte |= ARM_LPAE_PTE_SH_IS; > > else > > pte |= ARM_LPAE_PTE_SH_OS; > > *** > > I set pte |= ARM_LPAE_PTE_SH_NS. > > > > If I set pte to ARM_LPAE_PTE_SH_OS or > > ARM_LPAE_PTE_SH_IS,whether I use singel core GPU or multi > > core GPU,it will occur GPU Fault. > > if I set pte to ARM_LPAE_PTE_SH_NS,whether I use singel core > > GPU or multi core GPU,it will not occur GPU Fault. > > Hi, > > So this is a difference between Panfrost and kbase. Panfrost (well > technically the IOMMU framework) enables the inner-shareable bit for > all memory, whereas kbase only enables it for some memory types (the > BASE_MEM_COHERENT_LOCAL flag in the UABI controls it). However this > should only be a performance/power difference (and AFAIK probably an > irrelevant one) and it's definitely required that "inner shareable" > (i.e. within the GPU) works for communication between the different > units of the GPU. > > You didn't answer my previous question: > > > Is this device working with the kbase/DDK proprietary driver? > > What you are describing sounds like a hardware integration issue, so > it would be good to check that the hardware is working with the > proprietary driver to rule that out. And perhaps there is something > in the kbase for this device that is setting a chicken bit to 'fix' > the coherency? > > Steve
Re: [PATCH v2] drm/panfrost:report the full raw fault information instead
Hi Robin, thinks for you reply. I had send more detal to Steve in another mail,thinks for your messages. ?? Mon, 28 Jun 2021 15:17:51 +0100 Robin Murphy : > On 2021-06-28 11:48, Steven Price wrote: > > On 25/06/2021 10:49, Chunyou Tang wrote: > >> Hi Steve, > >>Thinks for your reply. > >>When I only set the pte |= ARM_LPAE_PTE_SH_NS;there have > >> no "GPU Fault",When I set the pte |= ARM_LPAE_PTE_SH_IS(or > >> ARM_LPAE_PTE_SH_OS);there have "GPU Fault".I don't know how the pte > >> effect this issue? > >>Can you give me some suggestions again? > >> > >> Thinks. > >> > >> Chunyou > > > > Hi Chunyou, > > > > You haven't given me much context so I'm not entirely sure which > > PTE you are talking about (GPU or CPU), or indeed where you are > > changing the PTE values. > > > > The PTEs control whether a page is shareable or not, the GPU > > requires that accesses are consistent (i.e. either all accesses to > > a page are shareable or all are non-shareable) and will race a > > fault if it detects this isn't the case. Mali also has a quirk for > > its version of 'LPAE' where inner shareable actually means only > > within the GPU and outer shareable means outside the GPU (which I > > think usually means Inner Shareable on the external bus). > > Furthermore, the way the io-pgtable code works for ARM_MALI_LPAE > format means that *all* GPU mappings are unconditionally > outer-shareable, so it's not clear how the GPU could observe a > mismatch in the first place (other than major integration issues > causing data corruption). > > Robin. > > > > > Steve > > > >> ?? Thu, 24 Jun 2021 14:22:04 +0100 > >> Steven Price : > >> > >>> On 22/06/2021 02:40, Chunyou Tang wrote: > >>>> Hi Steve, > >>>> I will send a new patch with suitable subject/commit > >>>> message. But I send a V3 or a new patch? > >>> > >>> Send a V3 - it is a new version of this patch. > >>> > >>>> I met a bug about the GPU,I have no idea about how to fix > >>>> it, If you can give me some suggestion,it is perfect. > >>>> > >>>> You can see such kernel log: > >>>> > >>>> Jun 20 10:20:13 icube kernel: [ 774.566760] mvp_gpu > >>>> :05:00.0: GPU Fault 0x0088 (SHAREABILITY_FAULT) at > >>>> 0x0310fd00 Jun 20 10:20:13 icube kernel: [ 774.566764] > >>>> mvp_gpu :05:00.0: There were multiple GPU faults - some have > >>>> not been reported Jun 20 10:20:13 icube kernel: [ 774.667542] > >>>> mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:13 icube > >>>> kernel: [ 774.767900] mvp_gpu :05:00.0: AS_ACTIVE bit stuck > >>>> Jun 20 10:20:13 icube kernel: [ 774.868546] mvp_gpu > >>>> :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:13 icube kernel: > >>>> [ 774.968910] mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 > >>>> 10:20:13 icube kernel: [ 775.069251] mvp_gpu :05:00.0: > >>>> AS_ACTIVE bit stuck Jun 20 10:20:22 icube kernel: [ 783.693971] > >>>> mvp_gpu :05:00.0: gpu sched timeout, js=1, config=0x7300, > >>>> status=0x8, head=0x362c900, tail=0x362c100, > >>>> sched_job=3252fb84 > >>>> > >>>> In > >>>> https://lore.kernel.org/dri-devel/20200510165538.19720-1-peron.c...@gmail.com/ > >>>> there had a same bug like mine,and I found you at the mail list,I > >>>> don't know how it fixed? > >>> > >>> The GPU_SHAREABILITY_FAULT error means that a cache line has been > >>> accessed both as shareable and non-shareable and therefore > >>> coherency cannot be guaranteed. Although the "multiple GPU > >>> faults" means that this may not be the underlying cause. > >>> > >>> The fact that your dmesg log has PCI style identifiers > >>> (":05:00.0") suggests this is an unusual platform - I've not > >>> previously been aware of a Mali device behind PCI. Is this device > >>> working with the kbase/DDK proprietary driver? It would be worth > >>> looking at the kbase kernel code for the platform to see if there > >>> is anything special done for the platform. > >>> > >>> From the dmesg lo
Re: [PATCH v2] drm/panfrost:report the full raw fault information instead
Hi Steve, thinks for your reply. I set the pte in arm_lpae_prot_to_pte(), *** /* * Also Mali has its own notions of shareability wherein its Inner * domain covers the cores within the GPU, and its Outer domain is * "outside the GPU" (i.e. either the Inner or System domain in CPU * terms, depending on coherency). */ if (prot & IOMMU_CACHE && data->iop.fmt != ARM_MALI_LPAE) pte |= ARM_LPAE_PTE_SH_IS; else pte |= ARM_LPAE_PTE_SH_OS; *** I set pte |= ARM_LPAE_PTE_SH_NS. If I set pte to ARM_LPAE_PTE_SH_OS or ARM_LPAE_PTE_SH_IS,whether I use singel core GPU or multi core GPU,it will occur GPU Fault. if I set pte to ARM_LPAE_PTE_SH_NS,whether I use singel core GPU or multi core GPU,it will not occur GPU Fault. Thinks Chunyou ?? Mon, 28 Jun 2021 11:48:59 +0100 Steven Price : > On 25/06/2021 10:49, Chunyou Tang wrote: > > Hi Steve, > > Thinks for your reply. > > When I only set the pte |= ARM_LPAE_PTE_SH_NS;there have no > > "GPU Fault",When I set the pte |= ARM_LPAE_PTE_SH_IS(or > > ARM_LPAE_PTE_SH_OS);there have "GPU Fault".I don't know how the pte > > effect this issue? > > Can you give me some suggestions again? > > > > Thinks. > > > > Chunyou > > Hi Chunyou, > > You haven't given me much context so I'm not entirely sure which PTE > you are talking about (GPU or CPU), or indeed where you are changing > the PTE values. > > The PTEs control whether a page is shareable or not, the GPU requires > that accesses are consistent (i.e. either all accesses to a page are > shareable or all are non-shareable) and will race a fault if it > detects this isn't the case. Mali also has a quirk for its version of > 'LPAE' where inner shareable actually means only within the GPU and > outer shareable means outside the GPU (which I think usually means > Inner Shareable on the external bus). > > Steve > > > ?? Thu, 24 Jun 2021 14:22:04 +0100 > > Steven Price : > > > >> On 22/06/2021 02:40, Chunyou Tang wrote: > >>> Hi Steve, > >>> I will send a new patch with suitable subject/commit > >>> message. But I send a V3 or a new patch? > >> > >> Send a V3 - it is a new version of this patch. > >> > >>> I met a bug about the GPU,I have no idea about how to fix > >>> it, If you can give me some suggestion,it is perfect. > >>> > >>> You can see such kernel log: > >>> > >>> Jun 20 10:20:13 icube kernel: [ 774.566760] mvp_gpu :05:00.0: > >>> GPU Fault 0x0088 (SHAREABILITY_FAULT) at 0x0310fd00 > >>> Jun 20 10:20:13 icube kernel: [ 774.566764] mvp_gpu :05:00.0: > >>> There were multiple GPU faults - some have not been reported Jun > >>> 20 10:20:13 icube kernel: [ 774.667542] mvp_gpu :05:00.0: > >>> AS_ACTIVE bit stuck Jun 20 10:20:13 icube kernel: [ 774.767900] > >>> mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:13 icube > >>> kernel: [ 774.868546] mvp_gpu :05:00.0: AS_ACTIVE bit stuck > >>> Jun 20 10:20:13 icube kernel: [ 774.968910] mvp_gpu :05:00.0: > >>> AS_ACTIVE bit stuck Jun 20 10:20:13 icube kernel: [ 775.069251] > >>> mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:22 icube > >>> kernel: [ 783.693971] mvp_gpu :05:00.0: gpu sched timeout, > >>> js=1, config=0x7300, status=0x8, head=0x362c900, tail=0x362c100, > >>> sched_job=3252fb84 > >>> > >>> In > >>> https://lore.kernel.org/dri-devel/20200510165538.19720-1-peron.c...@gmail.com/ > >>> there had a same bug like mine,and I found you at the mail list,I > >>> don't know how it fixed? > >> > >> The GPU_SHAREABILITY_FAULT error means that a cache line has been > >> accessed both as shareable and non-shareable and therefore > >> coherency cannot be guaranteed. Although the "multiple GPU faults" > >> means that this may not be the underlying cause. > >> > >> The fact that your dmesg log has PCI style identifiers > >> (":05:00.0") suggests this is an unusual platform - I've not > >> previously been aware of a Mali device behind PCI. Is this device > >> working with the kbase/DDK proprietary driver? It would b
Re: [PATCH v2] drm/panfrost:report the full raw fault information instead
Hi Steve, Thinks for your reply. When I only set the pte |= ARM_LPAE_PTE_SH_NS;there have no "GPU Fault",When I set the pte |= ARM_LPAE_PTE_SH_IS(or ARM_LPAE_PTE_SH_OS);there have "GPU Fault".I don't know how the pte effect this issue? Can you give me some suggestions again? Thinks. Chunyou ?? Thu, 24 Jun 2021 14:22:04 +0100 Steven Price : > On 22/06/2021 02:40, Chunyou Tang wrote: > > Hi Steve, > > I will send a new patch with suitable subject/commit > > message. But I send a V3 or a new patch? > > Send a V3 - it is a new version of this patch. > > > I met a bug about the GPU,I have no idea about how to fix > > it, If you can give me some suggestion,it is perfect. > > > > You can see such kernel log: > > > > Jun 20 10:20:13 icube kernel: [ 774.566760] mvp_gpu :05:00.0: > > GPU Fault 0x0088 (SHAREABILITY_FAULT) at 0x0310fd00 Jun > > 20 10:20:13 icube kernel: [ 774.566764] mvp_gpu :05:00.0: > > There were multiple GPU faults - some have not been reported Jun 20 > > 10:20:13 icube kernel: [ 774.667542] mvp_gpu :05:00.0: > > AS_ACTIVE bit stuck Jun 20 10:20:13 icube kernel: [ 774.767900] > > mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:13 icube > > kernel: [ 774.868546] mvp_gpu :05:00.0: AS_ACTIVE bit stuck > > Jun 20 10:20:13 icube kernel: [ 774.968910] mvp_gpu :05:00.0: > > AS_ACTIVE bit stuck Jun 20 10:20:13 icube kernel: [ 775.069251] > > mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:22 icube > > kernel: [ 783.693971] mvp_gpu :05:00.0: gpu sched timeout, > > js=1, config=0x7300, status=0x8, head=0x362c900, tail=0x362c100, > > sched_job=3252fb84 > > > > In > > https://lore.kernel.org/dri-devel/20200510165538.19720-1-peron.c...@gmail.com/ > > there had a same bug like mine,and I found you at the mail list,I > > don't know how it fixed? > > The GPU_SHAREABILITY_FAULT error means that a cache line has been > accessed both as shareable and non-shareable and therefore coherency > cannot be guaranteed. Although the "multiple GPU faults" means that > this may not be the underlying cause. > > The fact that your dmesg log has PCI style identifiers > (":05:00.0") suggests this is an unusual platform - I've not > previously been aware of a Mali device behind PCI. Is this device > working with the kbase/DDK proprietary driver? It would be worth > looking at the kbase kernel code for the platform to see if there is > anything special done for the platform. > > From the dmesg logs all I can really tell is that the GPU seems > unhappy about the memory system. > > Steve > > > I need your help! > > > > thinks very much! > > > > Chunyou > > > > ?? Mon, 21 Jun 2021 11:45:20 +0100 > > Steven Price : > > > >> On 19/06/2021 04:18, Chunyou Tang wrote: > >>> Hi Steve, > >>> 1,Now I know how to write the subject > >>> 2,the low 8 bits is the exception type in spec. > >>> > >>> and you can see prnfrost_exception_name() > >>> > >>> switch (exception_code) { > >>> /* Non-Fault Status code */ > >>> case 0x00: return "NOT_STARTED/IDLE/OK"; > >>> case 0x01: return "DONE"; > >>> case 0x02: return "INTERRUPTED"; > >>> case 0x03: return "STOPPED"; > >>> case 0x04: return "TERMINATED"; > >>> case 0x08: return "ACTIVE"; > >>> > >>> > >>> case 0xD8: return "ACCESS_FLAG"; > >>> case 0xD9 ... 0xDF: return "ACCESS_FLAG"; > >>> case 0xE0 ... 0xE7: return "ADDRESS_SIZE_FAULT"; > >>> case 0xE8 ... 0xEF: return "MEMORY_ATTRIBUTES_FAULT"; > >>> } > >>> return "UNKNOWN"; > >>> } > >>> > >>> the exception_code in case is only 8 bits,so if fault_status > >>> in panfrost_gpu_irq_handler() don't & 0xFF,it can't get correct > >>> exception reason,it will be always UNKNOWN. > >> > >> Yes, I'm happy with the change - I just need a patch that I can > >> apply. At the moment this patch only changes the first '0x%08x' > >> output rather than the call to panfrost_exception_name() as well. > >> So we just need a patch which does: > >> > >> - fault_status & 0xFF, panfrost_exception_name(pfdev, > >> fault_status), > >&
Re: [PATCH v2] drm/panfrost:report the full raw fault information instead
Hi Steve, I will send a new patch with suitable subject/commit message. But I send a V3 or a new patch? I met a bug about the GPU,I have no idea about how to fix it, If you can give me some suggestion,it is perfect. You can see such kernel log: Jun 20 10:20:13 icube kernel: [ 774.566760] mvp_gpu :05:00.0: GPU Fault 0x0088 (SHAREABILITY_FAULT) at 0x0310fd00 Jun 20 10:20:13 icube kernel: [ 774.566764] mvp_gpu :05:00.0: There were multiple GPU faults - some have not been reported Jun 20 10:20:13 icube kernel: [ 774.667542] mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:13 icube kernel: [ 774.767900] mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:13 icube kernel: [ 774.868546] mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:13 icube kernel: [ 774.968910] mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:13 icube kernel: [ 775.069251] mvp_gpu :05:00.0: AS_ACTIVE bit stuck Jun 20 10:20:22 icube kernel: [ 783.693971] mvp_gpu :05:00.0: gpu sched timeout, js=1, config=0x7300, status=0x8, head=0x362c900, tail=0x362c100, sched_job=3252fb84 In https://lore.kernel.org/dri-devel/20200510165538.19720-1-peron.c...@gmail.com/ there had a same bug like mine,and I found you at the mail list,I don't know how it fixed? I need your help! thinks very much! Chunyou ?? Mon, 21 Jun 2021 11:45:20 +0100 Steven Price : > On 19/06/2021 04:18, Chunyou Tang wrote: > > Hi Steve, > > 1,Now I know how to write the subject > > 2,the low 8 bits is the exception type in spec. > > > > and you can see prnfrost_exception_name() > > > > switch (exception_code) { > > /* Non-Fault Status code */ > > case 0x00: return "NOT_STARTED/IDLE/OK"; > > case 0x01: return "DONE"; > > case 0x02: return "INTERRUPTED"; > > case 0x03: return "STOPPED"; > > case 0x04: return "TERMINATED"; > > case 0x08: return "ACTIVE"; > > > > > > case 0xD8: return "ACCESS_FLAG"; > > case 0xD9 ... 0xDF: return "ACCESS_FLAG"; > > case 0xE0 ... 0xE7: return "ADDRESS_SIZE_FAULT"; > > case 0xE8 ... 0xEF: return "MEMORY_ATTRIBUTES_FAULT"; > > } > > return "UNKNOWN"; > > } > > > > the exception_code in case is only 8 bits,so if fault_status > > in panfrost_gpu_irq_handler() don't & 0xFF,it can't get correct > > exception reason,it will be always UNKNOWN. > > Yes, I'm happy with the change - I just need a patch that I can apply. > At the moment this patch only changes the first '0x%08x' output rather > than the call to panfrost_exception_name() as well. So we just need a > patch which does: > > - fault_status & 0xFF, panfrost_exception_name(pfdev, fault_status), > + fault_status, panfrost_exception_name(pfdev, fault_status & 0xFF), > > along with a suitable subject/commit message describing the change. If > you can send me that I can apply it. > > Thanks, > > Steve > > PS. Sorry for going round in circles here - I'm trying to help you get > setup so you'll be able to contribute patches easily in future. An > important part of that is ensuring you can send a properly formatted > patch to the list. > > PPS. I'm still not receiving your emails directly. I don't think it's > a problem at my end because I'm receiving other emails, but if you can > somehow fix the problem you're likely to receive a faster response. > > > ?? Fri, 18 Jun 2021 13:43:24 +0100 > > Steven Price : > > > >> On 17/06/2021 07:20, ChunyouTang wrote: > >>> From: ChunyouTang > >>> > >>> of the low 8 bits. > >> > >> Please don't split the subject like this. The first line of the > >> commit should be a (very short) summary of the patch. Then a blank > >> line and then a longer description of what the purpose of the > >> patch is and why it's needed. > >> > >> Also you previously had this as part of a series (the first part > >> adding the "& 0xFF" in the panfrost_exception_name() call). I'm not > >> sure we need two patches for the single line, but as it stands this > >> patch doesn't apply. > >> > >> Also I'm still not receiving any emails from you directly (only via > >> the list), so it's possible I might have missed something you sent. > >> > >> Steve > >> > >>> > >>> Signed-off-by: ChunyouTang > >>> --- > >>> drivers/
Re: [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation
Hi Steve, I make a mistake about the code branch,I will test it later, thinks for your reply. Chunyou ?? Mon, 21 Jun 2021 11:45:18 +0100 Steven Price : > On 19/06/2021 04:09, Chunyou Tang wrote: > > Hi Steve, > > 1, > > from > > https://lore.kernel.org/lkml/31644881-134a-2d6e-dddf-e658a3a81...@arm.com/ > > I can see what your sent,I used a wrong email address,Now it > > correct. 2, > >>> Unless I'm mistaken the situation where some mappings may be NULL > >>> is caused by the loop in panfrost_lookup_bos() not completing > >>> successfully > >>> (panfrost_gem_mapping_get() returning NULL). In this case if > >>> mappings[i] > >>> is NULL then all following mappings must also be NULL. So 'break' > >>> allows > >>> us to skip the later ones. Admittedly the performance here isn't > >>> important so I'm not sure it's worth the optimisation, but AIUI > >>> this code isn't actually wrong. > > > > from panfrost_lookup_bos(),you can see: > > for (i = 0; i < job->bo_count; i++) { > > struct panfrost_gem_mapping *mapping; > > > > bo = to_panfrost_bo(job->bos[i]); > > ICUBE_DEBUG_PRINTK("panfrost bo gem handle=0x%x > > is_dumb=%d\n", bo->gem_handle, bo->is_dumb); > > if (!bo->is_dumb) { > >mapping = panfrost_gem_mapping_get(bo, priv); > >if (!mapping) { > > ret = -EINVAL; > > break; > >} > > > > atomic_inc(&bo->gpu_usecount); > > job->mappings[i] = mapping; > > } else { > > atomic_inc(&bo->gpu_usecount); > > job->mappings[i] = NULL; > > } > > } > > This code isn't upstream - in drm-misc/drm-misc-next (and all mainline > kernels from what I can tell) this doesn't have any "is_dumb" test. > Which branch are you using? > > > if bo->is_dumb is TRUE,the job->mappings[i] will set to NULL,and the > > while will be continue,so if job->mappings[i] is NULL,the following > > can not be NULL. > > I agree that with the above code the panfrost_job_cleanup() would need > changing. But we don't (currently) have this code upstream, so this > change doesn't make sense upstream. > > Thanks, > > Steve > > > 3, > > I've had this problem in our project,the value of is_dumb like > > these: 0 > > 0 > > 0 > > 1 > > 0 > > 0 > > 0 > > so,when job->mappings[i] is NULL,we can not break the while in > > panfrost_job_cleanup(). > > > > thanks > > Chunyou > > > > ?? Fri, 18 Jun 2021 13:43:25 +0100 > > Steven Price : > > > >> On 17/06/2021 09:04, ChunyouTang wrote: > >>> From: ChunyouTang > >>> > >>> The 'break' can cause 'Memory manager not clean during takedown' > >>> > >>> It cannot use break to finish the circulation,it should use > >>> > >>> continue to traverse the circulation.it should put every mapping > >>> > >>> which is not NULL. > >> > >> You don't appear to have answered my question about whether you've > >> actually seen this happen (and ideally what circumstances). In my > >> previous email[1] I explained why I don't think this is needed. You > >> need to convince me that I've overlooked something. > >> > >> Thanks, > >> > >> Steve > >> > >> [1] > >> https://lore.kernel.org/r/31644881-134a-2d6e-dddf-e658a3a8176b%40arm.com > >> > >>> Signed-off-by: ChunyouTang > >>> --- > >>> drivers/gpu/drm/panfrost/panfrost_job.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c > >>> b/drivers/gpu/drm/panfrost/panfrost_job.c index > >>> 6003cfeb1322..52bccc1d2d42 100644 --- > >>> a/drivers/gpu/drm/panfrost/panfrost_job.c +++ > >>> b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -281,7 +281,7 @@ > >>> static void panfrost_job_cleanup(struct kref *ref) if > >>> (job->mappings) { for (i = 0; i < job->bo_count; i++) { > >>> if (!job->mappings[i]) > >>> - break; > >>> + continue; > >>> > >>> atomic_dec(&job->mappings[i]->obj->gpu_usecount); > >>> panfrost_gem_mapping_put(job->mappings[i]); > >>> > > > >
Re: [PATCH v2] drm/panfrost:report the full raw fault information instead
Hi Steve, 1,Now I know how to write the subject 2,the low 8 bits is the exception type in spec. and you can see prnfrost_exception_name() switch (exception_code) { /* Non-Fault Status code */ case 0x00: return "NOT_STARTED/IDLE/OK"; case 0x01: return "DONE"; case 0x02: return "INTERRUPTED"; case 0x03: return "STOPPED"; case 0x04: return "TERMINATED"; case 0x08: return "ACTIVE"; case 0xD8: return "ACCESS_FLAG"; case 0xD9 ... 0xDF: return "ACCESS_FLAG"; case 0xE0 ... 0xE7: return "ADDRESS_SIZE_FAULT"; case 0xE8 ... 0xEF: return "MEMORY_ATTRIBUTES_FAULT"; } return "UNKNOWN"; } the exception_code in case is only 8 bits,so if fault_status in panfrost_gpu_irq_handler() don't & 0xFF,it can't get correct exception reason,it will be always UNKNOWN. ?? Fri, 18 Jun 2021 13:43:24 +0100 Steven Price : > On 17/06/2021 07:20, ChunyouTang wrote: > > From: ChunyouTang > > > > of the low 8 bits. > > Please don't split the subject like this. The first line of the commit > should be a (very short) summary of the patch. Then a blank line and > then a longer description of what the purpose of the patch is and why > it's needed. > > Also you previously had this as part of a series (the first part > adding the "& 0xFF" in the panfrost_exception_name() call). I'm not > sure we need two patches for the single line, but as it stands this > patch doesn't apply. > > Also I'm still not receiving any emails from you directly (only via > the list), so it's possible I might have missed something you sent. > > Steve > > > > > Signed-off-by: ChunyouTang > > --- > > drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c > > b/drivers/gpu/drm/panfrost/panfrost_gpu.c index > > 1fffb6a0b24f..d2d287bbf4e7 100644 --- > > a/drivers/gpu/drm/panfrost/panfrost_gpu.c +++ > > b/drivers/gpu/drm/panfrost/panfrost_gpu.c @@ -33,7 +33,7 @@ static > > irqreturn_t panfrost_gpu_irq_handler(int irq, void *data) address > > |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO); > > dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at > > 0x%016llx\n", > > -fault_status & 0xFF, > > panfrost_exception_name(pfdev, fault_status & 0xFF), > > +fault_status, > > panfrost_exception_name(pfdev, fault_status & 0xFF), address); > > > > if (state & GPU_IRQ_MULTIPLE_FAULT) > >
Re: [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation
Hi Steve, 1, from https://lore.kernel.org/lkml/31644881-134a-2d6e-dddf-e658a3a81...@arm.com/ I can see what your sent,I used a wrong email address,Now it correct. 2, > >Unless I'm mistaken the situation where some mappings may be NULL is > >caused by the loop in panfrost_lookup_bos() not completing > >successfully > >(panfrost_gem_mapping_get() returning NULL). In this case if > >mappings[i] > >is NULL then all following mappings must also be NULL. So 'break' > >allows > >us to skip the later ones. Admittedly the performance here isn't > >important so I'm not sure it's worth the optimisation, but AIUI this > >code isn't actually wrong. from panfrost_lookup_bos(),you can see: for (i = 0; i < job->bo_count; i++) { struct panfrost_gem_mapping *mapping; bo = to_panfrost_bo(job->bos[i]); ICUBE_DEBUG_PRINTK("panfrost bo gem handle=0x%x is_dumb=%d\n", bo->gem_handle, bo->is_dumb); if (!bo->is_dumb) { mapping = panfrost_gem_mapping_get(bo, priv); if (!mapping) { ret = -EINVAL; break; } atomic_inc(&bo->gpu_usecount); job->mappings[i] = mapping; } else { atomic_inc(&bo->gpu_usecount); job->mappings[i] = NULL; } } if bo->is_dumb is TRUE,the job->mappings[i] will set to NULL,and the while will be continue,so if job->mappings[i] is NULL,the following can not be NULL. 3, I've had this problem in our project,the value of is_dumb like these: 0 0 0 1 0 0 0 so,when job->mappings[i] is NULL,we can not break the while in panfrost_job_cleanup(). thanks Chunyou ?? Fri, 18 Jun 2021 13:43:25 +0100 Steven Price : > On 17/06/2021 09:04, ChunyouTang wrote: > > From: ChunyouTang > > > > The 'break' can cause 'Memory manager not clean during takedown' > > > > It cannot use break to finish the circulation,it should use > > > > continue to traverse the circulation.it should put every mapping > > > > which is not NULL. > > You don't appear to have answered my question about whether you've > actually seen this happen (and ideally what circumstances). In my > previous email[1] I explained why I don't think this is needed. You > need to convince me that I've overlooked something. > > Thanks, > > Steve > > [1] > https://lore.kernel.org/r/31644881-134a-2d6e-dddf-e658a3a8176b%40arm.com > > > Signed-off-by: ChunyouTang > > --- > > drivers/gpu/drm/panfrost/panfrost_job.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c > > b/drivers/gpu/drm/panfrost/panfrost_job.c index > > 6003cfeb1322..52bccc1d2d42 100644 --- > > a/drivers/gpu/drm/panfrost/panfrost_job.c +++ > > b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -281,7 +281,7 @@ > > static void panfrost_job_cleanup(struct kref *ref) if > > (job->mappings) { for (i = 0; i < job->bo_count; i++) { > > if (!job->mappings[i]) > > - break; > > + continue; > > > > atomic_dec(&job->mappings[i]->obj->gpu_usecount); > > panfrost_gem_mapping_put(job->mappings[i]); > >
Re: [PATCH] modified: gpu/drm/panfrost/panfrost_gpu.c
Hi steve, After I send the V2,I found I setting a wrong email configuration,I hope it doesn't affect the patch submission :) Did you received my another patch about panfrost_job.c? Author: tangchunyou Date: Wed Jun 9 14:44:52 2021 +0800 modified: gpu/drm/panfrost/panfrost_job.c The 'break' can cause 'Memory manager not clean during takedown' It cannot use break to finish the circulation,it should use continue to traverse the circulation. Signed-off-by: tangchunyou diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 6003cfeb1322..52bccc1d2d42 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -281,7 +281,7 @@ static void panfrost_job_cleanup(struct kref *ref) if (job->mappings) { for (i = 0; i < job->bo_count; i++) { if (!job->mappings[i]) - break; + continue; atomic_dec(&job->mappings[i]->obj->gpu_usecount); panfrost_gem_mapping_put(job->mappings[i]); Thank you! ?? Fri, 11 Jun 2021 11:10:16 +0100 Steven Price : > On 10/06/2021 14:06, Chunyou Tang wrote: > > Hi Steven, > > Hi Chunyou, > > For some reason I'm not directly receiving your emails (only via the > list) - can you double check your email configuration? > > >>> The GPU exception fault status register(0x3C),the low 8 bit is the > >>> EXCEPTION_TYPE.We can see the description at P3-78 in spec. > > > > You can see the spec > > . > > Thanks - please include that in the commit message - there are many > TRMs (one for each GPU) so without the information about exactly which > specification the page number is pretty useless. Sadly this > documentation isn't public which would be even better but I don't > think there are any public specs for this information. > > >> However this change is correct - panfrost_exception_name() should > >> be taking only the lower 8 bits. Even better though would be to to > >> report the full raw fault information as well as the high bits can > >> contain useful information: > >> > >>dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at > >> 0x%016llx\n", fault_status, > >> panfrost_exception_name(pfdev, fault_status & > >> 0xFF), address); > > > > So I change it according to what you said? > > Yes, please send a v2. > > Thanks, > > Steve > > > ?? Thu, 10 Jun 2021 11:41:52 +0100 > > Steven Price : > > > >> The subject should have the prefix "drm/panfrost" and should > >> mention what the patch is changing (not just the filename). > >> > >> On 09/06/2021 07:38, ChunyouTang wrote: > >>> From: tangchunyou > >>> > >>> The GPU exception fault status register(0x3C),the low 8 bit is the > >>> EXCEPTION_TYPE.We can see the description at P3-78 in spec. > >> > >> Nit: When referring to a spec it's always good to mention the name > >> - I'm not sure which specification you found this in. > >> > >>> > >>> Signed-off-by: tangchunyou > >>> --- > >>> drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c > >>> b/drivers/gpu/drm/panfrost/panfrost_gpu.c index > >>> 2aae636f1cf5..1fffb6a0b24f 100644 --- > >>> a/drivers/gpu/drm/panfrost/panfrost_gpu.c +++ > >>> b/drivers/gpu/drm/panfrost/panfrost_gpu.c @@ -33,7 +33,7 @@ static > >>> irqreturn_t panfrost_gpu_irq_handler(int irq, void *data) address > >>> |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO); > >>> dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at > >>> 0x%016llx\n", > >>> - fault_status & 0xFF, > >>> panfrost_exception_name(pfdev, fault_status), > >>> + fault_status & 0xFF, > >>> panfrost_exception_name(pfdev, fault_status & 0xFF), > >> > >> However this change is correct - panfrost_exception_name() should > >> be taking only the lower 8 bits. Even better though would be to to > >> report the full raw fault information as well as the high bits can > >> contain useful information: > >> > >>dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at > >> 0x%016llx\n", fault_status, > >> panfrost_exception_name(pfdev, fault_status & > >> 0xFF), address); > >> > >> Thanks, > >> > >> Steve > >> > >>>address); > >>> > >>> if (state & GPU_IRQ_MULTIPLE_FAULT) > >>> > > > >
Re: [PATCH] modified: gpu/drm/panfrost/panfrost_gpu.c
?? Fri, 11 Jun 2021 11:10:16 +0100 Steven Price : > On 10/06/2021 14:06, Chunyou Tang wrote: > > Hi Steven, > > Hi Chunyou, > > For some reason I'm not directly receiving your emails (only via the > list) - can you double check your email configuration? > > >>> The GPU exception fault status register(0x3C),the low 8 bit is the > >>> EXCEPTION_TYPE.We can see the description at P3-78 in spec. > > > > You can see the spec > > . > > Thanks - please include that in the commit message - there are many > TRMs (one for each GPU) so without the information about exactly which > specification the page number is pretty useless. Sadly this > documentation isn't public which would be even better but I don't > think there are any public specs for this information. > > >> However this change is correct - panfrost_exception_name() should > >> be taking only the lower 8 bits. Even better though would be to to > >> report the full raw fault information as well as the high bits can > >> contain useful information: > >> > >>dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at > >> 0x%016llx\n", fault_status, > >> panfrost_exception_name(pfdev, fault_status & > >> 0xFF), address); > > > > So I change it according to what you said? > > Yes, please send a v2. > > Thanks, > > Steve > > > ?? Thu, 10 Jun 2021 11:41:52 +0100 > > Steven Price : > > > >> The subject should have the prefix "drm/panfrost" and should > >> mention what the patch is changing (not just the filename). > >> > >> On 09/06/2021 07:38, ChunyouTang wrote: ok > >>> From: tangchunyou > >>> > >>> The GPU exception fault status register(0x3C),the low 8 bit is the > >>> EXCEPTION_TYPE.We can see the description at P3-78 in spec. > >> > >> Nit: When referring to a spec it's always good to mention the name > >> - I'm not sure which specification you found this in. > >> > >>> > >>> Signed-off-by: tangchunyou > >>> --- > >>> drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c > >>> b/drivers/gpu/drm/panfrost/panfrost_gpu.c index > >>> 2aae636f1cf5..1fffb6a0b24f 100644 --- > >>> a/drivers/gpu/drm/panfrost/panfrost_gpu.c +++ > >>> b/drivers/gpu/drm/panfrost/panfrost_gpu.c @@ -33,7 +33,7 @@ static > >>> irqreturn_t panfrost_gpu_irq_handler(int irq, void *data) address > >>> |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO); > >>> dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at > >>> 0x%016llx\n", > >>> - fault_status & 0xFF, > >>> panfrost_exception_name(pfdev, fault_status), > >>> + fault_status & 0xFF, > >>> panfrost_exception_name(pfdev, fault_status & 0xFF), > >> > >> However this change is correct - panfrost_exception_name() should > >> be taking only the lower 8 bits. Even better though would be to to > >> report the full raw fault information as well as the high bits can > >> contain useful information: > >> > >>dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at > >> 0x%016llx\n", fault_status, > >> panfrost_exception_name(pfdev, fault_status & > >> 0xFF), address); > >> > >> Thanks, > >> > >> Steve > >> > >>>address); > >>> > >>> if (state & GPU_IRQ_MULTIPLE_FAULT) > >>> > > > >
Re: [PATCH] modified: gpu/drm/panfrost/panfrost_gpu.c
Hi Steven, > > The GPU exception fault status register(0x3C),the low 8 bit is the > > EXCEPTION_TYPE.We can see the description at P3-78 in spec. You can see the spec . > However this change is correct - panfrost_exception_name() should be > taking only the lower 8 bits. Even better though would be to to report > the full raw fault information as well as the high bits can contain > useful information: > > dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n", >fault_status, >panfrost_exception_name(pfdev, fault_status & 0xFF), >address); So I change it according to what you said? ?? Thu, 10 Jun 2021 11:41:52 +0100 Steven Price : > The subject should have the prefix "drm/panfrost" and should mention > what the patch is changing (not just the filename). > > On 09/06/2021 07:38, ChunyouTang wrote: > > From: tangchunyou > > > > The GPU exception fault status register(0x3C),the low 8 bit is the > > EXCEPTION_TYPE.We can see the description at P3-78 in spec. > > Nit: When referring to a spec it's always good to mention the name - > I'm not sure which specification you found this in. > > > > > Signed-off-by: tangchunyou > > --- > > drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c > > b/drivers/gpu/drm/panfrost/panfrost_gpu.c index > > 2aae636f1cf5..1fffb6a0b24f 100644 --- > > a/drivers/gpu/drm/panfrost/panfrost_gpu.c +++ > > b/drivers/gpu/drm/panfrost/panfrost_gpu.c @@ -33,7 +33,7 @@ static > > irqreturn_t panfrost_gpu_irq_handler(int irq, void *data) address > > |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO); > > dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at > > 0x%016llx\n", > > -fault_status & 0xFF, > > panfrost_exception_name(pfdev, fault_status), > > +fault_status & 0xFF, > > panfrost_exception_name(pfdev, fault_status & 0xFF), > > However this change is correct - panfrost_exception_name() should be > taking only the lower 8 bits. Even better though would be to to report > the full raw fault information as well as the high bits can contain > useful information: > > dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n", >fault_status, >panfrost_exception_name(pfdev, fault_status & 0xFF), >address); > > Thanks, > > Steve > > > address); > > > > if (state & GPU_IRQ_MULTIPLE_FAULT) > >
Re: [PATCH] drivers/video/fbdev:modify 0 to NULL
Hi,Gustavo On Wed, 17 Mar 2021 20:54:41 -0500 "Gustavo A. R. Silva" wrote: > On 3/17/21 21:47, Chunyou Tang wrote: > > > I think "if (info == NULL)" is more intuitive,and there have many > > compare likes "if (info == NULL)" in this file. > > In that case, all those instances should be changed to if (!foo), > instead. > > -- > Gustavo OK,I change it. -- ChunyouTang ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drivers/video/fbdev:modify 0 to NULL
HI,Gustavo On Wed, 17 Mar 2021 20:41:15 -0500 "Gustavo A. R. Silva" wrote: > On 3/17/21 21:33, ChunyouTang wrote: > > From: tangchunyou > > > > modify 0 to NULL,info is a pointer,it should be > > > > compared with NULL,not 0 > > > > Signed-off-by: tangchunyou > > --- > > drivers/video/fbdev/offb.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/video/fbdev/offb.c b/drivers/video/fbdev/offb.c > > index 4501e84..cd1042f 100644 > > --- a/drivers/video/fbdev/offb.c > > +++ b/drivers/video/fbdev/offb.c > > @@ -412,7 +412,7 @@ static void __init offb_init_fb(const char > > *name, > > info = framebuffer_alloc(sizeof(u32) * 16, NULL); > > > > - if (info == 0) { > > + if (info == NULL) { > > if (!info) is better. > > -- > Gustavo > > > release_mem_region(res_start, res_size); > > return; > > } > > I think "if (info == NULL)" is more intuitive,and there have many compare likes "if (info == NULL)" in this file. -- ChunyouTang ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel