Re: [Intel-gfx] [PATCH 4/4] drm/i915: Enabling WD Transcoder

["Kandpal, Suraj" <suraj.kandpal@intel.com>]

> > Adding support for writeback transcoder to start capturing frames using
> > interrupt mechanism
> 
> I stopped reviewing this after a while, because there's just way too
> much unrelated noise in the patch to even be able to focus on what's
> actually being done functionally here. There are some comments inline
> before I stopped.
> 
> Please don't do random superfluous whitespace or checkpatch changes in
> the same patch. It's just a distraction.
> 
> Functionally the most questionable thing I spotted is adding the
> intel_crtc and intel_wd pointer members in struct
> drm_i915_private. That's not the kind of design we'll want. It should
> all be in the atomic state.

Will remove the checkpatch changes from this and reduce the noise
> 
> Also, what is this in intel_wd.c:
> 
> > +static const struct drm_display_mode drm_dmt_modes[] = {
> 
> Please no.
> 
This will be removed in the next series of patches
> 
> BR,
> Jani.
> 
> 
> 
> > diff --git a/drivers/gpu/drm/i915/Makefile
> b/drivers/gpu/drm/i915/Makefile
> > index 5c8e022a7383..5472bb48196b 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -206,6 +206,7 @@ i915-y += \
> >  	display/intel_connector.o \
> >  	display/intel_crtc.o \
> >  	display/intel_cursor.o \
> > +	display/intel_wd.o \
> 
> Adding it twice?
Typo will remove it
> 
> >  	display/intel_display.o \
> >  	display/intel_display_power.o \
> >  	display/intel_dmc.o \
> > @@ -276,6 +277,7 @@ i915-y += \
> >  	display/intel_tv.o \
> >  	display/intel_vdsc.o \
> >  	display/intel_vrr.o \
> > +	display/intel_wd.o \
> >  	display/vlv_dsi.o \
> >  	display/vlv_dsi_pll.o
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c
> b/drivers/gpu/drm/i915/display/intel_acpi.c
> > index 72cac55c0f0f..98537c7de20c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> > @@ -244,6 +244,7 @@ static u32 acpi_display_type(struct intel_connector
> *connector)
> >  	case DRM_MODE_CONNECTOR_LVDS:
> >  	case DRM_MODE_CONNECTOR_eDP:
> >  	case DRM_MODE_CONNECTOR_DSI:
> > +	case DRM_MODE_CONNECTOR_WRITEBACK:
> >  		display_type = ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL;
> >  		break;
> >  	case DRM_MODE_CONNECTOR_Unknown:
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> > index f27c294beb92..4a9e5972f381 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -105,6 +105,7 @@
> >  #include "intel_tc.h"
> >  #include "intel_vga.h"
> >  #include "i9xx_plane.h"
> > +#include "intel_wd.h"
> 
> Please keep include lists sorted.
Noted will change the order
> 
> >  #include "skl_scaler.h"
> >  #include "skl_universal_plane.h"
> >
> > @@ -195,10 +196,13 @@ skl_wa_827(struct drm_i915_private *dev_priv,
> enum pipe pipe, bool enable)
> >  {
> >  	if (enable)
> >  		intel_de_write(dev_priv, CLKGATE_DIS_PSL(pipe),
> > -		               intel_de_read(dev_priv, CLKGATE_DIS_PSL(pipe)) |
> DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> > +			intel_de_read(dev_priv, CLKGATE_DIS_PSL(pipe)) |
> > +			DUPS1_GATING_DIS |
> > +			DUPS2_GATING_DIS);
> >  	else
> >  		intel_de_write(dev_priv, CLKGATE_DIS_PSL(pipe),
> > -		               intel_de_read(dev_priv, CLKGATE_DIS_PSL(pipe)) &
> ~(DUPS1_GATING_DIS | DUPS2_GATING_DIS));
> > +			intel_de_read(dev_priv, CLKGATE_DIS_PSL(pipe)) &
> > +			~(DUPS1_GATING_DIS | DUPS2_GATING_DIS));
> >  }
> 
> Superfluous changes that should not be part of this patch.
> 
> >
> >  /* Wa_2006604312:icl,ehl */
> > @@ -208,10 +212,10 @@ icl_wa_scalerclkgating(struct drm_i915_private
> *dev_priv, enum pipe pipe,
> >  {
> >  	if (enable)
> >  		intel_de_write(dev_priv, CLKGATE_DIS_PSL(pipe),
> > -		               intel_de_read(dev_priv, CLKGATE_DIS_PSL(pipe)) |
> DPFR_GATING_DIS);
> > +			       intel_de_read(dev_priv, CLKGATE_DIS_PSL(pipe))
> | DPFR_GATING_DIS);
> >  	else
> >  		intel_de_write(dev_priv, CLKGATE_DIS_PSL(pipe),
> > -		               intel_de_read(dev_priv, CLKGATE_DIS_PSL(pipe)) &
> ~DPFR_GATING_DIS);
> > +			       intel_de_read(dev_priv, CLKGATE_DIS_PSL(pipe))
> & ~DPFR_GATING_DIS);
> >  }
> >
> >  static bool
> > @@ -342,6 +346,7 @@ static void assert_fdi_tx(struct drm_i915_private
> *dev_priv,
> >  		cur_state = !!(val & TRANS_DDI_FUNC_ENABLE);
> >  	} else {
> >  		u32 val = intel_de_read(dev_priv, FDI_TX_CTL(pipe));
> > +
> >  		cur_state = !!(val & FDI_TX_ENABLE);
> >  	}
> >  	I915_STATE_WARN(cur_state != state,
> > @@ -469,6 +474,7 @@ void assert_transcoder(struct drm_i915_private
> *dev_priv,
> >  	wakeref = intel_display_power_get_if_enabled(dev_priv,
> power_domain);
> >  	if (wakeref) {
> >  		u32 val = intel_de_read(dev_priv,
> PIPECONF(cpu_transcoder));
> > +
> >  		cur_state = !!(val & PIPECONF_ENABLE);
> >
> >  		intel_display_power_put(dev_priv, power_domain,
> wakeref);
> > @@ -1850,6 +1856,7 @@ bool intel_has_pending_fb_unpin(struct
> drm_i915_private *dev_priv)
> >
> >  	drm_for_each_crtc(crtc, &dev_priv->drm) {
> >  		struct drm_crtc_commit *commit;
> > +
> >  		spin_lock(&crtc->commit_lock);
> >  		commit = list_first_entry_or_null(&crtc->commit_list,
> >  						  struct drm_crtc_commit,
> commit_entry);
> > @@ -1895,7 +1902,7 @@ static void lpt_program_iclkip(const struct
> intel_crtc_state *crtc_state)
> >  	lpt_disable_iclkip(dev_priv);
> >
> >  	/* The iCLK virtual clock root frequency is in MHz,
> > -	 * but the adjusted_mode->crtc_clock in in KHz. To get the
> > +	 * but the adjusted_mode->crtc_clock in KHz. To get the
> >  	 * divisors, it is necessary to divide one by another, so we
> >  	 * convert the virtual clock precision to KHz here for higher
> >  	 * precision.
> > @@ -2571,7 +2578,7 @@ static void intel_crtc_disable_planes(struct
> intel_atomic_state *state,
> >  	unsigned int update_mask = new_crtc_state->update_planes;
> >  	const struct intel_plane_state *old_plane_state;
> >  	struct intel_plane *plane;
> > -	unsigned fb_bits = 0;
> > +	unsigned int fb_bits = 0;
> >  	int i;
> >
> 
> Ditto for all of the above hunks.
> 
> >  	intel_crtc_dpms_overlay_disable(crtc);
> > @@ -2665,6 +2672,66 @@ static void
> intel_encoders_update_complete(struct intel_atomic_state *state)
> >  	}
> >  }
> >
> > +static void intel_queue_writeback_job(struct intel_atomic_state *state,
> > +		struct drm_i915_private *dev_priv, struct intel_crtc
> *intel_crtc,
> > +		struct intel_crtc_state *crtc_state)
> > +{
> 
> No need to pass dev_priv around, you can get at it via the other
> pointers. Also, for new code the variable should be named i915.
> 
Will change the variable names and will derive dev_priv rather than passing it around
> > +	struct drm_connector_state *new_conn_state;
> > +	struct drm_connector *connector;
> > +	struct drm_device *dev = &dev_priv->drm;
> 
> Usually we only have i915 local var, no struct drm_device. Look around.
> 
> > +	int i;
> > +
> > +	drm_dbg(dev, "[%s]:%d ", __func__, __LINE__);
> 
> What? Why?
> 
> All kms debugs should use drm_dbg_kms().
> 
> Please don't add any of this __func__ and __LINE__ nonsense. The
> function gets added by drm_dbg_* anyway.
> 
> Same applies all over the place.
Got it will update drm_dbg to drm_dbg_kms() 
> 
> > +	if (dev_priv->wd_crtc != intel_crtc) {
> 
> Oh noes, we won't be adding random crtc pointers in drm_i915_private!
> 
> > +		drm_dbg(dev, "not the wd crtc");
> > +		return;
> > +	}
> > +
> > +	for_each_new_connector_in_state(&state->base, connector,
> new_conn_state,
> > +					i) {
> > +		if (!new_conn_state->writeback_job)
> > +			continue;
> > +
> > +		drm_writeback_queue_job(connector->wb_connector,
> new_conn_state);
> > +		drm_dbg(dev, "[%s]:%d queueing writeback job", __func__,
> __LINE__);
> > +	}
> > +}
> > +
> > +static void intel_find_writeback_connector(struct intel_atomic_state
> *state,
> > +		struct drm_i915_private *dev_priv,
> > +		struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_connector_state *new_conn_state;
> > +	struct drm_connector *connector;
> > +	struct drm_device *dev = &dev_priv->drm;
> > +	int i;
> > +
> > +	drm_dbg(dev, "[%s]:%d ", __func__, __LINE__);
> > +	if (dev_priv->wd_crtc != intel_crtc) {
> > +		drm_dbg(dev, "not the wd crtc");
> > +		return;
> > +	}
> > +
> > +	for_each_new_connector_in_state(&state->base, connector,
> new_conn_state,
> > +					i) {
> > +		struct intel_connector *intel_connector;
> > +		struct intel_encoder *encoder;
> > +
> > +		drm_dbg(dev, "[%s]:%d ", __func__, __LINE__);
> > +
> > +		intel_connector = to_intel_connector(connector);
> > +		drm_dbg(dev, "[CONNECTOR:%d:%s]: status: %s\n",
> > +				connector->base.id, connector->name,
> > +				drm_get_connector_status_name(connector-
> >status));
> > +		encoder =
> intel_connector_primary_encoder(intel_connector);
> > +		if (encoder->type == INTEL_OUTPUT_WD) {
> > +			drm_dbg(dev, "encoder intel_output_wd found");
> > +			intel_wd_enable_capture(dev_priv, encoder,
> crtc_state, new_conn_state);
> > +		}
> > +	}
> > +
> > +}
> > +
> >  static void intel_encoders_pre_pll_enable(struct intel_atomic_state *state,
> >  					  struct intel_crtc *crtc)
> >  {
> > @@ -2728,6 +2795,7 @@ static void intel_encoders_enable(struct
> intel_atomic_state *state,
> >  		if (encoder->enable)
> >  			encoder->enable(state, encoder,
> >  					crtc_state, conn_state);
> > +
> 
> No superfluous whitespace changes please. Same all over the place. It's
> just noise in the patch.
> 
> >  		intel_opregion_notify_encoder(encoder, true);
> >  	}
> >  }
> > @@ -2751,6 +2819,7 @@ static void intel_encoders_pre_disable(struct
> intel_atomic_state *state,
> >  		if (encoder->pre_disable)
> >  			encoder->pre_disable(state, encoder, old_crtc_state,
> >  					     old_conn_state);
> > +
> >  	}
> >  }
> >
> > @@ -2774,6 +2843,7 @@ static void intel_encoders_disable(struct
> intel_atomic_state *state,
> >  		if (encoder->disable)
> >  			encoder->disable(state, encoder,
> >  					 old_crtc_state, old_conn_state);
> > +
> >  	}
> >  }
> >
> > @@ -3078,7 +3148,8 @@ static void hsw_crtc_enable(struct
> intel_atomic_state *state,
> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> >  		bdw_set_pipemisc(new_crtc_state);
> >
> > -	if (!new_crtc_state->bigjoiner_slave &&
> !transcoder_is_dsi(cpu_transcoder)) {
> > +	if (!new_crtc_state->bigjoiner_slave &&
> !transcoder_is_dsi(cpu_transcoder)
> > +		&& !transcoder_is_wd(cpu_transcoder)) {
> >  		intel_set_transcoder_timings(new_crtc_state);
> >
> >  		if (cpu_transcoder != TRANSCODER_EDP)
> > @@ -4307,7 +4378,7 @@ static void intel_set_transcoder_timings(const
> struct intel_crtc_state *crtc_sta
> >
> >  	if (DISPLAY_VER(dev_priv) > 3)
> >  		intel_de_write(dev_priv, VSYNCSHIFT(cpu_transcoder),
> > -		               vsyncshift);
> > +			       vsyncshift);
> >
> >  	intel_de_write(dev_priv, HTOTAL(cpu_transcoder),
> >  		       (adjusted_mode->crtc_hdisplay - 1) | ((adjusted_mode-
> >crtc_htotal - 1) << 16));
> > @@ -4330,7 +4401,7 @@ static void intel_set_transcoder_timings(const
> struct intel_crtc_state *crtc_sta
> >  	if (IS_HASWELL(dev_priv) && cpu_transcoder == TRANSCODER_EDP
> &&
> >  	    (pipe == PIPE_B || pipe == PIPE_C))
> >  		intel_de_write(dev_priv, VTOTAL(pipe),
> > -		               intel_de_read(dev_priv, VTOTAL(cpu_transcoder)));
> > +			       intel_de_read(dev_priv,
> VTOTAL(cpu_transcoder)));
> >
> >  }
> >
> > @@ -4980,18 +5051,18 @@ void lpt_disable_clkout_dp(struct
> drm_i915_private *dev_priv)
> >  #define BEND_IDX(steps) ((50 + (steps)) / 5)
> >
> >  static const u16 sscdivintphase[] = {
> > -	[BEND_IDX( 50)] = 0x3B23,
> > -	[BEND_IDX( 45)] = 0x3B23,
> > -	[BEND_IDX( 40)] = 0x3C23,
> > -	[BEND_IDX( 35)] = 0x3C23,
> > -	[BEND_IDX( 30)] = 0x3D23,
> > -	[BEND_IDX( 25)] = 0x3D23,
> > -	[BEND_IDX( 20)] = 0x3E23,
> > -	[BEND_IDX( 15)] = 0x3E23,
> > -	[BEND_IDX( 10)] = 0x3F23,
> > -	[BEND_IDX(  5)] = 0x3F23,
> > -	[BEND_IDX(  0)] = 0x0025,
> > -	[BEND_IDX( -5)] = 0x0025,
> > +	[BEND_IDX(50)] = 0x3B23,
> > +	[BEND_IDX(45)] = 0x3B23,
> > +	[BEND_IDX(40)] = 0x3C23,
> > +	[BEND_IDX(35)] = 0x3C23,
> > +	[BEND_IDX(30)] = 0x3D23,
> > +	[BEND_IDX(25)] = 0x3D23,
> > +	[BEND_IDX(20)] = 0x3E23,
> > +	[BEND_IDX(15)] = 0x3E23,
> > +	[BEND_IDX(10)] = 0x3F23,
> > +	[BEND_IDX(5)] = 0x3F23,
> > +	[BEND_IDX(0)] = 0x0025,
> > +	[BEND_IDX(-5)] = 0x0025,
> >  	[BEND_IDX(-10)] = 0x0125,
> >  	[BEND_IDX(-15)] = 0x0125,
> >  	[BEND_IDX(-20)] = 0x0225,
> > @@ -5335,6 +5406,7 @@ int ilk_get_lanes_required(int target_clock, int
> link_bw, int bpp)
> >  	 * is 2.5%; use 5% for safety's sake.
> >  	 */
> >  	u32 bps = target_clock * bpp * 21 / 20;
> > +
> >  	return DIV_ROUND_UP(bps, link_bw * 8);
> >  }
> >
> > @@ -5572,7 +5644,7 @@ static bool ilk_get_pipe_config(struct intel_crtc
> *crtc,
> >  			if (tmp & TRANS_DPLLB_SEL(crtc->pipe))
> >  				pll_id = DPLL_ID_PCH_PLL_B;
> >  			else
> > -				pll_id= DPLL_ID_PCH_PLL_A;
> > +				pll_id = DPLL_ID_PCH_PLL_A;
> 
> Useful change, but does not belong in this patch.
> 
> Stopping review here.

Will work on the comments and address them in next set of patches

Regards,
Suraj Kandpal