[RFC] Documentation: DRM framework documentation

[Laurent Pinchart]

Hi Rob,

On Tuesday 05 June 2012 04:31:54 Rob Clark wrote:
> Hey, thanks Laurent, this was quite needed!
> 
> I apologize in advance for the html mail.. but reviewing this on the flight
> home from connect and can't figure out how to do plain text email w/
> google's offline mail client :-(

No worries. Thanks for the review.

> On Wednesday, May 30, 2012, Laurent Pinchart wrote:
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > 
> >  Documentation/drm.txt | 1265
> > 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> > 
> >  1 files changed, 1265 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/drm.txt
> > 
> > Hi everybody,
> > 
> > Here's the DRM kernel framework documentation I wrote while developing the
> > Renesas SH Mobile DRM driver. It hopefully covers most of what's needed to
> > write a simple DRM driver (although some areas are not documented, such as
> > properties or the fbdev compatibility layer).
> > 
> > I can convert the documentation to DocBook if needed and integrate it with
> > the existing "documentation stub". In that case I'm thinking of splitting
> > the DocBook documentation in two parts, userspace API documentation (that
> > someone would have to fill, any volunteer ? ;-)) and kernel API
> > documentation. Would that be fine ?
> > 
> > Last but not least, now that documentation exists (albeit in an incomplete
> > state), we need to make sure it won't get outdated too quickly. As nobody
> > will volunteer to maintain it (feel free to prove me wrong though), I'd
> > like to propose the same rule that we already follow in V4L: any patch
> > that touches the API won't get merged if it doesn't update the
> > documentation. Do you think this could work out ?
> 
> seems like a pretty reasonable idea

Let's work together on nacking patches that don't include documentation then 
:-)

> > As usual, review will be appreciated.
> > 
> > diff --git a/Documentation/drm.txt b/Documentation/drm.txt
> > new file mode 100644
> > index 0000000..4d8843d
> > --- /dev/null
> > +++ b/Documentation/drm.txt
> > @@ -0,0 +1,1265 @@

[snip]

> > +  - int (*firstopen) (struct drm_device *)
> > +  - void (*lastclose) (struct drm_device *)
> > +  - int (*open) (struct drm_device *, struct drm_file *)
> > +  - void (*preclose) (struct drm_device *, struct drm_file *)
> > +  - void (*postclose) (struct drm_device *, struct drm_file *)
> > +
> > +  Open and close handlers. None of those methods are mandatory.
> > +
> > +  The .firstopen() method is called by the DRM core when an application
> > opens
> > +  a device that has no other opened file handle. Similarly the
> > .lastclose()
> > +  method is called when the last application holding a file handle opened
> > on
> > +  the device closes it. Both methods are mostly used for UMS (User Mode
> > +  Setting) drivers to acquire and release device resources which should
> > be
> > +  done in the .load() and .unload() methods for KMS drivers.
> > +
> > +  Note that the .lastclose() method is also called at module unload time
> > or,
> > +  for hot-pluggable devices, when the device is unplugged. The
> > .firstopen()
> > +  and .lastclose() calls can thus be unbalanced.
> > +
> 
> AFAIK lastclose() should also be drm_fb_helper_restore_fbdev_mode() to
> restore fbcon mode.

Good point. I haven't documented the fbdev helper yet, I'll keep that in mind 
when updating the documentation.

> I'm also restoring crtc and plane properties to default values here, so a
> subsequent open of the device isn't inheriting some state from the previous
> user.

That's very unlike V4L2, where we explicitly require state to be kept across 
opens. Is restoring properties a DRM standard behaviour, implemented by other 
drivers as well ?

> (maybe some of this could be handled instead in core, rather than each
> driver.. could be an idea for a cleanup?)
> 
> > +  The .open() method is called every time the device is opened by an
> > +  application. Drivers can allocate per-file private data in this method
> > and
> > +  store them in the struct drm_file::driver_priv field. Note that the
> > .open()
> > +  method is called before .firstopen().
> > +
> > +  The close operation is split into .preclose() and .postclose() methods.
> > +  Drivers must stop and cleanup all per-file operations in the
> > .preclose()
> > +  method. For instance pending vertical blanking and page flip events
> > must be
> > +  cancelled. No per-file operation is allowed on the file handle after
> > +  returning from the .preclose() method.
> 
> oh, heh, I completely missed that in omapdrm, so looks like I need to do
> some cleanup around here..  I wonder if there is any sane way to make this
> simpler for the driver writer, since basically all drivers would need to do
> the same thing here..  Ie. keep track of pending page_flip events, sort of
> analogous to how the core keeps track of pending vblank events..
>
> Are there any drivers that can queue up more than one page flip per crtc at
> a time?  If no, we could track pending page flip event in 'struct drm_crtc'
> and then have a drm_crtc_handle_page_flip_events(crtc), similar
> to drm_handle_vblank_events()..

I've been thinking about that as well, as you noted in a comment at the end of 
section 9 below (more complete answer there).

[snip]

> > +6. Memory Management
> > +--------------------
> > +
> > +Modern Linux systems require large amount of graphics memory to store
> > frame
> > +buffers, textures, vertices and other graphics-related data. Given the
> > very
> > +dynamic nature of many of that data, managing graphics memory efficiently
> > is
> > +thus crucial for the graphics stack and plays a central role in the DRM
> > +infrastructure.
> > +
> > +The DRM core includes two memory managers, namely Translation Table Maps
> > (TTM)
> > +and Graphics Execution Manager (GEM). TTM was the first DRM memory
> > manager to
> > +be developed and tried to be a one-size-fits-them all solution. It
> > provides a
> > +single userspace API to accomodate the need of all hardware. This
> > resulted in
> > +a large, complex piece of code that turned out to be hard to use for
> > driver
> > +development and.
> 
> not sure if that is a spurious 'and' or you somehow lost part of that
> sentence?

Oops. I think I lost part of the sentence, but I don't remember what I wanted 
to say :-)
> 
> > +
> > +GEM started as an Intel-sponsored project in reaction to TTM's
> > complexity. Its
> > +design philosophy is completely different: instead of providing a
> > solution to
> > +every graphics memory-related problems, GEM identified common code
> > between
> > +drivers and created a support library to share it.
> > +
> > +This document describes the use of the GEM memory manager only.
> > +
> > +The GEM design approach has resulted in a memory manager that doesn't
> > provide
> > +full coverage of all (or even all common) use cass in its userspace or
> > kernel
> > +API. GEM exposes a set of standard memory-related operations to userspace
> > and
> > +a set of helper functions to drivers, and let drivers implement
> > +hardware-specific operations with their own private API.
> > +
> > +The GEM userspace API is described in http://lwn.net/Articles/283798/.
> > While
> > +slightly outdated, the document provides a good overview of the GEM API
> > +principles. Buffer allocation and read and write operations, described as
> > part
> > +of the common GEM API, are currently implemented using driver-specific
> > ioctls.
> > +
> > +GEM is data-agnostic. It manages abstract buffer objects without knowing
> > what
> > +individual buffers contain. APIs that require knowledge of buffer
> > contents or
> > +purpose, such as buffer allocation or synchronization primitives, are
> > thus
> > +outside of the scope of GEM and must be implemented using driver-specific
> > +ioctls.
> > +
> > +- GEM Initialization
> > +
> > +  Drivers that use GEM must set the DRIVER_GEM bit in the struct
> > drm_driver
> > +  driver_features field. The DRM core will then automatically initialize
> > the
> > +  GEM core before calling the .load() operation.
> > +
> > +- GEM Objects Creation
> > +
> > +  GEM splits creation of GEM objects and allocation of the memory that
> > backs
> > +  them in two distinct operations.
> > +
> > +  GEM objects are represented by an instance of struct drm_gem_object.
> > Drivers
> > +  usually need to extend GEM objects with private information and thus
> > create
> > +  a driver-specific GEM object structure type that embeds an instance of
> > +  struct drm_gem_object.
> > +
> > +  To create a GEM object, a driver allocates memory for an instance of
> > its
> > +  specific GEM object type and initializes the embedded struct
> > drm_gem_object
> > +  with a call to drm_gem_object_init(). The function takes a pointer to
> > the
> > +  DRM device, a pointer to the GEM object and the buffer object size in
> > bytes.
> > +
> > +  GEM automatically allocate anonymous pageable memory through shmfs when
> > an
> 
> s/allocate/allocates/
> 
> Also, maybe worth mentioning that actual 'struct page's are allocated
> when drm_gem_get_pages() is called..

I don't think that's in mainline yet, is it ?

> (I suppose you could say that only the storage but not the memory is
> allocated when the object is created, but maybe that confuses things more.)

I'll try to make this clear.

> > +  object is initialized. drm_gem_object_init() will create an shmfs file
> > of
> > +  the requested size and store it into the struct drm_gem_object filp
> > field.
> > +  The memory is used as either main storage for the object when the
> > graphics
> > +  hardware uses system memory directly or as a backing store otherwise.
> > +
> > +  Anonymous pageable memory allocation is not always desired, for
> > instance
> > +  when the hardware requires physically contiguous system memory as is
> > often
> > +  the case in embedded devices. Drivers can create GEM objects with no
> > shmfs
> > +  backing (called private GEM objects) by initializing them with a call
> > to
> > +  drm_gem_private_object_init() instead of drm_gem_object_init(). Storage
> > for
> > +  private GEM objects must be managed by drivers.
> > +
> > +  Drivers that do no need to extend GEM objects with private information
> > can
> 
> s/no/not/
> 
> > +  call the drm_gem_object_alloc() function to allocate and initialize a
> > struct
> > +  drm_gem_object instance. The GEM core will call the optional driver
> > +  .gem_init_object() operation after initializing the GEM object with
> > +  drm_gem_object_init().
> > +
> > +  int (*gem_init_object) (struct drm_gem_object *obj)
> > +
> > +  No alloc-and-init function exists for private GEM objects.
> > +
> > +- GEM Objects Lifetime
> > +
> > +  All GEM objects are reference-counted by the GEM core. References can
> > be
> > +  acquired and release by calling drm_gem_object_reference() and
> > +  drm_gem_object_unreference() respectively. The caller must hold the
> > +  drm_device struct_mutex lock. As a convenience, GEM provides the
> > +  drm_gem_object_reference_unlocked() and
> > +  drm_gem_object_unreference_unlocked() functions that can be called
> > without
> > +  holding the lock.
> > +
> > +  When the last reference to a GEM object is released the GEM core calls
> > the
> > +  drm_driver .gem_free_object() operation. That operation is mandatory
> > for
> > +  GEM-enabled drivers and must free the GEM object and all associated
> > +  resources.
> > +
> > +  void (*gem_free_object) (struct drm_gem_object *obj)
> > +
> > +  Drivers are responsible for freeing all GEM object resources, including
> > the
> > +  resources created by the GEM core. If an mmap offset has been created
> > for
> > +  the object (in which case drm_gem_object::map_list::map is not NULL) it
> > must
> > +  be freed by a call to drm_gem_free_mmap_offset(). The shmfs backing
> > store
> > +  must be released by calling drm_gem_object_release() (that function can
> > +  safely be called if no shmfs backing store has been created).
> > +
> > +- GEM Objects Naming
> > +
> > +  Communication between userspace and the kernel refers to GEM objects
> > using
> > +  local handles, global names or, more recently, file descriptors. All of
> > +  those are 32-bit integer values; the usual Linux kernel limits apply to
> > the
> > +  file descriptors.
> > +
> > +  GEM handles are local to a DRM file. Applications get a handle to a GEM
> > +  object through a driver-specific ioctl, and can use that handle to
> > refer
> > +  to the GEM object in other standard or driver-specific ioctls. Closing
> > a DRM
> > +  file handle frees all its GEM handles and dereferences the associated
> > GEM
> > +  objects.
> > +
> > +  To create a handle for a GEM object drivers call
> > drm_gem_handle_create().
> > +  The function takes a pointer to the DRM file and the GEM object and
> > returns
> > +  a locally unique handle. When the handle is no longer needed drivers
> > delete
> > +  it with a call to drm_gem_handle_delete(). Finally the GEM object
> > associated
> > +  with a handle can be retrieved by a call to drm_gem_object_lookup().
> 
> I suppose it is easy enough seen from the code, but might be worth
> mentioning that drm_gem_handle_create() increment's the refcnt of the gem
> obj, rather than taking ownership of the gem obj.. so in some cases the
> driver writer would want to drop that reference to the obj

Do you really mean that driver sometimes need to drop the reference taken by 
the handle. My understanding is that they sometimes need to drop the reference 
they already hold. This of course makes no difference in the code, but from a 
documentation point of view that seems less confusing to me.

> > +  GEM names are similar in purpose to handles but are not local to DRM
> > files.
> > +  They can be passed between processes to reference a GEM object
> > globally.
> > +  Names can't be used directly to refer to objects in the DRM API,
> > +  applications must convert handles to names and names to handles using
> > the
> > +  DRM_IOCTL_GEM_FLINK and DRM_IOCTL_GEM_OPEN ioctls respectively. The
> > +  conversion is handled by the DRM core without any driver-specific
> > support.
> > +
> > +  Similar to global names, GEM file descriptors are also used to share
> > GEM
> > +  objects across processes. They offer additional security: as file
> > +  descriptors must be explictly sent over UNIX domain sockets to be
> > shared
> > +  between applications, they can't be guessed like the globally unique
> > GEM
> > +  names.
> > +
> 
> +  Drivers that support GEM file descriptors, also known as the DRM PRIME
> 
> > API,
> > +  must set the DRIVER_PRIME bit in the struct drm_driver driver_features
> > field
> > +  and implement the .prime_handle_to_fd() and .prime_fd_to_handle()
> > +  operations.
> > +
> > +  int (*prime_handle_to_fd)(struct drm_device *dev,
> > +                           struct drm_file *file_priv, uint32_t handle,
> > +                           uint32_t flags, int *prime_fd)
> > +  int (*prime_fd_to_handle)(struct drm_device *dev,
> > +                           struct drm_file *file_priv, int prime_fd,
> > +                           uint32_t *handle)
> > +
> > +  Those two operations convert a GEM handle to a PRIME file descriptor
> > and
> > +  vice versa. While the PRIME file descriptors can be specific to a
> > device,
> > +  their true power come from making them shareable between multiple
> > devices
> > +  using the cross-subsystem dma-buf buffer sharing framework. For that
> > reason
> > +  drivers are advised to use the drm_gem_prime_handle_to_fd() and
> > +  drm_gem_prime_fd_to_handle() helper functions as their PRIME operations
> > +  handlers.
> > +
> 
> editorial nag: maybe move the notes about dma-buf / cross-device /
> cross-subsystem stuff above the prime_x_to_y fxn ptrs..  the real purpose
> of drm_gem_prime_x_to_y() was to allow dma-buf/prime to be used by drivers
> that do not use GEM.  Any driver using GEM in some form or another, either
> by itself or in combination w/ TTM, probably wants to use
> drm_gem_prime_x_to_y() for these two fxn ptrs and instead provide the
> .gem_prime_{import,export} fxn ptr hooks which are used by
> drm_gem_prime_x_to_y().

I've rephrased the documentation a bit, but exchanging the two parts 
completely is difficult and confusing.

> But if your buffer object handle is not a GEM handle, then you instead need
> to implement .prime_x_to_y() fxn ptrs.. I think this applies only just to
> one driver (vmwgfx?  I don't remember for sure, airlied mentioned it at one
> point).  This was the reason for the split into two sets of fxn ptrs, so
> that non GEM drivers don't get left out.

So can I state in the documentation that the PRIME FD will always be a DMABUF 
FD, or is that non-mandatory ?

