Support for external displays #93

krnlyng · 2021-12-27T13:27:15Z

Based on @thp s and @elros34 s work. Rebased to latest master, added some workarounds for certain devices.

[v11] introduce no-active-config quirk. JB#56560 Can be used on devices where getActiveConfig is not functional in the v11 backend. Co-authored-by: Franz-Josef Haider <[email protected]>

[v11] Support for external displays. JB#56560 Co-authored-by: elros34 <[email protected]> Co-authored-by: Franz-Josef Haider <[email protected]>

[v11] multiscreen: don't set transform to anything but 0 for now. JB#56560

…frame since it's really slow on some devices. [v11] multiscreen: avoid calling getDisplayAttributes on every frame since it's really slow on some devices. JB#56560

[v11] multiscreen: close releaseFenceFd of framebuffer layer. JB#56560

elros34 · 2021-12-27T23:57:14Z

hwcomposer/hwcomposer_backend_v11.cpp

+        // glitches and warnings in logcat. By setting the planarAlpha to non-
+        // opaque, we attempt to force the HWC into using HWC_FRAMEBUFFER for this
+        // layer so the HWC_FRAMEBUFFER_TARGET layer actually gets used.
+        bool tryToForceGLES = !qgetenv("QPA_HWC_FORCE_GLES").isEmpty();


What do you think about static bool instead calling qgetenv on every frame.

Suggested change

bool tryToForceGLES = !qgetenv("QPA_HWC_FORCE_GLES").isEmpty();

static bool tryToForceGLES = qEnvironmentVariableIsSet("QPA_HWC_FORCE_GLES");

elros34 · 2021-12-27T23:57:33Z

It was quite a long time since I last tried this code but I remember I had crash in hwcomposer when connecting hdmi without this: elros34@9556cd5 (and close fence elros34@a823e4f). hnd pointer is expected to be not NULL: https://github.com/mer-hybris/android_hardware_qcom_display-caf/blob/hybris-11.0-44S/libhwcomposer/hwc_copybit.cpp#L191. This should also prevents "layer handle is NULL" spam in logcat: https://github.com/mer-hybris/android_hardware_qcom_display-caf/blob/hybris-11.0-44S/libhwcomposer/hwc_mdpcomp.cpp#L299.

adenexter · 2021-12-28T08:38:39Z

hwcomposer/hwcomposer_backend_v11.cpp

+    {
+        free(contents);
+
+        for (auto &screen: screens) {


auto & -> auto? Unless you were writing back to the position in the vector I can't think why a reference to a pointer would be favorable to just a pointer.

adenexter · 2021-12-28T08:47:47Z

hwcomposer/hwcomposer_backend_v11.h

+    ~RetireFencePool()
+    {
+        for (auto fd: m_fds) {
+            fprintf(stderr, "Waiting and closing retire fence fd: %d\n", fd);


Can you add a macro with a environment variable or something for the debug output? A lot of the others are infrequent enough to not really matter but this is going to print every frame.

adenexter · 2021-12-28T08:58:01Z

hwcomposer/hwcomposer_backend_v11.cpp

+
+        // Destination rectangle on the actual screen
+        const hwc_rect_t dest_rect = {
+            0, 0, m_screen_width, m_screen_height


What happens when the screens have different aspect ratios? Is the content just stretched to fit on the external screens?

I can only tell that from my old test based on: https://github.com/elros34/qt5-qpa-hwcomposer-plugin/tree/a31 I have only managed to get this code working correctly on native landscape display tablet (screen width > screen height). The content on tv had same aspect ratio as device use. I had to resize content in tv options.
On regular device (moto photon q) which has screen width < screen height content is totally mess up. I have manged to get it working once in portrait mode while messing with width/height parameters in relayout but never managed to rotate content to landscape mode, that is why I dropped it. In nemo bug report (https://bugs.nemomobile.org/show_bug.cgi?id=765: Nemo bug 765 in Hybris-ing "[hammerhead][others] Enable external (Slimport/MHL) screen Mirroring/Extending on Wayland" [Task,New]) there were photos of some nexus devices which worked fine in portrait mode but link is no longer valid and contains some different bug.

…layer." This reverts commit 4c06c77.

[v11] use static variable for QPA_HWC_FORCE_GLES. JB#56560

[v11] use pointer instead of reference to pointer. JB#56560

[v11] move too verbose fence debug behind a macro. JB#56560

elros34 · 2021-12-29T14:46:13Z

hwcomposer/hwcomposer_backend_v11.cpp

+        };
+
+        // Don't call getDisplayAttributes too often here since it can cause big slowdowns.
+        if (m_screen_width <= 0 || m_screen_height <= 0) {


This will be still executed because not connected displays (external and virtual) will have m_screen_width/height = 0 until first connection. I wonder whether HwComposerScreen_v11 should be created on demand for external and virtual display. For now relayout is called for 3 displays even without external display connection.
Is it really needed to call relayout() on every frame? From my tests calling it after unblank display was enough but I never manged to get whole code working correctly on native portrait display device so take it with grain of salt.

Does it have to be after unblank or is it enough if it's called on hotplug?

I have added relayout(HwComposerBackend_v11->widht/height) in hotplug and in createWindow() after width/height are set and indeed both builtin and external display works on A31 based tablet I use for testing.

[v11] don't call relayout on every frame. JB#56560

[v11] update screen sizes on hotplug. JB#56560

…hotplug [v11] update screen sizes not just for the external display on hotplug. JB#56560

[v11] remove unused variable. JB#56560

[v11] set initial screen sizes. JB#56560

[v11] fix QPA_HWC_FORCE_GLES. JB#56560

jpetrell

The external display support has so far seen limited testing (landscape homescreen exported to external landscape display) so this is probably not fully working yet, but a good first step. Added few comments, but overall looking good. 🙂

jpetrell · 2022-03-04T12:01:31Z

hwcomposer/hwcomposer_backend_v11.cpp

+    }
+
+    ((struct HwcProcs_v11*)procs)->backend->screenPlugged();
+    ((struct HwcProcs_v11*)procs)->content->update_screen_sizes(((struct HwcProcs_v11*)procs)->backend);
 }

 HwComposerBackend_v11::~HwComposerBackend_v11()
 {
    hwc_device->eventControl(hwc_device, 0, HWC_EVENT_VSYNC, 0);


Should there be hwc_device->eventControl(hwc_device, 1, HWC_EVENT_VSYNC, 0) also, i.e. for the external display?

jpetrell · 2022-03-04T12:01:45Z

hwcomposer/hwcomposer_backend_v11.cpp

+    HWC_PLUGIN_EXPECT_ZERO(status);
+    g_unblanked_displays[display] = (status == blank);
+
+    // on certian devices this is needed


Typo certain

jpetrell · 2022-03-04T12:03:00Z

hwcomposer/hwcomposer_backend_v11.cpp

 void
 HwComposerBackend_v11::sleepDisplay(bool sleep)
 {
+    // XXX: For debugging only
+    //dump_attributes(hwc_device, num_displays);


Ideally such debug traces would be put behind Qt logging sub-categories for enabling them without recompilation

jpetrell · 2022-03-04T12:03:39Z

hwcomposer/hwcomposer_backend_v11.cpp

@@ -393,7 +687,7 @@ HwComposerBackend_v11::sleepDisplay(bool sleep)
    }
 }

-int HwComposerBackend_v11::getSingleAttribute(uint32_t attribute)
+int HwComposerBackend_v11::getSingleAttribute(uint32_t attribute, int dpy)


When I see function like "getSingleAttribute(uint32_t attribute, int dpy)" it wass not automatically obvious to me that dpy refered to display's index number. In Qt style abbreviations are avoided (expect in fully local, obvious cases like for (int i; i < count; i++)... Could align with blankDisplay(int display, ..)

Also would be good to consistently have display index as the first or second attribute, now both are used

jpetrell · 2022-03-04T12:07:12Z

hwcomposer/hwcomposer_backend_v11.cpp

+                blankDisplay(0, true);
+            }
+            fprintf(stderr, "Unblanking external display\n");
+            blankDisplay(1, false);


Since display 0 is assumed to be always internal display and display 1 the external display would normally avoid hard-coded magical numbers and instead introduce enumerations like InternalDisplay, ExternalDisplay passed to the functions. Now debug prints above are used to document 0=internal, 1=external

jpetrell · 2022-03-04T12:07:35Z

hwcomposer/hwcomposer_backend_v11.cpp

+    // on certian devices this is needed
+    screenPlugged();
+}
+
 void
 HwComposerBackend_v11::sleepDisplay(bool sleep)


Sounds like sleepDisplay() is nowadays sleepDisplays()

jpetrell · 2022-03-04T12:10:49Z

hwcomposer/hwcomposer_backend_v11.cpp

-            HWC_PLUGIN_EXPECT_ZERO(hwc_device->blank(hwc_device, 0, 1));
+        for (int i=0; i<num_displays; i++) {
+            if (g_unblanked_displays[i]) {
+                blankDisplay(i, true);


As terms go personally I like blanking more than sleeping, but ideally whould stick to one term and at least use it consistently within the scope of internal variables. Now display on/off -> sleepDisplay true/false -> blankDisplay true/false -> device setPowerMode on/off.

jpetrell · 2022-03-04T12:11:50Z

hwcomposer/hwcomposer_backend_v11.cpp

+
+static int g_external_connected = 0;
+static int g_external_connected_next = 0;
+static int g_unblanked_displays[HWC_NUM_DISPLAY_TYPES] = { 0 };


Non-CamelCase function names get_screen_size and using of fprintf instead of qDebug irks me slightly, but these platform plugin codes are bit in the middle of two words so maybe mixing C-style with C++-style is fine

jpetrell · 2022-03-04T12:13:10Z

hwcomposer/hwcomposer_backend_v11.cpp

+#if 0
+        for (int i=0; i<num_displays; i++) {
+            blankDisplay(i, false);
+        }


Wonder if other routines could also have been written to handle n displays instead of hardcoding to 2 (internal, external). Even if initially only two were supported. Maybe not worth the effort at this point

jpetrell · 2022-03-04T12:26:16Z

hwcomposer/hwcomposer_backend_v11.h

@@ -63,6 +99,7 @@ class HwComposerBackend_v11 : public QObject, public HwComposerBackend {
    virtual EGLNativeWindowType createWindow(int width, int height);
    virtual void destroyWindow(EGLNativeWindowType window);
    virtual void swap(EGLNativeDisplayType display, EGLSurface surface);
+    virtual void blankDisplay(int display, bool blank);
    virtual void sleepDisplay(bool sleep);
    virtual float refreshRate();
    virtual bool getScreenSizes(int *width, int *height, float *physical_width, float *physical_height);


getScreenSize calls getSingleAttribute without display/screen id, which causes getSingleAttribute to default to display 0. If the device has no internal display but only an external one the screen info exposed by the Qt platform plugin reports false values?

NotKit and others added 5 commits December 27, 2021 13:19

hwcomposer: introduce no-active-config quirk in the v11 backend.

b3cf8b3

[v11] introduce no-active-config quirk. JB#56560 Can be used on devices where getActiveConfig is not functional in the v11 backend. Co-authored-by: Franz-Josef Haider <[email protected]>

hwcomposer: Support for external displays.

482e5eb

[v11] Support for external displays. JB#56560 Co-authored-by: elros34 <[email protected]> Co-authored-by: Franz-Josef Haider <[email protected]>

hwcomposer: multiscreen: don't set transform to anything but 0 for now.

52cc2a7

[v11] multiscreen: don't set transform to anything but 0 for now. JB#56560

hwcomposer: multiscreen: avoid calling getDisplayAttributes on every …

7cbc52a

…frame since it's really slow on some devices. [v11] multiscreen: avoid calling getDisplayAttributes on every frame since it's really slow on some devices. JB#56560

hwcomposer: multiscreen: close releaseFenceFd of framebuffer layer.

4c06c77

[v11] multiscreen: close releaseFenceFd of framebuffer layer. JB#56560

krnlyng requested review from adenexter, rainemak and mlehtima December 27, 2021 13:27

elros34 reviewed Dec 27, 2021

View reviewed changes

adenexter reviewed Dec 28, 2021

View reviewed changes

krnlyng and others added 6 commits December 29, 2021 13:44

Revert "hwcomposer: multiscreen: close releaseFenceFd of framebuffer …

0c75f45

…layer." This reverts commit 4c06c77.

[hwcomposer] Close layer0's release fence

6f845c4

hwcomposer: use static variable for QPA_HWC_FORCE_GLES

9f612f5

[v11] use static variable for QPA_HWC_FORCE_GLES. JB#56560

[hwcomposer] Fix crash when external display is connected

c0c3cb5

hwcomposer: use pointer instead of reference to pointer.

c6d37a4

[v11] use pointer instead of reference to pointer. JB#56560

hwcomposer: move too verbose fence debug behind a macro.

c80111e

[v11] move too verbose fence debug behind a macro. JB#56560

elros34 reviewed Dec 29, 2021

View reviewed changes

krnlyng added 6 commits January 17, 2022 13:38

hwcomposer: don't call relayout on every frame.

f0a3e56

[v11] don't call relayout on every frame. JB#56560

hwcomposer: update screen sizes on hotplug

a4f5f68

[v11] update screen sizes on hotplug. JB#56560

hwcomposer: update screen sizes not just for the external display on …

f178e19

…hotplug [v11] update screen sizes not just for the external display on hotplug. JB#56560

hwcomposer: remove unused variable

14a8399

[v11] remove unused variable. JB#56560

hwcomposer: set initial screen sizes

ba4f9bd

[v11] set initial screen sizes. JB#56560

hwcomposer: fix QPA_HWC_FORCE_GLES

d7c307d

[v11] fix QPA_HWC_FORCE_GLES. JB#56560

mkosola approved these changes Jan 26, 2022

View reviewed changes

jpetrell suggested changes Mar 4, 2022

View reviewed changes

krnlyng mentioned this pull request Mar 18, 2022

hybris: eglplatform_hwcomposer: allow creating multiple native windows libhybris/libhybris#497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for external displays #93

Support for external displays #93

krnlyng commented Dec 27, 2021

elros34 Dec 27, 2021

elros34 commented Dec 27, 2021

adenexter Dec 28, 2021

adenexter Dec 28, 2021

adenexter Dec 28, 2021

elros34 Dec 29, 2021 •

edited

Loading

elros34 Dec 29, 2021

krnlyng Jan 17, 2022

elros34 Jan 31, 2022 •

edited

Loading

jpetrell left a comment

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

jpetrell Mar 4, 2022

	bool tryToForceGLES = !qgetenv("QPA_HWC_FORCE_GLES").isEmpty();
	static bool tryToForceGLES = qEnvironmentVariableIsSet("QPA_HWC_FORCE_GLES");

Support for external displays #93

Are you sure you want to change the base?

Support for external displays #93

Conversation

krnlyng commented Dec 27, 2021

Choose a reason for hiding this comment

elros34 commented Dec 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elros34 Dec 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elros34 Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

jpetrell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elros34 Dec 29, 2021 •

edited

Loading

elros34 Jan 31, 2022 •

edited

Loading