| Message ID | 20240908-adreno-fix-cpas-v1-1-57697a0d747f@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: allow returning NULL from crete_address_space | expand |
> Under some circumstances it might be required to return NULL from the > create_address_space callback, meaning that the driver should use global > address space. … Please avoid a typo in the summary phrase. Regards, Markus
On 8.09.2024 7:59 PM, Dmitry Baryshkov wrote:
> Under some circumstance
Under what circumstances?
This branch is only taken if there's a .create_private_address_space
callback and it only seems to be there on a[67]xx.
a6xx_create_address_space returns:
- an ERR_PTR if msm_iommu_pagetable_create() fails
- retval of msm_gem_address_space_create() otherwise
- retval of msm_iommu_pagetable_create() is nullchecked here
again because we apparently we want to be double sure
- err_ptr(-enomem) is returned if allocating aspace fails
- otherwise aspace is allocated somewhere
Konrad
On Mon, 9 Sept 2024 at 13:34, Konrad Dybcio <konradybcio@kernel.org> wrote: > > On 8.09.2024 7:59 PM, Dmitry Baryshkov wrote: > > Under some circumstance > > Under what circumstances? > > This branch is only taken if there's a .create_private_address_space > callback and it only seems to be there on a[67]xx. Existing code doesn't. I stumbled upon it while debugging private address space translation. And that's why I wrote 'it might be required' rather than 'the function returns'. So yes, there is no issue with the current code. And at the same time not having this in place makes debugging more difficult. > a6xx_create_address_space returns: > > - an ERR_PTR if msm_iommu_pagetable_create() fails > - retval of msm_gem_address_space_create() otherwise > - retval of msm_iommu_pagetable_create() is nullchecked here > again because we apparently we want to be double sure > - err_ptr(-enomem) is returned if allocating aspace fails > - otherwise aspace is allocated somewhere > > Konrad
On 9.09.2024 1:25 PM, Dmitry Baryshkov wrote: > On Mon, 9 Sept 2024 at 13:34, Konrad Dybcio <konradybcio@kernel.org> wrote: >> >> On 8.09.2024 7:59 PM, Dmitry Baryshkov wrote: >>> Under some circumstance >> >> Under what circumstances? >> >> This branch is only taken if there's a .create_private_address_space >> callback and it only seems to be there on a[67]xx. > > Existing code doesn't. I stumbled upon it while debugging private > address space translation. And that's why I wrote 'it might be > required' rather than 'the function returns'. > So yes, there is no issue with the current code. And at the same time > not having this in place makes debugging more difficult. Feel free to submit your debugging code in a way that won't mess with non-debug paths then.. This is trying to solve a non-existent issue. Konrad
On Mon, 9 Sept 2024 at 14:29, Konrad Dybcio <konradybcio@kernel.org> wrote: > > On 9.09.2024 1:25 PM, Dmitry Baryshkov wrote: > > On Mon, 9 Sept 2024 at 13:34, Konrad Dybcio <konradybcio@kernel.org> wrote: > >> > >> On 8.09.2024 7:59 PM, Dmitry Baryshkov wrote: > >>> Under some circumstance > >> > >> Under what circumstances? > >> > >> This branch is only taken if there's a .create_private_address_space > >> callback and it only seems to be there on a[67]xx. > > > > Existing code doesn't. I stumbled upon it while debugging private > > address space translation. And that's why I wrote 'it might be > > required' rather than 'the function returns'. > > So yes, there is no issue with the current code. And at the same time > > not having this in place makes debugging more difficult. > > Feel free to submit your debugging code in a way that won't mess > with non-debug paths then.. This is trying to solve a non-existent > issue. Ack
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index a274b8466423..47803f410dbb 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -833,7 +833,7 @@ msm_gpu_create_private_address_space(struct msm_gpu *gpu, struct task_struct *ta */ if (gpu->funcs->create_private_address_space) { aspace = gpu->funcs->create_private_address_space(gpu); - if (!IS_ERR(aspace)) + if (!IS_ERR_OR_NULL(aspace)) aspace->pid = get_pid(task_pid(task)); }
Under some circumstances it might be required to return NULL from the create_address_space callback, meaning that the driver should use global address space. Use IS_ERR_OR_NULL() to guard aspace->pid assignment instead of just IS_ERR(). This plays well with the IS_ERR_OR_NULL() check few lines below. Fixes: 25faf2f2e065 ("drm/msm: Show process names in gem_describe") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/msm_gpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8 change-id: 20240908-adreno-fix-cpas-48f3b992439d Best regards,