| Message ID | 20240605102553.187309-12-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Panel Replay eDP support | expand |
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Wednesday, June 5, 2024 3:56 PM > To: intel-gfx@lists.freedesktop.org > Cc: Manna, Animesh <animesh.manna@intel.com>; Kahola, Mika > <mika.kahola@intel.com>; Hogander, Jouni <jouni.hogander@intel.com> > Subject: [PATCH v6 11/26] drm/i915/psr: Move vblank length check to > separate function > > We are about to add more complexity to vblank length check. It makes sense > to move it to separate function for sake of clarity. > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index 3530e5f44096..23c3fed1f983 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1243,6 +1243,20 @@ static int intel_psr_entry_setup_frames(struct > intel_dp *intel_dp, > return entry_setup_frames; > } > > +static bool vblank_length_valid(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state) { As this function specific to psr2, maybe good to have name as psr2_vblank_length_valid(). Otherwise the changes looks ok to me. Regards, Animesh > + int vblank = crtc_state->hw.adjusted_mode.crtc_vblank_end - > + crtc_state->hw.adjusted_mode.crtc_vblank_start; > + int wake_lines = psr2_block_count_lines(intel_dp); > + > + /* Vblank >= PSR2_CTL Block Count Number maximum line count */ > + if (vblank < wake_lines) > + return false; > + > + return true; > +} > + > static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > struct intel_crtc_state *crtc_state) { @@ - > 1333,9 +1347,7 @@ static bool intel_psr2_config_valid(struct intel_dp > *intel_dp, > } > > /* Vblank >= PSR2_CTL Block Count Number maximum line count */ > - if (crtc_state->hw.adjusted_mode.crtc_vblank_end - > - crtc_state->hw.adjusted_mode.crtc_vblank_start < > - psr2_block_count_lines(intel_dp)) { > + if (!vblank_length_valid(intel_dp, crtc_state)) { > drm_dbg_kms(&dev_priv->drm, > "PSR2 not enabled, too short vblank time\n"); > return false; > -- > 2.34.1
On Thu, 2024-06-06 at 14:58 +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Hogander, Jouni <jouni.hogander@intel.com> > > Sent: Wednesday, June 5, 2024 3:56 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Manna, Animesh <animesh.manna@intel.com>; Kahola, Mika > > <mika.kahola@intel.com>; Hogander, Jouni <jouni.hogander@intel.com> > > Subject: [PATCH v6 11/26] drm/i915/psr: Move vblank length check to > > separate function > > > > We are about to add more complexity to vblank length check. It > > makes sense > > to move it to separate function for sake of clarity. > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index 3530e5f44096..23c3fed1f983 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -1243,6 +1243,20 @@ static int > > intel_psr_entry_setup_frames(struct > > intel_dp *intel_dp, > > return entry_setup_frames; > > } > > > > +static bool vblank_length_valid(struct intel_dp *intel_dp, > > + const struct intel_crtc_state > > *crtc_state) { > > As this function specific to psr2, maybe good to have name as > psr2_vblank_length_valid(). Otherwise the changes looks ok to me. Please check patch 19. That is actually moving this to be common for Panel Replay and PSR. BR, Jouni Högander > > Regards, > Animesh > > + int vblank = crtc_state->hw.adjusted_mode.crtc_vblank_end - > > + crtc_state->hw.adjusted_mode.crtc_vblank_start; > > + int wake_lines = psr2_block_count_lines(intel_dp); > > + > > + /* Vblank >= PSR2_CTL Block Count Number maximum line count > > */ > > + if (vblank < wake_lines) > > + return false; > > + > > + return true; > > +} > > + > > static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > > struct intel_crtc_state > > *crtc_state) { @@ - > > 1333,9 +1347,7 @@ static bool intel_psr2_config_valid(struct > > intel_dp > > *intel_dp, > > } > > > > /* Vblank >= PSR2_CTL Block Count Number maximum line count > > */ > > - if (crtc_state->hw.adjusted_mode.crtc_vblank_end - > > - crtc_state->hw.adjusted_mode.crtc_vblank_start < > > - psr2_block_count_lines(intel_dp)) { > > + if (!vblank_length_valid(intel_dp, crtc_state)) { > > drm_dbg_kms(&dev_priv->drm, > > "PSR2 not enabled, too short vblank > > time\n"); > > return false; > > -- > > 2.34.1 >
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Thursday, June 6, 2024 9:12 PM > To: Manna, Animesh <animesh.manna@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Kahola, Mika <mika.kahola@intel.com> > Subject: Re: [PATCH v6 11/26] drm/i915/psr: Move vblank length check to > separate function > > On Thu, 2024-06-06 at 14:58 +0000, Manna, Animesh wrote: > > > > > > > -----Original Message----- > > > From: Hogander, Jouni <jouni.hogander@intel.com> > > > Sent: Wednesday, June 5, 2024 3:56 PM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Manna, Animesh <animesh.manna@intel.com>; Kahola, Mika > > > <mika.kahola@intel.com>; Hogander, Jouni <jouni.hogander@intel.com> > > > Subject: [PATCH v6 11/26] drm/i915/psr: Move vblank length check to > > > separate function > > > > > > We are about to add more complexity to vblank length check. It makes > > > sense to move it to separate function for sake of clarity. > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 18 +++++++++++++++--- > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 3530e5f44096..23c3fed1f983 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -1243,6 +1243,20 @@ static int > > > intel_psr_entry_setup_frames(struct > > > intel_dp *intel_dp, > > > return entry_setup_frames; > > > } > > > > > > +static bool vblank_length_valid(struct intel_dp *intel_dp, > > > + const struct intel_crtc_state > > > *crtc_state) { > > > > As this function specific to psr2, maybe good to have name as > > psr2_vblank_length_valid(). Otherwise the changes looks ok to me. > > Please check patch 19. That is actually moving this to be common for Panel > Replay and PSR. How about su_vblank_length_valid() ? this function is specific to psr2/pr and the name sounds generic to me. Regards, Animesh > > BR, > > Jouni Högander > > > > > Regards, > > Animesh > > > + int vblank = crtc_state->hw.adjusted_mode.crtc_vblank_end - > > > + crtc_state->hw.adjusted_mode.crtc_vblank_start; > > > + int wake_lines = psr2_block_count_lines(intel_dp); > > > + > > > + /* Vblank >= PSR2_CTL Block Count Number maximum line count > > > */ > > > + if (vblank < wake_lines) > > > + return false; > > > + > > > + return true; > > > +} > > > + > > > static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > > > struct intel_crtc_state > > > *crtc_state) { @@ - > > > 1333,9 +1347,7 @@ static bool intel_psr2_config_valid(struct > > > intel_dp *intel_dp, > > > } > > > > > > /* Vblank >= PSR2_CTL Block Count Number maximum line count > > > */ > > > - if (crtc_state->hw.adjusted_mode.crtc_vblank_end - > > > - crtc_state->hw.adjusted_mode.crtc_vblank_start < > > > - psr2_block_count_lines(intel_dp)) { > > > + if (!vblank_length_valid(intel_dp, crtc_state)) { > > > drm_dbg_kms(&dev_priv->drm, > > > "PSR2 not enabled, too short vblank > > > time\n"); > > > return false; > > > -- > > > 2.34.1 > >
On Fri, 2024-06-07 at 11:09 +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Hogander, Jouni <jouni.hogander@intel.com> > > Sent: Thursday, June 6, 2024 9:12 PM > > To: Manna, Animesh <animesh.manna@intel.com>; intel- > > gfx@lists.freedesktop.org > > Cc: Kahola, Mika <mika.kahola@intel.com> > > Subject: Re: [PATCH v6 11/26] drm/i915/psr: Move vblank length > > check to > > separate function > > > > On Thu, 2024-06-06 at 14:58 +0000, Manna, Animesh wrote: > > > > > > > > > > -----Original Message----- > > > > From: Hogander, Jouni <jouni.hogander@intel.com> > > > > Sent: Wednesday, June 5, 2024 3:56 PM > > > > To: intel-gfx@lists.freedesktop.org > > > > Cc: Manna, Animesh <animesh.manna@intel.com>; Kahola, Mika > > > > <mika.kahola@intel.com>; Hogander, Jouni > > > > <jouni.hogander@intel.com> > > > > Subject: [PATCH v6 11/26] drm/i915/psr: Move vblank length > > > > check to > > > > separate function > > > > > > > > We are about to add more complexity to vblank length check. It > > > > makes > > > > sense to move it to separate function for sake of clarity. > > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_psr.c | 18 +++++++++++++++- > > > > -- > > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > > index 3530e5f44096..23c3fed1f983 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > @@ -1243,6 +1243,20 @@ static int > > > > intel_psr_entry_setup_frames(struct > > > > intel_dp *intel_dp, > > > > return entry_setup_frames; > > > > } > > > > > > > > +static bool vblank_length_valid(struct intel_dp *intel_dp, > > > > + const struct intel_crtc_state > > > > *crtc_state) { > > > > > > As this function specific to psr2, maybe good to have name as > > > psr2_vblank_length_valid(). Otherwise the changes looks ok to me. > > > > Please check patch 19. That is actually moving this to be common > > for Panel > > Replay and PSR. > > How about su_vblank_length_valid() ? this function is specific to > psr2/pr and the name sounds generic to me. Ok, I will try to figure out something else... BR, Jouni Högander > > Regards, > Animesh > > > > > BR, > > > > Jouni Högander > > > > > > > > Regards, > > > Animesh > > > > + int vblank = crtc_state- > > > > >hw.adjusted_mode.crtc_vblank_end - > > > > + crtc_state->hw.adjusted_mode.crtc_vblank_start; > > > > + int wake_lines = psr2_block_count_lines(intel_dp); > > > > + > > > > + /* Vblank >= PSR2_CTL Block Count Number maximum line > > > > count > > > > */ > > > > + if (vblank < wake_lines) > > > > + return false; > > > > + > > > > + return true; > > > > +} > > > > + > > > > static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > > > > struct intel_crtc_state > > > > *crtc_state) { @@ - > > > > 1333,9 +1347,7 @@ static bool intel_psr2_config_valid(struct > > > > intel_dp *intel_dp, > > > > } > > > > > > > > /* Vblank >= PSR2_CTL Block Count Number maximum line > > > > count > > > > */ > > > > - if (crtc_state->hw.adjusted_mode.crtc_vblank_end - > > > > - crtc_state->hw.adjusted_mode.crtc_vblank_start < > > > > - psr2_block_count_lines(intel_dp)) { > > > > + if (!vblank_length_valid(intel_dp, crtc_state)) { > > > > drm_dbg_kms(&dev_priv->drm, > > > > "PSR2 not enabled, too short vblank > > > > time\n"); > > > > return false; > > > > -- > > > > 2.34.1 > > > >
On Fri, 2024-06-07 at 16:19 +0300, Hogander, Jouni wrote: > On Fri, 2024-06-07 at 11:09 +0000, Manna, Animesh wrote: > > > > > > > -----Original Message----- > > > From: Hogander, Jouni <jouni.hogander@intel.com> > > > Sent: Thursday, June 6, 2024 9:12 PM > > > To: Manna, Animesh <animesh.manna@intel.com>; intel- > > > gfx@lists.freedesktop.org > > > Cc: Kahola, Mika <mika.kahola@intel.com> > > > Subject: Re: [PATCH v6 11/26] drm/i915/psr: Move vblank length > > > check to > > > separate function > > > > > > On Thu, 2024-06-06 at 14:58 +0000, Manna, Animesh wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: Hogander, Jouni <jouni.hogander@intel.com> > > > > > Sent: Wednesday, June 5, 2024 3:56 PM > > > > > To: intel-gfx@lists.freedesktop.org > > > > > Cc: Manna, Animesh <animesh.manna@intel.com>; Kahola, Mika > > > > > <mika.kahola@intel.com>; Hogander, Jouni > > > > > <jouni.hogander@intel.com> > > > > > Subject: [PATCH v6 11/26] drm/i915/psr: Move vblank length > > > > > check to > > > > > separate function > > > > > > > > > > We are about to add more complexity to vblank length check. > > > > > It > > > > > makes > > > > > sense to move it to separate function for sake of clarity. > > > > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 18 > > > > > +++++++++++++++- > > > > > -- > > > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > index 3530e5f44096..23c3fed1f983 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > @@ -1243,6 +1243,20 @@ static int > > > > > intel_psr_entry_setup_frames(struct > > > > > intel_dp *intel_dp, > > > > > return entry_setup_frames; > > > > > } > > > > > > > > > > +static bool vblank_length_valid(struct intel_dp *intel_dp, > > > > > + const struct intel_crtc_state > > > > > *crtc_state) { > > > > > > > > As this function specific to psr2, maybe good to have name as > > > > psr2_vblank_length_valid(). Otherwise the changes looks ok to > > > > me. > > > > > > Please check patch 19. That is actually moving this to be common > > > for Panel > > > Replay and PSR. > > > > How about su_vblank_length_valid() ? this function is specific to > > psr2/pr and the name sounds generic to me. > > Ok, I will try to figure out something else... This actually revealed that patch 19 is wrong. This is not SU specific. We should check this for eDP PR full frame update as well. I will take care of fixing patch 19. Here I will change name to wake_lines_fit_into_vblank. BR, Jouni Högander > > BR, > > Jouni Högander > > > > > Regards, > > Animesh > > > > > > > > BR, > > > > > > Jouni Högander > > > > > > > > > > > Regards, > > > > Animesh > > > > > + int vblank = crtc_state- > > > > > > hw.adjusted_mode.crtc_vblank_end - > > > > > + crtc_state- > > > > > >hw.adjusted_mode.crtc_vblank_start; > > > > > + int wake_lines = psr2_block_count_lines(intel_dp); > > > > > + > > > > > + /* Vblank >= PSR2_CTL Block Count Number maximum line > > > > > count > > > > > */ > > > > > + if (vblank < wake_lines) > > > > > + return false; > > > > > + > > > > > + return true; > > > > > +} > > > > > + > > > > > static bool intel_psr2_config_valid(struct intel_dp > > > > > *intel_dp, > > > > > struct intel_crtc_state > > > > > *crtc_state) { @@ - > > > > > 1333,9 +1347,7 @@ static bool intel_psr2_config_valid(struct > > > > > intel_dp *intel_dp, > > > > > } > > > > > > > > > > /* Vblank >= PSR2_CTL Block Count Number maximum line > > > > > count > > > > > */ > > > > > - if (crtc_state->hw.adjusted_mode.crtc_vblank_end - > > > > > - crtc_state->hw.adjusted_mode.crtc_vblank_start < > > > > > - psr2_block_count_lines(intel_dp)) { > > > > > + if (!vblank_length_valid(intel_dp, crtc_state)) { > > > > > drm_dbg_kms(&dev_priv->drm, > > > > > "PSR2 not enabled, too short > > > > > vblank > > > > > time\n"); > > > > > return false; > > > > > -- > > > > > 2.34.1 > > > > > > >
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 3530e5f44096..23c3fed1f983 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1243,6 +1243,20 @@ static int intel_psr_entry_setup_frames(struct intel_dp *intel_dp, return entry_setup_frames; } +static bool vblank_length_valid(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state) +{ + int vblank = crtc_state->hw.adjusted_mode.crtc_vblank_end - + crtc_state->hw.adjusted_mode.crtc_vblank_start; + int wake_lines = psr2_block_count_lines(intel_dp); + + /* Vblank >= PSR2_CTL Block Count Number maximum line count */ + if (vblank < wake_lines) + return false; + + return true; +} + static bool intel_psr2_config_valid(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state) { @@ -1333,9 +1347,7 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, } /* Vblank >= PSR2_CTL Block Count Number maximum line count */ - if (crtc_state->hw.adjusted_mode.crtc_vblank_end - - crtc_state->hw.adjusted_mode.crtc_vblank_start < - psr2_block_count_lines(intel_dp)) { + if (!vblank_length_valid(intel_dp, crtc_state)) { drm_dbg_kms(&dev_priv->drm, "PSR2 not enabled, too short vblank time\n"); return false;
We are about to add more complexity to vblank length check. It makes sense to move it to separate function for sake of clarity. Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)