On Wed, May 25, 2016 at 1:20 PM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxx> 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@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
>> >> >>
>> >> >
>> >>
>> >> --
>> >> 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
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
