* [PATCH 2/3] drm/i915/display: Replace crtc_state's has_drrs by drrs_downclock_mode
2022-04-21 19:22 [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches José Roberto de Souza
@ 2022-04-21 19:22 ` José Roberto de Souza
2022-04-25 11:55 ` [Intel-gfx] " Jani Nikula
2022-04-21 19:22 ` [PATCH 3/3] drm/i915/display: Implement seamless mode switch José Roberto de Souza
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: José Roberto de Souza @ 2022-04-21 19:22 UTC (permalink / raw)
To: intel-gfx, dri-devel
Cc: José Roberto de Souza, Sean Paul, Vidya Srinivas
Will be adding some additional control options to DRRS that will
require to have the DRRS downclock mode stored in the crtc_state.
So to optimize memory usage a bit here using it to replace has_drrs
as we can check if the drrs_downclock_mode pointer is different than
null to have the same behavior has has_drrs.
Cc: Vidya Srinivas <vidya.srinivas@intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 4 ++--
drivers/gpu/drm/i915/display/intel_display_debugfs.c | 4 ++--
drivers/gpu/drm/i915/display/intel_display_types.h | 4 +++-
drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
drivers/gpu/drm/i915/display/intel_drrs.c | 4 ++--
5 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 0ddfce21a828d..a5f5caeced9a0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5360,7 +5360,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
drm_dbg_kms(&dev_priv->drm, "ips: %i, double wide: %i, drrs: %i\n",
pipe_config->ips_enabled, pipe_config->double_wide,
- pipe_config->has_drrs);
+ CRTC_STATE_HAS_DRRS(pipe_config));
intel_dpll_dump_hw_state(dev_priv, &pipe_config->dpll_hw_state);
@@ -7088,7 +7088,7 @@ static void intel_crtc_copy_fastset(const struct intel_crtc_state *old_crtc_stat
new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
- new_crtc_state->has_drrs = old_crtc_state->has_drrs;
+ new_crtc_state->drrs_downclock_mode = old_crtc_state->drrs_downclock_mode;
}
static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 452d773fd4e34..f9720562336da 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -1096,7 +1096,7 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
/* DRRS Supported */
seq_printf(m, "\tDRRS Enabled: %s\n",
- str_yes_no(crtc_state->has_drrs));
+ str_yes_no(CRTC_STATE_HAS_DRRS(crtc_state)));
seq_printf(m, "\tDRRS Active: %s\n",
str_yes_no(intel_drrs_is_active(crtc)));
@@ -1786,7 +1786,7 @@ static int i915_drrs_ctl_set(void *data, u64 val)
crtc_state = to_intel_crtc_state(crtc->base.state);
if (!crtc_state->hw.active ||
- !crtc_state->has_drrs)
+ !CRTC_STATE_HAS_DRRS(crtc_state))
goto out;
commit = crtc_state->uapi.commit;
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index cfd042117b109..f0b3cfd3138ce 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1056,7 +1056,7 @@ struct intel_crtc_state {
/* m2_n2 for eDP downclock */
struct intel_link_m_n dp_m2_n2;
- bool has_drrs;
+ const struct drm_display_mode *drrs_downclock_mode;
/* PSR is supported but might not be enabled due the lack of enabled planes */
bool has_psr;
@@ -1264,6 +1264,8 @@ enum drrs_refresh_rate {
DRRS_REFRESH_RATE_LOW,
};
+#define CRTC_STATE_HAS_DRRS(crtc_state) (!!((crtc_state)->drrs_downclock_mode))
+
#define INTEL_PIPE_CRC_ENTRIES_NR 128
struct intel_pipe_crc {
spinlock_t lock;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index d55acc4a028a8..feea172dd2753 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1881,7 +1881,7 @@ intel_dp_drrs_compute_config(struct intel_connector *connector,
if (IS_IRONLAKE(i915) || IS_SANDYBRIDGE(i915) || IS_IVYBRIDGE(i915))
pipe_config->msa_timing_delay = i915->vbt.edp.drrs_msa_timing_delay;
- pipe_config->has_drrs = true;
+ pipe_config->drrs_downclock_mode = downclock_mode;
pixel_clock = downclock_mode->clock;
if (pipe_config->splitter.enable)
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
index 166caf293f7bc..dd527dfc2d1d5 100644
--- a/drivers/gpu/drm/i915/display/intel_drrs.c
+++ b/drivers/gpu/drm/i915/display/intel_drrs.c
@@ -144,7 +144,7 @@ void intel_drrs_activate(const struct intel_crtc_state *crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
- if (!crtc_state->has_drrs)
+ if (!CRTC_STATE_HAS_DRRS(crtc_state))
return;
if (!crtc_state->hw.active)
@@ -176,7 +176,7 @@ void intel_drrs_deactivate(const struct intel_crtc_state *old_crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
- if (!old_crtc_state->has_drrs)
+ if (!CRTC_STATE_HAS_DRRS(old_crtc_state))
return;
if (!old_crtc_state->hw.active)
--
2.36.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Intel-gfx] [PATCH 2/3] drm/i915/display: Replace crtc_state's has_drrs by drrs_downclock_mode
2022-04-21 19:22 ` [PATCH 2/3] drm/i915/display: Replace crtc_state's has_drrs by drrs_downclock_mode José Roberto de Souza
@ 2022-04-25 11:55 ` Jani Nikula
2022-04-26 18:09 ` Souza, Jose
0 siblings, 1 reply; 9+ messages in thread
From: Jani Nikula @ 2022-04-25 11:55 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx, dri-devel; +Cc: Sean Paul
On Thu, 21 Apr 2022, José Roberto de Souza <jose.souza@intel.com> wrote:
> Will be adding some additional control options to DRRS that will
> require to have the DRRS downclock mode stored in the crtc_state.
>
> So to optimize memory usage a bit here using it to replace has_drrs
> as we can check if the drrs_downclock_mode pointer is different than
> null to have the same behavior has has_drrs.
>
> Cc: Vidya Srinivas <vidya.srinivas@intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 4 ++--
> drivers/gpu/drm/i915/display/intel_display_debugfs.c | 4 ++--
> drivers/gpu/drm/i915/display/intel_display_types.h | 4 +++-
> drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> drivers/gpu/drm/i915/display/intel_drrs.c | 4 ++--
> 5 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 0ddfce21a828d..a5f5caeced9a0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5360,7 +5360,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
>
> drm_dbg_kms(&dev_priv->drm, "ips: %i, double wide: %i, drrs: %i\n",
> pipe_config->ips_enabled, pipe_config->double_wide,
> - pipe_config->has_drrs);
> + CRTC_STATE_HAS_DRRS(pipe_config));
I'll mostly let Ville comment on the series, but that macro is an
eyesore and also just out of place in intel_display_types.h. Please make
it a proper function intel_drrs_something_something() in
intel_drrs.[ch].
BR,
Jani.
>
> intel_dpll_dump_hw_state(dev_priv, &pipe_config->dpll_hw_state);
>
> @@ -7088,7 +7088,7 @@ static void intel_crtc_copy_fastset(const struct intel_crtc_state *old_crtc_stat
> new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> - new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> + new_crtc_state->drrs_downclock_mode = old_crtc_state->drrs_downclock_mode;
> }
>
> static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 452d773fd4e34..f9720562336da 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -1096,7 +1096,7 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
>
> /* DRRS Supported */
> seq_printf(m, "\tDRRS Enabled: %s\n",
> - str_yes_no(crtc_state->has_drrs));
> + str_yes_no(CRTC_STATE_HAS_DRRS(crtc_state)));
>
> seq_printf(m, "\tDRRS Active: %s\n",
> str_yes_no(intel_drrs_is_active(crtc)));
> @@ -1786,7 +1786,7 @@ static int i915_drrs_ctl_set(void *data, u64 val)
> crtc_state = to_intel_crtc_state(crtc->base.state);
>
> if (!crtc_state->hw.active ||
> - !crtc_state->has_drrs)
> + !CRTC_STATE_HAS_DRRS(crtc_state))
> goto out;
>
> commit = crtc_state->uapi.commit;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index cfd042117b109..f0b3cfd3138ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1056,7 +1056,7 @@ struct intel_crtc_state {
>
> /* m2_n2 for eDP downclock */
> struct intel_link_m_n dp_m2_n2;
> - bool has_drrs;
> + const struct drm_display_mode *drrs_downclock_mode;
>
> /* PSR is supported but might not be enabled due the lack of enabled planes */
> bool has_psr;
> @@ -1264,6 +1264,8 @@ enum drrs_refresh_rate {
> DRRS_REFRESH_RATE_LOW,
> };
>
> +#define CRTC_STATE_HAS_DRRS(crtc_state) (!!((crtc_state)->drrs_downclock_mode))
> +
> #define INTEL_PIPE_CRC_ENTRIES_NR 128
> struct intel_pipe_crc {
> spinlock_t lock;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index d55acc4a028a8..feea172dd2753 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1881,7 +1881,7 @@ intel_dp_drrs_compute_config(struct intel_connector *connector,
> if (IS_IRONLAKE(i915) || IS_SANDYBRIDGE(i915) || IS_IVYBRIDGE(i915))
> pipe_config->msa_timing_delay = i915->vbt.edp.drrs_msa_timing_delay;
>
> - pipe_config->has_drrs = true;
> + pipe_config->drrs_downclock_mode = downclock_mode;
>
> pixel_clock = downclock_mode->clock;
> if (pipe_config->splitter.enable)
> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
> index 166caf293f7bc..dd527dfc2d1d5 100644
> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> @@ -144,7 +144,7 @@ void intel_drrs_activate(const struct intel_crtc_state *crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>
> - if (!crtc_state->has_drrs)
> + if (!CRTC_STATE_HAS_DRRS(crtc_state))
> return;
>
> if (!crtc_state->hw.active)
> @@ -176,7 +176,7 @@ void intel_drrs_deactivate(const struct intel_crtc_state *old_crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>
> - if (!old_crtc_state->has_drrs)
> + if (!CRTC_STATE_HAS_DRRS(old_crtc_state))
> return;
>
> if (!old_crtc_state->hw.active)
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Intel-gfx] [PATCH 2/3] drm/i915/display: Replace crtc_state's has_drrs by drrs_downclock_mode
2022-04-25 11:55 ` [Intel-gfx] " Jani Nikula
@ 2022-04-26 18:09 ` Souza, Jose
0 siblings, 0 replies; 9+ messages in thread
From: Souza, Jose @ 2022-04-26 18:09 UTC (permalink / raw)
To: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
jani.nikula@linux.intel.com
Cc: seanpaul@chromium.org
On Mon, 2022-04-25 at 14:55 +0300, Jani Nikula wrote:
> On Thu, 21 Apr 2022, José Roberto de Souza <jose.souza@intel.com> wrote:
> > Will be adding some additional control options to DRRS that will
> > require to have the DRRS downclock mode stored in the crtc_state.
> >
> > So to optimize memory usage a bit here using it to replace has_drrs
> > as we can check if the drrs_downclock_mode pointer is different than
> > null to have the same behavior has has_drrs.
> >
> > Cc: Vidya Srinivas <vidya.srinivas@intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 4 ++--
> > drivers/gpu/drm/i915/display/intel_display_debugfs.c | 4 ++--
> > drivers/gpu/drm/i915/display/intel_display_types.h | 4 +++-
> > drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_drrs.c | 4 ++--
> > 5 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 0ddfce21a828d..a5f5caeced9a0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5360,7 +5360,7 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
> >
> > drm_dbg_kms(&dev_priv->drm, "ips: %i, double wide: %i, drrs: %i\n",
> > pipe_config->ips_enabled, pipe_config->double_wide,
> > - pipe_config->has_drrs);
> > + CRTC_STATE_HAS_DRRS(pipe_config));
>
> I'll mostly let Ville comment on the series, but that macro is an
> eyesore and also just out of place in intel_display_types.h. Please make
> it a proper function intel_drrs_something_something() in
> intel_drrs.[ch].
Okay in making this a function but I don't have a good name for it neither.
intel_crtc_state_has_drrs() is worst than current name of the macro.
>
> BR,
> Jani.
>
> >
> > intel_dpll_dump_hw_state(dev_priv, &pipe_config->dpll_hw_state);
> >
> > @@ -7088,7 +7088,7 @@ static void intel_crtc_copy_fastset(const struct intel_crtc_state *old_crtc_stat
> > new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > - new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > + new_crtc_state->drrs_downclock_mode = old_crtc_state->drrs_downclock_mode;
> > }
> >
> > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index 452d773fd4e34..f9720562336da 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -1096,7 +1096,7 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
> >
> > /* DRRS Supported */
> > seq_printf(m, "\tDRRS Enabled: %s\n",
> > - str_yes_no(crtc_state->has_drrs));
> > + str_yes_no(CRTC_STATE_HAS_DRRS(crtc_state)));
> >
> > seq_printf(m, "\tDRRS Active: %s\n",
> > str_yes_no(intel_drrs_is_active(crtc)));
> > @@ -1786,7 +1786,7 @@ static int i915_drrs_ctl_set(void *data, u64 val)
> > crtc_state = to_intel_crtc_state(crtc->base.state);
> >
> > if (!crtc_state->hw.active ||
> > - !crtc_state->has_drrs)
> > + !CRTC_STATE_HAS_DRRS(crtc_state))
> > goto out;
> >
> > commit = crtc_state->uapi.commit;
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index cfd042117b109..f0b3cfd3138ce 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1056,7 +1056,7 @@ struct intel_crtc_state {
> >
> > /* m2_n2 for eDP downclock */
> > struct intel_link_m_n dp_m2_n2;
> > - bool has_drrs;
> > + const struct drm_display_mode *drrs_downclock_mode;
> >
> > /* PSR is supported but might not be enabled due the lack of enabled planes */
> > bool has_psr;
> > @@ -1264,6 +1264,8 @@ enum drrs_refresh_rate {
> > DRRS_REFRESH_RATE_LOW,
> > };
> >
> > +#define CRTC_STATE_HAS_DRRS(crtc_state) (!!((crtc_state)->drrs_downclock_mode))
> > +
> > #define INTEL_PIPE_CRC_ENTRIES_NR 128
> > struct intel_pipe_crc {
> > spinlock_t lock;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index d55acc4a028a8..feea172dd2753 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1881,7 +1881,7 @@ intel_dp_drrs_compute_config(struct intel_connector *connector,
> > if (IS_IRONLAKE(i915) || IS_SANDYBRIDGE(i915) || IS_IVYBRIDGE(i915))
> > pipe_config->msa_timing_delay = i915->vbt.edp.drrs_msa_timing_delay;
> >
> > - pipe_config->has_drrs = true;
> > + pipe_config->drrs_downclock_mode = downclock_mode;
> >
> > pixel_clock = downclock_mode->clock;
> > if (pipe_config->splitter.enable)
> > diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
> > index 166caf293f7bc..dd527dfc2d1d5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> > @@ -144,7 +144,7 @@ void intel_drrs_activate(const struct intel_crtc_state *crtc_state)
> > {
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >
> > - if (!crtc_state->has_drrs)
> > + if (!CRTC_STATE_HAS_DRRS(crtc_state))
> > return;
> >
> > if (!crtc_state->hw.active)
> > @@ -176,7 +176,7 @@ void intel_drrs_deactivate(const struct intel_crtc_state *old_crtc_state)
> > {
> > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> >
> > - if (!old_crtc_state->has_drrs)
> > + if (!CRTC_STATE_HAS_DRRS(old_crtc_state))
> > return;
> >
> > if (!old_crtc_state->hw.active)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] drm/i915/display: Implement seamless mode switch
2022-04-21 19:22 [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches José Roberto de Souza
2022-04-21 19:22 ` [PATCH 2/3] drm/i915/display: Replace crtc_state's has_drrs by drrs_downclock_mode José Roberto de Souza
@ 2022-04-21 19:22 ` José Roberto de Souza
2022-04-26 16:53 ` [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches Srinivas, Vidya
2022-04-26 18:08 ` Ville Syrjälä
3 siblings, 0 replies; 9+ messages in thread
From: José Roberto de Souza @ 2022-04-21 19:22 UTC (permalink / raw)
To: intel-gfx, dri-devel
Cc: José Roberto de Souza, Sean Paul, Vidya Srinivas
As described in previous commit "drm: Add infrastructure to allow
seamless mode switches" here doing the i915 implementation.
The main steps are:
- drm_atomic_helper_check_modeset() will call
atomic_seamless_mode_switch_check()/intel_drrs_seamless_mode_switch_check()
if conditions match
- intel_drrs_seamless_mode_switch_check() will check if seamless DRRS
is enabled and if the requested mode matches with fixed or downclock
mode
- now at the atomic commit phase during the execution of
intel_pre_plane_update(), intel_drrs_deactivate() will
return without change the DRRS state if seamless_mode_switch_enabled
or seamless_mode_changed is set and there is no modeset happening in
this pipe(something after drm_atomic_helper_check_modeset() could
still require a modeset and set mode_changed)
- then in intel_post_plane_update(), intel_drrs_activate() is called
and DRRS mode is switched if seamless_mode_changed is set or the
function is skipped if seamless_mode_switch_enabled is set
and pipe don't need a modeset
After a modeset, seamless_mode_switch_enabled is set to 0 and
DRRS is back to automatic mode until the next commit
that intel_drrs_seamless_mode_switch_check() is executed and
it supports the mode.
Cc: Vidya Srinivas <vidya.srinivas@intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/display/intel_crtc.c | 10 +++
drivers/gpu/drm/i915/display/intel_display.c | 5 +-
.../drm/i915/display/intel_display_debugfs.c | 5 +-
.../drm/i915/display/intel_display_types.h | 3 +-
drivers/gpu/drm/i915/display/intel_dp.c | 2 +
drivers/gpu/drm/i915/display/intel_drrs.c | 73 ++++++++++++++++++-
drivers/gpu/drm/i915/display/intel_drrs.h | 5 +-
7 files changed, 96 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index 4442aa355f868..d411daa0b2bab 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -215,6 +215,15 @@ static int intel_crtc_late_register(struct drm_crtc *crtc)
return 0;
}
+static int intel_crtc_seamless_mode_switch_check(struct drm_atomic_state *_state,
+ struct drm_crtc *_crtc)
+{
+ struct intel_atomic_state *state = to_intel_atomic_state(_state);
+ struct intel_crtc *crtc = to_intel_crtc(_crtc);
+
+ return intel_drrs_seamless_mode_switch_check(state, crtc);
+}
+
#define INTEL_CRTC_FUNCS \
.set_config = drm_atomic_helper_set_config, \
.destroy = intel_crtc_destroy, \
@@ -233,6 +242,7 @@ static const struct drm_crtc_funcs bdw_crtc_funcs = {
.enable_vblank = bdw_enable_vblank,
.disable_vblank = bdw_disable_vblank,
.get_vblank_timestamp = intel_crtc_get_vblank_timestamp,
+ .atomic_seamless_mode_switch_check = intel_crtc_seamless_mode_switch_check,
};
static const struct drm_crtc_funcs ilk_crtc_funcs = {
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a5f5caeced9a0..ebfa7d68e35fe 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1329,7 +1329,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
intel_atomic_get_new_crtc_state(state, crtc);
enum pipe pipe = crtc->pipe;
- intel_drrs_deactivate(old_crtc_state);
+ intel_drrs_deactivate(old_crtc_state, new_crtc_state);
intel_psr_pre_plane_update(state, crtc);
@@ -7089,6 +7089,7 @@ static void intel_crtc_copy_fastset(const struct intel_crtc_state *old_crtc_stat
new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
new_crtc_state->drrs_downclock_mode = old_crtc_state->drrs_downclock_mode;
+ new_crtc_state->drrs_fixed_mode = old_crtc_state->drrs_fixed_mode;
}
static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
@@ -7716,6 +7717,8 @@ static int intel_atomic_check(struct drm_device *dev,
if (!intel_crtc_needs_modeset(new_crtc_state)) {
if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
copy_bigjoiner_crtc_state_nomodeset(state, crtc);
+ else if (new_crtc_state->uapi.seamless_mode_changed)
+ intel_crtc_copy_uapi_to_hw_state_modeset(state, crtc);
else
intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc);
continue;
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index f9720562336da..1d27ed2b68210 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -1108,6 +1108,9 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
crtc->drrs.refresh_rate == DRRS_REFRESH_RATE_LOW ?
"low" : "high");
+ seq_printf(m, "Seamless mode switch enabled: %s\n",
+ str_yes_no(crtc->drrs.seamless_mode_switch_enabled));
+
mutex_unlock(&crtc->drrs.mutex);
}
@@ -1802,7 +1805,7 @@ static int i915_drrs_ctl_set(void *data, u64 val)
if (val)
intel_drrs_activate(crtc_state);
else
- intel_drrs_deactivate(crtc_state);
+ intel_drrs_deactivate(crtc_state, crtc_state);
out:
drm_modeset_unlock(&crtc->base.mutex);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index f0b3cfd3138ce..b04922fd45e45 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1056,7 +1056,7 @@ struct intel_crtc_state {
/* m2_n2 for eDP downclock */
struct intel_link_m_n dp_m2_n2;
- const struct drm_display_mode *drrs_downclock_mode;
+ const struct drm_display_mode *drrs_fixed_mode, *drrs_downclock_mode;
/* PSR is supported but might not be enabled due the lack of enabled planes */
bool has_psr;
@@ -1316,6 +1316,7 @@ struct intel_crtc {
unsigned int busy_frontbuffer_bits;
enum transcoder cpu_transcoder;
struct intel_link_m_n m_n, m2_n2;
+ bool seamless_mode_switch_enabled;
} drrs;
int scanline_offset;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index feea172dd2753..82f13c3b0f2b3 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1881,6 +1881,8 @@ intel_dp_drrs_compute_config(struct intel_connector *connector,
if (IS_IRONLAKE(i915) || IS_SANDYBRIDGE(i915) || IS_IVYBRIDGE(i915))
pipe_config->msa_timing_delay = i915->vbt.edp.drrs_msa_timing_delay;
+ pipe_config->drrs_fixed_mode = intel_panel_fixed_mode(connector,
+ &pipe_config->hw.adjusted_mode);
pipe_config->drrs_downclock_mode = downclock_mode;
pixel_clock = downclock_mode->clock;
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
index dd527dfc2d1d5..3a801f7e79187 100644
--- a/drivers/gpu/drm/i915/display/intel_drrs.c
+++ b/drivers/gpu/drm/i915/display/intel_drrs.c
@@ -155,6 +155,23 @@ void intel_drrs_activate(const struct intel_crtc_state *crtc_state)
mutex_lock(&crtc->drrs.mutex);
+ /* Until the next modeset user-space will control the refresh rate */
+ if (crtc_state->uapi.seamless_mode_changed &&
+ !intel_crtc_needs_modeset(crtc_state)) {
+ enum drrs_refresh_rate rate = DRRS_REFRESH_RATE_HIGH;
+
+ if (drm_mode_match(&crtc_state->hw.adjusted_mode,
+ crtc_state->drrs_downclock_mode,
+ DRM_MODE_MATCH_CLOCK))
+ rate = DRRS_REFRESH_RATE_LOW;
+
+ crtc->drrs.seamless_mode_switch_enabled = true;
+ intel_drrs_set_state(crtc, rate);
+ }
+
+ if (crtc->drrs.seamless_mode_switch_enabled)
+ goto unlock;
+
crtc->drrs.cpu_transcoder = crtc_state->cpu_transcoder;
crtc->drrs.m_n = crtc_state->dp_m_n;
crtc->drrs.m2_n2 = crtc_state->dp_m2_n2;
@@ -162,7 +179,7 @@ void intel_drrs_activate(const struct intel_crtc_state *crtc_state)
crtc->drrs.busy_frontbuffer_bits = 0;
intel_drrs_schedule_work(crtc);
-
+unlock:
mutex_unlock(&crtc->drrs.mutex);
}
@@ -172,7 +189,8 @@ void intel_drrs_activate(const struct intel_crtc_state *crtc_state)
*
* Deactivates DRRS on the crtc.
*/
-void intel_drrs_deactivate(const struct intel_crtc_state *old_crtc_state)
+void intel_drrs_deactivate(const struct intel_crtc_state *old_crtc_state,
+ const struct intel_crtc_state *new_crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
@@ -187,13 +205,25 @@ void intel_drrs_deactivate(const struct intel_crtc_state *old_crtc_state)
mutex_lock(&crtc->drrs.mutex);
+ /*
+ * When seamless_mode_switch_enabled is enabled, user-space is in
+ * control of DRRS and it will only go to the automatic mode on the
+ * next modeset
+ */
+ if ((crtc->drrs.seamless_mode_switch_enabled ||
+ new_crtc_state->uapi.seamless_mode_changed) &&
+ !intel_crtc_needs_modeset(new_crtc_state))
+ goto unlock;
+
if (intel_drrs_is_active(crtc))
intel_drrs_set_state(crtc, DRRS_REFRESH_RATE_HIGH);
crtc->drrs.cpu_transcoder = INVALID_TRANSCODER;
crtc->drrs.frontbuffer_bits = 0;
crtc->drrs.busy_frontbuffer_bits = 0;
+ crtc->drrs.seamless_mode_switch_enabled = false;
+unlock:
mutex_unlock(&crtc->drrs.mutex);
cancel_delayed_work_sync(&crtc->drrs.work);
@@ -205,7 +235,8 @@ static void intel_drrs_downclock_work(struct work_struct *work)
mutex_lock(&crtc->drrs.mutex);
- if (intel_drrs_is_active(crtc) && !crtc->drrs.busy_frontbuffer_bits)
+ if (intel_drrs_is_active(crtc) && !crtc->drrs.busy_frontbuffer_bits &&
+ !crtc->drrs.seamless_mode_switch_enabled)
intel_drrs_set_state(crtc, DRRS_REFRESH_RATE_LOW);
mutex_unlock(&crtc->drrs.mutex);
@@ -236,6 +267,11 @@ static void intel_drrs_frontbuffer_update(struct drm_i915_private *dev_priv,
else
crtc->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
+ if (crtc->drrs.seamless_mode_switch_enabled) {
+ mutex_unlock(&crtc->drrs.mutex);
+ continue;
+ }
+
/* flush/invalidate means busy screen hence upclock */
intel_drrs_set_state(crtc, DRRS_REFRESH_RATE_HIGH);
@@ -300,3 +336,34 @@ void intel_crtc_drrs_init(struct intel_crtc *crtc)
mutex_init(&crtc->drrs.mutex);
crtc->drrs.cpu_transcoder = INVALID_TRANSCODER;
}
+
+int intel_drrs_seamless_mode_switch_check(struct intel_atomic_state *state,
+ struct intel_crtc *crtc)
+{
+ const unsigned int match_mode_flags = DRM_MODE_MATCH_TIMINGS |
+ DRM_MODE_MATCH_FLAGS |
+ DRM_MODE_MATCH_3D_FLAGS |
+ DRM_MODE_MATCH_ASPECT_RATIO;
+ struct intel_crtc_state *new_crtc_state;
+
+ new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
+ if (IS_ERR(new_crtc_state))
+ return PTR_ERR(new_crtc_state);
+
+ if (!CRTC_STATE_HAS_DRRS(new_crtc_state))
+ return -EOPNOTSUPP;
+
+ /* Requested mode matches with fixed or downclock mode? */
+ if (!drm_mode_match(&new_crtc_state->uapi.mode,
+ new_crtc_state->drrs_fixed_mode,
+ match_mode_flags) &&
+ !drm_mode_match(&new_crtc_state->uapi.mode,
+ new_crtc_state->drrs_downclock_mode,
+ match_mode_flags))
+ return -EOPNOTSUPP;
+
+ drm_mode_copy(&new_crtc_state->uapi.adjusted_mode,
+ &new_crtc_state->uapi.mode);
+
+ return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.h b/drivers/gpu/drm/i915/display/intel_drrs.h
index 3ad1be1ad9c13..85ad987c45945 100644
--- a/drivers/gpu/drm/i915/display/intel_drrs.h
+++ b/drivers/gpu/drm/i915/display/intel_drrs.h
@@ -18,11 +18,14 @@ struct intel_connector;
const char *intel_drrs_type_str(enum drrs_type drrs_type);
bool intel_drrs_is_active(struct intel_crtc *crtc);
void intel_drrs_activate(const struct intel_crtc_state *crtc_state);
-void intel_drrs_deactivate(const struct intel_crtc_state *crtc_state);
+void intel_drrs_deactivate(const struct intel_crtc_state *old_crtc_state,
+ const struct intel_crtc_state *new_crtc_state);
void intel_drrs_invalidate(struct drm_i915_private *dev_priv,
unsigned int frontbuffer_bits);
void intel_drrs_flush(struct drm_i915_private *dev_priv,
unsigned int frontbuffer_bits);
void intel_crtc_drrs_init(struct intel_crtc *crtc);
+int intel_drrs_seamless_mode_switch_check(struct intel_atomic_state *state,
+ struct intel_crtc *crtc);
#endif /* __INTEL_DRRS_H__ */
--
2.36.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* RE: [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches
2022-04-21 19:22 [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches José Roberto de Souza
2022-04-21 19:22 ` [PATCH 2/3] drm/i915/display: Replace crtc_state's has_drrs by drrs_downclock_mode José Roberto de Souza
2022-04-21 19:22 ` [PATCH 3/3] drm/i915/display: Implement seamless mode switch José Roberto de Souza
@ 2022-04-26 16:53 ` Srinivas, Vidya
2022-04-26 18:08 ` Ville Syrjälä
3 siblings, 0 replies; 9+ messages in thread
From: Srinivas, Vidya @ 2022-04-26 16:53 UTC (permalink / raw)
To: Souza, Jose, intel-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org
Cc: Sean Paul
Hello Jose,
Thanks much for the patch. I tested it on chrome system and the patch works.
Adding my Tested-by.
Tested-by: Vidya Srinivas <vidya.srinivas@intel.com>
Regards
Vidya
> -----Original Message-----
> From: Souza, Jose <jose.souza@intel.com>
> Sent: Friday, April 22, 2022 12:52 AM
> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Srinivas, Vidya <vidya.srinivas@intel.com>; Sean Paul
> <seanpaul@chromium.org>; Ville Syrjälä <ville.syrjala@linux.intel.com>;
> Souza, Jose <jose.souza@intel.com>
> Subject: [PATCH 1/3] drm: Add infrastructure to allow seamless mode
> switches
>
> Intel hardware supports change between modes with different refresh rates
> without any glitches or visual artifacts, that feature is called seamless DRRS.
>
> So far i915 driver was automatically changing between preferred panel mode
> and lower refresh rate mode based on idleness but ChromeOS compositor
> team is requesting to be in control of the mode switch.
> So for a certain types of content it can switch to mode with a lower refresh
> rate without user noticing a thing and saving power.
>
> This seamless mode switch will be triggered when user-space dispatch a
> atomic commit with the new mode and clears the
> DRM_MODE_ATOMIC_ALLOW_MODESET flag.
>
> A driver that don't implement atomic_seamless_mode_switch_check
> function will continue to fail in the atomic check phase with "[CRTC:%d:%s]
> requires full modeset" debug message.
> While a driver that implements it and support the seamless change between
> old and new mode will return 0 otherwise it should return the appropried
> errno.
>
> So here adding basic drm infrastructure to that be supported by i915 and
> other drivers.
>
> Cc: Vidya Srinivas <vidya.srinivas@intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 1 +
> drivers/gpu/drm/drm_atomic_helper.c | 16 +++++++++++++++
> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> include/drm/drm_crtc.h | 25 +++++++++++++++++++++++
> 4 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 58c0283fb6b0c..21525f9f4b4c1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -437,6 +437,7 @@ static void drm_atomic_crtc_print_state(struct
> drm_printer *p,
> drm_printf(p, "\tself_refresh_active=%d\n", state-
> >self_refresh_active);
> drm_printf(p, "\tplanes_changed=%d\n", state->planes_changed);
> drm_printf(p, "\tmode_changed=%d\n", state->mode_changed);
> + drm_printf(p, "\tseamless_mode_changed=%d\n",
> +state->seamless_mode_changed);
> drm_printf(p, "\tactive_changed=%d\n", state->active_changed);
> drm_printf(p, "\tconnectors_changed=%d\n", state-
> >connectors_changed);
> drm_printf(p, "\tcolor_mgmt_changed=%d\n", state-
> >color_mgmt_changed); diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 9603193d2fa13..e6f3a966f7b86 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -631,6 +631,22 @@ drm_atomic_helper_check_modeset(struct
> drm_device *dev,
> drm_dbg_atomic(dev, "[CRTC:%d:%s] mode
> changed\n",
> crtc->base.id, crtc->name);
> new_crtc_state->mode_changed = true;
> +
> + if (!state->allow_modeset &&
> + crtc->funcs-
> >atomic_seamless_mode_switch_check) {
> + ret = crtc->funcs-
> >atomic_seamless_mode_switch_check(state, crtc);
> + if (ret == -EOPNOTSUPP) {
> + drm_dbg_atomic(dev, "[CRTC:%d:%s]
> Seamless mode switch not supported\n",
> + crtc->base.id, crtc-
> >name);
> + return ret;
> + }
> +
> + if (ret < 0)
> + return ret;
> +
> + new_crtc_state->seamless_mode_changed =
> true;
> + new_crtc_state->mode_changed = false;
> + }
> }
>
> if (old_crtc_state->enable != new_crtc_state->enable) { diff --
> git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 3b6d3bdbd0996..c093073ea6e11 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -142,6 +142,7 @@ void
> __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> if (state->gamma_lut)
> drm_property_blob_get(state->gamma_lut);
> state->mode_changed = false;
> + state->seamless_mode_changed = false;
> state->active_changed = false;
> state->planes_changed = false;
> state->connectors_changed = false;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
> a70baea0636ca..b7ce378d679d3 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -140,6 +140,16 @@ struct drm_crtc_state {
> */
> bool mode_changed : 1;
>
> + /**
> + * @seamless_mode_changed: @mode has been changed but user-
> space
> + * is requesting to change to the new mode with a fastset and driver
> + * supports this request.
> + * To be used by drivers to steer the atomic commit control flow to
> + * appropriate paths to change mode without any visual corruption.
> + * Never set together with @mode_changed.
> + */
> + bool seamless_mode_changed : 1;
> +
> /**
> * @active_changed: @active has been toggled. Used by the atomic
> * helpers and drivers to steer the atomic commit control flow. See
> also @@ -939,6 +949,21 @@ struct drm_crtc_funcs {
> int *max_error,
> ktime_t *vblank_time,
> bool in_vblank_irq);
> +
> + /**
> + * @atomic_seamless_mode_switch_check
> + *
> + * Called when user-space wants to change mode without do a
> modeset.
> + * Drivers can optionally support do a mode switch without any visual
> + * corruption when changing between certain modes.
> + *
> + * Returns:
> + * Zero if possible to seamless switch mode, -EOPNOTSUPP if not
> + * supported seamless mode change or appropriate errno if an error
> + * happened.
> + */
> + int (*atomic_seamless_mode_switch_check)(struct
> drm_atomic_state *state,
> + struct drm_crtc *crtc);
> };
>
> /**
> --
> 2.36.0
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches
2022-04-21 19:22 [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches José Roberto de Souza
` (2 preceding siblings ...)
2022-04-26 16:53 ` [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches Srinivas, Vidya
@ 2022-04-26 18:08 ` Ville Syrjälä
2022-04-26 18:32 ` Souza, Jose
3 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2022-04-26 18:08 UTC (permalink / raw)
To: José Roberto de Souza
Cc: intel-gfx, Sean Paul, Vidya Srinivas, dri-devel
On Thu, Apr 21, 2022 at 12:22:03PM -0700, José Roberto de Souza wrote:
> Intel hardware supports change between modes with different refresh
> rates without any glitches or visual artifacts, that feature is called
> seamless DRRS.
>
> So far i915 driver was automatically changing between preferred panel
> mode and lower refresh rate mode based on idleness but ChromeOS
> compositor team is requesting to be in control of the mode switch.
> So for a certain types of content it can switch to mode with a lower
> refresh rate without user noticing a thing and saving power.
>
> This seamless mode switch will be triggered when user-space dispatch
> a atomic commit with the new mode and clears the
> DRM_MODE_ATOMIC_ALLOW_MODESET flag.
>
> A driver that don't implement atomic_seamless_mode_switch_check
> function will continue to fail in the atomic check phase with
> "[CRTC:%d:%s] requires full modeset" debug message.
> While a driver that implements it and support the seamless change
> between old and new mode will return 0 otherwise it should return the
> appropried errno.
>
> So here adding basic drm infrastructure to that be supported by i915
> and other drivers.
I don't see the need for any extra infrastructure for this.
I think we just need:
- fix the fastset code to not suck
- reprogram M/N during fastset
- calculate eDP link params using panel's highest refresh rate mode
to make sure we get the same link params for all refresh rates
>
> Cc: Vidya Srinivas <vidya.srinivas@intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 1 +
> drivers/gpu/drm/drm_atomic_helper.c | 16 +++++++++++++++
> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> include/drm/drm_crtc.h | 25 +++++++++++++++++++++++
> 4 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 58c0283fb6b0c..21525f9f4b4c1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -437,6 +437,7 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
> drm_printf(p, "\tself_refresh_active=%d\n", state->self_refresh_active);
> drm_printf(p, "\tplanes_changed=%d\n", state->planes_changed);
> drm_printf(p, "\tmode_changed=%d\n", state->mode_changed);
> + drm_printf(p, "\tseamless_mode_changed=%d\n", state->seamless_mode_changed);
> drm_printf(p, "\tactive_changed=%d\n", state->active_changed);
> drm_printf(p, "\tconnectors_changed=%d\n", state->connectors_changed);
> drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed);
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9603193d2fa13..e6f3a966f7b86 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -631,6 +631,22 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
> crtc->base.id, crtc->name);
> new_crtc_state->mode_changed = true;
> +
> + if (!state->allow_modeset &&
> + crtc->funcs->atomic_seamless_mode_switch_check) {
> + ret = crtc->funcs->atomic_seamless_mode_switch_check(state, crtc);
> + if (ret == -EOPNOTSUPP) {
> + drm_dbg_atomic(dev, "[CRTC:%d:%s] Seamless mode switch not supported\n",
> + crtc->base.id, crtc->name);
> + return ret;
> + }
> +
> + if (ret < 0)
> + return ret;
> +
> + new_crtc_state->seamless_mode_changed = true;
> + new_crtc_state->mode_changed = false;
> + }
> }
>
> if (old_crtc_state->enable != new_crtc_state->enable) {
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 3b6d3bdbd0996..c093073ea6e11 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -142,6 +142,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> if (state->gamma_lut)
> drm_property_blob_get(state->gamma_lut);
> state->mode_changed = false;
> + state->seamless_mode_changed = false;
> state->active_changed = false;
> state->planes_changed = false;
> state->connectors_changed = false;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a70baea0636ca..b7ce378d679d3 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -140,6 +140,16 @@ struct drm_crtc_state {
> */
> bool mode_changed : 1;
>
> + /**
> + * @seamless_mode_changed: @mode has been changed but user-space
> + * is requesting to change to the new mode with a fastset and driver
> + * supports this request.
> + * To be used by drivers to steer the atomic commit control flow to
> + * appropriate paths to change mode without any visual corruption.
> + * Never set together with @mode_changed.
> + */
> + bool seamless_mode_changed : 1;
> +
> /**
> * @active_changed: @active has been toggled. Used by the atomic
> * helpers and drivers to steer the atomic commit control flow. See also
> @@ -939,6 +949,21 @@ struct drm_crtc_funcs {
> int *max_error,
> ktime_t *vblank_time,
> bool in_vblank_irq);
> +
> + /**
> + * @atomic_seamless_mode_switch_check
> + *
> + * Called when user-space wants to change mode without do a modeset.
> + * Drivers can optionally support do a mode switch without any visual
> + * corruption when changing between certain modes.
> + *
> + * Returns:
> + * Zero if possible to seamless switch mode, -EOPNOTSUPP if not
> + * supported seamless mode change or appropriate errno if an error
> + * happened.
> + */
> + int (*atomic_seamless_mode_switch_check)(struct drm_atomic_state *state,
> + struct drm_crtc *crtc);
> };
>
> /**
> --
> 2.36.0
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches
2022-04-26 18:08 ` Ville Syrjälä
@ 2022-04-26 18:32 ` Souza, Jose
2022-04-26 18:44 ` Ville Syrjälä
0 siblings, 1 reply; 9+ messages in thread
From: Souza, Jose @ 2022-04-26 18:32 UTC (permalink / raw)
To: ville.syrjala@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org, seanpaul@chromium.org,
Srinivas, Vidya, dri-devel@lists.freedesktop.org
On Tue, 2022-04-26 at 21:08 +0300, Ville Syrjälä wrote:
> On Thu, Apr 21, 2022 at 12:22:03PM -0700, José Roberto de Souza wrote:
> > Intel hardware supports change between modes with different refresh
> > rates without any glitches or visual artifacts, that feature is called
> > seamless DRRS.
> >
> > So far i915 driver was automatically changing between preferred panel
> > mode and lower refresh rate mode based on idleness but ChromeOS
> > compositor team is requesting to be in control of the mode switch.
> > So for a certain types of content it can switch to mode with a lower
> > refresh rate without user noticing a thing and saving power.
> >
> > This seamless mode switch will be triggered when user-space dispatch
> > a atomic commit with the new mode and clears the
> > DRM_MODE_ATOMIC_ALLOW_MODESET flag.
> >
> > A driver that don't implement atomic_seamless_mode_switch_check
> > function will continue to fail in the atomic check phase with
> > "[CRTC:%d:%s] requires full modeset" debug message.
> > While a driver that implements it and support the seamless change
> > between old and new mode will return 0 otherwise it should return the
> > appropried errno.
> >
> > So here adding basic drm infrastructure to that be supported by i915
> > and other drivers.
>
> I don't see the need for any extra infrastructure for this.
>
> I think we just need:
> - fix the fastset code to not suck
How would it know that only mode changed and not all other things that causes mode_changed to be set?
For example: intel_digital_connector_atomic_check()
> - reprogram M/N during fastset
> - calculate eDP link params using panel's highest refresh rate mode
> to make sure we get the same link params for all refresh rates
>
> >
> > Cc: Vidya Srinivas <vidya.srinivas@intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 1 +
> > drivers/gpu/drm/drm_atomic_helper.c | 16 +++++++++++++++
> > drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> > include/drm/drm_crtc.h | 25 +++++++++++++++++++++++
> > 4 files changed, 43 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 58c0283fb6b0c..21525f9f4b4c1 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -437,6 +437,7 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
> > drm_printf(p, "\tself_refresh_active=%d\n", state->self_refresh_active);
> > drm_printf(p, "\tplanes_changed=%d\n", state->planes_changed);
> > drm_printf(p, "\tmode_changed=%d\n", state->mode_changed);
> > + drm_printf(p, "\tseamless_mode_changed=%d\n", state->seamless_mode_changed);
> > drm_printf(p, "\tactive_changed=%d\n", state->active_changed);
> > drm_printf(p, "\tconnectors_changed=%d\n", state->connectors_changed);
> > drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed);
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 9603193d2fa13..e6f3a966f7b86 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -631,6 +631,22 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
> > crtc->base.id, crtc->name);
> > new_crtc_state->mode_changed = true;
> > +
> > + if (!state->allow_modeset &&
> > + crtc->funcs->atomic_seamless_mode_switch_check) {
> > + ret = crtc->funcs->atomic_seamless_mode_switch_check(state, crtc);
> > + if (ret == -EOPNOTSUPP) {
> > + drm_dbg_atomic(dev, "[CRTC:%d:%s] Seamless mode switch not supported\n",
> > + crtc->base.id, crtc->name);
> > + return ret;
> > + }
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + new_crtc_state->seamless_mode_changed = true;
> > + new_crtc_state->mode_changed = false;
> > + }
> > }
> >
> > if (old_crtc_state->enable != new_crtc_state->enable) {
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 3b6d3bdbd0996..c093073ea6e11 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -142,6 +142,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> > if (state->gamma_lut)
> > drm_property_blob_get(state->gamma_lut);
> > state->mode_changed = false;
> > + state->seamless_mode_changed = false;
> > state->active_changed = false;
> > state->planes_changed = false;
> > state->connectors_changed = false;
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index a70baea0636ca..b7ce378d679d3 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -140,6 +140,16 @@ struct drm_crtc_state {
> > */
> > bool mode_changed : 1;
> >
> > + /**
> > + * @seamless_mode_changed: @mode has been changed but user-space
> > + * is requesting to change to the new mode with a fastset and driver
> > + * supports this request.
> > + * To be used by drivers to steer the atomic commit control flow to
> > + * appropriate paths to change mode without any visual corruption.
> > + * Never set together with @mode_changed.
> > + */
> > + bool seamless_mode_changed : 1;
> > +
> > /**
> > * @active_changed: @active has been toggled. Used by the atomic
> > * helpers and drivers to steer the atomic commit control flow. See also
> > @@ -939,6 +949,21 @@ struct drm_crtc_funcs {
> > int *max_error,
> > ktime_t *vblank_time,
> > bool in_vblank_irq);
> > +
> > + /**
> > + * @atomic_seamless_mode_switch_check
> > + *
> > + * Called when user-space wants to change mode without do a modeset.
> > + * Drivers can optionally support do a mode switch without any visual
> > + * corruption when changing between certain modes.
> > + *
> > + * Returns:
> > + * Zero if possible to seamless switch mode, -EOPNOTSUPP if not
> > + * supported seamless mode change or appropriate errno if an error
> > + * happened.
> > + */
> > + int (*atomic_seamless_mode_switch_check)(struct drm_atomic_state *state,
> > + struct drm_crtc *crtc);
> > };
> >
> > /**
> > --
> > 2.36.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches
2022-04-26 18:32 ` Souza, Jose
@ 2022-04-26 18:44 ` Ville Syrjälä
0 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2022-04-26 18:44 UTC (permalink / raw)
To: Souza, Jose
Cc: intel-gfx@lists.freedesktop.org, seanpaul@chromium.org,
Srinivas, Vidya, dri-devel@lists.freedesktop.org
On Tue, Apr 26, 2022 at 06:32:01PM +0000, Souza, Jose wrote:
> On Tue, 2022-04-26 at 21:08 +0300, Ville Syrjälä wrote:
> > On Thu, Apr 21, 2022 at 12:22:03PM -0700, José Roberto de Souza wrote:
> > > Intel hardware supports change between modes with different refresh
> > > rates without any glitches or visual artifacts, that feature is called
> > > seamless DRRS.
> > >
> > > So far i915 driver was automatically changing between preferred panel
> > > mode and lower refresh rate mode based on idleness but ChromeOS
> > > compositor team is requesting to be in control of the mode switch.
> > > So for a certain types of content it can switch to mode with a lower
> > > refresh rate without user noticing a thing and saving power.
> > >
> > > This seamless mode switch will be triggered when user-space dispatch
> > > a atomic commit with the new mode and clears the
> > > DRM_MODE_ATOMIC_ALLOW_MODESET flag.
> > >
> > > A driver that don't implement atomic_seamless_mode_switch_check
> > > function will continue to fail in the atomic check phase with
> > > "[CRTC:%d:%s] requires full modeset" debug message.
> > > While a driver that implements it and support the seamless change
> > > between old and new mode will return 0 otherwise it should return the
> > > appropried errno.
> > >
> > > So here adding basic drm infrastructure to that be supported by i915
> > > and other drivers.
> >
> > I don't see the need for any extra infrastructure for this.
> >
> > I think we just need:
> > - fix the fastset code to not suck
>
> How would it know that only mode changed and not all other things that causes mode_changed to be set?
> For example: intel_digital_connector_atomic_check()
That's what the fastset stuff does. It checks if anything changes
that needs a full modeset or not.
>
> > - reprogram M/N during fastset
> > - calculate eDP link params using panel's highest refresh rate mode
> > to make sure we get the same link params for all refresh rates
> >
> > >
> > > Cc: Vidya Srinivas <vidya.srinivas@intel.com>
> > > Cc: Sean Paul <seanpaul@chromium.org>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 1 +
> > > drivers/gpu/drm/drm_atomic_helper.c | 16 +++++++++++++++
> > > drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
> > > include/drm/drm_crtc.h | 25 +++++++++++++++++++++++
> > > 4 files changed, 43 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 58c0283fb6b0c..21525f9f4b4c1 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -437,6 +437,7 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
> > > drm_printf(p, "\tself_refresh_active=%d\n", state->self_refresh_active);
> > > drm_printf(p, "\tplanes_changed=%d\n", state->planes_changed);
> > > drm_printf(p, "\tmode_changed=%d\n", state->mode_changed);
> > > + drm_printf(p, "\tseamless_mode_changed=%d\n", state->seamless_mode_changed);
> > > drm_printf(p, "\tactive_changed=%d\n", state->active_changed);
> > > drm_printf(p, "\tconnectors_changed=%d\n", state->connectors_changed);
> > > drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed);
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 9603193d2fa13..e6f3a966f7b86 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -631,6 +631,22 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > > drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
> > > crtc->base.id, crtc->name);
> > > new_crtc_state->mode_changed = true;
> > > +
> > > + if (!state->allow_modeset &&
> > > + crtc->funcs->atomic_seamless_mode_switch_check) {
> > > + ret = crtc->funcs->atomic_seamless_mode_switch_check(state, crtc);
> > > + if (ret == -EOPNOTSUPP) {
> > > + drm_dbg_atomic(dev, "[CRTC:%d:%s] Seamless mode switch not supported\n",
> > > + crtc->base.id, crtc->name);
> > > + return ret;
> > > + }
> > > +
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + new_crtc_state->seamless_mode_changed = true;
> > > + new_crtc_state->mode_changed = false;
> > > + }
> > > }
> > >
> > > if (old_crtc_state->enable != new_crtc_state->enable) {
> > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > index 3b6d3bdbd0996..c093073ea6e11 100644
> > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > @@ -142,6 +142,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> > > if (state->gamma_lut)
> > > drm_property_blob_get(state->gamma_lut);
> > > state->mode_changed = false;
> > > + state->seamless_mode_changed = false;
> > > state->active_changed = false;
> > > state->planes_changed = false;
> > > state->connectors_changed = false;
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index a70baea0636ca..b7ce378d679d3 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -140,6 +140,16 @@ struct drm_crtc_state {
> > > */
> > > bool mode_changed : 1;
> > >
> > > + /**
> > > + * @seamless_mode_changed: @mode has been changed but user-space
> > > + * is requesting to change to the new mode with a fastset and driver
> > > + * supports this request.
> > > + * To be used by drivers to steer the atomic commit control flow to
> > > + * appropriate paths to change mode without any visual corruption.
> > > + * Never set together with @mode_changed.
> > > + */
> > > + bool seamless_mode_changed : 1;
> > > +
> > > /**
> > > * @active_changed: @active has been toggled. Used by the atomic
> > > * helpers and drivers to steer the atomic commit control flow. See also
> > > @@ -939,6 +949,21 @@ struct drm_crtc_funcs {
> > > int *max_error,
> > > ktime_t *vblank_time,
> > > bool in_vblank_irq);
> > > +
> > > + /**
> > > + * @atomic_seamless_mode_switch_check
> > > + *
> > > + * Called when user-space wants to change mode without do a modeset.
> > > + * Drivers can optionally support do a mode switch without any visual
> > > + * corruption when changing between certain modes.
> > > + *
> > > + * Returns:
> > > + * Zero if possible to seamless switch mode, -EOPNOTSUPP if not
> > > + * supported seamless mode change or appropriate errno if an error
> > > + * happened.
> > > + */
> > > + int (*atomic_seamless_mode_switch_check)(struct drm_atomic_state *state,
> > > + struct drm_crtc *crtc);
> > > };
> > >
> > > /**
> > > --
> > > 2.36.0
> >
>
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 9+ messages in thread