[PATCH v2 5/5] DRI2: Allow DDX to validate swap_limit changes

[Pauli Nieminen]

On 11/11/10 18:46 +0100, ext Jesse Barnes wrote:
> On Mon,  1 Nov 2010 16:22:01 +0200
> Pauli Nieminen <ext-pauli.nieminen at nokia.com> wrote:
> 
> > DDX can now implement validation for swap_limit changes to prevent
> > configurations that are not support in driver.
> > 
> > Signed-off-by: Pauli Nieminen <ext-pauli.nieminen at nokia.com>
> > CC: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
> > ---
> >  hw/xfree86/dri2/dri2.c |   18 +++++++++++++++++-
> >  hw/xfree86/dri2/dri2.h |   14 ++++++++++++++
> >  2 files changed, 31 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> > index 255fed0..7c6a0e2 100644
> > --- a/hw/xfree86/dri2/dri2.c
> > +++ b/hw/xfree86/dri2/dri2.c
> > @@ -102,6 +102,7 @@ typedef struct _DRI2Screen {
> >      DRI2ScheduleWaitMSCProcPtr	 ScheduleWaitMSC;
> >      DRI2AuthMagicProcPtr	 AuthMagic;
> >      DRI2ReuseBufferNotifyProcPtr ReuseBufferNotify;
> > +    DRI2SwapLimitValidateProcPtr SwapLimitValidate;
> >  
> >      HandleExposuresProcPtr       HandleExposures;
> >  
> > @@ -191,13 +192,23 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
> >      return pPriv;
> >  }
> >  
> > +static Bool
> > +DRI2DefaultSwapLimitValidate(DrawablePtr pDraw, int swap_limit)
> > +{
> > +    return swap_limit == 1;
> > +}
> > +
> >  Bool
> >  DRI2SwapLimit(DrawablePtr pDraw, int swap_limit)
> >  {
> >      DRI2DrawablePtr pPriv = DRI2GetDrawable(pDraw);
> > +    DRI2ScreenPtr ds = pPriv->dri2_screen;

Null dereference            ^^
Fixing.

> >      if (!pPriv)
> >  	return FALSE;
> >  
> > +    if (!ds->SwapLimitValidate(pDraw, swap_limit))
> > +	return FALSE;
> > +
> >      pPriv->swap_limit = swap_limit;
> >  
> >      /* Check throttling */
> > @@ -1134,8 +1145,10 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
> >          ds->AuthMagic = info->AuthMagic;
> >      }
> >  
> > -    if (info->version >= 6)
> > +    if (info->version >= 6) {
> >  	ds->ReuseBufferNotify = info->ReuseBufferNotify;
> > +	ds->SwapLimitValidate = info->SwapLimitValidate;
> > +    }
> >  
> >      /*
> >       * if the driver doesn't provide an AuthMagic function or the info struct
> > @@ -1148,6 +1161,9 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
> >          goto err_out;
> >  #endif
> >  
> > +    if (!ds->SwapLimitValidate)
> > +	ds->SwapLimitValidate = DRI2DefaultSwapLimitValidate;
> > +
> >      /* Initialize minor if needed and set to minimum provied by DDX */
> >      if (!dri2_minor || dri2_minor > cur_minor)
> >  	dri2_minor = cur_minor;
> > diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> > index 3d01c55..a4bba02 100644
> > --- a/hw/xfree86/dri2/dri2.h
> > +++ b/hw/xfree86/dri2/dri2.h
> > @@ -169,6 +169,19 @@ typedef void		(*DRI2InvalidateProcPtr)(DrawablePtr pDraw,
> >  						 void *data);
> >  
> >  /**
> > + * DRI2 calls this hook when ever swap_limit is going to be changed. Default
> > + * implementation for the hook only accepts one as swap_limit. If driver can
> > + * support other swap_limits it has to implement supported limits with this
> > + * callback.
> > + *
> > + * \param pDraw drawable whos swap_limit is going to be changed
> > + * \param swap_limit new swap_limit that going to be set
> > + * \return TRUE if limit is support, FALSE if not.
> > + */
> > +typedef Bool		(*DRI2SwapLimitValidateProcPtr)(DrawablePtr pDraw,
> > +							int swap_limit);
> > +
> > +/**
> >   * Version of the DRI2InfoRec structure defined in this header
> >   */
> >  #define DRI2INFOREC_VERSION 6
> > @@ -203,6 +216,7 @@ typedef struct {
> >      /* added in version 6 */
> >  
> >      DRI2ReuseBufferNotifyProcPtr ReuseBufferNotify;
> > +    DRI2SwapLimitValidateProcPtr SwapLimitValidate;
> >  }  DRI2InfoRec, *DRI2InfoPtr;
> >  
> >  extern _X_EXPORT int DRI2EventBase;
> 
> This is to validate N buffering on the driver side, right?  I'd really
> prefer drivers to just implement all the hooks, even if they're only
> stubs, since that avoids some complexity on the server side and makes
> the code easier to follow (applies to the GetMSC patch from before as
> well).
> 
> But I don't feel strongly enough to nack on those grounds, I'll leave
> that up to Keith.

Would following change look better to you?

if (!ds->SwapLimitValidate || !ds->SwapLimitValidate(pDraw, swap_limit)
	return FALSE;

> 
> Other than that,
> Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center