drm/bridge/sii8620: fix display modes validation

Commit Message

Maciej Purski Jan. 23, 2018, 11:15 a.m. UTC

Current implementation of mode_valid() and mode_fixup() callbacks
handle packed pixel modes improperly.

Fix it by using proper maximum clock values from the documentation.

Signed-off-by: Maciej Purski <[e-mail]>
---
 drivers/gpu/drm/bridge/sil-sii8620.c | 90 ++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 39 deletions(-)

Comments

kernel test robot Jan. 25, 2018, 11:27 p.m. UTC | #1

Hi Maciej,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on next-20180119]
[cannot apply to v4.15-rc9]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Maciej-Purski/drm-bridge-sii8620-fix-display-modes-validation/20180125-174703
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: i386-randconfig-sb0-01260635 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/bridge/sil-sii8620.c: In function 'sii8620_mode_valid':
>> drivers/gpu/drm/bridge/sil-sii8620.c:2242:2: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
     return ret;
     ^

vim +/ret +2242 drivers/gpu/drm/bridge/sil-sii8620.c

  2220	
  2221	static enum drm_mode_status sii8620_mode_valid(struct drm_bridge *bridge,
  2222						 const struct drm_display_mode *mode)
  2223	{
  2224		enum drm_mode_status ret;
  2225		struct sii8620 *ctx = bridge_to_sii8620(bridge);
  2226		int pack_required = sii8620_is_packing_required(ctx, mode);
  2227		bool can_pack = ctx->devcap[MHL_DCAP_VID_LINK_MODE] &
  2228				MHL_DCAP_VID_LINK_PPIXEL;
  2229	
  2230		switch (pack_required) {
  2231		case -1:
  2232			ret = MODE_CLOCK_HIGH;
  2233			break;
  2234		case 1:
  2235			ret = (can_pack) ? MODE_OK : MODE_CLOCK_HIGH;
  2236			break;
  2237		case 0:
  2238			ret = MODE_OK;
  2239			break;
  2240		}
  2241	
> 2242		return ret;
  2243	}
  2244	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Patch

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 5168783..1718c2e 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -34,8 +34,11 @@ 
 
 #define SII8620_BURST_BUF_LEN 288
 #define VAL_RX_HDMI_CTRL2_DEFVAL VAL_RX_HDMI_CTRL2_IDLE_CNT(3)
-#define MHL1_MAX_LCLK 225000
-#define MHL3_MAX_LCLK 600000
+
+#define MHL1_MAX_PCLK 75000
+#define MHL1_MAX_PCLK_PP_MODE 150000
+#define MHL3_MAX_PCLK 200000
+#define MHL3_MAX_PCLK_PP_MODE 300000
 
 enum sii8620_mode {
 	CM_DISCONNECTED,
@@ -2123,61 +2126,70 @@  static void sii8620_detach(struct drm_bridge *bridge)
 	rc_unregister_device(ctx->rc_dev);
 }
 
+static int sii8620_is_packing_required(struct sii8620 *ctx,
+				 const struct drm_display_mode *mode)
+{
+	int ret;
+
+	if (sii8620_is_mhl3(ctx)) {
+		if (mode->clock < MHL3_MAX_PCLK)
+			ret = 0;
+		else if (mode->clock < MHL3_MAX_PCLK_PP_MODE)
+			ret = 1;
+		else
+			ret = -1;
+	} else {
+		if (mode->clock < MHL1_MAX_PCLK)
+			ret = 0;
+		else if (mode->clock < MHL1_MAX_PCLK_PP_MODE)
+			ret = 1;
+		else
+			ret = -1;
+	}
+
+	return ret;
+}
+
 static enum drm_mode_status sii8620_mode_valid(struct drm_bridge *bridge,
 					 const struct drm_display_mode *mode)
 {
+	enum drm_mode_status ret;
 	struct sii8620 *ctx = bridge_to_sii8620(bridge);
+	int pack_required = sii8620_is_packing_required(ctx, mode);
 	bool can_pack = ctx->devcap[MHL_DCAP_VID_LINK_MODE] &
 			MHL_DCAP_VID_LINK_PPIXEL;
-	unsigned int max_pclk = sii8620_is_mhl3(ctx) ? MHL3_MAX_LCLK :
-						       MHL1_MAX_LCLK;
-	max_pclk /= can_pack ? 2 : 3;
 
-	return (mode->clock > max_pclk) ? MODE_CLOCK_HIGH : MODE_OK;
+	switch (pack_required) {
+	case -1:
+		ret = MODE_CLOCK_HIGH;
+		break;
+	case 1:
+		ret = (can_pack) ? MODE_OK : MODE_CLOCK_HIGH;
+		break;
+	case 0:
+		ret = MODE_OK;
+		break;
+	}
+
+	return ret;
 }
 
+
 static bool sii8620_mode_fixup(struct drm_bridge *bridge,
 			       const struct drm_display_mode *mode,
 			       struct drm_display_mode *adjusted_mode)
 {
 	struct sii8620 *ctx = bridge_to_sii8620(bridge);
-	int max_lclk;
-	bool ret = true;
 
 	mutex_lock(&ctx->lock);
 
-	max_lclk = sii8620_is_mhl3(ctx) ? MHL3_MAX_LCLK : MHL1_MAX_LCLK;
-	if (max_lclk > 3 * adjusted_mode->clock) {
-		ctx->use_packed_pixel = 0;
-		goto end;
-	}
-	if ((ctx->devcap[MHL_DCAP_VID_LINK_MODE] & MHL_DCAP_VID_LINK_PPIXEL) &&
-	    max_lclk > 2 * adjusted_mode->clock) {
-		ctx->use_packed_pixel = 1;
-		goto end;
-	}
-	ret = false;
-end:
-	if (ret) {
-		u8 vic = drm_match_cea_mode(adjusted_mode);
-
-		if (!vic) {
-			union hdmi_infoframe frm;
-			u8 mhl_vic[] = { 0, 95, 94, 93, 98 };
-
-			/* FIXME: We need the connector here */
-			drm_hdmi_vendor_infoframe_from_display_mode(
-				&frm.vendor.hdmi, NULL, adjusted_mode);
-			vic = frm.vendor.hdmi.vic;
-			if (vic >= ARRAY_SIZE(mhl_vic))
-				vic = 0;
-			vic = mhl_vic[vic];
-		}
-		ctx->video_code = vic;
-		ctx->pixel_clock = adjusted_mode->clock;
-	}
+	ctx->use_packed_pixel = sii8620_is_packing_required(ctx, adjusted_mode);
+	ctx->video_code = drm_match_cea_mode(adjusted_mode);
+	ctx->pixel_clock = adjusted_mode->clock;
+
 	mutex_unlock(&ctx->lock);
-	return ret;
+
+	return true;
 }
 
 static const struct drm_bridge_funcs sii8620_bridge_funcs = {