[PATCH 0/3] Small improvements to ingenic-drm

[PATCH 0/3] Small improvements to ingenic-drm

[Paul Cercueil]

Hi,

Here are three improvements to the ingenic-drm driver.

Patch 1 adds 30-bit RGB support for the SoCs that support it. Not much
to see here.

Patch 2 is here to allow the pixel clock to be re-set when the SoC's
main PLL changes, which can happen at any time. We get a callback before
and after the PLL clock rate is changed, which allows the ingenic-drm
driver to synchronize the clock rate update with vblank. The
synchronization mechanism is implemented with a mutex. I am not sure it
is the best solution, there may be something better/simpler to do here,
but in practice it works just fine.

Patch 3 adds support for using a reserved memory area as storage space
for GEM buffers. On memory-constrained devices, it is hard to find
contiguous space even for a small 320x240 buffer, and sometimes dumb
buffer allocation from userspace fails with -ENOMEM. Using a reserved
memory area makes sure that there will always be space for our GEM
buffers (provided they fit in the memory area).

Cheers,
-Paul

Paul Cercueil (3):
  drm/ingenic: Add support for 30-bit modes
  drm/ingenic: Reset pixclock rate when parent clock rate changes
  drm/ingenic: Add support for reserved memory

 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 109 +++++++++++++++++++---
 drivers/gpu/drm/ingenic/ingenic-drm.h     |   1 +
 2 files changed, 99 insertions(+), 11 deletions(-)

-- 
2.28.0

[PATCH 1/3] drm/ingenic: Add support for 30-bit modes

[Paul Cercueil]

