[PATCH hwc v1] [RFC] drm_hwcomposer: Flatten composition using writeback connector

Tue Mar 20 13:51:33 UTC 2018

On Tue, Mar 20, 2018 at 09:43:53AM -0400, Sean Paul wrote:
> On Tue, Mar 20, 2018 at 01:35:53PM +0000, Alexandru-Cosmin Gheorghe wrote:
> > Hi,
> > 
> > On Tue, Mar 20, 2018 at 09:28:50AM -0400, Sean Paul wrote:
> > > On Tue, Mar 20, 2018 at 11:26:39AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > > Hi Sean,
> > > > 
> > > > Thank you for the feedback.
> > > > I will comeback with v2, later on.
> > > > 
> > > > On Mon, Mar 19, 2018 at 02:03:54PM -0400, Sean Paul wrote:
> > > > > On Tue, Mar 13, 2018 at 04:21:20PM +0000, Alexandru Gheorghe wrote:
> > > > > > This patchset tries to add support for using writeback connector to
> > > > > > flatten a scene when it doesn't change for a while. This idea had
> > > > > > been floated around on IRC here [1] and here [2].
> > > > > > 
> > > > > > Developed on top of the latest writeback series, sent by Liviu here
> > > > > > [3].
> > > > > > 
> > > > > > Probably the patch should/could be broken in more patches, but since I
> > > > > > want to put this out there to get feedback, I kept them as a single
> > > > > > patch for now.
> > > > > 
> > > > > Thank you for your patch, it's looking good. Feel free to submit the v2 in
> > > > > multiple patches. Also note that we've migrated to gitlab, the new tree is
> > > > > located here:
> > > > > 
> > > > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer
> > > > 
> > > > This seems to be behind
> > > > https://anongit.freedesktop.org/git/drm_hwcomposer.git.
> > > > Is this location deprecated or does it serve other purposes.
> > > > 
> > > 
> > > Ah, thanks for catching this. It's a migration glitch, I've made it whole
> > > again.
> > > 
> > > <snip />
> > > 
> > > > > >  #include "drmdisplaycompositor.h"
> > > > > > 
> > > > > >  #include <pthread.h>
> > > > > > @@ -36,9 +35,24 @@
> > > > > >  #include "drmplane.h"
> > > > > >  #include "drmresources.h"
> > > > > >  #include "glworker.h"
> > > > > > +static const uint32_t kWaitWritebackFence = 100; //ms
> > > > > > 
> > > > > >  namespace android {
> > > > > > 
> > > > > > +class CompositorVsyncCallback : public VsyncCallback {
> > > > > > + public:
> > > > > > +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> > > > > > +      :compositor_(compositor) {
> > > > > > +  }
> > > > > > +
> > > > > > +  void Callback(int display, int64_t timestamp) {
> > > > > > +    compositor_->Vsync(display, timestamp);
> > > > > > +  }
> > > > > > +
> > > > > > + private:
> > > > > > +  DrmDisplayCompositor *compositor_;
> > > > > > +};
> > > > > > +
> > > > > 
> > > > > The GLCompositor already has flattening, so we should tie into that. I assume
> > > > > writeback is going to be more efficient than using GPU (given that the GPU is
> > > > > probably turned off in these scenarios), so you're probably safe to use
> > > > > writeback when available and fall back to GL if it's not.
> > > > > 
> > > > > 
> > > > 
> > > > As far as I understood, this was done by the drmcompositorworker.cpp
> > > > by calling SquashAll but that's removed from build now. GLCompositor
> > > > seems to do compositing only when there are not enough overlays or 
> > > > atomic check fails.
> > > > 
> > > > Am I missing something ? 
> > > > Is it still doing flattening when then scene does not change ?
> > > 
> > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/drmcompositorworker.cpp#L62
> > > 
> > > If the scene doesn't change, everything is squashed.
> > 
> > Yeah, but that file it's not even built, see [1].
> > So, I kind of assumed someone had a reason of deleting it. 
> > Any idea about that?
> 
> Ha, WTF! Yeah, guess that's a regression. 
> 
> /me wonders whether we should just rip out the GLCompositor since no one seems
> to care about it.

I thought about removing/disabling it as well, but it turnout to be
not as easy as I expected, since on validateDisplay we don't do
anything and we do the planning and atomic check on presentDisplay and
then we fallback on GLCompositor if needed.

> 
> I'll circle back on the review and take a look at how you're doing the
> inactivity stuff.

Thanks!.

> 
> Sean
> 
> > 
> > [1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/blob/master/Android.mk#L53
> > 
> > > 
> > > <snip />
> > > 
> > > > > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > > > > index 32dd376..880cef2 100644
> > > > > > --- a/drmresources.cpp
> > > > > > +++ b/drmresources.cpp
> > > > > > @@ -33,6 +33,8 @@
> > > > > >  #include <cutils/log.h>
> > > > > >  #include <cutils/properties.h>
> > > > > > 
> > > > > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS    4
> > > > > > +
> > > > > 
> > > > > Presumably this should come from libdrm headers?
> > > > 
> > > > Yes it will, this was just a quick fix to be able to compile against
> > > > older libdrm.
> > > > 
> > > > Btw, this would make drm_hwcomposer compilable only with newer libdrm
> > > > version, but I suppose that's acceptable.
> > > 
> > > You can use preprocessor checks to determine if
> > > DRM_CLIENT_CAP_WRITEBACK_CONNECTORS is defined, and disable the functionality if
> > > it's not.
> > > 
> > > > 
> > > > > 
> > > > > >  namespace android {
> > > > > > 
> > > > > >  DrmResources::DrmResources() : event_listener_(this) {
> > > > > > @@ -65,6 +67,11 @@ int DrmResources::Init() {
> > > > > >      return ret;
> > > > > >    }
> > > > > > 
> > > > > > +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > > > > > +  if (ret) {
> > > > > > +    ALOGI("Failed to set writeback cap %d", ret);
> > > > > > +    ret = 0;
> > > > > > +  }
> > > > > >    drmModeResPtr res = drmModeGetResources(fd());
> > > > > >    if (!res) {
> > > > > >      ALOGE("Failed to get DrmResources resources");
> > > > > > @@ -77,6 +84,7 @@ int DrmResources::Init() {
> > > > > >        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
> > > > > > 
> > > > > >    bool found_primary = false;
> > > > > > +  int primary_index = 0;
> > > > > >    int display_num = 1;
> > > > > > 
> > > > > >    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> > > > > > @@ -162,18 +170,22 @@ int DrmResources::Init() {
> > > > > >      if (conn->internal() && !found_primary) {
> > > > > >        conn->set_display(0);
> > > > > >        found_primary = true;
> > > > > > -    } else {
> > > > > > +    } else if (conn->external()) {
> > > > > > +      if (!found_primary) primary_index++;
> > > > > 
> > > > > This is against style guide (and same below).
> > > > 
> > > > I suppose that's what happens when you skip some steps from 
> > > > "Submitting patches", clang will catch this next time.
> > > > 
> > > 
> > > I might look into adding a hook to gitlab to automatically run this, time
> > > permitting.
> > > 
> > > Sean
> > > 
> > > <snip />
> > > -- 
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> > -- 
> > Cheers,
> > Alex G
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Cheers,
Alex G