[Intel-gfx] [PATCH 4/4] drm/i915: Enabling WD Transcoder
Kandpal, Suraj
suraj.kandpal at intel.com
Thu Jan 6 09:24:50 UTC 2022
> > 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
More information about the Intel-gfx
mailing list