From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 4/5] drm/i915: Add an encoder hook to sanitize its state during init/resume
Date: Tue, 6 Oct 2020 02:39:13 +0300 [thread overview]
Message-ID: <20201005233913.GQ6112@intel.com> (raw)
In-Reply-To: <20201005204617.GC1378377@ideak-desk.fi.intel.com>
On Mon, Oct 05, 2020 at 11:46:17PM +0300, Imre Deak wrote:
> On Mon, Oct 05, 2020 at 11:30:55PM +0300, Ville Syrjälä wrote:
> > On Sat, Oct 03, 2020 at 03:18:45AM +0300, Imre Deak wrote:
> > > Atm, if a full modeset is performed during the initial modeset the link
> > > training will happen with uninitialized max DP rate and lane count. Make
> > > sure the corresponding encoder state is initialized by adding an encoder
> > > hook called during driver init and system resume.
> > >
> > > A better alternative would be to store all states in the CRTC state and
> > > make this state available for the link re-training code. Also instead of
> > > the DPCD read in the hook there should be really a proper sink HW
> > > readout in place. Both of these require a bigger rework, so for now opting
> > > for this minimal fix to make at least full initial modesets work.
> > >
> > > The patch is based on
> > > https://patchwork.freedesktop.org/patch/101473/?series=10354&rev=3
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_ddi.c | 8 +++++
> > > drivers/gpu/drm/i915/display/intel_display.c | 4 +++
> > > .../drm/i915/display/intel_display_types.h | 7 +++++
> > > drivers/gpu/drm/i915/display/intel_dp.c | 31 +++++++++++++++++++
> > > drivers/gpu/drm/i915/display/intel_dp.h | 2 ++
> > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++
> > > 6 files changed, 62 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 4e54c55ec99f..a0805260b224 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -4564,6 +4564,13 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> > > intel_read_dp_sdp(encoder, pipe_config, DP_SDP_VSC);
> > > }
> > >
> > > +static void intel_ddi_sanitize_state(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state)
> > > +{
> > > + if (crtc_state && intel_crtc_has_dp_encoder(crtc_state))
> > > + intel_dp_sanitize_state(encoder, crtc_state);
> > > +}
> >
> > I think we usually use 'sanitize' to mean "hw state is garbage -> must
> > take steps to sanitize it". This one is just filling in our intel_dp
> > sidechannel state. So the name isn't super consistnet with existing
> > practies.
>
> It is called during init/resume time when encoders are sanitized as
> well, but yea it's a separate step from HW readout. So I can rename it
> for instance (back) to sync_state, or any better idea?
All I know is that I suck at naming things.
>
> >
> > > +
> > > static bool intel_ddi_initial_fastset_check(struct intel_encoder *encoder,
> > > struct intel_crtc_state *crtc_state)
> > > {
> > > @@ -5182,6 +5189,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> > > encoder->update_pipe = intel_ddi_update_pipe;
> > > encoder->get_hw_state = intel_ddi_get_hw_state;
> > > encoder->get_config = intel_ddi_get_config;
> > > + encoder->sanitize_state = intel_ddi_sanitize_state;
> > > encoder->initial_fastset_check = intel_ddi_initial_fastset_check;
> > > encoder->suspend = intel_dp_encoder_suspend;
> > > encoder->get_power_domains = intel_ddi_get_power_domains;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 31be63225b10..e61311ee8b8c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -18725,8 +18725,12 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > >
> > > encoder->base.crtc = &crtc->base;
> > > encoder->get_config(encoder, crtc_state);
> > > + if (encoder->sanitize_state)
> > > + encoder->sanitize_state(encoder, crtc_state);
> > > } else {
> > > encoder->base.crtc = NULL;
> > > + if (encoder->sanitize_state)
> > > + encoder->sanitize_state(encoder, NULL);
> >
> > I wonder if we should even bother calling it in this case.
>
> Yes, it would be just a nop atm, and can't think what state would need
> to be updated, so will remove it.
>
> >
> > > }
> > >
> > > drm_dbg_kms(&dev_priv->drm,
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 5297b2f08ff9..b2b458144f5a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -188,6 +188,13 @@ struct intel_encoder {
> > > void (*get_config)(struct intel_encoder *,
> > > struct intel_crtc_state *pipe_config);
> > >
> > > + /*
> > > + * Optional hook called during init/resume to sanitize any state
> > > + * stored in the encoder (eg. DP link parameters).
> > > + */
> > > + void (*sanitize_state)(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state);
> > > +
> > > /*
> > > * Optional hook, returning true if this encoder allows a fastset
> > > * during the initial commit, false otherwise.
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index df5277c2b9ba..9b6fe3b3b5b2 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -3703,6 +3703,36 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
> > > }
> > > }
> > >
> > > +static bool
> > > +intel_dp_get_dpcd(struct intel_dp *intel_dp);
> > > +
> > > +/**
> > > + * intel_dp_sanitize_state - sanitize the encoder state during init/resume
> > > + * @encoder: intel encoder to sanitize
> > > + * @crtc_state: state for the CRTC connected to the encoder
> > > + *
> > > + * Sanitize any state stored in the encoder during driver init and system
> > > + * resume.
> > > + */
> > > +void intel_dp_sanitize_state(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state)
> > > +{
> > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > +
> > > + if (!crtc_state)
> > > + return;
> > > +
> > > + /*
> > > + * Don't clobber DPCD if it's been already read out during output
> > > + * setup (eDP) or detect.
> > > + */
> > > + if (!memchr_inv(intel_dp->dpcd, 0, sizeof(intel_dp->dpcd)))
> > > + intel_dp_get_dpcd(intel_dp);
> > > +
> > > + intel_dp->max_link_lane_count = intel_dp_max_common_lane_count(intel_dp);
> > > + intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
> > > +}
> > > +
> > > bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
> > > struct intel_crtc_state *crtc_state)
> > > {
> > > @@ -8090,6 +8120,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
> > > intel_encoder->compute_config = intel_dp_compute_config;
> > > intel_encoder->get_hw_state = intel_dp_get_hw_state;
> > > intel_encoder->get_config = intel_dp_get_config;
> > > + intel_encoder->sanitize_state = intel_dp_sanitize_state;
> > > intel_encoder->initial_fastset_check = intel_dp_initial_fastset_check;
> > > intel_encoder->update_pipe = intel_panel_update_backlight;
> > > intel_encoder->suspend = intel_dp_encoder_suspend;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > > index 977585aea3c8..1ab741e0be67 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > > @@ -143,5 +143,7 @@ int intel_dp_init_hdcp(struct intel_digital_port *dig_port,
> > >
> > > bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
> > > struct intel_crtc_state *crtc_state);
> > > +void intel_dp_sanitize_state(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state);
> > >
> > > #endif /* __INTEL_DP_H__ */
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index e948aacbd4ab..0831d1ee7978 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -591,6 +591,15 @@ static void intel_dp_mst_enc_get_config(struct intel_encoder *encoder,
> > > intel_ddi_get_config(&dig_port->base, pipe_config);
> > > }
> > >
> > > +static void intel_dp_mst_sync_state(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state)
> > > +{
> > > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
> > > + struct intel_digital_port *dig_port = intel_mst->primary;
> > > +
> > > + return intel_dp_sanitize_state(&dig_port->base, crtc_state);
> > > +}
> > > +
> > > static bool intel_dp_mst_initial_fastset_check(struct intel_encoder *encoder,
> > > struct intel_crtc_state *crtc_state)
> > > {
> > > @@ -906,6 +915,7 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port *dig_port, enum pipe
> > > intel_encoder->enable = intel_mst_enable_dp;
> > > intel_encoder->get_hw_state = intel_dp_mst_enc_get_hw_state;
> > > intel_encoder->get_config = intel_dp_mst_enc_get_config;
> > > + intel_encoder->sanitize_state = intel_dp_mst_sync_state;
> > > intel_encoder->initial_fastset_check = intel_dp_mst_initial_fastset_check;
> > >
> > > return intel_mst;
> > > --
> > > 2.25.1
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-10-06 0:35 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-03 0:18 [Intel-gfx] [PATCH 0/5] drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock Imre Deak
2020-10-03 0:18 ` [Intel-gfx] [PATCH 1/5] drm/i915/skl: Work around incorrect BIOS WRPLL PDIV programming Imre Deak
2020-10-05 20:08 ` Ville Syrjälä
2020-10-05 20:26 ` Imre Deak
2020-10-05 23:37 ` Ville Syrjälä
2020-10-06 1:24 ` Imre Deak
2020-10-06 1:35 ` [Intel-gfx] [PATCH v3 " Imre Deak
2020-10-06 8:59 ` Ville Syrjälä
2020-10-03 0:18 ` [Intel-gfx] [PATCH 2/5] drm/i915: Move the initial fastset commit check to encoder hooks Imre Deak
2020-10-03 1:07 ` [Intel-gfx] [PATCH v2 " Imre Deak
2020-10-05 20:24 ` Ville Syrjälä
2020-10-05 20:34 ` Imre Deak
2020-10-05 21:53 ` [Intel-gfx] [PATCH v3 " Imre Deak
2020-10-06 9:42 ` Jani Nikula
2020-10-06 9:55 ` Imre Deak
2020-10-06 10:00 ` Jani Nikula
2020-10-06 10:05 ` Imre Deak
2020-10-03 0:18 ` [Intel-gfx] [PATCH 3/5] drm/i915: Check for unsupported DP link rates during initial commit Imre Deak
2020-10-05 20:25 ` Ville Syrjälä
2020-10-03 0:18 ` [Intel-gfx] [PATCH 4/5] drm/i915: Add an encoder hook to sanitize its state during init/resume Imre Deak
2020-10-05 20:30 ` Ville Syrjälä
2020-10-05 20:46 ` Imre Deak
2020-10-05 23:39 ` Ville Syrjälä [this message]
2020-10-05 20:40 ` Ville Syrjälä
2020-10-05 20:57 ` Imre Deak
2020-10-05 20:51 ` Ville Syrjälä
2020-10-05 23:00 ` Imre Deak
2020-10-05 21:53 ` [Intel-gfx] [PATCH v2 " Imre Deak
2020-10-05 23:01 ` [Intel-gfx] [PATCH v3 " Imre Deak
2020-10-06 8:58 ` Ville Syrjälä
2020-10-03 0:18 ` [Intel-gfx] [PATCH 5/5] drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock Imre Deak
2020-10-03 0:40 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock (rev2) Patchwork
2020-10-03 0:58 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-10-03 1:31 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock (rev3) Patchwork
2020-10-03 1:52 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-10-03 3:56 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2020-10-03 13:48 ` Imre Deak
2020-10-04 6:12 ` Vudum, Lakshminarayana
2020-10-04 5:47 ` [Intel-gfx] ✓ Fi.CI.IGT: success " Patchwork
2020-10-06 0:01 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock (rev6) Patchwork
2020-10-06 0:22 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-10-06 1:46 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock (rev7) Patchwork
2020-10-06 2:09 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-10-06 2:39 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock (rev6) Patchwork
2020-10-06 10:32 ` Imre Deak
2020-10-06 11:04 ` Imre Deak
2020-10-06 5:39 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock (rev7) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201005233913.GQ6112@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.