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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20151207/398427ab/attachment-0001.sig>

Reply via email to