On Thu, Jun 7, 2012 at 2:52 AM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
On Tuesday 05 June 2012 04:31:54 Rob Clark wrote:
- - 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 ?
I think it is in a way required, because on lastclose you are restoring the state of the fbcon.. and at least some (probably all?) of the properties are adjusting the mode, so the fbcon mode would be incorrect if you didn't restore property state.
[snip]
+- 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 ?
oh, heh, you are right.. maybe I need to revisit that. Well, for now, s/drm_gem_get_pages()/shmem_read_mapping_page_gfp()/
[snip]
- 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.
yes, drop the ref they already hold
- 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 ?
I guess it isn't strictly mandatory currently (if you implement your own prime_x_to_y() fxns, although I can't think of why it would ever be anything else
[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.
hmm, I guess that is a bit of an implementation detail, since it is the dpms path where the display state gets set. But yeah, the result is setting a mode (or setting a plane) will turn things on.
[snip]
+- 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.
I guess if you can't handle an IRQ in ~16ms you are pretty screwed anyways ;-)
For UI you often care much more about latency than throughput.. ie. time from user input to results on the screen. At least this is what the user is more likely to notice. If you allow too many frames to be queued up, what the user sees on screen will be the response to their input several frames ago, which you really don't want because it feels laggy. Think about when you pick up a phone/tablet/whatever in the shop.. at least for me the first thing I do is try and scroll back and forth and see how closely the UI tracks my finger ;-)
It is perhaps a bit different from a camera or video playback which is more about throughput and less about user interactiveness. Mostly for UI you will be double buffered or sometimes triple buffered. But you want to minimize the time between doing some rendering in response to user input and when they see the result on the screen.
(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.
well, I think drm_crtc_handle_page_flip_event() would be called by driver after rendering and after vblank event.. so that is ok.
but I'm thinking we need to start keeping a drm TODO list somewhere so some of these cleanups don't get forgotten ;-)
- 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 ?
in sub-pixel case, the driver can just ignore the fractional part. The bigger problem is dealing w/ out of range.
I'm also not sure to like -EINVAL, it's pretty broad. A more precise error code might be better. Maybe -EDOM ?
this is an open issue.. it is partly addressed by Jesse Barnes's atomic mode set plane patch a while back, which would let you 'test' a configuration. Although I don't think it returned any status mask or something like this to let the driver indicate to userspace *what* it didn't like about the config.
BR, -R
[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