[RFC] drm: Introduce max width and height properties for planes
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed May 25 17:20:25 UTC 2016
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 at 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 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
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list