Re: [PATCH] drm/msm/adreno: Check for zap node availability
On Fri, May 17, 2024 at 12:50:19PM -0700, Rob Clark wrote:
> From: Rob Clark
>
> This should allow disabling the zap node via an overlay, for slbounce.
>
> Suggested-by: Nikita Travkin
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index d9ea15994ae9..a00241e3373b 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -46,7 +46,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const
> char *fwname,
> }
>
> np = of_get_child_by_name(dev->of_node, "zap-shader");
> - if (!np) {
> + if (!np || !of_device_is_available(np)) {
if (!of_device_is_available(np)) {
would cover both cases and be slightly cleaner imho...
But this looks reasonable either way.
Reviewed-by: Bjorn Andersson
Regards,
Bjorn
> zap_available = false;
> return -ENODEV;
> }
> --
> 2.45.1
>
Re: [PATCH] drm/msm/adreno: Check for zap node availability
On Fri, May 17, 2024 at 12:50:19PM -0700, Rob Clark wrote: > From: Rob Clark > > This should allow disabling the zap node via an overlay, for slbounce. > > Suggested-by: Nikita Travkin > Signed-off-by: Rob Clark > --- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid
On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
> Prefer the struct drm_edid based functions for reading the EDID and
> updating the connector.
>
> Simplify the flow by updating the EDID property when the EDID is read
> instead of at .get_modes.
>
> Signed-off-by: Jani Nikula
>
> ---
The patch looks good to me, I'd like to hear an opinion from Doug (added
to CC).
Reviewed-by: Dmitry Baryshkov
What is the merge strategy for the series? Do you plan to pick up all
the patches to drm-misc or should we pick up individual patches into
driver trees?
>
> Cc: Rob Clark
> Cc: Abhinav Kumar
> Cc: Dmitry Baryshkov
> Cc: Sean Paul
> Cc: Marijn Suijten
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 11 +++
> drivers/gpu/drm/msm/dp/dp_panel.c | 47 +
> drivers/gpu/drm/msm/dp/dp_panel.h | 2 +-
> 3 files changed, 20 insertions(+), 40 deletions(-)
[skipped]
> @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel
> *dp_panel)
> panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>
> if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
> + /* FIXME: get rid of drm_edid_raw() */
The code here can get use of something like drm_edid_smth_checksum().
'Something', because I could not come up with the word that would make
it clear that it is the declared checksum instead of the actual /
computed one.
> + const struct edid *edid = drm_edid_raw(dp_panel->drm_edid);
> u8 checksum;
>
> - if (dp_panel->edid)
> - checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> + if (edid)
> + checksum = dp_panel_get_edid_checksum(edid);
> else
> checksum = dp_panel->connector->real_edid_checksum;
>
--
With best wishes
Dmitry
Re: [PATCH] drm/msm: Add obj flags to gpu devcoredump
On Mon, May 13, 2024 at 08:51:47AM -0700, Rob Clark wrote: > From: Rob Clark > > When debugging faults, it is useful to know how the BO is mapped (cached > vs WC, gpu readonly, etc). > > Signed-off-by: Rob Clark > --- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 1 + > drivers/gpu/drm/msm/msm_gpu.c | 6 -- > drivers/gpu/drm/msm/msm_gpu.h | 1 + > 3 files changed, 6 insertions(+), 2 deletions(-) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [RFC PATCH 4/4] drm/msm: switch msm_kms to use msm_iommu_disp_new()
On Fri, May 17, 2024 at 04:37:59PM -0700, Abhinav Kumar wrote: > Switch msm_kms to use msm_iommu_disp_new() so that the newly > registered fault handler will kick-in during any mmu faults. > > Signed-off-by: Abhinav Kumar > --- > drivers/gpu/drm/msm/msm_kms.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c > index 62c8e6163e81..1859efbbff1d 100644 > --- a/drivers/gpu/drm/msm/msm_kms.c > +++ b/drivers/gpu/drm/msm/msm_kms.c > @@ -181,7 +181,7 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct > drm_device *dev) > else > iommu_dev = mdss_dev; > > - mmu = msm_iommu_new(iommu_dev, 0); > + mmu = msm_iommu_disp_new(iommu_dev, 0); > if (IS_ERR(mmu)) > return ERR_CAST(mmu); Reviewed-by: Dmitry Baryshkov Note to myself: make mdp4 use msm_kms_init_aspace(). -- With best wishes Dmitry
Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults
On Fri, May 17, 2024 at 04:37:56PM -0700, Abhinav Kumar wrote:
> In preparation to register a iommu fault handler for display
> related modules, register a fault handler for the backing
> mmu object of msm_kms.
>
> Currently, the fault handler only captures the display snapshot
> but we can expand this later if more information needs to be
> added to debug display mmu faults.
>
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/msm/msm_kms.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index af6a6fcb1173..62c8e6163e81 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
> @@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct
> drm_device *dev)
> return aspace;
> }
>
> +static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags,
> void *data)
> +{
> + struct msm_kms *kms = arg;
> + struct msm_disp_state *state;
> + int ret;
> +
> + ret = mutex_lock_interruptible(>dump_mutex);
> + if (ret)
> + return ret;
> +
> + state = msm_disp_snapshot_state_sync(kms);
> +
> + mutex_unlock(>dump_mutex);
> +
> + if (IS_ERR(state)) {
> + DRM_DEV_ERROR(kms->dev->dev, "failed to capture snapshot\n");
> + return PTR_ERR(state);
> + }
> +
> + return 0;
Hmm, after reading the rest of the code, this means that we won't get
the error on the console. Could you please change this to -ENOSYS?
> +}
> +
> void msm_drm_kms_uninit(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> @@ -261,6 +283,9 @@ int msm_drm_kms_init(struct device *dev, const struct
> drm_driver *drv)
> goto err_msm_uninit;
> }
>
> + if (kms->aspace)
> + msm_mmu_set_fault_handler(kms->aspace->mmu, kms,
> msm_kms_fault_handler);
> +
> drm_helper_move_panel_connectors_to_head(ddev);
>
> drm_for_each_crtc(crtc, ddev) {
> --
> 2.44.0
>
--
With best wishes
Dmitry
Re: [RFC PATCH 3/4] drm/msm/iommu: introduce msm_iommu_disp_new() for msm_kms
On Fri, May 17, 2024 at 04:37:58PM -0700, Abhinav Kumar wrote:
> Introduce a new API msm_iommu_disp_new() for display use-cases.
>
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/msm/msm_iommu.c | 28
> drivers/gpu/drm/msm/msm_mmu.h | 1 +
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index a79cd18bc4c9..3d5c1bb4c013 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -343,6 +343,19 @@ static int msm_gpu_fault_handler(struct iommu_domain
> *domain, struct device *dev
> return 0;
> }
>
> +static int msm_disp_fault_handler(struct iommu_domain *domain, struct device
> *dev,
> + unsigned long iova, int flags, void *arg)
> +{
> + struct msm_iommu *iommu = arg;
> +
> + if (iommu->base.handler)
> + return iommu->base.handler(iommu->base.arg, iova, flags, NULL);
> +
> + pr_warn_ratelimited("*** fault: iova=%16lx, flags=%d\n", iova, flags);
> +
> + return 0;
I'd say, drop pr_warn and return -ENOSYS, letting the
arm_smmu_context_fault() report the error.
> +}
> +
> static void msm_iommu_resume_translation(struct msm_mmu *mmu)
> {
> struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(mmu->dev);
> @@ -434,6 +447,21 @@ struct msm_mmu *msm_iommu_new(struct device *dev,
> unsigned long quirks)
> return >base;
> }
>
> +struct msm_mmu *msm_iommu_disp_new(struct device *dev, unsigned long quirks)
> +{
> + struct msm_iommu *iommu;
> + struct msm_mmu *mmu;
> +
> + mmu = msm_iommu_new(dev, quirks);
> + if (IS_ERR_OR_NULL(mmu))
> + return mmu;
> +
> + iommu = to_msm_iommu(mmu);
> + iommu_set_fault_handler(iommu->domain, msm_disp_fault_handler, iommu);
> +
> + return mmu;
> +}
> +
> struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu,
> unsigned long quirks)
> {
> struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(dev);
> diff --git a/drivers/gpu/drm/msm/msm_mmu.h b/drivers/gpu/drm/msm/msm_mmu.h
> index 88af4f490881..730458d08d6b 100644
> --- a/drivers/gpu/drm/msm/msm_mmu.h
> +++ b/drivers/gpu/drm/msm/msm_mmu.h
> @@ -42,6 +42,7 @@ static inline void msm_mmu_init(struct msm_mmu *mmu, struct
> device *dev,
>
> struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks);
> struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu,
> unsigned long quirks);
> +struct msm_mmu *msm_iommu_disp_new(struct device *dev, unsigned long quirks);
>
> static inline void msm_mmu_set_fault_handler(struct msm_mmu *mmu, void *arg,
> int (*handler)(void *arg, unsigned long iova, int flags, void
> *data))
> --
> 2.44.0
>
--
With best wishes
Dmitry
Re: [RFC PATCH 2/4] drm/msm/iommu: rename msm_fault_handler to msm_gpu_fault_handler
On Fri, May 17, 2024 at 04:37:57PM -0700, Abhinav Kumar wrote: > In preparation of registering a separate fault handler for > display, lets rename the existing msm_fault_handler to > msm_gpu_fault_handler. > > Signed-off-by: Abhinav Kumar > --- > drivers/gpu/drm/msm/msm_iommu.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults
On Fri, May 17, 2024 at 04:37:56PM -0700, Abhinav Kumar wrote:
> In preparation to register a iommu fault handler for display
> related modules, register a fault handler for the backing
> mmu object of msm_kms.
>
> Currently, the fault handler only captures the display snapshot
> but we can expand this later if more information needs to be
> added to debug display mmu faults.
>
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/msm/msm_kms.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index af6a6fcb1173..62c8e6163e81 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
> @@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct
> drm_device *dev)
> return aspace;
> }
>
> +static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags,
> void *data)
> +{
> + struct msm_kms *kms = arg;
> + struct msm_disp_state *state;
> + int ret;
> +
> + ret = mutex_lock_interruptible(>dump_mutex);
> + if (ret)
> + return ret;
> +
> + state = msm_disp_snapshot_state_sync(kms);
> +
> + mutex_unlock(>dump_mutex);
> +
> + if (IS_ERR(state)) {
> + DRM_DEV_ERROR(kms->dev->dev, "failed to capture snapshot\n");
> + return PTR_ERR(state);
> + }
> +
> + return 0;
> +}
> +
> void msm_drm_kms_uninit(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> @@ -261,6 +283,9 @@ int msm_drm_kms_init(struct device *dev, const struct
> drm_driver *drv)
> goto err_msm_uninit;
> }
>
> + if (kms->aspace)
> + msm_mmu_set_fault_handler(kms->aspace->mmu, kms,
> msm_kms_fault_handler);
> +
Can we move this to msm_kms_init_aspace() instead of checking for
kms->aspace?
> drm_helper_move_panel_connectors_to_head(ddev);
>
> drm_for_each_crtc(crtc, ddev) {
> --
> 2.44.0
>
--
With best wishes
Dmitry
Re: [RFC PATCH 0/4] drm/msm: add a display mmu fault handler
On Fri, May 17, 2024 at 04:37:55PM -0700, Abhinav Kumar wrote: > To debug display mmu faults, this series introduces a display fault > handler similar to the gpu one. > > This is only compile tested at the moment, till a suitable method > to trigger the fault is found and see if this handler does the needful > on the device. You should always be able to trigger the issue by programming wrong values to the SSPP. > > Abhinav Kumar (4): > drm/msm: register a fault handler for display mmu faults > drm/msm/iommu: rename msm_fault_handler to msm_gpu_fault_handler > drm/msm/iommu: introduce msm_iommu_disp_new() for msm_kms > drm/msm: switch msm_kms to use msm_iommu_disp_new() > > drivers/gpu/drm/msm/msm_iommu.c | 34 ++--- > drivers/gpu/drm/msm/msm_kms.c | 27 +- > drivers/gpu/drm/msm/msm_mmu.h | 1 + > 3 files changed, 58 insertions(+), 4 deletions(-) > > -- > 2.44.0 > -- With best wishes Dmitry
Re: [PATCH v4] drm/msm/a6xx: request memory region
On Sun, May 12, 2024 at 05:03:53AM -0400, Kiarash Hajian wrote:
> The driver's memory regions are currently just ioremap()ed, but not
> reserved through a request. That's not a bug, but having the request is
> a little more robust.
>
> Implement the region-request through the corresponding managed
> devres-function.
>
> Signed-off-by: Kiarash Hajian
> ---
> Changes in v4:
> - Combine v3 commits into a singel commit
> - Link to v3:
> https://lore.kernel.org/r/20240512-msm-adreno-memory-region-v3-0-0a728ad45...@gmail.com
>
> Changes in v3:
> - Remove redundant devm_iounmap calls, relying on devres for automatic
> resource cleanup.
>
> Changes in v2:
> - update the subject prefix to "drm/msm/a6xx:", to match the majority of
> other changes to this file.
> ---
> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 22 +-
> 1 file changed, 1 insertion(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 8bea8ef26f77..cf0b3b3d8f34 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -524,9 +524,6 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
> uint32_t pdc_address_offset;
> bool pdc_in_aop = false;
>
> - if (IS_ERR(pdcptr))
> - goto err;
So, if there is an error, we just continue through? What about the code
that accesses the region afterwards?
If error handling becomes void, then there should be an early return
instead of dropping the error check completely.
> -
> if (adreno_is_a650(adreno_gpu) ||
> adreno_is_a660_family(adreno_gpu) ||
> adreno_is_a7xx(adreno_gpu))
> @@ -540,8 +537,6 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
>
> if (!pdc_in_aop) {
> seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq");
> - if (IS_ERR(seqptr))
> - goto err;
Same question.
> }
>
> /* Disable SDE clock gating */
> @@ -633,12 +628,6 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
> wmb();
>
> a6xx_rpmh_stop(gmu);
> -
> -err:
> - if (!IS_ERR_OR_NULL(pdcptr))
> - iounmap(pdcptr);
> - if (!IS_ERR_OR_NULL(seqptr))
> - iounmap(seqptr);
> }
>
> /*
> @@ -1503,7 +1492,7 @@ static void __iomem *a6xx_gmu_get_mmio(struct
> platform_device *pdev,
> return ERR_PTR(-EINVAL);
> }
>
> - ret = ioremap(res->start, resource_size(res));
> + ret = devm_ioremap_resource(>dev, res);
> if (!ret) {
> DRM_DEV_ERROR(>dev, "Unable to map the %s registers\n",
> name);
> return ERR_PTR(-EINVAL);
> @@ -1613,13 +1602,11 @@ int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu,
> struct device_node *node)
> gmu->mmio = a6xx_gmu_get_mmio(pdev, "gmu");
> if (IS_ERR(gmu->mmio)) {
> ret = PTR_ERR(gmu->mmio);
> - goto err_mmio;
And this is even worse. See the comment below.
> }
>
> gmu->cxpd = dev_pm_domain_attach_by_name(gmu->dev, "cx");
> if (IS_ERR(gmu->cxpd)) {
> ret = PTR_ERR(gmu->cxpd);
> - goto err_mmio;
> }
>
> if (!device_link_add(gmu->dev, gmu->cxpd, DL_FLAG_PM_RUNTIME)) {
> @@ -1635,7 +1622,6 @@ int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu,
> struct device_node *node)
> gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
> if (IS_ERR(gmu->gxpd)) {
> ret = PTR_ERR(gmu->gxpd);
> - goto err_mmio;
> }
>
> gmu->initialized = true;
> @@ -1645,9 +1631,6 @@ int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu,
> struct device_node *node)
> detach_cxpd:
> dev_pm_domain_detach(gmu->cxpd, false);
>
> -err_mmio:
> - iounmap(gmu->mmio);
> -
You have dropped the iounmap(). However now the error path should
remain. The put_device() must be called. So fix the label name and just
drop the iounmap().
> /* Drop reference taken in of_find_device_by_node */
> put_device(gmu->dev);
>
> @@ -1825,9 +1808,6 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
> device_node *node)
> dev_pm_domain_detach(gmu->cxpd, false);
>
> err_mmio:
> - iounmap(gmu->mmio);
> - if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "rscc"))
> - iounmap(gmu->rscc);
Same comment here.
> free_irq(gmu->gmu_irq, gmu);
> free_irq(gmu->hfi_irq, gmu);
>
>
> ---
> base-commit: cf87f46fd34d6c19283d9625a7822f20d90b64a4
> change-id: 20240511-msm-adreno-memory-region-2bcb1c958621
>
> Best regards,
> --
> Kiarash Hajian
>
--
With best wishes
Dmitry
