2018-01-03 12:16 GMT+01:00 Robert Foss <robert.foss@xxxxxxxxxxxxx>:
Hey Mauro,
Thanks for the patch! It builds and looks good to me, but I have some
suggestions however.
On Wed, 2018-01-03 at 11:10 +0100, Mauro Rossi wrote:
> These changes avoid following logcat error on integrated and
> dedicated GPUs:
>
> ... 2245 2245 E hwc-drm-resources: Could not find a suitable
> encoder/crtc for display 2
> ... 2245 2245 E hwc-drm-resources: Failed CreateDisplayPipe 56 with
> -19
> ... 2245 2245 E hwcomposer-drm: Can't initialize Drm object -19
It isn't quite clear clear which errors belong to which changes,
to make this patch a bit more bisectable it would be nice to see
independent commits created for each error.
Hi Robert,
In this case I honestly do not think that splitting would add too much value,
original commit (v1) is well documented in [1] and tackles with bug in drmresources.cpp
which currently fails to find workable crtc -> encoder -> display output combination
for integrated and dedicated GPUs. Besides that it was also adding DisplayPort drm mode connector type
So changes in drmresources.cpp belong to solving "Could not find a suitable encoder/crtc for display X" problem,
which is a show stopper for enabling mainline graphics in android-x86
Other developers observed independently that the current implementation is "embedded oriented" and only checks the first display output,
isn't it?
The only change I did in drmconnector.cpp is to account for the additional external drm mode connectors types
which were missing (DVID, DVII, VGA) . One question on my side: Do we need more?
Besides these minor types lists discussions, I would propose you to check with Jim Bish if he should have the credit for the patch
and review the coding of his changes.
>
> (v1) There are various places where we should be really taking
> connection
> state into account before querying the properties or assuming it
> as primary. This patch fixes them.
>
> BUG=None.
> TEST=System boots up and shows UI.
>
> (v1) Signed-off-by: Jim Bish <jim.bish@xxxxxxxxx>
>
> (v2) porting of original commit 76fb87e675 of android-ia master
> with additional external connector types (DisplayPort, DVID, DVII,
> VGA)
> Tested with i965 on Sandybridge and nouveau on GT120, GT610
The commit message isn't really following the usual format. It doesn't
need to include a changelog (that is typically placed between the "---"
markers in the email instead),
and it is missing a signoff from the submitter (you).
Sorry I don't have a signature, as usually my patches need sign-off from professionals,
I'm a (crash test dummy) hobbyist..really :-)
Maybe Rob Herring, Qiang Yu or Chih-Wei could review and sign-off the proposed patch
The BUG and TEST fields are not strictly required either, but aren't a
problem.
That part is the original Jim Bish commit message [1], my changes version is (v2)
Sorry if I was not clear enought
> ---
> drmconnector.cpp | 4 +++-
> drmresources.cpp | 9 +++++++--
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drmconnector.cpp b/drmconnector.cpp
> index 247f56b..145518f 100644
> --- a/drmconnector.cpp
> +++ b/drmconnector.cpp
> @@ -73,7 +73,9 @@ bool DrmConnector::internal() const {
> }
>
> bool DrmConnector::external() const {
> - return type_ == DRM_MODE_CONNECTOR_HDMIA;
> + return type_ == DRM_MODE_CONNECTOR_HDMIA || type_ ==
> DRM_MODE_CONNECTOR_DisplayPort ||
> + type_ == DRM_MODE_CONNECTOR_DVID || type_ ==
> DRM_MODE_CONNECTOR_DVII ||
> + type_ == DRM_MODE_CONNECTOR_VGA;
> }
The changes to external() should probably be broken out into it's own
commit, since external() is called elsewhere changes to it _could_
introduce issues.
Will you check the code, involving Jim Bish if necessary, about these potential issues?
I am available to perform changes/tests, but the original maker will be better.
Cheers
Mauro
>
> bool DrmConnector::valid_type() const {
> diff --git a/drmresources.cpp b/drmresources.cpp
> index 32dd376..d582cfe 100644
> --- a/drmresources.cpp
> +++ b/drmresources.cpp
> @@ -159,7 +159,7 @@ int DrmResources::Init() {
>
> // First look for primary amongst internal connectors
> for (auto &conn : connectors_) {
> - if (conn->internal() && !found_primary) {
> + if (conn->state() == DRM_MODE_CONNECTED && conn->internal() &&
> !found_primary) {
> conn->set_display(0);
> found_primary = true;
> } else {
> @@ -170,7 +170,7 @@ int DrmResources::Init() {
>
> // Then look for primary amongst external connectors
> for (auto &conn : connectors_) {
> - if (conn->external() && !found_primary) {
> + if (conn->state() == DRM_MODE_CONNECTED && conn->external() &&
> !found_primary) {
> conn->set_display(0);
> found_primary = true;
> }
> @@ -288,6 +288,11 @@ int DrmResources::TryEncoderForDisplay(int
> display, DrmEncoder *enc) {
>
> int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
> int display = connector->display();
> +
> + // skip not connected
> + if (connector->state() == DRM_MODE_DISCONNECTED)
> + return 0;
> +
> /* Try to use current setup first */
> if (connector->encoder()) {
> int ret = TryEncoderForDisplay(display, connector->encoder());
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
