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@xxxxxxxxxxxxxx>
> ---
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
