[RFC] drm: add hint to userspace about whether a plane can scale
Daniel Vetter
daniel at ffwll.ch
Fri Oct 21 12:35:15 UTC 2016
On Fri, Oct 21, 2016 at 09:58:45AM +0100, Brian Starkey wrote:
> Hi Rob,
>
> On Thu, Oct 20, 2016 at 04:17:14PM -0400, Rob Clark wrote:
> > When you have a mix of planes that can scale and those that cannot
> > scale, userspace really wants to have some hint to know which planes
> > can definitely not scale so it knows to assign them to unscaled layers.
> > I don't think it is fully possible to describe scaling constraints in
> > a generic way, so I don't think it is even worth trying, so this is
> > not a substitute for atomic TESTONLY step, but it does reduce the
> > search-space for userspace. In the common case, most layers will not
> > be scaled so knowing the best planes to pick for those layers is
> > useful.
>
> Somewhat related to what Daniel mentioned on IRC about driver
> consistency - how about making it "cannot_scale". This is then opt-in
> for drivers, and should mean userspace can always trust it if it's
> set.
>
> i.e. if cannot_scale == false, userspace can give it a shot, but if
> cannot_scale == true it should never bother.
>
> Either way, even with a device-specific planner we would want this
> hint to manage different HW versions so I'm in favour. But...
I think first thing we should do is add some backoff heuristics to
drm_hwcomposer to try the same plane also with a non-scaled surface (after
having tried it with a scaled one). That would probably be as effective as
adding "can_scale", but with the upshot that we're not at the mercy of
drivers exposing it correctly.
I'm always vary when we have the same limit checks 2 (in this case once in
atomic_check, and once in the code that registers the property). And I'd
like to avoid that as much as possible. We could avoid this by making the
can_scale property mandatory, and enforcing it in the drm core. I.e. if
it's not set, we reject scaled planes. That should give some decent
motivation for driver writers to update them correctly. But without such a
self-consistency check I don't really like this. It would be akin to
adding "can_rotate" besides the "rotation" prop, and allowing drivers to
botch things up and not register stuff consistently.
-Daniel
>
> > ---
> > drivers/gpu/drm/drm_crtc.c | 1 +
> > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 1 +
> > include/drm/drm_crtc.h | 2 ++
> > include/uapi/drm/drm_mode.h | 3 +++
> > 4 files changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index b4b973f..d7e0e0d 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -2389,6 +2389,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
> > plane_resp->plane_id = plane->base.id;
> > plane_resp->possible_crtcs = plane->possible_crtcs;
> > plane_resp->gamma_size = 0;
> > + plane_resp->can_scale = plane->can_scale;
> >
> > /*
> > * This ioctl is called twice, once to determine how much space is
> > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> > index 692c888..2061c83 100644
> > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> > @@ -908,6 +908,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
> > mdp5_plane->pipe = pipe;
> > mdp5_plane->name = pipe2name(pipe);
> > mdp5_plane->caps = caps;
> > + plane->can_scale = !!(caps & MDP_PIPE_CAP_SCALE);
> >
> > mdp5_plane->nformats = mdp_get_formats(mdp5_plane->formats,
> > ARRAY_SIZE(mdp5_plane->formats),
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index d74d47a..6e290b6 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1679,6 +1679,7 @@ enum drm_plane_type {
> > * @format_types: array of formats supported by this plane
> > * @format_count: number of formats supported
> > * @format_default: driver hasn't supplied supported formats for the plane
> > + * @can_scale: a hint to userspace that this plane can (or cannot) scale
> > * @crtc: currently bound CRTC
> > * @fb: currently bound fb
> > * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by
> > @@ -1710,6 +1711,7 @@ struct drm_plane {
> > uint32_t *format_types;
> > unsigned int format_count;
> > bool format_default;
> > + bool can_scale;
> >
> > struct drm_crtc *crtc;
> > struct drm_framebuffer *fb;
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index ce71ad5..5bf9361 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -191,6 +191,9 @@ struct drm_mode_get_plane {
> >
> > __u32 count_format_types;
> > __u64 format_type_ptr;
> > +
> > + __u32 can_scale;
>
> ... 32 bits for a bool does seem a bit wasteful.
>
> Thanks,
> Brian
>
> > + __u32 pad;
> > };
> >
> > struct drm_mode_get_plane_res {
> > --
> > 2.7.4
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list