Re: [PATCH] gpu: host1x: Set up device DMA parameters
On Mon, Sep 16, 2024 at 03:33:20PM GMT, Thierry Reding wrote: > From: Thierry Reding > > In order to store device DMA parameters, the DMA framework depends on > the device's dma_parms field to point at a valid memory location. Add > backing storage for this in struct host1x_memory_context and point to > it. > > Reported-by: Jonathan Hunter > Signed-off-by: Thierry Reding > --- > drivers/gpu/host1x/context.c | 1 + > include/linux/host1x.h | 1 + > 2 files changed, 2 insertions(+) Applied. Thierry signature.asc Description: PGP signature
Re: [PATCH] gpu: host1x: Fix boot regression for Tegra
On Wed, Sep 25, 2024 at 05:05:04PM GMT, Jon Hunter wrote:
> Commit 4c27ac45e622 ("gpu: host1x: Request syncpoint IRQs only during
> probe") caused a boot regression for the Tegra186 device. Following this
> update the function host1x_intr_init() now calls
> host1x_hw_intr_disable_all_syncpt_intrs() during probe. However,
> host1x_intr_init() is called before runtime power-management is enabled
> for Host1x and the function host1x_hw_intr_disable_all_syncpt_intrs() is
> accessing hardware registers. So if the Host1x hardware is not enabled
> prior to probing then the device will now hang on attempting to access
> the registers. So far this is only observed on Tegra186, but potentially
> could be seen on other devices.
>
> Fix this by moving the call to the function host1x_intr_init() in probe
> to after enabling the runtime power-management in the probe and update
> the failure path in probe as necessary.
>
> Fixes: 4c27ac45e622 ("gpu: host1x: Request syncpoint IRQs only during probe")
> Signed-off-by: Jon Hunter
> ---
> drivers/gpu/host1x/dev.c | 18 --
> 1 file changed, 8 insertions(+), 10 deletions(-)
Applied, thanks.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH -next 2/3] drm/tegra: Enable module autoloading
On Mon, Sep 23, 2024 at 07:21:21AM GMT, Dmitry Baryshkov wrote:
> On Mon, Sep 02, 2024 at 11:33:19AM GMT, Liao Chen wrote:
> > Add MODULE_DEVICE_TABLE(), so modules could be properly autoloaded based
> > on the alias from of_device_id table.
> >
> > Signed-off-by: Liao Chen
> > ---
> > drivers/gpu/drm/tegra/drm.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index 03d1c76aec2d..1a12f2eaad86 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -1390,6 +1390,7 @@ static const struct of_device_id host1x_drm_subdevs[]
> > = {
> > { .compatible = "nvidia,tegra234-nvdec", },
> > { /* sentinel */ }
> > };
> > +MODULE_DEVICE_TABLE(of, host1x_drm_subdevs);
>
> I don't think it is correct. See how subdevs are handled in
> host1x_device_parse_dt(). I'll pick up two other patches though.
Agreed. This shouldn't be needed since all of these compatible strings
should show up in the OF device ID table of their corresponding drivers,
which is where the export should happen.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] gpu: host1x: Request syncpoint IRQs only during probe
On Tue, Sep 24, 2024 at 07:33:05PM GMT, Jon Hunter wrote:
>
> On 06/09/2024 09:38, Jon Hunter wrote:
> > Hi Mikko,
> >
> > On 31/05/2024 08:07, Mikko Perttunen wrote:
> > > From: Mikko Perttunen
> > >
> > > Syncpoint IRQs are currently requested in a code path that runs
> > > during resume. Due to this, we get multiple overlapping registered
> > > interrupt handlers as host1x is suspended and resumed.
> > >
> > > Rearrange interrupt code to only request IRQs during initialization.
> > >
> > > Signed-off-by: Mikko Perttunen
>
> ...
>
> > This change is causing a boot regression on Tegra186 with the latest
> > -next. I have reverted this to confirm that this fixes the problem. I
> > don't see any crash log but the board appears to just hang.
>
>
> I had a look at this and I was able to fix this by moving where
> we initialise the interrupts to after the PM runtime enable ...
>
> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> index b62e4f0e8130..ff98d4903cac 100644
> --- a/drivers/gpu/host1x/dev.c
> +++ b/drivers/gpu/host1x/dev.c
> @@ -625,12 +625,6 @@ static int host1x_probe(struct platform_device *pdev)
> goto free_contexts;
> }
> - err = host1x_intr_init(host);
> - if (err) {
> - dev_err(&pdev->dev, "failed to initialize interrupts\n");
> - goto deinit_syncpt;
> - }
> -
> pm_runtime_enable(&pdev->dev);
> err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
> @@ -642,6 +636,12 @@ static int host1x_probe(struct platform_device *pdev)
> if (err)
> goto pm_disable;
> + err = host1x_intr_init(host);
> + if (err) {
> + dev_err(&pdev->dev, "failed to initialize interrupts\n");
> + goto pm_put;
> + }
> +
I think the reason why this might fail now is because host1x_intr_init()
ends up writing some registers during the call to the
host1x_hw_intr_disable_all_syncpt_intrs() function, which would hang or
crash if the device isn't on (which in turn happens during
pm_runtime_resume_and_get()).
Not sure exactly why this used to work because prior to Mikko's patch
because the sequence was the same before.
> host1x_debug_init(host);
> err = host1x_register(host);
> @@ -658,14 +658,11 @@ static int host1x_probe(struct platform_device *pdev)
> host1x_unregister(host);
> deinit_debugfs:
> host1x_debug_deinit(host);
> -
> + host1x_intr_deinit(host);
> +pm_put:
> pm_runtime_put_sync_suspend(&pdev->dev);
> pm_disable:
> pm_runtime_disable(&pdev->dev);
> -
> - host1x_intr_deinit(host);
> -deinit_syncpt:
> - host1x_syncpt_deinit(host);
> free_contexts:
> host1x_memory_context_list_free(&host->context_list);
> free_channels:
>
>
> Thierry, do you want to me to send a fix for the above or do you
> want to squash with the original (assuming that OK with Mikko)?
In any case, this looks like a good fix, so please send a proper patch.
This is merged through drm-misc, so squashing this into the original
patch is not an option any longer.
Thanks,
Thierry
signature.asc
Description: PGP signature
[PATCH] gpu: host1x: Set up device DMA parameters
From: Thierry Reding
In order to store device DMA parameters, the DMA framework depends on
the device's dma_parms field to point at a valid memory location. Add
backing storage for this in struct host1x_memory_context and point to
it.
Reported-by: Jonathan Hunter
Signed-off-by: Thierry Reding
---
drivers/gpu/host1x/context.c | 1 +
include/linux/host1x.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/host1x/context.c b/drivers/gpu/host1x/context.c
index 955c971c528d..a6f6779662a3 100644
--- a/drivers/gpu/host1x/context.c
+++ b/drivers/gpu/host1x/context.c
@@ -58,6 +58,7 @@ int host1x_memory_context_list_init(struct host1x *host1x)
ctx->dev.parent = host1x->dev;
ctx->dev.release = host1x_memory_context_release;
+ ctx->dev.dma_parms = &ctx->dma_parms;
dma_set_max_seg_size(&ctx->dev, UINT_MAX);
err = device_add(&ctx->dev);
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 9c8119ed13a4..c4dde3aafcac 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -466,6 +466,7 @@ struct host1x_memory_context {
refcount_t ref;
struct pid *owner;
+ struct device_dma_parameters dma_parms;
struct device dev;
u64 dma_mask;
u32 stream_id;
--
2.45.2
Re: [PATCH v2 1/3] drm/nouveau/tegra: Use iommu_paging_domain_alloc()
On Mon, Sep 02, 2024 at 09:46:58AM GMT, Lu Baolu wrote: > In nvkm_device_tegra_probe_iommu(), a paging domain is allocated for @dev > and attached to it on success. Use iommu_paging_domain_alloc() to make it > explicit. > > Signed-off-by: Lu Baolu > --- > drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH v2 3/3] drm/tegra: Use iommu_paging_domain_alloc()
On Mon, Sep 02, 2024 at 09:47:00AM GMT, Lu Baolu wrote:
> Commit <17de3f5fdd35> ("iommu: Retire bus ops") removes iommu ops from
> the bus structure. The iommu subsystem no longer relies on bus for
> operations. So iommu_domain_alloc() interface is no longer relevant.
>
> Replace iommu_domain_alloc() with iommu_paging_domain_alloc() which takes
> the physical device from which the host1x_device virtual device was
> instantiated. This physical device is a common parent to all physical
> devices that are part of the virtual device.
>
> Suggested-by: Thierry Reding
> Signed-off-by: Lu Baolu
> ---
> drivers/gpu/drm/tegra/drm.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH] drm/tegra: fix potential uninitialized variable use
On Mon, Sep 02, 2024 at 07:13:17PM GMT, Jani Nikula wrote:
> It's likely either output->drm_edid or output->ddc is non-NULL, but
> avoid the uninitialized variable usage anyway.
>
> Reported-by: kernel test robot
> Closes: https://lore.kernel.org/r/ZtXLyXxew7z6H2bD@stanley.mountain
> Fixes: 98365ca74cbf ("drm/tegra: convert to struct drm_edid")
> Cc: Thierry Reding
> Cc: Daniel Vetter
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Jani Nikula
> ---
> drivers/gpu/drm/tegra/output.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Do you want to apply this or should I? In the former case:
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH 5/5] gpu: host1x: fence: Disable timeout on pre-silicon
On Thu, Apr 25, 2024 at 08:02:37AM GMT, Mikko Perttunen wrote: > From: Mikko Perttunen > > Timing can be wonky on pre-silicon platforms, so disable fence timeouts > on pre-silicon platforms. > > Signed-off-by: Mikko Perttunen > --- > drivers/gpu/host1x/fence.c | 9 + > 1 file changed, 9 insertions(+) This fails to build if host1x is built as a module, so I've only applied the first four patches for now. We'd first need a patch to export the tegra_is_silicon() helper. Thierry signature.asc Description: PGP signature
Re: [PATCH][next] drm/tegra: hdmi: make read-only const array possible_nvram_sizes static
On Thu, Aug 22, 2024 at 09:50:47PM GMT, Colin Ian King wrote:
> Don't populate the const read-only array possible_nvram_sizes on the
I've changed this (and the occurrence in the subject) to reflect the
actual array name ("freqs") that's being changed here.
Applied, thanks.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] gpu: host1x: Make host1x_context_device_bus_type constant
On Fri, Aug 23, 2024 at 04:07:24PM GMT, Kunwu Chan wrote:
> From: Kunwu Chan
>
> Since commit d492cc2573a0 ("driver core: device.h: make struct
> bus_type a const *"), the driver core can properly handle constant
> struct bus_type, move the host1x_context_device_bus_type variable
> to be a constant structure as well, placing it into read-only memory
> which can not be modified at runtime.
>
> Cc: Greg Kroah-Hartman
> Suggested-by: Greg Kroah-Hartman
> Signed-off-by: Kunwu Chan
> ---
> drivers/gpu/host1x/context_bus.c | 2 +-
> include/linux/host1x_context_bus.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Applied, thanks.
Thierry
signature.asc
Description: PGP signature
[PATCH] Revert "drm/tegra: gr3d: Convert into dev_pm_domain_attach|detach_list()"
From: Thierry Reding
This reverts commit f790b5c09665cab0d51dfcc84832d79d2b1e6c0e. An updated
version of patch was applied to the PM tree. Sorry for the mixup.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/tegra/gr3d.c | 46 ++--
1 file changed, 33 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 4de1ea0fc7c0..00c8564520e7 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -46,7 +46,6 @@ struct gr3d {
unsigned int nclocks;
struct reset_control_bulk_data resets[RST_GR3D_MAX];
unsigned int nresets;
- struct dev_pm_domain_list *pd_list;
DECLARE_BITMAP(addr_regs, GR3D_NUM_REGS);
};
@@ -370,12 +369,18 @@ static int gr3d_power_up_legacy_domain(struct device
*dev, const char *name,
return 0;
}
+static void gr3d_del_link(void *link)
+{
+ device_link_del(link);
+}
+
static int gr3d_init_power(struct device *dev, struct gr3d *gr3d)
{
- struct dev_pm_domain_attach_data pd_data = {
- .pd_names = (const char *[]) { "3d0", "3d1" },
- .num_pd_names = 2,
- };
+ static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };
+ const u32 link_flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME;
+ struct device **opp_virt_devs, *pd_dev;
+ struct device_link *link;
+ unsigned int i;
int err;
err = of_count_phandle_with_args(dev->of_node, "power-domains",
@@ -409,10 +414,29 @@ static int gr3d_init_power(struct device *dev, struct
gr3d *gr3d)
if (dev->pm_domain)
return 0;
- err = dev_pm_domain_attach_list(dev, &pd_data, &gr3d->pd_list);
- if (err < 0)
+ err = devm_pm_opp_attach_genpd(dev, opp_genpd_names, &opp_virt_devs);
+ if (err)
return err;
+ for (i = 0; opp_genpd_names[i]; i++) {
+ pd_dev = opp_virt_devs[i];
+ if (!pd_dev) {
+ dev_err(dev, "failed to get %s power domain\n",
+ opp_genpd_names[i]);
+ return -EINVAL;
+ }
+
+ link = device_link_add(dev, pd_dev, link_flags);
+ if (!link) {
+ dev_err(dev, "failed to link to %s\n",
dev_name(pd_dev));
+ return -EINVAL;
+ }
+
+ err = devm_add_action_or_reset(dev, gr3d_del_link, link);
+ if (err)
+ return err;
+ }
+
return 0;
}
@@ -503,13 +527,13 @@ static int gr3d_probe(struct platform_device *pdev)
err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
if (err)
- goto err;
+ return err;
err = host1x_client_register(&gr3d->client.base);
if (err < 0) {
dev_err(&pdev->dev, "failed to register host1x client: %d\n",
err);
- goto err;
+ return err;
}
/* initialize address register map */
@@ -517,9 +541,6 @@ static int gr3d_probe(struct platform_device *pdev)
set_bit(gr3d_addr_regs[i], gr3d->addr_regs);
return 0;
-err:
- dev_pm_domain_detach_list(gr3d->pd_list);
- return err;
}
static void gr3d_remove(struct platform_device *pdev)
@@ -528,7 +549,6 @@ static void gr3d_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
host1x_client_unregister(&gr3d->client.base);
- dev_pm_domain_detach_list(gr3d->pd_list);
}
static int __maybe_unused gr3d_runtime_suspend(struct device *dev)
--
2.45.2
Re: [PATCH 4/6] drm/tegra: convert to struct drm_edid
On Thu, Aug 22, 2024 at 08:42:50PM GMT, Jani Nikula wrote:
> Prefer the struct drm_edid based functions for reading the EDID and
> updating the connector.
>
> Signed-off-by: Jani Nikula
>
> ---
>
> Cc: Thierry Reding
> Cc: Mikko Perttunen
> Cc: Jonathan Hunter
> Cc: linux-te...@vger.kernel.org
> ---
> drivers/gpu/drm/tegra/drm.h| 2 +-
> drivers/gpu/drm/tegra/output.c | 29 +
> 2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 682011166a8f..2f3781e04b0a 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -133,7 +133,7 @@ struct tegra_output {
> struct drm_bridge *bridge;
> struct drm_panel *panel;
> struct i2c_adapter *ddc;
> - const struct edid *edid;
> + const struct drm_edid *drm_edid;
It perhaps might've been less confusing if this wasn't gratuitously
renamed, but either way (and assuming you want to take this through
drm-misc):
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH 3/3] drm/tegra: Remove call to iommu_domain_alloc()
On Mon, Aug 12, 2024 at 03:10:34PM GMT, Lu Baolu wrote:
> Commit <17de3f5fdd35> ("iommu: Retire bus ops") removes iommu ops from
> the bus structure. The iommu subsystem no longer relies on bus for
> operations. So iommu_domain_alloc() interface is no longer relevant.
>
> Normally, iommu_paging_domain_alloc() could be a replacement for
> iommu_domain_alloc() if the caller has the right device for IOMMU API
> use. Unfortunately, this is not the case for this driver.
>
> Iterate the devices on the platform bus and find a suitable device
> whose device DMA is translated by an IOMMU. Then use this device to
> allocate an iommu domain. The iommu subsystem prevents domains
> allocated by one iommu driver from being attached to devices managed
> by any different iommu driver.
>
> Signed-off-by: Lu Baolu
> Link:
> https://lore.kernel.org/r/2024061008.88197-20-baolu...@linux.intel.com
> ---
> drivers/gpu/drm/tegra/drm.c | 34 +-
> 1 file changed, 25 insertions(+), 9 deletions(-)
Actually I think we can just do something like this:
--- >8 ---
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index d9f0728c3afd..d35e411d536b 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1150,7 +1150,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
}
if (host1x_drm_wants_iommu(dev) && iommu_present(&platform_bus_type)) {
- tegra->domain = iommu_domain_alloc(&platform_bus_type);
+ tegra->domain = iommu_paging_domain_alloc(dev->dev.parent);
if (!tegra->domain) {
err = -ENOMEM;
goto free;
--- >8 ---
That refers to the physical device that the host1x_device virtual device
was instantiated from and is a common parent to all physical devices
that are part of the virtual device.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 1/1] gpu: host1x: Use iommu_paging_domain_alloc()
On Mon, Aug 12, 2024 at 03:16:05PM GMT, Lu Baolu wrote: > An iommu domain is allocated in host1x_iommu_attach() and is attached to > host->dev. Use iommu_paging_domain_alloc() to make it explicit. > > Signed-off-by: Lu Baolu > Reviewed-by: Jason Gunthorpe > Link: > https://lore.kernel.org/r/2024061008.88197-8-baolu...@linux.intel.com > --- > drivers/gpu/host1x/dev.c | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH] drm/tegra: hub: Use fn parameter directly to fix Coccinelle warning
On Wed, Jul 10, 2024 at 11:00:35PM GMT, Thorsten Blum wrote: > The function parameter out can be used directly instead of assigning it > to a temporary u64 variable first. > > Remove the local variable tmp2 and use the parameter out directly as the > divisor in do_div() to remove the following Coccinelle/coccicheck > warning reported by do_div.cocci: > > WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 > instead > > Signed-off-by: Thorsten Blum > --- > drivers/gpu/drm/tegra/hub.c | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH] gpu: host1x: Request syncpoint IRQs only during probe
On Fri, May 31, 2024 at 10:07:18AM GMT, Mikko Perttunen wrote: > From: Mikko Perttunen > > Syncpoint IRQs are currently requested in a code path that runs > during resume. Due to this, we get multiple overlapping registered > interrupt handlers as host1x is suspended and resumed. > > Rearrange interrupt code to only request IRQs during initialization. > > Signed-off-by: Mikko Perttunen > --- > drivers/gpu/host1x/dev.h| 2 ++ > drivers/gpu/host1x/hw/intr_hw.c | 37 +++-- > drivers/gpu/host1x/intr.c | 21 ++- > drivers/gpu/host1x/intr.h | 5 + > 4 files changed, 30 insertions(+), 35 deletions(-) Sorry, I only stumbled across this now. Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags
On Wed, Jul 10, 2024 at 02:10:09PM GMT, Maxime Ripard wrote: > On Fri, Jul 05, 2024 at 04:31:34PM GMT, Thierry Reding wrote: > > On Thu, Jul 04, 2024 at 02:24:49PM GMT, Maxime Ripard wrote: > > > On Fri, Jun 28, 2024 at 04:42:35PM GMT, Thierry Reding wrote: > > > > On Fri, Jun 28, 2024 at 03:08:46PM GMT, Maxime Ripard wrote: > > > > > Hi, > > > > > > > > > > On Fri, Jun 28, 2024 at 01:29:17PM GMT, Thierry Reding wrote: > > > > > > On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote: > > > > > > > On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote: > > > > > > > > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter > > > > > > > > wrote: > > > > > > > > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote: > > > > > > > > > > But it makes me a little nervous to add a new generic > > > > > > > > > > allocation flag > > > > > > > > > > for a feature most hardware doesn't support (yet, at > > > > > > > > > > least). So it's > > > > > > > > > > hard to weigh how common the actual usage will be across > > > > > > > > > > all the > > > > > > > > > > heaps. > > > > > > > > > > > > > > > > > > > > I apologize as my worry is mostly born out of seeing > > > > > > > > > > vendors really > > > > > > > > > > push opaque feature flags in their old ion heaps, so in > > > > > > > > > > providing a > > > > > > > > > > flags argument, it was mostly intended as an escape hatch > > > > > > > > > > for > > > > > > > > > > obviously common attributes. So having the first be > > > > > > > > > > something that > > > > > > > > > > seems reasonable, but isn't actually that common makes me > > > > > > > > > > fret some. > > > > > > > > > > > > > > > > > > > > So again, not an objection, just something for folks to > > > > > > > > > > stew on to > > > > > > > > > > make sure this is really the right approach. > > > > > > > > > > > > > > > > > > Another good reason to go with full heap names instead of > > > > > > > > > opaque flags on > > > > > > > > > existing heaps is that with the former we can use symlinks in > > > > > > > > > sysfs to > > > > > > > > > specify heaps, with the latter we need a new idea. We haven't > > > > > > > > > yet gotten > > > > > > > > > around to implement this anywhere, but it's been in the > > > > > > > > > dma-buf/heap todo > > > > > > > > > since forever, and I like it as a design approach. So would > > > > > > > > > be a good idea > > > > > > > > > to not toss it. With that display would have symlinks to > > > > > > > > > cma-ecc and cma, > > > > > > > > > and rendering maybe cma-ecc, shmem, cma heaps (in priority > > > > > > > > > order) for a > > > > > > > > > SoC where the display needs contig memory for scanout. > > > > > > > > > > > > > > > > So indeed that is a good point to keep in mind, but I also > > > > > > > > think it > > > > > > > > might re-inforce the choice of having ECC as a flag here. > > > > > > > > > > > > > > > > Since my understanding of the sysfs symlinks to heaps idea is > > > > > > > > about > > > > > > > > being able to figure out a common heap from a collection of > > > > > > > > devices, > > > > > > > > it's really about the ability for the driver to access the type > > > > > > > > of > > > > > > > > memory. If ECC is just an attribute of the type of memory (as > > > > > > > > in this > > > > > > > > patch serie
Re: [Linaro-mm-sig] Re: [PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags
On Mon, Jul 08, 2024 at 09:14:14AM GMT, Christian König wrote: > Am 05.07.24 um 17:35 schrieb Daniel Vetter: > > Just figured I'll jump in on one detail here. > > > > On Fri, Jul 05, 2024 at 04:31:34PM +0200, Thierry Reding wrote: > > > On Thu, Jul 04, 2024 at 02:24:49PM GMT, Maxime Ripard wrote: > > > > On Fri, Jun 28, 2024 at 04:42:35PM GMT, Thierry Reding wrote: > > > > > On Fri, Jun 28, 2024 at 03:08:46PM GMT, Maxime Ripard wrote: > > > > > > Hi, > > > > > > > > > > > > On Fri, Jun 28, 2024 at 01:29:17PM GMT, Thierry Reding wrote: > > > > > > > On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote: > > > > > > > > On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote: > > > > > > > > > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter > > > > > > > > > wrote: > > > > > > > > > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote: > > > > > > > > > > > But it makes me a little nervous to add a new generic > > > > > > > > > > > allocation flag > > > > > > > > > > > for a feature most hardware doesn't support (yet, at > > > > > > > > > > > least). So it's > > > > > > > > > > > hard to weigh how common the actual usage will be across > > > > > > > > > > > all the > > > > > > > > > > > heaps. > > > > > > > > > > > > > > > > > > > > > > I apologize as my worry is mostly born out of seeing > > > > > > > > > > > vendors really > > > > > > > > > > > push opaque feature flags in their old ion heaps, so in > > > > > > > > > > > providing a > > > > > > > > > > > flags argument, it was mostly intended as an escape hatch > > > > > > > > > > > for > > > > > > > > > > > obviously common attributes. So having the first be > > > > > > > > > > > something that > > > > > > > > > > > seems reasonable, but isn't actually that common makes me > > > > > > > > > > > fret some. > > > > > > > > > > > > > > > > > > > > > > So again, not an objection, just something for folks to > > > > > > > > > > > stew on to > > > > > > > > > > > make sure this is really the right approach. > > > > > > > > > > Another good reason to go with full heap names instead of > > > > > > > > > > opaque flags on > > > > > > > > > > existing heaps is that with the former we can use symlinks > > > > > > > > > > in sysfs to > > > > > > > > > > specify heaps, with the latter we need a new idea. We > > > > > > > > > > haven't yet gotten > > > > > > > > > > around to implement this anywhere, but it's been in the > > > > > > > > > > dma-buf/heap todo > > > > > > > > > > since forever, and I like it as a design approach. So would > > > > > > > > > > be a good idea > > > > > > > > > > to not toss it. With that display would have symlinks to > > > > > > > > > > cma-ecc and cma, > > > > > > > > > > and rendering maybe cma-ecc, shmem, cma heaps (in priority > > > > > > > > > > order) for a > > > > > > > > > > SoC where the display needs contig memory for scanout. > > > > > > > > > So indeed that is a good point to keep in mind, but I also > > > > > > > > > think it > > > > > > > > > might re-inforce the choice of having ECC as a flag here. > > > > > > > > > > > > > > > > > > Since my understanding of the sysfs symlinks to heaps idea is > > > > > > > > > about > > > > > > > > > being able to figure out a common heap from a collection of > > > > > > > > > devices, > > > > > >
Re: [PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags
On Wed, Jul 10, 2024 at 02:10:09PM GMT, Maxime Ripard wrote: > On Fri, Jul 05, 2024 at 04:31:34PM GMT, Thierry Reding wrote: > > On Thu, Jul 04, 2024 at 02:24:49PM GMT, Maxime Ripard wrote: > > > On Fri, Jun 28, 2024 at 04:42:35PM GMT, Thierry Reding wrote: > > > > On Fri, Jun 28, 2024 at 03:08:46PM GMT, Maxime Ripard wrote: > > > > > Hi, > > > > > > > > > > On Fri, Jun 28, 2024 at 01:29:17PM GMT, Thierry Reding wrote: > > > > > > On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote: > > > > > > > On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote: > > > > > > > > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter > > > > > > > > wrote: > > > > > > > > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote: > > > > > > > > > > But it makes me a little nervous to add a new generic > > > > > > > > > > allocation flag > > > > > > > > > > for a feature most hardware doesn't support (yet, at > > > > > > > > > > least). So it's > > > > > > > > > > hard to weigh how common the actual usage will be across > > > > > > > > > > all the > > > > > > > > > > heaps. > > > > > > > > > > > > > > > > > > > > I apologize as my worry is mostly born out of seeing > > > > > > > > > > vendors really > > > > > > > > > > push opaque feature flags in their old ion heaps, so in > > > > > > > > > > providing a > > > > > > > > > > flags argument, it was mostly intended as an escape hatch > > > > > > > > > > for > > > > > > > > > > obviously common attributes. So having the first be > > > > > > > > > > something that > > > > > > > > > > seems reasonable, but isn't actually that common makes me > > > > > > > > > > fret some. > > > > > > > > > > > > > > > > > > > > So again, not an objection, just something for folks to > > > > > > > > > > stew on to > > > > > > > > > > make sure this is really the right approach. > > > > > > > > > > > > > > > > > > Another good reason to go with full heap names instead of > > > > > > > > > opaque flags on > > > > > > > > > existing heaps is that with the former we can use symlinks in > > > > > > > > > sysfs to > > > > > > > > > specify heaps, with the latter we need a new idea. We haven't > > > > > > > > > yet gotten > > > > > > > > > around to implement this anywhere, but it's been in the > > > > > > > > > dma-buf/heap todo > > > > > > > > > since forever, and I like it as a design approach. So would > > > > > > > > > be a good idea > > > > > > > > > to not toss it. With that display would have symlinks to > > > > > > > > > cma-ecc and cma, > > > > > > > > > and rendering maybe cma-ecc, shmem, cma heaps (in priority > > > > > > > > > order) for a > > > > > > > > > SoC where the display needs contig memory for scanout. > > > > > > > > > > > > > > > > So indeed that is a good point to keep in mind, but I also > > > > > > > > think it > > > > > > > > might re-inforce the choice of having ECC as a flag here. > > > > > > > > > > > > > > > > Since my understanding of the sysfs symlinks to heaps idea is > > > > > > > > about > > > > > > > > being able to figure out a common heap from a collection of > > > > > > > > devices, > > > > > > > > it's really about the ability for the driver to access the type > > > > > > > > of > > > > > > > > memory. If ECC is just an attribute of the type of memory (as > > > > > > > > in this > > > > > > > > patch serie
Re: [PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags
On Thu, Jul 04, 2024 at 02:24:49PM GMT, Maxime Ripard wrote: > On Fri, Jun 28, 2024 at 04:42:35PM GMT, Thierry Reding wrote: > > On Fri, Jun 28, 2024 at 03:08:46PM GMT, Maxime Ripard wrote: > > > Hi, > > > > > > On Fri, Jun 28, 2024 at 01:29:17PM GMT, Thierry Reding wrote: > > > > On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote: > > > > > On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote: > > > > > > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter > > > > > > wrote: > > > > > > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote: > > > > > > > > But it makes me a little nervous to add a new generic > > > > > > > > allocation flag > > > > > > > > for a feature most hardware doesn't support (yet, at least). So > > > > > > > > it's > > > > > > > > hard to weigh how common the actual usage will be across all the > > > > > > > > heaps. > > > > > > > > > > > > > > > > I apologize as my worry is mostly born out of seeing vendors > > > > > > > > really > > > > > > > > push opaque feature flags in their old ion heaps, so in > > > > > > > > providing a > > > > > > > > flags argument, it was mostly intended as an escape hatch for > > > > > > > > obviously common attributes. So having the first be something > > > > > > > > that > > > > > > > > seems reasonable, but isn't actually that common makes me fret > > > > > > > > some. > > > > > > > > > > > > > > > > So again, not an objection, just something for folks to stew on > > > > > > > > to > > > > > > > > make sure this is really the right approach. > > > > > > > > > > > > > > Another good reason to go with full heap names instead of opaque > > > > > > > flags on > > > > > > > existing heaps is that with the former we can use symlinks in > > > > > > > sysfs to > > > > > > > specify heaps, with the latter we need a new idea. We haven't yet > > > > > > > gotten > > > > > > > around to implement this anywhere, but it's been in the > > > > > > > dma-buf/heap todo > > > > > > > since forever, and I like it as a design approach. So would be a > > > > > > > good idea > > > > > > > to not toss it. With that display would have symlinks to cma-ecc > > > > > > > and cma, > > > > > > > and rendering maybe cma-ecc, shmem, cma heaps (in priority order) > > > > > > > for a > > > > > > > SoC where the display needs contig memory for scanout. > > > > > > > > > > > > So indeed that is a good point to keep in mind, but I also think it > > > > > > might re-inforce the choice of having ECC as a flag here. > > > > > > > > > > > > Since my understanding of the sysfs symlinks to heaps idea is about > > > > > > being able to figure out a common heap from a collection of devices, > > > > > > it's really about the ability for the driver to access the type of > > > > > > memory. If ECC is just an attribute of the type of memory (as in > > > > > > this > > > > > > patch series), it being on or off won't necessarily affect > > > > > > compatibility of the buffer with the device. Similarly "uncached" > > > > > > seems more of an attribute of memory type and not a type itself. > > > > > > Hardware that can access non-contiguous "system" buffers can access > > > > > > uncached system buffers. > > > > > > > > > > Yeah, but in graphics there's a wide band where "shit performance" is > > > > > defacto "not useable (as intended at least)". > > > > > > > > > > So if we limit the symlink idea to just making sure zero-copy access > > > > > is > > > > > possible, then we might not actually solve the real world problem we > > > > > need > > > > > to solve. And so the symlin
Re: [PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags
On Fri, Jun 28, 2024 at 03:08:46PM GMT, Maxime Ripard wrote: > Hi, > > On Fri, Jun 28, 2024 at 01:29:17PM GMT, Thierry Reding wrote: > > On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote: > > > On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote: > > > > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter wrote: > > > > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote: > > > > > > But it makes me a little nervous to add a new generic allocation > > > > > > flag > > > > > > for a feature most hardware doesn't support (yet, at least). So it's > > > > > > hard to weigh how common the actual usage will be across all the > > > > > > heaps. > > > > > > > > > > > > I apologize as my worry is mostly born out of seeing vendors really > > > > > > push opaque feature flags in their old ion heaps, so in providing a > > > > > > flags argument, it was mostly intended as an escape hatch for > > > > > > obviously common attributes. So having the first be something that > > > > > > seems reasonable, but isn't actually that common makes me fret some. > > > > > > > > > > > > So again, not an objection, just something for folks to stew on to > > > > > > make sure this is really the right approach. > > > > > > > > > > Another good reason to go with full heap names instead of opaque > > > > > flags on > > > > > existing heaps is that with the former we can use symlinks in sysfs to > > > > > specify heaps, with the latter we need a new idea. We haven't yet > > > > > gotten > > > > > around to implement this anywhere, but it's been in the dma-buf/heap > > > > > todo > > > > > since forever, and I like it as a design approach. So would be a good > > > > > idea > > > > > to not toss it. With that display would have symlinks to cma-ecc and > > > > > cma, > > > > > and rendering maybe cma-ecc, shmem, cma heaps (in priority order) for > > > > > a > > > > > SoC where the display needs contig memory for scanout. > > > > > > > > So indeed that is a good point to keep in mind, but I also think it > > > > might re-inforce the choice of having ECC as a flag here. > > > > > > > > Since my understanding of the sysfs symlinks to heaps idea is about > > > > being able to figure out a common heap from a collection of devices, > > > > it's really about the ability for the driver to access the type of > > > > memory. If ECC is just an attribute of the type of memory (as in this > > > > patch series), it being on or off won't necessarily affect > > > > compatibility of the buffer with the device. Similarly "uncached" > > > > seems more of an attribute of memory type and not a type itself. > > > > Hardware that can access non-contiguous "system" buffers can access > > > > uncached system buffers. > > > > > > Yeah, but in graphics there's a wide band where "shit performance" is > > > defacto "not useable (as intended at least)". > > > > > > So if we limit the symlink idea to just making sure zero-copy access is > > > possible, then we might not actually solve the real world problem we need > > > to solve. And so the symlinks become somewhat useless, and we need to > > > somewhere encode which flags you need to use with each symlink. > > > > > > But I also see the argument that there's a bit a combinatorial explosion > > > possible. So I guess the question is where we want to handle it ... > > > > Sorry for jumping into this discussion so late. But are we really > > concerned about this combinatorial explosion in practice? It may be > > theoretically possible to create any combination of these, but do we > > expect more than a couple of heaps to exist in any given system? > > I don't worry too much about the number of heaps available in a given > system, it would indeed be fairly low. > > My concern is about the semantics combinatorial explosion. So far, the > name has carried what semantics we were supposed to get from the buffer > we allocate from that heap. > > The more variations and concepts we'll have, the more heap names we'll > need, and with confusing names since we wo
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
On Fri, Jun 28, 2024 at 03:21:51PM GMT, mrip...@kernel.org wrote: > On Fri, Jun 28, 2024 at 01:47:01PM GMT, Thierry Reding wrote: > > On Thu, Jun 27, 2024 at 04:40:02PM GMT, mrip...@kernel.org wrote: > > > On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > > > > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > > > > External email : Please do not click links or open attachments > > > > > until > > > > > > you have verified the sender or the content. > > > > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049...@kernel.org/ > > > > > > > > > > > > I think it would be better to have the user configurable > > > > > > private > > > > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > > > > requirement can get their private information from dma-buf > > > > > > directly > > > > > > > > > > and > > > > > > > > > > no need to change or add the interface. > > > > > > > > > > > > What's your opinion in this point? > > > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > > > What happens if you get a non-secure buffer imported in your > > > > > > secure > > > > > > > > > device? > > > > > > > > > > > We use the same mediatek-drm driver for secure and > > > > > non-secure > > > > > > buffer. > > > > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > > > > the > > > > > > > > normal flow with normal hardware settings. > > > > > > > > > > > We use different configurations to make hardware have > > > > > different > > > > > > > > permission to access the buffer it should access. > > > > > > > > > > > So if we can't get the information of "the buffer is > > > > > allocated > > > > > > from > > > > > > > > restricted_mtk_cma" when importing the buffer into the driver, > > > > > > > > we > > > > > > won't > > > > > > > > be able to configure the hardware correctly. > > > > > > > > > Why can't you get this information from userspace? > > > > > > > Same reason amd and i915/xe also pass this around internally in > > > > > > > the > > > > > > kernel, it's just that for those gpus the render and kms node are > > > > > > the > > > > > > same > > > > > > driver so this is easy. > > > > > > > > > > > > > > The reason I ask is that encryption here looks just like another > > > > parameter > > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > > > So instead of this during buffer import: > > > > > > > > mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", > > > > 10)); > > > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > > > mtk_gem->size = attach->dmabuf->size; > > > > mtk_gem->sg = sg; > > > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive > > > > key > > > > exchange or something like that. > > > > > > That doesn't work in all cases, unfortunately. > > > > > > If you're doing secure video playback, the firmware is typically in > > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > > aren't accessible by the CPU (or at least, not at the execution level > > > Lin
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
On Fri, Jun 28, 2024 at 02:34:24PM GMT, Christian König wrote: > Am 28.06.24 um 13:47 schrieb Thierry Reding: > > [SNIP] > > > > The reason I ask is that encryption here looks just like another > > > > parameter > > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > > > So instead of this during buffer import: > > > > > > > > mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", > > > > 10)); > > > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > > > mtk_gem->size = attach->dmabuf->size; > > > > mtk_gem->sg = sg; > > > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive > > > > key > > > > exchange or something like that. > > > That doesn't work in all cases, unfortunately. > > > > > > If you're doing secure video playback, the firmware is typically in > > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > > aren't accessible by the CPU (or at least, not at the execution level > > > Linux runs with). > > Can you clarify which firmware you're talking about? Is this secure > > firmware, or firmware running on the video decoding hardware? > > > > > So nobody can map that buffer, and the firmware driver is the one who > > > knows that this buffer cannot be accessed by anyone. Putting this on the > > > userspace to know would be pretty weird, and wouldn't solve the case > > > where the kernel would try to map it. > > Doesn't userspace need to know from the start whether it's trying to do > > secure playback or not? Typically this involves more than just the > > decoding part. You'd typically set up things like HDCP as part of the > > process, so userspace probably already does know that the buffers being > > passed around are protected. > > > > Also, the kernel shouldn't really be mapping these buffers unless > > explicitly told to. In most cases you also wouldn't want the kernel to > > map these kinds of buffers, right? Are there any specific cases where > > you expect the kernel to need to map these? > > > > I've been looking at this on the Tegra side recently and the way it > > works on these chips is that you basically get an opaque carveout region > > that has been locked down by secure firmware or early bootloaders, so > > only certain hardware blocks can access it. We can allocate from that > > carveout and then pass the buffers around. > > > > It may be possible to use these protected carveout regions exclusively > > from the DRM/KMS driver and share them with multimedia engines via DMA- > > BUF, but I've also been looking into perhaps using DMA-BUF heaps to > > expose the carveout, which would make this a bit more flexible and allow > > either userspace to allocate the buffers or have multiple kernel drivers > > share the carveout via the DMA-BUF heap. Though the latter would require > > that there be in-kernel APIs for heaps, so not too sure about that yet. > > Yeah as far as I can see that would be a perfectly valid use case for > DMA-Buf heaps. > > One question here: How does the HDCP setup work on Tegra? From your comment > I guess you pass most of the information through userspace as well. Well, we don't currently support HDCP at all. I do have proof-of-concept patches from a long time ago and, yes, judging by that we'd need to control all of this from userspace. The way I imagine that this would work is that userspace needs to first set the "Content Protection" and "HDCP Content Type" properties and wait for the state change. Once that has happened it can go and allocate the protected memory and start decoding into it and then scan out from these buffers. > Or is there any info inside the DMA-buf for this? In other words would you > also need to know if a buffer is then allocated from this special carveout? I don't think so. It's possible to scan out an unprotected buffer with HDCP enabled. It may also be possible to scan out a protected buffer even if HDCP wasn't enabled, though you would obviously want to prevent that somehow. Not sure if there's a common way to do this, but I guess in end-user devices you'd need a fully trusted boot chain to do that in a compliant way. It's been a long time since I looked at this, but I seem to recall that at the time all software that could do DRM-protected playback on Linux was proprietary for reasons like these. Thierry signature.asc Description: PGP signature
Re: [PATCH v5 7/9] dma-buf: heaps: restricted_heap: Add MediaTek restricted heap and heap_init
On Wed, May 15, 2024 at 07:23:06PM GMT, Yong Wu wrote:
> Add a MediaTek restricted heap which uses TEE service call to restrict
> buffer. Currently this restricted heap is NULL, Prepare for the later
> patch. Mainly there are two changes:
> a) Add a heap_init ops since TEE probe late than restricted heap, thus
>initialize the heap when we require the buffer the first time.
> b) Add a priv_data for each heap, like the special data used by MTK
>(such as "TEE session") can be placed in priv_data.
>
> Currently our heap depends on CMA which could only be bool, thus
> depend on "TEE=y".
>
> Signed-off-by: Yong Wu
> ---
> drivers/dma-buf/heaps/Kconfig | 7 ++
> drivers/dma-buf/heaps/Makefile | 1 +
> drivers/dma-buf/heaps/restricted_heap.c | 11 ++
> drivers/dma-buf/heaps/restricted_heap.h | 2 +
> drivers/dma-buf/heaps/restricted_heap_mtk.c | 115
> 5 files changed, 136 insertions(+)
> create mode 100644 drivers/dma-buf/heaps/restricted_heap_mtk.c
>
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index e54506f480ea..84f748fb2856 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -21,3 +21,10 @@ config DMABUF_HEAPS_RESTRICTED
> heap is to manage buffers that are inaccessible to the kernel and
> user space.
> There may be several ways to restrict it, for example it may be
> encrypted or
> protected by a TEE or hypervisor. If in doubt, say N.
> +
> +config DMABUF_HEAPS_RESTRICTED_MTK
> + bool "MediaTek DMA-BUF Restricted Heap"
> + depends on DMABUF_HEAPS_RESTRICTED && TEE=y
> + help
> + Enable restricted dma-buf heaps for MediaTek platform. This heap is
> backed by
> + TEE client interfaces. If in doubt, say N.
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index a2437c1817e2..0028aa9d875f 100644
> --- a/drivers/dma-buf/heaps/Makefile
> +++ b/drivers/dma-buf/heaps/Makefile
> @@ -1,4 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
> obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED)+= restricted_heap.o
> +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK)+= restricted_heap_mtk.o
> obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)+= system_heap.o
> diff --git a/drivers/dma-buf/heaps/restricted_heap.c
> b/drivers/dma-buf/heaps/restricted_heap.c
> index 4e45d46a6467..8bc8a5e3f969 100644
> --- a/drivers/dma-buf/heaps/restricted_heap.c
> +++ b/drivers/dma-buf/heaps/restricted_heap.c
> @@ -151,11 +151,22 @@ restricted_heap_allocate(struct dma_heap *heap,
> unsigned long size,
>unsigned long fd_flags, unsigned long heap_flags)
> {
> struct restricted_heap *rheap = dma_heap_get_drvdata(heap);
> + const struct restricted_heap_ops *ops = rheap->ops;
> struct restricted_buffer *restricted_buf;
> DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> struct dma_buf *dmabuf;
> int ret;
>
> + /*
> + * In some implements, TEE is required to protect buffer. However TEE
> probe
> + * may be late, Thus heap_init is performed when the first buffer is
> requested.
> + */
> + if (ops->heap_init) {
> + ret = ops->heap_init(rheap);
> + if (ret)
> + return ERR_PTR(ret);
> + }
I wonder if we should make this parameterized rather than the default.
Perhaps we can add a "init_on_demand" (or whatever other name) flag to
struct restricted_heap_ops and then call this from heap initialization
if possible and defer initialization depending on the restricted heap
provider?
> +
> restricted_buf = kzalloc(sizeof(*restricted_buf), GFP_KERNEL);
> if (!restricted_buf)
> return ERR_PTR(-ENOMEM);
> diff --git a/drivers/dma-buf/heaps/restricted_heap.h
> b/drivers/dma-buf/heaps/restricted_heap.h
> index 6d9599a4a34e..2a33a1c7a48b 100644
> --- a/drivers/dma-buf/heaps/restricted_heap.h
> +++ b/drivers/dma-buf/heaps/restricted_heap.h
> @@ -19,6 +19,8 @@ struct restricted_heap {
> const char *name;
>
> const struct restricted_heap_ops *ops;
> +
> + void*priv_data;
Honestly, I would just get rid of any of this extra padding/indentation
in these structures. There's really no benefit to this, except maybe if
you *really* like things to be aligned, in which case the above is now
probably worse than if you didn't try to align in the first place.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v5 5/9] dma-buf: heaps: restricted_heap: Add private heap ops
On Wed, May 15, 2024 at 07:23:04PM GMT, Yong Wu wrote:
> Add "struct restricted_heap_ops". For the restricted memory, totally there
> are two steps:
> a) alloc: Allocate the buffer in kernel;
> b) restrict_buf: Restrict/Protect/Secure that buffer.
> The "alloc" is mandatory while "restrict_buf" is optional since it may
> be part of "alloc".
>
> Signed-off-by: Yong Wu
> ---
> drivers/dma-buf/heaps/restricted_heap.c | 41 -
> drivers/dma-buf/heaps/restricted_heap.h | 12
> 2 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/heaps/restricted_heap.c
> b/drivers/dma-buf/heaps/restricted_heap.c
> index c2ae19ba7d7e..8bb3c1876a69 100644
> --- a/drivers/dma-buf/heaps/restricted_heap.c
> +++ b/drivers/dma-buf/heaps/restricted_heap.c
> @@ -12,10 +12,44 @@
>
> #include "restricted_heap.h"
>
> +static int
> +restricted_heap_memory_allocate(struct restricted_heap *rheap, struct
> restricted_buffer *buf)
> +{
> + const struct restricted_heap_ops *ops = rheap->ops;
> + int ret;
> +
> + ret = ops->alloc(rheap, buf);
> + if (ret)
> + return ret;
> +
> + if (ops->restrict_buf) {
> + ret = ops->restrict_buf(rheap, buf);
> + if (ret)
> + goto buf_free;
> + }
> + return 0;
> +
> +buf_free:
> + ops->free(rheap, buf);
> + return ret;
> +}
> +
> +static void
> +restricted_heap_memory_free(struct restricted_heap *rheap, struct
> restricted_buffer *buf)
> +{
> + const struct restricted_heap_ops *ops = rheap->ops;
> +
> + if (ops->unrestrict_buf)
> + ops->unrestrict_buf(rheap, buf);
> +
> + ops->free(rheap, buf);
> +}
> +
> static struct dma_buf *
> restricted_heap_allocate(struct dma_heap *heap, unsigned long size,
>unsigned long fd_flags, unsigned long heap_flags)
> {
> + struct restricted_heap *rheap = dma_heap_get_drvdata(heap);
> struct restricted_buffer *restricted_buf;
> DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> struct dma_buf *dmabuf;
> @@ -28,6 +62,9 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned
> long size,
> restricted_buf->size = ALIGN(size, PAGE_SIZE);
> restricted_buf->heap = heap;
>
> + ret = restricted_heap_memory_allocate(rheap, restricted_buf);
> + if (ret)
> + goto err_free_buf;
> exp_info.exp_name = dma_heap_get_name(heap);
> exp_info.size = restricted_buf->size;
> exp_info.flags = fd_flags;
> @@ -36,11 +73,13 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned
> long size,
> dmabuf = dma_buf_export(&exp_info);
> if (IS_ERR(dmabuf)) {
> ret = PTR_ERR(dmabuf);
> - goto err_free_buf;
> + goto err_free_rstrd_mem;
> }
>
> return dmabuf;
>
> +err_free_rstrd_mem:
> + restricted_heap_memory_free(rheap, restricted_buf);
> err_free_buf:
> kfree(restricted_buf);
> return ERR_PTR(ret);
> diff --git a/drivers/dma-buf/heaps/restricted_heap.h
> b/drivers/dma-buf/heaps/restricted_heap.h
> index b448f77616ac..5783275d5714 100644
> --- a/drivers/dma-buf/heaps/restricted_heap.h
> +++ b/drivers/dma-buf/heaps/restricted_heap.h
> @@ -15,6 +15,18 @@ struct restricted_buffer {
>
> struct restricted_heap {
> const char *name;
> +
> + const struct restricted_heap_ops *ops;
> +};
> +
> +struct restricted_heap_ops {
> + int (*heap_init)(struct restricted_heap *rheap);
It might be worth moving this to a later patch when it's actually
getting used.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
On Thu, Jun 27, 2024 at 04:40:02PM GMT, mrip...@kernel.org wrote: > On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > > External email : Please do not click links or open attachments > > > until > > > > you have verified the sender or the content. > > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049...@kernel.org/ > > > > > > > > > > I think it would be better to have the user configurable > > > > private > > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > > requirement can get their private information from dma-buf > > > > directly > > > > > > > > and > > > > > > > > no need to change or add the interface. > > > > > > > > > > What's your opinion in this point? > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > What happens if you get a non-secure buffer imported in your > > > > secure > > > > > > > device? > > > > > > > > > We use the same mediatek-drm driver for secure and > > > non-secure > > > > buffer. > > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > > the > > > > > > normal flow with normal hardware settings. > > > > > > > > > We use different configurations to make hardware have > > > different > > > > > > permission to access the buffer it should access. > > > > > > > > > So if we can't get the information of "the buffer is > > > allocated > > > > from > > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > > won't > > > > > > be able to configure the hardware correctly. > > > > > > > Why can't you get this information from userspace? > > > > > Same reason amd and i915/xe also pass this around internally in the > > > > kernel, it's just that for those gpus the render and kms node are the > > > > same > > > > driver so this is easy. > > > > > > > > The reason I ask is that encryption here looks just like another parameter > > for the buffer, e.g. like format, stride, tilling etc.. > > > > So instead of this during buffer import: > > > > mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", 10)); > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > mtk_gem->size = attach->dmabuf->size; > > mtk_gem->sg = sg; > > > > You can trivially say during use hey this buffer is encrypted. > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > exchange or something like that. > > That doesn't work in all cases, unfortunately. > > If you're doing secure video playback, the firmware is typically in > charge of the frame decryption/decoding, and you'd get dma-buf back that > aren't accessible by the CPU (or at least, not at the execution level > Linux runs with). Can you clarify which firmware you're talking about? Is this secure firmware, or firmware running on the video decoding hardware? > So nobody can map that buffer, and the firmware driver is the one who > knows that this buffer cannot be accessed by anyone. Putting this on the > userspace to know would be pretty weird, and wouldn't solve the case > where the kernel would try to map it. Doesn't userspace need to know from the start whether it's trying to do secure playback or not? Typically this involves more than just the decoding part. You'd typically set up things like HDCP as part of the process, so userspace probably already does know that the buffers being passed around are protected. Also, the kernel shouldn't really be mapping these buffers unless explicitly told to. In most cases you also wouldn't want the kernel to map these kinds of buffers, right? Are there any specific cases where you expect the kernel to need to map these? I've been looking at this on the Tegra side recently and the way it works on these chips is that you basically get an opaque carveout region that has been locked down by secure firmware or early bootloaders, so only certain hardware blocks can access it. We can allocate from that carveout and then pass the buffers around. It may be possible to use these protected carveout regions exclusively from the DRM/KMS driver and share them with multimedia engines via DMA- BUF, but I've also been looking into perhaps using DMA-BUF heaps to expose the carveout, which would make this a bit more flexible and allow either userspace to allocate the buffers or have multiple kernel drivers share the carveout via the DMA-BUF heap. Though the latter would require that there be in-kernel APIs for heaps, so not too sure about that yet. Thierry signature.asc Description: PGP signature
Re: [PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags
On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote: > On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote: > > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter wrote: > > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote: > > > > But it makes me a little nervous to add a new generic allocation flag > > > > for a feature most hardware doesn't support (yet, at least). So it's > > > > hard to weigh how common the actual usage will be across all the > > > > heaps. > > > > > > > > I apologize as my worry is mostly born out of seeing vendors really > > > > push opaque feature flags in their old ion heaps, so in providing a > > > > flags argument, it was mostly intended as an escape hatch for > > > > obviously common attributes. So having the first be something that > > > > seems reasonable, but isn't actually that common makes me fret some. > > > > > > > > So again, not an objection, just something for folks to stew on to > > > > make sure this is really the right approach. > > > > > > Another good reason to go with full heap names instead of opaque flags on > > > existing heaps is that with the former we can use symlinks in sysfs to > > > specify heaps, with the latter we need a new idea. We haven't yet gotten > > > around to implement this anywhere, but it's been in the dma-buf/heap todo > > > since forever, and I like it as a design approach. So would be a good idea > > > to not toss it. With that display would have symlinks to cma-ecc and cma, > > > and rendering maybe cma-ecc, shmem, cma heaps (in priority order) for a > > > SoC where the display needs contig memory for scanout. > > > > So indeed that is a good point to keep in mind, but I also think it > > might re-inforce the choice of having ECC as a flag here. > > > > Since my understanding of the sysfs symlinks to heaps idea is about > > being able to figure out a common heap from a collection of devices, > > it's really about the ability for the driver to access the type of > > memory. If ECC is just an attribute of the type of memory (as in this > > patch series), it being on or off won't necessarily affect > > compatibility of the buffer with the device. Similarly "uncached" > > seems more of an attribute of memory type and not a type itself. > > Hardware that can access non-contiguous "system" buffers can access > > uncached system buffers. > > Yeah, but in graphics there's a wide band where "shit performance" is > defacto "not useable (as intended at least)". > > So if we limit the symlink idea to just making sure zero-copy access is > possible, then we might not actually solve the real world problem we need > to solve. And so the symlinks become somewhat useless, and we need to > somewhere encode which flags you need to use with each symlink. > > But I also see the argument that there's a bit a combinatorial explosion > possible. So I guess the question is where we want to handle it ... Sorry for jumping into this discussion so late. But are we really concerned about this combinatorial explosion in practice? It may be theoretically possible to create any combination of these, but do we expect more than a couple of heaps to exist in any given system? Would it perhaps make more sense to let a platform override the heap name to make it more easily identifiable? Maybe this is a naive assumption, but aren't userspace applications and drivers not primarily interested in the "type" of heap rather than whatever specific flags have been set for it? For example, if an applications wants to use a protected buffer, the application doesn't (and shouldn't need to) care about whether the heap for that buffer supports ECC or is backed by CMA. All it really needs to know is that it's the system's "protected" heap. This rather than try to represent every possible combination we basically make this a "configuration" issue. System designers need to settle on whatever combination of flags work for all the desired use- cases and then we expose that combination as a named heap. One problem that this doesn't solve is that we still don't have a way of retrieving these flags in drivers which may need them. Perhaps one way to address this would be to add in-kernel APIs to allocate from a heap. That way a DRM/KMS driver (for example) could find a named heap, allocate from it and implicitly store flags about the heap/buffer. Or maybe we could add in-kernel API to retrieve flags, which would be a bit better than having to expose them to userspace. Thierry signature.asc Description: PGP signature
Re: [PATCH V2] drm/tegra: fix a possible null pointer dereference
On Sun Jun 2, 2024 at 10:46 AM CEST, Huai-Yuan Liu wrote: > In malidp_tegra_crtc_reset, new memory is allocated with kzalloc, but > no check is performed. Before calling __drm_atomic_helper_crtc_reset, > mw_state should be checked to prevent possible null pointer dereference. The commit message here still references variables that don't exist in this driver. Looks like copy/paste leftovers from a similar patch? Thierry signature.asc Description: PGP signature
Re: [PATCH v2 3/8] drm/tegra: Call drm_atomic_helper_shutdown() at shutdown time
On Thu Jun 13, 2024 at 12:23 AM CEST, Douglas Anderson wrote: > Based on grepping through the source code this driver appears to be > missing a call to drm_atomic_helper_shutdown() at system shutdown > time. Among other things, this means that if a panel is in use that it > won't be cleanly powered off at system shutdown time. > > The fact that we should call drm_atomic_helper_shutdown() in the case > of OS shutdown/restart comes straight out of the kernel doc "driver > instance overview" in drm_drv.c. > > Suggested-by: Maxime Ripard > Reviewed-by: Maxime Ripard > Signed-off-by: Douglas Anderson > --- > This commit is only compile-time tested. > > (no changes since v1) > > drivers/gpu/drm/tegra/drm.c | 6 ++ > 1 file changed, 6 insertions(+) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH] drm/tegra: fix a possible null pointer dereference
On Fri May 31, 2024 at 3:56 PM CEST, Huai-Yuan Liu wrote:
> In malidp_tegra_crtc_reset, new memory is allocated with kzalloc, but
> no check is performed. Before calling __drm_atomic_helper_crtc_reset,
> mw_state should be checked to prevent possible null pointer dereferene.
Please check that all these variable name references actually are valid.
Also, "dereference".
> Fixes: b7e0b04ae450 ("drm/tegra: Convert to using
> __drm_atomic_helper_crtc_reset() for reset.")
> Signed-off-by: Huai-Yuan Liu
> ---
> drivers/gpu/drm/tegra/dc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index be61c9d1a4f0..7648e129c212 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -1388,7 +1388,8 @@ static void tegra_crtc_reset(struct drm_crtc *crtc)
> if (crtc->state)
> tegra_crtc_atomic_destroy_state(crtc, crtc->state);
>
> - __drm_atomic_helper_crtc_reset(crtc, &state->base);
> + if (state)
> + __drm_atomic_helper_crtc_reset(crtc, &state->base);
Looking at how other drivers do this, they call
__drm_atomic_helper_crtc_reset(crtc, NULL);
if they fail to allocate a new state. Any reason why we wouldn't want to
do the same thing here?
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] docs: document python version used for compilation
On Fri May 31, 2024 at 9:33 AM CEST, Geert Uytterhoeven wrote:
> Hi Thierry,
>
> On Thu, May 30, 2024 at 7:07 PM Thierry Reding
> wrote:
> > Alternatively, maybe Kconfig could be taught about build dependencies?
>
> git grep "depends on \$(" -- "*Kconf*"
Duh... of course there's something like this already. =)
Maybe something like the attached patch?
Thierry
From 153eaec61513e14f5a7f8f2a998600d07f17bc84 Mon Sep 17 00:00:00 2001
From: Thierry Reding
Date: Fri, 31 May 2024 10:51:42 +0200
Subject: [PATCH] kbuild: Allow Kconfig dependendencies on Python
Recently drivers have started depending on Python to generate register
definitions during the build process. In order to prevent such drivers
from breaking builds on systems that don't have Python installed, make
them depend on the minimum required Python version that they need via
Kconfig. If Python is not installed on the system, these drivers will
be automatically disabled.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/msm/Kconfig | 1 +
scripts/min-tool-version.sh | 3 +++
scripts/python-version.sh | 41 +
3 files changed, 45 insertions(+)
create mode 100755 scripts/python-version.sh
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 1931ecf73e32..5f7f84de55e4 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -11,6 +11,7 @@ config DRM_MSM
depends on QCOM_LLCC || QCOM_LLCC=n
depends on QCOM_COMMAND_DB || QCOM_COMMAND_DB=n
depends on PM
+ depends on $(success,$(srctree)/scripts/python-version.sh)
select IOMMU_IO_PGTABLE
select QCOM_MDT_LOADER if ARCH_QCOM
select REGULATOR
diff --git a/scripts/min-tool-version.sh b/scripts/min-tool-version.sh
index 91c91201212c..447a3ad4c0bf 100755
--- a/scripts/min-tool-version.sh
+++ b/scripts/min-tool-version.sh
@@ -38,6 +38,9 @@ rustc)
bindgen)
echo 0.65.1
;;
+python)
+ echo 3.5.0
+ ;;
*)
echo "$1: unknown tool" >&2
exit 1
diff --git a/scripts/python-version.sh b/scripts/python-version.sh
new file mode 100755
index ..c997d40418dc
--- /dev/null
+++ b/scripts/python-version.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Print the Python version in a 5 or 6-digit form.
+# Also, perform the minimum version check.
+
+set -e
+
+PYTHON=${PYTHON:-python}
+
+get_canonical_version()
+{
+ IFS=.
+ set -- $1
+
+ # If the 2nd or 3rd field is missing, fill it with a zero.
+ echo $((1 * $1 + 100 * ${2:-0} + ${3:-0}))
+}
+
+output=$(LC_ALL=C "$PYTHON" --version)
+
+# Split the line on spaces.
+IFS=' '
+set -- $output
+name=$1
+version=$2
+
+min_tool_version=$(dirname $0)/min-tool-version.sh
+min_version=$($min_tool_version python)
+
+cversion=$(get_canonical_version $version)
+min_version=$(get_canonical_version $min_version)
+
+if [ "$cversion" -lt "$min_version" ]; then
+ echo >&2 "***"
+ echo >&2 "*** Python is too old."
+ echo >&2 "***"
+ exit 1
+fi
+
+echo $name $version
--
2.44.0
signature.asc
Description: PGP signature
Re: [BUG] Build failure and alleged fix for next-20240523
On Thu May 30, 2024 at 6:55 PM CEST, Paul E. McKenney wrote:
> On Fri, May 24, 2024 at 12:57:58PM -0700, Abhinav Kumar wrote:
> > Hello
> >
> > On 5/24/2024 12:55 PM, Paul E. McKenney wrote:
> > > Hello!
> > >
> > > I get the following allmodconfig build error on x86 in next-20240523:
> > >
> > > Traceback (most recent call last):
> > > File "drivers/gpu/drm/msm/registers/gen_header.py", line 970, in
> > >
> > > main()
> > > File "drivers/gpu/drm/msm/registers/gen_header.py", line 951, in main
> > > parser.add_argument('--validate',
> > > action=argparse.BooleanOptionalAction)
> > > AttributeError: module 'argparse' has no attribute 'BooleanOptionalAction'
> > >
> > > The following patch allows the build to complete successfully:
> > >
> > > https://patchwork.kernel.org/project/dri-devel/patch/20240508091751.336654-1-jonath...@nvidia.com/#25842751
> > >
> > > As to whether this is a proper fix, I must defer to the DRM folks on CC.
> >
> > Thanks for the report.
> >
> > I have raised a merge request for
> > https://patchwork.freedesktop.org/patch/593057/ to make it available for the
> > next fixes release for msm.
>
> Thank you!
>
> This still is not in -next, so I am putting it into -rcu just to silence
> the diagnostic. Or should I push this to mainline via -rcu?
I pushed this to drm-misc-fixes earlier today, so should show up in
linux-next soon and hopefully in v6.10-rc2.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] docs: document python version used for compilation
On Fri May 10, 2024 at 10:04 PM CEST, Rob Clark wrote: > On Fri, May 10, 2024 at 3:09 AM Jani Nikula wrote: > > > > On Fri, 10 May 2024, Mauro Carvalho Chehab wrote: > > > Em Fri, 10 May 2024 11:08:38 +0300 > > > Jani Nikula escreveu: > > > > > >> On Thu, 09 May 2024, Dmitry Baryshkov > > >> wrote: > > >> > The drm/msm driver had adopted using Python3 script to generate > > >> > register > > >> > header files instead of shipping pre-generated header files. Document > > >> > the minimal Python version supported by the script. > > >> > > > >> > Signed-off-by: Dmitry Baryshkov > > >> > --- > > >> > Documentation/process/changes.rst | 1 + > > >> > 1 file changed, 1 insertion(+) > > >> > > > >> > diff --git a/Documentation/process/changes.rst > > >> > b/Documentation/process/changes.rst > > >> > index 5685d7bfe4d0..8d225a9f65a2 100644 > > >> > --- a/Documentation/process/changes.rst > > >> > +++ b/Documentation/process/changes.rst > > >> > @@ -63,6 +63,7 @@ cpio any cpio > > >> > --version > > >> > GNU tar1.28 tar --version > > >> > gtags (optional) 6.6.5gtags --version > > >> > mkimage (optional) 2017.01 mkimage --version > > >> > +Python (optional) 3.5.xpython3 --version > > >> > > >> Python 3.5 reached end-of-life 3½ years ago [1]. What's the point in > > >> using anything older than the oldest supported version of Python, > > >> i.e. 3.8 at this time? > > > > > > What's the point of breaking compilation with on older distros? > > > The idea of minimal versions here is to specify the absolute minimum > > > version that it is required for the build to happen. If 3.5 is > > > the minimal one, then be it. > > > > AFAICT 3.5 was an arbitrary rather than a deliberate choice. We should > > at least be aware *why* we'd be sticking to old versions. > > > > Minimum versions here also means sticking to features available in said > > versions, for Python just as well as for GCC or any other tool. That's > > not zero cost. > > At this point, the cost to having a lower minimum version is pretty > small, so I'm not worrying too much about it. > > Maybe once kernel developers discover mako, and start generating more > at build time, we'll have to re-evaluate. ;-) You're making an interesting point. Does the build dependency here denote Python (& standard library) or do we assume that if people have Python installed that they can also install arbitrary extra packages? Would a Mako dependency need to be explicitly mentioned here? Thierry signature.asc Description: PGP signature
Re: [PATCH] docs: document python version used for compilation
On Thu May 9, 2024 at 6:48 PM CEST, Jonathan Corbet wrote: > Dmitry Baryshkov writes: > > > The drm/msm driver had adopted using Python3 script to generate register > > header files instead of shipping pre-generated header files. Document > > the minimal Python version supported by the script. > > > > Signed-off-by: Dmitry Baryshkov > > --- > > Documentation/process/changes.rst | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/Documentation/process/changes.rst > > b/Documentation/process/changes.rst > > index 5685d7bfe4d0..8d225a9f65a2 100644 > > --- a/Documentation/process/changes.rst > > +++ b/Documentation/process/changes.rst > > @@ -63,6 +63,7 @@ cpio any cpio --version > > GNU tar1.28 tar --version > > gtags (optional) 6.6.5gtags --version > > mkimage (optional) 2017.01 mkimage --version > > +Python (optional) 3.5.xpython3 --version > > == === > > > > Is it really optional - can you build the driver without it? > > This document needs some help... I'm missing a number of things that are > *not* marked as "optional" (jfsutils, reiserfsprogs, pcmciautils, ppp, > ...) and somehow my system works fine :) It would be nice to document > *why* users might need a specific tool. > > But I guess we aren't going to do that now. I can apply this, but I do > wonder about the "optional" marking. I guess it depends a bit on what exactly "optional" implies. It's optional in the sense that you can easily disable the driver and then build without Python. So does "optional" mean that allmodconfig for all platforms builds without the dependency? Or does it mean some definition of "core" kernel builds for a set of defined platforms? Maybe this really needs to be annotated with the exact Kconfig options that need this. Although that could get out of hands rather quickly. At some point we may have to list a *lot* of these options. Alternatively, maybe Kconfig could be taught about build dependencies? Thierry signature.asc Description: PGP signature
Re: [PATCH] drm/msm: remove python 3.9 dependency for compiling msm
On Wed May 8, 2024 at 1:04 AM CEST, Abhinav Kumar wrote:
> Since commit 5acf49119630 ("drm/msm: import gen_header.py script from Mesa"),
> compilation is broken on machines having python versions older than 3.9
> due to dependency on argparse.BooleanOptionalAction.
>
> Switch to use simple bool for the validate flag to remove the dependency.
>
> Fixes: 5acf49119630 ("drm/msm: import gen_header.py script from Mesa")
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/msm/registers/gen_header.py | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Irrespective of whether we want to allow Python as a build dependency or
not, it already is since v6.10-rc1, so in the meantime I'm going to
apply this to drm-misc-fixes to unbreak things.
If we decide that we don't want the extra dependency we need revert the
whole generation infrastructure.
Thierry
signature.asc
Description: PGP signature
Re: (subset) [PATCH v7 0/5] Add Tegra Security Engine driver
On Fri Apr 26, 2024 at 5:32 PM CEST, Thierry Reding wrote: > From: Thierry Reding > > > On Wed, 03 Apr 2024 15:30:34 +0530, Akhil R wrote: > > Add support for Tegra Security Engine which can accelerates various > > crypto algorithms. The Engine has two separate instances within for > > AES and HASH algorithms respectively. > > > > The driver registers two crypto engines - one for AES and another for > > HASH algorithms and these operate independently and both uses the host1x > > bus. Additionally, it provides hardware-assisted key protection for up to > > 15 symmetric keys which it can use for the cipher operations. > > > > [...] > > Applied, thanks! > > [4/5] arm64: defconfig: Enable Tegra Security Engine > commit: 4d4d3fe6b3cc2a0b2a334a08bb9c64ba1dcbbea4 For the record, I've also applied patch 5/5 but it didn't apply cleanly and so b4 didn't track it properly. Thanks, Thierry signature.asc Description: PGP signature
Re: (subset) [PATCH v7 0/5] Add Tegra Security Engine driver
From: Thierry Reding On Wed, 03 Apr 2024 15:30:34 +0530, Akhil R wrote: > Add support for Tegra Security Engine which can accelerates various > crypto algorithms. The Engine has two separate instances within for > AES and HASH algorithms respectively. > > The driver registers two crypto engines - one for AES and another for > HASH algorithms and these operate independently and both uses the host1x > bus. Additionally, it provides hardware-assisted key protection for up to > 15 symmetric keys which it can use for the cipher operations. > > [...] Applied, thanks! [4/5] arm64: defconfig: Enable Tegra Security Engine commit: 4d4d3fe6b3cc2a0b2a334a08bb9c64ba1dcbbea4 Best regards, -- Thierry Reding
Re: [PATCH] gpu: host1x: mipi: Benefit from devm_clk_get_prepared()
On Tue Apr 9, 2024 at 6:50 PM CEST, Uwe Kleine-König wrote: > When using devm_clk_get_prepared() instead of devm_clk_get() the clock > is already returned prepared. So probe doesn't need to call > clk_prepare() and at remove time the call to clk_unprepare() can be > dropped. The latter makes the remove callback empty, so it can be > dropped, too. > > Signed-off-by: Uwe Kleine-König > --- > Hello, > > the motivation for this patch is that the driver uses struct > platform_driver::remove() which I plan to change the prototype of. Instead > of converting the driver to the temporal .remove_new() and then back to > the new .remove(), drop the remove callback completely. > > Best regards > Uwe > > drivers/gpu/host1x/mipi.c | 17 + > 1 file changed, 1 insertion(+), 16 deletions(-) Feel free to take this in through whatever tree is most appropriate for this ongoing work: Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH 3/4] gpu: host1x: Convert to platform remove callback returning void
On Tue Apr 9, 2024 at 7:02 PM CEST, Uwe Kleine-König wrote: > The .remove() callback for a platform driver returns an int which makes > many driver authors wrongly assume it's possible to do error handling by > returning an error code. However the value returned is ignored (apart > from emitting a warning) and this typically results in resource leaks. > > To improve here there is a quest to make the remove callback return > void. In the first step of this quest all drivers are converted to > .remove_new(), which already returns void. Eventually after all drivers > are converted, .remove_new() will be renamed to .remove(). > > Trivially convert this driver from always returning zero in the remove > callback to the void returning variant. > > Signed-off-by: Uwe Kleine-König > --- > drivers/gpu/host1x/dev.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH 0/4] gpu: Convert to platform remove callback returning void
On Fri Apr 19, 2024 at 9:20 AM CEST, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Apr 09, 2024 at 07:02:47PM +0200, Uwe Kleine-König wrote:
> > with some patches sent earlier[1], this series converts all platform
> > drivers below drivers/gpu to not use struct platform_device::remove()
> > any more.
> >
> > See commit 5c5a7680e67b ("platform: Provide a remove callback that
> > returns no value") for an extended explanation and the eventual goal.
> >
> > All conversations are trivial, because the driver's .remove() callbacks
> > returned zero unconditionally.
> >
> > There are no interdependencies between these patches. This is merge
> > window material.
>
> I wonder how this series will make it in. While I would prefer these
> patches to go in together (that I can consider this thread completed in
> one go), I think with how drm maintenace works, it's best if the patches
> are picked up by their individual maintainers. I guess that's:
>
> - Frank Binns + Matt Coster for imagination
>
> - Chun-Kuang Hu + Philipp Zabel for mediatek
>
> - Thierry Reding + Mikko Perttunen for the host1x driver
>(Note there is another patch for this driver set at
> 20240409165043.105137-2-u.kleine-koe...@pengutronix.de that is
> relevant for the same quest.)
>
> - Philipp Zabel for ipu-v3
>
> I plan to send a patch changing struct platform_driver::remove after the
> end of the merge window leading to 6.10-rc1 for inclusion in next via
> Greg's driver core. So please either care the patches land in 6.10-rc1
> or ack that I include them in the submission to Greg.
I think the latter would make more sense. I'll go ack those patches.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] gpu: host1x: Do not setup DMA for virtual devices
On Wed Apr 3, 2024 at 12:07 PM CEST, Jon Hunter wrote:
> Hi Thierry,
>
> On 15/03/2024 11:25, Jon Hunter wrote:
> >
> > On 14/03/2024 15:49, Thierry Reding wrote:
> >> From: Thierry Reding
> >>
> >> The host1x devices are virtual compound devices and do not perform DMA
> >> accesses themselves, so they do not need to be set up for DMA.
> >>
> >> Ideally we would also not need to set up DMA masks for the virtual
> >> devices, but we currently still need those for legacy support on old
> >> hardware.
> >>
> >> Signed-off-by: Thierry Reding
> >> ---
> >> drivers/gpu/host1x/bus.c | 8
> >> 1 file changed, 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> >> index 783975d1384f..7c52757a89db 100644
> >> --- a/drivers/gpu/host1x/bus.c
> >> +++ b/drivers/gpu/host1x/bus.c
> >> @@ -351,11 +351,6 @@ static int host1x_device_uevent(const struct
> >> device *dev,
> >> return 0;
> >> }
> >> -static int host1x_dma_configure(struct device *dev)
> >> -{
> >> - return of_dma_configure(dev, dev->of_node, true);
> >> -}
> >> -
> >> static const struct dev_pm_ops host1x_device_pm_ops = {
> >> .suspend = pm_generic_suspend,
> >> .resume = pm_generic_resume,
> >> @@ -369,7 +364,6 @@ const struct bus_type host1x_bus_type = {
> >> .name = "host1x",
> >> .match = host1x_device_match,
> >> .uevent = host1x_device_uevent,
> >> - .dma_configure = host1x_dma_configure,
> >> .pm = &host1x_device_pm_ops,
> >> };
> >> @@ -458,8 +452,6 @@ static int host1x_device_add(struct host1x *host1x,
> >> device->dev.bus = &host1x_bus_type;
> >> device->dev.parent = host1x->dev;
> >> - of_dma_configure(&device->dev, host1x->dev->of_node, true);
> >> -
> >> device->dev.dma_parms = &device->dma_parms;
> >> dma_set_max_seg_size(&device->dev, UINT_MAX);
> >
> >
> > Tested-by: Jon Hunter
> > Acked-by: Jon Hunter
>
>
> I don't see this in -next yet?
>
> Ideally, if we don't see any issues with this we should pull this into
> v6.8.y stable branch because I am now seeing the warning there. Should
> we apply a fixes tag to this?
I was finally able to run some finally tests on this and pushed it to
drm-misc-fixes, so it should go into linux-next and then Linus' tree
sometime soon.
I decided against adding a Fixes tag because it's difficult to backport
this all the way to the release which contains the commit that added
the issue. Adding a Fixes tag to the commit that ended up exposing the
issue didn't seem right either, so let's get this into mainline first
and then manually ask stable maintainers to pick this up.
Thierry
signature.asc
Description: PGP signature
[PATCH] gpu: host1x: Do not setup DMA for virtual devices
From: Thierry Reding
The host1x devices are virtual compound devices and do not perform DMA
accesses themselves, so they do not need to be set up for DMA.
Ideally we would also not need to set up DMA masks for the virtual
devices, but we currently still need those for legacy support on old
hardware.
Signed-off-by: Thierry Reding
---
drivers/gpu/host1x/bus.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 783975d1384f..7c52757a89db 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -351,11 +351,6 @@ static int host1x_device_uevent(const struct device *dev,
return 0;
}
-static int host1x_dma_configure(struct device *dev)
-{
- return of_dma_configure(dev, dev->of_node, true);
-}
-
static const struct dev_pm_ops host1x_device_pm_ops = {
.suspend = pm_generic_suspend,
.resume = pm_generic_resume,
@@ -369,7 +364,6 @@ const struct bus_type host1x_bus_type = {
.name = "host1x",
.match = host1x_device_match,
.uevent = host1x_device_uevent,
- .dma_configure = host1x_dma_configure,
.pm = &host1x_device_pm_ops,
};
@@ -458,8 +452,6 @@ static int host1x_device_add(struct host1x *host1x,
device->dev.bus = &host1x_bus_type;
device->dev.parent = host1x->dev;
- of_dma_configure(&device->dev, host1x->dev->of_node, true);
-
device->dev.dma_parms = &device->dma_parms;
dma_set_max_seg_size(&device->dev, UINT_MAX);
--
2.44.0
[PATCH] drm: Remove drm_num_crtcs() helper
From: Thierry Reding
The drm_num_crtcs() helper determines the number of CRTCs by iterating
over the list of CRTCs that have been registered with the mode config.
However, we already keep track of that number in the mode config's
num_crtcs field, so we can simply retrieve the value from that and
remove the extra helper function.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_crtc.c | 15 +--
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 6795624f16e7..82c665d3e74b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -107,18 +107,6 @@ int drm_crtc_force_disable(struct drm_crtc *crtc)
return drm_mode_set_config_internal(&set);
}
-static unsigned int drm_num_crtcs(struct drm_device *dev)
-{
- unsigned int num = 0;
- struct drm_crtc *tmp;
-
- drm_for_each_crtc(tmp, dev) {
- num++;
- }
-
- return num;
-}
-
int drm_crtc_register_all(struct drm_device *dev)
{
struct drm_crtc *crtc;
@@ -278,8 +266,7 @@ static int __drm_crtc_init_with_planes(struct drm_device
*dev, struct drm_crtc *
if (name) {
crtc->name = kvasprintf(GFP_KERNEL, name, ap);
} else {
- crtc->name = kasprintf(GFP_KERNEL, "crtc-%d",
- drm_num_crtcs(dev));
+ crtc->name = kasprintf(GFP_KERNEL, "crtc-%d", config->num_crtc);
}
if (!crtc->name) {
drm_mode_object_unregister(dev, &crtc->base);
--
2.44.0
Re: [PATCH v2] drm/tegra: Remove existing framebuffer only if we support display
On Mon Feb 26, 2024 at 1:08 PM CET, Robert Foss wrote:
> On Mon, Feb 26, 2024 at 12:36 PM Javier Martinez Canillas
> wrote:
> >
> > Thomas Zimmermann writes:
> >
> > > Hi
> > >
> > > Am 23.02.24 um 16:03 schrieb Thierry Reding:
> > >> From: Thierry Reding
> > >>
> > >> Tegra DRM doesn't support display on Tegra234 and later, so make sure
> > >> not to remove any existing framebuffers in that case.
> > >>
> > >> v2: - add comments explaining how this situation can come about
> > >> - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits
> > >>
>
> Fixes: 6848c291a54f ("drm/aperture: Convert drivers to aperture interfaces")
>
> Maybe this is more of a philosophical question, but either the
> introduction of this hardware generation is where this regression was
> introduced or this possibly this commit.
>
> Either way, I'd like to get this into the drm-misc-fixes branch.
That commit looks about right. Technically Tegra234 support was
introduced in Linux 5.10 but the first platform where you we would've
seen this wasn't added until 5.17. The above commit is from 5.14, which
puts it about right in between there, so I think that's fine.
Backporting to anything before 5.14 would need to be manual and isn't
worth it.
Thierry
signature.asc
Description: PGP signature
[PATCH v2] drm/tegra: Remove existing framebuffer only if we support display
From: Thierry Reding
Tegra DRM doesn't support display on Tegra234 and later, so make sure
not to remove any existing framebuffers in that case.
v2: - add comments explaining how this situation can come about
- clear DRIVER_MODESET and DRIVER_ATOMIC feature bits
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/tegra/drm.c | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index b1e1a78e30c6..2e1cfe6f6d74 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1220,9 +1220,26 @@ static int host1x_drm_probe(struct host1x_device *dev)
drm_mode_config_reset(drm);
- err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
- if (err < 0)
- goto hub;
+ /*
+* Only take over from a potential firmware framebuffer if any CRTCs
+* have been registered. This must not be a fatal error because there
+* are other accelerators that are exposed via this driver.
+*
+* Another case where this happens is on Tegra234 where the display
+* hardware is no longer part of the host1x complex, so this driver
+* will not expose any modesetting features.
+*/
+ if (drm->mode_config.num_crtc > 0) {
+ err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
+ if (err < 0)
+ goto hub;
+ } else {
+ /*
+* Indicate to userspace that this doesn't expose any display
+* capabilities.
+*/
+ drm->driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
+ }
err = drm_dev_register(drm, 0);
if (err < 0)
--
2.43.2
Re: [PATCH] gpu: host1x: Skip reset assert on Tegra186
On Thu Feb 22, 2024 at 2:05 AM CET, Mikko Perttunen wrote: > From: Mikko Perttunen > > On Tegra186, secure world applications may need to access host1x > during suspend/resume, and rely on the kernel to keep Host1x out > of reset during the suspend cycle. As such, as a quirk, > skip asserting Host1x's reset on Tegra186. > > We don't need to keep the clocks enabled, as BPMP ensures the clock > stays on while Host1x is being used. On newer SoC's, the reset line > is inaccessible, so there is no need for the quirk. > > Signed-off-by: Mikko Perttunen > --- > drivers/gpu/host1x/dev.c | 15 +-- > drivers/gpu/host1x/dev.h | 6 ++ > 2 files changed, 15 insertions(+), 6 deletions(-) Applied to drm-misc-fixes, though I added the Fixes: tag that Jon mentioned in reply to v1 of this as well as his Reviewed-by and Tested-by as well, since this is pretty much the same patch except for the comments. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH] gpu: host1x: Skip reset assert on Tegra186
On Mon Feb 19, 2024 at 3:18 AM CET, Mikko Perttunen wrote: > On 2/16/24 19:02, Thierry Reding wrote: > > On Wed Feb 14, 2024 at 12:40 PM CET, Mikko Perttunen wrote: > >> From: Mikko Perttunen > >> > >> On Tegra186, other software components may rely on the kernel to > >> keep Host1x operational even during suspend. As such, as a quirk, > >> skip asserting Host1x's reset on Tegra186. > > > > This all sounds a bit vague. What other software components rely on the > > kernel to keep host1x operational during suspend? And why do they do so? > > Why is this not a problem elsewhere? > > My assumption is that it's due to a secure world application accessing > NVDEC or display engines during suspend or resume. This happening > without kernel knowledge is a bad thing, but it's hard to change at this > point. > > The reset line (CAR vs BPMP vs non-accessible reset line), and the > secure application code programming this stuff is slightly different in > every chip generation, which is where I think the differences happen. *sigh* I guess it is what it is. Please add a bit more background information to the commit message and also a comment for the skip_reset field so that people (including myself) will remember down the road why this exists. Thierry signature.asc Description: PGP signature
Re: [PATCH] phy: constify of_phandle_args in xlate
On Sat Feb 17, 2024 at 10:39 AM CET, Krzysztof Kozlowski wrote: > The xlate callbacks are supposed to translate of_phandle_args to proper > provider without modifying the of_phandle_args. Make the argument > pointer to const for code safety and readability. > > Signed-off-by: Krzysztof Kozlowski > --- > drivers/phy/allwinner/phy-sun4i-usb.c | 2 +- > drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c | 2 +- > drivers/phy/broadcom/phy-bcm-sr-pcie.c | 2 +- > drivers/phy/broadcom/phy-bcm-sr-usb.c | 2 +- > drivers/phy/broadcom/phy-bcm63xx-usbh.c| 2 +- > drivers/phy/broadcom/phy-brcm-usb.c| 2 +- > drivers/phy/freescale/phy-fsl-imx8qm-lvds-phy.c| 2 +- > drivers/phy/freescale/phy-fsl-lynx-28g.c | 2 +- > drivers/phy/hisilicon/phy-histb-combphy.c | 2 +- > drivers/phy/intel/phy-intel-lgm-combo.c| 2 +- > drivers/phy/lantiq/phy-lantiq-vrx200-pcie.c| 2 +- > drivers/phy/marvell/phy-armada375-usb2.c | 2 +- > drivers/phy/marvell/phy-armada38x-comphy.c | 2 +- > drivers/phy/marvell/phy-berlin-sata.c | 2 +- > drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 2 +- > drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 2 +- > drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c| 2 +- > drivers/phy/mediatek/phy-mtk-tphy.c| 2 +- > drivers/phy/mediatek/phy-mtk-xsphy.c | 2 +- > drivers/phy/microchip/lan966x_serdes.c | 2 +- > drivers/phy/microchip/sparx5_serdes.c | 2 +- > drivers/phy/mscc/phy-ocelot-serdes.c | 2 +- > drivers/phy/phy-core.c | 8 > drivers/phy/phy-xgene.c| 2 +- > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 2 +- > drivers/phy/ralink/phy-mt7621-pci.c| 2 +- > drivers/phy/renesas/phy-rcar-gen2.c| 2 +- > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 2 +- > drivers/phy/renesas/r8a779f0-ether-serdes.c| 2 +- > drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 2 +- > drivers/phy/rockchip/phy-rockchip-pcie.c | 2 +- > drivers/phy/samsung/phy-exynos-mipi-video.c| 2 +- > drivers/phy/samsung/phy-exynos5-usbdrd.c | 2 +- > drivers/phy/samsung/phy-samsung-usb2.c | 2 +- > drivers/phy/socionext/phy-uniphier-usb2.c | 2 +- > drivers/phy/st/phy-miphy28lp.c | 2 +- > drivers/phy/st/phy-spear1310-miphy.c | 2 +- > drivers/phy/st/phy-spear1340-miphy.c | 2 +- > drivers/phy/st/phy-stm32-usbphyc.c | 2 +- > drivers/phy/tegra/xusb.c | 2 +- > drivers/phy/ti/phy-am654-serdes.c | 2 +- > drivers/phy/ti/phy-da8xx-usb.c | 2 +- > drivers/phy/ti/phy-gmii-sel.c | 2 +- > drivers/phy/xilinx/phy-zynqmp.c| 2 +- > drivers/pinctrl/tegra/pinctrl-tegra-xusb.c | 2 +- > include/linux/phy/phy.h| 14 +++--- > 46 files changed, 55 insertions(+), 55 deletions(-) Makes sense: Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH][next] gpu: host1x: remove redundant assignment to variable space
On Thu Feb 15, 2024 at 11:43 PM CET, Colin Ian King wrote: > The variable space is being initialized with a value that is never read, > it is being re-assigned later on. The initialization is redundant and > can be removed. Also merge two declaration lines together. > > Cleans up clang scan build warning: > drivers/gpu/host1x/cdma.c:628:15: warning: Value stored to 'space' > during its initialization is never read [deadcode.DeadStores] > > Signed-off-by: Colin Ian King > --- > drivers/gpu/host1x/cdma.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Applied to drm-misc-next, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH] gpu: host1x: Skip reset assert on Tegra186
On Wed Feb 14, 2024 at 12:40 PM CET, Mikko Perttunen wrote: > From: Mikko Perttunen > > On Tegra186, other software components may rely on the kernel to > keep Host1x operational even during suspend. As such, as a quirk, > skip asserting Host1x's reset on Tegra186. This all sounds a bit vague. What other software components rely on the kernel to keep host1x operational during suspend? And why do they do so? Why is this not a problem elsewhere? Thierry signature.asc Description: PGP signature
Re: [PATCH] gpu: host1x: bus: make host1x_bus_type const
On Tue Feb 13, 2024 at 3:44 PM CET, Ricardo B. Marliere wrote:
> Since commit d492cc2573a0 ("driver core: device.h: make struct
> bus_type a const *"), the driver core can properly handle constant
> struct bus_type, move the host1x_bus_type variable to be a constant
> structure as well, placing it into read-only memory which can not be
> modified at runtime.
>
> Cc: Greg Kroah-Hartman
> Suggested-by: Greg Kroah-Hartman
> Signed-off-by: Ricardo B. Marliere
> ---
> drivers/gpu/host1x/bus.c | 2 +-
> drivers/gpu/host1x/bus.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Applied to drm-misc-next, thanks.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 0/6] drm/tegra: Fix some error handling paths
On Sat, Sep 02, 2023 at 05:22:07PM +0200, Christophe JAILLET wrote: > Most of the patches are retated to tegra_output_probe() and missing > tegra_output_remove(). Others are things spotted while writting the serie. > > > Patches 1, 3, 4 are verbose, but some functions called in the probe can > return -EPROBE_DEFER, so it is nice to correctly release resources. > > Maybe moving the tegra_output_probe() call would minimize the changes, but I'm > always reluctant to move code, because of possible side-effects. > > > Christophe JAILLET (6): > drm/tegra: dsi: Fix some error handling paths in tegra_dsi_probe() > drm/tegra: dsi: Fix missing pm_runtime_disable() in the error handling > path of tegra_dsi_probe() > drm/tegra: dsi: Fix some error handling paths in tegra_hdmi_probe() > drm/tegra: rgb: Fix some error handling paths in tegra_dc_rgb_probe() > drm/tegra: rgb: Fix missing clk_put() in the error handling paths of > tegra_dc_rgb_probe() > drm/tegra: output: Fix missing i2c_put_adapter() in the error handling > paths of tegra_output_probe() > > drivers/gpu/drm/tegra/dsi.c| 55 ++ > drivers/gpu/drm/tegra/hdmi.c | 20 - > drivers/gpu/drm/tegra/output.c | 16 +++--- > drivers/gpu/drm/tegra/rgb.c| 18 +++ > 4 files changed, 74 insertions(+), 35 deletions(-) Sorry, this fell through the cracks. Applied now, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH] drm/tegra: dpaux: Fix PM disable depth imbalance in tegra_dpaux_probe
On Wed, Oct 04, 2023 at 10:10:55PM +0800, Zhang Shurong wrote:
> The pm_runtime_enable function increases the power disable depth,
> which means that we must perform a matching decrement on the error
> handling path to maintain balance within the given context.
> Additionally, we need to address the same issue for pm_runtime_get_sync.
> We fix this by invoking pm_runtime_disable and pm_runtime_put_sync
> when error returns.
>
> Fixes: 82b81b3ec1a7 ("drm/tegra: dpaux: Implement runtime PM")
> Signed-off-by: Zhang Shurong
> ---
> drivers/gpu/drm/tegra/dpaux.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
Applied, thanks.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v2] drm/tegra: include drm/drm_edid.h only where needed
On Wed, Dec 13, 2023 at 12:19:51PM +0200, Jani Nikula wrote: > Reduce the need for rebuilds when drm_edid.h is modified by including it > only where needed. > > v2: Fix build (kernel test robot ) > > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/tegra/drm.h| 2 +- > drivers/gpu/drm/tegra/output.c | 1 + > drivers/gpu/drm/tegra/sor.c| 1 + > 3 files changed, 3 insertions(+), 1 deletion(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH] drm/tegra: dsi: Add missing check for of_find_device_by_node
On Tue, Oct 24, 2023 at 08:07:38AM +, Chen Ni wrote:
> Add check for the return value of of_find_device_by_node() and return
> the error if it fails in order to avoid NULL pointer dereference.
>
> Fixes: e94236cde4d5 ("drm/tegra: dsi: Add ganged mode support")
> Signed-off-by: Chen Ni
> ---
> drivers/gpu/drm/tegra/dsi.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Applied, thanks.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] fbdev/simplefb: change loglevel when the power domains cannot be parsed
On Tue, Dec 12, 2023 at 02:57:54PM -0500, Brian Masney wrote:
> When the power domains cannot be parsed, the message is incorrectly
> logged as an info message. Let's change this to an error since an error
> is returned.
>
> Fixes: 92a511a568e4 ("fbdev/simplefb: Add support for generic power-domains")
> Signed-off-by: Brian Masney
> ---
> drivers/video/fbdev/simplefb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH v4 000/115] pwm: Fix lifetime issues for pwm_chips
On Fri, Dec 08, 2023 at 07:50:33PM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Fri, Dec 08, 2023 at 04:41:39PM +0100, Thierry Reding wrote: > > On Wed, Dec 06, 2023 at 12:43:14PM +0100, Uwe Kleine-König wrote: > > > This series is based on Thierry's for-next. > > > > > > It starts with some cleanups that were all sent out separately already: > > > > > > - "pwm: Reduce number of pointer dereferences in pwm_device_request()" > > > > > > https://lore.kernel.org/linux-pwm/20231130072105.966848-1-u.kleine-koe...@pengutronix.de > > > - "pwm: crc: Use consistent variable naming for driver data" > > > > > > https://lore.kernel.org/linux-pwm/20231130074133.969806-1-u.kleine-koe...@pengutronix.de > > > - Two leds/qcom-lpg patches > > > > > > https://lore.kernel.org/linux-leds/20231126095230.683204-1-u.kleine-koe...@pengutronix.de > > >Lee already claimed to have taken the series already, but it's not yet > > > in > > >next. > > > - "leds: qcom-lpg: Introduce a wrapper for getting driver data from a > > > pwm chip" > > > > > > https://lore.kernel.org/linux-leds/20231124215208.616551-3-u.kleine-koe...@pengutronix.de > > > > > > In the following patches I changed: > > > > > > - "pwm: cros-ec: Change prototype of helper to prepare further changes" + > > >This was simplified in response to feedback by Tzung-Bi Shih > > > - Make pwmchip_priv() static (and don't export it), let drivers use a new > > >pwmchip_get_drvdata() instead. > > > - For drm/ti-sn65dsi86.c and leds/qcom-lpg make use of > > >pwmchip_set_drvdata() which makes the conversion to > > >devm_pwmchip_alloc() much prettier. > > > - Some cleanups here and there > > > - Add review tags received in v3 > > >I kept all tags even though the pwmchip_alloc() patches changed > > >slightly. Most of the time that's only > > >s/pwmchip_priv/pwmchip_get_drvdata/ though. Still, if you object, > > >just tell me. (This affects Paul Cercueil on patch #68, Conor Dooley > > >on patch #76 and Greg for patch #109.) > > > > > > I kept the pwmchip_alloc() + pwmchip_register() approach despite Bart > > > not liking it. To balance that out I don't like Bart's alternative > > > approach. There are no technically relevant differences between the two > > > approaches and no benchmarks that show either of the two to be better > > > than the other. Conceptually the design ideas around pwmchip_alloc() + > > > pwmchip_register() are used in several other subsystems, so it's a > > > proven way to do things. > > > > [Trimming the recipients, keeping only Bart and the mailing lists.] > > > > I do think there are technically relevant differences. For one, the > > better we isolate the internal data structure, the easier this becomes > > to manage. I'm attaching a patch that I've prototyped which should > > basically get us to somewhere around patch 110 of this series but with > > something like 1/8th of the changes. It doesn't need every driver to > > change and (mostly) decouples driver code from the core code. > > You don't need to touch all drivers because you didn't change struct > pwm_chip::dev yet. (If you really want, you don't need to change that, > but then you have some duplication as chip->dev holds the same value as > priv->dev.parent in the end.) I don't think that's a problem. These are for two logically separate things, after all. Duplication can also sometimes be useful to simplify things. There are plently of cases where we use local variables for the same reason. > > Now, I know that you think this is all bad because it's not a single > > allocation, but I much prefer the end result because it's got the driver > > and internals much more cleanly separated. Going forward I think it > > would be easier to apply all the ref-counting on top of that because we > > only need to keep the PWM framework-internal data structure alive after > > a PWM chip has gone away. > > Nearly all drivers need driver private data. Isn't it a nice service by > the pwm core to make handling this private data easier for the lowlevel > drivers? That's only true if you think pwmchip_alloc() makes it easier. I happen to think that it doesn't. On the contrary, I think having drivers pass in a PWM chip descriptor that can be embe
Re: [PATCH v4 000/115] pwm: Fix lifetime issues for pwm_chips
On Wed, Dec 06, 2023 at 12:43:14PM +0100, Uwe Kleine-König wrote: > Hello, > > This series is based on Thierry's for-next. > > It starts with some cleanups that were all sent out separately already: > > - "pwm: Reduce number of pointer dereferences in pwm_device_request()" > > https://lore.kernel.org/linux-pwm/20231130072105.966848-1-u.kleine-koe...@pengutronix.de > - "pwm: crc: Use consistent variable naming for driver data" > > https://lore.kernel.org/linux-pwm/20231130074133.969806-1-u.kleine-koe...@pengutronix.de > - Two leds/qcom-lpg patches > > https://lore.kernel.org/linux-leds/20231126095230.683204-1-u.kleine-koe...@pengutronix.de >Lee already claimed to have taken the series already, but it's not yet in >next. > - "leds: qcom-lpg: Introduce a wrapper for getting driver data from a pwm > chip" > > https://lore.kernel.org/linux-leds/20231124215208.616551-3-u.kleine-koe...@pengutronix.de > > In the following patches I changed: > > - "pwm: cros-ec: Change prototype of helper to prepare further changes" + >This was simplified in response to feedback by Tzung-Bi Shih > - Make pwmchip_priv() static (and don't export it), let drivers use a new >pwmchip_get_drvdata() instead. > - For drm/ti-sn65dsi86.c and leds/qcom-lpg make use of >pwmchip_set_drvdata() which makes the conversion to >devm_pwmchip_alloc() much prettier. > - Some cleanups here and there > - Add review tags received in v3 >I kept all tags even though the pwmchip_alloc() patches changed >slightly. Most of the time that's only >s/pwmchip_priv/pwmchip_get_drvdata/ though. Still, if you object, >just tell me. (This affects Paul Cercueil on patch #68, Conor Dooley >on patch #76 and Greg for patch #109.) > > I kept the pwmchip_alloc() + pwmchip_register() approach despite Bart > not liking it. To balance that out I don't like Bart's alternative > approach. There are no technically relevant differences between the two > approaches and no benchmarks that show either of the two to be better > than the other. Conceptually the design ideas around pwmchip_alloc() + > pwmchip_register() are used in several other subsystems, so it's a > proven way to do things. [Trimming the recipients, keeping only Bart and the mailing lists.] I do think there are technically relevant differences. For one, the better we isolate the internal data structure, the easier this becomes to manage. I'm attaching a patch that I've prototyped which should basically get us to somewhere around patch 110 of this series but with something like 1/8th of the changes. It doesn't need every driver to change and (mostly) decouples driver code from the core code. Now, I know that you think this is all bad because it's not a single allocation, but I much prefer the end result because it's got the driver and internals much more cleanly separated. Going forward I think it would be easier to apply all the ref-counting on top of that because we only need to keep the PWM framework-internal data structure alive after a PWM chip has gone away. Thierry From 72ea79887d96850f9ccc832ce52b907ca276c940 Mon Sep 17 00:00:00 2001 From: Thierry Reding Date: Tue, 28 Nov 2023 15:42:39 +0100 Subject: [PATCH] pwm: Isolate internal data into a separate structure In order to prepare for proper reference counting of PWM chips and PWM devices, move the internal data from the public PWM chip to a private PWM chip structure. This ensures that the data that the subsystem core may need to reference later on can stick around beyond the lifetime of the driver-private data. Signed-off-by: Thierry Reding --- drivers/pwm/core.c| 185 +- drivers/pwm/internal.h| 38 +++ drivers/pwm/pwm-atmel-hlcdc.c | 8 +- drivers/pwm/pwm-fsl-ftm.c | 6 +- drivers/pwm/pwm-lpc18xx-sct.c | 4 +- drivers/pwm/pwm-lpss.c| 14 +-- drivers/pwm/pwm-pca9685.c | 6 +- drivers/pwm/pwm-samsung.c | 6 +- drivers/pwm/pwm-sifive.c | 4 +- drivers/pwm/pwm-stm32-lp.c| 6 +- drivers/pwm/pwm-stm32.c | 6 +- drivers/pwm/pwm-tiecap.c | 6 +- drivers/pwm/pwm-tiehrpwm.c| 6 +- drivers/pwm/sysfs.c | 48 - include/linux/pwm.h | 26 + 15 files changed, 228 insertions(+), 141 deletions(-) create mode 100644 drivers/pwm/internal.h diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index f60b715abe62..54d57dec6dce 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -24,17 +24,19 @@ #define CREATE_TRACE_POINTS #include +#include "internal.h" + static DEFINE_MUTEX(pwm_lookup_lock); static LIST_HEAD(pwm_lookup_list); -/* protects access to pwmchip_idr */ +/* protects acce
[PATCH] drm/nouveau: Fixup gk20a instobj hierarchy
From: Thierry Reding
Commit 12c9b05da918 ("drm/nouveau/imem: support allocations not
preserved across suspend") uses container_of() to cast from struct
nvkm_memory to struct nvkm_instobj, assuming that all instance objects
are derived from struct nvkm_instobj. For the gk20a family that's not
the case and they are derived from struct nvkm_memory instead. This
causes some subtle data corruption (nvkm_instobj.preserve ends up
mapping to gk20a_instobj.vaddr) that causes a NULL pointer dereference
in gk20a_instobj_acquire_iommu() (and possibly elsewhere) and also
prevents suspend/resume from working.
Fix this by making struct gk20a_instobj derive from struct nvkm_instobj
instead.
Fixes: 12c9b05da918 ("drm/nouveau/imem: support allocations not preserved
across suspend")
Reported-by: Jonathan Hunter
Signed-off-by: Thierry Reding
---
Note that this was probably subtly wrong before the above-mentioned
commit already, but I don't think we've seen any reports that would
indicate any actual failures related to this before. So I think it's
good enough to apply this fix for v6.7. The next closest thing would
be commit d8e83994aaf6 ("drm/nouveau/imem: improve management of
instance memory"), but that's 8 years old (Linux v4.3)...
---
.../drm/nouveau/nvkm/subdev/instmem/gk20a.c| 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
index 1b811d6972a1..201022ae9214 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
@@ -49,14 +49,14 @@
#include
struct gk20a_instobj {
- struct nvkm_memory memory;
+ struct nvkm_instobj base;
struct nvkm_mm_node *mn;
struct gk20a_instmem *imem;
/* CPU mapping */
u32 *vaddr;
};
-#define gk20a_instobj(p) container_of((p), struct gk20a_instobj, memory)
+#define gk20a_instobj(p) container_of((p), struct gk20a_instobj, base.memory)
/*
* Used for objects allocated using the DMA API
@@ -148,7 +148,7 @@ gk20a_instobj_iommu_recycle_vaddr(struct
gk20a_instobj_iommu *obj)
list_del(&obj->vaddr_node);
vunmap(obj->base.vaddr);
obj->base.vaddr = NULL;
- imem->vaddr_use -= nvkm_memory_size(&obj->base.memory);
+ imem->vaddr_use -= nvkm_memory_size(&obj->base.base.memory);
nvkm_debug(&imem->base.subdev, "vaddr used: %x/%x\n", imem->vaddr_use,
imem->vaddr_max);
}
@@ -283,7 +283,7 @@ gk20a_instobj_map(struct nvkm_memory *memory, u64 offset,
struct nvkm_vmm *vmm,
{
struct gk20a_instobj *node = gk20a_instobj(memory);
struct nvkm_vmm_map map = {
- .memory = &node->memory,
+ .memory = &node->base.memory,
.offset = offset,
.mem = node->mn,
};
@@ -391,8 +391,8 @@ gk20a_instobj_ctor_dma(struct gk20a_instmem *imem, u32
npages, u32 align,
return -ENOMEM;
*_node = &node->base;
- nvkm_memory_ctor(&gk20a_instobj_func_dma, &node->base.memory);
- node->base.memory.ptrs = &gk20a_instobj_ptrs;
+ nvkm_memory_ctor(&gk20a_instobj_func_dma, &node->base.base.memory);
+ node->base.base.memory.ptrs = &gk20a_instobj_ptrs;
node->base.vaddr = dma_alloc_attrs(dev, npages << PAGE_SHIFT,
&node->handle, GFP_KERNEL,
@@ -438,8 +438,8 @@ gk20a_instobj_ctor_iommu(struct gk20a_instmem *imem, u32
npages, u32 align,
*_node = &node->base;
node->dma_addrs = (void *)(node->pages + npages);
- nvkm_memory_ctor(&gk20a_instobj_func_iommu, &node->base.memory);
- node->base.memory.ptrs = &gk20a_instobj_ptrs;
+ nvkm_memory_ctor(&gk20a_instobj_func_iommu, &node->base.base.memory);
+ node->base.base.memory.ptrs = &gk20a_instobj_ptrs;
/* Allocate backing memory */
for (i = 0; i < npages; i++) {
@@ -533,7 +533,7 @@ gk20a_instobj_new(struct nvkm_instmem *base, u32 size, u32
align, bool zero,
else
ret = gk20a_instobj_ctor_dma(imem, size >> PAGE_SHIFT,
align, &node);
- *pmemory = node ? &node->memory : NULL;
+ *pmemory = node ? &node->base.memory : NULL;
if (ret)
return ret;
--
2.43.0
Re: (subset) [PATCH 00/17] dt-bindings: samsung: add specific compatibles for existing SoC
On Tue, Nov 28, 2023 at 09:58:41PM +0100, Uwe Kleine-König wrote: > On Tue, Nov 28, 2023 at 06:49:23PM +0100, Thierry Reding wrote: > > > > On Wed, 08 Nov 2023 11:43:26 +0100, Krzysztof Kozlowski wrote: > > > Merging > > > === > > > I propose to take entire patchset through my tree (Samsung SoC), because: > ^^^ > > > > 1. Next cycle two new SoCs will be coming (Google GS101 and > > > ExynosAutov920), so > > >they will touch the same lines in some of the DT bindings (not all, > > > though). > > >It is reasonable for me to take the bindings for the new SoCs, to have > > > clean > > >`make dtbs_check` on the new DTS. > > > 2. Having it together helps me to have clean `make dtbs_check` within my > > > tree > > >on the existing DTS. > > > 3. No drivers are affected by this change. > > > 4. I plan to do the same for Tesla FSD and Exynos ARM32 SoCs, thus expect > > >follow up patchsets. > > > > > > [...] > > > > Applied, thanks! > > > > [12/17] dt-bindings: pwm: samsung: add specific compatibles for existing SoC > > commit: 5d67b8f81b9d598599366214e3b2eb5f84003c9f > > You didn't honor (or even comment) Krzysztof's proposal to take the > whole patchset via his tree (marked above). Was there some off-list > agreement? I had read all that and then looking at patchwork saw that you had marked all other patches in the series as "handled-elsewhere" and only this one was left as "new", so I assumed that, well, everything else was handled elsewhere and I was supposed to pick this one up... I'll drop this one. Thierry signature.asc Description: PGP signature
Re: [PATCH 08/10] iommu/tegra: Use tegra_dev_iommu_get_stream_id() in the remaining places
On Wed, Nov 29, 2023 at 03:26:03PM -0400, Jason Gunthorpe wrote:
> On Wed, Nov 29, 2023 at 05:23:13PM +0100, Thierry Reding wrote:
> > > diff --git a/drivers/memory/tegra/tegra186.c
> > > b/drivers/memory/tegra/tegra186.c
> > > index 533f85a4b2bdb7..3e4fbe94dd666e 100644
> > > --- a/drivers/memory/tegra/tegra186.c
> > > +++ b/drivers/memory/tegra/tegra186.c
> > > @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(struct
> > > tegra_mc *mc,
> > > static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device
> > > *dev)
> > > {
> > > #if IS_ENABLED(CONFIG_IOMMU_API)
> > > - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > > struct of_phandle_args args;
> > > unsigned int i, index = 0;
> > > + u32 sid;
> > >
> > > + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid));
> >
> > I know the code previously didn't check for any errors, but we may want
> > to do so now. If tegra_dev_iommu_get_stream_id() ever fails we may end
> > up writing some undefined value into the override register.
>
> My assumption was it never fails otherwise this probably already
> doesn't work?
I guess the point I was trying to make is that previously we would not
have written anything to the stream ID register and so ignoring the
error here might end up writing to a register that previously we would
not have written to. Looking at the current code more closely I see now
that the reason why we wouldn't have written to the register is because
we would've crashed before.
So I think this okay.
>
> > I'm also unsure if WARN_ON() is appropriate here. I vaguely recall that
> > ->probe_device() was called for all devices on the bus and not all of
> > them may have been associated with the IOMMU. Not all of them may in
> > fact access memory in the first place.
>
> So you are thinkin that of_parse_phandle_with_args() is a NOP
> sometimes so it will tolerate the failure?
>
> Seems like the best thing to do is just continue to ignore it then?
Yeah, exactly. It would've just skipped over everything, basically.
> > Perhaps I'm misremembering and the IOMMU core now takes care of only
> > calling this when fwspec is indeed valid?
>
> Can't advise, I have no idea what tegra_mc_ops is for :)
In a nutshell, it's a hook that allows us to configure the memory
controller when a device is attached to the IOMMU. The memory controller
contains a set of registers that specify which memory client uses which
stream ID by default. For some devices this can be overridden (which is
where tegra_dev_iommu_get_stream_id() comes into play in those drivers)
and for other devices we can't override, which is when the memory
controller defaults come into play.
Anyway, I took a closer look at this and ran some tests. Turns out that
tegra186_mc_probe_device() really only gets called for devices that have
their fwspec properly initialized anyway, so I don't think there's
anything special we need to do here.
Strictly from a static analysis point of view I suppose we could now
have a situation that sid is uninitialized when the call to
tegra_dev_iommu_get_stream_id() fails and so using it in the loop is not
correct, theoretically, but I think that's just not a case that we'll
ever hit in practice.
So either way is fine with me. I have a slight preference for just
returning 0 in case tegra_dev_iommu_get_stream_id() fails, because it's
simple to do and avoids any of these (theoretical) ambiguities. So
whichever way you decide:
Reviewed-by: Thierry Reding
signature.asc
Description: PGP signature
Re: [PATCH 08/10] iommu/tegra: Use tegra_dev_iommu_get_stream_id() in the remaining places
On Tue, Nov 28, 2023 at 08:48:04PM -0400, Jason Gunthorpe wrote:
> This API was defined to formalize the access to internal iommu details on
> some Tegra SOCs, but a few callers got missed. Add them.
>
> The helper already masks by 0x so remove this code from the callers.
>
> Suggested-by: Thierry Reding
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/dma/tegra186-gpc-dma.c | 8 +++-
> drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 7 ++-
> drivers/memory/tegra/tegra186.c | 12 ++--
> 3 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
> index fa4d4142a68a21..88547a23825b18 100644
> --- a/drivers/dma/tegra186-gpc-dma.c
> +++ b/drivers/dma/tegra186-gpc-dma.c
> @@ -1348,8 +1348,8 @@ static int tegra_dma_program_sid(struct
> tegra_dma_channel *tdc, int stream_id)
> static int tegra_dma_probe(struct platform_device *pdev)
> {
> const struct tegra_dma_chip_data *cdata = NULL;
> - struct iommu_fwspec *iommu_spec;
> - unsigned int stream_id, i;
> + unsigned int i;
> + u32 stream_id;
> struct tegra_dma *tdma;
> int ret;
>
> @@ -1378,12 +1378,10 @@ static int tegra_dma_probe(struct platform_device
> *pdev)
>
> tdma->dma_dev.dev = &pdev->dev;
>
> - iommu_spec = dev_iommu_fwspec_get(&pdev->dev);
> - if (!iommu_spec) {
> + if (!tegra_dev_iommu_get_stream_id(&pdev->dev, &stream_id)) {
> dev_err(&pdev->dev, "Missing iommu stream-id\n");
> return -EINVAL;
> }
> - stream_id = iommu_spec->ids[0] & 0x;
>
> ret = device_property_read_u32(&pdev->dev, "dma-channel-mask",
> &tdma->chan_mask);
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c
> index e7e8fdf3adab7a..b40fd1dbb21617 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c
> @@ -28,16 +28,13 @@ static void
> gp10b_ltc_init(struct nvkm_ltc *ltc)
> {
> struct nvkm_device *device = ltc->subdev.device;
> - struct iommu_fwspec *spec;
> + u32 sid;
>
> nvkm_wr32(device, 0x17e27c, ltc->ltc_nr);
> nvkm_wr32(device, 0x17e000, ltc->ltc_nr);
> nvkm_wr32(device, 0x100800, ltc->ltc_nr);
>
> - spec = dev_iommu_fwspec_get(device->dev);
> - if (spec) {
> - u32 sid = spec->ids[0] & 0x;
> -
> + if (tegra_dev_iommu_get_stream_id(device->dev, &sid)) {
> /* stream ID */
> nvkm_wr32(device, 0x16, sid << 2);
We could probably also remove the comment now since the function and
variable names make it obvious what's being written here.
> }
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index 533f85a4b2bdb7..3e4fbe94dd666e 100644
> --- a/drivers/memory/tegra/tegra186.c
> +++ b/drivers/memory/tegra/tegra186.c
> @@ -111,21 +111,21 @@ static void tegra186_mc_client_sid_override(struct
> tegra_mc *mc,
> static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
> {
> #if IS_ENABLED(CONFIG_IOMMU_API)
> - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct of_phandle_args args;
> unsigned int i, index = 0;
> + u32 sid;
>
> + WARN_ON(!tegra_dev_iommu_get_stream_id(dev, &sid));
I know the code previously didn't check for any errors, but we may want
to do so now. If tegra_dev_iommu_get_stream_id() ever fails we may end
up writing some undefined value into the override register.
I'm also unsure if WARN_ON() is appropriate here. I vaguely recall that
->probe_device() was called for all devices on the bus and not all of
them may have been associated with the IOMMU. Not all of them may in
fact access memory in the first place.
Perhaps I'm misremembering and the IOMMU core now takes care of only
calling this when fwspec is indeed valid?
Thierry
signature.asc
Description: PGP signature
Re: (subset) [PATCH 00/17] dt-bindings: samsung: add specific compatibles for existing SoC
On Wed, 08 Nov 2023 11:43:26 +0100, Krzysztof Kozlowski wrote: > Merging > === > I propose to take entire patchset through my tree (Samsung SoC), because: > 1. Next cycle two new SoCs will be coming (Google GS101 and ExynosAutov920), > so >they will touch the same lines in some of the DT bindings (not all, > though). >It is reasonable for me to take the bindings for the new SoCs, to have > clean >`make dtbs_check` on the new DTS. > 2. Having it together helps me to have clean `make dtbs_check` within my tree >on the existing DTS. > 3. No drivers are affected by this change. > 4. I plan to do the same for Tesla FSD and Exynos ARM32 SoCs, thus expect >follow up patchsets. > > [...] Applied, thanks! [12/17] dt-bindings: pwm: samsung: add specific compatibles for existing SoC commit: 5d67b8f81b9d598599366214e3b2eb5f84003c9f Best regards, -- Thierry Reding
Re: [PATCH v5 1/4] pwm: rename pwm_apply_state() to pwm_apply_cansleep()
On Sat, Nov 18, 2023 at 04:16:17PM +, Sean Young wrote: > In order to introduce a pwm api which can be used from atomic context, > we will need two functions for applying pwm changes: > > int pwm_apply_cansleep(struct pwm *, struct pwm_state *); > int pwm_apply_atomic(struct pwm *, struct pwm_state *); > > This commit just deals with renaming pwm_apply_state(), a following > commit will introduce the pwm_apply_atomic() function. Sorry, I still don't agree with that _cansleep suffix. I think it's the wrong terminology. Just because something can sleep doesn't mean that it ever will. "Might sleep" is much more accurate because it says exactly what might happen and indicates what we're guarding against. Thierry signature.asc Description: PGP signature
[PATCH v2 2/2] fbdev/simplefb: Add support for generic power-domains
From: Thierry Reding
The simple-framebuffer device tree bindings document the power-domains
property, so make sure that simplefb supports it. This ensures that the
power domains remain enabled as long as simplefb is active.
v2: - remove unnecessary call to simplefb_detach_genpds() since that's
already done automatically by devres
- fix crash if power-domains property is missing in DT
Signed-off-by: Thierry Reding
---
drivers/video/fbdev/simplefb.c | 93 ++
1 file changed, 93 insertions(+)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 18025f34fde7..fe682af63827 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#include
static const struct fb_fix_screeninfo simplefb_fix = {
@@ -78,6 +79,11 @@ struct simplefb_par {
unsigned int clk_count;
struct clk **clks;
#endif
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+ unsigned int num_genpds;
+ struct device **genpds;
+ struct device_link **genpd_links;
+#endif
#if defined CONFIG_OF && defined CONFIG_REGULATOR
bool regulators_enabled;
u32 regulator_count;
@@ -432,6 +438,89 @@ static void simplefb_regulators_enable(struct simplefb_par
*par,
static void simplefb_regulators_destroy(struct simplefb_par *par) { }
#endif
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+static void simplefb_detach_genpds(void *res)
+{
+ struct simplefb_par *par = res;
+ unsigned int i = par->num_genpds;
+
+ if (par->num_genpds <= 1)
+ return;
+
+ while (i--) {
+ if (par->genpd_links[i])
+ device_link_del(par->genpd_links[i]);
+
+ if (!IS_ERR_OR_NULL(par->genpds[i]))
+ dev_pm_domain_detach(par->genpds[i], true);
+ }
+}
+
+static int simplefb_attach_genpds(struct simplefb_par *par,
+ struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ unsigned int i;
+ int err;
+
+ err = of_count_phandle_with_args(dev->of_node, "power-domains",
+"#power-domain-cells");
+ if (err < 0) {
+ dev_info(dev, "failed to parse power-domains: %d\n", err);
+ return err;
+ }
+
+ par->num_genpds = err;
+
+ /*
+* Single power-domain devices are handled by the driver core, so
+* nothing to do here.
+*/
+ if (par->num_genpds <= 1)
+ return 0;
+
+ par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
+ GFP_KERNEL);
+ if (!par->genpds)
+ return -ENOMEM;
+
+ par->genpd_links = devm_kcalloc(dev, par->num_genpds,
+ sizeof(*par->genpd_links),
+ GFP_KERNEL);
+ if (!par->genpd_links)
+ return -ENOMEM;
+
+ for (i = 0; i < par->num_genpds; i++) {
+ par->genpds[i] = dev_pm_domain_attach_by_id(dev, i);
+ if (IS_ERR(par->genpds[i])) {
+ err = PTR_ERR(par->genpds[i]);
+ if (err == -EPROBE_DEFER) {
+ simplefb_detach_genpds(par);
+ return err;
+ }
+
+ dev_warn(dev, "failed to attach domain %u: %d\n", i,
err);
+ continue;
+ }
+
+ par->genpd_links[i] = device_link_add(dev, par->genpds[i],
+ DL_FLAG_STATELESS |
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_RPM_ACTIVE);
+ if (!par->genpd_links[i])
+ dev_warn(dev, "failed to link power-domain %u\n", i);
+ }
+
+ return devm_add_action_or_reset(dev, simplefb_detach_genpds, par);
+}
+#else
+static int simplefb_attach_genpds(struct simplefb_par *par,
+ struct platform_device *pdev)
+{
+ return 0;
+}
+#endif
+
static int simplefb_probe(struct platform_device *pdev)
{
int ret;
@@ -518,6 +607,10 @@ static int simplefb_probe(struct platform_device *pdev)
if (ret < 0)
goto error_clocks;
+ ret = simplefb_attach_genpds(par, pdev);
+ if (ret < 0)
+ goto error_regulators;
+
simplefb_clocks_enable(par, pdev);
simplefb_regulators_enable(par, pdev);
--
2.42.0
[PATCH v2 1/2] fbdev/simplefb: Support memory-region property
From: Thierry Reding
The simple-framebuffer bindings specify that the "memory-region"
property can be used as an alternative to the "reg" property to define
the framebuffer memory used by the display hardware. Implement support
for this in the simplefb driver.
Reviewed-by: Hans de Goede
Signed-off-by: Thierry Reding
---
drivers/video/fbdev/simplefb.c | 35 +-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 62f99f6fccd3..18025f34fde7 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -121,12 +122,13 @@ struct simplefb_params {
u32 height;
u32 stride;
struct simplefb_format *format;
+ struct resource memory;
};
static int simplefb_parse_dt(struct platform_device *pdev,
struct simplefb_params *params)
{
- struct device_node *np = pdev->dev.of_node;
+ struct device_node *np = pdev->dev.of_node, *mem;
int ret;
const char *format;
int i;
@@ -166,6 +168,23 @@ static int simplefb_parse_dt(struct platform_device *pdev,
return -EINVAL;
}
+ mem = of_parse_phandle(np, "memory-region", 0);
+ if (mem) {
+ ret = of_address_to_resource(mem, 0, ¶ms->memory);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to parse memory-region\n");
+ of_node_put(mem);
+ return ret;
+ }
+
+ if (of_property_present(np, "reg"))
+ dev_warn(&pdev->dev, "preferring \"memory-region\" over
\"reg\" property\n");
+
+ of_node_put(mem);
+ } else {
+ memset(¶ms->memory, 0, sizeof(params->memory));
+ }
+
return 0;
}
@@ -193,6 +212,8 @@ static int simplefb_parse_pd(struct platform_device *pdev,
return -EINVAL;
}
+ memset(¶ms->memory, 0, sizeof(params->memory));
+
return 0;
}
@@ -431,10 +452,14 @@ static int simplefb_probe(struct platform_device *pdev)
if (ret)
return ret;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(&pdev->dev, "No memory resource\n");
- return -EINVAL;
+ if (params.memory.start == 0 && params.memory.end == 0) {
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "No memory resource\n");
+ return -EINVAL;
+ }
+ } else {
+ res = ¶ms.memory;
}
mem = request_mem_region(res->start, resource_size(res), "simplefb");
--
2.42.0
[PATCH v2 0/2] fbdev/simplefb: Add missing simple-framebuffer features
From: Thierry Reding Hi, This contains two patches that bring simplefb up to feature parity with simpledrm. The patches add support for the "memory-region" property that provides an alternative to the "reg" property to describe the memory used for the framebuffer and allow attaching the simple-framebuffer device to one or more generic power domains to make sure they aren't turned off during the boot process and take down the display configuration. Changes in v2: - remove unnecessary call to simplefb_detach_genpds() since that's already done automatically by devres - fix crash if power-domains property is missing in DT Thanks, Thierry Thierry Reding (2): fbdev/simplefb: Support memory-region property fbdev/simplefb: Add support for generic power-domains drivers/video/fbdev/simplefb.c | 128 +++-- 1 file changed, 123 insertions(+), 5 deletions(-) -- 2.42.0
Re: [PATCH 2/2] fbdev/simplefb: Add support for generic power-domains
On Thu, Oct 26, 2023 at 02:50:27PM +0200, Hans de Goede wrote:
> Hi,
>
> Thank you for your patches.
>
> On 10/11/23 16:38, Thierry Reding wrote:
> > From: Thierry Reding
> >
> > The simple-framebuffer device tree bindings document the power-domains
> > property, so make sure that simplefb supports it. This ensures that the
> > power domains remain enabled as long as simplefb is active.
> >
> > Signed-off-by: Thierry Reding
> > ---
> > drivers/video/fbdev/simplefb.c | 93 +-
> > 1 file changed, 91 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> > index 18025f34fde7..e69fb0ad2d54 100644
> > --- a/drivers/video/fbdev/simplefb.c
> > +++ b/drivers/video/fbdev/simplefb.c
> > @@ -25,6 +25,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > static const struct fb_fix_screeninfo simplefb_fix = {
> > @@ -78,6 +79,11 @@ struct simplefb_par {
> > unsigned int clk_count;
> > struct clk **clks;
> > #endif
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > + unsigned int num_genpds;
> > + struct device **genpds;
> > + struct device_link **genpd_links;
> > +#endif
> > #if defined CONFIG_OF && defined CONFIG_REGULATOR
> > bool regulators_enabled;
> > u32 regulator_count;
> > @@ -432,6 +438,83 @@ static void simplefb_regulators_enable(struct
> > simplefb_par *par,
> > static void simplefb_regulators_destroy(struct simplefb_par *par) { }
> > #endif
> >
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > +static void simplefb_detach_genpds(void *res)
> > +{
> > + struct simplefb_par *par = res;
> > + unsigned int i = par->num_genpds;
> > +
> > + if (par->num_genpds <= 1)
> > + return;
> > +
> > + while (i--) {
> > + if (par->genpd_links[i])
> > + device_link_del(par->genpd_links[i]);
> > +
> > + if (!IS_ERR_OR_NULL(par->genpds[i]))
> > + dev_pm_domain_detach(par->genpds[i], true);
> > + }
>
> Using this i-- construct means that genpd at index 0 will
> not be cleaned up.
This is actually a common variant to clean up in reverse order. You'll
find this used a lot in core code and so on. It has the advantage that
you can use it to unwind (not the case here) because i will already be
set to the right value, typically. It's also nice because it works for
unsigned integers.
Note that this uses the postfix decrement, so evaluation happens before
the decrement and therefore the last iteration of the loop will run with
i == 0. For unsigned integers this also means that after the loop the
variable will actually have wrapped around, but that's usually not a
problem since it isn't used after this point anymore.
> > static int simplefb_probe(struct platform_device *pdev)
> > {
> > int ret;
> > @@ -518,6 +601,10 @@ static int simplefb_probe(struct platform_device *pdev)
> > if (ret < 0)
> > goto error_clocks;
> >
> > + ret = simplefb_attach_genpd(par, pdev);
> > + if (ret < 0)
> > + goto error_regulators;
> > +
> > simplefb_clocks_enable(par, pdev);
> > simplefb_regulators_enable(par, pdev);
> >
> > @@ -534,18 +621,20 @@ static int simplefb_probe(struct platform_device
> > *pdev)
> > ret = devm_aperture_acquire_for_platform_device(pdev, par->base,
> > par->size);
> > if (ret) {
> > dev_err(&pdev->dev, "Unable to acquire aperture: %d\n", ret);
> > - goto error_regulators;
> > + goto error_genpds;
>
> This is not necessary since simplefb_attach_genpd() ends with:
>
> devm_add_action_or_reset(dev, simplefb_detach_genpds, par)
>
> Which causes simplefb_detach_genpds() to run when probe() fails.
Yes, you're right. I've removed all these explicit cleanup paths since
they are not needed.
>
> > }
> > ret = register_framebuffer(info);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
> > - goto error_regulators;
> > + goto error_genpds;
> > }
> >
> > dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
> >
> > return 0;
> >
> > +error_genpds:
> > + simplefb_detach_genpds(par);
>
> As the kernel test robot (LKP) already pointed out this is causing
> compile errors when #if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> evaluates as false.
>
> Since there is no simplefb_detach_genpds() stub in the #else, but as
> mentioned above this is not necessary so just remove it.
Yep, done.
Thanks,
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 1/2] gpu: host1x: Correct allocated size for contexts
On Fri, Sep 01, 2023 at 02:59:09PM +0300, Mikko Perttunen wrote:
> From: Johnny Liu
>
> Original implementation over allocates the memory size for the
> contexts list. The size of memory for the contexts list is based
> on the number of iommu groups specified in the device tree.
>
> Fixes: 8aa5bcb61612 ("gpu: host1x: Add context device management code")
> Signed-off-by: Johnny Liu
> Signed-off-by: Mikko Perttunen
> ---
> drivers/gpu/host1x/context.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Both patches applied, thanks.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] gpu: host1x: Syncpoint interrupt sharding
On Fri, Sep 01, 2023 at 02:40:07PM +0300, Mikko Perttunen wrote: > From: Mikko Perttunen > > Support sharded syncpoint interrupts on Tegra234+. This feature > allows specifying one of eight interrupt lines for each syncpoint > to lower processing latency of syncpoint threshold > interrupts. > > Signed-off-by: Mikko Perttunen > --- > drivers/gpu/host1x/dev.c| 28 +--- > drivers/gpu/host1x/dev.h| 3 ++- > drivers/gpu/host1x/hw/intr_hw.c | 46 - > 3 files changed, 60 insertions(+), 17 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH 1/3] gpu: host1x: Add locking in channel allocation
On Fri, Sep 01, 2023 at 02:15:07PM +0300, Mikko Perttunen wrote: > From: Mikko Perttunen > > Add locking around channel allocation to avoid race conditions. > > Signed-off-by: Mikko Perttunen > --- > drivers/gpu/host1x/channel.c | 7 +++ > drivers/gpu/host1x/channel.h | 3 +++ > 2 files changed, 10 insertions(+) All three patches applied, thanks. Thierry signature.asc Description: PGP signature
[PATCH 1/2] fbdev/simplefb: Support memory-region property
From: Thierry Reding
The simple-framebuffer bindings specify that the "memory-region"
property can be used as an alternative to the "reg" property to define
the framebuffer memory used by the display hardware. Implement support
for this in the simplefb driver.
Signed-off-by: Thierry Reding
---
drivers/video/fbdev/simplefb.c | 35 +-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 62f99f6fccd3..18025f34fde7 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -121,12 +122,13 @@ struct simplefb_params {
u32 height;
u32 stride;
struct simplefb_format *format;
+ struct resource memory;
};
static int simplefb_parse_dt(struct platform_device *pdev,
struct simplefb_params *params)
{
- struct device_node *np = pdev->dev.of_node;
+ struct device_node *np = pdev->dev.of_node, *mem;
int ret;
const char *format;
int i;
@@ -166,6 +168,23 @@ static int simplefb_parse_dt(struct platform_device *pdev,
return -EINVAL;
}
+ mem = of_parse_phandle(np, "memory-region", 0);
+ if (mem) {
+ ret = of_address_to_resource(mem, 0, ¶ms->memory);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to parse memory-region\n");
+ of_node_put(mem);
+ return ret;
+ }
+
+ if (of_property_present(np, "reg"))
+ dev_warn(&pdev->dev, "preferring \"memory-region\" over
\"reg\" property\n");
+
+ of_node_put(mem);
+ } else {
+ memset(¶ms->memory, 0, sizeof(params->memory));
+ }
+
return 0;
}
@@ -193,6 +212,8 @@ static int simplefb_parse_pd(struct platform_device *pdev,
return -EINVAL;
}
+ memset(¶ms->memory, 0, sizeof(params->memory));
+
return 0;
}
@@ -431,10 +452,14 @@ static int simplefb_probe(struct platform_device *pdev)
if (ret)
return ret;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(&pdev->dev, "No memory resource\n");
- return -EINVAL;
+ if (params.memory.start == 0 && params.memory.end == 0) {
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "No memory resource\n");
+ return -EINVAL;
+ }
+ } else {
+ res = ¶ms.memory;
}
mem = request_mem_region(res->start, resource_size(res), "simplefb");
--
2.42.0
[PATCH 2/2] fbdev/simplefb: Add support for generic power-domains
From: Thierry Reding
The simple-framebuffer device tree bindings document the power-domains
property, so make sure that simplefb supports it. This ensures that the
power domains remain enabled as long as simplefb is active.
Signed-off-by: Thierry Reding
---
drivers/video/fbdev/simplefb.c | 93 +-
1 file changed, 91 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 18025f34fde7..e69fb0ad2d54 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#include
static const struct fb_fix_screeninfo simplefb_fix = {
@@ -78,6 +79,11 @@ struct simplefb_par {
unsigned int clk_count;
struct clk **clks;
#endif
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+ unsigned int num_genpds;
+ struct device **genpds;
+ struct device_link **genpd_links;
+#endif
#if defined CONFIG_OF && defined CONFIG_REGULATOR
bool regulators_enabled;
u32 regulator_count;
@@ -432,6 +438,83 @@ static void simplefb_regulators_enable(struct simplefb_par
*par,
static void simplefb_regulators_destroy(struct simplefb_par *par) { }
#endif
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+static void simplefb_detach_genpds(void *res)
+{
+ struct simplefb_par *par = res;
+ unsigned int i = par->num_genpds;
+
+ if (par->num_genpds <= 1)
+ return;
+
+ while (i--) {
+ if (par->genpd_links[i])
+ device_link_del(par->genpd_links[i]);
+
+ if (!IS_ERR_OR_NULL(par->genpds[i]))
+ dev_pm_domain_detach(par->genpds[i], true);
+ }
+}
+
+static int simplefb_attach_genpd(struct simplefb_par *par,
+struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ unsigned int i;
+ int err;
+
+ par->num_genpds = of_count_phandle_with_args(dev->of_node,
+"power-domains",
+"#power-domain-cells");
+ /*
+* Single power-domain devices are handled by the driver core, so
+* nothing to do here.
+*/
+ if (par->num_genpds <= 1)
+ return 0;
+
+ par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds),
+ GFP_KERNEL);
+ if (!par->genpds)
+ return -ENOMEM;
+
+ par->genpd_links = devm_kcalloc(dev, par->num_genpds,
+ sizeof(*par->genpd_links),
+ GFP_KERNEL);
+ if (!par->genpd_links)
+ return -ENOMEM;
+
+ for (i = 0; i < par->num_genpds; i++) {
+ par->genpds[i] = dev_pm_domain_attach_by_id(dev, i);
+ if (IS_ERR(par->genpds[i])) {
+ err = PTR_ERR(par->genpds[i]);
+ if (err == -EPROBE_DEFER) {
+ simplefb_detach_genpds(par);
+ return err;
+ }
+
+ dev_warn(dev, "failed to attach domain %u: %d\n", i,
err);
+ continue;
+ }
+
+ par->genpd_links[i] = device_link_add(dev, par->genpds[i],
+ DL_FLAG_STATELESS |
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_RPM_ACTIVE);
+ if (!par->genpd_links[i])
+ dev_warn(dev, "failed to link power-domain %u\n", i);
+ }
+
+ return devm_add_action_or_reset(dev, simplefb_detach_genpds, par);
+}
+#else
+static int simplefb_attach_genpd(struct simplefb_par *par,
+struct platform_device *pdev)
+{
+ return 0;
+}
+#endif
+
static int simplefb_probe(struct platform_device *pdev)
{
int ret;
@@ -518,6 +601,10 @@ static int simplefb_probe(struct platform_device *pdev)
if (ret < 0)
goto error_clocks;
+ ret = simplefb_attach_genpd(par, pdev);
+ if (ret < 0)
+ goto error_regulators;
+
simplefb_clocks_enable(par, pdev);
simplefb_regulators_enable(par, pdev);
@@ -534,18 +621,20 @@ static int simplefb_probe(struct platform_device *pdev)
ret = devm_aperture_acquire_for_platform_device(pdev, par->base,
par->size);
if (ret) {
dev_err(&pdev->dev, "Unable to acquire aperture: %d\n", ret);
- goto error_regulators;
+ goto error_genpds;
}
[PATCH 0/2] fbdev/simplefb: Add missing simple-framebuffer features
From: Thierry Reding Hi, This contains two patches that bring simplefb up to feature parity with simpledrm. The patches add support for the "memory-region" property that provides an alternative to the "reg" property to describe the memory used for the framebuffer and allow attaching the simple-framebuffer device to one or more generic power domains to make sure they aren't turned off during the boot process and take down the display configuration. Thanks, Thierry Thierry Reding (2): fbdev/simplefb: Support memory-region property fbdev/simplefb: Add support for generic power-domains drivers/video/fbdev/simplefb.c | 128 +++-- 1 file changed, 121 insertions(+), 7 deletions(-) -- 2.42.0
[PATCH] drm/simpledrm: Fix power domain device link validity check
From: Thierry Reding
We need to check if a link is non-NULL before trying to delete it.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/tiny/simpledrm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 9c597461d1e2..8bdaf66044fc 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -506,7 +506,7 @@ static void simpledrm_device_detach_genpd(void *res)
return;
for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
- if (!sdev->pwr_dom_links[i])
+ if (sdev->pwr_dom_links[i])
device_link_del(sdev->pwr_dom_links[i]);
if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
--
2.42.0
Re: (subset) [PATCH v3 0/5] Support bridge/connector by Tegra HDMI
From: Thierry Reding On Mon, 07 Aug 2023 17:35:10 +0300, Svyatoslav Ryhel wrote: > This patch adds support for the bridge/connector attached to the > HDMI output, allowing to model the hardware properly. It keeps > backwards compatibility with existing bindings and is required > by devices which have a simple or MHL bridge connected to HDMI > output like ASUS P1801-T or LG P880/P895 or HTC One X. > > Tested on ASUS Transformers which have no dedicated bridge but > have type d HDMI connector directly available. Tests went smoothly. > > [...] Applied, thanks! [1/5] ARM: dts: tegra: Drop unit-address from parallel RGB output port (no commit info) Best regards, -- Thierry Reding
Re: [PATCH -next] drm/tegra: Remove two unused function declarations
On Wed, Aug 09, 2023 at 11:02:26AM +0800, Yue Haibing wrote:
> Commit 776dc3840367 ("drm/tegra: Move subdevice infrastructure to host1x")
> removed the implementation but not the declaration.
>
> Signed-off-by: Yue Haibing
> ---
> drivers/gpu/drm/tegra/drm.h | 3 ---
> 1 file changed, 3 deletions(-)
Applied, thanks.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 1/2] drm/tegra: Return an error code if fails
On Tue, Oct 10, 2023 at 03:22:56PM +0200, Thierry Reding wrote:
> On Mon, Jun 26, 2023 at 10:33:30PM +0800, Sui Jingfeng wrote:
> > Return -ENOMEM if tegra_bo_mmap() fails.
> >
> > Signed-off-by: Sui Jingfeng
> > ---
> > drivers/gpu/drm/tegra/gem.c | 2 ++
> > 1 file changed, 2 insertions(+)
>
> Sorry, this fell through the cracks. I think it'd be better if
> tegra_bo_mmap() were to be improved to always return either an ERR_PTR()
> encoded error code or a valid pointer. Throwing NULL into the mix isn't
> useful because it typically means something like -ENOMEM anyway. Error
> codes are more explicit, so since we're already using them for some
> cases, might as well return them for all.
>
> Actually, looks like tegra_bo_mmap() never actually returns an ERR_PTR()
> encoded error code. It's either obj->vaddr, the return value of vmap()
> (which is either NULL or the address of the mapping), or the address
> obtained from dma_buf_vmap_unlocked() (i.e. map.vaddr) or NULL on
> failure. So I think it would equally make sense to keep your patch and
> to remove the IS_ERR() check below it.
>
> I would slightly prefer the first option, but either is fine.
How about the attached patch?
Thierry
From b34a09efcf7b1d2c25d3baf8c6d91c5ca09b4e0f Mon Sep 17 00:00:00 2001
From: Thierry Reding
Date: Tue, 10 Oct 2023 17:26:14 +0200
Subject: [PATCH] drm/tegra: gem: Do not return NULL in tegra_bo_mmap()
It's confusing for a function to return NULL and ERR_PTR()-encoded error
codes on failure. Make sure we only ever return the latter since that's
what callers already expect.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/tegra/gem.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 11296de59c5a..679460e05c05 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -178,6 +178,7 @@ static void *tegra_bo_mmap(struct host1x_bo *bo)
{
struct tegra_bo *obj = host1x_to_tegra_bo(bo);
struct iosys_map map;
+ void *vaddr;
int ret;
if (obj->vaddr)
@@ -185,10 +186,18 @@ static void *tegra_bo_mmap(struct host1x_bo *bo)
if (obj->gem.import_attach) {
ret = dma_buf_vmap_unlocked(obj->gem.import_attach->dmabuf,
&map);
- return ret ? NULL : map.vaddr;
+ if (ret < 0)
+ return ERR_PTR(ret);
+
+ return map.vaddr;
}
- return vmap(obj->pages, obj->num_pages, VM_MAP,
pgprot_writecombine(PAGE_KERNEL));
+ vaddr = vmap(obj->pages, obj->num_pages, VM_MAP,
+pgprot_writecombine(PAGE_KERNEL));
+ if (!vaddr)
+ return -ENOMEM;
+
+ return vaddr;
}
static void tegra_bo_munmap(struct host1x_bo *bo, void *addr)
--
2.42.0
signature.asc
Description: PGP signature
Re: [PATCH 2/2] drm/tegra: Remove surplus else after return
On Mon, Jun 26, 2023 at 10:33:31PM +0800, Sui Jingfeng wrote: > else is not generally useful after return > > Signed-off-by: Sui Jingfeng > --- > drivers/gpu/drm/tegra/gem.c | 19 ++- > 1 file changed, 10 insertions(+), 9 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH 1/2] drm/tegra: Return an error code if fails
On Mon, Jun 26, 2023 at 10:33:30PM +0800, Sui Jingfeng wrote: > Return -ENOMEM if tegra_bo_mmap() fails. > > Signed-off-by: Sui Jingfeng > --- > drivers/gpu/drm/tegra/gem.c | 2 ++ > 1 file changed, 2 insertions(+) Sorry, this fell through the cracks. I think it'd be better if tegra_bo_mmap() were to be improved to always return either an ERR_PTR() encoded error code or a valid pointer. Throwing NULL into the mix isn't useful because it typically means something like -ENOMEM anyway. Error codes are more explicit, so since we're already using them for some cases, might as well return them for all. Actually, looks like tegra_bo_mmap() never actually returns an ERR_PTR() encoded error code. It's either obj->vaddr, the return value of vmap() (which is either NULL or the address of the mapping), or the address obtained from dma_buf_vmap_unlocked() (i.e. map.vaddr) or NULL on failure. So I think it would equally make sense to keep your patch and to remove the IS_ERR() check below it. I would slightly prefer the first option, but either is fine. Thierry > diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c > index dea38892d6e6..0ce22935fbd3 100644 > --- a/drivers/gpu/drm/tegra/gem.c > +++ b/drivers/gpu/drm/tegra/gem.c > @@ -710,6 +710,8 @@ static int tegra_gem_prime_vmap(struct dma_buf *buf, > struct iosys_map *map) > void *vaddr; > > vaddr = tegra_bo_mmap(&bo->base); > + if (!vaddr) > + return -ENOMEM; > if (IS_ERR(vaddr)) > return PTR_ERR(vaddr); > > -- > 2.25.1 > signature.asc Description: PGP signature
Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display
On Thu, Aug 31, 2023 at 10:04:29AM +0200, Daniel Vetter wrote:
> On Thu, 31 Aug 2023 at 08:33, Mikko Perttunen wrote:
> >
> > On 8/30/23 13:19, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 25.08.23 um 15:22 schrieb Thierry Reding:
> > >> From: Thierry Reding
> > >>
> > >> Tegra DRM doesn't support display on Tegra234 and later, so make sure
> > >> not to remove any existing framebuffers in that case.
> > >>
> > >> Signed-off-by: Thierry Reding
> > >> ---
> > >> drivers/gpu/drm/tegra/drm.c | 8 +---
> > >> 1 file changed, 5 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > >> index b1e1a78e30c6..7a38dadbc264 100644
> > >> --- a/drivers/gpu/drm/tegra/drm.c
> > >> +++ b/drivers/gpu/drm/tegra/drm.c
> > >> @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct
> > >> host1x_device *dev)
> > >> drm_mode_config_reset(drm);
> > >> -err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> > >> -if (err < 0)
> > >> -goto hub;
> > >> +if (drm->mode_config.num_crtc > 0) {
> > >
> > > If you don't support the hardware, wouldn't it be better to return
> > > -ENODEV if !num_crtc?
> >
> > While display is not supported through TegraDRM on Tegra234+, certain
> > multimedia accelerators are supported, so we need to finish probe for those.
>
> Ideally you also register the tegra driver without DRIVER_MODESET |
> DRIVER_ATOMIC in that case, to avoid unecessary userspace confusion.
> Most userspace can cope with a display driver without any crtc, but I
> think xorg-modesettting actually falls over. Or at least I've seen
> some hacks that the agx people added to make sure X doesn't
> accidentally open the wrong driver.
That's a good point. However I recall from earlier attempts at doing
something like this in Nouveau (although this is now very long ago) that
it's not very easy. The problem, as I recall, is that the driver is a
singleton, so we would essentially be supporting either modesetting or
not, for any device in the system.
Now, it's unlikely that we'd have a mix of one Tegra DRM driver with
display support and another without, but it's something that I recall
back at the time with Nouveau was problematic because you could have the
Tegra integrated graphics (without display support) and a PCI-connected
discrete GPU (with display support) within the same system.
I need to look into it a bit more to see if I can come up with something
good to account for this.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display
On Wed, Aug 30, 2023 at 08:13:04AM +0200, Javier Martinez Canillas wrote:
> Thierry Reding writes:
>
> Hello Thierry,
>
> > From: Thierry Reding
> >
> > Tegra DRM doesn't support display on Tegra234 and later, so make sure
> > not to remove any existing framebuffers in that case.
> >
>
> I see, this makes sense to me.
>
> Acked-by: Javier Martinez Canillas
>
> A couple of comments below though:
>
> > Signed-off-by: Thierry Reding
> > ---
> > drivers/gpu/drm/tegra/drm.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index b1e1a78e30c6..7a38dadbc264 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct host1x_device
> > *dev)
> >
> > drm_mode_config_reset(drm);
> >
> > - err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> > - if (err < 0)
> > - goto hub;
> > + if (drm->mode_config.num_crtc > 0) {
>
> Maybe you can add a comment here explaining why the check is needed?
Sure, will do.
> I also wonder if is worth to move the drm_num_crtcs() function from
> drivers/gpu/drm/drm_crtc.c to include/drm/drm_crtc.h and use that helper
> instead?
I've been looking at this, there's a few things that come to mind. It
seems like we have a couple of different ways to get the number of CRTCs
for a device. We have struct drm_device's num_crtcs, which is set during
drm_vblank_init(), then we have struct drm_mode_config's num_crtc, which
is incremented every time a new CRTC is added (and decremented when a
CRTC is removed), and finally we've got the drm_num_crtcs() which
"computes" the number of CRTCs registered by iterating over all CRTCs
that have been registered.
Are there any cases where these three can yield different values? Would
it not make sense to consolidate these into a single variable?
Thierry
signature.asc
Description: PGP signature
[PATCH] drm/tegra: Remove existing framebuffer only if we support display
From: Thierry Reding
Tegra DRM doesn't support display on Tegra234 and later, so make sure
not to remove any existing framebuffers in that case.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/tegra/drm.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index b1e1a78e30c6..7a38dadbc264 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct host1x_device *dev)
drm_mode_config_reset(drm);
- err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
- if (err < 0)
- goto hub;
+ if (drm->mode_config.num_crtc > 0) {
+ err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
+ if (err < 0)
+ goto hub;
+ }
err = drm_dev_register(drm, 0);
if (err < 0)
--
2.41.0
Re: [PATCH 15/20] drm/tegra/hub: Increase buffer size to ensure all possible values can be stored
On Thu, Aug 24, 2023 at 01:01:24PM +0100, Lee Jones wrote: > On Thu, 24 Aug 2023, Jani Nikula wrote: > > > On Thu, 24 Aug 2023, Thierry Reding wrote: > > > On Thu, Aug 24, 2023 at 08:37:00AM +0100, Lee Jones wrote: > > >> When converting from int to string, we must allow for up to 10-chars > > >> (2147483647). > > >> > > >> Fixes the following W=1 kernel build warning(s): > > >> > > >> drivers/gpu/drm/tegra/hub.c: In function ‘tegra_display_hub_probe’: > > >> drivers/gpu/drm/tegra/hub.c:1106:47: warning: ‘%u’ directive output may > > >> be truncated writing between 1 and 10 bytes into a region of size 4 > > >> [-Wformat-truncation=] > > >> drivers/gpu/drm/tegra/hub.c:1106:42: note: directive argument in the > > >> range [0, 4294967294] > > >> drivers/gpu/drm/tegra/hub.c:1106:17: note: ‘snprintf’ output between 6 > > >> and 15 bytes into a destination of size 8 > > > > > > I wish there was (perhaps there is?) a better way to annotate that i > > > will always be within a given range. In practice this is always going to > > > be smaller than 10 and even in future hardware I wouldn't expect this to > > > ever exceed anything like 32 or 64, so 8 characters is already generous. > > > > I assume you could do > > > > snprintf(id, sizeof(id), "wgrp%u", (unsigned char)i); > > > > but it's perhaps less obvious than just increasing the size of the > > buffer. > > I had the very same thought process. It's not a big deal, this happens all on the stack, so adding a couple of bytes isn't going to hurt very much. Still with all the tooling that we have it'd be nice if something could be added. Perhaps an alternative would be to reject values for num_wgrp that are bigger than 32. With that the compiler might have enough information not to warn about this. Anyway, no need to spend any more time on this, I'm fine with the patch as-is. Thierry signature.asc Description: PGP signature
Re: [PATCH 15/20] drm/tegra/hub: Increase buffer size to ensure all possible values can be stored
On Thu, Aug 24, 2023 at 08:37:00AM +0100, Lee Jones wrote:
> When converting from int to string, we must allow for up to 10-chars
> (2147483647).
>
> Fixes the following W=1 kernel build warning(s):
>
> drivers/gpu/drm/tegra/hub.c: In function ‘tegra_display_hub_probe’:
> drivers/gpu/drm/tegra/hub.c:1106:47: warning: ‘%u’ directive output may be
> truncated writing between 1 and 10 bytes into a region of size 4
> [-Wformat-truncation=]
> drivers/gpu/drm/tegra/hub.c:1106:42: note: directive argument in the range
> [0, 4294967294]
> drivers/gpu/drm/tegra/hub.c:1106:17: note: ‘snprintf’ output between 6 and
> 15 bytes into a destination of size 8
I wish there was (perhaps there is?) a better way to annotate that i
will always be within a given range. In practice this is always going to
be smaller than 10 and even in future hardware I wouldn't expect this to
ever exceed anything like 32 or 64, so 8 characters is already generous.
Thierry
>
> Signed-off-by: Lee Jones
> ---
> Cc: Thierry Reding
> Cc: Mikko Perttunen
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Jonathan Hunter
> Cc: Philipp Zabel
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-te...@vger.kernel.org
> ---
> drivers/gpu/drm/tegra/hub.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c
> index 1af5f8318d914..f21e57e8599ee 100644
> --- a/drivers/gpu/drm/tegra/hub.c
> +++ b/drivers/gpu/drm/tegra/hub.c
> @@ -1101,7 +1101,7 @@ static int tegra_display_hub_probe(struct
> platform_device *pdev)
>
> for (i = 0; i < hub->soc->num_wgrps; i++) {
> struct tegra_windowgroup *wgrp = &hub->wgrps[i];
> - char id[8];
> + char id[16];
>
> snprintf(id, sizeof(id), "wgrp%u", i);
> mutex_init(&wgrp->lock);
> --
> 2.42.0.rc1.204.g551eb34607-goog
>
signature.asc
Description: PGP signature
Re: [PATCH v3 0/5] Add JDI LPM102A188A display panel support
On Mon, Aug 07, 2023 at 02:33:00PM +0100, Diogo Ivo wrote: > Hello, > > These patches add support for the JDI LPM102A188A display panel, > found in the Google Pixel C. > > Patch 1 adds the DT bindings for the panel. > > Patch 2 adds the panel driver, which is based on the downstream > kernel driver published by Google and developed by Sean Paul. > > Patches 3-5 add DT nodes for the regulator, backlight controller and > display panel. > > The first version of this patch series can be found at: > https://lore.kernel.org/all/20220929170502.1034040-1-diogo@tecnico.ulisboa.pt/ > > The first submission of v2 can be found at: > https://lore.kernel.org/all/20221025153746.101278-1-diogo@tecnico.ulisboa.pt/ > > Changes in v2: > - Patch 1: remove touchscreen reset gpio property > - Patch 2: clear register based on its value rather than a DT property > - Patch 3: tune backlight delay values > - Patch 4: add generic node names, remove underscores > > Changes in v3: > - Patch 1: add Reviewed-by > - Patch 2: fix error handling, remove enabled/prepared booleans, add >dc/dc setting > - Patches 3-5: Split previous patch 3 into three different patches, >each adding a separate node > - removed previous patch 2 pertaining to Tegra DSI reset as it was upstreamed > > Diogo Ivo (5): > dt-bindings: display: Add bindings for JDI LPM102A188A > drm/panel: Add driver for JDI LPM102A188A > arm64: dts: smaug: Add DSI/CSI regulator > arm64: dts: smaug: Add backlight node > arm64: dts: smaug: Add display panel node I've picked up patches 3-5 into the Tegra tree and I assume the other two will go in through drm-misc? Thierry signature.asc Description: PGP signature
Re: (subset) [PATCH v3 0/5] Add JDI LPM102A188A display panel support
From: Thierry Reding On Mon, 07 Aug 2023 14:33:00 +0100, Diogo Ivo wrote: > These patches add support for the JDI LPM102A188A display panel, > found in the Google Pixel C. > > Patch 1 adds the DT bindings for the panel. > > Patch 2 adds the panel driver, which is based on the downstream > kernel driver published by Google and developed by Sean Paul. > > [...] Applied, thanks! [3/5] arm64: dts: smaug: Add DSI/CSI regulator (no commit info) [4/5] arm64: dts: smaug: Add backlight node (no commit info) [5/5] arm64: dts: smaug: Add display panel node (no commit info) Best regards, -- Thierry Reding
Re: [PATCH v2 0/2] Support bridge/connector by Tegra HDMI
On Thu, Jul 27, 2023 at 07:24:56PM +0300, Svyatoslav Ryhel wrote: > > > 27 липня 2023 р. 18:09:22 GMT+03:00, Thierry Reding > написав(-ла): > >On Sun, Jun 18, 2023 at 11:50:44AM +0300, Svyatoslav Ryhel wrote: > >> This patch adds support for the bridge/connector attached to the > >> HDMI output, allowing to model the hardware properly. It keeps > >> backwards compatibility with existing bindings and is required > >> by devices which have a simple or MHL bridge connected to HDMI > >> output like ASUS P1801-T or LG P880/P895 or HTC One X. > >> > >> Tested on ASUS Transformers which have no dedicated bridge but > >> have type d HDMI connector directly available. Tests went smoothly. > > > >If I understand correctly, we still need the drm/tegra patch to be > >applied before the DT change, otherwise the driver won't know what to do > >about the connector, right? > > > >That shouldn't be big problem, but it means that the patches need to be > >staged in correctly to avoid breaking things. > > Patchset contains drm/tegra patch I understand, but my point is that if we apply the DT patch before the driver patch, then the display won't be correctly initialized because the old driver code only looks within the HDMI node for the additional properties. Only after the drm/tegra patch is applied will the move in DT be recognized by the driver. So for now I've picked up the drm/tegra patch and then I'll apply the DT change later on. Thierry signature.asc Description: PGP signature
Re: [PATCH v2 2/2] ARM: tegra: transformers: add connector node
On Thu, Jul 27, 2023 at 07:26:28PM +0300, Svyatoslav Ryhel wrote:
>
>
> 27 липня 2023 р. 18:11:15 GMT+03:00, Thierry Reding
> написав(-ла):
> >On Sun, Jun 18, 2023 at 11:50:46AM +0300, Svyatoslav Ryhel wrote:
> >> All ASUS Transformers have micro-HDMI connector directly available.
> >> After Tegra HDMI got bridge/connector support, we should use connector
> >> framework for proper HW description.
> >>
> >> Tested-by: Andreas Westman Dorcsak # ASUS TF T30
> >> Tested-by: Robert Eckelmann # ASUS TF101 T20
> >> Tested-by: Svyatoslav Ryhel # ASUS TF201 T30
> >> Signed-off-by: Svyatoslav Ryhel
> >> ---
> >> arch/arm/boot/dts/tegra20-asus-tf101.dts | 22 ---
> >> .../dts/tegra30-asus-transformer-common.dtsi | 21 --
> >> 2 files changed, 38 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/tegra20-asus-tf101.dts
> >> b/arch/arm/boot/dts/tegra20-asus-tf101.dts
> >> index c2a9c3fb5b33..97350f566539 100644
> >> --- a/arch/arm/boot/dts/tegra20-asus-tf101.dts
> >> +++ b/arch/arm/boot/dts/tegra20-asus-tf101.dts
> >> @@ -82,9 +82,11 @@ hdmi@5428 {
> >>pll-supply = <&hdmi_pll_reg>;
> >>hdmi-supply = <&vdd_hdmi_en>;
> >>
> >> - nvidia,ddc-i2c-bus = <&hdmi_ddc>;
> >> - nvidia,hpd-gpio = <&gpio TEGRA_GPIO(N, 7)
> >> - GPIO_ACTIVE_HIGH>;
> >> + port@0 {
> >> + hdmi_out: endpoint {
> >> + remote-endpoint = <&connector_in>;
> >> + };
> >> + };
> >
> >Does this need a bindings change? nvidia,tegra20-hdmi currently doesn't
> >support OF graphs, so this would probably fail to validate if we merge
> >it without a corresponding DT bindings update.
>
> drm/tegra patch is backwards compatible and connector node is optional.
We still need to document the connector node, otherwise the DT
validation will complain about port@0 being used here, won't it?
Thierry
signature.asc
Description: PGP signature
Re: (subset) [PATCH 1/3] dt-bindings: display: panel: Move HannStar HSD101PWW2 to LVDS
From: Thierry Reding On Wed, 26 Jul 2023 20:48:55 +0200, Thierry Reding wrote: > From: Thierry Reding > > The HannStar HSD101PWW2 is an LVDS panel, so move it to the correct > bindings file. > > Applied, thanks! [3/3] ARM: tegra: Use Hannstar HSD101PWW2 on Pegatron Chagall commit: b28d3af99ac4885f136f6330fec6499b15ad5b25 Best regards, -- Thierry Reding
Re: (subset) [PATCH 1/3] dt-bindings: display: panel: Move Chunghwa CLAA070WP03XG to LVDS
From: Thierry Reding On Wed, 26 Jul 2023 20:50:08 +0200, Thierry Reding wrote: > From: Thierry Reding > > The Chunghwa CLAA070WP03XG is an LVDS panel, so move it to the correct > bindings file. > > Applied, thanks! [3/3] ARM: tegra: Provide specific compatible string for Nexus 7 panel commit: c9a706ab227ef59cc49923358513251ca4965563 Best regards, -- Thierry Reding
Re: [PATCH v2 2/2] ARM: tegra: transformers: add connector node
On Sun, Jun 18, 2023 at 11:50:46AM +0300, Svyatoslav Ryhel wrote:
> All ASUS Transformers have micro-HDMI connector directly available.
> After Tegra HDMI got bridge/connector support, we should use connector
> framework for proper HW description.
>
> Tested-by: Andreas Westman Dorcsak # ASUS TF T30
> Tested-by: Robert Eckelmann # ASUS TF101 T20
> Tested-by: Svyatoslav Ryhel # ASUS TF201 T30
> Signed-off-by: Svyatoslav Ryhel
> ---
> arch/arm/boot/dts/tegra20-asus-tf101.dts | 22 ---
> .../dts/tegra30-asus-transformer-common.dtsi | 21 --
> 2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/boot/dts/tegra20-asus-tf101.dts
> b/arch/arm/boot/dts/tegra20-asus-tf101.dts
> index c2a9c3fb5b33..97350f566539 100644
> --- a/arch/arm/boot/dts/tegra20-asus-tf101.dts
> +++ b/arch/arm/boot/dts/tegra20-asus-tf101.dts
> @@ -82,9 +82,11 @@ hdmi@5428 {
> pll-supply = <&hdmi_pll_reg>;
> hdmi-supply = <&vdd_hdmi_en>;
>
> - nvidia,ddc-i2c-bus = <&hdmi_ddc>;
> - nvidia,hpd-gpio = <&gpio TEGRA_GPIO(N, 7)
> - GPIO_ACTIVE_HIGH>;
> + port@0 {
> + hdmi_out: endpoint {
> + remote-endpoint = <&connector_in>;
> + };
> + };
Does this need a bindings change? nvidia,tegra20-hdmi currently doesn't
support OF graphs, so this would probably fail to validate if we merge
it without a corresponding DT bindings update.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v2 0/2] Support bridge/connector by Tegra HDMI
On Sun, Jun 18, 2023 at 11:50:44AM +0300, Svyatoslav Ryhel wrote: > This patch adds support for the bridge/connector attached to the > HDMI output, allowing to model the hardware properly. It keeps > backwards compatibility with existing bindings and is required > by devices which have a simple or MHL bridge connected to HDMI > output like ASUS P1801-T or LG P880/P895 or HTC One X. > > Tested on ASUS Transformers which have no dedicated bridge but > have type d HDMI connector directly available. Tests went smoothly. If I understand correctly, we still need the drm/tegra patch to be applied before the DT change, otherwise the driver won't know what to do about the connector, right? That shouldn't be big problem, but it means that the patches need to be staged in correctly to avoid breaking things. Thierry signature.asc Description: PGP signature
[PATCH 3/3] ARM: tegra: Provide specific compatible string for Nexus 7 panel
From: Thierry Reding
panel-lvds alone is not a valid compatible string and we always need a
specific compatible string as well. Nexus 7 can come with one of (at
least) two panels, so pick one of them as the specific compatible
string.
Signed-off-by: Thierry Reding
---
.../nvidia/tegra30-asus-nexus7-grouper-common.dtsi | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/arm/boot/dts/nvidia/tegra30-asus-nexus7-grouper-common.dtsi
b/arch/arm/boot/dts/nvidia/tegra30-asus-nexus7-grouper-common.dtsi
index 4fa6b20c4fdb..a9342e04b14b 100644
--- a/arch/arm/boot/dts/nvidia/tegra30-asus-nexus7-grouper-common.dtsi
+++ b/arch/arm/boot/dts/nvidia/tegra30-asus-nexus7-grouper-common.dtsi
@@ -1092,15 +1092,11 @@ cpu3: cpu@3 {
display-panel {
/*
-* Nexus 7 supports two compatible panel models:
-*
-* 1. hydis,hv070wx2-1e0
-* 2. chunghwa,claa070wp03xg
-*
-* We want to use timing which is optimized for Nexus 7,
-* hence we need to customize the timing.
+* Some device variants come with a Hydis HV070WX2-1E0, but
+* since they are all largely compatible, we'll go with the
+* Chunghwa one here.
*/
- compatible = "panel-lvds";
+ compatible = "chunghwa,claa070wp03xg", "panel-lvds";
width-mm = <94>;
height-mm = <150>;
--
2.41.0
[PATCH 1/3] dt-bindings: display: panel: Move Chunghwa CLAA070WP03XG to LVDS
From: Thierry Reding The Chunghwa CLAA070WP03XG is an LVDS panel, so move it to the correct bindings file. Signed-off-by: Thierry Reding --- Documentation/devicetree/bindings/display/panel/panel-lvds.yaml | 2 ++ .../devicetree/bindings/display/panel/panel-simple.yaml | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml index 344e5df40c2f..dbbf32a8be87 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml @@ -40,6 +40,8 @@ properties: items: - enum: - auo,b101ew05 + # Chunghwa Picture Tubes Ltd. 7" WXGA (800x1280) TFT LCD LVDS panel + - chunghwa,claa070wp03xg # HannStar Display Corp. HSD101PWW2 10.1" WXGA (1280x800) LVDS panel - hannstar,hsd101pww2 - tbs,a711-panel diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml index f4d9da4afefd..67959290b212 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml @@ -103,8 +103,6 @@ properties: - cdtech,s070wv95-ct16 # Chefree CH101OLHLWH-002 10.1" (1280x800) color TFT LCD panel - chefree,ch101olhlwh-002 -# Chunghwa Picture Tubes Ltd. 7" WXGA TFT LCD panel - - chunghwa,claa070wp03xg # Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel - chunghwa,claa101wa01a # Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel -- 2.41.0
[PATCH 2/3] dt-bindings: display: panel: Document Hydis HV070WX2-1E0
From: Thierry Reding The Hydis HV070WX2-1E0 is a 7" WXGA (800x1280) TFT LCD LVDS panel that is one of the variants used on Google Nexus 7. Signed-off-by: Thierry Reding --- Documentation/devicetree/bindings/display/panel/panel-lvds.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml index dbbf32a8be87..9f1016551e0b 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml @@ -44,6 +44,8 @@ properties: - chunghwa,claa070wp03xg # HannStar Display Corp. HSD101PWW2 10.1" WXGA (1280x800) LVDS panel - hannstar,hsd101pww2 + # Hydis Technologies 7" WXGA (800x1280) TFT LCD LVDS panel + - hydis,hv070wx2-1e0 - tbs,a711-panel - const: panel-lvds -- 2.41.0
[PATCH 3/3] ARM: tegra: Use Hannstar HSD101PWW2 on Pegatron Chagall
From: Thierry Reding
The LVDS bindings require a specific compatible string in addition to
the generic "panel-lvds". Add the HannStar HSD101PWW2 which is used on
a similar device (ASUS TF201) and seems to work fine with slightly
modified timings in DT.
Suggested-by: Svyatoslav Ryhel
Signed-off-by: Thierry Reding
---
arch/arm/boot/dts/nvidia/tegra30-pegatron-chagall.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/nvidia/tegra30-pegatron-chagall.dts
b/arch/arm/boot/dts/nvidia/tegra30-pegatron-chagall.dts
index c81d5875c31c..4012f9c799a8 100644
--- a/arch/arm/boot/dts/nvidia/tegra30-pegatron-chagall.dts
+++ b/arch/arm/boot/dts/nvidia/tegra30-pegatron-chagall.dts
@@ -2628,7 +2628,7 @@ cpu3: cpu@3 {
};
display-panel {
- compatible = "panel-lvds";
+ compatible = "hannstar,hsd101pww2", "panel-lvds";
width-mm = <217>;
height-mm = <136>;
--
2.41.0
[PATCH 2/3] drm/panel: Relax porches for HannStar HSD101PWW2
From: Thierry Reding
The porch maximum values for the HannStar HSD101PWW2 are unusually
small. Make them a bit larger to allow a more flexibility when
overriding the timings in device tree. Unfortunately the datasheet
doesn't list porch limits in detail, so this is a bit of guesswork.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/panel/panel-simple.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index 4badda6570d5..4bab181e9d4b 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -2104,13 +2104,13 @@ static const struct panel_desc hannstar_hsd100pxn1 = {
static const struct display_timing hannstar_hsd101pww2_timing = {
.pixelclock = { 6430, 7110, 8200 },
.hactive = { 1280, 1280, 1280 },
- .hfront_porch = { 1, 1, 10 },
- .hback_porch = { 1, 1, 10 },
- .hsync_len = { 58, 158, 661 },
+ .hfront_porch = { 1, 1, 64 },
+ .hback_porch = { 1, 1, 64 },
+ .hsync_len = { 58, 158, 553 },
.vactive = { 800, 800, 800 },
- .vfront_porch = { 1, 1, 10 },
- .vback_porch = { 1, 1, 10 },
- .vsync_len = { 1, 21, 203 },
+ .vfront_porch = { 1, 1, 32 },
+ .vback_porch = { 1, 1, 32 },
+ .vsync_len = { 1, 21, 159 },
.flags = DISPLAY_FLAGS_DE_HIGH,
};
--
2.41.0
[PATCH 1/3] dt-bindings: display: panel: Move HannStar HSD101PWW2 to LVDS
From: Thierry Reding The HannStar HSD101PWW2 is an LVDS panel, so move it to the correct bindings file. Signed-off-by: Thierry Reding --- Documentation/devicetree/bindings/display/panel/panel-lvds.yaml | 2 ++ .../devicetree/bindings/display/panel/panel-simple.yaml | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml index 929fe046d1e7..344e5df40c2f 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml @@ -40,6 +40,8 @@ properties: items: - enum: - auo,b101ew05 + # HannStar Display Corp. HSD101PWW2 10.1" WXGA (1280x800) LVDS panel + - hannstar,hsd101pww2 - tbs,a711-panel - const: panel-lvds diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml index df1cec8fd21b..f4d9da4afefd 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml @@ -168,8 +168,6 @@ properties: - hannstar,hsd070pww1 # HannStar Display Corp. HSD100PXN1 10.1" XGA LVDS panel - hannstar,hsd100pxn1 -# HannStar Display Corp. HSD101PWW2 10.1" WXGA (1280x800) LVDS panel - - hannstar,hsd101pww2 # Hitachi Ltd. Corporation 9" WVGA (800x480) TFT LCD panel - hit,tx23d38vm0caa # InfoVision Optoelectronics M133NWF4 R0 13.3" FHD (1920x1080) TFT LCD panel -- 2.41.0
