REGRESSION(47dee03): Broke DRM plugin on RPi4

[clopez]

Commit 47dee03 broke the DRM plugin on the RPi4 (My test build is based on Yocto version 4.2 (Mickeledore) with Mesa drivers)

I bisected it and reverting this commit fixes the issue.

To test this on the RPi4 you have to pass to Cog the renderer=gles platform parameters (see below how to execute cog)

On the terminal you can see this:

# cog --platform=drm --platform-params=renderer=gles https://igalia.com
EGLDisplay Initialization failed: EGL_NOT_INITIALIZED
Cog-Core-Message: 20:50:55.787: <https://igalia.com/> Load started.
Cog-Core-Message: 20:50:55.820: <https://www.igalia.com/> Redirected.
Cog-Core-Message: 20:50:56.157: <https://www.igalia.com/> Loading...

(cog:6469): Cog-DRM-WARNING **: 20:50:59.149: cog_drm_gles_renderer_handle_egl_image: Cannot set mode (Invalid argument)
Cog-Core-Message: 20:51:00.285: <https://www.igalia.com/> Loaded successfully.

And nothing gets rendered on the screen

The error (cog:6469): Cog-DRM-WARNING **: 20:50:59.149: cog_drm_gles_renderer_handle_egl_image: Cannot set mode (Invalid argument) is new.

If you run with env var G_MESSAGES_DEBUG=all then you get this: http://ix.io/4JfT

Related: #590

//cc @zhani @psaavedra @aperezdc

[clopez]

The following patch fixes the issue

diff --git a/platform/drm/cog-platform-drm.c b/platform/drm/cog-platform-drm.c
index be8521b..25a4f22 100644
--- a/platform/drm/cog-platform-drm.c
+++ b/platform/drm/cog-platform-drm.c
@@ -359,6 +359,7 @@ find_crtc_for_encoder(const drmModeRes *resources, const drmModeEncoder *encoder
         const uint32_t crtc_mask = 1 << i;
         const uint32_t crtc_id = resources->crtcs[i];
         if (encoder->possible_crtcs & crtc_mask) {
+            //g_assert (encoder->crtc_id == crtc_id); // should this two values match? should be an if instead of an assert?
             return crtc_id;
         }
     }
@@ -517,6 +518,9 @@ init_drm(void)
             continue;
         }
 
+        if (drm_data.encoder->encoder_id != drm_data.connector.obj->encoder_id)
+            continue;
+
         const int32_t crtc_id = find_crtc_for_encoder(drm_data.base_resources, drm_data.encoder);
         if (crtc_id != -1) {
             drm_data.crtc.obj_id = crtc_id;

But I'm not sure if is the best approach because I'm not very familiar with this DRM code.

Some notes:

• The commented assert on the patch above won't trigger in the RPi4 case, so is safe to keep the assert. Maybe it should be an if instead of an assert.

• See the following debug output produced with this patch: http://ix.io/4Jg0

Encoder[0]: has drm_data.encoder->encoder_id=31 and drm_data.connector.obj->encoder_id=41 and drm_data.encoder->crtc_id 0!
Encoder[1]: has drm_data.encoder->encoder_id=41 and drm_data.connector.obj->encoder_id=41 and drm_data.encoder->crtc_id 107!
Encoder[2]: has drm_data.encoder->encoder_id=58 and drm_data.connector.obj->encoder_id=41 and drm_data.encoder->crtc_id 0!

I have 6 crts with the following crt_ids
	 crt_number=0 has crt_id=57
	 crt_number=1 has crt_id=74
	 crt_number=2 has crt_id=85
	 crt_number=3 has crt_id=96
	 crt_number=4 has crt_id=107
	 crt_number=5 has crt_id=118

As you see, in this case (RPi4) I have 3 encoders and only one of those has a non-zero value for the crtc_id. So maybe checking for drm_data.encoder->crtc_id != 0 is a god idea?

[clopez]

As you see, in this case (RPi4) I have 3 encoders and only one of those has a non-zero value for the crtc_id. So maybe checking for drm_data.encoder->crtc_id != 0 is a god idea?

Following that, I can see this alternative patch also fixes the issue on the RPi4

diff --git a/platform/drm/cog-platform-drm.c b/platform/drm/cog-platform-drm.c
index be8521b..b32d6a4 100644
--- a/platform/drm/cog-platform-drm.c
+++ b/platform/drm/cog-platform-drm.c
@@ -359,6 +359,7 @@ find_crtc_for_encoder(const drmModeRes *resources, const drmModeEncoder *encoder
         const uint32_t crtc_mask = 1 << i;
         const uint32_t crtc_id = resources->crtcs[i];
         if (encoder->possible_crtcs & crtc_mask) {
+            //g_assert (encoder->crtc_id == crtc_id); // should this two values match? should be an if instead of an assert?
             return crtc_id;
         }
     }
@@ -517,6 +518,9 @@ init_drm(void)
             continue;
         }
 
+        if (!drm_data.encoder->crtc_id)
+            continue;
+
         const int32_t crtc_id = find_crtc_for_encoder(drm_data.base_resources, drm_data.encoder);
         if (crtc_id != -1) {
             drm_data.crtc.obj_id = crtc_id;

Basically we have to call the function find_crtc_for_encoder() only with an encoder that has valid crtc_id value (non-zero) .. Otherwise that function does not work as expected and returns something that causes issues later.

Not sure if is better this second patch or the previous one.

Comments?

[clopez]

Interesting, this other patch fixes the issue as well:

diff --git a/platform/drm/cog-platform-drm.c b/platform/drm/cog-platform-drm.c
index be8521b..7516eec 100644
--- a/platform/drm/cog-platform-drm.c
+++ b/platform/drm/cog-platform-drm.c
@@ -359,7 +359,8 @@ find_crtc_for_encoder(const drmModeRes *resources, const drmModeEncoder *encoder
         const uint32_t crtc_mask = 1 << i;
         const uint32_t crtc_id = resources->crtcs[i];
         if (encoder->possible_crtcs & crtc_mask) {
-            return crtc_id;
+            if (encoder->crtc_id == crtc_id)
+                return crtc_id;
         }
     }

Basically turn the previous commented assertion into an if and do not check in the previous loop the value of the crtc_id of the encoder.

Looks like a winner to me this last one.

I would like to know if this also works on the imx6 use case

[WebReflection]

I wonder if this is the same bug/issue I am seeing here: #611

[clopez]

Just merged a fix for this in #680