Zijie He (Gerrit)

unread,

Sep 29, 2025, 7:19:17 PM (8 days ago) Sep 29

to Vasiliy Telezhnikov, Robert Kroeger, David Worsham, AyeAye, Chromium LUCI CQ, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org, emi...@google.com

Attention needed from Vasiliy Telezhnikov and Wez

Zijie He voted and added 1 comment

Votes added by Zijie He

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Zijie He . resolved

Please take a look, thank you.

Open in Gerrit

Related details

Attention is currently required from:

Vasiliy Telezhnikov
Wez

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Wez (Gerrit)

unread,

Sep 30, 2025, 9:18:27 AM (7 days ago) Sep 30

to Zijie He, Vasiliy Telezhnikov, Robert Kroeger, David Worsham, AyeAye, Chromium LUCI CQ, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org, emi...@google.com

Attention needed from Vasiliy Telezhnikov and Zijie He

Wez voted and added 1 comment

Votes added by Wez

Code-Review

+1

1 comment

File ui/ozone/platform/flatland/client_native_pixmap_factory_flatland.cc

Line 195, Patchset 2 (Latest):


  void UnmapImpl() {
    DCHECK(mapping_);
    DCHECK(logically_mapped_);

    // Flush the CPu cache in case the GPU reads the data directly from RAM.
    // Keep the mapping to avoid unnecessary overhead when later reusing the
    // pixmap. The actual unmap happens when the pixmap is destroyed.
    if (handle_.ram_coherency) {
      zx_status_t status =
          zx_cache_flush(mapping_, mapping_size_,
                         ZX_CACHE_FLUSH_DATA | ZX_CACHE_FLUSH_INVALIDATE);
      ZX_DCHECK(status == ZX_OK, status) << "zx_cache_flush";
    }
    logically_mapped_ = false;
  }

Wez . unresolved

Why do we need to move this into a separate function?

Open in Gerrit

Related details

Attention is currently required from:

Vasiliy Telezhnikov
Zijie He

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Wez (Gerrit)

unread,

Sep 30, 2025, 9:18:51 AM (7 days ago) Sep 30

to Zijie He, Vasiliy Telezhnikov, Robert Kroeger, David Worsham, AyeAye, Chromium LUCI CQ, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org, emi...@google.com

Attention needed from Vasiliy Telezhnikov and Zijie He

Wez added 1 comment

File ui/ozone/platform/flatland/client_native_pixmap_factory_flatland.cc

Line 195, Patchset 2 (Latest):


  void UnmapImpl() {
    DCHECK(mapping_);
    DCHECK(logically_mapped_);

    // Flush the CPu cache in case the GPU reads the data directly from RAM.
    // Keep the mapping to avoid unnecessary overhead when later reusing the
    // pixmap. The actual unmap happens when the pixmap is destroyed.
    if (handle_.ram_coherency) {
      zx_status_t status =
          zx_cache_flush(mapping_, mapping_size_,
                         ZX_CACHE_FLUSH_DATA | ZX_CACHE_FLUSH_INVALIDATE);
      ZX_DCHECK(status == ZX_OK, status) << "zx_cache_flush";
    }
    logically_mapped_ = false;
  }

Wez . resolved

Why do we need to move this into a separate function?

Wez

Ah, also called in the destructor.

Open in Gerrit

Related details

Attention is currently required from:

Vasiliy Telezhnikov
Zijie He

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Zijie He (Gerrit)

unread,

Sep 30, 2025, 11:49:37 AM (7 days ago) Sep 30

to Vasiliy Telezhnikov, Robert Kroeger, David Worsham, AyeAye, Chromium LUCI CQ, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org, emi...@google.com

Attention needed from Vasiliy Telezhnikov

Zijie He voted and added 2 comments

Votes added by Zijie He

Commit-Queue

+1

2 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Zijie He . resolved

Silly me, I forgot to add `Cq-Include-Trybots: luci.chrome.try:fuchsia-fyi-sherlock`. Hopefully it runs, we will see.

File ui/ozone/platform/flatland/client_native_pixmap_factory_flatland.cc

Line 195, Patchset 2:


  void UnmapImpl() {
    DCHECK(mapping_);
    DCHECK(logically_mapped_);

    // Flush the CPu cache in case the GPU reads the data directly from RAM.
    // Keep the mapping to avoid unnecessary overhead when later reusing the
    // pixmap. The actual unmap happens when the pixmap is destroyed.
    if (handle_.ram_coherency) {
      zx_status_t status =
          zx_cache_flush(mapping_, mapping_size_,
                         ZX_CACHE_FLUSH_DATA | ZX_CACHE_FLUSH_INVALIDATE);
      ZX_DCHECK(status == ZX_OK, status) << "zx_cache_flush";
    }
    logically_mapped_ = false;
  }

Wez . resolved

Why do we need to move this into a separate function?

Wez

Ah, also called in the destructor.

Zijie He

Exactly, and it's conditional, it was the most noticeable issue. 😞

Very likely I can skip the unmap call and assert that the unmap has been called, but it's hard to verify it across all possible scenarios.

Open in Gerrit

Related details

Attention is currently required from:

Vasiliy Telezhnikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Zijie He (Gerrit)

unread,

Sep 30, 2025, 1:13:40 PM (7 days ago) Sep 30

to Vasiliy Telezhnikov, Robert Kroeger, David Worsham, AyeAye, Chromium LUCI CQ, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org, emi...@google.com

Attention needed from Vasiliy Telezhnikov

Zijie He voted and added 1 comment

Votes added by Zijie He

Commit-Queue

+2

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Zijie He . resolved

Awesome, it runs. Let's call it a day 😊

satisfied_requirement

open

diffy

Vasiliy Telezhnikov (Gerrit)

unread,

Sep 30, 2025, 1:14:29 PM (7 days ago) Sep 30

to Zijie He, Robert Kroeger, David Worsham, AyeAye, Chromium LUCI CQ, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org, emi...@google.com

Attention needed from Zijie He

Vasiliy Telezhnikov voted and added 1 comment

Votes added by Vasiliy Telezhnikov

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Vasiliy Telezhnikov . resolved

lgtm, thanks.

Open in Gerrit

Related details

Attention is currently required from:

Zijie He

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Sep 30, 2025, 1:16:24 PM (7 days ago) Sep 30

to Zijie He, Vasiliy Telezhnikov, Robert Kroeger, David Worsham, AyeAye, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org, emi...@google.com

Chromium LUCI CQ submitted the change

Change information

Commit message:

[fuchsia] Enforce sequence affinity for ClientNativePixmapFuchsia Map/Unmap.

This change introduces a `SEQUENCE_CHECKER` to ensure that `Map()` and
`Unmap()` calls on a `ClientNativePixmapFuchsia` instance always occur
on the same sequence. The object can still be created and destroyed on
different sequences. A new `UnmapImpl()` method is added to encapsulate
the unmapping logic, used by both `Unmap()` and the destructor.

Cq-Include-Trybots: luci.chrome.try:fuchsia-fyi-sherlock

Fixed: 436929831, 436930319, 447414768

Change-Id: I1aa0e1527e6a638368ebb6d4517f720cc10280fd

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6997330

Commit-Queue: Zijie He <zij...@google.com>

Reviewed-by: Wez <w...@chromium.org>

Reviewed-by: Vasiliy Telezhnikov <vas...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1522967}

Files:

M ui/ozone/platform/flatland/client_native_pixmap_factory_flatland.cc

Change size: M

Delta: 1 file changed, 32 insertions(+), 19 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Vasiliy Telezhnikov, +1 by Wez

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[fuchsia] Enforce sequence affinity for ClientNativePixmapFuchsia Map/Unmap. [chromium/src : main]

Zijie He (Gerrit)

Zijie He voted and added 1 comment

Votes added by Zijie He

1 comment

Related details

Wez (Gerrit)

Wez voted and added 1 comment

Votes added by Wez

1 comment

Related details

Wez (Gerrit)

Wez added 1 comment

Related details

Zijie He (Gerrit)

Zijie He voted and added 2 comments

Votes added by Zijie He

2 comments

Related details

Zijie He (Gerrit)

Zijie He voted and added 1 comment

Votes added by Zijie He

1 comment

Vasiliy Telezhnikov (Gerrit)

Vasiliy Telezhnikov voted and added 1 comment

Votes added by Vasiliy Telezhnikov

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information