[PATCH 0/7] prepare for atomic.. the great propertyification
Rob Clark
robdclark at gmail.com
Thu Jul 24 06:36:20 PDT 2014
On Thu, Jul 24, 2014 at 8:25 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Wed, Jul 23, 2014 at 03:38:13PM -0400, Rob Clark wrote:
>> This is mostly just a rebase+resend. Was going to send it a bit earlier
>> but had a few things to fix up as a result of the rebase.
>>
>> At this point, I think next steps are roughly:
>> 1) introduce plane->mutex
>> 2) decide what we want to do about events
>> 3) add actual ioctl
>>
>> I think we could shoot for merging this series next, and then adding
>> plane->mutex in 3.18?
>>
>> Before we add the ioctl, I think we want to sort out events for updates
>> to non-primary layers, and what the interface to drivers should look like.
>> Ie. just add event to ->update_plane() or should we completely ditch
>> ->page_flip() and ->update_plane()?
>>
>> Technically, I think we could get away without a new API and just let
>> drivers grab all the events in their ->atomic_commit(), but I suspect
>> core could provide something more useful to drivers. I guess it would
>> be useful to have a few more drivers converted over to see what makes
>> sense.
>
> Ok so we've had a lot of discussions about this on irc and overall I'm not
> terribly happy with what this looks like. I have a few concerns:
>
> - The entire atomic conversion for drivers is monolithic, at least at the
> interface level. So if you want to do this step-by-step you're forced to
> add hacks and e.g. bypass pageflips until that's implemented. E.g. on
> i915 I see no chance that we'll get atomic ready for all cases (so
> nonblocking flips and multi-crtc and everything on all platforms) in one
> go.
So, there interface is *intended* to be monolithic, in a way.. many
years ago, I started by adding side by side atomic vs legacy paths,
and it was pretty ugly in core. I'm much happier with the current
iteration.
A slightly later iteration preserved atomic helpers (so drivers could
do completely their own thing). I ended up dropping that because most
of what atomic does is manage the interface to whatever is doing
modeset/pageflip/whatever. I completely expect some changes around
the commit stuff.. that part is intended to either be
wrapped/extended by the driver or replaced. Possibly it could be made
more clear what drivers are expected to use vs extend or replace. I
expect this to evolve a bit as more drivers are converted.
Note that the current atomic "layer" results in 1:1 conversion from
old userspace API to legacy vfuncs. This way what is on top of atomic
API and what is beneath (ie. the driver) has same code paths either
way (old or new userspace, converted or not driver). The edge cases
don't come in until atomic ioctl is exposed, and for that I expect a
driver flag to enable the new ioctl. You could even set the flag
based on hw generation if you want.
> - Related to that is that we force legacy ioctls through atomic. E.g. on
> older i915 platforms I very much expect that we won't ever convert the
> pageflip code to atomic for them and just not support atomic flips of
> cursor + primary plane. At least not at the beginning. I know that's
> different from my stance a year ago, but I've become a bit more
> realistic.
>
> - The entire conversion is monolithic and we can't do it on a
> driver-by-driver basis. Everyone has to go through the new atomic
> interfaces on a flag day. drm is much bigger now and forcing everyone to
> convert at the same time is really risky. Again I know I've changed my
> mind again, but drm is a lot bigger and there's a lot more drivers that
> implement pageflip and cursors, i.e. stuff that's special.
the only flag day part is plugging in atomic functions and couple
vfunc API changes around set_prop().. the current design allows for
conversion on driver by driver basis.
> - The separation between interface marshalling code in the drm core and
> helper functions for drivers to implement stuff is fuzzy at best. Thus
> far we've had an excellent seperation in this are for kms, I don't think
> it's vise to throw that out.
Interface marshalling should not be helper. Everyone needs the same
thing, since what is on top is the same.
> So here's my proposal for how to get this in without breaking the world
>
> 1. We add a complete new set of ->atomic_foo driver entry points to all
> relevant objects. So instead of changing all the set_prop functions in a
> flag-day like operation we just add a new interface. I haven't double
> checked, but I guess that means per-obj ->atomic_set_prop functions plus a
> global ->atomic_check and ->atomic_commit.
that sounds much worse
> 2. We add a new drm_atomic_helper.c library which provides functions to
> implement all the legacy interfaces (page_flip, update_plane, set_prop)
> using atomic interfaces. We should be able to 1:1 reuse Rob's code for
> this, but instead of changing the actual current interface code we put
> that into helpers.
So, I've started ripping out page_flip.. now w/ primary plane helpers
we can do everything in terms of update_plane(). I have an idea to
handle events. It isn't so bad, but forces me to do some re-arranging
in drm/msm that I was planning to postpone otherwise.
(And I just got a fedex package w/ new toys, er, hardware.. but I'll
try to finish this today)
> We don't need anything for cursor ioctls since cursor plane helpers
> already map the legacy cursor ioctls.
>
> 3. Rob's current code reuses the existing ->update_plane, ->pageflip and
> other entry points to implement the atomic commit helper functions. Imo
> that's a bad layering violation, and if we plug helpers in there we can't
> use them.
Well, it is only a layering violation because you have a slightly
different idea of where the layers should be. ;-)
Atomic (or really the state stuff.. maybe I should have called it
"transaction" or something like that to avoid the atomic
connotation..) should be on top of, not below, helpers.
Probably the commit part should be more clearly separated out and
marked as "helper", because that is the part that is about taking the
state that userspace (or fbdev) has asked for and applying it to the
hw. This is really the part where drivers for some hw might want to
do something different. Although I admit lumping it in w/
drm_atomic.c makes this not very clear.
And then beneath atomic we introduce new interfaces (if needed..
although update_plane isn't so bad once we toss out page_flip). We
could introduce new ->commit_state() vfuncs, and alternate set of
commit helper which uses those.. at least for planes I suspect
->commit_state() ends up looking a lot like ->update_plane.
> Instead I think we should add a new per-plane ->atomic_commit functions
> clearly marked as optional. Maybe even as part of an opaque
> plane_helper_funcs pointer like we have with crtc/encoder/connector and
> crtc helpers. msm would then implement it's atomic commit function in
> there (since it's the only driver currently supporting atomic for real).
>
> 3b. We could adjust crtc helpers to use the plane commit hook instead of
> the set_base function when avialable.
would be a nice cleanup either way.. but I think it is independent..
> 4. We do a pimped atomic helper based upon crtc helpers and the above
> plane commit function added in 3. It would essentially implement what
> Rob's current helper does, i.e.
>
> Step 1: Shut down all crtcs which change using the crtc helpers. This step
> is obviously optional and ommitted when there's nothing to do.
>
> Step 2: Loop over all planes and call ->plane_commit or whatever we will
> call the interface added in 3. above.
>
> Step 3: Enable all crtcs that need enabling.
>
> 5. We start converting drivers. Every driver can convert at it's own pace,
> opting in for atomic behaviour step-by-step.
>
> 6. We optionally use the atomic interface in the fb helper. It's imo
> important to keep the code here parallel so that drivers can convert at
> their own leisure.
this is one thing I really wanted to avoid. It was already getting
ugly the first time I tried it, and I hadn't even converted
everything.
BR,
-R
> 7. We add the atomic ioctl.
>
> 8. Various cleanups once 5. is completed for all drivers - which will
> likely take at least a year:
> - Remove ->set_base from crtc helpers.
> - Remove legacy functions from fbdev helpers.
> - ...
>
> I know this is a stack of work, but most of it just shuffles code around
> that already exists 1:1 in Rob's series. And I really think that this is
> where we should ultimately end up at, and going indirectly to that point
> will be much more painful.
>
> Also note that nothing in here really touches shared code, so we can
> squeeze this into 3.17 even really late without risk to existing drivers.
> Imo it's too late already in the 3.17 cycle for the current series, which
> touches everything. If we get everything up to step 4. in now we can
> convert 1-2 drivers in 3.18 and enable the atomic ioctl after that.
>
> And I realize that everyon will totally hate this because apparently
> google is making a hard move with adf and plans to enforce usage of that,
> so this entire thing is a bit ugly.
>
> Cheers, Daniel
>
>>
>> Rob Clark (5):
>> drm: add atomic fxns
>> drm: split propvals out and blob property support
>> drm: convert plane to properties/state
>> drm: convert crtc to properties/state
>> drm/msm: add atomic support
>>
>> Sean Paul (1):
>> drm: Fix up the atomic legacy paths so they work
>>
>> Ville Syrjälä (1):
>> drm: Refactor object property check code
>>
>> drivers/gpu/drm/Makefile | 2 +-
>> drivers/gpu/drm/armada/armada_crtc.c | 14 +-
>> drivers/gpu/drm/armada/armada_output.c | 3 +-
>> drivers/gpu/drm/armada/armada_overlay.c | 14 +-
>> drivers/gpu/drm/ast/ast_drv.c | 6 +
>> drivers/gpu/drm/ast/ast_drv.h | 1 +
>> drivers/gpu/drm/ast/ast_mode.c | 1 +
>> drivers/gpu/drm/cirrus/cirrus_drv.c | 6 +
>> drivers/gpu/drm/cirrus/cirrus_drv.h | 1 +
>> drivers/gpu/drm/cirrus/cirrus_mode.c | 1 +
>> drivers/gpu/drm/drm_atomic.c | 733 +++++++++++++++
>> drivers/gpu/drm/drm_crtc.c | 1351 ++++++++++++++++++---------
>> drivers/gpu/drm/drm_fb_helper.c | 55 +-
>> drivers/gpu/drm/drm_irq.c | 8 +-
>> drivers/gpu/drm/drm_modeset_lock.c | 28 +
>> drivers/gpu/drm/drm_plane_helper.c | 2 +
>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 11 +-
>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 7 +
>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 11 +-
>> drivers/gpu/drm/gma500/cdv_intel_crt.c | 4 +-
>> drivers/gpu/drm/gma500/cdv_intel_display.c | 1 +
>> drivers/gpu/drm/gma500/cdv_intel_dp.c | 7 +-
>> drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 7 +-
>> drivers/gpu/drm/gma500/cdv_intel_lvds.c | 10 +-
>> drivers/gpu/drm/gma500/mdfld_dsi_output.c | 12 +-
>> drivers/gpu/drm/gma500/psb_drv.c | 7 +
>> drivers/gpu/drm/gma500/psb_drv.h | 1 +
>> drivers/gpu/drm/gma500/psb_intel_display.c | 1 +
>> drivers/gpu/drm/gma500/psb_intel_drv.h | 4 +-
>> drivers/gpu/drm/gma500/psb_intel_lvds.c | 10 +-
>> drivers/gpu/drm/gma500/psb_intel_sdvo.c | 23 +-
>> drivers/gpu/drm/i2c/ch7006_drv.c | 4 +-
>> drivers/gpu/drm/i915/i915_drv.c | 8 +
>> drivers/gpu/drm/i915/intel_crt.c | 4 +-
>> drivers/gpu/drm/i915/intel_display.c | 6 +-
>> drivers/gpu/drm/i915/intel_dp.c | 7 +-
>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>> drivers/gpu/drm/i915/intel_hdmi.c | 7 +-
>> drivers/gpu/drm/i915/intel_lvds.c | 4 +-
>> drivers/gpu/drm/i915/intel_sdvo.c | 23 +-
>> drivers/gpu/drm/i915/intel_sprite.c | 1 +
>> drivers/gpu/drm/i915/intel_tv.c | 12 +-
>> drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +
>> drivers/gpu/drm/mgag200/mgag200_drv.h | 1 +
>> drivers/gpu/drm/mgag200/mgag200_mode.c | 1 +
>> drivers/gpu/drm/msm/Makefile | 1 +
>> drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 66 +-
>> drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 6 +
>> drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.h | 1 +
>> drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 14 +-
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 65 +-
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 6 +
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 2 +-
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 14 +-
>> drivers/gpu/drm/msm/msm_atomic.c | 141 +++
>> drivers/gpu/drm/msm/msm_drv.c | 26 +
>> drivers/gpu/drm/msm/msm_drv.h | 8 +
>> drivers/gpu/drm/msm/msm_gem.c | 24 +-
>> drivers/gpu/drm/msm/msm_gem.h | 13 +
>> drivers/gpu/drm/msm/msm_kms.h | 1 +
>> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 1 +
>> drivers/gpu/drm/nouveau/dispnv04/overlay.c | 13 +-
>> drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 3 +-
>> drivers/gpu/drm/nouveau/nouveau_connector.c | 7 +-
>> drivers/gpu/drm/nouveau/nouveau_drm.c | 7 +
>> drivers/gpu/drm/nouveau/nouveau_drm.h | 1 +
>> drivers/gpu/drm/nouveau/nv50_display.c | 1 +
>> drivers/gpu/drm/omapdrm/omap_crtc.c | 16 +-
>> drivers/gpu/drm/omapdrm/omap_drv.c | 12 +-
>> drivers/gpu/drm/omapdrm/omap_drv.h | 4 +-
>> drivers/gpu/drm/omapdrm/omap_plane.c | 10 +-
>> drivers/gpu/drm/qxl/qxl_display.c | 6 +-
>> drivers/gpu/drm/qxl/qxl_drv.c | 9 +
>> drivers/gpu/drm/radeon/radeon_connectors.c | 9 +-
>> drivers/gpu/drm/radeon/radeon_display.c | 2 +
>> drivers/gpu/drm/radeon/radeon_drv.c | 9 +
>> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +
>> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 7 +
>> drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c | 3 +-
>> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 12 +-
>> drivers/gpu/drm/rcar-du/rcar_du_vgacon.c | 3 +-
>> drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 6 +-
>> drivers/gpu/drm/shmobile/shmob_drm_drv.c | 7 +
>> drivers/gpu/drm/shmobile/shmob_drm_plane.c | 2 +
>> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 1 +
>> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 +
>> drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 +
>> drivers/gpu/drm/tilcdc/tilcdc_slave.c | 3 +-
>> drivers/gpu/drm/udl/udl_connector.c | 6 +-
>> drivers/gpu/drm/udl/udl_drv.c | 8 +
>> drivers/gpu/drm/udl/udl_modeset.c | 2 +
>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 7 +
>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 1 +
>> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +-
>> drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 +-
>> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 1 +
>> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 1 +
>> include/drm/drmP.h | 80 ++
>> include/drm/drm_atomic.h | 170 ++++
>> include/drm/drm_crtc.h | 238 ++++-
>> include/drm/drm_modeset_lock.h | 47 +
>> include/uapi/drm/drm_mode.h | 3 +
>> 102 files changed, 2901 insertions(+), 650 deletions(-)
>> create mode 100644 drivers/gpu/drm/drm_atomic.c
>> create mode 100644 drivers/gpu/drm/msm/msm_atomic.c
>> create mode 100644 include/drm/drm_atomic.h
>>
>> --
>> 1.9.3
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list