Re: [PATCH 1/6] drm/doc: Update kerneldoc for drm_crtc.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Daniel,

Some review comments:

On Tue, May 31, 2016 at 11:11:10PM +0200, Daniel Vetter wrote:
> Apparently not everyone has been super dutiful with updating this
> stuff.
> 
> I still decided to leave out the documentation for all the *_property
> pointers we have in drm_mode_config.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
>  include/drm/drm_crtc.h | 80 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 67 insertions(+), 13 deletions(-)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c0edd87fa0ee..aa3f7ea41b1b 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -315,6 +315,7 @@ struct drm_plane_helper_funcs;
>   * 	update to ensure framebuffer cleanup isn't done too early
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
>   * @mode: current mode timings
> + * @mode_blob: &drm_property_blob for @mode
>   * @degamma_lut: Lookup table for converting framebuffer pixel data
>   *	before apply the conversion matrix
>   * @ctm: Transformation matrix
> @@ -709,6 +710,7 @@ struct drm_crtc_funcs {
>   * @dev: parent DRM device
>   * @port: OF node used by drm_of_find_possible_crtcs()
>   * @head: list management
> + * @name: human readable name, can be set by the driver
>   * @mutex: per-CRTC locking
>   * @base: base KMS object for ID tracking etc.
>   * @primary: primary plane for this CRTC
> @@ -736,12 +738,13 @@ struct drm_crtc {
>  
>  	char *name;
>  
> -	/*
> -	 * crtc mutex
> +	/**
> +	 * @mutex:
>  	 *
>  	 * This provides a read lock for the overall crtc state (mode, dpms
>  	 * state, ...) and a write lock for everything which can be update
> -	 * without a full modeset (fb, cursor data, ...)
> +	 * without a full modeset (fb, cursor data, crtc properties ...). Full
> +	 * modeset also need to grab dev->mode_config.connection_mutex.
>  	 */
>  	struct drm_modeset_lock mutex;
>  
> @@ -1149,6 +1152,7 @@ struct drm_encoder {
>   * @head: list management
>   * @base: base KMS object
>   * @name: connector name
> + * @connector_id: compacted connector id useful indexing arrays
>   * @connector_type: one of the %DRM_MODE_CONNECTOR_<foo> types from drm_mode.h
>   * @connector_type_id: index into connector type enum
>   * @interlace_allowed: can this connector handle interlaced modes?
> @@ -1161,7 +1165,6 @@ struct drm_encoder {
>   * @funcs: connector control functions
>   * @edid_blob_ptr: DRM property containing EDID if present
>   * @properties: property tracking for this connector
> - * @path_blob_ptr: DRM blob property data for the DP MST path property
>   * @polled: a %DRM_CONNECTOR_POLL_<foo> value for core driven polling
>   * @dpms: current dpms state
>   * @helper_private: mid-layer private data
> @@ -1224,8 +1227,23 @@ struct drm_connector {
>  	struct drm_property_blob *edid_blob_ptr;
>  	struct drm_object_properties properties;
>  
> +	/**
> +	 * @path_blob_ptr:
> +	 *
> +	 * DRM blob property data for the DP MST path property.
> +	 */
>  	struct drm_property_blob *path_blob_ptr;
>  
> +	/**
> +	 * @tile_blob_ptr:
> +	 *
> +	 * DRM blob property data for the tile property (used mostly by DP MST).
> +	 * This is meant for screens which are driven through separate display
> +	 * pipelines represented by &drm_crtc, which might not be in running

s/in// ?

> +	 * with genlocked clocks. For tiled panels which are genlocked, like
> +	 * dual-link LVDS or dual-link DSI, the driver should try to not expose
> +	 * the tiling and virtual both &drm_crtc and &drm_plane if needed.

That last sentence doesn't sound right. Maybe "the driver should try to hide
the tiling and virtual (?virtualize?) both &drm_crtc and &drm_plane if needed." ?

I'm not sure what that virtual part refers to.



> +	 */
>  	struct drm_property_blob *tile_blob_ptr;
>  
>  	uint8_t polled; /* DRM_CONNECTOR_POLL_* */
> @@ -1287,6 +1305,7 @@ struct drm_connector {
>   *	plane (in 16.16)
>   * @src_w: width of visible portion of plane (in 16.16)
>   * @src_h: height of visible portion of plane (in 16.16)
> + * @rotation: rotation of the plane
>   * @state: backpointer to global drm_atomic_state
>   */
>  struct drm_plane_state {
> @@ -1527,6 +1546,7 @@ enum drm_plane_type {
>   * struct drm_plane - central DRM plane control structure
>   * @dev: DRM device this plane belongs to
>   * @head: for list management
> + * @name: human readable name, can be set by the driver

whenever I see these "can" sentences I automatically start to wonder what happens
when that doesn't happen. Maybe a short hint on what @name value will be if not set?

>   * @base: base mode object
>   * @possible_crtcs: pipes this plane can be bound to
>   * @format_types: array of formats supported by this plane
> @@ -1540,6 +1560,7 @@ enum drm_plane_type {
>   * @properties: property tracking for this plane
>   * @type: type of plane (overlay, primary, cursor)
>   * @state: current atomic state for this plane
> + * @helper_private: mid-layer private data
>   */
>  struct drm_plane {
>  	struct drm_device *dev;
> @@ -1547,6 +1568,13 @@ struct drm_plane {
>  
>  	char *name;
>  
> +	/**
> +	 * @mutex:
> +	 *
> +	 * Protects modeset plane state, together with the mutex of &drm_crtc
> +	 * this plane is linked to (when active, getting actived or getting
> +	 * disabled).
> +	 */
>  	struct drm_modeset_lock mutex;
>  
>  	struct drm_mode_object base;
> @@ -1719,6 +1747,10 @@ struct drm_bridge {
>  
>  /**
>   * struct drm_crtc_commit - track modeset commits on a CRTC
> + *
> + * This structure is used to track asynchronously running atomic commit
> + * operations. For an implementation of how to use this look at
> + * drm_atomic_helper_setup_commit() from the atomic helper library.
>   */
>  struct drm_crtc_commit {
>  	/**
> @@ -1764,7 +1796,7 @@ struct drm_crtc_commit {
>  	struct completion hw_done;
>  
>  	/**
> -	 * cleanup_done:
> +	 * @cleanup_done:
>  	 *
>  	 * Will be signalled after old buffers have been cleaned up again by
>  	 * calling drm_atomic_helper_cleanup_planes(). Since this can only
> @@ -1813,7 +1845,6 @@ struct __drm_connnectors_state {
>   * @legacy_set_config: Disable conflicting encoders instead of failing with -EINVAL.
>   * @planes: pointer to array of structures with per-plane data
>   * @crtcs: pointer to array of CRTC pointers
> - * @crtc_states: pointer to array of CRTC states pointers
>   * @num_connector: size of the @connectors and @connector_states arrays
>   * @connectors: pointer to array of structures with per-connector data
>   * @acquire_ctx: acquire context for this atomic modeset state update
> @@ -2137,8 +2168,6 @@ struct drm_mode_config_funcs {
>   * @connection_mutex: ww mutex protecting connector state and routing
>   * @acquire_ctx: global implicit acquire context used by atomic drivers for
>   * 	legacy IOCTLs
> - * @idr_mutex: mutex for KMS ID allocation and management
> - * @crtc_idr: main KMS ID tracking object
>   * @fb_lock: mutex to protect fb state and lists
>   * @num_fb: number of fbs available
>   * @fb_list: list of framebuffers available
> @@ -2160,6 +2189,7 @@ struct drm_mode_config_funcs {
>   * @fb_base: base address of the framebuffer
>   * @poll_enabled: track polling support for this device
>   * @poll_running: track polling status for this device
> + * @delayed_event: track delayed poll uevent deliver for this device
>   * @output_poll_work: delayed work for polling in process context
>   * @property_blob_list: list of all the blob property objects
>   * @blob_lock: mutex for blob property allocation and management
> @@ -2188,10 +2218,30 @@ struct drm_mode_config {
>  	struct mutex mutex; /* protects configuration (mode lists etc.) */
>  	struct drm_modeset_lock connection_mutex; /* protects connector->encoder and encoder->crtc links */
>  	struct drm_modeset_acquire_ctx *acquire_ctx; /* for legacy _lock_all() / _unlock_all() */
> -	struct mutex idr_mutex; /* for IDR management */
> -	struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */
> -	struct idr tile_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */
> -	/* this is limited to one for now */
> +
> +	/**
> +	 * @idr_mutex:
> +	 *
> +	 * Mutex for KMS ID allocation and management. Protects both @crtc_idr
> +	 * and @tile_idr.
> +	 */
> +	struct mutex idr_mutex;
> +
> +	/**
> +	 * @crtc_idr:
> +	 *
> +	 * Main KMS ID tracking object. Use this idr for all IDs, fb, crtc,
> +	 * connector, modes - just makes life easier to have only one.
> +	 */
> +	struct idr crtc_idr;
> +
> +	/**
> +	 * @tile_idr:
> +	 *
> +	 * Use this idr for allocating new IDs for tiled sinks like use in some
> +	 * high-res DP MST screens.
> +	 */
> +	struct idr tile_idr;
>  
>  	struct mutex fb_lock; /* proctects global and per-file fb lists */
>  	int num_fb;
> @@ -2293,7 +2343,11 @@ struct drm_mode_config {
>  	/* whether async page flip is supported or not */
>  	bool async_page_flip;
>  
> -	/* whether the driver supports fb modifiers */
> +	/**
> +	 * @allow_fb_modifiers:
> +	 *
> +	 * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> +	 */
>  	bool allow_fb_modifiers;
>  
>  	/* cursor size */
> -- 
> 2.8.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Otherwise, this one looks good.

Best regards,
Liviu


-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux