drm/meson: add mode selection limits against specific SoC revisions
diff mbox series

Message ID 20200421134410.30603-1-narmstrong@baylibre.com
State Superseded
Delegated to: Neil Armstrong
Headers show
Series
  • drm/meson: add mode selection limits against specific SoC revisions
Related show

Commit Message

Neil Armstrong April 21, 2020, 1:44 p.m. UTC
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.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/meson/meson_drv.c     | 29 ++++++++++++++++++++++++++-
 drivers/gpu/drm/meson/meson_drv.h     |  6 ++++++
 drivers/gpu/drm/meson/meson_dw_hdmi.c |  7 +++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

Comments

Daniel Vetter April 21, 2020, 1:59 p.m. UTC | #1
On Tue, Apr 21, 2020 at 03:44:10PM +0200, 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.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Just a drive-by, but the code organization between the dw-hdmi bridge and
the driver looks pretty terribly and really leaky. Can't we do better?
Either by fixing the dw-hdmi bridge abstraction to actually abstract
something, or by givin up the dw-hdmi is a bridge and convert it to some
helper to implement a drm_encoder. Current status just doesn't make too
much sense to me.
-Daniel

> ---
>  drivers/gpu/drm/meson/meson_drv.c     | 29 ++++++++++++++++++++++++++-
>  drivers/gpu/drm/meson/meson_drv.h     |  6 ++++++
>  drivers/gpu/drm/meson/meson_dw_hdmi.c |  7 +++++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
> index 6f29fab79952..621f6de0f076 100644
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -11,6 +11,7 @@
>  #include <linux/component.h>
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
> +#include <linux/sys_soc.h>
>  #include <linux/platform_device.h>
>  #include <linux/soc/amlogic/meson-canvas.h>
>  
> @@ -183,6 +184,24 @@ static void meson_remove_framebuffers(void)
>  	kfree(ap);
>  }
>  
> +struct meson_drm_soc_attr {
> +	struct meson_drm_soc_limits limits;
> +	const struct soc_device_attribute *attrs;
> +};
> +
> +static const struct meson_drm_soc_attr meson_drm_soc_attrs[] = {
> +	/* S805X/S805Y HDMI PLL won't lock for HDMI PHY freq > 1,65GHz */
> +	{
> +		.limits = {
> +			.max_hdmi_phy_freq = 1650000,
> +		},
> +		.attrs = (const struct soc_device_attribute []) {
> +			{ .soc_id = "GXL (S805*)", },
> +			{ /* sentinel */ },
> +		}
> +	},
> +};
> +
>  static int meson_drv_bind_master(struct device *dev, bool has_components)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -191,7 +210,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
>  	struct drm_device *drm;
>  	struct resource *res;
>  	void __iomem *regs;
> -	int ret;
> +	int ret, i;
>  
>  	/* Checks if an output connector is available */
>  	if (!meson_vpu_has_available_connectors(dev)) {
> @@ -281,6 +300,14 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
>  	if (ret)
>  		goto free_drm;
>  
> +	/* Assign limits per soc revision/package */
> +	for (i = 0 ; i < ARRAY_SIZE(meson_drm_soc_attrs) ; ++i) {
> +		if (soc_device_match(meson_drm_soc_attrs[i].attrs)) {
> +			priv->limits = &meson_drm_soc_attrs[i].limits;
> +			break;
> +		}
> +	}
> +
>  	/* Remove early framebuffers (ie. simplefb) */
>  	meson_remove_framebuffers();
>  
> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
> index 04fdf3826643..5b23704a80d6 100644
> --- a/drivers/gpu/drm/meson/meson_drv.h
> +++ b/drivers/gpu/drm/meson/meson_drv.h
> @@ -30,6 +30,10 @@ struct meson_drm_match_data {
>  	struct meson_afbcd_ops *afbcd_ops;
>  };
>  
> +struct meson_drm_soc_limits {
> +	unsigned int max_hdmi_phy_freq;
> +};
> +
>  struct meson_drm {
>  	struct device *dev;
>  	enum vpu_compatible compat;
> @@ -48,6 +52,8 @@ struct meson_drm {
>  	struct drm_plane *primary_plane;
>  	struct drm_plane *overlay_plane;
>  
> +	const struct meson_drm_soc_limits *limits;
> +
>  	/* Components Data */
>  	struct {
>  		bool osd1_enabled;
> 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;
> +	}
> +
>  	return meson_vclk_vic_supported_freq(phy_freq, vclk_freq);
>  }
>  
> -- 
> 2.22.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Neil Armstrong April 21, 2020, 3:38 p.m. UTC | #2
On 21/04/2020 15:59, Daniel Vetter wrote:
> On Tue, Apr 21, 2020 at 03:44:10PM +0200, 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.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> Just a drive-by, but the code organization between the dw-hdmi bridge and
> the driver looks pretty terribly and really leaky. Can't we do better?
> Either by fixing the dw-hdmi bridge abstraction to actually abstract
> something, or by givin up the dw-hdmi is a bridge and convert it to some
> helper to implement a drm_encoder. Current status just doesn't make too
> much sense to me.


Yep, the encoder part should be moved out of the dw-hdmi glue driver for sure.
I'll a draft something, but it won't really affect this patch.

Neil

> -Daniel
> 
>> ---
>>  drivers/gpu/drm/meson/meson_drv.c     | 29 ++++++++++++++++++++++++++-
>>  drivers/gpu/drm/meson/meson_drv.h     |  6 ++++++
>>  drivers/gpu/drm/meson/meson_dw_hdmi.c |  7 +++++++
>>  3 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
>> index 6f29fab79952..621f6de0f076 100644
>> --- a/drivers/gpu/drm/meson/meson_drv.c
>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/component.h>
>>  #include <linux/module.h>
>>  #include <linux/of_graph.h>
>> +#include <linux/sys_soc.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/soc/amlogic/meson-canvas.h>
>>  
>> @@ -183,6 +184,24 @@ static void meson_remove_framebuffers(void)
>>  	kfree(ap);
>>  }
>>  
>> +struct meson_drm_soc_attr {
>> +	struct meson_drm_soc_limits limits;
>> +	const struct soc_device_attribute *attrs;
>> +};
>> +
>> +static const struct meson_drm_soc_attr meson_drm_soc_attrs[] = {
>> +	/* S805X/S805Y HDMI PLL won't lock for HDMI PHY freq > 1,65GHz */
>> +	{
>> +		.limits = {
>> +			.max_hdmi_phy_freq = 1650000,
>> +		},
>> +		.attrs = (const struct soc_device_attribute []) {
>> +			{ .soc_id = "GXL (S805*)", },
>> +			{ /* sentinel */ },
>> +		}
>> +	},
>> +};
>> +
>>  static int meson_drv_bind_master(struct device *dev, bool has_components)
>>  {
>>  	struct platform_device *pdev = to_platform_device(dev);
>> @@ -191,7 +210,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
>>  	struct drm_device *drm;
>>  	struct resource *res;
>>  	void __iomem *regs;
>> -	int ret;
>> +	int ret, i;
>>  
>>  	/* Checks if an output connector is available */
>>  	if (!meson_vpu_has_available_connectors(dev)) {
>> @@ -281,6 +300,14 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
>>  	if (ret)
>>  		goto free_drm;
>>  
>> +	/* Assign limits per soc revision/package */
>> +	for (i = 0 ; i < ARRAY_SIZE(meson_drm_soc_attrs) ; ++i) {
>> +		if (soc_device_match(meson_drm_soc_attrs[i].attrs)) {
>> +			priv->limits = &meson_drm_soc_attrs[i].limits;
>> +			break;
>> +		}
>> +	}
>> +
>>  	/* Remove early framebuffers (ie. simplefb) */
>>  	meson_remove_framebuffers();
>>  
>> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
>> index 04fdf3826643..5b23704a80d6 100644
>> --- a/drivers/gpu/drm/meson/meson_drv.h
>> +++ b/drivers/gpu/drm/meson/meson_drv.h
>> @@ -30,6 +30,10 @@ struct meson_drm_match_data {
>>  	struct meson_afbcd_ops *afbcd_ops;
>>  };
>>  
>> +struct meson_drm_soc_limits {
>> +	unsigned int max_hdmi_phy_freq;
>> +};
>> +
>>  struct meson_drm {
>>  	struct device *dev;
>>  	enum vpu_compatible compat;
>> @@ -48,6 +52,8 @@ struct meson_drm {
>>  	struct drm_plane *primary_plane;
>>  	struct drm_plane *overlay_plane;
>>  
>> +	const struct meson_drm_soc_limits *limits;
>> +
>>  	/* Components Data */
>>  	struct {
>>  		bool osd1_enabled;
>> 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;
>> +	}
>> +
>>  	return meson_vclk_vic_supported_freq(phy_freq, vclk_freq);
>>  }
>>  
>> -- 
>> 2.22.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Martin Blumenstingl April 22, 2020, 9:12 p.m. UTC | #3
Hi Neil,

On Tue, Apr 21, 2020 at 3:44 PM Neil Armstrong <narmstrong@baylibre.com> 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
Neil Armstrong April 28, 2020, 9:11 a.m. UTC | #4
Hi,

On 22/04/2020 23:12, Martin Blumenstingl wrote:
> Hi Neil,
> 
> On Tue, Apr 21, 2020 at 3:44 PM Neil Armstrong <narmstrong@baylibre.com> 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?

It would need to add priv to meson_vclk_vic_supported_freq(), but indeed,
would be cleaner.

And the meson_vclk_dmt_supported_freq() would also need this test aswell.

I'll resend with these fixed.

Neil

> 
> 
> Thank you!
> Martin
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index 6f29fab79952..621f6de0f076 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -11,6 +11,7 @@ 
 #include <linux/component.h>
 #include <linux/module.h>
 #include <linux/of_graph.h>
+#include <linux/sys_soc.h>
 #include <linux/platform_device.h>
 #include <linux/soc/amlogic/meson-canvas.h>
 
@@ -183,6 +184,24 @@  static void meson_remove_framebuffers(void)
 	kfree(ap);
 }
 
+struct meson_drm_soc_attr {
+	struct meson_drm_soc_limits limits;
+	const struct soc_device_attribute *attrs;
+};
+
+static const struct meson_drm_soc_attr meson_drm_soc_attrs[] = {
+	/* S805X/S805Y HDMI PLL won't lock for HDMI PHY freq > 1,65GHz */
+	{
+		.limits = {
+			.max_hdmi_phy_freq = 1650000,
+		},
+		.attrs = (const struct soc_device_attribute []) {
+			{ .soc_id = "GXL (S805*)", },
+			{ /* sentinel */ },
+		}
+	},
+};
+
 static int meson_drv_bind_master(struct device *dev, bool has_components)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -191,7 +210,7 @@  static int meson_drv_bind_master(struct device *dev, bool has_components)
 	struct drm_device *drm;
 	struct resource *res;
 	void __iomem *regs;
-	int ret;
+	int ret, i;
 
 	/* Checks if an output connector is available */
 	if (!meson_vpu_has_available_connectors(dev)) {
@@ -281,6 +300,14 @@  static int meson_drv_bind_master(struct device *dev, bool has_components)
 	if (ret)
 		goto free_drm;
 
+	/* Assign limits per soc revision/package */
+	for (i = 0 ; i < ARRAY_SIZE(meson_drm_soc_attrs) ; ++i) {
+		if (soc_device_match(meson_drm_soc_attrs[i].attrs)) {
+			priv->limits = &meson_drm_soc_attrs[i].limits;
+			break;
+		}
+	}
+
 	/* Remove early framebuffers (ie. simplefb) */
 	meson_remove_framebuffers();
 
diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
index 04fdf3826643..5b23704a80d6 100644
--- a/drivers/gpu/drm/meson/meson_drv.h
+++ b/drivers/gpu/drm/meson/meson_drv.h
@@ -30,6 +30,10 @@  struct meson_drm_match_data {
 	struct meson_afbcd_ops *afbcd_ops;
 };
 
+struct meson_drm_soc_limits {
+	unsigned int max_hdmi_phy_freq;
+};
+
 struct meson_drm {
 	struct device *dev;
 	enum vpu_compatible compat;
@@ -48,6 +52,8 @@  struct meson_drm {
 	struct drm_plane *primary_plane;
 	struct drm_plane *overlay_plane;
 
+	const struct meson_drm_soc_limits *limits;
+
 	/* Components Data */
 	struct {
 		bool osd1_enabled;
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;
+	}
+
 	return meson_vclk_vic_supported_freq(phy_freq, vclk_freq);
 }