[Libva] [PATCH intel-driver 1/4] decoder: h264: don't deallocate surface storage of older frames.

[Gwenole Beauchesne]

Hi,

2014-05-12 8:17 GMT+02:00 Zhao, Yakui <yakui.zhao at intel.com>:
> On Sun, 2014-05-11 at 23:31 -0600, Gwenole Beauchesne wrote:
>> Hi,
>>
>> 2014-05-12 7:21 GMT+02:00 Zhao, Yakui <yakui.zhao at intel.com>:
>> > On Sun, 2014-05-11 at 22:35 -0600, Gwenole Beauchesne wrote:
>> >> Hi,
>> >>
>> >> 2014-05-12 2:55 GMT+02:00 Zhao Yakui <yakui.zhao at intel.com>:
>> >> > On Fri, 2014-05-09 at 08:34 -0600, Gwenole Beauchesne wrote:
>> >> >> Drop the optimization whereby surfaces that are no longer marked as
>> >> >> reference and that were already displayed are to be destroyed. This
>> >> >> is wrong mainly for two reasons:
>> >> >>
>> >> >> 1. The surface was displayed... once but it may still be needed for
>> >> >>    subsequent operations like displaying it again, using it for a
>> >> >>    transcode pipeline (encode) for instance, etc.
>> >> >>
>> >> >> 2. The new set of ReferenceFrames[] correspond to the active set of
>> >> >>    reference frames used for decoding the current slice. In presence
>> >> >>    of Multiview Coding (MVC), that could correspond to the current
>> >> >>    view, in view order index, but the surface may still be needed
>> >> >>    for decoding the next view with the same view_id, while also
>> >> >>    decoding other views with another set of reference frames for them.
>> >> >
>> >> > Hi, Gwenole
>> >> >
>> >> >     Thanks for working on the h264 reference frame list. And I have
>> >> > some concerns about the patch set:
>> >> >     1. When will the unused reference surface be destroyed?
>> >>
>> >> As it ought to be: at vaDestroySurfaces() time.
>> >>
>> >> >     2. How to find one free slot in decode_state->reference_surfaces
>> >> > array to store the corresponding reference frame?
>> >>
>> >> It's the same algorithm. :)
>> >> At this time, the reference_surfaces[] contents exactly matches
>> >> ReferenceFrames[], hole for hole, if there is any. The former holds
>> >> object_surface, the latter holds the associated VAPictureH264. i.e.
>> >> pointer to object_surface [i] is really the reference_surface[i] that
>> >> matches VAPictureH264 at ReferenceFrames[i]. Previously, there could
>> >> be a mismatch between both.
>> >
>> > As you know, the current policy in the driver is based on the following
>> > flowchart:
>> >     1. the driver maintains one array for the hardware-requirement.
>> >     2. When one new frame is decoded, it will check whether the array
>> > element  maintained by the driver is still used by the DPB. If not, the
>> > internal data structure will be free and then be used for the later
>> > selection.
>> >     3. Find the free slot in the array to store the reference frames in
>> > DPB.
>> >
>> > But I don't see how this patch set stores/updates the reference frames
>> > of DPB into the driver array and how to decide which slot can be
>> > replaced by the reference frames in new DPB.
>>
>> I don't understand your concern. It works the same as before, but
>> replaces ReferenceFrames[] with reference_surface[], which now hold
>> the same contents, in order. The free_slots[] array is filled in order
>> too so that to avoid the extraneous iterations again to look for the
>> same free slot id.
>
> My point is that you can describe how you find the free slot for the
> later update about the reference frame in DPB.

OK, will do but there is really zero change in the algorithm beyond
the natural and higher order optimization.

>> > It will be better that you can describe the keypoint in the patch
>> > especially when it updates the content required by the hardware.
>>
>> What contents do you think we need to maintain for the hardware? The
>> hardware is designed to be stateless, so it normally doesn't keep
>> anything. This is necessary for context switching performance.
>
> As I said in the previous email, the array of reference surface is one
> content required by the hardware.

I think there was a misunderstanding. The reference_surface[] is not
changed (yet), only the reference_objects[]. That's totally different.
The former is the frame store to maintain for the HW (and internal
info we have in the DMV buffers actually); the latter should be a 1:1
correspondence with VA surface ids in ReferenceFrames[] to the
associated object_surface objects (stored in reference_surface).

I will provide another patch set with a couple more patches:
1. Definitely fix MVC decode use cases ;
2. Optimize the process for Haswell and newer.

(3. if this really interests anyone, I may submit doc fixes and
clarifications if I find time to, but the code will be used as
reference).

Regards,
Gwenole.

>> >> >>  src/i965_decoder_utils.c | 13 -------------
>> >> >>  1 file changed, 13 deletions(-)
>> >> >>
>> >> >> diff --git a/src/i965_decoder_utils.c b/src/i965_decoder_utils.c
>> >> >> index 2533381..9d537ff 100644
>> >> >> --- a/src/i965_decoder_utils.c
>> >> >> +++ b/src/i965_decoder_utils.c
>> >> >> @@ -382,19 +382,6 @@ intel_update_avc_frame_store_index(VADriverContextP ctx,
>> >> >>
>> >> >>          /* remove it from the internal DPB */
>> >> >>          if (!found) {
>> >> >> -            struct object_surface *obj_surface = frame_store[i].obj_surface;
>> >> >> -
>> >> >> -            obj_surface->flags &= ~SURFACE_REFERENCED;
>> >> >> -
>> >> >> -            if ((obj_surface->flags & SURFACE_ALL_MASK) == SURFACE_DISPLAYED) {
>> >> >> -                dri_bo_unreference(obj_surface->bo);
>> >> >> -                obj_surface->bo = NULL;
>> >> >> -                obj_surface->flags &= ~SURFACE_REF_DIS_MASK;
>> >> >> -            }
>> >> >> -
>> >> >> -            if (obj_surface->free_private_data)
>> >> >> -                obj_surface->free_private_data(&obj_surface->private_data);
>> >> >> -
>> >> >>              frame_store[i].surface_id = VA_INVALID_ID;
>> >> >>              frame_store[i].frame_store_id = -1;
>> >> >>              frame_store[i].obj_surface = NULL;
>> >> >
>> >> >
>> >
>> >
>
>