On Mon, Sep 14, 2020 at 09:52:57PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 14, 2020 at 11:32:48AM -0700, Navare, Manasi wrote:
> > On Mon, Sep 07, 2020 at 03:35:23PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 03, 2020 at 09:40:44PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Sep 03, 2020 at 11:04:33AM -0700, Navare, Manasi wrote:
> > > > > On Thu, Sep 03, 2020 at 08:49:44PM +0300, Ville Syrjälä wrote:
> > > > > > On Wed, Jul 15, 2020 at 03:42:13PM -0700, Manasi Navare wrote:
> > > > > > > From: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > > > > > >
> > > > > > > The members in hw.mode can be used from adjusted_mode as well,
> > > > > > > use that when available.
> > > > > > >
> > > > > > > Some places that use hw.mode can be converted to use adjusted_mode
> > > > > > > as well.
> > > > > > >
> > > > > > > v2:
> > > > > > > * Manual rebase (Manasi)
> > > > > > > * remove the use of pipe_mode defined in patch 3 (Manasi)
> > > > > > >
> > > > > > > v3:
> > > > > > > * Rebase on drm-tip (Manasi)
> > > > > >
> > > > > > Previous review was apparently ignored. Or is there a better version
> > > > > > somewhere? If not, this still looks very wrong.
> > > > >
> > > > > This was the latest rev that Maarten had in his local tree which he
> > > > > said should address all the review comments.
> > > > > What in particular looks wrong or what review comments were
> > > > > unaddressed here?
> > > >
> > > > The dvo/sdvo changes.
> > >
> > > I recommend just dropping this patch entirely. It doesn't seem to have
> > > anything to do with the bigjoiner anyway.
> >
> > So for the dvo/svdo changes, no need to use the adjusted_mode instead keep
> > using hw.mode?
> > How about other cleanups like: intel_crtc_copy_hw_to_uapi_state(crtc_state,
> > &mode); and
> > static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state
> > *crtc_state,
> > + struct drm_display_mode *user_mode)
> >
> > You think we dont need mode as an argument there either?
>
> Not in this patch if all the other stuff disappears. No idea if some
> later patch might need something like it.
Hi Ville,
So this patch basically removes the hw.mode and just keeps hw.adjusted_mode
So no need to remove that?
But basically from this patch onwards we say that there is hw.pipe_mode
and hw.adjusted_mode, there is no hw.mode.
Are you suggesting keeping hw.mode as well? Would this be replacing
hw.pipe_mode then?
Manasi
>
> >
> > Manasi
> > >
> > > >
> > > > >
> > > > > @Maarten any feedback on Ville's unaddressed comments?
> > > > >
> > > > > Manasi
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Maarten Lankhorst
> > > > > > > <maarten.lankho...@linux.intel.com>
> > > > > > > Signed-off-by: Manasi Navare <manasi.d.nav...@intel.com>
> > > > > > > ---
> > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 29
> > > > > > > ++++++++++---------
> > > > > > > .../drm/i915/display/intel_display_types.h | 2 +-
> > > > > > > drivers/gpu/drm/i915/display/intel_dvo.c | 2 +-
> > > > > > > drivers/gpu/drm/i915/display/intel_sdvo.c | 16 ++++------
> > > > > > > 4 files changed, 23 insertions(+), 26 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > index 729ec6e0d43a..8652a7c6bf11 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > @@ -8892,9 +8892,6 @@ static void intel_get_pipe_src_size(struct
> > > > > > > intel_crtc *crtc,
> > > > > > > tmp = intel_de_read(dev_priv, PIPESRC(crtc->pipe));
> > > > > > > pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> > > > > > > pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> > > > > > > -
> > > > > > > - pipe_config->hw.mode.vdisplay = pipe_config->pipe_src_h;
> > > > > > > - pipe_config->hw.mode.hdisplay = pipe_config->pipe_src_w;
> > > > > > > }
> > > > > > >
> > > > > > > void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> > > > > > > @@ -13079,7 +13076,7 @@ static void intel_dump_pipe_config(const
> > > > > > > struct intel_crtc_state *pipe_config,
> > > > > > > intel_dump_dp_vsc_sdp(dev_priv,
> > > > > > > &pipe_config->infoframes.vsc);
> > > > > > >
> > > > > > > drm_dbg_kms(&dev_priv->drm, "requested mode:\n");
> > > > > > > - drm_mode_debug_printmodeline(&pipe_config->hw.mode);
> > > > > > > + drm_mode_debug_printmodeline(&pipe_config->uapi.mode);
> > > > > > > drm_dbg_kms(&dev_priv->drm, "adjusted mode:\n");
> > > > > > > drm_mode_debug_printmodeline(&pipe_config->hw.adjusted_mode);
> > > > > > > intel_dump_crtc_timings(dev_priv,
> > > > > > > &pipe_config->hw.adjusted_mode);
> > > > > > > @@ -13221,17 +13218,17 @@ intel_crtc_copy_uapi_to_hw_state(struct
> > > > > > > intel_crtc_state *crtc_state)
> > > > > > > {
> > > > > > > crtc_state->hw.enable = crtc_state->uapi.enable;
> > > > > > > crtc_state->hw.active = crtc_state->uapi.active;
> > > > > > > - crtc_state->hw.mode = crtc_state->uapi.mode;
> > > > > > > crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
> > > > > > > intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
> > > > > > > }
> > > > > > >
> > > > > > > -static void intel_crtc_copy_hw_to_uapi_state(struct
> > > > > > > intel_crtc_state *crtc_state)
> > > > > > > +static void intel_crtc_copy_hw_to_uapi_state(struct
> > > > > > > intel_crtc_state *crtc_state,
> > > > > > > + struct drm_display_mode
> > > > > > > *user_mode)
> > > > > > > {
> > > > > > > crtc_state->uapi.enable = crtc_state->hw.enable;
> > > > > > > crtc_state->uapi.active = crtc_state->hw.active;
> > > > > > > drm_WARN_ON(crtc_state->uapi.crtc->dev,
> > > > > > > - drm_atomic_set_mode_for_crtc(&crtc_state->uapi,
> > > > > > > &crtc_state->hw.mode) < 0);
> > > > > > > + drm_atomic_set_mode_for_crtc(&crtc_state->uapi,
> > > > > > > user_mode) < 0);
> > > > > > >
> > > > > > > crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> > > > > > >
> > > > > > > @@ -13277,6 +13274,10 @@ intel_crtc_prepare_cleared_state(struct
> > > > > > > intel_crtc_state *crtc_state)
> > > > > > > memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> > > > > > > kfree(saved_state);
> > > > > > >
> > > > > > > + /* Clear I915_MODE_FLAG_INHERITED */
> > > > > > > + crtc_state->uapi.mode.private_flags = 0;
> > > > > > > + crtc_state->uapi.adjusted_mode.private_flags = 0;
> > > > > > > +
> > > > > > > intel_crtc_copy_uapi_to_hw_state(crtc_state);
> > > > > > >
> > > > > > > return 0;
> > > > > > > @@ -13324,7 +13325,7 @@ intel_modeset_pipe_config(struct
> > > > > > > intel_crtc_state *pipe_config)
> > > > > > > * computation to clearly distinguish it from the adjusted
> > > > > > > mode, which
> > > > > > > * can be changed by the connectors in the below retry loop.
> > > > > > > */
> > > > > > > - drm_mode_get_hv_timing(&pipe_config->hw.mode,
> > > > > > > + drm_mode_get_hv_timing(&pipe_config->hw.adjusted_mode,
> > > > > > > &pipe_config->pipe_src_w,
> > > > > > > &pipe_config->pipe_src_h);
> > > > > > >
> > > > > > > @@ -18461,15 +18462,11 @@ static void
> > > > > > > intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > > > > int min_cdclk = 0;
> > > > > > >
> > > > > > > if (crtc_state->hw.active) {
> > > > > > > - struct drm_display_mode *mode =
> > > > > > > &crtc_state->hw.mode;
> > > > > > > + struct drm_display_mode mode;
> > > > > > >
> > > > > > >
> > > > > > > intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
> > > > > > > crtc_state);
> > > > > > >
> > > > > > > - *mode = crtc_state->hw.adjusted_mode;
> > > > > > > - mode->hdisplay = crtc_state->pipe_src_w;
> > > > > > > - mode->vdisplay = crtc_state->pipe_src_h;
> > > > > > > -
> > > > > > > /*
> > > > > > > * The initial mode needs to be set in order to
> > > > > > > keep
> > > > > > > * the atomic core happy. It wants a valid mode
> > > > > > > if the
> > > > > > > @@ -18481,11 +18478,15 @@ static void
> > > > > > > intel_modeset_readout_hw_state(struct drm_device *dev)
> > > > > > > */
> > > > > > > crtc_state->inherited = true;
> > > > > > >
> > > > > > > + mode = crtc_state->hw.adjusted_mode;
> > > > > > > + mode.hdisplay = crtc_state->pipe_src_w;
> > > > > > > + mode.vdisplay = crtc_state->pipe_src_h;
> > > > > > > +
> > > > > > > intel_crtc_compute_pixel_rate(crtc_state);
> > > > > > >
> > > > > > > intel_crtc_update_active_timings(crtc_state);
> > > > > > >
> > > > > > > - intel_crtc_copy_hw_to_uapi_state(crtc_state);
> > > > > > > + intel_crtc_copy_hw_to_uapi_state(crtc_state,
> > > > > > > &mode);
> > > > > > > }
> > > > > > >
> > > > > > > for_each_intel_plane_on_crtc(&dev_priv->drm, crtc,
> > > > > > > plane) {
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > index e8f809161c75..f1e29d9a75d0 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > @@ -807,7 +807,7 @@ struct intel_crtc_state {
> > > > > > > struct {
> > > > > > > bool active, enable;
> > > > > > > struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> > > > > > > - struct drm_display_mode mode, adjusted_mode;
> > > > > > > + struct drm_display_mode adjusted_mode;
> > > > > > > } hw;
> > > > > > >
> > > > > > > /**
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > > b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > > index 307ed8ae9a19..0b9bf1fec0f4 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> > > > > > > @@ -209,7 +209,7 @@ static void intel_enable_dvo(struct
> > > > > > > intel_atomic_state *state,
> > > > > > > u32 temp = intel_de_read(dev_priv, dvo_reg);
> > > > > > >
> > > > > > > intel_dvo->dev.dev_ops->mode_set(&intel_dvo->dev,
> > > > > > > - &pipe_config->hw.mode,
> > > > > > > + &pipe_config->hw.adjusted_mode,
> > > > > > >
> > > > > > > &pipe_config->hw.adjusted_mode);
> > > > > > >
> > > > > > > intel_de_write(dev_priv, dvo_reg, temp | DVO_ENABLE);
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > > b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > > index 2da4388e1540..8b78ae0c39a0 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > > > > @@ -1223,7 +1223,6 @@
> > > > > > > intel_sdvo_set_output_timings_from_mode(struct intel_sdvo
> > > > > > > *intel_sdvo,
> > > > > > > static bool
> > > > > > > intel_sdvo_get_preferred_input_mode(struct intel_sdvo
> > > > > > > *intel_sdvo,
> > > > > > > struct intel_sdvo_connector
> > > > > > > *intel_sdvo_connector,
> > > > > > > - const struct drm_display_mode *mode,
> > > > > > > struct drm_display_mode
> > > > > > > *adjusted_mode)
> > > > > > > {
> > > > > > > struct intel_sdvo_dtd input_dtd;
> > > > > > > @@ -1234,9 +1233,9 @@ intel_sdvo_get_preferred_input_mode(struct
> > > > > > > intel_sdvo *intel_sdvo,
> > > > > > >
> > > > > > > if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
> > > > > > >
> > > > > > > intel_sdvo_connector,
> > > > > > > - mode->clock / 10,
> > > > > > > - mode->hdisplay,
> > > > > > > - mode->vdisplay))
> > > > > > > +
> > > > > > > adjusted_mode->clock / 10,
> > > > > > > +
> > > > > > > adjusted_mode->hdisplay,
> > > > > > > +
> > > > > > > adjusted_mode->vdisplay))
> > > > > > > return false;
> > > > > > >
> > > > > > > if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
> > > > > > > @@ -1308,7 +1307,6 @@ static int intel_sdvo_compute_config(struct
> > > > > > > intel_encoder *encoder,
> > > > > > > struct intel_sdvo_connector *intel_sdvo_connector =
> > > > > > > to_intel_sdvo_connector(conn_state->connector);
> > > > > > > struct drm_display_mode *adjusted_mode =
> > > > > > > &pipe_config->hw.adjusted_mode;
> > > > > > > - struct drm_display_mode *mode = &pipe_config->hw.mode;
> > > > > > >
> > > > > > > DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
> > > > > > > pipe_config->pipe_bpp = 8*3;
> > > > > > > @@ -1324,12 +1322,12 @@ static int
> > > > > > > intel_sdvo_compute_config(struct intel_encoder *encoder,
> > > > > > > * the sequence to do it. Oh well.
> > > > > > > */
> > > > > > > if (IS_TV(intel_sdvo_connector)) {
> > > > > > > - if
> > > > > > > (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> > > > > > > + if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> > > > > > > +
> > > > > > > adjusted_mode))
> > > > > > > return -EINVAL;
> > > > > > >
> > > > > > > (void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > > > > >
> > > > > > > intel_sdvo_connector,
> > > > > > > - mode,
> > > > > > >
> > > > > > > adjusted_mode);
> > > > > > > pipe_config->sdvo_tv_clock = true;
> > > > > > > } else if (IS_LVDS(intel_sdvo_connector)) {
> > > > > > > @@ -1339,7 +1337,6 @@ static int intel_sdvo_compute_config(struct
> > > > > > > intel_encoder *encoder,
> > > > > > >
> > > > > > > (void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > > > > >
> > > > > > > intel_sdvo_connector,
> > > > > > > - mode,
> > > > > > >
> > > > > > > adjusted_mode);
> > > > > > > }
> > > > > > >
> > > > > > > @@ -1458,7 +1455,6 @@ static void intel_sdvo_pre_enable(struct
> > > > > > > intel_atomic_state *state,
> > > > > > > to_intel_sdvo_connector_state(conn_state);
> > > > > > > const struct intel_sdvo_connector *intel_sdvo_connector =
> > > > > > > to_intel_sdvo_connector(conn_state->connector);
> > > > > > > - const struct drm_display_mode *mode = &crtc_state->hw.mode;
> > > > > > > struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
> > > > > > > u32 sdvox;
> > > > > > > struct intel_sdvo_in_out_map in_out;
> > > > > > > @@ -1491,7 +1487,7 @@ static void intel_sdvo_pre_enable(struct
> > > > > > > intel_atomic_state *state,
> > > > > > > intel_sdvo_get_dtd_from_mode(&output_dtd,
> > > > > > >
> > > > > > > intel_sdvo_connector->base.panel.fixed_mode);
> > > > > > > else
> > > > > > > - intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> > > > > > > + intel_sdvo_get_dtd_from_mode(&output_dtd,
> > > > > > > adjusted_mode);
> > > > > > > if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
> > > > > > > drm_info(&dev_priv->drm,
> > > > > > > "Setting output timings on %s failed\n",
> > > > > > > --
> > > > > > > 2.19.1
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > Intel-gfx mailing list
> > > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > > >
> > > > > > --
> > > > > > 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
> > >
> > > --
> > > 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
