[kmsxx] [PATCH 0/2] Support writeback connectors

[kmsxx] [PATCH 0/2] Support writeback connectors

[Laurent Pinchart]

Hello,

This small patch series adds support for writeback connectors to kmsxx.
The implementation is fairly straightforward, with patch 1/2 enabling
the enumeration of writeback connectors (conditioned to support in
libdrm), and patch 2/2 adding a method to dump the contents of a
framebuffer to allow capturing frames from writeback connectors.

Laurent Pinchart (2):
  card: Add support for writeback connectors
  utils: Add a dump_framebuffer() method

 kms++/inc/kms++/card.h              |  2 ++
 kms++/src/card.cpp                  | 13 +++++++++++++
 kms++/src/connector.cpp             |  1 +
 kms++util/CMakeLists.txt            |  2 ++
 kms++util/inc/kms++util/kms++util.h |  2 ++
 kms++util/src/drawing.cpp           |  7 +++++++
 kms++util/src/resourcemanager.cpp   |  5 +++++
 py/pykms/pykmsbase.cpp              |  1 +
 py/pykms/pykmsutil.cpp              |  6 +++++-
 utils/kmsblank.cpp                  |  5 +++++
 utils/kmstest.cpp                   |  4 ++++
 11 files changed, 47 insertions(+), 1 deletion(-)

-- 
Regards,

Laurent Pinchart

[kmsxx] [PATCH 1/2] card: Add support for writeback connectors

[Laurent Pinchart]

Enable enumeration of writeback connectors if both libdrm and the device
support it. The new Card::has_writeback() method report if the card
support writeback connectors.

Existing code that expect all connectors to model an output may be
confused by the sudden availability of new connectors. To handle this
issue,

- add a KMSXX_DISABLE_WRITEBACK_CONNECTORS environment variable to
  disable enumeration of writeback connectors, similarly to universal
  planes ; and

- ignore writeback connectors where no specific connector is requested
  (Card::get_first_connected_connector(),
  ResourceManager::reserve_connector() if no connector name is
  specified, and applications that use all connected outputs).

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 kms++/inc/kms++/card.h            |  2 ++
 kms++/src/card.cpp                | 13 +++++++++++++
 kms++/src/connector.cpp           |  1 +
 kms++util/CMakeLists.txt          |  2 ++
 kms++util/src/resourcemanager.cpp |  5 +++++
 py/pykms/pykmsbase.cpp            |  1 +
 utils/kmsblank.cpp                |  5 +++++
 utils/kmstest.cpp                 |  4 ++++
 8 files changed, 33 insertions(+)

diff --git a/kms++/inc/kms++/card.h b/kms++/inc/kms++/card.h
index 0a7eaaf81a8d..26468edbb503 100644
--- a/kms++/inc/kms++/card.h
+++ b/kms++/inc/kms++/card.h
@@ -53,6 +53,7 @@ public:
 	bool has_universal_planes() const { return m_has_universal_planes; }
 	bool has_dumb_buffers() const { return m_has_dumb; }
 	bool has_kms() const;
+	bool has_writeback() const { return m_has_writeback; }
 
 	std::vector<Connector*> get_connectors() const { return m_connectors; }
 	std::vector<Encoder*> get_encoders() const { return m_encoders; }
@@ -91,6 +92,7 @@ private:
 	bool m_has_atomic;
 	bool m_has_universal_planes;
 	bool m_has_dumb;
+	bool m_has_writeback;
 
 	CardVersion m_version;
 };
diff --git a/kms++/src/card.cpp b/kms++/src/card.cpp
index 3a7ab700ed49..cbb5d50b505f 100644
--- a/kms++/src/card.cpp
+++ b/kms++/src/card.cpp
@@ -217,6 +217,17 @@ void Card::setup()
 	m_has_atomic = false;
 #endif
 
+#ifdef DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
+	if (getenv("KMSXX_DISABLE_WRITEBACK_CONNECTORS") == 0) {
+		r = drmSetClientCap(m_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
+		m_has_writeback = r == 0;
+	} else {
+		m_has_writeback = false;
+	}
+#else
+	m_has_writeback = false;
+#endif
+
 	uint64_t has_dumb;
 	r = drmGetCap(m_fd, DRM_CAP_DUMB_BUFFER, &has_dumb);
 	m_has_dumb = r == 0 && has_dumb;
@@ -316,6 +327,8 @@ void Card::restore_modes()
 Connector* Card::get_first_connected_connector() const
 {
 	for(auto c : m_connectors) {
+		if (c->connector_type() == DRM_MODE_CONNECTOR_WRITEBACK)
+			continue;
 		if (c->connected())
 			return c;
 	}
diff --git a/kms++/src/connector.cpp b/kms++/src/connector.cpp
index a40861957c67..6f5f79f0e523 100644
--- a/kms++/src/connector.cpp
+++ b/kms++/src/connector.cpp
@@ -36,6 +36,7 @@ static const map<int, string> connector_names = {
 	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
 	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
 	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
+	{ DRM_MODE_CONNECTOR_WRITEBACK, "writeback" },
 };
 
 static const map<int, string> connection_str = {
diff --git a/kms++util/CMakeLists.txt b/kms++util/CMakeLists.txt
index 0bfb56b0d58f..362824ad13d5 100644
--- a/kms++util/CMakeLists.txt
+++ b/kms++util/CMakeLists.txt
@@ -1,3 +1,5 @@
+include_directories(${LIBDRM_INCLUDE_DIRS})
+
 file(GLOB SRCS "src/*.cpp" "src/*.h")
 file(GLOB PUB_HDRS "inc/kms++util/*.h")
 add_library(kms++util ${SRCS} ${PUB_HDRS})
diff --git a/kms++util/src/resourcemanager.cpp b/kms++util/src/resourcemanager.cpp
index 5a9f016c06ab..01edaf39202b 100644
--- a/kms++util/src/resourcemanager.cpp
+++ b/kms++util/src/resourcemanager.cpp
@@ -2,6 +2,8 @@
 #include <algorithm>
 #include <kms++util/strhelpers.h>
 
+#include <xf86drmMode.h>
+
 using namespace kms;
 using namespace std;
 
@@ -20,6 +22,9 @@ void ResourceManager::reset()
 static Connector* find_connector(Card& card, const set<Connector*> reserved)
 {
 	for (Connector* conn : card.get_connectors()) {
+		if (conn->connector_type() == DRM_MODE_CONNECTOR_WRITEBACK)
+			continue;
+
 		if (!conn->connected())
 			continue;
 
diff --git a/py/pykms/pykmsbase.cpp b/py/pykms/pykmsbase.cpp
index 0448e0264ded..8276b72b663b 100644
--- a/py/pykms/pykmsbase.cpp
+++ b/py/pykms/pykmsbase.cpp
@@ -47,6 +47,7 @@ void init_pykmsbase(py::module &m)
 			})
 
 			.def_property_readonly("has_atomic", &Card::has_atomic)
+			.def_property_readonly("has_writeback", &Card::has_writeback)
 			.def("get_prop", (Property* (Card::*)(uint32_t) const)&Card::get_prop)
 
 			.def_property_readonly("version_name", &Card::version_name);
diff --git a/utils/kmsblank.cpp b/utils/kmsblank.cpp
index 286c7f793e6d..fa18510e7299 100644
--- a/utils/kmsblank.cpp
+++ b/utils/kmsblank.cpp
@@ -5,6 +5,8 @@
 #include <kms++/kms++.h>
 #include <kms++util/kms++util.h>
 
+#include <xf86drmMode.h>
+
 using namespace std;
 using namespace kms;
 
@@ -79,6 +81,9 @@ int main(int argc, char **argv)
 
 	while (true) {
 		for (Connector* conn : conns) {
+			if (conn->connector_type() == DRM_MODE_CONNECTOR_WRITEBACK)
+				continue;
+
 			if (!conn->connected()) {
 				printf("Connector %u not connected\n", conn->idx());
 				continue;
diff --git a/utils/kmstest.cpp b/utils/kmstest.cpp
index b4bf24b52a1b..39b4b9d352a6 100644
--- a/utils/kmstest.cpp
+++ b/utils/kmstest.cpp
@@ -17,6 +17,8 @@
 
 #include <kms++util/kms++util.h>
 
+#include <xf86drmMode.h>
+
 using namespace std;
 using namespace kms;
 
@@ -643,6 +645,8 @@ static vector<OutputInfo> setups_to_outputs(Card& card, ResourceManager& resman,
 	if (outputs.empty()) {
 		// no outputs defined, show a pattern on all connected screens
 		for (Connector* conn : card.get_connectors()) {
+			if (conn->connector_type() == DRM_MODE_CONNECTOR_WRITEBACK)
+				continue;
 			if (!conn->connected())
 				continue;
 
-- 
Regards,

Laurent Pinchart

[kmsxx] [PATCH 2/2] utils: Add a dump_framebuffer() method

[Laurent Pinchart]

Add a new method to write the contents of a framebuffer to a file
descriptor. This can be used to capture frames from writeback
connectors.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 kms++util/inc/kms++util/kms++util.h | 2 ++
 kms++util/src/drawing.cpp           | 7 +++++++
 py/pykms/pykmsutil.cpp              | 6 +++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/kms++util/inc/kms++util/kms++util.h b/kms++util/inc/kms++util/kms++util.h
index 8fc6c8b81e48..58ad7e6ef39f 100644
--- a/kms++util/inc/kms++util/kms++util.h
+++ b/kms++util/inc/kms++util/kms++util.h
@@ -29,6 +29,8 @@ void draw_text(IFramebuffer& buf, uint32_t x, uint32_t y, const std::string& str
 void draw_color_bar(IFramebuffer& buf, int old_xpos, int xpos, int width);
 
 void draw_test_pattern(IFramebuffer &fb, YUVType yuvt = YUVType::BT601_Lim);
+
+void dump_framebuffer(IFramebuffer &fb, int fd);
 }
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp
index 3752f94695e0..24a995489a9e 100644
--- a/kms++util/src/drawing.cpp
+++ b/kms++util/src/drawing.cpp
@@ -1,5 +1,6 @@
 
 #include <cmath>
+#include <unistd.h>
 
 #include <kms++/kms++.h>
 #include <kms++util/kms++util.h>
@@ -569,4 +570,10 @@ void draw_text(IFramebuffer& buf, uint32_t x, uint32_t y, const string& str, RGB
 		draw_char(buf, (x + 8 * i), y, str[i], color);
 }
 
+void dump_framebuffer(IFramebuffer &fb, int fd)
+{
+	for (unsigned int i = 0; i < fb.num_planes(); ++i)
+		::write(fd, fb.map(i), fb.size(i));
+}
+
 }
diff --git a/py/pykms/pykmsutil.cpp b/py/pykms/pykmsutil.cpp
index d5d7fde4a4e1..92d8ebc684b9 100644
--- a/py/pykms/pykmsutil.cpp
+++ b/py/pykms/pykmsutil.cpp
@@ -61,5 +61,9 @@ void init_pykmstest(py::module &m)
 		draw_circle(fb, xCenter, yCenter, radius, color);
 	} );
 	m.def("draw_text", [](Framebuffer& fb, uint32_t x, uint32_t y, const string& str, RGB color) {
-		draw_text(fb, x, y, str, color); } );
+		draw_text(fb, x, y, str, color);
+	} );
+	m.def("dump_framebuffer", [](Framebuffer& fb, int fd) {
+		dump_framebuffer(fb, fd);
+	} );
 }
-- 
Regards,

Laurent Pinchart

Re: [kmsxx] [PATCH 1/2] card: Add support for writeback connectors

[Tomi Valkeinen]

Hi Laurent,

On 24/08/2020 01:11, Laurent Pinchart wrote:
> Enable enumeration of writeback connectors if both libdrm and the device
> support it. The new Card::has_writeback() method report if the card
> support writeback connectors.
> 
> Existing code that expect all connectors to model an output may be
> confused by the sudden availability of new connectors. To handle this
> issue,
> 
> - add a KMSXX_DISABLE_WRITEBACK_CONNECTORS environment variable to
>   disable enumeration of writeback connectors, similarly to universal
>   planes ; and
> 
> - ignore writeback connectors where no specific connector is requested
>   (Card::get_first_connected_connector(),
>   ResourceManager::reserve_connector() if no connector name is
>   specified, and applications that use all connected outputs).
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  kms++/inc/kms++/card.h            |  2 ++
>  kms++/src/card.cpp                | 13 +++++++++++++
>  kms++/src/connector.cpp           |  1 +
>  kms++util/CMakeLists.txt          |  2 ++
>  kms++util/src/resourcemanager.cpp |  5 +++++
>  py/pykms/pykmsbase.cpp            |  1 +
>  utils/kmsblank.cpp                |  5 +++++
>  utils/kmstest.cpp                 |  4 ++++
>  8 files changed, 33 insertions(+)
> 
> diff --git a/kms++/inc/kms++/card.h b/kms++/inc/kms++/card.h
> index 0a7eaaf81a8d..26468edbb503 100644
> --- a/kms++/inc/kms++/card.h
> +++ b/kms++/inc/kms++/card.h
> @@ -53,6 +53,7 @@ public:
>  	bool has_universal_planes() const { return m_has_universal_planes; }
>  	bool has_dumb_buffers() const { return m_has_dumb; }
>  	bool has_kms() const;
> +	bool has_writeback() const { return m_has_writeback; }
>  
>  	std::vector<Connector*> get_connectors() const { return m_connectors; }
>  	std::vector<Encoder*> get_encoders() const { return m_encoders; }
> @@ -91,6 +92,7 @@ private:
>  	bool m_has_atomic;
>  	bool m_has_universal_planes;
>  	bool m_has_dumb;
> +	bool m_has_writeback;
>  
>  	CardVersion m_version;
>  };
> diff --git a/kms++/src/card.cpp b/kms++/src/card.cpp
> index 3a7ab700ed49..cbb5d50b505f 100644
> --- a/kms++/src/card.cpp
> +++ b/kms++/src/card.cpp
> @@ -217,6 +217,17 @@ void Card::setup()
>  	m_has_atomic = false;
>  #endif
>  
> +#ifdef DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> +	if (getenv("KMSXX_DISABLE_WRITEBACK_CONNECTORS") == 0) {
> +		r = drmSetClientCap(m_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> +		m_has_writeback = r == 0;
> +	} else {
> +		m_has_writeback = false;
> +	}
> +#else
> +	m_has_writeback = false;
> +#endif
> +
>  	uint64_t has_dumb;
>  	r = drmGetCap(m_fd, DRM_CAP_DUMB_BUFFER, &has_dumb);
>  	m_has_dumb = r == 0 && has_dumb;
> @@ -316,6 +327,8 @@ void Card::restore_modes()
>  Connector* Card::get_first_connected_connector() const
>  {
>  	for(auto c : m_connectors) {
> +		if (c->connector_type() == DRM_MODE_CONNECTOR_WRITEBACK)
> +			continue;
>  		if (c->connected())
>  			return c;
>  	}
> diff --git a/kms++/src/connector.cpp b/kms++/src/connector.cpp
> index a40861957c67..6f5f79f0e523 100644
> --- a/kms++/src/connector.cpp
> +++ b/kms++/src/connector.cpp
> @@ -36,6 +36,7 @@ static const map<int, string> connector_names = {
>  	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
>  	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
>  	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
> +	{ DRM_MODE_CONNECTOR_WRITEBACK, "writeback" },
>  };
>  
>  static const map<int, string> connection_str = {
> diff --git a/kms++util/CMakeLists.txt b/kms++util/CMakeLists.txt
> index 0bfb56b0d58f..362824ad13d5 100644
> --- a/kms++util/CMakeLists.txt
> +++ b/kms++util/CMakeLists.txt
> @@ -1,3 +1,5 @@
> +include_directories(${LIBDRM_INCLUDE_DIRS})
> +
>  file(GLOB SRCS "src/*.cpp" "src/*.h")
>  file(GLOB PUB_HDRS "inc/kms++util/*.h")
>  add_library(kms++util ${SRCS} ${PUB_HDRS})
> diff --git a/kms++util/src/resourcemanager.cpp b/kms++util/src/resourcemanager.cpp
> index 5a9f016c06ab..01edaf39202b 100644
> --- a/kms++util/src/resourcemanager.cpp
> +++ b/kms++util/src/resourcemanager.cpp
> @@ -2,6 +2,8 @@
>  #include <algorithm>
>  #include <kms++util/strhelpers.h>
>  
> +#include <xf86drmMode.h>

This and adding the LIBDRM_INCLUDE_DIRS is not ok. I don't want kms++ to leak libdrm C headers.

We need to add an enum class for connector types, and map the libdrm types to that.

As a simple quick alternative, I think just conn->is_writeback() would do the trick, as we have no
users for the connector type as such.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [kmsxx] [PATCH 2/2] utils: Add a dump_framebuffer() method

[Tomi Valkeinen]

Hi Laurent,

On 24/08/2020 01:11, Laurent Pinchart wrote:
> Add a new method to write the contents of a framebuffer to a file
> descriptor. This can be used to capture frames from writeback
> connectors.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  kms++util/inc/kms++util/kms++util.h | 2 ++
>  kms++util/src/drawing.cpp           | 7 +++++++
>  py/pykms/pykmsutil.cpp              | 6 +++++-
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/kms++util/inc/kms++util/kms++util.h b/kms++util/inc/kms++util/kms++util.h
> index 8fc6c8b81e48..58ad7e6ef39f 100644
> --- a/kms++util/inc/kms++util/kms++util.h
> +++ b/kms++util/inc/kms++util/kms++util.h
> @@ -29,6 +29,8 @@ void draw_text(IFramebuffer& buf, uint32_t x, uint32_t y, const std::string& str
>  void draw_color_bar(IFramebuffer& buf, int old_xpos, int xpos, int width);
>  
>  void draw_test_pattern(IFramebuffer &fb, YUVType yuvt = YUVType::BT601_Lim);
> +
> +void dump_framebuffer(IFramebuffer &fb, int fd);
>  }
>  
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp
> index 3752f94695e0..24a995489a9e 100644
> --- a/kms++util/src/drawing.cpp
> +++ b/kms++util/src/drawing.cpp
> @@ -1,5 +1,6 @@
>  
>  #include <cmath>
> +#include <unistd.h>
>  
>  #include <kms++/kms++.h>
>  #include <kms++util/kms++util.h>
> @@ -569,4 +570,10 @@ void draw_text(IFramebuffer& buf, uint32_t x, uint32_t y, const string& str, RGB
>  		draw_char(buf, (x + 8 * i), y, str[i], color);
>  }
>  
> +void dump_framebuffer(IFramebuffer &fb, int fd)
> +{
> +	for (unsigned int i = 0; i < fb.num_planes(); ++i)
> +		::write(fd, fb.map(i), fb.size(i));
> +}
> +

Well, my first thought was that it's c++, we should use ostream or such. I don't know how that plays
with pybind11, though.

I guess on the py side you need to use os.open & co here, not the normal file operations? Or I guess
you can get the fd with fileno(), but it's still "odd" to do it in python. So using fd here is kind
of not really c++, and not really python.

That said, it's simple and works, so maybe it's fine until someone wants to put more effort to it.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [kmsxx] [PATCH 1/2] card: Add support for writeback connectors

[Laurent Pinchart]

Hi Tomi,

On Mon, Aug 24, 2020 at 09:30:35AM +0300, Tomi Valkeinen wrote:
> On 24/08/2020 01:11, Laurent Pinchart wrote:
> > Enable enumeration of writeback connectors if both libdrm and the device
> > support it. The new Card::has_writeback() method report if the card
> > support writeback connectors.
> > 
> > Existing code that expect all connectors to model an output may be
> > confused by the sudden availability of new connectors. To handle this
> > issue,
> > 
> > - add a KMSXX_DISABLE_WRITEBACK_CONNECTORS environment variable to
> >   disable enumeration of writeback connectors, similarly to universal
> >   planes ; and
> > 
> > - ignore writeback connectors where no specific connector is requested
> >   (Card::get_first_connected_connector(),
> >   ResourceManager::reserve_connector() if no connector name is
> >   specified, and applications that use all connected outputs).
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  kms++/inc/kms++/card.h            |  2 ++
> >  kms++/src/card.cpp                | 13 +++++++++++++
> >  kms++/src/connector.cpp           |  1 +
> >  kms++util/CMakeLists.txt          |  2 ++
> >  kms++util/src/resourcemanager.cpp |  5 +++++
> >  py/pykms/pykmsbase.cpp            |  1 +
> >  utils/kmsblank.cpp                |  5 +++++
> >  utils/kmstest.cpp                 |  4 ++++
> >  8 files changed, 33 insertions(+)
> > 
> > diff --git a/kms++/inc/kms++/card.h b/kms++/inc/kms++/card.h
> > index 0a7eaaf81a8d..26468edbb503 100644
> > --- a/kms++/inc/kms++/card.h
> > +++ b/kms++/inc/kms++/card.h
> > @@ -53,6 +53,7 @@ public:
> >  	bool has_universal_planes() const { return m_has_universal_planes; }
> >  	bool has_dumb_buffers() const { return m_has_dumb; }
> >  	bool has_kms() const;
> > +	bool has_writeback() const { return m_has_writeback; }
> >  
> >  	std::vector<Connector*> get_connectors() const { return m_connectors; }
> >  	std::vector<Encoder*> get_encoders() const { return m_encoders; }
> > @@ -91,6 +92,7 @@ private:
> >  	bool m_has_atomic;
> >  	bool m_has_universal_planes;
> >  	bool m_has_dumb;
> > +	bool m_has_writeback;
> >  
> >  	CardVersion m_version;
> >  };
> > diff --git a/kms++/src/card.cpp b/kms++/src/card.cpp
> > index 3a7ab700ed49..cbb5d50b505f 100644
> > --- a/kms++/src/card.cpp
> > +++ b/kms++/src/card.cpp
> > @@ -217,6 +217,17 @@ void Card::setup()
> >  	m_has_atomic = false;
> >  #endif
> >  
> > +#ifdef DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> > +	if (getenv("KMSXX_DISABLE_WRITEBACK_CONNECTORS") == 0) {
> > +		r = drmSetClientCap(m_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> > +		m_has_writeback = r == 0;
> > +	} else {
> > +		m_has_writeback = false;
> > +	}
> > +#else
> > +	m_has_writeback = false;
> > +#endif
> > +
> >  	uint64_t has_dumb;
> >  	r = drmGetCap(m_fd, DRM_CAP_DUMB_BUFFER, &has_dumb);
> >  	m_has_dumb = r == 0 && has_dumb;
> > @@ -316,6 +327,8 @@ void Card::restore_modes()
> >  Connector* Card::get_first_connected_connector() const
> >  {
> >  	for(auto c : m_connectors) {
> > +		if (c->connector_type() == DRM_MODE_CONNECTOR_WRITEBACK)
> > +			continue;
> >  		if (c->connected())
> >  			return c;
> >  	}
> > diff --git a/kms++/src/connector.cpp b/kms++/src/connector.cpp
> > index a40861957c67..6f5f79f0e523 100644
> > --- a/kms++/src/connector.cpp
> > +++ b/kms++/src/connector.cpp
> > @@ -36,6 +36,7 @@ static const map<int, string> connector_names = {
> >  	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
> >  	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
> >  	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
> > +	{ DRM_MODE_CONNECTOR_WRITEBACK, "writeback" },
> >  };
> >  
> >  static const map<int, string> connection_str = {
> > diff --git a/kms++util/CMakeLists.txt b/kms++util/CMakeLists.txt
> > index 0bfb56b0d58f..362824ad13d5 100644
> > --- a/kms++util/CMakeLists.txt
> > +++ b/kms++util/CMakeLists.txt
> > @@ -1,3 +1,5 @@
> > +include_directories(${LIBDRM_INCLUDE_DIRS})
> > +
> >  file(GLOB SRCS "src/*.cpp" "src/*.h")
> >  file(GLOB PUB_HDRS "inc/kms++util/*.h")
> >  add_library(kms++util ${SRCS} ${PUB_HDRS})
> > diff --git a/kms++util/src/resourcemanager.cpp b/kms++util/src/resourcemanager.cpp
> > index 5a9f016c06ab..01edaf39202b 100644
> > --- a/kms++util/src/resourcemanager.cpp
> > +++ b/kms++util/src/resourcemanager.cpp
> > @@ -2,6 +2,8 @@
> >  #include <algorithm>
> >  #include <kms++util/strhelpers.h>
> >  
> > +#include <xf86drmMode.h>
> 
> This and adding the LIBDRM_INCLUDE_DIRS is not ok. I don't want kms++
> to leak libdrm C headers.

I was expecting that feedback :-)

> We need to add an enum class for connector types, and map the libdrm
> types to that.
> 
> As a simple quick alternative, I think just conn->is_writeback() would
> do the trick, as we have no users for the connector type as such.

Should I go for that, or byte the bullet and wrap the connector types ?

-- 
Regards,

Laurent Pinchart

Re: [kmsxx] [PATCH 1/2] card: Add support for writeback connectors

[Tomi Valkeinen]

On 24/08/2020 17:27, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Mon, Aug 24, 2020 at 09:30:35AM +0300, Tomi Valkeinen wrote:
>> On 24/08/2020 01:11, Laurent Pinchart wrote:
>>> Enable enumeration of writeback connectors if both libdrm and the device
>>> support it. The new Card::has_writeback() method report if the card
>>> support writeback connectors.
>>>
>>> Existing code that expect all connectors to model an output may be
>>> confused by the sudden availability of new connectors. To handle this
>>> issue,
>>>
>>> - add a KMSXX_DISABLE_WRITEBACK_CONNECTORS environment variable to
>>>   disable enumeration of writeback connectors, similarly to universal
>>>   planes ; and
>>>
>>> - ignore writeback connectors where no specific connector is requested
>>>   (Card::get_first_connected_connector(),
>>>   ResourceManager::reserve_connector() if no connector name is
>>>   specified, and applications that use all connected outputs).
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>  kms++/inc/kms++/card.h            |  2 ++
>>>  kms++/src/card.cpp                | 13 +++++++++++++
>>>  kms++/src/connector.cpp           |  1 +
>>>  kms++util/CMakeLists.txt          |  2 ++
>>>  kms++util/src/resourcemanager.cpp |  5 +++++
>>>  py/pykms/pykmsbase.cpp            |  1 +
>>>  utils/kmsblank.cpp                |  5 +++++
>>>  utils/kmstest.cpp                 |  4 ++++
>>>  8 files changed, 33 insertions(+)
>>>
>>> diff --git a/kms++/inc/kms++/card.h b/kms++/inc/kms++/card.h
>>> index 0a7eaaf81a8d..26468edbb503 100644
>>> --- a/kms++/inc/kms++/card.h
>>> +++ b/kms++/inc/kms++/card.h
>>> @@ -53,6 +53,7 @@ public:
>>>  	bool has_universal_planes() const { return m_has_universal_planes; }
>>>  	bool has_dumb_buffers() const { return m_has_dumb; }
>>>  	bool has_kms() const;
>>> +	bool has_writeback() const { return m_has_writeback; }
>>>  
>>>  	std::vector<Connector*> get_connectors() const { return m_connectors; }
>>>  	std::vector<Encoder*> get_encoders() const { return m_encoders; }
>>> @@ -91,6 +92,7 @@ private:
>>>  	bool m_has_atomic;
>>>  	bool m_has_universal_planes;
>>>  	bool m_has_dumb;
>>> +	bool m_has_writeback;
>>>  
>>>  	CardVersion m_version;
>>>  };
>>> diff --git a/kms++/src/card.cpp b/kms++/src/card.cpp
>>> index 3a7ab700ed49..cbb5d50b505f 100644
>>> --- a/kms++/src/card.cpp
>>> +++ b/kms++/src/card.cpp
>>> @@ -217,6 +217,17 @@ void Card::setup()
>>>  	m_has_atomic = false;
>>>  #endif
>>>  
>>> +#ifdef DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
>>> +	if (getenv("KMSXX_DISABLE_WRITEBACK_CONNECTORS") == 0) {
>>> +		r = drmSetClientCap(m_fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
>>> +		m_has_writeback = r == 0;
>>> +	} else {
>>> +		m_has_writeback = false;
>>> +	}
>>> +#else
>>> +	m_has_writeback = false;
>>> +#endif
>>> +
>>>  	uint64_t has_dumb;
>>>  	r = drmGetCap(m_fd, DRM_CAP_DUMB_BUFFER, &has_dumb);
>>>  	m_has_dumb = r == 0 && has_dumb;
>>> @@ -316,6 +327,8 @@ void Card::restore_modes()
>>>  Connector* Card::get_first_connected_connector() const
>>>  {
>>>  	for(auto c : m_connectors) {
>>> +		if (c->connector_type() == DRM_MODE_CONNECTOR_WRITEBACK)
>>> +			continue;
>>>  		if (c->connected())
>>>  			return c;
>>>  	}
>>> diff --git a/kms++/src/connector.cpp b/kms++/src/connector.cpp
>>> index a40861957c67..6f5f79f0e523 100644
>>> --- a/kms++/src/connector.cpp
>>> +++ b/kms++/src/connector.cpp
>>> @@ -36,6 +36,7 @@ static const map<int, string> connector_names = {
>>>  	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
>>>  	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
>>>  	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
>>> +	{ DRM_MODE_CONNECTOR_WRITEBACK, "writeback" },
>>>  };
>>>  
>>>  static const map<int, string> connection_str = {
>>> diff --git a/kms++util/CMakeLists.txt b/kms++util/CMakeLists.txt
>>> index 0bfb56b0d58f..362824ad13d5 100644
>>> --- a/kms++util/CMakeLists.txt
>>> +++ b/kms++util/CMakeLists.txt
>>> @@ -1,3 +1,5 @@
>>> +include_directories(${LIBDRM_INCLUDE_DIRS})
>>> +
>>>  file(GLOB SRCS "src/*.cpp" "src/*.h")
>>>  file(GLOB PUB_HDRS "inc/kms++util/*.h")
>>>  add_library(kms++util ${SRCS} ${PUB_HDRS})
>>> diff --git a/kms++util/src/resourcemanager.cpp b/kms++util/src/resourcemanager.cpp
>>> index 5a9f016c06ab..01edaf39202b 100644
>>> --- a/kms++util/src/resourcemanager.cpp
>>> +++ b/kms++util/src/resourcemanager.cpp
>>> @@ -2,6 +2,8 @@
>>>  #include <algorithm>
>>>  #include <kms++util/strhelpers.h>
>>>  
>>> +#include <xf86drmMode.h>
>>
>> This and adding the LIBDRM_INCLUDE_DIRS is not ok. I don't want kms++
>> to leak libdrm C headers.
> 
> I was expecting that feedback :-)
> 
>> We need to add an enum class for connector types, and map the libdrm
>> types to that.
>>
>> As a simple quick alternative, I think just conn->is_writeback() would
>> do the trick, as we have no users for the connector type as such.
> 
> Should I go for that, or byte the bullet and wrap the connector types ?

At the moment I don't see a need exposing the connector types. Usually it's better to only add code
that's actually used for something =).

So I'd just go with the helper function. Although while thinking about this, I wonder if an inverse
helper would be better, like is_display() or such. As that's what the callers are looking for, a
connector that can be used as a display.

That said, I don't know what other non-display connectors we might have. Still, usually, I think a
bool helper is better if it's checking for the thing we actually want (a display).

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki