[PATCH] drm/qxl: validate monitors config modes

[Dave Airlie]

> Due to some recent changes in
> drm_helper_probe_single_connector_modes_merge_bits(), old custom modes
> were not being pruned properly. In current kernels,
> drm_mode_validate_basic() is called to sanity-check each mode in the
> list. If the sanity-check passes, the mode's status gets set to to
> MODE_OK. In older kernels this check was not done, so old custom modes
> would still have a status of MODE_UNVERIFIED at this point, and would
> therefore be pruned later in the function.
>
> As a result of this new behavior, the list of modes for a device always
> includes every custom mode ever configured for the device, with the
> largest one listed first. Since desktop environments usually choose the
> first preferred mode when a hotplug event is emitted, this had the
> result of making it very difficult for the user to reduce the size of
> the display.
>
> The qxl driver did implement the mode_valid connector function, but it
> was empty. In order to restore the old behavior where old custom modes
> are pruned, we implement a proper mode_valid function for the qxl
> driver. This function now checks each mode against the last configured
> custom mode and the list of standard modes. If the mode doesn't match
> any of these, its status is set to MODE_BAD so that it will be pruned as
> expected.

Hi Jonathon,

this seems reasonable, it is missing a Signed-off-by line though, and we should
probably also cc stable for it.

Dave.
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 66 ++++++++++++++++++++++++---------------
>  drivers/gpu/drm/qxl/qxl_drv.h     |  2 ++
>  2 files changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index a8dbb3e..7c6225c 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -160,9 +160,35 @@ static int qxl_add_monitors_config_modes(struct drm_connector *connector,
>         *pwidth = head->width;
>         *pheight = head->height;
>         drm_mode_probed_add(connector, mode);
> +       /* remember the last custom size for mode validation */
> +       qdev->monitors_config_width = mode->hdisplay;
> +       qdev->monitors_config_height = mode->vdisplay;
>         return 1;
>  }
>
> +static struct mode_size {
> +       int w;
> +       int h;
> +} common_modes[] = {
> +       { 640,  480},
> +       { 720,  480},
> +       { 800,  600},
> +       { 848,  480},
> +       {1024,  768},
> +       {1152,  768},
> +       {1280,  720},
> +       {1280,  800},
> +       {1280,  854},
> +       {1280,  960},
> +       {1280, 1024},
> +       {1440,  900},
> +       {1400, 1050},
> +       {1680, 1050},
> +       {1600, 1200},
> +       {1920, 1080},
> +       {1920, 1200}
> +};
> +
>  static int qxl_add_common_modes(struct drm_connector *connector,
>                                  unsigned pwidth,
>                                  unsigned pheight)
> @@ -170,29 +196,6 @@ static int qxl_add_common_modes(struct drm_connector *connector,
>         struct drm_device *dev = connector->dev;
>         struct drm_display_mode *mode = NULL;
>         int i;
> -       struct mode_size {
> -               int w;
> -               int h;
> -       } common_modes[] = {
> -               { 640,  480},
> -               { 720,  480},
> -               { 800,  600},
> -               { 848,  480},
> -               {1024,  768},
> -               {1152,  768},
> -               {1280,  720},
> -               {1280,  800},
> -               {1280,  854},
> -               {1280,  960},
> -               {1280, 1024},
> -               {1440,  900},
> -               {1400, 1050},
> -               {1680, 1050},
> -               {1600, 1200},
> -               {1920, 1080},
> -               {1920, 1200}
> -       };
> -
>         for (i = 0; i < ARRAY_SIZE(common_modes); i++) {
>                 mode = drm_cvt_mode(dev, common_modes[i].w, common_modes[i].h,
>                                     60, false, false, false);
> @@ -823,11 +826,22 @@ static int qxl_conn_get_modes(struct drm_connector *connector)
>  static int qxl_conn_mode_valid(struct drm_connector *connector,
>                                struct drm_display_mode *mode)
>  {
> +       struct drm_device *ddev = connector->dev;
> +       struct qxl_device *qdev = ddev->dev_private;
> +       int i;
> +
>         /* TODO: is this called for user defined modes? (xrandr --add-mode)
>          * TODO: check that the mode fits in the framebuffer */
> -       DRM_DEBUG("%s: %dx%d status=%d\n", mode->name, mode->hdisplay,
> -                 mode->vdisplay, mode->status);
> -       return MODE_OK;
> +
> +       if(qdev->monitors_config_width == mode->hdisplay &&
> +          qdev->monitors_config_height == mode->vdisplay)
> +               return MODE_OK;
> +
> +       for (i = 0; i < ARRAY_SIZE(common_modes); i++) {
> +               if (common_modes[i].w == mode->hdisplay && common_modes[i].h == mode->vdisplay)
> +                       return MODE_OK;
> +       }
> +       return MODE_BAD;
>  }
>
>  static struct drm_encoder *qxl_best_encoder(struct drm_connector *connector)
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index d854969..01a8694 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -325,6 +325,8 @@ struct qxl_device {
>         struct work_struct fb_work;
>
>         struct drm_property *hotplug_mode_update_property;
> +       int monitors_config_width;
> +       int monitors_config_height;
>  };
>
>  /* forward declaration for QXL_INFO_IO */
> --
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel