[PATCH] Add framebuffer max width/height fields to drm_mode_config

[PATCH] Add framebuffer max width/height fields to drm_mode_config

[Jeykumar Sankaran]

Below two discussion threads will provide the context behind this patch.

https://www.spinics.net/lists/dri-devel/msg229070.html
https://lore.kernel.org/linux-arm-msm/db26145b-3f64-a334-f698-76f972332881@baylibre.com/T/

Seperating out the core framework patch from vendor implementation.

Jeykumar Sankaran (1):
  drm: add fb max width/height fields to drm_mode_config

 drivers/gpu/drm/drm_framebuffer.c | 17 +++++++++++++----
 include/drm/drm_mode_config.h     |  3 +++
 2 files changed, 16 insertions(+), 4 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] drm: add fb max width/height fields to drm_mode_config

[Jeykumar Sankaran]

The mode_config max width/height values determine the maximum
resolution the pixel reader can handle. But the same values are
used to restrict the size of the framebuffer creation. Hardware's
with scaling blocks can operate on framebuffers larger/smaller than
that of the pixel reader resolutions by scaling them down/up before
rendering.

This changes adds a separate framebuffer max width/height fields
in drm_mode_config to allow vendors to set if they are different
than that of the default max resolution values.

Vendors setting these fields should fix their mode_set paths too
by filtering and validating the modes against the appropriate max
fields in their mode_valid() implementations.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
---
 drivers/gpu/drm/drm_framebuffer.c | 15 +++++++++++----
 include/drm/drm_mode_config.h     |  3 +++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 5756431..2083168 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -300,14 +300,21 @@ struct drm_framebuffer *
 		return ERR_PTR(-EINVAL);
 	}
 
-	if ((config->min_width > r->width) || (r->width > config->max_width)) {
+	if ((config->min_width > r->width) ||
+	    (!config->max_fb_width && r->width > config->max_width) ||
+	    (config->max_fb_width && r->width > config->max_fb_width)) {
 		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
-			  r->width, config->min_width, config->max_width);
+			r->width, config->min_width, config->max_fb_width ?
+			config->max_fb_width : config->max_width);
 		return ERR_PTR(-EINVAL);
 	}
-	if ((config->min_height > r->height) || (r->height > config->max_height)) {
+
+	if ((config->min_height > r->height) ||
+	    (!config->max_fb_height && r->height > config->max_height) ||
+	    (config->max_fb_height && r->height > config->max_fb_height)) {
 		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
-			  r->height, config->min_height, config->max_height);
+			r->height, config->min_height, config->max_fb_width ?
+			config->max_fb_height : config->max_height);
 		return ERR_PTR(-EINVAL);
 	}
 
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 3bcbe30..c6394ed 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -339,6 +339,8 @@ struct drm_mode_config_funcs {
  * @min_height: minimum fb pixel height on this device
  * @max_width: maximum fb pixel width on this device
  * @max_height: maximum fb pixel height on this device
+ * @max_fb_width: maximum fb buffer width if differs from max_width
+ * @max_fb_height: maximum fb buffer height if differs from  max_height
  * @funcs: core driver provided mode setting functions
  * @fb_base: base address of the framebuffer
  * @poll_enabled: track polling support for this device
@@ -523,6 +525,7 @@ struct drm_mode_config {
 
 	int min_width, min_height;
 	int max_width, max_height;
+	int max_fb_width, max_fb_height;
 	const struct drm_mode_config_funcs *funcs;
 	resource_size_t fb_base;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: drm: add fb max width/height fields to drm_mode_config

[james qian wang (Arm Technology China)]

On Fri, Sep 27, 2019 at 06:31:24PM -0700, Jeykumar Sankaran wrote:
> The mode_config max width/height values determine the maximum
> resolution the pixel reader can handle. But the same values are
> used to restrict the size of the framebuffer creation. Hardware's
> with scaling blocks can operate on framebuffers larger/smaller than
> that of the pixel reader resolutions by scaling them down/up before
> rendering.
> 
> This changes adds a separate framebuffer max width/height fields
> in drm_mode_config to allow vendors to set if they are different
> than that of the default max resolution values.
> 
> Vendors setting these fields should fix their mode_set paths too
> by filtering and validating the modes against the appropriate max
> fields in their mode_valid() implementations.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>

Hi Jeykumar:

Komeda driver also meets this problem, thank for the fix.

Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>

Thanks
James

> ---
>  drivers/gpu/drm/drm_framebuffer.c | 15 +++++++++++----
>  include/drm/drm_mode_config.h     |  3 +++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 5756431..2083168 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -300,14 +300,21 @@ struct drm_framebuffer *
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	if ((config->min_width > r->width) || (r->width > config->max_width)) {
> +	if ((config->min_width > r->width) ||
> +	    (!config->max_fb_width && r->width > config->max_width) ||
> +	    (config->max_fb_width && r->width > config->max_fb_width)) {
>  		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
> -			  r->width, config->min_width, config->max_width);
> +			r->width, config->min_width, config->max_fb_width ?
> +			config->max_fb_width : config->max_width);
>  		return ERR_PTR(-EINVAL);
>  	}
> -	if ((config->min_height > r->height) || (r->height > config->max_height)) {
> +
> +	if ((config->min_height > r->height) ||
> +	    (!config->max_fb_height && r->height > config->max_height) ||
> +	    (config->max_fb_height && r->height > config->max_fb_height)) {
>  		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
> -			  r->height, config->min_height, config->max_height);
> +			r->height, config->min_height, config->max_fb_width ?
> +			config->max_fb_height : config->max_height);
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 3bcbe30..c6394ed 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -339,6 +339,8 @@ struct drm_mode_config_funcs {
>   * @min_height: minimum fb pixel height on this device
>   * @max_width: maximum fb pixel width on this device
>   * @max_height: maximum fb pixel height on this device
> + * @max_fb_width: maximum fb buffer width if differs from max_width
> + * @max_fb_height: maximum fb buffer height if differs from  max_height
>   * @funcs: core driver provided mode setting functions
>   * @fb_base: base address of the framebuffer
>   * @poll_enabled: track polling support for this device
> @@ -523,6 +525,7 @@ struct drm_mode_config {
>  
>  	int min_width, min_height;
>  	int max_width, max_height;
> +	int max_fb_width, max_fb_height;
>  	const struct drm_mode_config_funcs *funcs;
>  	resource_size_t fb_base;
>  

[PATCH] Add framebuffer max width/height fields to drm_mode_config

[Jeykumar Sankaran]

Below two discussion threads will provide the context behind this patch.

https://www.spinics.net/lists/dri-devel/msg229070.html
https://lore.kernel.org/linux-arm-msm/db26145b-3f64-a334-f698-76f972332881@baylibre.com/T/

Seperating out the core framework patch from vendor implementation.

Jeykumar Sankaran (1):
  drm: add fb max width/height fields to drm_mode_config

 drivers/gpu/drm/drm_framebuffer.c | 17 +++++++++++++----
 include/drm/drm_mode_config.h     |  3 +++
 2 files changed, 16 insertions(+), 4 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] drm: add fb max width/height fields to drm_mode_config

[Jeykumar Sankaran]

The mode_config max width/height values determine the maximum
resolution the pixel reader can handle. But the same values are
used to restrict the size of the framebuffer creation. Hardware's
with scaling blocks can operate on framebuffers larger/smaller than
that of the pixel reader resolutions by scaling them down/up before
rendering.

This changes adds a separate framebuffer max width/height fields
in drm_mode_config to allow vendors to set if they are different
than that of the default max resolution values.

Vendors setting these fields should fix their mode_set paths too
by filtering and validating the modes against the appropriate max
fields in their mode_valid() implementations.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
---
 drivers/gpu/drm/drm_framebuffer.c | 15 +++++++++++----
 include/drm/drm_mode_config.h     |  3 +++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 5756431..2083168 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -300,14 +300,21 @@ struct drm_framebuffer *
 		return ERR_PTR(-EINVAL);
 	}
 
-	if ((config->min_width > r->width) || (r->width > config->max_width)) {
+	if ((config->min_width > r->width) ||
+	    (!config->max_fb_width && r->width > config->max_width) ||
+	    (config->max_fb_width && r->width > config->max_fb_width)) {
 		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
-			  r->width, config->min_width, config->max_width);
+			r->width, config->min_width, config->max_fb_width ?
+			config->max_fb_width : config->max_width);
 		return ERR_PTR(-EINVAL);
 	}
-	if ((config->min_height > r->height) || (r->height > config->max_height)) {
+
+	if ((config->min_height > r->height) ||
+	    (!config->max_fb_height && r->height > config->max_height) ||
+	    (config->max_fb_height && r->height > config->max_fb_height)) {
 		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
-			  r->height, config->min_height, config->max_height);
+			r->height, config->min_height, config->max_fb_width ?
+			config->max_fb_height : config->max_height);
 		return ERR_PTR(-EINVAL);
 	}
 
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 3bcbe30..c6394ed 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -339,6 +339,8 @@ struct drm_mode_config_funcs {
  * @min_height: minimum fb pixel height on this device
  * @max_width: maximum fb pixel width on this device
  * @max_height: maximum fb pixel height on this device
+ * @max_fb_width: maximum fb buffer width if differs from max_width
+ * @max_fb_height: maximum fb buffer height if differs from  max_height
  * @funcs: core driver provided mode setting functions
  * @fb_base: base address of the framebuffer
  * @poll_enabled: track polling support for this device
@@ -523,6 +525,7 @@ struct drm_mode_config {
 
 	int min_width, min_height;
 	int max_width, max_height;
+	int max_fb_width, max_fb_height;
 	const struct drm_mode_config_funcs *funcs;
 	resource_size_t fb_base;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] drm: add fb max width/height fields to drm_mode_config

[Ville Syrjälä]

On Fri, Sep 27, 2019 at 06:28:51PM -0700, Jeykumar Sankaran wrote:
> The mode_config max width/height values determine the maximum
> resolution the pixel reader can handle.

Not according to the docs I "fixed" a while ago.

> But the same values are
> used to restrict the size of the framebuffer creation. Hardware's
> with scaling blocks can operate on framebuffers larger/smaller than
> that of the pixel reader resolutions by scaling them down/up before
> rendering.
> 
> This changes adds a separate framebuffer max width/height fields
> in drm_mode_config to allow vendors to set if they are different
> than that of the default max resolution values.

If you're going to change the meaning of the old values you need
to fix the drivers too.

Personally I don't see too much point in this since you most likely
want to validate all the other timings as well, and so likely need
some kind of mode_valid implementation anyway. Hence to validate
modes there's not much benefit of having global min/max values.

> 
> Vendors setting these fields should fix their mode_set paths too
> by filtering and validating the modes against the appropriate max
> fields in their mode_valid() implementations.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 15 +++++++++++----
>  include/drm/drm_mode_config.h     |  3 +++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 5756431..2083168 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -300,14 +300,21 @@ struct drm_framebuffer *
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	if ((config->min_width > r->width) || (r->width > config->max_width)) {
> +	if ((config->min_width > r->width) ||
> +	    (!config->max_fb_width && r->width > config->max_width) ||
> +	    (config->max_fb_width && r->width > config->max_fb_width)) {
>  		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
> -			  r->width, config->min_width, config->max_width);
> +			r->width, config->min_width, config->max_fb_width ?
> +			config->max_fb_width : config->max_width);
>  		return ERR_PTR(-EINVAL);
>  	}
> -	if ((config->min_height > r->height) || (r->height > config->max_height)) {
> +
> +	if ((config->min_height > r->height) ||
> +	    (!config->max_fb_height && r->height > config->max_height) ||
> +	    (config->max_fb_height && r->height > config->max_fb_height)) {
>  		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
> -			  r->height, config->min_height, config->max_height);
> +			r->height, config->min_height, config->max_fb_width ?
> +			config->max_fb_height : config->max_height);
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 3bcbe30..c6394ed 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -339,6 +339,8 @@ struct drm_mode_config_funcs {
>   * @min_height: minimum fb pixel height on this device
>   * @max_width: maximum fb pixel width on this device
>   * @max_height: maximum fb pixel height on this device
> + * @max_fb_width: maximum fb buffer width if differs from max_width
> + * @max_fb_height: maximum fb buffer height if differs from  max_height
>   * @funcs: core driver provided mode setting functions
>   * @fb_base: base address of the framebuffer
>   * @poll_enabled: track polling support for this device
> @@ -523,6 +525,7 @@ struct drm_mode_config {
>  
>  	int min_width, min_height;
>  	int max_width, max_height;
> +	int max_fb_width, max_fb_height;
>  	const struct drm_mode_config_funcs *funcs;
>  	resource_size_t fb_base;
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: add fb max width/height fields to drm_mode_config

[Jeykumar Sankaran]

On 2019-09-30 03:39, Ville Syrjälä wrote:
> On Fri, Sep 27, 2019 at 06:28:51PM -0700, Jeykumar Sankaran wrote:
>> The mode_config max width/height values determine the maximum
>> resolution the pixel reader can handle.
> 
> Not according to the docs I "fixed" a while ago.
> 
>> But the same values are
>> used to restrict the size of the framebuffer creation. Hardware's
>> with scaling blocks can operate on framebuffers larger/smaller than
>> that of the pixel reader resolutions by scaling them down/up before
>> rendering.
>> 
>> This changes adds a separate framebuffer max width/height fields
>> in drm_mode_config to allow vendors to set if they are different
>> than that of the default max resolution values.
> 
> If you're going to change the meaning of the old values you need
> to fix the drivers too.
> 
> Personally I don't see too much point in this since you most likely
> want to validate all the other timings as well, and so likely need
> some kind of mode_valid implementation anyway. Hence to validate
> modes there's not much benefit of having global min/max values.
> 
https://patchwork.kernel.org/patch/10467155/

I believe you are referring to this patch.

I am primarily interested in the scaling scenario mentioned here. MSM
and a few other hardware have scaling block that are used both ways:

1) Where FB limits are larger than the display limits. Scalar blocks are 
used to
    downscale the framebuffers and render within display limits.

In this scenario, with your patch, are you suggesting the drivers 
maintain the
display limits locally and use those values in fill_modes() / 
mode_valid() to filter
out invalid modes explicitly instead of mode_config.max_width/height?

2) Where FB limits are smaller than display limits. Enforced for 
performance reasons on low tier hardware.
It reduces the fetch bandwidth and uses post blending scalar block to 
scale up the pixel stream
to match the display resolution.

Any suggestions on how this topology can be handled with a single set of 
max/min values?

Thanks and Regards,
Jeykumar S.
>> 
>> Vendors setting these fields should fix their mode_set paths too
>> by filtering and validating the modes against the appropriate max
>> fields in their mode_valid() implementations.
>> 
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/drm_framebuffer.c | 15 +++++++++++----
>>  include/drm/drm_mode_config.h     |  3 +++
>>  2 files changed, 14 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_framebuffer.c
> b/drivers/gpu/drm/drm_framebuffer.c
>> index 5756431..2083168 100644
>> --- a/drivers/gpu/drm/drm_framebuffer.c
>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>> @@ -300,14 +300,21 @@ struct drm_framebuffer *
>>  		return ERR_PTR(-EINVAL);
>>  	}
>> 
>> -	if ((config->min_width > r->width) || (r->width >
> config->max_width)) {
>> +	if ((config->min_width > r->width) ||
>> +	    (!config->max_fb_width && r->width > config->max_width) ||
>> +	    (config->max_fb_width && r->width > config->max_fb_width)) {
>>  		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d
> && <= %d\n",
>> -			  r->width, config->min_width, config->max_width);
>> +			r->width, config->min_width, config->max_fb_width
> ?
>> +			config->max_fb_width : config->max_width);
>>  		return ERR_PTR(-EINVAL);
>>  	}
>> -	if ((config->min_height > r->height) || (r->height >
> config->max_height)) {
>> +
>> +	if ((config->min_height > r->height) ||
>> +	    (!config->max_fb_height && r->height > config->max_height) ||
>> +	    (config->max_fb_height && r->height > config->max_fb_height))
> {
>>  		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d
> && <= %d\n",
>> -			  r->height, config->min_height,
> config->max_height);
>> +			r->height, config->min_height,
> config->max_fb_width ?
>> +			config->max_fb_height : config->max_height);
>>  		return ERR_PTR(-EINVAL);
>>  	}
>> 
>> diff --git a/include/drm/drm_mode_config.h
> b/include/drm/drm_mode_config.h
>> index 3bcbe30..c6394ed 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -339,6 +339,8 @@ struct drm_mode_config_funcs {
>>   * @min_height: minimum fb pixel height on this device
>>   * @max_width: maximum fb pixel width on this device
>>   * @max_height: maximum fb pixel height on this device
>> + * @max_fb_width: maximum fb buffer width if differs from max_width
>> + * @max_fb_height: maximum fb buffer height if differs from  
>> max_height
>>   * @funcs: core driver provided mode setting functions
>>   * @fb_base: base address of the framebuffer
>>   * @poll_enabled: track polling support for this device
>> @@ -523,6 +525,7 @@ struct drm_mode_config {
>> 
>>  	int min_width, min_height;
>>  	int max_width, max_height;
>> +	int max_fb_width, max_fb_height;
>>  	const struct drm_mode_config_funcs *funcs;
>>  	resource_size_t fb_base;
>> 
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
>> a Linux Foundation Collaborative Project
>> 
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> --
> Ville Syrjälä
> Intel

-- 
Jeykumar S

Re: [PATCH] drm: add fb max width/height fields to drm_mode_config

[Ville Syrjälä]

On Tue, Oct 01, 2019 at 02:20:55PM -0700, Jeykumar Sankaran wrote:
> On 2019-09-30 03:39, Ville Syrjälä wrote:
> > On Fri, Sep 27, 2019 at 06:28:51PM -0700, Jeykumar Sankaran wrote:
> >> The mode_config max width/height values determine the maximum
> >> resolution the pixel reader can handle.
> > 
> > Not according to the docs I "fixed" a while ago.
> > 
> >> But the same values are
> >> used to restrict the size of the framebuffer creation. Hardware's
> >> with scaling blocks can operate on framebuffers larger/smaller than
> >> that of the pixel reader resolutions by scaling them down/up before
> >> rendering.
> >> 
> >> This changes adds a separate framebuffer max width/height fields
> >> in drm_mode_config to allow vendors to set if they are different
> >> than that of the default max resolution values.
> > 
> > If you're going to change the meaning of the old values you need
> > to fix the drivers too.
> > 
> > Personally I don't see too much point in this since you most likely
> > want to validate all the other timings as well, and so likely need
> > some kind of mode_valid implementation anyway. Hence to validate
> > modes there's not much benefit of having global min/max values.
> > 
> https://patchwork.kernel.org/patch/10467155/
> 
> I believe you are referring to this patch.
> 
> I am primarily interested in the scaling scenario mentioned here. MSM
> and a few other hardware have scaling block that are used both ways:
> 
> 1) Where FB limits are larger than the display limits. Scalar blocks are 
> used to
>     downscale the framebuffers and render within display limits.
> 
> In this scenario, with your patch, are you suggesting the drivers 
> maintain the
> display limits locally and use those values in fill_modes() / 
> mode_valid() to filter
> out invalid modes explicitly instead of mode_config.max_width/height?
> 
> 2) Where FB limits are smaller than display limits. Enforced for 
> performance reasons on low tier hardware.
> It reduces the fetch bandwidth and uses post blending scalar block to 
> scale up the pixel stream
> to match the display resolution.

As Daniel mentioned in that discussion your typical userspace
assumes that it can use a single unscaled framebuffer with any
advertised mode. Hence I believe limiting the mode list based
on the max framebuffer size is pretty much required unless
you want to break existing userspace.

In i915 I went a bit further than that recently and now we
filter the mode list based on the maximum plane size [1] 
(which can be less than the max fb size and less than the
maximum crtc dimensions). And again that's because userspace
assumes that it can just use a single unscaled fullscreen
plane to cover the entire crtc.

These assumption are also carved into the legacy setcrtc uapi
where you can't even specify multiple framebuffers. In theory
a driver could internally use multiple planes to overcome some
of the limitations, but in i915 at least we don't.

[1] https://cgit.freedesktop.org/drm/drm-intel/commit/?id=2d20411e25a3bf3d2914a2219f47ed48dc57aed5

> 
> Any suggestions on how this topology can be handled with a single set of 
> max/min values?
>

I think a safe way to relax these rules would be to either:
a) Add a client cap by which userspace can inform the kernel
   it understands there are more complicated limits at play
   and thus can't assume that everything will just work
b) Maybe we could just tie that in with the atomic cap since
   atomic clients are pretty much required to do the TEST_ONLY
   dance anyway, so one might hope they have a working fallback
   strategy. Though I suspect eg. the modesetting ddx wouldn't
   like this. But we no longer allow atomic with X anyway so
   that partcular argument may not hold much weight anymore.

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: add fb max width/height fields to drm_mode_config

[Rob Clark]

On Wed, Oct 2, 2019 at 9:45 AM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Tue, Oct 01, 2019 at 02:20:55PM -0700, Jeykumar Sankaran wrote:
> > On 2019-09-30 03:39, Ville Syrjälä wrote:
> > > On Fri, Sep 27, 2019 at 06:28:51PM -0700, Jeykumar Sankaran wrote:
> > >> The mode_config max width/height values determine the maximum
> > >> resolution the pixel reader can handle.
> > >
> > > Not according to the docs I "fixed" a while ago.
> > >
> > >> But the same values are
> > >> used to restrict the size of the framebuffer creation. Hardware's
> > >> with scaling blocks can operate on framebuffers larger/smaller than
> > >> that of the pixel reader resolutions by scaling them down/up before
> > >> rendering.
> > >>
> > >> This changes adds a separate framebuffer max width/height fields
> > >> in drm_mode_config to allow vendors to set if they are different
> > >> than that of the default max resolution values.
> > >
> > > If you're going to change the meaning of the old values you need
> > > to fix the drivers too.
> > >
> > > Personally I don't see too much point in this since you most likely
> > > want to validate all the other timings as well, and so likely need
> > > some kind of mode_valid implementation anyway. Hence to validate
> > > modes there's not much benefit of having global min/max values.
> > >
> > https://patchwork.kernel.org/patch/10467155/
> >
> > I believe you are referring to this patch.
> >
> > I am primarily interested in the scaling scenario mentioned here. MSM
> > and a few other hardware have scaling block that are used both ways:
> >
> > 1) Where FB limits are larger than the display limits. Scalar blocks are
> > used to
> >     downscale the framebuffers and render within display limits.
> >
> > In this scenario, with your patch, are you suggesting the drivers
> > maintain the
> > display limits locally and use those values in fill_modes() /
> > mode_valid() to filter
> > out invalid modes explicitly instead of mode_config.max_width/height?
> >
> > 2) Where FB limits are smaller than display limits. Enforced for
> > performance reasons on low tier hardware.
> > It reduces the fetch bandwidth and uses post blending scalar block to
> > scale up the pixel stream
> > to match the display resolution.
>
> As Daniel mentioned in that discussion your typical userspace
> assumes that it can use a single unscaled framebuffer with any
> advertised mode. Hence I believe limiting the mode list based
> on the max framebuffer size is pretty much required unless
> you want to break existing userspace.
>
> In i915 I went a bit further than that recently and now we
> filter the mode list based on the maximum plane size [1]
> (which can be less than the max fb size and less than the
> maximum crtc dimensions). And again that's because userspace
> assumes that it can just use a single unscaled fullscreen
> plane to cover the entire crtc.
>
> These assumption are also carved into the legacy setcrtc uapi
> where you can't even specify multiple framebuffers. In theory
> a driver could internally use multiple planes to overcome some
> of the limitations, but in i915 at least we don't.
>
> [1] https://cgit.freedesktop.org/drm/drm-intel/commit/?id=2d20411e25a3bf3d2914a2219f47ed48dc57aed5
>
> >
> > Any suggestions on how this topology can be handled with a single set of
> > max/min values?
> >
>
> I think a safe way to relax these rules would be to either:
> a) Add a client cap by which userspace can inform the kernel
>    it understands there are more complicated limits at play
>    and thus can't assume that everything will just work
> b) Maybe we could just tie that in with the atomic cap since
>    atomic clients are pretty much required to do the TEST_ONLY
>    dance anyway, so one might hope they have a working fallback
>    strategy. Though I suspect eg. the modesetting ddx wouldn't
>    like this. But we no longer allow atomic with X anyway so
>    that partcular argument may not hold much weight anymore.

What was the conclusion of the hack to not expose atomic to
modesetting ddx, due to the brokenness of it's atomic use?  I guess
that could also make the modesetting case go away..

BR,
-R

Re: [PATCH] drm: add fb max width/height fields to drm_mode_config

[Ville Syrjälä]

On Wed, Oct 02, 2019 at 03:55:10PM -0400, Rob Clark wrote:
> On Wed, Oct 2, 2019 at 9:45 AM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Tue, Oct 01, 2019 at 02:20:55PM -0700, Jeykumar Sankaran wrote:
> > > On 2019-09-30 03:39, Ville Syrjälä wrote:
> > > > On Fri, Sep 27, 2019 at 06:28:51PM -0700, Jeykumar Sankaran wrote:
> > > >> The mode_config max width/height values determine the maximum
> > > >> resolution the pixel reader can handle.
> > > >
> > > > Not according to the docs I "fixed" a while ago.
> > > >
> > > >> But the same values are
> > > >> used to restrict the size of the framebuffer creation. Hardware's
> > > >> with scaling blocks can operate on framebuffers larger/smaller than
> > > >> that of the pixel reader resolutions by scaling them down/up before
> > > >> rendering.
> > > >>
> > > >> This changes adds a separate framebuffer max width/height fields
> > > >> in drm_mode_config to allow vendors to set if they are different
> > > >> than that of the default max resolution values.
> > > >
> > > > If you're going to change the meaning of the old values you need
> > > > to fix the drivers too.
> > > >
> > > > Personally I don't see too much point in this since you most likely
> > > > want to validate all the other timings as well, and so likely need
> > > > some kind of mode_valid implementation anyway. Hence to validate
> > > > modes there's not much benefit of having global min/max values.
> > > >
> > > https://patchwork.kernel.org/patch/10467155/
> > >
> > > I believe you are referring to this patch.
> > >
> > > I am primarily interested in the scaling scenario mentioned here. MSM
> > > and a few other hardware have scaling block that are used both ways:
> > >
> > > 1) Where FB limits are larger than the display limits. Scalar blocks are
> > > used to
> > >     downscale the framebuffers and render within display limits.
> > >
> > > In this scenario, with your patch, are you suggesting the drivers
> > > maintain the
> > > display limits locally and use those values in fill_modes() /
> > > mode_valid() to filter
> > > out invalid modes explicitly instead of mode_config.max_width/height?
> > >
> > > 2) Where FB limits are smaller than display limits. Enforced for
> > > performance reasons on low tier hardware.
> > > It reduces the fetch bandwidth and uses post blending scalar block to
> > > scale up the pixel stream
> > > to match the display resolution.
> >
> > As Daniel mentioned in that discussion your typical userspace
> > assumes that it can use a single unscaled framebuffer with any
> > advertised mode. Hence I believe limiting the mode list based
> > on the max framebuffer size is pretty much required unless
> > you want to break existing userspace.
> >
> > In i915 I went a bit further than that recently and now we
> > filter the mode list based on the maximum plane size [1]
> > (which can be less than the max fb size and less than the
> > maximum crtc dimensions). And again that's because userspace
> > assumes that it can just use a single unscaled fullscreen
> > plane to cover the entire crtc.
> >
> > These assumption are also carved into the legacy setcrtc uapi
> > where you can't even specify multiple framebuffers. In theory
> > a driver could internally use multiple planes to overcome some
> > of the limitations, but in i915 at least we don't.
> >
> > [1] https://cgit.freedesktop.org/drm/drm-intel/commit/?id=2d20411e25a3bf3d2914a2219f47ed48dc57aed5
> >
> > >
> > > Any suggestions on how this topology can be handled with a single set of
> > > max/min values?
> > >
> >
> > I think a safe way to relax these rules would be to either:
> > a) Add a client cap by which userspace can inform the kernel
> >    it understands there are more complicated limits at play
> >    and thus can't assume that everything will just work
> > b) Maybe we could just tie that in with the atomic cap since
> >    atomic clients are pretty much required to do the TEST_ONLY
> >    dance anyway, so one might hope they have a working fallback
> >    strategy. Though I suspect eg. the modesetting ddx wouldn't
> >    like this. But we no longer allow atomic with X anyway so
> >    that partcular argument may not hold much weight anymore.
> 
> What was the conclusion of the hack to not expose atomic to
> modesetting ddx, due to the brokenness of it's atomic use?  I guess
> that could also make the modesetting case go away..

I thought it went in? Maybe I'm mistaken.

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: add fb max width/height fields to drm_mode_config

[Daniel Vetter]

