Re: [PATCH v12 4/7] drm/meson: gate px_clk when setting rate

[Martin Blumenstingl]

Hi Neil,

On Wed, Apr 3, 2024 at 9:46 AM Neil Armstrong  wrote:
>
> Disable the px_clk when setting the rate to recover a fully
> configured and correctly reset VCLK clock tree after the rate
> is set.
>
> Fixes: 77d9e1e6b846 ("drm/meson: add support for MIPI-DSI transceiver")
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/gpu/drm/meson/meson_dw_mipi_dsi.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c 
> b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
> index a6bc1bdb3d0d..a10cff3ca1fe 100644
> --- a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
> @@ -95,6 +95,7 @@ static int dw_mipi_dsi_phy_init(void *priv_data)
> return ret;
> }
>
> +   clk_disable_unprepare(mipi_dsi->px_clk);
nit-pick: clk_disable(mipi_dsi->px_clk); should be enough here as my
understanding is that we only need to {un,}prepare a clock once.

> ret = clk_set_rate(mipi_dsi->px_clk, mipi_dsi->mode->clock * 1000);
>
> if (ret) {
> @@ -103,6 +104,12 @@ static int dw_mipi_dsi_phy_init(void *priv_data)
> return ret;
> }
>
> +   ret = clk_prepare_enable(mipi_dsi->px_clk);
> +   if (ret) {
> +   dev_err(mipi_dsi->dev, "Failed to enable DSI Pixel clock (ret 
> %d)\n", ret);
> +   return ret;
If we ever hit this error case then there will be a lot of additional
errors in the kernel log:
- initially the clock is prepared and enabled in
meson_dw_mipi_dsi_probe() by calling devm_clk_get_enabled()
- we then disable the clock above (generally disabling a clock is
expected to always succeed)
- if the clock can NOT be re-enabled here we just log the error
- in case a user tries to rmmod the driver (to modprobe it again) to
try and recover from an error the automatic disabling of the pix_clk
(based on devm_clk_get_enabled() where it was enabled initially) there
will be a splat because the clock is already disabled (and enabled
count is zero, so it cannot be disabled any further)

For the 32-bit SoC video clocks I keep track of them being enabled or
disabled, see [0], [1] and [2].
In my case this is important because we can run into cases where the
PLL doesn't lock (I am not sure how likely this is for your case).

It *seems* like we need to do something similar as
dw_mipi_dsi_phy_init() can be called when changing the display
resolution (or whenever drm_bridge_funcs.atomic_pre_enable) is called.
To illustrate what I have in mind I attached a diff (it's based on
this patch) - it's compile tested only as I have no DSI hardware.
In case dw_mipi_dsi_phy_init() is called only once per device
lifecycle things may get easier.

PS: I'm so happy that we don't need any clock notifiers for this!
So: good work with the clock driver bits.


Let me know what you think,
Martin


[0] 
https://github.com/xdarklight/linux/blob/meson-mx-integration-6.9-20240323/drivers/gpu/drm/meson/meson_vclk.c#L1177-L1179
[1] 
https://github.com/xdarklight/linux/blob/meson-mx-integration-6.9-20240323/drivers/gpu/drm/meson/meson_vclk.c#L1077
[2] 
https://github.com/xdarklight/linux/blob/meson-mx-integration-6.9-20240323/drivers/gpu/drm/meson/meson_vclk.c#L1053
diff --git a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
index a6bc1bdb3d0d..926618d0e622 100644
--- a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
+++ b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
@@ -46,6 +46,7 @@ struct meson_dw_mipi_dsi {
 	struct clk *bit_clk;
 	struct clk *px_clk;
 	struct reset_control *top_rst;
+	bool px_clk_enabled;
 };
 
 #define encoder_to_meson_dw_mipi_dsi(x) \
@@ -87,6 +88,11 @@ static int dw_mipi_dsi_phy_init(void *priv_data)
 		return ret;
 	}
 
+	if (mipi_dsi->px_clk_enabled) {
+		clk_disable(mipi_dsi->px_clk);
+		mipi_dsi->px_clk_enabled = false;
+	}
+
 	/* Make sure the rate of the bit clock is not modified by someone else */
 	ret = clk_rate_exclusive_get(mipi_dsi->bit_clk);
 	if (ret) {
@@ -103,6 +109,14 @@ static int dw_mipi_dsi_phy_init(void *priv_data)
 		return ret;
 	}
 
+	ret = clk_prepare_enable(mipi_dsi->px_clk);
+	if (ret) {
+		dev_err(mipi_dsi->dev, "Failed to enable DSI Pixel clock (ret %d)\n", ret);
+		return ret;
+	}
+
+	mipi_dsi->px_clk_enabled = true;
+
 	switch (mipi_dsi->dsi_device->format) {
 	case MIPI_DSI_FMT_RGB888:
 		dpi_data_format = DPI_COLOR_24BIT;
@@ -287,7 +301,7 @@ static int meson_dw_mipi_dsi_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, ret, "Unable to get enabled bit_clk\n");
 	}
 
-	mipi_dsi->px_clk = devm_clk_get_enabled(dev, "px");
+	mipi_dsi->px_clk = devm_clk_get_prepared(dev, "px");
 	if (IS_ERR(mipi_dsi->px_clk))
 		return dev_err_probe(dev, PTR_ERR(mipi_dsi->px_clk),
  "Unable to get enabled px_clk\n");
@@ -327,6 +341,9 @@ static void meson_dw_mipi_dsi_remove(struct platform_device *pdev)
 {
 	struct meson_dw_mipi_dsi *mipi_dsi = platform_get_drvdata(pdev);
 
+	if (mipi_dsi->px_clk_enabled)
+		

Re: [PATCH 19/21] drm/meson: Allow build with COMPILE_TEST=y

[Martin Blumenstingl]

On Mon, Apr 8, 2024 at 7:05 PM Ville Syrjala
 wrote:
>
> From: Ville Syrjälä 
>
> Allow meson to be built with COMPILE_TEST=y for greater
> coverage. Builds fine on x86/x86_64 at least.
>
> Cc: Neil Armstrong 
> Cc: linux-amlo...@lists.infradead.org
> Signed-off-by: Ville Syrjälä 
Thank you for spotting this Ville!

> ---
>  drivers/gpu/drm/meson/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/meson/Kconfig b/drivers/gpu/drm/meson/Kconfig
> index 5520b9e3f010..d8f67bd9c755 100644
> --- a/drivers/gpu/drm/meson/Kconfig
> +++ b/drivers/gpu/drm/meson/Kconfig
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config DRM_MESON
> tristate "DRM Support for Amlogic Meson Display Controller"
> -   depends on DRM && OF && (ARM || ARM64)
> +   depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST)
Neil, I wonder if we should just have "depends on DRM && OF" here as
the next line already has:
> depends on ARCH_MESON || COMPILE_TEST
ARCH_MESON is only defined for ARM and ARM64

[PATCH v1] drm/meson: improve encoder probe / initialization error handling

[Martin Blumenstingl]

Rename meson_encoder_{cvbs,dsi,hdmi}_init() to
meson_encoder_{cvbs,dsi,hdmi}_probe() so it's clear that these functions
are used at probe time during driver initialization. Also switch all
error prints inside those functions to use dev_err_probe() for
consistency.

This makes the code more straight forward to read and makes the error
prints within those functions consistent (by logging all -EPROBE_DEFER
with dev_dbg(), while actual errors are logged with dev_err() and get
the error value printed).

Signed-off-by: Martin Blumenstingl 
---
This is meant to be applied on top of my other patch called
"drm/meson: Don't remove bridges which are created by other drivers" [0]


[0] 
https://lore.kernel.org/dri-devel/20240215220442.1343152-1-martin.blumensti...@googlemail.com/


 drivers/gpu/drm/meson/meson_drv.c  |  6 +++---
 drivers/gpu/drm/meson/meson_encoder_cvbs.c | 24 ++
 drivers/gpu/drm/meson/meson_encoder_cvbs.h |  2 +-
 drivers/gpu/drm/meson/meson_encoder_dsi.c  | 23 +
 drivers/gpu/drm/meson/meson_encoder_dsi.h  |  2 +-
 drivers/gpu/drm/meson/meson_encoder_hdmi.c | 15 +++---
 drivers/gpu/drm/meson/meson_encoder_hdmi.h |  2 +-
 7 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_drv.c 
b/drivers/gpu/drm/meson/meson_drv.c
index cb674966e9ac..17a5cca007e2 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -312,7 +312,7 @@ static int meson_drv_bind_master(struct device *dev, bool 
has_components)
 
/* Encoder Initialization */
 
-   ret = meson_encoder_cvbs_init(priv);
+   ret = meson_encoder_cvbs_probe(priv);
if (ret)
goto exit_afbcd;
 
@@ -326,12 +326,12 @@ static int meson_drv_bind_master(struct device *dev, bool 
has_components)
}
}
 
-   ret = meson_encoder_hdmi_init(priv);
+   ret = meson_encoder_hdmi_probe(priv);
if (ret)
goto exit_afbcd;
 
if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
-   ret = meson_encoder_dsi_init(priv);
+   ret = meson_encoder_dsi_probe(priv);
if (ret)
goto exit_afbcd;
}
diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c 
b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
index 3407450435e2..d1191de855d9 100644
--- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
+++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
@@ -219,7 +219,7 @@ static const struct drm_bridge_funcs 
meson_encoder_cvbs_bridge_funcs = {
.atomic_reset = drm_atomic_helper_bridge_reset,
 };
 
-int meson_encoder_cvbs_init(struct meson_drm *priv)
+int meson_encoder_cvbs_probe(struct meson_drm *priv)
 {
struct drm_device *drm = priv->drm;
struct meson_encoder_cvbs *meson_encoder_cvbs;
@@ -240,10 +240,9 @@ int meson_encoder_cvbs_init(struct meson_drm *priv)
 
meson_encoder_cvbs->next_bridge = of_drm_find_bridge(remote);
of_node_put(remote);
-   if (!meson_encoder_cvbs->next_bridge) {
-   dev_err(priv->dev, "Failed to find CVBS Connector bridge\n");
-   return -EPROBE_DEFER;
-   }
+   if (!meson_encoder_cvbs->next_bridge)
+   return dev_err_probe(priv->dev, -EPROBE_DEFER,
+"Failed to find CVBS Connector bridge\n");
 
/* CVBS Encoder Bridge */
meson_encoder_cvbs->bridge.funcs = _encoder_cvbs_bridge_funcs;
@@ -259,10 +258,9 @@ int meson_encoder_cvbs_init(struct meson_drm *priv)
/* Encoder */
ret = drm_simple_encoder_init(priv->drm, _encoder_cvbs->encoder,
  DRM_MODE_ENCODER_TVDAC);
-   if (ret) {
-   dev_err(priv->dev, "Failed to init CVBS encoder: %d\n", ret);
-   return ret;
-   }
+   if (ret)
+   return dev_err_probe(priv->dev, ret,
+"Failed to init CVBS encoder\n");
 
meson_encoder_cvbs->encoder.possible_crtcs = BIT(0);
 
@@ -276,10 +274,10 @@ int meson_encoder_cvbs_init(struct meson_drm *priv)
 
/* Initialize & attach Bridge Connector */
connector = drm_bridge_connector_init(priv->drm, 
_encoder_cvbs->encoder);
-   if (IS_ERR(connector)) {
-   dev_err(priv->dev, "Unable to create CVBS bridge connector\n");
-   return PTR_ERR(connector);
-   }
+   if (IS_ERR(connector))
+   return dev_err_probe(priv->dev, PTR_ERR(connector),
+"Unable to create CVBS bridge 
connector\n");
+
drm_connector_attach_encoder(connector, _encoder_cvbs->encoder);
 
priv->encoders[MESON_ENC_CVBS] = meson_encoder_cvbs;
diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.h 
b/drivers/gpu/drm/meson/meson_enc

[PATCH] drm/meson: Don't remove bridges which are created by other drivers

[Martin Blumenstingl]

Stop calling drm_bridge_remove() for bridges allocated/managed by other
drivers in the remove paths of meson_encoder_{cvbs,dsi,hdmi}.
drm_bridge_remove() unregisters the bridge so it cannot be used
anymore. Doing so for bridges we don't own can lead to the video
pipeline not being able to come up after -EPROBE_DEFER of the VPU
because we're unregistering a bridge that's managed by another driver.
The other driver doesn't know that we have unregistered it's bridge
and on subsequent .probe() we're not able to find those bridges anymore
(since nobody re-creates them).

This fixes probe errors on Meson8b boards with the CVBS outputs enabled.

Fixes: 09847723c12f ("drm/meson: remove drm bridges at aggregate driver unbind 
time")
Fixes: 42dcf15f901c ("drm/meson: add DSI encoder")
Cc: sta...@vger.kernel.org
Reported-by: Steve Morvai 
Signed-off-by: Martin Blumenstingl 
---
This issue was reported by Steve off-list to me (thanks again for your
patience and sorry it took so long)!
The Meson8b VPU driver is not upstream, but the problematic code is.
Meaning: This issue can also appear on SoCs which are supported
upstream if the meson DRM driver probe has to be re-tried (with
-EPROBE_DEFER). That's why I chose to Cc the stable list.


 drivers/gpu/drm/meson/meson_encoder_cvbs.c | 1 -
 drivers/gpu/drm/meson/meson_encoder_dsi.c  | 1 -
 drivers/gpu/drm/meson/meson_encoder_hdmi.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c 
b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
index 3f73b211fa8e..3407450435e2 100644
--- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
+++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
@@ -294,6 +294,5 @@ void meson_encoder_cvbs_remove(struct meson_drm *priv)
if (priv->encoders[MESON_ENC_CVBS]) {
meson_encoder_cvbs = priv->encoders[MESON_ENC_CVBS];
drm_bridge_remove(_encoder_cvbs->bridge);
-   drm_bridge_remove(meson_encoder_cvbs->next_bridge);
}
 }
diff --git a/drivers/gpu/drm/meson/meson_encoder_dsi.c 
b/drivers/gpu/drm/meson/meson_encoder_dsi.c
index 3f93c70488ca..311b91630fbe 100644
--- a/drivers/gpu/drm/meson/meson_encoder_dsi.c
+++ b/drivers/gpu/drm/meson/meson_encoder_dsi.c
@@ -168,6 +168,5 @@ void meson_encoder_dsi_remove(struct meson_drm *priv)
if (priv->encoders[MESON_ENC_DSI]) {
meson_encoder_dsi = priv->encoders[MESON_ENC_DSI];
drm_bridge_remove(_encoder_dsi->bridge);
-   drm_bridge_remove(meson_encoder_dsi->next_bridge);
}
 }
diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c 
b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
index 25ea76558690..c4686568c9ca 100644
--- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
@@ -474,6 +474,5 @@ void meson_encoder_hdmi_remove(struct meson_drm *priv)
if (priv->encoders[MESON_ENC_HDMI]) {
meson_encoder_hdmi = priv->encoders[MESON_ENC_HDMI];
drm_bridge_remove(_encoder_hdmi->bridge);
-   drm_bridge_remove(meson_encoder_hdmi->next_bridge);
}
 }
-- 
2.43.2

Re: [PATCH v5 05/17] clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF

[Martin Blumenstingl]

Hi Neil,

On Tue, May 30, 2023 at 5:57 PM Neil Armstrong
 wrote:
[...]
> >> The mipi_dsi_pxclk_div is set as RO in order to use the same GP0
> >> for mipi_dsi_pxclk and vclk2_input.
> >
> > I don't think notifiers is the appropriate approach here.
> > Whenever there is clock change the motifiers would trigger an off/on of
> > the clock, regardless of the clock usage or state.
> > If you have several consummers on this vclk2, this would
> > cause glitches and maybe this is not desirable.
> >
> > I think it would be better to handle the enable and reset with a
> > specific gate driver, in prepare() or enable(), and the give the clock
> > CLK_SET_RATE_GATE flag.
> >
> > This would require the clock to be properly turn off before changing the
> > rate.
>
> Sure, will see how to switch to that, seem Martin did than on Meson8.
You can start here: [0]
It may not be the nicest logic but so far it works (for me).

Please note that I don't mix between CCF and direct register IO clock handling:
For the old SoCs I'm relying only on CCF to manage the clocks.


Best regards,
Martin


[0] 
https://github.com/xdarklight/linux/blob/meson-mx-integration-6.3-20230410/drivers/gpu/drm/meson/meson_vclk.c#L1177-L1179

Re: [PATCH 30/53] drm/meson: Convert to platform remove callback returning void

[Martin Blumenstingl]

On Sun, May 7, 2023 at 6:26 PM 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 (mostly) ignored
> 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.
>
> Trivially convert meson drm drivers from always returning zero in the
> remove callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König 
Reviewed-by: Martin Blumenstingl 

Re: [PATCH] drm/meson: set variables meson_hdmi_* storage-class-specifier to static

[Martin Blumenstingl]

On Sun, Apr 23, 2023 at 4:53 PM Tom Rix  wrote:
>
> smatch has several simailar warnings to
s/simailar/similar/

> drivers/gpu/drm/meson/meson_venc.c:189:28: warning: symbol
>   'meson_hdmi_enci_mode_480i' was not declared. Should it be static?
>
> These variables are only used in their defining file so should be static
>
> Signed-off-by: Tom Rix 
With above typo fixed (or with a comment from the maintainers that
they can fix it while applying):
Acked-by: Martin Blumenstingl 

Re: [PATCH 3/8] drm/aperture: Remove primary argument

[Martin Blumenstingl]

On Tue, Apr 4, 2023 at 10:18 PM Daniel Vetter  wrote:
>
> Only really pci devices have a business setting this - it's for
> figuring out whether the legacy vga stuff should be nuked too. And
> with the preceeding two patches those are all using the pci version of
I think it's spelled "preceding"

[...]
>  drivers/gpu/drm/meson/meson_drv.c   |  2 +-
for the meson driver:
Acked-by: Martin Blumenstingl 


Thank you and best regards,
Martin

Re: [PATCH] drm/meson: fix missing component unbind on bind errors

[Martin Blumenstingl]

Hi Johan,

thanks for your patch!

On Mon, Mar 6, 2023 at 11:35 AM Johan Hovold  wrote:
[...]
> @@ -325,23 +325,23 @@ static int meson_drv_bind_master(struct device *dev, 
> bool has_components)
>
> ret = meson_encoder_hdmi_init(priv);
I'm wondering if component_bind_all() can be moved further down.
Right now it's between meson_encoder_cvbs_init() and
meson_encoder_hdmi_init(). So it seems that encoders don't rely on
component registration.

Unfortunately I am also not familiar with this and I'm hoping that
Neil can comment on this.


Best regards,
Martin

Re: [PATCH] drm/meson: dw-hdmi: Fix devm_regulator_*get_enable*() conversion again

[Martin Blumenstingl]

On Thu, Mar 9, 2023 at 4:24 PM Marek Szyprowski
 wrote:
>
> devm_regulator_get_enable_optional() returns -ENODEV if requested
> optional regulator is not present. Adjust code for that, because in the
> 67d0a30128c9 I've incorrectly assumed that it also returns 0 when
> regulator is not present.
>
> Reported-by: Ricardo Cañuelo 
> Fixes: 67d0a30128c9 ("drm/meson: dw-hdmi: Fix devm_regulator_*get_enable*() 
> conversion")
> Signed-off-by: Marek Szyprowski 
Acked-by: Martin Blumenstingl 

Re: [PATCH 11/22] drm/meson: Use GEM DMA fbdev emulation

[Martin Blumenstingl]

On Wed, Mar 1, 2023 at 4:31 PM Thomas Zimmermann  wrote:
>
> Use the fbdev emulation that is optimized for DMA helpers. Avoids
> possible shadow buffering and makes the code simpler.
>
> Signed-off-by: Thomas Zimmermann 
Acked-by: Martin Blumenstingl 

Re: [PATCH v2] drm/meson: fix 1px pink line on GXM when scaling video overlay

[Martin Blumenstingl]

On Fri, Mar 3, 2023 at 1:33 PM Christian Hewitt
 wrote:
>
> Playing media with a resolution smaller than the crtc size requires the
> video overlay to be scaled for output and GXM boards display a 1px pink
> line on the bottom of the scaled overlay. Comparing with the downstream
> vendor driver revealed VPP_DUMMY_DATA not being set [0].
>
> Setting VPP_DUMMY_DATA prevents the 1px pink line from being seen.
>
> [0] 
> https://github.com/endlessm/linux-s905x/blob/master/drivers/amlogic/amports/video.c#L7869
>
> Fixes: bbbe775ec5b5 ("drm: Add support for Amlogic Meson Graphic Controller")
> Suggested-by: Martin Blumenstingl 
> Signed-off-by: Christian Hewitt 
Acked-by: Martin Blumenstingl 

> ---
> Change since v1:
> This time I sent the right patch from the correct branch; the wording in
> v1 is incorrect and the change to meson_registers.h is not required.
Thanks Christian - I was about to ask whether you could isolate which
part of the original patch fixes the issue, but you've been quicker
:-)

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

On Tue, Feb 14, 2023 at 10:35 PM Ville Syrjälä
 wrote:
[...]
> > > We should perhaps just get rid of HDMI_VENDOR_INFOFRAME_SIZE
> > > entirely.
> > My thought was to make it the correct size for
> > drm_hdmi_vendor_infoframe_from_display_mode(). Then developers using
> > this "common" vendor infoframe don't have to worry much.
> > If there's another vendor infoframe implementation (which I'm not
> > aware of, but it may exist - since as you point out: it's vendor
> > specific) then the driver code shouldn't use
> > drm_hdmi_vendor_infoframe_from_display_mode() but rather implement
> > something custom. At that point the person implementing that will also
> > need to know their specific infoframe maximum size.
>
> Yes but that other infoframe will still have
> type==HDMI_INFOFRAME_TYPE_VENDOR, and
> HDMI_INFOFRAME_SIZE(VENDOR) would again
> give the wrong answer.
So this means the way forward is to remove HDMI_VENDOR_INFOFRAME_SIZE?
That means it's up to the (HDMI) driver developers to use a big enough
buffer (by hard-coding the size). Last time I checked all drivers did.


Best regards,
Martin

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

On Mon, Feb 13, 2023 at 12:11 PM Ville Syrjälä
 wrote:
[...]
> > One could use HDMI_VENDOR_INFOFRAME_SIZE with this as well:
> >   u8 buffer[HDMI_INFOFRAME_SIZE(VENDOR)];
> > But it would only result in an 8 byte wide buffer.
> > Nobody uses it like this yet.
>
> Not sure that would make any sense since a vendor
> specific infoframe has no defined size until you
> figure out which vendor defined it (via the OUI).
My understanding is that all of the existing HDMI vendor infoframe
code is built for HDMI_IEEE_OUI.

> I suppose the current value of 4 is also a bit nonsense
> as well then, becasue that is a legal value for the
> HDMI 1.4 vendor specific infoframe, but might not be
> valid for any other infoframe.
>
> We should perhaps just get rid of HDMI_VENDOR_INFOFRAME_SIZE
> entirely.
My thought was to make it the correct size for
drm_hdmi_vendor_infoframe_from_display_mode(). Then developers using
this "common" vendor infoframe don't have to worry much.
If there's another vendor infoframe implementation (which I'm not
aware of, but it may exist - since as you point out: it's vendor
specific) then the driver code shouldn't use
drm_hdmi_vendor_infoframe_from_display_mode() but rather implement
something custom. At that point the person implementing that will also
need to know their specific infoframe maximum size.


Best regards,
Martin

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

Hello Ville.

On Mon, Feb 6, 2023 at 10:58 AM Ville Syrjälä
 wrote:
[...]
> > Change HDMI_VENDOR_INFOFRAME_SIZE to 6 bytes so
> > hdmi_vendor_infoframe_pack_only() can properly check the passed buffer
> > size and avoid an out of bounds write to ptr[8] or ptr[9].
>
> The function should return -ENOSPC if the caller didn't
> provide a big enough buffer.
Indeed, I'm not sure why I didn't notice when I sent the patch.

> Are you saying there are drivers that are passing a bogus size here?
Thankfully not - at least when I checked the last time drivers passed
a 10 byte - or bigger - buffer.
My main concern is the HDMI_INFOFRAME_SIZE macro. It's used in various
drivers like this:
  u8 buffer[HDMI_INFOFRAME_SIZE(AVI)];

One could use HDMI_VENDOR_INFOFRAME_SIZE with this as well:
  u8 buffer[HDMI_INFOFRAME_SIZE(VENDOR)];
But it would only result in an 8 byte wide buffer.
Nobody uses it like this yet.

Do you see any reason why my patch could cause problems?
If not then I want to re-send it with an updated description.


Best regards,
Martin

Re: [PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

Hello Jani, Hello Ville,

On Tue, Jan 10, 2023 at 7:20 PM Jani Nikula  wrote:
[...]
> > I'm not an expert on this topic and I'm not sure if the size still
> > depends on that if condition from long time ago. So please share your
> > thoughts.
>
> I tried to look at this quickly, but it makes my brain hurt. I don't
> think simply changing the size here is right either.
I think I see what you're saying here: hdmi_vendor_infoframe_length()
has logic to determine the "correct" size.

My idea here is to use the maximum possible size for
HDMI_VENDOR_INFOFRAME_SIZE so it can be used with the
HDMI_INFOFRAME_SIZE macro (just like the other _SIZE definitions right
above the vendor infoframe one).
If you have suggestions on my patch then please let me know.


Best regards,
Martin

[PATCH v1 RFC] video/hdmi: Fix HDMI_VENDOR_INFOFRAME_SIZE

[Martin Blumenstingl]

When support for the HDMI vendor infoframe was introduced back with
commit 7d27becb3532 ("video/hdmi: Introduce helpers for the HDMI vendor
specific infoframe") it's payload size was either 5 or 6 bytes,
depending on:
  if (frame->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
When true the size was 6 bytes, otherwise 5 bytes.

Drivers that are using hdmi_infoframe_pack() are reserving 10 bytes (4
bytes for the header and up to 6 bytes for the infoframe payload data)
or more (exynos_hdmi reserves 25 bytes).

Over time the frame payload length was reduced to 4 bytes. This however
does not match the code from hdmi_hdmi_infoframe_pack() where ptr[8] and
ptr[9] are written, which means the infoframe has to allow up to 6 bytes
of payload data (considering that the header takes 4 bytes).

Change HDMI_VENDOR_INFOFRAME_SIZE to 6 bytes so
hdmi_vendor_infoframe_pack_only() can properly check the passed buffer
size and avoid an out of bounds write to ptr[8] or ptr[9].

Fixes: c5e69ab35c0d ("video/hdmi: Constify infoframe passed to the pack 
functions")
Fixes: d43be2554b58 ("drivers: video: hdmi: cleanup coding style in video a 
bit")
Signed-off-by: Martin Blumenstingl 
---
I'm not an expert on this topic and I'm not sure if the size still
depends on that if condition from long time ago. So please share your
thoughts.


 include/linux/hdmi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 2f4dcc8d060e..026c5ef5a1a5 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -57,7 +57,7 @@ enum hdmi_infoframe_type {
 #define HDMI_SPD_INFOFRAME_SIZE25
 #define HDMI_AUDIO_INFOFRAME_SIZE  10
 #define HDMI_DRM_INFOFRAME_SIZE26
-#define HDMI_VENDOR_INFOFRAME_SIZE  4
+#define HDMI_VENDOR_INFOFRAME_SIZE  6
 
 #define HDMI_INFOFRAME_SIZE(type)  \
(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
-- 
2.39.0

Re: [PATCH] drm/meson: Reduce the FIFO lines held when AFBC is not used

[Martin Blumenstingl]

Hi Carlo,

On Mon, Dec 19, 2022 at 9:43 AM Carlo Caione  wrote:
>
> Having a bigger number of FIFO lines held after vsync is only useful to
> SoCs using AFBC to give time to the AFBC decoder to be reset, configured
> and enabled again.
>
> For SoCs not using AFBC this, on the contrary, is causing on some
> displays issues and a few pixels vertical offset in the displayed image.
On the 32-bit SoCs (for which VPU support is not upstream yet) it has
caused screen tearing instead of shifting the image.

> Conditionally increase the number of lines held after vsync only for
> SoCs using AFBC, leaving the default value for all the others.
That was also my approach (for a not-yet-upstream patch).
Since it's affecting already supported SoCs I suggest adding
"Fixed-by: 24e0d4058eff ..." (maybe Neil can do so when he agrees and
is applying the patch).

Acked-by: Martin Blumenstingl 

Re: [PATCH RESEND v4 2/2] gpu: drm: meson: Use devm_regulator_*get_enable*()

[Martin Blumenstingl]

On Wed, Nov 16, 2022 at 2:03 PM Matti Vaittinen
 wrote:
>
> Simplify using the devm_regulator_get_enable_optional(). Also drop the
> seemingly unused struct member 'hdmi_supply'.
Personally I'd replace "seemingly" with "now" because hdmi_supply was
used before (although only in this one function, which makes it a bit
pointless).
This is minor enough. So with or without that change this gets my:
Acked-by: Martin Blumenstingl 

Re: [PATCH] drm/meson: Fix refcount bugs in meson_vpu_has_available_connectors()

[Martin Blumenstingl]

Hello,

On Fri, Jul 15, 2022 at 3:22 PM Liang He  wrote:
>
> In this function, there are two refcount leak bugs:
> (1) when breaking out of for_each_endpoint_of_node(), we need call
> the of_node_put() for the 'ep';
> (2) we should call of_node_put() for the reference returned by
> of_graph_get_remote_port() when it is not used anymore.
>
> Fixes: bbbe775ec5b5 ("drm: Add support for Amlogic Meson Graphic Controller")
> Signed-off-by: Liang He 
Acked-by: Martin Blumenstingl 

It's easy for me to miss patches if the linux-amlogic list is not part
of the recipient list.
Can you please re-send this patch and include the linux-amlogic
mailing list (with my acked-by added)? Then it should also show up in
Neil's inbox so he can apply this patch.


Thank you
Martin

Re: [PATCH v3 5/6] drm/meson: add DSI encoder

[Martin Blumenstingl]

On Fri, Jun 17, 2022 at 9:27 AM Neil Armstrong  wrote:
>
> This adds an encoder bridge designed to drive a MIPI-DSI display
> by using the ENCL encoder through the internal MIPI DSI transceiver
> connected to the output of the ENCL pixel encoder.
>
> Signed-off-by: Neil Armstrong 
> Reviewed-by: Jagan Teki 
Acked-by: Martin Blumenstingl 

Re: [PATCH v3 6/6] drm/meson: add support for MIPI-DSI transceiver

[Martin Blumenstingl]

Hi Neil,

On Fri, Jun 17, 2022 at 9:27 AM Neil Armstrong  wrote:
> +/* [31:16] RW intr_stat/clr. Default 0.
> + * For each bit, read as this interrupt level status,
> + * write 1 to clear.
Do you know if an interrupt line from GIC is routed to the MIPI-DSI
transceiver? If so, we should make it mandatory in patch #1 of this
series (dt-bindings patch), even though it's not in use by the driver
at the moment.

Re: [PATCH v2 2/2] drm/meson: encoder_hdmi: Fix refcount leak in meson_encoder_hdmi_init

[Martin Blumenstingl]

Hello,

thank you for working on this!

On Wed, Jun 1, 2022 at 5:40 AM Miaoqian Lin  wrote:
>
> of_graph_get_remote_node() returns remote device nodepointer with
> refcount incremented, we should use of_node_put() on it when done.
> Add missing of_node_put() to avoid refcount leak.
>
> Fixes: e67f6037ae1b ("drm/meson: split out encoder from meson_dw_hdmi")
> Signed-off-by: Miaoqian Lin 
Reviewed-by: Martin Blumenstingl 

Note to self: at first I thought the following code needs to be changed as well:
  notifier = cec_notifier_conn_register(>dev, NULL, _info);
  if (!notifier)
return -ENOMEM;
But a few lines before this we already have:
of_node_put(remote);
Meaning: this patch is fine as is.


Best regards,
Martin

Re: [PATCH v2 1/2] drm/meson: encoder_cvbs: Fix refcount leak in meson_encoder_cvbs_init

[Martin Blumenstingl]

Hello,

thank you for your patch!

On Wed, Jun 1, 2022 at 5:39 AM Miaoqian Lin  wrote:
>
> of_graph_get_remote_node() returns remote device nodepointer with
> refcount incremented, we should use of_node_put() on it when done.
> Add missing of_node_put() to avoid refcount leak.
>
> Fixes: 318ba02cd8a8 ("drm/meson: encoder_cvbs: switch to bridge with 
> ATTACH_NO_CONNECTOR")
> Signed-off-by: Miaoqian Lin 
As far as I can tell this patch is identical to the one from v1.
Please keep my Reviewed-by from the previous version in case nothing
has changed for this specific patch:
Reviewed-by: Martin Blumenstingl 


Best regards,
Martin

Re: [PATCH 2/2] drm/meson: encoder_hdmi: Fix refcount leak in meson_encoder_hdmi_init

[Martin Blumenstingl]

Hello,

first of all: thank you for spotting this and sending a patch!

On Tue, May 31, 2022 at 4:49 PM Miaoqian Lin  wrote:
[...]
> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c 
> b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 5e306de6f485..f3341458f8b7 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> @@ -363,6 +363,7 @@ int meson_encoder_hdmi_init(struct meson_drm *priv)
> }
>
> meson_encoder_hdmi->next_bridge = of_drm_find_bridge(remote);
> +   of_node_put(remote);
further down in the same function remote is used again:
  pdev = of_find_device_by_node(remote);

My understanding is that we should only use of_node_put() once we
don't need to access the node (in this case the variable is "remote")
anymore.


Best regards,
Martin

Re: [PATCH 1/2] drm/meson: encoder_cvbs: Fix refcount leak in meson_encoder_cvbs_init

[Martin Blumenstingl]

On Tue, May 31, 2022 at 4:49 PM Miaoqian Lin  wrote:
>
> of_graph_get_remote_node() returns remote device nodepointer with
> refcount incremented, we should use of_node_put() on it when done.
> Add missing of_node_put() to avoid refcount leak.
>
> Fixes: 318ba02cd8a8 ("drm/meson: encoder_cvbs: switch to bridge with 
> ATTACH_NO_CONNECTOR")
> Signed-off-by: Miaoqian Lin 
Reviewed-by: Martin Blumenstingl 

Re: [PATCH] drm/meson: Fix refcount leak in meson_encoder_hdmi_init

[Martin Blumenstingl]

On Wed, May 11, 2022 at 7:41 AM Miaoqian Lin  wrote:
>
> of_find_device_by_node() takes reference, we should use put_device()
> to release it when not need anymore.
> Add missing put_device() in error path to avoid refcount
> leak.
>
> Fixes: 0af5e0b41110 ("drm/meson: encoder_hdmi: switch to bridge 
> DRM_BRIDGE_ATTACH_NO_CONNECTOR")
> Signed-off-by: Miaoqian Lin 
Reviewed-by: Martin Blumenstingl 

Thanks for sending this patch!

Neil, while reviewing this I noticed that on module unload we're also
not calling put_device().
This note doesn't affect this patch - but I am wondering if we need to
put that put_device() during module unload on our TODO-list?


Best regards,
Martin

Re: [PATCH] drm: Drop commas after SoC match table sentinels

[Martin Blumenstingl]

On Thu, Mar 3, 2022 at 1:45 PM Geert Uytterhoeven
 wrote:
>
> It does not make sense to have a comma after a sentinel, as any new
> elements must be added before the sentinel.
agreed, thanks for taking care of this!

> Signed-off-by: Geert Uytterhoeven 
> ---
>  drivers/gpu/drm/bridge/nwl-dsi.c  | 2 +-
>  drivers/gpu/drm/meson/meson_drv.c | 2 +-
for drivers/gpu/drm/meson/meson_drv.c:
Acked-by: Martin Blumenstingl 


Best regards,
Martin

Re: [PATCH 6/6] drm/meson: add support for MIPI-DSI transceiver

[Martin Blumenstingl]

Hi Neil,

some high-level comments from me below.

On Fri, Jan 7, 2022 at 3:58 PM Neil Armstrong  wrote:
[...]
> +/*  MIPI DSI Relative REGISTERs Definitions */
> +/* For MIPI_DSI_TOP_CNTL */
> +#define BIT_DPI_COLOR_MODE20
> +#define BIT_IN_COLOR_MODE 16
> +#define BIT_CHROMA_SUBSAMPLE  14
> +#define BIT_COMP2_SEL 12
> +#define BIT_COMP1_SEL 10
> +#define BIT_COMP0_SEL  8
> +#define BIT_DE_POL 6
> +#define BIT_HSYNC_POL  5
> +#define BIT_VSYNC_POL  4
> +#define BIT_DPICOLORM  3
> +#define BIT_DPISHUTDN  2
> +#define BIT_EDPITE_INTR_PULSE  1
> +#define BIT_ERR_INTR_PULSE 0
Why not use BIT() and GENMASK() for these and prefixing them with
MIPI_DSI_TOP_CNTL_?
That would make them consistent with other parts of the meson sub-driver.

[...]
> +static void meson_dw_mipi_dsi_hw_init(struct meson_dw_mipi_dsi *mipi_dsi)
> +{
> +   writel_relaxed((1 << 4) | (1 << 5) | (0 << 6),
> +  mipi_dsi->base + MIPI_DSI_TOP_CNTL);
please use the macros from above

> +   writel_bits_relaxed(0xf, 0xf, mipi_dsi->base + MIPI_DSI_TOP_SW_RESET);
> +   writel_bits_relaxed(0xf, 0, mipi_dsi->base + MIPI_DSI_TOP_SW_RESET);

[...]
> +   phy_power_on(mipi_dsi->phy);
Please propagate the error code here.
Also shouldn't this go to a new dw_mipi_dsi_phy_power_on() as the PHY
driver uses the updated settings from phy_configure only in it's
.power_on callback?

[...]
> +   phy_configure(mipi_dsi->phy, _dsi->phy_opts);
please propagate the error code here as the PHY driver has some
explicit code to return an error in it's .phy_configure callback

[...]
> +   phy_init(mipi_dsi->phy);
please propagate the error code here

[...]
> +   phy_exit(mipi_dsi->phy);
please propagate the error code here

[...]
> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +   mipi_dsi->base = devm_ioremap_resource(>dev, res);
other parts of the meson DRM driver have been converted to use
devm_platform_ioremap_resource()
I suggest updating this as well to simplify the code here

[...]
> +   mipi_dsi->phy = devm_phy_get(>dev, "dphy");
> +   if (IS_ERR(mipi_dsi->phy)) {
> +   ret = PTR_ERR(mipi_dsi->phy);
> +   dev_err(>dev, "failed to get mipi dphy: %d\n", ret);
> +   return ret;
you can simplify this with:
  return dev_err_probe(>dev, PTR_ERR(mipi_dsi->phy, "failed to
get mipi dphy\n");

[...]
> +   mipi_dsi->px_clk = devm_clk_get(>dev, "px_clk");
> +   if (IS_ERR(mipi_dsi->px_clk)) {
> +   dev_err(>dev, "Unable to get PLL clk\n");
> +   return PTR_ERR(mipi_dsi->px_clk);
you can simplify this with:
  return dev_err_probe(>dev, PTR_ERR(mipi_dsi->px_clk, "Unable
to get PLL clk\n");
Also should it say s/PLL clk/px clock/?

[...]
> +   top_rst = devm_reset_control_get_exclusive(>dev, "top");
> +   if (IS_ERR(top_rst)) {
> +   ret = PTR_ERR(top_rst);
> +
> +   if (ret != -EPROBE_DEFER)
> +   dev_err(>dev, "Unable to get reset control: 
> %d\n", ret);
> +
> +   return ret;
you can simplify this with:
  return dev_err_probe(>dev, PTR_ERR(top_rst, "Unable to get
reset control\n");

[...]
> +   mipi_dsi->dmd = dw_mipi_dsi_probe(pdev, _dsi->pdata);
> +   if (IS_ERR(mipi_dsi->dmd)) {
> +   ret = PTR_ERR(mipi_dsi->dmd);
> +   if (ret != -EPROBE_DEFER)
> +   dev_err(>dev,
> +   "Failed to probe dw_mipi_dsi: %d\n", ret);
you can simplify this with:
  dev_err_probe(>dev, ret, "Failed to probe dw_mipi_dsi\n");


Best regards,
Martin

Re: [PATCH 5/6] drm/meson: add DSI encoder

[Martin Blumenstingl]

Hi Neil,

On Fri, Jan 7, 2022 at 3:57 PM Neil Armstrong  wrote:

[...]
> +   writel_bits_relaxed(BIT(3), BIT(3), priv->io_base + 
> _REG(ENCL_VIDEO_MODE_ADV));
see my comment on patch #3 from this series for BIT(3)


Best regards,
Martin

Re: [PATCH 3/6] drm/meson: venc: add ENCL encoder setup for MIPI-DSI output

[Martin Blumenstingl]

 Hi Neil,

On Fri, Jan 7, 2022 at 3:57 PM Neil Armstrong  wrote:
>
> This adds supports for the ENCL encoder connected to a MIPI-DSI transceiver 
> on the
> Amlogic AXG SoCs.
Should this be "AXG and newer SoCs" or is this really AXG specific?

[...]
> +#define GAMMA_VCOM_POL7 /* RW */
> +#define GAMMA_RVS_OUT 6 /* RW */
> +#define ADR_RDY   5 /* Read Only */
> +#define WR_RDY4 /* Read Only */
> +#define RD_RDY3 /* Read Only */
> +#define GAMMA_TR  2 /* RW */
> +#define GAMMA_SET 1 /* RW */
> +#define GAMMA_EN  0 /* RW */
> +
> +#define H_RD  12
> +#define H_AUTO_INC11
> +#define H_SEL_R   10
> +#define H_SEL_G   9
> +#define H_SEL_B   8
I think all values above can be wrapped in the BIT() macro, then you
don't need that below.

> +#define HADR_MSB  7/* 7:0 */
> +#define HADR  0/* 7:0 */
Here GENMASK(7, 0) can be used for HADR

Also I think prefixing all macros above with their register name
(L_GAMMA_CNTL_PORT_ or L_GAMMA_ADDR_PORT_) will make the code easier
to read.

[...]
> +   writel_relaxed(0x8000, priv->io_base + _REG(ENCL_VIDEO_MODE));
The public S905 datasheet calls 0x8000 ENCL_PX_LN_CNT_SHADOW_EN

> +   writel_relaxed(0x0418, priv->io_base + _REG(ENCL_VIDEO_MODE_ADV));
According to the public S905 datasheet this is:
- BIT(3): ENCL_VIDEO_MODE_ADV_VFIFO_EN
- BIT(4): ENCL_VIDEO_MODE_ADV_GAIN_HDTV
- BIT(10): ENCL_SEL_GAMMA_RGB_IN

> +   writel_relaxed(0x1000, priv->io_base + _REG(ENCL_VIDEO_FILT_CTRL));
I don't know the exact name but the 32-bit vendor kernel sources have
a comment [0] saying that 0x1000 is "bypass filter"
But maybe we can simply call it ENCL_VIDEO_FILT_CTRL_BYPASS_FILTER

[...]
> +   writel_relaxed(3, priv->io_base + _REG(ENCL_VIDEO_RGBIN_CTRL));
The public S905 datasheet says:
- BIT(0): USE RGB data from VIU, furthermore a comment in the 3.10
kernel sources make this more clear: bit[0] 1:RGB, 0:YUV
- BIT(1): CFG_VIDEO_RGBIN_ZBLK

> +   /* default black pattern */
> +   writel_relaxed(0, priv->io_base + _REG(ENCL_TST_MDSEL));
> +   writel_relaxed(0, priv->io_base + _REG(ENCL_TST_Y));
> +   writel_relaxed(0, priv->io_base + _REG(ENCL_TST_CB));
> +   writel_relaxed(0, priv->io_base + _REG(ENCL_TST_CR));
> +   writel_relaxed(1, priv->io_base + _REG(ENCL_TST_EN));
> +   writel_bits_relaxed(BIT(3), 0, priv->io_base + 
> _REG(ENCL_VIDEO_MODE_ADV));
same as above: ENCL_VIDEO_MODE_ADV_VFIFO_EN

> +
> +   writel_relaxed(1, priv->io_base + _REG(ENCL_VIDEO_EN));
> +
> +   writel_relaxed(0, priv->io_base + _REG(L_RGB_BASE_ADDR));
> +   writel_relaxed(0x400, priv->io_base + _REG(L_RGB_COEFF_ADDR));
note to self: L_RGB_COEFF_ADDR seems to contain some "magic" value,
there's no further info in the 3.10 kernel sources or datasheet

> +   writel_relaxed(0x400, priv->io_base + _REG(L_DITH_CNTL_ADDR));
According to the public S905 datasheet BIT(10) is DITH10_EN (10-bits
Dithering to 8 Bits Enable).
I am not sure if this would belong to the selected video mode/bit depth.
I'll let other reviewers decide if this is relevant or not because I don't know.

[...]
> +   writel_relaxed(0, priv->io_base + _REG(L_INV_CNT_ADDR));
> +   writel_relaxed(BIT(4) | BIT(5),
> +  priv->io_base + _REG(L_TCON_MISC_SEL_ADDR));
the public S905 datasheet states:
- BIT(4): STV1_SEL (STV1 is frame Signal)
- BIT(5): STV2_SEL (STV2 is frame Signal)
This doesn't seem helpful to me though, but maybe you can still create
preprocessor macros for this (for consistency)?

[...]
> +   switch (priv->venc.current_mode) {
> +   case MESON_VENC_MODE_MIPI_DSI:
> +   writel_relaxed(0x200,
> +  priv->io_base + _REG(VENC_INTCTRL));
the public S905 datasheet documents this as:
- BIT(9): ENCP_LNRST_INT_EN (Progressive encoder filed change interrupt enable)
Please add a preprocessor macro to make it consistent with
VENC_INTCTRL_ENCI_LNRST_INT_EN which already exists and is used below.


Best regards,
Martin

Re: [PATCH 2/6] dt-bindings: display: meson-vpu: add third DPI output port

[Martin Blumenstingl]

On Fri, Jan 7, 2022 at 3:56 PM Neil Armstrong  wrote:
>
> Add third port corresponding to the ENCL DPI encoder used to connect
> to DSI or LVDS transceivers.
>
> Signed-off-by: Neil Armstrong 
Reviewed-by: Martin Blumenstingl 

[PATCH 2/2] drm/meson: Fix error handling when afbcd.ops->init fails

[Martin Blumenstingl]

When afbcd.ops->init fails we need to free the struct drm_device. Also
all errors which come after afbcd.ops->init was successful need to exit
the AFBCD, just like meson_drv_unbind() does.

Fixes: d1b5e41e13a7e9 ("drm/meson: Add AFBCD module driver")
Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/meson/meson_drv.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_drv.c 
b/drivers/gpu/drm/meson/meson_drv.c
index b919271a6e50..26aeaf0ab86e 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -302,42 +302,42 @@ static int meson_drv_bind_master(struct device *dev, bool 
has_components)
if (priv->afbcd.ops) {
ret = priv->afbcd.ops->init(priv);
if (ret)
-   return ret;
+   goto free_drm;
}
 
/* Encoder Initialization */
 
ret = meson_encoder_cvbs_init(priv);
if (ret)
-   goto free_drm;
+   goto exit_afbcd;
 
if (has_components) {
ret = component_bind_all(drm->dev, drm);
if (ret) {
dev_err(drm->dev, "Couldn't bind all components\n");
-   goto free_drm;
+   goto exit_afbcd;
}
}
 
ret = meson_encoder_hdmi_init(priv);
if (ret)
-   goto free_drm;
+   goto exit_afbcd;
 
ret = meson_plane_create(priv);
if (ret)
-   goto free_drm;
+   goto exit_afbcd;
 
ret = meson_overlay_create(priv);
if (ret)
-   goto free_drm;
+   goto exit_afbcd;
 
ret = meson_crtc_create(priv);
if (ret)
-   goto free_drm;
+   goto exit_afbcd;
 
ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name, 
drm);
if (ret)
-   goto free_drm;
+   goto exit_afbcd;
 
drm_mode_config_reset(drm);
 
@@ -355,6 +355,9 @@ static int meson_drv_bind_master(struct device *dev, bool 
has_components)
 
 uninstall_irq:
free_irq(priv->vsync_irq, drm);
+exit_afbcd:
+   if (priv->afbcd.ops)
+   priv->afbcd.ops->exit(priv);
 free_drm:
drm_dev_put(drm);
 
-- 
2.34.1

[PATCH 1/2] drm/meson: osd_afbcd: Add an exit callback to struct meson_afbcd_ops

[Martin Blumenstingl]

Use this to simplify the driver shutdown. It will also come handy when
fixing the error handling in meson_drv_bind_master().

Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/meson/meson_drv.c   |  6 ++--
 drivers/gpu/drm/meson/meson_osd_afbcd.c | 41 -
 drivers/gpu/drm/meson/meson_osd_afbcd.h |  1 +
 3 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_drv.c 
b/drivers/gpu/drm/meson/meson_drv.c
index 80f1d439841a..b919271a6e50 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -385,10 +385,8 @@ static void meson_drv_unbind(struct device *dev)
free_irq(priv->vsync_irq, drm);
drm_dev_put(drm);
 
-   if (priv->afbcd.ops) {
-   priv->afbcd.ops->reset(priv);
-   meson_rdma_free(priv);
-   }
+   if (priv->afbcd.ops)
+   priv->afbcd.ops->exit(priv);
 }
 
 static const struct component_master_ops meson_drv_master_ops = {
diff --git a/drivers/gpu/drm/meson/meson_osd_afbcd.c 
b/drivers/gpu/drm/meson/meson_osd_afbcd.c
index ffc6b584dbf8..0cdbe899402f 100644
--- a/drivers/gpu/drm/meson/meson_osd_afbcd.c
+++ b/drivers/gpu/drm/meson/meson_osd_afbcd.c
@@ -79,11 +79,6 @@ static bool meson_gxm_afbcd_supported_fmt(u64 modifier, 
uint32_t format)
return meson_gxm_afbcd_pixel_fmt(modifier, format) >= 0;
 }
 
-static int meson_gxm_afbcd_init(struct meson_drm *priv)
-{
-   return 0;
-}
-
 static int meson_gxm_afbcd_reset(struct meson_drm *priv)
 {
writel_relaxed(VIU_SW_RESET_OSD1_AFBCD,
@@ -93,6 +88,16 @@ static int meson_gxm_afbcd_reset(struct meson_drm *priv)
return 0;
 }
 
+static int meson_gxm_afbcd_init(struct meson_drm *priv)
+{
+   return 0;
+}
+
+static void meson_gxm_afbcd_exit(struct meson_drm *priv)
+{
+   meson_gxm_afbcd_reset(priv);
+}
+
 static int meson_gxm_afbcd_enable(struct meson_drm *priv)
 {
writel_relaxed(FIELD_PREP(OSD1_AFBCD_ID_FIFO_THRD, 0x40) |
@@ -172,6 +177,7 @@ static int meson_gxm_afbcd_setup(struct meson_drm *priv)
 
 struct meson_afbcd_ops meson_afbcd_gxm_ops = {
.init = meson_gxm_afbcd_init,
+   .exit = meson_gxm_afbcd_exit,
.reset = meson_gxm_afbcd_reset,
.enable = meson_gxm_afbcd_enable,
.disable = meson_gxm_afbcd_disable,
@@ -269,6 +275,18 @@ static bool meson_g12a_afbcd_supported_fmt(u64 modifier, 
uint32_t format)
return meson_g12a_afbcd_pixel_fmt(modifier, format) >= 0;
 }
 
+static int meson_g12a_afbcd_reset(struct meson_drm *priv)
+{
+   meson_rdma_reset(priv);
+
+   meson_rdma_writel_sync(priv, VIU_SW_RESET_G12A_AFBC_ARB |
+  VIU_SW_RESET_G12A_OSD1_AFBCD,
+  VIU_SW_RESET);
+   meson_rdma_writel_sync(priv, 0, VIU_SW_RESET);
+
+   return 0;
+}
+
 static int meson_g12a_afbcd_init(struct meson_drm *priv)
 {
int ret;
@@ -286,16 +304,10 @@ static int meson_g12a_afbcd_init(struct meson_drm *priv)
return 0;
 }
 
-static int meson_g12a_afbcd_reset(struct meson_drm *priv)
+static void meson_g12a_afbcd_exit(struct meson_drm *priv)
 {
-   meson_rdma_reset(priv);
-
-   meson_rdma_writel_sync(priv, VIU_SW_RESET_G12A_AFBC_ARB |
-  VIU_SW_RESET_G12A_OSD1_AFBCD,
-  VIU_SW_RESET);
-   meson_rdma_writel_sync(priv, 0, VIU_SW_RESET);
-
-   return 0;
+   meson_g12a_afbcd_reset(priv);
+   meson_rdma_free(priv);
 }
 
 static int meson_g12a_afbcd_enable(struct meson_drm *priv)
@@ -380,6 +392,7 @@ static int meson_g12a_afbcd_setup(struct meson_drm *priv)
 
 struct meson_afbcd_ops meson_afbcd_g12a_ops = {
.init = meson_g12a_afbcd_init,
+   .exit = meson_g12a_afbcd_exit,
.reset = meson_g12a_afbcd_reset,
.enable = meson_g12a_afbcd_enable,
.disable = meson_g12a_afbcd_disable,
diff --git a/drivers/gpu/drm/meson/meson_osd_afbcd.h 
b/drivers/gpu/drm/meson/meson_osd_afbcd.h
index 5e5523304f42..e77ddeb6416f 100644
--- a/drivers/gpu/drm/meson/meson_osd_afbcd.h
+++ b/drivers/gpu/drm/meson/meson_osd_afbcd.h
@@ -14,6 +14,7 @@
 
 struct meson_afbcd_ops {
int (*init)(struct meson_drm *priv);
+   void (*exit)(struct meson_drm *priv);
int (*reset)(struct meson_drm *priv);
int (*enable)(struct meson_drm *priv);
int (*disable)(struct meson_drm *priv);
-- 
2.34.1

[PATCH 0/2] drm/meson: Error handling fix when AFBCD is used

[Martin Blumenstingl]

Hi Neil,

this series consists of a small cleanup patch and a fix for the error
handling in meson_drv_bind_master() when AFBCD is used.
The patches are based on drm-misc to not conflict with your previous
driver rework. Since the problem has not been observed in the wild I
decided not to Cc linux-stable.


Best regards,
Martin


Martin Blumenstingl (2):
  drm/meson: osd_afbcd: Add an exit callback to struct meson_afbcd_ops
  drm/meson: Fix error handling when afbcd.ops->init fails

 drivers/gpu/drm/meson/meson_drv.c   | 25 +++
 drivers/gpu/drm/meson/meson_osd_afbcd.c | 41 -
 drivers/gpu/drm/meson/meson_osd_afbcd.h |  1 +
 3 files changed, 41 insertions(+), 26 deletions(-)

-- 
2.34.1

Re: [PATCH v2 6/6] drm/meson: encoder_cvbs: switch to bridge with ATTACH_NO_CONNECTOR

[Martin Blumenstingl]

Hi Neil, Hi Sam,

On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  wrote:
[...]
> +static const struct drm_bridge_funcs meson_encoder_cvbs_bridge_funcs = {
> +   .attach = meson_encoder_cvbs_attach,
> +   .enable = meson_encoder_cvbs_enable,
> +   .disable = meson_encoder_cvbs_disable,
> +   .mode_valid = meson_encoder_cvbs_mode_valid,
> +   .get_modes = meson_encoder_cvbs_get_modes,
> +   .atomic_enable = meson_encoder_cvbs_atomic_enable,
I did some testing on boards where u-boot doesn't initialize the video outputs.
It seems that meson_encoder_cvbs_enable() is never called with this patch.
meson_encoder_cvbs_atomic_enable() is called though.

>From what I can see in drm_bridge.c [0] this is even expected.
Does this mean that we need to move all logic from .enable to
.atomic_enable (and same with .disable -> moving that logic to
.atomic_disable)?

The same comment applies to the HDMI patch.


Best regards,
Martin


[0] 
https://elixir.bootlin.com/linux/v5.15-rc5/source/drivers/gpu/drm/drm_bridge.c#L717

Re: [PATCH v2 6/6] drm/meson: encoder_cvbs: switch to bridge with ATTACH_NO_CONNECTOR

[Martin Blumenstingl]

On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  wrote:
>
> Drop the local connector and move all callback to bridge funcs in order
> to leverage the generic CVBS display connector.
>
> This will also permit adding custom cvbs connectors for ADC based HPD
> detection on some Amlogic SoC based boards.
>
> Signed-off-by: Neil Armstrong 
> Acked-by: Sam Ravnborg 
Acked-by: Martin Blumenstingl 

Re: [PATCH v2 5/6] drm/meson: rename venc_cvbs to encoder_cvbs

[Martin Blumenstingl]

On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  wrote:
>
> Rename the cvbs encoder to match the newly introduced meson_encoder_hdmi.
>
> Signed-off-by: Neil Armstrong 
> Acked-by: Sam Ravnborg 
Acked-by: Martin Blumenstingl 

Re: [PATCH v2 4/6] drm/meson: encoder_hdmi: switch to bridge DRM_BRIDGE_ATTACH_NO_CONNECTOR

[Martin Blumenstingl]

Hi Neil,

On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  wrote:
>
> This implements the necessary change to no more use the embedded
> connector in dw-hdmi and use the dedicated bridge connector driver
> by passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to the bridge attach call.
>
> The necessary connector properties are added to handle the same
> functionalities as the embedded dw-hdmi connector, i.e. the HDR
> metadata, the CEC notifier & other flags.
>
> The dw-hdmi output_port is set to 1 in order to look for a connector
> next bridge in order to get DRM_BRIDGE_ATTACH_NO_CONNECTOR working.
>
> Signed-off-by: Neil Armstrong 
> Acked-by: Sam Ravnborg 
another great piece which helps a lot with HDMI support for the 32-bit SoCs!
I have one question below - but regardless of the answer there this gets my:
Acked-by: Martin Blumenstingl 

[...]
> +   pdev = of_find_device_by_node(remote);
I am wondering if we should use something like:
encoder_hdmi->cec_notifier_pdev

> +   if (pdev) {
> +   struct cec_connector_info conn_info;
> +   struct cec_notifier *notifier;
> +
> +   cec_fill_conn_info_from_drm(_info, 
> meson_encoder_hdmi->connector);
> +
> +   notifier = cec_notifier_conn_register(>dev, NULL, 
> _info);
> +   if (!notifier)
> +   return -ENOMEM;
> +
> +   meson_encoder_hdmi->cec_notifier = notifier;
> +   }
and then move this logic to meson_encoder_hdmi_attach()
This would be important if .detach() and .attach() can be called
multiple times (for example during suspend and resume). But I am not
sure if that's a supported use-case.


Best regards,
Martin

Re: [PATCH v2 3/6] drm/meson: split out encoder from meson_dw_hdmi

[Martin Blumenstingl]

Hi Neil,

On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  wrote:
>
> This moves all the non-DW-HDMI code where it should be:
> an encoder in the drm/meson core driver.
>
> The bridge functions are copied as-is, except:
> - the encoder init uses the simple kms helper
> - the mode_set has been moved to atomic_enable()
> - debug prints are converted to dev_debg()
>
> For now the bridge attach flags is 0, DRM_BRIDGE_ATTACH_NO_CONNECTOR
> will be handled later.
>
> The meson dw-hdmi glue is slighly fixed to live without the
typo: slightly

> encoder in the same driver.
>
> Signed-off-by: Neil Armstrong 
apart from that typo (please fix it up when applying the patch) this
is some great work!
it helps me a lot with HDMI support on the 32-bit SoCs
with that said:
Acked-by: Martin Blumenstingl 

Re: [PATCH v2 2/6] drm/meson: remove useless recursive components matching

[Martin Blumenstingl]

On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  wrote:
>
> The initial design was recursive to cover all port/endpoints, but only the 
> first layer
> of endpoints should be covered by the components list.
> This also breaks the MIPI-DSI init/bridge attach sequence, thus only parse the
> first endpoints instead of recursing.
>
> Signed-off-by: Neil Armstrong 
> Acked-by: Sam Ravnborg 
Acked-by: Martin Blumenstingl 

Re: [PATCH] drm/meson: Convert to Linux IRQ interfaces

[Martin Blumenstingl]

Hi Thomas,

On Tue, Jul 6, 2021 at 9:45 AM Thomas Zimmermann  wrote:
>
> Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
> IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
> don't benefit from using it.
>
> Signed-off-by: Thomas Zimmermann 
Tested-by: Martin Blumenstingl 
and also (although I am no drm subsystem expert):
Reviewed-by: Martin Blumenstingl 

[...]
> -   ret = drm_irq_install(drm, priv->vsync_irq);
> +   ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name, 
> drm);
I'd like to use dev_name(dev) instead of drm->driver->name in the
future as that'll make it much easier to identify the corresponding
IRQ in /proc/interrupts for example
your patch makes this possible - thanks for this!


Best regards,
Martin

Re: [PATCH v2] drm/meson: fix potential NULL pointer exception in meson_drv_unbind()

[Martin Blumenstingl]

Hello,

first of all: thanks for your patch and sorry for being late with my
review question.

On Fri, Jun 18, 2021 at 7:28 AM Jiajun Cao  wrote:
>
> Fix a potential NULL pointer exception when meson_drv_unbind()
> attempts to operate on the driver_data priv which may be NULL.
> Add a null pointer check on the priv struct to avoid the NULL
> pointer dereference after calling dev_get_drvdata(), just like
> the null pointer checks done on the struct priv in the function
> meson_drv_shutdown(), meson_drv_pm_suspend() and meson_drv_pm_resume().
I am trying to review Amlogic Meson related patches in the DRM
subsystem so I can help Neil with this.
However, I am still new to this so please help me educate on this topic.

[...]
>  static void meson_drv_unbind(struct device *dev)
>  {
> struct meson_drm *priv = dev_get_drvdata(dev);
> -   struct drm_device *drm = priv->drm;
> +   struct drm_device *drm;
> +
> +   if (!priv)
> +   return;
My understanding of the component framework is that meson_drv_unbind()
is only called if previously meson_drv_bind() was called (and did not
return any error).
This is different from meson_drv_shutdown() (for example) because that
can be called if meson_drv_probe() returns 0 (success) in case the
"count" variable was 0 (then the probe function does nothing).

As I mentioned before: I am still learning about the DRM subsystem in
the Linux kernel.
So it would be great if you could help me understand for which
scenarios this newly added if-condition is needed.


Thank you!
Best regards,
Martin

Re: [PATCH 06/11] drm/: drm_gem_plane_helper_prepare_fb is now the default

[Martin Blumenstingl]

On Fri, May 21, 2021 at 11:10 AM Daniel Vetter  wrote:
>
> No need to set it explicitly.
>
> Signed-off-by: Daniel Vetter 
> Cc: Laurentiu Palcu 
> Cc: Lucas Stach 
> Cc: Shawn Guo 
> Cc: Sascha Hauer 
> Cc: Pengutronix Kernel Team 
> Cc: Fabio Estevam 
> Cc: NXP Linux Team 
> Cc: Philipp Zabel 
> Cc: Paul Cercueil 
> Cc: Chun-Kuang Hu 
> Cc: Matthias Brugger 
> Cc: Neil Armstrong 
> Cc: Kevin Hilman 
> Cc: Jerome Brunet 
> Cc: Martin Blumenstingl 
> Cc: Marek Vasut 
> Cc: Stefan Agner 
> Cc: Sandy Huang 
> Cc: "Heiko Stübner" 
> Cc: Yannick Fertre 
> Cc: Philippe Cornu 
> Cc: Benjamin Gaignard 
> Cc: Maxime Coquelin 
> Cc: Alexandre Torgue 
> Cc: Maxime Ripard 
> Cc: Chen-Yu Tsai 
> Cc: Jernej Skrabec 
> Cc: Jyri Sarha 
> Cc: Tomi Valkeinen 
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-m...@vger.kernel.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-amlo...@lists.infradead.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-st...@st-md-mailman.stormreply.com
> Cc: linux-su...@lists.linux.dev
> ---
>  drivers/gpu/drm/imx/dcss/dcss-plane.c   | 1 -
>  drivers/gpu/drm/imx/ipuv3-plane.c   | 1 -
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c   | 1 -
>  drivers/gpu/drm/ingenic/ingenic-ipu.c   | 1 -
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c| 1 -
>  drivers/gpu/drm/meson/meson_overlay.c   | 1 -
>  drivers/gpu/drm/meson/meson_plane.c | 1 -
For drivers/gpu/drm/meson/*:
Acked-by: Martin Blumenstingl 

Re: [PATCH] drm/meson: fix shutdown crash when component not probed

[Martin Blumenstingl]

Hi Neil,

since this has not received any Reviewed-by yet I tried my best to
review it myself

On Fri, Apr 30, 2021 at 10:28 AM Neil Armstrong  wrote:
[...]
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -485,11 +485,12 @@ static int meson_probe_remote(struct platform_device 
> *pdev,
>  static void meson_drv_shutdown(struct platform_device *pdev)
>  {
> struct meson_drm *priv = dev_get_drvdata(>dev);
this part made it hard for me because I was wondering where the
matching dev_set_drvdata call is
it turns out platform_set_drvdata is used instead, meaning for me it
would have been easier to understand if platform_get_drvdata was used
here
that's however nothing which has changed with this patch

> -   struct drm_device *drm = priv->drm;
>
> -   DRM_DEBUG_DRIVER("\n");
> -   drm_kms_helper_poll_fini(drm);
> -   drm_atomic_helper_shutdown(drm);
> +   if (!priv)
> +   return;
> +
> +   drm_kms_helper_poll_fini(priv->drm);
> +   drm_atomic_helper_shutdown(priv->drm);
>  }
then this part finally made sense to me (as non-drm person), as
platform_set_drvdata comes near the end of meson_drv_bind_master (so
any errors would cause the drvdata to be NULL).

with this I can also give me:
Reviewed-by: Martin Blumenstingl 
in addition to my:
Tested-by: Martin Blumenstingl 

Can you please queue this up for -fixes or do we need to ask someone to do it?


Best regards,
Martin

Re: [PATCH] drm/meson: fix shutdown crash when component not probed

[Martin Blumenstingl]

On Fri, Apr 30, 2021 at 10:28 AM Neil Armstrong  wrote:
>
> When main component is not probed, by example when the dw-hdmi module is
> not loaded yet or in probe defer, the following crash appears on shutdown:
>
> Unable to handle kernel NULL pointer dereference at virtual address 
> 0038
> ...
> pc : meson_drv_shutdown+0x24/0x50
> lr : platform_drv_shutdown+0x20/0x30
> ...
> Call trace:
> meson_drv_shutdown+0x24/0x50
> platform_drv_shutdown+0x20/0x30
> device_shutdown+0x158/0x360
> kernel_restart_prepare+0x38/0x48
> kernel_restart+0x18/0x68
> __do_sys_reboot+0x224/0x250
> __arm64_sys_reboot+0x24/0x30
> ...
>
> Simply check if the priv struct has been allocated before using it.
>
> Fixes: fa0c16caf3d7 ("drm: meson_drv add shutdown function")
> Reported-by: Stefan Agner 
> Signed-off-by: Neil Armstrong 
Tested-by: Martin Blumenstingl 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: discussion: re-structuring of the Amlogic Meson VPU DRM driver

[Martin Blumenstingl]

Hi Neil,

On Mon, Jan 4, 2021 at 2:29 PM Neil Armstrong  wrote:
>
> Hi,
>
> Sorry for the delay...
>
> On 31/12/2020 00:24, Martin Blumenstingl wrote:
> > Hi Neil and all interested people,
> >
> > in the past there were concerns about how some of the components are
> > coupled in our Meson DRM driver(s).
> > With this discussion I would like to achieve four things:
> > 1. understand the current issues that we have> 2. come up with a TODO list 
> > of things that need to be tackled as well
> > as recommendations how to solve it (for example: "driver ABC function
> > ABC uses the recommended way - take that as reference")
> > 3. one by one work on the items on the TODO list
> > 4. add support for the 32-bit SoCs to the Meson VPU DRM driver
> > (without adding more "not recommended" code)
> >
> > Disclaimer: I am not familiar with the DRM subsystem - so apologies if
> > the terminology is not correct.
> >
> > drivers/gpu/drm/meson/meson_dw_hdmi.c currently serves four purposes:
> > 1. manage the TOP (glue) registers for the dw-hdmi IP
> > This is Amlogic specific and consists of things like HPD filtering,
> > some internal resets, etc.
> > In my opinion this part is supposed to stay in this driver
> Yep, it's tightly coupled to the DW-HDMI core
>
> >
> > 2. load the driver for the dw-hdmi IP by calling dw_hdmi_probe()
> > I read somewhere that this is not recommended anymore and should be 
> > replaced.
> > Is my understanding correct and what is the recommended replacement?
> Yeah in fact the dw-hdmi glue should be a pure bridge, not a component 
> anymore.
>
> This means it should probe by itself entirely, should not use the component 
> stuff.
OK, I see

> This also means all the VPU related part (mainly encoder and clock) should be 
> moved
> out of this file as a bridge and built with the meson_drm driver,
> then find the "next bridge" like other drivers.
is that linkage automatically set up with the endpoint definitions
inside the .dts?

> > 3. it manages the HDMI PHY registers in the HHI register area
> > For the 32-bit SoCs I will not follow this pattern and will create a
> > separate PHY instead.
> > As a long-term goal I think we should also move this into a dedicated
> > PHY driver.
>
> I looked at it, and ... it's complex. For the 32-bit socs it's easy because
> you only have a single PHY setup, for the new gens you have to deal with the
> 4k modes and co. This could be handle by adding a new parameters set to the
> phy_configure union, but what should we add in it to be super generic ?
you are right, on the 32-bit SoCs it's pretty easy actually.
it's only "4k" and "everything else".

I think using the phy_configure approach is the right way
but to be honest: I have not thought about which parameters to add to
that union (for the 64-bit SoCs) to make it "generic" enough
also I think this is a TODO "for later", so no action needed now - but
it's great to see that you had the same idea in mind :)

> >
> > 4. call back into VPU/VENC functions to set up these registers
> > This is a blocker for 32-bit SoC support as I would have to duplicate
> > this code if we don't make any changes. This includes things like
> > calculating (and setting) clock frequencies, calling
> > meson_venc_hdmi_mode_set for setting up the DVI pixel encoder, etc.
> > My understanding is that this part should not be part of the
> > meson_dw_hdmi driver, but "some other" driver. I don't understand
> > which driver that's supposed to be though and how things would be
> > wired up in the end.
>
> Yep it should be a bridge. You can chain bridges, it's designed for such use 
> case.
>
> We will have internal bridges for encoders, ENCL+ENCP grouped for HDMI and 
> ENCL.
I see. adding to my question above: would this mean that we have then
more "endpoints" defined in our .dts - one for the ENCI+ENCP (HDMI)
output, another one for the ENCL (DSI), ...?

> CVBS can be handled separately without bridges.
indeed, let's postpone CVBS for now as it's easy to adapt the current
code for the 32-bit SoCs.
in a perfect world I think the CVBS encoder/bridge (whatever the
correct type is) would be it's own driver as it's part of the HHI
registers

> I can have a try to move stuff if you can review/test on your side.
> Would it be a good start ?
that would be awesome
if there's any way I can help (you add FIXMEs/TODOs to your code which
you want me to solve, testing, etc.) then please let me know!

> >
> > In addition to HDMI my understanding is that for adding MIPI DSI
> > support you would
> > a) e

[PATCH RESEND v1] drm/meson: viu: fix setting the OSD burst length in VIU_OSD1_FIFO_CTRL_STAT

[Martin Blumenstingl]

The burst length is configured in VIU_OSD1_FIFO_CTRL_STAT[31] and
VIU_OSD1_FIFO_CTRL_STAT[11:10]. The public S905D3 datasheet describes
this as:
- 0x0 = up to 24 per burst
- 0x1 = up to 32 per burst
- 0x2 = up to 48 per burst
- 0x3 = up to 64 per burst
- 0x4 = up to 96 per burst
- 0x5 = up to 128 per burst

The lower two bits map to VIU_OSD1_FIFO_CTRL_STAT[11:10] while the upper
bit maps to VIU_OSD1_FIFO_CTRL_STAT[31].

Replace meson_viu_osd_burst_length_reg() with pre-defined macros which
set these values. meson_viu_osd_burst_length_reg() always returned 0
(for the two used values: 32 and 64 at least) and thus incorrectly set
the burst size to 24.

Fixes: 147ae1cbaa1842 ("drm: meson: viu: use proper macros instead of magic 
constants")
Signed-off-by: Martin Blumenstingl 
---
re-send of v1 [0] with no changes as I still noticed that this is
sitting in my tree and I wasn't asked to change anything in v1.


[0] https://patchwork.kernel.org/patch/11510045/


 drivers/gpu/drm/meson/meson_registers.h |  6 ++
 drivers/gpu/drm/meson/meson_viu.c   | 11 ++-
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_registers.h 
b/drivers/gpu/drm/meson/meson_registers.h
index 8ea00546cd4e..049c4bfe2a3a 100644
--- a/drivers/gpu/drm/meson/meson_registers.h
+++ b/drivers/gpu/drm/meson/meson_registers.h
@@ -261,6 +261,12 @@
 #define VIU_OSD_FIFO_DEPTH_VAL(val)  ((val & 0x7f) << 12)
 #define VIU_OSD_WORDS_PER_BURST(words)   (((words & 0x4) >> 1) << 22)
 #define VIU_OSD_FIFO_LIMITS(size)((size & 0xf) << 24)
+#define VIU_OSD_BURST_LENGTH_24  (0x0 << 31 | 0x0 << 10)
+#define VIU_OSD_BURST_LENGTH_32  (0x0 << 31 | 0x1 << 10)
+#define VIU_OSD_BURST_LENGTH_48  (0x0 << 31 | 0x2 << 10)
+#define VIU_OSD_BURST_LENGTH_64  (0x0 << 31 | 0x3 << 10)
+#define VIU_OSD_BURST_LENGTH_96  (0x1 << 31 | 0x0 << 10)
+#define VIU_OSD_BURST_LENGTH_128 (0x1 << 31 | 0x1 << 10)
 
 #define VD1_IF0_GEN_REG 0x1a50
 #define VD1_IF0_CANVAS0 0x1a51
diff --git a/drivers/gpu/drm/meson/meson_viu.c 
b/drivers/gpu/drm/meson/meson_viu.c
index 304f8ff1339c..aede0c67a57f 100644
--- a/drivers/gpu/drm/meson/meson_viu.c
+++ b/drivers/gpu/drm/meson/meson_viu.c
@@ -411,13 +411,6 @@ void meson_viu_gxm_disable_osd1_afbc(struct meson_drm 
*priv)
priv->io_base + _REG(VIU_MISC_CTRL1));
 }
 
-static inline uint32_t meson_viu_osd_burst_length_reg(uint32_t length)
-{
-   uint32_t val = (((length & 0x80) % 24) / 12);
-
-   return (((val & 0x3) << 10) | (((val & 0x4) >> 2) << 31));
-}
-
 void meson_viu_init(struct meson_drm *priv)
 {
uint32_t reg;
@@ -444,9 +437,9 @@ void meson_viu_init(struct meson_drm *priv)
VIU_OSD_FIFO_LIMITS(2);  /* fifo_lim: 2*16=32 */
 
if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
-   reg |= meson_viu_osd_burst_length_reg(32);
+   reg |= VIU_OSD_BURST_LENGTH_32;
else
-   reg |= meson_viu_osd_burst_length_reg(64);
+   reg |= VIU_OSD_BURST_LENGTH_64;
 
writel_relaxed(reg, priv->io_base + _REG(VIU_OSD1_FIFO_CTRL_STAT));
writel_relaxed(reg, priv->io_base + _REG(VIU_OSD2_FIFO_CTRL_STAT));
-- 
2.27.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/meson: viu: fix setting the OSD burst length in VIU_OSD1_FIFO_CTRL_STAT

[Martin Blumenstingl]

Hi Neil,

On Tue, Apr 28, 2020 at 10:38 AM Neil Armstrong  wrote:
[...]
> > @@ -444,9 +437,9 @@ void meson_viu_init(struct meson_drm *priv)
> >   VIU_OSD_FIFO_LIMITS(2);  /* fifo_lim: 2*16=32 */
> >
> >   if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> > - reg |= meson_viu_osd_burst_length_reg(32);
> > + reg |= VIU_OSD_BURST_LENGTH_32;
> >   else
> > - reg |= meson_viu_osd_burst_length_reg(64);
> > + reg |= VIU_OSD_BURST_LENGTH_64;
> >
> >   writel_relaxed(reg, priv->io_base + _REG(VIU_OSD1_FIFO_CTRL_STAT));
> >   writel_relaxed(reg, priv->io_base + _REG(VIU_OSD2_FIFO_CTRL_STAT));
> >
>
> Thanks,
> Will run some tests !
Does this fix/improve anything for you?
On the 32-bit SoCs kmscube is not smooth (neither on the CVBS nor on
the HDMI output) with a burst length of 24 (which was the old
"accidentally used" value).


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/meson: add mode selection limits against specific SoC revisions

[Martin Blumenstingl]

Hi Neil,

On Tue, Apr 28, 2020 at 11:21 AM Neil Armstrong  wrote:
>
> The Amlogic S805X/Y uses the same die as the S905X, but with more
> limited graphics capabilities.
>
> This adds a soc version detection adding specific limitations on the HDMI
> mode selections.
>
> Here, we limit to HDMI 1.3a max HDMI PHY clock frequency.
for my own education: 1.65GHz from the PLL will be divided down to 165MHz
isn't this more like the limit of HDMI 1.2a?

> Changes sinces v1:
> - Moved frequency check in the vclk code, and also checks DMT modes
>
> Signed-off-by: Neil Armstrong 
Acked-by: Martin Blumenstingl 

This looks good to me based on the current limitations of meson_vclk.c
If we switch to CCF based VPU clock rate changes then we should do
this in the clock driver by calling clk_hw_set_rate_range(hdmi_pll, 0,
1.65GHz)

The good thing is: we can re-use struct meson_drm_soc_limits even
after switching to CCF.
We will just need to set the max PHY freq using
clk_round_rate(hdmi_pll, ULONG_MAX)


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/meson: viu: fix setting the OSD burst length in VIU_OSD1_FIFO_CTRL_STAT

[Martin Blumenstingl]

The burst length is configured in VIU_OSD1_FIFO_CTRL_STAT[31] and
VIU_OSD1_FIFO_CTRL_STAT[11:10]. The public S905D3 datasheet describes
this as:
- 0x0 = up to 24 per burst
- 0x1 = up to 32 per burst
- 0x2 = up to 48 per burst
- 0x3 = up to 64 per burst
- 0x4 = up to 96 per burst
- 0x5 = up to 128 per burst

The lower two bits map to VIU_OSD1_FIFO_CTRL_STAT[11:10] while the upper
bit maps to VIU_OSD1_FIFO_CTRL_STAT[31].

Replace meson_viu_osd_burst_length_reg() with pre-defined macros which
set these values. meson_viu_osd_burst_length_reg() always returned 0
(for the two used values: 32 and 64 at least) and thus incorrectly set
the burst size to 24.

Fixes: 147ae1cbaa1842 ("drm: meson: viu: use proper macros instead of magic 
constants")
Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/meson/meson_registers.h |  6 ++
 drivers/gpu/drm/meson/meson_viu.c   | 11 ++-
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_registers.h 
b/drivers/gpu/drm/meson/meson_registers.h
index 8ea00546cd4e..049c4bfe2a3a 100644
--- a/drivers/gpu/drm/meson/meson_registers.h
+++ b/drivers/gpu/drm/meson/meson_registers.h
@@ -261,6 +261,12 @@
 #define VIU_OSD_FIFO_DEPTH_VAL(val)  ((val & 0x7f) << 12)
 #define VIU_OSD_WORDS_PER_BURST(words)   (((words & 0x4) >> 1) << 22)
 #define VIU_OSD_FIFO_LIMITS(size)((size & 0xf) << 24)
+#define VIU_OSD_BURST_LENGTH_24  (0x0 << 31 | 0x0 << 10)
+#define VIU_OSD_BURST_LENGTH_32  (0x0 << 31 | 0x1 << 10)
+#define VIU_OSD_BURST_LENGTH_48  (0x0 << 31 | 0x2 << 10)
+#define VIU_OSD_BURST_LENGTH_64  (0x0 << 31 | 0x3 << 10)
+#define VIU_OSD_BURST_LENGTH_96  (0x1 << 31 | 0x0 << 10)
+#define VIU_OSD_BURST_LENGTH_128 (0x1 << 31 | 0x1 << 10)
 
 #define VD1_IF0_GEN_REG 0x1a50
 #define VD1_IF0_CANVAS0 0x1a51
diff --git a/drivers/gpu/drm/meson/meson_viu.c 
b/drivers/gpu/drm/meson/meson_viu.c
index 304f8ff1339c..aede0c67a57f 100644
--- a/drivers/gpu/drm/meson/meson_viu.c
+++ b/drivers/gpu/drm/meson/meson_viu.c
@@ -411,13 +411,6 @@ void meson_viu_gxm_disable_osd1_afbc(struct meson_drm 
*priv)
priv->io_base + _REG(VIU_MISC_CTRL1));
 }
 
-static inline uint32_t meson_viu_osd_burst_length_reg(uint32_t length)
-{
-   uint32_t val = (((length & 0x80) % 24) / 12);
-
-   return (((val & 0x3) << 10) | (((val & 0x4) >> 2) << 31));
-}
-
 void meson_viu_init(struct meson_drm *priv)
 {
uint32_t reg;
@@ -444,9 +437,9 @@ void meson_viu_init(struct meson_drm *priv)
VIU_OSD_FIFO_LIMITS(2);  /* fifo_lim: 2*16=32 */
 
if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
-   reg |= meson_viu_osd_burst_length_reg(32);
+   reg |= VIU_OSD_BURST_LENGTH_32;
else
-   reg |= meson_viu_osd_burst_length_reg(64);
+   reg |= VIU_OSD_BURST_LENGTH_64;
 
writel_relaxed(reg, priv->io_base + _REG(VIU_OSD1_FIFO_CTRL_STAT));
writel_relaxed(reg, priv->io_base + _REG(VIU_OSD2_FIFO_CTRL_STAT));
-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/meson: add mode selection limits against specific SoC revisions

[Martin Blumenstingl]

Hi Neil,

On Tue, Apr 21, 2020 at 3:44 PM Neil Armstrong  wrote:
[...]
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index e8c94915a4fc..dc3d5122475a 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -695,6 +695,13 @@ dw_hdmi_mode_valid(struct drm_connector *connector,
> dev_dbg(connector->dev->dev, "%s: vclk:%d phy=%d venc=%d hdmi=%d\n",
> __func__, phy_freq, vclk_freq, venc_freq, hdmi_freq);
>
> +   /* Check against soc revision/package limits */
> +   if (priv->limits) {
> +   if (priv->limits->max_hdmi_phy_freq &&
> +   phy_freq > priv->limits->max_hdmi_phy_freq)
> +   return MODE_CLOCK_HIGH;
> +   }
I think that this will also be useful for the 32-bit SoCs as well.
is there a chance you can move it to meson_vclk_vic_supported_freq
(called right below), where all the existing frequency limit checks
are already?


Thank you!
Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5] dt-bindings: gpu: mali-utgard: Add the #cooling-cells property

[Martin Blumenstingl]

The GPU can be one of the big heat sources on a SoC. Allow the
"#cooling-cells" property to be specified for ARM Mali Utgard GPUs so
the GPU clock speeds (and voltages) can be reduced to prevent a SoC from
overheating.

Reviewed-by: Qiang Yu 
Signed-off-by: Martin Blumenstingl 
---
Changes since v4 at [0]:
- Added Qiang's Reviewed-by (many thanks)
- re-send because I missed the devicetree mailing list in v4


[0] https://patchwork.kernel.org/patch/11448013/


 Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml 
b/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml
index f5401cc8de4a..4869258daadb 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml
@@ -107,6 +107,9 @@ properties:
 
   operating-points-v2: true
 
+  "#cooling-cells":
+const: 2
+
 required:
   - compatible
   - reg
@@ -164,6 +167,7 @@ examples:
   clocks = < 1>, < 2>;
   clock-names = "bus", "core";
   resets = < 1>;
+  #cooling-cells = <2>;
 };
 
 ...
-- 
2.26.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: lima, panfrost: multiple definition of `of_devfreq_cooling_register_power'

[Martin Blumenstingl]

On Thu, Apr 2, 2020 at 9:46 AM Thomas Zimmermann  wrote:
>
> Hi Martin
>
> Am 02.04.20 um 09:39 schrieb Martin Blumenstingl:
> > Hi Thomas,
> >
> > On Thu, Apr 2, 2020 at 9:26 AM Thomas Zimmermann  
> > wrote:
> >>
> >> Hi,
> >>
> >> building lima and panfrost drivers from drm-tip, I currently get the
> >> following linker error
> >>
> >>   > make clean
> >>   > make
> >>   [...]
> >>   LD  vmlinux.o
> >>   arm-suse-linux-gnueabi-ld: drivers/gpu/drm/panfrost
> >> /panfrost_devfreq.o: in function
> >> `of_devfreq_cooling_register_power':
> >>   panfrost_devfreq.c:(.text+0x18c): multiple definition of
> >> `of_devfreq_cooling_register_power'; drivers/gpu/drm/lima
> >> /lima_devfreq.o:lima_devfreq.c:(.text+0x1a0): first defined here
> >>   make[1]: *** [/home/tzimmermann/Projekte/linux/Makefile:1078: vmlinux]
> >> Error 1
> >>   make[1]: Leaving directory '/home/tzimmermann/Projekte/linux/build-
> >> arm'
> >>   make: *** [Makefile:180: sub-make] Error 2
> > can you please try building again with the attached patch?
>
> Yes, fixes the bug. Thanks for responding quickly.
I just sent a fix to the correct mailing lists: [0]
it would be awesome if you could give it your "Tested-by"


Regards
Martin


[0] https://lore.kernel.org/patchwork/patch/1220292/
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: lima, panfrost: multiple definition of `of_devfreq_cooling_register_power'

[Martin Blumenstingl]

Hi Thomas,

On Thu, Apr 2, 2020 at 9:26 AM Thomas Zimmermann  wrote:
>
> Hi,
>
> building lima and panfrost drivers from drm-tip, I currently get the
> following linker error
>
>   > make clean
>   > make
>   [...]
>   LD  vmlinux.o
>   arm-suse-linux-gnueabi-ld: drivers/gpu/drm/panfrost
> /panfrost_devfreq.o: in function
> `of_devfreq_cooling_register_power':
>   panfrost_devfreq.c:(.text+0x18c): multiple definition of
> `of_devfreq_cooling_register_power'; drivers/gpu/drm/lima
> /lima_devfreq.o:lima_devfreq.c:(.text+0x1a0): first defined here
>   make[1]: *** [/home/tzimmermann/Projekte/linux/Makefile:1078: vmlinux]
> Error 1
>   make[1]: Leaving directory '/home/tzimmermann/Projekte/linux/build-
> arm'
>   make: *** [Makefile:180: sub-make] Error 2
can you please try building again with the attached patch?

> Seems related to
>
>   commit 1996970773a323533e1cc1b6b97f00a95d675f32
>   Author: Martin Blumenstingl 
>   Date:   Thu Mar 19 21:34:27 2020 +0100
>
> drm/lima: Add optional devfreq and cooling device support
>
> https://cgit.freedesktop.org/drm/drm-tip/commit/?id=1996970773a323533e1cc1b6b97f00a95d675f32
it's also possible that this was originally caused by a76caf55e5b356
("thermal: Add devfreq cooling") and that my commit only exposes this
bug


Thank you in advance!
Regards
Martin
diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index a06a9fb35d72..94a487e05d35 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -107,18 +107,22 @@ opp-81600 {
 			opp-microvolt = <875000>;
 		};
 		opp-100800 {
+			status = "disabled";
 			opp-hz = /bits/ 64 <100800>;
 			opp-microvolt = <925000>;
 		};
 		opp-12 {
+			status = "disabled";
 			opp-hz = /bits/ 64 <12>;
 			opp-microvolt = <975000>;
 		};
 		opp-141600 {
+			status = "disabled";
 			opp-hz = /bits/ 64 <141600>;
 			opp-microvolt = <1025000>;
 		};
 		opp-160800 {
+			status = "disabled";
 			opp-hz = /bits/ 64 <160800>;
 			opp-microvolt = <110>;
 		};
@@ -144,19 +148,23 @@ opp-182142857 {
 		opp-31875 {
 			opp-hz = /bits/ 64 <31875>;
 			opp-microvolt = <115>;
+			status = "disabled";
 		};
 		opp-42500 {
 			opp-hz = /bits/ 64 <42500>;
 			opp-microvolt = <115>;
+			status = "disabled";
 		};
 		opp-51000 {
 			opp-hz = /bits/ 64 <51000>;
 			opp-microvolt = <115>;
+			status = "disabled";
 		};
 		opp-63750 {
 			opp-hz = /bits/ 64 <63750>;
 			opp-microvolt = <115>;
 			turbo-mode;
+			status = "disabled";
 		};
 	};
 
diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index 2b49a6bb8718..eaf4d61e5043 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -349,7 +349,6 @@ vpu: vpu@10 {
 			nvmem-cell-names = "cvbs_trimming";
 			#address-cells = <1>;
 			#size-cells = <0>;
-			status = "disabled";
 
 			/* CVBS VDAC output port */
 			cvbs_vdac_port: port@0 {
diff --git a/include/linux/devfreq_cooling.h b/include/linux/devfreq_cooling.h
index 4635f95000a4..79a6e37a1d6f 100644
--- a/include/linux/devfreq_cooling.h
+++ b/include/linux/devfreq_cooling.h
@@ -75,7 +75,7 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *dfc);
 
 #else /* !CONFIG_DEVFREQ_THERMAL */
 
-struct thermal_cooling_device *
+static inline struct thermal_cooling_device *
 of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
   struct devfreq_cooling_power *dfc_power)
 {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] dt-bindings: display: meson-vpu: fix indentation of reg-names' "items"

[Martin Blumenstingl]

Use two spaces for indentation instead of one to be consistent with the
rest of the file. No functional changes.

Fixes: 6b9ebf1e0e678b ("dt-bindings: display: amlogic, meson-vpu: convert to 
yaml")
Signed-off-by: Martin Blumenstingl 
---
 .../devicetree/bindings/display/amlogic,meson-vpu.yaml  | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/amlogic,meson-vpu.yaml 
b/Documentation/devicetree/bindings/display/amlogic,meson-vpu.yaml
index d1205a6697a0..cd8ad2af52c9 100644
--- a/Documentation/devicetree/bindings/display/amlogic,meson-vpu.yaml
+++ b/Documentation/devicetree/bindings/display/amlogic,meson-vpu.yaml
@@ -71,9 +71,9 @@ properties:
 maxItems: 2
 
   reg-names:
-   items:
- - const: vpu
- - const: hhi
+items:
+  - const: vpu
+  - const: hhi
 
   interrupts:
 maxItems: 1
-- 
2.26.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/2] drm/lima: Add optional devfreq and cooling device support

[Martin Blumenstingl]

On Sat, Mar 28, 2020 at 9:40 AM Qiang Yu  wrote:
>
> Applied to drm-misc-next.
thank you!

regarding patch #1 - can you apply this as well?
patch #1 just takes this midgard change [0] and ports it to utgard


Thank you!
Martin


[0] 
https://cgit.freedesktop.org/drm/drm-misc/commit/Documentation/devicetree/bindings/gpu?id=982c0500fd1a8012c31d3c9dd8de285129904656
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 0/2] drm: lima: devfreq and cooling device support

[Martin Blumenstingl]

This is my attempt at adding devfreq (and cooling device) support to
the lima driver.

Test results from a Meson8m2 board:
TEST #1: glmark2-es2-drm --off-screen in an infinite loop while cycling
 through all available frequencies using the userspace governor

 From  :   To
   : 182142857 31875 42500 51000 63750   time(ms)
  182142857: 0  1274  1274  1273  1279   5399468
  31875:  1274 0  1274  1273  1272   5114700
  42500:  1276  1274 0  1272  1271   5122008
  51000:  1909  1273  1273 0   636   5274292
* 63750:   640  1272  1272  1273 0   5186796
Total transition : 24834

TEST #2: glmark2-es2-drm --off-screen in an infinite loop with the
 simple_ondemand governor
 From  :   To
   : 182142857 31875 42500 51000 63750   time(ms)
  182142857: 0 0 0 0   203318328
  31875:53 0 0 021 56044
  42500:2718 0 0 2 34172
  51000:27 614 0 1 41348
* 63750:95503348 0   2085312


Changes since RFC v3 at [2]:
- fix devfreq init error handling and allow lima_devfreq_fini to be
  called multiple times (thanks Qiang)
- switch from atomic counter to a simple int which is only accessed
  under the devfreq spinlock (thanks Qiang)
- union lock areas in same function (thanks Qiang)
- select DEVFREQ_GOV_SIMPLE_ONDEMAND like panfrost does (thanks Qiang)
- move lima_devfreq struct to lima_devfreq.h (thanks Qiang)
- fix duplicate variable in lima_sched_pipe_task_done
- only call dev_pm_opp_of_remove_table if dev_pm_opp_of_add_table
  succeeded previously
- update copyright year to 2020
- rebased on top of drm-misc-next
- dropped RFC status

Changes since RFC v2 at [1]:
- added #cooling-cells to the dt-bindings (new patch #1)
- skip devfreq initialization when the operating-points-v2 property is
  absent
- call dev_pm_opp_set_regulators() so devfreq will actually manage the
  mali-supply regulator
- rebased on top of drm-misc-next-2020-02-21

Changes since RFC v1 at [0]:
- added lock to protect the statistics as these can be written
  concurrently for example when the GP and PP IRQ are firing at the
  same time. Thanks to Qiang Yu for the suggestion!
- updated the copyright notice of lima_devfreq.c to indicate that the
  code is derived from panfrost_devfreq.c. Thanks to  Chen-Yu Tsai  for
  the suggestion!
- I did not unify the code with panfrost yet because I don't know where
  to put the result. any suggestion is welcome though!


[0] https://patchwork.freedesktop.org/series/70967/
[1] https://patchwork.kernel.org/cover/11311293/
[2] https://patchwork.kernel.org/cover/11398365/


Martin Blumenstingl (2):
  dt-bindings: gpu: mali-utgard: Add the #cooling-cells property
  drm/lima: Add optional devfreq and cooling device support

 .../bindings/gpu/arm,mali-utgard.yaml |   4 +
 drivers/gpu/drm/lima/Kconfig  |   2 +
 drivers/gpu/drm/lima/Makefile |   3 +-
 drivers/gpu/drm/lima/lima_devfreq.c   | 234 ++
 drivers/gpu/drm/lima/lima_devfreq.h   |  41 +++
 drivers/gpu/drm/lima/lima_device.c|   4 +
 drivers/gpu/drm/lima/lima_device.h|   3 +
 drivers/gpu/drm/lima/lima_drv.c   |  14 +-
 drivers/gpu/drm/lima/lima_sched.c |   7 +
 drivers/gpu/drm/lima/lima_sched.h |   3 +
 10 files changed, 312 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.c
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.h

-- 
2.25.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 2/2] drm/lima: Add optional devfreq and cooling device support

[Martin Blumenstingl]

Most platforms with a Mali-400 or Mali-450 GPU also have support for
changing the GPU clock frequency. Add devfreq support so the GPU clock
rate is updated based on the actual GPU usage when the
"operating-points-v2" property is present in the board.dts.

The actual devfreq code is taken from panfrost_devfreq.c and modified so
it matches what the lima hardware needs:
- a call to dev_pm_opp_set_clkname() during initialization because there
  are two clocks on Mali-4x0 IPs. "core" is the one that actually clocks
  the GPU so we need to control it using devfreq.
- locking when reading or writing the devfreq statistics because (unlike
  than panfrost) we have multiple PP and GP IRQs which may finish jobs
  concurrently.

Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/lima/Kconfig|   2 +
 drivers/gpu/drm/lima/Makefile   |   3 +-
 drivers/gpu/drm/lima/lima_devfreq.c | 234 
 drivers/gpu/drm/lima/lima_devfreq.h |  41 +
 drivers/gpu/drm/lima/lima_device.c  |   4 +
 drivers/gpu/drm/lima/lima_device.h  |   3 +
 drivers/gpu/drm/lima/lima_drv.c |  14 +-
 drivers/gpu/drm/lima/lima_sched.c   |   7 +
 drivers/gpu/drm/lima/lima_sched.h   |   3 +
 9 files changed, 308 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.c
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.h

diff --git a/drivers/gpu/drm/lima/Kconfig b/drivers/gpu/drm/lima/Kconfig
index d589f09d04d9..fa1d4f5df31e 100644
--- a/drivers/gpu/drm/lima/Kconfig
+++ b/drivers/gpu/drm/lima/Kconfig
@@ -10,5 +10,7 @@ config DRM_LIMA
depends on OF
select DRM_SCHED
select DRM_GEM_SHMEM_HELPER
+   select PM_DEVFREQ
+   select DEVFREQ_GOV_SIMPLE_ONDEMAND
help
 DRM driver for ARM Mali 400/450 GPUs.
diff --git a/drivers/gpu/drm/lima/Makefile b/drivers/gpu/drm/lima/Makefile
index a85444b0a1d4..5e5c29875e9c 100644
--- a/drivers/gpu/drm/lima/Makefile
+++ b/drivers/gpu/drm/lima/Makefile
@@ -14,6 +14,7 @@ lima-y := \
lima_sched.o \
lima_ctx.o \
lima_dlbu.o \
-   lima_bcast.o
+   lima_bcast.o \
+   lima_devfreq.o
 
 obj-$(CONFIG_DRM_LIMA) += lima.o
diff --git a/drivers/gpu/drm/lima/lima_devfreq.c 
b/drivers/gpu/drm/lima/lima_devfreq.c
new file mode 100644
index ..8c4d21d07529
--- /dev/null
+++ b/drivers/gpu/drm/lima/lima_devfreq.c
@@ -0,0 +1,234 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Martin Blumenstingl 
+ *
+ * Based on panfrost_devfreq.c:
+ *   Copyright 2019 Collabora ltd.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "lima_device.h"
+#include "lima_devfreq.h"
+
+static void lima_devfreq_update_utilization(struct lima_devfreq *devfreq)
+{
+   ktime_t now, last;
+
+   now = ktime_get();
+   last = devfreq->time_last_update;
+
+   if (devfreq->busy_count > 0)
+   devfreq->busy_time += ktime_sub(now, last);
+   else
+   devfreq->idle_time += ktime_sub(now, last);
+
+   devfreq->time_last_update = now;
+}
+
+static int lima_devfreq_target(struct device *dev, unsigned long *freq,
+  u32 flags)
+{
+   struct dev_pm_opp *opp;
+   int err;
+
+   opp = devfreq_recommended_opp(dev, freq, flags);
+   if (IS_ERR(opp))
+   return PTR_ERR(opp);
+   dev_pm_opp_put(opp);
+
+   err = dev_pm_opp_set_rate(dev, *freq);
+   if (err)
+   return err;
+
+   return 0;
+}
+
+static void lima_devfreq_reset(struct lima_devfreq *devfreq)
+{
+   devfreq->busy_time = 0;
+   devfreq->idle_time = 0;
+   devfreq->time_last_update = ktime_get();
+}
+
+static int lima_devfreq_get_dev_status(struct device *dev,
+  struct devfreq_dev_status *status)
+{
+   struct lima_device *ldev = dev_get_drvdata(dev);
+   struct lima_devfreq *devfreq = >devfreq;
+   unsigned long irqflags;
+
+   status->current_frequency = clk_get_rate(ldev->clk_gpu);
+
+   spin_lock_irqsave(>lock, irqflags);
+
+   lima_devfreq_update_utilization(devfreq);
+
+   status->total_time = ktime_to_ns(ktime_add(devfreq->busy_time,
+  devfreq->idle_time));
+   status->busy_time = ktime_to_ns(devfreq->busy_time);
+
+   lima_devfreq_reset(devfreq);
+
+   spin_unlock_irqrestore(>lock, irqflags);
+
+   dev_dbg(ldev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
+   status->busy_time, status->total_time,
+   status->busy_time / (status->total_time / 100),
+   status->current_frequency / 1000 / 1000);
+
+   return 0;
+}
+
+static struct devfreq_dev_profile lima_devfreq_profile = {
+   .polling_ms = 50, /* ~3 frames */
+   .target = lima_devfreq_target,
+  

[PATCH v4 1/2] dt-bindings: gpu: mali-utgard: Add the #cooling-cells property

[Martin Blumenstingl]

The GPU can be one of the big heat sources on a SoC. Allow the
"#cooling-cells" property to be specified for ARM Mali Utgard GPUs so
the GPU clock speeds (and voltages) can be reduced to prevent a SoC from
overheating.

Signed-off-by: Martin Blumenstingl 
---
 Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml 
b/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml
index afde81be3c29..33548ca2a759 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml
@@ -107,6 +107,9 @@ properties:
 
   operating-points-v2: true
 
+  "#cooling-cells":
+const: 2
+
 required:
   - compatible
   - reg
@@ -162,6 +165,7 @@ examples:
   clocks = < 1>, < 2>;
   clock-names = "bus", "core";
   resets = < 1>;
+  #cooling-cells = <2>;
 };
 
 ...
-- 
2.25.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC v3 1/2] dt-bindings: gpu: mali-utgard: Add the #cooling-cells property

[Martin Blumenstingl]

The GPU can be one of the big heat sources on a SoC. Allow the
"#cooling-cells" property to be specified for ARM Mali Utgard GPUs so
the GPU clock speeds (and voltages) can be reduced to prevent a SoC from
overheating.

Signed-off-by: Martin Blumenstingl 
---
 Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml 
b/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml
index afde81be3c29..33548ca2a759 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-utgard.yaml
@@ -107,6 +107,9 @@ properties:
 
   operating-points-v2: true
 
+  "#cooling-cells":
+const: 2
+
 required:
   - compatible
   - reg
@@ -162,6 +165,7 @@ examples:
   clocks = < 1>, < 2>;
   clock-names = "bus", "core";
   resets = < 1>;
+  #cooling-cells = <2>;
 };
 
 ...
-- 
2.25.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC v3 2/2] drm/lima: Add optional devfreq and cooling device support

[Martin Blumenstingl]

Most platforms with a Mali-400 or Mali-450 GPU also have support for
changing the GPU clock frequency. Add devfreq support so the GPU clock
rate is updated based on the actual GPU usage when the
"operating-points-v2" property is present in the board.dts.

The actual devfreq code is taken from panfrost_devfreq.c and modified so
it matches what the lima hardware needs:
- a call to dev_pm_opp_set_clkname() during initialization because there
  are two clocks on Mali-4x0 IPs. "core" is the one that actually clocks
  the GPU so we need to control it using devfreq.
- locking when reading or writing the devfreq statistics because (unlike
  than panfrost) we have multiple PP and GP IRQs which may finish jobs
  concurrently.

Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/lima/Kconfig|   1 +
 drivers/gpu/drm/lima/Makefile   |   3 +-
 drivers/gpu/drm/lima/lima_devfreq.c | 215 
 drivers/gpu/drm/lima/lima_devfreq.h |  15 ++
 drivers/gpu/drm/lima/lima_device.c  |   4 +
 drivers/gpu/drm/lima/lima_device.h  |  18 +++
 drivers/gpu/drm/lima/lima_drv.c |  14 +-
 drivers/gpu/drm/lima/lima_sched.c   |   9 ++
 drivers/gpu/drm/lima/lima_sched.h   |   3 +
 9 files changed, 279 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.c
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.h

diff --git a/drivers/gpu/drm/lima/Kconfig b/drivers/gpu/drm/lima/Kconfig
index d589f09d04d9..09404bc96ad8 100644
--- a/drivers/gpu/drm/lima/Kconfig
+++ b/drivers/gpu/drm/lima/Kconfig
@@ -10,5 +10,6 @@ config DRM_LIMA
depends on OF
select DRM_SCHED
select DRM_GEM_SHMEM_HELPER
+   select PM_DEVFREQ
help
 DRM driver for ARM Mali 400/450 GPUs.
diff --git a/drivers/gpu/drm/lima/Makefile b/drivers/gpu/drm/lima/Makefile
index a85444b0a1d4..5e5c29875e9c 100644
--- a/drivers/gpu/drm/lima/Makefile
+++ b/drivers/gpu/drm/lima/Makefile
@@ -14,6 +14,7 @@ lima-y := \
lima_sched.o \
lima_ctx.o \
lima_dlbu.o \
-   lima_bcast.o
+   lima_bcast.o \
+   lima_devfreq.o
 
 obj-$(CONFIG_DRM_LIMA) += lima.o
diff --git a/drivers/gpu/drm/lima/lima_devfreq.c 
b/drivers/gpu/drm/lima/lima_devfreq.c
new file mode 100644
index ..3a6b315136ce
--- /dev/null
+++ b/drivers/gpu/drm/lima/lima_devfreq.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 Martin Blumenstingl 
+ *
+ * Based on panfrost_devfreq.c:
+ *   Copyright 2019 Collabora ltd.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "lima_device.h"
+#include "lima_devfreq.h"
+
+static void lima_devfreq_update_utilization(struct lima_device *ldev)
+{
+   unsigned long irqflags;
+   ktime_t now, last;
+
+   if (!ldev->devfreq.devfreq)
+   return;
+
+   spin_lock_irqsave(>devfreq.lock, irqflags);
+
+   now = ktime_get();
+   last = ldev->devfreq.time_last_update;
+
+   if (atomic_read(>devfreq.busy_count) > 0)
+   ldev->devfreq.busy_time += ktime_sub(now, last);
+   else
+   ldev->devfreq.idle_time += ktime_sub(now, last);
+
+   ldev->devfreq.time_last_update = now;
+
+   spin_unlock_irqrestore(>devfreq.lock, irqflags);
+}
+
+static int lima_devfreq_target(struct device *dev, unsigned long *freq,
+  u32 flags)
+{
+   struct dev_pm_opp *opp;
+   int err;
+
+   opp = devfreq_recommended_opp(dev, freq, flags);
+   if (IS_ERR(opp))
+   return PTR_ERR(opp);
+   dev_pm_opp_put(opp);
+
+   err = dev_pm_opp_set_rate(dev, *freq);
+   if (err)
+   return err;
+
+   return 0;
+}
+
+static void lima_devfreq_reset(struct lima_device *ldev)
+{
+   unsigned long irqflags;
+
+   spin_lock_irqsave(>devfreq.lock, irqflags);
+
+   ldev->devfreq.busy_time = 0;
+   ldev->devfreq.idle_time = 0;
+   ldev->devfreq.time_last_update = ktime_get();
+
+   spin_unlock_irqrestore(>devfreq.lock, irqflags);
+}
+
+static int lima_devfreq_get_dev_status(struct device *dev,
+  struct devfreq_dev_status *status)
+{
+   struct lima_device *ldev = dev_get_drvdata(dev);
+   unsigned long irqflags;
+
+   lima_devfreq_update_utilization(ldev);
+
+   status->current_frequency = clk_get_rate(ldev->clk_gpu);
+
+   spin_lock_irqsave(>devfreq.lock, irqflags);
+
+   status->total_time = ktime_to_ns(ktime_add(ldev->devfreq.busy_time,
+  ldev->devfreq.idle_time));
+   status->busy_time = ktime_to_ns(ldev->devfreq.busy_time);
+
+   spin_unlock_irqrestore(>devfreq.lock, irqflags);
+
+   lima_devfreq_reset(ldev);
+
+   dev_dbg(ldev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
+   sta

Re: [PATCH RFT v2 0/3] devfreq fixes for panfrost

[Martin Blumenstingl]

Hi Steven,

On Sun, Jan 12, 2020 at 1:16 AM Martin Blumenstingl
 wrote:
>
> These are a bunch of devfreq fixes for panfrost that came up in a
> discussion with Robin Murphy during the code-review of the lima
> devfreq patches: [0]
>
> I am only able to test patch #1 properly because the only boards with
> panfrost GPU that I have are using an Amlogic SoC. We don't have
> support for the OPP tables or dynamic clock changes there yet.
> So patches #2 and #3 are compile-tested only.
>
>
> Changes since v1 at [1]
> - added Steven's Reviewed-by to patch #2 (thank you!)
> - only use dev_pm_opp_put_regulators() to clean up in
>   panfrost_devfreq_init() if regulators_opp_table is not NULL to fix
>   a potential crash inside dev_pm_opp_put_regulators() as spotted by
>   Steven Price (thank you!). While here, I also switched to "goto err"
>   pattern to avoid lines with more than 80 characters.
>
> Known discussion topics (I have no way to test either of these, so I am
> looking for help here):
> - Steven Price reported the following message on his firefly (RK3288)
>   board:
>   "debugfs: Directory 'ffa3.gpu-mali' with parent 'vdd_gpu' already
>   present!"
> - Robin Murphy suggested that patch #1 may not work once the OPP table
>   for the GPU comes from SCMI
>
>
> [0] https://patchwork.freedesktop.org/patch/346898/
> [1] https://patchwork.freedesktop.org/series/71744/
>
>
> Martin Blumenstingl (3):
>   drm/panfrost: enable devfreq based the "operating-points-v2" property
>   drm/panfrost: call dev_pm_opp_of_remove_table() in all error-paths
>   drm/panfrost: Use the mali-supply regulator for control again
I don't have time to work on these patches in the near future
can you (or if someone else is interested then please speak up) please
take these over? you are familiar with the panfrost devfreq code and
you have at least one board where the GPU regulator actually has to
change the voltage (which means you can test this properly; on Amlogic
SoCs the GPU voltage is fixed across all frequencies).


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC v3 0/2] drm: lima: devfreq and cooling device support

[Martin Blumenstingl]

This is my attempt at adding devfreq (and cooling device) support to
the lima driver.

Test results from a Meson8m2 board:
TEST #1: glmark2-es2-drm --off-screen in an infinite loop while cycling
 through all available frequencies using the userspace governor

 From  :   To
   : 182142857 31875 42500 51000 63750   time(ms)
  182142857: 0  1274  1274  1273  1279   5399468
  31875:  1274 0  1274  1273  1272   5114700
  42500:  1276  1274 0  1272  1271   5122008
  51000:  1909  1273  1273 0   636   5274292
* 63750:   640  1272  1272  1273 0   5186796
Total transition : 24834

TEST #2: glmark2-es2-drm --off-screen in an infinite loop with the
 simple_ondemand governor
 From  :   To
   : 182142857 31875 42500 51000 63750   time(ms)
  182142857: 0 0 0 0   203318328
  31875:53 0 0 021 56044
  42500:2718 0 0 2 34172
  51000:27 614 0 1 41348
* 63750:95503348 0   2085312


Changes since RFC v2 at [1]:
- added #cooling-cells to the dt-bindings (new patch #1)
- skip devfreq initialization when the operating-points-v2 property is
  absent
- call dev_pm_opp_set_regulators() so devfreq will actually manage the
  mali-supply regulator
- rebased on top of drm-misc-next-2020-02-21

Changes since RFC v1 at [0]:
- added lock to protect the statistics as these can be written 
  concurrently for example when the GP and PP IRQ are firing at the
  same time. Thanks to Qiang Yu for the suggestion!
- updated the copyright notice of lima_devfreq.c to indicate that the
  code is derived from panfrost_devfreq.c. Thanks to  Chen-Yu Tsai  for
  the suggestion!
- I did not unify the code with panfrost yet because I don't know where
  to put the result. any suggestion is welcome though!


[0] https://patchwork.freedesktop.org/series/70967/
[1] https://patchwork.kernel.org/cover/11311293/


Martin Blumenstingl (2):
  dt-bindings: gpu: mali-utgard: Add the #cooling-cells property
  drm/lima: Add optional devfreq and cooling device support

 .../bindings/gpu/arm,mali-utgard.yaml |   4 +
 drivers/gpu/drm/lima/Kconfig  |   1 +
 drivers/gpu/drm/lima/Makefile |   3 +-
 drivers/gpu/drm/lima/lima_devfreq.c   | 215 ++
 drivers/gpu/drm/lima/lima_devfreq.h   |  15 ++
 drivers/gpu/drm/lima/lima_device.c|   4 +
 drivers/gpu/drm/lima/lima_device.h|  18 ++
 drivers/gpu/drm/lima/lima_drv.c   |  14 +-
 drivers/gpu/drm/lima/lima_sched.c |   9 +
 drivers/gpu/drm/lima/lima_sched.h |   3 +
 10 files changed, 283 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.c
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.h

-- 
2.25.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFT v1 3/3] drm/panfrost: Use the mali-supply regulator for control again

[Martin Blumenstingl]

Hi Steven,

On Mon, Jan 13, 2020 at 6:10 PM Steven Price  wrote:
>
> On 09/01/2020 17:27, Martin Blumenstingl wrote:
> > On Thu, Jan 9, 2020 at 12:31 PM Steven Price  wrote:
> >>
> >> On 07/01/2020 23:06, Martin Blumenstingl wrote:
> >>> dev_pm_opp_set_rate() needs a reference to the regulator which should be
> >>> updated when updating the GPU frequency. The name of the regulator has
> >>> to be passed at initialization-time using dev_pm_opp_set_regulators().
> >>> Add the call to dev_pm_opp_set_regulators() so dev_pm_opp_set_rate()
> >>> will update the GPU regulator when updating the frequency (just like
> >>> we did this manually before when we open-coded dev_pm_opp_set_rate()).
> >>
> >> This patch causes a warning from debugfs on my firefly (RK3288) board:
> >>
> >> debugfs: Directory 'ffa3.gpu-mali' with parent 'vdd_gpu' already
> >> present!
> >>
> >> So it looks like the regulator is being added twice - but I haven't
> >> investigated further.
> > I *think* it's because the regulator is already fetched by the
> > panfrost driver itself to enable it
> > (the devfreq code currently does not support enabling the regulator,
> > it can only control the voltage)
> >
> > I'm not sure what to do about this though
>
> Having a little play around with this, I think you can simply remove the
> panfrost_regulator_init() call. This at least works for me - the call to
> dev_pm_opp_set_regulators() seems to set everything up. However I
> suspect you need to do this unconditionally even if there are no
> operating points defined.
I'm not sure if I can safely remove panfrost_regulator_init() because
it calls regulator_enable()
but there's no regulator_enable() equivalent in devfreq or OPP

I'm not sure how this is supposed to work
if someone has an idea: please let me know

> > [...]
> >>>   ret = dev_pm_opp_of_add_table(dev);
> >>> - if (ret)
> >>> + if (ret) {
> >>> + 
> >>> dev_pm_opp_put_regulators(pfdev->devfreq.regulators_opp_table);
> >>
> >> If we don't have a regulator then regulators_opp_table will be NULL and
> >> sadly dev_pm_opp_put_regulators() doesn't handle a NULL argument. The
> >> same applies to the two below calls obviously.
> > good catch, thank you!
> > are you happy with the general approach here or do you think that
> > dev_pm_opp_set_regulators is the wrong way to go (for whatever
> > reason)?
>
> To be honest this is an area I still don't fully understand. There's a
> lot of magic helper functions and very little in the way of helpful
> documentation to work out which are the right ones to call. It seems
> reasonable to me, hopefully someone more in the know will chime in it
> there's something fundamentally wrong!
OK, if you know anybody who could help then please Cc them


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFT v2 2/3] drm/panfrost: call dev_pm_opp_of_remove_table() in all error-paths

[Martin Blumenstingl]

If devfreq_recommended_opp() fails we need to undo
dev_pm_opp_of_add_table() by calling dev_pm_opp_of_remove_table() (just
like we do it in the other error-path below).

Fixes: f3ba91228e8e91 ("drm/panfrost: Add initial panfrost driver")
Reviewed-by: Steven Price 
Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 1471588763ce..170f6c8c9651 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -93,8 +93,10 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
cur_freq = clk_get_rate(pfdev->clock);
 
opp = devfreq_recommended_opp(dev, _freq, 0);
-   if (IS_ERR(opp))
+   if (IS_ERR(opp)) {
+   dev_pm_opp_of_remove_table(dev);
return PTR_ERR(opp);
+   }
 
panfrost_devfreq_profile.initial_freq = cur_freq;
dev_pm_opp_put(opp);
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFT v2 0/3] devfreq fixes for panfrost

[Martin Blumenstingl]

These are a bunch of devfreq fixes for panfrost that came up in a
discussion with Robin Murphy during the code-review of the lima
devfreq patches: [0]

I am only able to test patch #1 properly because the only boards with
panfrost GPU that I have are using an Amlogic SoC. We don't have
support for the OPP tables or dynamic clock changes there yet.
So patches #2 and #3 are compile-tested only.


Changes since v1 at [1]
- added Steven's Reviewed-by to patch #2 (thank you!)
- only use dev_pm_opp_put_regulators() to clean up in
  panfrost_devfreq_init() if regulators_opp_table is not NULL to fix
  a potential crash inside dev_pm_opp_put_regulators() as spotted by
  Steven Price (thank you!). While here, I also switched to "goto err"
  pattern to avoid lines with more than 80 characters.

Known discussion topics (I have no way to test either of these, so I am
looking for help here):
- Steven Price reported the following message on his firefly (RK3288)
  board:
  "debugfs: Directory 'ffa3.gpu-mali' with parent 'vdd_gpu' already
  present!"
- Robin Murphy suggested that patch #1 may not work once the OPP table
  for the GPU comes from SCMI


[0] https://patchwork.freedesktop.org/patch/346898/
[1] https://patchwork.freedesktop.org/series/71744/


Martin Blumenstingl (3):
  drm/panfrost: enable devfreq based the "operating-points-v2" property
  drm/panfrost: call dev_pm_opp_of_remove_table() in all error-paths
  drm/panfrost: Use the mali-supply regulator for control again

 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 44 +
 drivers/gpu/drm/panfrost/panfrost_device.h  |  1 +
 2 files changed, 37 insertions(+), 8 deletions(-)

-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFT v2 1/3] drm/panfrost: enable devfreq based the "operating-points-v2" property

[Martin Blumenstingl]

Decouple the check to see whether we want to enable devfreq for the GPU
from dev_pm_opp_set_regulators(). This is preparation work for adding
back support for regulator control (which means we need to call
dev_pm_opp_set_regulators() before dev_pm_opp_of_add_table(), which
means having a check for "is devfreq enabled" that is not tied to
dev_pm_opp_of_add_table() makes things easier).

Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 413987038fbf..1471588763ce 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -5,6 +5,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "panfrost_device.h"
@@ -79,10 +80,12 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
struct devfreq *devfreq;
struct thermal_cooling_device *cooling;
 
-   ret = dev_pm_opp_of_add_table(dev);
-   if (ret == -ENODEV) /* Optional, continue without devfreq */
+   if (!device_property_present(dev, "operating-points-v2"))
+   /* Optional, continue without devfreq */
return 0;
-   else if (ret)
+
+   ret = dev_pm_opp_of_add_table(dev);
+   if (ret)
return ret;
 
panfrost_devfreq_reset(pfdev);
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFT v2 3/3] drm/panfrost: Use the mali-supply regulator for control again

[Martin Blumenstingl]

dev_pm_opp_set_rate() needs a reference to the regulator which should be
updated when updating the GPU frequency. The name of the regulator has
to be passed at initialization-time using dev_pm_opp_set_regulators().
Add the call to dev_pm_opp_set_regulators() so dev_pm_opp_set_rate()
will update the GPU regulator when updating the frequency (just like
we did this manually before when we open-coded dev_pm_opp_set_rate()).

Fixes: 221bc77914cbcc ("drm/panfrost: Use generic code for devfreq")
Reported-by: Robin Murphy 
Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 33 +
 drivers/gpu/drm/panfrost/panfrost_device.h  |  1 +
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 170f6c8c9651..3b580a0123e1 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -74,6 +74,7 @@ static struct devfreq_dev_profile panfrost_devfreq_profile = {
 int panfrost_devfreq_init(struct panfrost_device *pfdev)
 {
int ret;
+   struct opp_table *opp_table;
struct dev_pm_opp *opp;
unsigned long cur_freq;
struct device *dev = >pdev->dev;
@@ -84,9 +85,22 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
/* Optional, continue without devfreq */
return 0;
 
+   opp_table = dev_pm_opp_set_regulators(dev,
+ (const char *[]){ "mali" },
+ 1);
+   if (IS_ERR(opp_table)) {
+   ret = PTR_ERR(opp_table);
+
+   /* Continue if the optional regulator is missing */
+   if (ret != -ENODEV)
+   return ret;
+   } else {
+   pfdev->devfreq.regulators_opp_table = opp_table;
+   }
+
ret = dev_pm_opp_of_add_table(dev);
if (ret)
-   return ret;
+   goto err_opp_put_regulators;
 
panfrost_devfreq_reset(pfdev);
 
@@ -94,8 +108,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 
opp = devfreq_recommended_opp(dev, _freq, 0);
if (IS_ERR(opp)) {
-   dev_pm_opp_of_remove_table(dev);
-   return PTR_ERR(opp);
+   ret = PTR_ERR(opp);
+   goto err_opp_of_remove_table;
}
 
panfrost_devfreq_profile.initial_freq = cur_freq;
@@ -105,8 +119,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
  DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
if (IS_ERR(devfreq)) {
DRM_DEV_ERROR(dev, "Couldn't initialize GPU devfreq\n");
-   dev_pm_opp_of_remove_table(dev);
-   return PTR_ERR(devfreq);
+   ret = PTR_ERR(devfreq);
+   goto err_opp_of_remove_table;
}
pfdev->devfreq.devfreq = devfreq;
 
@@ -117,6 +131,13 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
pfdev->devfreq.cooling = cooling;
 
return 0;
+
+err_opp_of_remove_table:
+   dev_pm_opp_of_remove_table(dev);
+err_opp_put_regulators:
+   if (pfdev->devfreq.regulators_opp_table)
+   dev_pm_opp_put_regulators(pfdev->devfreq.regulators_opp_table);
+   return ret;
 }
 
 void panfrost_devfreq_fini(struct panfrost_device *pfdev)
@@ -124,6 +145,8 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
if (pfdev->devfreq.cooling)
devfreq_cooling_unregister(pfdev->devfreq.cooling);
dev_pm_opp_of_remove_table(>pdev->dev);
+   if (pfdev->devfreq.regulators_opp_table)
+   dev_pm_opp_put_regulators(pfdev->devfreq.regulators_opp_table);
 }
 
 void panfrost_devfreq_resume(struct panfrost_device *pfdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h 
b/drivers/gpu/drm/panfrost/panfrost_device.h
index 06713811b92c..4878b239e301 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -85,6 +85,7 @@ struct panfrost_device {
 
struct {
struct devfreq *devfreq;
+   struct opp_table *regulators_opp_table;
struct thermal_cooling_device *cooling;
ktime_t busy_time;
ktime_t idle_time;
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFT v1 3/3] drm/panfrost: Use the mali-supply regulator for control again

[Martin Blumenstingl]

On Thu, Jan 9, 2020 at 12:31 PM Steven Price  wrote:
>
> On 07/01/2020 23:06, Martin Blumenstingl wrote:
> > dev_pm_opp_set_rate() needs a reference to the regulator which should be
> > updated when updating the GPU frequency. The name of the regulator has
> > to be passed at initialization-time using dev_pm_opp_set_regulators().
> > Add the call to dev_pm_opp_set_regulators() so dev_pm_opp_set_rate()
> > will update the GPU regulator when updating the frequency (just like
> > we did this manually before when we open-coded dev_pm_opp_set_rate()).
>
> This patch causes a warning from debugfs on my firefly (RK3288) board:
>
> debugfs: Directory 'ffa3.gpu-mali' with parent 'vdd_gpu' already
> present!
>
> So it looks like the regulator is being added twice - but I haven't
> investigated further.
I *think* it's because the regulator is already fetched by the
panfrost driver itself to enable it
(the devfreq code currently does not support enabling the regulator,
it can only control the voltage)

I'm not sure what to do about this though

[...]
> >   ret = dev_pm_opp_of_add_table(dev);
> > - if (ret)
> > + if (ret) {
> > + 
> > dev_pm_opp_put_regulators(pfdev->devfreq.regulators_opp_table);
>
> If we don't have a regulator then regulators_opp_table will be NULL and
> sadly dev_pm_opp_put_regulators() doesn't handle a NULL argument. The
> same applies to the two below calls obviously.
good catch, thank you!
are you happy with the general approach here or do you think that
dev_pm_opp_set_regulators is the wrong way to go (for whatever
reason)?


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFT v1 1/3] drm/panfrost: enable devfreq based the "operating-points-v2" property

[Martin Blumenstingl]

Hi Robin,

On Wed, Jan 8, 2020 at 12:18 PM Robin Murphy  wrote:
>
> On 07/01/2020 11:06 pm, Martin Blumenstingl wrote:
> > Decouple the check to see whether we want to enable devfreq for the GPU
> > from dev_pm_opp_set_regulators(). This is preparation work for adding
> > back support for regulator control (which means we need to call
> > dev_pm_opp_set_regulators() before dev_pm_opp_of_add_table(), which
> > means having a check for "is devfreq enabled" that is not tied to
> > dev_pm_opp_of_add_table() makes things easier).
>
> Hmm, what about cases like the SCMI DVFS protocol where the OPPs are
> dynamically discovered rather than statically defined in DT?
where can I find such an example (Amlogic SoCs use SCPI instead of
SCMI, so I don't think that I have any board with SCMI support) or
some documentation?
(I could only find SCPI clock and CPU DVFS implementations, but no
generic "OPPs for any device" implementation)


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFT v1 2/3] drm/panfrost: call dev_pm_opp_of_remove_table() in all error-paths

[Martin Blumenstingl]

If devfreq_recommended_opp() fails we need to undo
dev_pm_opp_of_add_table() by calling dev_pm_opp_of_remove_table() (just
like we do it in the other error-path below).

Fixes: f3ba91228e8e91 ("drm/panfrost: Add initial panfrost driver")
Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 1471588763ce..170f6c8c9651 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -93,8 +93,10 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
cur_freq = clk_get_rate(pfdev->clock);
 
opp = devfreq_recommended_opp(dev, _freq, 0);
-   if (IS_ERR(opp))
+   if (IS_ERR(opp)) {
+   dev_pm_opp_of_remove_table(dev);
return PTR_ERR(opp);
+   }
 
panfrost_devfreq_profile.initial_freq = cur_freq;
dev_pm_opp_put(opp);
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFT v1 0/3] devfreq fixes for panfrost

[Martin Blumenstingl]

These are a bunch of devfreq fixes for panfrost that came up in a
discussion with Robin Murphy during the code-review of the lima
devfreq patches: [0]

I am only able to test patch #1 properly because the only boards with
panfrost GPU that I have are using an Amlogic SoC. We don't have
support for the OPP tables or dynamic clock changes there yet.
So patches #2 and #3 are compile-tested only.


[0] https://patchwork.freedesktop.org/patch/346898/

Martin Blumenstingl (3):
  drm/panfrost: enable devfreq based the "operating-points-v2" property
  drm/panfrost: call dev_pm_opp_of_remove_table() in all error-paths
  drm/panfrost: Use the mali-supply regulator for control again

 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 33 ++---
 drivers/gpu/drm/panfrost/panfrost_device.h  |  1 +
 2 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFT v1 3/3] drm/panfrost: Use the mali-supply regulator for control again

[Martin Blumenstingl]

dev_pm_opp_set_rate() needs a reference to the regulator which should be
updated when updating the GPU frequency. The name of the regulator has
to be passed at initialization-time using dev_pm_opp_set_regulators().
Add the call to dev_pm_opp_set_regulators() so dev_pm_opp_set_rate()
will update the GPU regulator when updating the frequency (just like
we did this manually before when we open-coded dev_pm_opp_set_rate()).

Fixes: 221bc77914cbcc ("drm/panfrost: Use generic code for devfreq")
Reported-by: Robin Murphy 
Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 22 -
 drivers/gpu/drm/panfrost/panfrost_device.h  |  1 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 170f6c8c9651..4f7999c7b44c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -74,6 +74,7 @@ static struct devfreq_dev_profile panfrost_devfreq_profile = {
 int panfrost_devfreq_init(struct panfrost_device *pfdev)
 {
int ret;
+   struct opp_table *opp_table;
struct dev_pm_opp *opp;
unsigned long cur_freq;
struct device *dev = >pdev->dev;
@@ -84,9 +85,24 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
/* Optional, continue without devfreq */
return 0;
 
+   opp_table = dev_pm_opp_set_regulators(dev,
+ (const char *[]){ "mali" },
+ 1);
+   if (IS_ERR(opp_table)) {
+   ret = PTR_ERR(opp_table);
+
+   /* Continue if the optional regulator is missing */
+   if (ret != -ENODEV)
+   return ret;
+   } else {
+   pfdev->devfreq.regulators_opp_table = opp_table;
+   }
+
ret = dev_pm_opp_of_add_table(dev);
-   if (ret)
+   if (ret) {
+   dev_pm_opp_put_regulators(pfdev->devfreq.regulators_opp_table);
return ret;
+   }
 
panfrost_devfreq_reset(pfdev);
 
@@ -95,6 +111,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
opp = devfreq_recommended_opp(dev, _freq, 0);
if (IS_ERR(opp)) {
dev_pm_opp_of_remove_table(dev);
+   dev_pm_opp_put_regulators(pfdev->devfreq.regulators_opp_table);
return PTR_ERR(opp);
}
 
@@ -106,6 +123,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
if (IS_ERR(devfreq)) {
DRM_DEV_ERROR(dev, "Couldn't initialize GPU devfreq\n");
dev_pm_opp_of_remove_table(dev);
+   dev_pm_opp_put_regulators(pfdev->devfreq.regulators_opp_table);
return PTR_ERR(devfreq);
}
pfdev->devfreq.devfreq = devfreq;
@@ -124,6 +142,8 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
if (pfdev->devfreq.cooling)
devfreq_cooling_unregister(pfdev->devfreq.cooling);
dev_pm_opp_of_remove_table(>pdev->dev);
+   if (pfdev->devfreq.regulators_opp_table)
+   dev_pm_opp_put_regulators(pfdev->devfreq.regulators_opp_table);
 }
 
 void panfrost_devfreq_resume(struct panfrost_device *pfdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h 
b/drivers/gpu/drm/panfrost/panfrost_device.h
index 06713811b92c..4878b239e301 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -85,6 +85,7 @@ struct panfrost_device {
 
struct {
struct devfreq *devfreq;
+   struct opp_table *regulators_opp_table;
struct thermal_cooling_device *cooling;
ktime_t busy_time;
ktime_t idle_time;
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFT v1 1/3] drm/panfrost: enable devfreq based the "operating-points-v2" property

[Martin Blumenstingl]

Decouple the check to see whether we want to enable devfreq for the GPU
from dev_pm_opp_set_regulators(). This is preparation work for adding
back support for regulator control (which means we need to call
dev_pm_opp_set_regulators() before dev_pm_opp_of_add_table(), which
means having a check for "is devfreq enabled" that is not tied to
dev_pm_opp_of_add_table() makes things easier).

Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 413987038fbf..1471588763ce 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -5,6 +5,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "panfrost_device.h"
@@ -79,10 +80,12 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
struct devfreq *devfreq;
struct thermal_cooling_device *cooling;
 
-   ret = dev_pm_opp_of_add_table(dev);
-   if (ret == -ENODEV) /* Optional, continue without devfreq */
+   if (!device_property_present(dev, "operating-points-v2"))
+   /* Optional, continue without devfreq */
return 0;
-   else if (ret)
+
+   ret = dev_pm_opp_of_add_table(dev);
+   if (ret)
return ret;
 
panfrost_devfreq_reset(pfdev);
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 1/1] drm/lima: Add optional devfreq support

[Martin Blumenstingl]

Hi Robin,

On Wed, Jan 1, 2020 at 1:55 PM Robin Murphy  wrote:
>
> On 2019-12-31 4:47 pm, Martin Blumenstingl wrote:
> > Hi Robin,
> >
> > On Tue, Dec 31, 2019 at 5:40 PM Robin Murphy  wrote:
> >>
> >> On 2019-12-31 2:17 pm, Martin Blumenstingl wrote:
> >>> Hi Robin,
> >>>
> >>> On Mon, Dec 30, 2019 at 1:47 AM Robin Murphy  wrote:
> >>>>
> >>>> On 2019-12-29 11:19 pm, Martin Blumenstingl wrote:
> >>>>> Hi Robin,
> >>>>>
> >>>>> On Sun, Dec 29, 2019 at 11:58 PM Robin Murphy  
> >>>>> wrote:
> >>>>>>
> >>>>>> Hi Martin,
> >>>>>>
> >>>>>> On 2019-12-27 5:37 pm, Martin Blumenstingl wrote:
> >>>>>>> Most platforms with a Mali-400 or Mali-450 GPU also have support for
> >>>>>>> changing the GPU clock frequency. Add devfreq support so the GPU clock
> >>>>>>> rate is updated based on the actual GPU usage when the
> >>>>>>> "operating-points-v2" property is present in the board.dts.
> >>>>>>>
> >>>>>>> The actual devfreq code is taken from panfrost_devfreq.c and modified 
> >>>>>>> so
> >>>>>>> it matches what the lima hardware needs:
> >>>>>>> - a call to dev_pm_opp_set_clkname() during initialization because 
> >>>>>>> there
> >>>>>>>   are two clocks on Mali-4x0 IPs. "core" is the one that actually 
> >>>>>>> clocks
> >>>>>>>   the GPU so we need to control it using devfreq.
> >>>>>>> - locking when reading or writing the devfreq statistics because 
> >>>>>>> (unlike
> >>>>>>>   than panfrost) we have multiple PP and GP IRQs which may finish 
> >>>>>>> jobs
> >>>>>>>   concurrently.
> >>>>>>
> >>>>>> I gave this a quick try on my RK3328, and the clock scaling indeed 
> >>>>>> kicks
> >>>>>> in nicely on the glmark2 scenes that struggle, however something 
> >>>>>> appears
> >>>>>> to be missing in terms of regulator association, as the appropriate OPP
> >>>>>> voltages aren't reflected in the GPU supply (fortunately the initial
> >>>>>> voltage seems close enough to that of the highest OPP not to cause 
> >>>>>> major
> >>>>>> problems, on my box at least). With panfrost on RK3399 I do see the
> >>>>>> supply voltage scaling accordingly, but I don't know my way around
> >>>>>> devfreq well enough to know what matters in the difference :/
> >>>>> first of all: thank you for trying this out! :-)
> >>>>>
> >>>>> does your kernel include commit 221bc77914cbcc ("drm/panfrost: Use
> >>>>> generic code for devfreq") for your panfrost test?
> >>>>> if I understand the devfreq API correct then I suspect with that
> >>>>> commit panfrost also won't change the voltage anymore.
> >>>>
> >>>> Oh, you're quite right - I was already considering that change as
> >>>> ancient history, but indeed it's only in 5.5-rc, while that board is
> >>>> still on 5.4.y release kernels. No wonder I couldn't make sense of how
> >>>> the (current) code could possibly be working :)
> >>>>
> >>>> I'll try the latest -rc kernel tomorrow to confirm (now that PCIe is
> >>>> hopefully fixed), but I'm already fairly confident you've called it
> >>>> correctly.
> >>> I just tested it with the lima driver (by undervolting the GPU by
> >>> 0.05V) and it seems that dev_pm_opp_set_regulators is really needed.
> >>> I'll fix this in the next version of this patch and also submit a fix
> >>> for panfrost (I won't be able to test that though, so help is
> >>> appreciated in terms of testing :))
> >>
> >> Yeah, I started hacking something up for panfrost yesterday, but at the
> >> point of realising the core OPP code wants refactoring to actually
> >> handle optional regulators without spewing errors, decided that was
> >> crossing the line into "work" and thus could wait until next week :D
> > I'm not sure 

Re: [RFC v2 1/1] drm/lima: Add optional devfreq support

[Martin Blumenstingl]

Hi Qiang,

On Tue, Dec 31, 2019 at 3:54 AM Qiang Yu  wrote:
[...]
> > diff --git a/drivers/gpu/drm/lima/lima_sched.c 
> > b/drivers/gpu/drm/lima/lima_sched.c
> > index f522c5f99729..851c496a168b 100644
> > --- a/drivers/gpu/drm/lima/lima_sched.c
> > +++ b/drivers/gpu/drm/lima/lima_sched.c
> > @@ -5,6 +5,7 @@
> >  #include 
> >  #include 
> >
> > +#include "lima_devfreq.h"
> >  #include "lima_drv.h"
> >  #include "lima_sched.h"
> >  #include "lima_vm.h"
> > @@ -213,6 +214,8 @@ static struct dma_fence *lima_sched_run_job(struct 
> > drm_sched_job *job)
> >  */
> > ret = dma_fence_get(task->fence);
> >
> > +   lima_devfreq_record_busy(pipe->ldev);
> > +
> > pipe->current_task = task;
> >
> > /* this is needed for MMU to work correctly, otherwise GP/PP
> > @@ -280,6 +283,8 @@ static void lima_sched_handle_error_task(struct 
> > lima_sched_pipe *pipe,
> > pipe->current_vm = NULL;
> > pipe->current_task = NULL;
> >
> > +   lima_devfreq_record_idle(pipe->ldev);
> > +
> > drm_sched_resubmit_jobs(>base);
> > drm_sched_start(>base, true);
> >  }
> > @@ -348,6 +353,8 @@ void lima_sched_pipe_fini(struct lima_sched_pipe *pipe)
> >
> >  void lima_sched_pipe_task_done(struct lima_sched_pipe *pipe)
> >  {
> > +   lima_devfreq_record_idle(pipe->ldev);
>
> This should be moved to the else {} part of the following code. As you
> have added the save
> idle record to the lima_sched_handle_error_task which is just the if
> {} part of the following
> code, so no need to call it twice for error tasks. BTW.
> lima_sched_handle_error_task is also
> used for timeout tasks, so add idle record in it is fine.
oh, that is a good catch - thank you!
I will fix that in the next version


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 1/1] drm/lima: Add optional devfreq support

[Martin Blumenstingl]

Hi Robin,

On Mon, Dec 30, 2019 at 1:47 AM Robin Murphy  wrote:
>
> On 2019-12-29 11:19 pm, Martin Blumenstingl wrote:
> > Hi Robin,
> >
> > On Sun, Dec 29, 2019 at 11:58 PM Robin Murphy  wrote:
> >>
> >> Hi Martin,
> >>
> >> On 2019-12-27 5:37 pm, Martin Blumenstingl wrote:
> >>> Most platforms with a Mali-400 or Mali-450 GPU also have support for
> >>> changing the GPU clock frequency. Add devfreq support so the GPU clock
> >>> rate is updated based on the actual GPU usage when the
> >>> "operating-points-v2" property is present in the board.dts.
> >>>
> >>> The actual devfreq code is taken from panfrost_devfreq.c and modified so
> >>> it matches what the lima hardware needs:
> >>> - a call to dev_pm_opp_set_clkname() during initialization because there
> >>> are two clocks on Mali-4x0 IPs. "core" is the one that actually clocks
> >>> the GPU so we need to control it using devfreq.
> >>> - locking when reading or writing the devfreq statistics because (unlike
> >>> than panfrost) we have multiple PP and GP IRQs which may finish jobs
> >>> concurrently.
> >>
> >> I gave this a quick try on my RK3328, and the clock scaling indeed kicks
> >> in nicely on the glmark2 scenes that struggle, however something appears
> >> to be missing in terms of regulator association, as the appropriate OPP
> >> voltages aren't reflected in the GPU supply (fortunately the initial
> >> voltage seems close enough to that of the highest OPP not to cause major
> >> problems, on my box at least). With panfrost on RK3399 I do see the
> >> supply voltage scaling accordingly, but I don't know my way around
> >> devfreq well enough to know what matters in the difference :/
> > first of all: thank you for trying this out! :-)
> >
> > does your kernel include commit 221bc77914cbcc ("drm/panfrost: Use
> > generic code for devfreq") for your panfrost test?
> > if I understand the devfreq API correct then I suspect with that
> > commit panfrost also won't change the voltage anymore.
>
> Oh, you're quite right - I was already considering that change as
> ancient history, but indeed it's only in 5.5-rc, while that board is
> still on 5.4.y release kernels. No wonder I couldn't make sense of how
> the (current) code could possibly be working :)
>
> I'll try the latest -rc kernel tomorrow to confirm (now that PCIe is
> hopefully fixed), but I'm already fairly confident you've called it
> correctly.
I just tested it with the lima driver (by undervolting the GPU by
0.05V) and it seems that dev_pm_opp_set_regulators is really needed.
I'll fix this in the next version of this patch and also submit a fix
for panfrost (I won't be able to test that though, so help is
appreciated in terms of testing :))


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 1/1] drm/lima: Add optional devfreq support

[Martin Blumenstingl]

Hi Robin,

On Tue, Dec 31, 2019 at 5:40 PM Robin Murphy  wrote:
>
> On 2019-12-31 2:17 pm, Martin Blumenstingl wrote:
> > Hi Robin,
> >
> > On Mon, Dec 30, 2019 at 1:47 AM Robin Murphy  wrote:
> >>
> >> On 2019-12-29 11:19 pm, Martin Blumenstingl wrote:
> >>> Hi Robin,
> >>>
> >>> On Sun, Dec 29, 2019 at 11:58 PM Robin Murphy  
> >>> wrote:
> >>>>
> >>>> Hi Martin,
> >>>>
> >>>> On 2019-12-27 5:37 pm, Martin Blumenstingl wrote:
> >>>>> Most platforms with a Mali-400 or Mali-450 GPU also have support for
> >>>>> changing the GPU clock frequency. Add devfreq support so the GPU clock
> >>>>> rate is updated based on the actual GPU usage when the
> >>>>> "operating-points-v2" property is present in the board.dts.
> >>>>>
> >>>>> The actual devfreq code is taken from panfrost_devfreq.c and modified so
> >>>>> it matches what the lima hardware needs:
> >>>>> - a call to dev_pm_opp_set_clkname() during initialization because there
> >>>>>  are two clocks on Mali-4x0 IPs. "core" is the one that actually 
> >>>>> clocks
> >>>>>  the GPU so we need to control it using devfreq.
> >>>>> - locking when reading or writing the devfreq statistics because (unlike
> >>>>>  than panfrost) we have multiple PP and GP IRQs which may finish 
> >>>>> jobs
> >>>>>  concurrently.
> >>>>
> >>>> I gave this a quick try on my RK3328, and the clock scaling indeed kicks
> >>>> in nicely on the glmark2 scenes that struggle, however something appears
> >>>> to be missing in terms of regulator association, as the appropriate OPP
> >>>> voltages aren't reflected in the GPU supply (fortunately the initial
> >>>> voltage seems close enough to that of the highest OPP not to cause major
> >>>> problems, on my box at least). With panfrost on RK3399 I do see the
> >>>> supply voltage scaling accordingly, but I don't know my way around
> >>>> devfreq well enough to know what matters in the difference :/
> >>> first of all: thank you for trying this out! :-)
> >>>
> >>> does your kernel include commit 221bc77914cbcc ("drm/panfrost: Use
> >>> generic code for devfreq") for your panfrost test?
> >>> if I understand the devfreq API correct then I suspect with that
> >>> commit panfrost also won't change the voltage anymore.
> >>
> >> Oh, you're quite right - I was already considering that change as
> >> ancient history, but indeed it's only in 5.5-rc, while that board is
> >> still on 5.4.y release kernels. No wonder I couldn't make sense of how
> >> the (current) code could possibly be working :)
> >>
> >> I'll try the latest -rc kernel tomorrow to confirm (now that PCIe is
> >> hopefully fixed), but I'm already fairly confident you've called it
> >> correctly.
> > I just tested it with the lima driver (by undervolting the GPU by
> > 0.05V) and it seems that dev_pm_opp_set_regulators is really needed.
> > I'll fix this in the next version of this patch and also submit a fix
> > for panfrost (I won't be able to test that though, so help is
> > appreciated in terms of testing :))
>
> Yeah, I started hacking something up for panfrost yesterday, but at the
> point of realising the core OPP code wants refactoring to actually
> handle optional regulators without spewing errors, decided that was
> crossing the line into "work" and thus could wait until next week :D
I'm not sure what you mean, dev_pm_opp_set_regulators uses
regulator_get_optional.
doesn't that mean that it is optional already?


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 1/1] drm/lima: Add optional devfreq support

[Martin Blumenstingl]

Hi Robin,

On Sun, Dec 29, 2019 at 11:58 PM Robin Murphy  wrote:
>
> Hi Martin,
>
> On 2019-12-27 5:37 pm, Martin Blumenstingl wrote:
> > Most platforms with a Mali-400 or Mali-450 GPU also have support for
> > changing the GPU clock frequency. Add devfreq support so the GPU clock
> > rate is updated based on the actual GPU usage when the
> > "operating-points-v2" property is present in the board.dts.
> >
> > The actual devfreq code is taken from panfrost_devfreq.c and modified so
> > it matches what the lima hardware needs:
> > - a call to dev_pm_opp_set_clkname() during initialization because there
> >are two clocks on Mali-4x0 IPs. "core" is the one that actually clocks
> >the GPU so we need to control it using devfreq.
> > - locking when reading or writing the devfreq statistics because (unlike
> >than panfrost) we have multiple PP and GP IRQs which may finish jobs
> >concurrently.
>
> I gave this a quick try on my RK3328, and the clock scaling indeed kicks
> in nicely on the glmark2 scenes that struggle, however something appears
> to be missing in terms of regulator association, as the appropriate OPP
> voltages aren't reflected in the GPU supply (fortunately the initial
> voltage seems close enough to that of the highest OPP not to cause major
> problems, on my box at least). With panfrost on RK3399 I do see the
> supply voltage scaling accordingly, but I don't know my way around
> devfreq well enough to know what matters in the difference :/
first of all: thank you for trying this out! :-)

does your kernel include commit 221bc77914cbcc ("drm/panfrost: Use
generic code for devfreq") for your panfrost test?
if I understand the devfreq API correct then I suspect with that
commit panfrost also won't change the voltage anymore.

this is probably due to a missing call to dev_pm_opp_set_regulators()
which is supposed to attach the regulator to the devfreq instance.
I didn't notice this yet because on Amlogic SoCs the voltage is the
same for all OPPs.

I'll debug this in the next days and send an updated patch (and drop
the RFC prefix if there are no more comments).


Regards
Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v2 0/1] drm: lima: devfreq and cooling device support

[Martin Blumenstingl]

This is my attempt at adding devfreq (and cooling device) support to
the lima driver.

I am seeking comments in two general areas:
- regarding the integration into the existing lima code
- for the actual devfreq code (I had to adapt the panfrost code
  slightly, because lima uses a bus and a GPU/core clock)

My own TODO list includes "more" testing on various Amlogic SoCs.
So far I have tested this on Meson8b and Meson8m2 (which both have a
GPU OPP table defined). However, I still need to test this on a GXL
board (which is currently missing the GPU OPP table).

Test results from a Meson8m2 board:
TEST #1: glmark2-es2-drm --off-screen in an infinite loop while cycling
 through all available frequencies using the userspace governor

 From  :   To
   : 182142857 31875 42500 51000 63750   time(ms)
  182142857: 0  1274  1274  1273  1279   5399468
  31875:  1274 0  1274  1273  1272   5114700
  42500:  1276  1274 0  1272  1271   5122008
  51000:  1909  1273  1273 0   636   5274292
* 63750:   640  1272  1272  1273 0   5186796
Total transition : 24834

TEST #2: glmark2-es2-drm --off-screen in an infinite loop with the
 simple_ondemand governor
 From  :   To
   : 182142857 31875 42500 51000 63750   time(ms)
  182142857: 0 0 0 0   203318328
  31875:53 0 0 021 56044
  42500:2718 0 0 2 34172
  51000:27 614 0 1 41348
* 63750:95503348 0   2085312


Changes since RFC v1 at [0]:
- added lock to protect the statistics as these can be written 
  concurrently for example when the GP and PP IRQ are firing at the
  same time. Thanks to Qiang Yu for the suggestion!
- updated the copyright notice of lima_devfreq.c to indicate that the
  code is derived from panfrost_devfreq.c. Thanks to  Chen-Yu Tsai  for
  the suggestion!
- I did not unify the code with panfrost yet because I don't know where
  to put the result. any suggestion is welcome though!


[0] https://patchwork.freedesktop.org/series/70967/


Martin Blumenstingl (1):
  drm/lima: Add optional devfreq support

 drivers/gpu/drm/lima/Kconfig|   1 +
 drivers/gpu/drm/lima/Makefile   |   3 +-
 drivers/gpu/drm/lima/lima_devfreq.c | 183 
 drivers/gpu/drm/lima/lima_devfreq.h |  15 +++
 drivers/gpu/drm/lima/lima_device.c  |   4 +
 drivers/gpu/drm/lima/lima_device.h  |  17 +++
 drivers/gpu/drm/lima/lima_drv.c |  14 ++-
 drivers/gpu/drm/lima/lima_sched.c   |   7 ++
 drivers/gpu/drm/lima/lima_sched.h   |   3 +
 9 files changed, 244 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.c
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.h

-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v2 1/1] drm/lima: Add optional devfreq support

[Martin Blumenstingl]

Most platforms with a Mali-400 or Mali-450 GPU also have support for
changing the GPU clock frequency. Add devfreq support so the GPU clock
rate is updated based on the actual GPU usage when the
"operating-points-v2" property is present in the board.dts.

The actual devfreq code is taken from panfrost_devfreq.c and modified so
it matches what the lima hardware needs:
- a call to dev_pm_opp_set_clkname() during initialization because there
  are two clocks on Mali-4x0 IPs. "core" is the one that actually clocks
  the GPU so we need to control it using devfreq.
- locking when reading or writing the devfreq statistics because (unlike
  than panfrost) we have multiple PP and GP IRQs which may finish jobs
  concurrently.

Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/lima/Kconfig|   1 +
 drivers/gpu/drm/lima/Makefile   |   3 +-
 drivers/gpu/drm/lima/lima_devfreq.c | 183 
 drivers/gpu/drm/lima/lima_devfreq.h |  15 +++
 drivers/gpu/drm/lima/lima_device.c  |   4 +
 drivers/gpu/drm/lima/lima_device.h  |  17 +++
 drivers/gpu/drm/lima/lima_drv.c |  14 ++-
 drivers/gpu/drm/lima/lima_sched.c   |   7 ++
 drivers/gpu/drm/lima/lima_sched.h   |   3 +
 9 files changed, 244 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.c
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.h

diff --git a/drivers/gpu/drm/lima/Kconfig b/drivers/gpu/drm/lima/Kconfig
index 571dc369a7e9..cdd24b68b5d4 100644
--- a/drivers/gpu/drm/lima/Kconfig
+++ b/drivers/gpu/drm/lima/Kconfig
@@ -10,5 +10,6 @@ config DRM_LIMA
depends on OF
select DRM_SCHED
select DRM_GEM_SHMEM_HELPER
+   select PM_DEVFREQ
help
  DRM driver for ARM Mali 400/450 GPUs.
diff --git a/drivers/gpu/drm/lima/Makefile b/drivers/gpu/drm/lima/Makefile
index a85444b0a1d4..5e5c29875e9c 100644
--- a/drivers/gpu/drm/lima/Makefile
+++ b/drivers/gpu/drm/lima/Makefile
@@ -14,6 +14,7 @@ lima-y := \
lima_sched.o \
lima_ctx.o \
lima_dlbu.o \
-   lima_bcast.o
+   lima_bcast.o \
+   lima_devfreq.o
 
 obj-$(CONFIG_DRM_LIMA) += lima.o
diff --git a/drivers/gpu/drm/lima/lima_devfreq.c 
b/drivers/gpu/drm/lima/lima_devfreq.c
new file mode 100644
index ..a5fd6b8faa77
--- /dev/null
+++ b/drivers/gpu/drm/lima/lima_devfreq.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 Martin Blumenstingl 
+ *
+ * Based on panfrost_devfreq.c:
+ *   Copyright 2019 Collabora ltd.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "lima_device.h"
+#include "lima_devfreq.h"
+
+static void lima_devfreq_update_utilization(struct lima_device *ldev)
+{
+   unsigned long irqflags;
+   ktime_t now, last;
+
+   if (!ldev->devfreq.devfreq)
+   return;
+
+   spin_lock_irqsave(>devfreq.lock, irqflags);
+
+   now = ktime_get();
+   last = ldev->devfreq.time_last_update;
+
+   if (atomic_read(>devfreq.busy_count) > 0)
+   ldev->devfreq.busy_time += ktime_sub(now, last);
+   else
+   ldev->devfreq.idle_time += ktime_sub(now, last);
+
+   ldev->devfreq.time_last_update = now;
+
+   spin_unlock_irqrestore(>devfreq.lock, irqflags);
+}
+
+static int lima_devfreq_target(struct device *dev, unsigned long *freq,
+  u32 flags)
+{
+   struct dev_pm_opp *opp;
+   int err;
+
+   opp = devfreq_recommended_opp(dev, freq, flags);
+   if (IS_ERR(opp))
+   return PTR_ERR(opp);
+   dev_pm_opp_put(opp);
+
+   err = dev_pm_opp_set_rate(dev, *freq);
+   if (err)
+   return err;
+
+   return 0;
+}
+
+static void lima_devfreq_reset(struct lima_device *ldev)
+{
+   unsigned long irqflags;
+
+   spin_lock_irqsave(>devfreq.lock, irqflags);
+
+   ldev->devfreq.busy_time = 0;
+   ldev->devfreq.idle_time = 0;
+   ldev->devfreq.time_last_update = ktime_get();
+
+   spin_unlock_irqrestore(>devfreq.lock, irqflags);
+}
+
+static int lima_devfreq_get_dev_status(struct device *dev,
+  struct devfreq_dev_status *status)
+{
+   struct lima_device *ldev = dev_get_drvdata(dev);
+   unsigned long irqflags;
+
+   lima_devfreq_update_utilization(ldev);
+
+   status->current_frequency = clk_get_rate(ldev->clk_gpu);
+
+   spin_lock_irqsave(>devfreq.lock, irqflags);
+
+   status->total_time = ktime_to_ns(ktime_add(ldev->devfreq.busy_time,
+  ldev->devfreq.idle_time));
+   status->busy_time = ktime_to_ns(ldev->devfreq.busy_time);
+
+   spin_unlock_irqrestore(>devfreq.lock, irqflags);
+
+   lima_devfreq_reset(ldev);
+
+   dev_dbg(ldev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
+   status->busy_time, 

Re: [RFC v1 0/1] drm: lima: devfreq and cooling device support

[Martin Blumenstingl]

On Mon, Dec 16, 2019 at 4:03 AM Chen-Yu Tsai  wrote:
>
> On Mon, Dec 16, 2019 at 5:12 AM Martin Blumenstingl
>  wrote:
> >
> > This is my attempt at adding devfreq (and cooling device) support to
> > the lima driver.
> > I didn't have much time to do in-depth testing. However, I'm sending
> > this out early because there are many SoCs with Mali-400/450 GPU so
> > I want to avoid duplicating the work with somebody else.
> >
> > The code is derived from panfrost_devfreq.c which is why I kept the
> > Collabora copyright in lima_devfreq.c. Please let me know if I should
> > drop this or how I can make it more clear that I "borrowed" the code
> > from panfrost.
>
> I think it's more common to have separate copyright notices. First you
> have yours, then a second paragraph stating the code is derived from
> foo, and then attach the copyright statements for foo.
thank you for the hint!
I found other examples that do it like this, so I'll update it.


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v1 0/1] drm: lima: devfreq and cooling device support

[Martin Blumenstingl]

Hi Alyssa,

On Mon, Dec 16, 2019 at 4:48 PM Alyssa Rosenzweig
 wrote:
>
> If so much code is being duplicated over, I'm wondering if it makes
> sense for us to move some of the common devfreq code to core DRM
> helpers?
if you have any recommendation where to put it then please let me know
(I am not familiar with the DRM subsystem at all)

my initial idea was that the devfreq logic needs the same information
that the scheduler needs (whether we're submitting something to be
executed, there was a timeout, ...).
however, looking at drivers/gpu/drm/scheduler/ this seems pretty
stand-alone so I'm not sure it should go there
also the Mali-4x0 GPUs have some "PMU" which *may* be used instead of
polling the statistics internally
so this is where I realize that with my current knowledge I don't know
enough about lima, panfrost, DRM or the devfreq subsystem to get a
good idea where to put the code.


Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v1 0/1] drm: lima: devfreq and cooling device support

[Martin Blumenstingl]

On Mon, Dec 16, 2019 at 3:51 AM Qiang Yu  wrote:
[...]
> For the code, I think you may need some lock to protect the time records as
> there are two kernel threads gp/pp will try to mark GPU busy and several
> interrupts try to mark GPU idle.
good catch, thank you for this!
I assume the reason is that the panfrost GPUs are using a "unified"
architecture, while the ones supported by lima don't

I'll add locking so I don't run into trouble.


Thank you!
Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v1 0/1] drm: lima: devfreq and cooling device support

[Martin Blumenstingl]

This is my attempt at adding devfreq (and cooling device) support to
the lima driver.
I didn't have much time to do in-depth testing. However, I'm sending
this out early because there are many SoCs with Mali-400/450 GPU so
I want to avoid duplicating the work with somebody else.

The code is derived from panfrost_devfreq.c which is why I kept the
Collabora copyright in lima_devfreq.c. Please let me know if I should
drop this or how I can make it more clear that I "borrowed" the code
from panfrost.

I am seeking comments in two general areas:
- regarding the integration into the existing lima code
- for the actual devfreq code (I had to adapt the panfrost code
  slightly, because lima uses a bus and a GPU/core clock)

My own TODO list includes "more" testing on various Amlogic SoCs.
So far I have tested this on Meson8b and Meson8m2 (which both have a
GPU OPP table defined). However, I still need to test this on a GXL
board (which is currently missing the GPU OPP table).


Martin Blumenstingl (1):
  drm/lima: Add optional devfreq support

 drivers/gpu/drm/lima/Kconfig|   1 +
 drivers/gpu/drm/lima/Makefile   |   3 +-
 drivers/gpu/drm/lima/lima_devfreq.c | 162 
 drivers/gpu/drm/lima/lima_devfreq.h |  15 +++
 drivers/gpu/drm/lima/lima_device.c  |   4 +
 drivers/gpu/drm/lima/lima_device.h  |  11 ++
 drivers/gpu/drm/lima/lima_drv.c |  14 ++-
 drivers/gpu/drm/lima/lima_sched.c   |   7 ++
 drivers/gpu/drm/lima/lima_sched.h   |   3 +
 9 files changed, 217 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.c
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.h

-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v1 1/1] drm/lima: Add optional devfreq support

[Martin Blumenstingl]

Most platforms with a Mali-400 or Mali-450 GPU also have support for
changing the GPU clock frequency. Add devfreq support so the GPU clock
rate is updated based on the actual GPU usage when the
"operating-points-v2" property is present in the board.dts.

The actual devfreq code is taken from panfrost_devfreq.c.

Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/lima/Kconfig|   1 +
 drivers/gpu/drm/lima/Makefile   |   3 +-
 drivers/gpu/drm/lima/lima_devfreq.c | 162 
 drivers/gpu/drm/lima/lima_devfreq.h |  15 +++
 drivers/gpu/drm/lima/lima_device.c  |   4 +
 drivers/gpu/drm/lima/lima_device.h  |  11 ++
 drivers/gpu/drm/lima/lima_drv.c |  14 ++-
 drivers/gpu/drm/lima/lima_sched.c   |   7 ++
 drivers/gpu/drm/lima/lima_sched.h   |   3 +
 9 files changed, 217 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.c
 create mode 100644 drivers/gpu/drm/lima/lima_devfreq.h

diff --git a/drivers/gpu/drm/lima/Kconfig b/drivers/gpu/drm/lima/Kconfig
index 571dc369a7e9..cdd24b68b5d4 100644
--- a/drivers/gpu/drm/lima/Kconfig
+++ b/drivers/gpu/drm/lima/Kconfig
@@ -10,5 +10,6 @@ config DRM_LIMA
depends on OF
select DRM_SCHED
select DRM_GEM_SHMEM_HELPER
+   select PM_DEVFREQ
help
  DRM driver for ARM Mali 400/450 GPUs.
diff --git a/drivers/gpu/drm/lima/Makefile b/drivers/gpu/drm/lima/Makefile
index a85444b0a1d4..5e5c29875e9c 100644
--- a/drivers/gpu/drm/lima/Makefile
+++ b/drivers/gpu/drm/lima/Makefile
@@ -14,6 +14,7 @@ lima-y := \
lima_sched.o \
lima_ctx.o \
lima_dlbu.o \
-   lima_bcast.o
+   lima_bcast.o \
+   lima_devfreq.o
 
 obj-$(CONFIG_DRM_LIMA) += lima.o
diff --git a/drivers/gpu/drm/lima/lima_devfreq.c 
b/drivers/gpu/drm/lima/lima_devfreq.c
new file mode 100644
index ..9cefce6352db
--- /dev/null
+++ b/drivers/gpu/drm/lima/lima_devfreq.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright 2019 Collabora ltd. */
+/* Copyright 2019 Martin Blumenstingl  */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "lima_device.h"
+#include "lima_devfreq.h"
+
+static void lima_devfreq_update_utilization(struct lima_device *ldev)
+{
+   ktime_t now;
+   ktime_t last;
+
+   if (!ldev->devfreq.devfreq)
+   return;
+
+   now = ktime_get();
+   last = ldev->devfreq.time_last_update;
+
+   if (atomic_read(>devfreq.busy_count) > 0)
+   ldev->devfreq.busy_time += ktime_sub(now, last);
+   else
+   ldev->devfreq.idle_time += ktime_sub(now, last);
+
+   ldev->devfreq.time_last_update = now;
+}
+
+static int lima_devfreq_target(struct device *dev, unsigned long *freq,
+  u32 flags)
+{
+   struct dev_pm_opp *opp;
+   int err;
+
+   opp = devfreq_recommended_opp(dev, freq, flags);
+   if (IS_ERR(opp))
+   return PTR_ERR(opp);
+   dev_pm_opp_put(opp);
+
+   err = dev_pm_opp_set_rate(dev, *freq);
+   if (err)
+   return err;
+
+   return 0;
+}
+
+static void lima_devfreq_reset(struct lima_device *ldev)
+{
+   ldev->devfreq.busy_time = 0;
+   ldev->devfreq.idle_time = 0;
+   ldev->devfreq.time_last_update = ktime_get();
+}
+
+static int lima_devfreq_get_dev_status(struct device *dev,
+  struct devfreq_dev_status *status)
+{
+   struct lima_device *ldev = dev_get_drvdata(dev);
+
+   lima_devfreq_update_utilization(ldev);
+
+   status->current_frequency = clk_get_rate(ldev->clk_gpu);
+   status->total_time = ktime_to_ns(ktime_add(ldev->devfreq.busy_time,
+  ldev->devfreq.idle_time));
+
+   status->busy_time = ktime_to_ns(ldev->devfreq.busy_time);
+
+   lima_devfreq_reset(ldev);
+
+   dev_dbg(ldev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
+   status->busy_time, status->total_time,
+   status->busy_time / (status->total_time / 100),
+   status->current_frequency / 1000 / 1000);
+
+   return 0;
+}
+
+static struct devfreq_dev_profile lima_devfreq_profile = {
+   .polling_ms = 50, /* ~3 frames */
+   .target = lima_devfreq_target,
+   .get_dev_status = lima_devfreq_get_dev_status,
+};
+
+int lima_devfreq_init(struct lima_device *ldev)
+{
+   struct thermal_cooling_device *cooling;
+   struct device *dev = >pdev->dev;
+   struct devfreq *devfreq;
+   struct dev_pm_opp *opp;
+   unsigned long cur_freq;
+   int ret;
+
+   ldev->devfreq.opp_table = dev_pm_opp_set_clkname(dev, "core");
+   if (IS_ERR(ldev->devfreq.opp_table))
+   return PTR_ERR(ldev->devfreq.opp_table);
+
+   ret = dev_pm_opp_of_add_table(dev);
+   i

[PATCH v2 0/2] Meson VPU: fix CVBS output

[Martin Blumenstingl]

The goal of this series is to fix the CVBS output with the Meson VPU
driver. Prior to this series kmscube reported:
  failed to set mode: Invalid argument

Changes since v1 at [0]:
- add patch to remove duplicate code (to match patch #2 easier)
- use drm_mode_match without DRM_MODE_MATCH_ASPECT_RATIO as suggested
  by Neil


[0] https://patchwork.kernel.org/patch/11268161/


Martin Blumenstingl (2):
  drm: meson: venc: cvbs: deduplicate the meson_cvbs_mode lookup code
  drm: meson: venc: cvbs: fix CVBS mode matching

 drivers/gpu/drm/meson/meson_venc_cvbs.c | 48 ++---
 1 file changed, 27 insertions(+), 21 deletions(-)

-- 
2.24.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm: meson: venc: cvbs: deduplicate the meson_cvbs_mode lookup code

[Martin Blumenstingl]

Use a utility function to remove a bit of code duplication between
meson_venc_cvbs_encoder_atomic_check() and
meson_venc_cvbs_encoder_mode_set(). Both need to look up the struct
meson_venc_cvbs based on a drm_display_mode.

Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/meson/meson_venc_cvbs.c | 44 +
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_venc_cvbs.c 
b/drivers/gpu/drm/meson/meson_venc_cvbs.c
index 9ab27aecfcf3..6b8a074e4ff4 100644
--- a/drivers/gpu/drm/meson/meson_venc_cvbs.c
+++ b/drivers/gpu/drm/meson/meson_venc_cvbs.c
@@ -64,6 +64,21 @@ struct meson_cvbs_mode 
meson_cvbs_modes[MESON_CVBS_MODES_COUNT] = {
},
 };
 
+static const struct meson_cvbs_mode *
+meson_cvbs_get_mode(const struct drm_display_mode *req_mode)
+{
+   int i;
+
+   for (i = 0; i < MESON_CVBS_MODES_COUNT; ++i) {
+   struct meson_cvbs_mode *meson_mode = _cvbs_modes[i];
+
+   if (drm_mode_equal(req_mode, _mode->mode))
+   return meson_mode;
+   }
+
+   return NULL;
+}
+
 /* Connector */
 
 static void meson_cvbs_connector_destroy(struct drm_connector *connector)
@@ -136,14 +151,8 @@ static int meson_venc_cvbs_encoder_atomic_check(struct 
drm_encoder *encoder,
struct drm_crtc_state *crtc_state,
struct drm_connector_state *conn_state)
 {
-   int i;
-
-   for (i = 0; i < MESON_CVBS_MODES_COUNT; ++i) {
-   struct meson_cvbs_mode *meson_mode = _cvbs_modes[i];
-
-   if (drm_mode_equal(_state->mode, _mode->mode))
-   return 0;
-   }
+   if (meson_cvbs_get_mode(_state->mode))
+   return 0;
 
return -EINVAL;
 }
@@ -191,24 +200,17 @@ static void meson_venc_cvbs_encoder_mode_set(struct 
drm_encoder *encoder,
   struct drm_display_mode *mode,
   struct drm_display_mode *adjusted_mode)
 {
+   const struct meson_cvbs_mode *meson_mode = meson_cvbs_get_mode(mode);
struct meson_venc_cvbs *meson_venc_cvbs =
encoder_to_meson_venc_cvbs(encoder);
struct meson_drm *priv = meson_venc_cvbs->priv;
-   int i;
 
-   for (i = 0; i < MESON_CVBS_MODES_COUNT; ++i) {
-   struct meson_cvbs_mode *meson_mode = _cvbs_modes[i];
+   if (meson_mode) {
+   meson_venci_cvbs_mode_set(priv, meson_mode->enci);
 
-   if (drm_mode_equal(mode, _mode->mode)) {
-   meson_venci_cvbs_mode_set(priv,
- meson_mode->enci);
-
-   /* Setup 27MHz vclk2 for ENCI and VDAC */
-   meson_vclk_setup(priv, MESON_VCLK_TARGET_CVBS,
-MESON_VCLK_CVBS, MESON_VCLK_CVBS,
-MESON_VCLK_CVBS, true);
-   break;
-   }
+   /* Setup 27MHz vclk2 for ENCI and VDAC */
+   meson_vclk_setup(priv, MESON_VCLK_TARGET_CVBS, MESON_VCLK_CVBS,
+MESON_VCLK_CVBS, MESON_VCLK_CVBS, true);
}
 }
 
-- 
2.24.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm: meson: venc: cvbs: fix CVBS mode matching

[Martin Blumenstingl]

With commit 222ec1618c3ace ("drm: Add aspect ratio parsing in DRM
layer") the drm core started honoring the picture_aspect_ratio field
when comparing two drm_display_modes. Prior to that it was ignored.
When the CVBS encoder driver was initially submitted there was no aspect
ratio check.

Switch from drm_mode_equal() to drm_mode_match() without
DRM_MODE_MATCH_ASPECT_RATIO to fix "kmscube" and X.org output using the
CVBS connector. When (for example) kmscube sets the output mode when
using the CVBS connector it passes HDMI_PICTURE_ASPECT_NONE, making the
drm_mode_equal() fail as it include the aspect ratio.

Prior to this patch kmscube reported:
  failed to set mode: Invalid argument

The CVBS mode checking in the sun4i (drivers/gpu/drm/sun4i/sun4i_tv.c
sun4i_tv_mode_to_drm_mode) and ZTE (drivers/gpu/drm/zte/zx_tvenc.c
tvenc_mode_{pal,ntsc}) drivers don't set the "picture_aspect_ratio" at
all. The Meson VPU driver does not rely on the aspect ratio for the CVBS
output so we can safely decouple it from the hdmi_picture_aspect
setting.

Fixes: 222ec1618c3ace ("drm: Add aspect ratio parsing in DRM layer")
Fixes: bbbe775ec5b5da ("drm: Add support for Amlogic Meson Graphic Controller")
Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/meson/meson_venc_cvbs.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/meson/meson_venc_cvbs.c 
b/drivers/gpu/drm/meson/meson_venc_cvbs.c
index 6b8a074e4ff4..1bd6b6d15ffb 100644
--- a/drivers/gpu/drm/meson/meson_venc_cvbs.c
+++ b/drivers/gpu/drm/meson/meson_venc_cvbs.c
@@ -72,7 +72,11 @@ meson_cvbs_get_mode(const struct drm_display_mode *req_mode)
for (i = 0; i < MESON_CVBS_MODES_COUNT; ++i) {
struct meson_cvbs_mode *meson_mode = _cvbs_modes[i];
 
-   if (drm_mode_equal(req_mode, _mode->mode))
+   if (drm_mode_match(req_mode, _mode->mode,
+  DRM_MODE_MATCH_TIMINGS |
+  DRM_MODE_MATCH_CLOCK |
+  DRM_MODE_MATCH_FLAGS |
+  DRM_MODE_MATCH_3D_FLAGS))
return meson_mode;
}
 
-- 
2.24.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: meson: venc: cvbs: fix CVBS mode matching

[Martin Blumenstingl]

Drop the picture_aspect_ratio from the drm_display_modes which are valid
for the Amlogic Meson CVBS encoder. meson_venc_cvbs_encoder_atomic_check
and meson_venc_cvbs_encoder_mode_set only support two very specific
drm_display_modes.

With commit 222ec1618c3ace ("drm: Add aspect ratio parsing in DRM
layer") the drm core started honoring the picture_aspect_ratio field
when comparing two drm_display_modes. Prior to that it was ignored.
When the CVBS encoder driver was initially submitted there was no aspect
ratio check.

This patch fixes "kmscube" and X.org output using the CVBS connector
with the Amlogic Meson VPU driver. Prior to this patch kmscube reported:
  failed to set mode: Invalid argument
Additionally it makes the CVBS mode checking behave identical to the
sun4i (drivers/gpu/drm/sun4i/sun4i_tv.c sun4i_tv_mode_to_drm_mode) and
ZTE (drivers/gpu/drm/zte/zx_tvenc.c tvenc_mode_{pal,ntsc}) which are
both not setting "picture_aspect_ratio" either.

Fixes: 222ec1618c3ace ("drm: Add aspect ratio parsing in DRM layer")
Fixes: bbbe775ec5b5da ("drm: Add support for Amlogic Meson Graphic Controller")
Signed-off-by: Martin Blumenstingl 
---
 drivers/gpu/drm/meson/meson_venc_cvbs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_venc_cvbs.c 
b/drivers/gpu/drm/meson/meson_venc_cvbs.c
index 9ab27aecfcf3..2ddcda8fa5b0 100644
--- a/drivers/gpu/drm/meson/meson_venc_cvbs.c
+++ b/drivers/gpu/drm/meson/meson_venc_cvbs.c
@@ -49,7 +49,6 @@ struct meson_cvbs_mode 
meson_cvbs_modes[MESON_CVBS_MODES_COUNT] = {
 720, 732, 795, 864, 0, 576, 580, 586, 625, 0,
 DRM_MODE_FLAG_INTERLACE),
.vrefresh = 50,
-   .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3,
},
},
{ /* NTSC */
@@ -59,7 +58,6 @@ struct meson_cvbs_mode 
meson_cvbs_modes[MESON_CVBS_MODES_COUNT] = {
720, 739, 801, 858, 0, 480, 488, 494, 525, 0,
DRM_MODE_FLAG_INTERLACE),
.vrefresh = 60,
-   .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3,
},
},
 };
-- 
2.24.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/meson: vclk: use the correct G12A frac max value

[Martin Blumenstingl]

On Mon, Aug 26, 2019 at 4:47 PM Neil Armstrong  wrote:
>
> When calculating the HDMI PLL settings for a DMT mode PHY frequency,
> use the correct max fractional PLL value for G12A VPU.
>
> With this fix, we can finally setup the 1024x76-60 mode.
nit-pick: is this really 1024x76 or 1024x768?

Re: [PATCH] drm/meson: Fix atomic mode switching regression

[Martin Blumenstingl]

Hi Neil,

On Wed, Jan 9, 2019 at 2:36 PM Neil Armstrong  wrote:
>
> Since commit 2bcd3ecab773 when switching mode from X11 (ubuntu mate for
> example) the display gets blurry, looking like an invalid framebuffer width.
>
> This commit fixed atomic crtc modesetting but didn't update the display
> parameters when changing mode, but only when starting a mode setting after
> a crtc disable.
>
> This commit setups the crctc parameter in _begin() and _enable() to
> take in account the current ctrc parameters.
>
> Reported-by: Tony McKahan 
> Fixes: 2bcd3ecab773 ("drm/meson: Fixes for drm_crtc_vblank_on/off support")
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/gpu/drm/meson/meson_crtc.c | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_crtc.c 
> b/drivers/gpu/drm/meson/meson_crtc.c
> index 75d97f1b2e8f..5bb432021caf 100644
> --- a/drivers/gpu/drm/meson/meson_crtc.c
> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> @@ -82,14 +82,12 @@ static const struct drm_crtc_funcs meson_crtc_funcs = {
>
>  };
>
> -static void meson_crtc_enable(struct drm_crtc *crtc)
> +static void meson_crtc_setup(struct drm_crtc *crtc)
>  {
> struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
> struct drm_crtc_state *crtc_state = crtc->state;
> struct meson_drm *priv = meson_crtc->priv;
>
> -   DRM_DEBUG_DRIVER("\n");
> -
> if (!crtc_state) {
> DRM_ERROR("Invalid crtc_state\n");
> return;
> @@ -98,6 +96,16 @@ static void meson_crtc_enable(struct drm_crtc *crtc)
> /* Enable VPP Postblend */
nit-pick: this "enable" comment is now in meson_crtc_setup().
I would drop it because my interpretation of the following lines is
now "setting VPP_POSTBLEND_H_SIZE enables the VPP postblend"

> writel(crtc_state->mode.hdisplay,
>priv->io_base + _REG(VPP_POSTBLEND_H_SIZE));
> +}
> +
> +static void meson_crtc_enable(struct drm_crtc *crtc)
> +{
> +   struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
> +   struct meson_drm *priv = meson_crtc->priv;
> +
> +   DRM_DEBUG_DRIVER("\n");
> +
> +   meson_crtc_setup(crtc);
>
> /* VD1 Preblend vertical start/end */
> writel(FIELD_PREP(GENMASK(11, 0), 2303),
> @@ -121,6 +129,8 @@ static void meson_crtc_atomic_enable(struct drm_crtc 
> *crtc,
>
> if (!meson_crtc->enabled)
> meson_crtc_enable(crtc);
> +   else
> +   meson_crtc_setup(crtc);
it's probably only personal preference, but have you thought about
re-ordering this:
  meson_crtc_setup(crtc);

  if (!meson_crtc->enabled)
meson_crtc_enable(crtc);

with that you could get rid of the meson_crtc_setup() call in
meson_crtc_enable(), leaving only one code-path (instead of two) which
calls meson_crtc_setup()


Regards
Martin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] clk: meson: meson8b: add the GPU clock tree

[Martin Blumenstingl]

Hi Neil,

On Tue, Dec 11, 2018 at 10:21 AM Neil Armstrong  wrote:
>
> On 08/12/2018 18:12, Martin Blumenstingl wrote:
> > Add the GPU clock tree on Meson8, Meson8b and Meson8m2.
> >
> > The GPU clock tree on Meson8b and Meson8m2 is almost identical to the
> > one one GXBB:
> > - there's a glitch-free mux at HHI_MALI_CLK_CNTL[31]
> > - there are two identical parents for this mux: mali_0 and mali_1, each
> >   with a gate, divider and mux
> > - the parents of mali_0_sel and mali_1_sel are identical to GXBB except
> >   there's no GP0_PLL on these 32-bit SoCs
> >
> > Meson8 is different because it does not have the glitch-free mux.
> > Instead if only has the mali_0 clock tree. The parents of mali_0_sel are
> > identical to the ones on Meson8b and Meson8m2.
> >
> > Signed-off-by: Martin Blumenstingl 
> > ---
> >  drivers/clk/meson/meson8b.c | 146 
> >  drivers/clk/meson/meson8b.h |   9 ++-
> >  2 files changed, 154 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
> > index 0b9353d8d4fd..748552c5f6c8 100644
> > --- a/drivers/clk/meson/meson8b.c
> > +++ b/drivers/clk/meson/meson8b.c
> > @@ -1573,6 +1573,135 @@ static struct clk_regmap meson8b_hdmi_sys = {
> >   },
> >  };
> >
> > +/*
> > + * The MALI IP is clocked by two identical clocks (mali_0 and mali_1)
> > + * muxed by a glitch-free switch on Meson8b and Meson8m2. Meson8 only
> > + * has mali_0 and no glitch-free mux.
> > + */
> > +static const char * const meson8b_mali_0_1_parent_names[] = {
> > + "xtal", "mpll2", "mpll1", "fclk_div7", "fclk_div4", "fclk_div3",
> > + "fclk_div5"
> > +};
> > +
> > +static u32 meson8b_mali_0_1_mux_table[] = { 0, 2, 3, 4, 5, 6, 7 };
> > +
> > +static struct clk_regmap meson8b_mali_0_sel = {
> > + .data = &(struct clk_regmap_mux_data){
> > + .offset = HHI_MALI_CLK_CNTL,
> > + .mask = 0x7,
> > + .shift = 9,
> > + .table = meson8b_mali_0_1_mux_table,
> > + },
> > + .hw.init = &(struct clk_init_data){
> > + .name = "mali_0_sel",
> > + .ops = _regmap_mux_ops,
> > + .parent_names = meson8b_mali_0_1_parent_names,
> > + .num_parents = ARRAY_SIZE(meson8b_mali_0_1_parent_names),
> > + /*
> > +  * Don't propagate rate changes up because the only changeable
> > +  * parents are mpll1 and mpll2 but we need those for audio and
> > +  * RGMII (Ethernet). We don't want to change the audio or
> > +  * Ethernet clocks when setting the GPU frequency.
> > +  */
> > + .flags = 0,
> > + },
> > +};
> > +
> > +static struct clk_regmap meson8b_mali_0_div = {
> > + .data = &(struct clk_regmap_div_data){
> > + .offset = HHI_MALI_CLK_CNTL,
> > + .shift = 0,
> > + .width = 7,
> > + },
> > + .hw.init = &(struct clk_init_data){
> > + .name = "mali_0_div",
> > + .ops = _regmap_divider_ops,
> > + .parent_names = (const char *[]){ "mali_0_sel" },
> > + .num_parents = 1,
> > + .flags = CLK_SET_RATE_PARENT,
> > + },
> > +};
> > +
> > +static struct clk_regmap meson8b_mali_0 = {
> > + .data = &(struct clk_regmap_gate_data){
> > + .offset = HHI_MALI_CLK_CNTL,
> > + .bit_idx = 8,
> > + },
> > + .hw.init = &(struct clk_init_data){
> > + .name = "mali_0",
> > + .ops = _regmap_gate_ops,
> > + .parent_names = (const char *[]){ "mali_0_div" },
> > + .num_parents = 1,
> > + .flags = CLK_SET_RATE_PARENT,
> > + },
> > +};
> > +
> > +static struct clk_regmap meson8b_mali_1_sel = {
> > + .data = &(struct clk_regmap_mux_data){
> > + .offset = HHI_MALI_CLK_CNTL,
> > + .mask = 0x7,
> > + .shift = 25,
> > + .table = meson8b_mali_0_1_mux_table,
> > + },
> > + .hw.init = &(struct clk_init_data){
> > + .name = "mali_1_sel",
> > + .ops = _regmap_mux_ops,
> > + .parent_names = meson8b_mali_0_1_parent_na

[PATCH 4/5] ARM: dts: meson8: add the Mali-450 MP6 GPU

[Martin Blumenstingl]

Add the Mali-450 GPU and it's OPP table for the Meson8 and Meson8m2 (the
latter inherits meson8.dtsi).
These SoCs have a Mali-450 GPU with six pixel processors. The OPP table
is taken from the 3.10 vendor kernel which uses the following table:
  FCLK_DEV7 | 1, /* 182.1 Mhz */
  FCLK_DEV4 | 1, /* 318.7 Mhz */
  FCLK_DEV3 | 1, /* 425 Mhz */
  FCLK_DEV5 | 0, /* 510 Mhz */
  FCLK_DEV4 | 0, /* 637.5 Mhz */
This describes the mux (FCLK_DEVx) and a 0-based divider in the clock
controller. "FCLK" is "fixed_pll" which is running at 2550MHz.
The "turbo" setting is described by "turbo_clock = 4" where 4 is the
index of the table above.

Signed-off-by: Martin Blumenstingl 
---
 arch/arm/boot/dts/meson8.dtsi | 58 +++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index 3fd8260eba92..1ea5a36c5040 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -166,6 +166,32 @@
};
};
 
+   gpu_opp_table: gpu-opp-table {
+   compatible = "operating-points-v2";
+
+   opp-18215 {
+   opp-hz = /bits/ 64 <18215>;
+   opp-microvolt = <115>;
+   };
+   opp-31875 {
+   opp-hz = /bits/ 64 <31875>;
+   opp-microvolt = <115>;
+   };
+   opp-42500 {
+   opp-hz = /bits/ 64 <42500>;
+   opp-microvolt = <115>;
+   };
+   opp-51000 {
+   opp-hz = /bits/ 64 <51000>;
+   opp-microvolt = <115>;
+   };
+   opp-63750 {
+   opp-hz = /bits/ 64 <63750>;
+   opp-microvolt = <115>;
+   turbo-mode;
+   };
+   };
+
pmu {
compatible = "arm,cortex-a9-pmu";
interrupts = ,
@@ -208,6 +234,38 @@
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0xd000 0x20>;
+
+   mali: gpu@c {
+   compatible = "amlogic,meson8-mali", "arm,mali-450";
+   reg = <0xc 0x4>;
+   interrupts = ,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+,
+;
+   interrupt-names = "gp", "gpmmu", "pp", "pmu",
+ "pp0", "ppmmu0", "pp1", "ppmmu1",
+ "pp2", "ppmmu2", "pp4", "ppmmu4",
+ "pp5", "ppmmu5", "pp6", "ppmmu6";
+   resets = < RESET_MALI>;
+   clocks = < CLKID_CLK81>, < CLKID_MALI>;
+   clock-names = "bus", "core";
+   operating-points-v2 = <_opp_table>;
+   switch-delay = <0x>;
+   };
};
 }; /* end of / */
 
-- 
2.19.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] clk: meson: meson8b: use a separate clock table for Meson8

[Martin Blumenstingl]

The Meson8 SoC is slightly different compared to Meson8b and Meson8m2
because it does not have the glitch-free Mali GPU clock mux. For Meson8b
and Meson8m2 there are currently no known differences.

Add a separate clk_hw_onecell_data table for Meson8 so these differences
can be implemented. For now meson8_hw_onecell_data is a clone of our
existing meson8b_hw_onecell_data.

Signed-off-by: Martin Blumenstingl 
---
 drivers/clk/meson/meson8b.c | 203 ++--
 1 file changed, 197 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index 950d0e548c75..0b9353d8d4fd 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -1659,6 +1659,185 @@ static MESON_GATE(meson8b_ao_ahb_sram, HHI_GCLK_AO, 1);
 static MESON_GATE(meson8b_ao_ahb_bus, HHI_GCLK_AO, 2);
 static MESON_GATE(meson8b_ao_iface, HHI_GCLK_AO, 3);
 
+static struct clk_hw_onecell_data meson8_hw_onecell_data = {
+   .hws = {
+   [CLKID_XTAL] = _xtal.hw,
+   [CLKID_PLL_FIXED] = _fixed_pll.hw,
+   [CLKID_PLL_VID] = _vid_pll.hw,
+   [CLKID_PLL_SYS] = _sys_pll.hw,
+   [CLKID_FCLK_DIV2] = _fclk_div2.hw,
+   [CLKID_FCLK_DIV3] = _fclk_div3.hw,
+   [CLKID_FCLK_DIV4] = _fclk_div4.hw,
+   [CLKID_FCLK_DIV5] = _fclk_div5.hw,
+   [CLKID_FCLK_DIV7] = _fclk_div7.hw,
+   [CLKID_CPUCLK] = _cpu_clk.hw,
+   [CLKID_MPEG_SEL] = _mpeg_clk_sel.hw,
+   [CLKID_MPEG_DIV] = _mpeg_clk_div.hw,
+   [CLKID_CLK81] = _clk81.hw,
+   [CLKID_DDR] = _ddr.hw,
+   [CLKID_DOS] = _dos.hw,
+   [CLKID_ISA] = _isa.hw,
+   [CLKID_PL301]   = _pl301.hw,
+   [CLKID_PERIPHS] = _periphs.hw,
+   [CLKID_SPICC]   = _spicc.hw,
+   [CLKID_I2C] = _i2c.hw,
+   [CLKID_SAR_ADC] = _sar_adc.hw,
+   [CLKID_SMART_CARD]  = _smart_card.hw,
+   [CLKID_RNG0]= _rng0.hw,
+   [CLKID_UART0]   = _uart0.hw,
+   [CLKID_SDHC]= _sdhc.hw,
+   [CLKID_STREAM]  = _stream.hw,
+   [CLKID_ASYNC_FIFO]  = _async_fifo.hw,
+   [CLKID_SDIO]= _sdio.hw,
+   [CLKID_ABUF]= _abuf.hw,
+   [CLKID_HIU_IFACE]   = _hiu_iface.hw,
+   [CLKID_ASSIST_MISC] = _assist_misc.hw,
+   [CLKID_SPI] = _spi.hw,
+   [CLKID_I2S_SPDIF]   = _i2s_spdif.hw,
+   [CLKID_ETH] = _eth.hw,
+   [CLKID_DEMUX]   = _demux.hw,
+   [CLKID_AIU_GLUE]= _aiu_glue.hw,
+   [CLKID_IEC958]  = _iec958.hw,
+   [CLKID_I2S_OUT] = _i2s_out.hw,
+   [CLKID_AMCLK]   = _amclk.hw,
+   [CLKID_AIFIFO2] = _aififo2.hw,
+   [CLKID_MIXER]   = _mixer.hw,
+   [CLKID_MIXER_IFACE] = _mixer_iface.hw,
+   [CLKID_ADC] = _adc.hw,
+   [CLKID_BLKMV]   = _blkmv.hw,
+   [CLKID_AIU] = _aiu.hw,
+   [CLKID_UART1]   = _uart1.hw,
+   [CLKID_G2D] = _g2d.hw,
+   [CLKID_USB0]= _usb0.hw,
+   [CLKID_USB1]= _usb1.hw,
+   [CLKID_RESET]   = _reset.hw,
+   [CLKID_NAND]= _nand.hw,
+   [CLKID_DOS_PARSER]  = _dos_parser.hw,
+   [CLKID_USB] = _usb.hw,
+   [CLKID_VDIN1]   = _vdin1.hw,
+   [CLKID_AHB_ARB0]= _ahb_arb0.hw,
+   [CLKID_EFUSE]   = _efuse.hw,
+   [CLKID_BOOT_ROM]= _boot_rom.hw,
+   [CLKID_AHB_DATA_BUS]= _ahb_data_bus.hw,
+   [CLKID_AHB_CTRL_BUS]= _ahb_ctrl_bus.hw,
+   [CLKID_HDMI_INTR_SYNC]  = _hdmi_intr_sync.hw,
+   [CLKID_HDMI_PCLK]   = _hdmi_pclk.hw,
+   [CLKID_USB1_DDR_BRIDGE] = _usb1_ddr_bridge.hw,
+   [CLKID_USB0_DDR_BRIDGE] = _usb0_ddr_bridge.hw,
+   [CLKID_MMC_PCLK]= _mmc_pclk.hw,
+   [CLKID_DVIN]= _dvin.hw,
+   [CLKID_UART2]   = _uart2.hw,
+   [CLKID_SANA]= _sana.hw,
+   [CLKID_VPU_INTR]= _vpu_intr.hw,
+   [CLKID_SEC_AHB_AHB3_BRIDGE] = _sec_ahb_ahb3_bridge.hw,
+   [CLKID_CLK81_A9]= _clk81_a9.hw

[PATCH 1/5] dt-bindings: gpu: mali-utgard: add Amlogic Meson8 and Meson8b compatible

[Martin Blumenstingl]

Add a compatible string for the Mali-450 GPU on Amlogic Meson8 and
Meson8b SoCs. Meson8 uses an "MP6" variant with six pixel processors
while Meson8b (as cost-reduced SoC) uses an "MP2" variant with two pixel
processors. Both have a reset line to bring the GPU into a well-defined
state.

Signed-off-by: Martin Blumenstingl 
---
 Documentation/devicetree/bindings/gpu/arm,mali-utgard.txt | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-utgard.txt 
b/Documentation/devicetree/bindings/gpu/arm,mali-utgard.txt
index 63cd91176a68..efa1077a90cb 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-utgard.txt
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-utgard.txt
@@ -12,6 +12,8 @@ Required properties:
   + allwinner,sun7i-a20-mali
   + allwinner,sun8i-h3-mali
   + allwinner,sun50i-h5-mali
+  + amlogic,meson8-mali
+  + amlogic,meson8b-mali
   + amlogic,meson-gxbb-mali
   + amlogic,meson-gxl-mali
   + rockchip,rk3036-mali
@@ -77,6 +79,10 @@ to specify one more vendor-specific compatible, among:
 Required properties:
   * resets: phandle to the reset line for the GPU
 
+  - amlogic,meson8-mali and amlogic,meson8b-mali
+Required properties:
+  * resets: phandle to the reset line for the GPU
+
   - Rockchip variants:
 Required properties:
   * resets: phandle to the reset line for the GPU
-- 
2.19.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/5] Meson (32-bit): add support for the Mali GPU

[Martin Blumenstingl]

This series adds support for the Mali-450 GPU on Meson8 and Meson8b.
Meson6 uses a Mali-400 GPU but since we don't have a clock driver (and
I don't have a device for testing) Meson6 is left out in this series.

Meson8 uses a Mali-450 MP6 with six pixel processors. Meson8b (as
cost-reduced SoC) uses a Mali-450 MP2 with two pixel processors.
I tested both using the open source lima driver and a patched mesa
from the lima project as well. Since we don't have display support
on the 32-bit SoCs I used off-screen rendering as described in [0].
The result is (for example): [1]

The bootloader (at least on my boards) leaves the Mali clock disabled
by default. The board crashes when trying to access the Mali registers
with the "core" clock disabled.
Thus this series also implements the required clock driver changes. The
Mali clock tree on Meson8b and Meson8m2 is almost identical to the one
on GXBB (see the description of patch #3 for more details). Only Meson8
is slightly different as it doesn't have a glitch-free mux. Patch #2
prepares the meson8b clock driver so we can have different clocks per
SoC.

Dependencies:
- the .dts changes depend on my other series "ARM: dts: meson: add the
  APB/APB2 busses" from [2]
- the .dts changes from this series have no compile-time dependency on
  the clock driver changes because CLKID_MALI was defined in the
  dt-bindings since the first version of the clock driver (but it was
  not used until now).
- the .dts changes from this series have a runtime dependency on the
  clock driver changes (also from this series) if you have a kernel
  patched with the lima driver (without the lima driver there's no
  runtime dependency)

Other notes:
By default the GPU runs off the XTAL clock (24MHz). The lima driver
currently does not update the GPU clock rate. Different frequencies
have to be requested by adding the following properties to the Mali
GPU node (to run it at 510MHz for example):
  assigned-clocks = < CLKID_MALI>;
  assigned-clock-rates = <51000>;


[0] https://gitlab.freedesktop.org/lima/web/wikis/home
[1] https://abload.de/img/dump0myic0.png
[2] https://patchwork.kernel.org/cover/10719445/


Martin Blumenstingl (5):
  dt-bindings: gpu: mali-utgard: add Amlogic Meson8 and Meson8b
compatible
  clk: meson: meson8b: use a separate clock table for Meson8
  clk: meson: meson8b: add the GPU clock tree
  ARM: dts: meson8: add the Mali-450 MP6 GPU
  ARM: dts: meson8b: add the Mali-450 MP2 GPU

 .../bindings/gpu/arm,mali-utgard.txt  |   6 +
 arch/arm/boot/dts/meson8.dtsi |  58 +++
 arch/arm/boot/dts/meson8b.dtsi|  46 +++
 drivers/clk/meson/meson8b.c   | 349 +-
 drivers/clk/meson/meson8b.h   |   9 +-
 5 files changed, 461 insertions(+), 7 deletions(-)

-- 
2.19.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] clk: meson: meson8b: add the GPU clock tree

[Martin Blumenstingl]

Add the GPU clock tree on Meson8, Meson8b and Meson8m2.

The GPU clock tree on Meson8b and Meson8m2 is almost identical to the
one one GXBB:
- there's a glitch-free mux at HHI_MALI_CLK_CNTL[31]
- there are two identical parents for this mux: mali_0 and mali_1, each
  with a gate, divider and mux
- the parents of mali_0_sel and mali_1_sel are identical to GXBB except
  there's no GP0_PLL on these 32-bit SoCs

Meson8 is different because it does not have the glitch-free mux.
Instead if only has the mali_0 clock tree. The parents of mali_0_sel are
identical to the ones on Meson8b and Meson8m2.

Signed-off-by: Martin Blumenstingl 
---
 drivers/clk/meson/meson8b.c | 146 
 drivers/clk/meson/meson8b.h |   9 ++-
 2 files changed, 154 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index 0b9353d8d4fd..748552c5f6c8 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -1573,6 +1573,135 @@ static struct clk_regmap meson8b_hdmi_sys = {
},
 };
 
+/*
+ * The MALI IP is clocked by two identical clocks (mali_0 and mali_1)
+ * muxed by a glitch-free switch on Meson8b and Meson8m2. Meson8 only
+ * has mali_0 and no glitch-free mux.
+ */
+static const char * const meson8b_mali_0_1_parent_names[] = {
+   "xtal", "mpll2", "mpll1", "fclk_div7", "fclk_div4", "fclk_div3",
+   "fclk_div5"
+};
+
+static u32 meson8b_mali_0_1_mux_table[] = { 0, 2, 3, 4, 5, 6, 7 };
+
+static struct clk_regmap meson8b_mali_0_sel = {
+   .data = &(struct clk_regmap_mux_data){
+   .offset = HHI_MALI_CLK_CNTL,
+   .mask = 0x7,
+   .shift = 9,
+   .table = meson8b_mali_0_1_mux_table,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "mali_0_sel",
+   .ops = _regmap_mux_ops,
+   .parent_names = meson8b_mali_0_1_parent_names,
+   .num_parents = ARRAY_SIZE(meson8b_mali_0_1_parent_names),
+   /*
+* Don't propagate rate changes up because the only changeable
+* parents are mpll1 and mpll2 but we need those for audio and
+* RGMII (Ethernet). We don't want to change the audio or
+* Ethernet clocks when setting the GPU frequency.
+*/
+   .flags = 0,
+   },
+};
+
+static struct clk_regmap meson8b_mali_0_div = {
+   .data = &(struct clk_regmap_div_data){
+   .offset = HHI_MALI_CLK_CNTL,
+   .shift = 0,
+   .width = 7,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "mali_0_div",
+   .ops = _regmap_divider_ops,
+   .parent_names = (const char *[]){ "mali_0_sel" },
+   .num_parents = 1,
+   .flags = CLK_SET_RATE_PARENT,
+   },
+};
+
+static struct clk_regmap meson8b_mali_0 = {
+   .data = &(struct clk_regmap_gate_data){
+   .offset = HHI_MALI_CLK_CNTL,
+   .bit_idx = 8,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "mali_0",
+   .ops = _regmap_gate_ops,
+   .parent_names = (const char *[]){ "mali_0_div" },
+   .num_parents = 1,
+   .flags = CLK_SET_RATE_PARENT,
+   },
+};
+
+static struct clk_regmap meson8b_mali_1_sel = {
+   .data = &(struct clk_regmap_mux_data){
+   .offset = HHI_MALI_CLK_CNTL,
+   .mask = 0x7,
+   .shift = 25,
+   .table = meson8b_mali_0_1_mux_table,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "mali_1_sel",
+   .ops = _regmap_mux_ops,
+   .parent_names = meson8b_mali_0_1_parent_names,
+   .num_parents = ARRAY_SIZE(meson8b_mali_0_1_parent_names),
+   /*
+* Don't propagate rate changes up because the only changeable
+* parents are mpll1 and mpll2 but we need those for audio and
+* RGMII (Ethernet). We don't want to change the audio or
+* Ethernet clocks when setting the GPU frequency.
+*/
+   .flags = 0,
+   },
+};
+
+static struct clk_regmap meson8b_mali_1_div = {
+   .data = &(struct clk_regmap_div_data){
+   .offset = HHI_MALI_CLK_CNTL,
+   .shift = 16,
+   .width = 7,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "mali_1_div",
+   .ops = _regmap_divider_ops,
+   .parent_names = (const char *[]){ "mali_1_sel" },
+   .num_parents = 1,
+   .flags = CLK_SET_RATE_PARENT,
+   },
+};
+
+static struct clk_regmap meson

[PATCH 5/5] ARM: dts: meson8b: add the Mali-450 MP2 GPU

[Martin Blumenstingl]

Add the Mali-450 GPU and it's OPP table for Meson8. The GPU uses two
pixel processors in this configuration. The OPP table is taken from the
3.10 vendor kernel which uses the following table:
  FCLK_DEV5 | 1, /* 255 Mhz */
  FCLK_DEV7 | 0, /* 364 Mhz */
  FCLK_DEV3 | 1, /* 425 Mhz */
  FCLK_DEV5 | 0, /* 510 Mhz */
  FCLK_DEV4 | 0, /* 637.5 Mhz */
This describes the mux (FCLK_DEVx) and a 0-based divider in the clock
controller. "FCLK" is "fixed_pll" which is running at 2550MHz.
The "turbo" setting is described by "turbo_clock = 4" where 4 is the
index of the table above.

Signed-off-by: Martin Blumenstingl 
---
 arch/arm/boot/dts/meson8b.dtsi | 46 ++
 1 file changed, 46 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index 5d036842c355..dd498e681939 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -158,6 +158,32 @@
};
};
 
+   gpu_opp_table: gpu-opp-table {
+   compatible = "operating-points-v2";
+
+   opp-25500 {
+   opp-hz = /bits/ 64 <25500>;
+   opp-microvolt = <115>;
+   };
+   opp-36430 {
+   opp-hz = /bits/ 64 <36430>;
+   opp-microvolt = <115>;
+   };
+   opp-42500 {
+   opp-hz = /bits/ 64 <42500>;
+   opp-microvolt = <115>;
+   };
+   opp-51000 {
+   opp-hz = /bits/ 64 <51000>;
+   opp-microvolt = <115>;
+   };
+   opp-63750 {
+   opp-hz = /bits/ 64 <63750>;
+   opp-microvolt = <115>;
+   turbo-mode;
+   };
+   };
+
pmu {
compatible = "arm,cortex-a5-pmu";
interrupts = ,
@@ -185,6 +211,26 @@
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0xd000 0x20>;
+
+   mali: gpu@c {
+   compatible = "amlogic,meson8b-mali", "arm,mali-450";
+   reg = <0xc 0x4>;
+   interrupts = ,
+,
+,
+,
+,
+,
+,
+;
+   interrupt-names = "gp", "gpmmu", "pp", "pmu",
+ "pp0", "ppmmu0", "pp1", "ppmmu1";
+   resets = < RESET_MALI>;
+   clocks = < CLKID_CLK81>, < CLKID_MALI>;
+   clock-names = "bus", "core";
+   operating-points-v2 = <_opp_table>;
+   switch-delay = <0x>;
+   };
};
 }; /* end of / */
 
-- 
2.19.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel