High resoultion modes (4K/5K) are supported by encoder/crtc hardware, but the backing hardware planes to scanout these buffers generally don't support such large widths. Two (or more) hardware planes are used in conjunction to scan out the entire width.
One way to support this is to virtualize the drm_plane objects that we expose to userspace, and let the kms driver internally figure out whether it needs to club multiple hardware pipes to run a particular mode.
The other option is to represent a drm_plane as it is in hardware, and leave it to userspace use to manage multiple drm_planes for scanning out these modes.
The advantage of the latter option is that the kms driver doesn't get too complicated. It doesn't need to do the decision making on how to distibute the hardware resources among the drm_planes. It also allows userspace to continue doing some of the heavy lifting (figure out the most power efficient combination of pipes, estimate bandwidth consumption etc) which would have to be moved to the kernel if we virtualized planes.
In order to achieve the latter, a drm_plane should expose an immutable property to userspace describing the maximum width and height it can support. Userspace can then prepare the drm_planes accordingly and commit the state. This patch introduces these properties.
The main disadvantage is that legacy userspace (i.e. calling SetCrtc on a high resolution framebuffer) wouldn't work. This, on the other hand, wouldn't be a problem if we virtualized the planes and manipulated the hardware pipes in the kenrel. One way around this is to fail large resolution modes when using legacy setcrtc, or introduce some minimal helpers in the kernel that automatically use multiple drm_planes when we try to set such a mode. A solution to this issue isn't a part of the RFC yet.
We're looking for feedback here and wondering whether this is the right way to go or not.
Signed-off-by: Archit Taneja architt@codeaurora.org --- drivers/gpu/drm/drm_atomic.c | 9 +++++++++ drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 3 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8ee1db8..fded22a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ERANGE; }
+ if (plane->max_width && (state->src_w >> 16) > plane->max_width || + plane->max_height && (state->src_h >> 16) > plane->max_height) { + DRM_DEBUG_ATOMIC("Invalid source width/height " + "%u.%06ux%u.%06u\n", + state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10, + state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10); + return -ERANGE; + } + fb_width = state->fb->width << 16; fb_height = state->fb->height << 16;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e08f962..f2d3b92 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->possible_crtcs = possible_crtcs; plane->type = type;
+ plane->max_width = 0; + plane->max_height = 0; + list_add_tail(&plane->head, &config->plane_list); config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY) @@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, } EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width) +{ + struct drm_property *prop; + + prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE, + "MAX_W", max_width, max_width); + if (!prop) + return -ENOMEM; + + drm_object_attach_property(&plane->base, prop, max_width); + + plane->max_width = max_width; + + return 0; +} +EXPORT_SYMBOL(drm_plane_create_max_width_prop); + +int drm_plane_create_max_height_prop(struct drm_plane *plane, + uint32_t max_height) +{ + struct drm_property *prop; + + prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE, + "MAX_H", max_height, max_height); + if (!prop) + return -ENOMEM; + + drm_object_attach_property(&plane->base, prop, max_height); + + plane->max_height = max_height; + + return 0; +} +EXPORT_SYMBOL(drm_plane_create_max_height_prop); + /** * drm_mode_obj_get_properties_ioctl - get the current value of a object's property * @dev: DRM device diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e0170bf..6104527 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1531,6 +1531,8 @@ struct drm_plane { uint32_t *format_types; unsigned int format_count; bool format_default; + uint32_t max_width; + uint32_t max_height;
struct drm_crtc *crtc; struct drm_framebuffer *fb; @@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device unsigned int supported_rotations); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations); +extern int drm_plane_create_max_width_prop(struct drm_plane *plane, + uint32_t max_width); +extern int drm_plane_create_max_height_prop(struct drm_plane *plane, + uint32_t max_height);
/* Helpers */
On Wed, May 25, 2016 at 12:03:39PM +0530, Archit Taneja wrote:
High resoultion modes (4K/5K) are supported by encoder/crtc hardware, but the backing hardware planes to scanout these buffers generally don't support such large widths. Two (or more) hardware planes are used in conjunction to scan out the entire width.
"generally" seems to be a bit an overstatement. Most sensible hw has no problem at all scanning that kind of stuff out ;-)
One way to support this is to virtualize the drm_plane objects that we expose to userspace, and let the kms driver internally figure out whether it needs to club multiple hardware pipes to run a particular mode.
The other option is to represent a drm_plane as it is in hardware, and leave it to userspace use to manage multiple drm_planes for scanning out these modes.
The advantage of the latter option is that the kms driver doesn't get too complicated. It doesn't need to do the decision making on how to distibute the hardware resources among the drm_planes. It also allows userspace to continue doing some of the heavy lifting (figure out the most power efficient combination of pipes, estimate bandwidth consumption etc) which would have to be moved to the kernel if we virtualized planes.
In order to achieve the latter, a drm_plane should expose an immutable property to userspace describing the maximum width and height it can support. Userspace can then prepare the drm_planes accordingly and commit the state. This patch introduces these properties.
The main disadvantage is that legacy userspace (i.e. calling SetCrtc on a high resolution framebuffer) wouldn't work. This, on the other hand, wouldn't be a problem if we virtualized the planes and manipulated the hardware pipes in the kenrel. One way around this is to fail large resolution modes when using legacy setcrtc, or introduce some minimal helpers in the kernel that automatically use multiple drm_planes when we try to set such a mode. A solution to this issue isn't a part of the RFC yet.
We're looking for feedback here and wondering whether this is the right way to go or not.
So just this week there was a bit a discussion on irc (and I haven't gotten around to typing the writeup and asking everyone for feedback on it) on new properties. Super-short summary:
- new properties are new abi like anything else
- drm wants a userspace demonstration vehicle to prove new abi which is: open source, patches reviewed & tested by respective canonical upstream (so not a vendor fork or something like that). Testcases are explicitly not enough, it needs to be the real deal.
- for props specifically which are meant to be used/interpreted by compositor this means a patch for drm_hwcomposer, one of the many wayland compositors there are (weston, ozone, mutter), or a patch for xfree86-video-modesetting. Other userspace thingies are probably a bit too much fringe to count.
- this isn't really new, but thus far arm simply sailed in the shadow of intel doing all that hard work. Now some arm drivers start to pull ahead, adding new ABI that's not yet proven by i915 folks, and with success comes a bit more responsibility.
For the interface itself just a few questions: - We should make the cursor size stuff obselete by this, and instead first look up the size limits of the cursor plane, before checking those legacy limits. - Is the size/width really independent of e.g. rotation/pixel format/... Should it be the maximum that's possible under best circumstance (things could still fail), or the minimum that's guaranteed to work everwhere. This is the kind of stuff we need the userspace part for, too.
Cheers, Daniel
Signed-off-by: Archit Taneja architt@codeaurora.org
drivers/gpu/drm/drm_atomic.c | 9 +++++++++ drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 3 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8ee1db8..fded22a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ERANGE; }
- if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
plane->max_height && (state->src_h >> 16) > plane->max_height) {DRM_DEBUG_ATOMIC("Invalid source width/height ""%u.%06ux%u.%06u\n",state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);return -ERANGE;- }
- fb_width = state->fb->width << 16; fb_height = state->fb->height << 16;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e08f962..f2d3b92 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->possible_crtcs = possible_crtcs; plane->type = type;
- plane->max_width = 0;
- plane->max_height = 0;
- list_add_tail(&plane->head, &config->plane_list); config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY)
@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, } EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width) +{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_W", max_width, max_width);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_width);
- plane->max_width = max_width;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
+int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height)+{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_H", max_height, max_height);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_height);
- plane->max_height = max_height;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
/**
- drm_mode_obj_get_properties_ioctl - get the current value of a object's property
- @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e0170bf..6104527 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1531,6 +1531,8 @@ struct drm_plane { uint32_t *format_types; unsigned int format_count; bool format_default;
uint32_t max_width;
uint32_t max_height;
struct drm_crtc *crtc; struct drm_framebuffer *fb;
@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device unsigned int supported_rotations); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations); +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
uint32_t max_width);+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height);/* Helpers */
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
On 05/25/2016 12:40 PM, Daniel Vetter wrote:
On Wed, May 25, 2016 at 12:03:39PM +0530, Archit Taneja wrote:
High resoultion modes (4K/5K) are supported by encoder/crtc hardware, but the backing hardware planes to scanout these buffers generally don't support such large widths. Two (or more) hardware planes are used in conjunction to scan out the entire width.
"generally" seems to be a bit an overstatement. Most sensible hw has no problem at all scanning that kind of stuff out ;-)
One way to support this is to virtualize the drm_plane objects that we expose to userspace, and let the kms driver internally figure out whether it needs to club multiple hardware pipes to run a particular mode.
The other option is to represent a drm_plane as it is in hardware, and leave it to userspace use to manage multiple drm_planes for scanning out these modes.
The advantage of the latter option is that the kms driver doesn't get too complicated. It doesn't need to do the decision making on how to distibute the hardware resources among the drm_planes. It also allows userspace to continue doing some of the heavy lifting (figure out the most power efficient combination of pipes, estimate bandwidth consumption etc) which would have to be moved to the kernel if we virtualized planes.
In order to achieve the latter, a drm_plane should expose an immutable property to userspace describing the maximum width and height it can support. Userspace can then prepare the drm_planes accordingly and commit the state. This patch introduces these properties.
The main disadvantage is that legacy userspace (i.e. calling SetCrtc on a high resolution framebuffer) wouldn't work. This, on the other hand, wouldn't be a problem if we virtualized the planes and manipulated the hardware pipes in the kenrel. One way around this is to fail large resolution modes when using legacy setcrtc, or introduce some minimal helpers in the kernel that automatically use multiple drm_planes when we try to set such a mode. A solution to this issue isn't a part of the RFC yet.
We're looking for feedback here and wondering whether this is the right way to go or not.
So just this week there was a bit a discussion on irc (and I haven't gotten around to typing the writeup and asking everyone for feedback on it) on new properties. Super-short summary:
new properties are new abi like anything else
drm wants a userspace demonstration vehicle to prove new abi which is: open source, patches reviewed & tested by respective canonical upstream (so not a vendor fork or something like that). Testcases are explicitly not enough, it needs to be the real deal.
for props specifically which are meant to be used/interpreted by compositor this means a patch for drm_hwcomposer, one of the many wayland compositors there are (weston, ozone, mutter), or a patch for xfree86-video-modesetting. Other userspace thingies are probably a bit too much fringe to count.
this isn't really new, but thus far arm simply sailed in the shadow of intel doing all that hard work. Now some arm drivers start to pull ahead, adding new ABI that's not yet proven by i915 folks, and with success comes a bit more responsibility.
That sounds fair enough.
For the interface itself just a few questions:
- We should make the cursor size stuff obselete by this, and instead first look up the size limits of the cursor plane, before checking those legacy limits.
Yeah, I guess querying the DRM_CAP_CURSOR_WIDTH/HEIGHT caps would become obsolete.
- Is the size/width really independent of e.g. rotation/pixel format/... Should it be the maximum that's possible under best circumstance (things could still fail), or the minimum that's guaranteed to work everwhere. This is the kind of stuff we need the userspace part for, too.
Yeah, it isn't independent of these parameters. I'm not entirely sure about this either.
Does it make sense to impose a rule that the user first sets the rotation/format plane properties, and only then read the maximum width? I'm assuming user space hates such kind of stuff.
If we use the 'best circumstance' max_width, we can first start with a minimum number of planes that need to be grouped to achieve the target mode. If that fails the atomic test, then we can try to add one plane at a time, and reduce the width for each plane.
If we use the minimum/'guaranteed to work' max_width, we'll get a higher number of planes than needed for this mode. This would pass the atomic test. We could then reduce a plane at a time and see when we fail the atomic test.
I guess we need to chose the one that's more probable to get right the first time. Considering only pixel formats for now, the minimum/'guaranteed to work' would map to the RGB formats. The best circumstance ones would probably be the planar video formats. Since we use RGB more often, the minimum one might make more sense.
We could, of course, give the property a range of max widths to confuse user space even more.
Archit
Cheers, Daniel
Signed-off-by: Archit Taneja architt@codeaurora.org
drivers/gpu/drm/drm_atomic.c | 9 +++++++++ drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 3 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8ee1db8..fded22a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ERANGE; }
- if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
plane->max_height && (state->src_h >> 16) > plane->max_height) {DRM_DEBUG_ATOMIC("Invalid source width/height ""%u.%06ux%u.%06u\n",state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);return -ERANGE;- }
- fb_width = state->fb->width << 16; fb_height = state->fb->height << 16;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e08f962..f2d3b92 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->possible_crtcs = possible_crtcs; plane->type = type;
- plane->max_width = 0;
- plane->max_height = 0;
- list_add_tail(&plane->head, &config->plane_list); config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY)
@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, } EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width) +{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_W", max_width, max_width);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_width);
- plane->max_width = max_width;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
+int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height)+{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_H", max_height, max_height);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_height);
- plane->max_height = max_height;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
- /**
- drm_mode_obj_get_properties_ioctl - get the current value of a object's property
- @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e0170bf..6104527 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1531,6 +1531,8 @@ struct drm_plane { uint32_t *format_types; unsigned int format_count; bool format_default;
uint32_t max_width;
uint32_t max_height;
struct drm_crtc *crtc; struct drm_framebuffer *fb;
@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device unsigned int supported_rotations); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations); +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
uint32_t max_width);+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height);/* Helpers */
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
On 05/25/2016 12:40 PM, Daniel Vetter wrote:
- Is the size/width really independent of e.g. rotation/pixel format/... Should it be the maximum that's possible under best circumstance (things could still fail), or the minimum that's guaranteed to work everwhere. This is the kind of stuff we need the userspace part for, too.
Yeah, it isn't independent of these parameters. I'm not entirely sure about this either.
Does it make sense to impose a rule that the user first sets the rotation/format plane properties, and only then read the maximum width? I'm assuming user space hates such kind of stuff.
If we use the 'best circumstance' max_width, we can first start with a minimum number of planes that need to be grouped to achieve the target mode. If that fails the atomic test, then we can try to add one plane at a time, and reduce the width for each plane.
If we use the minimum/'guaranteed to work' max_width, we'll get a higher number of planes than needed for this mode. This would pass the atomic test. We could then reduce a plane at a time and see when we fail the atomic test.
I guess we need to chose the one that's more probable to get right the first time. Considering only pixel formats for now, the minimum/'guaranteed to work' would map to the RGB formats. The best circumstance ones would probably be the planar video formats. Since we use RGB more often, the minimum one might make more sense.
We could, of course, give the property a range of max widths to confuse user space even more.
An entirely different idea for cases where a simple hint property doesn't work (other ideas floating around are can_scale, to give a hint whether a plan can at least in theory up/downscale, or not at all), is that the kernel gives more specific hints about what it would like to change.
So if userspace asks for a plane, but for the given pixel format it's too wide, the kernel could return a new proposed value for width. That would be super-flexible and could cover all kinds of use-case like rotation needing a specific tiling (fb_modifier) or specific pixel format, or specific stride.
For the case at hand there's even more worms: What about stride requirements? Afaik on some hw you just need to split the buffers into 2 planes, but can keep the wide stride (since the limit is the size of the linebuffers in the hw). On others you need to split the buffer itself into 2, because the plane hw can't cope with huge strides. Again might depend upon the pixel format.
So in a way height/width is both too much information and not precise enough. Entirely different approches: - We just add a might_need_split_plane prop to crtcs where this might be needed. Userspace then gets to retry with split buffers if it doesn't work with a huge one.
- When I discussed this with qualcom folks for msm we concluded that the simplest approach would be to hide this in the kernel. So if you have a too wide plane, and need 2 hw planes to scan it out, then do that transparently in the kernel. Of course this means that there will be 1 (or 3 if you need a 2x2 split) fewer planes available, but userspace needs to iteratively build up the plane config using ATOMIC_TEST anyway.
If possible for your hw I'm heavily leaning towards this last approach. If fits entirely into the current atomic design, and all the complexity is restricted to your driver (you need to have some allocation map between drm planes and real hw planes, but that's it).
Thoughts? -Daniel
Archit
Cheers, Daniel
Signed-off-by: Archit Taneja architt@codeaurora.org
drivers/gpu/drm/drm_atomic.c | 9 +++++++++ drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 3 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8ee1db8..fded22a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ERANGE; }
- if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
plane->max_height && (state->src_h >> 16) > plane->max_height) {DRM_DEBUG_ATOMIC("Invalid source width/height ""%u.%06ux%u.%06u\n",state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);return -ERANGE;- }
- fb_width = state->fb->width << 16; fb_height = state->fb->height << 16;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e08f962..f2d3b92 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->possible_crtcs = possible_crtcs; plane->type = type;
- plane->max_width = 0;
- plane->max_height = 0;
- list_add_tail(&plane->head, &config->plane_list); config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY)
@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, } EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width) +{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_W", max_width, max_width);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_width);
- plane->max_width = max_width;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
+int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height)+{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_H", max_height, max_height);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_height);
- plane->max_height = max_height;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
/**
- drm_mode_obj_get_properties_ioctl - get the current value of a object's property
- @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e0170bf..6104527 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1531,6 +1531,8 @@ struct drm_plane { uint32_t *format_types; unsigned int format_count; bool format_default;
uint32_t max_width;
uint32_t max_height;
struct drm_crtc *crtc; struct drm_framebuffer *fb;
@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device unsigned int supported_rotations); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations); +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
uint32_t max_width);+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height);/* Helpers */
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
On 05/25/2016 12:40 PM, Daniel Vetter wrote:
- Is the size/width really independent of e.g. rotation/pixel format/... Should it be the maximum that's possible under best circumstance (things could still fail), or the minimum that's guaranteed to work everwhere. This is the kind of stuff we need the userspace part for, too.
Yeah, it isn't independent of these parameters. I'm not entirely sure about this either.
Does it make sense to impose a rule that the user first sets the rotation/format plane properties, and only then read the maximum width? I'm assuming user space hates such kind of stuff.
If we use the 'best circumstance' max_width, we can first start with a minimum number of planes that need to be grouped to achieve the target mode. If that fails the atomic test, then we can try to add one plane at a time, and reduce the width for each plane.
If we use the minimum/'guaranteed to work' max_width, we'll get a higher number of planes than needed for this mode. This would pass the atomic test. We could then reduce a plane at a time and see when we fail the atomic test.
I guess we need to chose the one that's more probable to get right the first time. Considering only pixel formats for now, the minimum/'guaranteed to work' would map to the RGB formats. The best circumstance ones would probably be the planar video formats. Since we use RGB more often, the minimum one might make more sense.
We could, of course, give the property a range of max widths to confuse user space even more.
An entirely different idea for cases where a simple hint property doesn't work (other ideas floating around are can_scale, to give a hint whether a plan can at least in theory up/downscale, or not at all), is that the kernel gives more specific hints about what it would like to change.
So if userspace asks for a plane, but for the given pixel format it's too wide, the kernel could return a new proposed value for width. That would be super-flexible and could cover all kinds of use-case like rotation needing a specific tiling (fb_modifier) or specific pixel format, or specific stride.
For the case at hand there's even more worms: What about stride requirements? Afaik on some hw you just need to split the buffers into 2 planes, but can keep the wide stride (since the limit is the size of the linebuffers in the hw). On others you need to split the buffer itself into 2, because the plane hw can't cope with huge strides. Again might depend upon the pixel format.
So in a way height/width is both too much information and not precise enough. Entirely different approches:
We just add a might_need_split_plane prop to crtcs where this might be needed. Userspace then gets to retry with split buffers if it doesn't work with a huge one.
When I discussed this with qualcom folks for msm we concluded that the simplest approach would be to hide this in the kernel. So if you have a too wide plane, and need 2 hw planes to scan it out, then do that transparently in the kernel. Of course this means that there will be 1 (or 3 if you need a 2x2 split) fewer planes available, but userspace needs to iteratively build up the plane config using ATOMIC_TEST anyway.
Just fwiw, there are a few things that we will still end up abstracting in the kernel by virtualizing the mapping between kms planes and hw pipes. And the approach of weston atomic of incrementally adding more planes w/ TESTONLY flag should work well for that. (Let's hope the weston bits get upstream some day..)
But exposing width limit avoids the one-plane to multiple-pipes case, considerably simplifying things. And seemed like a generic enough limit (iirc, it applies to omapdss and probably others), that it would be cleaner to expose the limit to userspace. So there should be at least a couple other drivers that could avoid virtualizing planes with some help from userspace for this case.
Regarding rotation, I'm not 100% sure.. seems like we could just document these as the un-rotated limits. If we really had to, we could do some sort of dance where userspace sets rotation property on an un-used plane, and then reads-back the current values of the read-only prop's. But that seems awkward.
BR, -R
If possible for your hw I'm heavily leaning towards this last approach. If fits entirely into the current atomic design, and all the complexity is restricted to your driver (you need to have some allocation map between drm planes and real hw planes, but that's it).
Thoughts? -Daniel
Archit
Cheers, Daniel
Signed-off-by: Archit Taneja architt@codeaurora.org
drivers/gpu/drm/drm_atomic.c | 9 +++++++++ drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 3 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8ee1db8..fded22a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ERANGE; }
- if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
plane->max_height && (state->src_h >> 16) > plane->max_height) {DRM_DEBUG_ATOMIC("Invalid source width/height ""%u.%06ux%u.%06u\n",state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);return -ERANGE;- }
- fb_width = state->fb->width << 16; fb_height = state->fb->height << 16;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e08f962..f2d3b92 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->possible_crtcs = possible_crtcs; plane->type = type;
- plane->max_width = 0;
- plane->max_height = 0;
- list_add_tail(&plane->head, &config->plane_list); config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY)
@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, } EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width) +{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_W", max_width, max_width);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_width);
- plane->max_width = max_width;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
+int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height)+{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_H", max_height, max_height);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_height);
- plane->max_height = max_height;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
/**
- drm_mode_obj_get_properties_ioctl - get the current value of a object's property
- @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e0170bf..6104527 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1531,6 +1531,8 @@ struct drm_plane { uint32_t *format_types; unsigned int format_count; bool format_default;
uint32_t max_width;
uint32_t max_height;
struct drm_crtc *crtc; struct drm_framebuffer *fb;
@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device unsigned int supported_rotations); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations); +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
uint32_t max_width);+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height);/* Helpers */
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, May 25, 2016 at 12:36:53PM -0400, Rob Clark wrote:
On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
On 05/25/2016 12:40 PM, Daniel Vetter wrote:
- Is the size/width really independent of e.g. rotation/pixel format/... Should it be the maximum that's possible under best circumstance (things could still fail), or the minimum that's guaranteed to work everwhere. This is the kind of stuff we need the userspace part for, too.
Yeah, it isn't independent of these parameters. I'm not entirely sure about this either.
Does it make sense to impose a rule that the user first sets the rotation/format plane properties, and only then read the maximum width? I'm assuming user space hates such kind of stuff.
If we use the 'best circumstance' max_width, we can first start with a minimum number of planes that need to be grouped to achieve the target mode. If that fails the atomic test, then we can try to add one plane at a time, and reduce the width for each plane.
If we use the minimum/'guaranteed to work' max_width, we'll get a higher number of planes than needed for this mode. This would pass the atomic test. We could then reduce a plane at a time and see when we fail the atomic test.
I guess we need to chose the one that's more probable to get right the first time. Considering only pixel formats for now, the minimum/'guaranteed to work' would map to the RGB formats. The best circumstance ones would probably be the planar video formats. Since we use RGB more often, the minimum one might make more sense.
We could, of course, give the property a range of max widths to confuse user space even more.
An entirely different idea for cases where a simple hint property doesn't work (other ideas floating around are can_scale, to give a hint whether a plan can at least in theory up/downscale, or not at all), is that the kernel gives more specific hints about what it would like to change.
So if userspace asks for a plane, but for the given pixel format it's too wide, the kernel could return a new proposed value for width. That would be super-flexible and could cover all kinds of use-case like rotation needing a specific tiling (fb_modifier) or specific pixel format, or specific stride.
For the case at hand there's even more worms: What about stride requirements? Afaik on some hw you just need to split the buffers into 2 planes, but can keep the wide stride (since the limit is the size of the linebuffers in the hw). On others you need to split the buffer itself into 2, because the plane hw can't cope with huge strides. Again might depend upon the pixel format.
So in a way height/width is both too much information and not precise enough. Entirely different approches:
We just add a might_need_split_plane prop to crtcs where this might be needed. Userspace then gets to retry with split buffers if it doesn't work with a huge one.
When I discussed this with qualcom folks for msm we concluded that the simplest approach would be to hide this in the kernel. So if you have a too wide plane, and need 2 hw planes to scan it out, then do that transparently in the kernel. Of course this means that there will be 1 (or 3 if you need a 2x2 split) fewer planes available, but userspace needs to iteratively build up the plane config using ATOMIC_TEST anyway.
Just fwiw, there are a few things that we will still end up abstracting in the kernel by virtualizing the mapping between kms planes and hw pipes. And the approach of weston atomic of incrementally adding more planes w/ TESTONLY flag should work well for that. (Let's hope the weston bits get upstream some day..)
But exposing width limit avoids the one-plane to multiple-pipes case, considerably simplifying things. And seemed like a generic enough limit (iirc, it applies to omapdss and probably others), that it would be cleaner to expose the limit to userspace. So there should be at least a couple other drivers that could avoid virtualizing planes with some help from userspace for this case.
Regarding rotation, I'm not 100% sure.. seems like we could just document these as the un-rotated limits. If we really had to, we could do some sort of dance where userspace sets rotation property on an un-used plane, and then reads-back the current values of the read-only prop's. But that seems awkward.
I'm thinking all of this is doomed to fail. So right now people seem to want some kind of maximum size of the source viewport. What about the destination size? Is the max size for unscaled/scaled/smething else? Rotation/pixel format were already mentioned. How does this interact with scaling limits? What if the user is willing/not willing to do a modeset to get a higher clock speed to get moar scaling? How could the user request a higher clock speed upfront?
There are just so many variables that you can't expose them with a few numbers. I think maybe the only number we might be expose easily is the global maximum.
As far as virtualizing the resources goes. I think that would need a whole new API. Or at least a separate set of objects perhaps. I'm too lazy to dig up all the old arguments now, but I'm pretty sure there were many. If this would be done, I suspect the only sane way to do it would be to just have a hwc implementation in the kernel. As in user space would pass in the desired configuration, and the kernel would assign resources as best it can and return the result back to userspace.
BR, -R
If possible for your hw I'm heavily leaning towards this last approach. If fits entirely into the current atomic design, and all the complexity is restricted to your driver (you need to have some allocation map between drm planes and real hw planes, but that's it).
Thoughts? -Daniel
Archit
Cheers, Daniel
Signed-off-by: Archit Taneja architt@codeaurora.org
drivers/gpu/drm/drm_atomic.c | 9 +++++++++ drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 3 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8ee1db8..fded22a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ERANGE; }
- if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
plane->max_height && (state->src_h >> 16) > plane->max_height) {DRM_DEBUG_ATOMIC("Invalid source width/height ""%u.%06ux%u.%06u\n",state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);return -ERANGE;- }
- fb_width = state->fb->width << 16; fb_height = state->fb->height << 16;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e08f962..f2d3b92 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->possible_crtcs = possible_crtcs; plane->type = type;
- plane->max_width = 0;
- plane->max_height = 0;
- list_add_tail(&plane->head, &config->plane_list); config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY)
@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, } EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width) +{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_W", max_width, max_width);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_width);
- plane->max_width = max_width;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
+int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height)+{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_H", max_height, max_height);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_height);
- plane->max_height = max_height;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
/**
- drm_mode_obj_get_properties_ioctl - get the current value of a object's property
- @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e0170bf..6104527 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1531,6 +1531,8 @@ struct drm_plane { uint32_t *format_types; unsigned int format_count; bool format_default;
uint32_t max_width;
uint32_t max_height;
struct drm_crtc *crtc; struct drm_framebuffer *fb;
@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device unsigned int supported_rotations); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations); +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
uint32_t max_width);+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height);/* Helpers */
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, May 25, 2016 at 1:20 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, May 25, 2016 at 12:36:53PM -0400, Rob Clark wrote:
On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
On 05/25/2016 12:40 PM, Daniel Vetter wrote:
- Is the size/width really independent of e.g. rotation/pixel format/... Should it be the maximum that's possible under best circumstance (things could still fail), or the minimum that's guaranteed to work everwhere. This is the kind of stuff we need the userspace part for, too.
Yeah, it isn't independent of these parameters. I'm not entirely sure about this either.
Does it make sense to impose a rule that the user first sets the rotation/format plane properties, and only then read the maximum width? I'm assuming user space hates such kind of stuff.
If we use the 'best circumstance' max_width, we can first start with a minimum number of planes that need to be grouped to achieve the target mode. If that fails the atomic test, then we can try to add one plane at a time, and reduce the width for each plane.
If we use the minimum/'guaranteed to work' max_width, we'll get a higher number of planes than needed for this mode. This would pass the atomic test. We could then reduce a plane at a time and see when we fail the atomic test.
I guess we need to chose the one that's more probable to get right the first time. Considering only pixel formats for now, the minimum/'guaranteed to work' would map to the RGB formats. The best circumstance ones would probably be the planar video formats. Since we use RGB more often, the minimum one might make more sense.
We could, of course, give the property a range of max widths to confuse user space even more.
An entirely different idea for cases where a simple hint property doesn't work (other ideas floating around are can_scale, to give a hint whether a plan can at least in theory up/downscale, or not at all), is that the kernel gives more specific hints about what it would like to change.
So if userspace asks for a plane, but for the given pixel format it's too wide, the kernel could return a new proposed value for width. That would be super-flexible and could cover all kinds of use-case like rotation needing a specific tiling (fb_modifier) or specific pixel format, or specific stride.
For the case at hand there's even more worms: What about stride requirements? Afaik on some hw you just need to split the buffers into 2 planes, but can keep the wide stride (since the limit is the size of the linebuffers in the hw). On others you need to split the buffer itself into 2, because the plane hw can't cope with huge strides. Again might depend upon the pixel format.
So in a way height/width is both too much information and not precise enough. Entirely different approches:
We just add a might_need_split_plane prop to crtcs where this might be needed. Userspace then gets to retry with split buffers if it doesn't work with a huge one.
When I discussed this with qualcom folks for msm we concluded that the simplest approach would be to hide this in the kernel. So if you have a too wide plane, and need 2 hw planes to scan it out, then do that transparently in the kernel. Of course this means that there will be 1 (or 3 if you need a 2x2 split) fewer planes available, but userspace needs to iteratively build up the plane config using ATOMIC_TEST anyway.
Just fwiw, there are a few things that we will still end up abstracting in the kernel by virtualizing the mapping between kms planes and hw pipes. And the approach of weston atomic of incrementally adding more planes w/ TESTONLY flag should work well for that. (Let's hope the weston bits get upstream some day..)
But exposing width limit avoids the one-plane to multiple-pipes case, considerably simplifying things. And seemed like a generic enough limit (iirc, it applies to omapdss and probably others), that it would be cleaner to expose the limit to userspace. So there should be at least a couple other drivers that could avoid virtualizing planes with some help from userspace for this case.
Regarding rotation, I'm not 100% sure.. seems like we could just document these as the un-rotated limits. If we really had to, we could do some sort of dance where userspace sets rotation property on an un-used plane, and then reads-back the current values of the read-only prop's. But that seems awkward.
I'm thinking all of this is doomed to fail. So right now people seem to want some kind of maximum size of the source viewport. What about the destination size? Is the max size for unscaled/scaled/smething else? Rotation/pixel format were already mentioned. How does this interact with scaling limits? What if the user is willing/not willing to do a modeset to get a higher clock speed to get moar scaling? How could the user request a higher clock speed upfront?
There are just so many variables that you can't expose them with a few numbers. I think maybe the only number we might be expose easily is the global maximum.
global maximum sounds reasonable.. perhaps exposing more fine grained limits should be the domain of some more-fine-grained error reporting mechanism (that is probably needed, but I think yet to be invented)
As far as virtualizing the resources goes. I think that would need a whole new API. Or at least a separate set of objects perhaps. I'm too lazy to dig up all the old arguments now, but I'm pretty sure there were many. If this would be done, I suspect the only sane way to do it would be to just have a hwc implementation in the kernel. As in user space would pass in the desired configuration, and the kernel would assign resources as best it can and return the result back to userspace.
actually, without introducing a completely new/different uapi, it should work pretty reasonably with the way weston atomic works, incrementally adding planes in TESTONLY steps until it fails. I suspect on some more creative hw (msm is prime candidate) we'll still see custom hwc implementations to squeeze out the last bit of performance/power. But with the approach that weston uses a generic userspace should still work pretty well on that hw.
BR, -R
BR, -R
If possible for your hw I'm heavily leaning towards this last approach. If fits entirely into the current atomic design, and all the complexity is restricted to your driver (you need to have some allocation map between drm planes and real hw planes, but that's it).
Thoughts? -Daniel
Archit
Cheers, Daniel
Signed-off-by: Archit Taneja architt@codeaurora.org
drivers/gpu/drm/drm_atomic.c | 9 +++++++++ drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 3 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8ee1db8..fded22a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ERANGE; }
- if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
plane->max_height && (state->src_h >> 16) > plane->max_height) {DRM_DEBUG_ATOMIC("Invalid source width/height ""%u.%06ux%u.%06u\n",state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);return -ERANGE;- }
- fb_width = state->fb->width << 16; fb_height = state->fb->height << 16;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e08f962..f2d3b92 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->possible_crtcs = possible_crtcs; plane->type = type;
- plane->max_width = 0;
- plane->max_height = 0;
- list_add_tail(&plane->head, &config->plane_list); config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY)
@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, } EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width) +{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_W", max_width, max_width);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_width);
- plane->max_width = max_width;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
+int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height)+{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_H", max_height, max_height);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_height);
- plane->max_height = max_height;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
/**
- drm_mode_obj_get_properties_ioctl - get the current value of a object's property
- @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e0170bf..6104527 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1531,6 +1531,8 @@ struct drm_plane { uint32_t *format_types; unsigned int format_count; bool format_default;
uint32_t max_width;
uint32_t max_height;
struct drm_crtc *crtc; struct drm_framebuffer *fb;
@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device unsigned int supported_rotations); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations); +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
uint32_t max_width);+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height);/* Helpers */
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
On Wed, May 25, 2016 at 7:57 PM, Rob Clark robdclark@gmail.com wrote:
As far as virtualizing the resources goes. I think that would need a whole new API. Or at least a separate set of objects perhaps. I'm too lazy to dig up all the old arguments now, but I'm pretty sure there were many. If this would be done, I suspect the only sane way to do it would be to just have a hwc implementation in the kernel. As in user space would pass in the desired configuration, and the kernel would assign resources as best it can and return the result back to userspace.
actually, without introducing a completely new/different uapi, it should work pretty reasonably with the way weston atomic works, incrementally adding planes in TESTONLY steps until it fails. I suspect on some more creative hw (msm is prime candidate) we'll still see custom hwc implementations to squeeze out the last bit of performance/power. But with the approach that weston uses a generic userspace should still work pretty well on that hw.
So summary from my points on the m-l: - Whether we do this in all drivers that need this for dual-pipe 4k, or in all compositors is probably similar amounts of work. The upside of doing it in drivers is that it's closer to the metal, avoid midlayer-like mistakes in trying to funnel tons of stuff between the top and bottom layers through drm core, and still failing in corner cases. Drivers can simply fix things up if they're in control.
- For a bunch of reasons we (well Ville is really big on those) want to add clipped_src/dst rectangles to drm_plane_state. That's a bit of churn, but it's per-driver opt-in churn, so manageable. Once we have clipped rectangles and so fully decoupled the hw state from the uabi state it's easy to clip a plane to a sub-pipe instead of the entire virtual area of the crtc.
- Once we can clip planes we can add a bit of helper magic to steal additional&unused planes for use as the 2nd/3rd/4th plane scanning out one logical surface (drm_buffer) attached to a logical plane (drm_plane). Add a helper on top which splits a 4/5k screen at a passed in limit, and this boils down to a few lines in drivers. While still giving the drivers full control over all the details, if they want to.
- Really assigning resources within the driver while exposing whether there's enough to userspace using TEST_ONLY is the point of atomic. Doesn't matter whether it's fifo space, scalers, wizzbang gizmo A, B or C or this thing called "scanout engine". If your hw needs more than one of those to get a basic job done (fullscreen primary plane) then it's imo the drivers job to juggle all the internal resources around. KMS is supposed to work from workstation gpus down to soc in a somewhat uniform way, even on crappy hw.
Summary: Still firmly in the "virtualize it!" camp ;-)
Cheers, Daniel
On 05/25/2016 10:06 PM, Rob Clark wrote:
On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
On 05/25/2016 12:40 PM, Daniel Vetter wrote:
- Is the size/width really independent of e.g. rotation/pixel format/... Should it be the maximum that's possible under best circumstance (things could still fail), or the minimum that's guaranteed to work everwhere. This is the kind of stuff we need the userspace part for, too.
Yeah, it isn't independent of these parameters. I'm not entirely sure about this either.
Does it make sense to impose a rule that the user first sets the rotation/format plane properties, and only then read the maximum width? I'm assuming user space hates such kind of stuff.
If we use the 'best circumstance' max_width, we can first start with a minimum number of planes that need to be grouped to achieve the target mode. If that fails the atomic test, then we can try to add one plane at a time, and reduce the width for each plane.
If we use the minimum/'guaranteed to work' max_width, we'll get a higher number of planes than needed for this mode. This would pass the atomic test. We could then reduce a plane at a time and see when we fail the atomic test.
I guess we need to chose the one that's more probable to get right the first time. Considering only pixel formats for now, the minimum/'guaranteed to work' would map to the RGB formats. The best circumstance ones would probably be the planar video formats. Since we use RGB more often, the minimum one might make more sense.
We could, of course, give the property a range of max widths to confuse user space even more.
An entirely different idea for cases where a simple hint property doesn't work (other ideas floating around are can_scale, to give a hint whether a plan can at least in theory up/downscale, or not at all), is that the kernel gives more specific hints about what it would like to change.
So if userspace asks for a plane, but for the given pixel format it's too wide, the kernel could return a new proposed value for width. That would be super-flexible and could cover all kinds of use-case like rotation needing a specific tiling (fb_modifier) or specific pixel format, or specific stride.
This would be great to have, but it sounds like a new class of ABI altogether, where you set a configuration, the kernel rejects it, but gives a hint about what it wants. Do we already have something in DRI that follows such a mechanism?
For the case at hand there's even more worms: What about stride requirements? Afaik on some hw you just need to split the buffers into 2 planes, but can keep the wide stride (since the limit is the size of the linebuffers in the hw). On others you need to split the buffer itself into 2, because the plane hw can't cope with huge strides. Again might depend upon the pixel format.
I assumed that hw always belonged to the first category. For the latter, the userspace strategy of allocating buffers would have to change itself.
If we do decide to hide pipe virtualization in the kernel as you mentioned below, how would we manage plane hw that can't do huge strides. Would we need to copy the userspace populated buffer into two separate kernel buffers that fit the stride requirements?
So in a way height/width is both too much information and not precise enough. Entirely different approches:
We just add a might_need_split_plane prop to crtcs where this might be needed. Userspace then gets to retry with split buffers if it doesn't work with a huge one.
When I discussed this with qualcom folks for msm we concluded that the simplest approach would be to hide this in the kernel. So if you have a too wide plane, and need 2 hw planes to scan it out, then do that transparently in the kernel. Of course this means that there will be 1 (or 3 if you need a 2x2 split) fewer planes available, but userspace needs to iteratively build up the plane config using ATOMIC_TEST anyway.
Just fwiw, there are a few things that we will still end up abstracting in the kernel by virtualizing the mapping between kms planes and hw pipes. And the approach of weston atomic of incrementally adding more planes w/ TESTONLY flag should work well for that. (Let's hope the weston bits get upstream some day..)
But exposing width limit avoids the one-plane to multiple-pipes case, considerably simplifying things. And seemed like a generic enough limit (iirc, it applies to omapdss and probably others), that it would be cleaner to expose the limit to userspace. So there should be at least a couple other drivers that could avoid virtualizing planes with some help from userspace for this case.
Regarding rotation, I'm not 100% sure.. seems like we could just document these as the un-rotated limits. If we really had to, we could do some sort of dance where userspace sets rotation property on an un-used plane, and then reads-back the current values of the read-only prop's. But that seems awkward.
BR, -R
If possible for your hw I'm heavily leaning towards this last approach. If fits entirely into the current atomic design, and all the complexity is restricted to your driver (you need to have some allocation map between drm planes and real hw planes, but that's it).
I guess the last approach seems more apt for compositors that run the same code for all hw (weston, etc). In android userspace, each display hw has the luxury of having it's own middleware where they have more freedom on how they manage planes and how they interpret the DRM ioctls. For such middleware, preventing virtualization helps in moving the bulk of the code in userspace. I don't know if android will continue to have such middleware when all SoCs move to drm hwcomposer, though.
This probably needs more feedback Qcom and other SoC people. Although, as Rob said, if a lot of hw has a limitation over the line buffer width they have on a plane, it could be handy to have such expose such a property for drivers that don't want to virtualize their plane hw just for this one usecase. But, as you said, max_width isn't precise enough information either. So, yeah, we'll probably need to think over this a bit more.
Archit
Thoughts? -Daniel
Archit
Cheers, Daniel
Signed-off-by: Archit Taneja architt@codeaurora.org
drivers/gpu/drm/drm_atomic.c | 9 +++++++++ drivers/gpu/drm/drm_crtc.c | 38 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 ++++++ 3 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8ee1db8..fded22a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ERANGE; }
- if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
plane->max_height && (state->src_h >> 16) > plane->max_height) {DRM_DEBUG_ATOMIC("Invalid source width/height ""%u.%06ux%u.%06u\n",state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);return -ERANGE;- }
- fb_width = state->fb->width << 16; fb_height = state->fb->height << 16;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e08f962..f2d3b92 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->possible_crtcs = possible_crtcs; plane->type = type;
- plane->max_width = 0;
- plane->max_height = 0;
- list_add_tail(&plane->head, &config->plane_list); config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY)
@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane, } EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width) +{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_W", max_width, max_width);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_width);
- plane->max_width = max_width;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
+int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height)+{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
"MAX_H", max_height, max_height);- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_height);
- plane->max_height = max_height;
- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
- /**
- drm_mode_obj_get_properties_ioctl - get the current value of a object's property
- @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e0170bf..6104527 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1531,6 +1531,8 @@ struct drm_plane { uint32_t *format_types; unsigned int format_count; bool format_default;
uint32_t max_width;
uint32_t max_height;
struct drm_crtc *crtc; struct drm_framebuffer *fb;
@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device unsigned int supported_rotations); extern unsigned int drm_rotation_simplify(unsigned int rotation, unsigned int supported_rotations); +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
uint32_t max_width);+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
uint32_t max_height);/* Helpers */
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Mon, May 30, 2016 at 02:09:17PM +0530, Archit Taneja wrote:
On 05/25/2016 10:06 PM, Rob Clark wrote:
On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
On 05/25/2016 12:40 PM, Daniel Vetter wrote:
- Is the size/width really independent of e.g. rotation/pixel format/... Should it be the maximum that's possible under best circumstance (things could still fail), or the minimum that's guaranteed to work everwhere. This is the kind of stuff we need the userspace part for, too.
Yeah, it isn't independent of these parameters. I'm not entirely sure about this either.
Does it make sense to impose a rule that the user first sets the rotation/format plane properties, and only then read the maximum width? I'm assuming user space hates such kind of stuff.
If we use the 'best circumstance' max_width, we can first start with a minimum number of planes that need to be grouped to achieve the target mode. If that fails the atomic test, then we can try to add one plane at a time, and reduce the width for each plane.
If we use the minimum/'guaranteed to work' max_width, we'll get a higher number of planes than needed for this mode. This would pass the atomic test. We could then reduce a plane at a time and see when we fail the atomic test.
I guess we need to chose the one that's more probable to get right the first time. Considering only pixel formats for now, the minimum/'guaranteed to work' would map to the RGB formats. The best circumstance ones would probably be the planar video formats. Since we use RGB more often, the minimum one might make more sense.
We could, of course, give the property a range of max widths to confuse user space even more.
An entirely different idea for cases where a simple hint property doesn't work (other ideas floating around are can_scale, to give a hint whether a plan can at least in theory up/downscale, or not at all), is that the kernel gives more specific hints about what it would like to change.
So if userspace asks for a plane, but for the given pixel format it's too wide, the kernel could return a new proposed value for width. That would be super-flexible and could cover all kinds of use-case like rotation needing a specific tiling (fb_modifier) or specific pixel format, or specific stride.
This would be great to have, but it sounds like a new class of ABI altogether, where you set a configuration, the kernel rejects it, but gives a hint about what it wants. Do we already have something in DRI that follows such a mechanism?
Yeah, we definitely don't want to start out with that.
For the case at hand there's even more worms: What about stride requirements? Afaik on some hw you just need to split the buffers into 2 planes, but can keep the wide stride (since the limit is the size of the linebuffers in the hw). On others you need to split the buffer itself into 2, because the plane hw can't cope with huge strides. Again might depend upon the pixel format.
I assumed that hw always belonged to the first category. For the latter, the userspace strategy of allocating buffers would have to change itself.
If we do decide to hide pipe virtualization in the kernel as you mentioned below, how would we manage plane hw that can't do huge strides. Would we need to copy the userspace populated buffer into two separate kernel buffers that fit the stride requirements?
I think at that point the kernel needs to make sure that the preferred mode is _not_ a mode that requires such a stride. And then userspace needs to piece stuff together on it's own. This is hw-mis-design on a level that I don't think makes much sense to abstract away ;-)
But afaik the discussion here is just for hw where we might need 2 or 4 planes to scan out one logical buffer.
So in a way height/width is both too much information and not precise enough. Entirely different approches:
We just add a might_need_split_plane prop to crtcs where this might be needed. Userspace then gets to retry with split buffers if it doesn't work with a huge one.
When I discussed this with qualcom folks for msm we concluded that the simplest approach would be to hide this in the kernel. So if you have a too wide plane, and need 2 hw planes to scan it out, then do that transparently in the kernel. Of course this means that there will be 1 (or 3 if you need a 2x2 split) fewer planes available, but userspace needs to iteratively build up the plane config using ATOMIC_TEST anyway.
Just fwiw, there are a few things that we will still end up abstracting in the kernel by virtualizing the mapping between kms planes and hw pipes. And the approach of weston atomic of incrementally adding more planes w/ TESTONLY flag should work well for that. (Let's hope the weston bits get upstream some day..)
But exposing width limit avoids the one-plane to multiple-pipes case, considerably simplifying things. And seemed like a generic enough limit (iirc, it applies to omapdss and probably others), that it would be cleaner to expose the limit to userspace. So there should be at least a couple other drivers that could avoid virtualizing planes with some help from userspace for this case.
Regarding rotation, I'm not 100% sure.. seems like we could just document these as the un-rotated limits. If we really had to, we could do some sort of dance where userspace sets rotation property on an un-used plane, and then reads-back the current values of the read-only prop's. But that seems awkward.
BR, -R
If possible for your hw I'm heavily leaning towards this last approach. If fits entirely into the current atomic design, and all the complexity is restricted to your driver (you need to have some allocation map between drm planes and real hw planes, but that's it).
I guess the last approach seems more apt for compositors that run the same code for all hw (weston, etc). In android userspace, each display hw has the luxury of having it's own middleware where they have more freedom on how they manage planes and how they interpret the DRM ioctls. For such middleware, preventing virtualization helps in moving the bulk of the code in userspace. I don't know if android will continue to have such middleware when all SoCs move to drm hwcomposer, though.
drm_hwcomposer (developed by Google) is a generic hwc implementation along the lines of weston and any other generic kms compositor. I think it makes a lot of sense to aim for a reasonable baseline API which works the same way everywhere. And that reasonable baseline abi imo includes that at least 1 plane can be used full-screen, without jumping through hooks.
Because if you don't have that, then it's not just weston and android you need to patch up. But also - fbdev - any boot splash - any igt testcases (I'm starting to push hard to make those a requirement for kms drivers) - X - any other compositor, like mutter, kwin, ozone, ...
At that point we're not doing a real generic platform any more, but fully leaking all the details into userspace (like hwc does on Android). That's not terribly useful.
Imo the rule should be that special code can only be required by userspace for optimizations (more power efficient or whatever), not for basic functionality. Displaying a boot splash is _very_ basic ;-)
This probably needs more feedback Qcom and other SoC people. Although, as Rob said, if a lot of hw has a limitation over the line buffer width they have on a plane, it could be handy to have such expose such a property for drivers that don't want to virtualize their plane hw just for this one usecase. But, as you said, max_width isn't precise enough information either. So, yeah, we'll probably need to think over this a bit more.
I massively disagree with "this one usecase". If you can't do all the things above, there's no reason at all to have a kms driver imo. At least not in upstream. If it only works on that one vendor tree with the one vendor hwc and no where else, I don't think it makes sense to have it in upstream.
But like discussed with Rob, I think it should be pretty easy to have a set of generic helpers in-kernel to make this virtualization really easy. In the end you want to share the code between different compositors anyway, and the best place to put such shared code is within the kernel itself imo. Of course for such a helper we'd need to have 2-3 drivers using it, to make sure it's good enough.
Cheers, Daniel
On Mon, May 30, 2016 at 5:32 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, May 30, 2016 at 02:09:17PM +0530, Archit Taneja wrote:
On 05/25/2016 10:06 PM, Rob Clark wrote:
On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
On 05/25/2016 12:40 PM, Daniel Vetter wrote:
- Is the size/width really independent of e.g. rotation/pixel format/... Should it be the maximum that's possible under best circumstance (things could still fail), or the minimum that's guaranteed to work everwhere. This is the kind of stuff we need the userspace part for, too.
Yeah, it isn't independent of these parameters. I'm not entirely sure about this either.
Does it make sense to impose a rule that the user first sets the rotation/format plane properties, and only then read the maximum width? I'm assuming user space hates such kind of stuff.
If we use the 'best circumstance' max_width, we can first start with a minimum number of planes that need to be grouped to achieve the target mode. If that fails the atomic test, then we can try to add one plane at a time, and reduce the width for each plane.
If we use the minimum/'guaranteed to work' max_width, we'll get a higher number of planes than needed for this mode. This would pass the atomic test. We could then reduce a plane at a time and see when we fail the atomic test.
I guess we need to chose the one that's more probable to get right the first time. Considering only pixel formats for now, the minimum/'guaranteed to work' would map to the RGB formats. The best circumstance ones would probably be the planar video formats. Since we use RGB more often, the minimum one might make more sense.
We could, of course, give the property a range of max widths to confuse user space even more.
An entirely different idea for cases where a simple hint property doesn't work (other ideas floating around are can_scale, to give a hint whether a plan can at least in theory up/downscale, or not at all), is that the kernel gives more specific hints about what it would like to change.
So if userspace asks for a plane, but for the given pixel format it's too wide, the kernel could return a new proposed value for width. That would be super-flexible and could cover all kinds of use-case like rotation needing a specific tiling (fb_modifier) or specific pixel format, or specific stride.
This would be great to have, but it sounds like a new class of ABI altogether, where you set a configuration, the kernel rejects it, but gives a hint about what it wants. Do we already have something in DRI that follows such a mechanism?
Yeah, we definitely don't want to start out with that.
For the case at hand there's even more worms: What about stride requirements? Afaik on some hw you just need to split the buffers into 2 planes, but can keep the wide stride (since the limit is the size of the linebuffers in the hw). On others you need to split the buffer itself into 2, because the plane hw can't cope with huge strides. Again might depend upon the pixel format.
I assumed that hw always belonged to the first category. For the latter, the userspace strategy of allocating buffers would have to change itself.
If we do decide to hide pipe virtualization in the kernel as you mentioned below, how would we manage plane hw that can't do huge strides. Would we need to copy the userspace populated buffer into two separate kernel buffers that fit the stride requirements?
I think at that point the kernel needs to make sure that the preferred mode is _not_ a mode that requires such a stride. And then userspace needs to piece stuff together on it's own. This is hw-mis-design on a level that I don't think makes much sense to abstract away ;-)
But afaik the discussion here is just for hw where we might need 2 or 4 planes to scan out one logical buffer.
right, if you need userspace to composite split buffers, you've already lost..
So in a way height/width is both too much information and not precise enough. Entirely different approches:
We just add a might_need_split_plane prop to crtcs where this might be needed. Userspace then gets to retry with split buffers if it doesn't work with a huge one.
When I discussed this with qualcom folks for msm we concluded that the simplest approach would be to hide this in the kernel. So if you have a too wide plane, and need 2 hw planes to scan it out, then do that transparently in the kernel. Of course this means that there will be 1 (or 3 if you need a 2x2 split) fewer planes available, but userspace needs to iteratively build up the plane config using ATOMIC_TEST anyway.
Just fwiw, there are a few things that we will still end up abstracting in the kernel by virtualizing the mapping between kms planes and hw pipes. And the approach of weston atomic of incrementally adding more planes w/ TESTONLY flag should work well for that. (Let's hope the weston bits get upstream some day..)
But exposing width limit avoids the one-plane to multiple-pipes case, considerably simplifying things. And seemed like a generic enough limit (iirc, it applies to omapdss and probably others), that it would be cleaner to expose the limit to userspace. So there should be at least a couple other drivers that could avoid virtualizing planes with some help from userspace for this case.
Regarding rotation, I'm not 100% sure.. seems like we could just document these as the un-rotated limits. If we really had to, we could do some sort of dance where userspace sets rotation property on an un-used plane, and then reads-back the current values of the read-only prop's. But that seems awkward.
BR, -R
If possible for your hw I'm heavily leaning towards this last approach. If fits entirely into the current atomic design, and all the complexity is restricted to your driver (you need to have some allocation map between drm planes and real hw planes, but that's it).
I guess the last approach seems more apt for compositors that run the same code for all hw (weston, etc). In android userspace, each display hw has the luxury of having it's own middleware where they have more freedom on how they manage planes and how they interpret the DRM ioctls. For such middleware, preventing virtualization helps in moving the bulk of the code in userspace. I don't know if android will continue to have such middleware when all SoCs move to drm hwcomposer, though.
drm_hwcomposer (developed by Google) is a generic hwc implementation along the lines of weston and any other generic kms compositor. I think it makes a lot of sense to aim for a reasonable baseline API which works the same way everywhere. And that reasonable baseline abi imo includes that at least 1 plane can be used full-screen, without jumping through hooks.
Because if you don't have that, then it's not just weston and android you need to patch up. But also
- fbdev
- any boot splash
- any igt testcases (I'm starting to push hard to make those a requirement for kms drivers)
- X
- any other compositor, like mutter, kwin, ozone, ...
I think it's not so bad. We fix the setcrtc helpers to do this for legacy crtc path (which should pretty much cover most existing compositors, X, splash, fbdev, etc). After all they already pick one plane, it shouldn't be too hard to make them pick two.
But for atomic, we make userspace deal with it, since things get more complicated than what setcrtc/pageflip has to deal with. Afaiu there are basically two atomic compositors (and the weston one isn't even merged), so I think dealing with this in userspace is not so bad.
BR, -R
At that point we're not doing a real generic platform any more, but fully leaking all the details into userspace (like hwc does on Android). That's not terribly useful.
Imo the rule should be that special code can only be required by userspace for optimizations (more power efficient or whatever), not for basic functionality. Displaying a boot splash is _very_ basic ;-)
This probably needs more feedback Qcom and other SoC people. Although, as Rob said, if a lot of hw has a limitation over the line buffer width they have on a plane, it could be handy to have such expose such a property for drivers that don't want to virtualize their plane hw just for this one usecase. But, as you said, max_width isn't precise enough information either. So, yeah, we'll probably need to think over this a bit more.
I massively disagree with "this one usecase". If you can't do all the things above, there's no reason at all to have a kms driver imo. At least not in upstream. If it only works on that one vendor tree with the one vendor hwc and no where else, I don't think it makes sense to have it in upstream.
But like discussed with Rob, I think it should be pretty easy to have a set of generic helpers in-kernel to make this virtualization really easy. In the end you want to share the code between different compositors anyway, and the best place to put such shared code is within the kernel itself imo. Of course for such a helper we'd need to have 2-3 drivers using it, to make sure it's good enough.
Cheers, Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Mon, May 30, 2016 at 09:47:53AM -0400, Rob Clark wrote:
On Mon, May 30, 2016 at 5:32 AM, Daniel Vetter daniel@ffwll.ch wrote:
drm_hwcomposer (developed by Google) is a generic hwc implementation along the lines of weston and any other generic kms compositor. I think it makes a lot of sense to aim for a reasonable baseline API which works the same way everywhere. And that reasonable baseline abi imo includes that at least 1 plane can be used full-screen, without jumping through hooks.
Because if you don't have that, then it's not just weston and android you need to patch up. But also
- fbdev
- any boot splash
- any igt testcases (I'm starting to push hard to make those a requirement for kms drivers)
- X
- any other compositor, like mutter, kwin, ozone, ...
I think it's not so bad. We fix the setcrtc helpers to do this for legacy crtc path (which should pretty much cover most existing compositors, X, splash, fbdev, etc). After all they already pick one plane, it shouldn't be too hard to make them pick two.
But for atomic, we make userspace deal with it, since things get more complicated than what setcrtc/pageflip has to deal with. Afaiu there are basically two atomic compositors (and the weston one isn't even merged), so I think dealing with this in userspace is not so bad.
What exactly is more complicated with doing this for full atomic? If you do it in userspace for setcrtc, plus fbdev, plus weston/ozone/drm_hwcomposer/ and maybe more, then that's already an impressive pile of copies of that logic. And I really think it should be pretty simple to type this up into a helper that works on any plane. -Daniel
On Mon, May 30, 2016 at 10:38 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, May 30, 2016 at 09:47:53AM -0400, Rob Clark wrote:
On Mon, May 30, 2016 at 5:32 AM, Daniel Vetter daniel@ffwll.ch wrote:
drm_hwcomposer (developed by Google) is a generic hwc implementation along the lines of weston and any other generic kms compositor. I think it makes a lot of sense to aim for a reasonable baseline API which works the same way everywhere. And that reasonable baseline abi imo includes that at least 1 plane can be used full-screen, without jumping through hooks.
Because if you don't have that, then it's not just weston and android you need to patch up. But also
- fbdev
- any boot splash
- any igt testcases (I'm starting to push hard to make those a requirement for kms drivers)
- X
- any other compositor, like mutter, kwin, ozone, ...
I think it's not so bad. We fix the setcrtc helpers to do this for legacy crtc path (which should pretty much cover most existing compositors, X, splash, fbdev, etc). After all they already pick one plane, it shouldn't be too hard to make them pick two.
But for atomic, we make userspace deal with it, since things get more complicated than what setcrtc/pageflip has to deal with. Afaiu there are basically two atomic compositors (and the weston one isn't even merged), so I think dealing with this in userspace is not so bad.
What exactly is more complicated with doing this for full atomic? If you do it in userspace for setcrtc, plus fbdev, plus
s/userspace/kernel/ (for setcrtc/pageflip)
weston/ozone/drm_hwcomposer/ and maybe more, then that's already an impressive pile of copies of that logic. And I really think it should be pretty simple to type this up into a helper that works on any plane.
For legacy setcrtc/pageflip, I think we just change crtc->primary into an array of N (where N is worst case # of planes). It is a scenario where you don't have to care about scaling, or really other planes in use, and a static association of planes to crtc's should be fine. And we wouldn't expose the additional primaries to userspace so no conflicts to worry about..
Seems like a fairly small change in the atomic helpers, vs having to invent virtual planes mapping to physical planes, etc.
BR, -R
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Archit Taneja -
Daniel Vetter -
Rob Clark -
Ville Syrjälä