Re: [Freedreno] [PATCH v18 1/5] iommu/arm-smmu: Add pm_runtime/sleep ops
On Fri, Nov 30, 2018 at 11:45 PM Will Deacon wrote:
>
> On Thu, Nov 29, 2018 at 08:25:20PM +0530, Vivek Gautam wrote:
> > On Wed, Nov 28, 2018 at 10:07 PM Robin Murphy wrote:
> > >
> > > On 28/11/2018 16:24, Stephen Boyd wrote:
> > > > Quoting Vivek Gautam (2018-11-27 02:11:41)
> > > >> @@ -1966,6 +1970,23 @@ static const struct of_device_id
> > > >> arm_smmu_of_match[] = {
> > > >> };
> > > >> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
> > > >>
> > > >> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
> > > >> + const char * const *clks)
> > > >> +{
> > > >> + int i;
> > > >> +
> > > >> + if (smmu->num_clks < 1)
> > > >> + return;
> > > >> +
> > > >> + smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
> > > >> + sizeof(*smmu->clks), GFP_KERNEL);
> > > >> + if (!smmu->clks)
> > > >> + return;
> > > >> +
> > > >> + for (i = 0; i < smmu->num_clks; i++)
> > > >> + smmu->clks[i].id = clks[i];
> > > >
> > > > Is this clk_bulk_get_all()?
> >
> > From what I remember, and now I could go back to v7 and check [1], we parked
> > clk_bulk_get out of OF's sole purview as we also have
> > arm_smmu_device_acpi_probe() besides arm_smmu_device_dt_probe().
> >
> > arm_smmu_device_dt_probe() could get the clocks from dt and fill in
> > the clock bulk data, and
> > similarly, arm_smmu_device_acpi_probe() could fill the clock bulk data
> > by getting it from ACPI.
> >
> > clk_bulk_get_all() seems like going only the OF way.
> > Is there another way here to have something common between ACPI
> > and OF, and then do the clk_bulk_get?
>
> I'd say just go with clk_bulk_get_all() and if somebody really wants to
> mess with the SMMU clocks on a system booted via ACPI, then it's their
> problem to solve. My understanding is that the design of IORT makes this
> next to impossible to solve anyway, because a static table is used and
> therefore we're unable to run whatever ASL methods need to be invoked to
> mess with the clocks.
Sure then. I will respin this patch-series.
>
> Will
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops
On Sat, Dec 1, 2018 at 8:54 AM Rob Clark wrote:
>
> This solves a problem we see with drm/msm, caused by getting
> iommu_dma_ops while we attach our own domain and manage it directly at
> the iommu API level:
>
> [0038] user address but active_mm is swapper
> Internal error: Oops: 9605 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: GW 4.19.3 #90
> Hardware name: xxx (DT)
> Workqueue: events deferred_probe_work_func
> pstate: 80c9 (Nzcv daif +PAN +UAO)
> pc : iommu_dma_map_sg+0x7c/0x2c8
> lr : iommu_dma_map_sg+0x40/0x2c8
> sp : ff80095eb4f0
> x29: ff80095eb4f0 x28:
> x27: ffc0f9431578 x26:
> x25: x24: 0003
> x23: 0001 x22: ffc0fa9ac010
> x21: x20: ffc0fab40980
> x19: ffc0fab40980 x18: 0003
> x17: 01c4 x16: 0007
> x15: 000e x14:
> x13: x12: 0028
> x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> x9 : x8 : ffc0fab409a0
> x7 : x6 : 0002
> x5 : 0001 x4 :
> x3 : 0001 x2 : 0002
> x1 : ffc0f9431578 x0 :
> Process kworker/7:1 (pid: 70, stack limit = 0x17d08ffb)
> Call trace:
>iommu_dma_map_sg+0x7c/0x2c8
>__iommu_map_sg_attrs+0x70/0x84
>get_pages+0x170/0x1e8
>msm_gem_get_iova+0x8c/0x128
>_msm_gem_kernel_new+0x6c/0xc8
>msm_gem_kernel_new+0x4c/0x58
>dsi_tx_buf_alloc_6g+0x4c/0x8c
>msm_dsi_host_modeset_init+0xc8/0x108
>msm_dsi_modeset_init+0x54/0x18c
>_dpu_kms_drm_obj_init+0x430/0x474
>dpu_kms_hw_init+0x5f8/0x6b4
>msm_drm_bind+0x360/0x6c8
>try_to_bring_up_master.part.7+0x28/0x70
>component_master_add_with_match+0xe8/0x124
>msm_pdev_probe+0x294/0x2b4
>platform_drv_probe+0x58/0xa4
>really_probe+0x150/0x294
>driver_probe_device+0xac/0xe8
>__device_attach_driver+0xa4/0xb4
>bus_for_each_drv+0x98/0xc8
>__device_attach+0xac/0x12c
>device_initial_probe+0x24/0x30
>bus_probe_device+0x38/0x98
>deferred_probe_work_func+0x78/0xa4
>process_one_work+0x24c/0x3dc
>worker_thread+0x280/0x360
>kthread+0x134/0x13c
>ret_from_fork+0x10/0x18
> Code: d284 91000725 6b17039f 5400048a (f9401f40)
> ---[ end trace f22dda57f3648e2c ]---
> Kernel panic - not syncing: Fatal exception
> SMP: stopping secondary CPUs
> Kernel Offset: disabled
> CPU features: 0x0,22802a18
> Memory Limit: none
>
> The problem is that when drm/msm does it's own iommu_attach_device(),
> now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> domain, and it doesn't have domain->iova_cookie.
>
> We kind of avoided this problem prior to sdm845/dpu because the iommu
> was attached to the mdp node in dt, which is a child of the toplevel
> mdss node (which corresponds to the dev passed in dma_map_sg()). But
> with sdm845, now the iommu is attached at the mdss level so we hit the
> iommu_dma_ops in dma_map_sg().
>
> But auto allocating/attaching a domain before the driver is probed was
> already a blocking problem for enabling per-context pagetables for the
> GPU. This problem is also now solved with this patch.
>
> Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in
> of_dma_configure
> Tested-by: Douglas Anderson
> Signed-off-by: Rob Clark
> ---
> This is an alternative/replacement for [1]. What it lacks in elegance
> it makes up for in practicality ;-)
>
> [1] https://patchwork.freedesktop.org/patch/264930/
>
> drivers/of/device.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 5957cd4fa262..15ffee00fb22 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> return device_add(&ofdev->dev);
> }
>
> +static const struct of_device_id iommu_blacklist[] = {
> + { .compatible = "qcom,mdp4" },
> + { .compatible = "qcom,mdss" },
> + { .compatible = "qcom,sdm845-mdss" },
> + { .compatible = "qcom,adreno" },
> + {}
> +};
> +
> /**
> * of_dma_configure - Setup DMA configuration
> * @dev: Device to apply DMA configuration
> @@ -164,6 +172,20 @@ int of_dma_configure(struct device *dev, struct
> device_node *np, bool force_dma)
> dev_dbg(dev, "device is%sbehind an iommu\n",
> iommu ? " " : " not ");
>
> + /*
> +* There is at least one case where the driver wants to directly
> +* manage the IOMMU, but if we end up with iommu dma_ops, that
> +* interferes with the drivers ability to use dma_map_sg() for
> +* cache operations. Since we don't currently have a better
> +* solution, and this code runs befo
Re: [Freedreno] [PATCH v3 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*
On Sat, Dec 1, 2018 at 3:47 AM Rob Clark wrote: > > On Fri, Nov 30, 2018 at 9:05 PM Tomasz Figa wrote: > > > > On Thu, Nov 29, 2018 at 4:23 PM Tomasz Figa wrote: > > > > > > On Thu, Nov 29, 2018 at 12:03 PM Robin Murphy > > > wrote: > > > > > > > > On 29/11/2018 19:57, Tomasz Figa wrote: > > > > > On Thu, Nov 29, 2018 at 11:40 AM Jordan Crouse > > > > > wrote: > > > > >> > > > > >> On Thu, Nov 29, 2018 at 01:48:15PM -0500, Rob Clark wrote: > > > > >>> On Thu, Nov 29, 2018 at 10:54 AM Christoph Hellwig > > > > >>> wrote: > > > > > > > > On Thu, Nov 29, 2018 at 09:42:50AM -0500, Rob Clark wrote: > > > > > Maybe the thing we need to do is just implement a blacklist of > > > > > compatible strings for devices which should skip the automatic > > > > > iommu/dma hookup. Maybe a bit ugly, but it would also solve a > > > > > problem > > > > > preventing us from enabling per-process pagetables for a5xx > > > > > (where we > > > > > need to control the domain/context-bank that is allocated by the > > > > > dma > > > > > api). > > > > > > > > You can detach from the dma map attachment using > > > > arm_iommu_detach_device, > > > > which a few drm drivers do, but I don't think this is the problem. > > > > >>> > > > > >>> I think even with detach, we wouldn't end up with the context-bank > > > > >>> that the gpu firmware was hard-coded to expect, and so it would > > > > >>> overwrite the incorrect page table address register. (I could be > > > > >>> mis-remembering that, Jordan spent more time looking at that. But > > > > >>> it > > > > >>> was something along those lines.) > > > > >> > > > > >> Right - basically the DMA domain steals context bank 0 and the GPU > > > > >> is hard coded > > > > >> to use that context bank for pagetable switching. > > > > >> > > > > >> I believe the Tegra guys also had a similar problem with a hard > > > > >> coded context > > > > >> bank. > > > > > > > > AIUI, they don't need a specific hardware context, they just need to > > > > know which one they're actually using, which the domain abstraction > > > > hides. > > > > > > > > > Wait, if we detach the GPU/display struct device from the default > > > > > domain and attach it to a newly allocated domain, wouldn't the newly > > > > > allocated domain use the context bank we need? Note that we're already > > > > > > > > The arm-smmu driver doesn't, but there's no fundamental reason it > > > > couldn't. That should just need code to refcount domain users and > > > > release hardware contexts for domains with no devices currently > > > > attached. > > > > > > > > Robin. > > > > > > > > > doing that, except that we're doing it behind the back of the DMA > > > > > mapping subsystem, so that it keeps using the IOMMU version of the DMA > > > > > ops for the device and doing any mapping operations on the default > > > > > domain. If we ask the DMA mapping to detach, wouldn't it essentially > > > > > solve the problem? > > > > > > Thanks Robin. > > > > > > Still, my point is that the MSM DRM driver attaches the GPU struct > > > device to a new domain it allocates using iommu_domain_alloc() and it > > > seems to work fine, so I believe it's not the problem we're looking > > > into with this patch. > > > > Could we just make the MSM DRM call arch_teardown_dma_ops() and then > > arch_setup_dma_ops() with the `iommu` argument set to NULL and be done > > with it? > > I don't think those are exported to modules? > Indeed, if we compile MSM DRM as modules, it wouldn't work... > I have actually a simpler patch, that adds a small blacklist to check > in of_dma_configure() before calling arch_setup_dma_ops(), which can > replace this patch. It also solves the problem of dma api allocating > the context bank that he gpu wants to use for context-switching, and > should be a simple thing to backport to stable branches. > > I was just spending some time trying to figure out what changed > recently to start causing dma_map_sg() to opps on boot for us, so I > could write a more detailed commit msg. Yeah, that sounds much better, thanks. Reviewed that patch. Best regards, Tomasz ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 5/9] drm/msm: add headless gpu device (for imx5)
On Wed, Nov 21, 2018 at 8:55 PM Jonathan Marek wrote:
>
> This patch allows using drm/msm without qcom display hardware. This is
> especially useful for iMX5 hardware, which has a a2xx GPU but uses the
> imx-drm driver for display.
>
> Signed-off-by: Jonathan Marek
> ---
> v2: added commit message and removed unnecessary comment
>
> drivers/gpu/drm/msm/Kconfig | 4 ++--
> drivers/gpu/drm/msm/msm_debugfs.c | 2 +-
> drivers/gpu/drm/msm/msm_drv.c | 21 +++--
> 3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index 843a9d40c05e..cf549f1ed403 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -2,7 +2,7 @@
> config DRM_MSM
> tristate "MSM DRM"
> depends on DRM
> - depends on ARCH_QCOM || (ARM && COMPILE_TEST)
> + depends on ARCH_QCOM || SOC_IMX5 || (ARM && COMPILE_TEST)
> depends on OF && COMMON_CLK
> depends on MMU
> select QCOM_MDT_LOADER if ARCH_QCOM
> @@ -11,7 +11,7 @@ config DRM_MSM
> select DRM_PANEL
> select SHMEM
> select TMPFS
> - select QCOM_SCM
> + select QCOM_SCM if ARCH_QCOM
> select WANT_DEV_COREDUMP
> select SND_SOC_HDMI_CODEC if SND_SOC
> select SYNC_FILE
> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c
> b/drivers/gpu/drm/msm/msm_debugfs.c
> index f0da0d3c8a80..1ca99ca356a4 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -235,7 +235,7 @@ int msm_debugfs_init(struct drm_minor *minor)
> debugfs_create_file("gpu", S_IRUSR, minor->debugfs_root,
> dev, &msm_gpu_fops);
>
> - if (priv->kms->funcs->debugfs_init) {
> + if (priv->kms && priv->kms->funcs->debugfs_init) {
> ret = priv->kms->funcs->debugfs_init(priv->kms, minor);
> if (ret)
> return ret;
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 9f219e02f3c7..3f35e57202ef 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -344,6 +344,7 @@ static int msm_drm_uninit(struct device *dev)
> return 0;
> }
>
> +#define KMS_HEADLESS 1
> #define KMS_MDP4 4
> #define KMS_MDP5 5
> #define KMS_DPU 3
> @@ -495,6 +496,9 @@ static int msm_drm_init(struct device *dev, struct
> drm_driver *drv)
> msm_gem_shrinker_init(ddev);
>
> switch (get_mdp_ver(pdev)) {
> + case KMS_HEADLESS:
> + priv->kms = kms = NULL;
> + break;
> case KMS_MDP4:
> kms = mdp4_kms_init(ddev);
> priv->kms = kms;
> @@ -512,12 +516,6 @@ static int msm_drm_init(struct device *dev, struct
> drm_driver *drv)
> }
>
> if (IS_ERR(kms)) {
> - /*
> -* NOTE: once we have GPU support, having no kms should not
> -* be considered fatal.. ideally we would still support gpu
> -* and (for example) use dmabuf/prime to share buffers with
> -* imx drm driver on iMX5
> -*/
> dev_err(dev, "failed to load kms\n");
> ret = PTR_ERR(kms);
> goto err_msm_uninit;
> @@ -633,7 +631,7 @@ static int msm_drm_init(struct device *dev, struct
> drm_driver *drv)
> drm_mode_config_reset(ddev);
>
> #ifdef CONFIG_DRM_FBDEV_EMULATION
> - if (fbdev)
> + if (kms && fbdev)
> priv->fbdev = msm_fbdev_init(ddev);
> #endif
>
> @@ -1315,9 +1313,11 @@ static int msm_pdev_probe(struct platform_device *pdev)
> struct component_match *match = NULL;
> int ret;
>
> - ret = add_display_components(&pdev->dev, &match);
> - if (ret)
> - return ret;
> + if (get_mdp_ver(pdev) != KMS_HEADLESS) {
> + ret = add_display_components(&pdev->dev, &match);
> + if (ret)
> + return ret;
> + }
>
> ret = add_gpu_components(&pdev->dev, &match);
> if (ret)
> @@ -1342,6 +1342,7 @@ static int msm_pdev_remove(struct platform_device *pdev)
> }
>
> static const struct of_device_id dt_match[] = {
> + { .compatible = "qcom,adreno-headless", .data = (void *)KMS_HEADLESS
> },
so, I don't really think we should have a "headless" dt node, because
that doesn't really represent the hw. Maybe instead for the gpu
device node, we could use a different compatible string like:
gpu@xyz {
compatible = "amd,imageon-205.0", "amd,imageon";
...
};
(ie. amd,imageon instead of qcom,adreno)
Then in adreno_bind() (or somewhere around there), if we have the
amd,imageon compat string instead of qcom,adreno, then programatically
create a dummy "headless" device that the toplevel drm driver can bind
to. I think something like this should be possible, and avoid needing
a qcom,adreno-headless node
Re: [Freedreno] [PATCH v2 3/9] drm/msm/mdp4: add lcdc-align-lsb flag to control lane alignment
On Wed, Nov 21, 2018 at 8:55 PM Jonathan Marek wrote: > > Controls which of the 8 lanes are used for 6 bit color. > > Signed-off-by: Jonathan Marek > --- > .../gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 22 --- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > index e19ab2ab63f7..7d8d11c8150a 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > @@ -377,20 +377,26 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder > *encoder) > unsigned long pc = mdp4_lcdc_encoder->pixclock; > struct mdp4_kms *mdp4_kms = get_kms(encoder); > struct drm_panel *panel; > + uint32_t config; > int i, ret; > > if (WARN_ON(mdp4_lcdc_encoder->enabled)) > return; > > /* TODO: hard-coded for 18bpp: */ > - mdp4_crtc_set_config(encoder->crtc, > - MDP4_DMA_CONFIG_R_BPC(BPC6) | > - MDP4_DMA_CONFIG_G_BPC(BPC6) | > - MDP4_DMA_CONFIG_B_BPC(BPC6) | > - MDP4_DMA_CONFIG_PACK_ALIGN_MSB | > - MDP4_DMA_CONFIG_PACK(0x21) | > - MDP4_DMA_CONFIG_DEFLKR_EN | > - MDP4_DMA_CONFIG_DITHER_EN); > + config = > + MDP4_DMA_CONFIG_R_BPC(BPC6) | > + MDP4_DMA_CONFIG_G_BPC(BPC6) | > + MDP4_DMA_CONFIG_B_BPC(BPC6) | > + MDP4_DMA_CONFIG_PACK(0x21) | > + MDP4_DMA_CONFIG_DEFLKR_EN | > + MDP4_DMA_CONFIG_DITHER_EN; > + > + if (!of_find_property(dev->dev->of_node, "lcdc-align-lsb", NULL)) > + config |= MDP4_DMA_CONFIG_PACK_ALIGN_MSB; > + > + looks reasonable, except extra newline (minor nit) and more importantly the new dt property needs to be documented (and cc'd to devicetree list) BR, -R > + mdp4_crtc_set_config(encoder->crtc, config); > mdp4_crtc_set_intf(encoder->crtc, INTF_LCDC_DTV, 0); > > bs_set(mdp4_lcdc_encoder, 1); > -- > 2.17.1 > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
