<div dir="ltr"><div>Hi,<br><br></div><div>Thx for the comments. I forgot to mention that there is nothing really new in these patches.<br></div><div>All the low level code relative to nouveau was already there.<br></div><div>The patches are almost "just" moving code in order to call nouveau_vp3_bsp and nvc0_decoder_bsp<br></div><div>multiple times between each begin/end frame. But any refactoring is a source of regressions so the "just" may be not that trivial :)<br><br></div><div>I made sure that vdpau was still working. Actually it was more than that, without vdpau I think I would not have been able to do this refactoring. I also compared that for one begin/end frame the nouveau_bo buffer contains the same data for vdpau and vaapi. I dumped them for the same stream and several seconds and there were all the same.<br></div><div><br></div>Would it be ok if I squash the patches like this (4 patches instead of 8):<br><br><u>A: Common to nvc0 and nv98:</u><br> nouveau: extract memcpy loop from nouveau_vp3_bsp<br>nouveau: remove nouveau_vp3_bsp to use begin/next/end<br><br><u>B: Only for nvc0 but same split can be applied to nv98:</u><br>+nvc0(-nouveau): split nvc0_decoder_bsp in begin/next/end<br>+nvc0(-nouveau): remove nvc0_decoder_bsp and use begin/next/end instead<br>+nvc0(-nouveau): preserve content buffer when calling nvc0_decoder_bsp_next<br><div>nvc0: implement pipe_video_codec::begin_frame/end_frame<br><br><u>C: Common to nvc0 and nv98:</u><br>nouveau: fix chunk decoding by updating number of slices<br><br><u>D: Common to nvc0 and nv98:</u><br>build: enable st/va with nouveau driver<br><br></div><div>I can still squash A and B, no pb. Let me know.<br></div><div>Also should I use <span>"--in-reply-to" or should I submit a new patch set since some will be squashed ?<br></span></div><div><br></div><div>Cheers<br></div><div>Julien<br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 27 August 2015 at 22:36, Emil Velikov <span dir="ltr"><<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 27 August 2015 at 18:35, Ilia Mirkin <<a href="mailto:imirkin@alum.mit.edu">imirkin@alum.mit.edu</a>> wrote:<br>
> On Thu, Aug 27, 2015 at 1:11 PM, Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>> wrote:<br>
>> HI Julien,<br>
>><br>
>> On 27 August 2015 at 15:15, Julien Isorce <<a href="mailto:j.isorce@samsung.com">j.isorce@samsung.com</a>> wrote:<br>
>>> Currently nouveau does not support chunk decoding<br>
>>> which is required to support st/va<br>
>>><br>
>>> The following patches refactor nouveau_vp3_bsp<br>
>>> and nvc0_decoder_bsp in order to implement<br>
>>> pipe_video_codec::begin_frame/decode_bitstream/end_frame.<br>
>>> So that decode_bitstream can be call multiple times<br>
>>> between each begin/end.<br>
>>><br>
>>> TODO: apply same logic for nv98 but I do not have the<br>
>>> material to test it.<br>
>>><br>
>>> <a href="https://bugs.freedesktop.org/show_bug.cgi?id=89969" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=89969</a><br>
>>><br>
>>> Julien Isorce (8):<br>
>>>   nouveau: extract memcpy loop from nouveau_vp3_bsp<br>
>>>   nouveau: remove nouveau_vp3_bsp to use begin/next/end<br>
>>>   nouveau: split nvc0_decoder_bsp in begin/next/end<br>
>>>   nouveau: preserve content buffer when calling nvc0_decoder_bsp_next<br>
>>>   nouveau: remove nvc0_decoder_bsp and use begin/next/end instead<br>
>>>   nvc0: implement pipe_video_codec::begin_frame/end_frame<br>
>>>   nouveau: fix chunk decoding by updating number of slices<br>
>>>   build: enable st/va with nouveau driver<br>
>>><br>
>> Glad to hear that you've got vaapi working. Did you check that vdpau<br>
>> did not regress ?<br>
>><br>
>> I'm wondering if it's going to be better if you gradually re-factor<br>
>> the existing code. The current "1. add 'new' implementation (incl.<br>
>> variables we don't want/need until halfway through), 2. remove 'old'<br>
>> implementation" approach does not seem too appealing imho.<br>
>><br>
>> Others might disagree with my suggestion, though :)<br>
><br>
> I wholeheartedly agree... the current patch series makes it very<br>
> difficult to verify that you're doing stuff "right" since I have to<br>
> look at the "add" and "remove" patches at once. Also your "preserve<br>
> content buffer" thing should probably be folded into the previous<br>
> patch, since that's integral to what those functions do. I realize<br>
> that in the end it'll end up creating just one or two mega-patches,<br>
> but I'd rather have that than the functionality scattered all over.<br>
><br>
</div></div>Actually it seems that one can get away without big patches.<br>
Two begin/next/end combos = 6 patches, a cleanup/fix or two plus the<br>
enable patch.<br>
<span class=""><br>
> You don't have to worry about nv98_video if you don't want to, I can<br>
> take care of fixing that up afterwards, but do try to leave as much<br>
> functionality in nouveau_vp3 as possible. (Perhaps you've done that, I<br>
> haven't looked in great detail yet.)<br>
><br>
</span>From a quick look all the nouveau_vp3 code is still there.<br>
<span class="HOEnZb"><font color="#888888"><br>
-Emil<br>
</font></span><div class="HOEnZb"><div class="h5">_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></div><br></div>