[RFC PATCH 00/37] Modesetting for atomic modesetting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Well, that escalated quickly.

I've been looking at adding modesetting support to the atomic ioctl, and this
is what I've ended up with so far. It's definitely not perfect, but given how
out of hand it's got at the moment, I wanted to send this out as an RFC before
I spent too long polishing it up.

This series ends up touching pretty much all the drivers, by virtue of turning
crtc->mode (in particular) into both a const and a pointer.

The reasoning behind this is that currently, we just treat modes as unboxed
data to shovel around. With atomic modesetting, and user-supplied modes, we
want to do better here. Whilst sketching out userspace/compositor
requirements, I came up with the following invariants, which necessitated
turning modes into a refcounted, immutable, type:
  - modes can come from one of three sources: connector list, current
    mode, userspace-created
  - as far as possible, modes should be relateable to their source, e.g. if
    Plymouth pulls a particular mode from the encoder, and you pick up on that
    mode as part of current configuration during handover, you should be able
    to work backwards to where Plymouth sourced it, i.e. the encoder list
  - userspace should be able to tell the current status by looking at the IDs
    returned by property queries, rather than having to pull the entire mode
    out: if we make them do that, they won't bother minimising the deltas and
    will just dump the full state in every time, and that makes debugging the
    entire thing that much harder
  - setting a mode current should hold a reference for as long as it's current,
    e.g. if you create a magic user-supplied mode, set that on the CRTC and
    then terminate the connection, the mode should still live on for handover
    purposes
  - persistence of system-supplied (from-connector or from-CRTC) modes is not
    important beyond their natural lifetime: if you pick a mode up from a
    connector's probed list and then that connector disappears, setting that
    mode is unlikely to be what you want, so failure because the mode no
    longer exists is entirely acceptable


The patchset so far breaks down into a few natural sets:


Hopefully-uncontroversial fixes (2).

  drm: mode: Fix typo in kerneldoc
  drm: fb_helper: Simplify exit condition
  drm: mode: Allow NULL modes for equality check


Stop drivers changing crtc->mode (7) - some drivers were directly bashing
crtc->mode at various points they shouldn't, which destroys our immutability
guarantees, and is also generally bad form. Some of them didn't have an
obvious way to pass adjusted_mode through, so I've resurrected the earlier
usage of hwmode, to allow them to store and retrieve adjusted_mode from there.

  drm: crtc_helper: Update hwmode before mode_set call
  drm: Exynos: Remove mode validation inside mode_fixup
  drm: Exynos: Use hwmode for adjusted_mode
  drm: sti: Use crtc->hwmode for adjusted mode
  drm: ast: Split register set from get_vbios_mode_info
  drm: ast: Split mode adjustment from get_vbios_mode_info
  drm: armada: Use crtc->hwmode for adjusted mode


Make crtc->mode const (9) - this is what I used to discover the previous
7, and is perhaps a good idea anyway. Currently this causes a bunch of
warnings, which I'll get to below; the vmwgfx conversion is also totally
botched and needs re-doing. i915 is probably already broken by this
point.

  drm: bridge: Constify mode parameters
  drm: encoder-slave: Constify mode parameters
  drm: connector-helper: Constify mode_valid parameter
  drm: connector-helper: Constify mode_set parameters
  drm: crtc-helper: Constify mode_set parameters
  drm: fb_helper: Constify modeset mode member
  DRM: Atomic: Use pointer for mode in CRTC state
  DRM: CRTC: Use pointer for display mode
  DRM: Constify crtc->mode pointer


Modes as first-class objects (9) - make modes refcountable; by this stage
they're already immutable.

  DRM: mode: Rename and combine drm_crtc_convert_umode
  DRM: mode: Un-staticise drm_mode_new_from_umode
  DRM: mode: Un-staticise drm_crtc_convert_to_umode
  drm: mode: Cache userspace mode representation
  drm: mode: Use cached usermode representation
  drm: atomic: Expose CRTC active property
  drm: atomic: Allow setting CRTC active property
  drm: mode: Add kref to modes
  drm: mode: Add drm_mode_reference


Use mode references throughout (4) - duplicating modes invalidates a bunch of
the requirements listed above. Go through and push the core to reference every
mode it gets, rather than duplicating/copying it. Applying this series causes a
boatload of constness warnings due to reference counting, which I intend to fix.

  drm: fb_helper: Reference, not duplicate, modes
  drm: crtc_helper: Reference, not duplicate, modes
  drm: atomic_helper: Reference, not duplicate, modes
  drm: i915/tegra: atomic: Reference, not duplicate, modes


Modes as properties (4) - expose the mode as a property, and also allow users
to create their own mode-properties. The getblob one I'm probably most iffy
about: I'd previously created a drm_property_blob underneath the mode, however
juggling the translation between our two object IDs (mode and blob) was much
more error-prone. It also meant we lost information just looking at the raw
property stream, and had to add type/priv pointers to drm_property_blob. In
the end I settled on just keeping a single drm_display_mode object, and
overloading getblob to also be able to pull the userspace representation of
modes.

  drm: mode: Allow userspace to fetch mode as blob
  drm: atomic: Add MODE_ID property
  drm: property: Allow non-global blob properties
  drm: mode: Add user object-creation ioctl


Misc (1) - hi Thierry. This could probably just get squashed into the commit
where we make crtc->mode a pointer though ...

  Tegra: SOR: Don't always assume a valid mode


If this doesn't seem like a totally insane approach, I'll continue on with it.
I think it's going to be really hard to develop a credible userspace interface
without the invariance guarantees above - without that, userspace is just going
to dump (hopefully) its entire state on us every frame, which is extra overhead
to process, and also makes it more difficult to reason about the deltas it's
providing us; debugging that is a pain.

This is in a pretty rough state right now. It's enough to get it working on
Tegra (well, once hacked to claim DRIVER_ATOMIC), and is compile-tested on
everything else I could enable on ARM. I haven't been able to run it on i915 or
Exynos as yet, thanks to unrelated breakage in linux-next. I'm planning to sort
that out this week, and get those two actually working.

Don't look too closely at the exact usage of reference counting: we're
definitely leaking quite a few references, though those are easily enough fixed.

Also, the constness warnings need to be fixed up, though it hits a moderate
impasse: having userspace-exposed mode objects be more obviously immutable is
a good thing, however drm_display_mode is also used for internal driver code,
and for generating modes in the first place; these shouldn't be immutable. My
rough thought around that would be to split modes like so:

struct drm_mode_timings {
	hdisplay, vdisplay, ...;
};

struct drm_mode_internal {
	struct drm_mode_timings base_timings;
	struct drm_mode_timings crtc_timings;
};

struct drm_mode_user {
	struct drm_mode_object base;
	struct kref refcount;
	struct drm_mode_timings base_timings;
};

struct drm_mode *drm_mode_from_internal(const struct drm_mode_internal *src);
struct drm_mode *drm_mode_from_user(const struct drm_mode_modeinfo *src);

That would be a pretty huge Coccinelle run though, and certainly more invasive
than what I've done right now, so whilst it's a good idea, I didn't want to go
off and do it if it was just going to be immediately shot down.

So ... anyone feel like casting an eye over it?

Cheers,
Daniel

 drivers/gpu/drm/armada/armada_crtc.c               |  21 +-
 drivers/gpu/drm/armada/armada_output.c             |   2 +-
 drivers/gpu/drm/armada/armada_output.h             |   2 +-
 drivers/gpu/drm/armada/armada_overlay.c            |   4 +-
 drivers/gpu/drm/ast/ast_drv.h                      |   1 +
 drivers/gpu/drm/ast/ast_mode.c                     | 174 ++++++-----
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c       |   2 +-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |   6 +-
 drivers/gpu/drm/bochs/bochs.h                      |   2 +-
 drivers/gpu/drm/bochs/bochs_hw.c                   |   2 +-
 drivers/gpu/drm/bochs/bochs_kms.c                  |  10 +-
 drivers/gpu/drm/bridge/dw_hdmi.c                   |   9 +-
 drivers/gpu/drm/cirrus/cirrus_mode.c               |   8 +-
 drivers/gpu/drm/drm_atomic.c                       |  56 +++-
 drivers/gpu/drm/drm_atomic_helper.c                |  67 +++--
 drivers/gpu/drm/drm_crtc.c                         | 332 ++++++++++++++-------
 drivers/gpu/drm/drm_crtc_helper.c                  |  65 ++--
 drivers/gpu/drm/drm_encoder_slave.c                |   4 +-
 drivers/gpu/drm/drm_fb_helper.c                    |  35 ++-
 drivers/gpu/drm/drm_fops.c                         |   7 +-
 drivers/gpu/drm/drm_ioctl.c                        |   2 +
 drivers/gpu/drm/drm_modes.c                        | 147 ++++++++-
 drivers/gpu/drm/drm_plane_helper.c                 |   6 +-
 drivers/gpu/drm/exynos/exynos7_drm_decon.c         |   4 +-
 drivers/gpu/drm/exynos/exynos_drm_connector.c      |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_crtc.c           |  14 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.h            |   4 +-
 drivers/gpu/drm/exynos/exynos_drm_dsi.c            |   4 +-
 drivers/gpu/drm/exynos/exynos_drm_encoder.c        |   4 +-
 drivers/gpu/drm/exynos/exynos_drm_fimd.c           |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_plane.c          |  14 +-
 drivers/gpu/drm/exynos/exynos_hdmi.c               |  49 +--
 drivers/gpu/drm/exynos/exynos_mixer.c              |   2 +-
 drivers/gpu/drm/exynos/exynos_mixer.h              |   2 +-
 drivers/gpu/drm/gma500/cdv_intel_crt.c             |   6 +-
 drivers/gpu/drm/gma500/cdv_intel_display.c         |   4 +-
 drivers/gpu/drm/gma500/cdv_intel_dp.c              |   7 +-
 drivers/gpu/drm/gma500/cdv_intel_hdmi.c            |   6 +-
 drivers/gpu/drm/gma500/cdv_intel_lvds.c            |   6 +-
 drivers/gpu/drm/gma500/mdfld_dsi_dpi.c             |   4 +-
 drivers/gpu/drm/gma500/mdfld_dsi_dpi.h             |   4 +-
 drivers/gpu/drm/gma500/mdfld_dsi_output.c          |   2 +-
 drivers/gpu/drm/gma500/mdfld_intel_display.c       |   8 +-
 drivers/gpu/drm/gma500/oaktrail.h                  |   6 +-
 drivers/gpu/drm/gma500/oaktrail_crtc.c             |   4 +-
 drivers/gpu/drm/gma500/oaktrail_hdmi.c             |  10 +-
 drivers/gpu/drm/gma500/oaktrail_lvds.c             |   4 +-
 drivers/gpu/drm/gma500/psb_intel_display.c         |   4 +-
 drivers/gpu/drm/gma500/psb_intel_lvds.c            |   6 +-
 drivers/gpu/drm/gma500/psb_intel_sdvo.c            |   6 +-
 drivers/gpu/drm/i2c/adv7511.c                      |   6 +-
 drivers/gpu/drm/i2c/ch7006_drv.c                   |   6 +-
 drivers/gpu/drm/i2c/sil164_drv.c                   |   8 +-
 drivers/gpu/drm/i2c/tda998x_drv.c                  |  24 +-
 drivers/gpu/drm/i915/intel_atomic.c                |  12 +-
 drivers/gpu/drm/i915/intel_crt.c                   |   2 +-
 drivers/gpu/drm/i915/intel_dp.c                    |   2 +-
 drivers/gpu/drm/i915/intel_dp_mst.c                |   2 +-
 drivers/gpu/drm/i915/intel_dsi.c                   |   2 +-
 drivers/gpu/drm/i915/intel_dvo.c                   |   2 +-
 drivers/gpu/drm/i915/intel_fbdev.c                 |   4 +
 drivers/gpu/drm/i915/intel_hdmi.c                  |   2 +-
 drivers/gpu/drm/i915/intel_lvds.c                  |   2 +-
 drivers/gpu/drm/i915/intel_sdvo.c                  |   2 +-
 drivers/gpu/drm/i915/intel_tv.c                    |   2 +-
 drivers/gpu/drm/imx/dw_hdmi-imx.c                  |   8 +-
 drivers/gpu/drm/imx/imx-ldb.c                      |   4 +-
 drivers/gpu/drm/imx/imx-tve.c                      |   6 +-
 drivers/gpu/drm/imx/ipuv3-crtc.c                   |   4 +-
 drivers/gpu/drm/imx/ipuv3-plane.c                  |   2 +-
 drivers/gpu/drm/imx/ipuv3-plane.h                  |   2 +-
 drivers/gpu/drm/imx/parallel-display.c             |   4 +-
 drivers/gpu/drm/mgag200/mgag200_mode.c             |  12 +-
 drivers/gpu/drm/msm/edp/edp_bridge.c               |   4 +-
 drivers/gpu/drm/msm/edp/edp_connector.c            |   2 +-
 drivers/gpu/drm/msm/edp/edp_ctrl.c                 |   2 +-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c             |   7 +-
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c          |   2 +-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_dtv_encoder.c    |   6 +-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lcdc_encoder.c   |   6 +-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c |   2 +-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c           |   4 +-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c        |   6 +-
 drivers/gpu/drm/nouveau/dispnv04/crtc.c            |  17 +-
 drivers/gpu/drm/nouveau/dispnv04/cursor.c          |   2 +-
 drivers/gpu/drm/nouveau/dispnv04/dac.c             |   4 +-
 drivers/gpu/drm/nouveau/dispnv04/dfp.c             |   4 +-
 drivers/gpu/drm/nouveau/dispnv04/tvmodesnv17.c     |   4 +-
 drivers/gpu/drm/nouveau/dispnv04/tvnv04.c          |   4 +-
 drivers/gpu/drm/nouveau/dispnv04/tvnv17.c          |   8 +-
 drivers/gpu/drm/nouveau/nouveau_connector.c        |   4 +-
 drivers/gpu/drm/nouveau/nv50_display.c             |  29 +-
 drivers/gpu/drm/omapdrm/omap_connector.c           |   9 +-
 drivers/gpu/drm/omapdrm/omap_crtc.c                |  10 +-
 drivers/gpu/drm/omapdrm/omap_drv.h                 |   2 +-
 drivers/gpu/drm/omapdrm/omap_encoder.c             |   4 +-
 drivers/gpu/drm/qxl/qxl_display.c                  |  16 +-
 drivers/gpu/drm/radeon/atombios_crtc.c             |  27 +-
 drivers/gpu/drm/radeon/atombios_dp.c               |   2 +-
 drivers/gpu/drm/radeon/atombios_encoders.c         |  10 +-
 drivers/gpu/drm/radeon/cik.c                       |   8 +-
 drivers/gpu/drm/radeon/evergreen.c                 |  14 +-
 drivers/gpu/drm/radeon/r100.c                      |   8 +-
 drivers/gpu/drm/radeon/radeon_asic.h               |   4 +-
 drivers/gpu/drm/radeon/radeon_audio.c              |  20 +-
 drivers/gpu/drm/radeon/radeon_audio.h              |   7 +-
 drivers/gpu/drm/radeon/radeon_connectors.c         |  12 +-
 drivers/gpu/drm/radeon/radeon_cursor.c             |   4 +-
 drivers/gpu/drm/radeon/radeon_display.c            |  12 +-
 drivers/gpu/drm/radeon/radeon_legacy_crtc.c        |  14 +-
 drivers/gpu/drm/radeon/radeon_legacy_encoders.c    |  20 +-
 drivers/gpu/drm/radeon/radeon_legacy_tv.c          |   4 +-
 drivers/gpu/drm/radeon/radeon_mode.h               |  10 +-
 drivers/gpu/drm/radeon/rs600.c                     |   8 +-
 drivers/gpu/drm/radeon/rs690.c                     |  28 +-
 drivers/gpu/drm/radeon/rs780_dpm.c                 |   4 +-
 drivers/gpu/drm/radeon/rv515.c                     |  32 +-
 drivers/gpu/drm/radeon/si.c                        |  14 +-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |   2 +-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c          |   6 +-
 drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c          |   2 +-
 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c          |   6 +-
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c          |   2 +-
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        |   6 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  12 +-
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c          |  10 +-
 drivers/gpu/drm/sti/sti_cursor.c                   |   4 +-
 drivers/gpu/drm/sti/sti_drm_crtc.c                 |  19 +-
 drivers/gpu/drm/sti/sti_drm_plane.c                |   2 +-
 drivers/gpu/drm/sti/sti_dvo.c                      |   6 +-
 drivers/gpu/drm/sti/sti_gdp.c                      |   2 +-
 drivers/gpu/drm/sti/sti_hda.c                      |   8 +-
 drivers/gpu/drm/sti/sti_hdmi.c                     |   6 +-
 drivers/gpu/drm/sti/sti_layer.c                    |   2 +-
 drivers/gpu/drm/sti/sti_layer.h                    |   4 +-
 drivers/gpu/drm/sti/sti_mixer.c                    |   4 +-
 drivers/gpu/drm/sti/sti_mixer.h                    |   2 +-
 drivers/gpu/drm/sti/sti_tvout.c                    |   4 +-
 drivers/gpu/drm/sti/sti_vid.c                      |   2 +-
 drivers/gpu/drm/tegra/dc.c                         |   7 +
 drivers/gpu/drm/tegra/dsi.c                        |  10 +-
 drivers/gpu/drm/tegra/hdmi.c                       |  10 +-
 drivers/gpu/drm/tegra/rgb.c                        |   8 +-
 drivers/gpu/drm/tegra/sor.c                        |  11 +-
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c               |  17 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   3 +-
 drivers/gpu/drm/tilcdc/tilcdc_panel.c              |   6 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c              |   3 +-
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c             |   6 +-
 drivers/gpu/drm/udl/udl_connector.c                |   2 +-
 drivers/gpu/drm/udl/udl_encoder.c                  |   4 +-
 drivers/gpu/drm/udl/udl_modeset.c                  |   7 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c                |  32 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c                |  12 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c               |  10 +-
 include/drm/bridge/dw_hdmi.h                       |   2 +-
 include/drm/drmP.h                                 |   6 +
 include/drm/drm_crtc.h                             |  25 +-
 include/drm/drm_crtc_helper.h                      |  20 +-
 include/drm/drm_encoder_slave.h                    |  10 +-
 include/drm/drm_fb_helper.h                        |   3 +
 include/drm/drm_modes.h                            |  10 +
 include/uapi/drm/drm.h                             |   2 +
 include/uapi/drm/drm_mode.h                        |  22 ++
 164 files changed, 1253 insertions(+), 798 deletions(-)

-- 
2.3.2

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux