Hi Am 12.05.22 um 12:38 schrieb Jocelyn Falempe:
On 09/05/2022 12:35, Thomas Zimmermann wrote:Split up the connector's mode_valid helper into a simple-pipe and a mode-config helper. The simple-pipe helper tests for display-size limits while the mode-config helper tests for memory-bandwidth limits. Also add the mgag200_ prefix to mga_vga_calculate_mode_bandwidth() and comment on the function's purpose. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> --- drivers/gpu/drm/mgag200/mgag200_mode.c | 146 ++++++++++++------------- 1 file changed, 69 insertions(+), 77 deletions(-)diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.cindex 92d3ae9489f0..a808827d7a2c 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c@@ -681,38 +681,27 @@ static int mgag200_vga_connector_helper_get_modes(struct drm_connector *connectoreturn ret; }-static uint32_t mga_vga_calculate_mode_bandwidth(struct drm_display_mode *mode,- 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);+static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {+ .get_modes = mgag200_vga_connector_helper_get_modes, +}; - return (uint32_t)(bandwidth); -} +static const struct drm_connector_funcs mga_vga_connector_funcs = { + .reset = drm_atomic_helper_connector_reset, + .fill_modes = drm_helper_probe_single_connector_modes, + .destroy = drm_connector_cleanup,+ .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,+ .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; -#define MODE_BANDWIDTH MODE_BAD +/* + * Simple Display Pipe + */-static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,- struct drm_display_mode *mode) +static enum drm_mode_status+mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,+ const struct drm_display_mode *mode) { - struct drm_device *dev = connector->dev; - struct mga_device *mdev = to_mga_device(dev); - int bpp = 32; + struct mga_device *mdev = to_mga_device(pipe->crtc.dev); if (IS_G200_SE(mdev)) { u32 unique_rev_id = mdev->model.g200se.unique_rev_id;@@ -722,42 +711,17 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,return MODE_VIRTUAL_X; if (mode->vdisplay > 1200) return MODE_VIRTUAL_Y; - if (mga_vga_calculate_mode_bandwidth(mode, bpp) - > (24400 * 1024)) - return MODE_BANDWIDTH; } else if (unique_rev_id == 0x02) { if (mode->hdisplay > 1920) return MODE_VIRTUAL_X; if (mode->vdisplay > 1200) return MODE_VIRTUAL_Y; - if (mga_vga_calculate_mode_bandwidth(mode, bpp) - > (30100 * 1024)) - return MODE_BANDWIDTH; - } else { - if (mga_vga_calculate_mode_bandwidth(mode, bpp) - > (55000 * 1024)) - return MODE_BANDWIDTH; } } else if (mdev->type == G200_WB) { if (mode->hdisplay > 1280) return MODE_VIRTUAL_X; if (mode->vdisplay > 1024) return MODE_VIRTUAL_Y; - if (mga_vga_calculate_mode_bandwidth(mode, bpp) > - (31877 * 1024)) - return MODE_BANDWIDTH; - } else if (mdev->type == G200_EV && - (mga_vga_calculate_mode_bandwidth(mode, bpp) - > (32700 * 1024))) { - return MODE_BANDWIDTH; - } else if (mdev->type == G200_EH && - (mga_vga_calculate_mode_bandwidth(mode, bpp) - > (37500 * 1024))) { - return MODE_BANDWIDTH; - } else if (mdev->type == G200_ER && - (mga_vga_calculate_mode_bandwidth(mode, - bpp) > (55000 * 1024))) { - return MODE_BANDWIDTH; } if ((mode->hdisplay % 8) != 0 || (mode->hsync_start % 8) != 0 ||@@ -775,30 +739,6 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,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 = mga_vga_mode_valid, -}; - -static const struct drm_connector_funcs mga_vga_connector_funcs = { - .reset = drm_atomic_helper_connector_reset, - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup,- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,- .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - -/* - * Simple Display Pipe - */ - -static enum drm_mode_status-mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,- const struct drm_display_mode *mode) -{ - return MODE_OK; -} - static voidmgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb,struct drm_rect *clip, const struct iosys_map *map)@@ -1009,6 +949,31 @@ 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) {@@ -1024,6 +989,33 @@ static enum drm_mode_status mgag200_mode_config_mode_valid(struct drm_device *deif (fbpages > max_fbpages) return MODE_MEM; + if (IS_G200_SE(mdev)) { + u32 unique_rev_id = mdev->model.g200se.unique_rev_id; + + if (unique_rev_id == 0x01) {+ if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (24400 * 1024))+ return MODE_BAD; + } else if (unique_rev_id == 0x02) {+ if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (30100 * 1024))+ return MODE_BAD; + } else {+ if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (55000 * 1024))+ return MODE_BAD; + } + } else if (mdev->type == G200_WB) {+ if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (31877 * 1024))+ return MODE_BAD; + } else if (mdev->type == G200_EV) {+ if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (32700 * 1024))+ return MODE_BAD; + } else if (mdev->type == G200_EH) {+ if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (37500 * 1024))+ return MODE_BAD; + } else if (mdev->type == G200_ER) {+ if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (55000 * 1024))+ return MODE_BAD; + } + return MODE_OK; }One suggestion to avoid too much repetition: static int mgag200_get_bandwidth_kbps(mdev) { if (IS_G200_SE(mdev)) { u32 unique_rev_id = mdev->model.g200se.unique_rev_id; if (unique_rev_id == 0x01) { return 24400; } else if (unique_rev_id == 0x02) { return 30100; ... } else if (mdev->type == G200_ER) { return 55000; } /* No bandwidth defined */ return 0; } then in mgag200_mode_config_mode_valid() int g200_bandwidth = mgag200_get_bandwidth_kbps(mdev);if (g200_bandwidth && mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > g200_bandwidth * 1024)return MODE_BAD; I've also tested this patchset, and have seen no regression. you can add Reviewed-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx> Tested-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx> for the whole series.
Great! Thank you so much.Since posting this patchset, I talked to Adam Jackson about why G200SE sometimes prefers 16-bit color depth. [1] He told me that some very early revisions of the G200SE had only 1.75 MiB. Reducing the bpp allowed them to run at 1024x768. He mentioned that this was required to pass some sort of conformance test.
As patch 6/7 now hardcodes a requirement for all modes to support 4 bpp, these old devices are stuck at 640x480. That should still be enough for a server. I don't expect that anyone really cares any longer, but if it ever comes up we can relax the requirement again. I'll mention that in the commit message before merging the patch.
I'll also wait for your patches to land. So you won't run into merge conflicts.
Best regards Thomas[1] https://elixir.bootlin.com/linux/v5.17.6/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L1053
-- 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:
OpenPGP_signature
Description: OpenPGP digital signature
