[RFC] drm: Introduce max width and height properties for planes
Archit Taneja
architt at codeaurora.org
Wed May 25 10:58:36 UTC 2016
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 at 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
More information about the dri-devel
mailing list