Hi,
2014-05-12 8:17 GMT+02:00 Zhao, Yakui <yakui.z...@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.z...@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.z...@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;
>> >> >
>> >> >
>> >
>> >
>
>
_______________________________________________
Libva mailing list
Libva@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libva
