Re: [PATCH 26/28] drm: Move drm_display_mode an related docs into kerneldoc

[Thierry Reding <thierry.reding@gmail.com>]

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

On Fri, Dec 04, 2015 at 09:46:07AM +0100, Daniel Vetter wrote:
[...]
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
[...]
> @@ -96,17 +141,124 @@ enum drm_mode_status {
>  
>  #define DRM_MODE_FLAG_3D_MAX	DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF
>  
> +/**
> + * struct drm_display_mode - DRM kernel-internal display mode structure
[...]
> + * @crtc_hdisplay: hardware modehorizontal display size
> + * @crtc_hsync_start: hardware modehorizontal sync start
> + * @crtc_hsync_end: hardware modehorizontal sync end
> + * @crtc_htotal: hardware modehorizontal total size
> + * @crtc_hskew: hardware modehorizontal skew?!
> + * @crtc_vdisplay: hardware modevertical display size
> + * @crtc_vsync_start: hardware modevertical sync start
> + * @crtc_vsync_end: hardware modevertical sync end
> + * @crtc_vtotal: hardware modevertical total size
> + * @crtc_vscan: hardware modevertical scan?!

These are missing a space between "mode" and the description.

> + *
> + * The horizontal an vertical timings are defined per the following diagram:
> + *
> + *
> + *               Active                 Front           Sync           Back
> + *              Region                 Porch                          Porch
> + *     <-----------------------><----------------><-------------><-------------->
> + *
> + *       //////////////////////|
> + *      ////////////////////// |
> + *     //////////////////////  |..................               ................
> + *                                                _______________
> + *
> + *     <----- [hv]display ----->
> + *     <------------- [hv]sync_start ------------>
> + *     <--------------------- [hv]sync_end --------------------->
> + *     <-------------------------------- [hv]total ----------------------------->*
> + *
> + * This structure contains two copies of timings. First are the plain timings,
> + * which specify the logical mode, as it would be for a progressive 1:1 scanout
> + * at the refresh rate userspace can observe through vblank timestamps. Then
> + * there's the hardware timings, which are corrected for interlacing,
> + * double-clocking and similar things. They are provided as a convience, and can

"convenience"

> + * be appropriately computed using drm_mode_set_crtcinfo().
> + */
>  struct drm_display_mode {
> -	/* Header */
> +	/**
> +	 * @head:
> +	 *
> +	 * struct list_head for mode lists.
> +	 */
>  	struct list_head head;
> +
> +	/**
> +	 * @base:
> +	 *
> +	 * A display mode is a normal modeset object, possibly including public
> +	 * userspace id.
> +	 *
> +	 * FIXME:
> +	 *
> +	 * This can probably be removed since the entire concept of userspace
> +	 * managing modes explicitly hasn't ever landed in upstream kernel mode

"has never"

> +	 * setting support.
> +	 */
>  	struct drm_mode_object base;
>  
> +	/**
> +	 * @name:
> +	 *
> +	 * Human-readable name of the mode, filled out with drm_mode_set_name().
> +	 */
>  	char name[DRM_DISPLAY_MODE_LEN];
>  
> +	/**
> +	 * @status:
> +	 *
> +	 * Status of the mode, used to filter out modes not supported by the
> +	 * hardware. See enum &drm_mode_status.
> +	 */
>  	enum drm_mode_status status;
> +
> +	/**
> +	 * @type:
> +	 *
> +	 * A bitmask of flags, mostly about the source of a mode. Possible flags
> +	 * are:
> +	 *
> +	 *  - DRM_MODE_TYPE_BUILTIN: Meant for hard-coded modes, effectively
> +	 *    unused.
> +	 *  - DRM_MODE_TYPE_PREFERRED: Preferred mode, usually the native
> +	 *    resolution of an LCD panel. There should only be one preferred
> +	 *    mode per connector at any given time.
> +	 *  - DRM_MODE_TYPE_DRIVER: Mode created by the driver, which is all of
> +	 *    them really. Drivers must set this bit for all modes they create
> +	 *    and expose to userspace.
> +	 *
> +	 * Plus a big list of flags which shouldn't be used at all, but are
> +	 * still around since these flags are also used in the userspace ABI:
> +	 *
> +	 *  - DRM_MODE_TYPE_DEFAULT: Again a leftover, use
> +	 *    DRM_MODE_TYPE_PREFERRED instead.
> +	 *  - DRM_MODE_TYPE_CLOCK_C and DRM_MODE_TYPE_CRTC_C: Define leftovers
> +	 *    which are stuck around for hysterical raisins only. No one has an
> +	 *    idea what they where meant for. Don't use.

"were"

> +	 *  - DRM_MODE_TYPE_USERDEF: Mode defined by userspace, again a vestige
> +	 *    from older kms designs where userspace had to first add a custom
> +	 *    mode to the kernel's mode list before it could use it. Don't use.
> +	 */
>  	unsigned int type;
>  
> -	/* Proposed mode values */
> +	/**
> +	 * @clock:
> +	 *
> +	 * Pixel clock in kHz.
> +	 */
>  	int clock;		/* in kHz */
>  	int hdisplay;
>  	int hsync_start;
> @@ -118,14 +270,74 @@ struct drm_display_mode {
>  	int vsync_end;
>  	int vtotal;
>  	int vscan;

I'm thinking that these could all use "unsigned", but that's definitely
something for a separate patch.

> +	/**
> +	 * @flags:
> +	 *
> +	 * Sync and timing flags:
> +	 *
> +	 *  - DRM_MODE_FLAG_PHSYNC: horizontal sync is active high.
> +	 *  - DRM_MODE_FLAG_NHSYNC: horizontal sync is active low.
> +	 *  - DRM_MODE_FLAG_PVSYNC: vertical sync is active high.
> +	 *  - DRM_MODE_FLAG_NVSYNC: vertical sync is active low.
> +	 *  - DRM_MODE_FLAG_INTERLACE: mode is interlaced.
> +	 *  - DRM_MODE_FLAG_DBLSCAN: mode uses doublescan.
> +	 *  - DRM_MODE_FLAG_CSYNC: mode uses composite sync.
> +	 *  - DRM_MODE_FLAG_PCSYNC: composite sync is active high.
> +	 *  - DRM_MODE_FLAG_NCSYNC: composite sync is active low.
> +	 *  - DRM_MODE_FLAG_HSKEW: hskew provided (not used?).
> +	 *  - DRM_MODE_FLAG_BCAST: not used?
> +	 *  - DRM_MODE_FLAG_PIXMUX: not used?
> +	 *  - DRM_MODE_FLAG_DBLCLK: double-clocked mode.
> +	 *  - DRM_MODE_FLAG_CLKDIV2: half-clocked mode.
> +	 *
> +	 * Additionally there's flags to specify how 3D modes are packed:
> +	 *
> +	 *  - DRM_MODE_FLAG_3D_NONE: normal, non-3D mode.
> +	 *  - DRM_MODE_FLAG_3D_FRAME_PACKING: 2 full frames for left and right.
> +	 *  - DRM_MODE_FLAG_3D_FIELD_ALTERNATIVE: interleaved like fields.
> +	 *  - DRM_MODE_FLAG_3D_LINE_ALTERNATIVE: interleaved lines.
> +	 *  - DRM_MODE_FLAG_3D_SIDE_BY_SIDE_FULL: side-by-side full frames.
> +	 *  - DRM_MODE_FLAG_3D_L_DEPTH:	?
                                       ^

Stray tab.

> +	 * Note that with digital outputs like HDMI or DP there's usually a
> +	 * massive confusion between the dot clock and the signal clock at the
> +	 * bit encoding level. Especially when a 8b/10b encoding is used and the
> +	 * differences is exactly a factor of 10.

"difference"

> +	 */
> +	int crtc_clock;
>  	int crtc_hdisplay;
>  	int crtc_hblank_start;
>  	int crtc_hblank_end;
> @@ -140,12 +352,48 @@ struct drm_display_mode {
>  	int crtc_vsync_end;
>  	int crtc_vtotal;
>  
> -	/* Driver private mode info */
> +	/**
> +	 * @private:
> +	 *
> +	 * Pointer for driver private data. This can only be used for mode
> +	 * objects passed to drivers in modeset operations. It shouldn't be used
> +	 * by atomic drivers since they can store any additional data by
> +	 * subclassing state structures.
> +	 */
>  	int *private;

Off-topic: Any reasons why this is int * and not void *?

> +
> +	/**
> +	 * @private_flags:
> +	 *
> +	 * Similar to @private, but just an integer.
> +	 */
>  	int private_flags;
>  
> -	int vrefresh;		/* in Hz */
> -	int hsync;		/* in kHz */
> +	/**
> +	 * @vrefresh:
> +	 *
> +	 * Vertical refresh rate, for debug output in human readable form. Not
> +	 * used in a functional way.
> +	 *
> +	 * This value is in Hz.
> +	 */
> +	int vrefresh;
> +
> +	/**
> +	 * @hsync:
> +	 *
> +	 * Horizontal refresh rate, for debug output in human readable form. Not
> +	 * used in a functional way.
> +	 *
> +	 * This value is in kHz.
> +	 */
> +	int hsync;
> +
> +	/**
> +	 * @picture_aspect_ratio:
> +	 *
> +	 * Filed for setting the HDMI picture aspect ratio of a mode.

"Field".

Thierry

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

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

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