On Thu, Oct 03, 2019 at 01:27:18PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 02, 2019 at 03:55:10PM -0400, Rob Clark wrote:
> > On Wed, Oct 2, 2019 at 9:45 AM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Tue, Oct 01, 2019 at 02:20:55PM -0700, Jeykumar Sankaran wrote:
> > > > On 2019-09-30 03:39, Ville Syrjälä wrote:
> > > > > On Fri, Sep 27, 2019 at 06:28:51PM -0700, Jeykumar Sankaran wrote:
> > > > >> The mode_config max width/height values determine the maximum
> > > > >> resolution the pixel reader can handle.
> > > > >
> > > > > Not according to the docs I "fixed" a while ago.
> > > > >
> > > > >> But the same values are
> > > > >> used to restrict the size of the framebuffer creation. Hardware's
> > > > >> with scaling blocks can operate on framebuffers larger/smaller than
> > > > >> that of the pixel reader resolutions by scaling them down/up before
> > > > >> rendering.
> > > > >>
> > > > >> This changes adds a separate framebuffer max width/height fields
> > > > >> in drm_mode_config to allow vendors to set if they are different
> > > > >> than that of the default max resolution values.
> > > > >
> > > > > If you're going to change the meaning of the old values you need
> > > > > to fix the drivers too.
> > > > >
> > > > > Personally I don't see too much point in this since you most likely
> > > > > want to validate all the other timings as well, and so likely need
> > > > > some kind of mode_valid implementation anyway. Hence to validate
> > > > > modes there's not much benefit of having global min/max values.
> > > > >
> > > > https://patchwork.kernel.org/patch/10467155/
> > > >
> > > > I believe you are referring to this patch.
> > > >
> > > > I am primarily interested in the scaling scenario mentioned here. MSM
> > > > and a few other hardware have scaling block that are used both ways:
> > > >
> > > > 1) Where FB limits are larger than the display limits. Scalar blocks are
> > > > used to
> > > >     downscale the framebuffers and render within display limits.
> > > >
> > > > In this scenario, with your patch, are you suggesting the drivers
> > > > maintain the
> > > > display limits locally and use those values in fill_modes() /
> > > > mode_valid() to filter
> > > > out invalid modes explicitly instead of mode_config.max_width/height?
> > > >
> > > > 2) Where FB limits are smaller than display limits. Enforced for
> > > > performance reasons on low tier hardware.
> > > > It reduces the fetch bandwidth and uses post blending scalar block to
> > > > scale up the pixel stream
> > > > to match the display resolution.
> > >
> > > As Daniel mentioned in that discussion your typical userspace
> > > assumes that it can use a single unscaled framebuffer with any
> > > advertised mode. Hence I believe limiting the mode list based
> > > on the max framebuffer size is pretty much required unless
> > > you want to break existing userspace.
> > >
> > > In i915 I went a bit further than that recently and now we
> > > filter the mode list based on the maximum plane size [1]
> > > (which can be less than the max fb size and less than the
> > > maximum crtc dimensions). And again that's because userspace
> > > assumes that it can just use a single unscaled fullscreen
> > > plane to cover the entire crtc.
> > >
> > > These assumption are also carved into the legacy setcrtc uapi
> > > where you can't even specify multiple framebuffers. In theory
> > > a driver could internally use multiple planes to overcome some
> > > of the limitations, but in i915 at least we don't.
> > >
> > > [1] https://cgit.freedesktop.org/drm/drm-intel/commit/?id=2d20411e25a3bf3d2914a2219f47ed48dc57aed5
> > >
> > > >
> > > > Any suggestions on how this topology can be handled with a single set of
> > > > max/min values?
> > > >
> > >
> > > I think a safe way to relax these rules would be to either:
> > > a) Add a client cap by which userspace can inform the kernel
> > >    it understands there are more complicated limits at play
> > >    and thus can't assume that everything will just work

+1 on this approach. We already have that for 3d modes, another client cap
for "modes bigger than max fb" sounds like a good idea.

For "max plane size" I'm leaning towards drivers should virtualize at
least the primary plane if that's needed to scan out the biggest
resolution. Since there's way too much userspace which will simply not
work otherwise (iirc that's what a bunch of dual-pipe dsi drivers did).

> > > b) Maybe we could just tie that in with the atomic cap since
> > >    atomic clients are pretty much required to do the TEST_ONLY
> > >    dance anyway, so one might hope they have a working fallback
> > >    strategy. Though I suspect eg. the modesetting ddx wouldn't
> > >    like this. But we no longer allow atomic with X anyway so
> > >    that partcular argument may not hold much weight anymore.
> > 
> > What was the conclusion of the hack to not expose atomic to
> > modesetting ddx, due to the brokenness of it's atomic use?  I guess
> > that could also make the modesetting case go away..
> 
> I thought it went in? Maybe I'm mistaken.

I did:

commit 26b1d3b527e7bf3e24b814d617866ac5199ce68d
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Sep 5 20:53:18 2019 +0200

    drm/atomic: Take the atomic toys away from X

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch