Skip to content

Conversation

Bennctu
Copy link
Collaborator

@Bennctu Bennctu commented Nov 3, 2024

Currently, using the DRM legacy interface is suitable for Mado's backend because we only need basic window display without advanced features.

@jserv jserv requested a review from shengwen-tw November 3, 2024 15:05
@jserv
Copy link
Contributor

jserv commented Nov 3, 2024

Currently, using the DRM legacy interface is suitable for Mado's backend because we only need basic window display without advanced features.

What compatibility risks exist when using DRM legacy interfaces with newer Linux kernels? Could you verify the compatibility status of libdrm across different kernel versions?

What prevents the DRM backend from rendering content across the full screen resolution?

@jserv
Copy link
Contributor

jserv commented Nov 3, 2024

@a1091150, You might check DRM backend for your environment as well.


return true;

bail_crtc:
Copy link
Collaborator

@huaxinliao huaxinliao Nov 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about this. It should be as below.

/* Retrieve resources */
if (!get_resources(tx->drm_dri_fd, &RES(tx)))
    goto bail_res;
....
bail_fb:
    drmModeRmFB(tx->drm_dri_fd, tx->fb_id);
    munmap(tx->fb_base, tx->fb_len);
bail_crtc:
    drmModeFreeCrtc(CRTC(tx));
bail_conn:
    drmModeFreeConnector(CONN(tx));
bail_res:
    drmModeFreeResources(RES(tx));

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase the latest main branch and resolve conflicts.

@Bennctu
Copy link
Collaborator Author

Bennctu commented Nov 6, 2024

Currently, using the DRM legacy interface is suitable for Mado's backend because we only need basic window display without advanced features.

What compatibility risks exist when using DRM legacy interfaces with newer Linux kernels? Could you verify the compatibility status of libdrm across different kernel versions?

What prevents the DRM backend from rendering content across the full screen resolution?

I need more time to research and adjust.

@jserv
Copy link
Contributor

jserv commented Nov 11, 2024

Check the DRM implementation for reference: https://github.com/zlgopen/awtk-linux-fb/blob/master/awtk-port/lcd_linux/lcd_linux_drm.c

jserv

This comment was marked as resolved.

@Bennctu Bennctu force-pushed the drm_dev branch 2 times, most recently from 32e2217 to 3cba956 Compare November 23, 2024 07:19

/* Register the Linux DRM backend */

const twin_backend_t g_twin_backend = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add poll operation as it is required in recent main branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the original implementation, we creates another thread to handle the events in function twin_linux_evdev_thread. So do you suggest refactoring linux_input.c#L255~278?

Or we just assign NULL to .poll?

/* Register the Linux DRM backend */

const twin_backend_t g_twin_backend = {
    .init = twin_drm_init,
    .poll = NULL,  /*without implementation*/
    .configure = twin_drm_configure,
    .exit = twin_drm_exit,
};

$ sudo ./demo-drm
```

The DRM device can be assigened via the environment variable `DRI_CARD`.
Copy link
Contributor

@jserv jserv Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DRI_CARD is not common for understanding. How about DRMDEVICE?

@jserv jserv requested a review from alanjian85 November 28, 2024 16:23
jserv

This comment was marked as resolved.

Implemented DRM-based framebuffer setup, including:
 - Opening DRM device
 - Creating dumb buffers and mapping them to framebuffers
 - Setting mode and handling CRTC for connected display output

Currently, using the DRM legacy interface is suitable for Mado's backend
if we only need basic window display without advanced features. The DRM
legacy is simpler to implement.

Close sysprog21#60
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refine git commit messages:

  1. Use Close #60 instead of Close sysprog21#60
  2. Always use third-person perspective: Avoid personal pronouns like "I", "you", and "we", focusing on an objective description of changes.
  3. More explanation for using legacy DRM interface. Provide context for why the alternatives are not suitable.

if (drmModeAddFB(tx->drm_dri_fd, tx->width, tx->height, 24, 32,
create_req.pitch, create_req.handle, &tx->fb_id) != 0) {
log_error("Cannot create framebubber");
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider that this line should be changed to goto bail_dumb.

bail_dumb:
    struct drm_mode_destroy_dumb destroy = { .handle = create_req.handle };
    ioctl(tx->drm_dri_fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy);
    return false;

/* Create framebuffer object for the dumb-buffer */
if (drmModeAddFB(tx->drm_dri_fd, tx->width, tx->height, 24, 32,
create_req.pitch, create_req.handle, &tx->fb_id) != 0) {
log_error("Cannot create framebubber");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct the typo in framebubber.

twin_screen_resize(SCREEN(ctx), width, height);
}

static void twin_drm_exit(twin_context_t *ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider that twin_drm_exit should handle ioctl(tx->drm_dri_fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy) to ensure allocated resources are properly released.

@huaxinliao
Copy link
Collaborator

I ran the DRM backend on a VM with Ubuntu 24.04. However, I noticed that the screen froze.

drm.mov

@Bennctu
Copy link
Collaborator Author

Bennctu commented Feb 3, 2025

I ran the DRM backend on a VM with Ubuntu 24.04. However, I noticed that the screen froze.
drm.mov

@huaxinliao Thank you for reporting this issue.

@sysprog21 sysprog21 deleted a comment from cubic-dev-ai bot Oct 16, 2025
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 files

Prompt for AI agents (all 1 issues)

Understand the root cause of the following 1 issues and fix them.


<file name="README.md">

<violation number="1" location="README.md:140">
Documentation points users to set `DRI_CARD`, but the DRM backend actually reads `$DRMDEVICE`, so following the new instructions will not select the intended card and the line also contains a typo.</violation>
</file>

Since this is your first cubic review, here's how it works:

  • cubic automatically reviews your code and comments on bugs and improvements
  • Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
  • Ask questions if you need clarification on any suggestion

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

$ sudo ./demo-drm
```

The DRM device can be assigened via the environment variable `DRI_CARD`.
Copy link

@cubic-dev-ai cubic-dev-ai bot Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation points users to set DRI_CARD, but the DRM backend actually reads $DRMDEVICE, so following the new instructions will not select the intended card and the line also contains a typo.

Prompt for AI agents
Address the following comment on README.md at line 140:

<comment>Documentation points users to set `DRI_CARD`, but the DRM backend actually reads `$DRMDEVICE`, so following the new instructions will not select the intended card and the line also contains a typo.</comment>

<file context>
@@ -128,6 +131,14 @@ $ sudo usermod -a -G video $USERNAME
+$ sudo ./demo-drm
+```
+
+The DRM device can be assigened via the environment variable `DRI_CARD`.
+
 To run demo program with the VNC backend:
</file context>
Suggested change
The DRM device can be assigened via the environment variable `DRI_CARD`.
The DRM device can be assigned via the environment variable `DRMDEVICE`.
Fix with Cubic

@sysprog21 sysprog21 deleted a comment from cubic-dev-ai bot Oct 16, 2025
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 4 files

Prompt for AI agents (all 2 issues)

Understand the root cause of the following 2 issues and fix them.


<file name="README.md">

<violation number="1" location="README.md:140">
Typo: &quot;assigened&quot; should be spelled &quot;assigned&quot; to keep the documentation clear.</violation>
</file>

<file name="backend/drm.c">

<violation number="1" location="backend/drm.c:64">
Frame buffer writes should advance by the mapped buffer pitch, but this code uses the visible width, so scanlines misalign whenever the dumb buffer pitch exceeds the mode width. Use the stride returned by create_fb() instead of tx-&gt;width to compute offsets.</violation>
</file>

Since this is your first cubic review, here's how it works:

  • cubic automatically reviews your code and comments on bugs and improvements
  • Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
  • Ask questions if you need clarification on any suggestion

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

$ sudo ./demo-drm
```

The DRM device can be assigened via the environment variable `DRI_CARD`.
Copy link

@cubic-dev-ai cubic-dev-ai bot Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "assigened" should be spelled "assigned" to keep the documentation clear.

Prompt for AI agents
Address the following comment on README.md at line 140:

<comment>Typo: &quot;assigened&quot; should be spelled &quot;assigned&quot; to keep the documentation clear.</comment>

<file context>
@@ -128,6 +131,14 @@ $ sudo usermod -a -G video $USERNAME
+$ sudo ./demo-drm
+```
+
+The DRM device can be assigened via the environment variable `DRI_CARD`.
+
 To run demo program with the VNC backend:
</file context>
Suggested change
The DRM device can be assigened via the environment variable `DRI_CARD`.
The DRM device can be assigned via the environment variable `DRI_CARD`.
Fix with Cubic

return;

twin_coord_t width = right - left;
off_t off = tx->width * top + left;
Copy link

@cubic-dev-ai cubic-dev-ai bot Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frame buffer writes should advance by the mapped buffer pitch, but this code uses the visible width, so scanlines misalign whenever the dumb buffer pitch exceeds the mode width. Use the stride returned by create_fb() instead of tx->width to compute offsets.

Prompt for AI agents
Address the following comment on backend/drm.c at line 64:

<comment>Frame buffer writes should advance by the mapped buffer pitch, but this code uses the visible width, so scanlines misalign whenever the dumb buffer pitch exceeds the mode width. Use the stride returned by create_fb() instead of tx-&gt;width to compute offsets.</comment>

<file context>
@@ -0,0 +1,302 @@
+        return;
+
+    twin_coord_t width = right - left;
+    off_t off = tx-&gt;width * top + left;
+    uint32_t *dest =
+        (uint32_t *) ((uintptr_t) tx-&gt;fb_base + (off * sizeof(*dest)));
</file context>
Fix with Cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants