[PATCH v2 0/2] drm: bridge: dw-hdmi: Add support for Custom PHYs

[PATCH v2 0/2] drm: bridge: dw-hdmi: Add support for Custom PHYs

[Neil Armstrong]

The Amlogic GX SoCs implements a Synopsys DesignWare HDMI TX Controller
in combination with a very custom PHY.

Thanks to Laurent Pinchart's changes, the HW report the following :
 Detected HDMI TX controller v2.01a with HDCP (Vendor PHY)

The following differs from common PHY integration as managed in the current
driver :
 - Amlogic PHY is not configured through the internal I2C link
 - Amlogic PHY do not use the ENTMDS, SVSRET, PDDQ, ... signals from the controller
 - Amlogic PHY do not export HPD ands RxSense signals to the controller

And finally, concerning the controller integration :
 - the Controller registers are not flat memory-mapped, and uses an
    addr+read/write register pair to write all registers.
 - Inputs only YUV444 pixel data

Most of these uses case are implemented in Laurent Pinchart v4 patchset at [3] :
 - Conversion to regmap for register access
 - Add more callbacks ops to handle Custom PHYs
 - Fixes a bug that considers the input to be always RBG and sends bad pixel
   format to a DVI sink by disabling CSC

This is why the following patchset implements :
 - Configure the Input format from the plat_data
 - Add PHY callback to handle HPD and RxSens out of the dw-hdmi drive

This patchset makes the Amlogix GX SoCs HDMI output successfully work, and is
also tested on the RK3288 ACT8846 EVB Board.

Changes since v1 at [2] :

Changes since RFC at [1] :
 - Regmap fixup for 4bytes register access, tested on RK3288 SoC
 - Move phy callbacks to phy_ops and move Synopsys PHY calls into default ops
 - Move HDMI link data into shared header
 - Move Pixel Encoding enum to shared header

[1] http://lkml.kernel.org/r/1484656294-6140-1-git-send-email-narmstrong@baylibre.com
[2] http://lkml.kernel.org/r/1485774318-21916-1-git-send-email-narmstrong@baylibre.com
[3] http://lkml.kernel.org/r/20170301223915.29888-1-laurent.pinchart+renesas@ideasonboard.com


Neil Armstrong (2):
  drm: bridge: dw-hdmi: Take input format from plat_data
  drm: bridge: Move HPD handling to PHY operations

 drivers/gpu/drm/bridge/dw-hdmi.c | 193 +++++++++++++++++++++++++--------------
 include/drm/bridge/dw_hdmi.h     |  17 ++++
 2 files changed, 143 insertions(+), 67 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] drm: bridge: dw-hdmi: Take input format from plat_data

[Neil Armstrong]

Some display pipelines can only provide non-RBG input pixels to the HDMI TX
Controller, this patch takes the pixel format from the plat_data if provided.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 59 ++++++++++++++++++++--------------------
 include/drm/bridge/dw_hdmi.h     |  9 ++++++
 2 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 026a0dc..653ecd7 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -35,12 +35,6 @@
 
 #define HDMI_EDID_LEN		512
 
