Decode full monitor vendor name from EDID using hwdata
ClosedPublic

Authored by dvratil on Jan 23 2018, 12:10 PM.

Details

Test Plan

KScreen now shows "Dell Inc." instead of DEL and
"Eizo Nano Corporation" instead of ENC in output names, which
matches closer to what's written on my monitors.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dvratil created this revision.Jan 23 2018, 12:10 PM
Restricted Application added a project: KWin. · View Herald TranscriptJan 23 2018, 12:10 PM
Restricted Application added subscribers: KWin, kwin. · View Herald Transcript
dvratil requested review of this revision.Jan 23 2018, 12:10 PM
graesslin added a subscriber: mart.Jan 23 2018, 3:47 PM

@mart: what will be the impact on plamashell if the provided values are changed?

plugins/platforms/drm/drm_output.cpp
401

Can we be sure this path is the same on all distros? Should we runtime depend on it? E.g. a cmake check whether it's available?

@mart: what will be the impact on plamashell if the provided values are changed?

I thought you said in the other review that this interface is not to be used by anyone else but kscreen :-)

plugins/platforms/drm/drm_output.cpp
401

The path is somewhat standardized between all distros. There are some distros (like Arch) that simply don't have this file because they use different upstream source (gentoo vs. fedora) that is missing that but from I can tell they still use /usr/share/hwdata for all distros I checked.

I can add a cmake check if you want.

I thought you said in the other review that this interface is not to be used by anyone else but kscreen :-)

The data which is set on the OutputDevice is also synced into the Output (see line 350). And to my knowledge plasmashell uses this information for mapping containments to it.

plugins/platforms/drm/drm_output.cpp
401

I can add a cmake check if you want.

Yep, I would prefer this. Just to also make clear at compile time that it's missing. I don't like hidden dependencies which fail on some distros.

And to my knowledge plasmashell uses this information for mapping containments to it.

Not really. We use QScreen name. QScreen's name is:

("Screen%1").arg(id))

where id is the literal wl_id.

It's pretty rubbish

This will change when we implement XdgOutputV2 (which is still in review) and when that gets into Qt.

In any case it's not a reason to block this.

Restricted Application removed a subscriber: KWin. · View Herald TranscriptJun 1 2018, 10:43 AM
davidedmundson requested changes to this revision.Jun 1 2018, 10:44 AM
I can add a cmake check if you want.

Marking as "request changes" to remove it from the queue.

This revision now requires changes to proceed.Jun 1 2018, 10:44 AM
dvratil updated this revision to Diff 76383.Feb 25 2020, 3:37 PM
  • rebase on master
  • add cmake check for hwdata
apol added a subscriber: apol.Feb 26 2020, 10:22 AM

+1 patch looks good to me

davidedmundson accepted this revision.Feb 26 2020, 10:27 AM
This revision is now accepted and ready to land.Feb 26 2020, 10:27 AM
dvratil retitled this revision from Improve EDID information on Wayland to Decode full monitor vendor name from EDID using hwdata.Feb 26 2020, 11:36 AM
dvratil edited the summary of this revision. (Show Details)
dvratil edited the test plan for this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.

This causes a build failure for me:

/home/nate/kde/src/kwin/plugins/platforms/drm/edid.cpp:149:19: error: expected primary-expression before ‘enum’
  149 |     QFile pnpFile(QStringLiteral(HWDATA_PNPIDS_FILE));
      |                   ^~~~~~~~~~~~~~
cgiboudeaux added inline comments.
config-kwin.h.cmake
28

So it's not a 'RUNTIME' build dependency as stated in the main CMakeLists.txt. It's an 'OPTIONAL' one

This causes a build failure for me:

/home/nate/kde/src/kwin/plugins/platforms/drm/edid.cpp:149:19: error: expected primary-expression before ‘enum’
  149 |     QFile pnpFile(QStringLiteral(HWDATA_PNPIDS_FILE));
      |                   ^~~~~~~~~~~~~~

Same thing here:

[ 94%] Building CXX object plugins/platforms/drm/CMakeFiles/KWinWaylandDrmBackend.dir/edid.cpp.o
/data/kde/src/kwin/plugins/platforms/drm/edid.cpp:149:34: error: expected ')'
    QFile pnpFile(QStringLiteral(HWDATA_PNPIDS_FILE));
                                 ^
cmake/modules/Findhwdata.cmake
37

The correct syntax is if NOT hwdata_DIR_NAMES OR NOT hwdata_PNPIDS_FILE)

-NOTFOUND evaluates to FALSE [1]

[1] https://cmake.org/cmake/help/latest/command/if.html

config-kwin.h.cmake
47

This produces:

#if HAVE_HWDATA
/* #undef HWDATA_PNPIDS_FILE */
#endif
cgiboudeaux reopened this revision.Feb 28 2020, 8:21 AM
This revision is now accepted and ready to land.Feb 28 2020, 8:21 AM
cgiboudeaux added inline comments.Feb 28 2020, 8:22 AM
cmake/modules/Findhwdata.cmake
37

The correct syntax is if (NOT hwdata_DIR_NAMES OR NOT hwdata_PNPIDS_FILE)

-NOTFOUND evaluates to FALSE [1]

[1] https://cmake.org/cmake/help/latest/command/if.html

cgiboudeaux closed this revision.Mar 17 2020, 2:21 PM

Closing again. Thanks for fixing.