Message-ID: <20191112155012.GE4506@aptenodytes>
Date:   Tue, 12 Nov 2019 16:50:12 +0100
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Patrik Jakobsson <patrik.r.jakobsson@...il.com>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        James Hilliard <james.hilliard1@...il.com>
Subject: Re: [PATCH] drm/gma500: Fixup fbdev stolen size usage evaluation

Hi,

On Tue 12 Nov 19, 16:11, Paul Kocialkowski wrote:
> Hi,
> 
> On Tue 12 Nov 19, 11:20, Patrik Jakobsson wrote:
> > On Thu, Nov 7, 2019 at 4:30 PM Paul Kocialkowski
> > <paul.kocialkowski@...tlin.com> 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@...tlin.com>
> > > ---
> > >  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.
> 
> I'm not sure this is really for luck. As far as I can see, we need to do it
> properly for this size estimation since it's the final size that will be
> allocated (and thus needs to be available in whole).
> 
> For the other times there is explicit alignment, they seem justified too:
> - in psb_gem_create: it is common to pass the aligned size when creating the
>   associated GEM object with drm_gem_object_init, even though it's probably not
>   crucial given that this is not where allocation actually happens;
> - in psbfb_create: the full size is apparently only really used to memset 0
>   the allocated buffer. I think this makes sense for security reasons (and not
>   leak previous contents in the additional space required for alignment).
> 
> What strikes me however is that each call to psb_gtt_alloc_range takes the
> alignment as a parameter when it's really always PAGE_SIZE, so it should
> probably just be hardcoded in the call to allocate_resource.
> 
> What do you think?
> 
> > 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?
> 
> I'm not seeing this immediately, but I definitely have something strange
> after having printed more lines than the smallest display can handle or
> scrolling, where more than the actual size of the fb is used.
> 
> Maybe this is related to using the PowerVR-accelerated fb ops, that aren't
> quite ready for this use case?
> 
> I'll give it a try with psbfb_roll_ops and psbfb_unaccel_ops instead to see
> if it changes something for me. Maybe it would help for you too?

Some quick feedback about that:
- psbfb_unaccel_ops gives a correct result where the scrolling area is bound
  to the smallest display;
- psbfb_roll_ops gives a working scrolling but bound to the largest display
  (so the current shell line becomes invisible on the smallest one eventually);
- psbfb_ops gives the same issue as above and seems to add artifacts on top.

There's probably limited interest in working on that aspect on our side though.
I'd be interested to know if it affects the issue you're seeing though.

Cheers,

Paul

> I suspect that the generic implementation is already bullet-proof for these
> kinds of use case.
> 
> Cheers and thanks for the feedback,
> 
> Paul
> 
> > 
> > >                  sizes->surface_bpp = 16;
> > >                  sizes->surface_depth = 16;
> > >          }
> > > --
> > > 2.23.0
> > >
> 
> -- 
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com



-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists