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

[Francisco Jerez]

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?

>> 
>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20101112/78977f10/attachment.pgp>