* [RFC PATCH v2 1/9] drm: Add writeback connector type
2016-10-26 8:54 [RFC PATCH v2 0/9] Introduce writeback connectors Brian Starkey
@ 2016-10-26 8:55 ` Brian Starkey
2016-10-26 11:00 ` Daniel Vetter
2016-10-26 8:55 ` [RFC PATCH v2 2/9] drm: mali-dp: Clear CVAL when leaving config mode Brian Starkey
` (7 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 8:55 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, linux-media
Writeback connectors represent writeback engines which can write the
CRTC output to a memory framebuffer. Add a writeback connector type and
related support functions.
Drivers should initialize a writeback connector with
drm_writeback_connector_init() which takes care of setting up all the
writeback-specific details on top of the normal functionality of
drm_connector_init().
Writeback connectors have a WRITEBACK_FB_ID property, used to set the
output framebuffer, PIXEL_FORMATS and PIXEL_FORMATS_SIZE used to expose
the supported writeback formats to userspace.
The drm_atomic core takes care of enforcing fairly strict semantics on
the use of writeback connectors. In short, a writeback connector can
only be included in a commit if it has both a framebuffer and a CRTC.
Conversely, you may not attach a framebuffer if the connector is not
attached to a CRTC, or if the CRTC is disabled.
When a framebuffer is attached to a writeback connector with the
WRITEBACK_FB_ID property, it is used only once (for the commit in which
it was included), and userspace can never read back the value of
WRITEBACK_FB_ID.
Changes since v1:
- Added drm_writeback.c + documentation
- Added helper to initialize writeback connector in one go
- Added core checks
- Squashed into a single commit
- Dropped the client cap
- Writeback framebuffers are no longer persistent
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
Documentation/gpu/drm-kms.rst | 9 ++
drivers/gpu/drm/Makefile | 2 +-
drivers/gpu/drm/drm_atomic.c | 95 +++++++++++++++++++++
drivers/gpu/drm/drm_atomic_helper.c | 5 ++
drivers/gpu/drm/drm_connector.c | 4 +-
drivers/gpu/drm/drm_writeback.c | 157 +++++++++++++++++++++++++++++++++++
include/drm/drm_atomic.h | 3 +
include/drm/drm_connector.h | 12 +++
include/drm/drm_crtc.h | 20 +++++
include/drm/drm_writeback.h | 19 +++++
include/uapi/drm/drm_mode.h | 1 +
11 files changed, 325 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/drm_writeback.c
create mode 100644 include/drm/drm_writeback.h
diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 53b872c..c3d0370 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -149,6 +149,15 @@ Connector Functions Reference
.. kernel-doc:: drivers/gpu/drm/drm_connector.c
:export:
+Writeback Connectors
+--------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
+ :doc: overview
+
+.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
+ :export:
+
Encoder Abstraction
===================
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 25c7204..2dc4a48 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -15,7 +15,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
drm_modeset_lock.o drm_atomic.o drm_bridge.o \
drm_framebuffer.o drm_connector.o drm_blend.o \
drm_encoder.o drm_mode_object.o drm_property.o \
- drm_plane.o drm_color_mgmt.o
+ drm_plane.o drm_color_mgmt.o drm_writeback.o
drm-$(CONFIG_COMPAT) += drm_ioc32.o
drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index fe37987..f434f34 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -612,6 +612,44 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
}
/**
+ * drm_atomic_connector_check - check connector state
+ * @connector: connector to check
+ * @state: connector state to check
+ *
+ * Provides core sanity checks for connector state.
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+static int drm_atomic_connector_check(struct drm_connector *connector,
+ struct drm_connector_state *state)
+{
+ struct drm_crtc_state *crtc_state;
+
+ if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
+ return 0;
+
+ if (!state->fb != !state->crtc) {
+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] framebuffer/CRTC mismatch\n",
+ connector->base.id, connector->name);
+ return -EINVAL;
+ }
+
+ if (state->crtc)
+ crtc_state = drm_atomic_get_existing_crtc_state(state->state,
+ state->crtc);
+
+ if (state->fb && !crtc_state->active) {
+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] has framebuffer, but [CRTC:%d] is off\n",
+ connector->base.id, connector->name,
+ state->crtc->base.id);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/**
* drm_atomic_get_plane_state - get plane state
* @state: global atomic state object
* @plane: plane to get state object for
@@ -1004,12 +1042,19 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
* now?) atomic writes to DPMS property:
*/
return -EINVAL;
+ } else if (property == config->writeback_fb_id_property) {
+ struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
+ drm_atomic_set_fb_for_connector(state, fb);
+ if (fb)
+ drm_framebuffer_unreference(fb);
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
} else {
return -EINVAL;
}
+
+ return 0;
}
EXPORT_SYMBOL(drm_atomic_connector_set_property);
@@ -1040,6 +1085,9 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
*val = (state->crtc) ? state->crtc->base.id : 0;
} else if (property == config->dpms_property) {
*val = connector->dpms;
+ } else if (property == config->writeback_fb_id_property) {
+ /* Writeback framebuffer is one-shot, write and forget */
+ *val = 0;
} else if (connector->funcs->atomic_get_property) {
return connector->funcs->atomic_get_property(connector,
state, property, val);
@@ -1223,6 +1271,42 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
/**
+ * drm_atomic_set_fb_for_connector - set framebuffer for (writeback) connector
+ * @conn_state: atomic state object for the connector
+ * @fb: fb to use for the connector
+ *
+ * This is used to set the framebuffer for a writeback connector, which outputs
+ * to a buffer instead of an actual physical connector.
+ * Changing the assigned framebuffer requires us to grab a reference to the new
+ * fb and drop the reference to the old fb, if there is one. This function
+ * takes care of all these details besides updating the pointer in the
+ * state object itself.
+ *
+ * Note: The only way conn_state can already have an fb set is if the commit
+ * sets the property more than once.
+ *
+ * See also DOC: overview in drm_writeback.c
+ */
+void
+drm_atomic_set_fb_for_connector(struct drm_connector_state *conn_state,
+ struct drm_framebuffer *fb)
+{
+ if (conn_state->fb)
+ drm_framebuffer_unreference(conn_state->fb);
+ if (fb)
+ drm_framebuffer_reference(fb);
+ conn_state->fb = fb;
+
+ if (fb)
+ DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n",
+ fb->base.id, conn_state);
+ else
+ DRM_DEBUG_ATOMIC("Set [NOFB] for connector state %p\n",
+ conn_state);
+}
+EXPORT_SYMBOL(drm_atomic_set_fb_for_connector);
+
+/**
* drm_atomic_add_affected_connectors - add connectors for crtc
* @state: atomic state
* @crtc: DRM crtc
@@ -1376,6 +1460,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
struct drm_plane_state *plane_state;
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
+ struct drm_connector *conn;
+ struct drm_connector_state *conn_state;
int i, ret = 0;
DRM_DEBUG_ATOMIC("checking %p\n", state);
@@ -1398,6 +1484,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
}
}
+ for_each_connector_in_state(state, conn, conn_state, i) {
+ ret = drm_atomic_connector_check(conn, conn_state);
+ if (ret) {
+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] atomic core check failed\n",
+ conn->base.id, conn->name);
+ return ret;
+ }
+ }
+
if (config->funcs->atomic_check)
ret = config->funcs->atomic_check(state->dev, state);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2c44de3..bba8672 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3233,6 +3233,9 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
memcpy(state, connector->state, sizeof(*state));
if (state->crtc)
drm_connector_reference(connector);
+
+ /* Don't copy over framebuffers, they are used only once */
+ state->fb = NULL;
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
@@ -3360,6 +3363,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
*/
if (state->crtc)
drm_connector_unreference(state->connector);
+ if (state->fb)
+ drm_framebuffer_unreference(state->fb);
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 2db7fb5..e67084d 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -86,6 +86,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
{ DRM_MODE_CONNECTOR_DSI, "DSI" },
{ DRM_MODE_CONNECTOR_DPI, "DPI" },
+ { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
};
void drm_connector_ida_init(void)
@@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev,
list_add_tail(&connector->head, &config->connector_list);
config->num_connector++;
- if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
+ if ((connector_type != DRM_MODE_CONNECTOR_VIRTUAL) &&
+ (connector_type != DRM_MODE_CONNECTOR_WRITEBACK))
drm_object_attach_property(&connector->base,
config->edid_property,
0);
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
new file mode 100644
index 0000000..5a6e0ad
--- /dev/null
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -0,0 +1,157 @@
+/*
+ * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
+ * Author: Brian Starkey <brian.starkey@arm.com>
+ *
+ * This program is free software and is provided to you under the terms of the
+ * GNU General Public License version 2 as published by the Free Software
+ * Foundation, and any use by you of this program is subject to the terms
+ * of such GNU licence.
+ */
+
+#include <drm/drm_crtc.h>
+#include <drm/drm_property.h>
+#include <drm/drmP.h>
+
+/**
+ * DOC: overview
+ *
+ * Writeback connectors are used to expose hardware which can write the output
+ * from a CRTC to a memory buffer. They are used and act similarly to other
+ * types of connectors, with some important differences:
+ * - Writeback connectors don't provide a way to output visually to the user.
+ * - Writeback connectors should always report as "disconnected" (so that
+ * clients which don't understand them will ignore them).
+ * - Writeback connectors don't have EDID.
+ *
+ * Writeback connectors may only be attached to a CRTC when they have a
+ * framebuffer attached, and may only have a framebuffer attached when they are
+ * attached to a CRTC. The WRITEBACK_FB_ID property which sets the framebuffer
+ * applies only to a single commit (see below), which means that each and every
+ * commit which makes use of a writeback connector must set both its CRTC_ID and
+ * WRITEBACK_FB_ID. It also means that the connector's CRTC_ID must be
+ * explicitly cleared in order to make a subsequent commit which doesn't use
+ * writeback.
+ *
+ * Writeback connectors have several additional properties, which userspace
+ * can use to query and control them:
+ *
+ * "WRITEBACK_FB_ID":
+ * Write-only object property storing a DRM_MODE_OBJECT_FB: it stores the
+ * framebuffer to be written by the writeback connector. This property is
+ * similar to the FB_ID property on planes, but will always read as zero
+ * and is not preserved across commits.
+ * Userspace must set this property to an output buffer every time it
+ * wishes the buffer to get filled.
+ *
+ * "PIXEL_FORMATS":
+ * Immutable blob property to store the supported pixel formats table. The
+ * data is an array of u32 DRM_FORMAT_* fourcc values.
+ * Userspace can use this blob to find out what pixel formats are supported
+ * by the connector's writeback engine.
+ *
+ * "PIXEL_FORMATS_SIZE":
+ * Immutable unsigned range property storing the number of entries in the
+ * PIXEL_FORMATS array.
+ */
+
+/**
+ * create_writeback_properties - Create writeback connector-specific properties
+ * @dev: DRM device
+ *
+ * Create the properties specific to writeback connectors. These will be
+ * attached to the connector and initialised by drm_writeback_connector_init.
+ *
+ * Returns: true on success, or false if any property creation fails.
+ */
+static bool create_writeback_properties(struct drm_device *dev)
+{
+ struct drm_property *prop;
+
+ if (!dev->mode_config.writeback_fb_id_property) {
+ prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
+ "WRITEBACK_FB_ID",
+ DRM_MODE_OBJECT_FB);
+ if (!prop)
+ return false;
+ dev->mode_config.writeback_fb_id_property = prop;
+ }
+
+ if (!dev->mode_config.pixel_formats_property) {
+ prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,
+ "PIXEL_FORMATS", 0);
+ if (!prop)
+ return false;
+ dev->mode_config.pixel_formats_property = prop;
+ }
+
+ if (!dev->mode_config.pixel_formats_size_property) {
+ prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
+ "PIXEL_FORMATS_SIZE", 0,
+ UINT_MAX);
+ if (!prop)
+ return false;
+ dev->mode_config.pixel_formats_size_property = prop;
+ }
+
+ return true;
+}
+
+/**
+ * drm_writeback_connector_init - Initialize a writeback connector and its properties
+ * @dev: DRM device
+ * @connector: Connector to initialize
+ * @funcs: Connector funcs vtable
+ * @formats: Array of supported pixel formats for the writeback engine
+ * @n_formats: Length of the formats array
+ *
+ * This function creates the writeback-connector-specific properties if they
+ * have not been already created, initializes the connector as
+ * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
+ * values.
+ *
+ * Drivers should always use this function instead of drm_connector_init() to
+ * set up writeback connectors.
+ *
+ * Returns: 0 on success, or a negative error code
+ */
+int drm_writeback_connector_init(struct drm_device *dev,
+ struct drm_connector *connector,
+ const struct drm_connector_funcs *funcs,
+ u32 *formats, int n_formats)
+{
+ int ret;
+ struct drm_property_blob *blob;
+ struct drm_mode_config *config = &dev->mode_config;
+
+ if (!create_writeback_properties(dev))
+ return -EINVAL;
+
+ blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
+ formats);
+ if (IS_ERR(blob))
+ return PTR_ERR(blob);
+
+ ret = drm_connector_init(dev, connector, funcs,
+ DRM_MODE_CONNECTOR_WRITEBACK);
+ if (ret)
+ goto fail;
+
+ drm_object_attach_property(&connector->base,
+ config->writeback_fb_id_property, 0);
+
+ drm_object_attach_property(&connector->base,
+ config->pixel_formats_property,
+ blob->base.id);
+ connector->pixel_formats_blob_ptr = blob;
+
+ drm_object_attach_property(&connector->base,
+ config->pixel_formats_size_property,
+ n_formats);
+
+ return 0;
+
+fail:
+ drm_property_unreference_blob(blob);
+ return ret;
+}
+EXPORT_SYMBOL(drm_writeback_connector_init);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 9701f2d..d9aff06 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -319,6 +319,9 @@ void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
int __must_check
drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
struct drm_crtc *crtc);
+void
+drm_atomic_set_fb_for_connector(struct drm_connector_state *conn_state,
+ struct drm_framebuffer *fb);
int __must_check
drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
struct drm_crtc *crtc);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ac9d7d8..a5e3778 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -198,6 +198,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
* @connector: backpointer to the connector
* @best_encoder: can be used by helpers and drivers to select the encoder
* @state: backpointer to global drm_atomic_state
+ * @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK
*/
struct drm_connector_state {
struct drm_connector *connector;
@@ -213,6 +214,8 @@ struct drm_connector_state {
struct drm_encoder *best_encoder;
struct drm_atomic_state *state;
+
+ struct drm_framebuffer *fb; /* do not write directly, use drm_atomic_set_fb_for_connector() */
};
/**
@@ -612,6 +615,15 @@ struct drm_connector {
*/
struct drm_property_blob *tile_blob_ptr;
+ /**
+ * @pixel_formats_blob_ptr:
+ *
+ * DRM blob property data for the pixel formats list on writeback
+ * connectors
+ * See also DOC: overview in drm_writeback.c
+ */
+ struct drm_property_blob *pixel_formats_blob_ptr;
+
/* should we poll this connector for connects and disconnects */
/* hot plug detectable */
#define DRM_CONNECTOR_POLL_HPD (1 << 0)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index fe20d8f..378baee2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1345,6 +1345,26 @@ struct drm_mode_config {
*/
struct drm_property *suggested_y_property;
+ /**
+ * @writeback_fb_id_property: Property for writeback connectors, storing
+ * the ID of the output framebuffer.
+ * See also DOC: overview in drm_writeback.c
+ */
+ struct drm_property *writeback_fb_id_property;
+ /**
+ * @pixel_formats_property: Property for writeback connectors, storing
+ * an array of the supported pixel formats for the writeback engine
+ * (read-only).
+ * See also DOC: overview in drm_writeback.c
+ */
+ struct drm_property *pixel_formats_property;
+ /**
+ * @pixel_formats_size_property: Property for writeback connectors,
+ * stating the size of the pixel formats array (read-only).
+ * See also DOC: overview in drm_writeback.c
+ */
+ struct drm_property *pixel_formats_size_property;
+
/* dumb ioctl parameters */
uint32_t preferred_depth, prefer_shadow;
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
new file mode 100644
index 0000000..afdc2742
--- /dev/null
+++ b/include/drm/drm_writeback.h
@@ -0,0 +1,19 @@
+/*
+ * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
+ * Author: Brian Starkey <brian.starkey@arm.com>
+ *
+ * This program is free software and is provided to you under the terms of the
+ * GNU General Public License version 2 as published by the Free Software
+ * Foundation, and any use by you of this program is subject to the terms
+ * of such GNU licence.
+ */
+
+#ifndef __DRM_WRITEBACK_H__
+#define __DRM_WRITEBACK_H__
+
+int drm_writeback_connector_init(struct drm_device *dev,
+ struct drm_connector *connector,
+ const struct drm_connector_funcs *funcs,
+ u32 *formats, int n_formats);
+
+#endif
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index df0e350..e9cb4fe 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -247,6 +247,7 @@ struct drm_mode_get_encoder {
#define DRM_MODE_CONNECTOR_VIRTUAL 15
#define DRM_MODE_CONNECTOR_DSI 16
#define DRM_MODE_CONNECTOR_DPI 17
+#define DRM_MODE_CONNECTOR_WRITEBACK 18
struct drm_mode_get_connector {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 1/9] drm: Add writeback connector type
2016-10-26 8:55 ` [RFC PATCH v2 1/9] drm: Add writeback connector type Brian Starkey
@ 2016-10-26 11:00 ` Daniel Vetter
2016-10-26 12:42 ` Brian Starkey
0 siblings, 1 reply; 25+ messages in thread
From: Daniel Vetter @ 2016-10-26 11:00 UTC (permalink / raw)
To: Brian Starkey; +Cc: dri-devel, linux-kernel, linux-media
On Wed, Oct 26, 2016 at 09:55:00AM +0100, Brian Starkey wrote:
> Writeback connectors represent writeback engines which can write the
> CRTC output to a memory framebuffer. Add a writeback connector type and
> related support functions.
>
> Drivers should initialize a writeback connector with
> drm_writeback_connector_init() which takes care of setting up all the
> writeback-specific details on top of the normal functionality of
> drm_connector_init().
>
> Writeback connectors have a WRITEBACK_FB_ID property, used to set the
> output framebuffer, PIXEL_FORMATS and PIXEL_FORMATS_SIZE used to expose
> the supported writeback formats to userspace.
>
> The drm_atomic core takes care of enforcing fairly strict semantics on
> the use of writeback connectors. In short, a writeback connector can
> only be included in a commit if it has both a framebuffer and a CRTC.
> Conversely, you may not attach a framebuffer if the connector is not
> attached to a CRTC, or if the CRTC is disabled.
>
> When a framebuffer is attached to a writeback connector with the
> WRITEBACK_FB_ID property, it is used only once (for the commit in which
> it was included), and userspace can never read back the value of
> WRITEBACK_FB_ID.
>
> Changes since v1:
> - Added drm_writeback.c + documentation
> - Added helper to initialize writeback connector in one go
> - Added core checks
> - Squashed into a single commit
> - Dropped the client cap
> - Writeback framebuffers are no longer persistent
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
I think this is looking pretty good. Bunch of nitpicks below, but
otherwise I think we just need the userspace for this, plus an ack from
other people interested in this, and it's good to get merged.
-Daniel
> ---
> Documentation/gpu/drm-kms.rst | 9 ++
> drivers/gpu/drm/Makefile | 2 +-
> drivers/gpu/drm/drm_atomic.c | 95 +++++++++++++++++++++
> drivers/gpu/drm/drm_atomic_helper.c | 5 ++
> drivers/gpu/drm/drm_connector.c | 4 +-
> drivers/gpu/drm/drm_writeback.c | 157 +++++++++++++++++++++++++++++++++++
> include/drm/drm_atomic.h | 3 +
> include/drm/drm_connector.h | 12 +++
> include/drm/drm_crtc.h | 20 +++++
> include/drm/drm_writeback.h | 19 +++++
> include/uapi/drm/drm_mode.h | 1 +
> 11 files changed, 325 insertions(+), 2 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_writeback.c
> create mode 100644 include/drm/drm_writeback.h
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 53b872c..c3d0370 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -149,6 +149,15 @@ Connector Functions Reference
> .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> :export:
>
> +Writeback Connectors
> +--------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
> + :doc: overview
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
> + :export:
> +
> Encoder Abstraction
> ===================
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 25c7204..2dc4a48 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -15,7 +15,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
> drm_modeset_lock.o drm_atomic.o drm_bridge.o \
> drm_framebuffer.o drm_connector.o drm_blend.o \
> drm_encoder.o drm_mode_object.o drm_property.o \
> - drm_plane.o drm_color_mgmt.o
> + drm_plane.o drm_color_mgmt.o drm_writeback.o
>
> drm-$(CONFIG_COMPAT) += drm_ioc32.o
> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fe37987..f434f34 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -612,6 +612,44 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
> }
>
> /**
> + * drm_atomic_connector_check - check connector state
> + * @connector: connector to check
> + * @state: connector state to check
> + *
> + * Provides core sanity checks for connector state.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +static int drm_atomic_connector_check(struct drm_connector *connector,
> + struct drm_connector_state *state)
> +{
> + struct drm_crtc_state *crtc_state;
> +
> + if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> + return 0;
> +
> + if (!state->fb != !state->crtc) {
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] framebuffer/CRTC mismatch\n",
> + connector->base.id, connector->name);
> + return -EINVAL;
> + }
> +
> + if (state->crtc)
> + crtc_state = drm_atomic_get_existing_crtc_state(state->state,
> + state->crtc);
> +
> + if (state->fb && !crtc_state->active) {
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] has framebuffer, but [CRTC:%d] is off\n",
> + connector->base.id, connector->name,
> + state->crtc->base.id);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +/**
> * drm_atomic_get_plane_state - get plane state
> * @state: global atomic state object
> * @plane: plane to get state object for
> @@ -1004,12 +1042,19 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> * now?) atomic writes to DPMS property:
> */
> return -EINVAL;
> + } else if (property == config->writeback_fb_id_property) {
> + struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> + drm_atomic_set_fb_for_connector(state, fb);
> + if (fb)
> + drm_framebuffer_unreference(fb);
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> } else {
> return -EINVAL;
> }
> +
> + return 0;
> }
> EXPORT_SYMBOL(drm_atomic_connector_set_property);
>
> @@ -1040,6 +1085,9 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> *val = (state->crtc) ? state->crtc->base.id : 0;
> } else if (property == config->dpms_property) {
> *val = connector->dpms;
> + } else if (property == config->writeback_fb_id_property) {
> + /* Writeback framebuffer is one-shot, write and forget */
> + *val = 0;
> } else if (connector->funcs->atomic_get_property) {
> return connector->funcs->atomic_get_property(connector,
> state, property, val);
> @@ -1223,6 +1271,42 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
> EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>
> /**
> + * drm_atomic_set_fb_for_connector - set framebuffer for (writeback) connector
Hm, I'd call this set_writeback_fb_for_connector, for even more
obviousness.
> + * @conn_state: atomic state object for the connector
> + * @fb: fb to use for the connector
> + *
> + * This is used to set the framebuffer for a writeback connector, which outputs
> + * to a buffer instead of an actual physical connector.
> + * Changing the assigned framebuffer requires us to grab a reference to the new
> + * fb and drop the reference to the old fb, if there is one. This function
> + * takes care of all these details besides updating the pointer in the
> + * state object itself.
> + *
> + * Note: The only way conn_state can already have an fb set is if the commit
> + * sets the property more than once.
> + *
> + * See also DOC: overview in drm_writeback.c
> + */
> +void
> +drm_atomic_set_fb_for_connector(struct drm_connector_state *conn_state,
> + struct drm_framebuffer *fb)
> +{
Should we have a WARN_ON(conn_state->connector->type != WRITEBACK here)?
Just for safety.
> + if (conn_state->fb)
> + drm_framebuffer_unreference(conn_state->fb);
> + if (fb)
> + drm_framebuffer_reference(fb);
> + conn_state->fb = fb;
> +
> + if (fb)
> + DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n",
> + fb->base.id, conn_state);
> + else
> + DRM_DEBUG_ATOMIC("Set [NOFB] for connector state %p\n",
> + conn_state);
> +}
> +EXPORT_SYMBOL(drm_atomic_set_fb_for_connector);
> +
> +/**
> * drm_atomic_add_affected_connectors - add connectors for crtc
> * @state: atomic state
> * @crtc: DRM crtc
> @@ -1376,6 +1460,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> struct drm_plane_state *plane_state;
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> + struct drm_connector *conn;
> + struct drm_connector_state *conn_state;
> int i, ret = 0;
>
> DRM_DEBUG_ATOMIC("checking %p\n", state);
> @@ -1398,6 +1484,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> }
> }
>
> + for_each_connector_in_state(state, conn, conn_state, i) {
> + ret = drm_atomic_connector_check(conn, conn_state);
> + if (ret) {
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] atomic core check failed\n",
> + conn->base.id, conn->name);
> + return ret;
> + }
> + }
> +
> if (config->funcs->atomic_check)
> ret = config->funcs->atomic_check(state->dev, state);
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2c44de3..bba8672 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3233,6 +3233,9 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
> memcpy(state, connector->state, sizeof(*state));
> if (state->crtc)
> drm_connector_reference(connector);
> +
> + /* Don't copy over framebuffers, they are used only once */
> + state->fb = NULL;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>
> @@ -3360,6 +3363,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
> */
> if (state->crtc)
> drm_connector_unreference(state->connector);
> + if (state->fb)
> + drm_framebuffer_unreference(state->fb);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 2db7fb5..e67084d 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -86,6 +86,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
> { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
> { DRM_MODE_CONNECTOR_DSI, "DSI" },
> { DRM_MODE_CONNECTOR_DPI, "DPI" },
> + { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
> };
>
> void drm_connector_ida_init(void)
> @@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev,
> list_add_tail(&connector->head, &config->connector_list);
> config->num_connector++;
>
> - if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
> + if ((connector_type != DRM_MODE_CONNECTOR_VIRTUAL) &&
> + (connector_type != DRM_MODE_CONNECTOR_WRITEBACK))
> drm_object_attach_property(&connector->base,
> config->edid_property,
> 0);
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> new file mode 100644
> index 0000000..5a6e0ad
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -0,0 +1,157 @@
> +/*
> + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
> + * Author: Brian Starkey <brian.starkey@arm.com>
> + *
> + * This program is free software and is provided to you under the terms of the
> + * GNU General Public License version 2 as published by the Free Software
> + * Foundation, and any use by you of this program is subject to the terms
> + * of such GNU licence.
> + */
> +
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_property.h>
> +#include <drm/drmP.h>
> +
> +/**
> + * DOC: overview
> + *
> + * Writeback connectors are used to expose hardware which can write the output
> + * from a CRTC to a memory buffer. They are used and act similarly to other
> + * types of connectors, with some important differences:
> + * - Writeback connectors don't provide a way to output visually to the user.
> + * - Writeback connectors should always report as "disconnected" (so that
> + * clients which don't understand them will ignore them).
> + * - Writeback connectors don't have EDID.
> + *
> + * Writeback connectors may only be attached to a CRTC when they have a
> + * framebuffer attached, and may only have a framebuffer attached when they are
> + * attached to a CRTC. The WRITEBACK_FB_ID property which sets the framebuffer
> + * applies only to a single commit (see below), which means that each and every
> + * commit which makes use of a writeback connector must set both its CRTC_ID and
> + * WRITEBACK_FB_ID. It also means that the connector's CRTC_ID must be
> + * explicitly cleared in order to make a subsequent commit which doesn't use
> + * writeback.
Hm, this is a bit an ugly fallout from our safety checks, but I guess it's
a reasonable requirement for userspace. Writeback is kinda special. Or
should we make the ->crtc pointer one-shot too? Of course only for
writeback pointers. I think that would make is simpler to use in libraries
like igt.
> + *
> + * Writeback connectors have several additional properties, which userspace
> + * can use to query and control them:
> + *
> + * "WRITEBACK_FB_ID":
> + * Write-only object property storing a DRM_MODE_OBJECT_FB: it stores the
> + * framebuffer to be written by the writeback connector. This property is
> + * similar to the FB_ID property on planes, but will always read as zero
> + * and is not preserved across commits.
> + * Userspace must set this property to an output buffer every time it
> + * wishes the buffer to get filled.
> + *
> + * "PIXEL_FORMATS":
> + * Immutable blob property to store the supported pixel formats table. The
> + * data is an array of u32 DRM_FORMAT_* fourcc values.
> + * Userspace can use this blob to find out what pixel formats are supported
> + * by the connector's writeback engine.
> + *
> + * "PIXEL_FORMATS_SIZE":
> + * Immutable unsigned range property storing the number of entries in the
> + * PIXEL_FORMATS array.
Blob properties already have a size, you don't need to specify it.
Gamma tables have a size since they start out with a default of NULL, so
userspace has no idea what the size should be.
Docs are missing to clarify when the writeout is complete. Should we wait
with merging this until out fences are supported, or should we add a bit
of placeholder text here that the writeback will be completed 1 vblank
after this atomic commit was committed? I'm leaning towards including the
writeout out fences from the start.
> + */
> +
> +/**
> + * create_writeback_properties - Create writeback connector-specific properties
> + * @dev: DRM device
> + *
> + * Create the properties specific to writeback connectors. These will be
> + * attached to the connector and initialised by drm_writeback_connector_init.
> + *
> + * Returns: true on success, or false if any property creation fails.
> + */
We don't do kernel-doc for non-EXPORT_SYMBOL functions, since the intended
audience is driver authors. And I think this function here is clear enough
that we don't need even a normal comment for it.
> +static bool create_writeback_properties(struct drm_device *dev)
> +{
> + struct drm_property *prop;
> +
> + if (!dev->mode_config.writeback_fb_id_property) {
> + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> + "WRITEBACK_FB_ID",
> + DRM_MODE_OBJECT_FB);
> + if (!prop)
> + return false;
> + dev->mode_config.writeback_fb_id_property = prop;
> + }
> +
> + if (!dev->mode_config.pixel_formats_property) {
> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,
> + "PIXEL_FORMATS", 0);
> + if (!prop)
> + return false;
> + dev->mode_config.pixel_formats_property = prop;
> + }
> +
> + if (!dev->mode_config.pixel_formats_size_property) {
> + prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
> + "PIXEL_FORMATS_SIZE", 0,
> + UINT_MAX);
> + if (!prop)
> + return false;
> + dev->mode_config.pixel_formats_size_property = prop;
> + }
> +
> + return true;
> +}
> +
> +/**
> + * drm_writeback_connector_init - Initialize a writeback connector and its properties
> + * @dev: DRM device
> + * @connector: Connector to initialize
> + * @funcs: Connector funcs vtable
> + * @formats: Array of supported pixel formats for the writeback engine
> + * @n_formats: Length of the formats array
> + *
> + * This function creates the writeback-connector-specific properties if they
> + * have not been already created, initializes the connector as
> + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
> + * values.
> + *
> + * Drivers should always use this function instead of drm_connector_init() to
> + * set up writeback connectors.
> + *
> + * Returns: 0 on success, or a negative error code
> + */
> +int drm_writeback_connector_init(struct drm_device *dev,
> + struct drm_connector *connector,
> + const struct drm_connector_funcs *funcs,
> + u32 *formats, int n_formats)
> +{
> + int ret;
> + struct drm_property_blob *blob;
> + struct drm_mode_config *config = &dev->mode_config;
> +
> + if (!create_writeback_properties(dev))
> + return -EINVAL;
> +
> + blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
> + formats);
> + if (IS_ERR(blob))
> + return PTR_ERR(blob);
> +
> + ret = drm_connector_init(dev, connector, funcs,
> + DRM_MODE_CONNECTOR_WRITEBACK);
> + if (ret)
> + goto fail;
> +
> + drm_object_attach_property(&connector->base,
> + config->writeback_fb_id_property, 0);
> +
> + drm_object_attach_property(&connector->base,
> + config->pixel_formats_property,
> + blob->base.id);
> + connector->pixel_formats_blob_ptr = blob;
> +
> + drm_object_attach_property(&connector->base,
> + config->pixel_formats_size_property,
> + n_formats);
> +
> + return 0;
> +
> +fail:
> + drm_property_unreference_blob(blob);
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_writeback_connector_init);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 9701f2d..d9aff06 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -319,6 +319,9 @@ void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> int __must_check
> drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
> struct drm_crtc *crtc);
> +void
> +drm_atomic_set_fb_for_connector(struct drm_connector_state *conn_state,
> + struct drm_framebuffer *fb);
> int __must_check
> drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
> struct drm_crtc *crtc);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ac9d7d8..a5e3778 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -198,6 +198,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
> * @connector: backpointer to the connector
> * @best_encoder: can be used by helpers and drivers to select the encoder
> * @state: backpointer to global drm_atomic_state
> + * @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK
> */
> struct drm_connector_state {
> struct drm_connector *connector;
> @@ -213,6 +214,8 @@ struct drm_connector_state {
> struct drm_encoder *best_encoder;
>
> struct drm_atomic_state *state;
> +
> + struct drm_framebuffer *fb; /* do not write directly, use drm_atomic_set_fb_for_connector() */
> };
>
> /**
> @@ -612,6 +615,15 @@ struct drm_connector {
> */
> struct drm_property_blob *tile_blob_ptr;
>
> + /**
> + * @pixel_formats_blob_ptr:
> + *
> + * DRM blob property data for the pixel formats list on writeback
> + * connectors
> + * See also DOC: overview in drm_writeback.c
Ah, here's the link you wanted. Imo just use
drm_writeback_connector_init() instead, that's a link close enough to your
target.
> + */
> + struct drm_property_blob *pixel_formats_blob_ptr;
I'g go with a writeout prefix.
> +
> /* should we poll this connector for connects and disconnects */
> /* hot plug detectable */
> #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index fe20d8f..378baee2 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1345,6 +1345,26 @@ struct drm_mode_config {
> */
> struct drm_property *suggested_y_property;
>
> + /**
> + * @writeback_fb_id_property: Property for writeback connectors, storing
> + * the ID of the output framebuffer.
> + * See also DOC: overview in drm_writeback.c
Same here and all of the below.
> + */
> + struct drm_property *writeback_fb_id_property;
> + /**
> + * @pixel_formats_property: Property for writeback connectors, storing
> + * an array of the supported pixel formats for the writeback engine
> + * (read-only).
> + * See also DOC: overview in drm_writeback.c
> + */
> + struct drm_property *pixel_formats_property;
> + /**
> + * @pixel_formats_size_property: Property for writeback connectors,
> + * stating the size of the pixel formats array (read-only).
> + * See also DOC: overview in drm_writeback.c
> + */
> + struct drm_property *pixel_formats_size_property;
> +
> /* dumb ioctl parameters */
> uint32_t preferred_depth, prefer_shadow;
>
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> new file mode 100644
> index 0000000..afdc2742
> --- /dev/null
> +++ b/include/drm/drm_writeback.h
> @@ -0,0 +1,19 @@
> +/*
> + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
> + * Author: Brian Starkey <brian.starkey@arm.com>
> + *
> + * This program is free software and is provided to you under the terms of the
> + * GNU General Public License version 2 as published by the Free Software
> + * Foundation, and any use by you of this program is subject to the terms
> + * of such GNU licence.
> + */
> +
> +#ifndef __DRM_WRITEBACK_H__
> +#define __DRM_WRITEBACK_H__
> +
> +int drm_writeback_connector_init(struct drm_device *dev,
> + struct drm_connector *connector,
> + const struct drm_connector_funcs *funcs,
> + u32 *formats, int n_formats);
> +
> +#endif
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index df0e350..e9cb4fe 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -247,6 +247,7 @@ struct drm_mode_get_encoder {
> #define DRM_MODE_CONNECTOR_VIRTUAL 15
> #define DRM_MODE_CONNECTOR_DSI 16
> #define DRM_MODE_CONNECTOR_DPI 17
> +#define DRM_MODE_CONNECTOR_WRITEBACK 18
>
> struct drm_mode_get_connector {
>
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 1/9] drm: Add writeback connector type
2016-10-26 11:00 ` Daniel Vetter
@ 2016-10-26 12:42 ` Brian Starkey
2016-10-26 13:05 ` Daniel Vetter
0 siblings, 1 reply; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 12:42 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel, linux-kernel, linux-media
On Wed, Oct 26, 2016 at 01:00:21PM +0200, Daniel Vetter wrote:
>On Wed, Oct 26, 2016 at 09:55:00AM +0100, Brian Starkey wrote:
>> Writeback connectors represent writeback engines which can write the
>> CRTC output to a memory framebuffer. Add a writeback connector type and
>> related support functions.
>>
>> Drivers should initialize a writeback connector with
>> drm_writeback_connector_init() which takes care of setting up all the
>> writeback-specific details on top of the normal functionality of
>> drm_connector_init().
>>
>> Writeback connectors have a WRITEBACK_FB_ID property, used to set the
>> output framebuffer, PIXEL_FORMATS and PIXEL_FORMATS_SIZE used to expose
>> the supported writeback formats to userspace.
>>
>> The drm_atomic core takes care of enforcing fairly strict semantics on
>> the use of writeback connectors. In short, a writeback connector can
>> only be included in a commit if it has both a framebuffer and a CRTC.
>> Conversely, you may not attach a framebuffer if the connector is not
>> attached to a CRTC, or if the CRTC is disabled.
>>
>> When a framebuffer is attached to a writeback connector with the
>> WRITEBACK_FB_ID property, it is used only once (for the commit in which
>> it was included), and userspace can never read back the value of
>> WRITEBACK_FB_ID.
>>
>> Changes since v1:
>> - Added drm_writeback.c + documentation
>> - Added helper to initialize writeback connector in one go
>> - Added core checks
>> - Squashed into a single commit
>> - Dropped the client cap
>> - Writeback framebuffers are no longer persistent
>>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>
>I think this is looking pretty good. Bunch of nitpicks below, but
>otherwise I think we just need the userspace for this, plus an ack from
>other people interested in this, and it's good to get merged.
>-Daniel
>
Thanks for having another look,
>> ---
>> Documentation/gpu/drm-kms.rst | 9 ++
>> drivers/gpu/drm/Makefile | 2 +-
>> drivers/gpu/drm/drm_atomic.c | 95 +++++++++++++++++++++
>> drivers/gpu/drm/drm_atomic_helper.c | 5 ++
>> drivers/gpu/drm/drm_connector.c | 4 +-
>> drivers/gpu/drm/drm_writeback.c | 157 +++++++++++++++++++++++++++++++++++
>> include/drm/drm_atomic.h | 3 +
>> include/drm/drm_connector.h | 12 +++
>> include/drm/drm_crtc.h | 20 +++++
>> include/drm/drm_writeback.h | 19 +++++
>> include/uapi/drm/drm_mode.h | 1 +
>> 11 files changed, 325 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/gpu/drm/drm_writeback.c
>> create mode 100644 include/drm/drm_writeback.h
>>
>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
>> index 53b872c..c3d0370 100644
>> --- a/Documentation/gpu/drm-kms.rst
>> +++ b/Documentation/gpu/drm-kms.rst
>> @@ -149,6 +149,15 @@ Connector Functions Reference
>> .. kernel-doc:: drivers/gpu/drm/drm_connector.c
>> :export:
>>
>> +Writeback Connectors
>> +--------------------
>> +
>> +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
>> + :doc: overview
>> +
>> +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
>> + :export:
>> +
>> Encoder Abstraction
>> ===================
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 25c7204..2dc4a48 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -15,7 +15,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
>> drm_modeset_lock.o drm_atomic.o drm_bridge.o \
>> drm_framebuffer.o drm_connector.o drm_blend.o \
>> drm_encoder.o drm_mode_object.o drm_property.o \
>> - drm_plane.o drm_color_mgmt.o
>> + drm_plane.o drm_color_mgmt.o drm_writeback.o
>>
>> drm-$(CONFIG_COMPAT) += drm_ioc32.o
>> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index fe37987..f434f34 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -612,6 +612,44 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>> }
>>
>> /**
>> + * drm_atomic_connector_check - check connector state
>> + * @connector: connector to check
>> + * @state: connector state to check
>> + *
>> + * Provides core sanity checks for connector state.
>> + *
>> + * RETURNS:
>> + * Zero on success, error code on failure
>> + */
>> +static int drm_atomic_connector_check(struct drm_connector *connector,
>> + struct drm_connector_state *state)
>> +{
>> + struct drm_crtc_state *crtc_state;
>> +
>> + if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
>> + return 0;
>> +
>> + if (!state->fb != !state->crtc) {
>> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] framebuffer/CRTC mismatch\n",
>> + connector->base.id, connector->name);
>> + return -EINVAL;
>> + }
>> +
>> + if (state->crtc)
>> + crtc_state = drm_atomic_get_existing_crtc_state(state->state,
>> + state->crtc);
>> +
>> + if (state->fb && !crtc_state->active) {
>> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] has framebuffer, but [CRTC:%d] is off\n",
>> + connector->base.id, connector->name,
>> + state->crtc->base.id);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> * drm_atomic_get_plane_state - get plane state
>> * @state: global atomic state object
>> * @plane: plane to get state object for
>> @@ -1004,12 +1042,19 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>> * now?) atomic writes to DPMS property:
>> */
>> return -EINVAL;
>> + } else if (property == config->writeback_fb_id_property) {
>> + struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
>> + drm_atomic_set_fb_for_connector(state, fb);
>> + if (fb)
>> + drm_framebuffer_unreference(fb);
>> } else if (connector->funcs->atomic_set_property) {
>> return connector->funcs->atomic_set_property(connector,
>> state, property, val);
>> } else {
>> return -EINVAL;
>> }
>> +
>> + return 0;
>> }
>> EXPORT_SYMBOL(drm_atomic_connector_set_property);
>>
>> @@ -1040,6 +1085,9 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>> *val = (state->crtc) ? state->crtc->base.id : 0;
>> } else if (property == config->dpms_property) {
>> *val = connector->dpms;
>> + } else if (property == config->writeback_fb_id_property) {
>> + /* Writeback framebuffer is one-shot, write and forget */
>> + *val = 0;
>> } else if (connector->funcs->atomic_get_property) {
>> return connector->funcs->atomic_get_property(connector,
>> state, property, val);
>> @@ -1223,6 +1271,42 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>> EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
>>
>> /**
>> + * drm_atomic_set_fb_for_connector - set framebuffer for (writeback) connector
>
>Hm, I'd call this set_writeback_fb_for_connector, for even more
>obviousness.
>
Sure.
>> + * @conn_state: atomic state object for the connector
>> + * @fb: fb to use for the connector
>> + *
>> + * This is used to set the framebuffer for a writeback connector, which outputs
>> + * to a buffer instead of an actual physical connector.
>> + * Changing the assigned framebuffer requires us to grab a reference to the new
>> + * fb and drop the reference to the old fb, if there is one. This function
>> + * takes care of all these details besides updating the pointer in the
>> + * state object itself.
>> + *
>> + * Note: The only way conn_state can already have an fb set is if the commit
>> + * sets the property more than once.
>> + *
>> + * See also DOC: overview in drm_writeback.c
>> + */
>> +void
>> +drm_atomic_set_fb_for_connector(struct drm_connector_state *conn_state,
>> + struct drm_framebuffer *fb)
>> +{
>
>Should we have a WARN_ON(conn_state->connector->type != WRITEBACK here)?
>Just for safety.
>
Yeah OK, certainly won't do any harm.
>> + if (conn_state->fb)
>> + drm_framebuffer_unreference(conn_state->fb);
>> + if (fb)
>> + drm_framebuffer_reference(fb);
>> + conn_state->fb = fb;
>> +
>> + if (fb)
>> + DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n",
>> + fb->base.id, conn_state);
>> + else
>> + DRM_DEBUG_ATOMIC("Set [NOFB] for connector state %p\n",
>> + conn_state);
>> +}
>> +EXPORT_SYMBOL(drm_atomic_set_fb_for_connector);
>> +
>> +/**
>> * drm_atomic_add_affected_connectors - add connectors for crtc
>> * @state: atomic state
>> * @crtc: DRM crtc
>> @@ -1376,6 +1460,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>> struct drm_plane_state *plane_state;
>> struct drm_crtc *crtc;
>> struct drm_crtc_state *crtc_state;
>> + struct drm_connector *conn;
>> + struct drm_connector_state *conn_state;
>> int i, ret = 0;
>>
>> DRM_DEBUG_ATOMIC("checking %p\n", state);
>> @@ -1398,6 +1484,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>> }
>> }
>>
>> + for_each_connector_in_state(state, conn, conn_state, i) {
>> + ret = drm_atomic_connector_check(conn, conn_state);
>> + if (ret) {
>> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] atomic core check failed\n",
>> + conn->base.id, conn->name);
>> + return ret;
>> + }
>> + }
>> +
>> if (config->funcs->atomic_check)
>> ret = config->funcs->atomic_check(state->dev, state);
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 2c44de3..bba8672 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3233,6 +3233,9 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>> memcpy(state, connector->state, sizeof(*state));
>> if (state->crtc)
>> drm_connector_reference(connector);
>> +
>> + /* Don't copy over framebuffers, they are used only once */
>> + state->fb = NULL;
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>>
>> @@ -3360,6 +3363,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
>> */
>> if (state->crtc)
>> drm_connector_unreference(state->connector);
>> + if (state->fb)
>> + drm_framebuffer_unreference(state->fb);
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 2db7fb5..e67084d 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -86,6 +86,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
>> { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
>> { DRM_MODE_CONNECTOR_DSI, "DSI" },
>> { DRM_MODE_CONNECTOR_DPI, "DPI" },
>> + { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
>> };
>>
>> void drm_connector_ida_init(void)
>> @@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev,
>> list_add_tail(&connector->head, &config->connector_list);
>> config->num_connector++;
>>
>> - if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
>> + if ((connector_type != DRM_MODE_CONNECTOR_VIRTUAL) &&
>> + (connector_type != DRM_MODE_CONNECTOR_WRITEBACK))
>> drm_object_attach_property(&connector->base,
>> config->edid_property,
>> 0);
>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>> new file mode 100644
>> index 0000000..5a6e0ad
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_writeback.c
>> @@ -0,0 +1,157 @@
>> +/*
>> + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
>> + * Author: Brian Starkey <brian.starkey@arm.com>
>> + *
>> + * This program is free software and is provided to you under the terms of the
>> + * GNU General Public License version 2 as published by the Free Software
>> + * Foundation, and any use by you of this program is subject to the terms
>> + * of such GNU licence.
>> + */
>> +
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_property.h>
>> +#include <drm/drmP.h>
>> +
>> +/**
>> + * DOC: overview
>> + *
>> + * Writeback connectors are used to expose hardware which can write the output
>> + * from a CRTC to a memory buffer. They are used and act similarly to other
>> + * types of connectors, with some important differences:
>> + * - Writeback connectors don't provide a way to output visually to the user.
>> + * - Writeback connectors should always report as "disconnected" (so that
>> + * clients which don't understand them will ignore them).
>> + * - Writeback connectors don't have EDID.
>> + *
>> + * Writeback connectors may only be attached to a CRTC when they have a
>> + * framebuffer attached, and may only have a framebuffer attached when they are
>> + * attached to a CRTC. The WRITEBACK_FB_ID property which sets the framebuffer
>> + * applies only to a single commit (see below), which means that each and every
>> + * commit which makes use of a writeback connector must set both its CRTC_ID and
>> + * WRITEBACK_FB_ID. It also means that the connector's CRTC_ID must be
>> + * explicitly cleared in order to make a subsequent commit which doesn't use
>> + * writeback.
>
>Hm, this is a bit an ugly fallout from our safety checks, but I guess it's
>a reasonable requirement for userspace. Writeback is kinda special. Or
>should we make the ->crtc pointer one-shot too? Of course only for
>writeback pointers. I think that would make is simpler to use in libraries
>like igt.
>
I sort-of agree on the ugly fallout sentiment, it took me a bit by
surprise when some of my commits started failing :-). On the userspace
side it isn't really any additional effort once you understand the
requirements though (which you learn quickly by way of -EINVAL).
I'm actually leaning towards leaving it as-is for two reasons:
1) On hardware that actually *does* need a full-modeset for writeback
routing changes, a one-shot CRTC would cause problems (need a
full-modeset every frame).
2) If there's no explicit commit to disable, we have no way to
communicate to the driver that it should disable - no-one will
ever add the connector_state to the next commit.
Both of the above *could* be handled with some special casing, but my
gut feeling is it would be tricky and a bit of a mess.
The current implementation is reasonably clean, and does seem to have
removed a few corner cases which would be likely sources of driver
bugs.
>> + *
>> + * Writeback connectors have several additional properties, which userspace
>> + * can use to query and control them:
>> + *
>> + * "WRITEBACK_FB_ID":
>> + * Write-only object property storing a DRM_MODE_OBJECT_FB: it stores the
>> + * framebuffer to be written by the writeback connector. This property is
>> + * similar to the FB_ID property on planes, but will always read as zero
>> + * and is not preserved across commits.
>> + * Userspace must set this property to an output buffer every time it
>> + * wishes the buffer to get filled.
>> + *
>> + * "PIXEL_FORMATS":
>> + * Immutable blob property to store the supported pixel formats table. The
>> + * data is an array of u32 DRM_FORMAT_* fourcc values.
>> + * Userspace can use this blob to find out what pixel formats are supported
>> + * by the connector's writeback engine.
>> + *
>> + * "PIXEL_FORMATS_SIZE":
>> + * Immutable unsigned range property storing the number of entries in the
>> + * PIXEL_FORMATS array.
>
>Blob properties already have a size, you don't need to specify it.
>Gamma tables have a size since they start out with a default of NULL, so
>userspace has no idea what the size should be.
>
OK, that makes sense. I'll drop it.
>Docs are missing to clarify when the writeout is complete. Should we wait
>with merging this until out fences are supported, or should we add a bit
>of placeholder text here that the writeback will be completed 1 vblank
>after this atomic commit was committed? I'm leaning towards including the
>writeout out fences from the start.
>
Docs are missing because without fences I don't think it can be
defined ;-)
You think I should squash fences into this commit then? It's getting
somewhat large, but I'm not spotting any obvious places to split it
up anymore.
>> + */
>> +
>> +/**
>> + * create_writeback_properties - Create writeback connector-specific properties
>> + * @dev: DRM device
>> + *
>> + * Create the properties specific to writeback connectors. These will be
>> + * attached to the connector and initialised by drm_writeback_connector_init.
>> + *
>> + * Returns: true on success, or false if any property creation fails.
>> + */
>
>We don't do kernel-doc for non-EXPORT_SYMBOL functions, since the intended
>audience is driver authors. And I think this function here is clear enough
>that we don't need even a normal comment for it.
>
Right, I must have copied and blindly modified without thinking.
>> +static bool create_writeback_properties(struct drm_device *dev)
>> +{
>> + struct drm_property *prop;
>> +
>> + if (!dev->mode_config.writeback_fb_id_property) {
>> + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>> + "WRITEBACK_FB_ID",
>> + DRM_MODE_OBJECT_FB);
>> + if (!prop)
>> + return false;
>> + dev->mode_config.writeback_fb_id_property = prop;
>> + }
>> +
>> + if (!dev->mode_config.pixel_formats_property) {
>> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,
>> + "PIXEL_FORMATS", 0);
>> + if (!prop)
>> + return false;
>> + dev->mode_config.pixel_formats_property = prop;
>> + }
>> +
>> + if (!dev->mode_config.pixel_formats_size_property) {
>> + prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
>> + "PIXEL_FORMATS_SIZE", 0,
>> + UINT_MAX);
>> + if (!prop)
>> + return false;
>> + dev->mode_config.pixel_formats_size_property = prop;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +/**
>> + * drm_writeback_connector_init - Initialize a writeback connector and its properties
>> + * @dev: DRM device
>> + * @connector: Connector to initialize
>> + * @funcs: Connector funcs vtable
>> + * @formats: Array of supported pixel formats for the writeback engine
>> + * @n_formats: Length of the formats array
>> + *
>> + * This function creates the writeback-connector-specific properties if they
>> + * have not been already created, initializes the connector as
>> + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
>> + * values.
>> + *
>> + * Drivers should always use this function instead of drm_connector_init() to
>> + * set up writeback connectors.
>> + *
>> + * Returns: 0 on success, or a negative error code
>> + */
>> +int drm_writeback_connector_init(struct drm_device *dev,
>> + struct drm_connector *connector,
>> + const struct drm_connector_funcs *funcs,
>> + u32 *formats, int n_formats)
>> +{
>> + int ret;
>> + struct drm_property_blob *blob;
>> + struct drm_mode_config *config = &dev->mode_config;
>> +
>> + if (!create_writeback_properties(dev))
>> + return -EINVAL;
>> +
>> + blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
>> + formats);
>> + if (IS_ERR(blob))
>> + return PTR_ERR(blob);
>> +
>> + ret = drm_connector_init(dev, connector, funcs,
>> + DRM_MODE_CONNECTOR_WRITEBACK);
>> + if (ret)
>> + goto fail;
>> +
>> + drm_object_attach_property(&connector->base,
>> + config->writeback_fb_id_property, 0);
>> +
>> + drm_object_attach_property(&connector->base,
>> + config->pixel_formats_property,
>> + blob->base.id);
>> + connector->pixel_formats_blob_ptr = blob;
>> +
>> + drm_object_attach_property(&connector->base,
>> + config->pixel_formats_size_property,
>> + n_formats);
>> +
>> + return 0;
>> +
>> +fail:
>> + drm_property_unreference_blob(blob);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(drm_writeback_connector_init);
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 9701f2d..d9aff06 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -319,6 +319,9 @@ void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>> int __must_check
>> drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>> struct drm_crtc *crtc);
>> +void
>> +drm_atomic_set_fb_for_connector(struct drm_connector_state *conn_state,
>> + struct drm_framebuffer *fb);
>> int __must_check
>> drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
>> struct drm_crtc *crtc);
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index ac9d7d8..a5e3778 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -198,6 +198,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
>> * @connector: backpointer to the connector
>> * @best_encoder: can be used by helpers and drivers to select the encoder
>> * @state: backpointer to global drm_atomic_state
>> + * @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK
>> */
>> struct drm_connector_state {
>> struct drm_connector *connector;
>> @@ -213,6 +214,8 @@ struct drm_connector_state {
>> struct drm_encoder *best_encoder;
>>
>> struct drm_atomic_state *state;
>> +
>> + struct drm_framebuffer *fb; /* do not write directly, use drm_atomic_set_fb_for_connector() */
>> };
>>
>> /**
>> @@ -612,6 +615,15 @@ struct drm_connector {
>> */
>> struct drm_property_blob *tile_blob_ptr;
>>
>> + /**
>> + * @pixel_formats_blob_ptr:
>> + *
>> + * DRM blob property data for the pixel formats list on writeback
>> + * connectors
>> + * See also DOC: overview in drm_writeback.c
>
>Ah, here's the link you wanted. Imo just use
>drm_writeback_connector_init() instead, that's a link close enough to your
>target.
>
OK.
>> + */
>> + struct drm_property_blob *pixel_formats_blob_ptr;
>
>I'g go with a writeout prefix.
OK.
>> +
>> /* should we poll this connector for connects and disconnects */
>> /* hot plug detectable */
>> #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index fe20d8f..378baee2 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1345,6 +1345,26 @@ struct drm_mode_config {
>> */
>> struct drm_property *suggested_y_property;
>>
>> + /**
>> + * @writeback_fb_id_property: Property for writeback connectors, storing
>> + * the ID of the output framebuffer.
>> + * See also DOC: overview in drm_writeback.c
>
>Same here and all of the below.
>
>> + */
>> + struct drm_property *writeback_fb_id_property;
>> + /**
>> + * @pixel_formats_property: Property for writeback connectors, storing
>> + * an array of the supported pixel formats for the writeback engine
>> + * (read-only).
>> + * See also DOC: overview in drm_writeback.c
>> + */
>> + struct drm_property *pixel_formats_property;
>> + /**
>> + * @pixel_formats_size_property: Property for writeback connectors,
>> + * stating the size of the pixel formats array (read-only).
>> + * See also DOC: overview in drm_writeback.c
>> + */
>> + struct drm_property *pixel_formats_size_property;
>> +
>> /* dumb ioctl parameters */
>> uint32_t preferred_depth, prefer_shadow;
>>
>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
>> new file mode 100644
>> index 0000000..afdc2742
>> --- /dev/null
>> +++ b/include/drm/drm_writeback.h
>> @@ -0,0 +1,19 @@
>> +/*
>> + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
>> + * Author: Brian Starkey <brian.starkey@arm.com>
>> + *
>> + * This program is free software and is provided to you under the terms of the
>> + * GNU General Public License version 2 as published by the Free Software
>> + * Foundation, and any use by you of this program is subject to the terms
>> + * of such GNU licence.
>> + */
>> +
>> +#ifndef __DRM_WRITEBACK_H__
>> +#define __DRM_WRITEBACK_H__
>> +
>> +int drm_writeback_connector_init(struct drm_device *dev,
>> + struct drm_connector *connector,
>> + const struct drm_connector_funcs *funcs,
>> + u32 *formats, int n_formats);
>> +
>> +#endif
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index df0e350..e9cb4fe 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -247,6 +247,7 @@ struct drm_mode_get_encoder {
>> #define DRM_MODE_CONNECTOR_VIRTUAL 15
>> #define DRM_MODE_CONNECTOR_DSI 16
>> #define DRM_MODE_CONNECTOR_DPI 17
>> +#define DRM_MODE_CONNECTOR_WRITEBACK 18
>>
>> struct drm_mode_get_connector {
>>
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 1/9] drm: Add writeback connector type
2016-10-26 12:42 ` Brian Starkey
@ 2016-10-26 13:05 ` Daniel Vetter
0 siblings, 0 replies; 25+ messages in thread
From: Daniel Vetter @ 2016-10-26 13:05 UTC (permalink / raw)
To: Brian Starkey; +Cc: Daniel Vetter, dri-devel, linux-kernel, linux-media
On Wed, Oct 26, 2016 at 01:42:42PM +0100, Brian Starkey wrote:
> On Wed, Oct 26, 2016 at 01:00:21PM +0200, Daniel Vetter wrote:
> > On Wed, Oct 26, 2016 at 09:55:00AM +0100, Brian Starkey wrote:
> > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > > new file mode 100644
> > > index 0000000..5a6e0ad
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -0,0 +1,157 @@
> > > +/*
> > > + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
> > > + * Author: Brian Starkey <brian.starkey@arm.com>
> > > + *
> > > + * This program is free software and is provided to you under the terms of the
> > > + * GNU General Public License version 2 as published by the Free Software
> > > + * Foundation, and any use by you of this program is subject to the terms
> > > + * of such GNU licence.
> > > + */
> > > +
> > > +#include <drm/drm_crtc.h>
> > > +#include <drm/drm_property.h>
> > > +#include <drm/drmP.h>
> > > +
> > > +/**
> > > + * DOC: overview
> > > + *
> > > + * Writeback connectors are used to expose hardware which can write the output
> > > + * from a CRTC to a memory buffer. They are used and act similarly to other
> > > + * types of connectors, with some important differences:
> > > + * - Writeback connectors don't provide a way to output visually to the user.
> > > + * - Writeback connectors should always report as "disconnected" (so that
> > > + * clients which don't understand them will ignore them).
> > > + * - Writeback connectors don't have EDID.
> > > + *
> > > + * Writeback connectors may only be attached to a CRTC when they have a
> > > + * framebuffer attached, and may only have a framebuffer attached when they are
> > > + * attached to a CRTC. The WRITEBACK_FB_ID property which sets the framebuffer
> > > + * applies only to a single commit (see below), which means that each and every
> > > + * commit which makes use of a writeback connector must set both its CRTC_ID and
> > > + * WRITEBACK_FB_ID. It also means that the connector's CRTC_ID must be
> > > + * explicitly cleared in order to make a subsequent commit which doesn't use
> > > + * writeback.
> >
> > Hm, this is a bit an ugly fallout from our safety checks, but I guess it's
> > a reasonable requirement for userspace. Writeback is kinda special. Or
> > should we make the ->crtc pointer one-shot too? Of course only for
> > writeback pointers. I think that would make is simpler to use in libraries
> > like igt.
> >
>
> I sort-of agree on the ugly fallout sentiment, it took me a bit by
> surprise when some of my commits started failing :-). On the userspace
> side it isn't really any additional effort once you understand the
> requirements though (which you learn quickly by way of -EINVAL).
>
> I'm actually leaning towards leaving it as-is for two reasons:
> 1) On hardware that actually *does* need a full-modeset for writeback
> routing changes, a one-shot CRTC would cause problems (need a
> full-modeset every frame).
> 2) If there's no explicit commit to disable, we have no way to
> communicate to the driver that it should disable - no-one will
> ever add the connector_state to the next commit.
>
> Both of the above *could* be handled with some special casing, but my
> gut feeling is it would be tricky and a bit of a mess.
>
> The current implementation is reasonably clean, and does seem to have
> removed a few corner cases which would be likely sources of driver
> bugs.
One issue with not auto-disconnecting the writeback connector is that if
the next atomic ioctl does not touch it, nothing bad will happen. But if
it does touch it, then you get an -EINVAL. It's actually worse, since the
driver can choose to add any object to the update for whatever reason it
sees fit, userspace might get badly surprised with this.
Given all that I think my original suggestion to enforce
!state->writeback_fb == !state->crtc was a bit a mistake, and we should
only enforce that !writeback_fb || state->crtc is true, i.e. a set
writeback_fb implies that it must be connected to something. This means
drivers must always check for the existing of the writeback_fb, but I
think they need to do that anyway already.
> > > + * Writeback connectors have several additional properties, which userspace
> > > + * can use to query and control them:
> > > + *
> > > + * "WRITEBACK_FB_ID":
> > > + * Write-only object property storing a DRM_MODE_OBJECT_FB: it stores the
> > > + * framebuffer to be written by the writeback connector. This property is
> > > + * similar to the FB_ID property on planes, but will always read as zero
> > > + * and is not preserved across commits.
> > > + * Userspace must set this property to an output buffer every time it
> > > + * wishes the buffer to get filled.
> > > + *
> > > + * "PIXEL_FORMATS":
> > > + * Immutable blob property to store the supported pixel formats table. The
> > > + * data is an array of u32 DRM_FORMAT_* fourcc values.
> > > + * Userspace can use this blob to find out what pixel formats are supported
> > > + * by the connector's writeback engine.
> > > + *
> > > + * "PIXEL_FORMATS_SIZE":
> > > + * Immutable unsigned range property storing the number of entries in the
> > > + * PIXEL_FORMATS array.
> >
> > Blob properties already have a size, you don't need to specify it.
> > Gamma tables have a size since they start out with a default of NULL, so
> > userspace has no idea what the size should be.
> >
>
> OK, that makes sense. I'll drop it.
>
> > Docs are missing to clarify when the writeout is complete. Should we wait
> > with merging this until out fences are supported, or should we add a bit
> > of placeholder text here that the writeback will be completed 1 vblank
> > after this atomic commit was committed? I'm leaning towards including the
> > writeout out fences from the start.
> >
>
> Docs are missing because without fences I don't think it can be
> defined ;-)
>
> You think I should squash fences into this commit then? It's getting
> somewhat large, but I'm not spotting any obvious places to split it
> up anymore.
Not necessarily merge them, but definitely have them all before the first
driver patch to use this stuff. So just reordering the series a bit should
be all that's needed.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH v2 2/9] drm: mali-dp: Clear CVAL when leaving config mode
2016-10-26 8:54 [RFC PATCH v2 0/9] Introduce writeback connectors Brian Starkey
2016-10-26 8:55 ` [RFC PATCH v2 1/9] drm: Add writeback connector type Brian Starkey
@ 2016-10-26 8:55 ` Brian Starkey
2016-10-26 8:55 ` [RFC PATCH v2 3/9] drm: mali-dp: Rename malidp_input_format Brian Starkey
` (6 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 8:55 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, linux-media
It's possible for CVAL to get set whilst we are in config mode. If this
happens, afer we leave config mode the HW will latch whatever
configuration is in the registers at the next vsync. Most likely this
will be a partial configuration, as we'll be racing against the ongoing
atomic_commit.
To avoid this, clear CVAL before leaving config mode.
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
drivers/gpu/drm/arm/malidp_hw.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 7f4a0bd..65e667b 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -125,6 +125,7 @@ static void malidp500_leave_config_mode(struct malidp_hw_device *hwdev)
{
u32 status, count = 100;
+ malidp_hw_clearbits(hwdev, MALIDP_CFG_VALID, MALIDP500_CONFIG_VALID);
malidp_hw_clearbits(hwdev, MALIDP500_DC_CONFIG_REQ, MALIDP500_DC_CONTROL);
while (count) {
status = malidp_hw_read(hwdev, hwdev->map.dc_base + MALIDP_REG_STATUS);
@@ -271,6 +272,7 @@ static void malidp550_leave_config_mode(struct malidp_hw_device *hwdev)
{
u32 status, count = 100;
+ malidp_hw_clearbits(hwdev, MALIDP_CFG_VALID, MALIDP550_CONFIG_VALID);
malidp_hw_clearbits(hwdev, MALIDP550_DC_CONFIG_REQ, MALIDP550_DC_CONTROL);
while (count) {
status = malidp_hw_read(hwdev, hwdev->map.dc_base + MALIDP_REG_STATUS);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [RFC PATCH v2 3/9] drm: mali-dp: Rename malidp_input_format
2016-10-26 8:54 [RFC PATCH v2 0/9] Introduce writeback connectors Brian Starkey
2016-10-26 8:55 ` [RFC PATCH v2 1/9] drm: Add writeback connector type Brian Starkey
2016-10-26 8:55 ` [RFC PATCH v2 2/9] drm: mali-dp: Clear CVAL when leaving config mode Brian Starkey
@ 2016-10-26 8:55 ` Brian Starkey
2016-10-26 8:55 ` [RFC PATCH v2 4/9] drm: mali-dp: Add RGB writeback formats for DP550/DP650 Brian Starkey
` (5 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 8:55 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, linux-media
We're going to use the same format list for output formats, so rename
everything related to input formats to avoid confusion.
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
drivers/gpu/drm/arm/malidp_hw.c | 24 ++++++++++++------------
drivers/gpu/drm/arm/malidp_hw.h | 8 ++++----
drivers/gpu/drm/arm/malidp_planes.c | 8 ++++----
3 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 65e667b..eb8a4bf 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -21,7 +21,7 @@
#include "malidp_drv.h"
#include "malidp_hw.h"
-static const struct malidp_input_format malidp500_de_formats[] = {
+static const struct malidp_format_id malidp500_de_formats[] = {
/* fourcc, layers supporting the format, internal id */
{ DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_GRAPHICS2, 0 },
{ DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_GRAPHICS2, 1 },
@@ -69,7 +69,7 @@ static const struct malidp_input_format malidp500_de_formats[] = {
{ DRM_FORMAT_NV12, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 6) }, \
{ DRM_FORMAT_YUV420, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 7) }
-static const struct malidp_input_format malidp550_de_formats[] = {
+static const struct malidp_format_id malidp550_de_formats[] = {
MALIDP_COMMON_FORMATS,
};
@@ -441,8 +441,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
.irq_mask = MALIDP500_DE_IRQ_CONF_VALID,
.vsync_irq = MALIDP500_DE_IRQ_CONF_VALID,
},
- .input_formats = malidp500_de_formats,
- .n_input_formats = ARRAY_SIZE(malidp500_de_formats),
+ .pixel_formats = malidp500_de_formats,
+ .n_pixel_formats = ARRAY_SIZE(malidp500_de_formats),
.bus_align_bytes = 8,
},
.query_hw = malidp500_query_hw,
@@ -474,8 +474,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
},
- .input_formats = malidp550_de_formats,
- .n_input_formats = ARRAY_SIZE(malidp550_de_formats),
+ .pixel_formats = malidp550_de_formats,
+ .n_pixel_formats = ARRAY_SIZE(malidp550_de_formats),
.bus_align_bytes = 8,
},
.query_hw = malidp550_query_hw,
@@ -508,8 +508,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
},
- .input_formats = malidp550_de_formats,
- .n_input_formats = ARRAY_SIZE(malidp550_de_formats),
+ .pixel_formats = malidp550_de_formats,
+ .n_pixel_formats = ARRAY_SIZE(malidp550_de_formats),
.bus_align_bytes = 16,
},
.query_hw = malidp650_query_hw,
@@ -527,10 +527,10 @@ u8 malidp_hw_get_format_id(const struct malidp_hw_regmap *map,
{
unsigned int i;
- for (i = 0; i < map->n_input_formats; i++) {
- if (((map->input_formats[i].layer & layer_id) == layer_id) &&
- (map->input_formats[i].format == format))
- return map->input_formats[i].id;
+ for (i = 0; i < map->n_pixel_formats; i++) {
+ if (((map->pixel_formats[i].layer & layer_id) == layer_id) &&
+ (map->pixel_formats[i].format == format))
+ return map->pixel_formats[i].id;
}
return MALIDP_INVALID_FORMAT_ID;
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 087e1202..4f8c884 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -35,7 +35,7 @@ enum {
DE_SMART = BIT(4),
};
-struct malidp_input_format {
+struct malidp_format_id {
u32 format; /* DRM fourcc */
u8 layer; /* bitmask of layers supporting it */
u8 id; /* used internally */
@@ -85,9 +85,9 @@ struct malidp_hw_regmap {
const struct malidp_irq_map se_irq_map;
const struct malidp_irq_map dc_irq_map;
- /* list of supported input formats for each layer */
- const struct malidp_input_format *input_formats;
- const u8 n_input_formats;
+ /* list of supported pixel formats for each layer */
+ const struct malidp_format_id *pixel_formats;
+ const u8 n_pixel_formats;
/* pitch alignment requirement in bytes */
const u8 bus_align_bytes;
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index 9020c0d..f44a1cb 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -255,7 +255,7 @@ int malidp_de_planes_init(struct drm_device *drm)
u32 *formats;
int ret, i, j, n;
- formats = kcalloc(map->n_input_formats, sizeof(*formats), GFP_KERNEL);
+ formats = kcalloc(map->n_pixel_formats, sizeof(*formats), GFP_KERNEL);
if (!formats) {
ret = -ENOMEM;
goto cleanup;
@@ -271,9 +271,9 @@ int malidp_de_planes_init(struct drm_device *drm)
}
/* build the list of DRM supported formats based on the map */
- for (n = 0, j = 0; j < map->n_input_formats; j++) {
- if ((map->input_formats[j].layer & id) == id)
- formats[n++] = map->input_formats[j].format;
+ for (n = 0, j = 0; j < map->n_pixel_formats; j++) {
+ if ((map->pixel_formats[j].layer & id) == id)
+ formats[n++] = map->pixel_formats[j].format;
}
plane_type = (i == 0) ? DRM_PLANE_TYPE_PRIMARY :
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [RFC PATCH v2 4/9] drm: mali-dp: Add RGB writeback formats for DP550/DP650
2016-10-26 8:54 [RFC PATCH v2 0/9] Introduce writeback connectors Brian Starkey
` (2 preceding siblings ...)
2016-10-26 8:55 ` [RFC PATCH v2 3/9] drm: mali-dp: Rename malidp_input_format Brian Starkey
@ 2016-10-26 8:55 ` Brian Starkey
2016-10-26 8:55 ` [RFC PATCH v2 5/9] drm: mali-dp: Add support for writeback on DP550/DP650 Brian Starkey
` (4 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 8:55 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, linux-media
Add a layer bit for the SE memory-write, and add it to the pixel format
matrix for DP550/DP650.
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
drivers/gpu/drm/arm/malidp_hw.c | 28 ++++++++++++++--------------
drivers/gpu/drm/arm/malidp_hw.h | 1 +
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index eb8a4bf..df1fce0 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -46,20 +46,20 @@ static const struct malidp_format_id malidp500_de_formats[] = {
#define MALIDP_COMMON_FORMATS \
/* fourcc, layers supporting the format, internal id */ \
- { DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 0) }, \
- { DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 1) }, \
- { DRM_FORMAT_RGBA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 2) }, \
- { DRM_FORMAT_BGRA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 3) }, \
- { DRM_FORMAT_ARGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 0) }, \
- { DRM_FORMAT_ABGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 1) }, \
- { DRM_FORMAT_RGBA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 2) }, \
- { DRM_FORMAT_BGRA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 3) }, \
- { DRM_FORMAT_XRGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 0) }, \
- { DRM_FORMAT_XBGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 1) }, \
- { DRM_FORMAT_RGBX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 2) }, \
- { DRM_FORMAT_BGRX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 3) }, \
- { DRM_FORMAT_RGB888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 0) }, \
- { DRM_FORMAT_BGR888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 1) }, \
+ { DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 0) }, \
+ { DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 1) }, \
+ { DRM_FORMAT_RGBA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 2) }, \
+ { DRM_FORMAT_BGRA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 3) }, \
+ { DRM_FORMAT_ARGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 0) }, \
+ { DRM_FORMAT_ABGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 1) }, \
+ { DRM_FORMAT_RGBA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 2) }, \
+ { DRM_FORMAT_BGRA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 3) }, \
+ { DRM_FORMAT_XRGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 0) }, \
+ { DRM_FORMAT_XBGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 1) }, \
+ { DRM_FORMAT_RGBX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 2) }, \
+ { DRM_FORMAT_BGRX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 3) }, \
+ { DRM_FORMAT_RGB888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(3, 0) }, \
+ { DRM_FORMAT_BGR888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(3, 1) }, \
{ DRM_FORMAT_RGBA5551, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 0) }, \
{ DRM_FORMAT_ABGR1555, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 1) }, \
{ DRM_FORMAT_RGB565, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 2) }, \
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 4f8c884..ce4ea55 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -33,6 +33,7 @@ enum {
DE_GRAPHICS2 = BIT(2), /* used only in DP500 */
DE_VIDEO2 = BIT(3),
DE_SMART = BIT(4),
+ SE_MEMWRITE = BIT(5),
};
struct malidp_format_id {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [RFC PATCH v2 5/9] drm: mali-dp: Add support for writeback on DP550/DP650
2016-10-26 8:54 [RFC PATCH v2 0/9] Introduce writeback connectors Brian Starkey
` (3 preceding siblings ...)
2016-10-26 8:55 ` [RFC PATCH v2 4/9] drm: mali-dp: Add RGB writeback formats for DP550/DP650 Brian Starkey
@ 2016-10-26 8:55 ` Brian Starkey
2016-10-26 8:55 ` [RFC PATCH v2 6/9] drm: mali-dp: Add writeback connector Brian Starkey
` (3 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 8:55 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, linux-media, Liviu Dudau
From: Liviu Dudau <Liviu.Dudau@arm.com>
Mali-DP display processors are able to write the composition result to a
memory buffer via the SE.
Add entry points in the HAL for enabling/disabling this feature, and
implement support for it on DP650 and DP550. DP500 acts differently and
so is omitted from this change.
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
drivers/gpu/drm/arm/malidp_hw.c | 52 +++++++++++++++++++++++++++++++++++--
drivers/gpu/drm/arm/malidp_hw.h | 18 +++++++++++++
drivers/gpu/drm/arm/malidp_regs.h | 15 +++++++++++
3 files changed, 83 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index df1fce0..5004988 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -389,6 +389,48 @@ static int malidp550_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16
return w * bytes_per_col;
}
+static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev,
+ dma_addr_t *addrs, s32 *pitches,
+ int num_planes, u16 w, u16 h, u32 fmt_id)
+{
+ u32 base = MALIDP550_SE_MEMWRITE_BASE;
+ u32 de_base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
+
+ /* enable the scaling engine block */
+ malidp_hw_setbits(hwdev, MALIDP_SCALE_ENGINE_EN, de_base + MALIDP_DE_DISPLAY_FUNC);
+
+ malidp_hw_write(hwdev, fmt_id, base + MALIDP_MW_FORMAT);
+ switch (num_planes) {
+ case 2:
+ malidp_hw_write(hwdev, lower_32_bits(addrs[1]), base + MALIDP_MW_P2_PTR_LOW);
+ malidp_hw_write(hwdev, upper_32_bits(addrs[1]), base + MALIDP_MW_P2_PTR_HIGH);
+ malidp_hw_write(hwdev, pitches[1], base + MALIDP_MW_P2_STRIDE);
+ /* fall through */
+ case 1:
+ malidp_hw_write(hwdev, lower_32_bits(addrs[0]), base + MALIDP_MW_P1_PTR_LOW);
+ malidp_hw_write(hwdev, upper_32_bits(addrs[0]), base + MALIDP_MW_P1_PTR_HIGH);
+ malidp_hw_write(hwdev, pitches[0], base + MALIDP_MW_P1_STRIDE);
+ break;
+ default:
+ WARN(1, "Invalid number of planes");
+ }
+
+ malidp_hw_write(hwdev, MALIDP_DE_H_ACTIVE(w) | MALIDP_DE_V_ACTIVE(h),
+ MALIDP550_SE_MEMWRITE_OUT_SIZE);
+ malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
+ MALIDP550_SE_CONTROL);
+
+ return 0;
+}
+
+static void malidp550_disable_memwrite(struct malidp_hw_device *hwdev)
+{
+ u32 base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
+ malidp_hw_clearbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
+ MALIDP550_SE_CONTROL);
+ malidp_hw_clearbits(hwdev, MALIDP_SCALE_ENGINE_EN, base + MALIDP_DE_DISPLAY_FUNC);
+}
+
static int malidp650_query_hw(struct malidp_hw_device *hwdev)
{
u32 conf = malidp_hw_read(hwdev, MALIDP550_CONFIG_ID);
@@ -471,7 +513,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
MALIDP550_SE_IRQ_AXI_ERR,
},
.dc_irq_map = {
- .irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
+ .irq_mask = MALIDP550_DC_IRQ_CONF_VALID |
+ MALIDP550_DC_IRQ_SE,
.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
},
.pixel_formats = malidp550_de_formats,
@@ -485,6 +528,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
.set_config_valid = malidp550_set_config_valid,
.modeset = malidp550_modeset,
.rotmem_required = malidp550_rotmem_required,
+ .enable_memwrite = malidp550_enable_memwrite,
+ .disable_memwrite = malidp550_disable_memwrite,
},
[MALIDP_650] = {
.map = {
@@ -505,7 +550,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
MALIDP550_SE_IRQ_AXI_ERR,
},
.dc_irq_map = {
- .irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
+ .irq_mask = MALIDP550_DC_IRQ_CONF_VALID |
+ MALIDP550_DC_IRQ_SE,
.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
},
.pixel_formats = malidp550_de_formats,
@@ -519,6 +565,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
.set_config_valid = malidp550_set_config_valid,
.modeset = malidp550_modeset,
.rotmem_required = malidp550_rotmem_required,
+ .enable_memwrite = malidp550_enable_memwrite,
+ .disable_memwrite = malidp550_disable_memwrite,
},
};
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index ce4ea55..8056efa 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -147,6 +147,24 @@ struct malidp_hw_device {
*/
int (*rotmem_required)(struct malidp_hw_device *hwdev, u16 w, u16 h, u32 fmt);
+ /**
+ * Enable writing to memory the content of the next frame
+ * @param hwdev - malidp_hw_device structure containing the HW description
+ * @param addrs - array of addresses for each plane
+ * @param pitches - array of pitches for each plane
+ * @param num_planes - number of planes to be written
+ * @param w - width of the output frame
+ * @param h - height of the output frame
+ * @param fmt_id - internal format ID of output buffer
+ */
+ int (*enable_memwrite)(struct malidp_hw_device *hwdev, dma_addr_t *addrs,
+ s32 *pitches, int num_planes, u16 w, u16 h, u32 fmt_id);
+
+ /*
+ * Disable the writing to memory of the next frame's content.
+ */
+ void (*disable_memwrite)(struct malidp_hw_device *hwdev);
+
u8 features;
u8 min_line_size;
diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
index 73fecb3..cab086c 100644
--- a/drivers/gpu/drm/arm/malidp_regs.h
+++ b/drivers/gpu/drm/arm/malidp_regs.h
@@ -64,6 +64,8 @@
/* bit masks that are common between products */
#define MALIDP_CFG_VALID (1 << 0)
#define MALIDP_DISP_FUNC_ILACED (1 << 8)
+#define MALIDP_SCALE_ENGINE_EN (1 << 16)
+#define MALIDP_SE_MEMWRITE_EN (2 << 5)
/* register offsets for IRQ management */
#define MALIDP_REG_STATUS 0x00000
@@ -92,6 +94,15 @@
#define MALIDP_DE_H_ACTIVE(x) (((x) & 0x1fff) << 0)
#define MALIDP_DE_V_ACTIVE(x) (((x) & 0x1fff) << 16)
+/* register offsets relative to MALIDP5x0_SE_MEMWRITE_BASE */
+#define MALIDP_MW_FORMAT 0x00000
+#define MALIDP_MW_P1_STRIDE 0x00004
+#define MALIDP_MW_P2_STRIDE 0x00008
+#define MALIDP_MW_P1_PTR_LOW 0x0000c
+#define MALIDP_MW_P1_PTR_HIGH 0x00010
+#define MALIDP_MW_P2_PTR_LOW 0x0002c
+#define MALIDP_MW_P2_PTR_HIGH 0x00030
+
/* register offsets and bits specific to DP500 */
#define MALIDP500_DC_BASE 0x00000
#define MALIDP500_DC_CONTROL 0x0000c
@@ -149,6 +160,10 @@
#define MALIDP550_DE_LS_PTR_BASE 0x0042c
#define MALIDP550_DE_PERF_BASE 0x00500
#define MALIDP550_SE_BASE 0x08000
+#define MALIDP550_SE_CONTROL 0x08010
+#define MALIDP550_SE_MEMWRITE_ONESHOT (1 << 7)
+#define MALIDP550_SE_MEMWRITE_OUT_SIZE 0x08030
+#define MALIDP550_SE_MEMWRITE_BASE 0x08100
#define MALIDP550_DC_BASE 0x0c000
#define MALIDP550_DC_CONTROL 0x0c010
#define MALIDP550_DC_CONFIG_REQ (1 << 16)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [RFC PATCH v2 6/9] drm: mali-dp: Add writeback connector
2016-10-26 8:54 [RFC PATCH v2 0/9] Introduce writeback connectors Brian Starkey
` (4 preceding siblings ...)
2016-10-26 8:55 ` [RFC PATCH v2 5/9] drm: mali-dp: Add support for writeback on DP550/DP650 Brian Starkey
@ 2016-10-26 8:55 ` Brian Starkey
2016-10-26 8:55 ` [RFC PATCH v2 7/9] drm: atomic: factor out common out-fence operations Brian Starkey
` (2 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 8:55 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, linux-media
Mali-DP has a memory writeback engine which can be used to write the
composition result to a memory buffer.
Expose this functionality as a DRM writeback connector on supported
hardware.
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
drivers/gpu/drm/arm/Makefile | 1 +
drivers/gpu/drm/arm/malidp_crtc.c | 21 +++
drivers/gpu/drm/arm/malidp_drv.c | 28 +++-
drivers/gpu/drm/arm/malidp_drv.h | 7 +
drivers/gpu/drm/arm/malidp_hw.c | 40 ++++-
drivers/gpu/drm/arm/malidp_mw.c | 297 +++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/arm/malidp_mw.h | 25 ++++
7 files changed, 413 insertions(+), 6 deletions(-)
create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
create mode 100644 drivers/gpu/drm/arm/malidp_mw.h
diff --git a/drivers/gpu/drm/arm/Makefile b/drivers/gpu/drm/arm/Makefile
index bb8b158..3bf31d1 100644
--- a/drivers/gpu/drm/arm/Makefile
+++ b/drivers/gpu/drm/arm/Makefile
@@ -1,4 +1,5 @@
hdlcd-y := hdlcd_drv.o hdlcd_crtc.o
obj-$(CONFIG_DRM_HDLCD) += hdlcd.o
mali-dp-y := malidp_drv.o malidp_hw.o malidp_planes.o malidp_crtc.o
+mali-dp-y += malidp_mw.o
obj-$(CONFIG_DRM_MALI_DISPLAY) += mali-dp.o
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index 08e6a71..aadc223 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -68,6 +68,18 @@ static void malidp_crtc_enable(struct drm_crtc *crtc)
clk_set_rate(hwdev->pxlclk, crtc->state->adjusted_mode.crtc_clock * 1000);
hwdev->modeset(hwdev, &vm);
+ /*
+ * We should always disable the memory write when leaving config mode,
+ * otherwise the hardware will start writing right away - possibly with
+ * a stale config, and definitely before we've had a chance to configure
+ * the planes.
+ * If the memory write needs to be enabled, that will get taken care
+ * of later during the atomic commit
+ */
+ if (hwdev->disable_memwrite) {
+ DRM_DEV_DEBUG_DRIVER(crtc->dev->dev, "Disable memwrite\n");
+ hwdev->disable_memwrite(hwdev);
+ }
hwdev->leave_config_mode(hwdev);
drm_crtc_vblank_on(crtc);
}
@@ -157,6 +169,15 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
}
}
+ /* If only the writeback routing has changed, we don't need a modeset */
+ if (state->connectors_changed) {
+ u32 old_mask = crtc->state->connector_mask;
+ u32 new_mask = state->connector_mask;
+ if ((old_mask ^ new_mask) ==
+ (1 << drm_connector_index(&malidp->mw_connector)))
+ state->connectors_changed = false;
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index f63a4df..cdefbab 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -28,6 +28,7 @@
#include <drm/drm_of.h>
#include "malidp_drv.h"
+#include "malidp_mw.h"
#include "malidp_regs.h"
#include "malidp_hw.h"
@@ -92,6 +93,14 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
drm_atomic_helper_commit_modeset_disables(drm, state);
drm_atomic_helper_commit_modeset_enables(drm, state);
+
+ /*
+ * The order here is important. We must configure memory-write after
+ * the CRTC is already enabled, so that its configuration update is
+ * gated on the next CVAL.
+ */
+ malidp_mw_atomic_commit(drm, state);
+
drm_atomic_helper_commit_planes(drm, state, 0);
malidp_atomic_commit_hw_done(state);
@@ -147,12 +156,20 @@ static int malidp_init(struct drm_device *drm)
drm->mode_config.helper_private = &malidp_mode_config_helpers;
ret = malidp_crtc_init(drm);
- if (ret) {
- drm_mode_config_cleanup(drm);
- return ret;
- }
+ if (ret)
+ goto crtc_fail;
+
+ ret = malidp_mw_connector_init(drm);
+ if (ret)
+ goto mw_fail;
return 0;
+
+mw_fail:
+ malidp_de_planes_destroy(drm);
+crtc_fail:
+ drm_mode_config_cleanup(drm);
+ return ret;
}
static void malidp_fini(struct drm_device *drm)
@@ -357,6 +374,9 @@ static int malidp_bind(struct device *dev)
atomic_set(&malidp->config_valid, 0);
init_waitqueue_head(&malidp->wq);
+ INIT_LIST_HEAD(&malidp->finished_mw_jobs);
+ spin_lock_init(&malidp->mw_lock);
+
ret = malidp_init(drm);
if (ret < 0)
goto init_fail;
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index 9fc8a2e..0dce0cf 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -22,8 +22,15 @@ struct malidp_drm {
struct drm_fbdev_cma *fbdev;
struct list_head event_list;
struct drm_crtc crtc;
+ struct drm_encoder mw_encoder;
+ struct drm_connector mw_connector;
wait_queue_head_t wq;
atomic_t config_valid;
+
+ struct malidp_mw_job *current_mw;
+ /* lock for protecting the finished_mw_jobs list */
+ spinlock_t mw_lock;
+ struct list_head finished_mw_jobs;
};
#define crtc_to_malidp_device(x) container_of(x, struct malidp_drm, crtc)
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 5004988..1689547 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -20,6 +20,7 @@
#include "malidp_drv.h"
#include "malidp_hw.h"
+#include "malidp_mw.h"
static const struct malidp_format_id malidp500_de_formats[] = {
/* fourcc, layers supporting the format, internal id */
@@ -548,6 +549,7 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
.se_irq_map = {
.irq_mask = MALIDP550_SE_IRQ_EOW |
MALIDP550_SE_IRQ_AXI_ERR,
+ .vsync_irq = MALIDP550_SE_IRQ_EOW,
},
.dc_irq_map = {
.irq_mask = MALIDP550_DC_IRQ_CONF_VALID |
@@ -689,7 +691,9 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
struct drm_device *drm = arg;
struct malidp_drm *malidp = drm->dev_private;
struct malidp_hw_device *hwdev = malidp->dev;
+ const struct malidp_irq_map *se = &hwdev->map.se_irq_map;
u32 status, mask;
+ irqreturn_t ret = IRQ_HANDLED;
status = malidp_hw_read(hwdev, hwdev->map.se_base + MALIDP_REG_STATUS);
if (!(status & hwdev->map.se_irq_map.irq_mask))
@@ -698,15 +702,47 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
mask = malidp_hw_read(hwdev, hwdev->map.se_base + MALIDP_REG_MASKIRQ);
status = malidp_hw_read(hwdev, hwdev->map.se_base + MALIDP_REG_STATUS);
status &= mask;
- /* ToDo: status decoding and firing up of VSYNC and page flip events */
+
+ if (status & se->vsync_irq) {
+ unsigned long irqflags;
+ /*
+ * We can't unreference the framebuffer here, so we queue it
+ * up on our threaded handler.
+ */
+ spin_lock_irqsave(&malidp->mw_lock, irqflags);
+ list_add_tail(&malidp->current_mw->list,
+ &malidp->finished_mw_jobs);
+ malidp->current_mw = NULL;
+ spin_unlock_irqrestore(&malidp->mw_lock, irqflags);
+
+ ret = IRQ_WAKE_THREAD;
+ }
malidp_hw_clear_irq(hwdev, MALIDP_SE_BLOCK, status);
- return IRQ_HANDLED;
+ return ret;
}
static irqreturn_t malidp_se_irq_thread_handler(int irq, void *arg)
{
+ struct drm_device *drm = arg;
+ struct malidp_drm *malidp = drm->dev_private;
+ struct list_head finished;
+ struct malidp_mw_job *tmp, *pos;
+ unsigned long irqflags;
+
+ INIT_LIST_HEAD(&finished);
+
+ spin_lock_irqsave(&malidp->mw_lock, irqflags);
+ list_splice_tail_init(&malidp->finished_mw_jobs,
+ &finished);
+ spin_unlock_irqrestore(&malidp->mw_lock, irqflags);
+
+ list_for_each_entry_safe(pos, tmp, &finished, list) {
+ list_del(&pos->list);
+ malidp_mw_job_cleanup(drm, pos);
+ }
+
return IRQ_HANDLED;
}
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
new file mode 100644
index 0000000..69e423c
--- /dev/null
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -0,0 +1,297 @@
+/*
+ * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
+ * Author: Brian Starkey <brian.starkey@arm.com>
+ *
+ * This program is free software and is provided to you under the terms of the
+ * GNU General Public License version 2 as published by the Free Software
+ * Foundation, and any use by you of this program is subject to the terms
+ * of such GNU licence.
+ *
+ * ARM Mali DP Writeback connector implementation
+ */
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drmP.h>
+#include <drm/drm_writeback.h>
+
+#include "malidp_drv.h"
+#include "malidp_hw.h"
+#include "malidp_mw.h"
+
+#define mw_conn_to_malidp_device(x) container_of(x, struct malidp_drm, mw_connector)
+#define to_mw_state(_state) (struct malidp_mw_connector_state *)(_state)
+
+struct malidp_mw_connector_state {
+ struct drm_connector_state base;
+ struct malidp_mw_job *job;
+ dma_addr_t addrs[2];
+ s32 pitches[2];
+ u8 format;
+ u8 n_planes;
+};
+
+void malidp_mw_job_cleanup(struct drm_device *drm,
+ struct malidp_mw_job *job)
+{
+ DRM_DEV_DEBUG_DRIVER(drm->dev, "MW job cleanup %p\n", job);
+ drm_framebuffer_unreference(job->fb);
+ kfree(job);
+}
+
+static int malidp_mw_connector_get_modes(struct drm_connector *connector)
+{
+ struct drm_device *dev = connector->dev;
+
+ return drm_add_modes_noedid(connector, dev->mode_config.max_width,
+ dev->mode_config.max_height);
+}
+
+static enum drm_mode_status
+malidp_mw_connector_mode_valid(struct drm_connector *connector,
+ struct drm_display_mode *mode)
+{
+ struct drm_device *dev = connector->dev;
+ struct drm_mode_config *mode_config = &dev->mode_config;
+ int w = mode->hdisplay, h = mode->vdisplay;
+
+ if ((w < mode_config->min_width) || (w > mode_config->max_width))
+ return MODE_BAD_HVALUE;
+
+ if ((h < mode_config->min_height) || (h > mode_config->max_height))
+ return MODE_BAD_VVALUE;
+
+ return MODE_OK;
+}
+
+const struct drm_connector_helper_funcs malidp_mw_connector_helper_funcs = {
+ .get_modes = malidp_mw_connector_get_modes,
+ .mode_valid = malidp_mw_connector_mode_valid,
+};
+
+static enum drm_connector_status
+malidp_mw_connector_detect(struct drm_connector *connector, bool force)
+{
+ return connector_status_disconnected;
+}
+
+static void malidp_mw_connector_destroy(struct drm_connector *connector)
+{
+ drm_connector_cleanup(connector);
+}
+
+static struct drm_connector_state *
+malidp_mw_connector_duplicate_state(struct drm_connector *connector)
+{
+ struct malidp_mw_connector_state *mw_state;
+
+ if (WARN_ON(!connector->state))
+ return NULL;
+
+ mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
+ if (!mw_state)
+ return NULL;
+
+ /* No need to preserve any of our driver-local data */
+ __drm_atomic_helper_connector_duplicate_state(connector, &mw_state->base);
+
+ return &mw_state->base;
+}
+
+static void malidp_mw_connector_destroy_state(struct drm_connector *connector,
+ struct drm_connector_state *state)
+{
+ struct malidp_mw_connector_state *mw_state = to_mw_state(state);
+
+ __drm_atomic_helper_connector_destroy_state(&mw_state->base);
+ if (mw_state->job)
+ malidp_mw_job_cleanup(connector->dev, mw_state->job);
+ kfree(mw_state);
+}
+
+static const struct drm_connector_funcs malidp_mw_connector_funcs = {
+ .dpms = drm_atomic_helper_connector_dpms,
+ .reset = drm_atomic_helper_connector_reset,
+ .detect = malidp_mw_connector_detect,
+ .fill_modes = drm_helper_probe_single_connector_modes,
+ .destroy = malidp_mw_connector_destroy,
+ .atomic_duplicate_state = malidp_mw_connector_duplicate_state,
+ .atomic_destroy_state = malidp_mw_connector_destroy_state,
+};
+
+static int
+malidp_mw_encoder_atomic_check(struct drm_encoder *encoder,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ struct drm_connector *conn = conn_state->connector;
+ struct drm_framebuffer *fb = conn_state->fb;
+ struct malidp_drm *malidp = mw_conn_to_malidp_device(conn);
+ struct malidp_mw_connector_state *mw_state;
+ int i, n_planes;
+
+ mw_state = (struct malidp_mw_connector_state *)conn_state;
+
+ if (!conn_state->fb)
+ return 0;
+
+ if ((fb->width != crtc_state->mode.hdisplay) ||
+ (fb->height != crtc_state->mode.vdisplay)) {
+ DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
+ fb->width, fb->height);
+ return -EINVAL;
+ }
+
+ mw_state->format =
+ malidp_hw_get_format_id(&malidp->dev->map, SE_MEMWRITE,
+ fb->pixel_format);
+ if (mw_state->format == MALIDP_INVALID_FORMAT_ID) {
+ char *format_name = drm_get_format_name(fb->pixel_format);
+ DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
+ kfree(format_name);
+ return -EINVAL;
+ }
+
+ n_planes = drm_format_num_planes(fb->pixel_format);
+ for (i = 0; i < n_planes; i++) {
+ struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, i);
+ if (!malidp_hw_pitch_valid(malidp->dev, fb->pitches[i])) {
+ DRM_DEBUG_KMS("Invalid pitch %u for plane %d\n",
+ fb->pitches[i], i);
+ return -EINVAL;
+ }
+ mw_state->pitches[i] = fb->pitches[i];
+ mw_state->addrs[i] = obj->paddr + fb->offsets[i];
+ }
+ mw_state->n_planes = n_planes;
+
+ mw_state->job = kmalloc(sizeof(*mw_state->job), GFP_KERNEL);
+ if (!mw_state->job)
+ return -ENOMEM;
+
+ DRM_DEV_DEBUG_DRIVER(conn->dev->dev, "MW job create %p\n",
+ mw_state->job);
+ /* We can take ownership of the framebuffer reference in the job. */
+ mw_state->job->fb = conn_state->fb;
+ conn_state->fb = NULL;
+
+ return 0;
+}
+
+static const struct drm_encoder_helper_funcs malidp_mw_encoder_helper_funcs = {
+ .atomic_check = malidp_mw_encoder_atomic_check,
+};
+
+static void malidp_mw_encoder_destroy(struct drm_encoder *encoder)
+{
+ drm_encoder_cleanup(encoder);
+}
+
+static const struct drm_encoder_funcs malidp_mw_encoder_funcs = {
+ .destroy = malidp_mw_encoder_destroy,
+};
+
+static u32 *get_writeback_formats(struct malidp_drm *malidp, int *n_formats)
+{
+ const struct malidp_hw_regmap *map = &malidp->dev->map;
+ u32 *formats;
+ int n, i;
+
+ formats = kcalloc(map->n_pixel_formats, sizeof(*formats),
+ GFP_KERNEL);
+ if (!formats)
+ return NULL;
+
+ for (n = 0, i = 0; i < map->n_pixel_formats; i++) {
+ if (map->pixel_formats[i].layer & SE_MEMWRITE)
+ formats[n++] = map->pixel_formats[i].format;
+ }
+
+ *n_formats = n;
+ return formats;
+}
+
+int malidp_mw_connector_init(struct drm_device *drm)
+{
+ struct malidp_drm *malidp = drm->dev_private;
+ u32 *formats;
+ int ret, n_formats;
+
+ if (!malidp->dev->enable_memwrite)
+ return 0;
+
+ drm_encoder_helper_add(&malidp->mw_encoder, &malidp_mw_encoder_helper_funcs);
+ malidp->mw_encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
+ ret = drm_encoder_init(drm, &malidp->mw_encoder, &malidp_mw_encoder_funcs,
+ DRM_MODE_ENCODER_VIRTUAL, NULL);
+ if (ret)
+ return ret;
+
+ drm_connector_helper_add(&malidp->mw_connector,
+ &malidp_mw_connector_helper_funcs);
+ malidp->mw_connector.interlace_allowed = 0;
+
+ formats = get_writeback_formats(malidp, &n_formats);
+ if (!formats) {
+ ret = -ENOMEM;
+ goto err_encoder;
+ }
+
+ ret = drm_writeback_connector_init(drm, &malidp->mw_connector,
+ &malidp_mw_connector_funcs,
+ formats, n_formats);
+ kfree(formats);
+ if (ret)
+ goto err_encoder;
+
+ ret = drm_mode_connector_attach_encoder(&malidp->mw_connector,
+ &malidp->mw_encoder);
+ if (ret)
+ goto err_connector;
+
+ return 0;
+
+err_connector:
+ drm_connector_cleanup(&malidp->mw_connector);
+err_encoder:
+ drm_encoder_cleanup(&malidp->mw_encoder);
+ return ret;
+}
+
+void malidp_mw_atomic_commit(struct drm_device *drm,
+ struct drm_atomic_state *old_state)
+{
+ struct malidp_mw_connector_state *mw_state;
+ struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_hw_device *hwdev = malidp->dev;
+ struct drm_connector *mw_conn = &malidp->mw_connector;
+
+ mw_state = to_mw_state(mw_conn->state);
+
+ if (!mw_state)
+ return;
+
+ if (mw_state->job) {
+ struct drm_framebuffer *fb = mw_state->job->fb;
+ DRM_DEV_DEBUG_DRIVER(drm->dev, "Enable memwrite %ux%u:%d %pad fmt: %u\n",
+ fb->width, fb->height, mw_state->pitches[0],
+ &mw_state->addrs[0], mw_state->format);
+
+ /* Queue up the job for completion handling */
+ DRM_DEV_DEBUG_DRIVER(drm->dev, "MW job queue %p\n", mw_state->job);
+
+ WARN_ON(malidp->current_mw);
+ malidp->current_mw = mw_state->job;
+ mw_state->job = NULL;
+
+ hwdev->enable_memwrite(hwdev, mw_state->addrs, mw_state->pitches,
+ mw_state->n_planes, fb->width, fb->height,
+ mw_state->format);
+ } else {
+ DRM_DEV_DEBUG_DRIVER(drm->dev, "Disable memwrite\n");
+ hwdev->disable_memwrite(hwdev);
+ }
+}
diff --git a/drivers/gpu/drm/arm/malidp_mw.h b/drivers/gpu/drm/arm/malidp_mw.h
new file mode 100644
index 0000000..db7b2b0
--- /dev/null
+++ b/drivers/gpu/drm/arm/malidp_mw.h
@@ -0,0 +1,25 @@
+/*
+ * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
+ * Author: Brian Starkey <brian.starkey@arm.com>
+ *
+ * This program is free software and is provided to you under the terms of the
+ * GNU General Public License version 2 as published by the Free Software
+ * Foundation, and any use by you of this program is subject to the terms
+ * of such GNU licence.
+ *
+ */
+
+#ifndef __MALIDP_MW_H__
+#define __MALIDP_MW_H__
+
+struct malidp_mw_job {
+ struct list_head list;
+ struct drm_framebuffer *fb;
+};
+
+int malidp_mw_connector_init(struct drm_device *drm);
+void malidp_mw_atomic_commit(struct drm_device *drm,
+ struct drm_atomic_state *old_state);
+void malidp_mw_job_cleanup(struct drm_device *drm,
+ struct malidp_mw_job *job);
+#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [RFC PATCH v2 7/9] drm: atomic: factor out common out-fence operations
2016-10-26 8:54 [RFC PATCH v2 0/9] Introduce writeback connectors Brian Starkey
` (5 preceding siblings ...)
2016-10-26 8:55 ` [RFC PATCH v2 6/9] drm: mali-dp: Add writeback connector Brian Starkey
@ 2016-10-26 8:55 ` Brian Starkey
2016-10-26 11:01 ` Daniel Vetter
` (2 more replies)
2016-10-26 8:55 ` [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors Brian Starkey
2016-10-26 8:55 ` [RFC PATCH v2 9/9] drm: mali-dp: Add writeback out-fence support Brian Starkey
8 siblings, 3 replies; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 8:55 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, linux-media
Some parts of setting up the CRTC out-fence can be re-used for
writeback out-fences. Factor this out into a separate function.
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
drivers/gpu/drm/drm_atomic.c | 64 +++++++++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f434f34..3f8fc97 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1693,37 +1693,46 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_clean_old_fb);
-static int crtc_setup_out_fence(struct drm_crtc *crtc,
- struct drm_crtc_state *crtc_state,
- struct drm_out_fence_state *fence_state)
+static struct fence *get_crtc_fence(struct drm_crtc *crtc,
+ struct drm_crtc_state *crtc_state)
{
struct fence *fence;
- fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
- if (fence_state->fd < 0) {
- return fence_state->fd;
- }
-
- fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
- crtc_state->out_fence_ptr = NULL;
-
- if (put_user(fence_state->fd, fence_state->out_fence_ptr))
- return -EFAULT;
-
fence = kzalloc(sizeof(*fence), GFP_KERNEL);
if (!fence)
- return -ENOMEM;
+ return NULL;
fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
crtc->fence_context, ++crtc->fence_seqno);
+ crtc_state->event->base.fence = fence_get(fence);
+
+ return fence;
+}
+
+static int setup_out_fence(struct drm_out_fence_state *fence_state,
+ u64 __user *out_fence_ptr,
+ struct fence *fence)
+{
+ int ret;
+
+ DRM_DEBUG_ATOMIC("Putting out-fence %p into user ptr: %p\n",
+ fence, out_fence_ptr);
+
+ fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
+ if (fence_state->fd < 0)
+ return fence_state->fd;
+
+ ret = put_user(fence_state->fd, out_fence_ptr);
+ if (ret)
+ return ret;
+
+ fence_state->out_fence_ptr = out_fence_ptr;
+
fence_state->sync_file = sync_file_create(fence);
- if(!fence_state->sync_file) {
- fence_put(fence);
+ if (!fence_state->sync_file)
return -ENOMEM;
- }
- crtc_state->event->base.fence = fence_get(fence);
return 0;
}
@@ -1896,10 +1905,21 @@ retry:
}
if (crtc_state->out_fence_ptr) {
- ret = crtc_setup_out_fence(crtc, crtc_state,
- &fence_state[fence_idx++]);
- if (ret)
+ struct fence *fence = get_crtc_fence(crtc, crtc_state);
+ if (!fence) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = setup_out_fence(&fence_state[fence_idx++],
+ crtc_state->out_fence_ptr,
+ fence);
+ if (ret) {
+ fence_put(fence);
goto out;
+ }
+
+ crtc_state->out_fence_ptr = NULL;
}
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 7/9] drm: atomic: factor out common out-fence operations
2016-10-26 8:55 ` [RFC PATCH v2 7/9] drm: atomic: factor out common out-fence operations Brian Starkey
@ 2016-10-26 11:01 ` Daniel Vetter
2016-10-26 21:30 ` Gustavo Padovan
2016-10-26 21:45 ` Gustavo Padovan
2 siblings, 0 replies; 25+ messages in thread
From: Daniel Vetter @ 2016-10-26 11:01 UTC (permalink / raw)
To: Brian Starkey; +Cc: dri-devel, linux-kernel, linux-media
On Wed, Oct 26, 2016 at 09:55:06AM +0100, Brian Starkey wrote:
> Some parts of setting up the CRTC out-fence can be re-used for
> writeback out-fences. Factor this out into a separate function.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Cc: Gustavo here pls, probably best if he reviews this one.
-Daniel
> ---
> drivers/gpu/drm/drm_atomic.c | 64 +++++++++++++++++++++++++++---------------
> 1 file changed, 42 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f434f34..3f8fc97 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1693,37 +1693,46 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
> -static int crtc_setup_out_fence(struct drm_crtc *crtc,
> - struct drm_crtc_state *crtc_state,
> - struct drm_out_fence_state *fence_state)
> +static struct fence *get_crtc_fence(struct drm_crtc *crtc,
> + struct drm_crtc_state *crtc_state)
> {
> struct fence *fence;
>
> - fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> - if (fence_state->fd < 0) {
> - return fence_state->fd;
> - }
> -
> - fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> - crtc_state->out_fence_ptr = NULL;
> -
> - if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> - return -EFAULT;
> -
> fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> if (!fence)
> - return -ENOMEM;
> + return NULL;
>
> fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> crtc->fence_context, ++crtc->fence_seqno);
>
> + crtc_state->event->base.fence = fence_get(fence);
> +
> + return fence;
> +}
> +
> +static int setup_out_fence(struct drm_out_fence_state *fence_state,
> + u64 __user *out_fence_ptr,
> + struct fence *fence)
> +{
> + int ret;
> +
> + DRM_DEBUG_ATOMIC("Putting out-fence %p into user ptr: %p\n",
> + fence, out_fence_ptr);
> +
> + fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> + if (fence_state->fd < 0)
> + return fence_state->fd;
> +
> + ret = put_user(fence_state->fd, out_fence_ptr);
> + if (ret)
> + return ret;
> +
> + fence_state->out_fence_ptr = out_fence_ptr;
> +
> fence_state->sync_file = sync_file_create(fence);
> - if(!fence_state->sync_file) {
> - fence_put(fence);
> + if (!fence_state->sync_file)
> return -ENOMEM;
> - }
>
> - crtc_state->event->base.fence = fence_get(fence);
> return 0;
> }
>
> @@ -1896,10 +1905,21 @@ retry:
> }
>
> if (crtc_state->out_fence_ptr) {
> - ret = crtc_setup_out_fence(crtc, crtc_state,
> - &fence_state[fence_idx++]);
> - if (ret)
> + struct fence *fence = get_crtc_fence(crtc, crtc_state);
> + if (!fence) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = setup_out_fence(&fence_state[fence_idx++],
> + crtc_state->out_fence_ptr,
> + fence);
> + if (ret) {
> + fence_put(fence);
> goto out;
> + }
> +
> + crtc_state->out_fence_ptr = NULL;
> }
> }
>
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 7/9] drm: atomic: factor out common out-fence operations
2016-10-26 8:55 ` [RFC PATCH v2 7/9] drm: atomic: factor out common out-fence operations Brian Starkey
2016-10-26 11:01 ` Daniel Vetter
@ 2016-10-26 21:30 ` Gustavo Padovan
2016-10-26 21:45 ` Gustavo Padovan
2 siblings, 0 replies; 25+ messages in thread
From: Gustavo Padovan @ 2016-10-26 21:30 UTC (permalink / raw)
To: Brian Starkey; +Cc: dri-devel, linux-kernel, linux-media
2016-10-26 Brian Starkey <brian.starkey@arm.com>:
> Some parts of setting up the CRTC out-fence can be re-used for
> writeback out-fences. Factor this out into a separate function.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 64 +++++++++++++++++++++++++++---------------
> 1 file changed, 42 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f434f34..3f8fc97 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1693,37 +1693,46 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
> -static int crtc_setup_out_fence(struct drm_crtc *crtc,
> - struct drm_crtc_state *crtc_state,
> - struct drm_out_fence_state *fence_state)
> +static struct fence *get_crtc_fence(struct drm_crtc *crtc,
> + struct drm_crtc_state *crtc_state)
> {
> struct fence *fence;
>
> - fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> - if (fence_state->fd < 0) {
> - return fence_state->fd;
> - }
> -
> - fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> - crtc_state->out_fence_ptr = NULL;
> -
> - if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> - return -EFAULT;
> -
> fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> if (!fence)
> - return -ENOMEM;
> + return NULL;
>
> fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> crtc->fence_context, ++crtc->fence_seqno);
>
> + crtc_state->event->base.fence = fence_get(fence);
> +
> + return fence;
> +}
> +
> +static int setup_out_fence(struct drm_out_fence_state *fence_state,
> + u64 __user *out_fence_ptr,
> + struct fence *fence)
> +{
> + int ret;
> +
> + DRM_DEBUG_ATOMIC("Putting out-fence %p into user ptr: %p\n",
> + fence, out_fence_ptr);
> +
> + fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> + if (fence_state->fd < 0)
> + return fence_state->fd;
> +
> + ret = put_user(fence_state->fd, out_fence_ptr);
> + if (ret)
> + return ret;
> +
> + fence_state->out_fence_ptr = out_fence_ptr;
> +
> fence_state->sync_file = sync_file_create(fence);
> - if(!fence_state->sync_file) {
> - fence_put(fence);
> + if (!fence_state->sync_file)
> return -ENOMEM;
> - }
>
> - crtc_state->event->base.fence = fence_get(fence);
> return 0;
> }
>
> @@ -1896,10 +1905,21 @@ retry:
> }
>
> if (crtc_state->out_fence_ptr) {
> - ret = crtc_setup_out_fence(crtc, crtc_state,
> - &fence_state[fence_idx++]);
> - if (ret)
> + struct fence *fence = get_crtc_fence(crtc, crtc_state);
> + if (!fence) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = setup_out_fence(&fence_state[fence_idx++],
> + crtc_state->out_fence_ptr,
> + fence);
> + if (ret) {
> + fence_put(fence);
> goto out;
> + }
> +
> + crtc_state->out_fence_ptr = NULL;
Yes, this split makes sense. I can break crtc_setup_out_fence() in
my out_fence patch already.
Gustavo
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 7/9] drm: atomic: factor out common out-fence operations
2016-10-26 8:55 ` [RFC PATCH v2 7/9] drm: atomic: factor out common out-fence operations Brian Starkey
2016-10-26 11:01 ` Daniel Vetter
2016-10-26 21:30 ` Gustavo Padovan
@ 2016-10-26 21:45 ` Gustavo Padovan
2016-10-27 8:28 ` Brian Starkey
2 siblings, 1 reply; 25+ messages in thread
From: Gustavo Padovan @ 2016-10-26 21:45 UTC (permalink / raw)
To: Brian Starkey; +Cc: dri-devel, linux-kernel, linux-media
2016-10-26 Brian Starkey <brian.starkey@arm.com>:
> Some parts of setting up the CRTC out-fence can be re-used for
> writeback out-fences. Factor this out into a separate function.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 64 +++++++++++++++++++++++++++---------------
> 1 file changed, 42 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f434f34..3f8fc97 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1693,37 +1693,46 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
> -static int crtc_setup_out_fence(struct drm_crtc *crtc,
> - struct drm_crtc_state *crtc_state,
> - struct drm_out_fence_state *fence_state)
> +static struct fence *get_crtc_fence(struct drm_crtc *crtc,
> + struct drm_crtc_state *crtc_state)
> {
> struct fence *fence;
>
> - fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> - if (fence_state->fd < 0) {
> - return fence_state->fd;
> - }
> -
> - fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> - crtc_state->out_fence_ptr = NULL;
> -
> - if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> - return -EFAULT;
> -
> fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> if (!fence)
> - return -ENOMEM;
> + return NULL;
>
> fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> crtc->fence_context, ++crtc->fence_seqno);
>
> + crtc_state->event->base.fence = fence_get(fence);
> +
> + return fence;
> +}
> +
> +static int setup_out_fence(struct drm_out_fence_state *fence_state,
> + u64 __user *out_fence_ptr,
> + struct fence *fence)
> +{
> + int ret;
> +
> + DRM_DEBUG_ATOMIC("Putting out-fence %p into user ptr: %p\n",
> + fence, out_fence_ptr);
%p should be kept for your internal debug only. Make sure to remove
anything that exposes kernel address when sending patches.
Gustavo
^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
2016-10-26 8:54 [RFC PATCH v2 0/9] Introduce writeback connectors Brian Starkey
` (6 preceding siblings ...)
2016-10-26 8:55 ` [RFC PATCH v2 7/9] drm: atomic: factor out common out-fence operations Brian Starkey
@ 2016-10-26 8:55 ` Brian Starkey
2016-10-26 11:09 ` Daniel Vetter
` (2 more replies)
2016-10-26 8:55 ` [RFC PATCH v2 9/9] drm: mali-dp: Add writeback out-fence support Brian Starkey
8 siblings, 3 replies; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 8:55 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, linux-media
Add the OUT_FENCE_PTR property to writeback connectors, to enable
userspace to get a fence which will signal once the writeback is
complete.
A timeline is added to drm_connector for use by the writeback
out-fences. It is up to drivers to check for a fence in the
connector_state and signal the it appropriately when their writeback has
finished.
It is not allowed to request an out-fence without a framebuffer attached
to the connector.
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
drivers/gpu/drm/drm_atomic.c | 60 +++++++++++++++++++++++---
drivers/gpu/drm/drm_atomic_helper.c | 5 ++-
drivers/gpu/drm/drm_writeback.c | 80 +++++++++++++++++++++++++++++++++++
include/drm/drm_connector.h | 14 ++++++
include/drm/drm_writeback.h | 2 +
5 files changed, 155 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3f8fc97..061ea13 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -30,6 +30,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_mode.h>
#include <drm/drm_plane_helper.h>
+#include <drm/drm_writeback.h>
#include <linux/sync_file.h>
#include "drm_crtc_internal.h"
@@ -646,6 +647,12 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
return -EINVAL;
}
+ if (state->out_fence && !state->fb) {
+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n",
+ connector->base.id, connector->name);
+ return -EINVAL;
+ }
+
return 0;
}
@@ -1047,6 +1054,8 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
drm_atomic_set_fb_for_connector(state, fb);
if (fb)
drm_framebuffer_unreference(fb);
+ } else if (property == config->prop_out_fence_ptr) {
+ state->out_fence_ptr = u64_to_user_ptr(val);
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
@@ -1088,6 +1097,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
} else if (property == config->writeback_fb_id_property) {
/* Writeback framebuffer is one-shot, write and forget */
*val = 0;
+ } else if (property == config->prop_out_fence_ptr) {
+ *val = 0;
} else if (connector->funcs->atomic_get_property) {
return connector->funcs->atomic_get_property(connector,
state, property, val);
@@ -1736,6 +1747,39 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state,
return 0;
}
+static int setup_connector_out_fences(struct drm_atomic_state *state,
+ struct drm_out_fence_state *fence_state,
+ int *fence_idx)
+{
+ struct drm_connector *conn;
+ struct drm_connector_state *conn_state;
+ int i, ret;
+
+ for_each_connector_in_state(state, conn, conn_state, i) {
+ struct fence *fence;
+
+ if (!conn_state->out_fence_ptr)
+ continue;
+
+ fence = drm_writeback_get_out_fence(conn, conn_state);
+ if (!fence)
+ return -ENOMEM;
+
+ ret = setup_out_fence(&fence_state[(*fence_idx)++],
+ conn_state->out_fence_ptr,
+ fence);
+ if (ret) {
+ fence_put(fence);
+ return ret;
+ }
+
+ /* One-time usage only */
+ conn_state->out_fence_ptr = NULL;
+ }
+
+ return 0;
+}
+
int drm_mode_atomic_ioctl(struct drm_device *dev,
void *data, struct drm_file *file_priv)
{
@@ -1868,8 +1912,8 @@ retry:
drm_mode_object_unreference(obj);
}
- fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
- GFP_KERNEL);
+ fence_state = kcalloc(dev->mode_config.num_crtc + state->num_connector,
+ sizeof(*fence_state), GFP_KERNEL);
if (!fence_state) {
ret = -ENOMEM;
goto out;
@@ -1929,10 +1973,16 @@ retry:
* Below we call drm_atomic_state_free for it.
*/
ret = drm_atomic_check_only(state);
- } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
- ret = drm_atomic_nonblocking_commit(state);
} else {
- ret = drm_atomic_commit(state);
+ ret = setup_connector_out_fences(state, fence_state,
+ &fence_idx);
+ if (ret)
+ goto out;
+
+ if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK)
+ ret = drm_atomic_nonblocking_commit(state);
+ else
+ ret = drm_atomic_commit(state);
}
if (!ret)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index bba8672..88da299 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3234,8 +3234,9 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
if (state->crtc)
drm_connector_reference(connector);
- /* Don't copy over framebuffers, they are used only once */
+ /* Don't copy over framebuffers or fences, they are used only once */
state->fb = NULL;
+ state->out_fence = NULL;
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
@@ -3365,6 +3366,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
drm_connector_unreference(state->connector);
if (state->fb)
drm_framebuffer_unreference(state->fb);
+
+ fence_put(state->out_fence);
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 5a6e0ad..4a6ef30 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -11,6 +11,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_property.h>
#include <drm/drmP.h>
+#include <linux/fence.h>
/**
* DOC: overview
@@ -32,6 +33,16 @@
* explicitly cleared in order to make a subsequent commit which doesn't use
* writeback.
*
+ * Unlike with planes, when a writeback framebuffer is removed by userspace DRM
+ * makes no attempt to remove it from active use by the connector. This is
+ * because no method is provided to abort a writeback operation, and in any
+ * case making a new commit whilst a writeback is ongoing is undefined (see
+ * OUT_FENCE_PTR below). As soon as the current writeback is finished, the
+ * framebuffer will automatically no longer be in active use. As it will also
+ * have already been removed from the framebuffer list, there will be no way for
+ * any userspace application to retrieve a reference to it in the intervening
+ * period.
+ *
* Writeback connectors have several additional properties, which userspace
* can use to query and control them:
*
@@ -52,8 +63,48 @@
* "PIXEL_FORMATS_SIZE":
* Immutable unsigned range property storing the number of entries in the
* PIXEL_FORMATS array.
+ *
+ * "OUT_FENCE_PTR":
+ * Userspace can provide the address of a 64-bit integer in this property,
+ * which will be filled with a sync_file file descriptor by the kernel.
+ * The sync_file will signal when the associated writeback has finished.
+ * Userspace should wait for this fence to signal before making another
+ * commit affecting any of the same CRTCs, Planes or Connectors.
+ * **Failure to do so will result in undefined behaviour.**
+ * For this reason it is strongly recommended that all userspace
+ * applications making use of writeback connectors *always* retrieve an
+ * out-fence for the commit and use it appropriately.
+ * From userspace, this property will always read as zero.
*/
+#define fence_to_connector(x) container_of(x->lock, struct drm_connector, fence_lock)
+
+static const char *drm_writeback_fence_get_driver_name(struct fence *fence)
+{
+ struct drm_connector *connector = fence_to_connector(fence);
+
+ return connector->dev->driver->name;
+}
+
+static const char *drm_writeback_fence_get_timeline_name(struct fence *fence)
+{
+ struct drm_connector *connector = fence_to_connector(fence);
+
+ return connector->timeline_name;
+}
+
+static bool drm_writeback_fence_enable_signaling(struct fence *fence)
+{
+ return true;
+}
+
+static const struct fence_ops drm_writeback_fence_ops = {
+ .get_driver_name = drm_writeback_fence_get_driver_name,
+ .get_timeline_name = drm_writeback_fence_get_timeline_name,
+ .enable_signaling = drm_writeback_fence_enable_signaling,
+ .wait = fence_default_wait,
+};
+
/**
* create_writeback_properties - Create writeback connector-specific properties
* @dev: DRM device
@@ -136,6 +187,14 @@ int drm_writeback_connector_init(struct drm_device *dev,
if (ret)
goto fail;
+ connector->fence_context = fence_context_alloc(1);
+ spin_lock_init(&connector->fence_lock);
+ snprintf(connector->timeline_name, sizeof(connector->timeline_name),
+ "CONNECTOR:%d", connector->base.id);
+
+ drm_object_attach_property(&connector->base,
+ config->prop_out_fence_ptr, 0);
+
drm_object_attach_property(&connector->base,
config->writeback_fb_id_property, 0);
@@ -155,3 +214,24 @@ fail:
return ret;
}
EXPORT_SYMBOL(drm_writeback_connector_init);
+
+struct fence *drm_writeback_get_out_fence(struct drm_connector *connector,
+ struct drm_connector_state *conn_state)
+{
+ struct fence *fence;
+
+ if (WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK))
+ return NULL;
+
+ fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+ if (!fence)
+ return NULL;
+
+ fence_init(fence, &drm_writeback_fence_ops, &connector->fence_lock,
+ connector->fence_context, ++connector->fence_seqno);
+
+ conn_state->out_fence = fence_get(fence);
+
+ return fence;
+}
+EXPORT_SYMBOL(drm_writeback_get_out_fence);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index a5e3778..7d40537 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -199,6 +199,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
* @best_encoder: can be used by helpers and drivers to select the encoder
* @state: backpointer to global drm_atomic_state
* @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK
+ * @out_fence: Fence which will clear when the framebuffer write has finished
*/
struct drm_connector_state {
struct drm_connector *connector;
@@ -216,6 +217,9 @@ struct drm_connector_state {
struct drm_atomic_state *state;
struct drm_framebuffer *fb; /* do not write directly, use drm_atomic_set_fb_for_connector() */
+
+ struct fence *out_fence;
+ u64 __user *out_fence_ptr;
};
/**
@@ -546,6 +550,10 @@ struct drm_cmdline_mode {
* @tile_v_loc: vertical location of this tile
* @tile_h_size: horizontal size of this tile.
* @tile_v_size: vertical size of this tile.
+ * @fence_context: context for fence signalling
+ * @fence_lock: fence lock for the fence context
+ * @fence_seqno: seqno variable to create fences
+ * @timeline_name: fence timeline name
*
* Each connector may be connected to one or more CRTCs, or may be clonable by
* another connector if they can share a CRTC. Each connector also has a specific
@@ -694,6 +702,12 @@ struct drm_connector {
uint8_t num_h_tile, num_v_tile;
uint8_t tile_h_loc, tile_v_loc;
uint16_t tile_h_size, tile_v_size;
+
+ /* fence timelines info for DRM out-fences */
+ unsigned int fence_context;
+ spinlock_t fence_lock;
+ unsigned long fence_seqno;
+ char timeline_name[32];
};
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index afdc2742..01f33bc 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -16,4 +16,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
const struct drm_connector_funcs *funcs,
u32 *formats, int n_formats);
+struct fence *drm_writeback_get_out_fence(struct drm_connector *connector,
+ struct drm_connector_state *conn_state);
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
2016-10-26 8:55 ` [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors Brian Starkey
@ 2016-10-26 11:09 ` Daniel Vetter
2016-10-26 12:46 ` Brian Starkey
2016-10-26 21:40 ` Gustavo Padovan
2 siblings, 0 replies; 25+ messages in thread
From: Daniel Vetter @ 2016-10-26 11:09 UTC (permalink / raw)
To: Brian Starkey; +Cc: dri-devel, linux-kernel, linux-media
On Wed, Oct 26, 2016 at 09:55:07AM +0100, Brian Starkey wrote:
> Add the OUT_FENCE_PTR property to writeback connectors, to enable
> userspace to get a fence which will signal once the writeback is
> complete.
>
> A timeline is added to drm_connector for use by the writeback
> out-fences. It is up to drivers to check for a fence in the
> connector_state and signal the it appropriately when their writeback has
> finished.
>
> It is not allowed to request an out-fence without a framebuffer attached
> to the connector.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Ah, here it is, so much for reading patches strictly in-order ;-) One
small comment below, otherwise I think this looks good. Again review from
Gustavo for the out fences stuff would be really good (so pls cc him). And
I think some igt testcases to exercise all the corner-cases in here.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index a5e3778..7d40537 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -199,6 +199,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
> * @best_encoder: can be used by helpers and drivers to select the encoder
> * @state: backpointer to global drm_atomic_state
> * @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK
> + * @out_fence: Fence which will clear when the framebuffer write has finished
> */
> struct drm_connector_state {
> struct drm_connector *connector;
> @@ -216,6 +217,9 @@ struct drm_connector_state {
> struct drm_atomic_state *state;
>
> struct drm_framebuffer *fb; /* do not write directly, use drm_atomic_set_fb_for_connector() */
btw if you feel like adding a 2nd comment in-line like above, then that's
a clear signal that you should move your kerneldoc struct member comments
to the inline style. You can freely mix&match inline with top-level struct
member documentation, so no need to change them all. You also missed the
doc for out_fence_ptr, 0day won't like that.
> +
> + struct fence *out_fence;
> + u64 __user *out_fence_ptr;
writeback_ prefix for both imo, like in patch 1.
> };
>
> /**
> @@ -546,6 +550,10 @@ struct drm_cmdline_mode {
> * @tile_v_loc: vertical location of this tile
> * @tile_h_size: horizontal size of this tile.
> * @tile_v_size: vertical size of this tile.
> + * @fence_context: context for fence signalling
> + * @fence_lock: fence lock for the fence context
> + * @fence_seqno: seqno variable to create fences
> + * @timeline_name: fence timeline name
> *
> * Each connector may be connected to one or more CRTCs, or may be clonable by
> * another connector if they can share a CRTC. Each connector also has a specific
> @@ -694,6 +702,12 @@ struct drm_connector {
> uint8_t num_h_tile, num_v_tile;
> uint8_t tile_h_loc, tile_v_loc;
> uint16_t tile_h_size, tile_v_size;
> +
> + /* fence timelines info for DRM out-fences */
> + unsigned int fence_context;
> + spinlock_t fence_lock;
> + unsigned long fence_seqno;
> + char timeline_name[32];
Should all have writeout_ prefix. And at that point I wonder a bit whether
we shouldn't just go ahead and create a struct drm_writeout_connector, to
keep this stuff nicely separate. Only change visible to drivers would be
the type of drm_writeback_connector_init, and they'd need to
allocate/embedd a different struct. Worth it imo.
-Daniel
> };
>
> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index afdc2742..01f33bc 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -16,4 +16,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
> const struct drm_connector_funcs *funcs,
> u32 *formats, int n_formats);
>
> +struct fence *drm_writeback_get_out_fence(struct drm_connector *connector,
> + struct drm_connector_state *conn_state);
> #endif
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
2016-10-26 8:55 ` [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors Brian Starkey
2016-10-26 11:09 ` Daniel Vetter
@ 2016-10-26 12:46 ` Brian Starkey
2016-10-26 21:40 ` Gustavo Padovan
2 siblings, 0 replies; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 12:46 UTC (permalink / raw)
To: dri-devel; +Cc: gustavo, linux-kernel, linux-media
Hi Gustavo,
It would be great if you could cast your eye over this one as-well.
My intention was to be as similar to the CRTC out-fences as possible.
Thanks,
Brian
On Wed, Oct 26, 2016 at 09:55:07AM +0100, Brian Starkey wrote:
>Add the OUT_FENCE_PTR property to writeback connectors, to enable
>userspace to get a fence which will signal once the writeback is
>complete.
>
>A timeline is added to drm_connector for use by the writeback
>out-fences. It is up to drivers to check for a fence in the
>connector_state and signal the it appropriately when their writeback has
>finished.
>
>It is not allowed to request an out-fence without a framebuffer attached
>to the connector.
>
>Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>---
> drivers/gpu/drm/drm_atomic.c | 60 +++++++++++++++++++++++---
> drivers/gpu/drm/drm_atomic_helper.c | 5 ++-
> drivers/gpu/drm/drm_writeback.c | 80 +++++++++++++++++++++++++++++++++++
> include/drm/drm_connector.h | 14 ++++++
> include/drm/drm_writeback.h | 2 +
> 5 files changed, 155 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>index 3f8fc97..061ea13 100644
>--- a/drivers/gpu/drm/drm_atomic.c
>+++ b/drivers/gpu/drm/drm_atomic.c
>@@ -30,6 +30,7 @@
> #include <drm/drm_atomic.h>
> #include <drm/drm_mode.h>
> #include <drm/drm_plane_helper.h>
>+#include <drm/drm_writeback.h>
> #include <linux/sync_file.h>
>
> #include "drm_crtc_internal.h"
>@@ -646,6 +647,12 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
> return -EINVAL;
> }
>
>+ if (state->out_fence && !state->fb) {
>+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n",
>+ connector->base.id, connector->name);
>+ return -EINVAL;
>+ }
>+
> return 0;
> }
>
>@@ -1047,6 +1054,8 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> drm_atomic_set_fb_for_connector(state, fb);
> if (fb)
> drm_framebuffer_unreference(fb);
>+ } else if (property == config->prop_out_fence_ptr) {
>+ state->out_fence_ptr = u64_to_user_ptr(val);
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
>@@ -1088,6 +1097,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> } else if (property == config->writeback_fb_id_property) {
> /* Writeback framebuffer is one-shot, write and forget */
> *val = 0;
>+ } else if (property == config->prop_out_fence_ptr) {
>+ *val = 0;
> } else if (connector->funcs->atomic_get_property) {
> return connector->funcs->atomic_get_property(connector,
> state, property, val);
>@@ -1736,6 +1747,39 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state,
> return 0;
> }
>
>+static int setup_connector_out_fences(struct drm_atomic_state *state,
>+ struct drm_out_fence_state *fence_state,
>+ int *fence_idx)
>+{
>+ struct drm_connector *conn;
>+ struct drm_connector_state *conn_state;
>+ int i, ret;
>+
>+ for_each_connector_in_state(state, conn, conn_state, i) {
>+ struct fence *fence;
>+
>+ if (!conn_state->out_fence_ptr)
>+ continue;
>+
>+ fence = drm_writeback_get_out_fence(conn, conn_state);
>+ if (!fence)
>+ return -ENOMEM;
>+
>+ ret = setup_out_fence(&fence_state[(*fence_idx)++],
>+ conn_state->out_fence_ptr,
>+ fence);
>+ if (ret) {
>+ fence_put(fence);
>+ return ret;
>+ }
>+
>+ /* One-time usage only */
>+ conn_state->out_fence_ptr = NULL;
>+ }
>+
>+ return 0;
>+}
>+
> int drm_mode_atomic_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv)
> {
>@@ -1868,8 +1912,8 @@ retry:
> drm_mode_object_unreference(obj);
> }
>
>- fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
>- GFP_KERNEL);
>+ fence_state = kcalloc(dev->mode_config.num_crtc + state->num_connector,
>+ sizeof(*fence_state), GFP_KERNEL);
> if (!fence_state) {
> ret = -ENOMEM;
> goto out;
>@@ -1929,10 +1973,16 @@ retry:
> * Below we call drm_atomic_state_free for it.
> */
> ret = drm_atomic_check_only(state);
>- } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>- ret = drm_atomic_nonblocking_commit(state);
> } else {
>- ret = drm_atomic_commit(state);
>+ ret = setup_connector_out_fences(state, fence_state,
>+ &fence_idx);
>+ if (ret)
>+ goto out;
>+
>+ if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK)
>+ ret = drm_atomic_nonblocking_commit(state);
>+ else
>+ ret = drm_atomic_commit(state);
> }
>
> if (!ret)
>diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>index bba8672..88da299 100644
>--- a/drivers/gpu/drm/drm_atomic_helper.c
>+++ b/drivers/gpu/drm/drm_atomic_helper.c
>@@ -3234,8 +3234,9 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
> if (state->crtc)
> drm_connector_reference(connector);
>
>- /* Don't copy over framebuffers, they are used only once */
>+ /* Don't copy over framebuffers or fences, they are used only once */
> state->fb = NULL;
>+ state->out_fence = NULL;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>
>@@ -3365,6 +3366,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
> drm_connector_unreference(state->connector);
> if (state->fb)
> drm_framebuffer_unreference(state->fb);
>+
>+ fence_put(state->out_fence);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>
>diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>index 5a6e0ad..4a6ef30 100644
>--- a/drivers/gpu/drm/drm_writeback.c
>+++ b/drivers/gpu/drm/drm_writeback.c
>@@ -11,6 +11,7 @@
> #include <drm/drm_crtc.h>
> #include <drm/drm_property.h>
> #include <drm/drmP.h>
>+#include <linux/fence.h>
>
> /**
> * DOC: overview
>@@ -32,6 +33,16 @@
> * explicitly cleared in order to make a subsequent commit which doesn't use
> * writeback.
> *
>+ * Unlike with planes, when a writeback framebuffer is removed by userspace DRM
>+ * makes no attempt to remove it from active use by the connector. This is
>+ * because no method is provided to abort a writeback operation, and in any
>+ * case making a new commit whilst a writeback is ongoing is undefined (see
>+ * OUT_FENCE_PTR below). As soon as the current writeback is finished, the
>+ * framebuffer will automatically no longer be in active use. As it will also
>+ * have already been removed from the framebuffer list, there will be no way for
>+ * any userspace application to retrieve a reference to it in the intervening
>+ * period.
>+ *
> * Writeback connectors have several additional properties, which userspace
> * can use to query and control them:
> *
>@@ -52,8 +63,48 @@
> * "PIXEL_FORMATS_SIZE":
> * Immutable unsigned range property storing the number of entries in the
> * PIXEL_FORMATS array.
>+ *
>+ * "OUT_FENCE_PTR":
>+ * Userspace can provide the address of a 64-bit integer in this property,
>+ * which will be filled with a sync_file file descriptor by the kernel.
>+ * The sync_file will signal when the associated writeback has finished.
>+ * Userspace should wait for this fence to signal before making another
>+ * commit affecting any of the same CRTCs, Planes or Connectors.
>+ * **Failure to do so will result in undefined behaviour.**
>+ * For this reason it is strongly recommended that all userspace
>+ * applications making use of writeback connectors *always* retrieve an
>+ * out-fence for the commit and use it appropriately.
>+ * From userspace, this property will always read as zero.
> */
>
>+#define fence_to_connector(x) container_of(x->lock, struct drm_connector, fence_lock)
>+
>+static const char *drm_writeback_fence_get_driver_name(struct fence *fence)
>+{
>+ struct drm_connector *connector = fence_to_connector(fence);
>+
>+ return connector->dev->driver->name;
>+}
>+
>+static const char *drm_writeback_fence_get_timeline_name(struct fence *fence)
>+{
>+ struct drm_connector *connector = fence_to_connector(fence);
>+
>+ return connector->timeline_name;
>+}
>+
>+static bool drm_writeback_fence_enable_signaling(struct fence *fence)
>+{
>+ return true;
>+}
>+
>+static const struct fence_ops drm_writeback_fence_ops = {
>+ .get_driver_name = drm_writeback_fence_get_driver_name,
>+ .get_timeline_name = drm_writeback_fence_get_timeline_name,
>+ .enable_signaling = drm_writeback_fence_enable_signaling,
>+ .wait = fence_default_wait,
>+};
>+
> /**
> * create_writeback_properties - Create writeback connector-specific properties
> * @dev: DRM device
>@@ -136,6 +187,14 @@ int drm_writeback_connector_init(struct drm_device *dev,
> if (ret)
> goto fail;
>
>+ connector->fence_context = fence_context_alloc(1);
>+ spin_lock_init(&connector->fence_lock);
>+ snprintf(connector->timeline_name, sizeof(connector->timeline_name),
>+ "CONNECTOR:%d", connector->base.id);
>+
>+ drm_object_attach_property(&connector->base,
>+ config->prop_out_fence_ptr, 0);
>+
> drm_object_attach_property(&connector->base,
> config->writeback_fb_id_property, 0);
>
>@@ -155,3 +214,24 @@ fail:
> return ret;
> }
> EXPORT_SYMBOL(drm_writeback_connector_init);
>+
>+struct fence *drm_writeback_get_out_fence(struct drm_connector *connector,
>+ struct drm_connector_state *conn_state)
>+{
>+ struct fence *fence;
>+
>+ if (WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK))
>+ return NULL;
>+
>+ fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>+ if (!fence)
>+ return NULL;
>+
>+ fence_init(fence, &drm_writeback_fence_ops, &connector->fence_lock,
>+ connector->fence_context, ++connector->fence_seqno);
>+
>+ conn_state->out_fence = fence_get(fence);
>+
>+ return fence;
>+}
>+EXPORT_SYMBOL(drm_writeback_get_out_fence);
>diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>index a5e3778..7d40537 100644
>--- a/include/drm/drm_connector.h
>+++ b/include/drm/drm_connector.h
>@@ -199,6 +199,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
> * @best_encoder: can be used by helpers and drivers to select the encoder
> * @state: backpointer to global drm_atomic_state
> * @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK
>+ * @out_fence: Fence which will clear when the framebuffer write has finished
> */
> struct drm_connector_state {
> struct drm_connector *connector;
>@@ -216,6 +217,9 @@ struct drm_connector_state {
> struct drm_atomic_state *state;
>
> struct drm_framebuffer *fb; /* do not write directly, use drm_atomic_set_fb_for_connector() */
>+
>+ struct fence *out_fence;
>+ u64 __user *out_fence_ptr;
> };
>
> /**
>@@ -546,6 +550,10 @@ struct drm_cmdline_mode {
> * @tile_v_loc: vertical location of this tile
> * @tile_h_size: horizontal size of this tile.
> * @tile_v_size: vertical size of this tile.
>+ * @fence_context: context for fence signalling
>+ * @fence_lock: fence lock for the fence context
>+ * @fence_seqno: seqno variable to create fences
>+ * @timeline_name: fence timeline name
> *
> * Each connector may be connected to one or more CRTCs, or may be clonable by
> * another connector if they can share a CRTC. Each connector also has a specific
>@@ -694,6 +702,12 @@ struct drm_connector {
> uint8_t num_h_tile, num_v_tile;
> uint8_t tile_h_loc, tile_v_loc;
> uint16_t tile_h_size, tile_v_size;
>+
>+ /* fence timelines info for DRM out-fences */
>+ unsigned int fence_context;
>+ spinlock_t fence_lock;
>+ unsigned long fence_seqno;
>+ char timeline_name[32];
> };
>
> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
>diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
>index afdc2742..01f33bc 100644
>--- a/include/drm/drm_writeback.h
>+++ b/include/drm/drm_writeback.h
>@@ -16,4 +16,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
> const struct drm_connector_funcs *funcs,
> u32 *formats, int n_formats);
>
>+struct fence *drm_writeback_get_out_fence(struct drm_connector *connector,
>+ struct drm_connector_state *conn_state);
> #endif
>--
>1.7.9.5
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
2016-10-26 8:55 ` [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors Brian Starkey
2016-10-26 11:09 ` Daniel Vetter
2016-10-26 12:46 ` Brian Starkey
@ 2016-10-26 21:40 ` Gustavo Padovan
2016-10-27 10:07 ` Brian Starkey
2 siblings, 1 reply; 25+ messages in thread
From: Gustavo Padovan @ 2016-10-26 21:40 UTC (permalink / raw)
To: Brian Starkey; +Cc: dri-devel, linux-kernel, linux-media
2016-10-26 Brian Starkey <brian.starkey@arm.com>:
> Add the OUT_FENCE_PTR property to writeback connectors, to enable
> userspace to get a fence which will signal once the writeback is
> complete.
>
> A timeline is added to drm_connector for use by the writeback
> out-fences. It is up to drivers to check for a fence in the
> connector_state and signal the it appropriately when their writeback has
> finished.
>
> It is not allowed to request an out-fence without a framebuffer attached
> to the connector.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 60 +++++++++++++++++++++++---
> drivers/gpu/drm/drm_atomic_helper.c | 5 ++-
> drivers/gpu/drm/drm_writeback.c | 80 +++++++++++++++++++++++++++++++++++
> include/drm/drm_connector.h | 14 ++++++
> include/drm/drm_writeback.h | 2 +
> 5 files changed, 155 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3f8fc97..061ea13 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,6 +30,7 @@
> #include <drm/drm_atomic.h>
> #include <drm/drm_mode.h>
> #include <drm/drm_plane_helper.h>
> +#include <drm/drm_writeback.h>
> #include <linux/sync_file.h>
>
> #include "drm_crtc_internal.h"
> @@ -646,6 +647,12 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
> return -EINVAL;
> }
>
> + if (state->out_fence && !state->fb) {
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n",
> + connector->base.id, connector->name);
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> @@ -1047,6 +1054,8 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> drm_atomic_set_fb_for_connector(state, fb);
> if (fb)
> drm_framebuffer_unreference(fb);
> + } else if (property == config->prop_out_fence_ptr) {
> + state->out_fence_ptr = u64_to_user_ptr(val);
We need to move this out of the conn_state. For my v6 on CRTC out fences
I added drm_atomic_set_out_fence_for_crtc() and
drm_atomic_get_out_fence_for_crtc(). See padovan/fences.
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> @@ -1088,6 +1097,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> } else if (property == config->writeback_fb_id_property) {
> /* Writeback framebuffer is one-shot, write and forget */
> *val = 0;
> + } else if (property == config->prop_out_fence_ptr) {
> + *val = 0;
> } else if (connector->funcs->atomic_get_property) {
> return connector->funcs->atomic_get_property(connector,
> state, property, val);
> @@ -1736,6 +1747,39 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state,
> return 0;
> }
>
> +static int setup_connector_out_fences(struct drm_atomic_state *state,
> + struct drm_out_fence_state *fence_state,
> + int *fence_idx)
> +{
> + struct drm_connector *conn;
> + struct drm_connector_state *conn_state;
> + int i, ret;
> +
> + for_each_connector_in_state(state, conn, conn_state, i) {
> + struct fence *fence;
> +
> + if (!conn_state->out_fence_ptr)
> + continue;
> +
> + fence = drm_writeback_get_out_fence(conn, conn_state);
> + if (!fence)
> + return -ENOMEM;
> +
> + ret = setup_out_fence(&fence_state[(*fence_idx)++],
> + conn_state->out_fence_ptr,
> + fence);
> + if (ret) {
> + fence_put(fence);
> + return ret;
> + }
> +
> + /* One-time usage only */
> + conn_state->out_fence_ptr = NULL;
> + }
> +
> + return 0;
> +}
> +
> int drm_mode_atomic_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv)
> {
> @@ -1868,8 +1912,8 @@ retry:
> drm_mode_object_unreference(obj);
> }
>
> - fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
> - GFP_KERNEL);
> + fence_state = kcalloc(dev->mode_config.num_crtc + state->num_connector,
> + sizeof(*fence_state), GFP_KERNEL);
> if (!fence_state) {
> ret = -ENOMEM;
> goto out;
> @@ -1929,10 +1973,16 @@ retry:
> * Below we call drm_atomic_state_free for it.
> */
> ret = drm_atomic_check_only(state);
> - } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
> - ret = drm_atomic_nonblocking_commit(state);
> } else {
> - ret = drm_atomic_commit(state);
> + ret = setup_connector_out_fences(state, fence_state,
> + &fence_idx);
> + if (ret)
> + goto out;
I'd do this before this block. At the DRM_MODE_ATOMIC_TEST_ONLY or we
check or we commit. Moreover you should check if out_fences
are not set with TEST_ONLY.
> +
> + if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK)
> + ret = drm_atomic_nonblocking_commit(state);
> + else
> + ret = drm_atomic_commit(state);
> }
>
> if (!ret)
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index bba8672..88da299 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3234,8 +3234,9 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
> if (state->crtc)
> drm_connector_reference(connector);
>
> - /* Don't copy over framebuffers, they are used only once */
> + /* Don't copy over framebuffers or fences, they are used only once */
> state->fb = NULL;
> + state->out_fence = NULL;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>
> @@ -3365,6 +3366,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
> drm_connector_unreference(state->connector);
> if (state->fb)
> drm_framebuffer_unreference(state->fb);
> +
> + fence_put(state->out_fence);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 5a6e0ad..4a6ef30 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -11,6 +11,7 @@
> #include <drm/drm_crtc.h>
> #include <drm/drm_property.h>
> #include <drm/drmP.h>
> +#include <linux/fence.h>
>
> /**
> * DOC: overview
> @@ -32,6 +33,16 @@
> * explicitly cleared in order to make a subsequent commit which doesn't use
> * writeback.
> *
> + * Unlike with planes, when a writeback framebuffer is removed by userspace DRM
> + * makes no attempt to remove it from active use by the connector. This is
> + * because no method is provided to abort a writeback operation, and in any
> + * case making a new commit whilst a writeback is ongoing is undefined (see
> + * OUT_FENCE_PTR below). As soon as the current writeback is finished, the
> + * framebuffer will automatically no longer be in active use. As it will also
> + * have already been removed from the framebuffer list, there will be no way for
> + * any userspace application to retrieve a reference to it in the intervening
> + * period.
> + *
> * Writeback connectors have several additional properties, which userspace
> * can use to query and control them:
> *
> @@ -52,8 +63,48 @@
> * "PIXEL_FORMATS_SIZE":
> * Immutable unsigned range property storing the number of entries in the
> * PIXEL_FORMATS array.
> + *
> + * "OUT_FENCE_PTR":
> + * Userspace can provide the address of a 64-bit integer in this property,
> + * which will be filled with a sync_file file descriptor by the kernel.
> + * The sync_file will signal when the associated writeback has finished.
> + * Userspace should wait for this fence to signal before making another
> + * commit affecting any of the same CRTCs, Planes or Connectors.
> + * **Failure to do so will result in undefined behaviour.**
> + * For this reason it is strongly recommended that all userspace
> + * applications making use of writeback connectors *always* retrieve an
> + * out-fence for the commit and use it appropriately.
> + * From userspace, this property will always read as zero.
> */
>
> +#define fence_to_connector(x) container_of(x->lock, struct drm_connector, fence_lock)
> +
> +static const char *drm_writeback_fence_get_driver_name(struct fence *fence)
> +{
> + struct drm_connector *connector = fence_to_connector(fence);
> +
> + return connector->dev->driver->name;
> +}
> +
> +static const char *drm_writeback_fence_get_timeline_name(struct fence *fence)
> +{
> + struct drm_connector *connector = fence_to_connector(fence);
> +
> + return connector->timeline_name;
> +}
> +
> +static bool drm_writeback_fence_enable_signaling(struct fence *fence)
> +{
> + return true;
> +}
> +
> +static const struct fence_ops drm_writeback_fence_ops = {
> + .get_driver_name = drm_writeback_fence_get_driver_name,
> + .get_timeline_name = drm_writeback_fence_get_timeline_name,
> + .enable_signaling = drm_writeback_fence_enable_signaling,
> + .wait = fence_default_wait,
> +};
> +
> /**
> * create_writeback_properties - Create writeback connector-specific properties
> * @dev: DRM device
> @@ -136,6 +187,14 @@ int drm_writeback_connector_init(struct drm_device *dev,
> if (ret)
> goto fail;
>
> + connector->fence_context = fence_context_alloc(1);
> + spin_lock_init(&connector->fence_lock);
> + snprintf(connector->timeline_name, sizeof(connector->timeline_name),
> + "CONNECTOR:%d", connector->base.id);
> +
> + drm_object_attach_property(&connector->base,
> + config->prop_out_fence_ptr, 0);
> +
> drm_object_attach_property(&connector->base,
> config->writeback_fb_id_property, 0);
>
> @@ -155,3 +214,24 @@ fail:
> return ret;
> }
> EXPORT_SYMBOL(drm_writeback_connector_init);
> +
> +struct fence *drm_writeback_get_out_fence(struct drm_connector *connector,
> + struct drm_connector_state *conn_state)
> +{
> + struct fence *fence;
> +
> + if (WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK))
> + return NULL;
> +
> + fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> + if (!fence)
> + return NULL;
> +
> + fence_init(fence, &drm_writeback_fence_ops, &connector->fence_lock,
> + connector->fence_context, ++connector->fence_seqno);
> +
> + conn_state->out_fence = fence_get(fence);
> +
> + return fence;
> +}
> +EXPORT_SYMBOL(drm_writeback_get_out_fence);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index a5e3778..7d40537 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -199,6 +199,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
> * @best_encoder: can be used by helpers and drivers to select the encoder
> * @state: backpointer to global drm_atomic_state
> * @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK
> + * @out_fence: Fence which will clear when the framebuffer write has finished
> */
> struct drm_connector_state {
> struct drm_connector *connector;
> @@ -216,6 +217,9 @@ struct drm_connector_state {
> struct drm_atomic_state *state;
>
> struct drm_framebuffer *fb; /* do not write directly, use drm_atomic_set_fb_for_connector() */
> +
> + struct fence *out_fence;
> + u64 __user *out_fence_ptr;
> };
>
> /**
> @@ -546,6 +550,10 @@ struct drm_cmdline_mode {
> * @tile_v_loc: vertical location of this tile
> * @tile_h_size: horizontal size of this tile.
> * @tile_v_size: vertical size of this tile.
> + * @fence_context: context for fence signalling
> + * @fence_lock: fence lock for the fence context
> + * @fence_seqno: seqno variable to create fences
> + * @timeline_name: fence timeline name
> *
> * Each connector may be connected to one or more CRTCs, or may be clonable by
> * another connector if they can share a CRTC. Each connector also has a specific
> @@ -694,6 +702,12 @@ struct drm_connector {
> uint8_t num_h_tile, num_v_tile;
> uint8_t tile_h_loc, tile_v_loc;
> uint16_t tile_h_size, tile_v_size;
> +
> + /* fence timelines info for DRM out-fences */
> + unsigned int fence_context;
> + spinlock_t fence_lock;
> + unsigned long fence_seqno;
> + char timeline_name[32];
> };
>
> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index afdc2742..01f33bc 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -16,4 +16,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
> const struct drm_connector_funcs *funcs,
> u32 *formats, int n_formats);
>
> +struct fence *drm_writeback_get_out_fence(struct drm_connector *connector,
> + struct drm_connector_state *conn_state);
> #endif
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
2016-10-26 21:40 ` Gustavo Padovan
@ 2016-10-27 10:07 ` Brian Starkey
0 siblings, 0 replies; 25+ messages in thread
From: Brian Starkey @ 2016-10-27 10:07 UTC (permalink / raw)
To: Gustavo Padovan, dri-devel, linux-kernel, linux-media
Hi Gustavo,
On Wed, Oct 26, 2016 at 07:40:29PM -0200, Gustavo Padovan wrote:
>2016-10-26 Brian Starkey <brian.starkey@arm.com>:
>
>> Add the OUT_FENCE_PTR property to writeback connectors, to enable
>> userspace to get a fence which will signal once the writeback is
>> complete.
>>
>> A timeline is added to drm_connector for use by the writeback
>> out-fences. It is up to drivers to check for a fence in the
>> connector_state and signal the it appropriately when their writeback has
>> finished.
>>
>> It is not allowed to request an out-fence without a framebuffer attached
>> to the connector.
>>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> ---
>> drivers/gpu/drm/drm_atomic.c | 60 +++++++++++++++++++++++---
>> drivers/gpu/drm/drm_atomic_helper.c | 5 ++-
>> drivers/gpu/drm/drm_writeback.c | 80 +++++++++++++++++++++++++++++++++++
>> include/drm/drm_connector.h | 14 ++++++
>> include/drm/drm_writeback.h | 2 +
>> 5 files changed, 155 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 3f8fc97..061ea13 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -30,6 +30,7 @@
>> #include <drm/drm_atomic.h>
>> #include <drm/drm_mode.h>
>> #include <drm/drm_plane_helper.h>
>> +#include <drm/drm_writeback.h>
>> #include <linux/sync_file.h>
>>
>> #include "drm_crtc_internal.h"
>> @@ -646,6 +647,12 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
>> return -EINVAL;
>> }
>>
>> + if (state->out_fence && !state->fb) {
>> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n",
>> + connector->base.id, connector->name);
>> + return -EINVAL;
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -1047,6 +1054,8 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>> drm_atomic_set_fb_for_connector(state, fb);
>> if (fb)
>> drm_framebuffer_unreference(fb);
>> + } else if (property == config->prop_out_fence_ptr) {
>> + state->out_fence_ptr = u64_to_user_ptr(val);
>
>We need to move this out of the conn_state. For my v6 on CRTC out fences
>I added drm_atomic_set_out_fence_for_crtc() and
>drm_atomic_get_out_fence_for_crtc(). See padovan/fences.
>
Yep, will do. I was expecting this to change
>> } else if (connector->funcs->atomic_set_property) {
>> return connector->funcs->atomic_set_property(connector,
>> state, property, val);
>> @@ -1088,6 +1097,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>> } else if (property == config->writeback_fb_id_property) {
>> /* Writeback framebuffer is one-shot, write and forget */
>> *val = 0;
>> + } else if (property == config->prop_out_fence_ptr) {
>> + *val = 0;
>> } else if (connector->funcs->atomic_get_property) {
>> return connector->funcs->atomic_get_property(connector,
>> state, property, val);
>> @@ -1736,6 +1747,39 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state,
>> return 0;
>> }
>>
>> +static int setup_connector_out_fences(struct drm_atomic_state *state,
>> + struct drm_out_fence_state *fence_state,
>> + int *fence_idx)
>> +{
>> + struct drm_connector *conn;
>> + struct drm_connector_state *conn_state;
>> + int i, ret;
>> +
>> + for_each_connector_in_state(state, conn, conn_state, i) {
>> + struct fence *fence;
>> +
>> + if (!conn_state->out_fence_ptr)
>> + continue;
>> +
>> + fence = drm_writeback_get_out_fence(conn, conn_state);
>> + if (!fence)
>> + return -ENOMEM;
>> +
>> + ret = setup_out_fence(&fence_state[(*fence_idx)++],
>> + conn_state->out_fence_ptr,
>> + fence);
>> + if (ret) {
>> + fence_put(fence);
>> + return ret;
>> + }
>> +
>> + /* One-time usage only */
>> + conn_state->out_fence_ptr = NULL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> int drm_mode_atomic_ioctl(struct drm_device *dev,
>> void *data, struct drm_file *file_priv)
>> {
>> @@ -1868,8 +1912,8 @@ retry:
>> drm_mode_object_unreference(obj);
>> }
>>
>> - fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
>> - GFP_KERNEL);
>> + fence_state = kcalloc(dev->mode_config.num_crtc + state->num_connector,
>> + sizeof(*fence_state), GFP_KERNEL);
>> if (!fence_state) {
>> ret = -ENOMEM;
>> goto out;
>> @@ -1929,10 +1973,16 @@ retry:
>> * Below we call drm_atomic_state_free for it.
>> */
>> ret = drm_atomic_check_only(state);
>> - } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>> - ret = drm_atomic_nonblocking_commit(state);
>> } else {
>> - ret = drm_atomic_commit(state);
>> + ret = setup_connector_out_fences(state, fence_state,
>> + &fence_idx);
>> + if (ret)
>> + goto out;
>
>I'd do this before this block. At the DRM_MODE_ATOMIC_TEST_ONLY or we
>check or we commit. Moreover you should check if out_fences
>are not set with TEST_ONLY.
>
I don't quite follow the second sentence; you mean you'd rather I
was finished setting things up before the TEST_ONLY check? That sounds
OK.
For TEST_ONLY I actually think we shouldn't reject commits with
OUT_FENCE_PTRs and TEST_ONLY. I thought the idea of TEST_ONLY was for
userspace to build a commit, then see if it can work, then commit it
for real.
By rejecting TEST_ONLY + OUT_FENCE_PTRs you're saying userspace can
never test what it actually intends to commit. It has to test, then
add the out-fences, then commit and hope for the best.
How about always setting the out-fences to -1 for TEST_ONLY?
Thanks for the review,
-Brian
>> +
>> + if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK)
>> + ret = drm_atomic_nonblocking_commit(state);
>> + else
>> + ret = drm_atomic_commit(state);
>> }
>>
>> if (!ret)
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index bba8672..88da299 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3234,8 +3234,9 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>> if (state->crtc)
>> drm_connector_reference(connector);
>>
>> - /* Don't copy over framebuffers, they are used only once */
>> + /* Don't copy over framebuffers or fences, they are used only once */
>> state->fb = NULL;
>> + state->out_fence = NULL;
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>>
>> @@ -3365,6 +3366,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
>> drm_connector_unreference(state->connector);
>> if (state->fb)
>> drm_framebuffer_unreference(state->fb);
>> +
>> + fence_put(state->out_fence);
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>>
>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>> index 5a6e0ad..4a6ef30 100644
>> --- a/drivers/gpu/drm/drm_writeback.c
>> +++ b/drivers/gpu/drm/drm_writeback.c
>> @@ -11,6 +11,7 @@
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_property.h>
>> #include <drm/drmP.h>
>> +#include <linux/fence.h>
>>
>> /**
>> * DOC: overview
>> @@ -32,6 +33,16 @@
>> * explicitly cleared in order to make a subsequent commit which doesn't use
>> * writeback.
>> *
>> + * Unlike with planes, when a writeback framebuffer is removed by userspace DRM
>> + * makes no attempt to remove it from active use by the connector. This is
>> + * because no method is provided to abort a writeback operation, and in any
>> + * case making a new commit whilst a writeback is ongoing is undefined (see
>> + * OUT_FENCE_PTR below). As soon as the current writeback is finished, the
>> + * framebuffer will automatically no longer be in active use. As it will also
>> + * have already been removed from the framebuffer list, there will be no way for
>> + * any userspace application to retrieve a reference to it in the intervening
>> + * period.
>> + *
>> * Writeback connectors have several additional properties, which userspace
>> * can use to query and control them:
>> *
>> @@ -52,8 +63,48 @@
>> * "PIXEL_FORMATS_SIZE":
>> * Immutable unsigned range property storing the number of entries in the
>> * PIXEL_FORMATS array.
>> + *
>> + * "OUT_FENCE_PTR":
>> + * Userspace can provide the address of a 64-bit integer in this property,
>> + * which will be filled with a sync_file file descriptor by the kernel.
>> + * The sync_file will signal when the associated writeback has finished.
>> + * Userspace should wait for this fence to signal before making another
>> + * commit affecting any of the same CRTCs, Planes or Connectors.
>> + * **Failure to do so will result in undefined behaviour.**
>> + * For this reason it is strongly recommended that all userspace
>> + * applications making use of writeback connectors *always* retrieve an
>> + * out-fence for the commit and use it appropriately.
>> + * From userspace, this property will always read as zero.
>> */
>>
>> +#define fence_to_connector(x) container_of(x->lock, struct drm_connector, fence_lock)
>> +
>> +static const char *drm_writeback_fence_get_driver_name(struct fence *fence)
>> +{
>> + struct drm_connector *connector = fence_to_connector(fence);
>> +
>> + return connector->dev->driver->name;
>> +}
>> +
>> +static const char *drm_writeback_fence_get_timeline_name(struct fence *fence)
>> +{
>> + struct drm_connector *connector = fence_to_connector(fence);
>> +
>> + return connector->timeline_name;
>> +}
>> +
>> +static bool drm_writeback_fence_enable_signaling(struct fence *fence)
>> +{
>> + return true;
>> +}
>> +
>> +static const struct fence_ops drm_writeback_fence_ops = {
>> + .get_driver_name = drm_writeback_fence_get_driver_name,
>> + .get_timeline_name = drm_writeback_fence_get_timeline_name,
>> + .enable_signaling = drm_writeback_fence_enable_signaling,
>> + .wait = fence_default_wait,
>> +};
>> +
>> /**
>> * create_writeback_properties - Create writeback connector-specific properties
>> * @dev: DRM device
>> @@ -136,6 +187,14 @@ int drm_writeback_connector_init(struct drm_device *dev,
>> if (ret)
>> goto fail;
>>
>> + connector->fence_context = fence_context_alloc(1);
>> + spin_lock_init(&connector->fence_lock);
>> + snprintf(connector->timeline_name, sizeof(connector->timeline_name),
>> + "CONNECTOR:%d", connector->base.id);
>> +
>> + drm_object_attach_property(&connector->base,
>> + config->prop_out_fence_ptr, 0);
>> +
>> drm_object_attach_property(&connector->base,
>> config->writeback_fb_id_property, 0);
>>
>> @@ -155,3 +214,24 @@ fail:
>> return ret;
>> }
>> EXPORT_SYMBOL(drm_writeback_connector_init);
>> +
>> +struct fence *drm_writeback_get_out_fence(struct drm_connector *connector,
>> + struct drm_connector_state *conn_state)
>> +{
>> + struct fence *fence;
>> +
>> + if (WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK))
>> + return NULL;
>> +
>> + fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>> + if (!fence)
>> + return NULL;
>> +
>> + fence_init(fence, &drm_writeback_fence_ops, &connector->fence_lock,
>> + connector->fence_context, ++connector->fence_seqno);
>> +
>> + conn_state->out_fence = fence_get(fence);
>> +
>> + return fence;
>> +}
>> +EXPORT_SYMBOL(drm_writeback_get_out_fence);
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index a5e3778..7d40537 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -199,6 +199,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
>> * @best_encoder: can be used by helpers and drivers to select the encoder
>> * @state: backpointer to global drm_atomic_state
>> * @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK
>> + * @out_fence: Fence which will clear when the framebuffer write has finished
>> */
>> struct drm_connector_state {
>> struct drm_connector *connector;
>> @@ -216,6 +217,9 @@ struct drm_connector_state {
>> struct drm_atomic_state *state;
>>
>> struct drm_framebuffer *fb; /* do not write directly, use drm_atomic_set_fb_for_connector() */
>> +
>> + struct fence *out_fence;
>> + u64 __user *out_fence_ptr;
>> };
>>
>> /**
>> @@ -546,6 +550,10 @@ struct drm_cmdline_mode {
>> * @tile_v_loc: vertical location of this tile
>> * @tile_h_size: horizontal size of this tile.
>> * @tile_v_size: vertical size of this tile.
>> + * @fence_context: context for fence signalling
>> + * @fence_lock: fence lock for the fence context
>> + * @fence_seqno: seqno variable to create fences
>> + * @timeline_name: fence timeline name
>> *
>> * Each connector may be connected to one or more CRTCs, or may be clonable by
>> * another connector if they can share a CRTC. Each connector also has a specific
>> @@ -694,6 +702,12 @@ struct drm_connector {
>> uint8_t num_h_tile, num_v_tile;
>> uint8_t tile_h_loc, tile_v_loc;
>> uint16_t tile_h_size, tile_v_size;
>> +
>> + /* fence timelines info for DRM out-fences */
>> + unsigned int fence_context;
>> + spinlock_t fence_lock;
>> + unsigned long fence_seqno;
>> + char timeline_name[32];
>> };
>>
>> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
>> index afdc2742..01f33bc 100644
>> --- a/include/drm/drm_writeback.h
>> +++ b/include/drm/drm_writeback.h
>> @@ -16,4 +16,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
>> const struct drm_connector_funcs *funcs,
>> u32 *formats, int n_formats);
>>
>> +struct fence *drm_writeback_get_out_fence(struct drm_connector *connector,
>> + struct drm_connector_state *conn_state);
>> #endif
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH v2 9/9] drm: mali-dp: Add writeback out-fence support
2016-10-26 8:54 [RFC PATCH v2 0/9] Introduce writeback connectors Brian Starkey
` (7 preceding siblings ...)
2016-10-26 8:55 ` [RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors Brian Starkey
@ 2016-10-26 8:55 ` Brian Starkey
2016-10-26 21:43 ` Gustavo Padovan
8 siblings, 1 reply; 25+ messages in thread
From: Brian Starkey @ 2016-10-26 8:55 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, linux-media
If userspace has asked for an out-fence for the writeback, we add a
fence to malidp_mw_job, to be signaled when the writeback job has
completed.
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
drivers/gpu/drm/arm/malidp_hw.c | 5 ++++-
drivers/gpu/drm/arm/malidp_mw.c | 18 +++++++++++++++++-
drivers/gpu/drm/arm/malidp_mw.h | 3 +++
3 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 1689547..3032226 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -707,8 +707,11 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
unsigned long irqflags;
/*
* We can't unreference the framebuffer here, so we queue it
- * up on our threaded handler.
+ * up on our threaded handler. However, signal the fence
+ * as soon as possible
*/
+ malidp_mw_job_signal(drm, malidp->current_mw, 0);
+
spin_lock_irqsave(&malidp->mw_lock, irqflags);
list_add_tail(&malidp->current_mw->list,
&malidp->finished_mw_jobs);
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index 69e423c..05880b1 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -34,11 +34,24 @@ struct malidp_mw_connector_state {
u8 n_planes;
};
+void malidp_mw_job_signal(struct drm_device *drm,
+ struct malidp_mw_job *job, int status)
+{
+ DRM_DEV_DEBUG_DRIVER(drm->dev, "MW job signal %p\n", job);
+
+ if (!job->fence)
+ return;
+
+ job->fence->status = status;
+ fence_signal(job->fence);
+}
+
void malidp_mw_job_cleanup(struct drm_device *drm,
struct malidp_mw_job *job)
{
DRM_DEV_DEBUG_DRIVER(drm->dev, "MW job cleanup %p\n", job);
drm_framebuffer_unreference(job->fb);
+ fence_put(job->fence);
kfree(job);
}
@@ -107,8 +120,10 @@ static void malidp_mw_connector_destroy_state(struct drm_connector *connector,
struct malidp_mw_connector_state *mw_state = to_mw_state(state);
__drm_atomic_helper_connector_destroy_state(&mw_state->base);
- if (mw_state->job)
+ if (mw_state->job) {
+ malidp_mw_job_signal(connector->dev, mw_state->job, -EPIPE);
malidp_mw_job_cleanup(connector->dev, mw_state->job);
+ }
kfree(mw_state);
}
@@ -177,6 +192,7 @@ malidp_mw_encoder_atomic_check(struct drm_encoder *encoder,
/* We can take ownership of the framebuffer reference in the job. */
mw_state->job->fb = conn_state->fb;
conn_state->fb = NULL;
+ mw_state->job->fence = fence_get(conn_state->out_fence);
return 0;
}
diff --git a/drivers/gpu/drm/arm/malidp_mw.h b/drivers/gpu/drm/arm/malidp_mw.h
index db7b2b0..5d0bd1d 100644
--- a/drivers/gpu/drm/arm/malidp_mw.h
+++ b/drivers/gpu/drm/arm/malidp_mw.h
@@ -15,11 +15,14 @@
struct malidp_mw_job {
struct list_head list;
struct drm_framebuffer *fb;
+ struct fence *fence;
};
int malidp_mw_connector_init(struct drm_device *drm);
void malidp_mw_atomic_commit(struct drm_device *drm,
struct drm_atomic_state *old_state);
+void malidp_mw_job_signal(struct drm_device *drm,
+ struct malidp_mw_job *job, int status);
void malidp_mw_job_cleanup(struct drm_device *drm,
struct malidp_mw_job *job);
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [RFC PATCH v2 9/9] drm: mali-dp: Add writeback out-fence support
2016-10-26 8:55 ` [RFC PATCH v2 9/9] drm: mali-dp: Add writeback out-fence support Brian Starkey
@ 2016-10-26 21:43 ` Gustavo Padovan
2016-10-27 10:18 ` Brian Starkey
0 siblings, 1 reply; 25+ messages in thread
From: Gustavo Padovan @ 2016-10-26 21:43 UTC (permalink / raw)
To: Brian Starkey; +Cc: dri-devel, linux-kernel, linux-media
2016-10-26 Brian Starkey <brian.starkey@arm.com>:
> If userspace has asked for an out-fence for the writeback, we add a
> fence to malidp_mw_job, to be signaled when the writeback job has
> completed.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
> drivers/gpu/drm/arm/malidp_hw.c | 5 ++++-
> drivers/gpu/drm/arm/malidp_mw.c | 18 +++++++++++++++++-
> drivers/gpu/drm/arm/malidp_mw.h | 3 +++
> 3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> index 1689547..3032226 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.c
> +++ b/drivers/gpu/drm/arm/malidp_hw.c
> @@ -707,8 +707,11 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
> unsigned long irqflags;
> /*
> * We can't unreference the framebuffer here, so we queue it
> - * up on our threaded handler.
> + * up on our threaded handler. However, signal the fence
> + * as soon as possible
> */
> + malidp_mw_job_signal(drm, malidp->current_mw, 0);
Drivers should not deal with fences directly. We need some sort of
drm_writeback_finished() that will do the signalling for you.
Gustavo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC PATCH v2 9/9] drm: mali-dp: Add writeback out-fence support
2016-10-26 21:43 ` Gustavo Padovan
@ 2016-10-27 10:18 ` Brian Starkey
2016-10-27 11:25 ` Gustavo Padovan
0 siblings, 1 reply; 25+ messages in thread
From: Brian Starkey @ 2016-10-27 10:18 UTC (permalink / raw)
To: Gustavo Padovan, dri-devel, linux-kernel, linux-media
On Wed, Oct 26, 2016 at 07:43:57PM -0200, Gustavo Padovan wrote:
>2016-10-26 Brian Starkey <brian.starkey@arm.com>:
>
>> If userspace has asked for an out-fence for the writeback, we add a
>> fence to malidp_mw_job, to be signaled when the writeback job has
>> completed.
>>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> ---
>> drivers/gpu/drm/arm/malidp_hw.c | 5 ++++-
>> drivers/gpu/drm/arm/malidp_mw.c | 18 +++++++++++++++++-
>> drivers/gpu/drm/arm/malidp_mw.h | 3 +++
>> 3 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
>> index 1689547..3032226 100644
>> --- a/drivers/gpu/drm/arm/malidp_hw.c
>> +++ b/drivers/gpu/drm/arm/malidp_hw.c
>> @@ -707,8 +707,11 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
>> unsigned long irqflags;
>> /*
>> * We can't unreference the framebuffer here, so we queue it
>> - * up on our threaded handler.
>> + * up on our threaded handler. However, signal the fence
>> + * as soon as possible
>> */
>> + malidp_mw_job_signal(drm, malidp->current_mw, 0);
>
>Drivers should not deal with fences directly. We need some sort of
>drm_writeback_finished() that will do the signalling for you.
>
With a signature like this?
drm_writeback_finished(struct drm_connector_state *state);
I'll have to think about how to achieve that. The state isn't
refcounted and the driver isn't in charge of it's lifetime. I'm not
sure how/where to ensure the state doesn't get destroyed before its
been signaled.
-Brian
>Gustavo
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC PATCH v2 9/9] drm: mali-dp: Add writeback out-fence support
2016-10-27 10:18 ` Brian Starkey
@ 2016-10-27 11:25 ` Gustavo Padovan
2016-10-27 14:11 ` Daniel Vetter
0 siblings, 1 reply; 25+ messages in thread
From: Gustavo Padovan @ 2016-10-27 11:25 UTC (permalink / raw)
To: Brian Starkey; +Cc: dri-devel, linux-kernel, linux-media
2016-10-27 Brian Starkey <brian.starkey@arm.com>:
> On Wed, Oct 26, 2016 at 07:43:57PM -0200, Gustavo Padovan wrote:
> > 2016-10-26 Brian Starkey <brian.starkey@arm.com>:
> >
> > > If userspace has asked for an out-fence for the writeback, we add a
> > > fence to malidp_mw_job, to be signaled when the writeback job has
> > > completed.
> > >
> > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > > ---
> > > drivers/gpu/drm/arm/malidp_hw.c | 5 ++++-
> > > drivers/gpu/drm/arm/malidp_mw.c | 18 +++++++++++++++++-
> > > drivers/gpu/drm/arm/malidp_mw.h | 3 +++
> > > 3 files changed, 24 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> > > index 1689547..3032226 100644
> > > --- a/drivers/gpu/drm/arm/malidp_hw.c
> > > +++ b/drivers/gpu/drm/arm/malidp_hw.c
> > > @@ -707,8 +707,11 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
> > > unsigned long irqflags;
> > > /*
> > > * We can't unreference the framebuffer here, so we queue it
> > > - * up on our threaded handler.
> > > + * up on our threaded handler. However, signal the fence
> > > + * as soon as possible
> > > */
> > > + malidp_mw_job_signal(drm, malidp->current_mw, 0);
> >
> > Drivers should not deal with fences directly. We need some sort of
> > drm_writeback_finished() that will do the signalling for you.
> >
>
> With a signature like this?
> drm_writeback_finished(struct drm_connector_state *state);
>
> I'll have to think about how to achieve that. The state isn't
> refcounted and the driver isn't in charge of it's lifetime. I'm not
> sure how/where to ensure the state doesn't get destroyed before its
> been signaled.
Maybe we should do something similar to the crtc vblank handlers and
even hide the connector_state. Those helpers only take the crtc.
They are able to hold ref to the event as well.
Gustavo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC PATCH v2 9/9] drm: mali-dp: Add writeback out-fence support
2016-10-27 11:25 ` Gustavo Padovan
@ 2016-10-27 14:11 ` Daniel Vetter
0 siblings, 0 replies; 25+ messages in thread
From: Daniel Vetter @ 2016-10-27 14:11 UTC (permalink / raw)
To: Gustavo Padovan, Brian Starkey, dri-devel, linux-kernel, linux-media
On Thu, Oct 27, 2016 at 09:25:19AM -0200, Gustavo Padovan wrote:
> 2016-10-27 Brian Starkey <brian.starkey@arm.com>:
>
> > On Wed, Oct 26, 2016 at 07:43:57PM -0200, Gustavo Padovan wrote:
> > > 2016-10-26 Brian Starkey <brian.starkey@arm.com>:
> > >
> > > > If userspace has asked for an out-fence for the writeback, we add a
> > > > fence to malidp_mw_job, to be signaled when the writeback job has
> > > > completed.
> > > >
> > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > > > ---
> > > > drivers/gpu/drm/arm/malidp_hw.c | 5 ++++-
> > > > drivers/gpu/drm/arm/malidp_mw.c | 18 +++++++++++++++++-
> > > > drivers/gpu/drm/arm/malidp_mw.h | 3 +++
> > > > 3 files changed, 24 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> > > > index 1689547..3032226 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_hw.c
> > > > +++ b/drivers/gpu/drm/arm/malidp_hw.c
> > > > @@ -707,8 +707,11 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
> > > > unsigned long irqflags;
> > > > /*
> > > > * We can't unreference the framebuffer here, so we queue it
> > > > - * up on our threaded handler.
> > > > + * up on our threaded handler. However, signal the fence
> > > > + * as soon as possible
> > > > */
> > > > + malidp_mw_job_signal(drm, malidp->current_mw, 0);
> > >
> > > Drivers should not deal with fences directly. We need some sort of
> > > drm_writeback_finished() that will do the signalling for you.
> > >
> >
> > With a signature like this?
> > drm_writeback_finished(struct drm_connector_state *state);
> >
> > I'll have to think about how to achieve that. The state isn't
> > refcounted and the driver isn't in charge of it's lifetime. I'm not
> > sure how/where to ensure the state doesn't get destroyed before its
> > been signaled.
>
> Maybe we should do something similar to the crtc vblank handlers and
> even hide the connector_state. Those helpers only take the crtc.
> They are able to hold ref to the event as well.
I guess we could reuse the drm_event stuff, but not sure that's too much
overkill. It would at least be a consistent driver interface, and drivers
could reuse stuff like arm_event and similar functions. Which might be
rather beneficial.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 25+ messages in thread