> > +  The dma-buf PRIME helpers rely on the driver .gem_prime_export() and
> > +  .gem_prime_import() operations to create a dma-buf instance from a GEM
> > +  object (exporter role) and to create a GEM object from a dma-buf
> > instance
> > +  (importer role). These two operations are mandatory when using dma-buf
> > with
> > +  DRM PRIME.
> > +
> 
> only mandatory when using GEM+prime
> 
> > +- GEM Objects Mapping
> > +
> > +  Because mapping operations are fairly heavyweight GEM favours
> > read/write-
> > +  like access to buffers, implemented through driver-specific ioctls,
> > over
> > +  mapping buffers to userspace. However, when random access to the buffer
> > is
> > +  needed (to perform software rendering for instance), direct access to
> > the
> > +  object can be more efficient.
> > +
> > +  The mmap system call can't be used directly to map GEM objects, as they
> > +  don't have their own file handle. Two alternative methods currently
> > co-exist
> > +  to map GEM objects to userspace. The first method uses a
> > driver-specific
> > +  ioctl to perform the mapping operation, calling do_mmap() under the
> > hood.
> > +  This is often considered dubious, seems to be discouraged for new
> > +  GEM-enabled driver, and will thus not be described here.
> > +
> > +  The second method uses the mmap system call on the DRM file handle.
> > +
> > +  void *mmap(void *addr, size_t length, int prot, int flags, int fd,
> > +            off_t offset)
> > +
> > +  DRM identifies the GEM object to be mapped by a fake offset passed
> > through
> > +  the mmap offset argument. Prior to being mapped, a GEM object must thus
> > be
> > +  associated with a fake offset. To do so, drivers must call
> > +  drm_gem_create_mmap_offset() on the object. The function allocates a
> > fake
> > +  offset range from a pool and stores the offset divided by PAGE_SIZE in
> > +  obj->map_list.hash.key. Care must be taken not to call
> > +  drm_gem_create_mmap_offset() if a fake offset has already been
> > allocated for
> > +  the object. This can be tested by obj->map_list.map being non-NULL.
> > +
> > +  Once allocated, the fake offset value (obj->map_list.hash.key <<
> > PAGE_SHIFT)
> > +  must be passed to the application in a driver-specific way and can then
> > be
> > +  used as the mmap offset argument.
> > +
> > +  The GEM core provides a helper method drm_gem_mmap() to handle object
> > +  mapping. The method can be set directly as the mmap file operation
> > handler.
> > +  It will look up the GEM object based on the offset value and set the
> > VMA
> > +  operations to the drm_driver gem_vm_ops field. Note that drm_gem_mmap()
> > +  doesn't map memory to userspace, but relies on the driver-provided
> > fault
> > +  handler to map pages individually.
> > +
> > +  To use drm_gem_mmap(), drivers must fill the struct drm_driver
> > gem_vm_ops
> > +  field with a pointer to VM operations.
> > +
> > +  struct vm_operations_struct *gem_vm_ops
> > +
> > +  struct vm_operations_struct {
> > +       void (*open)(struct vm_area_struct * area);
> > +       void (*close)(struct vm_area_struct * area);
> > +       int (*fault)(struct vm_area_struct *vma, struct vm_fault *vmf);
> > +  }
> > +
> > +  The open and close operations must update the GEM object reference
> > count.
> > +  Drivers can use the drm_gem_vm_open() and drm_gem_vm_close() helper
> > +  functions directly as open and close handlers.
> > +
> > +  The fault operation handler is responsible for mapping individual pages
> > to
> > +  userspace when a page fault occurs. Depending on the memory allocation
> > +  scheme, drivers can allocate pages at fault time, or can decide to
> > allocate
> > +  memory for the GEM object at the time the object is created.
> > +
> > +  Drivers that want to map the GEM object upfront instead of handling
> > page
> > +  faults can implement their own mmap file operation handler.
> > +
> > +- Dumb GEM Objects
> > +
> > +  The GEM API doesn't standardize GEM objects creation and leaves it to
> > +  driver-specific ioctls. While not an issue for full-fledged graphics
> > stacks
> > +  that include device-specific userspace components (in libdrm for
> > instance),
> > +  this limit makes DRM-based early boot graphics unnecessarily complex.
> > +
> > +  Dumb GEM objects partly alleviate the problem by providing a standard
> > API to
> > +  create dumb buffers suitable for scanout, which can then be used to
> > create
> > +  KMS frame buffers.
> > +
> > +  To support dumb GEM objects drivers must implement the .dumb_create(),
> > +  .dumb_destroy() and .dumb_map_offset() operations.
> > +
> > +  int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
> > +                    struct drm_mode_create_dumb *args)
> > +
> > +  The .dumb_create() operation creates a GEM object suitable for scanout
> > based
> > +  on the width, height and depth from the struct drm_mode_create_dumb
> > +  argument. It fills the argument's handle, pitch and size fields with a
> > +  handle for the newly created GEM object and its line pitch and size in
> > +  bytes.
> > +
> > +  int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
> > +                     uint32_t handle)
> > +
> > +  The .dumb_destroy() operation destroys a dumb GEM object created by
> > +  .dumb_create().
> > +
> > +  int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device
> > *dev,
> > +                        uint32_t handle, uint64_t *offset)
> > +
> > +  The .dumb_map_offset() operation associates an mmap fake offset with
> > the GEM
> > +  object given by the handle and returns it. Drivers must use the
> > +  drm_gem_create_mmap_offset() function to associate the fake offset as
> > +  described in the GEM Objects Mapping section.

[snip]

> > +9. CRTC Operations
> > +-------------------
> > +
> > +- void (*gamma_set)(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b,
> > +                   uint32_t start, uint32_t size)
> > +
> > +  Apply a gamma table to the device. The operation is optional.
> > +
> > +- void (*destroy)(struct drm_crtc *crtc)
> > +
> > +  Destroy the CRTC when not needed anymore. See the KMS cleanup section.
> > +
> > +- int (*set_config)(struct drm_mode_set *set)
> > +
> > +  Apply a new CRTC configuration to the device. The configuration
> > specifies a
> > +  CRTC, a frame buffer to scan out from, a (x,y) position in the frame
> > buffer,
> > +  a display mode and an array of connectors to drive with the CRTC if
> > +  possible.
> > +
> > +  If the frame buffer specified in the configuration is NULL, the driver
> > must
> > +  detach all encoders connected to the CRTC and all connectors attached
> > to
> > +  those encoders and disable them.
> > +
> > +  This operation is called with the mode config lock held.
> > +
> > +  (FIXME: How should set_config interact with DPMS? If the CRTC is
> > suspended,
> > +  should it be resumed?)
> 
> DPMS calls from userspace are made on the connector, and internally
> propagated to encoder/crtc/etc via drm_helper_connector_dpms().  When it is
> resumed, it should (IIRC) keep it's old settings.

Then what about the following ? :-)

static void omap_crtc_commit(struct drm_crtc *crtc)
{
        struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
        DBG("%s", omap_crtc->name);
        omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
}

The CRTC is resumed when the mode is set.

> > +
> > +  The mid-layer provides a drm_crtc_helper_set_config() helper function.
> > The
> > +  helper will try to locate the best encoder for each connector by
> > calling the
> > +  connector .best_encoder helper operation. That operation is mandatory
> > and
> > +  must return a pointer to the best encoder for the connector. For
> > devices
> > +  that map connectors to encoders 1:1, the function simply returns the
> > pointer
> > +  to the associated encoder.
> > +
> > +  After locating the appropriate encoders, the helper function will call
> > the
> > +  mandatory mode_fixup encoder and CRTC helper operations.
> > +
> > +  - bool (*mode_fixup)(struct drm_encoder *encoder,
> > +                      const struct drm_display_mode *mode,
> > +                      struct drm_display_mode *adjusted_mode)
> > +  - bool (*mode_fixup)(struct drm_crtc *crtc,
> > +                      const struct drm_display_mode *mode,
> > +                      struct drm_display_mode *adjusted_mode)
> > +
> > +    (FIXME: Should the mode argument be const? The i915 driver modifies
> > +     mode->clock in intel_dp_mode_fixup().)
> > +
> > +    Let encoders and CRTC adjust the requested mode or reject it
> > completely.
> > +    Those operations return true if the mode is accepted (possibly after
> > being
> > +    adjusted) or false if it is rejected.
> > +
> > +    The mode_fixup operation should reject the mode if it can't
> > reasonably use
> > +    it. The definition of "reasonable" is currently fuzzy in this
> > context. One
> > +    possible behaviour would be to set the adjusted mode to the panel
> > timings
> > +    when a fixed-mode panel is used with hardware capable of scaling.
> > Anothe
> 
> s/Anothe/Another/
> 
> > +    behaviour would be to accept any input mode and adjust it to the
> > closest
> > +    mode supported by the hardware (FIXME: This needs to be clarified).
> > +
> > +  If the new configuration after mode adjustment is identical to the
> > current
> > +  configuration the helper function will return without performing any
> > other
> > +  operation.
> > +
> > +  If the adjusted mode is identical to the current mode but changes to
> > the
> > +  frame buffer need to be applied, the drm_crtc_helper_set_config()
> > function
> > +  will call the CRTC .mode_set_base() helper operation.
> > +
> > +  - int (*mode_set_base)(struct drm_crtc *crtc, int x, int y,
> > +                        struct drm_framebuffer *old_fb)
> > +
> > +    Move the CRTC on the current frame buffer (stored in crtc->fb) to
> > position
> > +    (x,y). Any of the frame buffer, x position or y position may have
> > been
> > +    modified.
> > +
> > +    This helper operation is optional. If not provided, the
> > +    drm_crtc_helper_set_config() function will fall back to the
> > .mode_set()
> > +    helper operation.
> > +
> > +    (FIXME: Why are x and y passed as arguments, as they can be accessed
> > +    through crtc->x and crtc->y?)
> > +
> > +  If the adjusted mode differs from the current mode, or if the
> > +  .mode_set_base() helper operation is not provided, the helper function
> > +  performs a full mode set sequence by calling the following mandatory
> > +  CRTC and encoder operations in order.
> > +
> > +  - void (*prepare)(struct drm_encoder *encoder)
> > +  - void (*prepare)(struct drm_crtc *crtc)
> > +
> > +    Those operations are called after validating the requested mode.
> > Drivers
> > +    use them to perform device-specific operations required before
> > setting the
> > +    new mode.
> > +
> > +  - int (*mode_set)(struct drm_crtc *crtc, struct drm_display_mode *mode,
> > +                   struct drm_display_mode *adjusted_mode, int x, int y,
> > +                   struct drm_framebuffer *old_fb)
> > +  - void (*mode_set)(struct drm_encoder *encoder,
> > +                    struct drm_display_mode *mode,
> > +                    struct drm_display_mode *adjusted_mode)
> > +
> > +    Those operations set the new mode. Depending on the device
> > requirements,
> > +    the mode can be stored internally by the driver and applied in the
> > commit
> > +    operations, or programmed to the hardware here.
> > +
> > +    The crtc::mode_set operation returns 0 on success or a negative error
> > code
> > +    if an error occurs. The encoder::mode_set operation isn't allowed to
> > fail.
> > +
> > +  - void (*commit)(struct drm_crtc *crtc)
> > +  - void (*commit)(struct drm_encoder *encoder)
> > +
> > +    Those operations are called after setting the new mode. Upon return
> > the
> > +    device must use the new mode and be fully operational.
> > +
> > +- int (*page_flip)(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > +                  struct drm_pending_vblank_event *event)
> > +
> > +  Schedule a page flip to the given frame buffer for the CRTC. This
> > operation
> > +  is called with the mode config mutex held.
> > +
> > +  Page flipping is a synchronization mechanism that replaces the frame
> > buffer
> > +  being scanned out by the CRTC with a new frame buffer during vertical
> > +  blanking, avoiding tearing. When an application requests a page flip
> > the DRM
> > +  core verifies that the new frame buffer is large enough to be scanned
> > out by
> > +  the CRTC in the currently configured mode and then calls the CRTC
> > +  .page_flip() operation with a pointer to the new frame buffer.
> > +
> > +  The .page_flip() operation schedules a page flip. Once any pending
> > rendering
> > +  targetting the new frame buffer has completed, the CRTC will be
> > reprogrammed
> > +  to display that frame buffer after the next vertical refresh. The
> > operation
> > +  must return immediately without waiting for rendering or page flip to
> > +  complete and must block any new rendering to the frame buffer until the
> > page
> > +  flip completes.
> > +
> > +  If a page flip is already pending, the .page_flip() operation must
> > return
> > +  -EBUSY.
> > +
> > +  (FIXME: Should DRM allow queueing multiple page flips?)
> 
> Queuing could be done in userspace (which gets a page_flip event when flip
> completes).. this at least simplifies the driver somewhat.  And in cases
> where UI interactivity is required you really don't want to let rendering
> get too far ahead of scanout or the user will see this as laggy display.
>  So I don't see a particularly good reason to support queuing.

It could be useful to make up for system latency, but I suppose that if your 
system load peaks prevent you from handling the page flip fast enough compared 
to the vertical frequency, you're screwed anyway.

> (Also, makes it easier to move page_flip event handling to the core if you
> can have only at most one outstanding page flip.)
> 
> > +
> > +  To synchronize page flip to vertical blanking the driver will likely
> > need to
> > +  enable vertical blanking interrupts. It should call drm_vblank_get()
> > for
> > +  that purpose, and call drm_vblank_put() after the page flip completes.
> > +
> > +  If the application has requested to be notified when page flip
> > completes the
> > +  .page_flip() operation will be called with a non-NULL event argument
> > +  pointing to a drm_pending_vblank_event instance. Upon page flip
> > completion
> > +  the driver must fill the event::event sequence, tv_sec and tv_usec
> > fields
> > +  with the associated vertical blanking count and timestamp, add the
> > event to
> > +  the drm_file list of events to be signaled, and wake up any waiting
> > process.
> > +  This can be performed with
> > +
> > +       struct timeval now;
> > +
> > +       event->event.sequence = drm_vblank_count_and_time(..., &now);
> > +       event->event.tv_sec = now.tv_sec;
> > +       event->event.tv_usec = now.tv_usec;
> > +
> > +       spin_lock_irqsave(&dev->event_lock, flags);
> > +       list_add_tail(&event->base.link,
> > &event->base.file_priv->event_list);
> > +       wake_up_interruptible(&event->base.file_priv->event_wait);
> > +       spin_unlock_irqrestore(&dev->event_lock, flags);
> > +
> > +  (FIXME: Could drivers that don't need to wait for rendering to complete
> > just
> > +   add the event to dev->vblank_event_list and let the DRM core handle
> > +   everything, as for "normal" vertical blanking events?)
> 
> Or just drm_crtc_handle_page_flip_event() which would also let us simplify
> some of the cleanup when userspace closes the device, as mentioned earlier
> ;-)  I think when I have a few spare minutes I'll try this.

I think it's a good idea, I'm waiting for your patch ;-) For drivers that can 
process page flips immediately that should not be difficult. For drivers that 
must wait for rendering to complete before processing the page flip, this 
might be more difficult.

> > +
> > +  While waiting for the page flip to complete, the event->base.link list
> > head
> > +  can be used freely by the driver to store the pending event in a
> > +  driver-specific list.
> > +
> > +  If the file handle is closed before the event is signaled, drivers must
> > take
> > +  care to destroy the event in their .preclose() operation (and, if
> > needed,
> > +  call drm_vblank_put()).
> > +
> > +
> > +10. Plane Operations
> > +-------------------
> > +
> > +- int (*update_plane)(struct drm_plane *plane, struct drm_crtc *crtc,
> > +                     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> > +                     unsigned int crtc_w, unsigned int crtc_h,
> > +                     uint32_t src_x, uint32_t src_y,
> > +                     uint32_t src_w, uint32_t src_h)
> > +
> > +  Enable and configure the plane to use the given CRTC and frame buffer.
> > +
> > +  The source rectangle in frame buffer memory coordinates is given by the
> > +  src_x, src_y, src_w and src_h parameters (as 16.16 fixed point values).
> > +  Devices that don't support subpixel plane coordinates can ignore the
> > +  fractional part.
> > +
> > +  The destination rectangle in CRTC coordinates is given by the crtc_x,
> > +  crtc_y, crtc_w and crtc_h parameters (as integer values). Devices scale
> > +  the source rectangle to the destination rectangle. If scaling is not
> > +  supported, the src_w and src_h values can be ignored.
> 
> hmm, ignored seems bad.. maybe driver should -EINVAL?

What if scaling is supported but src_w and src_h values are out of range ? Or 
if they specify sub-pixel coordinates while the hardware only supports pixel 
coordinates ? Should we return an error in all those cases, or can some of 
them be ignored ?

I'm also not sure to like -EINVAL, it's pretty broad. A more precise error 
code might be better. Maybe -EDOM ?

> > +- int (*disable_plane)(struct drm_plane *plane)
> > +
> > +  Disable the plane. The DRM core calls this method in response to a
> > +  DRM_IOCTL_MODE_SETPLANE ioctl call with the frame buffer ID set to 0.
> > +  Disabled planes must not be processed by the CRTC.
> > +
> > +- void (*destroy)(struct drm_plane *plane)
> > +
> > +  Destroy the plane when not needed anymore. See the KMS cleanup section.

[snip]

> > +13. TODO
> > +--------
> > +
> > +- Document the struct_mutex catch-all lock
> > +- Document connector properties
> > +
> > +- crtc and encoder dpms helper operations are only mandatory if the
> > disable
> > +  operation isn't provided.
> > +- crtc and connector .save and .restore operations are only used
> > internally in
> > +  drivers, should they be removed from the core?
> > +- encoder mid-layer .save and .restore operations are only used
> > internally in
> > +  drivers, should they be removed from the core?
> > +- encoder mid-layer .detect operation is only used internally in drivers,
> > +  should it be removed from the core?
> > +
> > +- KMS drivers must call drm_vblank_pre_modeset() and
> > drm_vblank_post_modeset()
> > +  around mode setting. Should this be done in the DRM core?
> > +- vblank_disable_allowed is set to 1 in the first
> > drm_vblank_post_modeset()
> > +  call and never set back to 0. It seems to be safe to permanently set it
> > to 1
> > +  in drm_vblank_init() for KMS driver, and it might be safe for UMS
> > drivers as
> > +  well. This should be investigated.
> 
> cool, thx.. I may have missed some things since I was trying to hurry up
> and finish reviewing before my battery died ;-)

Thanks for the review. If you get a bit more time, you could give me your 
opinion on the questions in the TODO section :-)

-- 
Regards,

Laurent Pinchart