Re: [PATCH] drm/gma500: Fixup fbdev stolen size usage evaluation

[Patrik Jakobsson]

On Thu, Nov 7, 2019 at 4:30 PM Paul Kocialkowski
<paul.kocialkowski@xxxxxxxxxxx> wrote:
>
> psbfb_probe performs an evaluation of the required size from the stolen
> GTT memory, but gets it wrong in two distinct ways:
> - The resulting size must be page-size-aligned;
> - The size to allocate is derived from the surface dimensions, not the fb
>   dimensions.
>
> When two connectors are connected with different modes, the smallest will
> be stored in the fb dimensions, but the size that needs to be allocated must
> match the largest (surface) dimensions. This is what is used in the actual
> allocation code.
>
> Fix this by correcting the evaluation to conform to the two points above.
> It allows correctly switching to 16bpp when one connector is e.g. 1920x1080
> and the other is 1024x768.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/gma500/framebuffer.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 218f3bb15276..90237abee088 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -462,6 +462,7 @@ static int psbfb_probe(struct drm_fb_helper *helper,
>                 container_of(helper, struct psb_fbdev, psb_fb_helper);
>         struct drm_device *dev = psb_fbdev->psb_fb_helper.dev;
>         struct drm_psb_private *dev_priv = dev->dev_private;
> +       unsigned int fb_size;
>         int bytespp;
>
>         bytespp = sizes->surface_bpp / 8;
> @@ -471,8 +472,11 @@ static int psbfb_probe(struct drm_fb_helper *helper,
>         /* If the mode will not fit in 32bit then switch to 16bit to get
>            a console on full resolution. The X mode setting server will
>            allocate its own 32bit GEM framebuffer */
> -       if (ALIGN(sizes->fb_width * bytespp, 64) * sizes->fb_height >
> -                       dev_priv->vram_stolen_size) {
> +       fb_size = ALIGN(sizes->surface_width * bytespp, 64) *
> +                 sizes->surface_height;
> +       fb_size = ALIGN(fb_size, PAGE_SIZE);
> +
> +       if (fb_size > dev_priv->vram_stolen_size) {

psb_gtt_alloc_range() already aligns by PAGE_SIZE for us. Looks like
we align a couple of times extra for luck. This needs cleaning up
instead of adding even more aligns.

Your size calculation looks correct and indeed makes my 1024x600 +
1920x1080 setup actually display something, but for some reason I get
an incorrect panning on the smaller screen and stale data on the
surface only visible by the larger CRTC. Any idea what's going on?

>                  sizes->surface_bpp = 16;
>                  sizes->surface_depth = 16;
>          }
> --
> 2.23.0
>