Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560563 > unrolled thread

[RFC/RFT PATCH 0/4] drm/bridge: dw-hdmi: Add support for Custom PHYs

Back to article view | Back to linux.kernel

Contents

  [RFC/RFT PATCH 0/4] drm/bridge: dw-hdmi: Add support for Custom PHYs Neil Armstrong <narmstrong@baylibre.com> - 2017-01-17 13:40 +0100
    [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI Neil Armstrong <narmstrong@baylibre.com> - 2017-01-17 13:40 +0100
      Re: [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-01-17 15:50 +0100
      Re: [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-18 11:40 +0100
        Re: [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI Neil Armstrong <narmstrong@baylibre.com> - 2017-01-18 12:30 +0100
    [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data Neil Armstrong <narmstrong@baylibre.com> - 2017-01-17 13:40 +0100
      Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-01-17 16:00 +0100
      Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from  plat_data Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-18 11:40 +0100
        Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from  plat_data Neil Armstrong <narmstrong@baylibre.com> - 2017-01-18 12:30 +0100
          Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-01-18 22:00 +0100
            Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from  plat_data Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-19 16:30 +0100
              Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-01-19 17:50 +0100
              Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from  plat_data Hans Verkuil <hansverk@cisco.com> - 2017-01-19 18:00 +0100

#1560563 — [RFC/RFT PATCH 0/4] drm/bridge: dw-hdmi: Add support for Custom PHYs

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

This is why the following patchset implements :
 - Conversion to regmap for register access
 - Add more callbacks ops to handle Custom PHYs
 - Configure the Input format from the plat_data
 - Fixes a bug that considers the input to be always RBG and sends bad pixel
   format to a DVI sink by disabling CSC

This patchset makes the Amlogix GX SoCs HDMI output successfully work, but I
do not have access to Renesas, i.MX or RockChip SoCs to test against
potentiel regressions, like the regmap conversion.

This patchset is based on the latest Laurent Pinchart dw-hdmi serie at [1].

[1] http://lkml.kernel.org/r/20170117082910.27023-1-laurent.pinchart+renesas@ideasonboard.com

Neil Armstrong (4):
  drm/bridge: dw-hdmi: Switch to regmap for register access
  drm/bridge: dw-hdmi: Add support for custom PHY handling
  drm/bridge: dw-hdmi: Enable CSC even for DVI
  drm/bridge: dw-hdmi: Take input format from plat_data

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

-- 
1.9.1

[toc] | [next] | [standalone]

#1560565 — [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI

If the input pixel format is not RGB, the CSC must be enabled in order to
provide valid pixel to DVI sinks.
This patch removes the hdmi only dependency on the CSC enabling.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 923e250..8a6a183 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1331,8 +1331,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
 		hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS);
 	}
 
-	/* Enable color space conversion if needed (for HDMI sinks only). */
-	if (hdmi->sink_is_hdmi && is_color_space_conversion(hdmi))
+	/* Enable color space conversion if needed */
+	if (is_color_space_conversion(hdmi))
 		hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
 			    HDMI_MC_FLOWCTRL);
 	else
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1560664 — Re: [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI

Hi Neil,

Thank you for the patch.

On Tuesday 17 Jan 2017 13:31:33 Neil Armstrong wrote:
> If the input pixel format is not RGB, the CSC must be enabled in order to
> provide valid pixel to DVI sinks.
> This patch removes the hdmi only dependency on the CSC enabling.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> b/drivers/gpu/drm/bridge/dw-hdmi.c index 923e250..8a6a183 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1331,8 +1331,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi
> *hdmi) hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS);
>  	}
> 
> -	/* Enable color space conversion if needed (for HDMI sinks only). */
> -	if (hdmi->sink_is_hdmi && is_color_space_conversion(hdmi))
> +	/* Enable color space conversion if needed */
> +	if (is_color_space_conversion(hdmi))
>  		hdmi_writeb(hdmi, 
HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
>  			    HDMI_MC_FLOWCTRL);
>  	else

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1561465 — Re: [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI

Hi Neil,


On 17-01-2017 12:31, Neil Armstrong wrote:
> If the input pixel format is not RGB, the CSC must be enabled in order to
> provide valid pixel to DVI sinks.
> This patch removes the hdmi only dependency on the CSC enabling.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

I was going to say that you should make sure the output
colorspace is RGB or else DVI will fail to work, but I just
noticed the output colorspace is fixed in the code to RGB.
Though, that may change in the future.

Reviewed-by: Jose Abreu <joabreu@synopsys.com>

Best regards,
Jose Miguel Abreu

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 923e250..8a6a183 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1331,8 +1331,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
>  		hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS);
>  	}
>  
> -	/* Enable color space conversion if needed (for HDMI sinks only). */
> -	if (hdmi->sink_is_hdmi && is_color_space_conversion(hdmi))
> +	/* Enable color space conversion if needed */
> +	if (is_color_space_conversion(hdmi))
>  		hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
>  			    HDMI_MC_FLOWCTRL);
>  	else

[toc] | [prev] | [next] | [standalone]

#1561646 — Re: [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI

Hi jose,

On 01/18/2017 11:22 AM, Jose Abreu wrote:
> Hi Neil,
> 
> 
> On 17-01-2017 12:31, Neil Armstrong wrote:
>> If the input pixel format is not RGB, the CSC must be enabled in order to
>> provide valid pixel to DVI sinks.
>> This patch removes the hdmi only dependency on the CSC enabling.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> I was going to say that you should make sure the output
> colorspace is RGB or else DVI will fail to work, but I just
> noticed the output colorspace is fixed in the code to RGB.
> Though, that may change in the future.

Thanks for the review,
It would certainly need to enforce RGB output colorspace when in DVI mode.

Neil

> 
> Reviewed-by: Jose Abreu <joabreu@synopsys.com>
> 
> Best regards,
> Jose Miguel Abreu
> 
>> ---
>>  drivers/gpu/drm/bridge/dw-hdmi.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
>> index 923e250..8a6a183 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -1331,8 +1331,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
>>  		hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS);
>>  	}
>>  
>> -	/* Enable color space conversion if needed (for HDMI sinks only). */
>> -	if (hdmi->sink_is_hdmi && is_color_space_conversion(hdmi))
>> +	/* Enable color space conversion if needed */
>> +	if (is_color_space_conversion(hdmi))
>>  		hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
>>  			    HDMI_MC_FLOWCTRL);
>>  	else
> 

[toc] | [prev] | [next] | [standalone]

#1560568 — [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data

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 | 7 +++++--
 include/drm/bridge/dw_hdmi.h     | 9 +++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 8a6a183..fa4147c 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1420,8 +1420,11 @@ 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 = RGB;
 
 	hdmi->hdmi_data.enc_out_format = RGB;
 
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index d6a0ab3..4f426c3 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_INPUT_FMT_RGB = 0,
+	DW_HDMI_INPUT_FMT_YCBCR444,
+	DW_HDMI_INPUT_FMT_YCBCR422_16BITS,
+	DW_HDMI_INPUT_FMT_YCBCR422_8BITS,
+	DW_HDMI_INPUT_FMT_XVYCC444,
+};
+
 enum dw_hdmi_phy_type {
 	DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
 	DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
@@ -68,6 +76,7 @@ struct dw_hdmi_plat_data {
 				 const struct dw_hdmi_plat_data *data);
 	bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi,
 			      const struct dw_hdmi_plat_data *data);
+	int input_fmt;
 };
 
 int dw_hdmi_probe(struct platform_device *pdev,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1560676 — Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data

Hi Neil,

Thank you for the patch.

On Tuesday 17 Jan 2017 13:31:34 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 | 7 +++++--
>  include/drm/bridge/dw_hdmi.h     | 9 +++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> b/drivers/gpu/drm/bridge/dw-hdmi.c index 8a6a183..fa4147c 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1420,8 +1420,11 @@ 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 = RGB;

This should ideally be queried dynamically. I believe we need to extend the 
DRM bridge API for this purpose. I might be willing to accept a pdata-based 
solution in the meantime though.

>  	hdmi->hdmi_data.enc_out_format = RGB;
> 
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index d6a0ab3..4f426c3 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_INPUT_FMT_RGB = 0,
> +	DW_HDMI_INPUT_FMT_YCBCR444,
> +	DW_HDMI_INPUT_FMT_YCBCR422_16BITS,
> +	DW_HDMI_INPUT_FMT_YCBCR422_8BITS,
> +	DW_HDMI_INPUT_FMT_XVYCC444,
> +};

How about giving the enum a name and dropping the

#define RGB                     0
#define YCBCR444                1
#define YCBCR422_16BITS         2
#define YCBCR422_8BITS          3
#define XVYCC444                4

macros from the driver ? Even better, how about using a media bus format code 
from include/uapi/linux/media-bus-format.h ? We would need an additional 
colorspace field to express the difference between the YCBCR and XVYCC 
formats, as both of them are YUV 4:4:4 and map to the same bus format.

>  enum dw_hdmi_phy_type {
>  	DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
>  	DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
> @@ -68,6 +76,7 @@ struct dw_hdmi_plat_data {
>  				 const struct dw_hdmi_plat_data *data);
>  	bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi,
>  			      const struct dw_hdmi_plat_data *data);
> +	int input_fmt;
>  };
> 
>  int dw_hdmi_probe(struct platform_device *pdev,

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1561466 — Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data

Hi Neil,


On 17-01-2017 12: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 | 7 +++++--
>  include/drm/bridge/dw_hdmi.h     | 9 +++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 8a6a183..fa4147c 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1420,8 +1420,11 @@ 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;

Also not a big fan of this approach, but its better than it was.
BTW see relevant discussion about colorspace (this is more in the
output path) here [1].

[1] https://patchwork.kernel.org/patch/9495153/

> +	else
> +		hdmi->hdmi_data.enc_in_format = RGB;
>  
>  	hdmi->hdmi_data.enc_out_format = RGB;
>  
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index d6a0ab3..4f426c3 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_INPUT_FMT_RGB = 0,
> +	DW_HDMI_INPUT_FMT_YCBCR444,
> +	DW_HDMI_INPUT_FMT_YCBCR422_16BITS,
> +	DW_HDMI_INPUT_FMT_YCBCR422_8BITS,

Hmm, did you test these two? I'm not sure if deep color can be
converted using CSC.

Best regards,
Jose Miguel Abreu

> +	DW_HDMI_INPUT_FMT_XVYCC444,
> +};
> +
>  enum dw_hdmi_phy_type {
>  	DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
>  	DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
> @@ -68,6 +76,7 @@ struct dw_hdmi_plat_data {
>  				 const struct dw_hdmi_plat_data *data);
>  	bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi,
>  			      const struct dw_hdmi_plat_data *data);
> +	int input_fmt;
>  };
>  
>  int dw_hdmi_probe(struct platform_device *pdev,

[toc] | [prev] | [next] | [standalone]

#1561631 — Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data

Hi Jose,

On 01/18/2017 11:28 AM, Jose Abreu wrote:
> Hi Neil,
> 
> 
> On 17-01-2017 12: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 | 7 +++++--
>>  include/drm/bridge/dw_hdmi.h     | 9 +++++++++
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
>> index 8a6a183..fa4147c 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -1420,8 +1420,11 @@ 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;
> 
> Also not a big fan of this approach, but its better than it was.
> BTW see relevant discussion about colorspace (this is more in the
> output path) here [1].
> 
> [1] https://patchwork.kernel.org/patch/9495153/
> 
>> +	else
>> +		hdmi->hdmi_data.enc_in_format = RGB;
>>  
>>  	hdmi->hdmi_data.enc_out_format = RGB;
>>  
>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>> index d6a0ab3..4f426c3 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_INPUT_FMT_RGB = 0,
>> +	DW_HDMI_INPUT_FMT_YCBCR444,
>> +	DW_HDMI_INPUT_FMT_YCBCR422_16BITS,
>> +	DW_HDMI_INPUT_FMT_YCBCR422_8BITS,
> 
> Hmm, did you test these two? I'm not sure if deep color can be
> converted using CSC.

I simply copied the dw-hdmi internal values.
Proper handling of input formats capabilities and
output format pixel clock handling should be added sometime.
In the meantime, it's worth adding which input is supported.

This approach is very limited, should I add a bitmask with all
support input formats and select between RGB, YUV444 or YUV422
in dw_hdmi_setup ?

> 
> Best regards,
> Jose Miguel Abreu
> 
>> +	DW_HDMI_INPUT_FMT_XVYCC444,
>> +};
>> +
>>  enum dw_hdmi_phy_type {
>>  	DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
>>  	DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
>> @@ -68,6 +76,7 @@ struct dw_hdmi_plat_data {
>>  				 const struct dw_hdmi_plat_data *data);
>>  	bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi,
>>  			      const struct dw_hdmi_plat_data *data);
>> +	int input_fmt;
>>  };
>>  
>>  int dw_hdmi_probe(struct platform_device *pdev,
> 

Thanks,
Neil

[toc] | [prev] | [next] | [standalone]

#1562174 — Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data

Hi Neil,

(CC'ing Hans Verkuil)

On Wednesday 18 Jan 2017 12:24:22 Neil Armstrong wrote:
> On 01/18/2017 11:28 AM, Jose Abreu wrote:
> > On 17-01-2017 12: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 | 7 +++++--
> >>  include/drm/bridge/dw_hdmi.h     | 9 +++++++++
> >>  2 files changed, 14 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> >> b/drivers/gpu/drm/bridge/dw-hdmi.c index 8a6a183..fa4147c 100644
> >> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> >> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> >> @@ -1420,8 +1420,11 @@ 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;
> > 
> > Also not a big fan of this approach, but its better than it was.
> > BTW see relevant discussion about colorspace (this is more in the
> > output path) here [1].
> > 
> > [1] https://patchwork.kernel.org/patch/9495153/
> > 
> >> +	else
> >> +		hdmi->hdmi_data.enc_in_format = RGB;
> >> 
> >>  	hdmi->hdmi_data.enc_out_format = RGB;
> >> 
> >> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> >> index d6a0ab3..4f426c3 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_INPUT_FMT_RGB = 0,
> >> +	DW_HDMI_INPUT_FMT_YCBCR444,
> >> +	DW_HDMI_INPUT_FMT_YCBCR422_16BITS,
> >> +	DW_HDMI_INPUT_FMT_YCBCR422_8BITS,
> > 
> > Hmm, did you test these two? I'm not sure if deep color can be
> > converted using CSC.
> 
> I simply copied the dw-hdmi internal values.
> Proper handling of input formats capabilities and
> output format pixel clock handling should be added sometime.
> In the meantime, it's worth adding which input is supported.
> 
> This approach is very limited, should I add a bitmask with all
> support input formats and select between RGB, YUV444 or YUV422
> in dw_hdmi_setup ?

Ideally the bridge mode set operation should be extended to take format and 
colorspace information (or another bridge operation should be created for that 
purpose, I'm still undecided). As that's quite a big change, I'm fine if you 
start by passing a fixed format and colorspace through platform data. I would 
however like the format to use the media bus format codes defined in 
include/uapi/linux/media-bus-format.h.

As far as I'm aware we have no colorspace definitions in DRM, so we would need 
to fix that. V4L2 handles colorspaces, and the API went through several 
iterations before we got it working, with stupid mistakes that we don't want 
to repeat here.

Jose pointed to https://patchwork.kernel.org/patch/9495153/ as a starting 
point, and I would like to point out the format and colorspace are two 
different things. A colorspace is a combination of an encoding and  
quantization and transfer functions. Hans Verkuil has researched this topic 
for V4L2 (see https://gstreamer.freedesktop.org/data/events/gstreamer-conference/2015/Hans Verkuil - Colorspaces and HDMI.pdf and 
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/colorspaces.html), we 
*really* want to involve him in this discussion.

I believe colorspace information should be shared between V4L2 and DRM the 
same way we share the media bus formats (We should have done so for 4CCs as 
well but it's unfortunately too late for that).

> >> +	DW_HDMI_INPUT_FMT_XVYCC444,
> >> +};
> >> +
> >>  enum dw_hdmi_phy_type {
> >>  	DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
> >>  	DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
> >> @@ -68,6 +76,7 @@ struct dw_hdmi_plat_data {
> >>  				 const struct dw_hdmi_plat_data *data);
> >>  	bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi,
> >>  			      const struct dw_hdmi_plat_data *data);
> >> +	int input_fmt;
> >>  };
> >>  
> >>  int dw_hdmi_probe(struct platform_device *pdev,

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1562820 — Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data

Hi Laurent,


On 18-01-2017 20:49, Laurent Pinchart wrote:
>
> Ideally the bridge mode set operation should be extended to take format and 
> colorspace information (or another bridge operation should be created for that 
> purpose, I'm still undecided). As that's quite a big change, I'm fine if you 
> start by passing a fixed format and colorspace through platform data. I would 
> however like the format to use the media bus format codes defined in 
> include/uapi/linux/media-bus-format.h.
>
> As far as I'm aware we have no colorspace definitions in DRM, so we would need 
> to fix that. V4L2 handles colorspaces, and the API went through several 
> iterations before we got it working, with stupid mistakes that we don't want 
> to repeat here.
>
> Jose pointed to https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9495153_&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=V21eO0BUlcWrbddiKml5I6ylkiX2ALOHPHmble7kxwI&e=  as a starting 
> point, and I would like to point out the format and colorspace are two 
> different things. A colorspace is a combination of an encoding and  
> quantization and transfer functions. Hans Verkuil has researched this topic 
> for V4L2 (see https://urldefense.proofpoint.com/v2/url?u=https-3A__gstreamer.freedesktop.org_data_events_gstreamer-2Dconference_2015_Hans&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=z9IMQn5gPepKFfsLUwUXiG8MA2s1SBB1jyQDE0JIKdk&e=  Verkuil - Colorspaces and HDMI.pdf and 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__linuxtv.org_downloads_v4l-2Ddvb-2Dapis_uapi_v4l_colorspaces.html&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=QReIV1CxgeALvbP5QObIvfwIh7hjEL8J4fxiATaMVYc&e= ), we 
> *really* want to involve him in this discussion.
>
> I believe colorspace information should be shared between V4L2 and DRM the 
> same way we share the media bus formats (We should have done so for 4CCs as 
> well but it's unfortunately too late for that).
>
>

Hmm, I am always confusing this :/ So, what I was refering as
"color space" is in reality the encoding (pixel encoding). In
HDMI the pixel encoding can be RGB 4:4:4, YCbCr 4:4:4, YCbCr
4:2:2 and YCbCr 4:2:0. We also have color depth which can be from
8-bit to 16-bit. Besides this there is also colorimetry.

I've used media-bus-format.h to successfully pass information
across a V4L2 driver but I've used it in BGR24 only, which is RGB
4:4:4 8 bit, which in media-bust-format.h would correspond to
MEDIA_BUS_FMT_BGR888_1X24. So, I don't know exactly what is the
support for other encodings and what if there's support for color
depth. If there is then great, but if not then support for this
should also be added.

DRM already parses successfully the supported encodings from EDID
and stores them in a structure. Note that this is the output
encoding, not the input one. We can still have RGB as input and
then output at YCbCr using CSC. The missing point is the
selection of encoding (either from userspace or from some sort of
automatic mechanism by the kernel).

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1562899 — Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data

Hi Hans and Jose,

On Thursday 19 Jan 2017 16:30:57 Hans Verkuil wrote:
> On 01/19/17 16:21, Jose Abreu wrote:
> > On 18-01-2017 20:49, Laurent Pinchart wrote:
> >> Ideally the bridge mode set operation should be extended to take format
> >> and colorspace information (or another bridge operation should be created
> >> for that purpose, I'm still undecided). As that's quite a big change, I'm
> >> fine if you start by passing a fixed format and colorspace through
> >> platform data. I would however like the format to use the media bus
> >> format codes defined in include/uapi/linux/media-bus-format.h.
> >> 
> >> As far as I'm aware we have no colorspace definitions in DRM, so we would
> >> need to fix that. V4L2 handles colorspaces, and the API went through
> >> several iterations before we got it working, with stupid mistakes that
> >> we don't want to repeat here.
> >> 
> >> Jose pointed to
> >> https://patchwork.kernel.org/patch/9495153/ as a starting point, and I
> >> would like to point out the format and colorspace are two different
> >> things. A colorspace is a combination of an encoding and quantization
> >> and transfer functions. Hans Verkuil has researched this topic for V4L2
> >> (see https://gstreamer.freedesktop.org/data/events/gstreamer-> >> conference/2015/Hans Verkuil - Colorspaces and HDMI.pdf and
> >> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/colorspaces.html), we
> >> *really* want to involve him in this discussion.
> >> 
> >> I believe colorspace information should be shared between V4L2 and DRM
> >> the same way we share the media bus formats (We should have done so for
> >> 4CCs as well but it's unfortunately too late for that).
> > 
> > Hmm, I am always confusing this :/ So, what I was refering as
> > "color space" is in reality the encoding (pixel encoding). In
> > HDMI the pixel encoding can be RGB 4:4:4, YCbCr 4:4:4, YCbCr
> > 4:2:2 and YCbCr 4:2:0. We also have color depth which can be from
> > 8-bit to 16-bit. Besides this there is also colorimetry.

There's two separate concepts here, the color encoding and the pixel format. 
The color encoding defines how non-linear R'G'B' is transformed to non-linear 
Y'CbCr (or whether to stick to R'G'B' of course). It's a mathematical formula, 
and along with the quantization defines how the numerical Y'CbCr values are 
obtained. The pixel format then defines the number of bits per sample, as well 
as the subsampling factors.

The media bus codes thus roughly correspond to pixel formats (although they 
also define whether the color encoding uses RGB or YCbCr, but don't define the 
color encoding itself or the quantization method).

> > I've used media-bus-format.h to successfully pass information
> > across a V4L2 driver but I've used it in BGR24 only, which is RGB
> > 4:4:4 8 bit, which in media-bust-format.h would correspond to
> > MEDIA_BUS_FMT_BGR888_1X24. So, I don't know exactly what is the
> > support for other encodings and what if there's support for color
> > depth. If there is then great, but if not then support for this
> > should also be added.

We have a bunch of YUV media bus formats defined in the kernel (see 
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/subdev-formats.html#v4l2-mbus-pixelcode). We're missing some that would be needed here (namely YUV 
4:4:4 in 12bpp and I believe the 4:2:0 formats), but it's just a matter of 
adding new definitions with the related documentation.

> > DRM already parses successfully the supported encodings from EDID
> > and stores them in a structure. Note that this is the output
> > encoding, not the input one. We can still have RGB as input and
> > then output at YCbCr using CSC. The missing point is the
> > selection of encoding (either from userspace or from some sort of
> > automatic mechanism by the kernel).

We need to select both the input and output formats and encodings, and, yes, I 
believe that at least the output format and encoding should come from 
userspace.

> The list of supported encodings for video capture depends on the HW
> capabilities. So the driver will list the supported formats (ENUMFMT)
> and userspace then selects a format (S_FMT) from that list.

Please note that this is about HDMI encoders and the DRM/KMS subsystem :-)

> Most HDMI receivers can convert the input to various RGB/YCbCr formats.
> 
> Deep color support for HDMI has not been added (surprisingly nobody needed
> it apparently), but it is pretty easy: new V4L2_PIX_FMT defines should be
> added for those.

New media bus formats in this case.

> When talking about RGB/YCbCr I prefer the term 'color encoding', since
> this has nothing to do with colorspaces (sRGB. AdobeRGB, BT.2020, etc.)

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1562933 — Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data

On 01/19/17 16:21, Jose Abreu wrote:
> Hi Laurent,
>
>
> On 18-01-2017 20:49, Laurent Pinchart wrote:
>>
>> Ideally the bridge mode set operation should be extended to take format and
>> colorspace information (or another bridge operation should be created for that
>> purpose, I'm still undecided). As that's quite a big change, I'm fine if you
>> start by passing a fixed format and colorspace through platform data. I would
>> however like the format to use the media bus format codes defined in
>> include/uapi/linux/media-bus-format.h.
>>
>> As far as I'm aware we have no colorspace definitions in DRM, so we would need
>> to fix that. V4L2 handles colorspaces, and the API went through several
>> iterations before we got it working, with stupid mistakes that we don't want
>> to repeat here.
>>
>> Jose pointed to https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9495153_&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=V21eO0BUlcWrbddiKml5I6ylkiX2ALOHPHmble7kxwI&e=  as a starting
>> point, and I would like to point out the format and colorspace are two
>> different things. A colorspace is a combination of an encoding and
>> quantization and transfer functions. Hans Verkuil has researched this topic
>> for V4L2 (see https://urldefense.proofpoint.com/v2/url?u=https-3A__gstreamer.freedesktop.org_data_events_gstreamer-2Dconference_2015_Hans&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=z9IMQn5gPepKFfsLUwUXiG8MA2s1SBB1jyQDE0JIKdk&e=  Verkuil - Colorspaces and HDMI.pdf and
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__linuxtv.org_downloads_v4l-2Ddvb-2Dapis_uapi_v4l_colorspaces.html&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=QReIV1CxgeALvbP5QObIvfwIh7hjEL8J4fxiATaMVYc&e= ), we
>> *really* want to involve him in this discussion.
>>
>> I believe colorspace information should be shared between V4L2 and DRM the
>> same way we share the media bus formats (We should have done so for 4CCs as
>> well but it's unfortunately too late for that).
>>
>>
>
> Hmm, I am always confusing this :/ So, what I was refering as
> "color space" is in reality the encoding (pixel encoding). In
> HDMI the pixel encoding can be RGB 4:4:4, YCbCr 4:4:4, YCbCr
> 4:2:2 and YCbCr 4:2:0. We also have color depth which can be from
> 8-bit to 16-bit. Besides this there is also colorimetry.
>
> I've used media-bus-format.h to successfully pass information
> across a V4L2 driver but I've used it in BGR24 only, which is RGB
> 4:4:4 8 bit, which in media-bust-format.h would correspond to
> MEDIA_BUS_FMT_BGR888_1X24. So, I don't know exactly what is the
> support for other encodings and what if there's support for color
> depth. If there is then great, but if not then support for this
> should also be added.
>
> DRM already parses successfully the supported encodings from EDID
> and stores them in a structure. Note that this is the output
> encoding, not the input one. We can still have RGB as input and
> then output at YCbCr using CSC. The missing point is the
> selection of encoding (either from userspace or from some sort of
> automatic mechanism by the kernel).

The list of supported encodings for video capture depends on the HW
capabilities. So the driver will list the supported formats (ENUMFMT)
and userspace then selects a format (S_FMT) from that list.

Most HDMI receivers can convert the input to various RGB/YCbCr formats.

Deep color support for HDMI has not been added (surprisingly nobody needed
it apparently), but it is pretty easy: new V4L2_PIX_FMT defines should be
added for those.

When talking about RGB/YCbCr I prefer the term 'color encoding', since
this has nothing to do with colorspaces (sRGB. AdobeRGB, BT.2020, etc.)

Regards,

	Hans

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web