[PATCH 0/8] drm: Various minor fixes and cleanups

[PATCH 0/8] drm: Various minor fixes and cleanups

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

This set of patches contains various fixes and cleanups that have
accumulated. Most of these were posted to the mailing list at some point
and some have been reviewed, while other didn't receive any feedback.

Thierry

Thierry Reding (8):
  drm: Try to acquire modeset lock on panic or sysrq
  drm/edid: Drop revision argument for drm_mode_std()
  drm/edid: Cleanup kerneldoc
  drm/fb: Fix typos
  drm: Fixup flip-work kerneldoc
  drm/dp: Fix typo in comment
  drm: Fix indentation of closing brace
  drm: Make drm_crtc_helper_disable() return void

 drivers/gpu/drm/drm_bufs.c        |   2 +-
 drivers/gpu/drm/drm_crtc_helper.c |   7 +-
 drivers/gpu/drm/drm_dp_helper.c   |   2 +-
 drivers/gpu/drm/drm_edid.c        | 139 +++++++++++++++++++++++---------------
 drivers/gpu/drm/drm_fb_helper.c   |  17 +++--
 include/drm/drm_flip_work.h       |   1 +
 6 files changed, 102 insertions(+), 66 deletions(-)

-- 
1.9.2

[PATCH 1/8] drm: Try to acquire modeset lock on panic or sysrq

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Commit 62ff94a54921 "drm/crtc-helper: remove LOCKING from kerneldoc"
causes drm_helper_crtc_in_use() and drm_helper_encoder_in_use() to
complain loudly during a kernel panic or sysrq processing. This is
caused by nobody acquiring the modeset lock in these code paths.

This patch fixes this by trying to acquire the modeset lock for each
FB helper that's forced to kernel mode. If the lock can't be acquired,
it's likely that somebody else is performing a modeset. However, doing
another modeset concurrently might make things even worse, so the safe
option is to simply bail out in that case.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 04d3fd3658f3..158d8e51db3d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -326,12 +326,21 @@ static bool drm_fb_helper_force_kernel_mode(void)
 		return false;
 
 	list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) {
-		if (helper->dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+		struct drm_device *dev = helper->dev;
+
+		if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+			continue;
+
+		if (!mutex_trylock(&dev->mode_config.mutex)) {
+			error = true;
 			continue;
+		}
 
 		ret = drm_fb_helper_restore_fbdev_mode(helper);
 		if (ret)
 			error = true;
+
+		mutex_unlock(&dev->mode_config.mutex);
 	}
 	return error;
 }
-- 
1.9.2

[PATCH 2/8] drm/edid: Drop revision argument for drm_mode_std()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This argument is used to pass in the revision of the EDID, but since the
EDID is passed in as well, we can just as well get the revision from the
EDID structure directly.

While at it, update the kerneldoc comment.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_edid.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d4e3f9d9370f..696186401955 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1595,14 +1595,13 @@ bad_std_timing(u8 a, u8 b)
  * @connector: connector of for the EDID block
  * @edid: EDID block to scan
  * @t: standard timing params
