Re: [PATCH] drm/i915: Convert ICL-style DSI to use phy namespace

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 04, 2019 at 06:20:37PM -0700, Matt Roper wrote:
> ICL+ DSI outputs operate a bit differently than DP/HDMI/eDP and are more
> closely tied to the PHY than to the DDI.  Since we've separated PHY's
> out into their own namespace it makes sense to operate on 'enum phy'
> throughout most of the ICL DSI code rather than 'enum port' which we
> generally use to refer to the DDI these days.  Part of this transition
> has already been done as part of commit dc867bc7d887 ("drm/i915/gen11:
> Convert combo PHY logic to use new 'enum phy' namespace"), although that
> patch only migrated the parts of the code that were specifically
> updating the combo PHY registers themselves.  Moving the rest of the
> code over to the PHY namespace as well will hopefully make things more
> consistent and less confusing.
> 
> Note that the change here is really just a naming change.  On all of the
> platforms that this code currently applies to, the DSI outputs always
> have port==phy so we're still passing the same integer values around no
> matter which type of enum they belong to..
> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/display/icl_dsi.c   | 236 ++++++++++++-----------
>  drivers/gpu/drm/i915/display/intel_dsi.h |   5 +-
>  2 files changed, 126 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> index 6e398c33a524..5cf32032e795 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -65,9 +65,9 @@ static void wait_for_payload_credits(struct drm_i915_private *dev_priv,
>  		DRM_ERROR("DSI payload credits not released\n");
>  }
>  
> -static enum transcoder dsi_port_to_transcoder(enum port port)
> +static enum transcoder dsi_phy_to_transcoder(enum phy phy)
>  {
> -	if (port == PORT_A)
> +	if (phy == PHY_A)
>  		return TRANSCODER_DSI_0;
>  	else
>  		return TRANSCODER_DSI_1;
> @@ -78,20 +78,20 @@ static void wait_for_cmds_dispatched_to_panel(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	struct mipi_dsi_device *dsi;
> -	enum port port;
> +	enum phy phy;
>  	enum transcoder dsi_trans;
>  	int ret;
>  
>  	/* wait for header/payload credits to be released */
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		dsi_trans = dsi_port_to_transcoder(port);
> +	for_each_dsi_phy(phy, intel_dsi->phys) {
> +		dsi_trans = dsi_phy_to_transcoder(phy);
>  		wait_for_header_credits(dev_priv, dsi_trans);
>  		wait_for_payload_credits(dev_priv, dsi_trans);
>  	}
>  
>  	/* send nop DCS command */
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		dsi = intel_dsi->dsi_hosts[port]->device;
> +	for_each_dsi_phy(phy, intel_dsi->phys) {
> +		dsi = intel_dsi->dsi_hosts[phy]->device;
>  		dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>  		dsi->channel = 0;
>  		ret = mipi_dsi_dcs_nop(dsi);
> @@ -100,14 +100,14 @@ static void wait_for_cmds_dispatched_to_panel(struct intel_encoder *encoder)
>  	}
>  
>  	/* wait for header credits to be released */
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		dsi_trans = dsi_port_to_transcoder(port);
> +	for_each_dsi_phy(phy, intel_dsi->phys) {
> +		dsi_trans = dsi_phy_to_transcoder(phy);
>  		wait_for_header_credits(dev_priv, dsi_trans);
>  	}
>  
>  	/* wait for LP TX in progress bit to be cleared */
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		dsi_trans = dsi_port_to_transcoder(port);
> +	for_each_dsi_phy(phy, intel_dsi->phys) {
> +		dsi_trans = dsi_phy_to_transcoder(phy);
>  		if (wait_for_us(!(I915_READ(DSI_LP_MSG(dsi_trans)) &
>  				  LPTX_IN_PROGRESS), 20))
>  			DRM_ERROR("LPTX bit not cleared\n");
> @@ -119,7 +119,7 @@ static bool add_payld_to_queue(struct intel_dsi_host *host, const u8 *data,
>  {
>  	struct intel_dsi *intel_dsi = host->intel_dsi;
>  	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
> -	enum transcoder dsi_trans = dsi_port_to_transcoder(host->port);
> +	enum transcoder dsi_trans = dsi_phy_to_transcoder(host->phy);
>  	int free_credits;
>  	int i, j;
>  
> @@ -146,7 +146,7 @@ static int dsi_send_pkt_hdr(struct intel_dsi_host *host,
>  {
>  	struct intel_dsi *intel_dsi = host->intel_dsi;
>  	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
> -	enum transcoder dsi_trans = dsi_port_to_transcoder(host->port);
> +	enum transcoder dsi_trans = dsi_phy_to_transcoder(host->phy);
>  	u32 tmp;
>  	int free_credits;
>  
> @@ -305,7 +305,7 @@ static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> -	enum port port;
> +	enum phy phy;
>  	u32 bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format);
>  	u32 afe_clk_khz; /* 8X Clock */
>  	u32 esc_clk_div_m;
> @@ -315,29 +315,29 @@ static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder)
>  
>  	esc_clk_div_m = DIV_ROUND_UP(afe_clk_khz, DSI_MAX_ESC_CLK);
>  
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		I915_WRITE(ICL_DSI_ESC_CLK_DIV(port),
> +	for_each_dsi_phy(phy, intel_dsi->phys) {
> +		I915_WRITE(ICL_DSI_ESC_CLK_DIV(phy),
>  			   esc_clk_div_m & ICL_ESC_CLK_DIV_MASK);
> -		POSTING_READ(ICL_DSI_ESC_CLK_DIV(port));
> +		POSTING_READ(ICL_DSI_ESC_CLK_DIV(phy));
>  	}
>  
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		I915_WRITE(ICL_DPHY_ESC_CLK_DIV(port),
> +	for_each_dsi_phy(phy, intel_dsi->phys) {
> +		I915_WRITE(ICL_DPHY_ESC_CLK_DIV(phy),
>  			   esc_clk_div_m & ICL_ESC_CLK_DIV_MASK);
> -		POSTING_READ(ICL_DPHY_ESC_CLK_DIV(port));
> +		POSTING_READ(ICL_DPHY_ESC_CLK_DIV(phy));
>  	}
>  }
>  
>  static void get_dsi_io_power_domains(struct drm_i915_private *dev_priv,
>  				     struct intel_dsi *intel_dsi)
>  {
> -	enum port port;
> +	enum phy phy;
>  
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		WARN_ON(intel_dsi->io_wakeref[port]);
> -		intel_dsi->io_wakeref[port] =
> +	for_each_dsi_phy(phy, intel_dsi->phys) {
> +		WARN_ON(intel_dsi->io_wakeref[phy]);
> +		intel_dsi->io_wakeref[phy] =
>  			intel_display_power_get(dev_priv,
> -						port == PORT_A ?
> +						phy == PHY_A ?
>  						POWER_DOMAIN_PORT_DDI_A_IO :
>  						POWER_DOMAIN_PORT_DDI_B_IO);

I think this is now getting a bit confusing w.r.t. the normal DDI code.
With normal DDI The IO power request is based on the DDI not the PHY,
and the same goes for stuff like DDI_BUF_CTL below. IMO would be nice
if we could keep these things consistent between the two paths.

As for the DSI specific registers/etc. I'm not sure if port or phy would
be more appropriate.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx