On Fri, Oct 21, 2016 at 8:35 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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.
well, ignoring the fact that drm-hwc2 just tries one thing then falls
back to gl (which should be fixed but it is a big pile of c++11 code
so that will be someone elses job ;-))...
I did think about this approach, but two many changing variables so
making userspace guess about this sort of thing just seems evil.
> 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.
Fair enough, I'll add a check in drm core. That is simple enough. I
guess remaining question should can_scale default to true?
I guess the first time someone tries to bring up drm-hwc or similar
(ie. something more complex than single fullscreen layer) on their hw,
they are going to run into a few bugs in their driver, so I guess I'd
be ok for it to default to false and let people fix it when the time
comes.
BR,
-R
> -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@xxxxxxxxxxxxxxxxxxxxx
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> 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
