On 28/07/2023 14:12, Roger Sewell wrote:
Thomas, Jocelyn,
JF> I think the culprit is probably this patch:
JF> https://patchwork.freedesktop.org/patch/486242/
JF>
JF> before this patch,
JF> mgag200_simple_display_pipe_mode_valid() always return MODE_OK
JF>
JF> after this patch, it checks the bandwidth limit too.
It turns out to be more complicated than that - and I still think it is
something to do with how the two functions
mgag200_simple_display_pipe_mode_valid and
mgag200_mode_config_mode_valid are made known to the rest of the drm
system, i.e. which slots in the various function structures they are put
in.
I attach a contiguous excerpt from /var/log/messages, recording what
happened when I did the following.
1. I instrumented the old mgag200 module with printk statements in
mgag200_simple_display_pipe_mode_valid and mga_vga_mode_valid and
mga_vga_calculate_mode_bandwidth, and also put in a call to the
latter in mgag200_simple_display_pipe_mode_valid so that I could see
what parameters it had been called with. I then rebooted the system,
getting the messages starting at Jul 28 10:42:45 . As you will see,
almost every time mgag200_simple_display_pipe_mode_valid is called it
is immediately following a return of MODE_OK from mga_vga_mode_valid
with the same display parameters - the two exceptions are:
a) at line 1156 is when it is called after "fbcon: mgag200drmfb (fb0)
is primary device", and
b) with the mode actually being set (1440x900) at line 2681 when it
of course returns MODE_OK (as that is what it always returns, as
you say).
2. I then switched to the new mgag200 module similarly instrumented, but
with the unique_rev_id increased by 1 to get sufficient bandwidth to
make 1440x900 usable. I then rebooted the system, getting the
messages starting at Jul 28 11:46:53 . Again, almost every time
mgag200_simple_display_pipe_mode_valid is called it is immediately
after a return of MODE_OK from mgag200_mode_config_mode_valid, and we
still have exception type (a) at line 5672. However, the exception
type (b) is no longer present, as at line 6590, on setting the
1440x900 mode, there is now a call of mgag200_mode_config_mode_valid
preceding the call of mgag200_simple_display_pipe_mode_valid.
3. I then modified that mgag200 module by forcing a return of MODE_OK
from mgag200_simple_display_pipe_mode_valid and removing the
increment to unique_rev_id, so that 1440x900 is no longer "valid"
according to the criteria being used. I rebooted, getting the
messages starting at Jul 28 12:12:34 . Now at line 11004 we have a
failure to set mode immediately followed by a return of MODE_BAD, not
from mgag200_simple_display_pipe_mode_valid but from
mgag200_mode_config_mode_valid.
Thus between the old mgag200 module and the new one, there is a change
in when the mode_config_mode_valid function gets called - there being
one extra call. So even if one were to revert to
mgag200_simple_display_pipe_mode_valid just always returning MODE_OK it
wouldn't fix the problem.
At present I don't see how the change of behaviour can be anything other
than to do with the way these function names are passed to the rest of
the drm system (though of course even if that were reverted, the fact
that mgag200_simple_display_pipe_mode_valid now tests bandwidth would
still cause what I want to do to fail).
Sadly I don't understand how the drm system works, so I'm not sure that
I can shed any more light - but if there are any more experiments that
would help, please do let me know.
I think the issue is that in v5.18, the memory check was done on the
connector mode_valid() callback, and in v6.0, it's done in the
mode_config mode_valid() callback.
Can you please try the patch attached, I moved the bandwidth check back
to the connector callback.
Best regards,
--
Jocelyn
Thanks,
Roger.
From f134c822b11f8e8d16c7f72fe8077c58f9ebb1fd Mon Sep 17 00:00:00 2001
From: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
Date: Tue, 1 Aug 2023 11:49:14 +0200
Subject: [PATCH] drm/mgag200: move the memory bandwidth check to the connector
callback
Prior to commit 475e2b970cc3 ("drm/mgag200: Split up connector's mode_valid
helper"), the memory bandwidth check was done on the connector helper.
It then moved to the drm_mode_config_funcs helper, which can't be
bypassed by the userspace, and some resolution (like 1440x900) that
used to work are now broken.
Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
---
drivers/gpu/drm/mgag200/mgag200_mode.c | 83 ++++++++++++++------------
1 file changed, 46 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 225cca2ed60e..6fd57340d677 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -707,8 +707,54 @@ static int mgag200_vga_connector_helper_get_modes(struct drm_connector *connecto
return ret;
}
+/* Calculates a mode's required memory bandwidth (in KiB/sec). */
+static uint32_t mgag200_calculate_mode_bandwidth(const struct drm_display_mode *mode,
+ unsigned int bits_per_pixel)
+{
+ uint32_t total_area, divisor;
+ uint64_t active_area, pixels_per_second, bandwidth;
+ uint64_t bytes_per_pixel = (bits_per_pixel + 7) / 8;
+
+ divisor = 1024;
+
+ if (!mode->htotal || !mode->vtotal || !mode->clock)
+ return 0;
+
+ active_area = mode->hdisplay * mode->vdisplay;
+ total_area = mode->htotal * mode->vtotal;
+
+ pixels_per_second = active_area * mode->clock * 1000;
+ do_div(pixels_per_second, total_area);
+
+ bandwidth = pixels_per_second * bytes_per_pixel * 100;
+ do_div(bandwidth, divisor);
+
+ return (uint32_t)bandwidth;
+}
+
+static enum drm_mode_status mgag200_vga_connector_helper_mode_valid(
+ struct drm_connector *connector,
+ struct drm_display_mode *mode)
+{
+ struct mga_device *mdev = to_mga_device(connector->dev);
+ const struct mgag200_device_info *info = mdev->info;
+
+ /*
+ * Test the mode's required memory bandwidth if the device
+ * specifies a maximum. Not all devices do though.
+ */
+ if (info->max_mem_bandwidth) {
+ uint32_t mode_bandwidth = mgag200_calculate_mode_bandwidth(mode, 32);
+
+ if (mode_bandwidth > (info->max_mem_bandwidth * 1024))
+ return MODE_BAD;
+ }
+ return MODE_OK;
+}
+
static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {
.get_modes = mgag200_vga_connector_helper_get_modes,
+ .mode_valid = mgag200_vga_connector_helper_mode_valid,
};
static const struct drm_connector_funcs mga_vga_connector_funcs = {
@@ -986,38 +1032,12 @@ static const uint64_t mgag200_simple_display_pipe_fmtmods[] = {
* Mode config
*/
-/* Calculates a mode's required memory bandwidth (in KiB/sec). */
-static uint32_t mgag200_calculate_mode_bandwidth(const struct drm_display_mode *mode,
- unsigned int bits_per_pixel)
-{
- uint32_t total_area, divisor;
- uint64_t active_area, pixels_per_second, bandwidth;
- uint64_t bytes_per_pixel = (bits_per_pixel + 7) / 8;
-
- divisor = 1024;
-
- if (!mode->htotal || !mode->vtotal || !mode->clock)
- return 0;
-
- active_area = mode->hdisplay * mode->vdisplay;
- total_area = mode->htotal * mode->vtotal;
-
- pixels_per_second = active_area * mode->clock * 1000;
- do_div(pixels_per_second, total_area);
-
- bandwidth = pixels_per_second * bytes_per_pixel * 100;
- do_div(bandwidth, divisor);
-
- return (uint32_t)bandwidth;
-}
-
static enum drm_mode_status mgag200_mode_config_mode_valid(struct drm_device *dev,
const struct drm_display_mode *mode)
{
static const unsigned int max_bpp = 4; // DRM_FORMAT_XRGB8888
struct mga_device *mdev = to_mga_device(dev);
unsigned long fbsize, fbpages, max_fbpages;
- const struct mgag200_device_info *info = mdev->info;
max_fbpages = mdev->vram_available >> PAGE_SHIFT;
@@ -1027,17 +1047,6 @@ static enum drm_mode_status mgag200_mode_config_mode_valid(struct drm_device *de
if (fbpages > max_fbpages)
return MODE_MEM;
- /*
- * Test the mode's required memory bandwidth if the device
- * specifies a maximum. Not all devices do though.
- */
- if (info->max_mem_bandwidth) {
- uint32_t mode_bandwidth = mgag200_calculate_mode_bandwidth(mode, max_bpp * 8);
-
- if (mode_bandwidth > (info->max_mem_bandwidth * 1024))
- return MODE_BAD;
- }
-
return MODE_OK;
}
--
2.41.0
