Re: [PATCH RFC v2 1/8] drm/bridge: dw-hdmi: Add SCDC and TMDS Scrambling support

Re: [PATCH RFC v2 1/8] drm/bridge: dw-hdmi: Add SCDC and TMDS Scrambling support

[Heiko Stuebner]

Am Freitag, 30. November 2018, 14:42:54 CET schrieb Neil Armstrong:
> Add support for SCDC Setup for TMDS Clock > 3.4GHz and enable TMDS
> Scrambling when supported or mandatory.
> 
> This patch also adds an helper to setup the control bit to support
> the high TMDS Bit Period/TMDS Clock-Period Ratio as required with
> TMDS Clock > 3.4GHz for HDMI2.0 3840x2160@60/50 modes.
> 
> These changes were based on work done by Huicong Xu <xhc@rock-chips.com>
> and Nickey Yang <nickey.yang@rock-chips.com> to support HDMI2.0 modes
> on the Rockchip 4.4 BSP kernel at [1]
> 
> [1] https://github.com/rockchip-linux/kernel/tree/release-4.4
> 
> Cc: Nickey Yang <nickey.yang@rock-chips.com>
> Cc: Huicong Xu <xhc@rock-chips.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

sorry this took a bit longer, but I can confirm that the 4 relevant
patches (1, 4, 5, 6) at least still provide 1080p hdmi output on
rk3288 (with internal hdmiphy) and rk3328 (with external innosilicon
hdmiphy). I don't know how to test newly added features, but at
least the patches don't seem to break existing users, so

on rk3288 and rk3328
Tested-by: Heiko Stuebner <heiko@sntech.de>




_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 4/8] drm/bridge: dw-hdmi: add support for YUV420 output

[Heiko Stuebner]

Am Freitag, 30. November 2018, 14:42:57 CET schrieb Neil Armstrong:
> In order to support the HDMI2.0 YUV420 display modes, this patch
> adds support for the YUV420 TMDS Clock divided by 2 and the controller
> passthrough mode.
> 
> YUV420 Synopsys PHY support will need some specific configuration table
> to support theses modes.
> 
> This patch is based on work from Zheng Yang <zhengyang@rock-chips.com> in
> the Rockchip Linux 4.4 BSP at [1]
> 
> [1] https://github.com/rockchip-linux/kernel/tree/release-4.4
> 
> Cc: Zheng Yang <zhengyang@rock-chips.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Please see comments in patch1 for details.

on rk3288 and rk3328
Tested-by: Heiko Stuebner <heiko@sntech.de>




_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 5/8] drm/bridge: dw-hdmi: support dynamically get input/out color info

[Heiko Stuebner]

Am Freitag, 30. November 2018, 14:42:58 CET schrieb Neil Armstrong:
> From: Zheng Yang <zhengyang@rock-chips.com>
> 
> To get input/output bus_format/enc_format dynamically, this patch
> introduce following funstion in plat_data:
> 	- get_input_bus_format
> 	- get_output_bus_format
> 	- get_enc_in_encoding
> 	- get_enc_out_encoding
> 
> Signed-off-by: Zheng Yang <zhengyang@rock-chips.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Please see comments in patch1 for details.

on rk3288 and rk3328
Tested-by: Heiko Stuebner <heiko@sntech.de>




_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 6/8] drm/bridge: dw-hdmi: allow ycbcr420 modes for >= 0x200a

[Heiko Stuebner]

Am Freitag, 30. November 2018, 14:42:59 CET schrieb Neil Armstrong:
> Now the DW-HDMI Controller supports the HDMI2.0 modes, enable support
> for these modes in the connector if the platform supports them.
> We limit these modes to DW-HDMI IP version >= 0x200a which
> are designed to support HDMI2.0 display modes.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Please see comments in patch1 for details.

on rk3288 and rk3328
Tested-by: Heiko Stuebner <heiko@sntech.de>




_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 1/8] drm/bridge: dw-hdmi: Add SCDC and TMDS Scrambling support

[Andrzej Hajda]

Hi Neil,


On 30.11.2018 14:42, Neil Armstrong wrote:
> Add support for SCDC Setup for TMDS Clock > 3.4GHz and enable TMDS
> Scrambling when supported or mandatory.
>
> This patch also adds an helper to setup the control bit to support
> the high TMDS Bit Period/TMDS Clock-Period Ratio as required with
> TMDS Clock > 3.4GHz for HDMI2.0 3840x2160@60/50 modes.
>
> These changes were based on work done by Huicong Xu <xhc@rock-chips.com>
> and Nickey Yang <nickey.yang@rock-chips.com> to support HDMI2.0 modes
> on the Rockchip 4.4 BSP kernel at [1]
>
> [1] https://github.com/rockchip-linux/kernel/tree/release-4.4
>
> Cc: Nickey Yang <nickey.yang@rock-chips.com>
> Cc: Huicong Xu <xhc@rock-chips.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 88 ++++++++++++++++++++++-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.h |  1 +
>  include/drm/bridge/dw_hdmi.h              |  1 +
>  3 files changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 64c3cf027518..fcd941d52753 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -28,6 +28,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_encoder_slave.h>
> +#include <drm/drm_scdc_helper.h>
>  #include <drm/bridge/dw_hdmi.h>
>  
>  #include <uapi/linux/media-bus-format.h>
> @@ -43,6 +44,11 @@
>  
>  #define HDMI_EDID_LEN		512
>  
> +/* DW-HDMI Controller >= 0x200a are at least compliant with SCDC version 1 */
> +#define SCDC_MIN_SOURCE_VERSION	0x1
> +
> +#define HDMI14_MAX_TMDSCLK	340000000
> +
>  enum hdmi_datamap {
>  	RGB444_8B = 0x01,
>  	RGB444_10B = 0x03,
> @@ -1015,6 +1021,33 @@ void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
>  }
>  EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_write);
>  
> +/*
> + * HDMI2.0 Specifies the following procedure for High TMDS Bit Rates:
> + * - The Source shall suspend transmission of the TMDS clock and data
> + * - The Source shall write to the TMDS_Bit_Clock_Ratio bit to change it
> + * from a 0 to a 1 or from a 1 to a 0
> + * - The Source shall allow a minimum of 1 ms and a maximum of 100 ms from
> + * the time the TMDS_Bit_Clock_Ratio bit is written until resuming
> + * transmission of TMDS clock and data
> + *
> + * To respect the 100ms maximum delay, the dw_hdmi_set_high_tmds_clock_ratio()
> + * helper should called right before enabling the TMDS Clock and Data in
> + * the PHY configuration callback.
> + */
> +void dw_hdmi_set_high_tmds_clock_ratio(struct dw_hdmi *hdmi)
> +{
> +	unsigned long mtmdsclock = hdmi->hdmi_data.video_mode.mpixelclock;
> +
> +	/* Control for TMDS Bit Period/TMDS Clock-Period Ratio */
> +	if (hdmi->connector.display_info.hdmi.scdc.supported) {
> +		if (mtmdsclock > HDMI14_MAX_TMDSCLK)
> +			drm_scdc_set_high_tmds_clock_ratio(hdmi->ddc, 1);
> +		else
> +			drm_scdc_set_high_tmds_clock_ratio(hdmi->ddc, 0);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_set_high_tmds_clock_ratio);
> +
>  static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable)
>  {
>  	hdmi_mask_writeb(hdmi, !enable, HDMI_PHY_CONF0,
> @@ -1216,6 +1249,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>  
>  	dw_hdmi_phy_power_off(hdmi);
>  
> +	dw_hdmi_set_high_tmds_clock_ratio(hdmi);
> +
>  	/* Leave low power consumption mode by asserting SVSRET. */
>  	if (phy->has_svsret)
>  		dw_hdmi_phy_enable_svsret(hdmi, 1);
> @@ -1237,6 +1272,10 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>  		return ret;
>  	}
>  
> +	/* Wait for resuming transmission of TMDS clock and data */
> +	if (mpixelclock > HDMI14_MAX_TMDSCLK)
> +		msleep(100);
> +
>  	return dw_hdmi_phy_power_on(hdmi);
>  }
>  
> @@ -1340,11 +1379,12 @@ static void hdmi_tx_hdcp_config(struct dw_hdmi *hdmi)
>  
>  static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  {
> +	bool is_hdmi2_sink = hdmi->connector.display_info.hdmi.scdc.supported;
>  	struct hdmi_avi_infoframe frame;
>  	u8 val;
>  
>  	/* Initialise info frame from DRM mode */
> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
> +	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, is_hdmi2_sink);
>  
>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> @@ -1503,7 +1543,8 @@ static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
>  static void hdmi_av_composer(struct dw_hdmi *hdmi,
>  			     const struct drm_display_mode *mode)
>  {
> -	u8 inv_val;
> +	u8 inv_val, bytes;
> +	struct drm_hdmi_info *hdmi_info = &hdmi->connector.display_info.hdmi;
>  	struct hdmi_vmode *vmode = &hdmi->hdmi_data.video_mode;
>  	int hblank, vblank, h_de_hs, v_de_vs, hsync_len, vsync_len;
>  	unsigned int vdisplay;
> @@ -1513,7 +1554,9 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
>  	dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);
>  
>  	/* Set up HDMI_FC_INVIDCONF */
> -	inv_val = (hdmi->hdmi_data.hdcp_enable ?
> +	inv_val = (hdmi->hdmi_data.hdcp_enable ||
> +		   vmode->mpixelclock > HDMI14_MAX_TMDSCLK ||
> +		   hdmi_info->scdc.scrambling.low_rates ?
>  		HDMI_FC_INVIDCONF_HDCP_KEEPOUT_ACTIVE :
>  		HDMI_FC_INVIDCONF_HDCP_KEEPOUT_INACTIVE);
>  
> @@ -1562,6 +1605,45 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
>  		vsync_len /= 2;
>  	}
>  
> +	/* Scrambling Control */
> +	if (hdmi_info->scdc.supported) {
> +		if (vmode->mpixelclock > HDMI14_MAX_TMDSCLK ||
> +		    hdmi_info->scdc.scrambling.low_rates) {
> +			/*
> +			 * HDMI2.0 Specifies the following procedure:
> +			 * After the Source Device has determined that
> +			 * SCDC_Present is set (=1), the Source Device should
> +			 * write the accurate Version of the Source Device
> +			 * to the Source Version field in the SCDCS.
> +			 * Source Devices compliant shall set the
> +			 * Source Version = 1.
> +			 */
> +			drm_scdc_readb(&hdmi->i2c->adap, SCDC_SINK_VERSION,
> +				       &bytes);
> +			drm_scdc_writeb(&hdmi->i2c->adap, SCDC_SOURCE_VERSION,
> +				min_t(u8, bytes, SCDC_MIN_SOURCE_VERSION));
> +
> +			/* Enabled Scrambling in the Sink */
> +			drm_scdc_set_scrambling(&hdmi->i2c->adap, 1);
> +
> +			/*
> +			 * To activate the scrambler feature, you must ensure
> +			 * that the quasi-static configuration bit
> +			 * fc_invidconf.HDCP_keepout is set at configuration
> +			 * time, before the required mc_swrstzreq.tmdsswrst_req
> +			 * reset request is issued.
> +			 */
> +			hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ,


Are you sure you need casting to u8?


> +				    HDMI_MC_SWRSTZ);
> +			hdmi_writeb(hdmi, 1, HDMI_FC_SCRAMBLER_CTRL);
> +		} else {
> +			hdmi_writeb(hdmi, 0, HDMI_FC_SCRAMBLER_CTRL);
> +			hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ,
> +				    HDMI_MC_SWRSTZ);
> +			drm_scdc_set_scrambling(&hdmi->i2c->adap, 0);
> +		}
> +	}
> +
>  	/* Set up horizontal active pixel width */
>  	hdmi_writeb(hdmi, mode->hdisplay >> 8, HDMI_FC_INHACTV1);
>  	hdmi_writeb(hdmi, mode->hdisplay, HDMI_FC_INHACTV0);
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> index 9d90eb9c46e5..3f3c616eba97 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> @@ -255,6 +255,7 @@
>  #define HDMI_FC_MASK2                           0x10DA
>  #define HDMI_FC_POL2                            0x10DB
>  #define HDMI_FC_PRCONF                          0x10E0
> +#define HDMI_FC_SCRAMBLER_CTRL                  0x10E1
>  
>  #define HDMI_FC_GMD_STAT                        0x1100
>  #define HDMI_FC_GMD_EN                          0x1101
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 9c56412bb2cf..7a02744ce0bc 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -157,6 +157,7 @@ void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, 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);
> +void dw_hdmi_set_high_tmds_clock_ratio(struct dw_hdmi *hdmi);
>  
>  /* PHY configuration */
>  void dw_hdmi_phy_i2c_set_addr(struct dw_hdmi *hdmi, u8 address);

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej




_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 2/8] drm/meson: add HDMI div40 TMDS mode

[Andrzej Hajda]

On 30.11.2018 14:42, Neil Armstrong wrote:
> Add support for TMDS Clock > 3.4GHz for HDMI2.0 display modes.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/meson/meson_dw_hdmi.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 807111ebfdd9..b8775102b100 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -365,7 +365,8 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>  	unsigned int wr_clk =
>  		readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
>  
> -	DRM_DEBUG_DRIVER("%d:\"%s\"\n", mode->base.id, mode->name);
> +	DRM_DEBUG_DRIVER("%d:\"%s\" div%d\n", mode->base.id, mode->name,
> +			 mode->clock > 340000 ? 40 : 10);
>  
>  	/* Enable clocks */
>  	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0xffff, 0x100);
> @@ -385,9 +386,17 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>  	/* Enable normal output to PHY */
>  	dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
>  
> -	/* TMDS pattern setup (TOFIX pattern for 4k2k scrambling) */
> -	dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01, 0x001f001f);
> -	dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23, 0x001f001f);
> +	/* TMDS pattern setup (TOFIX Handle the YUV420 case) */
> +	if (mode->clock > 340000) {
> +		dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01, 0);
> +		dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23,
> +				  0x03ff03ff);
> +	} else {
> +		dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
> +				  0x001f001f);
> +		dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23,
> +				  0x001f001f);
> +	}
>  
>  	/* Load TMDS pattern */
>  	dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_CNTL, 0x1);
> @@ -413,6 +422,8 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>  	/* Disable clock, fifo, fifo_wr */
>  	regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1, 0xf, 0);
>  
> +	dw_hdmi_set_high_tmds_clock_ratio(hdmi);
> +
>  	msleep(100);
>  
>  	/* Reset PHY 3 times in a row */
> @@ -562,6 +573,11 @@ dw_hdmi_mode_valid(struct drm_connector *connector,
>  		mode->vdisplay, mode->vsync_start,
>  		mode->vsync_end, mode->vtotal, mode->type, mode->flags);
>  
> +	/* If sink max TMDS clock < 340MHz, we reject the HDMI2.0 modes */
> +	if (mode->clock > 340000 &&
> +	    connector->display_info.max_tmds_clock < 340000)
> +		return MODE_BAD;
> +


Why not just:

if (mode->clock > connector->display_info.max_tmds_clock)
	return MODE_BAD;


Regards

Andrzej


>  	/* Check against non-VIC supported modes */
>  	if (!vic) {
>  		status = meson_venc_hdmi_supported_mode(mode);



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 3/8] drm/meson: add support for HDMI2.0 2160p modes

[Andrzej Hajda]

On 30.11.2018 14:42, Neil Armstrong wrote:
> Now we support the TMDS Clock > 3.4GHz and support the SCDC Control
> operation in the DW-HDMI Controller, we can enable support for the
> HDMI2.0 3840x2160@60/50 RGB444 display modes.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/meson/meson_venc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/meson/meson_venc.c b/drivers/gpu/drm/meson/meson_venc.c
> index 0ba04f6813e6..66d73a932d19 100644
> --- a/drivers/gpu/drm/meson/meson_venc.c
> +++ b/drivers/gpu/drm/meson/meson_venc.c
> @@ -848,6 +848,8 @@ struct meson_hdmi_venc_vic_mode {
>  	{ 93, &meson_hdmi_encp_mode_2160p24 },
>  	{ 94, &meson_hdmi_encp_mode_2160p25 },
>  	{ 95, &meson_hdmi_encp_mode_2160p30 },
> +	{ 96, &meson_hdmi_encp_mode_2160p25 },
> +	{ 97, &meson_hdmi_encp_mode_2160p30 },
>  	{ 0, NULL}, /* sentinel */
>  };
>  

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 1/8] drm/bridge: dw-hdmi: Add SCDC and TMDS Scrambling support

[Neil Armstrong]

On 18/12/2018 13:25, Andrzej Hajda wrote:
> Hi Neil,
> 
> 
> On 30.11.2018 14:42, Neil Armstrong wrote:
>> Add support for SCDC Setup for TMDS Clock > 3.4GHz and enable TMDS
>> Scrambling when supported or mandatory.
>>
>> This patch also adds an helper to setup the control bit to support
>> the high TMDS Bit Period/TMDS Clock-Period Ratio as required with
>> TMDS Clock > 3.4GHz for HDMI2.0 3840x2160@60/50 modes.
>>
>> These changes were based on work done by Huicong Xu <xhc@rock-chips.com>
>> and Nickey Yang <nickey.yang@rock-chips.com> to support HDMI2.0 modes
>> on the Rockchip 4.4 BSP kernel at [1]
>>
>> [1] https://github.com/rockchip-linux/kernel/tree/release-4.4
>>
>> Cc: Nickey Yang <nickey.yang@rock-chips.com>
>> Cc: Huicong Xu <xhc@rock-chips.com>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 88 ++++++++++++++++++++++-
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.h |  1 +
>>  include/drm/bridge/dw_hdmi.h              |  1 +
>>  3 files changed, 87 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 64c3cf027518..fcd941d52753 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -28,6 +28,7 @@
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_edid.h>
>>  #include <drm/drm_encoder_slave.h>
>> +#include <drm/drm_scdc_helper.h>
>>  #include <drm/bridge/dw_hdmi.h>
>>  
>>  #include <uapi/linux/media-bus-format.h>
>> @@ -43,6 +44,11 @@
>>  
>>  #define HDMI_EDID_LEN		512
>>  
>> +/* DW-HDMI Controller >= 0x200a are at least compliant with SCDC version 1 */
>> +#define SCDC_MIN_SOURCE_VERSION	0x1
>> +
>> +#define HDMI14_MAX_TMDSCLK	340000000
>> +
>>  enum hdmi_datamap {
>>  	RGB444_8B = 0x01,
>>  	RGB444_10B = 0x03,
>> @@ -1015,6 +1021,33 @@ void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
>>  }
>>  EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_write);
>>  
>> +/*
>> + * HDMI2.0 Specifies the following procedure for High TMDS Bit Rates:
>> + * - The Source shall suspend transmission of the TMDS clock and data
>> + * - The Source shall write to the TMDS_Bit_Clock_Ratio bit to change it
>> + * from a 0 to a 1 or from a 1 to a 0
>> + * - The Source shall allow a minimum of 1 ms and a maximum of 100 ms from
>> + * the time the TMDS_Bit_Clock_Ratio bit is written until resuming
>> + * transmission of TMDS clock and data
>> + *
>> + * To respect the 100ms maximum delay, the dw_hdmi_set_high_tmds_clock_ratio()
>> + * helper should called right before enabling the TMDS Clock and Data in
>> + * the PHY configuration callback.
>> + */
>> +void dw_hdmi_set_high_tmds_clock_ratio(struct dw_hdmi *hdmi)
>> +{
>> +	unsigned long mtmdsclock = hdmi->hdmi_data.video_mode.mpixelclock;
>> +
>> +	/* Control for TMDS Bit Period/TMDS Clock-Period Ratio */
>> +	if (hdmi->connector.display_info.hdmi.scdc.supported) {
>> +		if (mtmdsclock > HDMI14_MAX_TMDSCLK)
>> +			drm_scdc_set_high_tmds_clock_ratio(hdmi->ddc, 1);
>> +		else
>> +			drm_scdc_set_high_tmds_clock_ratio(hdmi->ddc, 0);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(dw_hdmi_set_high_tmds_clock_ratio);
>> +
>>  static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable)
>>  {
>>  	hdmi_mask_writeb(hdmi, !enable, HDMI_PHY_CONF0,
>> @@ -1216,6 +1249,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>>  
>>  	dw_hdmi_phy_power_off(hdmi);
>>  
>> +	dw_hdmi_set_high_tmds_clock_ratio(hdmi);
>> +
>>  	/* Leave low power consumption mode by asserting SVSRET. */
>>  	if (phy->has_svsret)
>>  		dw_hdmi_phy_enable_svsret(hdmi, 1);
>> @@ -1237,6 +1272,10 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>>  		return ret;
>>  	}
>>  
>> +	/* Wait for resuming transmission of TMDS clock and data */
>> +	if (mpixelclock > HDMI14_MAX_TMDSCLK)
>> +		msleep(100);
>> +
>>  	return dw_hdmi_phy_power_on(hdmi);
>>  }
>>  
>> @@ -1340,11 +1379,12 @@ static void hdmi_tx_hdcp_config(struct dw_hdmi *hdmi)
>>  
>>  static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>>  {
>> +	bool is_hdmi2_sink = hdmi->connector.display_info.hdmi.scdc.supported;
>>  	struct hdmi_avi_infoframe frame;
>>  	u8 val;
>>  
>>  	/* Initialise info frame from DRM mode */
>> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
>> +	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, is_hdmi2_sink);
>>  
>>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
>> @@ -1503,7 +1543,8 @@ static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
>>  static void hdmi_av_composer(struct dw_hdmi *hdmi,
>>  			     const struct drm_display_mode *mode)
>>  {
>> -	u8 inv_val;
>> +	u8 inv_val, bytes;
>> +	struct drm_hdmi_info *hdmi_info = &hdmi->connector.display_info.hdmi;
>>  	struct hdmi_vmode *vmode = &hdmi->hdmi_data.video_mode;
>>  	int hblank, vblank, h_de_hs, v_de_vs, hsync_len, vsync_len;
>>  	unsigned int vdisplay;
>> @@ -1513,7 +1554,9 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
>>  	dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);
>>  
>>  	/* Set up HDMI_FC_INVIDCONF */
>> -	inv_val = (hdmi->hdmi_data.hdcp_enable ?
>> +	inv_val = (hdmi->hdmi_data.hdcp_enable ||
>> +		   vmode->mpixelclock > HDMI14_MAX_TMDSCLK ||
>> +		   hdmi_info->scdc.scrambling.low_rates ?
>>  		HDMI_FC_INVIDCONF_HDCP_KEEPOUT_ACTIVE :
>>  		HDMI_FC_INVIDCONF_HDCP_KEEPOUT_INACTIVE);
>>  
>> @@ -1562,6 +1605,45 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
>>  		vsync_len /= 2;
>>  	}
>>  
>> +	/* Scrambling Control */
>> +	if (hdmi_info->scdc.supported) {
>> +		if (vmode->mpixelclock > HDMI14_MAX_TMDSCLK ||
>> +		    hdmi_info->scdc.scrambling.low_rates) {
>> +			/*
>> +			 * HDMI2.0 Specifies the following procedure:
>> +			 * After the Source Device has determined that
>> +			 * SCDC_Present is set (=1), the Source Device should
>> +			 * write the accurate Version of the Source Device
>> +			 * to the Source Version field in the SCDCS.
>> +			 * Source Devices compliant shall set the
>> +			 * Source Version = 1.
>> +			 */
>> +			drm_scdc_readb(&hdmi->i2c->adap, SCDC_SINK_VERSION,
>> +				       &bytes);
>> +			drm_scdc_writeb(&hdmi->i2c->adap, SCDC_SOURCE_VERSION,
>> +				min_t(u8, bytes, SCDC_MIN_SOURCE_VERSION));
>> +
>> +			/* Enabled Scrambling in the Sink */
>> +			drm_scdc_set_scrambling(&hdmi->i2c->adap, 1);
>> +
>> +			/*
>> +			 * To activate the scrambler feature, you must ensure
>> +			 * that the quasi-static configuration bit
>> +			 * fc_invidconf.HDCP_keepout is set at configuration
>> +			 * time, before the required mc_swrstzreq.tmdsswrst_req
>> +			 * reset request is issued.
>> +			 */
>> +			hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ,
> 
> 
> Are you sure you need casting to u8?

Not sure, it's already casted in dw_hdmi_clear_overflow() :

...
hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
...

I'll check if it's really needed.

> 
> 
>> +				    HDMI_MC_SWRSTZ);
>> +			hdmi_writeb(hdmi, 1, HDMI_FC_SCRAMBLER_CTRL);
>> +		} else {
>> +			hdmi_writeb(hdmi, 0, HDMI_FC_SCRAMBLER_CTRL);
>> +			hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ,
>> +				    HDMI_MC_SWRSTZ);
>> +			drm_scdc_set_scrambling(&hdmi->i2c->adap, 0);
>> +		}
>> +	}
>> +
>>  	/* Set up horizontal active pixel width */
>>  	hdmi_writeb(hdmi, mode->hdisplay >> 8, HDMI_FC_INHACTV1);
>>  	hdmi_writeb(hdmi, mode->hdisplay, HDMI_FC_INHACTV0);
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> index 9d90eb9c46e5..3f3c616eba97 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> @@ -255,6 +255,7 @@
>>  #define HDMI_FC_MASK2                           0x10DA
>>  #define HDMI_FC_POL2                            0x10DB
>>  #define HDMI_FC_PRCONF                          0x10E0
>> +#define HDMI_FC_SCRAMBLER_CTRL                  0x10E1
>>  
>>  #define HDMI_FC_GMD_STAT                        0x1100
>>  #define HDMI_FC_GMD_EN                          0x1101
>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>> index 9c56412bb2cf..7a02744ce0bc 100644
>> --- a/include/drm/bridge/dw_hdmi.h
>> +++ b/include/drm/bridge/dw_hdmi.h
>> @@ -157,6 +157,7 @@ void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, 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);
>> +void dw_hdmi_set_high_tmds_clock_ratio(struct dw_hdmi *hdmi);
>>  
>>  /* PHY configuration */
>>  void dw_hdmi_phy_i2c_set_addr(struct dw_hdmi *hdmi, u8 address);
> 
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Thanks,
Neil

> 
>  --
> Regards
> Andrzej
> 
> 
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 2/8] drm/meson: add HDMI div40 TMDS mode

[Neil Armstrong]

On 18/12/2018 13:36, Andrzej Hajda wrote:
> On 30.11.2018 14:42, Neil Armstrong wrote:
>> Add support for TMDS Clock > 3.4GHz for HDMI2.0 display modes.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/meson/meson_dw_hdmi.c | 24 ++++++++++++++++++++----
>>  1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> index 807111ebfdd9..b8775102b100 100644
>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> @@ -365,7 +365,8 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>>  	unsigned int wr_clk =
>>  		readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
>>  
>> -	DRM_DEBUG_DRIVER("%d:\"%s\"\n", mode->base.id, mode->name);
>> +	DRM_DEBUG_DRIVER("%d:\"%s\" div%d\n", mode->base.id, mode->name,
>> +			 mode->clock > 340000 ? 40 : 10);
>>  
>>  	/* Enable clocks */
>>  	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0xffff, 0x100);
>> @@ -385,9 +386,17 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>>  	/* Enable normal output to PHY */
>>  	dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
>>  
>> -	/* TMDS pattern setup (TOFIX pattern for 4k2k scrambling) */
>> -	dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01, 0x001f001f);
>> -	dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23, 0x001f001f);
>> +	/* TMDS pattern setup (TOFIX Handle the YUV420 case) */
>> +	if (mode->clock > 340000) {
>> +		dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01, 0);
>> +		dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23,
>> +				  0x03ff03ff);
>> +	} else {
>> +		dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
>> +				  0x001f001f);
>> +		dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23,
>> +				  0x001f001f);
>> +	}
>>  
>>  	/* Load TMDS pattern */
>>  	dw_hdmi_top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_CNTL, 0x1);
>> @@ -413,6 +422,8 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>>  	/* Disable clock, fifo, fifo_wr */
>>  	regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1, 0xf, 0);
>>  
>> +	dw_hdmi_set_high_tmds_clock_ratio(hdmi);
>> +
>>  	msleep(100);
>>  
>>  	/* Reset PHY 3 times in a row */
>> @@ -562,6 +573,11 @@ dw_hdmi_mode_valid(struct drm_connector *connector,
>>  		mode->vdisplay, mode->vsync_start,
>>  		mode->vsync_end, mode->vtotal, mode->type, mode->flags);
>>  
>> +	/* If sink max TMDS clock < 340MHz, we reject the HDMI2.0 modes */
>> +	if (mode->clock > 340000 &&
>> +	    connector->display_info.max_tmds_clock < 340000)
>> +		return MODE_BAD;
>> +
> 
> 
> Why not just:
> 
> if (mode->clock > connector->display_info.max_tmds_clock)
> 	return MODE_BAD;

Hmm, let me check, it may be better indeed.

Neil

> 
> 
> Regards
> 
> Andrzej
> 
> 
>>  	/* Check against non-VIC supported modes */
>>  	if (!vic) {
>>  		status = meson_venc_hdmi_supported_mode(mode);
> 
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 4/8] drm/bridge: dw-hdmi: add support for YUV420 output

[Andrzej Hajda]

On 30.11.2018 14:42, Neil Armstrong wrote:
> In order to support the HDMI2.0 YUV420 display modes, this patch
> adds support for the YUV420 TMDS Clock divided by 2 and the controller
> passthrough mode.
>
> YUV420 Synopsys PHY support will need some specific configuration table
> to support theses modes.
>
> This patch is based on work from Zheng Yang <zhengyang@rock-chips.com> in
> the Rockchip Linux 4.4 BSP at [1]
>
> [1] https://github.com/rockchip-linux/kernel/tree/release-4.4
>
> Cc: Zheng Yang <zhengyang@rock-chips.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>


Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 5/8] drm/bridge: dw-hdmi: support dynamically get input/out color info

[Andrzej Hajda]

On 30.11.2018 14:42, Neil Armstrong wrote:
> From: Zheng Yang <zhengyang@rock-chips.com>
>
> To get input/output bus_format/enc_format dynamically, this patch
> introduce following funstion in plat_data:
> 	- get_input_bus_format
> 	- get_output_bus_format
> 	- get_enc_in_encoding
> 	- get_enc_out_encoding


It seems fishy. On one side description says about dynamic resolution of
formats and encodings.

On the other side these functions as only argument takes platform_data
which should be rather static.

Where is this "dynamic" thing? The only usage of these callbacks I have
found in next patches is also not dynamic, the functions just return
some static value.

Moreover function takes void* argument, which is again something
suspicious, why cannot you pass know structure?

And finally encoding usually should depend on display mode, it should
not depend only static data.


What kind of problems do you want to solve here?


Regards

Andrzej



>
> Signed-off-by: Zheng Yang <zhengyang@rock-chips.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 28 +++++++++++++++++------
>  include/drm/bridge/dw_hdmi.h              |  5 ++++
>  2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 4a9a24e854db..bd564ffdf18b 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1810,6 +1810,7 @@ static void hdmi_disable_overflow_interrupts(struct dw_hdmi *hdmi)
>  static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  {
>  	int ret;
> +	void *data = hdmi->plat_data->phy_data;
>  
>  	hdmi_disable_overflow_interrupts(hdmi);
>  
> @@ -1821,10 +1822,13 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  		dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
>  	}
>  
> -	if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> -	    (hdmi->vic == 21) || (hdmi->vic == 22) ||
> -	    (hdmi->vic == 2) || (hdmi->vic == 3) ||
> -	    (hdmi->vic == 17) || (hdmi->vic == 18))
> +	if (hdmi->plat_data->get_enc_out_encoding)
> +		hdmi->hdmi_data.enc_out_encoding =
> +			hdmi->plat_data->get_enc_out_encoding(data);
> +	else if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> +		 (hdmi->vic == 21) || (hdmi->vic == 22) ||
> +		 (hdmi->vic == 2) || (hdmi->vic == 3) ||
> +		 (hdmi->vic == 17) || (hdmi->vic == 18))
>  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_601;
>  	else
>  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_709;
> @@ -1833,21 +1837,31 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  	hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;
>  
>  	/* TOFIX: Get input format from plat data or fallback to RGB888 */
> -	if (hdmi->plat_data->input_bus_format)
> +	if (hdmi->plat_data->get_input_bus_format)
> +		hdmi->hdmi_data.enc_in_bus_format =
> +			hdmi->plat_data->get_input_bus_format(data);
> +	else if (hdmi->plat_data->input_bus_format)
>  		hdmi->hdmi_data.enc_in_bus_format =
>  			hdmi->plat_data->input_bus_format;
>  	else
>  		hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  
>  	/* TOFIX: Get input encoding from plat data or fallback to none */
> -	if (hdmi->plat_data->input_bus_encoding)
> +	if (hdmi->plat_data->get_enc_in_encoding)
> +		hdmi->hdmi_data.enc_in_encoding =
> +			hdmi->plat_data->get_enc_in_encoding(data);
> +	else if (hdmi->plat_data->input_bus_encoding)
>  		hdmi->hdmi_data.enc_in_encoding =
>  			hdmi->plat_data->input_bus_encoding;
>  	else
>  		hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT;
>  
>  	/* TOFIX: Default to RGB888 output format */
> -	hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +	if (hdmi->plat_data->get_output_bus_format)
> +		hdmi->hdmi_data.enc_out_bus_format =
> +			hdmi->plat_data->get_output_bus_format(data);
> +	else
> +		hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  
>  	hdmi->hdmi_data.pix_repet_factor = 0;
>  	hdmi->hdmi_data.hdcp_enable = 0;
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 7a02744ce0bc..2e797f782c51 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -142,6 +142,11 @@ struct dw_hdmi_plat_data {
>  	int (*configure_phy)(struct dw_hdmi *hdmi,
>  			     const struct dw_hdmi_plat_data *pdata,
>  			     unsigned long mpixelclock);
> +
> +	unsigned long (*get_input_bus_format)(void *data);
> +	unsigned long (*get_output_bus_format)(void *data);
> +	unsigned long (*get_enc_in_encoding)(void *data);
> +	unsigned long (*get_enc_out_encoding)(void *data);
>  };
>  
>  struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 5/8] drm/bridge: dw-hdmi: support dynamically get input/out color info

[Laurent Pinchart]

Hello,

On Wednesday, 19 December 2018 09:26:08 EET Andrzej Hajda wrote:
> On 30.11.2018 14:42, Neil Armstrong wrote:
> > From: Zheng Yang <zhengyang@rock-chips.com>
> > 
> > To get input/output bus_format/enc_format dynamically, this patch
> > 
> > introduce following funstion in plat_data:
> > 	- get_input_bus_format
> > 	- get_output_bus_format
> > 	- get_enc_in_encoding
> > 	- get_enc_out_encoding
> 
> It seems fishy. On one side description says about dynamic resolution of
> formats and encodings.
> 
> On the other side these functions as only argument takes platform_data
> which should be rather static.
> 
> Where is this "dynamic" thing? The only usage of these callbacks I have
> found in next patches is also not dynamic, the functions just return
> some static value.
> 
> Moreover function takes void* argument, which is again something
> suspicious, why cannot you pass know structure?
> 
> And finally encoding usually should depend on display mode, it should
> not depend only static data.
> 
> 
> What kind of problems do you want to solve here?

I would add that this doesn't seem to be specific to dw-hdmi in any way. I'd 
prefer an API at the drm_bridge level to handle this.

> > Signed-off-by: Zheng Yang <zhengyang@rock-chips.com>
> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> > ---
> > 
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 28 +++++++++++++++++------
> >  include/drm/bridge/dw_hdmi.h              |  5 ++++
> >  2 files changed, 26 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> > 4a9a24e854db..bd564ffdf18b 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -1810,6 +1810,7 @@ static void hdmi_disable_overflow_interrupts(struct
> > dw_hdmi *hdmi)> 
> >  static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode
> >  *mode) {
> >  
> >  	int ret;
> > 
> > +	void *data = hdmi->plat_data->phy_data;
> > 
> >  	hdmi_disable_overflow_interrupts(hdmi);
> > 
> > @@ -1821,10 +1822,13 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
> > struct drm_display_mode *mode)> 
> >  		dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
> >  	
> >  	}
> > 
> > -	if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> > -	    (hdmi->vic == 21) || (hdmi->vic == 22) ||
> > -	    (hdmi->vic == 2) || (hdmi->vic == 3) ||
> > -	    (hdmi->vic == 17) || (hdmi->vic == 18))
> > +	if (hdmi->plat_data->get_enc_out_encoding)
> > +		hdmi->hdmi_data.enc_out_encoding =
> > +			hdmi->plat_data->get_enc_out_encoding(data);
> > +	else if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> > +		 (hdmi->vic == 21) || (hdmi->vic == 22) ||
> > +		 (hdmi->vic == 2) || (hdmi->vic == 3) ||
> > +		 (hdmi->vic == 17) || (hdmi->vic == 18))
> > 
> >  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_601;
> >  	
> >  	else
> >  	
> >  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_709;
> > 
> > @@ -1833,21 +1837,31 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
> > struct drm_display_mode *mode)> 
> >  	hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;
> >  	
> >  	/* TOFIX: Get input format from plat data or fallback to RGB888 */
> > 
> > -	if (hdmi->plat_data->input_bus_format)
> > +	if (hdmi->plat_data->get_input_bus_format)
> > +		hdmi->hdmi_data.enc_in_bus_format =
> > +			hdmi->plat_data->get_input_bus_format(data);
> > +	else if (hdmi->plat_data->input_bus_format)
> > 
> >  		hdmi->hdmi_data.enc_in_bus_format =
> >  		
> >  			hdmi->plat_data->input_bus_format;
> >  	
> >  	else
> >  	
> >  		hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> >  	
> >  	/* TOFIX: Get input encoding from plat data or fallback to none */
> > 
> > -	if (hdmi->plat_data->input_bus_encoding)
> > +	if (hdmi->plat_data->get_enc_in_encoding)
> > +		hdmi->hdmi_data.enc_in_encoding =
> > +			hdmi->plat_data->get_enc_in_encoding(data);
> > +	else if (hdmi->plat_data->input_bus_encoding)
> > 
> >  		hdmi->hdmi_data.enc_in_encoding =
> >  		
> >  			hdmi->plat_data->input_bus_encoding;
> >  	
> >  	else
> >  	
> >  		hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT;
> >  	
> >  	/* TOFIX: Default to RGB888 output format */
> > 
> > -	hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > +	if (hdmi->plat_data->get_output_bus_format)
> > +		hdmi->hdmi_data.enc_out_bus_format =
> > +			hdmi->plat_data->get_output_bus_format(data);
> > +	else
> > +		hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > 
> >  	hdmi->hdmi_data.pix_repet_factor = 0;
> >  	hdmi->hdmi_data.hdcp_enable = 0;
> > 
> > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> > index 7a02744ce0bc..2e797f782c51 100644
> > --- a/include/drm/bridge/dw_hdmi.h
> > +++ b/include/drm/bridge/dw_hdmi.h
> > @@ -142,6 +142,11 @@ struct dw_hdmi_plat_data {
> > 
> >  	int (*configure_phy)(struct dw_hdmi *hdmi,
> >  	
> >  			     const struct dw_hdmi_plat_data *pdata,
> >  			     unsigned long mpixelclock);
> > 
> > +
> > +	unsigned long (*get_input_bus_format)(void *data);
> > +	unsigned long (*get_output_bus_format)(void *data);
> > +	unsigned long (*get_enc_in_encoding)(void *data);
> > +	unsigned long (*get_enc_out_encoding)(void *data);
> > 
> >  };
> >  
> >  struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,


-- 
Regards,

Laurent Pinchart




_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC v2 5/8] drm/bridge: dw-hdmi: support dynamically get input/out color info

[Neil Armstrong]

Hi Andrzej, Laurent,

Thanks for your review.

On 19/12/2018 08:50, Laurent Pinchart wrote:
> Hello,
> 
> On Wednesday, 19 December 2018 09:26:08 EET Andrzej Hajda wrote:
>> On 30.11.2018 14:42, Neil Armstrong wrote:
>>> From: Zheng Yang <zhengyang@rock-chips.com>
>>>
>>> To get input/output bus_format/enc_format dynamically, this patch
>>>
>>> introduce following funstion in plat_data:
>>> 	- get_input_bus_format
>>> 	- get_output_bus_format
>>> 	- get_enc_in_encoding
>>> 	- get_enc_out_encoding
>>
>> It seems fishy. On one side description says about dynamic resolution of
>> formats and encodings.
>>
>> On the other side these functions as only argument takes platform_data
>> which should be rather static.

They are callbacks to the "glue" code, similar the PHY and HPD callbacks,
they will return different encodings and formats depending on the current mode
being atomically set.

>>
>> Where is this "dynamic" thing? The only usage of these callbacks I have
>> found in next patches is also not dynamic, the functions just return
>> some static value.

in patch 7 & 8 we return the current glue dw_hdmi->input_bus_format
and dw_hdmi->output_bus_format set during the encoder atomic_check()

>>
>> Moreover function takes void* argument, which is again something
>> suspicious, why cannot you pass know structure?

Yes, we should also pass dw_hdmi along the dw_plat_data->phy_data we
already pass.

>>
>> And finally encoding usually should depend on display mode, it should
>> not depend only static data.

It does not, there are fallbacks already in place, where you can override
with static data (the bus encoding and format can be fixed) or dynamic
to solve the yuv420 format.
Amlogic pipeline can *only* output in YUV (444, 422 or 420) so I pushed
support for statically describing the input format and encoding using
V4L2 definitions.

>>
>>
>> What kind of problems do you want to solve here?

We try to solve 2 things :
- The YUV420 HDMI2.0 mode, but the DW-HDMI CSC cannot convert to/from YUV420, so
it's in passthrought only. So the encoder must output in yuv420 and the controller
must know the input format and the output format, and this dynamically.
- Today the DW-HDMI forces RGB 8bit output, but we may prefer YUV444 or YU422 depending
on the sink and eventually output in 10, 12 or 16bit mode. This logic should
not be in the controller bridge code.

To solve these uses case, we put the logic in the encoder to determine what is
the DW-HDMI input format+encoding and the needed output format+encoding.

Today, the encoding callbacks are not used in this patchset, but they follow the
same scheme.

> 
> I would add that this doesn't seem to be specific to dw-hdmi in any way. I'd 
> prefer an API at the drm_bridge level to handle this.

Can you point me what you have in mind ? I'll be happy to implement it.

These callbacks are only an extension of the hdmi->plat_data->input_bus_format
and hdmi->plat_data->input_bus_encoding I introduced a few times ago.

I'd really like to solve this correctly, but still solve it at some point !
The YUV420 support is handy to easily support 4k60 for cheap and older TVs
without the hassle of SCDC and TMDS scrambling.

Neil

> 
>>> Signed-off-by: Zheng Yang <zhengyang@rock-chips.com>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>
>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 28 +++++++++++++++++------
>>>  include/drm/bridge/dw_hdmi.h              |  5 ++++
>>>  2 files changed, 26 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
>>> 4a9a24e854db..bd564ffdf18b 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -1810,6 +1810,7 @@ static void hdmi_disable_overflow_interrupts(struct
>>> dw_hdmi *hdmi)> 
>>>  static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode
>>>  *mode) {
>>>  
>>>  	int ret;
>>>
>>> +	void *data = hdmi->plat_data->phy_data;
>>>
>>>  	hdmi_disable_overflow_interrupts(hdmi);
>>>
>>> @@ -1821,10 +1822,13 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
>>> struct drm_display_mode *mode)> 
>>>  		dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
>>>  	
>>>  	}
>>>
>>> -	if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
>>> -	    (hdmi->vic == 21) || (hdmi->vic == 22) ||
>>> -	    (hdmi->vic == 2) || (hdmi->vic == 3) ||
>>> -	    (hdmi->vic == 17) || (hdmi->vic == 18))
>>> +	if (hdmi->plat_data->get_enc_out_encoding)
>>> +		hdmi->hdmi_data.enc_out_encoding =
>>> +			hdmi->plat_data->get_enc_out_encoding(data);
>>> +	else if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
>>> +		 (hdmi->vic == 21) || (hdmi->vic == 22) ||
>>> +		 (hdmi->vic == 2) || (hdmi->vic == 3) ||
>>> +		 (hdmi->vic == 17) || (hdmi->vic == 18))
>>>
>>>  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_601;
>>>  	
>>>  	else
>>>  	
>>>  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_709;
>>>
>>> @@ -1833,21 +1837,31 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
>>> struct drm_display_mode *mode)> 
>>>  	hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;
>>>  	
>>>  	/* TOFIX: Get input format from plat data or fallback to RGB888 */
>>>
>>> -	if (hdmi->plat_data->input_bus_format)
>>> +	if (hdmi->plat_data->get_input_bus_format)
>>> +		hdmi->hdmi_data.enc_in_bus_format =
>>> +			hdmi->plat_data->get_input_bus_format(data);
>>> +	else if (hdmi->plat_data->input_bus_format)
>>>
>>>  		hdmi->hdmi_data.enc_in_bus_format =
>>>  		
>>>  			hdmi->plat_data->input_bus_format;
>>>  	
>>>  	else
>>>  	
>>>  		hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>  	
>>>  	/* TOFIX: Get input encoding from plat data or fallback to none */
>>>
>>> -	if (hdmi->plat_data->input_bus_encoding)
>>> +	if (hdmi->plat_data->get_enc_in_encoding)
>>> +		hdmi->hdmi_data.enc_in_encoding =
>>> +			hdmi->plat_data->get_enc_in_encoding(data);
>>> +	else if (hdmi->plat_data->input_bus_encoding)
>>>
>>>  		hdmi->hdmi_data.enc_in_encoding =
>>>  		
>>>  			hdmi->plat_data->input_bus_encoding;
>>>  	
>>>  	else
>>>  	
>>>  		hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT;
>>>  	
>>>  	/* TOFIX: Default to RGB888 output format */
>>>
>>> -	hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>> +	if (hdmi->plat_data->get_output_bus_format)
>>> +		hdmi->hdmi_data.enc_out_bus_format =
>>> +			hdmi->plat_data->get_output_bus_format(data);
>>> +	else
>>> +		hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>
>>>  	hdmi->hdmi_data.pix_repet_factor = 0;
>>>  	hdmi->hdmi_data.hdcp_enable = 0;
>>>
>>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>>> index 7a02744ce0bc..2e797f782c51 100644
>>> --- a/include/drm/bridge/dw_hdmi.h
>>> +++ b/include/drm/bridge/dw_hdmi.h
>>> @@ -142,6 +142,11 @@ struct dw_hdmi_plat_data {
>>>
>>>  	int (*configure_phy)(struct dw_hdmi *hdmi,
>>>  	
>>>  			     const struct dw_hdmi_plat_data *pdata,
>>>  			     unsigned long mpixelclock);
>>>
>>> +
>>> +	unsigned long (*get_input_bus_format)(void *data);
>>> +	unsigned long (*get_output_bus_format)(void *data);
>>> +	unsigned long (*get_enc_in_encoding)(void *data);
>>> +	unsigned long (*get_enc_out_encoding)(void *data);
>>>
>>>  };
>>>  
>>>  struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
> 
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic