From: Rob Clark <robdclark@gmail.com> To: Jonathan Marek <jonathan@marek.ca> Cc: dri-devel <dri-devel@lists.freedesktop.org>, Rob Clark <robdclark@chromium.org>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Jordan Crouse <jcrouse@codeaurora.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, Brian Masney <masneyb@onstation.org>, John Stultz <john.stultz@linaro.org>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <linux-arm-msm@vger.kernel.org>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <freedreno@lists.freedesktop.org>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] drm/msm/adreno: fix gpu probe if no interconnect-names Date: Wed, 15 Jul 2020 11:46:55 -0700 [thread overview] Message-ID: <CAF6AEGt7b+9jGGARTEGiuNQMPTqixXWfvJ5CygU+2h-qL34pBg@mail.gmail.com> (raw) In-Reply-To: <baef95e0-e44f-be7d-f60f-0ba75b550050@marek.ca> 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; > >
WARNING: multiple messages have this Message-ID (diff)
From: Rob Clark <robdclark@gmail.com> To: Jonathan Marek <jonathan@marek.ca> Cc: Rob Clark <robdclark@chromium.org>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <freedreno@lists.freedesktop.org>, David Airlie <airlied@linux.ie>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <linux-arm-msm@vger.kernel.org>, open list <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, Sean Paul <sean@poorly.run>, Brian Masney <masneyb@onstation.org> Subject: Re: [PATCH] drm/msm/adreno: fix gpu probe if no interconnect-names Date: Wed, 15 Jul 2020 11:46:55 -0700 [thread overview] Message-ID: <CAF6AEGt7b+9jGGARTEGiuNQMPTqixXWfvJ5CygU+2h-qL34pBg@mail.gmail.com> (raw) In-Reply-To: <baef95e0-e44f-be7d-f60f-0ba75b550050@marek.ca> 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; > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-07-15 18:46 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-15 18:29 [PATCH] drm/msm/adreno: fix gpu probe if no interconnect-names Rob Clark 2020-07-15 18:29 ` Rob Clark 2020-07-15 18:36 ` Jonathan Marek 2020-07-15 18:36 ` Jonathan Marek 2020-07-15 18:46 ` Rob Clark [this message] 2020-07-15 18:46 ` Rob Clark 2020-07-15 19:07 Rob Clark 2020-07-15 19:07 ` Rob Clark 2020-07-15 20:05 ` Jordan Crouse 2020-07-15 20:05 ` Jordan Crouse
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAF6AEGt7b+9jGGARTEGiuNQMPTqixXWfvJ5CygU+2h-qL34pBg@mail.gmail.com \ --to=robdclark@gmail.com \ --cc=airlied@linux.ie \ --cc=bjorn.andersson@linaro.org \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=jcrouse@codeaurora.org \ --cc=john.stultz@linaro.org \ --cc=jonathan@marek.ca \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=masneyb@onstation.org \ --cc=robdclark@chromium.org \ --cc=sean@poorly.run \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.