[PATCH xf86-video-ati 2/2] kms: Move flip_count and co. to a per swap structure

[Ville Syrjälä]

On Thu, Jun 09, 2011 at 08:08:24PM -0700, Stéphane Marchesin wrote:
> On Wed, May 4, 2011 at 13:51, Ville Syrjala <syrjala at sci.fi> wrote:
> > If multiple drawables are doing page flipping, the global drmmode
> > structure can't be used to keep per swap information. For example
> > flip_count can increase prematurely due to another swap request,
> > and then the previous swap request never gets completed, leading to a
> > stuck client. Move the relevant pieces of data to a strucuture that
> > gets allocated once per swap request and shared by all involved CRTCs.
> >
> > Signed-off-by: Ville Syrjala <syrjala at sci.fi>
> > ---
> >  src/drmmode_display.c |   45 ++++++++++++++++++++++++++-------------------
> >  src/drmmode_display.h |   10 +++++++---
> >  2 files changed, 33 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> > index 7873d57..7dd5d86 100644
> > --- a/src/drmmode_display.c
> > +++ b/src/drmmode_display.c
> > @@ -1331,31 +1331,34 @@ drmmode_flip_handler(int fd, unsigned int frame, unsigned int tv_sec,
> >                     unsigned int tv_usec, void *event_data)
> >  {
> >        drmmode_flipevtcarrier_ptr flipcarrier = event_data;
> > -       drmmode_ptr drmmode = flipcarrier->drmmode;
> > +       drmmode_flipdata_ptr flipdata = flipcarrier->flipdata;
> > +       drmmode_ptr drmmode = flipdata->drmmode;
> >
> >        /* Is this the event whose info shall be delivered to higher level? */
> >        if (flipcarrier->dispatch_me) {
> >                /* Yes: Cache msc, ust for later delivery. */
> > -               drmmode->fe_frame = frame;
> > -               drmmode->fe_tv_sec = tv_sec;
> > -               drmmode->fe_tv_usec = tv_usec;
> > +               flipdata->fe_frame = frame;
> > +               flipdata->fe_tv_sec = tv_sec;
> > +               flipdata->fe_tv_usec = tv_usec;
> >        }
> >        free(flipcarrier);
> >
> >        /* Last crtc completed flip? */
> > -       drmmode->flip_count--;
> > -       if (drmmode->flip_count > 0)
> > +       flipdata->flip_count--;
> > +       if (flipdata->flip_count > 0)
> >                return;
> >
> >        /* Release framebuffer */
> > -       drmModeRmFB(drmmode->fd, drmmode->old_fb_id);
> > +       drmModeRmFB(drmmode->fd, flipdata->old_fb_id);
> >
> > -       if (drmmode->event_data == NULL)
> > +       if (flipdata->event_data == NULL)
> >                return;
> >
> >        /* Deliver cached msc, ust from reference crtc to flip event handler */
> > -       radeon_dri2_flip_event_handler(drmmode->fe_frame, drmmode->fe_tv_sec,
> > -                                      drmmode->fe_tv_usec, drmmode->event_data);
> > +       radeon_dri2_flip_event_handler(flipdata->fe_frame, flipdata->fe_tv_sec,
> > +                                      flipdata->fe_tv_usec, flipdata->event_data);
> > +
> > +       free(flipdata);
> >  }
> >
> >
> > @@ -1399,12 +1402,10 @@ Bool drmmode_pre_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int cpp)
> >
> >        xf86InitialConfiguration(pScrn, TRUE);
> >
> > -       drmmode->flip_count = 0;
> >        drmmode->event_context.version = DRM_EVENT_CONTEXT_VERSION;
> >        drmmode->event_context.vblank_handler = drmmode_vblank_handler;
> >        drmmode->event_context.page_flip_handler = drmmode_flip_handler;
> >        if (!pRADEONEnt->fd_wakeup_registered && info->dri->pKernelDRMVersion->version_minor >= 4) {
> > -               drmmode->flip_count = 0;
> >                AddGeneralSocket(drmmode->fd);
> >                RegisterBlockAndWakeupHandlers((BlockHandlerProcPtr)NoopDDA,
> >                                drm_wakeup_handler, drmmode);
> > @@ -1654,6 +1655,7 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, struct radeon_bo *new_front, void *dat
> >        int i, old_fb_id;
> >        uint32_t tiling_flags = 0;
> >        int height;
> > +       drmmode_flipdata_ptr flipdata;
> >        drmmode_flipevtcarrier_ptr flipcarrier;
> >
> >        if (info->allowColorTiling) {
> > @@ -1676,6 +1678,12 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, struct radeon_bo *new_front, void *dat
> >                         new_front->handle, &drmmode->fb_id))
> >                goto error_out;
> >
> > +        flipdata = calloc(1, sizeof(drmmode_flipdata_rec));
> 
> I think you leak that flipdata in the error_undo case.

Quite possible. The code already seemed to leak some stuff on
errors, and it didn't really seem to have proper roll back code
anyway in case of partial success, so I didn't pay much attention
to such details.

-- 
Ville Syrjälä
syrjala at sci.fi
http://www.sci.fi/~syrjala/