[RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[John Stultz]

On Wed, Jan 31, 2018 at 10:51 AM, Alexandru-Cosmin Gheorghe
<Alexandru-Cosmin.Gheorghe at arm.com> wrote:
> Hi John,
>
> I took your patches for a spin on Hikey960. Some findings:
> Even with this patch I'm getting some tearing on Hikey960, not a lot as you
> reported on Hikey, still there are some small black squares, less than 10 of
> aproximetly 20x20.

Sweet! I'm glad you were able to get it up and running!  I think the
black squares are a separate thing, more on that below.

> So I investigated a little bit through drm_hwcomposer and found some
> issues, maybe they could help you somehow, see bellow:

Very much appreciate the extra review and eyes on the code!

>> > Yes, I did spent a little bit of time earlier trying to rewrite the gl
>> > shader to try to build for the utgard level hardware, but wasn't very
>> > successful. At a deeper level I guess I'm not sure the glcompositor is
>> > useful in this case, since we're doing single plane client side
>> > compositing (as short of the glFinish bit, not running it doesn't seem
>> > to keep things from working).  But I'll look into that again.
>> >
>> > Again forgive as I really a bit in the dark on most graphics details,
>> > but the other more basic question I'm a bit unsure of is, does this
>> > patch even make any functional sense?
>>
>> I'd think not. The only thing that makes me question that is I've seen
>> glFinish calls in gralloc (framebuffer) cases. But those cases were
>> prior to explicit fence support.
>>
>> >  If we're not using the
>> > glcompositor and are using the atomic commit in ApplyFrame(), why the
>> > need for glFinish to avoid tearing?  Is it that the we commit the
>> > frame atomically to the display, but if we don't block w/ glFinish()
>> > the gpu is still drawing into it?  It seems we'd want a buffer
>> > specific fence to wait on rather then waiting for all gl calls to
>> > complete (if I'm understanding how glFinish() works).
>>
>> That seems like the right approach. Are we failing to pass fences
>> associated with input layers to DRM?
>> _______________________________________________
> It seems that we don't pass any explicit fences to the kernel for
> IN_FENCE_FD property. This particular line seems wrong.
>
> @@ -593,14 +594,17 @@ int
> DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>        else
>          rotation |= DRM_MODE_ROTATE_0;
>
> -      if (fence_fd < 0) {
> +      if (fence_fd >= 0) {

I'll give that a whirl.

> Also, if you track the value of the OUT_FENCE_PTR property, it seems
> that it gets lost. As far I was able to track it down it seem that
> it should have been used by AddFenceToRetireFence, and then be passed to
> SurfaceFlinger through retire_fence parameter of PresentDisplay. But,
> that doesn't happen because  AddFenceToRetireFence get's called each
> time on a new DrmDisplayComposition object, before calling
> ApplyComposition.

I'll look into this some more as well. I've not yet gotten into the
details of the fence handling in drm_hwcomposer yet, so I'll have to
check that out.

> Both of this findings should have explained the tearing, however I
> hacked something today and it didn't fix it for Hikey960, maybe
> you have more luck.

The issue with the black/grey blocks is a separate problem with the
4.9 based kernels that I'm still chasing down, we see it (though less
often) w/o the hwcomposer, and I think it has to do with something off
in the CMA allocations. We don't see it with the 4.4 or the 4.14 based
tree i'm also prepping.

Thanks again for the feedback! I'm really happy to have your input
here, so if you do generate any further changes, let me know and I'll
try to integrate them into the patch set!

thanks
-john