Tracking down bug #12503, looking for comment on my theory

[Alex Deucher]

On Jan 27, 2008 9:30 PM, Alex Villacís Lasso <a_villacis at palosanto.com> wrote:
> > I'm thinking we could pad the size out to 16 pixels and then use the
> > scissors (if necessary) to clip the final image.
> >
> > Alex
> >
>
> I don't quite follow the comment about the sissors. Could you please
> elaborate? There is no support, as far as I can see, for using just a
> portion of the source pitch for the secondary stream.

I just took a look at the documentation and the scissors aren't
relevant here.  Scissors are part of the 2D engine used to clip an
operation to a specific rectangle.

>
> I resend the patch for the multiples of 16 in BCI, along with three other
> patches that solve various issues with XVideo.
>
> 0001-BCI-can-only-handle-widths-that-are-multiple-of-16.patch
>
> Already explained in a previous mail, this patch prevents a lockup on an
> attempt to use the BCI to transform a frame with a non-multiple of 16
> pixels.
>

I'm pretty sure this should work somehow, but I don't have the time to
look into now, so I'll go ahead and apply your patch.

> 0002-Prevent-use-of-BCI-for-YV12-YUY2-conversion-from.patch
>
> This patch fixes an issue I found when toying with the idea of
> implementing a double-buffer for XVideo. When displaying an image in
> SavagePutImage(), the driver allocates video memory of a size of new_size
> = dstPitch * height. If the driver chooses to use the BCI for YV12 -> YUY2
> conversion, the function SavageCopyPlanarDataBCI() builds a pointer to an
> intermediate area to hold the data of the planar format, at an offset of 2
> * srcPitch * h (height) from the start of the allocated video buffer. From
> previous calculations in SavagePutImage(), srcPitch roughly equals
> dstPitch/2 (because dstPitch is calculated from width << 1). Therefore,
> the temporary area in the BCI case is approximately equal to an offset of
> dstPitch * height. However, the BCI case writes (srcPitch + srcPitch2) *
> height bytes to this area, past the allocated size of dstPitch * height.
> Therefore, the BCI case corrupts whatever happens to be lying past the end
> of the buffer (for example, offscreen pixmaps). This patch prevents this
> by including the required area for BCI planar data in the allocation of
> video memory. The condition for this depends on patch 1 (no BCI for
> multiples of 16 pixels) being already applied.

looks good.

>
> 0003-Reset-lastKnownPitch-to-0-right-after-enabling-strea.patch
>
> In SavageDisplayVideoOld() and SavageDisplayVideoNew(), the pitch must be
> programmed in CRTC index 0x92 and 0x93 for optimal display speed of the
> YUY2 stream. This register is programmed whenever the last known pitch (in
> pPriv->lastKnownPitch) changes from the current pitch. The problem is that
> this cached value is never flushed on video shutdown, but the register
> seems to be reset when shutting down the streams. If the next movie
> happens to have the exact same pitch as a previously shown one, the
> register is never programmed, resulting in a significant slowdown, most
> noticeable when displaying as fullscreen, and snow (when fullscreen). This
> patch assigns 0 to the last known pitch right after turning on the
> streams, so any previous value is flushed, and the register is programmed
> anew. This fixes the issue of snow and video slowdowns on repeated
> displays of the same movie in MPlayer.

good catch.

>
> 0004-Do-not-wait-for-vertical-retrace-anymore-on-old-stre.patch
>
> The old streams engine includes a wait for the vertical retrace. This wait
> is the single most expensive operation in SavageDisplayVideoOld().
> Removing the wait does not seem to cause much shearing (almost none in my
> case), and significantly speeds up the video display for MPlayer and
> Totem.

I think that was required for some reason, but I don't recall what.
If it works ok for you, I'll apply it.

Alex