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 <tzimmerm...@suse.de>
---
  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.c
index 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 *connecto
        return 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 void
  mgag200_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 *de
        if (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 <jfale...@redhat.com>
Tested-by: Jocelyn Falempe <jfale...@redhat.com>

for the whole series.

--

Jocelyn




Reply via email to