[PATCH 0/4] drm/probe-helper, modes: Helpers for single-mode displays

[PATCH 0/4] drm/probe-helper, modes: Helpers for single-mode displays

[Thomas Zimmermann]

This patchset moves code from simpledrm to probe and display-mode
helpers. The new functions will be useful for the upcomming driver
for PowerPC displays. [1] Where possible, existing drivers are
being converted to use them.

[1] https://patchwork.freedesktop.org/series/106538/

Thomas Zimmermann (4):
  drm/probe-helper: Add drm_connector_helper_get_modes_static()
  drm/probe-helper: Add drm_crtc_helper_mode_valid_static()
  drm/modes: Add initializer macro DRM_MODE_INIT()
  drm/format-helper: Add drm_fb_build_fourcc_list() helper

 drivers/gpu/drm/drm_format_helper.c          | 94 +++++++++++++++++++
 drivers/gpu/drm/drm_mipi_dbi.c               | 38 ++++----
 drivers/gpu/drm/drm_probe_helper.c           | 65 +++++++++++++
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c |  1 +
 drivers/gpu/drm/tiny/hx8357d.c               |  1 +
 drivers/gpu/drm/tiny/ili9163.c               |  1 +
 drivers/gpu/drm/tiny/ili9341.c               |  1 +
 drivers/gpu/drm/tiny/ili9486.c               |  1 +
 drivers/gpu/drm/tiny/mi0283qt.c              |  1 +
 drivers/gpu/drm/tiny/panel-mipi-dbi.c        |  1 +
 drivers/gpu/drm/tiny/repaper.c               | 26 +++---
 drivers/gpu/drm/tiny/simpledrm.c             | 96 +++-----------------
 drivers/gpu/drm/tiny/st7735r.c               |  1 +
 include/drm/drm_format_helper.h              | 11 ++-
 include/drm/drm_mipi_dbi.h                   |  2 +
 include/drm/drm_modes.h                      | 35 ++++++-
 include/drm/drm_probe_helper.h               |  9 +-
 17 files changed, 262 insertions(+), 122 deletions(-)

-- 
2.37.1

[PATCH 1/4] drm/probe-helper: Add drm_connector_helper_get_modes_static()

[Thomas Zimmermann]

Add drm_connector_helper_get_modes_static(), which duplicates a single
display mode for a connector. Convert drivers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_mipi_dbi.c     | 20 +--------------
 drivers/gpu/drm/drm_probe_helper.c | 40 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/tiny/repaper.c     | 16 +-----------
 drivers/gpu/drm/tiny/simpledrm.c   | 18 +-------------
 include/drm/drm_probe_helper.h     |  3 +++
 5 files changed, 46 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 84abc3920b57..b67ec9a5cda9 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -415,26 +415,8 @@ EXPORT_SYMBOL(mipi_dbi_pipe_disable);
 static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
-	struct drm_display_mode *mode;
 
-	mode = drm_mode_duplicate(connector->dev, &dbidev->mode);
-	if (!mode) {
-		DRM_ERROR("Failed to duplicate mode\n");
-		return 0;
-	}
-
-	if (mode->name[0] == '\0')
-		drm_mode_set_name(mode);
-
-	mode->type |= DRM_MODE_TYPE_PREFERRED;
-	drm_mode_probed_add(connector, mode);
-
-	if (mode->width_mm) {
-		connector->display_info.width_mm = mode->width_mm;
-		connector->display_info.height_mm = mode->height_mm;
-	}
-
-	return 1;
+	return drm_connector_helper_get_modes_static(connector, &dbidev->mode);
 }
 
 static const struct drm_connector_helper_funcs mipi_dbi_connector_hfuncs = {
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index bb427c5a4f1f..809187377e4e 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1050,6 +1050,46 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
 
+/**
+ * drm_connector_helper_get_modes_static - Duplicates a display mode for a connector
+ * @connector: the connector
+ * @hw_mode: the display hardware's mode
+ *
+ * This function duplicates a display modes for a connector. Drivers for hardware
+ * that only supports a single mode can use this function in there connector's
+ * get_modes helper.
+ *
+ * Returns:
+ * The number of created modes.
+ */
+int drm_connector_helper_get_modes_static(struct drm_connector *connector,
+					  const struct drm_display_mode *hw_mode)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(dev, hw_mode);
+	if (!mode) {
+		drm_err(dev, "Failed to duplicate mode " DRM_MODE_FMT "\n",
+			DRM_MODE_ARG(hw_mode));
+		return 0;
+	}
+
+	if (mode->name[0] == '\0')
+		drm_mode_set_name(mode);
+
+	mode->type |= DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	if (mode->width_mm)
+		connector->display_info.width_mm = mode->width_mm;
+	if (mode->height_mm)
+		connector->display_info.height_mm = mode->height_mm;
+
+	return 1;
+}
+EXPORT_SYMBOL(drm_connector_helper_get_modes_static);
+
 /**
  * drm_connector_helper_get_modes - Read EDID and update connector.
  * @connector: The connector
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index c4c1be3ac0b8..855968fd46af 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -839,22 +839,8 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
 static int repaper_connector_get_modes(struct drm_connector *connector)
 {
 	struct repaper_epd *epd = drm_to_epd(connector->dev);
-	struct drm_display_mode *mode;
 
-	mode = drm_mode_duplicate(connector->dev, epd->mode);
-	if (!mode) {
-		DRM_ERROR("Failed to duplicate mode\n");
-		return 0;
-	}
-
-	drm_mode_set_name(mode);
-	mode->type |= DRM_MODE_TYPE_PREFERRED;
-	drm_mode_probed_add(connector, mode);
-
-	connector->display_info.width_mm = mode->width_mm;
-	connector->display_info.height_mm = mode->height_mm;
-
-	return 1;
+	return drm_connector_helper_get_modes_static(connector, epd->mode);
 }
 
 static const struct drm_connector_helper_funcs repaper_connector_hfuncs = {
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index a81f91814595..2d5b56c4a77d 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -620,24 +620,8 @@ static const struct drm_encoder_funcs simpledrm_encoder_funcs = {
 static int simpledrm_connector_helper_get_modes(struct drm_connector *connector)
 {
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(connector->dev);
-	struct drm_display_mode *mode;
 
-	mode = drm_mode_duplicate(connector->dev, &sdev->mode);
-	if (!mode)
-		return 0;
-
-	if (mode->name[0] == '\0')
-		drm_mode_set_name(mode);
-
-	mode->type |= DRM_MODE_TYPE_PREFERRED;
-	drm_mode_probed_add(connector, mode);
-
-	if (mode->width_mm)
-		connector->display_info.width_mm = mode->width_mm;
-	if (mode->height_mm)
-		connector->display_info.height_mm = mode->height_mm;
-
-	return 1;
+	return drm_connector_helper_get_modes_static(connector, &sdev->mode);
 }
 
 static const struct drm_connector_helper_funcs simpledrm_connector_helper_funcs = {
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index 8075e02aa865..5a883ee9fc32 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -7,6 +7,7 @@
 
 struct drm_connector;
 struct drm_device;
+struct drm_display_mode;
 struct drm_modeset_acquire_ctx;
 
 int drm_helper_probe_single_connector_modes(struct drm_connector
@@ -27,6 +28,8 @@ void drm_kms_helper_poll_enable(struct drm_device *dev);
 bool drm_kms_helper_is_poll_worker(void);
 
 int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);
+int drm_connector_helper_get_modes_static(struct drm_connector *connector,
+					  const struct drm_display_mode *hw_mode);
 int drm_connector_helper_get_modes(struct drm_connector *connector);
 
 #endif
-- 
2.37.1

Re: [PATCH 1/4] drm/probe-helper: Add drm_connector_helper_get_modes_static()

[Sam Ravnborg]

Hi Thomas,

On Wed, Aug 10, 2022 at 01:20:50PM +0200, Thomas Zimmermann wrote:
> Add drm_connector_helper_get_modes_static(), which duplicates a single
> display mode for a connector. Convert drivers.

I like this helper!
There are a lot of panels that can benefit from the same helper.

The current users that are replaced do not do so, but some panels also
set:

        connector->display_info.bpc = 8;
        connector->display_info.bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
        drm_display_info_set_bus_formats(&connector->display_info, &bus_format, 1);

I looked at a similar helper for panels once, but for panels I stopped
there as we then had to pass bpc, bus_format and bus_mode as arguments.
Maybe that is over-engineering things.

Someone that knows when we must pass bpc, bus_mode, bus_flags and when
not can maybe help here.

The current helper is fine as is, but I wonder if we can cover more
use cases with an extra helper.

It would also be nice to convert the panels that can use the new helper,
but that should be in a new patch and can be done later.

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

but I have a few comments in the following.

> ---
>  drivers/gpu/drm/drm_mipi_dbi.c     | 20 +--------------
>  drivers/gpu/drm/drm_probe_helper.c | 40 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/tiny/repaper.c     | 16 +-----------
>  drivers/gpu/drm/tiny/simpledrm.c   | 18 +-------------
>  include/drm/drm_probe_helper.h     |  3 +++
>  5 files changed, 46 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index 84abc3920b57..b67ec9a5cda9 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -415,26 +415,8 @@ EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>  static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
> -	struct drm_display_mode *mode;
>  
> -	mode = drm_mode_duplicate(connector->dev, &dbidev->mode);
> -	if (!mode) {
> -		DRM_ERROR("Failed to duplicate mode\n");
> -		return 0;
> -	}
> -
> -	if (mode->name[0] == '\0')
> -		drm_mode_set_name(mode);
> -
> -	mode->type |= DRM_MODE_TYPE_PREFERRED;
> -	drm_mode_probed_add(connector, mode);
> -
> -	if (mode->width_mm) {
> -		connector->display_info.width_mm = mode->width_mm;
> -		connector->display_info.height_mm = mode->height_mm;
> -	}
> -
> -	return 1;
> +	return drm_connector_helper_get_modes_static(connector, &dbidev->mode);
>  }
>  
>  static const struct drm_connector_helper_funcs mipi_dbi_connector_hfuncs = {
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index bb427c5a4f1f..809187377e4e 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1050,6 +1050,46 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
>  
> +/**
> + * drm_connector_helper_get_modes_static - Duplicates a display mode for a connector
The hw mode is duplicated so maybe name it ..._modes_hw()?

But I dunno.. Naming is hard.

> + * @connector: the connector
> + * @hw_mode: the display hardware's mode
> + *
> + * This function duplicates a display modes for a connector. Drivers for hardware
> + * that only supports a single mode can use this function in there connector's
                                                                their? 
> + * get_modes helper.
> + *
> + * Returns:
> + * The number of created modes.
> + */
> +int drm_connector_helper_get_modes_static(struct drm_connector *connector,
> +					  const struct drm_display_mode *hw_mode)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(dev, hw_mode);
> +	if (!mode) {
> +		drm_err(dev, "Failed to duplicate mode " DRM_MODE_FMT "\n",
> +			DRM_MODE_ARG(hw_mode));
> +		return 0;
> +	}
> +
> +	if (mode->name[0] == '\0')
> +		drm_mode_set_name(mode);
Hmm, so we rely that it was set to something relevant before. I guess
that's OK.
> +
> +	mode->type |= DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_probed_add(connector, mode);
> +
> +	if (mode->width_mm)
> +		connector->display_info.width_mm = mode->width_mm;
> +	if (mode->height_mm)
> +		connector->display_info.height_mm = mode->height_mm;
> +
> +	return 1;
> +}
> +EXPORT_SYMBOL(drm_connector_helper_get_modes_static);
> +
>  /**
>   * drm_connector_helper_get_modes - Read EDID and update connector.
>   * @connector: The connector
> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> index c4c1be3ac0b8..855968fd46af 100644
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c
> @@ -839,22 +839,8 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
>  static int repaper_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct repaper_epd *epd = drm_to_epd(connector->dev);
> -	struct drm_display_mode *mode;
>  
> -	mode = drm_mode_duplicate(connector->dev, epd->mode);
> -	if (!mode) {
> -		DRM_ERROR("Failed to duplicate mode\n");
> -		return 0;
> -	}
> -
> -	drm_mode_set_name(mode);
> -	mode->type |= DRM_MODE_TYPE_PREFERRED;
> -	drm_mode_probed_add(connector, mode);
> -
> -	connector->display_info.width_mm = mode->width_mm;
> -	connector->display_info.height_mm = mode->height_mm;
> -
> -	return 1;
> +	return drm_connector_helper_get_modes_static(connector, epd->mode);
>  }
>  
>  static const struct drm_connector_helper_funcs repaper_connector_hfuncs = {
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index a81f91814595..2d5b56c4a77d 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -620,24 +620,8 @@ static const struct drm_encoder_funcs simpledrm_encoder_funcs = {
>  static int simpledrm_connector_helper_get_modes(struct drm_connector *connector)
>  {
>  	struct simpledrm_device *sdev = simpledrm_device_of_dev(connector->dev);
> -	struct drm_display_mode *mode;
>  
> -	mode = drm_mode_duplicate(connector->dev, &sdev->mode);
> -	if (!mode)
> -		return 0;
> -
> -	if (mode->name[0] == '\0')
> -		drm_mode_set_name(mode);
> -
> -	mode->type |= DRM_MODE_TYPE_PREFERRED;
> -	drm_mode_probed_add(connector, mode);
> -
> -	if (mode->width_mm)
> -		connector->display_info.width_mm = mode->width_mm;
> -	if (mode->height_mm)
> -		connector->display_info.height_mm = mode->height_mm;
> -
> -	return 1;
> +	return drm_connector_helper_get_modes_static(connector, &sdev->mode);
>  }
>  
>  static const struct drm_connector_helper_funcs simpledrm_connector_helper_funcs = {
> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
> index 8075e02aa865..5a883ee9fc32 100644
> --- a/include/drm/drm_probe_helper.h
> +++ b/include/drm/drm_probe_helper.h
> @@ -7,6 +7,7 @@
>  
>  struct drm_connector;
>  struct drm_device;
> +struct drm_display_mode;
>  struct drm_modeset_acquire_ctx;
>  
>  int drm_helper_probe_single_connector_modes(struct drm_connector
> @@ -27,6 +28,8 @@ void drm_kms_helper_poll_enable(struct drm_device *dev);
>  bool drm_kms_helper_is_poll_worker(void);
>  
>  int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);
> +int drm_connector_helper_get_modes_static(struct drm_connector *connector,
> +					  const struct drm_display_mode *hw_mode);
>  int drm_connector_helper_get_modes(struct drm_connector *connector);
>  
>  #endif
> -- 
> 2.37.1

Re: [PATCH 1/4] drm/probe-helper: Add drm_connector_helper_get_modes_static()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 9145 bytes --]

Hi Sam,

thanks for the reviews.

Am 10.08.22 um 21:21 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Wed, Aug 10, 2022 at 01:20:50PM +0200, Thomas Zimmermann wrote:
>> Add drm_connector_helper_get_modes_static(), which duplicates a single
>> display mode for a connector. Convert drivers.
> 
> I like this helper!
> There are a lot of panels that can benefit from the same helper.
> 
> The current users that are replaced do not do so, but some panels also
> set:
> 
>          connector->display_info.bpc = 8;
>          connector->display_info.bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
>          drm_display_info_set_bus_formats(&connector->display_info, &bus_format, 1);
> 
> I looked at a similar helper for panels once, but for panels I stopped
> there as we then had to pass bpc, bus_format and bus_mode as arguments.
> Maybe that is over-engineering things.
> 
> Someone that knows when we must pass bpc, bus_mode, bus_flags and when
> not can maybe help here.
> 
> The current helper is fine as is, but I wonder if we can cover more
> use cases with an extra helper.
> 
> It would also be nice to convert the panels that can use the new helper,
> but that should be in a new patch and can be done later.

Yeah, I saw other drivers that do something very similar to what the new 
helper does, but with a 'twist.' So I hesitated to change them. Maybe 
some rearchitecturing of the drivers or helpers can change that.

> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> but I have a few comments in the following.
> 
>> ---
>>   drivers/gpu/drm/drm_mipi_dbi.c     | 20 +--------------
>>   drivers/gpu/drm/drm_probe_helper.c | 40 ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tiny/repaper.c     | 16 +-----------
>>   drivers/gpu/drm/tiny/simpledrm.c   | 18 +-------------
>>   include/drm/drm_probe_helper.h     |  3 +++
>>   5 files changed, 46 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
>> index 84abc3920b57..b67ec9a5cda9 100644
>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>> @@ -415,26 +415,8 @@ EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>>   static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
>>   {
>>   	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
>> -	struct drm_display_mode *mode;
>>   
>> -	mode = drm_mode_duplicate(connector->dev, &dbidev->mode);
>> -	if (!mode) {
>> -		DRM_ERROR("Failed to duplicate mode\n");
>> -		return 0;
>> -	}
>> -
>> -	if (mode->name[0] == '\0')
>> -		drm_mode_set_name(mode);
>> -
>> -	mode->type |= DRM_MODE_TYPE_PREFERRED;
>> -	drm_mode_probed_add(connector, mode);
>> -
>> -	if (mode->width_mm) {
>> -		connector->display_info.width_mm = mode->width_mm;
>> -		connector->display_info.height_mm = mode->height_mm;
>> -	}
>> -
>> -	return 1;
>> +	return drm_connector_helper_get_modes_static(connector, &dbidev->mode);
>>   }
>>   
>>   static const struct drm_connector_helper_funcs mipi_dbi_connector_hfuncs = {
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index bb427c5a4f1f..809187377e4e 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -1050,6 +1050,46 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
>>   }
>>   EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
>>   
>> +/**
>> + * drm_connector_helper_get_modes_static - Duplicates a display mode for a connector
> The hw mode is duplicated so maybe name it ..._modes_hw()?
> 
> But I dunno.. Naming is hard.

I considered 'hw', 'static' and 'fixed', all of wich I didn't really 
like. I'll change all names to 'fixed'. It seems closest to what the 
helper is for (think of 'fixed resolution') and also close to DRM's 
common terminology.

> 
>> + * @connector: the connector
>> + * @hw_mode: the display hardware's mode
>> + *
>> + * This function duplicates a display modes for a connector. Drivers for hardware
>> + * that only supports a single mode can use this function in there connector's
>                                                                  their?

Oh, indeed.

>> + * get_modes helper.
>> + *
>> + * Returns:
>> + * The number of created modes.
>> + */
>> +int drm_connector_helper_get_modes_static(struct drm_connector *connector,
>> +					  const struct drm_display_mode *hw_mode)
>> +{
>> +	struct drm_device *dev = connector->dev;
>> +	struct drm_display_mode *mode;
>> +
>> +	mode = drm_mode_duplicate(dev, hw_mode);
>> +	if (!mode) {
>> +		drm_err(dev, "Failed to duplicate mode " DRM_MODE_FMT "\n",
>> +			DRM_MODE_ARG(hw_mode));
>> +		return 0;
>> +	}
>> +
>> +	if (mode->name[0] == '\0')
>> +		drm_mode_set_name(mode);
> Hmm, so we rely that it was set to something relevant before. I guess
> that's OK.

Name is an array within the structure. According to DRM's rules, it will 
be initialized to zero. If the caller set a name in hw_mode, we keep 
that instead, otherwise create a standard name with drm_mode_set_name(). 
That seems appropriate to me.

Best regards
Thomas

>> +
>> +	mode->type |= DRM_MODE_TYPE_PREFERRED;
>> +	drm_mode_probed_add(connector, mode);
>> +
>> +	if (mode->width_mm)
>> +		connector->display_info.width_mm = mode->width_mm;
>> +	if (mode->height_mm)
>> +		connector->display_info.height_mm = mode->height_mm;
>> +
>> +	return 1;
>> +}
>> +EXPORT_SYMBOL(drm_connector_helper_get_modes_static);
>> +
>>   /**
>>    * drm_connector_helper_get_modes - Read EDID and update connector.
>>    * @connector: The connector
>> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
>> index c4c1be3ac0b8..855968fd46af 100644
>> --- a/drivers/gpu/drm/tiny/repaper.c
>> +++ b/drivers/gpu/drm/tiny/repaper.c
>> @@ -839,22 +839,8 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
>>   static int repaper_connector_get_modes(struct drm_connector *connector)
>>   {
>>   	struct repaper_epd *epd = drm_to_epd(connector->dev);
>> -	struct drm_display_mode *mode;
>>   
>> -	mode = drm_mode_duplicate(connector->dev, epd->mode);
>> -	if (!mode) {
>> -		DRM_ERROR("Failed to duplicate mode\n");
>> -		return 0;
>> -	}
>> -
>> -	drm_mode_set_name(mode);
>> -	mode->type |= DRM_MODE_TYPE_PREFERRED;
>> -	drm_mode_probed_add(connector, mode);
>> -
>> -	connector->display_info.width_mm = mode->width_mm;
>> -	connector->display_info.height_mm = mode->height_mm;
>> -
>> -	return 1;
>> +	return drm_connector_helper_get_modes_static(connector, epd->mode);
>>   }
>>   
>>   static const struct drm_connector_helper_funcs repaper_connector_hfuncs = {
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>> index a81f91814595..2d5b56c4a77d 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -620,24 +620,8 @@ static const struct drm_encoder_funcs simpledrm_encoder_funcs = {
>>   static int simpledrm_connector_helper_get_modes(struct drm_connector *connector)
>>   {
>>   	struct simpledrm_device *sdev = simpledrm_device_of_dev(connector->dev);
>> -	struct drm_display_mode *mode;
>>   
>> -	mode = drm_mode_duplicate(connector->dev, &sdev->mode);
>> -	if (!mode)
>> -		return 0;
>> -
>> -	if (mode->name[0] == '\0')
>> -		drm_mode_set_name(mode);
>> -
>> -	mode->type |= DRM_MODE_TYPE_PREFERRED;
>> -	drm_mode_probed_add(connector, mode);
>> -
>> -	if (mode->width_mm)
>> -		connector->display_info.width_mm = mode->width_mm;
>> -	if (mode->height_mm)
>> -		connector->display_info.height_mm = mode->height_mm;
>> -
>> -	return 1;
>> +	return drm_connector_helper_get_modes_static(connector, &sdev->mode);
>>   }
>>   
>>   static const struct drm_connector_helper_funcs simpledrm_connector_helper_funcs = {
>> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
>> index 8075e02aa865..5a883ee9fc32 100644
>> --- a/include/drm/drm_probe_helper.h
>> +++ b/include/drm/drm_probe_helper.h
>> @@ -7,6 +7,7 @@
>>   
>>   struct drm_connector;
>>   struct drm_device;
>> +struct drm_display_mode;
>>   struct drm_modeset_acquire_ctx;
>>   
>>   int drm_helper_probe_single_connector_modes(struct drm_connector
>> @@ -27,6 +28,8 @@ void drm_kms_helper_poll_enable(struct drm_device *dev);
>>   bool drm_kms_helper_is_poll_worker(void);
>>   
>>   int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);
>> +int drm_connector_helper_get_modes_static(struct drm_connector *connector,
>> +					  const struct drm_display_mode *hw_mode);
>>   int drm_connector_helper_get_modes(struct drm_connector *connector);
>>   
>>   #endif
>> -- 
>> 2.37.1

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH 2/4] drm/probe-helper: Add drm_crtc_helper_mode_valid_static()

[Thomas Zimmermann]

Add drm_crtc_helper_mode_valid_static(), which validates a given mode
against a display hardware's mode. Convert simpledrm and use it in a
few other drivers with static modes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_mipi_dbi.c               | 18 ++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c           | 25 ++++++++++++++++++++
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c |  1 +
 drivers/gpu/drm/tiny/hx8357d.c               |  1 +
 drivers/gpu/drm/tiny/ili9163.c               |  1 +
 drivers/gpu/drm/tiny/ili9341.c               |  1 +
 drivers/gpu/drm/tiny/ili9486.c               |  1 +
 drivers/gpu/drm/tiny/mi0283qt.c              |  1 +
 drivers/gpu/drm/tiny/panel-mipi-dbi.c        |  1 +
 drivers/gpu/drm/tiny/repaper.c               | 10 ++++++++
 drivers/gpu/drm/tiny/simpledrm.c             | 10 +-------
 drivers/gpu/drm/tiny/st7735r.c               |  1 +
 include/drm/drm_mipi_dbi.h                   |  2 ++
 include/drm/drm_probe_helper.h               |  8 +++++--
 14 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index b67ec9a5cda9..d544a99df9df 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -309,6 +309,24 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 	drm_dev_exit(idx);
 }
 
+/**
+ * mipi_dbi_pipe_mode_valid - MIPI DBI mode-valid helper
+ * @pipe: Simple display pipe
+ * @mode: The mode to test
+ *
+ * This function validates a given display mode against the MIPI DBI's hardware
+ * display. Drivers can use this as their &drm_simple_display_pipe_funcs->mode_valid
+ * callback.
+ */
+enum drm_mode_status mipi_dbi_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+					      const struct drm_display_mode *mode)
+{
+	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+
+	return drm_crtc_helper_mode_valid_static(&pipe->crtc, mode, &dbidev->mode);
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
+
 /**
  * mipi_dbi_pipe_update - Display pipe update helper
  * @pipe: Simple display pipe
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 809187377e4e..bc3876853fca 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1014,6 +1014,31 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_helper_hpd_irq_event);
 
+/**
+ * drm_crtc_helper_mode_valid_static - Validates a display mode
+ * @crtc: the crtc
+ * @mode: the mode to validate
+ * @hw_mode: the display hardware's mode
+ *
+ * Returns:
+ * MODE_OK on success, or another mode-status code otherwise.
+ */
+enum drm_mode_status drm_crtc_helper_mode_valid_static(struct drm_crtc *crtc,
+						       const struct drm_display_mode *mode,
+						       const struct drm_display_mode *hw_mode)
+{
+
+	if (mode->hdisplay != hw_mode->hdisplay && mode->vdisplay != hw_mode->vdisplay)
+		return MODE_ONE_SIZE;
+	else if (mode->hdisplay != hw_mode->hdisplay)
+		return MODE_ONE_WIDTH;
+	else if (mode->vdisplay != hw_mode->vdisplay)
+		return MODE_ONE_HEIGHT;
+
+	return MODE_OK;
+}
+EXPORT_SYMBOL(drm_crtc_helper_mode_valid_static);
+
 /**
  * drm_connector_helper_get_modes_from_ddc - Updates the connector's EDID
  *                                           property from the connector's
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 7da09e34385d..39dc40cf681f 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -576,6 +576,7 @@ static void ili9341_dbi_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9341_dbi_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = ili9341_dbi_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c
index 57f229a785bf..48c24aa8c28a 100644
--- a/drivers/gpu/drm/tiny/hx8357d.c
+++ b/drivers/gpu/drm/tiny/hx8357d.c
@@ -181,6 +181,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = yx240qv29_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/ili9163.c b/drivers/gpu/drm/tiny/ili9163.c
index 86439e50e304..9a1a5943bee0 100644
--- a/drivers/gpu/drm/tiny/ili9163.c
+++ b/drivers/gpu/drm/tiny/ili9163.c
@@ -100,6 +100,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9163_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = yx240qv29_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c
index b8826a0b086b..69b265e78096 100644
--- a/drivers/gpu/drm/tiny/ili9341.c
+++ b/drivers/gpu/drm/tiny/ili9341.c
@@ -137,6 +137,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = yx240qv29_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
index a5b433a8e0d8..c80028bb1d11 100644
--- a/drivers/gpu/drm/tiny/ili9486.c
+++ b/drivers/gpu/drm/tiny/ili9486.c
@@ -150,6 +150,7 @@ static void waveshare_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs waveshare_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = waveshare_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c
index 27f1bd4da2f4..bc522fb3d94d 100644
--- a/drivers/gpu/drm/tiny/mi0283qt.c
+++ b/drivers/gpu/drm/tiny/mi0283qt.c
@@ -141,6 +141,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = mi0283qt_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
index a76fefa8adbc..955a61d628e7 100644
--- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
+++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
@@ -212,6 +212,7 @@ static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = panel_mipi_dbi_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index 855968fd46af..a7995a3c9397 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -621,6 +621,15 @@ static void power_off(struct repaper_epd *epd)
 	gpiod_set_value_cansleep(epd->discharge, 0);
 }
 
+static enum drm_mode_status repaper_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+						    const struct drm_display_mode *mode)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct repaper_epd *epd = drm_to_epd(crtc->dev);
+
+	return drm_crtc_helper_mode_valid_static(crtc, mode, epd->mode);
+}
+
 static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
 				struct drm_crtc_state *crtc_state,
 				struct drm_plane_state *plane_state)
@@ -831,6 +840,7 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
+	.mode_valid = repaper_pipe_mode_valid,
 	.enable = repaper_pipe_enable,
 	.disable = repaper_pipe_disable,
 	.update = repaper_pipe_update,
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 2d5b56c4a77d..31d3bc6c5acf 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -570,15 +570,7 @@ static enum drm_mode_status simpledrm_crtc_helper_mode_valid(struct drm_crtc *cr
 {
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(crtc->dev);
 
-	if (mode->hdisplay != sdev->mode.hdisplay &&
-	    mode->vdisplay != sdev->mode.vdisplay)
-		return MODE_ONE_SIZE;
-	else if (mode->hdisplay != sdev->mode.hdisplay)
-		return MODE_ONE_WIDTH;
-	else if (mode->vdisplay != sdev->mode.vdisplay)
-		return MODE_ONE_HEIGHT;
-
-	return MODE_OK;
+	return drm_crtc_helper_mode_valid_static(crtc, mode, &sdev->mode);
 }
 
 static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c
index d2042a0f02dd..c36ba08acda1 100644
--- a/drivers/gpu/drm/tiny/st7735r.c
+++ b/drivers/gpu/drm/tiny/st7735r.c
@@ -133,6 +133,7 @@ static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
+	.mode_valid	= mipi_dbi_pipe_mode_valid,
 	.enable		= st7735r_pipe_enable,
 	.disable	= mipi_dbi_pipe_disable,
 	.update		= mipi_dbi_pipe_update,
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index dad2f187b64b..14eaecb1825c 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -155,6 +155,8 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev *dbidev,
 int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
 		      const struct drm_simple_display_pipe_funcs *funcs,
 		      const struct drm_display_mode *mode, unsigned int rotation);
+enum drm_mode_status mipi_dbi_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+					      const struct drm_display_mode *mode);
 void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 			  struct drm_plane_state *old_state);
 void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index 5a883ee9fc32..22b283b35654 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -3,11 +3,11 @@
 #ifndef __DRM_PROBE_HELPER_H__
 #define __DRM_PROBE_HELPER_H__
 
-#include <linux/types.h>
+#include <drm/drm_modes.h>
 
 struct drm_connector;
+struct drm_crtc;
 struct drm_device;
-struct drm_display_mode;
 struct drm_modeset_acquire_ctx;
 
 int drm_helper_probe_single_connector_modes(struct drm_connector
@@ -27,6 +27,10 @@ void drm_kms_helper_poll_disable(struct drm_device *dev);
 void drm_kms_helper_poll_enable(struct drm_device *dev);
 bool drm_kms_helper_is_poll_worker(void);
 
+enum drm_mode_status drm_crtc_helper_mode_valid_static(struct drm_crtc *crtc,
+						       const struct drm_display_mode *mode,
+						       const struct drm_display_mode *hw_mode);
+
 int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);
 int drm_connector_helper_get_modes_static(struct drm_connector *connector,
 					  const struct drm_display_mode *hw_mode);
-- 
2.37.1

Re: [PATCH 2/4] drm/probe-helper: Add drm_crtc_helper_mode_valid_static()

[Sam Ravnborg]

Hi Thomas,

On Wed, Aug 10, 2022 at 01:20:51PM +0200, Thomas Zimmermann wrote:
> Add drm_crtc_helper_mode_valid_static(), which validates a given mode
> against a display hardware's mode. Convert simpledrm and use it in a
> few other drivers with static modes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

With the header file fixed,
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/drm_mipi_dbi.c               | 18 ++++++++++++++
>  drivers/gpu/drm/drm_probe_helper.c           | 25 ++++++++++++++++++++
>  drivers/gpu/drm/panel/panel-ilitek-ili9341.c |  1 +
>  drivers/gpu/drm/tiny/hx8357d.c               |  1 +
>  drivers/gpu/drm/tiny/ili9163.c               |  1 +
>  drivers/gpu/drm/tiny/ili9341.c               |  1 +
>  drivers/gpu/drm/tiny/ili9486.c               |  1 +
>  drivers/gpu/drm/tiny/mi0283qt.c              |  1 +
>  drivers/gpu/drm/tiny/panel-mipi-dbi.c        |  1 +
>  drivers/gpu/drm/tiny/repaper.c               | 10 ++++++++
>  drivers/gpu/drm/tiny/simpledrm.c             | 10 +-------
>  drivers/gpu/drm/tiny/st7735r.c               |  1 +
>  include/drm/drm_mipi_dbi.h                   |  2 ++
>  include/drm/drm_probe_helper.h               |  8 +++++--
>  14 files changed, 70 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index b67ec9a5cda9..d544a99df9df 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -309,6 +309,24 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>  	drm_dev_exit(idx);
>  }
>  
> +/**
> + * mipi_dbi_pipe_mode_valid - MIPI DBI mode-valid helper
> + * @pipe: Simple display pipe
> + * @mode: The mode to test
> + *
> + * This function validates a given display mode against the MIPI DBI's hardware
> + * display. Drivers can use this as their &drm_simple_display_pipe_funcs->mode_valid
> + * callback.
> + */
> +enum drm_mode_status mipi_dbi_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
> +					      const struct drm_display_mode *mode)
> +{
> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> +
> +	return drm_crtc_helper_mode_valid_static(&pipe->crtc, mode, &dbidev->mode);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
> +
>  /**
>   * mipi_dbi_pipe_update - Display pipe update helper
>   * @pipe: Simple display pipe
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 809187377e4e..bc3876853fca 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1014,6 +1014,31 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_helper_hpd_irq_event);
>  
> +/**
> + * drm_crtc_helper_mode_valid_static - Validates a display mode
> + * @crtc: the crtc
> + * @mode: the mode to validate
> + * @hw_mode: the display hardware's mode
> + *
> + * Returns:
> + * MODE_OK on success, or another mode-status code otherwise.
> + */
> +enum drm_mode_status drm_crtc_helper_mode_valid_static(struct drm_crtc *crtc,
> +						       const struct drm_display_mode *mode,
> +						       const struct drm_display_mode *hw_mode)
> +{
> +
> +	if (mode->hdisplay != hw_mode->hdisplay && mode->vdisplay != hw_mode->vdisplay)
> +		return MODE_ONE_SIZE;
> +	else if (mode->hdisplay != hw_mode->hdisplay)
> +		return MODE_ONE_WIDTH;
> +	else if (mode->vdisplay != hw_mode->vdisplay)
> +		return MODE_ONE_HEIGHT;
> +
> +	return MODE_OK;
> +}
> +EXPORT_SYMBOL(drm_crtc_helper_mode_valid_static);
> +
>  /**
>   * drm_connector_helper_get_modes_from_ddc - Updates the connector's EDID
>   *                                           property from the connector's
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> index 7da09e34385d..39dc40cf681f 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> @@ -576,6 +576,7 @@ static void ili9341_dbi_enable(struct drm_simple_display_pipe *pipe,
>  }
>  
>  static const struct drm_simple_display_pipe_funcs ili9341_dbi_funcs = {
> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>  	.enable = ili9341_dbi_enable,
>  	.disable = mipi_dbi_pipe_disable,
>  	.update = mipi_dbi_pipe_update,
> diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c
> index 57f229a785bf..48c24aa8c28a 100644
> --- a/drivers/gpu/drm/tiny/hx8357d.c
> +++ b/drivers/gpu/drm/tiny/hx8357d.c
> @@ -181,6 +181,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
>  }
>  
>  static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>  	.enable = yx240qv29_enable,
>  	.disable = mipi_dbi_pipe_disable,
>  	.update = mipi_dbi_pipe_update,
> diff --git a/drivers/gpu/drm/tiny/ili9163.c b/drivers/gpu/drm/tiny/ili9163.c
> index 86439e50e304..9a1a5943bee0 100644
> --- a/drivers/gpu/drm/tiny/ili9163.c
> +++ b/drivers/gpu/drm/tiny/ili9163.c
> @@ -100,6 +100,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
>  }
>  
>  static const struct drm_simple_display_pipe_funcs ili9163_pipe_funcs = {
> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>  	.enable = yx240qv29_enable,
>  	.disable = mipi_dbi_pipe_disable,
>  	.update = mipi_dbi_pipe_update,
> diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c
> index b8826a0b086b..69b265e78096 100644
> --- a/drivers/gpu/drm/tiny/ili9341.c
> +++ b/drivers/gpu/drm/tiny/ili9341.c
> @@ -137,6 +137,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
>  }
>  
>  static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>  	.enable = yx240qv29_enable,
>  	.disable = mipi_dbi_pipe_disable,
>  	.update = mipi_dbi_pipe_update,
> diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
> index a5b433a8e0d8..c80028bb1d11 100644
> --- a/drivers/gpu/drm/tiny/ili9486.c
> +++ b/drivers/gpu/drm/tiny/ili9486.c
> @@ -150,6 +150,7 @@ static void waveshare_enable(struct drm_simple_display_pipe *pipe,
>  }
>  
>  static const struct drm_simple_display_pipe_funcs waveshare_pipe_funcs = {
> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>  	.enable = waveshare_enable,
>  	.disable = mipi_dbi_pipe_disable,
>  	.update = mipi_dbi_pipe_update,
> diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c
> index 27f1bd4da2f4..bc522fb3d94d 100644
> --- a/drivers/gpu/drm/tiny/mi0283qt.c
> +++ b/drivers/gpu/drm/tiny/mi0283qt.c
> @@ -141,6 +141,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
>  }
>  
>  static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>  	.enable = mi0283qt_enable,
>  	.disable = mipi_dbi_pipe_disable,
>  	.update = mipi_dbi_pipe_update,
> diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> index a76fefa8adbc..955a61d628e7 100644
> --- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> @@ -212,6 +212,7 @@ static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
>  }
>  
>  static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>  	.enable = panel_mipi_dbi_enable,
>  	.disable = mipi_dbi_pipe_disable,
>  	.update = mipi_dbi_pipe_update,
> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> index 855968fd46af..a7995a3c9397 100644
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c
> @@ -621,6 +621,15 @@ static void power_off(struct repaper_epd *epd)
>  	gpiod_set_value_cansleep(epd->discharge, 0);
>  }
>  
> +static enum drm_mode_status repaper_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
> +						    const struct drm_display_mode *mode)
> +{
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	struct repaper_epd *epd = drm_to_epd(crtc->dev);
> +
> +	return drm_crtc_helper_mode_valid_static(crtc, mode, epd->mode);
> +}
> +
>  static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
>  				struct drm_crtc_state *crtc_state,
>  				struct drm_plane_state *plane_state)
> @@ -831,6 +840,7 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
>  }
>  
>  static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
> +	.mode_valid = repaper_pipe_mode_valid,
>  	.enable = repaper_pipe_enable,
>  	.disable = repaper_pipe_disable,
>  	.update = repaper_pipe_update,
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 2d5b56c4a77d..31d3bc6c5acf 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -570,15 +570,7 @@ static enum drm_mode_status simpledrm_crtc_helper_mode_valid(struct drm_crtc *cr
>  {
>  	struct simpledrm_device *sdev = simpledrm_device_of_dev(crtc->dev);
>  
> -	if (mode->hdisplay != sdev->mode.hdisplay &&
> -	    mode->vdisplay != sdev->mode.vdisplay)
> -		return MODE_ONE_SIZE;
> -	else if (mode->hdisplay != sdev->mode.hdisplay)
> -		return MODE_ONE_WIDTH;
> -	else if (mode->vdisplay != sdev->mode.vdisplay)
> -		return MODE_ONE_HEIGHT;
> -
> -	return MODE_OK;
> +	return drm_crtc_helper_mode_valid_static(crtc, mode, &sdev->mode);
>  }
>  
>  static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c
> index d2042a0f02dd..c36ba08acda1 100644
> --- a/drivers/gpu/drm/tiny/st7735r.c
> +++ b/drivers/gpu/drm/tiny/st7735r.c
> @@ -133,6 +133,7 @@ static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
>  }
>  
>  static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
> +	.mode_valid	= mipi_dbi_pipe_mode_valid,
>  	.enable		= st7735r_pipe_enable,
>  	.disable	= mipi_dbi_pipe_disable,
>  	.update		= mipi_dbi_pipe_update,
> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
> index dad2f187b64b..14eaecb1825c 100644
> --- a/include/drm/drm_mipi_dbi.h
> +++ b/include/drm/drm_mipi_dbi.h
> @@ -155,6 +155,8 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev *dbidev,
>  int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
>  		      const struct drm_simple_display_pipe_funcs *funcs,
>  		      const struct drm_display_mode *mode, unsigned int rotation);
> +enum drm_mode_status mipi_dbi_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
> +					      const struct drm_display_mode *mode);
>  void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
>  			  struct drm_plane_state *old_state);
>  void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
> index 5a883ee9fc32..22b283b35654 100644
> --- a/include/drm/drm_probe_helper.h
> +++ b/include/drm/drm_probe_helper.h
> @@ -3,11 +3,11 @@
>  #ifndef __DRM_PROBE_HELPER_H__
>  #define __DRM_PROBE_HELPER_H__
>  
> -#include <linux/types.h>
> +#include <drm/drm_modes.h>
There is no reason to pull in the header, a forward of struct drm_display_mode
should be enough. I expect this to be a left-over from a previous
iteration.

>  
>  struct drm_connector;
> +struct drm_crtc;
>  struct drm_device;
> -struct drm_display_mode;
>  struct drm_modeset_acquire_ctx;
>  
>  int drm_helper_probe_single_connector_modes(struct drm_connector
> @@ -27,6 +27,10 @@ void drm_kms_helper_poll_disable(struct drm_device *dev);
>  void drm_kms_helper_poll_enable(struct drm_device *dev);
>  bool drm_kms_helper_is_poll_worker(void);
>  
> +enum drm_mode_status drm_crtc_helper_mode_valid_static(struct drm_crtc *crtc,
> +						       const struct drm_display_mode *mode,
> +						       const struct drm_display_mode *hw_mode);
> +
>  int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);
>  int drm_connector_helper_get_modes_static(struct drm_connector *connector,
>  					  const struct drm_display_mode *hw_mode);
> -- 
> 2.37.1

Re: [PATCH 2/4] drm/probe-helper: Add drm_crtc_helper_mode_valid_static()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 13129 bytes --]

Hi Sam

Am 10.08.22 um 21:26 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Wed, Aug 10, 2022 at 01:20:51PM +0200, Thomas Zimmermann wrote:
>> Add drm_crtc_helper_mode_valid_static(), which validates a given mode
>> against a display hardware's mode. Convert simpledrm and use it in a
>> few other drivers with static modes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> With the header file fixed,

The include statement is required for enum drm_mode_status.

Best regards
Thomas

> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
>> ---
>>   drivers/gpu/drm/drm_mipi_dbi.c               | 18 ++++++++++++++
>>   drivers/gpu/drm/drm_probe_helper.c           | 25 ++++++++++++++++++++
>>   drivers/gpu/drm/panel/panel-ilitek-ili9341.c |  1 +
>>   drivers/gpu/drm/tiny/hx8357d.c               |  1 +
>>   drivers/gpu/drm/tiny/ili9163.c               |  1 +
>>   drivers/gpu/drm/tiny/ili9341.c               |  1 +
>>   drivers/gpu/drm/tiny/ili9486.c               |  1 +
>>   drivers/gpu/drm/tiny/mi0283qt.c              |  1 +
>>   drivers/gpu/drm/tiny/panel-mipi-dbi.c        |  1 +
>>   drivers/gpu/drm/tiny/repaper.c               | 10 ++++++++
>>   drivers/gpu/drm/tiny/simpledrm.c             | 10 +-------
>>   drivers/gpu/drm/tiny/st7735r.c               |  1 +
>>   include/drm/drm_mipi_dbi.h                   |  2 ++
>>   include/drm/drm_probe_helper.h               |  8 +++++--
>>   14 files changed, 70 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
>> index b67ec9a5cda9..d544a99df9df 100644
>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>> @@ -309,6 +309,24 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>>   	drm_dev_exit(idx);
>>   }
>>   
>> +/**
>> + * mipi_dbi_pipe_mode_valid - MIPI DBI mode-valid helper
>> + * @pipe: Simple display pipe
>> + * @mode: The mode to test
>> + *
>> + * This function validates a given display mode against the MIPI DBI's hardware
>> + * display. Drivers can use this as their &drm_simple_display_pipe_funcs->mode_valid
>> + * callback.
>> + */
>> +enum drm_mode_status mipi_dbi_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
>> +					      const struct drm_display_mode *mode)
>> +{
>> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
>> +
>> +	return drm_crtc_helper_mode_valid_static(&pipe->crtc, mode, &dbidev->mode);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
>> +
>>   /**
>>    * mipi_dbi_pipe_update - Display pipe update helper
>>    * @pipe: Simple display pipe
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 809187377e4e..bc3876853fca 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -1014,6 +1014,31 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
>>   }
>>   EXPORT_SYMBOL(drm_helper_hpd_irq_event);
>>   
>> +/**
>> + * drm_crtc_helper_mode_valid_static - Validates a display mode
>> + * @crtc: the crtc
>> + * @mode: the mode to validate
>> + * @hw_mode: the display hardware's mode
>> + *
>> + * Returns:
>> + * MODE_OK on success, or another mode-status code otherwise.
>> + */
>> +enum drm_mode_status drm_crtc_helper_mode_valid_static(struct drm_crtc *crtc,
>> +						       const struct drm_display_mode *mode,
>> +						       const struct drm_display_mode *hw_mode)
>> +{
>> +
>> +	if (mode->hdisplay != hw_mode->hdisplay && mode->vdisplay != hw_mode->vdisplay)
>> +		return MODE_ONE_SIZE;
>> +	else if (mode->hdisplay != hw_mode->hdisplay)
>> +		return MODE_ONE_WIDTH;
>> +	else if (mode->vdisplay != hw_mode->vdisplay)
>> +		return MODE_ONE_HEIGHT;
>> +
>> +	return MODE_OK;
>> +}
>> +EXPORT_SYMBOL(drm_crtc_helper_mode_valid_static);
>> +
>>   /**
>>    * drm_connector_helper_get_modes_from_ddc - Updates the connector's EDID
>>    *                                           property from the connector's
>> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
>> index 7da09e34385d..39dc40cf681f 100644
>> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
>> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
>> @@ -576,6 +576,7 @@ static void ili9341_dbi_enable(struct drm_simple_display_pipe *pipe,
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs ili9341_dbi_funcs = {
>> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>>   	.enable = ili9341_dbi_enable,
>>   	.disable = mipi_dbi_pipe_disable,
>>   	.update = mipi_dbi_pipe_update,
>> diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c
>> index 57f229a785bf..48c24aa8c28a 100644
>> --- a/drivers/gpu/drm/tiny/hx8357d.c
>> +++ b/drivers/gpu/drm/tiny/hx8357d.c
>> @@ -181,6 +181,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
>> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>>   	.enable = yx240qv29_enable,
>>   	.disable = mipi_dbi_pipe_disable,
>>   	.update = mipi_dbi_pipe_update,
>> diff --git a/drivers/gpu/drm/tiny/ili9163.c b/drivers/gpu/drm/tiny/ili9163.c
>> index 86439e50e304..9a1a5943bee0 100644
>> --- a/drivers/gpu/drm/tiny/ili9163.c
>> +++ b/drivers/gpu/drm/tiny/ili9163.c
>> @@ -100,6 +100,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs ili9163_pipe_funcs = {
>> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>>   	.enable = yx240qv29_enable,
>>   	.disable = mipi_dbi_pipe_disable,
>>   	.update = mipi_dbi_pipe_update,
>> diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c
>> index b8826a0b086b..69b265e78096 100644
>> --- a/drivers/gpu/drm/tiny/ili9341.c
>> +++ b/drivers/gpu/drm/tiny/ili9341.c
>> @@ -137,6 +137,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
>> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>>   	.enable = yx240qv29_enable,
>>   	.disable = mipi_dbi_pipe_disable,
>>   	.update = mipi_dbi_pipe_update,
>> diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
>> index a5b433a8e0d8..c80028bb1d11 100644
>> --- a/drivers/gpu/drm/tiny/ili9486.c
>> +++ b/drivers/gpu/drm/tiny/ili9486.c
>> @@ -150,6 +150,7 @@ static void waveshare_enable(struct drm_simple_display_pipe *pipe,
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs waveshare_pipe_funcs = {
>> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>>   	.enable = waveshare_enable,
>>   	.disable = mipi_dbi_pipe_disable,
>>   	.update = mipi_dbi_pipe_update,
>> diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c
>> index 27f1bd4da2f4..bc522fb3d94d 100644
>> --- a/drivers/gpu/drm/tiny/mi0283qt.c
>> +++ b/drivers/gpu/drm/tiny/mi0283qt.c
>> @@ -141,6 +141,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
>> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>>   	.enable = mi0283qt_enable,
>>   	.disable = mipi_dbi_pipe_disable,
>>   	.update = mipi_dbi_pipe_update,
>> diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
>> index a76fefa8adbc..955a61d628e7 100644
>> --- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
>> +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
>> @@ -212,6 +212,7 @@ static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
>> +	.mode_valid = mipi_dbi_pipe_mode_valid,
>>   	.enable = panel_mipi_dbi_enable,
>>   	.disable = mipi_dbi_pipe_disable,
>>   	.update = mipi_dbi_pipe_update,
>> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
>> index 855968fd46af..a7995a3c9397 100644
>> --- a/drivers/gpu/drm/tiny/repaper.c
>> +++ b/drivers/gpu/drm/tiny/repaper.c
>> @@ -621,6 +621,15 @@ static void power_off(struct repaper_epd *epd)
>>   	gpiod_set_value_cansleep(epd->discharge, 0);
>>   }
>>   
>> +static enum drm_mode_status repaper_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
>> +						    const struct drm_display_mode *mode)
>> +{
>> +	struct drm_crtc *crtc = &pipe->crtc;
>> +	struct repaper_epd *epd = drm_to_epd(crtc->dev);
>> +
>> +	return drm_crtc_helper_mode_valid_static(crtc, mode, epd->mode);
>> +}
>> +
>>   static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
>>   				struct drm_crtc_state *crtc_state,
>>   				struct drm_plane_state *plane_state)
>> @@ -831,6 +840,7 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
>> +	.mode_valid = repaper_pipe_mode_valid,
>>   	.enable = repaper_pipe_enable,
>>   	.disable = repaper_pipe_disable,
>>   	.update = repaper_pipe_update,
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>> index 2d5b56c4a77d..31d3bc6c5acf 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -570,15 +570,7 @@ static enum drm_mode_status simpledrm_crtc_helper_mode_valid(struct drm_crtc *cr
>>   {
>>   	struct simpledrm_device *sdev = simpledrm_device_of_dev(crtc->dev);
>>   
>> -	if (mode->hdisplay != sdev->mode.hdisplay &&
>> -	    mode->vdisplay != sdev->mode.vdisplay)
>> -		return MODE_ONE_SIZE;
>> -	else if (mode->hdisplay != sdev->mode.hdisplay)
>> -		return MODE_ONE_WIDTH;
>> -	else if (mode->vdisplay != sdev->mode.vdisplay)
>> -		return MODE_ONE_HEIGHT;
>> -
>> -	return MODE_OK;
>> +	return drm_crtc_helper_mode_valid_static(crtc, mode, &sdev->mode);
>>   }
>>   
>>   static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
>> diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c
>> index d2042a0f02dd..c36ba08acda1 100644
>> --- a/drivers/gpu/drm/tiny/st7735r.c
>> +++ b/drivers/gpu/drm/tiny/st7735r.c
>> @@ -133,6 +133,7 @@ static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
>> +	.mode_valid	= mipi_dbi_pipe_mode_valid,
>>   	.enable		= st7735r_pipe_enable,
>>   	.disable	= mipi_dbi_pipe_disable,
>>   	.update		= mipi_dbi_pipe_update,
>> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
>> index dad2f187b64b..14eaecb1825c 100644
>> --- a/include/drm/drm_mipi_dbi.h
>> +++ b/include/drm/drm_mipi_dbi.h
>> @@ -155,6 +155,8 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev *dbidev,
>>   int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
>>   		      const struct drm_simple_display_pipe_funcs *funcs,
>>   		      const struct drm_display_mode *mode, unsigned int rotation);
>> +enum drm_mode_status mipi_dbi_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
>> +					      const struct drm_display_mode *mode);
>>   void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
>>   			  struct drm_plane_state *old_state);
>>   void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
>> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
>> index 5a883ee9fc32..22b283b35654 100644
>> --- a/include/drm/drm_probe_helper.h
>> +++ b/include/drm/drm_probe_helper.h
>> @@ -3,11 +3,11 @@
>>   #ifndef __DRM_PROBE_HELPER_H__
>>   #define __DRM_PROBE_HELPER_H__
>>   
>> -#include <linux/types.h>
>> +#include <drm/drm_modes.h>
> There is no reason to pull in the header, a forward of struct drm_display_mode
> should be enough. I expect this to be a left-over from a previous
> iteration.
> 
>>   
>>   struct drm_connector;
>> +struct drm_crtc;
>>   struct drm_device;
>> -struct drm_display_mode;
>>   struct drm_modeset_acquire_ctx;
>>   
>>   int drm_helper_probe_single_connector_modes(struct drm_connector
>> @@ -27,6 +27,10 @@ void drm_kms_helper_poll_disable(struct drm_device *dev);
>>   void drm_kms_helper_poll_enable(struct drm_device *dev);
>>   bool drm_kms_helper_is_poll_worker(void);
>>   
>> +enum drm_mode_status drm_crtc_helper_mode_valid_static(struct drm_crtc *crtc,
>> +						       const struct drm_display_mode *mode,
>> +						       const struct drm_display_mode *hw_mode);
>> +
>>   int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);
>>   int drm_connector_helper_get_modes_static(struct drm_connector *connector,
>>   					  const struct drm_display_mode *hw_mode);
>> -- 
>> 2.37.1

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/4] drm/probe-helper: Add drm_crtc_helper_mode_valid_static()

[Sam Ravnborg]

On Fri, Aug 12, 2022 at 06:37:17PM +0200, Thomas Zimmermann wrote:
> Hi Sam
> 
> Am 10.08.22 um 21:26 schrieb Sam Ravnborg:
> > Hi Thomas,
> > 
> > On Wed, Aug 10, 2022 at 01:20:51PM +0200, Thomas Zimmermann wrote:
> > > Add drm_crtc_helper_mode_valid_static(), which validates a given mode
> > > against a display hardware's mode. Convert simpledrm and use it in a
> > > few other drivers with static modes.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > With the header file fixed,
> 
> The include statement is required for enum drm_mode_status.

Obviously - missed that.

	Sam

[PATCH 3/4] drm/modes: Add initializer macro DRM_MODE_INIT()

[Thomas Zimmermann]

The macro DRM_MODE_INIT() initializes an instance of
struct drm_display_mode with typical parameters. Convert simpledrm
and also update the macro DRM_SIMPLE_MODE().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 23 ++++++++-------------
 include/drm/drm_modes.h          | 35 +++++++++++++++++++++++++++-----
 2 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 31d3bc6c5acf..cc509154b296 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -30,16 +30,6 @@
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
-/*
- * Assume a monitor resolution of 96 dpi to
- * get a somewhat reasonable screen size.
- */
-#define RES_MM(d)	\
-	(((d) * 254ul) / (96ul * 10ul))
-
-#define SIMPLEDRM_MODE(hd, vd)	\
-	DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
-
 /*
  * Helpers for simplefb
  */
@@ -641,10 +631,15 @@ static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
 static struct drm_display_mode simpledrm_mode(unsigned int width,
 					      unsigned int height)
 {
-	struct drm_display_mode mode = { SIMPLEDRM_MODE(width, height) };
-
-	mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
-	drm_mode_set_name(&mode);
+	/*
+	 * Assume a monitor resolution of 96 dpi to
+	 * get a somewhat reasonable screen size.
+	 */
+	const struct drm_display_mode mode = {
+		DRM_MODE_INIT(60, width, height,
+			      DRM_MODE_RES_MM(width, 96ul),
+			      DRM_MODE_RES_MM(height, 96ul))
+	};
 
 	return mode;
 }
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index a80ae9639e96..bb932806f029 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -138,6 +138,35 @@ enum drm_mode_status {
 	.vsync_start = (vss), .vsync_end = (vse), .vtotal = (vt), \
 	.vscan = (vs), .flags = (f)
 
+/**
+ * DRM_MODE_RES_MM - Calculates the display size from resolution and DPI
+ * @res: The resolution in pixel
+ * @dpi: The number of dots per inch
+ */
+#define DRM_MODE_RES_MM(res, dpi)	\
+	(((res) * 254ul) / ((dpi) * 10ul))
+
+#define __DRM_MODE_INIT(pix, hd, vd, hd_mm, vd_mm) \
+	.type = DRM_MODE_TYPE_DRIVER, .clock = (pix), \
+	.hdisplay = (hd), .hsync_start = (hd), .hsync_end = (hd), \
+	.htotal = (hd), .vdisplay = (vd), .vsync_start = (vd), \
+	.vsync_end = (vd), .vtotal = (vd), .width_mm = (hd_mm), \
+	.height_mm = (vd_mm)
+
+/**
+ * DRM_SIMPLE_MODE_INIT - Initialize display mode
+ * @hz: Vertical refresh rate in Hertz
+ * @hd: Horizontal resolution, width
+ * @vd: Vertical resolution, height
+ * @hd_mm: Display width in millimeters
+ * @vd_mm: Display height in millimeters
+ *
+ * This macro initializes a &drm_display_mode that contains information about
+ * refresh rate, resolution and physical size.
+ */
+#define DRM_MODE_INIT(hz, hd, vd, hd_mm, vd_mm) \
+	__DRM_MODE_INIT((hd) * (vd) * (hz) / 1000 /* kHz */, hd, vd, hd_mm, vd_mm)
+
 /**
  * DRM_SIMPLE_MODE - Simple display mode
  * @hd: Horizontal resolution, width
@@ -149,11 +178,7 @@ enum drm_mode_status {
  * resolution and physical size.
  */
 #define DRM_SIMPLE_MODE(hd, vd, hd_mm, vd_mm) \
-	.type = DRM_MODE_TYPE_DRIVER, .clock = 1 /* pass validation */, \
-	.hdisplay = (hd), .hsync_start = (hd), .hsync_end = (hd), \
-	.htotal = (hd), .vdisplay = (vd), .vsync_start = (vd), \
-	.vsync_end = (vd), .vtotal = (vd), .width_mm = (hd_mm), \
-	.height_mm = (vd_mm)
+	__DRM_MODE_INIT(1 /* pass validation */, hd, vd, hd_mm, vd_mm)
 
 #define CRTC_INTERLACE_HALVE_V	(1 << 0) /* halve V values for interlacing */
 #define CRTC_STEREO_DOUBLE	(1 << 1) /* adjust timings for stereo modes */
-- 
2.37.1

Re: [PATCH 3/4] drm/modes: Add initializer macro DRM_MODE_INIT()

[Sam Ravnborg]

On Wed, Aug 10, 2022 at 01:20:52PM +0200, Thomas Zimmermann wrote:
> The macro DRM_MODE_INIT() initializes an instance of
> struct drm_display_mode with typical parameters. Convert simpledrm
> and also update the macro DRM_SIMPLE_MODE().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

It is nice that the macros are now documented.

	Sam

> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 23 ++++++++-------------
>  include/drm/drm_modes.h          | 35 +++++++++++++++++++++++++++-----
>  2 files changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 31d3bc6c5acf..cc509154b296 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -30,16 +30,6 @@
>  #define DRIVER_MAJOR	1
>  #define DRIVER_MINOR	0
>  
> -/*
> - * Assume a monitor resolution of 96 dpi to
> - * get a somewhat reasonable screen size.
> - */
> -#define RES_MM(d)	\
> -	(((d) * 254ul) / (96ul * 10ul))
> -
> -#define SIMPLEDRM_MODE(hd, vd)	\
> -	DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
> -
>  /*
>   * Helpers for simplefb
>   */
> @@ -641,10 +631,15 @@ static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
>  static struct drm_display_mode simpledrm_mode(unsigned int width,
>  					      unsigned int height)
>  {
> -	struct drm_display_mode mode = { SIMPLEDRM_MODE(width, height) };
> -
> -	mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
> -	drm_mode_set_name(&mode);
> +	/*
> +	 * Assume a monitor resolution of 96 dpi to
> +	 * get a somewhat reasonable screen size.
> +	 */
> +	const struct drm_display_mode mode = {
> +		DRM_MODE_INIT(60, width, height,
> +			      DRM_MODE_RES_MM(width, 96ul),
> +			      DRM_MODE_RES_MM(height, 96ul))
> +	};
>  
>  	return mode;
>  }
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index a80ae9639e96..bb932806f029 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -138,6 +138,35 @@ enum drm_mode_status {
>  	.vsync_start = (vss), .vsync_end = (vse), .vtotal = (vt), \
>  	.vscan = (vs), .flags = (f)
>  
> +/**
> + * DRM_MODE_RES_MM - Calculates the display size from resolution and DPI
> + * @res: The resolution in pixel
> + * @dpi: The number of dots per inch
> + */
> +#define DRM_MODE_RES_MM(res, dpi)	\
> +	(((res) * 254ul) / ((dpi) * 10ul))
> +
> +#define __DRM_MODE_INIT(pix, hd, vd, hd_mm, vd_mm) \
> +	.type = DRM_MODE_TYPE_DRIVER, .clock = (pix), \
> +	.hdisplay = (hd), .hsync_start = (hd), .hsync_end = (hd), \
> +	.htotal = (hd), .vdisplay = (vd), .vsync_start = (vd), \
> +	.vsync_end = (vd), .vtotal = (vd), .width_mm = (hd_mm), \
> +	.height_mm = (vd_mm)
> +
> +/**
> + * DRM_SIMPLE_MODE_INIT - Initialize display mode
> + * @hz: Vertical refresh rate in Hertz
> + * @hd: Horizontal resolution, width
> + * @vd: Vertical resolution, height
> + * @hd_mm: Display width in millimeters
> + * @vd_mm: Display height in millimeters
> + *
> + * This macro initializes a &drm_display_mode that contains information about
> + * refresh rate, resolution and physical size.
> + */
> +#define DRM_MODE_INIT(hz, hd, vd, hd_mm, vd_mm) \
> +	__DRM_MODE_INIT((hd) * (vd) * (hz) / 1000 /* kHz */, hd, vd, hd_mm, vd_mm)
> +
>  /**
>   * DRM_SIMPLE_MODE - Simple display mode
>   * @hd: Horizontal resolution, width
> @@ -149,11 +178,7 @@ enum drm_mode_status {
>   * resolution and physical size.
>   */
>  #define DRM_SIMPLE_MODE(hd, vd, hd_mm, vd_mm) \
> -	.type = DRM_MODE_TYPE_DRIVER, .clock = 1 /* pass validation */, \
> -	.hdisplay = (hd), .hsync_start = (hd), .hsync_end = (hd), \
> -	.htotal = (hd), .vdisplay = (vd), .vsync_start = (vd), \
> -	.vsync_end = (vd), .vtotal = (vd), .width_mm = (hd_mm), \
> -	.height_mm = (vd_mm)
> +	__DRM_MODE_INIT(1 /* pass validation */, hd, vd, hd_mm, vd_mm)
>  
>  #define CRTC_INTERLACE_HALVE_V	(1 << 0) /* halve V values for interlacing */
>  #define CRTC_STEREO_DOUBLE	(1 << 1) /* adjust timings for stereo modes */
> -- 
> 2.37.1

[PATCH 4/4] drm/format-helper: Add drm_fb_build_fourcc_list() helper

[Thomas Zimmermann]

Add drm_fb_build_fourcc_list() function that builds a list of supported
formats from native and emulated ones. Helpful for all drivers that do
format conversion as part of their plane updates. Update current caller.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_format_helper.c | 94 +++++++++++++++++++++++++++++
 drivers/gpu/drm/tiny/simpledrm.c    | 47 ++-------------
 include/drm/drm_format_helper.h     | 11 +++-
 3 files changed, 109 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 56642816fdff..dca5531615f3 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -793,3 +793,97 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
 	kfree(src32);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
+
+static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
+{
+	const uint32_t *fourccs_end = fourccs + nfourccs;
+
+	while (fourccs < fourccs_end) {
+		if (*fourccs == fourcc)
+			return true;
+		++fourccs;
+	}
+	return false;
+}
+
+/**
+ * drm_fb_build_fourcc_list - Filters a list of supported color formats against
+ *                            the device's native formats
+ * @dev: DRM device
+ * @native_fourccs: 4CC codes of natively supported color formats
+ * @native_nfourccs: The number of entries in @native_fourccs
+ * @extra_fourccs: 4CC codes of additionally supported color formats
+ * @extra_nfourccs: The number of entries in @extra_fourccs
+ * @fourccs_out: Returns 4CC codes of supported color formats
+ * @nfourccs_out: The number of available entries in @fourccs_out
+ *
+ * This function create a list of supported color format from natively
+ * supported formats and the emulated formats.  *
+ * At a minimum, most userspace programs expect at least support for
+ * XRGB8888 on the primary plane. Devices that have to emulate the
+ * format, and possibly others, can use drm_fb_build_fourcc_list() to
+ * create a list of supported color formats. The returned list can
+ * be handed over to drm_universal_plane_init() et al. Native formats
+ * will go before emulated formats. Other heuristics might be applied
+ * to optimize the order. Formats near the beginning of the list are
+ * usually preferred over formats near the end of the list.
+ *
+ * Returns:
+ * The number of color-formats 4CC codes returned in @fourccs_out.
+ */
+size_t drm_fb_build_fourcc_list(struct drm_device *dev,
+				const uint32_t *native_fourccs, size_t native_nfourccs,
+				const uint32_t *extra_fourccs, size_t extra_nfourccs,
+				uint32_t *fourccs_out, size_t nfourccs_out)
+{
+	uint32_t *fourccs = fourccs_out;
+	const uint32_t *fourccs_end = fourccs_out + nfourccs_out;
+	bool found_native = false;
+	size_t nfourccs, i;
+
+	/* native formats go first */
+
+	nfourccs = min_t(size_t, native_nfourccs, nfourccs_out);
+
+	for (i = 0; i < nfourccs; ++i) {
+		uint32_t fourcc = native_fourccs[i];
+
+		drm_dbg_kms(dev, "adding native format %p4cc\n", &fourcc);
+
+		if (!found_native)
+			found_native = is_listed_fourcc(extra_fourccs, extra_nfourccs, fourcc);
+		*fourccs = fourcc;
+		++fourccs;
+	}
+
+	/*
+	 * The plane's atomic_update helper converts the framebuffer's color format
+	 * to the native format when copying them to device memory.
+	 *
+	 * If there is not a single format supported by both, device and
+	 * plane, the native formats are likely not supported by the conversion
+	 * helpers. Therefore *only* support the native formats and add a
+	 * conversion helper ASAP.
+	 */
+	if (!found_native) {
+		drm_warn(dev, "format conversion helpers required to add extra formats\n");
+		goto out;
+	}
+
+	/* extra formats go second */
+
+	nfourccs = min_t(size_t, extra_nfourccs, fourccs_end - fourccs);
+
+	for (i = 0; i < nfourccs; ++i) {
+		uint32_t fourcc = extra_fourccs[i];
+
+		if (is_listed_fourcc(native_fourccs, native_nfourccs, fourcc))
+			continue; /* native formats already went first */
+		*fourccs = fourcc;
+		++fourccs;
+	}
+
+out:
+	return fourccs - fourccs_out;
+}
+EXPORT_SYMBOL(drm_fb_build_fourcc_list);
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index cc509154b296..79c9fd6bedf0 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -644,45 +644,6 @@ static struct drm_display_mode simpledrm_mode(unsigned int width,
 	return mode;
 }
 
-static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
-						size_t *nformats_out)
-{
-	struct drm_device *dev = &sdev->dev;
-	size_t i;
-
-	if (sdev->nformats)
-		goto out; /* don't rebuild list on recurring calls */
-
-	/* native format goes first */
-	sdev->formats[0] = sdev->format->format;
-	sdev->nformats = 1;
-
-	/* default formats go second */
-	for (i = 0; i < ARRAY_SIZE(simpledrm_primary_plane_formats); ++i) {
-		if (simpledrm_primary_plane_formats[i] == sdev->format->format)
-			continue; /* native format already went first */
-		sdev->formats[sdev->nformats] = simpledrm_primary_plane_formats[i];
-		sdev->nformats++;
-	}
-
-	/*
-	 * TODO: The simpledrm driver converts framebuffers to the native
-	 * format when copying them to device memory. If there are more
-	 * formats listed than supported by the driver, the native format
-	 * is not supported by the conversion helpers. Therefore *only*
-	 * support the native format and add a conversion helper ASAP.
-	 */
-	if (drm_WARN_ONCE(dev, i != sdev->nformats,
-			  "format conversion helpers required for %p4cc",
-			  &sdev->format->format)) {
-		sdev->nformats = 1;
-	}
-
-out:
-	*nformats_out = sdev->nformats;
-	return sdev->formats;
-}
-
 static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 							struct platform_device *pdev)
 {
@@ -699,7 +660,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	unsigned long max_width, max_height;
-	const uint32_t *formats;
 	size_t nformats;
 	int ret;
 
@@ -811,11 +771,14 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 
 	/* Primary plane */
 
-	formats = simpledrm_device_formats(sdev, &nformats);
+	nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
+					    simpledrm_primary_plane_formats,
+					    ARRAY_SIZE(simpledrm_primary_plane_formats),
+					    sdev->formats, ARRAY_SIZE(sdev->formats));
 
 	primary_plane = &sdev->primary_plane;
 	ret = drm_universal_plane_init(dev, primary_plane, 0, &simpledrm_primary_plane_funcs,
-				       formats, nformats,
+				       sdev->formats, nformats,
 				       simpledrm_primary_plane_format_modifiers,
 				       DRM_PLANE_TYPE_PRIMARY, NULL);
 	if (ret)
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index caa181194335..33278870e0d8 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -6,11 +6,15 @@
 #ifndef __LINUX_DRM_FORMAT_HELPER_H
 #define __LINUX_DRM_FORMAT_HELPER_H
 
-struct iosys_map;
+#include <linux/types.h>
+
+struct drm_device;
 struct drm_format_info;
 struct drm_framebuffer;
 struct drm_rect;
 
+struct iosys_map;
+
 unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info *format,
 				const struct drm_rect *clip);
 
@@ -44,4 +48,9 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
 			     const struct iosys_map *src, const struct drm_framebuffer *fb,
 			     const struct drm_rect *clip);
 
+size_t drm_fb_build_fourcc_list(struct drm_device *dev,
+				const uint32_t *native_fourccs, size_t native_nfourccs,
+				const uint32_t *extra_fourccs, size_t extra_nfourccs,
+				uint32_t *fourccs_out, size_t nfourccs_out);
+
 #endif /* __LINUX_DRM_FORMAT_HELPER_H */
-- 
2.37.1

Re: [PATCH 4/4] drm/format-helper: Add drm_fb_build_fourcc_list() helper

[Sam Ravnborg]

Hi Thomas,

On Wed, Aug 10, 2022 at 01:20:53PM +0200, Thomas Zimmermann wrote:
> Add drm_fb_build_fourcc_list() function that builds a list of supported
> formats from native and emulated ones. Helpful for all drivers that do
> format conversion as part of their plane updates. Update current caller.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

A few comments in the following. Consider to add the warning and with it
added or not:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/drm_format_helper.c | 94 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/tiny/simpledrm.c    | 47 ++-------------
>  include/drm/drm_format_helper.h     | 11 +++-
>  3 files changed, 109 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 56642816fdff..dca5531615f3 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -793,3 +793,97 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
>  	kfree(src32);
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
> +
> +static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
> +{
> +	const uint32_t *fourccs_end = fourccs + nfourccs;
> +
> +	while (fourccs < fourccs_end) {
> +		if (*fourccs == fourcc)
> +			return true;
> +		++fourccs;
> +	}
> +	return false;
> +}
> +
> +/**
> + * drm_fb_build_fourcc_list - Filters a list of supported color formats against
> + *                            the device's native formats
> + * @dev: DRM device
> + * @native_fourccs: 4CC codes of natively supported color formats
> + * @native_nfourccs: The number of entries in @native_fourccs
> + * @extra_fourccs: 4CC codes of additionally supported color formats
> + * @extra_nfourccs: The number of entries in @extra_fourccs
> + * @fourccs_out: Returns 4CC codes of supported color formats
> + * @nfourccs_out: The number of available entries in @fourccs_out
> + *
> + * This function create a list of supported color format from natively
> + * supported formats and the emulated formats.  *
Stray '*' at the end of the line.

> + * At a minimum, most userspace programs expect at least support for
> + * XRGB8888 on the primary plane. Devices that have to emulate the
> + * format, and possibly others, can use drm_fb_build_fourcc_list() to
> + * create a list of supported color formats. The returned list can
> + * be handed over to drm_universal_plane_init() et al. Native formats
> + * will go before emulated formats. Other heuristics might be applied
> + * to optimize the order. Formats near the beginning of the list are
> + * usually preferred over formats near the end of the list.
> + *
> + * Returns:
> + * The number of color-formats 4CC codes returned in @fourccs_out.
> + */
> +size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> +				const uint32_t *native_fourccs, size_t native_nfourccs,
> +				const uint32_t *extra_fourccs, size_t extra_nfourccs,
> +				uint32_t *fourccs_out, size_t nfourccs_out)

drm_fourcc.c uses the type u32 for fourcc codes, why no go with the same
here?

I wish we had a better way to express that we have a list of fourcc
codes, for example using list_head. But it is a bad mismatch with
the current drm_universal_plane_init() implementation.

The format negotiation operation in the bridges could benefit too.

> +{
> +	uint32_t *fourccs = fourccs_out;
> +	const uint32_t *fourccs_end = fourccs_out + nfourccs_out;
> +	bool found_native = false;
> +	size_t nfourccs, i;
> +
> +	/* native formats go first */
> +
Drop extra line, capital start
> +	nfourccs = min_t(size_t, native_nfourccs, nfourccs_out);
> +
> +	for (i = 0; i < nfourccs; ++i) {
> +		uint32_t fourcc = native_fourccs[i];
> +
> +		drm_dbg_kms(dev, "adding native format %p4cc\n", &fourcc);
> +
> +		if (!found_native)
> +			found_native = is_listed_fourcc(extra_fourccs, extra_nfourccs, fourcc);
> +		*fourccs = fourcc;
> +		++fourccs;
> +	}
> +
> +	/*
> +	 * The plane's atomic_update helper converts the framebuffer's color format
> +	 * to the native format when copying them to device memory.
> +	 *
> +	 * If there is not a single format supported by both, device and
> +	 * plane, the native formats are likely not supported by the conversion
> +	 * helpers. Therefore *only* support the native formats and add a
> +	 * conversion helper ASAP.
> +	 */
Despite the nice comment I had to think twice. In the end I agree with
this.

> +	if (!found_native) {
> +		drm_warn(dev, "format conversion helpers required to add extra formats\n");
> +		goto out;
> +	}
> +
> +	/* extra formats go second */
> +
Drop extra line, capital start.
> +	nfourccs = min_t(size_t, extra_nfourccs, fourccs_end - fourccs);
> +
> +	for (i = 0; i < nfourccs; ++i) {
> +		uint32_t fourcc = extra_fourccs[i];
> +
> +		if (is_listed_fourcc(native_fourccs, native_nfourccs, fourcc))
> +			continue; /* native formats already went first */
> +		*fourccs = fourcc;
> +		++fourccs;
> +	}
> +
> +out:
> +	return fourccs - fourccs_out;
> +}
It would be prudent to warn if the supplied fourccs_out array is too
small to the provided input formats. simpledrm is about to hit the limit.

> +EXPORT_SYMBOL(drm_fb_build_fourcc_list);
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index cc509154b296..79c9fd6bedf0 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -644,45 +644,6 @@ static struct drm_display_mode simpledrm_mode(unsigned int width,
>  	return mode;
>  }
>  
> -static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
> -						size_t *nformats_out)
> -{
> -	struct drm_device *dev = &sdev->dev;
> -	size_t i;
> -
> -	if (sdev->nformats)
> -		goto out; /* don't rebuild list on recurring calls */
> -
> -	/* native format goes first */
> -	sdev->formats[0] = sdev->format->format;
> -	sdev->nformats = 1;
> -
> -	/* default formats go second */
> -	for (i = 0; i < ARRAY_SIZE(simpledrm_primary_plane_formats); ++i) {
> -		if (simpledrm_primary_plane_formats[i] == sdev->format->format)
> -			continue; /* native format already went first */
> -		sdev->formats[sdev->nformats] = simpledrm_primary_plane_formats[i];
> -		sdev->nformats++;
> -	}
> -
> -	/*
> -	 * TODO: The simpledrm driver converts framebuffers to the native
> -	 * format when copying them to device memory. If there are more
> -	 * formats listed than supported by the driver, the native format
> -	 * is not supported by the conversion helpers. Therefore *only*
> -	 * support the native format and add a conversion helper ASAP.
> -	 */
> -	if (drm_WARN_ONCE(dev, i != sdev->nformats,
> -			  "format conversion helpers required for %p4cc",
> -			  &sdev->format->format)) {
> -		sdev->nformats = 1;
> -	}
> -
> -out:
> -	*nformats_out = sdev->nformats;
> -	return sdev->formats;
> -}
> -
>  static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  							struct platform_device *pdev)
>  {
> @@ -699,7 +660,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
>  	unsigned long max_width, max_height;
> -	const uint32_t *formats;
>  	size_t nformats;
>  	int ret;
>  
> @@ -811,11 +771,14 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  
>  	/* Primary plane */
>  
> -	formats = simpledrm_device_formats(sdev, &nformats);
> +	nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
> +					    simpledrm_primary_plane_formats,
> +					    ARRAY_SIZE(simpledrm_primary_plane_formats),
> +					    sdev->formats, ARRAY_SIZE(sdev->formats));
simpledrm_primary_plane_formats is 6 long, with a todo to add 2 more.
So the current array of 8 in sdev->formats is big enough for now.


>  
>  	primary_plane = &sdev->primary_plane;
>  	ret = drm_universal_plane_init(dev, primary_plane, 0, &simpledrm_primary_plane_funcs,
> -				       formats, nformats,
> +				       sdev->formats, nformats,
>  				       simpledrm_primary_plane_format_modifiers,
>  				       DRM_PLANE_TYPE_PRIMARY, NULL);
>  	if (ret)
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index caa181194335..33278870e0d8 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -6,11 +6,15 @@
>  #ifndef __LINUX_DRM_FORMAT_HELPER_H
>  #define __LINUX_DRM_FORMAT_HELPER_H
>  
> -struct iosys_map;
> +#include <linux/types.h>
> +
> +struct drm_device;
>  struct drm_format_info;
>  struct drm_framebuffer;
>  struct drm_rect;
>  
> +struct iosys_map;
> +
>  unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info *format,
>  				const struct drm_rect *clip);
>  
> @@ -44,4 +48,9 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
>  			     const struct iosys_map *src, const struct drm_framebuffer *fb,
>  			     const struct drm_rect *clip);
>  
> +size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> +				const uint32_t *native_fourccs, size_t native_nfourccs,
> +				const uint32_t *extra_fourccs, size_t extra_nfourccs,
> +				uint32_t *fourccs_out, size_t nfourccs_out);
> +
>  #endif /* __LINUX_DRM_FORMAT_HELPER_H */
> -- 
> 2.37.1

Re: [PATCH 4/4] drm/format-helper: Add drm_fb_build_fourcc_list() helper

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 10331 bytes --]

Hi

Am 12.08.22 um 20:19 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Wed, Aug 10, 2022 at 01:20:53PM +0200, Thomas Zimmermann wrote:
>> Add drm_fb_build_fourcc_list() function that builds a list of supported
>> formats from native and emulated ones. Helpful for all drivers that do
>> format conversion as part of their plane updates. Update current caller.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> A few comments in the following. Consider to add the warning and with it
> added or not:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
>> ---
>>   drivers/gpu/drm/drm_format_helper.c | 94 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tiny/simpledrm.c    | 47 ++-------------
>>   include/drm/drm_format_helper.h     | 11 +++-
>>   3 files changed, 109 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index 56642816fdff..dca5531615f3 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -793,3 +793,97 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
>>   	kfree(src32);
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
>> +
>> +static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
>> +{
>> +	const uint32_t *fourccs_end = fourccs + nfourccs;
>> +
>> +	while (fourccs < fourccs_end) {
>> +		if (*fourccs == fourcc)
>> +			return true;
>> +		++fourccs;
>> +	}
>> +	return false;
>> +}
>> +
>> +/**
>> + * drm_fb_build_fourcc_list - Filters a list of supported color formats against
>> + *                            the device's native formats
>> + * @dev: DRM device
>> + * @native_fourccs: 4CC codes of natively supported color formats
>> + * @native_nfourccs: The number of entries in @native_fourccs
>> + * @extra_fourccs: 4CC codes of additionally supported color formats
>> + * @extra_nfourccs: The number of entries in @extra_fourccs
>> + * @fourccs_out: Returns 4CC codes of supported color formats
>> + * @nfourccs_out: The number of available entries in @fourccs_out
>> + *
>> + * This function create a list of supported color format from natively
>> + * supported formats and the emulated formats.  *
> Stray '*' at the end of the line.
> 
>> + * At a minimum, most userspace programs expect at least support for
>> + * XRGB8888 on the primary plane. Devices that have to emulate the
>> + * format, and possibly others, can use drm_fb_build_fourcc_list() to
>> + * create a list of supported color formats. The returned list can
>> + * be handed over to drm_universal_plane_init() et al. Native formats
>> + * will go before emulated formats. Other heuristics might be applied
>> + * to optimize the order. Formats near the beginning of the list are
>> + * usually preferred over formats near the end of the list.
>> + *
>> + * Returns:
>> + * The number of color-formats 4CC codes returned in @fourccs_out.
>> + */
>> +size_t drm_fb_build_fourcc_list(struct drm_device *dev,
>> +				const uint32_t *native_fourccs, size_t native_nfourccs,
>> +				const uint32_t *extra_fourccs, size_t extra_nfourccs,
>> +				uint32_t *fourccs_out, size_t nfourccs_out)
> 
> drm_fourcc.c uses the type u32 for fourcc codes, why no go with the same
> here?
> 
> I wish we had a better way to express that we have a list of fourcc
> codes, for example using list_head. But it is a bad mismatch with
> the current drm_universal_plane_init() implementation.

I've changed the code to u32. struct list_head is somewhat overhead-ish, 
but I'd like to see a 'fourcc_t' typedef of the u32.

> 
> The format negotiation operation in the bridges could benefit too.
> 
>> +{
>> +	uint32_t *fourccs = fourccs_out;
>> +	const uint32_t *fourccs_end = fourccs_out + nfourccs_out;
>> +	bool found_native = false;
>> +	size_t nfourccs, i;
>> +
>> +	/* native formats go first */
>> +
> Drop extra line, capital start
>> +	nfourccs = min_t(size_t, native_nfourccs, nfourccs_out);
>> +
>> +	for (i = 0; i < nfourccs; ++i) {
>> +		uint32_t fourcc = native_fourccs[i];
>> +
>> +		drm_dbg_kms(dev, "adding native format %p4cc\n", &fourcc);
>> +
>> +		if (!found_native)
>> +			found_native = is_listed_fourcc(extra_fourccs, extra_nfourccs, fourcc);
>> +		*fourccs = fourcc;
>> +		++fourccs;
>> +	}
>> +
>> +	/*
>> +	 * The plane's atomic_update helper converts the framebuffer's color format
>> +	 * to the native format when copying them to device memory.
>> +	 *
>> +	 * If there is not a single format supported by both, device and
>> +	 * plane, the native formats are likely not supported by the conversion
>> +	 * helpers. Therefore *only* support the native formats and add a
>> +	 * conversion helper ASAP.
>> +	 */
> Despite the nice comment I had to think twice. In the end I agree with
> this.
> 
>> +	if (!found_native) {
>> +		drm_warn(dev, "format conversion helpers required to add extra formats\n");
>> +		goto out;
>> +	}
>> +
>> +	/* extra formats go second */
>> +
> Drop extra line, capital start.
>> +	nfourccs = min_t(size_t, extra_nfourccs, fourccs_end - fourccs);
>> +
>> +	for (i = 0; i < nfourccs; ++i) {
>> +		uint32_t fourcc = extra_fourccs[i];
>> +
>> +		if (is_listed_fourcc(native_fourccs, native_nfourccs, fourcc))
>> +			continue; /* native formats already went first */
>> +		*fourccs = fourcc;
>> +		++fourccs;
>> +	}
>> +
>> +out:
>> +	return fourccs - fourccs_out;
>> +}
> It would be prudent to warn if the supplied fourccs_out array is too
> small to the provided input formats. simpledrm is about to hit the limit.

Good idea. I've added a warning.

Best regards
Thomas

> 
>> +EXPORT_SYMBOL(drm_fb_build_fourcc_list);
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>> index cc509154b296..79c9fd6bedf0 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -644,45 +644,6 @@ static struct drm_display_mode simpledrm_mode(unsigned int width,
>>   	return mode;
>>   }
>>   
>> -static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
>> -						size_t *nformats_out)
>> -{
>> -	struct drm_device *dev = &sdev->dev;
>> -	size_t i;
>> -
>> -	if (sdev->nformats)
>> -		goto out; /* don't rebuild list on recurring calls */
>> -
>> -	/* native format goes first */
>> -	sdev->formats[0] = sdev->format->format;
>> -	sdev->nformats = 1;
>> -
>> -	/* default formats go second */
>> -	for (i = 0; i < ARRAY_SIZE(simpledrm_primary_plane_formats); ++i) {
>> -		if (simpledrm_primary_plane_formats[i] == sdev->format->format)
>> -			continue; /* native format already went first */
>> -		sdev->formats[sdev->nformats] = simpledrm_primary_plane_formats[i];
>> -		sdev->nformats++;
>> -	}
>> -
>> -	/*
>> -	 * TODO: The simpledrm driver converts framebuffers to the native
>> -	 * format when copying them to device memory. If there are more
>> -	 * formats listed than supported by the driver, the native format
>> -	 * is not supported by the conversion helpers. Therefore *only*
>> -	 * support the native format and add a conversion helper ASAP.
>> -	 */
>> -	if (drm_WARN_ONCE(dev, i != sdev->nformats,
>> -			  "format conversion helpers required for %p4cc",
>> -			  &sdev->format->format)) {
>> -		sdev->nformats = 1;
>> -	}
>> -
>> -out:
>> -	*nformats_out = sdev->nformats;
>> -	return sdev->formats;
>> -}
>> -
>>   static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>   							struct platform_device *pdev)
>>   {
>> @@ -699,7 +660,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>   	struct drm_encoder *encoder;
>>   	struct drm_connector *connector;
>>   	unsigned long max_width, max_height;
>> -	const uint32_t *formats;
>>   	size_t nformats;
>>   	int ret;
>>   
>> @@ -811,11 +771,14 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>   
>>   	/* Primary plane */
>>   
>> -	formats = simpledrm_device_formats(sdev, &nformats);
>> +	nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
>> +					    simpledrm_primary_plane_formats,
>> +					    ARRAY_SIZE(simpledrm_primary_plane_formats),
>> +					    sdev->formats, ARRAY_SIZE(sdev->formats));
> simpledrm_primary_plane_formats is 6 long, with a todo to add 2 more.
> So the current array of 8 in sdev->formats is big enough for now.
> 
> 
>>   
>>   	primary_plane = &sdev->primary_plane;
>>   	ret = drm_universal_plane_init(dev, primary_plane, 0, &simpledrm_primary_plane_funcs,
>> -				       formats, nformats,
>> +				       sdev->formats, nformats,
>>   				       simpledrm_primary_plane_format_modifiers,
>>   				       DRM_PLANE_TYPE_PRIMARY, NULL);
>>   	if (ret)
>> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
>> index caa181194335..33278870e0d8 100644
>> --- a/include/drm/drm_format_helper.h
>> +++ b/include/drm/drm_format_helper.h
>> @@ -6,11 +6,15 @@
>>   #ifndef __LINUX_DRM_FORMAT_HELPER_H
>>   #define __LINUX_DRM_FORMAT_HELPER_H
>>   
>> -struct iosys_map;
>> +#include <linux/types.h>
>> +
>> +struct drm_device;
>>   struct drm_format_info;
>>   struct drm_framebuffer;
>>   struct drm_rect;
>>   
>> +struct iosys_map;
>> +
>>   unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info *format,
>>   				const struct drm_rect *clip);
>>   
>> @@ -44,4 +48,9 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
>>   			     const struct iosys_map *src, const struct drm_framebuffer *fb,
>>   			     const struct drm_rect *clip);
>>   
>> +size_t drm_fb_build_fourcc_list(struct drm_device *dev,
>> +				const uint32_t *native_fourccs, size_t native_nfourccs,
>> +				const uint32_t *extra_fourccs, size_t extra_nfourccs,
>> +				uint32_t *fourccs_out, size_t nfourccs_out);
>> +
>>   #endif /* __LINUX_DRM_FORMAT_HELPER_H */
>> -- 
>> 2.37.1

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]