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@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 :-)