Starting from the JZ4760 SoC, the primary and overlay planes support
30-bit pixel modes (10 bits per color component). Add support for these
in the ingenic-drm driver.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 41 +++++++++++++++++------
 drivers/gpu/drm/ingenic/ingenic-drm.h     |  1 +
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 937d080f5d06..fb62869befdc 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -49,6 +49,8 @@ struct jz_soc_info {
 	bool needs_dev_clk;
 	bool has_osd;
 	unsigned int max_width, max_height;
+	const u32 *formats;
+	unsigned int num_formats;
 };
 
 struct ingenic_drm {
@@ -73,12 +75,6 @@ struct ingenic_drm {
 	bool no_vblank;
 };
 
-static const u32 ingenic_drm_primary_formats[] = {
-	DRM_FORMAT_XRGB1555,
-	DRM_FORMAT_RGB565,
-	DRM_FORMAT_XRGB8888,
-};
-
 static bool ingenic_drm_cached_gem_buf;
 module_param_named(cached_gem_buffers, ingenic_drm_cached_gem_buf, bool, 0400);
 MODULE_PARM_DESC(cached_gem_buffers,
@@ -411,6 +407,9 @@ void ingenic_drm_plane_config(struct device *dev,
 		case DRM_FORMAT_XRGB8888:
 			ctrl |= JZ_LCD_OSDCTRL_BPP_18_24;
 			break;
+		case DRM_FORMAT_XRGB2101010:
+			ctrl |= JZ_LCD_OSDCTRL_BPP_30;
+			break;
 		}
 
 		regmap_update_bits(priv->map, JZ_REG_LCD_OSDCTRL,
@@ -426,6 +425,9 @@ void ingenic_drm_plane_config(struct device *dev,
 		case DRM_FORMAT_XRGB8888:
 			ctrl |= JZ_LCD_CTRL_BPP_18_24;
 			break;
+		case DRM_FORMAT_XRGB2101010:
+			ctrl |= JZ_LCD_CTRL_BPP_30;
+			break;
 		}
 
 		regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
@@ -894,8 +896,8 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 	ret = drm_universal_plane_init(drm, &priv->f1, 1,
 				       &ingenic_drm_primary_plane_funcs,
-				       ingenic_drm_primary_formats,
-				       ARRAY_SIZE(ingenic_drm_primary_formats),
+				       priv->soc_info->formats,
+				       priv->soc_info->num_formats,
 				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
 	if (ret) {
 		dev_err(dev, "Failed to register plane: %i\n", ret);
@@ -919,8 +921,8 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 		ret = drm_universal_plane_init(drm, &priv->f0, 1,
 					       &ingenic_drm_primary_plane_funcs,
-					       ingenic_drm_primary_formats,
-					       ARRAY_SIZE(ingenic_drm_primary_formats),
+					       priv->soc_info->formats,
+					       priv->soc_info->num_formats,
 					       NULL, DRM_PLANE_TYPE_OVERLAY,
 					       NULL);
 		if (ret) {
@@ -1121,11 +1123,26 @@ static int ingenic_drm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const u32 jz4740_formats[] = {
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+};
+
+static const u32 jz4770_formats[] = {
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_XRGB2101010,
+};
+
 static const struct jz_soc_info jz4740_soc_info = {
 	.needs_dev_clk = true,
 	.has_osd = false,
 	.max_width = 800,
 	.max_height = 600,
+	.formats = jz4740_formats,
+	.num_formats = ARRAY_SIZE(jz4740_formats),
 };
 
 static const struct jz_soc_info jz4725b_soc_info = {
@@ -1133,6 +1150,8 @@ static const struct jz_soc_info jz4725b_soc_info = {
 	.has_osd = true,
 	.max_width = 800,
 	.max_height = 600,
+	.formats = jz4740_formats,
+	.num_formats = ARRAY_SIZE(jz4740_formats),
 };
 
 static const struct jz_soc_info jz4770_soc_info = {
@@ -1140,6 +1159,8 @@ static const struct jz_soc_info jz4770_soc_info = {
 	.has_osd = true,
 	.max_width = 1280,
 	.max_height = 720,
+	.formats = jz4770_formats,
+	.num_formats = ARRAY_SIZE(jz4770_formats),
 };
 
 static const struct of_device_id ingenic_drm_of_match[] = {
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h
index df99f0f75d39..f05e18e6b6fa 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm.h
+++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
@@ -124,6 +124,7 @@
 #define JZ_LCD_CTRL_BPP_8			0x3
 #define JZ_LCD_CTRL_BPP_15_16			0x4
 #define JZ_LCD_CTRL_BPP_18_24			0x5
+#define JZ_LCD_CTRL_BPP_30			0x7
 #define JZ_LCD_CTRL_BPP_MASK			(JZ_LCD_CTRL_RGB555 | 0x7)
 
 #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
-- 
2.28.0

Re: [PATCH 1/3] drm/ingenic: Add support for 30-bit modes

[Sam Ravnborg]

On Tue, Sep 15, 2020 at 02:38:16PM +0200, Paul Cercueil wrote:
> Starting from the JZ4760 SoC, the primary and overlay planes support
> 30-bit pixel modes (10 bits per color component). Add support for these
> in the ingenic-drm driver.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 41 +++++++++++++++++------
>  drivers/gpu/drm/ingenic/ingenic-drm.h     |  1 +
>  2 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 937d080f5d06..fb62869befdc 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -49,6 +49,8 @@ struct jz_soc_info {
>  	bool needs_dev_clk;
>  	bool has_osd;
>  	unsigned int max_width, max_height;
> +	const u32 *formats;
> +	unsigned int num_formats;
>  };
>  
>  struct ingenic_drm {
> @@ -73,12 +75,6 @@ struct ingenic_drm {
>  	bool no_vblank;
>  };
>  
> -static const u32 ingenic_drm_primary_formats[] = {
> -	DRM_FORMAT_XRGB1555,
> -	DRM_FORMAT_RGB565,
> -	DRM_FORMAT_XRGB8888,
> -};
> -
>  static bool ingenic_drm_cached_gem_buf;
>  module_param_named(cached_gem_buffers, ingenic_drm_cached_gem_buf, bool, 0400);
>  MODULE_PARM_DESC(cached_gem_buffers,
> @@ -411,6 +407,9 @@ void ingenic_drm_plane_config(struct device *dev,
>  		case DRM_FORMAT_XRGB8888:
>  			ctrl |= JZ_LCD_OSDCTRL_BPP_18_24;
>  			break;
> +		case DRM_FORMAT_XRGB2101010:
> +			ctrl |= JZ_LCD_OSDCTRL_BPP_30;
> +			break;
>  		}
>  
>  		regmap_update_bits(priv->map, JZ_REG_LCD_OSDCTRL,
> @@ -426,6 +425,9 @@ void ingenic_drm_plane_config(struct device *dev,
>  		case DRM_FORMAT_XRGB8888:
>  			ctrl |= JZ_LCD_CTRL_BPP_18_24;
>  			break;
> +		case DRM_FORMAT_XRGB2101010:
> +			ctrl |= JZ_LCD_CTRL_BPP_30;
> +			break;
>  		}
>  
>  		regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> @@ -894,8 +896,8 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  
>  	ret = drm_universal_plane_init(drm, &priv->f1, 1,
>  				       &ingenic_drm_primary_plane_funcs,
> -				       ingenic_drm_primary_formats,
> -				       ARRAY_SIZE(ingenic_drm_primary_formats),
> +				       priv->soc_info->formats,
> +				       priv->soc_info->num_formats,
>  				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>  	if (ret) {
>  		dev_err(dev, "Failed to register plane: %i\n", ret);
> @@ -919,8 +921,8 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  
>  		ret = drm_universal_plane_init(drm, &priv->f0, 1,
>  					       &ingenic_drm_primary_plane_funcs,
> -					       ingenic_drm_primary_formats,
> -					       ARRAY_SIZE(ingenic_drm_primary_formats),
> +					       priv->soc_info->formats,
> +					       priv->soc_info->num_formats,
>  					       NULL, DRM_PLANE_TYPE_OVERLAY,
>  					       NULL);
>  		if (ret) {
> @@ -1121,11 +1123,26 @@ static int ingenic_drm_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const u32 jz4740_formats[] = {
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static const u32 jz4770_formats[] = {
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XRGB2101010,
> +};
> +
>  static const struct jz_soc_info jz4740_soc_info = {
>  	.needs_dev_clk = true,
>  	.has_osd = false,
>  	.max_width = 800,
>  	.max_height = 600,
> +	.formats = jz4740_formats,
> +	.num_formats = ARRAY_SIZE(jz4740_formats),
>  };
>  
>  static const struct jz_soc_info jz4725b_soc_info = {
> @@ -1133,6 +1150,8 @@ static const struct jz_soc_info jz4725b_soc_info = {
>  	.has_osd = true,
>  	.max_width = 800,
>  	.max_height = 600,
> +	.formats = jz4740_formats,
> +	.num_formats = ARRAY_SIZE(jz4740_formats),
>  };
>  
>  static const struct jz_soc_info jz4770_soc_info = {
> @@ -1140,6 +1159,8 @@ static const struct jz_soc_info jz4770_soc_info = {
>  	.has_osd = true,
>  	.max_width = 1280,
>  	.max_height = 720,
> +	.formats = jz4770_formats,
> +	.num_formats = ARRAY_SIZE(jz4770_formats),
>  };
>  
>  static const struct of_device_id ingenic_drm_of_match[] = {
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h
> index df99f0f75d39..f05e18e6b6fa 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm.h
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
> @@ -124,6 +124,7 @@
>  #define JZ_LCD_CTRL_BPP_8			0x3
>  #define JZ_LCD_CTRL_BPP_15_16			0x4
>  #define JZ_LCD_CTRL_BPP_18_24			0x5
> +#define JZ_LCD_CTRL_BPP_30			0x7
>  #define JZ_LCD_CTRL_BPP_MASK			(JZ_LCD_CTRL_RGB555 | 0x7)
>  
>  #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
> -- 
> 2.28.0

[PATCH 2/3] drm/ingenic: Reset pixclock rate when parent clock rate changes

[Paul Cercueil]

Old Ingenic SoCs can overclock very well, up to +50% of their nominal
clock rate, whithout requiring overvolting or anything like that, just
by changing the rate of the main PLL. Unfortunately, all clocks on the
system are derived from that PLL, and when the PLL rate is updated, so
is our pixel clock.

To counter that issue, we make sure that the panel is in VBLANK before
the rate change happens, and we will then re-set the pixel clock rate
afterwards, once the PLL has been changed, to be as close as possible to
the pixel rate requested by the encoder.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 49 ++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index fb62869befdc..aa32660033d2 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -12,6 +12,7 @@
 #include <linux/dma-noncoherent.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -73,6 +74,9 @@ struct ingenic_drm {
 
 	bool panel_is_sharp;
 	bool no_vblank;
+	bool update_clk_rate;
+	struct mutex clk_mutex;
+	struct notifier_block clock_nb;
 };
 
 static bool ingenic_drm_cached_gem_buf;
@@ -115,6 +119,29 @@ static inline struct ingenic_drm *drm_crtc_get_priv(struct drm_crtc *crtc)
 	return container_of(crtc, struct ingenic_drm, crtc);
 }
 
+static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb)
+{
+	return container_of(nb, struct ingenic_drm, clock_nb);
+}
+
+static int ingenic_drm_update_pixclk(struct notifier_block *nb,
+				     unsigned long action,
+				     void *data)
+{
+	struct ingenic_drm *priv = drm_nb_get_priv(nb);
+
+	switch (action) {
+	case PRE_RATE_CHANGE:
+		mutex_lock(&priv->clk_mutex);
+		priv->update_clk_rate = true;
+		drm_crtc_wait_one_vblank(&priv->crtc);
+		return NOTIFY_OK;
+	default:
+		mutex_unlock(&priv->clk_mutex);
+		return NOTIFY_OK;
+	}
+}
+
 static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
@@ -280,8 +307,14 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
 
 	if (drm_atomic_crtc_needs_modeset(state)) {
 		ingenic_drm_crtc_update_timings(priv, &state->mode);
+		priv->update_clk_rate = true;
+	}
 
+	if (priv->update_clk_rate) {
+		mutex_lock(&priv->clk_mutex);
 		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
+		priv->update_clk_rate = false;
+		mutex_unlock(&priv->clk_mutex);
 	}
 
 	if (event) {
@@ -1046,16 +1079,28 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	if (soc_info->has_osd)
 		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
 
+	mutex_init(&priv->clk_mutex);
+	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
+
+	parent_clk = clk_get_parent(priv->pix_clk);
+	ret = clk_notifier_register(parent_clk, &priv->clock_nb);
+	if (ret) {
+		dev_err(dev, "Unable to register clock notifier\n");
+		goto err_devclk_disable;
+	}
+
 	ret = drm_dev_register(drm, 0);
 	if (ret) {
 		dev_err(dev, "Failed to register DRM driver\n");
-		goto err_devclk_disable;
+		goto err_clk_notifier_unregister;
 	}
 
 	drm_fbdev_generic_setup(drm, 32);
 
 	return 0;
 
+err_clk_notifier_unregister:
+	clk_notifier_unregister(parent_clk, &priv->clock_nb);
 err_devclk_disable:
 	if (priv->lcd_clk)
 		clk_disable_unprepare(priv->lcd_clk);
@@ -1077,7 +1122,9 @@ static int compare_of(struct device *dev, void *data)
 static void ingenic_drm_unbind(struct device *dev)
 {
 	struct ingenic_drm *priv = dev_get_drvdata(dev);
+	struct clk *parent_clk = clk_get_parent(priv->pix_clk);
 
+	clk_notifier_unregister(parent_clk, &priv->clock_nb);
 	if (priv->lcd_clk)
 		clk_disable_unprepare(priv->lcd_clk);
 	clk_disable_unprepare(priv->pix_clk);
-- 
2.28.0

Re: [PATCH 2/3] drm/ingenic: Reset pixclock rate when parent clock rate changes

[Sam Ravnborg]

Hi Paul.

On Tue, Sep 15, 2020 at 02:38:17PM +0200, Paul Cercueil wrote:
> Old Ingenic SoCs can overclock very well, up to +50% of their nominal
> clock rate, whithout requiring overvolting or anything like that, just
> by changing the rate of the main PLL. Unfortunately, all clocks on the
> system are derived from that PLL, and when the PLL rate is updated, so
> is our pixel clock.
> 
> To counter that issue, we make sure that the panel is in VBLANK before
> the rate change happens, and we will then re-set the pixel clock rate
> afterwards, once the PLL has been changed, to be as close as possible to
> the pixel rate requested by the encoder.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 49 ++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index fb62869befdc..aa32660033d2 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -12,6 +12,7 @@
>  #include <linux/dma-noncoherent.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> @@ -73,6 +74,9 @@ struct ingenic_drm {
>  
>  	bool panel_is_sharp;
>  	bool no_vblank;
> +	bool update_clk_rate;
> +	struct mutex clk_mutex;
Please add comment about what the mutex protects.
Especially since the mutex is locked and unlocked based on a
notification.

With the comment added:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

> +	struct notifier_block clock_nb;
>  };
>  
>  static bool ingenic_drm_cached_gem_buf;
> @@ -115,6 +119,29 @@ static inline struct ingenic_drm *drm_crtc_get_priv(struct drm_crtc *crtc)
>  	return container_of(crtc, struct ingenic_drm, crtc);
>  }
>  
> +static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb)
> +{
> +	return container_of(nb, struct ingenic_drm, clock_nb);
> +}
> +
> +static int ingenic_drm_update_pixclk(struct notifier_block *nb,
> +				     unsigned long action,
> +				     void *data)
> +{
> +	struct ingenic_drm *priv = drm_nb_get_priv(nb);
> +
> +	switch (action) {
> +	case PRE_RATE_CHANGE:
> +		mutex_lock(&priv->clk_mutex);
> +		priv->update_clk_rate = true;
> +		drm_crtc_wait_one_vblank(&priv->crtc);
> +		return NOTIFY_OK;
> +	default:
> +		mutex_unlock(&priv->clk_mutex);
Any risk the POST_RATE_CHANGE or ABORT_RATE_CHANGE may go missing so we
fail to unlock the mutex?
I think not but wanted to make sure you had thought about it.

> +		return NOTIFY_OK;
> +	}
> +}
> +
>  static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>  					   struct drm_crtc_state *state)
>  {
> @@ -280,8 +307,14 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>  	if (drm_atomic_crtc_needs_modeset(state)) {
>  		ingenic_drm_crtc_update_timings(priv, &state->mode);
> +		priv->update_clk_rate = true;
> +	}
>  
> +	if (priv->update_clk_rate) {
> +		mutex_lock(&priv->clk_mutex);
>  		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
> +		priv->update_clk_rate = false;
> +		mutex_unlock(&priv->clk_mutex);
>  	}
>  
>  	if (event) {
> @@ -1046,16 +1079,28 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  	if (soc_info->has_osd)
>  		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>  
> +	mutex_init(&priv->clk_mutex);
> +	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
> +
> +	parent_clk = clk_get_parent(priv->pix_clk);
> +	ret = clk_notifier_register(parent_clk, &priv->clock_nb);
> +	if (ret) {
> +		dev_err(dev, "Unable to register clock notifier\n");
> +		goto err_devclk_disable;
> +	}
> +
>  	ret = drm_dev_register(drm, 0);
>  	if (ret) {
>  		dev_err(dev, "Failed to register DRM driver\n");
> -		goto err_devclk_disable;
> +		goto err_clk_notifier_unregister;
>  	}
>  
>  	drm_fbdev_generic_setup(drm, 32);
>  
>  	return 0;
>  
> +err_clk_notifier_unregister:
> +	clk_notifier_unregister(parent_clk, &priv->clock_nb);
>  err_devclk_disable:
>  	if (priv->lcd_clk)
>  		clk_disable_unprepare(priv->lcd_clk);
> @@ -1077,7 +1122,9 @@ static int compare_of(struct device *dev, void *data)
>  static void ingenic_drm_unbind(struct device *dev)
>  {
>  	struct ingenic_drm *priv = dev_get_drvdata(dev);
> +	struct clk *parent_clk = clk_get_parent(priv->pix_clk);
>  
> +	clk_notifier_unregister(parent_clk, &priv->clock_nb);
>  	if (priv->lcd_clk)
>  		clk_disable_unprepare(priv->lcd_clk);
>  	clk_disable_unprepare(priv->pix_clk);
> -- 
> 2.28.0

Re: [PATCH 2/3] drm/ingenic: Reset pixclock rate when parent clock rate changes

[Paul Cercueil]

Hi Sam,

Le jeu. 24 sept. 2020 à 22:22, Sam Ravnborg <sam@ravnborg.org> a 
écrit :
> Hi Paul.
> 
> On Tue, Sep 15, 2020 at 02:38:17PM +0200, Paul Cercueil wrote:
>>  Old Ingenic SoCs can overclock very well, up to +50% of their 
>> nominal
>>  clock rate, whithout requiring overvolting or anything like that, 
>> just
>>  by changing the rate of the main PLL. Unfortunately, all clocks on 
>> the
>>  system are derived from that PLL, and when the PLL rate is updated, 
>> so
>>  is our pixel clock.
>> 
>>  To counter that issue, we make sure that the panel is in VBLANK 
>> before
>>  the rate change happens, and we will then re-set the pixel clock 
>> rate
>>  afterwards, once the PLL has been changed, to be as close as 
>> possible to
>>  the pixel rate requested by the encoder.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 49 
>> ++++++++++++++++++++++-
>>   1 file changed, 48 insertions(+), 1 deletion(-)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  index fb62869befdc..aa32660033d2 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  @@ -12,6 +12,7 @@
>>   #include <linux/dma-noncoherent.h>
>>   #include <linux/io.h>
>>   #include <linux/module.h>
>>  +#include <linux/mutex.h>
>>   #include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/regmap.h>
>>  @@ -73,6 +74,9 @@ struct ingenic_drm {
>> 
>>   	bool panel_is_sharp;
>>   	bool no_vblank;
>>  +	bool update_clk_rate;
>>  +	struct mutex clk_mutex;
> Please add comment about what the mutex protects.
> Especially since the mutex is locked and unlocked based on a
> notification.
> 
> With the comment added:
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> 
>>  +	struct notifier_block clock_nb;
>>   };
>> 
>>   static bool ingenic_drm_cached_gem_buf;
>>  @@ -115,6 +119,29 @@ static inline struct ingenic_drm 
>> *drm_crtc_get_priv(struct drm_crtc *crtc)
>>   	return container_of(crtc, struct ingenic_drm, crtc);
>>   }
>> 
>>  +static inline struct ingenic_drm *drm_nb_get_priv(struct 
>> notifier_block *nb)
>>  +{
>>  +	return container_of(nb, struct ingenic_drm, clock_nb);
>>  +}
>>  +
>>  +static int ingenic_drm_update_pixclk(struct notifier_block *nb,
>>  +				     unsigned long action,
>>  +				     void *data)
>>  +{
>>  +	struct ingenic_drm *priv = drm_nb_get_priv(nb);
>>  +
>>  +	switch (action) {
>>  +	case PRE_RATE_CHANGE:
>>  +		mutex_lock(&priv->clk_mutex);
>>  +		priv->update_clk_rate = true;
>>  +		drm_crtc_wait_one_vblank(&priv->crtc);
>>  +		return NOTIFY_OK;
>>  +	default:
>>  +		mutex_unlock(&priv->clk_mutex);
> Any risk the POST_RATE_CHANGE or ABORT_RATE_CHANGE may go missing so 
> we
> fail to unlock the mutex?
> I think not but wanted to make sure you had thought about it.

My assumption was that you always get POST_RATE_CHANGE or 
ABORT_RATE_CHANGE. But I am not 100% sure about that.

Michael, Stephen: is it safe to assume that I will always get notified 
with POST_RATE_CHANGE or ABORT_RATE_CHANGE, after I got notified with 
PRE_RATE_CHANGE?

Thanks,
-Paul

>>  +		return NOTIFY_OK;
>>  +	}
>>  +}
>>  +
>>   static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>>   					   struct drm_crtc_state *state)
>>   {
>>  @@ -280,8 +307,14 @@ static void 
>> ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>> 
>>   	if (drm_atomic_crtc_needs_modeset(state)) {
>>   		ingenic_drm_crtc_update_timings(priv, &state->mode);
>>  +		priv->update_clk_rate = true;
>>  +	}
>> 
>>  +	if (priv->update_clk_rate) {
>>  +		mutex_lock(&priv->clk_mutex);
>>   		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
>>  +		priv->update_clk_rate = false;
>>  +		mutex_unlock(&priv->clk_mutex);
>>   	}
>> 
>>   	if (event) {
>>  @@ -1046,16 +1079,28 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>   	if (soc_info->has_osd)
>>   		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>> 
>>  +	mutex_init(&priv->clk_mutex);
>>  +	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
>>  +
>>  +	parent_clk = clk_get_parent(priv->pix_clk);
>>  +	ret = clk_notifier_register(parent_clk, &priv->clock_nb);
>>  +	if (ret) {
>>  +		dev_err(dev, "Unable to register clock notifier\n");
>>  +		goto err_devclk_disable;
>>  +	}
>>  +
>>   	ret = drm_dev_register(drm, 0);
>>   	if (ret) {
>>   		dev_err(dev, "Failed to register DRM driver\n");
>>  -		goto err_devclk_disable;
>>  +		goto err_clk_notifier_unregister;
>>   	}
>> 
>>   	drm_fbdev_generic_setup(drm, 32);
>> 
>>   	return 0;
>> 
>>  +err_clk_notifier_unregister:
>>  +	clk_notifier_unregister(parent_clk, &priv->clock_nb);
>>   err_devclk_disable:
>>   	if (priv->lcd_clk)
>>   		clk_disable_unprepare(priv->lcd_clk);
>>  @@ -1077,7 +1122,9 @@ static int compare_of(struct device *dev, 
>> void *data)
>>   static void ingenic_drm_unbind(struct device *dev)
>>   {
>>   	struct ingenic_drm *priv = dev_get_drvdata(dev);
>>  +	struct clk *parent_clk = clk_get_parent(priv->pix_clk);
>> 
>>  +	clk_notifier_unregister(parent_clk, &priv->clock_nb);
>>   	if (priv->lcd_clk)
>>   		clk_disable_unprepare(priv->lcd_clk);
>>   	clk_disable_unprepare(priv->pix_clk);
>>  --
>>  2.28.0

Re: [PATCH 2/3] drm/ingenic: Reset pixclock rate when parent clock rate changes

[Stephen Boyd]

Quoting Paul Cercueil (2020-09-25 05:29:12)
> >>  +static int ingenic_drm_update_pixclk(struct notifier_block *nb,
> >>  +                                unsigned long action,
> >>  +                                void *data)
> >>  +{
> >>  +   struct ingenic_drm *priv = drm_nb_get_priv(nb);
> >>  +
> >>  +   switch (action) {
> >>  +   case PRE_RATE_CHANGE:
> >>  +           mutex_lock(&priv->clk_mutex);
> >>  +           priv->update_clk_rate = true;
> >>  +           drm_crtc_wait_one_vblank(&priv->crtc);
> >>  +           return NOTIFY_OK;
> >>  +   default:
> >>  +           mutex_unlock(&priv->clk_mutex);
> > Any risk the POST_RATE_CHANGE or ABORT_RATE_CHANGE may go missing so 
> > we
> > fail to unlock the mutex?
> > I think not but wanted to make sure you had thought about it.
> 
> My assumption was that you always get POST_RATE_CHANGE or 
> ABORT_RATE_CHANGE. But I am not 100% sure about that.
> 
> Michael, Stephen: is it safe to assume that I will always get notified 
> with POST_RATE_CHANGE or ABORT_RATE_CHANGE, after I got notified with 
> PRE_RATE_CHANGE?
> 

I think one or the other will happen. Of course, the notifiers are sort
of shunned so if you can avoid using notifiers entirely it would be
better.

[PATCH 3/3] drm/ingenic: Add support for reserved memory

[Paul Cercueil]

Add support for static memory reserved from Device Tree. Since we're
using GEM buffers backed by CMA, it is interesting to have an option to
specify the CMA area where the GEM buffers will be allocated.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index aa32660033d2..44b0d029095e 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
@@ -827,6 +828,11 @@ static void ingenic_drm_unbind_all(void *d)
 	component_unbind_all(priv->dev, &priv->drm);
 }
 
+static void __maybe_unused ingenic_drm_release_rmem(void *d)
+{
+	of_reserved_mem_device_release(d);
+}
+
 static int ingenic_drm_bind(struct device *dev, bool has_components)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -848,6 +854,19 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 		return -EINVAL;
 	}
 
+	if (IS_ENABLED(CONFIG_OF_RESERVED_MEM)) {
+		ret = of_reserved_mem_device_init(dev);
+
+		if (ret && ret != -ENODEV)
+			return dev_err_probe(dev, ret, "Failed to get reserved memory\n");
+
+		if (ret != -ENODEV) {
+			ret = devm_add_action_or_reset(dev, ingenic_drm_release_rmem, dev);
+			if (ret)
+				return ret;
+		}
+	}
+
 	priv = devm_drm_dev_alloc(dev, &ingenic_drm_driver_data,
 				  struct ingenic_drm, drm);
 	if (IS_ERR(priv))
-- 
2.28.0

Re: [PATCH 3/3] drm/ingenic: Add support for reserved memory

[Sam Ravnborg]

On Tue, Sep 15, 2020 at 02:38:18PM +0200, Paul Cercueil wrote:
> Add support for static memory reserved from Device Tree. Since we're
> using GEM buffers backed by CMA, it is interesting to have an option to
> specify the CMA area where the GEM buffers will be allocated.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index aa32660033d2..44b0d029095e 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -14,6 +14,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  
> @@ -827,6 +828,11 @@ static void ingenic_drm_unbind_all(void *d)
>  	component_unbind_all(priv->dev, &priv->drm);
>  }
>  
> +static void __maybe_unused ingenic_drm_release_rmem(void *d)
> +{
> +	of_reserved_mem_device_release(d);
> +}
> +
>  static int ingenic_drm_bind(struct device *dev, bool has_components)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -848,6 +854,19 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  		return -EINVAL;
>  	}
>  
> +	if (IS_ENABLED(CONFIG_OF_RESERVED_MEM)) {
> +		ret = of_reserved_mem_device_init(dev);
> +
> +		if (ret && ret != -ENODEV)
> +			return dev_err_probe(dev, ret, "Failed to get reserved memory\n");
> +
> +		if (ret != -ENODEV) {
> +			ret = devm_add_action_or_reset(dev, ingenic_drm_release_rmem, dev);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
>  	priv = devm_drm_dev_alloc(dev, &ingenic_drm_driver_data,
>  				  struct ingenic_drm, drm);
>  	if (IS_ERR(priv))
> -- 
> 2.28.0