Re: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters struct
On Wed, 2024-01-24 at 09:28 +, Murthy, Arun R wrote: > > > > -Original Message- > > From: Hogander, Jouni > > Sent: Wednesday, January 24, 2024 2:55 PM > > To: Murthy, Arun R ; > > intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters > > struct > > > > On Wed, 2024-01-24 at 09:17 +, Murthy, Arun R wrote: > > > > > > > > > > -Original Message- > > > > From: Intel-gfx On > > > > Behalf > > > > Of Jouni Högander > > > > Sent: Friday, January 5, 2024 7:45 PM > > > > To: intel-gfx@lists.freedesktop.org > > > > Subject: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters > > > > struct > > > > > > > > Add new alpm_parameters struct into intel_psr for all > > > > calculated > > > > alpm parameters. > > > > > > > > Signed-off-by: Jouni Högander > > > > --- > > > > .../drm/i915/display/intel_display_types.h | 8 -- > > > > drivers/gpu/drm/i915/display/intel_psr.c | 28 ++- > > > > > > > > > > > > 2 files changed, 21 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > index b9b9d9f2bc0b..889a8b34b7ac 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > @@ -1677,6 +1677,11 @@ struct intel_pps { > > > > struct edp_power_seq bios_pps_delays; > > > > }; > > > > > > > > +struct alpm_parameters { > > > > + u8 io_wake_lines; > > > > + u8 fast_wake_lines; > > > > +}; > > > Assume that this is being used in PSR as well and can be retained > > > in > > > intel_psr struct. > > > If exclusively used for alpm then having it in a new alpm struct > > > would > > > be better. > > > > Amount of these parameters are about to grow with AUXless ALPM. I > > was > > thinking having own struct would be more clear. Please note that > > alpm_params > > is still under struct intel_psr. What do you think? > > > Understand that his is quite a big feature and this struct tends to > grow in the coming patches. But w.r.t this variable since it was used > in psr and not exclusively for alpm felt so. I would leave it for > others in the community to add their views on this. I don't have that strong opinions on this. I can change it back. BR, Jouni Högander > > Thanks and Regards, > Arun R Murthy > --- > > BR, > > > > Jouni Högander > > > > > > Thanks and Regards, > > > Arun R Murthy > > > --- > > > > + > > > > struct intel_psr { > > > > /* Mutex for PSR state of the transcoder */ > > > > struct mutex lock; > > > > @@ -1706,8 +1711,6 @@ struct intel_psr { > > > > bool psr2_sel_fetch_cff_enabled; > > > > bool req_psr2_sdp_prior_scanline; > > > > u8 sink_sync_latency; > > > > - u8 io_wake_lines; > > > > - u8 fast_wake_lines; > > > > ktime_t last_entry_attempt; > > > > ktime_t last_exit; > > > > bool sink_not_reliable; > > > > @@ -1721,6 +1724,7 @@ struct intel_psr { > > > > u32 dc3co_exit_delay; > > > > struct delayed_work dc3co_work; > > > > u8 entry_setup_frames; > > > > + struct alpm_parameters alpm_params; > > > > }; > > > > > > > > struct intel_dp { > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > > index 54120b45958b..1709ebb31215 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > @@ -759,8 +759,8 @@ static u32 intel_psr2_get_tp_time(struct > > > > intel_dp > > > > *intel_dp) > > > > > > > > static int psr2_block_count_lines(struct intel_dp *intel_dp) > > > > { > > > > - return intel_dp->psr.io_wake_lines < 9 &&a
RE: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters struct
> -Original Message- > From: Hogander, Jouni > Sent: Wednesday, January 24, 2024 2:55 PM > To: Murthy, Arun R ; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters struct > > On Wed, 2024-01-24 at 09:17 +, Murthy, Arun R wrote: > > > > > > > -Original Message- > > > From: Intel-gfx On Behalf > > > Of Jouni Högander > > > Sent: Friday, January 5, 2024 7:45 PM > > > To: intel-gfx@lists.freedesktop.org > > > Subject: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters struct > > > > > > Add new alpm_parameters struct into intel_psr for all calculated > > > alpm parameters. > > > > > > Signed-off-by: Jouni Högander > > > --- > > > .../drm/i915/display/intel_display_types.h | 8 -- > > > drivers/gpu/drm/i915/display/intel_psr.c | 28 ++- > > > > > > 2 files changed, 21 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index b9b9d9f2bc0b..889a8b34b7ac 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -1677,6 +1677,11 @@ struct intel_pps { > > > struct edp_power_seq bios_pps_delays; > > > }; > > > > > > +struct alpm_parameters { > > > + u8 io_wake_lines; > > > + u8 fast_wake_lines; > > > +}; > > Assume that this is being used in PSR as well and can be retained in > > intel_psr struct. > > If exclusively used for alpm then having it in a new alpm struct would > > be better. > > Amount of these parameters are about to grow with AUXless ALPM. I was > thinking having own struct would be more clear. Please note that alpm_params > is still under struct intel_psr. What do you think? > Understand that his is quite a big feature and this struct tends to grow in the coming patches. But w.r.t this variable since it was used in psr and not exclusively for alpm felt so. I would leave it for others in the community to add their views on this. Thanks and Regards, Arun R Murthy --- > BR, > > Jouni Högander > > > > Thanks and Regards, > > Arun R Murthy > > --- > > > + > > > struct intel_psr { > > > /* Mutex for PSR state of the transcoder */ > > > struct mutex lock; > > > @@ -1706,8 +1711,6 @@ struct intel_psr { > > > bool psr2_sel_fetch_cff_enabled; > > > bool req_psr2_sdp_prior_scanline; > > > u8 sink_sync_latency; > > > - u8 io_wake_lines; > > > - u8 fast_wake_lines; > > > ktime_t last_entry_attempt; > > > ktime_t last_exit; > > > bool sink_not_reliable; > > > @@ -1721,6 +1724,7 @@ struct intel_psr { > > > u32 dc3co_exit_delay; > > > struct delayed_work dc3co_work; > > > u8 entry_setup_frames; > > > + struct alpm_parameters alpm_params; > > > }; > > > > > > struct intel_dp { > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 54120b45958b..1709ebb31215 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -759,8 +759,8 @@ static u32 intel_psr2_get_tp_time(struct > > > intel_dp > > > *intel_dp) > > > > > > static int psr2_block_count_lines(struct intel_dp *intel_dp) { > > > - return intel_dp->psr.io_wake_lines < 9 && > > > - intel_dp->psr.fast_wake_lines < 9 ? 8 : 12; > > > + return intel_dp->psr.alpm_params.io_wake_lines < 9 && > > > + intel_dp->psr.alpm_params.fast_wake_lines < 9 ? 8 : > > > 12; > > > } > > > > > > static int psr2_block_count(struct intel_dp *intel_dp) @@ -797,6 > > > +797,7 @@ > > > static void dg2_activate_panel_replay(struct intel_dp *intel_dp) > > > static void hsw_activate_psr2(struct intel_dp *intel_dp) { > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > + struct alpm_parameters *alpm_params = _dp- > > > >psr.alpm_params; > > > enum transcoder cpu_transcoder = intel_dp->psr
Re: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters struct
On Wed, 2024-01-24 at 09:17 +, Murthy, Arun R wrote: > > > > -Original Message- > > From: Intel-gfx On Behalf > > Of Jouni > > Högander > > Sent: Friday, January 5, 2024 7:45 PM > > To: intel-gfx@lists.freedesktop.org > > Subject: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters struct > > > > Add new alpm_parameters struct into intel_psr for all calculated > > alpm > > parameters. > > > > Signed-off-by: Jouni Högander > > --- > > .../drm/i915/display/intel_display_types.h | 8 -- > > drivers/gpu/drm/i915/display/intel_psr.c | 28 ++- > > > > 2 files changed, 21 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index b9b9d9f2bc0b..889a8b34b7ac 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1677,6 +1677,11 @@ struct intel_pps { > > struct edp_power_seq bios_pps_delays; > > }; > > > > +struct alpm_parameters { > > + u8 io_wake_lines; > > + u8 fast_wake_lines; > > +}; > Assume that this is being used in PSR as well and can be retained in > intel_psr struct. > If exclusively used for alpm then having it in a new alpm struct > would be better. Amount of these parameters are about to grow with AUXless ALPM. I was thinking having own struct would be more clear. Please note that alpm_params is still under struct intel_psr. What do you think? BR, Jouni Högander > > Thanks and Regards, > Arun R Murthy > --- > > + > > struct intel_psr { > > /* Mutex for PSR state of the transcoder */ > > struct mutex lock; > > @@ -1706,8 +1711,6 @@ struct intel_psr { > > bool psr2_sel_fetch_cff_enabled; > > bool req_psr2_sdp_prior_scanline; > > u8 sink_sync_latency; > > - u8 io_wake_lines; > > - u8 fast_wake_lines; > > ktime_t last_entry_attempt; > > ktime_t last_exit; > > bool sink_not_reliable; > > @@ -1721,6 +1724,7 @@ struct intel_psr { > > u32 dc3co_exit_delay; > > struct delayed_work dc3co_work; > > u8 entry_setup_frames; > > + struct alpm_parameters alpm_params; > > }; > > > > struct intel_dp { > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index 54120b45958b..1709ebb31215 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -759,8 +759,8 @@ static u32 intel_psr2_get_tp_time(struct > > intel_dp > > *intel_dp) > > > > static int psr2_block_count_lines(struct intel_dp *intel_dp) { > > - return intel_dp->psr.io_wake_lines < 9 && > > - intel_dp->psr.fast_wake_lines < 9 ? 8 : 12; > > + return intel_dp->psr.alpm_params.io_wake_lines < 9 && > > + intel_dp->psr.alpm_params.fast_wake_lines < 9 ? 8 : > > 12; > > } > > > > static int psr2_block_count(struct intel_dp *intel_dp) @@ -797,6 > > +797,7 @@ > > static void dg2_activate_panel_replay(struct intel_dp *intel_dp) > > static void > > hsw_activate_psr2(struct intel_dp *intel_dp) { > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + struct alpm_parameters *alpm_params = _dp- > > >psr.alpm_params; > > enum transcoder cpu_transcoder = intel_dp->psr.transcoder; > > u32 val = EDP_PSR2_ENABLE; > > u32 psr_val = 0; > > @@ -838,17 +839,17 @@ static void hsw_activate_psr2(struct intel_dp > > *intel_dp) > > */ > > int tmp; > > > > - tmp = map[intel_dp->psr.io_wake_lines - > > TGL_EDP_PSR2_IO_BUFFER_WAKE_MIN_LINES]; > > + tmp = map[alpm_params->io_wake_lines - > > +TGL_EDP_PSR2_IO_BUFFER_WAKE_MIN_LINES]; > > val |= TGL_EDP_PSR2_IO_BUFFER_WAKE(tmp + > > TGL_EDP_PSR2_IO_BUFFER_WAKE_MIN_LINES); > > > > - tmp = map[intel_dp->psr.fast_wake_lines - > > TGL_EDP_PSR2_FAST_WAKE_MIN_LINES]; > > + tmp = map[alpm_params->fast_wake_lines - > > +TGL_EDP_PSR2_FAST_WAKE_MIN_LINES]; > > val |= TGL_EDP_PSR2_FAST_WAKE(tmp + > > TGL_EDP_PSR2_FAST_WAKE_MIN_LINES); > > } else if (DISPLAY_VER(dev_priv) >= 12) { > > - val |= TGL_EDP_PSR2_IO_BUFFER_WAKE(intel_dp- > > > psr.io_wake_lines); > > - val |= TGL_EDP_PSR2_FAST_WAKE(intel_dp- > > > psr.fast_wake_lines); > > + val |= TGL_EDP_PSR2_IO_BUFFER_WAKE(alpm_params- > > > io_wake_lines); > > + val |= TGL_EDP_PSR2_FAST_WAKE(alpm_params- > > > fast_wake_lines); > > } else if (DISPLAY_VER(dev_priv) >= 9) { > > - val |= EDP_PSR2_IO_BUFFER_WAKE(intel_dp- > > > psr.io_wake_lines); > > - val |= EDP_PSR2_FAST_WAKE(intel_dp- > > >psr.fast_wake_lines); > > + val |= EDP_PSR2_IO_BUFFER_WAKE(alpm_params- > > > io_wake_lines); > >
RE: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters struct
> -Original Message- > From: Intel-gfx On Behalf Of Jouni > Högander > Sent: Friday, January 5, 2024 7:45 PM > To: intel-gfx@lists.freedesktop.org > Subject: [PATCH v2 2/4] drm/i915/psr: Add alpm_parameters struct > > Add new alpm_parameters struct into intel_psr for all calculated alpm > parameters. > > Signed-off-by: Jouni Högander > --- > .../drm/i915/display/intel_display_types.h| 8 -- > drivers/gpu/drm/i915/display/intel_psr.c | 28 ++- > 2 files changed, 21 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index b9b9d9f2bc0b..889a8b34b7ac 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1677,6 +1677,11 @@ struct intel_pps { > struct edp_power_seq bios_pps_delays; > }; > > +struct alpm_parameters { > + u8 io_wake_lines; > + u8 fast_wake_lines; > +}; Assume that this is being used in PSR as well and can be retained in intel_psr struct. If exclusively used for alpm then having it in a new alpm struct would be better. Thanks and Regards, Arun R Murthy --- > + > struct intel_psr { > /* Mutex for PSR state of the transcoder */ > struct mutex lock; > @@ -1706,8 +1711,6 @@ struct intel_psr { > bool psr2_sel_fetch_cff_enabled; > bool req_psr2_sdp_prior_scanline; > u8 sink_sync_latency; > - u8 io_wake_lines; > - u8 fast_wake_lines; > ktime_t last_entry_attempt; > ktime_t last_exit; > bool sink_not_reliable; > @@ -1721,6 +1724,7 @@ struct intel_psr { > u32 dc3co_exit_delay; > struct delayed_work dc3co_work; > u8 entry_setup_frames; > + struct alpm_parameters alpm_params; > }; > > struct intel_dp { > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index 54120b45958b..1709ebb31215 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -759,8 +759,8 @@ static u32 intel_psr2_get_tp_time(struct intel_dp > *intel_dp) > > static int psr2_block_count_lines(struct intel_dp *intel_dp) { > - return intel_dp->psr.io_wake_lines < 9 && > - intel_dp->psr.fast_wake_lines < 9 ? 8 : 12; > + return intel_dp->psr.alpm_params.io_wake_lines < 9 && > + intel_dp->psr.alpm_params.fast_wake_lines < 9 ? 8 : 12; > } > > static int psr2_block_count(struct intel_dp *intel_dp) @@ -797,6 +797,7 @@ > static void dg2_activate_panel_replay(struct intel_dp *intel_dp) static void > hsw_activate_psr2(struct intel_dp *intel_dp) { > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + struct alpm_parameters *alpm_params = _dp->psr.alpm_params; > enum transcoder cpu_transcoder = intel_dp->psr.transcoder; > u32 val = EDP_PSR2_ENABLE; > u32 psr_val = 0; > @@ -838,17 +839,17 @@ static void hsw_activate_psr2(struct intel_dp > *intel_dp) >*/ > int tmp; > > - tmp = map[intel_dp->psr.io_wake_lines - > TGL_EDP_PSR2_IO_BUFFER_WAKE_MIN_LINES]; > + tmp = map[alpm_params->io_wake_lines - > +TGL_EDP_PSR2_IO_BUFFER_WAKE_MIN_LINES]; > val |= TGL_EDP_PSR2_IO_BUFFER_WAKE(tmp + > TGL_EDP_PSR2_IO_BUFFER_WAKE_MIN_LINES); > > - tmp = map[intel_dp->psr.fast_wake_lines - > TGL_EDP_PSR2_FAST_WAKE_MIN_LINES]; > + tmp = map[alpm_params->fast_wake_lines - > +TGL_EDP_PSR2_FAST_WAKE_MIN_LINES]; > val |= TGL_EDP_PSR2_FAST_WAKE(tmp + > TGL_EDP_PSR2_FAST_WAKE_MIN_LINES); > } else if (DISPLAY_VER(dev_priv) >= 12) { > - val |= TGL_EDP_PSR2_IO_BUFFER_WAKE(intel_dp- > >psr.io_wake_lines); > - val |= TGL_EDP_PSR2_FAST_WAKE(intel_dp- > >psr.fast_wake_lines); > + val |= TGL_EDP_PSR2_IO_BUFFER_WAKE(alpm_params- > >io_wake_lines); > + val |= TGL_EDP_PSR2_FAST_WAKE(alpm_params- > >fast_wake_lines); > } else if (DISPLAY_VER(dev_priv) >= 9) { > - val |= EDP_PSR2_IO_BUFFER_WAKE(intel_dp- > >psr.io_wake_lines); > - val |= EDP_PSR2_FAST_WAKE(intel_dp->psr.fast_wake_lines); > + val |= EDP_PSR2_IO_BUFFER_WAKE(alpm_params- > >io_wake_lines); > + val |= EDP_PSR2_FAST_WAKE(alpm_params->fast_wake_lines); > } > > if (intel_dp->psr.req_psr2_sdp_prior_scanline) > @@ -1098,10 +1099,11 @@ static bool > _compute_psr2_sdp_prior_scanline_indication(struct intel_dp *intel_d > return true; > } > > -static bool _compute_psr2_wake_times(struct intel_dp *intel_dp, > - struct intel_crtc_state *crtc_state) > +static bool _compute_alpm_params(struct intel_dp *intel_dp, > + struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + struct