-#define RGB			0
-#define YCBCR444		1
-#define YCBCR422_16BITS		2
-#define YCBCR422_8BITS		3
-#define XVYCC444		4
-
 enum hdmi_datamap {
 	RGB444_8B = 0x01,
 	RGB444_10B = 0x03,
@@ -94,8 +88,8 @@ struct hdmi_vmode {
 };
 
 struct hdmi_data_info {
-	unsigned int enc_in_format;
-	unsigned int enc_out_format;
+	enum dw_hdmi_color_enc_format enc_in_format;
+	enum dw_hdmi_color_enc_format enc_out_format;
 	unsigned int enc_color_depth;
 	unsigned int colorimetry;
 	unsigned int pix_repet_factor;
@@ -569,7 +563,7 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
 	int color_format = 0;
 	u8 val;
 
-	if (hdmi->hdmi_data.enc_in_format == RGB) {
+	if (hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_RGB) {
 		if (hdmi->hdmi_data.enc_color_depth == 8)
 			color_format = 0x01;
 		else if (hdmi->hdmi_data.enc_color_depth == 10)
@@ -580,7 +574,7 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
 			color_format = 0x07;
 		else
 			return;
-	} else if (hdmi->hdmi_data.enc_in_format == YCBCR444) {
+	} else if (hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_YCBCR444) {
 		if (hdmi->hdmi_data.enc_color_depth == 8)
 			color_format = 0x09;
 		else if (hdmi->hdmi_data.enc_color_depth == 10)
@@ -591,7 +585,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
 			color_format = 0x0F;
 		else
 			return;
-	} else if (hdmi->hdmi_data.enc_in_format == YCBCR422_8BITS) {
+	} else if (hdmi->hdmi_data.enc_in_format ==
+					DW_HDMI_ENC_FMT_YCBCR422_8BITS) {
 		if (hdmi->hdmi_data.enc_color_depth == 8)
 			color_format = 0x16;
 		else if (hdmi->hdmi_data.enc_color_depth == 10)
@@ -627,20 +622,20 @@ static int is_color_space_conversion(struct dw_hdmi *hdmi)
 
 static int is_color_space_decimation(struct dw_hdmi *hdmi)
 {
-	if (hdmi->hdmi_data.enc_out_format != YCBCR422_8BITS)
+	if (hdmi->hdmi_data.enc_out_format != DW_HDMI_ENC_FMT_YCBCR422_8BITS)
 		return 0;
-	if (hdmi->hdmi_data.enc_in_format == RGB ||
-	    hdmi->hdmi_data.enc_in_format == YCBCR444)
+	if (hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_RGB ||
+	    hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_YCBCR444)
 		return 1;
 	return 0;
 }
 
 static int is_color_space_interpolation(struct dw_hdmi *hdmi)
 {
-	if (hdmi->hdmi_data.enc_in_format != YCBCR422_8BITS)
+	if (hdmi->hdmi_data.enc_in_format != DW_HDMI_ENC_FMT_YCBCR422_8BITS)
 		return 0;
-	if (hdmi->hdmi_data.enc_out_format == RGB ||
-	    hdmi->hdmi_data.enc_out_format == YCBCR444)
+	if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_RGB ||
+	    hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_YCBCR444)
 		return 1;
 	return 0;
 }
@@ -652,13 +647,14 @@ static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
 	u32 csc_scale = 1;
 
 	if (is_color_space_conversion(hdmi)) {
-		if (hdmi->hdmi_data.enc_out_format == RGB) {
+		if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_RGB) {
 			if (hdmi->hdmi_data.colorimetry ==
 					HDMI_COLORIMETRY_ITU_601)
 				csc_coeff = &csc_coeff_rgb_out_eitu601;
 			else
 				csc_coeff = &csc_coeff_rgb_out_eitu709;
-		} else if (hdmi->hdmi_data.enc_in_format == RGB) {
+		} else if (hdmi->hdmi_data.enc_in_format ==
+					DW_HDMI_ENC_FMT_RGB) {
 			if (hdmi->hdmi_data.colorimetry ==
 					HDMI_COLORIMETRY_ITU_601)
 				csc_coeff = &csc_coeff_rgb_in_eitu601;
@@ -730,8 +726,8 @@ static void hdmi_video_packetize(struct dw_hdmi *hdmi)
 	struct hdmi_data_info *hdmi_data = &hdmi->hdmi_data;
 	u8 val, vp_conf;
 
-	if (hdmi_data->enc_out_format == RGB ||
-	    hdmi_data->enc_out_format == YCBCR444) {
+	if (hdmi_data->enc_out_format == DW_HDMI_ENC_FMT_RGB ||
+	    hdmi_data->enc_out_format == DW_HDMI_ENC_FMT_YCBCR444) {
 		if (!hdmi_data->enc_color_depth) {
 			output_select = HDMI_VP_CONF_OUTPUT_SELECTOR_BYPASS;
 		} else if (hdmi_data->enc_color_depth == 8) {
@@ -746,7 +742,8 @@ static void hdmi_video_packetize(struct dw_hdmi *hdmi)
 		} else {
 			return;
 		}
-	} else if (hdmi_data->enc_out_format == YCBCR422_8BITS) {
+	} else if (hdmi_data->enc_out_format ==
+					DW_HDMI_ENC_FMT_YCBCR422_8BITS) {
 		if (!hdmi_data->enc_color_depth ||
 		    hdmi_data->enc_color_depth == 8)
 			remap_size = HDMI_VP_REMAP_YCC422_16bit;
@@ -1138,15 +1135,16 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 	/* Initialise info frame from DRM mode */
 	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode);
 
-	if (hdmi->hdmi_data.enc_out_format == YCBCR444)
+	if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_YCBCR444)
 		frame.colorspace = HDMI_COLORSPACE_YUV444;
-	else if (hdmi->hdmi_data.enc_out_format == YCBCR422_8BITS)
+	else if (hdmi->hdmi_data.enc_out_format ==
+					DW_HDMI_ENC_FMT_YCBCR422_8BITS)
 		frame.colorspace = HDMI_COLORSPACE_YUV422;
 	else
 		frame.colorspace = HDMI_COLORSPACE_RGB;
 
 	/* Set up colorimetry */
-	if (hdmi->hdmi_data.enc_out_format == XVYCC444) {
+	if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_XVYCC444) {
 		frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
 		if (hdmi->hdmi_data.colorimetry == HDMI_COLORIMETRY_ITU_601)
 			frame.extended_colorimetry =
@@ -1154,7 +1152,7 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 		else /*hdmi->hdmi_data.colorimetry == HDMI_COLORIMETRY_ITU_709*/
 			frame.extended_colorimetry =
 				HDMI_EXTENDED_COLORIMETRY_XV_YCC_709;
-	} else if (hdmi->hdmi_data.enc_out_format != RGB) {
+	} else if (hdmi->hdmi_data.enc_out_format != DW_HDMI_ENC_FMT_RGB) {
 		frame.colorimetry = hdmi->hdmi_data.colorimetry;
 		frame.extended_colorimetry = HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
 	} else { /* Carries no data */
@@ -1443,10 +1441,13 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 	hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0;
 	hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;
 
-	/* TODO: Get input format from IPU (via FB driver interface) */
-	hdmi->hdmi_data.enc_in_format = RGB;
+	/* Get input format from plat data or fallback to RGB */
+	if (hdmi->plat_data->input_fmt >= 0)
+		hdmi->hdmi_data.enc_in_format = hdmi->plat_data->input_fmt;
+	else
+		hdmi->hdmi_data.enc_in_format = DW_HDMI_ENC_FMT_RGB;
 
-	hdmi->hdmi_data.enc_out_format = RGB;
+	hdmi->hdmi_data.enc_out_format = DW_HDMI_ENC_FMT_RGB;
 
 	hdmi->hdmi_data.enc_color_depth = 8;
 	hdmi->hdmi_data.pix_repet_factor = 0;
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index bcceee8..8c0cc13 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -21,6 +21,14 @@ enum {
 	DW_HDMI_RES_MAX,
 };
 
+enum dw_hdmi_color_enc_format {
+	DW_HDMI_ENC_FMT_RGB = 0,
+	DW_HDMI_ENC_FMT_YCBCR444,
+	DW_HDMI_ENC_FMT_YCBCR422_16BITS,
+	DW_HDMI_ENC_FMT_YCBCR422_8BITS,
+	DW_HDMI_ENC_FMT_XVYCC444,
+};
+
 enum dw_hdmi_phy_type {
 	DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
 	DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
@@ -62,6 +70,7 @@ struct dw_hdmi_plat_data {
 	struct regmap *regm;
 	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
 					   struct drm_display_mode *mode);
+	enum dw_hdmi_color_enc_format input_fmt;
 
 	/* Vendor PHY support */
 	const struct dw_hdmi_phy_ops *phy_ops;
-- 
1.9.1

Re: [PATCH v2 1/2] drm: bridge: dw-hdmi: Take input format from plat_data

[Laurent Pinchart]

Hi Neil,

Thank you for the patch.

On Thursday 02 Mar 2017 16:29:31 Neil Armstrong wrote:
> Some display pipelines can only provide non-RBG input pixels to the HDMI TX
> Controller, this patch takes the pixel format from the plat_data if
> provided.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 59 ++++++++++++++++++-------------------
>  include/drm/bridge/dw_hdmi.h     |  9 ++++++
>  2 files changed, 39 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> b/drivers/gpu/drm/bridge/dw-hdmi.c index 026a0dc..653ecd7 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -35,12 +35,6 @@
> 
>  #define HDMI_EDID_LEN		512
> 
> -#define RGB			0
> -#define YCBCR444		1
> -#define YCBCR422_16BITS		2
> -#define YCBCR422_8BITS		3
> -#define XVYCC444		4
> -
>  enum hdmi_datamap {
>  	RGB444_8B = 0x01,
>  	RGB444_10B = 0x03,
> @@ -94,8 +88,8 @@ struct hdmi_vmode {
>  };
> 
>  struct hdmi_data_info {
> -	unsigned int enc_in_format;
> -	unsigned int enc_out_format;
> +	enum dw_hdmi_color_enc_format enc_in_format;
> +	enum dw_hdmi_color_enc_format enc_out_format;
>  	unsigned int enc_color_depth;
>  	unsigned int colorimetry;
>  	unsigned int pix_repet_factor;
> @@ -569,7 +563,7 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
>  	int color_format = 0;
>  	u8 val;
> 
> -	if (hdmi->hdmi_data.enc_in_format == RGB) {
> +	if (hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_RGB) {
>  		if (hdmi->hdmi_data.enc_color_depth == 8)
>  			color_format = 0x01;
>  		else if (hdmi->hdmi_data.enc_color_depth == 10)
> @@ -580,7 +574,7 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
>  			color_format = 0x07;
>  		else
>  			return;
> -	} else if (hdmi->hdmi_data.enc_in_format == YCBCR444) {
> +	} else if (hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_YCBCR444) 
{
>  		if (hdmi->hdmi_data.enc_color_depth == 8)
>  			color_format = 0x09;
>  		else if (hdmi->hdmi_data.enc_color_depth == 10)
> @@ -591,7 +585,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
>  			color_format = 0x0F;
>  		else
>  			return;
> -	} else if (hdmi->hdmi_data.enc_in_format == YCBCR422_8BITS) {
> +	} else if (hdmi->hdmi_data.enc_in_format ==
> +					DW_HDMI_ENC_FMT_YCBCR422_8BITS) {
>  		if (hdmi->hdmi_data.enc_color_depth == 8)
>  			color_format = 0x16;
>  		else if (hdmi->hdmi_data.enc_color_depth == 10)
> @@ -627,20 +622,20 @@ static int is_color_space_conversion(struct dw_hdmi
> *hdmi)
> 
>  static int is_color_space_decimation(struct dw_hdmi *hdmi)
>  {
> -	if (hdmi->hdmi_data.enc_out_format != YCBCR422_8BITS)
> +	if (hdmi->hdmi_data.enc_out_format != DW_HDMI_ENC_FMT_YCBCR422_8BITS)
>  		return 0;
> -	if (hdmi->hdmi_data.enc_in_format == RGB ||
> -	    hdmi->hdmi_data.enc_in_format == YCBCR444)
> +	if (hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_RGB ||
> +	    hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_YCBCR444)
>  		return 1;
>  	return 0;
>  }
> 
>  static int is_color_space_interpolation(struct dw_hdmi *hdmi)
>  {
> -	if (hdmi->hdmi_data.enc_in_format != YCBCR422_8BITS)
> +	if (hdmi->hdmi_data.enc_in_format != DW_HDMI_ENC_FMT_YCBCR422_8BITS)
>  		return 0;
> -	if (hdmi->hdmi_data.enc_out_format == RGB ||
> -	    hdmi->hdmi_data.enc_out_format == YCBCR444)
> +	if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_RGB ||
> +	    hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_YCBCR444)
>  		return 1;
>  	return 0;
>  }
> @@ -652,13 +647,14 @@ static void dw_hdmi_update_csc_coeffs(struct dw_hdmi
> *hdmi) u32 csc_scale = 1;
> 
>  	if (is_color_space_conversion(hdmi)) {
> -		if (hdmi->hdmi_data.enc_out_format == RGB) {
> +		if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_RGB) {
>  			if (hdmi->hdmi_data.colorimetry ==
>  					HDMI_COLORIMETRY_ITU_601)
>  				csc_coeff = &csc_coeff_rgb_out_eitu601;
>  			else
>  				csc_coeff = &csc_coeff_rgb_out_eitu709;
> -		} else if (hdmi->hdmi_data.enc_in_format == RGB) {
> +		} else if (hdmi->hdmi_data.enc_in_format ==
> +					DW_HDMI_ENC_FMT_RGB) {
>  			if (hdmi->hdmi_data.colorimetry ==
>  					HDMI_COLORIMETRY_ITU_601)
>  				csc_coeff = &csc_coeff_rgb_in_eitu601;
> @@ -730,8 +726,8 @@ static void hdmi_video_packetize(struct dw_hdmi *hdmi)
>  	struct hdmi_data_info *hdmi_data = &hdmi->hdmi_data;
>  	u8 val, vp_conf;
> 
> -	if (hdmi_data->enc_out_format == RGB ||
> -	    hdmi_data->enc_out_format == YCBCR444) {
> +	if (hdmi_data->enc_out_format == DW_HDMI_ENC_FMT_RGB ||
> +	    hdmi_data->enc_out_format == DW_HDMI_ENC_FMT_YCBCR444) {
>  		if (!hdmi_data->enc_color_depth) {
>  			output_select = HDMI_VP_CONF_OUTPUT_SELECTOR_BYPASS;
>  		} else if (hdmi_data->enc_color_depth == 8) {
> @@ -746,7 +742,8 @@ static void hdmi_video_packetize(struct dw_hdmi *hdmi)
>  		} else {
>  			return;
>  		}
> -	} else if (hdmi_data->enc_out_format == YCBCR422_8BITS) {
> +	} else if (hdmi_data->enc_out_format ==
> +					DW_HDMI_ENC_FMT_YCBCR422_8BITS) {
>  		if (!hdmi_data->enc_color_depth ||
>  		    hdmi_data->enc_color_depth == 8)
>  			remap_size = HDMI_VP_REMAP_YCC422_16bit;
> @@ -1138,15 +1135,16 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi,
> struct drm_display_mode *mode) /* Initialise info frame from DRM mode */
>  	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode);
> 
> -	if (hdmi->hdmi_data.enc_out_format == YCBCR444)
> +	if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_YCBCR444)
>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> -	else if (hdmi->hdmi_data.enc_out_format == YCBCR422_8BITS)
> +	else if (hdmi->hdmi_data.enc_out_format ==
> +					DW_HDMI_ENC_FMT_YCBCR422_8BITS)
>  		frame.colorspace = HDMI_COLORSPACE_YUV422;
>  	else
>  		frame.colorspace = HDMI_COLORSPACE_RGB;
> 
>  	/* Set up colorimetry */
> -	if (hdmi->hdmi_data.enc_out_format == XVYCC444) {
> +	if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_XVYCC444) {
>  		frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
>  		if (hdmi->hdmi_data.colorimetry == HDMI_COLORIMETRY_ITU_601)
>  			frame.extended_colorimetry =
> @@ -1154,7 +1152,7 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi,
> struct drm_display_mode *mode) else /*hdmi->hdmi_data.colorimetry ==
> HDMI_COLORIMETRY_ITU_709*/ frame.extended_colorimetry =
>  				HDMI_EXTENDED_COLORIMETRY_XV_YCC_709;
> -	} else if (hdmi->hdmi_data.enc_out_format != RGB) {
> +	} else if (hdmi->hdmi_data.enc_out_format != DW_HDMI_ENC_FMT_RGB) {
>  		frame.colorimetry = hdmi->hdmi_data.colorimetry;
>  		frame.extended_colorimetry = 
HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
>  	} else { /* Carries no data */
> @@ -1443,10 +1441,13 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
> struct drm_display_mode *mode)
> hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0;
>  	hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;
> 
> -	/* TODO: Get input format from IPU (via FB driver interface) */
> -	hdmi->hdmi_data.enc_in_format = RGB;
> +	/* Get input format from plat data or fallback to RGB */
> +	if (hdmi->plat_data->input_fmt >= 0)
> +		hdmi->hdmi_data.enc_in_format = hdmi->plat_data->input_fmt;
> +	else
> +		hdmi->hdmi_data.enc_in_format = DW_HDMI_ENC_FMT_RGB;
> 
> -	hdmi->hdmi_data.enc_out_format = RGB;
> +	hdmi->hdmi_data.enc_out_format = DW_HDMI_ENC_FMT_RGB;
> 
>  	hdmi->hdmi_data.enc_color_depth = 8;
>  	hdmi->hdmi_data.pix_repet_factor = 0;
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index bcceee8..8c0cc13 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -21,6 +21,14 @@ enum {
>  	DW_HDMI_RES_MAX,
>  };
> 
> +enum dw_hdmi_color_enc_format {
> +	DW_HDMI_ENC_FMT_RGB = 0,
> +	DW_HDMI_ENC_FMT_YCBCR444,
> +	DW_HDMI_ENC_FMT_YCBCR422_16BITS,
> +	DW_HDMI_ENC_FMT_YCBCR422_8BITS,
> +	DW_HDMI_ENC_FMT_XVYCC444,
> +};
> +
>  enum dw_hdmi_phy_type {
>  	DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
>  	DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
> @@ -62,6 +70,7 @@ struct dw_hdmi_plat_data {
>  	struct regmap *regm;
>  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>  					   struct drm_display_mode *mode);
> +	enum dw_hdmi_color_enc_format input_fmt;

As reported before, I think we should use a combination of media bus format 
and color space information instead of a custom enum. Ideally this should also 
be queried at runtime, but that could be done in a second step.

> 
>  	/* Vendor PHY support */
>  	const struct dw_hdmi_phy_ops *phy_ops;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] drm: bridge: dw-hdmi: Take input format from plat_data

[Neil Armstrong]

On 03/02/2017 04:45 PM, Laurent Pinchart wrote:
> Hi Neil,
> 
> Thank you for the patch.
> 
> On Thursday 02 Mar 2017 16:29:31 Neil Armstrong wrote:
>> Some display pipelines can only provide non-RBG input pixels to the HDMI TX
>> Controller, this patch takes the pixel format from the plat_data if
>> provided.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/bridge/dw-hdmi.c | 59 ++++++++++++++++++-------------------
>>  include/drm/bridge/dw_hdmi.h     |  9 ++++++
>>  2 files changed, 39 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
>> b/drivers/gpu/drm/bridge/dw-hdmi.c index 026a0dc..653ecd7 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -35,12 +35,6 @@
>>
>>  #define HDMI_EDID_LEN		512
>>
>> -#define RGB			0
>> -#define YCBCR444		1
>> -#define YCBCR422_16BITS		2
>> -#define YCBCR422_8BITS		3
>> -#define XVYCC444		4
>> -
>>  enum hdmi_datamap {
>>  	RGB444_8B = 0x01,
>>  	RGB444_10B = 0x03,
>> @@ -94,8 +88,8 @@ struct hdmi_vmode {
>>  };
>>
>>  struct hdmi_data_info {
>> -	unsigned int enc_in_format;
>> -	unsigned int enc_out_format;
>> +	enum dw_hdmi_color_enc_format enc_in_format;
>> +	enum dw_hdmi_color_enc_format enc_out_format;
>>  	unsigned int enc_color_depth;
>>  	unsigned int colorimetry;
>>  	unsigned int pix_repet_factor;
>> @@ -569,7 +563,7 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
>>  	int color_format = 0;
>>  	u8 val;
>>
>> -	if (hdmi->hdmi_data.enc_in_format == RGB) {
>> +	if (hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_RGB) {
>>  		if (hdmi->hdmi_data.enc_color_depth == 8)
>>  			color_format = 0x01;
>>  		else if (hdmi->hdmi_data.enc_color_depth == 10)
>> @@ -580,7 +574,7 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
>>  			color_format = 0x07;
>>  		else
>>  			return;
>> -	} else if (hdmi->hdmi_data.enc_in_format == YCBCR444) {
>> +	} else if (hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_YCBCR444) 
> {
>>  		if (hdmi->hdmi_data.enc_color_depth == 8)
>>  			color_format = 0x09;
>>  		else if (hdmi->hdmi_data.enc_color_depth == 10)
>> @@ -591,7 +585,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
>>  			color_format = 0x0F;
>>  		else
>>  			return;
>> -	} else if (hdmi->hdmi_data.enc_in_format == YCBCR422_8BITS) {
>> +	} else if (hdmi->hdmi_data.enc_in_format ==
>> +					DW_HDMI_ENC_FMT_YCBCR422_8BITS) {
>>  		if (hdmi->hdmi_data.enc_color_depth == 8)
>>  			color_format = 0x16;
>>  		else if (hdmi->hdmi_data.enc_color_depth == 10)
>> @@ -627,20 +622,20 @@ static int is_color_space_conversion(struct dw_hdmi
>> *hdmi)
>>
>>  static int is_color_space_decimation(struct dw_hdmi *hdmi)
>>  {
>> -	if (hdmi->hdmi_data.enc_out_format != YCBCR422_8BITS)
>> +	if (hdmi->hdmi_data.enc_out_format != DW_HDMI_ENC_FMT_YCBCR422_8BITS)
>>  		return 0;
>> -	if (hdmi->hdmi_data.enc_in_format == RGB ||
>> -	    hdmi->hdmi_data.enc_in_format == YCBCR444)
>> +	if (hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_RGB ||
>> +	    hdmi->hdmi_data.enc_in_format == DW_HDMI_ENC_FMT_YCBCR444)
>>  		return 1;
>>  	return 0;
>>  }
>>
>>  static int is_color_space_interpolation(struct dw_hdmi *hdmi)
>>  {
>> -	if (hdmi->hdmi_data.enc_in_format != YCBCR422_8BITS)
>> +	if (hdmi->hdmi_data.enc_in_format != DW_HDMI_ENC_FMT_YCBCR422_8BITS)
>>  		return 0;
>> -	if (hdmi->hdmi_data.enc_out_format == RGB ||
>> -	    hdmi->hdmi_data.enc_out_format == YCBCR444)
>> +	if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_RGB ||
>> +	    hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_YCBCR444)
>>  		return 1;
>>  	return 0;
>>  }
>> @@ -652,13 +647,14 @@ static void dw_hdmi_update_csc_coeffs(struct dw_hdmi
>> *hdmi) u32 csc_scale = 1;
>>
>>  	if (is_color_space_conversion(hdmi)) {
>> -		if (hdmi->hdmi_data.enc_out_format == RGB) {
>> +		if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_RGB) {
>>  			if (hdmi->hdmi_data.colorimetry ==
>>  					HDMI_COLORIMETRY_ITU_601)
>>  				csc_coeff = &csc_coeff_rgb_out_eitu601;
>>  			else
>>  				csc_coeff = &csc_coeff_rgb_out_eitu709;
>> -		} else if (hdmi->hdmi_data.enc_in_format == RGB) {
>> +		} else if (hdmi->hdmi_data.enc_in_format ==
>> +					DW_HDMI_ENC_FMT_RGB) {
>>  			if (hdmi->hdmi_data.colorimetry ==
>>  					HDMI_COLORIMETRY_ITU_601)
>>  				csc_coeff = &csc_coeff_rgb_in_eitu601;
>> @@ -730,8 +726,8 @@ static void hdmi_video_packetize(struct dw_hdmi *hdmi)
>>  	struct hdmi_data_info *hdmi_data = &hdmi->hdmi_data;
>>  	u8 val, vp_conf;
>>
>> -	if (hdmi_data->enc_out_format == RGB ||
>> -	    hdmi_data->enc_out_format == YCBCR444) {
>> +	if (hdmi_data->enc_out_format == DW_HDMI_ENC_FMT_RGB ||
>> +	    hdmi_data->enc_out_format == DW_HDMI_ENC_FMT_YCBCR444) {
>>  		if (!hdmi_data->enc_color_depth) {
>>  			output_select = HDMI_VP_CONF_OUTPUT_SELECTOR_BYPASS;
>>  		} else if (hdmi_data->enc_color_depth == 8) {
>> @@ -746,7 +742,8 @@ static void hdmi_video_packetize(struct dw_hdmi *hdmi)
>>  		} else {
>>  			return;
>>  		}
>> -	} else if (hdmi_data->enc_out_format == YCBCR422_8BITS) {
>> +	} else if (hdmi_data->enc_out_format ==
>> +					DW_HDMI_ENC_FMT_YCBCR422_8BITS) {
>>  		if (!hdmi_data->enc_color_depth ||
>>  		    hdmi_data->enc_color_depth == 8)
>>  			remap_size = HDMI_VP_REMAP_YCC422_16bit;
>> @@ -1138,15 +1135,16 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi,
>> struct drm_display_mode *mode) /* Initialise info frame from DRM mode */
>>  	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode);
>>
>> -	if (hdmi->hdmi_data.enc_out_format == YCBCR444)
>> +	if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_YCBCR444)
>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
>> -	else if (hdmi->hdmi_data.enc_out_format == YCBCR422_8BITS)
>> +	else if (hdmi->hdmi_data.enc_out_format ==
>> +					DW_HDMI_ENC_FMT_YCBCR422_8BITS)
>>  		frame.colorspace = HDMI_COLORSPACE_YUV422;
>>  	else
>>  		frame.colorspace = HDMI_COLORSPACE_RGB;
>>
>>  	/* Set up colorimetry */
>> -	if (hdmi->hdmi_data.enc_out_format == XVYCC444) {
>> +	if (hdmi->hdmi_data.enc_out_format == DW_HDMI_ENC_FMT_XVYCC444) {
>>  		frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
>>  		if (hdmi->hdmi_data.colorimetry == HDMI_COLORIMETRY_ITU_601)
>>  			frame.extended_colorimetry =
>> @@ -1154,7 +1152,7 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi,
>> struct drm_display_mode *mode) else /*hdmi->hdmi_data.colorimetry ==
>> HDMI_COLORIMETRY_ITU_709*/ frame.extended_colorimetry =
>>  				HDMI_EXTENDED_COLORIMETRY_XV_YCC_709;
>> -	} else if (hdmi->hdmi_data.enc_out_format != RGB) {
>> +	} else if (hdmi->hdmi_data.enc_out_format != DW_HDMI_ENC_FMT_RGB) {
>>  		frame.colorimetry = hdmi->hdmi_data.colorimetry;
>>  		frame.extended_colorimetry = 
> HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
>>  	} else { /* Carries no data */
>> @@ -1443,10 +1441,13 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
>> struct drm_display_mode *mode)
>> hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0;
>>  	hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;
>>
>> -	/* TODO: Get input format from IPU (via FB driver interface) */
>> -	hdmi->hdmi_data.enc_in_format = RGB;
>> +	/* Get input format from plat data or fallback to RGB */
>> +	if (hdmi->plat_data->input_fmt >= 0)
>> +		hdmi->hdmi_data.enc_in_format = hdmi->plat_data->input_fmt;
>> +	else
>> +		hdmi->hdmi_data.enc_in_format = DW_HDMI_ENC_FMT_RGB;
>>
>> -	hdmi->hdmi_data.enc_out_format = RGB;
>> +	hdmi->hdmi_data.enc_out_format = DW_HDMI_ENC_FMT_RGB;
>>
>>  	hdmi->hdmi_data.enc_color_depth = 8;
>>  	hdmi->hdmi_data.pix_repet_factor = 0;
>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>> index bcceee8..8c0cc13 100644
>> --- a/include/drm/bridge/dw_hdmi.h
>> +++ b/include/drm/bridge/dw_hdmi.h
>> @@ -21,6 +21,14 @@ enum {
>>  	DW_HDMI_RES_MAX,
>>  };
>>
>> +enum dw_hdmi_color_enc_format {
>> +	DW_HDMI_ENC_FMT_RGB = 0,
>> +	DW_HDMI_ENC_FMT_YCBCR444,
>> +	DW_HDMI_ENC_FMT_YCBCR422_16BITS,
>> +	DW_HDMI_ENC_FMT_YCBCR422_8BITS,
>> +	DW_HDMI_ENC_FMT_XVYCC444,
>> +};
>> +
>>  enum dw_hdmi_phy_type {
>>  	DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
>>  	DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
>> @@ -62,6 +70,7 @@ struct dw_hdmi_plat_data {
>>  	struct regmap *regm;
>>  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>>  					   struct drm_display_mode *mode);
>> +	enum dw_hdmi_color_enc_format input_fmt;
> 
> As reported before, I think we should use a combination of media bus format 
> and color space information instead of a custom enum. Ideally this should also 
> be queried at runtime, but that could be done in a second step.
> 

Sure, but I was struggling about finding an equivalence.

How should I replace these ?

DW_HDMI_ENC_FMT_RGB
DW_HDMI_ENC_FMT_YCBCR444
DW_HDMI_ENC_FMT_YCBCR422_16BITS
DW_HDMI_ENC_FMT_YCBCR422_8BITS
DW_HDMI_ENC_FMT_XVYCC444

I do not have the strict information about the bus format, what is RGB in 8bit per pixel ?
Are the 3 samples transmitted separately ?
What should be the RGB colorspace ?
Should we use the "Packed" formats, LVDS or a new buf format ?

Jose, what are the *exact* bus formats supported ?

Same for DW_HDMI_ENC_FMT_YCBCR* stuff, are they packed ?

I understand the YCBCR444/XVYCC444 needs a color space information instead.

Could we use the following list ?

Bus Format			| Colorspace 		| Encoding
-------------------------------------------------------------------------------
MEDIA_BUS_FMT_RGB888_1X24	| V4L2_COLORSPACE_SRGB	| V4L2_XFER_FUNC_SRGB
MEDIA_BUS_FMT_RGB101010_1X30	| V4L2_COLORSPACE_SRGB	| V4L2_XFER_FUNC_SRGB
MEDIA_BUS_FMT_RGB121212_1X36	| V4L2_COLORSPACE_SRGB	| V4L2_XFER_FUNC_SRGB
MEDIA_BUS_FMT_VUY8_1X24		| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
MEDIA_BUS_FMT_YUV10_1X30	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
MEDIA_BUS_FMT_YUV10_1X32	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
MEDIA_BUS_FMT_YUV10_1X32	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
MEDIA_BUS_FMT_YUV10_1X32	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_XV709

But all of these is complex, and I'm not sure how we should handle SDTV modes here,
and without knowing the exact inputs types supported by the IP, this needs to be
confirmed.

Meanwhile, all this can be fixed later, at least this patch moves the
definition out of the C file and uses better names for these custom enums.

>>
>>  	/* Vendor PHY support */
>>  	const struct dw_hdmi_phy_ops *phy_ops;
> 

Re: [PATCH v2 1/2] drm: bridge: dw-hdmi: Take input format from plat_data

[Jose Abreu]

Hi Neil,


On 03-03-2017 11:31, Neil Armstrong wrote:
>
> Sure, but I was struggling about finding an equivalence.
>
> How should I replace these ?
>
> DW_HDMI_ENC_FMT_RGB
> DW_HDMI_ENC_FMT_YCBCR444
> DW_HDMI_ENC_FMT_YCBCR422_16BITS
> DW_HDMI_ENC_FMT_YCBCR422_8BITS
> DW_HDMI_ENC_FMT_XVYCC444
>
> I do not have the strict information about the bus format, what is RGB in 8bit per pixel ?
> Are the 3 samples transmitted separately ?
> What should be the RGB colorspace ?
> Should we use the "Packed" formats, LVDS or a new buf format ?
>
> Jose, what are the *exact* bus formats supported ?

Well, the controller supports everything that is in the HDMI spec
(1.4 and 2.0), the implementation is the one that limits the
supported formats. There is also a CSC but it does not convert
from anything to everything :)

I think you can safely add every MBUS code that is compatible
with HDMI. Namely:
    -    24/30/36/48-bit RGB 4:4:4
    -    24/30/36/48-bit YCbCr 4:4:4
    -    16/20/24-bit YCbCr 4:2:2
    -    24/30/36/48-bit YCbCr 4:2:0

And then let the glue drivers decide which they support in input
and what they want in output (the output is a little tricky
because it will depend in EDID, this should be handled by
userspace + drm core and not the drivers so let it fixed to RGB,
just in case).

>
> Same for DW_HDMI_ENC_FMT_YCBCR* stuff, are they packed ?

Hmm, this depends on the implementation. The controller always
receives 48bits of data per pixel, its the implementation that is
responsible to correctly align that data.

>
> I understand the YCBCR444/XVYCC444 needs a color space information instead.
>
> Could we use the following list ?
>
> Bus Format			| Colorspace 		| Encoding
> -------------------------------------------------------------------------------
> MEDIA_BUS_FMT_RGB888_1X24	| V4L2_COLORSPACE_SRGB	| V4L2_XFER_FUNC_SRGB
> MEDIA_BUS_FMT_RGB101010_1X30	| V4L2_COLORSPACE_SRGB	| V4L2_XFER_FUNC_SRGB
> MEDIA_BUS_FMT_RGB121212_1X36	| V4L2_COLORSPACE_SRGB	| V4L2_XFER_FUNC_SRGB
> MEDIA_BUS_FMT_VUY8_1X24		| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
> MEDIA_BUS_FMT_YUV10_1X30	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
> MEDIA_BUS_FMT_YUV10_1X32	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
> MEDIA_BUS_FMT_YUV10_1X32	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
> MEDIA_BUS_FMT_YUV10_1X32	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_XV709

I think the HDMI spec dictates the default colorspace+encoding
for each bus format, not sure though, Laurent?

Best regards,
Jose Miguel Abreu

>
> But all of these is complex, and I'm not sure how we should handle SDTV modes here,
> and without knowing the exact inputs types supported by the IP, this needs to be
> confirmed.
>
> Meanwhile, all this can be fixed later, at least this patch moves the
> definition out of the C file and uses better names for these custom enums.
>

Re: [PATCH v2 1/2] drm: bridge: dw-hdmi: Take input format from plat_data

[Neil Armstrong]

On 03/03/2017 05:39 PM, Jose Abreu wrote:
> Hi Neil,
> 
> 
> On 03-03-2017 11:31, Neil Armstrong wrote:
>>
>> Sure, but I was struggling about finding an equivalence.
>>
>> How should I replace these ?
>>
>> DW_HDMI_ENC_FMT_RGB
>> DW_HDMI_ENC_FMT_YCBCR444
>> DW_HDMI_ENC_FMT_YCBCR422_16BITS
>> DW_HDMI_ENC_FMT_YCBCR422_8BITS
>> DW_HDMI_ENC_FMT_XVYCC444
>>
>> I do not have the strict information about the bus format, what is RGB in 8bit per pixel ?
>> Are the 3 samples transmitted separately ?
>> What should be the RGB colorspace ?
>> Should we use the "Packed" formats, LVDS or a new buf format ?
>>
>> Jose, what are the *exact* bus formats supported ?
> 
> Well, the controller supports everything that is in the HDMI spec
> (1.4 and 2.0), the implementation is the one that limits the
> supported formats. There is also a CSC but it does not convert
> from anything to everything :)

Sure, I was meaning the *input* format the controller receives from the
pixel encoder, I'm quite sure the format is strict.

> 
> I think you can safely add every MBUS code that is compatible
> with HDMI. Namely:
>     -    24/30/36/48-bit RGB 4:4:4
>     -    24/30/36/48-bit YCbCr 4:4:4
>     -    16/20/24-bit YCbCr 4:2:2
>     -    24/30/36/48-bit YCbCr 4:2:0
> 
> And then let the glue drivers decide which they support in input
> and what they want in output (the output is a little tricky
> because it will depend in EDID, this should be handled by
> userspace + drm core and not the drivers so let it fixed to RGB,
> just in case).
> 
>>
>> Same for DW_HDMI_ENC_FMT_YCBCR* stuff, are they packed ?
> 
> Hmm, this depends on the implementation. The controller always
> receives 48bits of data per pixel, its the implementation that is
> responsible to correctly align that data.
> 
>>
>> I understand the YCBCR444/XVYCC444 needs a color space information instead.
>>
>> Could we use the following list ?
>>
>> Bus Format			| Colorspace 		| Encoding
>> -------------------------------------------------------------------------------
>> MEDIA_BUS_FMT_RGB888_1X24	| V4L2_COLORSPACE_SRGB	| V4L2_XFER_FUNC_SRGB
>> MEDIA_BUS_FMT_RGB101010_1X30	| V4L2_COLORSPACE_SRGB	| V4L2_XFER_FUNC_SRGB
>> MEDIA_BUS_FMT_RGB121212_1X36	| V4L2_COLORSPACE_SRGB	| V4L2_XFER_FUNC_SRGB
>> MEDIA_BUS_FMT_VUY8_1X24		| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
>> MEDIA_BUS_FMT_YUV10_1X30	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
>> MEDIA_BUS_FMT_YUV10_1X32	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
>> MEDIA_BUS_FMT_YUV10_1X32	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_709
>> MEDIA_BUS_FMT_YUV10_1X32	| V4L2_XFER_FUNC_709	| V4L2_YCBCR_ENC_XV709
> 
> I think the HDMI spec dictates the default colorspace+encoding
> for each bus format, not sure though, Laurent?
> 
> Best regards,
> Jose Miguel Abreu
> 
>>
>> But all of these is complex, and I'm not sure how we should handle SDTV modes here,
>> and without knowing the exact inputs types supported by the IP, this needs to be
>> confirmed.
>>
>> Meanwhile, all this can be fixed later, at least this patch moves the
>> definition out of the C file and uses better names for these custom enums.
>>
> 

Re: [PATCH v2 1/2] drm: bridge: dw-hdmi: Take input format from plat_data

[Jose Abreu]

Hi Neil,


On 03-03-2017 16:42, Neil Armstrong wrote:
>
> Sure, I was meaning the *input* format the controller receives from the
> pixel encoder, I'm quite sure the format is strict.
>

Hmm, not quite following you here. As far as the controller goes
it supports the formats I mentioned:
    -    8/10/12/16 bits RGB 4:4:4
    -    8/10/12/16 bits YCbCr 4:4:4
    -    8/10/12 bits YCbCr 4:2:2
    -    8/10/12/16 bits YCbCr 4:2:0

As for the CSC it supports RGB 4:4:4 to/from YCbCr 4:4:4 or 4:2:2
in every defined color depth.

So, everything except 4:2:0 (I had to check documentation, I
though that CSC supported less formats).

Of course this is all limited by the implementation that HW team
decides to choose.

Best regards,
Jose Miguel Abreu

Re: [PATCH v2 1/2] drm: bridge: dw-hdmi: Take input format from plat_data

[Neil Armstrong]

On 03/03/2017 06:22 PM, Jose Abreu wrote:
> Hi Neil,
> 
> 
> On 03-03-2017 16:42, Neil Armstrong wrote:
>>
>> Sure, I was meaning the *input* format the controller receives from the
>> pixel encoder, I'm quite sure the format is strict.
>>
> 
> Hmm, not quite following you here. As far as the controller goes
> it supports the formats I mentioned:
>     -    8/10/12/16 bits RGB 4:4:4
>     -    8/10/12/16 bits YCbCr 4:4:4
>     -    8/10/12 bits YCbCr 4:2:2
>     -    8/10/12/16 bits YCbCr 4:2:0
> 
> As for the CSC it supports RGB 4:4:4 to/from YCbCr 4:4:4 or 4:2:2
> in every defined color depth.
> 
> So, everything except 4:2:0 (I had to check documentation, I
> though that CSC supported less formats).
> 
> Of course this is all limited by the implementation that HW team
> decides to choose.
> 
> Best regards,
> Jose Miguel Abreu
> 

Hi Jose,

Thanks for the clarifications.

I will try to add missing V4L formats

 - 8/10/12/16 bits RGB 4:4:4

MEDIA_BUS_FMT_RGB888_1X24
MEDIA_BUS_FMT_RGB101010_1X30 (to be added)
MEDIA_BUS_FMT_RGB121212_1X36 (to be added)
MEDIA_BUS_FMT_RGB161616_1X48 (to be added)

 - 8/10/12/16 bits YCbCr 4:4:4

MEDIA_BUS_FMT_YUV8_1X24
MEDIA_BUS_FMT_YUV10_1X30
MEDIA_BUS_FMT_YUV12_1X36 (to be added)
MEDIA_BUS_FMT_YUV16_1X48 (to be added)

 - 8/10/12 bits YCbCr 4:2:2

MEDIA_BUS_FMT_UYVY8_1X16
MEDIA_BUS_FMT_UYVY10_1X20
MEDIA_BUS_FMT_UYVY12_1X24

 - 8/10/12/16 bits YCbCr 4:2:0

Jose, how is supposed to be transmitted 4:2:0 over the controller ?

Amlogic uses clock doubling to transmit 4:2:0 in CrYCb format.

I assume, the transmission is in YYUYYV format in 2x clock.

So the formats should be (aligned on the MEDIA_BUS_FMT_YUYV8_1_5X8) :

MEDIA_BUS_FMT_YUYV8_1_1X24 (to be added)
MEDIA_BUS_FMT_YUYV10_1_1X30 (to be added)
MEDIA_BUS_FMT_YUYV12_1_1X36 (to be added)
MEDIA_BUS_FMT_YUYV16_1_1X48 (to be added)

to have the following transmissions scheme :

uuuuuuuu/yyyyyyyy/yyyyyyyy
vvvvvvvv/yyyyyyyy/yyyyyyyy

Can you confirm this ?

Neil

Re: [PATCH v2 1/2] drm: bridge: dw-hdmi: Take input format from plat_data

[Jose Abreu]

Hi Neil,


On 06-03-2017 10:41, Neil Armstrong wrote:
> On 03/03/2017 06:22 PM, Jose Abreu wrote:
>> Hi Neil,
>>
>>
>> On 03-03-2017 16:42, Neil Armstrong wrote:
>>> Sure, I was meaning the *input* format the controller receives from the
>>> pixel encoder, I'm quite sure the format is strict.
>>>
>> Hmm, not quite following you here. As far as the controller goes
>> it supports the formats I mentioned:
>>     -    8/10/12/16 bits RGB 4:4:4
>>     -    8/10/12/16 bits YCbCr 4:4:4
>>     -    8/10/12 bits YCbCr 4:2:2
>>     -    8/10/12/16 bits YCbCr 4:2:0
>>
>> As for the CSC it supports RGB 4:4:4 to/from YCbCr 4:4:4 or 4:2:2
>> in every defined color depth.
>>
>> So, everything except 4:2:0 (I had to check documentation, I
>> though that CSC supported less formats).
>>
>> Of course this is all limited by the implementation that HW team
>> decides to choose.
>>
>> Best regards,
>> Jose Miguel Abreu
>>
> Hi Jose,
>
> Thanks for the clarifications.
>
> I will try to add missing V4L formats
>
>  - 8/10/12/16 bits RGB 4:4:4
>
> MEDIA_BUS_FMT_RGB888_1X24
> MEDIA_BUS_FMT_RGB101010_1X30 (to be added)
> MEDIA_BUS_FMT_RGB121212_1X36 (to be added)
> MEDIA_BUS_FMT_RGB161616_1X48 (to be added)
>
>  - 8/10/12/16 bits YCbCr 4:4:4
>
> MEDIA_BUS_FMT_YUV8_1X24
> MEDIA_BUS_FMT_YUV10_1X30
> MEDIA_BUS_FMT_YUV12_1X36 (to be added)
> MEDIA_BUS_FMT_YUV16_1X48 (to be added)
>
>  - 8/10/12 bits YCbCr 4:2:2
>
> MEDIA_BUS_FMT_UYVY8_1X16
> MEDIA_BUS_FMT_UYVY10_1X20
> MEDIA_BUS_FMT_UYVY12_1X24
>
>  - 8/10/12/16 bits YCbCr 4:2:0
>
> Jose, how is supposed to be transmitted 4:2:0 over the controller ?
>
> Amlogic uses clock doubling to transmit 4:2:0 in CrYCb format.
>
> I assume, the transmission is in YYUYYV format in 2x clock.

Double rate or half rate? 4:2:0 requires half the bandwidth and
the horizontal parameters are cut off by half also. I'm not very
familiar with 4:2:0 but according to spec it shall be Cb[l][n]
Y[l][n] Y[l][n+1] in one line and then Cr[l][n] Y[l+1][n]
Y[l+1][n+1] in another line (where "l" is pixel line number and
"n" is pixel number).

I've worked with 4:2:0 previously but it was just for a prof of
concept (I never got to a point where I could submit anything
standard), maybe someone who has worked with this can expand a
little bit. Anyone?

>
> So the formats should be (aligned on the MEDIA_BUS_FMT_YUYV8_1_5X8) :
>
> MEDIA_BUS_FMT_YUYV8_1_1X24 (to be added)
> MEDIA_BUS_FMT_YUYV10_1_1X30 (to be added)
> MEDIA_BUS_FMT_YUYV12_1_1X36 (to be added)
> MEDIA_BUS_FMT_YUYV16_1_1X48 (to be added)
>
> to have the following transmissions scheme :
>
> uuuuuuuu/yyyyyyyy/yyyyyyyy
> vvvvvvvv/yyyyyyyy/yyyyyyyy
>
> Can you confirm this ?

Seems fine, chroma in consecutive lines and luma in every line.

Best regards,
Jose Miguel Abreu

>
> Neil

Re: [PATCH v2 1/2] drm: bridge: dw-hdmi: Take input format from plat_data

[Neil Armstrong]

On 03/06/2017 01:17 PM, Jose Abreu wrote:
> Hi Neil,
> 
> 
> On 06-03-2017 10:41, Neil Armstrong wrote:
>> On 03/03/2017 06:22 PM, Jose Abreu wrote:
>>> Hi Neil,
>>>
>>>
>>> On 03-03-2017 16:42, Neil Armstrong wrote:
>>>> Sure, I was meaning the *input* format the controller receives from the
>>>> pixel encoder, I'm quite sure the format is strict.
>>>>
>>> Hmm, not quite following you here. As far as the controller goes
>>> it supports the formats I mentioned:
>>>     -    8/10/12/16 bits RGB 4:4:4
>>>     -    8/10/12/16 bits YCbCr 4:4:4
>>>     -    8/10/12 bits YCbCr 4:2:2
>>>     -    8/10/12/16 bits YCbCr 4:2:0
>>>
>>> As for the CSC it supports RGB 4:4:4 to/from YCbCr 4:4:4 or 4:2:2
>>> in every defined color depth.
>>>
>>> So, everything except 4:2:0 (I had to check documentation, I
>>> though that CSC supported less formats).
>>>
>>> Of course this is all limited by the implementation that HW team
>>> decides to choose.
>>>
>>> Best regards,
>>> Jose Miguel Abreu
>>>
>> Hi Jose,
>>
>> Thanks for the clarifications.
>>
>> I will try to add missing V4L formats
>>
>>  - 8/10/12/16 bits RGB 4:4:4
>>
>> MEDIA_BUS_FMT_RGB888_1X24
>> MEDIA_BUS_FMT_RGB101010_1X30 (to be added)
>> MEDIA_BUS_FMT_RGB121212_1X36 (to be added)
>> MEDIA_BUS_FMT_RGB161616_1X48 (to be added)
>>
>>  - 8/10/12/16 bits YCbCr 4:4:4
>>
>> MEDIA_BUS_FMT_YUV8_1X24
>> MEDIA_BUS_FMT_YUV10_1X30
>> MEDIA_BUS_FMT_YUV12_1X36 (to be added)
>> MEDIA_BUS_FMT_YUV16_1X48 (to be added)
>>
>>  - 8/10/12 bits YCbCr 4:2:2
>>
>> MEDIA_BUS_FMT_UYVY8_1X16
>> MEDIA_BUS_FMT_UYVY10_1X20
>> MEDIA_BUS_FMT_UYVY12_1X24
>>
>>  - 8/10/12/16 bits YCbCr 4:2:0
>>
>> Jose, how is supposed to be transmitted 4:2:0 over the controller ?
>>
>> Amlogic uses clock doubling to transmit 4:2:0 in CrYCb format.
>>
>> I assume, the transmission is in YYUYYV format in 2x clock.
> 
> Double rate or half rate? 4:2:0 requires half the bandwidth and
> the horizontal parameters are cut off by half also. I'm not very
> familiar with 4:2:0 but according to spec it shall be Cb[l][n]
> Y[l][n] Y[l][n+1] in one line and then Cr[l][n] Y[l+1][n]
> Y[l+1][n+1] in another line (where "l" is pixel line number and
> "n" is pixel number).

Exact, sorry I mismatch, Amlogic doubles the clock in the encoder and
divides the height by 2 to achieve half bandwidth.

> 
> I've worked with 4:2:0 previously but it was just for a prof of
> concept (I never got to a point where I could submit anything
> standard), maybe someone who has worked with this can expand a
> little bit. Anyone?
> 
>>
>> So the formats should be (aligned on the MEDIA_BUS_FMT_YUYV8_1_5X8) :
>>
>> MEDIA_BUS_FMT_YUYV8_1_1X24 (to be added)
>> MEDIA_BUS_FMT_YUYV10_1_1X30 (to be added)
>> MEDIA_BUS_FMT_YUYV12_1_1X36 (to be added)
>> MEDIA_BUS_FMT_YUYV16_1_1X48 (to be added)
>>
>> to have the following transmissions scheme :
>>
>> uuuuuuuu/yyyyyyyy/yyyyyyyy
>> vvvvvvvv/yyyyyyyy/yyyyyyyy
>>
>> Can you confirm this ?
> 
> Seems fine, chroma in consecutive lines and luma in every line.
>

OK then, thanks, but I mismatched, it should be like the 4:2:2 entries :

MEDIA_BUS_FMT_UYVY8_1_1X24 (to be added)
MEDIA_BUS_FMT_UYVY10_1_1X30 (to be added)
MEDIA_BUS_FMT_UYVY12_1_1X36 (to be added)
MEDIA_BUS_FMT_UYVY16_1_1X48 (to be added)


> Best regards,
> Jose Miguel Abreu
> 
>>
>> Neil
> 

[PATCH v2 2/2] drm: bridge: Move HPD handling to PHY operations

[Neil Armstrong]

The HDMI TX controller support HPD and RXSENSE signaling from the PHY
via it's STAT0 PHY interface, but some vendor PHYs can manage these
signals independently from the controller, thus these STAT0 handling
should be moved to PHY specific operations and become optional.

The existing STAT0 HPD and RXSENSE handling code is refactored into
a supplementaty set of default PHY operations that are used automatically
when the platform glue doesn't provide its own operations.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 134 ++++++++++++++++++++++++++++-----------
 include/drm/bridge/dw_hdmi.h     |   8 +++
 2 files changed, 104 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 653ecd7..58dcf7d 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1098,10 +1098,50 @@ static enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
 		connector_status_connected : connector_status_disconnected;
 }
 
+static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
+				   bool force, bool disabled, bool rxsense)
+{
+	if (force || disabled || !rxsense)
+		hdmi->phy_mask |= HDMI_PHY_RX_SENSE;
+	else
+		hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE;
+}
+
+static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data)
+{
+	/* setup HPD and RXSENSE interrupt polarity */
+	hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0);
+}
+
+static void dw_hdmi_phy_configure_hpd(struct dw_hdmi *hdmi, void *data)
+{
+	/* enable cable hot plug irq */
+	hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
+}
+
+static void dw_hdmi_phy_clear_hpd(struct dw_hdmi *hdmi, void *data)
+{
+	/* Clear Hotplug interrupts */
+	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
+		    HDMI_IH_PHY_STAT0);
+}
+
+static void dw_hdmi_phy_unmute_hpd(struct dw_hdmi *hdmi, void *data)
+{
+	/* Unmute Hotplug interrupts */
+	hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE),
+		    HDMI_IH_MUTE_PHY_STAT0);
+}
+
 static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = {
 	.init = dw_hdmi_phy_init,
 	.disable = dw_hdmi_phy_disable,
 	.read_hpd = dw_hdmi_phy_read_hpd,
+	.update_hpd = dw_hdmi_phy_update_hpd,
+	.setup_hpd = dw_hdmi_phy_setup_hpd,
+	.configure_hpd = dw_hdmi_phy_configure_hpd,
+	.clear_hpd = dw_hdmi_phy_clear_hpd,
+	.unmute_hpd = dw_hdmi_phy_unmute_hpd,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1507,11 +1547,12 @@ static int dw_hdmi_fb_registered(struct dw_hdmi *hdmi)
 		    HDMI_PHY_I2CM_CTLINT_ADDR);
 
 	/* enable cable hot plug irq */
-	hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
+	if (hdmi->phy.ops->configure_hpd)
+		hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data);
 
 	/* Clear Hotplug interrupts */
-	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
-		    HDMI_IH_PHY_STAT0);
+	if (hdmi->phy.ops->clear_hpd)
+		hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data);
 
 	return 0;
 }
@@ -1622,13 +1663,14 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi *hdmi)
 {
 	u8 old_mask = hdmi->phy_mask;
 
-	if (hdmi->force || hdmi->disabled || !hdmi->rxsense)
-		hdmi->phy_mask |= HDMI_PHY_RX_SENSE;
-	else
-		hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE;
+	if (hdmi->phy.ops->update_hpd)
+		hdmi->phy.ops->update_hpd(hdmi, hdmi->phy.data,
+					  hdmi->force, hdmi->disabled,
+					  hdmi->rxsense);
 
-	if (old_mask != hdmi->phy_mask)
-		hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
+	if (old_mask != hdmi->phy_mask &&
+	    hdmi->phy.ops->configure_hpd)
+		hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data);
 }
 
 static enum drm_connector_status
@@ -1820,6 +1862,41 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id)
 	return ret;
 }
 
+void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
+{
+	mutex_lock(&hdmi->mutex);
+
+	if (!hdmi->disabled && !hdmi->force) {
+		/*
+		 * If the RX sense status indicates we're disconnected,
+		 * clear the software rxsense status.
+		 */
+		if (!rx_sense)
+			hdmi->rxsense = false;
+
+		/*
+		 * Only set the software rxsense status when both
+		 * rxsense and hpd indicates we're connected.
+		 * This avoids what seems to be bad behaviour in
+		 * at least iMX6S versions of the phy.
+		 */
+		if (hpd)
+			hdmi->rxsense = true;
+
+		dw_hdmi_update_power(hdmi);
+		dw_hdmi_update_phy_mask(hdmi);
+	}
+	mutex_unlock(&hdmi->mutex);
+}
+
+void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
+{
+	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
+
+	__dw_hdmi_setup_rx_sense(hdmi, hpd, rx_sense);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense);
+
 static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 {
 	struct dw_hdmi *hdmi = dev_id;
@@ -1852,30 +1929,10 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 	 * ask the source to re-read the EDID.
 	 */
 	if (intr_stat &
-	    (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) {
-		mutex_lock(&hdmi->mutex);
-		if (!hdmi->disabled && !hdmi->force) {
-			/*
-			 * If the RX sense status indicates we're disconnected,
-			 * clear the software rxsense status.
-			 */
-			if (!(phy_stat & HDMI_PHY_RX_SENSE))
-				hdmi->rxsense = false;
-
-			/*
-			 * Only set the software rxsense status when both
-			 * rxsense and hpd indicates we're connected.
-			 * This avoids what seems to be bad behaviour in
-			 * at least iMX6S versions of the phy.
-			 */
-			if (phy_stat & HDMI_PHY_HPD)
-				hdmi->rxsense = true;
-
-			dw_hdmi_update_power(hdmi);
-			dw_hdmi_update_phy_mask(hdmi);
-		}
-		mutex_unlock(&hdmi->mutex);
-	}
+	    (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD))
+		__dw_hdmi_setup_rx_sense(hdmi,
+					 phy_stat & HDMI_PHY_HPD,
+					 phy_stat & HDMI_PHY_RX_SENSE);
 
 	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
 		dev_dbg(hdmi->dev, "EVENT=%s\n",
@@ -2146,11 +2203,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
 	 * Configure registers related to HDMI interrupt
 	 * generation before registering IRQ.
 	 */
-	hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0);
+	if (hdmi->phy.ops->setup_hpd)
+		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
 
 	/* Clear Hotplug interrupts */
-	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
-		    HDMI_IH_PHY_STAT0);
+	if (hdmi->phy.ops->clear_hpd)
+		hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data);
 
 	hdmi->bridge.driver_private = hdmi;
 	hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
@@ -2163,8 +2221,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
 		goto err_iahb;
 
 	/* Unmute interrupts */
-	hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE),
-		    HDMI_IH_MUTE_PHY_STAT0);
+	if (hdmi->phy.ops->unmute_hpd)
+		hdmi->phy.ops->unmute_hpd(hdmi, hdmi->phy.data);
 
 	memset(&pdevinfo, 0, sizeof(pdevinfo));
 	pdevinfo.parent = dev;
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 8c0cc13..d72403f 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -64,6 +64,12 @@ struct dw_hdmi_phy_ops {
 		    struct drm_display_mode *mode);
 	void (*disable)(struct dw_hdmi *hdmi, void *data);
 	enum drm_connector_status (*read_hpd)(struct dw_hdmi *hdmi, void *data);
+	void (*update_hpd)(struct dw_hdmi *hdmi, void *data,
+			   bool force, bool disabled, bool rxsense);
+	void (*setup_hpd)(struct dw_hdmi *hdmi, void *data);
+	void (*configure_hpd)(struct dw_hdmi *hdmi, void *data);
+	void (*clear_hpd)(struct dw_hdmi *hdmi, void *data);
+	void (*unmute_hpd)(struct dw_hdmi *hdmi, void *data);
 };
 
 struct dw_hdmi_plat_data {
@@ -93,6 +99,8 @@ int dw_hdmi_probe(struct platform_device *pdev,
 int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
 		 const struct dw_hdmi_plat_data *plat_data);
 
+void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense);
+
 void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
 void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
 void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
-- 
1.9.1

Re: [PATCH v2 2/2] drm: bridge: Move HPD handling to PHY operations

[Laurent Pinchart]

Hi Neil,

Thank you for the patch.

On Thursday 02 Mar 2017 16:29:32 Neil Armstrong wrote:
> The HDMI TX controller support HPD and RXSENSE signaling from the PHY
> via it's STAT0 PHY interface, but some vendor PHYs can manage these
> signals independently from the controller, thus these STAT0 handling
> should be moved to PHY specific operations and become optional.
> 
> The existing STAT0 HPD and RXSENSE handling code is refactored into
> a supplementaty set of default PHY operations that are used automatically
> when the platform glue doesn't provide its own operations.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 134 +++++++++++++++++++++++++-----------
>  include/drm/bridge/dw_hdmi.h     |   8 +++
>  2 files changed, 104 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> b/drivers/gpu/drm/bridge/dw-hdmi.c index 653ecd7..58dcf7d 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1098,10 +1098,50 @@ static enum drm_connector_status
> dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, connector_status_connected :
> connector_status_disconnected;
>  }
> 
> +static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
> +				   bool force, bool disabled, bool rxsense)
> +{
> +	if (force || disabled || !rxsense)
> +		hdmi->phy_mask |= HDMI_PHY_RX_SENSE;
> +	else
> +		hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE;
> +}
> +
> +static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data)
> +{
> +	/* setup HPD and RXSENSE interrupt polarity */
> +	hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0);
> +}
> +
> +static void dw_hdmi_phy_configure_hpd(struct dw_hdmi *hdmi, void *data)
> +{
> +	/* enable cable hot plug irq */
> +	hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
> +}
> +
> +static void dw_hdmi_phy_clear_hpd(struct dw_hdmi *hdmi, void *data)
> +{
> +	/* Clear Hotplug interrupts */
> +	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
> +		    HDMI_IH_PHY_STAT0);
> +}
> +
> +static void dw_hdmi_phy_unmute_hpd(struct dw_hdmi *hdmi, void *data)
> +{
> +	/* Unmute Hotplug interrupts */
> +	hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | 
HDMI_IH_PHY_STAT0_RX_SENSE),
> +		    HDMI_IH_MUTE_PHY_STAT0);
> +}

Do we really need all those new operations ? It looks to me like a bit of 
refactoring could help lowering the number of operations. We essentially need

- an init function called at probe time (or likely rather at runtime PM resume 
time when we'll implement runtime PM) 

- an interrupt enable/disable function roughly equivalent to 
dw_hdmi_update_phy_mask()

- a read function to report the detection results called from 
dw_hdmi_connector_detect()

>  static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = {
>  	.init = dw_hdmi_phy_init,
>  	.disable = dw_hdmi_phy_disable,
>  	.read_hpd = dw_hdmi_phy_read_hpd,
> +	.update_hpd = dw_hdmi_phy_update_hpd,
> +	.setup_hpd = dw_hdmi_phy_setup_hpd,
> +	.configure_hpd = dw_hdmi_phy_configure_hpd,
> +	.clear_hpd = dw_hdmi_phy_clear_hpd,
> +	.unmute_hpd = dw_hdmi_phy_unmute_hpd,
>  };
> 
>  /* ------------------------------------------------------------------------
> @@ -1507,11 +1547,12 @@ static int dw_hdmi_fb_registered(struct dw_hdmi
> *hdmi) HDMI_PHY_I2CM_CTLINT_ADDR);
> 
>  	/* enable cable hot plug irq */
> -	hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
> +	if (hdmi->phy.ops->configure_hpd)
> +		hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data);
> 
>  	/* Clear Hotplug interrupts */
> -	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
> -		    HDMI_IH_PHY_STAT0);
> +	if (hdmi->phy.ops->clear_hpd)
> +		hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data);

The probe function contains the same code. Let's inline this function into 
probe, group all HPD-related initialization together and extract that into a 
function to keep probe easy to read. I think you can do that in a separate 
patch.

>  	return 0;
>  }
> @@ -1622,13 +1663,14 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi
> *hdmi) {
>  	u8 old_mask = hdmi->phy_mask;
>
> -	if (hdmi->force || hdmi->disabled || !hdmi->rxsense)
> -		hdmi->phy_mask |= HDMI_PHY_RX_SENSE;
> -	else
> -		hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE;
> +	if (hdmi->phy.ops->update_hpd)
> +		hdmi->phy.ops->update_hpd(hdmi, hdmi->phy.data,
> +					  hdmi->force, hdmi->disabled,
> +					  hdmi->rxsense);
> 
> -	if (old_mask != hdmi->phy_mask)
> -		hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
> +	if (old_mask != hdmi->phy_mask &&

phy_mask isn't accessible to glue code, so this test will never be true with 
vendor PHYs.

> +	    hdmi->phy.ops->configure_hpd)
> +		hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data);
>  }
> 
>  static enum drm_connector_status
> @@ -1820,6 +1862,41 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void
> *dev_id) return ret;
>  }
> 
> +void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool
> rx_sense)
> +{
> +	mutex_lock(&hdmi->mutex);
> +
> +	if (!hdmi->disabled && !hdmi->force) {
> +		/*
> +		 * If the RX sense status indicates we're disconnected,
> +		 * clear the software rxsense status.
> +		 */
> +		if (!rx_sense)
> +			hdmi->rxsense = false;
> +
> +		/*
> +		 * Only set the software rxsense status when both
> +		 * rxsense and hpd indicates we're connected.
> +		 * This avoids what seems to be bad behaviour in
> +		 * at least iMX6S versions of the phy.
> +		 */
> +		if (hpd)
> +			hdmi->rxsense = true;

This contradicts the above comment, hdmi->rxsense is set back to true solely 
based on the hpd parameter.

> +
> +		dw_hdmi_update_power(hdmi);
> +		dw_hdmi_update_phy_mask(hdmi);
> +	}

I'd add a blank line here.

> +	mutex_unlock(&hdmi->mutex);
> +}
> +
> +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
> +{
> +	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
> +
> +	__dw_hdmi_setup_rx_sense(hdmi, hpd, rx_sense);
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense);
> +
>  static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>  {
>  	struct dw_hdmi *hdmi = dev_id;
> @@ -1852,30 +1929,10 @@ static irqreturn_t dw_hdmi_irq(int irq, void
> *dev_id) * ask the source to re-read the EDID.
>  	 */
>  	if (intr_stat &
> -	    (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) {
> -		mutex_lock(&hdmi->mutex);
> -		if (!hdmi->disabled && !hdmi->force) {
> -			/*
> -			 * If the RX sense status indicates we're 
disconnected,
> -			 * clear the software rxsense status.
> -			 */
> -			if (!(phy_stat & HDMI_PHY_RX_SENSE))
> -				hdmi->rxsense = false;
> -
> -			/*
> -			 * Only set the software rxsense status when both
> -			 * rxsense and hpd indicates we're connected.
> -			 * This avoids what seems to be bad behaviour in
> -			 * at least iMX6S versions of the phy.
> -			 */
> -			if (phy_stat & HDMI_PHY_HPD)
> -				hdmi->rxsense = true;
> -
> -			dw_hdmi_update_power(hdmi);
> -			dw_hdmi_update_phy_mask(hdmi);
> -		}
> -		mutex_unlock(&hdmi->mutex);
> -	}
> +	    (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD))

Is this right ? If your SoC implements a custom HPD mechanism, does it still 
rely on the standard RX SENSE and HPD interrupts ? In particular, this 
function still writes the HDMI_IH_MUTE_PHY_STAT0 register directly, while 
you've extracted a write to the same register in the probe function into a PHY 
operation.

> +		__dw_hdmi_setup_rx_sense(hdmi,
> +					 phy_stat & HDMI_PHY_HPD,
> +					 phy_stat & HDMI_PHY_RX_SENSE);
> 
>  	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
>  		dev_dbg(hdmi->dev, "EVENT=%s\n",
> @@ -2146,11 +2203,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
>  	 * Configure registers related to HDMI interrupt
>  	 * generation before registering IRQ.
>  	 */
> -	hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0);
> +	if (hdmi->phy.ops->setup_hpd)
> +		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> 
>  	/* Clear Hotplug interrupts */
> -	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
> -		    HDMI_IH_PHY_STAT0);
> +	if (hdmi->phy.ops->clear_hpd)
> +		hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data);
> 
>  	hdmi->bridge.driver_private = hdmi;
>  	hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
> @@ -2163,8 +2221,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
>  		goto err_iahb;
> 
>  	/* Unmute interrupts */
> -	hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | 
HDMI_IH_PHY_STAT0_RX_SENSE),
> -		    HDMI_IH_MUTE_PHY_STAT0);
> +	if (hdmi->phy.ops->unmute_hpd)
> +		hdmi->phy.ops->unmute_hpd(hdmi, hdmi->phy.data);
> 
>  	memset(&pdevinfo, 0, sizeof(pdevinfo));
>  	pdevinfo.parent = dev;
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 8c0cc13..d72403f 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -64,6 +64,12 @@ struct dw_hdmi_phy_ops {
>  		    struct drm_display_mode *mode);
>  	void (*disable)(struct dw_hdmi *hdmi, void *data);
>  	enum drm_connector_status (*read_hpd)(struct dw_hdmi *hdmi, void 
*data);
> +	void (*update_hpd)(struct dw_hdmi *hdmi, void *data,
> +			   bool force, bool disabled, bool rxsense);
> +	void (*setup_hpd)(struct dw_hdmi *hdmi, void *data);
> +	void (*configure_hpd)(struct dw_hdmi *hdmi, void *data);
> +	void (*clear_hpd)(struct dw_hdmi *hdmi, void *data);
> +	void (*unmute_hpd)(struct dw_hdmi *hdmi, void *data);

That's quite a lot of new operations. I think we need documentation :-)

>  };
> 
>  struct dw_hdmi_plat_data {
> @@ -93,6 +99,8 @@ int dw_hdmi_probe(struct platform_device *pdev,
>  int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
> const struct dw_hdmi_plat_data *plat_data);
> 
> +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense);
> +
>  void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
>  void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
>  void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] drm: bridge: Move HPD handling to PHY operations

[Neil Armstrong]

On 03/02/2017 05:18 PM, Laurent Pinchart wrote:
> Hi Neil,
> 
> Thank you for the patch.
> 
> On Thursday 02 Mar 2017 16:29:32 Neil Armstrong wrote:
>> The HDMI TX controller support HPD and RXSENSE signaling from the PHY
>> via it's STAT0 PHY interface, but some vendor PHYs can manage these
>> signals independently from the controller, thus these STAT0 handling
>> should be moved to PHY specific operations and become optional.
>>
>> The existing STAT0 HPD and RXSENSE handling code is refactored into
>> a supplementaty set of default PHY operations that are used automatically
>> when the platform glue doesn't provide its own operations.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/bridge/dw-hdmi.c | 134 +++++++++++++++++++++++++-----------
>>  include/drm/bridge/dw_hdmi.h     |   8 +++
>>  2 files changed, 104 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
>> b/drivers/gpu/drm/bridge/dw-hdmi.c index 653ecd7..58dcf7d 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -1098,10 +1098,50 @@ static enum drm_connector_status
>> dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, connector_status_connected :
>> connector_status_disconnected;
>>  }
>>
>> +static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
>> +				   bool force, bool disabled, bool rxsense)
>> +{
>> +	if (force || disabled || !rxsense)
>> +		hdmi->phy_mask |= HDMI_PHY_RX_SENSE;
>> +	else
>> +		hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE;
>> +}
>> +
>> +static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data)
>> +{
>> +	/* setup HPD and RXSENSE interrupt polarity */
>> +	hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0);
>> +}
>> +
>> +static void dw_hdmi_phy_configure_hpd(struct dw_hdmi *hdmi, void *data)
>> +{
>> +	/* enable cable hot plug irq */
>> +	hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
>> +}
>> +
>> +static void dw_hdmi_phy_clear_hpd(struct dw_hdmi *hdmi, void *data)
>> +{
>> +	/* Clear Hotplug interrupts */
>> +	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
>> +		    HDMI_IH_PHY_STAT0);
>> +}
>> +
>> +static void dw_hdmi_phy_unmute_hpd(struct dw_hdmi *hdmi, void *data)
>> +{
>> +	/* Unmute Hotplug interrupts */
>> +	hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | 
> HDMI_IH_PHY_STAT0_RX_SENSE),
>> +		    HDMI_IH_MUTE_PHY_STAT0);
>> +}
> 
> Do we really need all those new operations ? It looks to me like a bit of 
> refactoring could help lowering the number of operations. We essentially need
> 
> - an init function called at probe time (or likely rather at runtime PM resume 
> time when we'll implement runtime PM) 
> 
> - an interrupt enable/disable function roughly equivalent to 
> dw_hdmi_update_phy_mask()
> 
> - a read function to report the detection results called from 
> dw_hdmi_connector_detect()

Well, I strictly copied the 5 different operations involved in the HPD handling,
if you reduce to these 3 it will change the functional behavior of the driver regarding HPD/RxSenSe...

I do not have enough documentation and HW to actually experiment these changes !

For Amlogic I need setup, read, configure and clear. Only the unmute is specific to Synopsys PHY.

> 
>>  static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = {
>>  	.init = dw_hdmi_phy_init,
>>  	.disable = dw_hdmi_phy_disable,
>>  	.read_hpd = dw_hdmi_phy_read_hpd,
>> +	.update_hpd = dw_hdmi_phy_update_hpd,
>> +	.setup_hpd = dw_hdmi_phy_setup_hpd,
>> +	.configure_hpd = dw_hdmi_phy_configure_hpd,
>> +	.clear_hpd = dw_hdmi_phy_clear_hpd,
>> +	.unmute_hpd = dw_hdmi_phy_unmute_hpd,
>>  };
>>
>>  /* ------------------------------------------------------------------------
>> @@ -1507,11 +1547,12 @@ static int dw_hdmi_fb_registered(struct dw_hdmi
>> *hdmi) HDMI_PHY_I2CM_CTLINT_ADDR);
>>
>>  	/* enable cable hot plug irq */
>> -	hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
>> +	if (hdmi->phy.ops->configure_hpd)
>> +		hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data);
>>
>>  	/* Clear Hotplug interrupts */
>> -	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
>> -		    HDMI_IH_PHY_STAT0);
>> +	if (hdmi->phy.ops->clear_hpd)
>> +		hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data);
> 
> The probe function contains the same code. Let's inline this function into 
> probe, group all HPD-related initialization together and extract that into a 
> function to keep probe easy to read. I think you can do that in a separate 
> patch.
> 
>>  	return 0;
>>  }
>> @@ -1622,13 +1663,14 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi
>> *hdmi) {
>>  	u8 old_mask = hdmi->phy_mask;
>>
>> -	if (hdmi->force || hdmi->disabled || !hdmi->rxsense)
>> -		hdmi->phy_mask |= HDMI_PHY_RX_SENSE;
>> -	else
>> -		hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE;
>> +	if (hdmi->phy.ops->update_hpd)
>> +		hdmi->phy.ops->update_hpd(hdmi, hdmi->phy.data,
>> +					  hdmi->force, hdmi->disabled,
>> +					  hdmi->rxsense);
>>
>> -	if (old_mask != hdmi->phy_mask)
>> -		hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
>> +	if (old_mask != hdmi->phy_mask &&
> 
> phy_mask isn't accessible to glue code, so this test will never be true with 
> vendor PHYs.

The problem is that the HPD/RxSense is tied to this phy_mask and glued into the
dw-hdmi driver.

The *real* solution would be to completely separate the HPD/RxSense irq handling to
a separate driver as a shared irq...

If Jose is willing to give me some documentation and Freescale some boards, I'll be
happy to do it !

> 
>> +	    hdmi->phy.ops->configure_hpd)
>> +		hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data);
>>  }
>>
>>  static enum drm_connector_status
>> @@ -1820,6 +1862,41 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void
>> *dev_id) return ret;
>>  }
>>
>> +void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool
>> rx_sense)
>> +{
>> +	mutex_lock(&hdmi->mutex);
>> +
>> +	if (!hdmi->disabled && !hdmi->force) {
>> +		/*
>> +		 * If the RX sense status indicates we're disconnected,
>> +		 * clear the software rxsense status.
>> +		 */
>> +		if (!rx_sense)
>> +			hdmi->rxsense = false;
>> +
>> +		/*
>> +		 * Only set the software rxsense status when both
>> +		 * rxsense and hpd indicates we're connected.
>> +		 * This avoids what seems to be bad behaviour in
>> +		 * at least iMX6S versions of the phy.
>> +		 */
>> +		if (hpd)
>> +			hdmi->rxsense = true;
> 
> This contradicts the above comment, hdmi->rxsense is set back to true solely 
> based on the hpd parameter.

The "hpd" here is an HPD event indicator, not the HPD pin status, so it makes sense.

I suppose the HPD and RxSense events don't come at the same time, but RxSense comes later on.


>> +
>> +		dw_hdmi_update_power(hdmi);
>> +		dw_hdmi_update_phy_mask(hdmi);
>> +	}
> 
> I'd add a blank line here.

Ok

> 
>> +	mutex_unlock(&hdmi->mutex);
>> +}
>> +
>> +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
>> +{
>> +	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
>> +
>> +	__dw_hdmi_setup_rx_sense(hdmi, hpd, rx_sense);
>> +}
>> +EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense);
>> +
>>  static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>>  {
>>  	struct dw_hdmi *hdmi = dev_id;
>> @@ -1852,30 +1929,10 @@ static irqreturn_t dw_hdmi_irq(int irq, void
>> *dev_id) * ask the source to re-read the EDID.
>>  	 */
>>  	if (intr_stat &
>> -	    (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) {
>> -		mutex_lock(&hdmi->mutex);
>> -		if (!hdmi->disabled && !hdmi->force) {
>> -			/*
>> -			 * If the RX sense status indicates we're 
> disconnected,
>> -			 * clear the software rxsense status.
>> -			 */
>> -			if (!(phy_stat & HDMI_PHY_RX_SENSE))
>> -				hdmi->rxsense = false;
>> -
>> -			/*
>> -			 * Only set the software rxsense status when both
>> -			 * rxsense and hpd indicates we're connected.
>> -			 * This avoids what seems to be bad behaviour in
>> -			 * at least iMX6S versions of the phy.
>> -			 */
>> -			if (phy_stat & HDMI_PHY_HPD)
>> -				hdmi->rxsense = true;
>> -
>> -			dw_hdmi_update_power(hdmi);
>> -			dw_hdmi_update_phy_mask(hdmi);
>> -		}
>> -		mutex_unlock(&hdmi->mutex);
>> -	}
>> +	    (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD))
> 
> Is this right ? If your SoC implements a custom HPD mechanism, does it still 
> rely on the standard RX SENSE and HPD interrupts ? In particular, this 
> function still writes the HDMI_IH_MUTE_PHY_STAT0 register directly, while 
> you've extracted a write to the same register in the probe function into a PHY 
> operation.

It won't since the IRQ is left masked and muted, and I moved all the HDMI_IH_*_PHY
into HPD ops.

> 
>> +		__dw_hdmi_setup_rx_sense(hdmi,
>> +					 phy_stat & HDMI_PHY_HPD,
>> +					 phy_stat & HDMI_PHY_RX_SENSE);
>>
>>  	if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
>>  		dev_dbg(hdmi->dev, "EVENT=%s\n",
>> @@ -2146,11 +2203,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
>>  	 * Configure registers related to HDMI interrupt
>>  	 * generation before registering IRQ.
>>  	 */
>> -	hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0);
>> +	if (hdmi->phy.ops->setup_hpd)
>> +		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
>>
>>  	/* Clear Hotplug interrupts */
>> -	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
>> -		    HDMI_IH_PHY_STAT0);
>> +	if (hdmi->phy.ops->clear_hpd)
>> +		hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data);
>>
>>  	hdmi->bridge.driver_private = hdmi;
>>  	hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
>> @@ -2163,8 +2221,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
>>  		goto err_iahb;
>>
>>  	/* Unmute interrupts */
>> -	hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | 
> HDMI_IH_PHY_STAT0_RX_SENSE),
>> -		    HDMI_IH_MUTE_PHY_STAT0);
>> +	if (hdmi->phy.ops->unmute_hpd)
>> +		hdmi->phy.ops->unmute_hpd(hdmi, hdmi->phy.data);
>>
>>  	memset(&pdevinfo, 0, sizeof(pdevinfo));
>>  	pdevinfo.parent = dev;
>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>> index 8c0cc13..d72403f 100644
>> --- a/include/drm/bridge/dw_hdmi.h
>> +++ b/include/drm/bridge/dw_hdmi.h
>> @@ -64,6 +64,12 @@ struct dw_hdmi_phy_ops {
>>  		    struct drm_display_mode *mode);
>>  	void (*disable)(struct dw_hdmi *hdmi, void *data);
>>  	enum drm_connector_status (*read_hpd)(struct dw_hdmi *hdmi, void 
> *data);
>> +	void (*update_hpd)(struct dw_hdmi *hdmi, void *data,
>> +			   bool force, bool disabled, bool rxsense);
>> +	void (*setup_hpd)(struct dw_hdmi *hdmi, void *data);
>> +	void (*configure_hpd)(struct dw_hdmi *hdmi, void *data);
>> +	void (*clear_hpd)(struct dw_hdmi *hdmi, void *data);
>> +	void (*unmute_hpd)(struct dw_hdmi *hdmi, void *data);
> 
> That's quite a lot of new operations. I think we need documentation :-)

We need documentation on all the other ops too !

Wehat would be the recommended format ?

> 
>>  };
>>
>>  struct dw_hdmi_plat_data {
>> @@ -93,6 +99,8 @@ int dw_hdmi_probe(struct platform_device *pdev,
>>  int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder,
>> const struct dw_hdmi_plat_data *plat_data);
>>
>> +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense);
>> +
>>  void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
>>  void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
>>  void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
> 

Re: [PATCH v2 2/2] drm: bridge: Move HPD handling to PHY operations

[Jose Abreu]

Hi Neil,


On 03-03-2017 09:07, Neil Armstrong wrote:
>
> The problem is that the HPD/RxSense is tied to this phy_mask and glued into the
> dw-hdmi driver.
>
> The *real* solution would be to completely separate the HPD/RxSense irq handling to
> a separate driver as a shared irq...
>
> If Jose is willing to give me some documentation and Freescale some boards, I'll be
> happy to do it !
>
>

Hmm, why don't get rid of phy_mask totally and just return the
new mask in update_hpd() function? Or add a get_hpd_status()
callback. (I also think there are too many callbacks. For example
we could have: setup, set_status, clear and then just use
parameters when needed:
    void setup(bool force, bool disabled, bool rxsense)
    void set_status(bool enable, bool enable_ints)
    void clear()

What do you think? I only checked quickly the code, don't know if
this is enough.

Best regards,
Jose Miguel Abreu

Re: [PATCH v2 2/2] drm: bridge: Move HPD handling to PHY operations

[Laurent Pinchart]

Hi Jose,

On Friday 03 Mar 2017 10:05:36 Jose Abreu wrote:
> On 03-03-2017 09:07, Neil Armstrong wrote:
> > The problem is that the HPD/RxSense is tied to this phy_mask and glued
> > into the dw-hdmi driver.
> > 
> > The *real* solution would be to completely separate the HPD/RxSense irq
> > handling to a separate driver as a shared irq...
> > 
> > If Jose is willing to give me some documentation and Freescale some
> > boards, I'll be happy to do it !
> 
> Hmm, why don't get rid of phy_mask totally and just return the
> new mask in update_hpd() function? Or add a get_hpd_status()
> callback. (I also think there are too many callbacks. For example
> we could have: setup, set_status, clear and then just use
> parameters when needed:
>     void setup(bool force, bool disabled, bool rxsense)
>     void set_status(bool enable, bool enable_ints)
>     void clear()
> 
> What do you think? I only checked quickly the code, don't know if
> this is enough.

For the record, that's more or less what I had in mind. The following
preparatory patch could be useful.

commit 5ceb6a93d78e21e5be195efe52c4c7e5578ac787
Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Date:   Fri Mar 3 14:14:56 2017 +0200

    drm: bridge: dw-hdmi: Extract PHY interrupt setup to a function
    
    In preparation for adding PHY operations to handle RX SENSE and HPD,
    group all the PHY interrupt setup code in a single location and extract
    it to a separate function.
    
    Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 026a0dce7661..1ed8bc12eed7 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1496,7 +1496,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 }
 
 /* Wait until we are registered to enable interrupts */
-static int dw_hdmi_fb_registered(struct dw_hdmi *hdmi)
+static void dw_hdmi_fb_registered(struct dw_hdmi *hdmi)
 {
 	hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL,
 		    HDMI_PHY_I2CM_INT_ADDR);
@@ -1504,15 +1504,6 @@ static int dw_hdmi_fb_registered(struct dw_hdmi *hdmi)
 	hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL |
 		    HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL,
 		    HDMI_PHY_I2CM_CTLINT_ADDR);
-
-	/* enable cable hot plug irq */
-	hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
-
-	/* Clear Hotplug interrupts */
-	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
-		    HDMI_IH_PHY_STAT0);
-
-	return 0;
 }
 
 static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi)
@@ -1630,6 +1621,26 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi *hdmi)
 		hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
 }
 
+static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi)
+{
+	/*
+	 * Configure the PHY RX SENSE and HPD interrupts polarities and clear
+	 * any pending interrupt.
+	 */
+	hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0);
+	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
+		    HDMI_IH_PHY_STAT0);
+
+	/* Enable cable hot plug irq. */
+	hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
+
+	/* Clear and unmute interrupts. */
+	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
+		    HDMI_IH_PHY_STAT0);
+	hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE),
+		    HDMI_IH_MUTE_PHY_STAT0);
+}
+
 static enum drm_connector_status
 dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
@@ -2141,29 +2152,14 @@ __dw_hdmi_probe(struct platform_device *pdev,
 			hdmi->ddc = NULL;
 	}
 
-	/*
-	 * Configure registers related to HDMI interrupt
-	 * generation before registering IRQ.
-	 */
-	hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0);
-
-	/* Clear Hotplug interrupts */
-	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE,
-		    HDMI_IH_PHY_STAT0);
-
 	hdmi->bridge.driver_private = hdmi;
 	hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
 #ifdef CONFIG_OF
 	hdmi->bridge.of_node = pdev->dev.of_node;
 #endif
 
-	ret = dw_hdmi_fb_registered(hdmi);
-	if (ret)
-		goto err_iahb;
-
-	/* Unmute interrupts */
-	hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE),
-		    HDMI_IH_MUTE_PHY_STAT0);
+	dw_hdmi_fb_registered(hdmi);
+	dw_hdmi_phy_setup_hpd(hdmi);
 
 	memset(&pdevinfo, 0, sizeof(pdevinfo));
 	pdevinfo.parent = dev;


-- 
Regards,

Laurent Pinchart