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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
