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

[Dan Nicholson]

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.

--
Dan