Re: [PATCH] drm: apple: mark local functions static
Hej Arnd,
On Tue, Jan 23, 2024, at 08:34, Arnd Bergmann wrote:
> On Mon, Jan 22, 2024, at 21:50, Janne Grunau wrote:
>> On Wed, Jan 17, 2024, at 11:44, Arnd Bergmann wrote:
>>>
>>> -int parse_sample_rate_bit(struct dcp_parse_ctx *handle, unsigned int
>>> *ratebit)
>>> +static int parse_sample_rate_bit(struct dcp_parse_ctx *handle,
>>> unsigned int *ratebit)
>>> {
>>> s64 rate;
>>> int ret = parse_int(handle, );
>>> @@ -715,7 +715,7 @@ int parse_sample_rate_bit(struct dcp_parse_ctx
>>> *handle, unsigned int *ratebit)
>>> return 0;
>>> }
>>>
>>> -int parse_sample_fmtbit(struct dcp_parse_ctx *handle, u64 *fmtbit)
>>> +static int parse_sample_fmtbit(struct dcp_parse_ctx *handle, u64 *fmtbit)
>>> {
>>> s64 sample_size;
>>> int ret = parse_int(handle, _size);
>>
>> thanks, patch included in my dev branch and will be in the next pull
>> request I'll send to Hector.
>>
>> I suppose the recipients are generated by an automated
>> get_maintainers.pl invocation. Is that desired for out of tree drivers?
>
> I was wondering about that as well, as I don't usually send
> patches for code that isn't at least in linux-next yet.
>
> I ended up using what is in the MAINTAINERS file for this driver
> in the branch as that is is all I have at this point:
>
> APPLE DRM DISPLAY DRIVER
> M: Alyssa Rosenzweig
> L: dri-devel@lists.freedesktop.org
> S: Maintained
> T: git git://anongit.freedesktop.org/drm/drm-misc
> F: drivers/gpu/drm/apple/
>
> I left out the drivers/gpu/ maintainer addresses though.
oops, answered to the wrong patch. The strscpy one has the drivers/gpu/
maintainers and the question. I replied here first assuming it has the same
recipient list. I'd consider the recipients for this mail reasonable.
Janne
Re: [PATCH] drm: apple: use strscpy() in place of strlcpy()
Hej,
On Mon, Jan 22, 2024, at 17:11, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Since commit d26270061ae6 ("string: Remove strlcpy()"), the strlcpy()
> function causes a build failure.
>
> Since the return value is ignored, changing it to the strscpy()
> causes no change in behavior but fixes the build failure.
>
> Fixes: f237c83e4302 ("drm: apple: DCP AFK/EPIC support")
> Signed-off-by: Arnd Bergmann
> ---
> The apple drm driver is not in mainline linux yet, this patch
> is against https://github.com/AsahiLinux/linux/tree/bits/200-dcp
> ---
> drivers/gpu/drm/apple/afk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/apple/afk.c b/drivers/gpu/drm/apple/afk.c
> index 99d579d5ce47..9fbcd18878e8 100644
> --- a/drivers/gpu/drm/apple/afk.c
> +++ b/drivers/gpu/drm/apple/afk.c
> @@ -236,7 +236,7 @@ static void afk_recv_handle_init(struct
> apple_dcp_afkep *ep, u32 channel,
> return;
> }
>
> - strlcpy(name, payload, sizeof(name));
> + strscpy(name, payload, sizeof(name));
>
> /*
>* in DCP firmware 13.2 DCP reports interface-name as name which starts
thanks, patch included in my dev branch and will be in the next pull request
I'll send to Hector.
best regards,
Janne
Re: [PATCH] drm: apple: mark local functions static
Hej,
On Wed, Jan 17, 2024, at 11:44, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> With linux-6.8, the kernel warns about functions that have no
> extern declaration, so mark both of these static.
>
> Fixes: 2d782b0d007d ("gpu: drm: apple: Add sound mode parsing")
> Signed-off-by: Arnd Bergmann
> ---
> This is for the bits/200-dcp branch in https://github.com/AsahiLinux/linux,
> the code is not yet upstream.
> ---
> drivers/gpu/drm/apple/parser.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/apple/parser.c
> b/drivers/gpu/drm/apple/parser.c
> index 44aad9a64f9a..ea9f40bb7de2 100644
> --- a/drivers/gpu/drm/apple/parser.c
> +++ b/drivers/gpu/drm/apple/parser.c
> @@ -694,7 +694,7 @@ int parse_epic_service_init(struct dcp_parse_ctx
> *handle, const char **name,
> return ret;
> }
>
> -int parse_sample_rate_bit(struct dcp_parse_ctx *handle, unsigned int
> *ratebit)
> +static int parse_sample_rate_bit(struct dcp_parse_ctx *handle,
> unsigned int *ratebit)
> {
> s64 rate;
> int ret = parse_int(handle, );
> @@ -715,7 +715,7 @@ int parse_sample_rate_bit(struct dcp_parse_ctx
> *handle, unsigned int *ratebit)
> return 0;
> }
>
> -int parse_sample_fmtbit(struct dcp_parse_ctx *handle, u64 *fmtbit)
> +static int parse_sample_fmtbit(struct dcp_parse_ctx *handle, u64 *fmtbit)
> {
> s64 sample_size;
> int ret = parse_int(handle, _size);
thanks, patch included in my dev branch and will be in the next pull request
I'll send to Hector.
I suppose the recipients are generated by an automated get_maintainers.pl
invocation. Is that desired for out of tree drivers?
best regards,
Janne
[PATCH v2] drm/simpledrm: Add support for multiple "power-domains"
From: Janne Grunau
Multiple power domains need to be handled explicitly in each driver. The
driver core can not handle it automatically since it is not aware of
power sequencing requirements the hardware might have. This is not a
problem for simpledrm since everything is expected to be powered on by
the bootloader. simpledrm has just ensure it remains powered on during
its lifetime.
This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
systems. The HDMI output initialized by the bootloader requires keeping
the display controller and a DP phy power domain on.
Signed-off-by: Janne Grunau
---
Changes in v2:
- removed broken drm_err() log statement only ment for debugging
- removed commented cast
- use correct format spcifier for 'int' in log statement
- add 'continue;' after failure to get device for power_domain
- use drm_warn() in non fatal error cases
- removed duplicate PTR_ERR conversion
- Link to v1:
https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc...@jannau.net
---
drivers/gpu/drm/tiny/simpledrm.c | 105 +++
1 file changed, 105 insertions(+)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index ff86ba1ae1b8..9c597461d1e2 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include
@@ -227,6 +228,12 @@ struct simpledrm_device {
unsigned int regulator_count;
struct regulator **regulators;
#endif
+ /* power-domains */
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+ int pwr_dom_count;
+ struct device **pwr_dom_devs;
+ struct device_link **pwr_dom_links;
+#endif
/* simplefb settings */
struct drm_display_mode mode;
@@ -468,6 +475,101 @@ static int simpledrm_device_init_regulators(struct
simpledrm_device *sdev)
}
#endif
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+/*
+ * Generic power domain handling code.
+ *
+ * Here we handle the power-domains properties of our "simple-framebuffer"
+ * dt node. This is only necessary if there is more than one power-domain.
+ * A single power-domains is handled automatically by the driver core. Multiple
+ * power-domains have to be handled by drivers since the driver core can't know
+ * the correct power sequencing. Power sequencing is not an issue for simpledrm
+ * since the bootloader has put the power domains already in the correct state.
+ * simpledrm has only to ensure they remain active for its lifetime.
+ *
+ * When the driver unloads, we detach from the power-domains.
+ *
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up the "simple-framebuffer" dt node, and the PM domain providers in the
+ * device tree. Chances are that there are no adverse effects, and if there
are,
+ * a clean teardown of the fb probe will not help us much either. So just
+ * complain and carry on, and hope that the user actually gets a working fb at
+ * the end of things.
+ */
+static void simpledrm_device_detach_genpd(void *res)
+{
+ int i;
+ struct simpledrm_device *sdev = res;
+
+ if (sdev->pwr_dom_count <= 1)
+ return;
+
+ for (i = sdev->pwr_dom_count - 1; i >= 0; 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);
+ }
+}
+
+static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
+{
+ struct device *dev = sdev->dev.dev;
+ int i;
+
+ sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node,
"power-domains",
+"#power-domain-cells");
+ /*
+* Single power-domain devices are handled by driver core nothing to do
+* here. The same for device nodes without "power-domains" property.
+*/
+ if (sdev->pwr_dom_count <= 1)
+ return 0;
+
+ sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
+ sizeof(*sdev->pwr_dom_devs),
+ GFP_KERNEL);
+ if (!sdev->pwr_dom_devs)
+ return -ENOMEM;
+
+ sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
+ sizeof(*sdev->pwr_dom_links),
+ GFP_KERNEL);
+ if (!sdev->pwr_dom_links)
+ return -ENOMEM;
+
+ for (i = 0; i < sdev->pwr_dom_count; i++) {
+ sdev->pwr_dom_de
Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
On 2023-09-11 14:26:10 +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 10.09.23 um 18:39 schrieb Janne Grunau via B4 Relay:
> > From: Janne Grunau
> >
> > Multiple power domains need to be handled explicitly in each driver. The
> > driver core can not handle it automatically since it is not aware of
> > power sequencing requirements the hardware might have. This is not a
> > problem for simpledrm since everything is expected to be powered on by
> > the bootloader. simpledrm has just ensure it remains powered on during
> > its lifetime.
> > This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> > systems. The HDMI output initialized by the bootloader requires keeping
> > the display controller and a DP phy power domain on.
> >
> > Signed-off-by: Janne Grunau
> > ---
> > drivers/gpu/drm/tiny/simpledrm.c | 106
> > +++
> > 1 file changed, 106 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c
> > b/drivers/gpu/drm/tiny/simpledrm.c
> > index ff86ba1ae1b8..efedede57d42 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -6,6 +6,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > @@ -227,6 +228,12 @@ struct simpledrm_device {
> > unsigned int regulator_count;
> > struct regulator **regulators;
> > #endif
> > + /* power-domains */
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > + int pwr_dom_count;
> > + struct device **pwr_dom_devs;
> > + struct device_link **pwr_dom_links;
> > +#endif
> > /* simplefb settings */
> > struct drm_display_mode mode;
> > @@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct
> > simpledrm_device *sdev)
> > }
> > #endif
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > +/*
> > + * Generic power domain handling code.
> > + *
> > + * Here we handle the power-domains properties of our "simple-framebuffer"
> > + * dt node. This is only necessary if there is more than one power-domain.
> > + * A single power-domains is handled automatically by the driver core.
> > Multiple
> > + * power-domains have to be handled by drivers since the driver core can't
> > know
> > + * the correct power sequencing. Power sequencing is not an issue for
> > simpledrm
> > + * since the bootloader has put the power domains already in the correct
> > state.
> > + * simpledrm has only to ensure they remain active for its lifetime.
> > + *
> > + * When the driver unloads, we detach from the power-domains.
> > + *
> > + * We only complain about errors here, no action is taken as the most
> > likely
> > + * error can only happen due to a mismatch between the bootloader which set
> > + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> > + * device tree. Chances are that there are no adverse effects, and if
> > there are,
> > + * a clean teardown of the fb probe will not help us much either. So just
> > + * complain and carry on, and hope that the user actually gets a working
> > fb at
> > + * the end of things.
> > + */
> > +static void simpledrm_device_detach_genpd(void *res)
> > +{
> > + int i;
> > + struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
> > +
> > +
> > + drm_err(>dev, "% power-domains count:%d\n", __func__,
> > sdev->pwr_dom_count);
>
> If anything, drm_dbg()
see my own reply, was never supposed to be there, removed locally
>
> > + if (sdev->pwr_dom_count <= 1)
> > + return;
> > +
> > + for (i = sdev->pwr_dom_count - 1; i >= 0; 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);
> > + }
> > +}
> > +
> > +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> > +{
> > + struct device *dev = sdev->dev.dev;
> > + int i;
> > +
> > + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node,
> > "power-domains",
> > +"#power-domain-cells");
> > + /*
> > +
Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
On 2023-09-10 22:16:05 +0200, Christophe JAILLET wrote:
> Le 10/09/2023 à 18:39, Janne Grunau via B4 Relay a écrit :
> > From: Janne Grunau
> >
> > Multiple power domains need to be handled explicitly in each driver. The
> > driver core can not handle it automatically since it is not aware of
> > power sequencing requirements the hardware might have. This is not a
> > problem for simpledrm since everything is expected to be powered on by
> > the bootloader. simpledrm has just ensure it remains powered on during
> > its lifetime.
> > This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> > systems. The HDMI output initialized by the bootloader requires keeping
> > the display controller and a DP phy power domain on.
> >
> > Signed-off-by: Janne Grunau
> > ---
> > drivers/gpu/drm/tiny/simpledrm.c | 106
> > +++
> > 1 file changed, 106 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c
> > b/drivers/gpu/drm/tiny/simpledrm.c
> > index ff86ba1ae1b8..efedede57d42 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -6,6 +6,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > @@ -227,6 +228,12 @@ struct simpledrm_device {
> > unsigned int regulator_count;
> > struct regulator **regulators;
> > #endif
> > + /* power-domains */
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > + int pwr_dom_count;
> > + struct device **pwr_dom_devs;
> > + struct device_link **pwr_dom_links;
> > +#endif
> > /* simplefb settings */
> > struct drm_display_mode mode;
> > @@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct
> > simpledrm_device *sdev)
> > }
> > #endif
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > +/*
> > + * Generic power domain handling code.
> > + *
> > + * Here we handle the power-domains properties of our "simple-framebuffer"
> > + * dt node. This is only necessary if there is more than one power-domain.
> > + * A single power-domains is handled automatically by the driver core.
> > Multiple
> > + * power-domains have to be handled by drivers since the driver core can't
> > know
> > + * the correct power sequencing. Power sequencing is not an issue for
> > simpledrm
> > + * since the bootloader has put the power domains already in the correct
> > state.
> > + * simpledrm has only to ensure they remain active for its lifetime.
> > + *
> > + * When the driver unloads, we detach from the power-domains.
> > + *
> > + * We only complain about errors here, no action is taken as the most
> > likely
> > + * error can only happen due to a mismatch between the bootloader which set
> > + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> > + * device tree. Chances are that there are no adverse effects, and if
> > there are,
> > + * a clean teardown of the fb probe will not help us much either. So just
> > + * complain and carry on, and hope that the user actually gets a working
> > fb at
> > + * the end of things.
> > + */
> > +static void simpledrm_device_detach_genpd(void *res)
> > +{
> > + int i;
> > + struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
> > +
> > +
> > + drm_err(>dev, "% power-domains count:%d\n", __func__,
> > sdev->pwr_dom_count);
> > + if (sdev->pwr_dom_count <= 1)
> > + return;
> > +
> > + for (i = sdev->pwr_dom_count - 1; i >= 0; 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);
> > + }
> > +}
> > +
> > +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> > +{
> > + struct device *dev = sdev->dev.dev;
> > + int i;
> > +
> > + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node,
> > "power-domains",
> > +"#power-domain-cells");
> > + /*
> > +* Single power-domain devices are handled by driver core nothing to do
> > +* here. The s
Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
On 2023-09-10 18:39:39 +0200, Janne Grunau via B4 Relay wrote:
> From: Janne Grunau
>
> Multiple power domains need to be handled explicitly in each driver. The
> driver core can not handle it automatically since it is not aware of
> power sequencing requirements the hardware might have. This is not a
> problem for simpledrm since everything is expected to be powered on by
> the bootloader. simpledrm has just ensure it remains powered on during
> its lifetime.
> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> systems. The HDMI output initialized by the bootloader requires keeping
> the display controller and a DP phy power domain on.
>
> Signed-off-by: Janne Grunau
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 106
> +++
> 1 file changed, 106 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
> b/drivers/gpu/drm/tiny/simpledrm.c
> index ff86ba1ae1b8..efedede57d42 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -227,6 +228,12 @@ struct simpledrm_device {
> unsigned int regulator_count;
> struct regulator **regulators;
> #endif
> + /* power-domains */
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> + int pwr_dom_count;
> + struct device **pwr_dom_devs;
> + struct device_link **pwr_dom_links;
> +#endif
>
> /* simplefb settings */
> struct drm_display_mode mode;
> @@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct
> simpledrm_device *sdev)
> }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +/*
> + * Generic power domain handling code.
> + *
> + * Here we handle the power-domains properties of our "simple-framebuffer"
> + * dt node. This is only necessary if there is more than one power-domain.
> + * A single power-domains is handled automatically by the driver core.
> Multiple
> + * power-domains have to be handled by drivers since the driver core can't
> know
> + * the correct power sequencing. Power sequencing is not an issue for
> simpledrm
> + * since the bootloader has put the power domains already in the correct
> state.
> + * simpledrm has only to ensure they remain active for its lifetime.
> + *
> + * When the driver unloads, we detach from the power-domains.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> + * device tree. Chances are that there are no adverse effects, and if there
> are,
> + * a clean teardown of the fb probe will not help us much either. So just
> + * complain and carry on, and hope that the user actually gets a working fb
> at
> + * the end of things.
> + */
> +static void simpledrm_device_detach_genpd(void *res)
> +{
> + int i;
> + struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
commented cast, removed locally for v2
> +
> +
> + drm_err(>dev, "% power-domains count:%d\n", __func__,
> sdev->pwr_dom_count);
broken log statement as pointed out by kernel test robot, not ment to be
included in this change. removed locally for v2
> + if (sdev->pwr_dom_count <= 1)
> + return;
> +
> + for (i = sdev->pwr_dom_count - 1; i >= 0; 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);
> + }
> +}
> +
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + struct device *dev = sdev->dev.dev;
> + int i;
> +
> + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node,
> "power-domains",
> + "#power-domain-cells");
> + /*
> + * Single power-domain devices are handled by driver core nothing to do
> + * here. The same for device nodes without "power-domains" property.
> + */
> + if (sdev->pwr_dom_count <= 1)
> + return 0;
> +
> + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> +sizeof(*sdev->pwr_dom_devs),
> +GFP_KERNEL
[PATCH] drm/simpledrm: Add support for multiple "power-domains"
From: Janne Grunau
Multiple power domains need to be handled explicitly in each driver. The
driver core can not handle it automatically since it is not aware of
power sequencing requirements the hardware might have. This is not a
problem for simpledrm since everything is expected to be powered on by
the bootloader. simpledrm has just ensure it remains powered on during
its lifetime.
This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
systems. The HDMI output initialized by the bootloader requires keeping
the display controller and a DP phy power domain on.
Signed-off-by: Janne Grunau
---
drivers/gpu/drm/tiny/simpledrm.c | 106 +++
1 file changed, 106 insertions(+)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index ff86ba1ae1b8..efedede57d42 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include
@@ -227,6 +228,12 @@ struct simpledrm_device {
unsigned int regulator_count;
struct regulator **regulators;
#endif
+ /* power-domains */
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+ int pwr_dom_count;
+ struct device **pwr_dom_devs;
+ struct device_link **pwr_dom_links;
+#endif
/* simplefb settings */
struct drm_display_mode mode;
@@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct
simpledrm_device *sdev)
}
#endif
+#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
+/*
+ * Generic power domain handling code.
+ *
+ * Here we handle the power-domains properties of our "simple-framebuffer"
+ * dt node. This is only necessary if there is more than one power-domain.
+ * A single power-domains is handled automatically by the driver core. Multiple
+ * power-domains have to be handled by drivers since the driver core can't know
+ * the correct power sequencing. Power sequencing is not an issue for simpledrm
+ * since the bootloader has put the power domains already in the correct state.
+ * simpledrm has only to ensure they remain active for its lifetime.
+ *
+ * When the driver unloads, we detach from the power-domains.
+ *
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up the "simple-framebuffer" dt node, and the PM domain providers in the
+ * device tree. Chances are that there are no adverse effects, and if there
are,
+ * a clean teardown of the fb probe will not help us much either. So just
+ * complain and carry on, and hope that the user actually gets a working fb at
+ * the end of things.
+ */
+static void simpledrm_device_detach_genpd(void *res)
+{
+ int i;
+ struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
+
+
+ drm_err(>dev, "% power-domains count:%d\n", __func__,
sdev->pwr_dom_count);
+ if (sdev->pwr_dom_count <= 1)
+ return;
+
+ for (i = sdev->pwr_dom_count - 1; i >= 0; 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);
+ }
+}
+
+static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
+{
+ struct device *dev = sdev->dev.dev;
+ int i;
+
+ sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node,
"power-domains",
+"#power-domain-cells");
+ /*
+* Single power-domain devices are handled by driver core nothing to do
+* here. The same for device nodes without "power-domains" property.
+*/
+ if (sdev->pwr_dom_count <= 1)
+ return 0;
+
+ sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
+ sizeof(*sdev->pwr_dom_devs),
+ GFP_KERNEL);
+ if (!sdev->pwr_dom_devs)
+ return -ENOMEM;
+
+ sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
+ sizeof(*sdev->pwr_dom_links),
+ GFP_KERNEL);
+ if (!sdev->pwr_dom_links)
+ return -ENOMEM;
+
+ for (i = 0; i < sdev->pwr_dom_count; i++) {
+ sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
+ if (IS_ERR(sdev->pwr_dom_devs[i])) {
+ int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
+ if (ret == -EPROBE_DEFER) {
+ simpledrm_
Re: [PATCH v6 3/3] drm/bridge_connector: implement oob_hotplug_event
On 2023-07-09 23:25:11 +0300, Dmitry Baryshkov wrote:
> Implement the oob_hotplug_event() callback. Translate it to the HPD
> notification sent to the HPD bridge in the chain.
>
> Signed-off-by: Dmitry Baryshkov
Reviewed-by: Janne Grunau
> ---
> drivers/gpu/drm/drm_bridge_connector.c | 29 +++---
> 1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c
> b/drivers/gpu/drm/drm_bridge_connector.c
> index 84d8d310ef04..364f6e37fbdc 100644
> --- a/drivers/gpu/drm/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -5,6 +5,8 @@
>
> #include
> #include
> +#include
> +#include
> #include
>
> #include
> @@ -107,10 +109,9 @@ static void drm_bridge_connector_hpd_notify(struct
> drm_connector *connector,
> }
> }
>
> -static void drm_bridge_connector_hpd_cb(void *cb_data,
> - enum drm_connector_status status)
> +static void drm_bridge_connector_handle_hpd(struct drm_bridge_connector
> *drm_bridge_connector,
> + enum drm_connector_status status)
> {
> - struct drm_bridge_connector *drm_bridge_connector = cb_data;
> struct drm_connector *connector = _bridge_connector->base;
> struct drm_device *dev = connector->dev;
>
> @@ -123,6 +124,21 @@ static void drm_bridge_connector_hpd_cb(void *cb_data,
> drm_kms_helper_hotplug_event(dev);
> }
>
> +static void drm_bridge_connector_hpd_cb(void *cb_data,
> + enum drm_connector_status status)
> +{
> + drm_bridge_connector_handle_hpd(cb_data, status);
> +}
> +
> +static void drm_bridge_connector_oob_hotplug_event(struct drm_connector
> *connector,
> +enum drm_connector_status
> status)
> +{
> + struct drm_bridge_connector *bridge_connector =
> + to_drm_bridge_connector(connector);
> +
> + drm_bridge_connector_handle_hpd(bridge_connector, status);
> +}
> +
> static void drm_bridge_connector_enable_hpd(struct drm_connector *connector)
> {
> struct drm_bridge_connector *bridge_connector =
> @@ -216,6 +232,7 @@ static const struct drm_connector_funcs
> drm_bridge_connector_funcs = {
> .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> .debugfs_init = drm_bridge_connector_debugfs_init,
> + .oob_hotplug_event = drm_bridge_connector_oob_hotplug_event,
> };
>
> /*
> -
> @@ -351,6 +368,12 @@ struct drm_connector *drm_bridge_connector_init(struct
> drm_device *drm,
> if (!drm_bridge_get_next_bridge(bridge))
> connector_type = bridge->type;
>
> +#ifdef CONFIG_OF
> + if (!drm_bridge_get_next_bridge(bridge) &&
> + bridge->of_node)
> + connector->fwnode =
> fwnode_handle_get(of_fwnode_handle(bridge->of_node));
> +#endif
> +
> if (bridge->ddc)
> ddc = bridge->ddc;
>
> --
> 2.39.2
>
Re: [PATCH v6 2/3] drm/bridge_connector: stop filtering events in drm_bridge_connector_hpd_cb()
On 2023-07-09 23:25:10 +0300, Dmitry Baryshkov wrote: > In some cases the bridge drivers would like to receive hotplug events > even in the case new status is equal to the old status. In the DP case > this is used to deliver "attention" messages to the DP host. Stop > filtering the events in the drm_bridge_connector_hpd_cb() and let > drivers decide whether they would like to receive the event or not. Worth mentioning in the commit message that all current bridges which set .hpd_notify appear to be safe w.r.t multiple calls with the same status. meson_encoder_hdmi.c will do unnecessary work when called repeatedly with status connected. Not sure if it is worth adressing since I don't expect multiple calls with connector_status_connected to occur for this particular bridge. Reviewed-By: Janne Grunau > Signed-off-by: Dmitry Baryshkov > --- > drivers/gpu/drm/drm_bridge_connector.c | 5 - > 1 file changed, 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c > b/drivers/gpu/drm/drm_bridge_connector.c > index 19ae4a177ac3..84d8d310ef04 100644 > --- a/drivers/gpu/drm/drm_bridge_connector.c > +++ b/drivers/gpu/drm/drm_bridge_connector.c > @@ -113,16 +113,11 @@ static void drm_bridge_connector_hpd_cb(void *cb_data, > struct drm_bridge_connector *drm_bridge_connector = cb_data; > struct drm_connector *connector = _bridge_connector->base; > struct drm_device *dev = connector->dev; > - enum drm_connector_status old_status; > > mutex_lock(>mode_config.mutex); > - old_status = connector->status; > connector->status = status; > mutex_unlock(>mode_config.mutex); > > - if (old_status == status) > - return; > - > drm_bridge_connector_hpd_notify(connector, status); > > drm_kms_helper_hotplug_event(dev); > -- > 2.39.2 >
Re: [PATCH] drm/probe_helper: fix the warning reported when calling drm_kms_helper_poll_disable during suspend
On 2023-04-20 23:07:01 +0300, Dmitry Baryshkov wrote: > On Thu, 20 Apr 2023 at 23:01, Janne Grunau wrote: > > > > On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote: > > > When drivers call drm_kms_helper_poll_disable from > > > their device suspend implementation without enabled output polling before, > > > following warning will be reported,due to work->func not be initialized: > > > > we see the same warning with the wpork in progress kms driver for apple > > silicon SoCs. The connectors do not need to polled so the driver never > > calls drm_kms_helper_poll_init(). > > > > > [ 55.141361] WARNING: CPU: 3 PID: 372 at kernel/workqueue.c:3066 > > > __flush_work+0x22f/0x240 > > > [ 55.141382] Modules linked in: nls_iso8859_1 snd_hda_codec_generic > > > ledtrig_audio snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi > > > snd_hda_codec snd_hda_core snd_hwdep snd_pcm snd_seq_midi > > > snd_seq_midi_event snd_rawmidi snd_seq intel_rapl_msr intel_rapl_common > > > bochs drm_vram_helper drm_ttm_helper snd_seq_device nfit ttm > > > crct10dif_pclmul snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3 > > > aesni_intel drm_kms_helper joydev input_leds syscopyarea crypto_simd snd > > > cryptd sysfillrect sysimgblt mac_hid serio_raw soundcore qemu_fw_cfg > > > sch_fq_codel msr parport_pc ppdev lp parport drm ramoops reed_solomon > > > pstore_blk pstore_zone efi_pstore virtio_rng ip_tables x_tables autofs4 > > > hid_generic usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse > > > virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover virtio_blk > > > xhci_pci_renesas failover > > > [ 55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not tainted 6.2.0-rc6+ > > > #16 > > > [ 55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > > > rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014 > > > [ 55.141435] Workqueue: events_unbound async_run_entry_fn > > > [ 55.141441] RIP: 0010:__flush_work+0x22f/0x240 > > > [ 55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff ff 4c 89 f7 > > > e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff ff 0f 0b e9 0a ff ff > > > ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2 54 d8 00 66 90 90 90 90 90 90 > > > [ 55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246 > > > [ 55.141449] RAX: RBX: RCX: > > > 9b72bcbe > > > [ 55.141450] RDX: 0001 RSI: 0001 RDI: > > > ff3ea01e4265e330 > > > [ 55.141451] RBP: ff59221940833c90 R08: R09: > > > 8080808080808080 > > > [ 55.141453] R10: ff3ea01e42b3caf4 R11: 000f R12: > > > ff3ea01e4265e330 > > > [ 55.141454] R13: 0001 R14: ff3ea01e505e5e80 R15: > > > 0001 > > > [ 55.141455] FS: () GS:ff3ea01fb7cc() > > > knlGS: > > > [ 55.141456] CS: 0010 DS: ES: CR0: 80050033 > > > [ 55.141458] CR2: 563543ad1546 CR3: 00010ee82005 CR4: > > > 00771ee0 > > > [ 55.141464] DR0: DR1: DR2: > > > > > > [ 55.141465] DR3: DR6: fffe0ff0 DR7: > > > 0400 > > > [ 55.141466] PKRU: 5554 > > > [ 55.141467] Call Trace: > > > [ 55.141469] > > > [ 55.141472] ? pcie_wait_cmd+0xdf/0x220 > > > [ 55.141478] ? mptcp_seq_show+0xe0/0x180 > > > [ 55.141484] __cancel_work_timer+0x124/0x1b0 > > > [ 55.141487] cancel_delayed_work_sync+0x17/0x20 > > > [ 55.141490] drm_kms_helper_poll_disable+0x26/0x40 [drm_kms_helper] > > > [ 55.141516] drm_mode_config_helper_suspend+0x25/0x90 [drm_kms_helper] > > > [ 55.141531] ? __pm_runtime_resume+0x64/0x90 > > > [ 55.141536] bochs_pm_suspend+0x16/0x20 [bochs] > > > [ 55.141540] pci_pm_suspend+0x8b/0x1b0 > > > [ 55.141545] ? __pfx_pci_pm_suspend+0x10/0x10 > > > [ 55.141547] dpm_run_callback+0x4c/0x160 > > > [ 55.141550] __device_suspend+0x14c/0x4c0 > > > [ 55.141553] async_suspend+0x24/0xa0 > > > [ 55.141555] async_run_entry_fn+0x34/0x120 > > > [ 55.141557] process_one_work+0x21a/0x3f0 > > > [ 55.141560] worker_thread+0x4e/0x3c0 > > > [ 55.141563] ? __pfx_worker_thread+0x10/0x10 > > > [ 55.141565] kthread+0xf2/0x120 > > > [ 55.141568] ? __pfx_kthread+0x10/0x10 &
Re: [PATCH] drm/probe_helper: fix the warning reported when calling drm_kms_helper_poll_disable during suspend
if (dev->mode_config.poll_enabled) > + cancel_delayed_work_sync(>mode_config.output_poll_work); Checking for dev->mode_config.poll_enabled at the start of the function and return early if it is not true looks more in style with the rest of drm_probe_helper.c. No difference functionally of course. Tested with the apple kms driver. Reviewed-by: Janne Grunau ciao Janne
Re: [RFC PATCH] drm/simpledrm: Allow physical width and height configuration via DT
Hej,
adding devicet...@vger.kernel.org and as...@lists.linux.dev to cc:, the
former for the obvious devictree/bindings related questions,
asahi for the prospect of supporting high DPI displays during early boot
and in u-boot.
On 2023-01-18 18:48:17 +, Rayyan Ansari wrote:
> Hello,
> The following draft patch adds support for configuring the
> height-mm and width-mm DRM properties in the simpledrm driver
> via devicetree.
> This is useful to get proper scaling in UIs such as Phosh.
> An example of using this property is this, taken from my local tree:
>
> framebuffer0: framebuffer@320 {
> compatible = "simple-framebuffer";
> reg = <0x320 0x80>;
> format = "a8r8g8b8";
> width = <720>;
> height = <1280>;
> stride = <(720 * 4)>;
> width-mm = /bits/ 16 <58>;
> height-mm = /bits/ 16 <103>;
>
> clocks = < MDSS_AHB_CLK>,
>< MDSS_AXI_CLK>,
>< MDSS_BYTE0_CLK>,
>< MDSS_MDP_CLK>,
>< MDSS_PCLK0_CLK>,
>< MDSS_VSYNC_CLK>;
> power-domains = < MDSS_GDSC>;
> };
>
> I have tested this on my Lumia 735, and it does indeed
> allow Phosh to scale correctly on the screen.
>
> However, I would like to get some feedback before I write the
> documentation.
> - What data type should be used?
> The width_mm and height_mm properties of the drm_display_mode
> struct are defined as u16. I have also made the devicetree
> properties as the u16 type, but this requires specifying
> "/bits/ 16" before the value. Should u32 be used instead to get
> rid of this? If so, how could the conversion from u32->u16 be
> handled?
u32 is the appropriate type. The device tree describes the hardware and
not the data types used in a "random" linux driver/subsystem. 65m is
probably enough for all practical purposes but u32 is the better choice.
Documentation/devicetree/bindings/display/panel/panel-common.yaml
already specifies "height-mm" and "width-mm" and all device tree files
using this binding code the properties as u32.
We probably do not want add height and width properties to the
simple-framebuffer node directly. At least for the static case I would
expect that it duplicates information already present in a panel node.
For that case parsing the panel dimensions via a phandle reference to
that panel node would be preferred.
I'm not sure if it worth considering the dynamic case. The bootloader
may be able to provide dimensions of HDMI, DP, ... connected displays
from the EDID. In that case "height-mm" and "width-mm" properties would
make sense.
The existing panel drivers seem to ignore the u32 -> u16 conversion
problem.
> - Style?
> I have split the arguments to the DRM_MODE_INIT macro across
> multiple lines to increase readability. I'm not sure if this
> is the correct style though.
I think the code would be more readable if width_mm and height_mm would
be calculated outside of DRM_MODE_INIT if they are zero.
> - Anything else?
> This is my first time writing code for a Linux driver, so I
> would be grateful if you have any suggestions for improvements.
Documentation/devicetree/bindings/display/simple-framebuffer.yaml needs
to be updates to list and document the properties added to the node.
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 49 +++-
> 1 file changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
> b/drivers/gpu/drm/tiny/simpledrm.c
> index 162eb44dcba8..92109f870b35 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -116,6 +116,15 @@ simplefb_get_format_pd(struct drm_device *dev,
> return simplefb_get_validated_format(dev, pd->format);
> }
>
> +static void
> +simplefb_read_u16_of_optional(struct drm_device *dev, struct device_node
> *of_node,
> + const char *name, u16 *value)
> +{
> + int ret = of_property_read_u16(of_node, name, value);
> + if (ret)
> + value = 0;
> +}
> +
> static int
> simplefb_read_u32_of(struct drm_device *dev, struct device_node *of_node,
>const char *name, u32 *value)
> @@ -184,6 +193,21 @@ simplefb_get_format_of(struct drm_device *dev, struct
> device_node *of_node)
> return simplefb_get_validated_format(dev, format);
> }
>
> +static u16
> +simplefb_get_width_mm_of(struct drm_device *dev, struct device_node *of_node)
> +{
> + u16 width_mm;
> + simplefb_read_u16_of_optional(dev, of_node, "width-mm", _mm);
> + return width_mm;
> +}
> +
> +static u16
> +simplefb_get_height_mm_of(struct drm_device *dev,
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On 2022-03-31 18:25:05 +0200, Thierry Reding wrote: > On Fri, Feb 11, 2022 at 12:15:44AM +0100, Janne Grunau wrote: > > On 2022-02-09 17:31:16 +0100, Thierry Reding wrote: > > > On Sun, Feb 06, 2022 at 11:27:00PM +0100, Janne Grunau wrote: > > > > On 2021-09-15 17:19:39 +0200, Thierry Reding wrote: > > > > > On Tue, Sep 07, 2021 at 07:44:44PM +0200, Thierry Reding wrote: > > > > > > On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote: > > > > > > > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding > > > > > > > wrote: > > > > > > > > > > > > > > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote: > > > > > > > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote: > > > > > > > > > > > > > > > > > > > > > > Couldn't we keep this all in /reserved-memory? Just add > > > > > > > > > > > an iova > > > > > > > > > > > version of reg. Perhaps abuse 'assigned-address' for this > > > > > > > > > > > purpose. The > > > > > > > > > > > issue I see would be handling reserved iova areas without > > > > > > > > > > > a physical > > > > > > > > > > > area. That can be handled with just a iova and no reg. We > > > > > > > > > > > already have > > > > > > > > > > > a no reg case. > > > > > > > > > > > > > > > > > > > > I had thought about that initially. One thing I'm worried > > > > > > > > > > about is that > > > > > > > > > > every child node in /reserved-memory will effectively cause > > > > > > > > > > the memory > > > > > > > > > > that it described to be reserved. But we don't want that > > > > > > > > > > for regions > > > > > > > > > > that are "virtual only" (i.e. IOMMU reservations). > > > > > > > > > > > > > > > > > > By virtual only, you mean no physical mapping, just a region > > > > > > > > > of > > > > > > > > > virtual space, right? For that we'd have no 'reg' and > > > > > > > > > therefore no > > > > > > > > > (physical) reservation by the OS. It's similar to non-static > > > > > > > > > regions. > > > > > > > > > You need a specific handler for them. We'd probably want a > > > > > > > > > compatible > > > > > > > > > as well for these virtual reservations. > > > > > > > > > > > > > > > > Yeah, these would be purely used for reserving regions in the > > > > > > > > IOVA so > > > > > > > > that they won't be used by the IOVA allocator. Typically these > > > > > > > > would be > > > > > > > > used for cases where those addresses have some special meaning. > > > > > > > > > > > > > > > > Do we want something like: > > > > > > > > > > > > > > > > compatible = "iommu-reserved"; > > > > > > > > > > > > > > > > for these? Or would that need to be: > > > > > > > > > > > > > > > > compatible = "linux,iommu-reserved"; > > > > > > > > > > > > > > > > ? There seems to be a mix of vendor-prefix vs. non-vendor-prefix > > > > > > > > compatible strings in the reserved-memory DT bindings directory. > > > > > > > > > > > > > > I would not use 'linux,' here. > > > > > > > > > > > > > > > > > > > > > > > On the other hand, do we actually need the compatible string? > > > > > > > > Because we > > > > > > > > don't really want to associate much extra information
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On 2022-02-09 17:31:16 +0100, Thierry Reding wrote: > On Sun, Feb 06, 2022 at 11:27:00PM +0100, Janne Grunau wrote: > > On 2021-09-15 17:19:39 +0200, Thierry Reding wrote: > > > On Tue, Sep 07, 2021 at 07:44:44PM +0200, Thierry Reding wrote: > > > > On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote: > > > > > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding > > > > > wrote: > > > > > > > > > > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote: > > > > > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding > > > > > > > wrote: > > > > > > > > > > > > > > > > On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote: > > > > > > > > > > > > > > > > > > Couldn't we keep this all in /reserved-memory? Just add an > > > > > > > > > iova > > > > > > > > > version of reg. Perhaps abuse 'assigned-address' for this > > > > > > > > > purpose. The > > > > > > > > > issue I see would be handling reserved iova areas without a > > > > > > > > > physical > > > > > > > > > area. That can be handled with just a iova and no reg. We > > > > > > > > > already have > > > > > > > > > a no reg case. > > > > > > > > > > > > > > > > I had thought about that initially. One thing I'm worried about > > > > > > > > is that > > > > > > > > every child node in /reserved-memory will effectively cause the > > > > > > > > memory > > > > > > > > that it described to be reserved. But we don't want that for > > > > > > > > regions > > > > > > > > that are "virtual only" (i.e. IOMMU reservations). > > > > > > > > > > > > > > By virtual only, you mean no physical mapping, just a region of > > > > > > > virtual space, right? For that we'd have no 'reg' and therefore no > > > > > > > (physical) reservation by the OS. It's similar to non-static > > > > > > > regions. > > > > > > > You need a specific handler for them. We'd probably want a > > > > > > > compatible > > > > > > > as well for these virtual reservations. > > > > > > > > > > > > Yeah, these would be purely used for reserving regions in the IOVA > > > > > > so > > > > > > that they won't be used by the IOVA allocator. Typically these > > > > > > would be > > > > > > used for cases where those addresses have some special meaning. > > > > > > > > > > > > Do we want something like: > > > > > > > > > > > > compatible = "iommu-reserved"; > > > > > > > > > > > > for these? Or would that need to be: > > > > > > > > > > > > compatible = "linux,iommu-reserved"; > > > > > > > > > > > > ? There seems to be a mix of vendor-prefix vs. non-vendor-prefix > > > > > > compatible strings in the reserved-memory DT bindings directory. > > > > > > > > > > I would not use 'linux,' here. > > > > > > > > > > > > > > > > > On the other hand, do we actually need the compatible string? > > > > > > Because we > > > > > > don't really want to associate much extra information with this > > > > > > like we > > > > > > do for example with "shared-dma-pool". The logic to handle this > > > > > > would > > > > > > all be within the IOMMU framework. All we really need is for the > > > > > > standard reservation code to skip nodes that don't have a reg > > > > > > property > > > > > > so we don't reserve memory for "virtual-only" allocations. > > > > > > > > > > It doesn't hurt to have one and I can imagine we might want to iterate > > > > > over all the nodes. It's slightly easier and more common to iterate > > > > > over compatible nodes rather than nodes with some property. > > > > > > > &
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On 2021-09-15 17:19:39 +0200, Thierry Reding wrote:
> On Tue, Sep 07, 2021 at 07:44:44PM +0200, Thierry Reding wrote:
> > On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote:
> > > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding
> > > wrote:
> > > >
> > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote:
> > > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding
> > > > > wrote:
> > > > > >
> > > > > > On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote:
> > > > > > >
> > > > > > > Couldn't we keep this all in /reserved-memory? Just add an iova
> > > > > > > version of reg. Perhaps abuse 'assigned-address' for this
> > > > > > > purpose. The
> > > > > > > issue I see would be handling reserved iova areas without a
> > > > > > > physical
> > > > > > > area. That can be handled with just a iova and no reg. We already
> > > > > > > have
> > > > > > > a no reg case.
> > > > > >
> > > > > > I had thought about that initially. One thing I'm worried about is
> > > > > > that
> > > > > > every child node in /reserved-memory will effectively cause the
> > > > > > memory
> > > > > > that it described to be reserved. But we don't want that for regions
> > > > > > that are "virtual only" (i.e. IOMMU reservations).
> > > > >
> > > > > By virtual only, you mean no physical mapping, just a region of
> > > > > virtual space, right? For that we'd have no 'reg' and therefore no
> > > > > (physical) reservation by the OS. It's similar to non-static regions.
> > > > > You need a specific handler for them. We'd probably want a compatible
> > > > > as well for these virtual reservations.
> > > >
> > > > Yeah, these would be purely used for reserving regions in the IOVA so
> > > > that they won't be used by the IOVA allocator. Typically these would be
> > > > used for cases where those addresses have some special meaning.
> > > >
> > > > Do we want something like:
> > > >
> > > > compatible = "iommu-reserved";
> > > >
> > > > for these? Or would that need to be:
> > > >
> > > > compatible = "linux,iommu-reserved";
> > > >
> > > > ? There seems to be a mix of vendor-prefix vs. non-vendor-prefix
> > > > compatible strings in the reserved-memory DT bindings directory.
> > >
> > > I would not use 'linux,' here.
> > >
> > > >
> > > > On the other hand, do we actually need the compatible string? Because we
> > > > don't really want to associate much extra information with this like we
> > > > do for example with "shared-dma-pool". The logic to handle this would
> > > > all be within the IOMMU framework. All we really need is for the
> > > > standard reservation code to skip nodes that don't have a reg property
> > > > so we don't reserve memory for "virtual-only" allocations.
> > >
> > > It doesn't hurt to have one and I can imagine we might want to iterate
> > > over all the nodes. It's slightly easier and more common to iterate
> > > over compatible nodes rather than nodes with some property.
> > >
> > > > > Are these being global in DT going to be a problem? Presumably we have
> > > > > a virtual space per IOMMU. We'd know which IOMMU based on a device's
> > > > > 'iommus' and 'memory-region' properties, but within /reserved-memory
> > > > > we wouldn't be able to distinguish overlapping addresses from separate
> > > > > address spaces. Or we could have 2 different IOVAs for 1 physical
> > > > > space. That could be solved with something like this:
> > > > >
> > > > > iommu-addresses = < >;
> > > >
> > > > The only case that would be problematic would be if we have overlapping
> > > > physical regions, because that will probably trip up the standard code.
> > > >
> > > > But this could also be worked around by looking at iommu-addresses. For
> > > > example, if we had something like this:
> > > >
> > > > reserved-memory {
> > > > fb_dc0: fb@8000 {
> > > > reg = <0x8000 0x0100>;
> > > > iommu-addresses = <0xa000 0x0100>;
> > > > };
> > > >
> > > > fb_dc1: fb@8000 {
> > >
> > > You can't have 2 nodes with the same name (actually, you can, they
> > > just get merged together). Different names with the same unit-address
> > > is a dtc warning. I'd really like to make that a full blown
> > > overlapping region check.
> >
> > Right... so this would be a lot easier to deal with using that earlier
> > proposal where the IOMMU regions were a separate thing and referencing
> > the reserved-memory nodes. In those cases we could just have the
> > physical reservation for the framebuffer once (so we don't get any
> > duplicates or overlaps) and then have each IOVA reservation reference
> > that to create the mapping.
> >
> > >
> > > > reg = <0x8000 0x0100>;
> > > > iommu-addresses = <0xb000 0x0100>;
> > > > };
> > > > };
> > > >
> > > > We could make the code identify that