- * @revision: standard timing level
  *
  * Take the standard timing params (in this case width, aspect, and refresh)
  * and convert them into a real mode using CVT/GTF/DMT.
  */
 static struct drm_display_mode *
 drm_mode_std(struct drm_connector *connector, struct edid *edid,
-	     struct std_timing *t, int revision)
+	     struct std_timing *t)
 {
 	struct drm_device *dev = connector->dev;
 	struct drm_display_mode *m, *mode = NULL;
@@ -1623,7 +1622,7 @@ drm_mode_std(struct drm_connector *connector, struct edid *edid,
 	vrefresh_rate = vfreq + 60;
 	/* the vdisplay is calculated based on the aspect ratio */
 	if (aspect_ratio == 0) {
-		if (revision < 3)
+		if (edid->revision < 3)
 			vsize = hsize;
 		else
 			vsize = (hsize * 10) / 16;
@@ -2191,8 +2190,7 @@ do_standard_modes(struct detailed_timing *timing, void *c)
 			struct drm_display_mode *newmode;
 
 			std = &data->data.timings[i];
-			newmode = drm_mode_std(connector, edid, std,
-					       edid->revision);
+			newmode = drm_mode_std(connector, edid, std);
 			if (newmode) {
 				drm_mode_probed_add(connector, newmode);
 				closure->modes++;
@@ -2221,8 +2219,7 @@ add_standard_modes(struct drm_connector *connector, struct edid *edid)
 		struct drm_display_mode *newmode;
 
 		newmode = drm_mode_std(connector, edid,
-				       &edid->standard_timings[i],
-				       edid->revision);
+				       &edid->standard_timings[i]);
 		if (newmode) {
 			drm_mode_probed_add(connector, newmode);
 			modes++;
-- 
1.9.2

[PATCH 3/8] drm/edid: Cleanup kerneldoc

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This fixes up a couple of inconsistencies with kerneldoc comments for
the EDID related functions. Functions that return a value now use the
"Return" section consistently. For some exported symbols the kernel-doc
comments were not in the appropriate format and therefore not included
in the generated documentation. Others had no kernel-doc at all.

While at it, fix a couple of whitespace issues and capitalize common
abbreviations (i2c -> I2C, edid -> EDID, ...).

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_edid.c | 128 ++++++++++++++++++++++++++++-----------------
 1 file changed, 79 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 696186401955..0a52ded1b380 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -984,9 +984,13 @@ static const u8 edid_header[] = {
 	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
 };
 
- /*
- * Sanity check the header of the base EDID block.  Return 8 if the header
- * is perfect, down to 0 if it's totally wrong.
+/**
+ * drm_edid_header_is_valid - sanity check the header of the base EDID block
+ * @raw_edid: pointer to raw base EDID block
+ *
+ * Sanity check the header of the base EDID block.
+ *
+ * Return: 8 if the header is perfect, down to 0 if it's totally wrong.
  */
 int drm_edid_header_is_valid(const u8 *raw_edid)
 {
@@ -1005,9 +1009,16 @@ module_param_named(edid_fixup, edid_fixup, int, 0400);
 MODULE_PARM_DESC(edid_fixup,
 		 "Minimum number of valid EDID header bytes (0-8, default 6)");
 
-/*
- * Sanity check the EDID block (base or extension).  Return 0 if the block
- * doesn't check out, or 1 if it's valid.
+/**
+ * drm_edid_block_valid - Sanity check the EDID block (base or extension)
+ * @raw_edid: pointer to raw EDID block
+ * @block: type of block to validate (0 for base, extension otherwise)
+ * @print_bad_edid: if true, dump bad EDID blocks to the console
+ *
+ * Validate a base or extension EDID block and optionally dump bad blocks to
+ * the console.
+ *
+ * Return: True if the block is valid, false otherwise.
  */
 bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 {
@@ -1077,6 +1088,8 @@ EXPORT_SYMBOL(drm_edid_block_valid);
  * @edid: EDID data
  *
  * Sanity-check an entire EDID record (including extensions)
+ *
+ * Return: True if the EDID data is valid, false otherwise.
  */
 bool drm_edid_is_valid(struct edid *edid)
 {
@@ -1096,18 +1109,15 @@ EXPORT_SYMBOL(drm_edid_is_valid);
 
 #define DDC_SEGMENT_ADDR 0x30
 /**
- * Get EDID information via I2C.
- *
- * @adapter : i2c device adaptor
+ * drm_do_probe_ddc_edid() - get EDID information via I2C
+ * @adapter: I2C device adaptor
  * @buf: EDID data buffer to be filled
  * @block: 128 byte EDID block to start fetching from
  * @len: EDID data buffer length to fetch
  *
- * Returns:
- *
- * 0 on success or -1 on failure.
+ * Try to fetch EDID information by calling I2C driver functions.
  *
- * Try to fetch EDID information by calling i2c driver function.
+ * Return: 0 on success or -1 on failure.
  */
 static int
 drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
@@ -1118,7 +1128,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
 	unsigned char xfers = segment ? 3 : 2;
 	int ret, retries = 5;
 
-	/* The core i2c driver will automatically retry the transfer if the
+	/*
+	 * The core I2C driver will automatically retry the transfer if the
 	 * adapter reports EAGAIN. However, we find that bit-banging transfers
 	 * are susceptible to errors under a heavily loaded machine and
 	 * generate spurious NAKs and timeouts. Retrying the transfer
@@ -1144,10 +1155,10 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
 			}
 		};
 
-	/*
-	 * Avoid sending the segment addr to not upset non-compliant ddc
-	 * monitors.
-	 */
+		/*
+		 * Avoid sending the segment addr to not upset non-compliant
+		 * DDC monitors.
+		 */
 		ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers);
 
 		if (ret == -ENXIO) {
@@ -1246,12 +1257,10 @@ out:
 }
 
 /**
- * Probe DDC presence.
- * @adapter: i2c adapter to probe
+ * drm_probe_ddc() - probe DDC presence
+ * @adapter: I2C adapter to probe
  *
- * Returns:
- *
- * 1 on success
+ * Return: True on success, false on failure.
  */
 bool
 drm_probe_ddc(struct i2c_adapter *adapter)
@@ -1265,12 +1274,12 @@ EXPORT_SYMBOL(drm_probe_ddc);
 /**
  * drm_get_edid - get EDID data, if available
  * @connector: connector we're probing
- * @adapter: i2c adapter to use for DDC
+ * @adapter: I2C adapter to use for DDC
  *
- * Poke the given i2c channel to grab EDID data if possible.  If found,
+ * Poke the given I2C channel to grab EDID data if possible.  If found,
  * attach it to the connector.
  *
- * Return edid data or NULL if we couldn't find any.
+ * Return: Pointer to valid EDID or NULL if we couldn't find any.
  */
 struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter)
@@ -1288,7 +1297,7 @@ EXPORT_SYMBOL(drm_get_edid);
  * drm_edid_duplicate - duplicate an EDID and the extensions
  * @edid: EDID to duplicate
  *
- * Return duplicate edid or NULL on allocation failure.
+ * Return: Pointer to duplicated EDID or NULL on allocation failure.
  */
 struct edid *drm_edid_duplicate(const struct edid *edid)
 {
@@ -1411,7 +1420,8 @@ mode_is_rb(const struct drm_display_mode *mode)
  * @rb: Mode reduced-blanking-ness
  *
  * Walk the DMT mode list looking for a match for the given parameters.
- * Return a newly allocated copy of the mode, or NULL if not found.
+ *
+ * Return: A newly allocated copy of the mode, or NULL if not found.
  */
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
@@ -2139,7 +2149,7 @@ do_established_modes(struct detailed_timing *timing, void *c)
 
 /**
  * add_established_modes - get est. modes from EDID and add them
- * @connector: connector of for the EDID block
+ * @connector: connector to add mode(s) to
  * @edid: EDID block to scan
  *
  * Each EDID block contains a bitmap of the supported "established modes" list
@@ -2201,7 +2211,7 @@ do_standard_modes(struct detailed_timing *timing, void *c)
 
 /**
  * add_standard_modes - get std. modes from EDID and add them
- * @connector: connector of for the EDID block
+ * @connector: connector to add mode(s) to
  * @edid: EDID block to scan
  *
  * Standard modes can be calculated using the appropriate standard (DMT,
@@ -2422,7 +2432,7 @@ cea_mode_alternate_clock(const struct drm_display_mode *cea_mode)
  * drm_match_cea_mode - look for a CEA mode matching given mode
  * @to_match: display mode
  *
- * Returns the CEA Video ID (VIC) of the mode or 0 if it isn't a CEA-861
+ * Return: The CEA Video ID (VIC) of the mode or 0 if it isn't a CEA-861
  * mode.
  */
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
@@ -3020,11 +3030,9 @@ monitor_name(struct detailed_timing *t, void *data)
  * @connector: connector corresponding to the HDMI/DP sink
  * @edid: EDID to parse
  *
- * Fill the ELD (EDID-Like Data) buffer for passing to the audio driver.
- * Some ELD fields are left to the graphics driver caller:
- * - Conn_Type
- * - HDCP
- * - Port_ID
+ * Fill the ELD (EDID-Like Data) buffer for passing to the audio driver. The
+ * Conn_Type, HDCP and Port_ID ELD fields are left for the graphics driver to
+ * fill in.
  */
 void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
 {
@@ -3108,9 +3116,10 @@ EXPORT_SYMBOL(drm_edid_to_eld);
  * @sads: pointer that will be set to the extracted SADs
  *
  * Looks for CEA EDID block and extracts SADs (Short Audio Descriptors) from it.
- * Note: returned pointer needs to be kfreed
  *
- * Return number of found SADs or negative number on error.
+ * Note: The returned pointer needs to be freed using kfree().
+ *
+ * Return: The number of found SADs or negative number on error.
  */
 int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads)
 {
@@ -3167,9 +3176,11 @@ EXPORT_SYMBOL(drm_edid_to_sad);
  * @sadb: pointer to the speaker block
  *
  * Looks for CEA EDID block and extracts the Speaker Allocation Data Block from it.
- * Note: returned pointer needs to be kfreed
  *
- * Return number of found Speaker Allocation Blocks or negative number on error.
+ * Note: The returned pointer needs to be freed using kfree().
+ *
+ * Return: The number of found Speaker Allocation Blocks or negative number on
+ * error.
  */
 int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb)
 {
@@ -3216,9 +3227,12 @@ int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb)
 EXPORT_SYMBOL(drm_edid_to_speaker_allocation);
 
 /**
- * drm_av_sync_delay - HDMI/DP sink audio-video sync delay in millisecond
+ * drm_av_sync_delay - compute the HDMI/DP sink audio-video sync delay
  * @connector: connector associated with the HDMI/DP sink
  * @mode: the display mode
+ *
+ * Return: The HDMI/DP sink's audio-video sync delay in milliseconds or 0 if
+ * the sink doesn't support audio or video.
  */
 int drm_av_sync_delay(struct drm_connector *connector,
 		      struct drm_display_mode *mode)
@@ -3260,6 +3274,9 @@ EXPORT_SYMBOL(drm_av_sync_delay);
  *
  * It's possible for one encoder to be associated with multiple HDMI/DP sinks.
  * The policy is now hard coded to simply use the first HDMI/DP sink's ELD.
+ *
+ * Return: The connector associated with the first HDMI/DP sink that has ELD
+ * attached to it.
  */
 struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
 				     struct drm_display_mode *mode)
@@ -3276,11 +3293,12 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
 EXPORT_SYMBOL(drm_select_eld);
 
 /**
- * drm_detect_hdmi_monitor - detect whether monitor is hdmi.
+ * drm_detect_hdmi_monitor - detect whether monitor is HDMI
  * @edid: monitor EDID information
  *
  * Parse the CEA extension according to CEA-861-B.
- * Return true if HDMI, false if not or unknown.
+ *
+ * Return: True if the monitor is HDMI, false if not or unknown.
  */
 bool drm_detect_hdmi_monitor(struct edid *edid)
 {
@@ -3318,6 +3336,7 @@ EXPORT_SYMBOL(drm_detect_hdmi_monitor);
  * audio format, assume at least 'basic audio' support, even if 'basic
  * audio' is not defined in EDID.
  *
+ * Return: True if the monitor supports audio, false otherwise.
  */
 bool drm_detect_monitor_audio(struct edid *edid)
 {
@@ -3361,6 +3380,8 @@ EXPORT_SYMBOL(drm_detect_monitor_audio);
  * Check whether the monitor reports the RGB quantization range selection
  * as supported. The AVI infoframe can then be used to inform the monitor
  * which quantization range (full or limited) is used.
+ *
+ * Return: True if the RGB quantization range is selectable, false otherwise.
  */
 bool drm_rgb_quant_range_selectable(struct edid *edid)
 {
@@ -3465,11 +3486,11 @@ static void drm_add_display_info(struct edid *edid,
 /**
  * drm_add_edid_modes - add modes from EDID data, if available
  * @connector: connector we're probing
- * @edid: edid data
+ * @edid: EDID data
  *
  * Add the specified modes to the connector's mode list.
  *
- * Return number of modes added or 0 if we couldn't find any.
+ * Return: The number of modes added or 0 if we couldn't find any.
  */
 int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 {
@@ -3531,7 +3552,7 @@ EXPORT_SYMBOL(drm_add_edid_modes);
  * Add the specified modes to the connector's mode list. Only when the
  * hdisplay/vdisplay is not beyond the given limit, it will be added.
  *
- * Return number of modes added or 0 if we couldn't find any.
+ * Return: The number of modes added or 0 if we couldn't find any.
  */
 int drm_add_modes_noedid(struct drm_connector *connector,
 			int hdisplay, int vdisplay)
@@ -3570,13 +3591,22 @@ int drm_add_modes_noedid(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_add_modes_noedid);
 
+/**
+ * drm_set_preferred_mode - Sets the preferred mode of a connector
+ * @connector: connector whose mode list should be processed
+ * @hpref: horizontal resolution of preferred mode
+ * @vpref: vertical resolution of preferred mode
+ *
+ * Marks a mode as preferred if it matches the resolution specified by @hpref
+ * and @vpref.
+ */
 void drm_set_preferred_mode(struct drm_connector *connector,
 			   int hpref, int vpref)
 {
 	struct drm_display_mode *mode;
 
 	list_for_each_entry(mode, &connector->probed_modes, head) {
-		if (mode->hdisplay  == hpref &&
+		if (mode->hdisplay == hpref &&
 		    mode->vdisplay == vpref)
 			mode->type |= DRM_MODE_TYPE_PREFERRED;
 	}
@@ -3589,7 +3619,7 @@ EXPORT_SYMBOL(drm_set_preferred_mode);
  * @frame: HDMI AVI infoframe
  * @mode: DRM display mode
  *
- * Returns 0 on success or a negative error code on failure.
+ * Return: 0 on success or a negative error code on failure.
  */
 int
 drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
@@ -3654,7 +3684,7 @@ s3d_structure_from_display_mode(const struct drm_display_mode *mode)
  * 4k or stereoscopic 3D mode. So when giving any other mode as input this
  * function will return -EINVAL, error that can be safely ignored.
  *
- * Returns 0 on success or a negative error code on failure.
+ * Return: 0 on success or a negative error code on failure.
  */
 int
 drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
-- 
1.9.2

Re: [PATCH 3/8] drm/edid: Cleanup kerneldoc

[Daniel Vetter]

On Tue, Apr 29, 2014 at 11:44:34AM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This fixes up a couple of inconsistencies with kerneldoc comments for
> the EDID related functions. Functions that return a value now use the
> "Return" section consistently. For some exported symbols the kernel-doc
> comments were not in the appropriate format and therefore not included
> in the generated documentation. Others had no kernel-doc at all.
> 
> While at it, fix a couple of whitespace issues and capitalize common
> abbreviations (i2c -> I2C, edid -> EDID, ...).
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Two comments below for further polish, but this looks good imo.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_edid.c | 128 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 79 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 696186401955..0a52ded1b380 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -984,9 +984,13 @@ static const u8 edid_header[] = {
>  	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>  };
>  
> - /*
> - * Sanity check the header of the base EDID block.  Return 8 if the header
> - * is perfect, down to 0 if it's totally wrong.
> +/**
> + * drm_edid_header_is_valid - sanity check the header of the base EDID block
> + * @raw_edid: pointer to raw base EDID block
> + *
> + * Sanity check the header of the base EDID block.
> + *
> + * Return: 8 if the header is perfect, down to 0 if it's totally wrong.
>   */
>  int drm_edid_header_is_valid(const u8 *raw_edid)
>  {
> @@ -1005,9 +1009,16 @@ module_param_named(edid_fixup, edid_fixup, int, 0400);
>  MODULE_PARM_DESC(edid_fixup,
>  		 "Minimum number of valid EDID header bytes (0-8, default 6)");
>  
> -/*
> - * Sanity check the EDID block (base or extension).  Return 0 if the block
> - * doesn't check out, or 1 if it's valid.
> +/**
> + * drm_edid_block_valid - Sanity check the EDID block (base or extension)
> + * @raw_edid: pointer to raw EDID block
> + * @block: type of block to validate (0 for base, extension otherwise)
> + * @print_bad_edid: if true, dump bad EDID blocks to the console
> + *
> + * Validate a base or extension EDID block and optionally dump bad blocks to
> + * the console.
> + *
> + * Return: True if the block is valid, false otherwise.
>   */
>  bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
>  {
> @@ -1077,6 +1088,8 @@ EXPORT_SYMBOL(drm_edid_block_valid);
>   * @edid: EDID data
>   *
>   * Sanity-check an entire EDID record (including extensions)
> + *
> + * Return: True if the EDID data is valid, false otherwise.
>   */
>  bool drm_edid_is_valid(struct edid *edid)
>  {
> @@ -1096,18 +1109,15 @@ EXPORT_SYMBOL(drm_edid_is_valid);
>  
>  #define DDC_SEGMENT_ADDR 0x30
>  /**
> - * Get EDID information via I2C.
> - *
> - * @adapter : i2c device adaptor
> + * drm_do_probe_ddc_edid() - get EDID information via I2C
> + * @adapter: I2C device adaptor
>   * @buf: EDID data buffer to be filled
>   * @block: 128 byte EDID block to start fetching from
>   * @len: EDID data buffer length to fetch
>   *
> - * Returns:
> - *
> - * 0 on success or -1 on failure.
> + * Try to fetch EDID information by calling I2C driver functions.
>   *
> - * Try to fetch EDID information by calling i2c driver function.
> + * Return: 0 on success or -1 on failure.

I wonder whether we shouldn't just switch to the common negative error
codes as the return value on failure, with -ENXIO as-is and -EIO when
running out of retry attempts. Just an idea for a follow-up patch.

>   */
>  static int
>  drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
> @@ -1118,7 +1128,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
>  	unsigned char xfers = segment ? 3 : 2;
>  	int ret, retries = 5;
>  
> -	/* The core i2c driver will automatically retry the transfer if the
> +	/*
> +	 * The core I2C driver will automatically retry the transfer if the
>  	 * adapter reports EAGAIN. However, we find that bit-banging transfers
>  	 * are susceptible to errors under a heavily loaded machine and
>  	 * generate spurious NAKs and timeouts. Retrying the transfer
> @@ -1144,10 +1155,10 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
>  			}
>  		};
>  
> -	/*
> -	 * Avoid sending the segment addr to not upset non-compliant ddc
> -	 * monitors.
> -	 */
> +		/*
> +		 * Avoid sending the segment addr to not upset non-compliant
> +		 * DDC monitors.
> +		 */
>  		ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers);
>  
>  		if (ret == -ENXIO) {
> @@ -1246,12 +1257,10 @@ out:
>  }
>  
>  /**
> - * Probe DDC presence.
> - * @adapter: i2c adapter to probe
> + * drm_probe_ddc() - probe DDC presence
> + * @adapter: I2C adapter to probe
>   *
> - * Returns:
> - *
> - * 1 on success
> + * Return: True on success, false on failure.
>   */
>  bool
>  drm_probe_ddc(struct i2c_adapter *adapter)
> @@ -1265,12 +1274,12 @@ EXPORT_SYMBOL(drm_probe_ddc);
>  /**
>   * drm_get_edid - get EDID data, if available
>   * @connector: connector we're probing
> - * @adapter: i2c adapter to use for DDC
> + * @adapter: I2C adapter to use for DDC
>   *
> - * Poke the given i2c channel to grab EDID data if possible.  If found,
> + * Poke the given I2C channel to grab EDID data if possible.  If found,
>   * attach it to the connector.
>   *
> - * Return edid data or NULL if we couldn't find any.
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
>   */
>  struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter)
> @@ -1288,7 +1297,7 @@ EXPORT_SYMBOL(drm_get_edid);
>   * drm_edid_duplicate - duplicate an EDID and the extensions
>   * @edid: EDID to duplicate
>   *
> - * Return duplicate edid or NULL on allocation failure.
> + * Return: Pointer to duplicated EDID or NULL on allocation failure.
>   */
>  struct edid *drm_edid_duplicate(const struct edid *edid)
>  {
> @@ -1411,7 +1420,8 @@ mode_is_rb(const struct drm_display_mode *mode)
>   * @rb: Mode reduced-blanking-ness
>   *
>   * Walk the DMT mode list looking for a match for the given parameters.
> - * Return a newly allocated copy of the mode, or NULL if not found.
> + *
> + * Return: A newly allocated copy of the mode, or NULL if not found.
>   */
>  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>  					   int hsize, int vsize, int fresh,
> @@ -2139,7 +2149,7 @@ do_established_modes(struct detailed_timing *timing, void *c)
>  
>  /**
>   * add_established_modes - get est. modes from EDID and add them
> - * @connector: connector of for the EDID block
> + * @connector: connector to add mode(s) to
>   * @edid: EDID block to scan
>   *
>   * Each EDID block contains a bitmap of the supported "established modes" list
> @@ -2201,7 +2211,7 @@ do_standard_modes(struct detailed_timing *timing, void *c)
>  
>  /**
>   * add_standard_modes - get std. modes from EDID and add them
> - * @connector: connector of for the EDID block
> + * @connector: connector to add mode(s) to
>   * @edid: EDID block to scan
>   *
>   * Standard modes can be calculated using the appropriate standard (DMT,
> @@ -2422,7 +2432,7 @@ cea_mode_alternate_clock(const struct drm_display_mode *cea_mode)
>   * drm_match_cea_mode - look for a CEA mode matching given mode
>   * @to_match: display mode
>   *
> - * Returns the CEA Video ID (VIC) of the mode or 0 if it isn't a CEA-861
> + * Return: The CEA Video ID (VIC) of the mode or 0 if it isn't a CEA-861
>   * mode.
>   */
>  u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
> @@ -3020,11 +3030,9 @@ monitor_name(struct detailed_timing *t, void *data)
>   * @connector: connector corresponding to the HDMI/DP sink
>   * @edid: EDID to parse
>   *
> - * Fill the ELD (EDID-Like Data) buffer for passing to the audio driver.
> - * Some ELD fields are left to the graphics driver caller:
> - * - Conn_Type
> - * - HDCP
> - * - Port_ID
> + * Fill the ELD (EDID-Like Data) buffer for passing to the audio driver. The
> + * Conn_Type, HDCP and Port_ID ELD fields are left for the graphics driver to
> + * fill in.
>   */
>  void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>  {
> @@ -3108,9 +3116,10 @@ EXPORT_SYMBOL(drm_edid_to_eld);
>   * @sads: pointer that will be set to the extracted SADs
>   *
>   * Looks for CEA EDID block and extracts SADs (Short Audio Descriptors) from it.
> - * Note: returned pointer needs to be kfreed
>   *
> - * Return number of found SADs or negative number on error.
> + * Note: The returned pointer needs to be freed using kfree().
> + *
> + * Return: The number of found SADs or negative number on error.
>   */
>  int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads)
>  {
> @@ -3167,9 +3176,11 @@ EXPORT_SYMBOL(drm_edid_to_sad);
>   * @sadb: pointer to the speaker block
>   *
>   * Looks for CEA EDID block and extracts the Speaker Allocation Data Block from it.
> - * Note: returned pointer needs to be kfreed
>   *
> - * Return number of found Speaker Allocation Blocks or negative number on error.
> + * Note: The returned pointer needs to be freed using kfree().
> + *
> + * Return: The number of found Speaker Allocation Blocks or negative number on
> + * error.
>   */
>  int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb)
>  {
> @@ -3216,9 +3227,12 @@ int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb)
>  EXPORT_SYMBOL(drm_edid_to_speaker_allocation);
>  
>  /**
> - * drm_av_sync_delay - HDMI/DP sink audio-video sync delay in millisecond
> + * drm_av_sync_delay - compute the HDMI/DP sink audio-video sync delay
>   * @connector: connector associated with the HDMI/DP sink
>   * @mode: the display mode
> + *
> + * Return: The HDMI/DP sink's audio-video sync delay in milliseconds or 0 if
> + * the sink doesn't support audio or video.
>   */
>  int drm_av_sync_delay(struct drm_connector *connector,
>  		      struct drm_display_mode *mode)
> @@ -3260,6 +3274,9 @@ EXPORT_SYMBOL(drm_av_sync_delay);
>   *
>   * It's possible for one encoder to be associated with multiple HDMI/DP sinks.
>   * The policy is now hard coded to simply use the first HDMI/DP sink's ELD.
> + *
> + * Return: The connector associated with the first HDMI/DP sink that has ELD
> + * attached to it.
>   */
>  struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
>  				     struct drm_display_mode *mode)
> @@ -3276,11 +3293,12 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder,
>  EXPORT_SYMBOL(drm_select_eld);
>  
>  /**
> - * drm_detect_hdmi_monitor - detect whether monitor is hdmi.
> + * drm_detect_hdmi_monitor - detect whether monitor is HDMI
>   * @edid: monitor EDID information
>   *
>   * Parse the CEA extension according to CEA-861-B.
> - * Return true if HDMI, false if not or unknown.
> + *
> + * Return: True if the monitor is HDMI, false if not or unknown.
>   */
>  bool drm_detect_hdmi_monitor(struct edid *edid)
>  {
> @@ -3318,6 +3336,7 @@ EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>   * audio format, assume at least 'basic audio' support, even if 'basic
>   * audio' is not defined in EDID.
>   *
> + * Return: True if the monitor supports audio, false otherwise.
>   */
>  bool drm_detect_monitor_audio(struct edid *edid)
>  {
> @@ -3361,6 +3380,8 @@ EXPORT_SYMBOL(drm_detect_monitor_audio);
>   * Check whether the monitor reports the RGB quantization range selection
>   * as supported. The AVI infoframe can then be used to inform the monitor
>   * which quantization range (full or limited) is used.
> + *
> + * Return: True if the RGB quantization range is selectable, false otherwise.
>   */
>  bool drm_rgb_quant_range_selectable(struct edid *edid)
>  {
> @@ -3465,11 +3486,11 @@ static void drm_add_display_info(struct edid *edid,
>  /**
>   * drm_add_edid_modes - add modes from EDID data, if available
>   * @connector: connector we're probing
> - * @edid: edid data
> + * @edid: EDID data
>   *
>   * Add the specified modes to the connector's mode list.
>   *
> - * Return number of modes added or 0 if we couldn't find any.
> + * Return: The number of modes added or 0 if we couldn't find any.
>   */
>  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  {
> @@ -3531,7 +3552,7 @@ EXPORT_SYMBOL(drm_add_edid_modes);
>   * Add the specified modes to the connector's mode list. Only when the
>   * hdisplay/vdisplay is not beyond the given limit, it will be added.
>   *
> - * Return number of modes added or 0 if we couldn't find any.
> + * Return: The number of modes added or 0 if we couldn't find any.
>   */
>  int drm_add_modes_noedid(struct drm_connector *connector,
>  			int hdisplay, int vdisplay)

I wonder whether this wouldn't make more sense in drm_modes.c now, but
again follow-up patch material.

> @@ -3570,13 +3591,22 @@ int drm_add_modes_noedid(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_add_modes_noedid);
>  
> +/**
> + * drm_set_preferred_mode - Sets the preferred mode of a connector
> + * @connector: connector whose mode list should be processed
> + * @hpref: horizontal resolution of preferred mode
> + * @vpref: vertical resolution of preferred mode
> + *
> + * Marks a mode as preferred if it matches the resolution specified by @hpref
> + * and @vpref.
> + */
>  void drm_set_preferred_mode(struct drm_connector *connector,
>  			   int hpref, int vpref)
>  {
>  	struct drm_display_mode *mode;
>  
>  	list_for_each_entry(mode, &connector->probed_modes, head) {
> -		if (mode->hdisplay  == hpref &&
> +		if (mode->hdisplay == hpref &&
>  		    mode->vdisplay == vpref)
>  			mode->type |= DRM_MODE_TYPE_PREFERRED;
>  	}
> @@ -3589,7 +3619,7 @@ EXPORT_SYMBOL(drm_set_preferred_mode);
>   * @frame: HDMI AVI infoframe
>   * @mode: DRM display mode
>   *
> - * Returns 0 on success or a negative error code on failure.
> + * Return: 0 on success or a negative error code on failure.
>   */
>  int
>  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> @@ -3654,7 +3684,7 @@ s3d_structure_from_display_mode(const struct drm_display_mode *mode)
>   * 4k or stereoscopic 3D mode. So when giving any other mode as input this
>   * function will return -EINVAL, error that can be safely ignored.
>   *
> - * Returns 0 on success or a negative error code on failure.
> + * Return: 0 on success or a negative error code on failure.
>   */
>  int
>  drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
> -- 
> 1.9.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 4/8] drm/fb: Fix typos

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Fix a few trivial typos in the framebuffer helper documentation.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 158d8e51db3d..e95ed5805f07 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -45,13 +45,13 @@ static LIST_HEAD(kernel_fb_helper_list);
  * DOC: fbdev helpers
  *
  * The fb helper functions are useful to provide an fbdev on top of a drm kernel
- * mode setting driver. They can be used mostly independantely from the crtc
+ * mode setting driver. They can be used mostly independently from the crtc
  * helper functions used by many drivers to implement the kernel mode setting
  * interfaces.
  *
  * Initialization is done as a three-step process with drm_fb_helper_init(),
  * drm_fb_helper_single_add_all_connectors() and drm_fb_helper_initial_config().
- * Drivers with fancier requirements than the default beheviour can override the
+ * Drivers with fancier requirements than the default behaviour can override the
  * second step with their own code.  Teardown is done with drm_fb_helper_fini().
  *
  * At runtime drivers should restore the fbdev console by calling
@@ -59,7 +59,7 @@ static LIST_HEAD(kernel_fb_helper_list);
  * should also notify the fb helper code from updates to the output
  * configuration by calling drm_fb_helper_hotplug_event(). For easier
  * integration with the output polling code in drm_crtc_helper.c the modeset
- * code proves a ->output_poll_changed callback.
+ * code provides a ->output_poll_changed callback.
  *
  * All other functions exported by the fb helper library can be used to
  * implement the fbdev driver interface by the driver.
-- 
1.9.2

[PATCH 5/8] drm: Fixup flip-work kerneldoc

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Describe the fifo parameter. It seems like kerneldoc doesn't properly
handle fields defined using a macro, so it will end up complaining about
this anyway and not generate the documentation for it either. At least
the kerneldoc is now complete.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/drm/drm_flip_work.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/drm/drm_flip_work.h b/include/drm/drm_flip_work.h
index 35c776ae7d3b..9eed34dcd6af 100644
--- a/include/drm/drm_flip_work.h
+++ b/include/drm/drm_flip_work.h
@@ -57,6 +57,7 @@ typedef void (*drm_flip_func_t)(struct drm_flip_work *work, void *val);
  * @count: number of committed items
  * @func: callback fxn called for each committed item
  * @worker: worker which calls @func
+ * @fifo: queue of committed items
  */
 struct drm_flip_work {
 	const char *name;
-- 
1.9.2

[PATCH 6/8] drm/dp: Fix typo in comment

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Replace an occurrence of "adapater" with "adapter".

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 4b6e6f3ba0a1..a13f1f51f68e 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -206,7 +206,7 @@ i2c_dp_aux_prepare_bus(struct i2c_adapter *adapter)
  * i2c_dp_aux_add_bus() - register an i2c adapter using the aux ch helper
  * @adapter: i2c adapter to register
  *
- * This registers an i2c adapater that uses dp aux channel as it's underlaying
+ * This registers an i2c adapter that uses dp aux channel as it's underlaying
  * transport. The driver needs to fill out the &i2c_algo_dp_aux_data structure
  * and store it in the algo_data member of the @adapter argument. This will be
  * used by the i2c over dp aux algorithm to drive the hardware.
-- 
1.9.2

[PATCH 7/8] drm: Fix indentation of closing brace

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The closing brace of the body of drm_addmap_core() is indented wrongly.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_bufs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index edec31fe3fed..593efc15f54b 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -363,7 +363,7 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset,
 		list->master = dev->primary->master;
 	*maplist = list;
 	return 0;
-	}
+}
 
 int drm_addmap(struct drm_device * dev, resource_size_t offset,
 	       unsigned int size, enum drm_map_type type,
-- 
1.9.2

[PATCH 8/8] drm: Make drm_crtc_helper_disable() return void

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The function can never fail, and always returns 0, so it may just as
well not return anything.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index df281b54db01..72ce15e21bcd 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -387,8 +387,7 @@ done:
 }
 EXPORT_SYMBOL(drm_crtc_helper_set_mode);
 
-
-static int
+static void
 drm_crtc_helper_disable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -417,7 +416,6 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
 	}
 
 	__drm_helper_disable_unused_functions(dev);
-	return 0;
 }
 
 /**
@@ -468,7 +466,8 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 				(int)set->num_connectors, set->x, set->y);
 	} else {
 		DRM_DEBUG_KMS("[CRTC:%d] [NOFB]\n", set->crtc->base.id);
-		return drm_crtc_helper_disable(set->crtc);
+		drm_crtc_helper_disable(set->crtc);
+		return 0;
 	}
 
 	dev = set->crtc->dev;
-- 
1.9.2

Re: [PATCH 0/8] drm: Various minor fixes and cleanups

[Daniel Vetter]

On Tue, Apr 29, 2014 at 11:44:31AM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Hi,
> 
> This set of patches contains various fixes and cleanups that have
> accumulated. Most of these were posted to the mailing list at some point
> and some have been reviewed, while other didn't receive any feedback.
> 
> Thierry

On the entire series

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I've pulled it all into my topic/core-stuff collector branch. After a bit
of testing in drm-intel-nightly I'll send out a pull request for it
(there's other stuff), in case you&Dave want to be lazy ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/8] drm: Various minor fixes and cleanups

[Thierry Reding]

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

On Tue, Apr 29, 2014 at 12:35:46PM +0200, Daniel Vetter wrote:
> On Tue, Apr 29, 2014 at 11:44:31AM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Hi,
> > 
> > This set of patches contains various fixes and cleanups that have
> > accumulated. Most of these were posted to the mailing list at some point
> > and some have been reviewed, while other didn't receive any feedback.
> > 
> > Thierry
> 
> On the entire series
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I've pulled it all into my topic/core-stuff collector branch. After a bit
> of testing in drm-intel-nightly I'll send out a pull request for it
> (there's other stuff), in case you&Dave want to be lazy ;-)

Great, thanks. If you already have this in a branch with things that you
were going to send to Dave anyway that's probably better. No need to
swamp Dave with excessive numbers of pull requests. So unless you *want*
me to make my own pull request for this I'll opt for laziness. =)

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 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