Re: [RFC] drm: Introduce max width and height properties for planes

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