Re: [PATCH 19/28] drm: document drm_crtc_funcs

From: Thierry Reding <thierry.reding@gmail.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 19/28] drm: document drm_crtc_funcs
Date: Mon, 7 Dec 2015 13:25:31 +0100	[thread overview]
Message-ID: <20151207122531.GW13177@ulmo> (raw)
In-Reply-To: <1449218769-16577-20-git-send-email-daniel.vetter@ffwll.ch>

[-- Attachment #1.1: Type: text/plain, Size: 5199 bytes --]

On Fri, Dec 04, 2015 at 09:46:00AM +0100, Daniel Vetter wrote:
[...]
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index fbfe617bc492..72a7e096dd42 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -320,12 +320,6 @@ struct drm_crtc_state {
>  
>  /**
>   * struct drm_crtc_funcs - control CRTCs for a given device
> - * @cursor_set: setup the cursor
> - * @cursor_set2: setup the cursor with hotspot, superseeds @cursor_set if set
> - * @cursor_move: move the cursor
> - * @gamma_set: specify color ramp for CRTC
> - * @set_config: apply a new CRTC configuration
> - * @page_flip: initiate a page flip
>   *
>   * The drm_crtc_funcs structure is the central CRTC management structure
>   * in the DRM.  Each CRTC controls one or more connectors (note that the name
> @@ -349,15 +343,86 @@ struct drm_crtc_funcs {
>  	 */
>  	void (*reset)(struct drm_crtc *crtc);
>  
> -	/* cursor controls */
> +	/**
> +	 * @cursor_set:
> +	 *
> +	 * Update the cursor image. The cursor position is relative to the CRTC
> +	 * and can be partially or fully outside of the visible area.
> +	 *
> +	 * Note that contrary to all other KMS function the legacy cursor entry

"functions"

> +	/**
> +	 * @cursor_move:
> +	 *
> +	 * Update the cursor position. The cursor does not need to be visible
> +	 * when this hook is called.
> +	 *
> +	 * This entry point is deprecated, drivers should instead implement
> +	 * universal plane support and register a proper cursor plane using
> +	 * drm_crtc_init_with_planes().
> +	 *
> +	 * This callback is optional

Nit: Append a full-stop here and in the above ->cursor_*() callback
descriptions.

> -	/*
> -	 * Flip to the given framebuffer.  This implements the page
> -	 * flip ioctl described in drm_mode.h, specifically, the
> -	 * implementation must return immediately and block all
> -	 * rendering to the current fb until the flip has completed.
> -	 * If userspace set the event flag in the ioctl, the event
> -	 * argument will point to an event to send back when the flip
> -	 * completes, otherwise it will be NULL.
> +	/**
> +	 * @page_flip:
> +	 *
> +	 * Legacy entry point to schedule a flip to the given framebuffer.
> +	 *
> +	 * 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 (except when requested otherwise
> +	 * through the DRM_MODE_PAGE_FLIP_ASYN flag). When an application

"DRM_MODE_PAGE_FLIP_ASYNC"

> +	 * requests a page flip the DRM core verifies that the new frame buffer is large

This line looks odd because it is wider than all the others.

> +	 * 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

"->page_flip()"?

> +	 * frame buffer.
> +	 *
> +	 * The driver must wait for any pending rendering to the new framebuffer
> +	 * to complete before executing the flip. It should also wait for any
> +	 * pending rendering from other drivers if the underlying buffer is a
> +	 * shared dma-buf.
> +	 *
> +	 * An application can request to be notified when the page flip has
> +	 * completed. The drm core will supply a struct &drm_event in the event
> +	 * parameter in this case. This can be handled by the
> +	 * drm_crtc_send_vblank_event() function, which the driver should call on
> +	 * the provided event upon completion of the flip. Note that if
> +	 * the driver supports vblank signalling and timestamping the vblank
> +	 * counters and timestamps must agree with the ones returned from page
> +	 * flip events. With the current vblank helper infrastructure this can
> +	 * be achieved by holding a vblank reference while the page flip is
> +	 * pending, acquired through drm_crtc_vblank_get() and released with
> +	 * drm_crtc_vblank_put(). Drivers are free to implement their own vblank
> +	 * counter and timestamp tracking though, e.g. if they have accurate
> +	 * timestamp registers in hardware.
> +	 *
> +	 * FIXME:
> +	 *
> +	 * Up to that point drivers need to manage events themselves and can use
> +	 * even->base.list freely for that. Specifically they need to ensure
> +	 * that they don't send out page flip (or vblank) events for which the
> +	 * corresponding drm file has been closed already. The drm core
> +	 * unfortunately does not (yet) take care of that. Therefore drivers
> +	 * currently must clean up and release pending events in their
> +	 * ->preclose driver function.
> +	 *
> +	 * This callback is optional.
> +	 *
> +	 * NOTE:
> +	 *
> +	 * Very early versions of the KMS ABI mandated that the driver must
> +	 * block (but not reject) any rendering to the old framebuffer until the
> +	 * flip operation has completed and the old framebuffer is not longer

"no longer"

> +	 * visible. This requirement has been lifted, and userspace is instead
> +	 * expected to request delivery of a event and wait with recycling old

"an event"

Otherwise:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx