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

[Pauli Nieminen]

On 12/11/10 18:06 +0100, ext Francisco Jerez wrote:
> Pauli Nieminen <ext-pauli.nieminen at nokia.com> writes:
> 
> > 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;
> >
> I don't get the purpose of this change. If I understood correctly,
> DRI2SwapLimit() is supposed to be initiated by the DDX itself based on
> driver-specific policy (e.g. a configuration file option or some
> suitable constant value). What's the point of calling back into the
> driver again to validate its own decision? Am I missing something?
> 

I tried to argue same but the argument was never taken seriously.

> >> 
> >> Other than that,
> >> Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> >> 
> >> -- 
> >> Jesse Barnes, Intel Open Source Technology Center
> > _______________________________________________
> > xorg-devel at lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: http://lists.x.org/mailman/listinfo/xorg-devel