[Intel-gfx] [PATCH] Xv: free tearing on textured video

[Xiang, Haihao]

On Fri, 2009-02-27 at 05:36 +0800, Dan Nicholson wrote:
> On Thu, Feb 26, 2009 at 12:30 AM, Xiang, Haihao <haihao.xiang at intel.com> wrote:
> > On Thu, 2009-02-26 at 09:07 +0800, Zhenyu Wang wrote:
> >> On 2009.02.26 00:30:19 +0800, Keith Packard wrote:
> >> > On Wed, 2009-02-25 at 15:56 +0800, Zou, Nanhai wrote:
> >> >
> >> > > I suggest turn this option default to be true or not use an option at all.
> >> > > We already have too many options to handle.
> >> >
> >> > Ideally, this code would do the right thing automatically, and be
> >> > configured when it 'automatic' wasn't working right. Nanhai's suggestion
> >> > of turning it on by default is a good one, as is Dave's suggestion of
> >> > using an Xv attribute instead of a config file option.
> >> >
> >> > I'd suggest one further change -- only sync when the video is 'large',
> >> > so that videos which are only a small part of the screen don't impact
> >> > other applications running at the same time.
> >> >
> >> > How about an Xv attribute which has three values 'on' 'off' and 'auto'?
> >> > 'on' means always sync, no matter what size, 'off' means never sync and
> >> > 'auto' means sync if the Xv image is more than half(?) of the pixels on
> >> > the screen. Then we set the default to 'auto'. Users can adjust the
> >> > behaviour of the driver using 'xvattr'.
> >> >
> >> > One problem with this plan is that 'xvattr' adjusts the attributes on a
> >> > single port, and as we offer 16 textured video ports (along with 1
> >> > overlay port on hardware with an overlay), the user would need to call
> >> > xvattr up to 17 times to effect the change uniformly. I'd say we should
> >> > just fix 'xvattr' to add a '-p all' option to adjust a parameter for all
> >> > ports?
> >> >
> >>
> >> Looks a good plan. I just checked spec for MI_WAIT_EVENT, it looks for newer
> >> G45s some definition might have been changed. Haihao may try it and possibly
> >> update the patch.
> >
> > Here is an update. I just test it on GM965 and G45 and MI_WAIT_EVENT works
> > for GM965 and G45.
> >
> > Add an Xv attribute XV_VSYNC which has three values -1 (off), 0 (auto)
> > and 1 (on) to control whether textured adapter synchronizes the screen
> > update to the vblank. The default value is 0.
> > ---
> >  src/i810_reg.h   |    4 ++
> >  src/i830_video.c |   97 ++++++++++++++++++++++++++++++++++++++++++++----------
> >  src/i830_video.h |    2 +
> >  3 files changed, 85 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/i810_reg.h b/src/i810_reg.h
> > index e2ffba1..cd7a634 100644
> > --- a/src/i810_reg.h
> > +++ b/src/i810_reg.h
> > @@ -2432,7 +2432,11 @@ SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> >
> >  /* Wait for Events */
> >  #define MI_WAIT_FOR_EVENT              (0x03<<23)
> > +#define MI_WAIT_FOR_PIPEB_SVBLANK      (1<<18)
> > +#define MI_WAIT_FOR_PIPEA_SVBLANK      (1<<17)
> >  #define MI_WAIT_FOR_OVERLAY_FLIP       (1<<16)
> > +#define MI_WAIT_FOR_PIPEB_VBLANK       (1<<7)
> > +#define MI_WAIT_FOR_PIPEA_VBLANK       (1<<3)
> >
> >  /* Flush */
> >  #define MI_FLUSH                       (0x04<<23)
> > diff --git a/src/i830_video.c b/src/i830_video.c
> > index cdb1072..bf359bc 100644
> > --- a/src/i830_video.c
> > +++ b/src/i830_video.c
> > @@ -117,6 +117,7 @@ static int I830QueryImageAttributesTextured(ScrnInfoPtr, int, unsigned short *,
> >
> >  static Atom xvBrightness, xvContrast, xvSaturation, xvColorKey, xvPipe, xvDoubleBuffer;
> >  static Atom xvGamma0, xvGamma1, xvGamma2, xvGamma3, xvGamma4, xvGamma5;
> > +static Atom xvVsync;
> >
> >  /* Limits for the overlay/textured video source sizes.  The documented hardware
> >  * limits are 2048x2048 or better for overlay and both of our textured video
> > @@ -247,10 +248,11 @@ static XF86AttributeRec Attributes[NUM_ATTRIBUTES] = {
> >     {XvSettable | XvGettable, 0, 1, "XV_DOUBLE_BUFFER"}
> >  };
> >
> > -#define NUM_TEXTURED_ATTRIBUTES 2
> > +#define NUM_TEXTURED_ATTRIBUTES 3
> >  static XF86AttributeRec TexturedAttributes[NUM_ATTRIBUTES] = {
> >     {XvSettable | XvGettable, -128, 127, "XV_BRIGHTNESS"},
> >     {XvSettable | XvGettable, 0, 255, "XV_CONTRAST"},
> > +    {XvSettable | XvGettable, -1, 1, "XV_VSYNC"},
> >  };
> 
> Maybe I'm being dense, but shouldn't the size of the
> TexturedAttributes array be NUM_TEXTURED_ATTRIBUTES instead of
> NUM_ATTRIBUTES? I'd imagine you would have received "warning: excess
> elements in array initializer" when building.
> NUM_TEXTURED_ATTRIBUTES
NUM_ATTRIBUTES is 5, so it is right. Of course
TexturedAttributes[NUM_TEXTURED_ATTRIBUTES] is more readable.

Thanks
Haihao
> --
> Dan