| Message ID | 20200715182955.3081774-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/adreno: fix gpu probe if no interconnect-names | expand |
On 7/15/20 2:29 PM, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > If there is no interconnect-names, but there is an interconnects > property, then of_icc_get(dev, "gfx-mem"); would return an error > rather than NULL. > > Also, if there is no interconnect-names property, there will never > be a ocmem path. But of_icc_get(dev, "ocmem") would return -EINVAL > instead of -ENODATA. Just don't bother trying in this case. > > Fixes: 8e29fb37b301 ("drm/msm: handle for EPROBE_DEFER for of_icc_get") > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index 0527e85184e1..c4ac998b90c8 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -979,6 +979,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > struct adreno_platform_config *config = dev->platform_data; > struct msm_gpu_config adreno_gpu_config = { 0 }; > struct msm_gpu *gpu = &adreno_gpu->base; > + bool has_interconnect_names = true; > int ret; > > adreno_gpu->funcs = funcs; > @@ -1005,12 +1006,13 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > /* Check for an interconnect path for the bus */ > gpu->icc_path = of_icc_get(dev, "gfx-mem"); > - if (!gpu->icc_path) { > + if (IS_ERR_OR_NULL(gpu->icc_path)) { > /* > * Keep compatbility with device trees that don't have an > * interconnect-names property. > */ > gpu->icc_path = of_icc_get(dev, NULL); This is misleading because if it gets a EPROBE_DEFER error (or any other error), it will hit this path. Maybe there's a specific error you can check for instead to identify the "no-interconnect-names" case? Also don't think its a good idea to be calling of_icc_get(dev, NULL) again when there's a EPROBE_DEFER, the interconnect driver could come up between the two calls > + has_interconnect_names = false; > } > if (IS_ERR(gpu->icc_path)) { > ret = PTR_ERR(gpu->icc_path); > @@ -1018,7 +1020,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > return ret; > } > > - gpu->ocmem_icc_path = of_icc_get(dev, "ocmem"); > + if (has_interconnect_names) > + gpu->ocmem_icc_path = of_icc_get(dev, "ocmem"); > + > if (IS_ERR(gpu->ocmem_icc_path)) { > ret = PTR_ERR(gpu->ocmem_icc_path); > gpu->ocmem_icc_path = NULL; >
On Wed, Jul 15, 2020 at 11:37 AM Jonathan Marek <jonathan@marek.ca> wrote: > > On 7/15/20 2:29 PM, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.org> > > > > If there is no interconnect-names, but there is an interconnects > > property, then of_icc_get(dev, "gfx-mem"); would return an error > > rather than NULL. > > > > Also, if there is no interconnect-names property, there will never > > be a ocmem path. But of_icc_get(dev, "ocmem") would return -EINVAL > > instead of -ENODATA. Just don't bother trying in this case. > > > > Fixes: 8e29fb37b301 ("drm/msm: handle for EPROBE_DEFER for of_icc_get") > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > > index 0527e85184e1..c4ac998b90c8 100644 > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > > @@ -979,6 +979,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > struct adreno_platform_config *config = dev->platform_data; > > struct msm_gpu_config adreno_gpu_config = { 0 }; > > struct msm_gpu *gpu = &adreno_gpu->base; > > + bool has_interconnect_names = true; > > int ret; > > > > adreno_gpu->funcs = funcs; > > @@ -1005,12 +1006,13 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > > > /* Check for an interconnect path for the bus */ > > gpu->icc_path = of_icc_get(dev, "gfx-mem"); > > - if (!gpu->icc_path) { > > + if (IS_ERR_OR_NULL(gpu->icc_path)) { > > /* > > * Keep compatbility with device trees that don't have an > > * interconnect-names property. > > */ > > gpu->icc_path = of_icc_get(dev, NULL); > > This is misleading because if it gets a EPROBE_DEFER error (or any other > error), it will hit this path. Maybe there's a specific error you can > check for instead to identify the "no-interconnect-names" case? good point, we should probably instead just explicitly check with of_find_property("interconnect-names") fwiw, of_icc_get() returns: - NULL if icc disabled, or no "interconnects" property - -EINVAL if name!=NULL and no "interconnect-names" - and looks like -ENODATA if name!=NULL but no match in interconnect-names The specific error returns aren't really called out in the API comment block, so not really sure how much we should rely on them not being implementation details. BR, -R > Also don't think its a good idea to be calling of_icc_get(dev, NULL) > again when there's a EPROBE_DEFER, the interconnect driver could come up > between the two calls > > > + has_interconnect_names = false; > > } > > if (IS_ERR(gpu->icc_path)) { > > ret = PTR_ERR(gpu->icc_path); > > @@ -1018,7 +1020,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, > > return ret; > > } > > > > - gpu->ocmem_icc_path = of_icc_get(dev, "ocmem"); > > + if (has_interconnect_names) > > + gpu->ocmem_icc_path = of_icc_get(dev, "ocmem"); > > + > > if (IS_ERR(gpu->ocmem_icc_path)) { > > ret = PTR_ERR(gpu->ocmem_icc_path); > > gpu->ocmem_icc_path = NULL; > >
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index 0527e85184e1..c4ac998b90c8 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -979,6 +979,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, struct adreno_platform_config *config = dev->platform_data; struct msm_gpu_config adreno_gpu_config = { 0 }; struct msm_gpu *gpu = &adreno_gpu->base; + bool has_interconnect_names = true; int ret; adreno_gpu->funcs = funcs; @@ -1005,12 +1006,13 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, /* Check for an interconnect path for the bus */ gpu->icc_path = of_icc_get(dev, "gfx-mem"); - if (!gpu->icc_path) { + if (IS_ERR_OR_NULL(gpu->icc_path)) { /* * Keep compatbility with device trees that don't have an * interconnect-names property. */ gpu->icc_path = of_icc_get(dev, NULL); + has_interconnect_names = false; } if (IS_ERR(gpu->icc_path)) { ret = PTR_ERR(gpu->icc_path); @@ -1018,7 +1020,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev, return ret; } - gpu->ocmem_icc_path = of_icc_get(dev, "ocmem"); + if (has_interconnect_names) + gpu->ocmem_icc_path = of_icc_get(dev, "ocmem"); + if (IS_ERR(gpu->ocmem_icc_path)) { ret = PTR_ERR(gpu->ocmem_icc_path); gpu->ocmem_icc_path = NULL;