Re: [PATCH v6 05/26] drm/display: Add missing Panel Replay Enable SU Region ET bit
On Wed, 2024-06-05 at 13:31 +0300, Jouni Högander wrote: > Add missing Panel Replay Enable SU Region ET bit defined in DP2.1 > specification. Hello drm-core maintainers, Could you please consider providing your ack on this patch? I'm planning to merge it via drm-intel tree. I have already r-b tag. BR, Jouni Högander > > Signed-off-by: Jouni Högander > --- > include/drm/display/drm_dp.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/drm/display/drm_dp.h > b/include/drm/display/drm_dp.h > index f246fa03a3cb..173548c6473a 100644 > --- a/include/drm/display/drm_dp.h > +++ b/include/drm/display/drm_dp.h > @@ -743,6 +743,7 @@ > # define DP_PANEL_REPLAY_RFB_STORAGE_ERROR_EN (1 << 4) > # define DP_PANEL_REPLAY_ACTIVE_FRAME_CRC_ERROR_EN (1 << 5) > # define DP_PANEL_REPLAY_SU_ENABLE (1 << 6) > +# define DP_PANEL_REPLAY_ENABLE_SU_REGION_ET (1 << 7) /* > DP 2.1 */ > > #define PANEL_REPLAY_CONFIG2 > 0x1b1 /* eDP 1.5 */ > # define DP_PANEL_REPLAY_SINK_REFRESH_RATE_UNLOCK_GRANTED (1 > << 0)
Re: [PATCH 02/17] drm/panel replay: Add edp1.5 Panel Replay bits and register
On Thu, 2024-05-16 at 11:53 +0300, Jouni Högander wrote: > Add PANEL_REPLAY_CONFIGURATION_2 register and some missing Panel > Replay > bits. Hello drm-core maintainers, Could you please consider providing your ack on this patch? I'm planning to merge it via drm-intel tree. I have already r-b tag. BR, Jouni Högander > > Signed-off-by: Jouni Högander > --- > include/drm/display/drm_dp.h | 16 +--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/include/drm/display/drm_dp.h > b/include/drm/display/drm_dp.h > index 906949ca3cee..79bde372b152 100644 > --- a/include/drm/display/drm_dp.h > +++ b/include/drm/display/drm_dp.h > @@ -544,9 +544,10 @@ > /* DFP Capability Extension */ > #define DP_DFP_CAPABILITY_EXTENSION_SUPPORT0x0a3 /* 2.0 */ > > -#define DP_PANEL_REPLAY_CAP 0x0b0 /* DP 2.0 */ > -# define DP_PANEL_REPLAY_SUPPORT (1 << 0) > -# define DP_PANEL_REPLAY_SU_SUPPORT (1 << 1) > +#define DP_PANEL_REPLAY_CAP0x0b0 /* DP > 2.0 */ > +# define DP_PANEL_REPLAY_SUPPORT (1 << 0) > +# define DP_PANEL_REPLAY_SU_SUPPORT(1 << 1) > +# define DP_PANEL_REPLAY_EARLY_TRANSPORT_SUPPORT (1 << 2) /* > eDP 1.5 */ > > #define DP_PANEL_PANEL_REPLAY_CAPABILITY 0xb1 > # define DP_PANEL_PANEL_REPLAY_SU_GRANULARITY_REQUIRED (1 << 5) > @@ -734,11 +735,20 @@ > > #define PANEL_REPLAY_CONFIG 0x1b0 /* DP > 2.0 */ > # define DP_PANEL_REPLAY_ENABLE (1 << 0) > +# define DP_PANEL_REPLAY_VSC_SDP_CRC_EN (1 << 1) /* > eDP 1.5 */ > # define DP_PANEL_REPLAY_UNRECOVERABLE_ERROR_EN (1 << 3) > # define DP_PANEL_REPLAY_RFB_STORAGE_ERROR_EN (1 << 4) > # define DP_PANEL_REPLAY_ACTIVE_FRAME_CRC_ERROR_EN (1 << 5) > # define DP_PANEL_REPLAY_SU_ENABLE (1 << 6) > > +#define PANEL_REPLAY_CONFIG2 > 0x1b1 /* eDP 1.5 */ > +# define DP_PANEL_REPLAY_SINK_REFRESH_RATE_UNLOCK_GRANTED (1 > << 0) > +# define DP_PANEL_REPLAY_CRC_VERIFICATION (1 > << 1) > +# define DP_PANEL_REPLAY_SU_Y_GRANULARITY_EXTENDED_EN (1 > << 2) > +# define DP_PANEL_REPLAY_SU_Y_GRANULARITY_EXTENDED_VAL_SEL_SHIFT 3 > +# define DP_PANEL_REPLAY_SU_Y_GRANULARITY_EXTENDED_VAL_SEL_MASK > (0xf << 3) > +# define DP_PANEL_REPLAY_SU_REGION_SCANLINE_CAPTURE (1 > << 7) > + > #define DP_PAYLOAD_ALLOCATE_SET 0x1c0 > #define DP_PAYLOAD_ALLOCATE_START_TIME_SLOT 0x1c1 > #define DP_PAYLOAD_ALLOCATE_TIME_SLOT_COUNT 0x1c2
Re: [PATCH v6 0/6] Link off between frames for edp
On Mon, 2024-05-27 at 13:56 +0530, Animesh Manna wrote: > Link Off Between Active Frames (LOBF) allows an eDP link to be turned > Off and On > durning long VBLANK durations without enabling any of the PSR/PSR2/PR > modes of operation. > > Bspec: 71477 > > Note: Lobf need to be enabled adaptive sync fixed refresh mode > where vmin = vmax = flipline, which will arise after cmmr feature > enablement. Currently existing code refactored and make compute- > config() > and enabling function ready. Will add enabling sequence in a separate > patch. For the whole set: Reviewed-by: Jouni Högander Couple of points that needs some attention later: PSR code should move to use intel_alpm_aux_wake_supported and intel_alpm_aux_less_wake_supported rather than read DP_RECEIVER_ALPM_CAP again. I will probably take care of this in my Panel Replay eDP patch set. We could have common intel_alpm_compute_config which would calculate alpm_params. Then common intel_alpm_configure call for both PSR and LOBF which would write what is computed in intel_alpm_compute_config, intel_alpm_lobf_compute_config and intel_psr_compute_config. BR, Jouni Högander > > Signed-off-by: Animesh Manna > > Animesh Manna (5): > drm/i915/alpm: Move alpm parameters from intel_psr > drm/i915/alpm: Move alpm related code to a new file > drm/i915/alpm: Add compute config for lobf > drm/i915/alpm: Enable lobf from source in ALPM_CTL > drm/i915/alpm: Add debugfs for LOBF > > Jouni Högander (1): > drm/display: Add missing aux less alpm wake related bits > > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/display/intel_alpm.c | 411 > ++ > drivers/gpu/drm/i915/display/intel_alpm.h | 25 ++ > .../drm/i915/display/intel_display_debugfs.c | 2 + > .../drm/i915/display/intel_display_types.h | 26 +- > drivers/gpu/drm/i915/display/intel_dp.c | 4 + > drivers/gpu/drm/i915/display/intel_psr.c | 303 + > drivers/gpu/drm/xe/Makefile | 1 + > include/drm/display/drm_dp.h | 5 +- > 9 files changed, 475 insertions(+), 303 deletions(-) > create mode 100644 drivers/gpu/drm/i915/display/intel_alpm.c > create mode 100644 drivers/gpu/drm/i915/display/intel_alpm.h >
Re: [PATCH v10 00/12] Panel replay selective update support
On Fri, 2024-05-10 at 12:38 +0300, Jouni Högander wrote: > This patch set is implementing panel replay selective update support > for Intel hardware. These are now merged into drm-intel-next including "drm/panelreplay: dpcd register definition for panelreplay SU". Thank you Animesh and Maarten for review and ack. BR, Jouni Högander > > v10: > - use always PSR2_STATUS for eDP Panel Replay > - handle SRD_STATUS vs. PSR2_STATUS in intel_psr_wait_exit_locked > as well > v9: > - do not add has_psr check into psr2 case in > intel_dp_compute_vsc_sdp > - use drm_dp_dpcd_readb instead of drm_dp_dpcd_read in > intel_dp_get_su_capability > - ensure intel_dp_get_su_capability returns 0 if drm_dp_dpcd_readb > fails > v8: > - use correct offset for DP_PANEL_PANEL_REPLAY_CAPABILITY > v7: > - use always vsc revision 0x6 for Panel Replay > v6: > - fixes split to separate patch set > v5: > - do not use PSR2_STATUS for PSR1 > v4: > - do not rename intel_psr_enabled > - do not add sel_update_et_enabled into struct intel_psr > v3: > - do not disable panel replay by default > - set has_psr for panel replay as well > - enable sink before link training > - do not apply all PSR workarounds for panel replay > - do not write/read registers/bits not applicable for panel replay > - use psr bit definitions in granularity configuration as well > - goto unsupported instead of return when global enabled check > fails > - update module parameter descriptions. > v2: > - make psr pause/resume to work for panel replay as well > > Jouni Högander (12): > drm/i915/psr: Rename has_psr2 as has_sel_update > drm/i915/display: Do not print "psr: enabled" for on Panel Replay > drm/i915/dp: Use always vsc revision 0x6 for Panel Replay > drm/i915/psr: Rename psr2_enabled as sel_update_enabled > drm/panelreplay: dpcd register definition for panelreplay SU > drm/i915/psr: Detect panel replay selective update support > drm/i915/psr: Modify intel_dp_get_su_granularity to support panel > replay > drm/i915/psr: Panel replay uses SRD_STATUS to track it's status > drm/i915/psr: Do not apply workarounds in case of panel replay > drm/i915/psr: Update PSR module parameter descriptions > drm/i915/psr: Split intel_psr2_config_valid for panel replay > drm/i915/psr: Add panel replay sel update support to debugfs > interface > > .../drm/i915/display/intel_crtc_state_dump.c | 7 +- > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > .../drm/i915/display/intel_display_params.c | 5 +- > .../drm/i915/display/intel_display_types.h | 5 +- > drivers/gpu/drm/i915/display/intel_dp.c | 17 +- > drivers/gpu/drm/i915/display/intel_fbc.c | 5 +- > drivers/gpu/drm/i915/display/intel_hdmi.c | 3 +- > drivers/gpu/drm/i915/display/intel_psr.c | 237 > -- > include/drm/display/drm_dp.h | 6 + > 9 files changed, 194 insertions(+), 93 deletions(-) >
Re: [PATCH v10 05/12] drm/panelreplay: dpcd register definition for panelreplay SU
Hello Maintainers, Could you please ack this patch? I'm planning to merge it via drm-intel tree. BR, Jouni Högander On Fri, 2024-05-10 at 13:26 +0300, Jouni Högander wrote: > Add definitions for panel replay selective update > > v2: Remove unnecessary Cc from commit message > > Signed-off-by: Jouni Högander > Reviewed-by: Animesh Manna > --- > include/drm/display/drm_dp.h | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/include/drm/display/drm_dp.h > b/include/drm/display/drm_dp.h > index 0b032faa8cf2..906949ca3cee 100644 > --- a/include/drm/display/drm_dp.h > +++ b/include/drm/display/drm_dp.h > @@ -548,6 +548,12 @@ > # define DP_PANEL_REPLAY_SUPPORT (1 << 0) > # define DP_PANEL_REPLAY_SU_SUPPORT (1 << 1) > > +#define DP_PANEL_PANEL_REPLAY_CAPABILITY 0xb1 > +# define DP_PANEL_PANEL_REPLAY_SU_GRANULARITY_REQUIRED (1 << 5) > + > +#define DP_PANEL_PANEL_REPLAY_X_GRANULARITY0xb2 > +#define DP_PANEL_PANEL_REPLAY_Y_GRANULARITY0xb4 > + > /* Link Configuration */ > #defineDP_LINK_BW_SET 0x100 > # define DP_LINK_RATE_TABLE 0x00 /* eDP 1.4 */
Re: [PATCH v4 4/6] drm/i915/alpm: Add compute config for lobf
On Thu, 2024-05-09 at 11:01 +0530, Animesh Manna wrote:
> Link Off Between Active Frames, is a new feature for eDP
> that allows the panel to go to lower power state after
> transmission of data. This is a feature on top of ALPM, AS SDP.
> Add compute config during atomic-check phase.
>
> v1: RFC version.
> v2: Add separate flag for auxless-alpm. [Jani]
> v3:
> - intel_dp->lobf_supported replaced with crtc_state->has_lobf.
> [Jouni]
> - Add DISPLAY_VER() check. [Jouni]
> - Modify function name of get_aux_less_status. [Jani]
> v4: Add enum alpm_mode to hold the aux-wake/less capability.
>
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_alpm.c | 58
> +++
> drivers/gpu/drm/i915/display/intel_alpm.h | 5 ++
> .../drm/i915/display/intel_display_types.h | 11
> drivers/gpu/drm/i915/display/intel_dp.c | 4 ++
> 4 files changed, 78 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index ee6c2a959f09..5979eab1f2e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -11,6 +11,23 @@
> #include "intel_dp_aux.h"
> #include "intel_psr_regs.h"
>
> +enum alpm_mode intel_alpm_get_capability(struct intel_dp *intel_dp)
> +{
> + u8 alpm_caps = 0;
> +
> + if (drm_dp_dpcd_readb(_dp->aux, DP_RECEIVER_ALPM_CAP,
> + _caps) != 1)
> + return ALPM_INVALID;
> +
> + if (alpm_caps & DP_ALPM_CAP)
> + return ALPM_AUX_WAKE;
> +
> + if (alpm_caps & DP_ALPM_AUX_LESS_CAP)
> + return ALPM_AUX_LESS;
> +
> + return ALPM_NOT_SUPPORTED;
> +}
This will always return ALPM_AUX_WAKE if both are supported. I don't
think this is what you want?
You could add alpm_dpcd into intel_dp. Then for this purpose add
aux_wake_supported() and aux_less_wake_supported()?
BR,
Jouni Högander
> +
> /*
> * See Bspec: 71632 for the table
> *
> @@ -242,6 +259,47 @@ bool intel_alpm_compute_params(struct intel_dp
> *intel_dp,
> return true;
> }
>
> +void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
> + struct intel_crtc_state
> *crtc_state,
> + struct drm_connector_state
> *conn_state)
> +{
> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> + struct drm_display_mode *adjusted_mode = _state-
> >hw.adjusted_mode;
> + int waketime_in_lines, first_sdp_position;
> + int context_latency, guardband;
> +
> + if (!intel_dp_is_edp(intel_dp))
> + return;
> +
> + if (DISPLAY_VER(i915) < 20)
> + return;
> +
> + if (!intel_dp_as_sdp_supported(intel_dp))
> + return;
> +
> + if (crtc_state->has_psr)
> + return;
> +
> + if (intel_dp->alpm_parameters.mode == ALPM_INVALID ||
> + intel_dp->alpm_parameters.mode == ALPM_NOT_SUPPORTED)
> + return;
> +
> + if (!intel_alpm_compute_params(intel_dp, crtc_state))
> + return;
> +
> + context_latency = adjusted_mode->crtc_vblank_start -
> adjusted_mode->crtc_vdisplay;
> + guardband = adjusted_mode->crtc_vtotal -
> + adjusted_mode->crtc_vdisplay - context_latency;
> + first_sdp_position = adjusted_mode->crtc_vtotal -
> adjusted_mode->crtc_vsync_start;
> + if (intel_dp->alpm_parameters.mode == ALPM_AUX_LESS)
> + waketime_in_lines = intel_dp-
> >alpm_parameters.io_wake_lines;
> + else
> + waketime_in_lines = intel_dp-
> >alpm_parameters.fast_wake_lines;
> +
> + crtc_state->has_lobf = (context_latency + guardband) >
> + (first_sdp_position + waketime_in_lines);
> +}
> +
> static void lnl_alpm_configure(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> b/drivers/gpu/drm/i915/display/intel_alpm.h
> index c45d078e5a6b..80c8a66b34af 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -10,9 +10,14 @@
>
> struct intel_dp;
> struct intel_crtc_state;
> +struct drm_connector_state;
>
> +enum alpm_mode intel_alpm_get_capability(struct intel_dp *intel_dp);
> bool intel_alpm_compute_params(struct intel_dp *intel_dp,
> struct intel_crtc_state *crtc_state);
> +void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
> + struct intel_crtc_state
> *crtc_state,
> + struct drm_connector_state
> *conn_state);
> void intel_alpm_configure(struct intel_dp *intel_dp);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index e81fd71ce57b..79e9e543020b 100644
>
Re: [PATCH v3 0/6] Link off between frames for edp
On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote: > Link Off Between Active Frames (LOBF) allows an eDP link to be turned > Off and On > durning long VBLANK durations without enabling any of the PSR/PSR2/PR > modes of operation. You could describe a bit more about what this patch set is implementing and what you see is missing. BR, Jouni Högander > > Bspec: 71477 > > Note: These patches are not tested, sending early for review > feedback. > > Signed-off-by: Animesh Manna > > Animesh Manna (5): > drm/i915/alpm: Move alpm parameters from intel_psr > drm/i915/alpm: Move alpm related code to a new file > drm/i915/alpm: Add compute config for lobf > drm/i915/alpm: Enable lobf from source in ALPM_CTL > drm/i915/alpm: Add debugfs for LOBF > > Jouni Högander (1): > drm/display: Add missing aux less alpm wake related bits > > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/display/intel_alpm.c | 393 > ++ > drivers/gpu/drm/i915/display/intel_alpm.h | 25 ++ > .../drm/i915/display/intel_display_debugfs.c | 2 + > .../drm/i915/display/intel_display_types.h | 25 +- > drivers/gpu/drm/i915/display/intel_dp.c | 4 + > drivers/gpu/drm/i915/display/intel_psr.c | 299 + > include/drm/display/drm_dp.h | 5 +- > 8 files changed, 455 insertions(+), 299 deletions(-) > create mode 100644 drivers/gpu/drm/i915/display/intel_alpm.c > create mode 100644 drivers/gpu/drm/i915/display/intel_alpm.h >
Re: [PATCH v3 4/6] drm/i915/alpm: Add compute config for lobf
On Fri, 2024-05-03 at 08:42 +, Manna, Animesh wrote:
>
>
> > -Original Message-
> > From: Hogander, Jouni
> > Sent: Friday, May 3, 2024 12:49 PM
> > To: Manna, Animesh ; intel-
> > g...@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org; Murthy, Arun R
> > ; Nikula, Jani
> > Subject: Re: [PATCH v3 4/6] drm/i915/alpm: Add compute config for
> > lobf
> >
> > On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote:
> > > Link Off Between Active Frames, is a new feature for eDP
> > > that allows the panel to go to lower power state after
> > > transmission of data. This is a feature on top of ALPM, AS SDP.
> > > Add compute config during atomic-check phase.
> > >
> > > v1: RFC version.
> > > v2: Add separate flag for auxless-alpm. [Jani]
> > > v3:
> > > - intel_dp->lobf_supported replaced with crtc_state->has_lobf.
> > > [Jouni]
> > > - Add DISPLAY_VER() check. [Jouni]
> > > - Modify function name of get_aux_less_status. [Jani]
> > >
> > > Signed-off-by: Animesh Manna
> > > ---
> > > drivers/gpu/drm/i915/display/intel_alpm.c | 48
> > > +++
> > > drivers/gpu/drm/i915/display/intel_alpm.h | 5 ++
> > > .../drm/i915/display/intel_display_types.h | 4 ++
> > > drivers/gpu/drm/i915/display/intel_dp.c | 4 ++
> > > 4 files changed, 61 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > index 13bac3e8c8fa..3bb69ed16aab 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > @@ -11,6 +11,16 @@
> > > #include "intel_dp_aux.h"
> > > #include "intel_psr_regs.h"
> > >
> > > +bool intel_alpm_get_aux_less_status(struct intel_dp *intel_dp)
> > > +{
> > > + u8 alpm_caps = 0;
> > > +
> > > + if (drm_dp_dpcd_readb(_dp->aux,
> > > DP_RECEIVER_ALPM_CAP,
> > > + _caps) != 1)
> > > + return false;
> > > + return alpm_caps & DP_ALPM_AUX_LESS_CAP;
> > > +}
> > > +
> > > /*
> > > * See Bspec: 71632 for the table
> > > *
> > > @@ -242,6 +252,44 @@ bool intel_alpm_compute_params(struct
> > > intel_dp
> > > *intel_dp,
> > > return true;
> > > }
> > >
> > > +void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
> > > + struct intel_crtc_state
> > > *crtc_state,
> > > + struct drm_connector_state
> > > *conn_state)
> > > +{
> > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > + struct drm_display_mode *adjusted_mode = _state-
> > > > hw.adjusted_mode;
> > > + int waketime_in_lines, first_sdp_position;
> > > + int context_latency, guardband;
> > > +
> > > + crtc_state->has_lobf = false;
> >
> > Drop this line. I think crtc_state is reset before doing this
> > compute_config
>
> Sure.
>
> >
> > > +
> > > + if (!intel_dp_is_edp(intel_dp))
> > > + return;
> > > +
> > > + if (DISPLAY_VER(i915) < 20)
> > > + return;
> > > +
> > > + if (!intel_dp_as_sdp_supported(intel_dp))
> > > + return;
> > > +
> > > + if (crtc_state->has_psr)
> > > + return;
> > > +
> > > + if (intel_alpm_compute_params(intel_dp, crtc_state)) {
> >
> > I think it is easier to read and helps avoiding big if blocks if
> > you:
> >
> > if (!intel_alpm_compute_params(intel_dp, crtc_state())
> > return;
>
> Ok.
>
> >
> > This actually brings up another thing: do we want to spread
> > intel_psr.c
> > pollution by continue using these boolean return values? I would
> > prefer
> > changing intel_alpm_compute_params return value to "normal" int
> > approach and return 0 on success. This would mean one more patch
> > changing it.
>
> Ok.
>
> >
> > > + context_latency = adjusted_mode-
> > > >crtc_vblank_start -
> > > adjusted_mode->crtc_vdisplay;
> > > +
Re: [PATCH v3 6/6] drm/i915/alpm: Add debugfs for LOBF
On Fri, 2024-05-03 at 08:30 +, Manna, Animesh wrote:
>
>
> > -Original Message-
> > From: Hogander, Jouni
> > Sent: Friday, May 3, 2024 1:02 PM
> > To: Manna, Animesh ; intel-
> > g...@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org; Murthy, Arun R
> > ; Nikula, Jani
> > Subject: Re: [PATCH v3 6/6] drm/i915/alpm: Add debugfs for LOBF
> >
> > On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote:
> > > For validation purpose add debugfs for LOBF.
> > >
> > > Signed-off-by: Animesh Manna
> > > ---
> > > drivers/gpu/drm/i915/display/intel_alpm.c | 48
> > > +++
> > > drivers/gpu/drm/i915/display/intel_alpm.h | 2 +
> > > .../drm/i915/display/intel_display_debugfs.c | 2 +
> > > 3 files changed, 52 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > index b08799586b58..2d3027c2fb0a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > @@ -343,3 +343,51 @@ void intel_alpm_configure(struct intel_dp
> > > *intel_dp,
> > > {
> > > lnl_alpm_configure(intel_dp, crtc_state);
> > > }
> > > +
> > > +static int i915_edp_lobf_info_show(struct seq_file *m, void
> > > *data)
> > > +{
> > > + struct intel_connector *connector = m->private;
> > > + struct drm_i915_private *dev_priv = to_i915(connector-
> > > > base.dev);
> > > + struct drm_crtc *crtc;
> > > + struct intel_crtc_state *crtc_state;
> > > + enum transcoder cpu_transcoder;
> > > + bool lobf_enabled;
> > > + int ret;
> > > +
> > > + ret = drm_modeset_lock_single_interruptible(_priv-
> > > > drm.mode_config.connection_mutex);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + crtc = connector->base.state->crtc;
> > > + if (connector->base.status != connector_status_connected
> > > ||
> > > !crtc) {
> > > + ret = -ENODEV;
> > > + goto out;
> > > + }
> > > +
> > > + crtc_state = to_intel_crtc_state(crtc->state);
> > > + seq_printf(m, "LOBF Criteria met: %s\n",
> > > str_yes_no(crtc_state->has_lobf));
> >
> > I'm still not convinced on this. has_lobf ~= ALPM_CTL_LOBF_ENABLE
> > in
> > ALPM_CTL. To my opinion it is enough to dump seq_printf(m, "LOBF
> > status: %s\n", str_enabled_disabled(lobf_enabled)) below. Maybe
> > AUX_WAKE and AUX_LESS_WAKE could be dumped instead?
>
> Can add aux-wake or aux-less info as well.
> But as LOBF feature is dependent on adaptive sync fixed refresh rate
> mode (which can be dynamic as per user input) and ALPM. I want to
> expose both the conditions are satisfying or not along with status.
If all those conditions are satisfied (i.e. has_lobf is true) then
ALPM_CTL & ALPM_CTL_LOBF_ENABLE is true? So I'm wondering what is the
benefit from dumping has_lobf?
BR,
Jouni Högander
>
> Regards,
> Animesh
>
> >
> > BR,
> >
> > Jouni Högander
> >
> > > +
> > > + cpu_transcoder = crtc_state->cpu_transcoder;
> > > + lobf_enabled = intel_de_read(dev_priv,
> > > ALPM_CTL(cpu_transcoder)) & ALPM_CTL_LOBF_ENABLE;
> > > + seq_printf(m, "LOBF status: %s\n",
> > > str_enabled_disabled(lobf_enabled));
> > > +
> > > +out:
> > > + drm_modeset_unlock(_priv-
> > > > drm.mode_config.connection_mutex);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_info);
> > > +
> > > +void intel_alpm_lobf_debugfs_add(struct intel_connector
> > > *connector)
> > > +{
> > > + struct drm_i915_private *i915 = to_i915(connector-
> > > >base.dev);
> > > + struct dentry *root = connector->base.debugfs_entry;
> > > +
> > > + if (DISPLAY_VER(i915) < 20 ||
> > > + connector->base.connector_type !=
> > DRM_MODE_CONNECTOR_eDP)
> > > + return;
> > > +
> > > + debugfs_create_file("i915_edp_lobf_info", 0444, root,
> > > + connector, _edp
Re: [PATCH v3 5/6] drm/i915/alpm: Enable lobf from source in ALPM_CTL
On Fri, 2024-05-03 at 08:19 +, Manna, Animesh wrote:
>
>
> > -Original Message-
> > From: Hogander, Jouni
> > Sent: Friday, May 3, 2024 1:18 PM
> > To: Manna, Animesh ; intel-
> > g...@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org; Murthy, Arun R
> > ; Nikula, Jani
> > Subject: Re: [PATCH v3 5/6] drm/i915/alpm: Enable lobf from source
> > in
> > ALPM_CTL
> >
> > On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote:
> > > Set the Link Off Between Frames Enable bit in ALPM_CTL register.
> > >
> > > Note: Lobf need to be enabled adaptive sync fixed refresh mode
> > > where
> > > vmin = vmax = flipline, which will arise after cmmr feature
> > > enablement. Will add enabling sequence in a separate patch.
> >
> > Is adaptive sync needed for both Aux Wake and Aux Less Wake?
>
> AFAIK, ALPM (aux-wake/aux-less) do not have any dependency on
> adaptive sync.
> But LOBF is dependent on ALPM (aux-wake/aux-less) and adaptive sync
> fixed refresh mode.
>
> >
> > >
> > > Signed-off-by: Animesh Manna
> > > ---
> > > drivers/gpu/drm/i915/display/intel_alpm.c | 13 +
> > > drivers/gpu/drm/i915/display/intel_alpm.h | 4 ++--
> > > drivers/gpu/drm/i915/display/intel_psr.c | 2 +-
> > > 3 files changed, 12 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > index 3bb69ed16aab..b08799586b58 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > @@ -290,10 +290,11 @@ void intel_alpm_compute_lobf_config(struct
> > > intel_dp *intel_dp,
> > > }
> > > }
> > >
> > > -static void lnl_alpm_configure(struct intel_dp *intel_dp)
> > > +static void lnl_alpm_configure(struct intel_dp *intel_dp,
> > > + const struct intel_crtc_state
> > > *crtc_state)
> > > {
> > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > - enum transcoder cpu_transcoder = intel_dp-
> > > >psr.transcoder;
> > > + enum transcoder cpu_transcoder = crtc_state-
> > > >cpu_transcoder;
> > > u32 alpm_ctl;
> > >
> > > if (DISPLAY_VER(dev_priv) < 20 || (!intel_dp-
> > > > psr.psr2_enabled &&
> > > @@ -329,12 +330,16 @@ static void lnl_alpm_configure(struct
> > > intel_dp
> > > *intel_dp)
> > >
> > > ALPM_CTL_EXTENDED_FAST_WAKE_TIME(intel_dp-
> > > > alpm_parameters.fast_wake_lines);
> > > }
> > >
> > > + if (crtc_state->has_lobf)
> > > + alpm_ctl |= ALPM_CTL_LOBF_ENABLE;
> > > +
> >
> > How you are planning to differentiate between AUX Wake and AUX Less
> > Wake for LOBF?
>
> LOBF can be enabled in both aux-wake or aux-less alpm. So, check for
> aux-wake or aux-less is not needed.
> For aux wake ALPM_CTL[ ALPM AUX Less Enable ] = 0
> and for aux less ALPM_CTL[ ALPM AUX Less Enable ] = 1.
> So, I am plaining to add has_lobf check and enable if needed before
> ALPM_CTL register is getting programmed. Do you see any issue here?
No, I was just wondering why this is missing from your patch set?
BR,
Jouni Högander
>
> Regards,
> Animesh
>
> >
> > BR,
> >
> > Jouni Högander
> >
> > > alpm_ctl |= ALPM_CTL_ALPM_ENTRY_CHECK(intel_dp-
> > > > alpm_parameters.check_entry_lines);
> > >
> > > intel_de_write(dev_priv, ALPM_CTL(cpu_transcoder),
> > > alpm_ctl);
> > > }
> > >
> > > -void intel_alpm_configure(struct intel_dp *intel_dp)
> > > +void intel_alpm_configure(struct intel_dp *intel_dp,
> > > + const struct intel_crtc_state
> > > *crtc_state)
> > > {
> > > - lnl_alpm_configure(intel_dp);
> > > + lnl_alpm_configure(intel_dp, crtc_state);
> > > }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> > > b/drivers/gpu/drm/i915/display/intel_alpm.h
> > > index b9602b71d28f..a9ca190da3e4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> > > @@ -18,6 +18,6 @@ bool intel_alpm_compute_params(stru
Re: [PATCH v3 5/6] drm/i915/alpm: Enable lobf from source in ALPM_CTL
On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote:
> Set the Link Off Between Frames Enable bit in ALPM_CTL register.
>
> Note: Lobf need to be enabled adaptive sync fixed refresh mode
> where vmin = vmax = flipline, which will arise after cmmr feature
> enablement. Will add enabling sequence in a separate patch.
Is adaptive sync needed for both Aux Wake and Aux Less Wake?
>
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_alpm.c | 13 +
> drivers/gpu/drm/i915/display/intel_alpm.h | 4 ++--
> drivers/gpu/drm/i915/display/intel_psr.c | 2 +-
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 3bb69ed16aab..b08799586b58 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -290,10 +290,11 @@ void intel_alpm_compute_lobf_config(struct
> intel_dp *intel_dp,
> }
> }
>
> -static void lnl_alpm_configure(struct intel_dp *intel_dp)
> +static void lnl_alpm_configure(struct intel_dp *intel_dp,
> + const struct intel_crtc_state
> *crtc_state)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> - enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> u32 alpm_ctl;
>
> if (DISPLAY_VER(dev_priv) < 20 || (!intel_dp-
> >psr.psr2_enabled &&
> @@ -329,12 +330,16 @@ static void lnl_alpm_configure(struct intel_dp
> *intel_dp)
> ALPM_CTL_EXTENDED_FAST_WAKE_TIME(intel_dp-
> >alpm_parameters.fast_wake_lines);
> }
>
> + if (crtc_state->has_lobf)
> + alpm_ctl |= ALPM_CTL_LOBF_ENABLE;
> +
How you are planning to differentiate between AUX Wake and AUX Less
Wake for LOBF?
BR,
Jouni Högander
> alpm_ctl |= ALPM_CTL_ALPM_ENTRY_CHECK(intel_dp-
> >alpm_parameters.check_entry_lines);
>
> intel_de_write(dev_priv, ALPM_CTL(cpu_transcoder), alpm_ctl);
> }
>
> -void intel_alpm_configure(struct intel_dp *intel_dp)
> +void intel_alpm_configure(struct intel_dp *intel_dp,
> + const struct intel_crtc_state *crtc_state)
> {
> - lnl_alpm_configure(intel_dp);
> + lnl_alpm_configure(intel_dp, crtc_state);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> b/drivers/gpu/drm/i915/display/intel_alpm.h
> index b9602b71d28f..a9ca190da3e4 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -18,6 +18,6 @@ bool intel_alpm_compute_params(struct intel_dp
> *intel_dp,
> void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
> struct intel_crtc_state
> *crtc_state,
> struct drm_connector_state
> *conn_state);
> -void intel_alpm_configure(struct intel_dp *intel_dp);
> -
> +void intel_alpm_configure(struct intel_dp *intel_dp,
> + const struct intel_crtc_state *crtc_state);
> #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index c4ab289dbc15..4eb45df20ad2 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1611,7 +1611,7 @@ static void intel_psr_enable_source(struct
> intel_dp *intel_dp,
> IGNORE_PSR2_HW_TRACKING : 0);
>
> if (intel_dp_is_edp(intel_dp))
> - intel_alpm_configure(intel_dp);
> + intel_alpm_configure(intel_dp, crtc_state);
>
> /*
> * Wa_16013835468
Re: [PATCH v3 6/6] drm/i915/alpm: Add debugfs for LOBF
On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote:
> For validation purpose add debugfs for LOBF.
>
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_alpm.c | 48
> +++
> drivers/gpu/drm/i915/display/intel_alpm.h | 2 +
> .../drm/i915/display/intel_display_debugfs.c | 2 +
> 3 files changed, 52 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index b08799586b58..2d3027c2fb0a 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -343,3 +343,51 @@ void intel_alpm_configure(struct intel_dp
> *intel_dp,
> {
> lnl_alpm_configure(intel_dp, crtc_state);
> }
> +
> +static int i915_edp_lobf_info_show(struct seq_file *m, void *data)
> +{
> + struct intel_connector *connector = m->private;
> + struct drm_i915_private *dev_priv = to_i915(connector-
> >base.dev);
> + struct drm_crtc *crtc;
> + struct intel_crtc_state *crtc_state;
> + enum transcoder cpu_transcoder;
> + bool lobf_enabled;
> + int ret;
> +
> + ret = drm_modeset_lock_single_interruptible(_priv-
> >drm.mode_config.connection_mutex);
> + if (ret)
> + return ret;
> +
> + crtc = connector->base.state->crtc;
> + if (connector->base.status != connector_status_connected ||
> !crtc) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + crtc_state = to_intel_crtc_state(crtc->state);
> + seq_printf(m, "LOBF Criteria met: %s\n",
> str_yes_no(crtc_state->has_lobf));
I'm still not convinced on this. has_lobf ~= ALPM_CTL_LOBF_ENABLE in
ALPM_CTL. To my opinion it is enough to dump seq_printf(m, "LOBF
status: %s\n", str_enabled_disabled(lobf_enabled)) below. Maybe
AUX_WAKE and AUX_LESS_WAKE could be dumped instead?
BR,
Jouni Högander
> +
> + cpu_transcoder = crtc_state->cpu_transcoder;
> + lobf_enabled = intel_de_read(dev_priv,
> ALPM_CTL(cpu_transcoder)) & ALPM_CTL_LOBF_ENABLE;
> + seq_printf(m, "LOBF status: %s\n",
> str_enabled_disabled(lobf_enabled));
> +
> +out:
> + drm_modeset_unlock(_priv-
> >drm.mode_config.connection_mutex);
> +
> + return ret;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_info);
> +
> +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector)
> +{
> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
> + struct dentry *root = connector->base.debugfs_entry;
> +
> + if (DISPLAY_VER(i915) < 20 ||
> + connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> + return;
> +
> + debugfs_create_file("i915_edp_lobf_info", 0444, root,
> + connector, _edp_lobf_info_fops);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> b/drivers/gpu/drm/i915/display/intel_alpm.h
> index a9ca190da3e4..01fd08eb96f5 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -11,6 +11,7 @@
> struct intel_dp;
> struct intel_crtc_state;
> struct drm_connector_state;
> +struct intel_connector;
>
> bool intel_alpm_get_aux_less_status(struct intel_dp *intel_dp);
> bool intel_alpm_compute_params(struct intel_dp *intel_dp,
> @@ -20,4 +21,5 @@ void intel_alpm_compute_lobf_config(struct intel_dp
> *intel_dp,
> struct drm_connector_state
> *conn_state);
> void intel_alpm_configure(struct intel_dp *intel_dp,
> const struct intel_crtc_state *crtc_state);
> +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector);
> #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 35f9f86ef70f..86d9900c40af 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -13,6 +13,7 @@
> #include "i915_debugfs.h"
> #include "i915_irq.h"
> #include "i915_reg.h"
> +#include "intel_alpm.h"
> #include "intel_crtc.h"
> #include "intel_de.h"
> #include "intel_crtc_state_dump.h"
> @@ -1515,6 +1516,7 @@ void intel_connector_debugfs_add(struct
> intel_connector *connector)
> intel_drrs_connector_debugfs_add(connector);
> intel_pps_connector_debugfs_add(connector);
> intel_psr_connector_debugfs_add(connector);
> + intel_alpm_lobf_debugfs_add(connector);
>
> if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> connector_type == DRM_MODE_CONNECTOR_HDMIA ||
Re: [PATCH v3 4/6] drm/i915/alpm: Add compute config for lobf
On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote:
> Link Off Between Active Frames, is a new feature for eDP
> that allows the panel to go to lower power state after
> transmission of data. This is a feature on top of ALPM, AS SDP.
> Add compute config during atomic-check phase.
>
> v1: RFC version.
> v2: Add separate flag for auxless-alpm. [Jani]
> v3:
> - intel_dp->lobf_supported replaced with crtc_state->has_lobf.
> [Jouni]
> - Add DISPLAY_VER() check. [Jouni]
> - Modify function name of get_aux_less_status. [Jani]
>
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_alpm.c | 48
> +++
> drivers/gpu/drm/i915/display/intel_alpm.h | 5 ++
> .../drm/i915/display/intel_display_types.h | 4 ++
> drivers/gpu/drm/i915/display/intel_dp.c | 4 ++
> 4 files changed, 61 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 13bac3e8c8fa..3bb69ed16aab 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -11,6 +11,16 @@
> #include "intel_dp_aux.h"
> #include "intel_psr_regs.h"
>
> +bool intel_alpm_get_aux_less_status(struct intel_dp *intel_dp)
> +{
> + u8 alpm_caps = 0;
> +
> + if (drm_dp_dpcd_readb(_dp->aux, DP_RECEIVER_ALPM_CAP,
> + _caps) != 1)
> + return false;
> + return alpm_caps & DP_ALPM_AUX_LESS_CAP;
> +}
> +
> /*
> * See Bspec: 71632 for the table
> *
> @@ -242,6 +252,44 @@ bool intel_alpm_compute_params(struct intel_dp
> *intel_dp,
> return true;
> }
>
> +void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
> + struct intel_crtc_state
> *crtc_state,
> + struct drm_connector_state
> *conn_state)
> +{
> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> + struct drm_display_mode *adjusted_mode = _state-
> >hw.adjusted_mode;
> + int waketime_in_lines, first_sdp_position;
> + int context_latency, guardband;
> +
> + crtc_state->has_lobf = false;
Drop this line. I think crtc_state is reset before doing this
compute_config
> +
> + if (!intel_dp_is_edp(intel_dp))
> + return;
> +
> + if (DISPLAY_VER(i915) < 20)
> + return;
> +
> + if (!intel_dp_as_sdp_supported(intel_dp))
> + return;
> +
> + if (crtc_state->has_psr)
> + return;
> +
> + if (intel_alpm_compute_params(intel_dp, crtc_state)) {
I think it is easier to read and helps avoiding big if blocks if you:
if (!intel_alpm_compute_params(intel_dp, crtc_state())
return;
This actually brings up another thing: do we want to spread intel_psr.c
pollution by continue using these boolean return values? I would prefer
changing intel_alpm_compute_params return value to "normal" int
approach and return 0 on success. This would mean one more patch
changing it.
> + context_latency = adjusted_mode->crtc_vblank_start -
> adjusted_mode->crtc_vdisplay;
> + guardband = adjusted_mode->crtc_vtotal -
> + adjusted_mode->crtc_vdisplay -
> context_latency;
> + first_sdp_position = adjusted_mode->crtc_vtotal -
> adjusted_mode->crtc_vsync_start;
> + if (intel_dp->alpm_parameters.auxless_alpm_supported)
> + waketime_in_lines = intel_dp-
> >alpm_parameters.io_wake_lines;
> + else
> + waketime_in_lines = intel_dp-
> >alpm_parameters.fast_wake_lines;
> +
> + if ((context_latency + guardband) >
> (first_sdp_position + waketime_in_lines))
> + crtc_state->has_lobf = true;
crtc_state->has_lobf = (context_latency + guardband) >
(first_sdp_position + waketime_in_lines);
> + }
> +}
> +
> static void lnl_alpm_configure(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> b/drivers/gpu/drm/i915/display/intel_alpm.h
> index c45d078e5a6b..b9602b71d28f 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -10,9 +10,14 @@
>
> struct intel_dp;
> struct intel_crtc_state;
> +struct drm_connector_state;
>
> +bool intel_alpm_get_aux_less_status(struct intel_dp *intel_dp);
> bool intel_alpm_compute_params(struct intel_dp *intel_dp,
> struct intel_crtc_state *crtc_state);
> +void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
> + struct intel_crtc_state
> *crtc_state,
> + struct drm_connector_state
> *conn_state);
> void intel_alpm_configure(struct intel_dp *intel_dp);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
>
Re: [PATCH v8 6/6] drm/{i915,xe}: Implement fbdev emulation as in-kernel client
On Tue, 2024-04-23 at 13:13 +0200, Thomas Zimmermann wrote: > Hi > > Am 22.04.24 um 16:11 schrieb Hogander, Jouni: > > On Tue, 2024-04-09 at 10:04 +0200, Thomas Zimmermann wrote: > > > Replace all code that initializes or releases fbdev emulation > > > throughout the driver. Instead initialize the fbdev client by a > > > single call to intel_fbdev_setup() after i915 has registered its > > > DRM device. Just like similar code in other drivers, i915 fbdev > > > emulation now acts like a regular DRM client. Do the same for xe. > > > > > > The fbdev client setup consists of the initial preparation and > > > the > > > hot-plugging of the display. The latter creates the fbdev device > > > and sets up the fbdev framebuffer. The setup performs display > > > hot-plugging once. If no display can be detected, DRM probe > > > helpers > > > re-run the detection on each hotplug event. > > > > > > A call to drm_client_dev_unregister() releases all in-kernel > > > clients > > > automatically. No further action is required within i915. If the > > > fbdev > > > framebuffer has been fully set up, struct fb_ops.fb_destroy > > > implements > > > the release. For partially initialized emulation, the fbdev > > > client > > > reverts the initial setup. Do the same for xe and remove its call > > > to > > > intel_fbdev_fini(). > > > > > > v8: > > > - setup client in intel_display_driver_register (Jouni) > > > - mention xe in commit message > > > > > > v7: > > > - update xe driver > > > - reword commit message > > > > > > v6: > > > - use 'i915' for i915 device (Jouni) > > > - remove unnecessary code for non-atomic mode setting (Jouni, > > > Ville) > > > - fix function name in commit message (Jouni) > > > > > > v3: > > > - as before, silently ignore devices without displays > > > > > > v2: > > > - let drm_client_register() handle initial hotplug > > > - fix driver name in error message (Jani) > > > - fix non-fbdev build (kernel test robot) > > > > > > Signed-off-by: Thomas Zimmermann > > Reviewed-by: Jouni Högander > > Thank you so much. All patches has R-bs. Can you add the series to > the > intel tree? Is it ok to merge patch 01/06 via intel tree as well? Rodrigo, This set is containing Xe display changes as well. Is it ok to push this via drm-intel? BR, Jouni Högander > > Best regards > Thomas > > > > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 1 - > > > .../drm/i915/display/intel_display_driver.c | 20 +- > > > drivers/gpu/drm/i915/display/intel_fbdev.c | 177 --- > > > - > > > -- > > > drivers/gpu/drm/i915/display/intel_fbdev.h | 20 +- > > > drivers/gpu/drm/xe/display/xe_display.c | 2 - > > > 5 files changed, 80 insertions(+), 140 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 614e60420a29a..161a5aabf6746 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -85,7 +85,6 @@ > > > #include "intel_dvo.h" > > > #include "intel_fb.h" > > > #include "intel_fbc.h" > > > -#include "intel_fbdev.h" > > > #include "intel_fdi.h" > > > #include "intel_fifo_underrun.h" > > > #include "intel_frontbuffer.h" > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > > > b/drivers/gpu/drm/i915/display/intel_display_driver.c > > > index e5f052d4ff1cc..ed8589fa35448 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > > > @@ -514,10 +514,6 @@ int intel_display_driver_probe(struct > > > drm_i915_private *i915) > > > > > > intel_overlay_setup(i915); > > > > > > - ret = intel_fbdev_init(>drm); > > > - if (ret) > > > - return ret; > > > - > > > /* Only enable hotplug handling once the fbdev is fully > > > set > > > up. */ > > > intel_hpd_init(i915); > > > > > > @@
Re: [PATCH v8 6/6] drm/{i915,xe}: Implement fbdev emulation as in-kernel client
On Tue, 2024-04-09 at 10:04 +0200, Thomas Zimmermann wrote: > Replace all code that initializes or releases fbdev emulation > throughout the driver. Instead initialize the fbdev client by a > single call to intel_fbdev_setup() after i915 has registered its > DRM device. Just like similar code in other drivers, i915 fbdev > emulation now acts like a regular DRM client. Do the same for xe. > > The fbdev client setup consists of the initial preparation and the > hot-plugging of the display. The latter creates the fbdev device > and sets up the fbdev framebuffer. The setup performs display > hot-plugging once. If no display can be detected, DRM probe helpers > re-run the detection on each hotplug event. > > A call to drm_client_dev_unregister() releases all in-kernel clients > automatically. No further action is required within i915. If the > fbdev > framebuffer has been fully set up, struct fb_ops.fb_destroy > implements > the release. For partially initialized emulation, the fbdev client > reverts the initial setup. Do the same for xe and remove its call to > intel_fbdev_fini(). > > v8: > - setup client in intel_display_driver_register (Jouni) > - mention xe in commit message > > v7: > - update xe driver > - reword commit message > > v6: > - use 'i915' for i915 device (Jouni) > - remove unnecessary code for non-atomic mode setting (Jouni, Ville) > - fix function name in commit message (Jouni) > > v3: > - as before, silently ignore devices without displays > > v2: > - let drm_client_register() handle initial hotplug > - fix driver name in error message (Jani) > - fix non-fbdev build (kernel test robot) > > Signed-off-by: Thomas Zimmermann Reviewed-by: Jouni Högander > --- > drivers/gpu/drm/i915/display/intel_display.c | 1 - > .../drm/i915/display/intel_display_driver.c | 20 +- > drivers/gpu/drm/i915/display/intel_fbdev.c | 177 > -- > drivers/gpu/drm/i915/display/intel_fbdev.h | 20 +- > drivers/gpu/drm/xe/display/xe_display.c | 2 - > 5 files changed, 80 insertions(+), 140 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 614e60420a29a..161a5aabf6746 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -85,7 +85,6 @@ > #include "intel_dvo.h" > #include "intel_fb.h" > #include "intel_fbc.h" > -#include "intel_fbdev.h" > #include "intel_fdi.h" > #include "intel_fifo_underrun.h" > #include "intel_frontbuffer.h" > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > b/drivers/gpu/drm/i915/display/intel_display_driver.c > index e5f052d4ff1cc..ed8589fa35448 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > @@ -514,10 +514,6 @@ int intel_display_driver_probe(struct > drm_i915_private *i915) > > intel_overlay_setup(i915); > > - ret = intel_fbdev_init(>drm); > - if (ret) > - return ret; > - > /* Only enable hotplug handling once the fbdev is fully set > up. */ > intel_hpd_init(i915); > > @@ -544,16 +540,6 @@ void intel_display_driver_register(struct > drm_i915_private *i915) > > intel_display_debugfs_register(i915); > > - /* > - * Some ports require correctly set-up hpd registers for > - * detection to work properly (leading to ghost connected > - * connector status), e.g. VGA on gm45. Hence we can only > set > - * up the initial fbdev config after hpd irqs are fully > - * enabled. We do it last so that the async config cannot run > - * before the connectors are registered. > - */ > - intel_fbdev_initial_config_async(i915); > - > /* > * We need to coordinate the hotplugs with the asynchronous > * fbdev configuration, for which we use the > @@ -562,6 +548,8 @@ void intel_display_driver_register(struct > drm_i915_private *i915) > drm_kms_helper_poll_init(>drm); > intel_hpd_poll_disable(i915); > > + intel_fbdev_setup(i915); > + > intel_display_device_info_print(DISPLAY_INFO(i915), > DISPLAY_RUNTIME_INFO(i915), > ); > } > @@ -597,9 +585,6 @@ void intel_display_driver_remove_noirq(struct > drm_i915_private *i915) > */ > intel_hpd_poll_fini(i915); > > - /* poll work can call into fbdev, hence clean that up > afterwards */ > - intel_fbdev_fini(i915); > - > intel_unregister_dsm_handler(); > > /* flush any delayed tasks or pending work */ > @@ -640,7 +625,6 @@ void intel_display_driver_unregister(struct > drm_i915_private *i915) > > drm_client_dev_unregister(>drm); > > - intel_fbdev_unregister(i915); > /* > * After flushing the fbdev (incl. a late async config which > * will have delayed queuing of a hotplug
Re: [PATCH v8 4/6] drm/{i915,xe}: Unregister in-kernel clients
On Tue, 2024-04-09 at 10:04 +0200, Thomas Zimmermann wrote: > Unregister all in-kernel clients before unloading the i915 driver. > For > other drivers, drm_dev_unregister() does this automatically. As i915 > and > xe do not use this helper, they have to perform the call by > themselves. > > Note that there are currently no in-kernel clients in i915 or xe. The > patch prepares the drivers for a related update of their fbdev > support. > > v8: > - unregister clients in intel_display_driver_unregister() (Jani) > - mention xe in commit message (Rodrigo, Jani) > > v7: > - update xe driver > > Signed-off-by: Thomas Zimmermann Reviewed-by: Jouni Högander > --- > drivers/gpu/drm/i915/display/intel_display_driver.c | 3 +++ > drivers/gpu/drm/xe/xe_device.c | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > b/drivers/gpu/drm/i915/display/intel_display_driver.c > index 87dd07e0d138d..b7d636980d83a 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > @@ -11,6 +11,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -638,6 +639,8 @@ void intel_display_driver_unregister(struct > drm_i915_private *i915) > if (!HAS_DISPLAY(i915)) > return; > > + drm_client_dev_unregister(>drm); > + > intel_fbdev_unregister(i915); > /* > * After flushing the fbdev (incl. a late async config which > diff --git a/drivers/gpu/drm/xe/xe_device.c > b/drivers/gpu/drm/xe/xe_device.c > index 01bd5ccf05ca6..231ab2f4cd0b9 100644 > --- a/drivers/gpu/drm/xe/xe_device.c > +++ b/drivers/gpu/drm/xe/xe_device.c > @@ -9,6 +9,7 @@ > > #include > #include > +#include > #include > #include > #include
Re: [PATCH v8 1/6] drm/client: Export drm_client_dev_unregister()
On Tue, 2024-04-09 at 10:04 +0200, Thomas Zimmermann wrote:
> Export drm_client_dev_unregister() for use by the i915 driver. The
> driver does not use drm_dev_unregister(), so it has to clean up the
> in-kernel DRM clients by itself.
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Jouni Högander
> ---
> drivers/gpu/drm/drm_client.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_client.c
> b/drivers/gpu/drm/drm_client.c
> index 77fe217aeaf36..2803ac111bbd8 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -172,6 +172,18 @@ void drm_client_release(struct drm_client_dev
> *client)
> }
> EXPORT_SYMBOL(drm_client_release);
>
> +/**
> + * drm_client_dev_unregister - Unregister clients
> + * @dev: DRM device
> + *
> + * This function releases all clients by calling each client's
> + * _client_funcs.unregister callback. The callback function
> + * is responsibe for releaseing all resources including the client
> + * itself.
> + *
> + * The helper drm_dev_unregister() calls this function. Drivers
> + * that use it don't need to call this function themselves.
> + */
> void drm_client_dev_unregister(struct drm_device *dev)
> {
> struct drm_client_dev *client, *tmp;
> @@ -191,6 +203,7 @@ void drm_client_dev_unregister(struct drm_device
> *dev)
> }
> mutex_unlock(>clientlist_mutex);
> }
> +EXPORT_SYMBOL(drm_client_dev_unregister);
>
> /**
> * drm_client_dev_hotplug - Send hotplug event to clients
Re: [PATCH v2 5/6] drm/i915/alpm: Enable lobf from source in ALPM_CTL
On Tue, 2024-04-16 at 08:20 +, Manna, Animesh wrote:
>
>
> > -Original Message-
> > From: Hogander, Jouni
> > Sent: Monday, April 15, 2024 3:39 PM
> > To: Manna, Animesh ; intel-
> > g...@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org; Murthy, Arun R
> > ; Nikula, Jani
> > Subject: Re: [PATCH v2 5/6] drm/i915/alpm: Enable lobf from source
> > in
> > ALPM_CTL
> >
> > On Fri, 2024-04-12 at 21:22 +0530, Animesh Manna wrote:
> > > Set the Link Off Between Frames Enable bit in ALPM_CTL register.
> > >
> > > Signed-off-by: Animesh Manna
> > > ---
> > > drivers/gpu/drm/i915/display/intel_alpm.c | 5 +
> > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 +
> > > 2 files changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > index 699f2f051766..ae894c85233c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > @@ -325,6 +325,11 @@ static void lnl_alpm_configure(struct
> > > intel_dp
> > > *intel_dp)
> > >
> > > ALPM_CTL_EXTENDED_FAST_WAKE_TIME(intel_dp-
> > > > alpm_parameters.fast_wake_lines);
> > > }
> > >
> > > + if (intel_dp->lobf_supported) {
> > > + alpm_ctl |= ALPM_CTL_LOBF_ENABLE;
> > > + intel_dp->lobf_enabled = true;
> > > + }
> > > +
> >
> > I don't see lnl_alpm_configure being called for lobf case in your
> > patches.
>
> Enabling/Disabling LOBF will be done along with alpm(aux-wake/aux-
> less) enablement.
> Here lobf_supported flag is the switch to enable LOBF or not.
> Please let me know if I am missing anything.
I might be missing something. E.g. in case of aux_less_alpm PR
lnl_alpm_configure is called from intel_psr_enable_source. Where it is
called for lobf case?
BR,
Jouni Högander
>
> Regards,
> Animesh
>
> >
> > BR,
> >
> > Jouni Högander
> >
> > > alpm_ctl |= ALPM_CTL_ALPM_ENTRY_CHECK(intel_dp-
> > > > alpm_parameters.check_entry_lines);
> > >
> > > intel_de_write(dev_priv, ALPM_CTL(cpu_transcoder),
> > > alpm_ctl);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 6116c383b543..f61ba582429b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1884,6 +1884,7 @@ struct intel_dp {
> > >
> > > /* LOBF flags*/
> > > bool lobf_supported;
> > > + bool lobf_enabled;
> > > };
> > >
> > > enum lspcon_vendor {
>
Re: [PATCH v2 4/6] drm/i915/alpm: Add compute config for lobf
On Tue, 2024-04-16 at 08:15 +, Manna, Animesh wrote:
>
>
> > -Original Message-
> > From: Hogander, Jouni
> > Sent: Monday, April 15, 2024 3:36 PM
> > To: Manna, Animesh ; intel-
> > g...@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org; Murthy, Arun R
> > ; Nikula, Jani
> > Subject: Re: [PATCH v2 4/6] drm/i915/alpm: Add compute config for
> > lobf
> >
> > On Fri, 2024-04-12 at 21:22 +0530, Animesh Manna wrote:
> > > Link Off Between Active Frames, is a new feature for eDP that
> > > allows
> > > the panel to go to lower power state after transmission of data.
> > > This
> > > is a feature on top of ALPM, AS SDP.
> > > Add compute config during atomic-check phase.
> > >
> > > v1: RFC version.
> > > v2: Add separate flag for auxless-alpm. [Jani]
> > >
> > > Signed-off-by: Animesh Manna
> > > ---
> > > drivers/gpu/drm/i915/display/intel_alpm.c | 44
> > > +++
> > > drivers/gpu/drm/i915/display/intel_alpm.h | 5 +++
> > > .../drm/i915/display/intel_display_types.h | 4 ++
> > > drivers/gpu/drm/i915/display/intel_dp.c | 5 +++
> > > 4 files changed, 58 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > index 13bac3e8c8fa..699f2f051766 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > @@ -11,6 +11,16 @@
> > > #include "intel_dp_aux.h"
> > > #include "intel_psr_regs.h"
> > >
> > > +bool intel_dp_get_aux_less_alpm_status(struct intel_dp
> > > *intel_dp) {
> > > + u8 alpm_caps = 0;
> > > +
> > > + if (drm_dp_dpcd_readb(_dp->aux,
> > > DP_RECEIVER_ALPM_CAP,
> > > + _caps) != 1)
> > > + return false;
> > > + return alpm_caps & DP_ALPM_AUX_LESS_CAP; }
> > > +
> > > /*
> > > * See Bspec: 71632 for the table
> > > *
> > > @@ -242,6 +252,40 @@ bool intel_alpm_compute_params(struct
> > > intel_dp
> > > *intel_dp,
> > > return true;
> > > }
> > >
> > > +void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
> > > + struct intel_crtc_state
> > > *crtc_state,
> > > + struct drm_connector_state
> > > *conn_state)
> > > +{
> > > + struct drm_display_mode *adjusted_mode = _state-
> > > > hw.adjusted_mode;
> > > + int waketime_in_lines, first_sdp_position;
> > > + int context_latency, guardband;
> > > +
> > > + intel_dp->lobf_supported = false;
> > > +
> > > + if (!intel_dp_is_edp(intel_dp))
> > > + return;
> > > +
> > > + if (!intel_dp_as_sdp_supported(intel_dp))
> > > + return;
> > > +
> > > + if (crtc_state->has_psr2 || crtc_state->has_panel_replay)
> > > + return;
> >
> > LOBF is not supported with PSR1? I think checking crtc_state-
> > >has_psr is
> > enough. That covers PSR1/2 and Panel Replay.
>
> Ok.
>
> >
> > > +
> > > + if (intel_alpm_compute_params(intel_dp, crtc_state)) {
> > > + context_latency = adjusted_mode-
> > > >crtc_vblank_start -
> > > adjusted_mode->crtc_vdisplay;
> > > + guardband = adjusted_mode->crtc_vtotal -
> > > + adjusted_mode->crtc_vdisplay -
> > > context_latency;
> > > + first_sdp_position = adjusted_mode->crtc_vtotal -
> > > adjusted_mode->crtc_vsync_start;
> > > + if (intel_dp-
> > > >alpm_parameters.auxless_alpm_supported)
> > > + waketime_in_lines = intel_dp-
> > > > alpm_parameters.io_wake_lines;
> > > + else
> > > + waketime_in_lines = intel_dp-
> > > > alpm_parameters.fast_wake_lines;
> > > +
> > > + if ((context_latency + guardband) >
> > > (first_sdp_position + waketime_in_lines))
> > > + intel_dp->lobf_supported = true;
> > > + }
> &g
Re: [PATCH v2 6/6] drm/i915/alpm: Add debugfs for LOBF
On Fri, 2024-04-12 at 21:22 +0530, Animesh Manna wrote:
> For validation purpose add debugfs for LOBF.
>
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_alpm.c | 47
> +++
> drivers/gpu/drm/i915/display/intel_alpm.h | 2 +
> .../drm/i915/display/intel_display_debugfs.c | 2 +
> 3 files changed, 51 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index ae894c85233c..21dfc06952d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -339,3 +339,50 @@ void intel_alpm_configure(struct intel_dp
> *intel_dp)
> {
> lnl_alpm_configure(intel_dp);
> }
> +
> +static int i915_edp_lobf_support_show(struct seq_file *m, void
> *data)
> +{
> + struct intel_connector *connector = m->private;
> + struct intel_dp *intel_dp = intel_attached_dp(connector);
> +
> + seq_printf(m, "LOBF support: = %s",
> + str_yes_no(intel_dp->lobf_supported));
> +
> + return 0;
What this debugfs is telling? Lobf may be supported by platform, but
not enabled because PSR is enabled. Saying LOBF support = no is
misleading.
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_support);
> +
> +static int i915_edp_lobf_status_show(struct seq_file *m, void *data)
> +{
> + struct intel_connector *connector = m->private;
> + struct intel_dp *intel_dp = intel_attached_dp(connector);
> + const char *status;
> +
> + if (intel_dp->lobf_enabled)
I think better option is to read it from the registers.
BR,
Jouni Högander
> + status = "enabled";
> + else
> + status = "disabled";
> +
> + seq_printf(m, "LOBF: %s\n", status);
> +
> + return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_status);
> +
> +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector)
> +{
> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
> + struct dentry *root = connector->base.debugfs_entry;
> +
> + if (DISPLAY_VER(i915) >= 20 &&
> + connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> + return;
> +
> + debugfs_create_file("i915_edp_lobf_supported", 0444, root,
> + connector, _edp_lobf_support_fops);
> +
> + debugfs_create_file("i915_edp_lobf_status", 0444, root,
> + connector, _edp_lobf_status_fops);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> b/drivers/gpu/drm/i915/display/intel_alpm.h
> index c341d2c2b7f7..66e81ed8b2fb 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -11,6 +11,7 @@
> struct intel_dp;
> struct intel_crtc_state;
> struct drm_connector_state;
> +struct intel_connector;
>
> bool intel_dp_get_aux_less_alpm_status(struct intel_dp *intel_dp);
> bool intel_alpm_compute_params(struct intel_dp *intel_dp,
> @@ -19,5 +20,6 @@ void intel_alpm_compute_lobf_config(struct intel_dp
> *intel_dp,
> struct intel_crtc_state
> *crtc_state,
> struct drm_connector_state
> *conn_state);
> void intel_alpm_configure(struct intel_dp *intel_dp);
> +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 0feffe8d4e45..ba1530149836 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -13,6 +13,7 @@
> #include "i915_debugfs.h"
> #include "i915_irq.h"
> #include "i915_reg.h"
> +#include "intel_alpm.h"
> #include "intel_crtc.h"
> #include "intel_de.h"
> #include "intel_crtc_state_dump.h"
> @@ -1542,6 +1543,7 @@ void intel_connector_debugfs_add(struct
> intel_connector *connector)
> intel_drrs_connector_debugfs_add(connector);
> intel_pps_connector_debugfs_add(connector);
> intel_psr_connector_debugfs_add(connector);
> + intel_alpm_lobf_debugfs_add(connector);
>
> if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> connector_type == DRM_MODE_CONNECTOR_HDMIA ||
Re: [PATCH v2 5/6] drm/i915/alpm: Enable lobf from source in ALPM_CTL
On Fri, 2024-04-12 at 21:22 +0530, Animesh Manna wrote:
> Set the Link Off Between Frames Enable bit in ALPM_CTL register.
>
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_alpm.c | 5 +
> drivers/gpu/drm/i915/display/intel_display_types.h | 1 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 699f2f051766..ae894c85233c 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -325,6 +325,11 @@ static void lnl_alpm_configure(struct intel_dp
> *intel_dp)
> ALPM_CTL_EXTENDED_FAST_WAKE_TIME(intel_dp-
> >alpm_parameters.fast_wake_lines);
> }
>
> + if (intel_dp->lobf_supported) {
> + alpm_ctl |= ALPM_CTL_LOBF_ENABLE;
> + intel_dp->lobf_enabled = true;
> + }
> +
I don't see lnl_alpm_configure being called for lobf case in your
patches.
BR,
Jouni Högander
> alpm_ctl |= ALPM_CTL_ALPM_ENTRY_CHECK(intel_dp-
> >alpm_parameters.check_entry_lines);
>
> intel_de_write(dev_priv, ALPM_CTL(cpu_transcoder), alpm_ctl);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 6116c383b543..f61ba582429b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1884,6 +1884,7 @@ struct intel_dp {
>
> /* LOBF flags*/
> bool lobf_supported;
> + bool lobf_enabled;
> };
>
> enum lspcon_vendor {
Re: [PATCH v2 4/6] drm/i915/alpm: Add compute config for lobf
On Fri, 2024-04-12 at 21:22 +0530, Animesh Manna wrote:
> Link Off Between Active Frames, is a new feature for eDP
> that allows the panel to go to lower power state after
> transmission of data. This is a feature on top of ALPM, AS SDP.
> Add compute config during atomic-check phase.
>
> v1: RFC version.
> v2: Add separate flag for auxless-alpm. [Jani]
>
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_alpm.c | 44
> +++
> drivers/gpu/drm/i915/display/intel_alpm.h | 5 +++
> .../drm/i915/display/intel_display_types.h | 4 ++
> drivers/gpu/drm/i915/display/intel_dp.c | 5 +++
> 4 files changed, 58 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 13bac3e8c8fa..699f2f051766 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -11,6 +11,16 @@
> #include "intel_dp_aux.h"
> #include "intel_psr_regs.h"
>
> +bool intel_dp_get_aux_less_alpm_status(struct intel_dp *intel_dp)
> +{
> + u8 alpm_caps = 0;
> +
> + if (drm_dp_dpcd_readb(_dp->aux, DP_RECEIVER_ALPM_CAP,
> + _caps) != 1)
> + return false;
> + return alpm_caps & DP_ALPM_AUX_LESS_CAP;
> +}
> +
> /*
> * See Bspec: 71632 for the table
> *
> @@ -242,6 +252,40 @@ bool intel_alpm_compute_params(struct intel_dp
> *intel_dp,
> return true;
> }
>
> +void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
> + struct intel_crtc_state
> *crtc_state,
> + struct drm_connector_state
> *conn_state)
> +{
> + struct drm_display_mode *adjusted_mode = _state-
> >hw.adjusted_mode;
> + int waketime_in_lines, first_sdp_position;
> + int context_latency, guardband;
> +
> + intel_dp->lobf_supported = false;
> +
> + if (!intel_dp_is_edp(intel_dp))
> + return;
> +
> + if (!intel_dp_as_sdp_supported(intel_dp))
> + return;
> +
> + if (crtc_state->has_psr2 || crtc_state->has_panel_replay)
> + return;
LOBF is not supported with PSR1? I think checking crtc_state->has_psr
is enough. That covers PSR1/2 and Panel Replay.
> +
> + if (intel_alpm_compute_params(intel_dp, crtc_state)) {
> + context_latency = adjusted_mode->crtc_vblank_start -
> adjusted_mode->crtc_vdisplay;
> + guardband = adjusted_mode->crtc_vtotal -
> + adjusted_mode->crtc_vdisplay -
> context_latency;
> + first_sdp_position = adjusted_mode->crtc_vtotal -
> adjusted_mode->crtc_vsync_start;
> + if (intel_dp->alpm_parameters.auxless_alpm_supported)
> + waketime_in_lines = intel_dp-
> >alpm_parameters.io_wake_lines;
> + else
> + waketime_in_lines = intel_dp-
> >alpm_parameters.fast_wake_lines;
> +
> + if ((context_latency + guardband) >
> (first_sdp_position + waketime_in_lines))
> + intel_dp->lobf_supported = true;
> + }
You are not checking display version here. This is supported only on
LNL and onwards.
> +}
> +
> static void lnl_alpm_configure(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> b/drivers/gpu/drm/i915/display/intel_alpm.h
> index c45d078e5a6b..c341d2c2b7f7 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -10,9 +10,14 @@
>
> struct intel_dp;
> struct intel_crtc_state;
> +struct drm_connector_state;
>
> +bool intel_dp_get_aux_less_alpm_status(struct intel_dp *intel_dp);
> bool intel_alpm_compute_params(struct intel_dp *intel_dp,
> struct intel_crtc_state *crtc_state);
> +void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
> + struct intel_crtc_state
> *crtc_state,
> + struct drm_connector_state
> *conn_state);
> void intel_alpm_configure(struct intel_dp *intel_dp);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 73197f014510..6116c383b543 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1876,10 +1876,14 @@ struct intel_dp {
> u8 fast_wake_lines;
>
> /* LNL and beyond */
> + bool auxless_alpm_supported;
> u8 check_entry_lines;
> u8 silence_period_sym_clocks;
> u8 lfps_half_cycle_num_of_syms;
> } alpm_parameters;
> +
> + /* LOBF flags*/
> + bool lobf_supported;
I think having it here and naming like this is misleading.
Re: [PATCH v7 6/6] drm/i915: Implement fbdev emulation as in-kernel client
On Fri, 2024-04-05 at 10:59 +0200, Thomas Zimmermann wrote: > Hi > > Am 05.04.24 um 10:34 schrieb Hogander, Jouni: > [...] > > > > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > > > b/drivers/gpu/drm/i915/i915_driver.c > > > index e0f13c62a1832..69178b73845e1 100644 > > > --- a/drivers/gpu/drm/i915/i915_driver.c > > > +++ b/drivers/gpu/drm/i915/i915_driver.c > > > @@ -816,6 +816,8 @@ int i915_driver_probe(struct pci_dev *pdev, > > > const > > > struct pci_device_id *ent) > > > > > > i915->do_release = true; > > > > > > + intel_fbdev_setup(i915); > > > + > > This doesn't work for Xe. I propose you move it to > > drivers/gpu/drm/i915/display/intel_display_dirver.c:intel_display_d > > rive > > r_probe? Otherwise patch looks ok to me. > > Can you say why it doesn't work? It's been a while, but IIRC I ran > this > patch on xe for testing. i915_driver_probe is not used by Xe driver and I can't find own call to intel_fbdev_setup in Xe driver. BR, Jouni Högander > > Best regards > Thomas > > > > > BR, > > > > Jouni Högander > > > > > > > return 0; > > > > > > out_cleanup_gem: > > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c > > > b/drivers/gpu/drm/xe/display/xe_display.c > > > index cdbc3f04c80a7..ca5cbe1d8a03b 100644 > > > --- a/drivers/gpu/drm/xe/display/xe_display.c > > > +++ b/drivers/gpu/drm/xe/display/xe_display.c > > > @@ -214,9 +214,7 @@ void xe_display_fini(struct xe_device *xe) > > > if (!xe->info.enable_display) > > > return; > > > > > > - /* poll work can call into fbdev, hence clean that up > > > afterwards */ > > > intel_hpd_poll_fini(xe); > > > - intel_fbdev_fini(xe); > > > > > > intel_hdcp_component_fini(xe); > > > intel_audio_deinit(xe); >
Re: [PATCH v7 4/6] drm/i915: Initialize fbdev DRM client with callback functions
On Fri, 2024-03-01 at 14:42 +0100, Thomas Zimmermann wrote:
> Initialize i915's fbdev client by giving an instance of struct
> drm_client_funcs to drm_client_init(). Also clean up with
> drm_client_release().
>
> Doing this in i915 prevents fbdev helpers from initializing and
> releasing the client internally (see drm_fb_helper_init()). No
> functional change yet; the client callbacks will be filled later.
>
> v6:
> * rename client to "intel-fbdev" (Jouni)
> v2:
> * call drm_fb_helper_unprepare() in error handling (Jani)
> * fix typo in commit message (Sam)
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Jouni Högander
> ---
> drivers/gpu/drm/i915/display/intel_fbdev.c | 43
> --
> 1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 76c0e89bf25e8..32aeb5faf706b 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -286,6 +286,7 @@ static void intel_fbdev_destroy(struct
> intel_fbdev *ifbdev)
> if (ifbdev->fb)
> drm_framebuffer_remove(>fb->base);
>
> + drm_client_release(>helper.client);
> drm_fb_helper_unprepare(>helper);
> kfree(ifbdev);
> }
> @@ -579,6 +580,30 @@ void intel_fbdev_restore_mode(struct
> drm_i915_private *dev_priv)
> intel_fbdev_invalidate(ifbdev);
> }
>
> +/*
> + * Fbdev client and struct drm_client_funcs
> + */
> +
> +static void intel_fbdev_client_unregister(struct drm_client_dev
> *client)
> +{ }
> +
> +static int intel_fbdev_client_restore(struct drm_client_dev *client)
> +{
> + return 0;
> +}
> +
> +static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
> +{
> + return 0;
> +}
> +
> +static const struct drm_client_funcs intel_fbdev_client_funcs = {
> + .owner = THIS_MODULE,
> + .unregister = intel_fbdev_client_unregister,
> + .restore= intel_fbdev_client_restore,
> + .hotplug= intel_fbdev_client_hotplug,
> +};
> +
> int intel_fbdev_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -600,16 +625,26 @@ int intel_fbdev_init(struct drm_device *dev)
> else
> ifbdev->preferred_bpp = ifbdev->helper.preferred_bpp;
>
> + ret = drm_client_init(dev, >helper.client, "intel-
> fbdev",
> + _fbdev_client_funcs);
> + if (ret)
> + goto err_drm_fb_helper_unprepare;
> +
> ret = drm_fb_helper_init(dev, >helper);
> - if (ret) {
> - kfree(ifbdev);
> - return ret;
> - }
> + if (ret)
> + goto err_drm_client_release;
>
> dev_priv->display.fbdev.fbdev = ifbdev;
> INIT_WORK(_priv->display.fbdev.suspend_work,
> intel_fbdev_suspend_worker);
>
> return 0;
> +
> +err_drm_client_release:
> + drm_client_release(>helper.client);
> +err_drm_fb_helper_unprepare:
> + drm_fb_helper_unprepare(>helper);
> + kfree(ifbdev);
> + return ret;
> }
>
> static void intel_fbdev_initial_config(void *data, async_cookie_t
> cookie)
Re: [PATCH v7 6/6] drm/i915: Implement fbdev emulation as in-kernel client
On Fri, 2024-03-01 at 14:42 +0100, Thomas Zimmermann wrote: > Replace all code that initializes or releases fbdev emulation > throughout the driver. Instead initialize the fbdev client by a > single call to intel_fbdev_setup() after i915 has registered its > DRM device. Just like similar code in other drivers, i915 fbdev > emulation now acts like a regular DRM client. > > The fbdev client setup consists of the initial preparation and the > hot-plugging of the display. The latter creates the fbdev device > and sets up the fbdev framebuffer. The setup performs display > hot-plugging once. If no display can be detected, DRM probe helpers > re-run the detection on each hotplug event. > > A call to drm_client_dev_unregister() releases all in-kernel clients > automatically. No further action is required within i915. If the > fbdev > framebuffer has been fully set up, struct fb_ops.fb_destroy > implements > the release. For partially initialized emulation, the fbdev client > reverts the initial setup. Do the same for xe and remove its call to > intel_fbdev_fini(). > > v7: > * update xe driver > * reword commit message > v6: > * use 'i915' for i915 device (Jouni) > * remove unnecessary code for non-atomic mode setting > (Jouni, Ville) > * fix function name in commit message (Jouni) > v3: > * as before, silently ignore devices without displays > v2: > * let drm_client_register() handle initial hotplug > * fix driver name in error message (Jani) > * fix non-fbdev build (kernel test robot) > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/i915/display/intel_display.c | 1 - > .../drm/i915/display/intel_display_driver.c | 18 -- > drivers/gpu/drm/i915/display/intel_fbdev.c | 177 > -- > drivers/gpu/drm/i915/display/intel_fbdev.h | 20 +- > drivers/gpu/drm/i915/i915_driver.c | 2 + > drivers/gpu/drm/xe/display/xe_display.c | 2 - > 6 files changed, 80 insertions(+), 140 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index ab2f52d21bad8..4ef1e73cffd1e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -85,7 +85,6 @@ > #include "intel_dvo.h" > #include "intel_fb.h" > #include "intel_fbc.h" > -#include "intel_fbdev.h" > #include "intel_fdi.h" > #include "intel_fifo_underrun.h" > #include "intel_frontbuffer.h" > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > b/drivers/gpu/drm/i915/display/intel_display_driver.c > index ca92c48fbdc49..6e875cacf0585 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > @@ -513,10 +513,6 @@ int intel_display_driver_probe(struct > drm_i915_private *i915) > > intel_overlay_setup(i915); > > - ret = intel_fbdev_init(>drm); > - if (ret) > - return ret; > - > /* Only enable hotplug handling once the fbdev is fully set > up. */ > intel_hpd_init(i915); > > @@ -543,16 +539,6 @@ void intel_display_driver_register(struct > drm_i915_private *i915) > > intel_display_debugfs_register(i915); > > - /* > - * Some ports require correctly set-up hpd registers for > - * detection to work properly (leading to ghost connected > - * connector status), e.g. VGA on gm45. Hence we can only > set > - * up the initial fbdev config after hpd irqs are fully > - * enabled. We do it last so that the async config cannot run > - * before the connectors are registered. > - */ > - intel_fbdev_initial_config_async(i915); > - > /* > * We need to coordinate the hotplugs with the asynchronous > * fbdev configuration, for which we use the > @@ -596,9 +582,6 @@ void intel_display_driver_remove_noirq(struct > drm_i915_private *i915) > */ > intel_hpd_poll_fini(i915); > > - /* poll work can call into fbdev, hence clean that up > afterwards */ > - intel_fbdev_fini(i915); > - > intel_unregister_dsm_handler(); > > /* flush any delayed tasks or pending work */ > @@ -637,7 +620,6 @@ void intel_display_driver_unregister(struct > drm_i915_private *i915) > if (!HAS_DISPLAY(i915)) > return; > > - intel_fbdev_unregister(i915); > /* > * After flushing the fbdev (incl. a late async config which > * will have delayed queuing of a hotplug event), then flush > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c > b/drivers/gpu/drm/i915/display/intel_fbdev.c > index 938ee709813df..062f4f937cb1c 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > @@ -24,7 +24,6 @@ > * David Airlie > */ > > -#include > #include > #include >
Re: [PATCH v7 5/6] drm/i915: Implement fbdev client callbacks
On Fri, 2024-03-01 at 14:42 +0100, Thomas Zimmermann wrote:
> Move code from ad-hoc fbdev callbacks into DRM client functions
> and remove the old callbacks. The functions instruct the client
> to poll for changed output or restore the display.
>
> The DRM core calls both, the old callbacks and the new client
> helpers, from the same places. The new functions perform the same
> operation as before, so there's no change in functionality.
>
> Fox xe, remove xe_display_last_close(), which restored the fbdev
> display. As with i915, the DRM core's drm_lastclose() performs
> this operation automatically.
>
> v7:
> * update xe driver
> v6:
> * return errors from client callbacks (Jouni)
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Jouni Högander
> ---
> .../drm/i915/display/intel_display_driver.c | 1 -
> drivers/gpu/drm/i915/display/intel_fbdev.c | 33 ++---
> --
> drivers/gpu/drm/i915/display/intel_fbdev.h | 9 -
> drivers/gpu/drm/i915/i915_driver.c | 22 -
> drivers/gpu/drm/xe/display/xe_display.c | 9 -
> 5 files changed, 25 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
> b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 87dd07e0d138d..ca92c48fbdc49 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -98,7 +98,6 @@ void intel_display_driver_init_hw(struct
> drm_i915_private *i915)
> static const struct drm_mode_config_funcs intel_mode_funcs = {
> .fb_create = intel_user_framebuffer_create,
> .get_format_info = intel_fb_get_format_info,
> - .output_poll_changed = intel_fbdev_output_poll_changed,
> .mode_valid = intel_mode_valid,
> .atomic_check = intel_atomic_check,
> .atomic_commit = intel_atomic_commit,
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 32aeb5faf706b..938ee709813df 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -546,13 +546,13 @@ void intel_fbdev_set_suspend(struct drm_device
> *dev, int state, bool synchronous
> intel_fbdev_hpd_set_suspend(dev_priv, state);
> }
>
> -void intel_fbdev_output_poll_changed(struct drm_device *dev)
> +static int intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> struct intel_fbdev *ifbdev = to_i915(dev)-
> >display.fbdev.fbdev;
> bool send_hpd;
>
> if (!ifbdev)
> - return;
> + return -EINVAL;
>
> intel_fbdev_sync(ifbdev);
>
> @@ -563,21 +563,29 @@ void intel_fbdev_output_poll_changed(struct
> drm_device *dev)
>
> if (send_hpd && (ifbdev->vma || ifbdev-
> >helper.deferred_setup))
> drm_fb_helper_hotplug_event(>helper);
> +
> + return 0;
> }
>
> -void intel_fbdev_restore_mode(struct drm_i915_private *dev_priv)
> +static int intel_fbdev_restore_mode(struct drm_i915_private
> *dev_priv)
> {
> struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
> + int ret;
>
> if (!ifbdev)
> - return;
> + return -EINVAL;
>
> intel_fbdev_sync(ifbdev);
> if (!ifbdev->vma)
> - return;
> + return -ENOMEM;
>
> - if (drm_fb_helper_restore_fbdev_mode_unlocked(
> >helper) == 0)
> - intel_fbdev_invalidate(ifbdev);
> + ret = drm_fb_helper_restore_fbdev_mode_unlocked(
> >helper);
> + if (ret)
> + return ret;
> +
> + intel_fbdev_invalidate(ifbdev);
> +
> + return 0;
> }
>
> /*
> @@ -589,12 +597,21 @@ static void
> intel_fbdev_client_unregister(struct drm_client_dev *client)
>
> static int intel_fbdev_client_restore(struct drm_client_dev *client)
> {
> + struct drm_i915_private *dev_priv = to_i915(client->dev);
> + int ret;
> +
> + ret = intel_fbdev_restore_mode(dev_priv);
> + if (ret)
> + return ret;
> +
> + vga_switcheroo_process_delayed_switch();
> +
> return 0;
> }
>
> static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
> {
> - return 0;
> + return intel_fbdev_output_poll_changed(client->dev);
> }
>
> static const struct drm_client_funcs intel_fbdev_client_funcs = {
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.h
> b/drivers/gpu/drm/i915/display/intel_fbdev.h
> index 04fd523a50232..8c953f102ba22 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.h
> @@ -19,8 +19,6 @@ void intel_fbdev_initial_config_async(struct
> drm_i915_private *dev_priv);
> void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
> void intel_fbdev_fini(struct drm_i915_private *dev_priv);
> void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool
>
Re: [PATCH 02/19] drm/dp: Add support for DP tunneling
On Tue, 2024-01-23 at 12:28 +0200, Imre Deak wrote: > Add support for Display Port DP tunneling. For now this includes the > support for Bandwidth Allocation Mode, leaving adding Panel Replay > support for later. > > BWA allows using displays that share the same (Thunderbolt) link with > their maximum resolution. Atm, this may not be possible due to the > coarse granularity of partitioning the link BW among the displays on > the > link: the BW allocation policy is in a SW/FW/HW component on the link > (on Thunderbolt it's the SW or FW Connection Manager), independent of > the driver. This policy will set the DPRX maximum rate and lane count > DPCD registers the GFX driver will see (0x0, 0x1, 0x02200, > 0x02201) based on the available link BW. > > The granularity of the current BW allocation policy is course, based > on > the required link rate in the 1.62Gbs..8.1Gbps range and it may > prevent > using higher resolutions all together: the display connected first > will > get a share of the link BW which corresponds to its full DPRX > capability > (regardless of the actual mode it uses). A subsequent display > connected > will only get the remaining BW, which could be well below its full > capability. > > BWA solves the above course granularity (reducing it to a > 250Mbs..1Gps > range) and first-come/first-served issues by letting the driver > request > the BW for each display on a link which reflects the actual modes the > displays use. > > This patch adds the DRM core helper functions, while a follow-up > change > in the patchset takes them into use in the i915 driver. > > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/display/Kconfig | 17 + > drivers/gpu/drm/display/Makefile | 2 + > drivers/gpu/drm/display/drm_dp_tunnel.c | 1715 > +++ > include/drm/display/drm_dp.h | 60 + > include/drm/display/drm_dp_tunnel.h | 270 > 5 files changed, 2064 insertions(+) > create mode 100644 drivers/gpu/drm/display/drm_dp_tunnel.c > create mode 100644 include/drm/display/drm_dp_tunnel.h > > diff --git a/drivers/gpu/drm/display/Kconfig > b/drivers/gpu/drm/display/Kconfig > index 09712b88a5b83..b024a84b94c1c 100644 > --- a/drivers/gpu/drm/display/Kconfig > +++ b/drivers/gpu/drm/display/Kconfig > @@ -17,6 +17,23 @@ config DRM_DISPLAY_DP_HELPER > help > DRM display helpers for DisplayPort. > > +config DRM_DISPLAY_DP_TUNNEL > + bool > + select DRM_DISPLAY_DP_HELPER > + help > + Enable support for DisplayPort tunnels. > + > +config DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE > + bool "Enable debugging the DP tunnel state" > + depends on REF_TRACKER > + depends on DRM_DISPLAY_DP_TUNNEL > + depends on DEBUG_KERNEL > + depends on EXPERT > + help > + Enables debugging the DP tunnel manager's status. > + > + If in doubt, say "N". > + > config DRM_DISPLAY_HDCP_HELPER > bool > depends on DRM_DISPLAY_HELPER > diff --git a/drivers/gpu/drm/display/Makefile > b/drivers/gpu/drm/display/Makefile > index 17ac4a1006a80..7ca61333c6696 100644 > --- a/drivers/gpu/drm/display/Makefile > +++ b/drivers/gpu/drm/display/Makefile > @@ -8,6 +8,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_HELPER) += > \ > drm_dp_helper.o \ > drm_dp_mst_topology.o \ > drm_dsc_helper.o > +drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_TUNNEL) += \ > + drm_dp_tunnel.o > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += > drm_hdcp_helper.o > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \ > drm_hdmi_helper.o \ > diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c > b/drivers/gpu/drm/display/drm_dp_tunnel.c > new file mode 100644 > index 0..58f6330db7d9d > --- /dev/null > +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c > @@ -0,0 +1,1715 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2023 Intel Corporation > + */ > + > +#include > +#include > + > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +#define to_group(__private_obj) \ > + container_of(__private_obj, struct drm_dp_tunnel_group, base) > + > +#define to_group_state(__private_state) \ > + container_of(__private_state, struct > drm_dp_tunnel_group_state, base) > + > +#define is_dp_tunnel_private_obj(__obj) \ > + ((__obj)->funcs == _group_funcs) > + > +#define for_each_new_group_in_state(__state, __new_group_state, __i) > \ > + for ((__i) = 0; \ > + (__i) < (__state)->num_private_objs; \ > + (__i)++) \ > + for_each_if ((__state)->private_objs[__i].ptr && \ > + is_dp_tunnel_private_obj((__state)- > >private_objs[__i].ptr) && \ > + ((__new_group_state) = \ > + to_group_state((__state)- > >private_objs[__i].new_state), 1)) > + > +#define for_each_old_group_in_state(__state,
Re: [PATCH 03/19] drm/i915/dp: Add support to notify MST connectors to retry modesets
On Tue, 2024-01-23 at 12:28 +0200, Imre Deak wrote:
> On shared (Thunderbolt) links with DP tunnels, the modeset may need
> to
> be retried on all connectors on the link due to a link BW limitation
> arising only after the atomic check phase. To support this add a
> helper
> function queuing a work to retry the modeset on a given port's
> connector
> and at the same time any MST connector with streams through the same
> port. A follow-up change enabling the DP tunnel Bandwidth Allocation
> Mode will take this into use.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 5 +-
> drivers/gpu/drm/i915/display/intel_dp.c | 55
> ++-
> drivers/gpu/drm/i915/display/intel_dp.h | 8 +++
> .../drm/i915/display/intel_dp_link_training.c | 3 +-
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +
> 5 files changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index a92e959c8ac7b..0caebbb3e2dbb 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8060,8 +8060,9 @@ void intel_hpd_poll_fini(struct
> drm_i915_private *i915)
> /* Kill all the work that may have been queued by hpd. */
> drm_connector_list_iter_begin(>drm, _iter);
> for_each_intel_connector_iter(connector, _iter) {
> - if (connector->modeset_retry_work.func)
> - cancel_work_sync(
> >modeset_retry_work);
> + if (connector->modeset_retry_work.func &&
> + cancel_work_sync(>modeset_retry_work))
> + drm_connector_put(>base);
> if (connector->hdcp.shim) {
> cancel_delayed_work_sync(
> >hdcp.check_work);
> cancel_work_sync(>hdcp.prop_work);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index ab415f41924d7..4e36c2c39888e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2837,6 +2837,50 @@ intel_dp_audio_compute_config(struct
> intel_encoder *encoder,
> intel_dp_is_uhbr(pipe_config)
> ;
> }
>
> +void intel_dp_queue_modeset_retry_work(struct intel_connector
> *connector)
> +{
> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
> +
> + drm_connector_get(>base);
> + if (!queue_work(i915->unordered_wq,
> >modeset_retry_work))
> + drm_connector_put(>base);
> +}
> +
> +void
> +intel_dp_queue_modeset_retry_for_link(struct intel_atomic_state
> *state,
> + struct intel_encoder *encoder,
> + const struct intel_crtc_state
> *crtc_state,
> + const struct
> drm_connector_state *conn_state)
> +{
> + struct drm_i915_private *i915 = to_i915(crtc_state-
> >uapi.crtc->dev);
> + struct intel_connector *connector;
> + struct intel_digital_connector_state *iter_conn_state;
> + struct intel_dp *intel_dp;
> + int i;
> +
> + if (conn_state) {
> + connector = to_intel_connector(conn_state-
> >connector);
> + intel_dp_queue_modeset_retry_work(connector);
> +
> + return;
> + }
> +
> + if (drm_WARN_ON(>drm,
> + !intel_crtc_has_type(crtc_state,
> INTEL_OUTPUT_DP_MST)))
> + return;
> +
> + intel_dp = enc_to_intel_dp(encoder);
> +
> + for_each_new_intel_connector_in_state(state, connector,
> iter_conn_state, i) {
> + (void)iter_conn_state;
Checked iter_conn_state->base->crtc documentation:
@crtc: CRTC to connect connector to, NULL if disabled.
Do we need to check if connector is "disabled" or is it impossible
scenario?
BR,
Jouni Högander
> +
> + if (connector->mst_port != intel_dp)
> + continue;
> +
> + intel_dp_queue_modeset_retry_work(connector);
> + }
> +}
> +
> int
> intel_dp_compute_config(struct intel_encoder *encoder,
> struct intel_crtc_state *pipe_config,
> @@ -6436,6 +6480,14 @@ static void
> intel_dp_modeset_retry_work_fn(struct work_struct *work)
> mutex_unlock(>dev->mode_config.mutex);
> /* Send Hotplug uevent so userspace can reprobe */
> drm_kms_helper_connector_hotplug_event(connector);
> +
> + drm_connector_put(connector);
> +}
> +
> +void intel_dp_init_modeset_retry_work(struct intel_connector
> *connector)
> +{
> + INIT_WORK(>modeset_retry_work,
> + intel_dp_modeset_retry_work_fn);
> }
>
> bool
> @@ -6452,8 +6504,7 @@ intel_dp_init_connector(struct
> intel_digital_port *dig_port,
> int type;
>
> /* Initialize the work for modeset in case of link train
>
Re: [PATCH 10/12] drm/panelreplay: dpcd register definition for panelreplay SU
On Thu, 2024-01-04 at 12:59 +0200, Dmitry Baryshkov wrote: > On Thu, 4 Jan 2024 at 12:49, Jouni Högander > wrote: > > > > Add definitions for panel replay selective update > > > > Cc: dri-devel@lists.freedesktop.org > > > > 1) This CC should not be necessary. It is already a part of > maintainers entry for this file > > 2) It probably doesn't work as expected. It is separated with the > blank link from the rest of the trailers, so most of the tools will > skip it. > > 3) You have skipped the rest of the maintainers for this file. Please > use ./scripts/get_maintainers.pl and pass corresponding options to > git > send-email. Thank you Dmitry for checking my patch. Sent now version 2. of my patch set. There I took care of your suggestions in patch 11. BR, Jouni Högander > > > Signed-off-by: Jouni Högander > > --- > > include/drm/display/drm_dp.h | 6 ++ > > 1 file changed, 6 insertions(+) > > The patch itself looks good to me. > > > > > diff --git a/include/drm/display/drm_dp.h > > b/include/drm/display/drm_dp.h > > index 3731828825bd..6a59d30b7b25 100644 > > --- a/include/drm/display/drm_dp.h > > +++ b/include/drm/display/drm_dp.h > > @@ -548,6 +548,12 @@ > > # define DP_PANEL_REPLAY_SUPPORT (1 << 0) > > # define DP_PANEL_REPLAY_SU_SUPPORT (1 << 1) > > > > +#define DP_PANEL_PANEL_REPLAY_CAPABILITY 0xb1 > > +# define DP_PANEL_PANEL_REPLAY_SU_GRANULARITY_REQUIRED (1 << 5) > > + > > +#define DP_PANEL_PANEL_REPLAY_X_GRANULARITY 0xb2 > > +#define DP_PANEL_PANEL_REPLAY_Y_GRANULARITY 0xb4 > > + > > /* Link Configuration */ > > #define DP_LINK_BW_SET 0x100 > > # define DP_LINK_RATE_TABLE 0x00 /* eDP 1.4 */ > > -- > > 2.34.1 > > > >
Re: [PATCH 1/7] drm: Add eDP 1.5 early transport definition
Hello All, I accidentally pushed this patch into drm-intel/drm-intel-next. Hopefully this doesn't cause problems. I'm very sorry for inconvenience. Best Regards, Jouni Högander On Wed, 2024-01-03 at 10:46 +, Kahola, Mika wrote: > > -Original Message- > > From: Intel-gfx On Behalf > > Of Jouni Högander > > Sent: Monday, December 18, 2023 7:50 PM > > To: intel-...@lists.freedesktop.org > > Cc: dri-devel@lists.freedesktop.org > > Subject: [PATCH 1/7] drm: Add eDP 1.5 early transport definition > > > > Add DP_PSR_ENABLE_SU_REGION_ET to enable panel early transport. > > > > Cc: dri-devel@lists.freedesktop.org > > > > Reviewed-by: Mika Kahola > > > Signed-off-by: Jouni Högander > > --- > > include/drm/display/drm_dp.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/drm/display/drm_dp.h > > b/include/drm/display/drm_dp.h index 3731828825bd..281afff6ee4e > > 100644 > > --- a/include/drm/display/drm_dp.h > > +++ b/include/drm/display/drm_dp.h > > @@ -718,6 +718,7 @@ > > # define DP_PSR_SU_REGION_SCANLINE_CAPTURE BIT(4) /* eDP 1.4a > > */ > > # define DP_PSR_IRQ_HPD_WITH_CRC_ERRORSBIT(5) /* > > eDP 1.4a */ > > # define DP_PSR_ENABLE_PSR2BIT(6) /* eDP 1.4a > > */ > > +# define DP_PSR_ENABLE_SU_REGION_ET BIT(7) /* eDP 1.5 > > */ > > > > #define DP_ADAPTER_CTRL 0x1a0 > > # define DP_ADAPTER_CTRL_FORCE_LOAD_SENSE (1 << 0) > > -- > > 2.34.1 >
Re: [PATCH] drm/i915/display: Use i915_gem_object_get_dma_address to get dma address
On Mon, 2023-12-04 at 14:49 +0100, Maarten Lankhorst wrote:
> Works better for xe like that. obj is no longer const.
>
> Signed-off-by: Maarten Lankhorst
Reviewed-by: Jouni Högander
> ---
> drivers/gpu/drm/i915/display/intel_cursor.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index a515ae2831f8..926e2de00eb5 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -24,6 +24,8 @@
> #include "intel_psr_regs.h"
> #include "skl_watermark.h"
>
> +#include "gem/i915_gem_object.h"
> +
> /* Cursor formats */
> static const u32 intel_cursor_formats[] = {
> DRM_FORMAT_ARGB,
> @@ -34,11 +36,11 @@ static u32 intel_cursor_base(const struct
> intel_plane_state *plane_state)
> struct drm_i915_private *dev_priv =
> to_i915(plane_state->uapi.plane->dev);
> const struct drm_framebuffer *fb = plane_state->hw.fb;
> - const struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> + struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> u32 base;
>
> if (DISPLAY_INFO(dev_priv)->cursor_needs_physical)
> - base = sg_dma_address(obj->mm.pages->sgl);
> + base = i915_gem_object_get_dma_address(obj, 0);
> else
> base = intel_plane_ggtt_offset(plane_state);
>
Re: [PATCH v9 6/6] drm/i915/panelreplay: Debugfs support for panel replay
On Wed, 2023-11-08 at 12:53 +0530, Animesh Manna wrote:
> Add debugfs support which will print source and sink status
> per connector basis. Existing i915_psr_status and
> i915_psr_sink_status will be used to get the source and
> sink status of panel replay.
>
> v1: Initial version. [rb-ed by Arun]
> v2: Added check for DP 2.0 and connector type in
> connector_debugfs_add().
> v3: Optimization and cosmetic changes. [Jouni]
>
> Cc: Jouni Högander
> Cc: Arun R Murthy
> Cc: Jani Nikula
> Reviewed-by: Arun R Murthy
> Signed-off-by: Animesh Manna
Reviewed-by: Jouni Högander
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 87 +-
> --
> 1 file changed, 63 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index ea292832ca47..b0e46fe4bfac 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -2865,12 +2865,19 @@ static int
> psr_get_status_and_error_status(struct intel_dp *intel_dp,
> {
> struct drm_dp_aux *aux = _dp->aux;
> int ret;
> + unsigned int offset;
>
> - ret = drm_dp_dpcd_readb(aux, DP_PSR_STATUS, status);
> + offset = intel_dp->psr.panel_replay_enabled ?
> + DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS :
> DP_PSR_STATUS;
> +
> + ret = drm_dp_dpcd_readb(aux, offset, status);
> if (ret != 1)
> return ret;
>
> - ret = drm_dp_dpcd_readb(aux, DP_PSR_ERROR_STATUS,
> error_status);
> + offset = intel_dp->psr.panel_replay_enabled ?
> + DP_PANEL_REPLAY_ERROR_STATUS : DP_PSR_ERROR_STATUS;
> +
> + ret = drm_dp_dpcd_readb(aux, offset, error_status);
> if (ret != 1)
> return ret;
>
> @@ -3091,7 +3098,7 @@ psr_source_status(struct intel_dp *intel_dp,
> struct seq_file *m)
> status = live_status[status_val];
> }
>
> - seq_printf(m, "Source PSR status: %s [0x%08x]\n", status,
> val);
> + seq_printf(m, "Source PSR/PanelReplay status: %s [0x%08x]\n",
> status, val);
> }
>
> static int intel_psr_status(struct seq_file *m, struct intel_dp
> *intel_dp)
> @@ -3104,18 +3111,22 @@ static int intel_psr_status(struct seq_file
> *m, struct intel_dp *intel_dp)
> bool enabled;
> u32 val;
>
> - seq_printf(m, "Sink support: %s", str_yes_no(psr-
> >sink_support));
> + seq_printf(m, "Sink support: PSR = %s",
> + str_yes_no(psr->sink_support));
> +
> if (psr->sink_support)
> seq_printf(m, " [0x%02x]", intel_dp->psr_dpcd[0]);
> - seq_puts(m, "\n");
> + seq_printf(m, ", Panel Replay = %s\n", str_yes_no(psr-
> >sink_panel_replay_support));
>
> - if (!psr->sink_support)
> + if (!(psr->sink_support || psr->sink_panel_replay_support))
> return 0;
>
> wakeref = intel_runtime_pm_get(_priv->runtime_pm);
> mutex_lock(>lock);
>
> - if (psr->enabled)
> + if (psr->panel_replay_enabled)
> + status = "Panel Replay Enabled";
> + else if (psr->enabled)
> status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1
> enabled";
> else
> status = "disabled";
> @@ -3128,14 +3139,17 @@ static int intel_psr_status(struct seq_file
> *m, struct intel_dp *intel_dp)
> goto unlock;
> }
>
> - if (psr->psr2_enabled) {
> + if (psr->panel_replay_enabled) {
> + val = intel_de_read(dev_priv,
> TRANS_DP2_CTL(cpu_transcoder));
> + enabled = val & TRANS_DP2_PANEL_REPLAY_ENABLE;
> + } else if (psr->psr2_enabled) {
> val = intel_de_read(dev_priv,
> EDP_PSR2_CTL(cpu_transcoder));
> enabled = val & EDP_PSR2_ENABLE;
> } else {
> val = intel_de_read(dev_priv, psr_ctl_reg(dev_priv,
> cpu_transcoder));
> enabled = val & EDP_PSR_ENABLE;
> }
> - seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> + seq_printf(m, "Source PSR/PanelReplay ctl: %s [0x%08x]\n",
> str_enabled_disabled(enabled), val);
> psr_source_status(intel_dp, m);
> seq_printf(m, "Busy frontbuffer bits: 0x%08x\n",
> @@ -3273,6 +3287,16 @@ void intel_psr_debugfs_register(struct
> drm_i915_private *i915)
> i915, _edp_psr_status_fops);
> }
>
> +static const char *psr_mode_str(struct intel_dp *intel_dp)
> +{
> + if (intel_dp->psr.panel_replay_enabled)
> + return "PANEL-REPLAY";
> + else if (intel_dp->psr.enabled)
> + return "PSR";
> +
> + return "unknown";
> +}
> +
> static int i915_psr_sink_status_show(struct seq_file *m, void *data)
> {
> struct intel_connector *connector = m->private;
> @@ -3287,12 +3311,19 @@ static int i915_psr_sink_status_show(struct
> seq_file *m, void *data)
>
Re: [PATCH v8 6/6] drm/i915/panelreplay: Debugfs support for panel replay
On Tue, 2023-11-07 at 05:01 +, Manna, Animesh wrote:
>
>
> > -Original Message-
> > From: Hogander, Jouni
> > Sent: Monday, November 6, 2023 1:33 PM
> > To: dri-devel@lists.freedesktop.org; Manna, Animesh
> > ; intel-...@lists.freedesktop.org
> > Cc: Murthy, Arun R ; Nikula, Jani
> >
> > Subject: Re: [PATCH v8 6/6] drm/i915/panelreplay: Debugfs support
> > for
> > panel replay
> >
> > Hello Animesh,
> >
> > Thank you for the changes. Now the patch is much shorter, cleaner
> > and
> > easier to review. See my inline comments below.
> >
> > On Sat, 2023-11-04 at 02:30 +0530, Animesh Manna wrote:
> > > Add debugfs support which will print source and sink status per
> > > connector basis. Existing i915_psr_status and
> > > i915_psr_sink_status
> > > will be used to get the source and sink status of panel replay.
> > >
> > > v1: Initial version. [rb-ed by Arun]
> > > v2: Added check for DP 2.0 and connector type in
> > > connector_debugfs_add().
> > > v3: Optimization and cosmetic changes. [Jouni]
> > >
> > > Cc: Jouni Högander
> > > Cc: Arun R Murthy
> > > Cc: Jani Nikula
> > > Reviewed-by: Arun R Murthy
> > > Signed-off-by: Animesh Manna
> > > ---
> > > drivers/gpu/drm/i915/display/intel_psr.c | 93 +-
> > >
> > > --
> > > 1 file changed, 66 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 8ed4684b4528..8b7c03cd4989 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -2813,12 +2813,19 @@ static int
> > > psr_get_status_and_error_status(struct intel_dp *intel_dp,
> > > {
> > > struct drm_dp_aux *aux = _dp->aux;
> > > int ret;
> > > + unsigned int offset;
> > >
> > > - ret = drm_dp_dpcd_readb(aux, DP_PSR_STATUS, status);
> > > + offset = intel_dp->psr.panel_replay_enabled ?
> > > + DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS :
> > > DP_PSR_STATUS;
> > > +
> > > + ret = drm_dp_dpcd_readb(aux, offset, status);
> > > if (ret != 1)
> > > return ret;
> > >
> > > - ret = drm_dp_dpcd_readb(aux, DP_PSR_ERROR_STATUS,
> > > error_status);
> > > + offset = intel_dp->psr.panel_replay_enabled ?
> > > + DP_PANEL_REPLAY_ERROR_STATUS :
> > > DP_PSR_ERROR_STATUS;
> > > +
> > > + ret = drm_dp_dpcd_readb(aux, offset, error_status);
> > > if (ret != 1)
> > > return ret;
> > >
> > > @@ -3039,7 +3046,7 @@ psr_source_status(struct intel_dp
> > > *intel_dp,
> > > struct seq_file *m)
> > > status = live_status[status_val];
> > > }
> > >
> > > - seq_printf(m, "Source PSR status: %s [0x%08x]\n", status,
> > > val);
> > > + seq_printf(m, "Source PSR/PanelReplay status: %s
> > > [0x%08x]\n",
> > > status, val);
> > > }
> > >
> > > static int intel_psr_status(struct seq_file *m, struct intel_dp
> > > *intel_dp)
> > > @@ -3052,18 +3059,23 @@ static int intel_psr_status(struct
> > > seq_file
> > > *m, struct intel_dp *intel_dp)
> > > bool enabled;
> > > u32 val;
> > >
> > > - seq_printf(m, "Sink support: %s", str_yes_no(psr-
> > > > sink_support));
> > > - if (psr->sink_support)
> > > + seq_printf(m, "Sink support: PSR = %s, Panel Replay =
> > > %s",
> > > + str_yes_no(psr->sink_support),
> > > + str_yes_no(psr->sink_panel_replay_support));
> > > +
> > > + if (psr->sink_support || psr->sink_panel_replay_support)
> > > seq_printf(m, " [0x%02x]", intel_dp-
> > > >psr_dpcd[0]);
> >
> > The output will look like this:
> >
> > Sink support: PSR = yes, Panel Replay = yes, [0x01]
> >
> > I think more logical would be:
> >
> > Sink support: PSR = yes [0x01], Panel Replay = yes
> >
> >
> > > seq_puts(m, "\n");
> > >
Re: [PATCH v8 6/6] drm/i915/panelreplay: Debugfs support for panel replay
Hello Animesh,
Thank you for the changes. Now the patch is much shorter, cleaner and
easier to review. See my inline comments below.
On Sat, 2023-11-04 at 02:30 +0530, Animesh Manna wrote:
> Add debugfs support which will print source and sink status
> per connector basis. Existing i915_psr_status and
> i915_psr_sink_status will be used to get the source and
> sink status of panel replay.
>
> v1: Initial version. [rb-ed by Arun]
> v2: Added check for DP 2.0 and connector type in
> connector_debugfs_add().
> v3: Optimization and cosmetic changes. [Jouni]
>
> Cc: Jouni Högander
> Cc: Arun R Murthy
> Cc: Jani Nikula
> Reviewed-by: Arun R Murthy
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 93 +-
> --
> 1 file changed, 66 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 8ed4684b4528..8b7c03cd4989 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -2813,12 +2813,19 @@ static int
> psr_get_status_and_error_status(struct intel_dp *intel_dp,
> {
> struct drm_dp_aux *aux = _dp->aux;
> int ret;
> + unsigned int offset;
>
> - ret = drm_dp_dpcd_readb(aux, DP_PSR_STATUS, status);
> + offset = intel_dp->psr.panel_replay_enabled ?
> + DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS :
> DP_PSR_STATUS;
> +
> + ret = drm_dp_dpcd_readb(aux, offset, status);
> if (ret != 1)
> return ret;
>
> - ret = drm_dp_dpcd_readb(aux, DP_PSR_ERROR_STATUS,
> error_status);
> + offset = intel_dp->psr.panel_replay_enabled ?
> + DP_PANEL_REPLAY_ERROR_STATUS : DP_PSR_ERROR_STATUS;
> +
> + ret = drm_dp_dpcd_readb(aux, offset, error_status);
> if (ret != 1)
> return ret;
>
> @@ -3039,7 +3046,7 @@ psr_source_status(struct intel_dp *intel_dp,
> struct seq_file *m)
> status = live_status[status_val];
> }
>
> - seq_printf(m, "Source PSR status: %s [0x%08x]\n", status,
> val);
> + seq_printf(m, "Source PSR/PanelReplay status: %s [0x%08x]\n",
> status, val);
> }
>
> static int intel_psr_status(struct seq_file *m, struct intel_dp
> *intel_dp)
> @@ -3052,18 +3059,23 @@ static int intel_psr_status(struct seq_file
> *m, struct intel_dp *intel_dp)
> bool enabled;
> u32 val;
>
> - seq_printf(m, "Sink support: %s", str_yes_no(psr-
> >sink_support));
> - if (psr->sink_support)
> + seq_printf(m, "Sink support: PSR = %s, Panel Replay = %s",
> + str_yes_no(psr->sink_support),
> + str_yes_no(psr->sink_panel_replay_support));
> +
> + if (psr->sink_support || psr->sink_panel_replay_support)
> seq_printf(m, " [0x%02x]", intel_dp->psr_dpcd[0]);
The output will look like this:
Sink support: PSR = yes, Panel Replay = yes, [0x01]
I think more logical would be:
Sink support: PSR = yes [0x01], Panel Replay = yes
> seq_puts(m, "\n");
>
> - if (!psr->sink_support)
> + if (!(psr->sink_support || psr->sink_panel_replay_support))
> return 0;
>
> wakeref = intel_runtime_pm_get(_priv->runtime_pm);
> mutex_lock(>lock);
>
> - if (psr->enabled)
> + if (psr->panel_replay_enabled)
> + status = "Panel Replay Enabled";
> + else if (psr->enabled)
> status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1
> enabled";
> else
> status = "disabled";
> @@ -3076,14 +3088,17 @@ static int intel_psr_status(struct seq_file
> *m, struct intel_dp *intel_dp)
> goto unlock;
> }
>
> - if (psr->psr2_enabled) {
> + if (psr->panel_replay_enabled) {
> + val = intel_de_read(dev_priv,
> TRANS_DP2_CTL(cpu_transcoder));
> + enabled = val & TRANS_DP2_PANEL_REPLAY_ENABLE;
> + } else if (psr->psr2_enabled) {
> val = intel_de_read(dev_priv,
> EDP_PSR2_CTL(cpu_transcoder));
> enabled = val & EDP_PSR2_ENABLE;
> } else {
> val = intel_de_read(dev_priv, psr_ctl_reg(dev_priv,
> cpu_transcoder));
> enabled = val & EDP_PSR_ENABLE;
> }
> - seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> + seq_printf(m, "Source PSR/PanelReplay ctl: %s [0x%08x]\n",
Use correct name here. Which is DP2 ctl.
> str_enabled_disabled(enabled), val);
> psr_source_status(intel_dp, m);
> seq_printf(m, "Busy frontbuffer bits: 0x%08x\n",
> @@ -3221,6 +3236,15 @@ void intel_psr_debugfs_register(struct
> drm_i915_private *i915)
> i915, _edp_psr_status_fops);
> }
>
> +static const char *psr_mode_str(struct intel_dp *intel_dp) {
> + if (intel_dp->psr.panel_replay_enabled)
> + return
Re: [PATCH v7 6/6] drm/i915/panelreplay: Debugfs support for panel replay
On Fri, 2023-11-03 at 06:10 +, Manna, Animesh wrote:
>
>
> > -Original Message-
> > From: Hogander, Jouni
> > Sent: Thursday, November 2, 2023 1:08 PM
> > To: Manna, Animesh ; intel-
> > g...@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org; Murthy, Arun R
> > ; Nikula, Jani
> > Subject: Re: [PATCH v7 6/6] drm/i915/panelreplay: Debugfs support
> > for
> > panel replay
> >
> > On Wed, 2023-10-11 at 16:39 +0530, Animesh Manna wrote:
> > > Add debugfs support which will print source and sink status per
> > > connector basis.
> >
> > Sorry for late review. Noticed only by now that you have added this
> > patch
> > into you set.
>
> Added from v5.
>
> >
> > Can you please describe in commit message how you see the output of
> > debugfs interface will look like after your changes?
>
> Sure.
>
> >
> > >
> > > v1: Initial version. [rb-ed by Arun]
> > > v2: Added check for DP 2.0 and connector type in
> > > connector_debugfs_add().
> > >
> > > Cc: Jouni Högander
> > > Cc: Arun R Murthy
> > > Cc: Jani Nikula
> > > Signed-off-by: Animesh Manna
> > > ---
> > > drivers/gpu/drm/i915/display/intel_psr.c | 136
> > > +
> > > --
> > > 1 file changed, 102 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 80de831c2f60..399fc0a8e636 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -2823,6 +2823,25 @@ static int
> > > psr_get_status_and_error_status(struct intel_dp *intel_dp,
> > > return 0;
> > > }
> > >
> > > +static int panel_replay_get_status_and_error_status(struct
> > > intel_dp
> > > *intel_dp,
> > > + u8 *status,
> > > u8
> > > *error_status)
> > > +{
> > > + struct drm_dp_aux *aux = _dp->aux;
> > > + int ret;
> > > +
> > > + ret = drm_dp_dpcd_readb(aux,
> > > DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS, status);
> > > + if (ret != 1)
> > > + return ret;
> > > +
> > > + ret = drm_dp_dpcd_readb(aux,
> > > DP_PANEL_REPLAY_ERROR_STATUS,
> > > error_status);
> > > + if (ret != 1)
> > > + return ret;
> > > +
> > > + *status = *status & DP_PSR_SINK_STATE_MASK;
> > > +
> > > + return 0;
> > > +}
> > > +
> >
> > I think you should modify psr_get_status_and_error_status instead
> > of
> > duplicating most of it.
>
> DPCD addresses are different for panel replay, I did not get the need
> of it.
I would like to see:
unsigned int offset = intel_dp->psr.panel_replay_enabled ?
DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS : DP_PSR_STATUS;
ret = drm_dp_dpcd_readb(aux, offset, status);
rather than duplicating it.
>
> >
> > > static void psr_alpm_check(struct intel_dp *intel_dp)
> > > {
> > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > @@
> > > -3035,7 +3054,7 @@ psr_source_status(struct intel_dp *intel_dp,
> > > struct
> > > seq_file *m)
> > > status = live_status[status_val];
> > > }
> > >
> > > - seq_printf(m, "Source PSR status: %s [0x%08x]\n", status,
> > > val);
> > > + seq_printf(m, "Source PSR/PanelReplay status: %s
> > > [0x%08x]\n",
> > > status, val);
> > > }
> > >
> > > static int intel_psr_status(struct seq_file *m, struct intel_dp
> > > *intel_dp)
> > > @@ -3048,18 +3067,23 @@ static int intel_psr_status(struct
> > > seq_file
> > > *m, struct intel_dp *intel_dp)
> > > bool enabled;
> > > u32 val;
> > >
> > > - seq_printf(m, "Sink support: %s", str_yes_no(psr-
> > > > sink_support));
> > > - if (psr->sink_support)
> > > + seq_printf(m, "Sink support: PSR = %s, Panel Replay =
> > > %s",
> > > + str_yes_no(psr->sink_support),
> > > + str_yes_no(psr->sink_panel_replay_support));
> > &
Re: [PATCH v7 6/6] drm/i915/panelreplay: Debugfs support for panel replay
On Wed, 2023-10-11 at 16:39 +0530, Animesh Manna wrote:
> Add debugfs support which will print source and sink status
> per connector basis.
Sorry for late review. Noticed only by now that you have added this
patch into you set.
Can you please describe in commit message how you see the output of
debugfs interface will look like after your changes?
>
> v1: Initial version. [rb-ed by Arun]
> v2: Added check for DP 2.0 and connector type in
> connector_debugfs_add().
>
> Cc: Jouni Högander
> Cc: Arun R Murthy
> Cc: Jani Nikula
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 136 +
> --
> 1 file changed, 102 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 80de831c2f60..399fc0a8e636 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -2823,6 +2823,25 @@ static int
> psr_get_status_and_error_status(struct intel_dp *intel_dp,
> return 0;
> }
>
> +static int panel_replay_get_status_and_error_status(struct intel_dp
> *intel_dp,
> + u8 *status, u8
> *error_status)
> +{
> + struct drm_dp_aux *aux = _dp->aux;
> + int ret;
> +
> + ret = drm_dp_dpcd_readb(aux,
> DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS, status);
> + if (ret != 1)
> + return ret;
> +
> + ret = drm_dp_dpcd_readb(aux, DP_PANEL_REPLAY_ERROR_STATUS,
> error_status);
> + if (ret != 1)
> + return ret;
> +
> + *status = *status & DP_PSR_SINK_STATE_MASK;
> +
> + return 0;
> +}
> +
I think you should modify psr_get_status_and_error_status instead of
duplicating most of it.
> static void psr_alpm_check(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> @@ -3035,7 +3054,7 @@ psr_source_status(struct intel_dp *intel_dp,
> struct seq_file *m)
> status = live_status[status_val];
> }
>
> - seq_printf(m, "Source PSR status: %s [0x%08x]\n", status,
> val);
> + seq_printf(m, "Source PSR/PanelReplay status: %s [0x%08x]\n",
> status, val);
> }
>
> static int intel_psr_status(struct seq_file *m, struct intel_dp
> *intel_dp)
> @@ -3048,18 +3067,23 @@ static int intel_psr_status(struct seq_file
> *m, struct intel_dp *intel_dp)
> bool enabled;
> u32 val;
>
> - seq_printf(m, "Sink support: %s", str_yes_no(psr-
> >sink_support));
> - if (psr->sink_support)
> + seq_printf(m, "Sink support: PSR = %s, Panel Replay = %s",
> + str_yes_no(psr->sink_support),
> + str_yes_no(psr->sink_panel_replay_support));
> +
> + if (psr->sink_support || psr->sink_panel_replay_support)
> seq_printf(m, " [0x%02x]", intel_dp->psr_dpcd[0]);
> seq_puts(m, "\n");
>
> - if (!psr->sink_support)
> + if (!(psr->sink_support || psr->sink_panel_replay_support))
> return 0;
>
> wakeref = intel_runtime_pm_get(_priv->runtime_pm);
> mutex_lock(>lock);
>
> - if (psr->enabled)
> + if (psr->panel_replay_enabled)
> + status = "Panel Replay Enabled";
> + else if (psr->enabled)
> status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1
> enabled";
> else
> status = "disabled";
> @@ -3072,14 +3096,17 @@ static int intel_psr_status(struct seq_file
> *m, struct intel_dp *intel_dp)
> goto unlock;
> }
>
> - if (psr->psr2_enabled) {
> + if (psr->panel_replay_enabled) {
> + val = intel_de_read(dev_priv,
> TRANS_DP2_CTL(cpu_transcoder));
> + enabled = val & TRANS_DP2_PANEL_REPLAY_ENABLE;
> + } else if (psr->psr2_enabled) {
> val = intel_de_read(dev_priv,
> EDP_PSR2_CTL(cpu_transcoder));
>
> enabled = val & EDP_PSR2_ENABLE;
> } else {
> val = intel_de_read(dev_priv, psr_ctl_reg(dev_priv,
> cpu_transcoder));
> enabled = val & EDP_PSR_ENABLE;
> }
> - seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> + seq_printf(m, "Source PSR/PanelReplay ctl: %s [0x%08x]\n",
> str_enabled_disabled(enabled), val);
> psr_source_status(intel_dp, m);
> seq_printf(m, "Busy frontbuffer bits: 0x%08x\n",
> @@ -3221,6 +3248,7 @@ static int i915_psr_sink_status_show(struct
> seq_file *m, void *data)
> {
> struct intel_connector *connector = m->private;
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> + struct intel_psr *psr = _dp->psr;
> static const char * const sink_status[] = {
> "inactive",
> "transition to active, capture and display",
> @@ -3231,45 +3259,82 @@ static int i915_psr_sink_status_show(struct
> seq_file *m,
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915: Initialize fbdev DRM client with callback functions
On Wed, 2023-11-01 at 09:11 +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 25.10.23 um 10:36 schrieb Hogander, Jouni:
> > Hi Thomas, One minor comment inline below.
>
> Thank you so much for taking the time to review these patches.
>
> >
> > On Wed, 2023-09-27 at 12:26 +0200, Thomas Zimmermann wrote:
> > > Initialize i915's fbdev client by giving an instance of struct
> > > drm_client_funcs to drm_client_init(). Also clean up with
> > > drm_client_release().
> > >
> > > Doing this in i915 prevents fbdev helpers from initializing and
> > > releasing the client internally (see drm_fb_helper_init()). No
> > > functional change yet; the client callbacks will be filled later.
> > >
> > > v2:
> > > * call drm_fb_helper_unprepare() in error handling
> > > (Jani)
> > > * fix typo in commit message (Sam)
> > >
> > > Signed-off-by: Thomas Zimmermann
> > > ---
> > > drivers/gpu/drm/i915/display/intel_fbdev.c | 43
> > > --
> > > 1 file changed, 39 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > index 2695c65b55ddc..d9e69471a782a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > @@ -378,6 +378,7 @@ static void intel_fbdev_destroy(struct
> > > intel_fbdev *ifbdev)
> > > if (ifbdev->fb)
> > > drm_framebuffer_remove(>fb->base);
> > >
> > > + drm_client_release(>helper.client);
> > > drm_fb_helper_unprepare(>helper);
> > > kfree(ifbdev);
> > > }
> > > @@ -671,6 +672,30 @@ void intel_fbdev_restore_mode(struct
> > > drm_i915_private *dev_priv)
> > > intel_fbdev_invalidate(ifbdev);
> > > }
> > >
> > > +/*
> > > + * Fbdev client and struct drm_client_funcs
> > > + */
> > > +
> > > +static void intel_fbdev_client_unregister(struct drm_client_dev
> > > *client)
> > > +{ }
> > > +
> > > +static int intel_fbdev_client_restore(struct drm_client_dev
> > > *client)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static int intel_fbdev_client_hotplug(struct drm_client_dev
> > > *client)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static const struct drm_client_funcs intel_fbdev_client_funcs =
> > > {
> > > + .owner = THIS_MODULE,
> > > + .unregister = intel_fbdev_client_unregister,
> > > + .restore= intel_fbdev_client_restore,
> > > + .hotplug= intel_fbdev_client_hotplug,
> > > +};
> > > +
> > > int intel_fbdev_init(struct drm_device *dev)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > @@ -692,16 +717,26 @@ int intel_fbdev_init(struct drm_device
> > > *dev)
> > > else
> > > ifbdev->preferred_bpp = ifbdev-
> > > >helper.preferred_bpp;
> > >
> > > + ret = drm_client_init(dev, >helper.client, "i915-
> > > fbdev",
> >
> > We are currently working on new driver named as Xe. Due to this it
>
> I've always thought that it's an entirely new driver. But I'm not
> really
> up-to-date. So the Xe driver is located under i915/ and also shares
> code
> with the existing i915 driver?
It will mainly share display code with i915.
>
> > might actually make sense to use intel-fbdev here rather than i915-
> > fbdev.
>
> That change could break user-space programs. See the comment at [1]
> and
> the commit 842470c4e211 ("Revert "drm/fb-helper: improve DRM fbdev
> emulation device names""). I'd rather leave the string as it is.
Client name doesn't affect name used in fbinfo. For i915 it is
i915drmfb as earlier and for xe its xedrmfb. Also this client name is
completely new added by your patches. I'm not sure where it will
visible. I was originally thinking using driver->name as done in
drm_fb_helper_fill_info, but I think common intel-fbdev is just fine.
BR,
Jouni Högander
>
> Best regards
> Thomas
>
> [1]
> https://elixir.bootlin.com/linux/v6.6/source/drivers/gpu/drm/drm_fb_helper.c#L1755
>
> >
> > BR,
> >
> > Jouni
Re: [PATCH] drm/i915/display: Only fail fastset on PSR2
On Tue, 2023-10-31 at 23:21 +, Paz Zcharya wrote:
> Currently, i915 fails fastset if both the sink and the source support
> any version of PSR and regardless of the configuration setting of the
> driver (i.e., i915.enable_psr kernel argument). However, the
> implementation of PSR1 enable sequence is already seamless
> and works smoothly with fastset. Accordingly, do not fail fastset
> if PSR2 is not enabled.
Thank you for the patch. Check similar patch I sent some time ago to
trybot:
https://patchwork.freedesktop.org/series/125392/
If we want to temporarily do this only for psr1 I think you could check
what I've done in drivers/gpu/drm/i915/display/intel_display.c in my
patch and modify your patch accordingly. Otherwise e.g. our IGT
testcases which are toggling PSR enable/disable/psr1/psr2 are to my
understanding performing full modeset and possible issues are not
revealed.
After modifying drivers/gpu/drm/i915/display/intel_display.c you can
also verify it is really seamless and smooth by toggling different PSR
states via /sys/kernel/debug/dri/0/i915_edp_psr_debug. That interface
is performing atomic commit when PSR mode is changed.
BR,
Jouni Högander
>
> Signed-off-by: Paz Zcharya
> ---
>
> drivers/gpu/drm/i915/display/intel_dp.c | 4 ++--
> drivers/gpu/drm/i915/display/intel_psr.c | 2 +-
> drivers/gpu/drm/i915/display/intel_psr.h | 1 +
> 3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index e0e4cb529284..a1af96e31518 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2584,8 +2584,8 @@ bool intel_dp_initial_fastset_check(struct
> intel_encoder *encoder,
> fastset = false;
> }
>
> - if (CAN_PSR(intel_dp)) {
> - drm_dbg_kms(>drm, "[ENCODER:%d:%s] Forcing full
> modeset to compute PSR state\n",
> + if (CAN_PSR(intel_dp) && psr2_global_enabled(intel_dp)) {
> + drm_dbg_kms(>drm, "[ENCODER:%d:%s] Forcing full
> modeset due to PSR2\n",
> encoder->base.base.id, encoder-
> >base.name);
> crtc_state->uapi.mode_changed = true;
> fastset = false;
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 97d5eef10130..388bc3246db9 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -187,7 +187,7 @@ static bool psr_global_enabled(struct intel_dp
> *intel_dp)
> }
> }
>
> -static bool psr2_global_enabled(struct intel_dp *intel_dp)
> +bool psr2_global_enabled(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> b/drivers/gpu/drm/i915/display/intel_psr.h
> index 0b95e8aa615f..6f3c36389cd3 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -21,6 +21,7 @@ struct intel_encoder;
> struct intel_plane;
> struct intel_plane_state;
>
> +bool psr2_global_enabled(struct intel_dp *intel_dp);
> void intel_psr_init_dpcd(struct intel_dp *intel_dp);
> void intel_psr_pre_plane_update(struct intel_atomic_state *state,
> struct intel_crtc *crtc);
Re: [PATCH v5 7/7] drm/i915: Implement fbdev emulation as in-kernel client
Hi Thomas, couple of minor comments and a question below.
On Wed, 2023-09-27 at 12:26 +0200, Thomas Zimmermann wrote:
> Replace all code that initializes or releases fbdev emulation
> throughout the driver. Instead initialize the fbdev client by a
> single call to i915_fbdev_setup() after i915 has registered its
intel_fbdev_setup
> DRM device. Just like similar code in other drivers, i915 fbdev
> emulation now acts as a regular DRM client.
>
> The fbdev client setup consists of the initial preparation and the
> hot-plugging of the display. The latter creates the fbdev device
> and sets up the fbdev framebuffer. The setup performs display
> hot-plugging once. If no display can be detected, DRM probe helpers
> re-run the detection on each hotplug event.
>
> A call to drm_dev_unregister() releases the client automatically.
> No further action is required within i915. If the fbdev framebuffer
> has been fully set up, struct fb_ops.fb_destroy implements the
> release. For partially initialized emulation, the fbdev client
> reverts the initial setup.
>
> v3:
> * as before, silently ignore devices without displays
> v2:
> * let drm_client_register() handle initial hotplug
> * fix driver name in error message (Jani)
> * fix non-fbdev build (kernel test robot)
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 1 -
> .../drm/i915/display/intel_display_driver.c | 18 --
> drivers/gpu/drm/i915/display/intel_fbdev.c | 180
> --
> drivers/gpu/drm/i915/display/intel_fbdev.h | 20 +-
> drivers/gpu/drm/i915/i915_driver.c | 2 +
> 5 files changed, 83 insertions(+), 138 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index edbcf5968804d..7efa8d2787c39 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -81,7 +81,6 @@
> #include "intel_dvo.h"
> #include "intel_fb.h"
> #include "intel_fbc.h"
> -#include "intel_fbdev.h"
> #include "intel_fdi.h"
> #include "intel_fifo_underrun.h"
> #include "intel_frontbuffer.h"
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
> b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index ffdcddd1943e0..213a4ee93ffc2 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -364,10 +364,6 @@ int intel_display_driver_probe(struct
> drm_i915_private *i915)
>
> intel_overlay_setup(i915);
>
> - ret = intel_fbdev_init(>drm);
> - if (ret)
> - return ret;
> -
> /* Only enable hotplug handling once the fbdev is fully set
> up. */
> intel_hpd_init(i915);
> intel_hpd_poll_disable(i915);
> @@ -392,16 +388,6 @@ void intel_display_driver_register(struct
> drm_i915_private *i915)
>
> intel_display_debugfs_register(i915);
>
> - /*
> - * Some ports require correctly set-up hpd registers for
> - * detection to work properly (leading to ghost connected
> - * connector status), e.g. VGA on gm45. Hence we can only
> set
> - * up the initial fbdev config after hpd irqs are fully
> - * enabled. We do it last so that the async config cannot run
> - * before the connectors are registered.
> - */
> - intel_fbdev_initial_config_async(i915);
> -
> /*
> * We need to coordinate the hotplugs with the asynchronous
> * fbdev configuration, for which we use the
> @@ -445,9 +431,6 @@ void intel_display_driver_remove_noirq(struct
> drm_i915_private *i915)
> */
> intel_hpd_poll_fini(i915);
>
> - /* poll work can call into fbdev, hence clean that up
> afterwards */
> - intel_fbdev_fini(i915);
> -
> intel_unregister_dsm_handler();
>
> /* flush any delayed tasks or pending work */
> @@ -484,7 +467,6 @@ void intel_display_driver_unregister(struct
> drm_i915_private *i915)
> if (!HAS_DISPLAY(i915))
> return;
>
> - intel_fbdev_unregister(i915);
> intel_audio_deinit(i915);
>
> /*
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 39de61d4e7906..100a4aaf1b7e4 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -24,7 +24,6 @@
> * David Airlie
> */
>
> -#include
> #include
> #include
> #include
> @@ -39,6 +38,7 @@
> #include
>
> #include
> +#include
> #include
> #include
> #include
> @@ -58,7 +58,6 @@ struct intel_fbdev {
> struct intel_framebuffer *fb;
> struct i915_vma *vma;
> unsigned long vma_flags;
> - async_cookie_t cookie;
> int preferred_bpp;
>
> /* Whether or not fbdev hpd processing is temporarily
> suspended */
Re: [Intel-gfx] [PATCH v5 6/7] drm/i915: Implement fbdev client callbacks
Hi Thomas, couple of inline commments/suggestions below.
On Wed, 2023-09-27 at 12:26 +0200, Thomas Zimmermann wrote:
> Move code from ad-hoc fbdev callbacks into DRM client functions
> and remove the old callbacks. The functions instruct the client
> to poll for changed output or restore the display.
>
> The DRM core calls both, the old callbacks and the new client
> helpers, from the same places. The new functions perform the same
> operation as before, so there's no change in functionality.
>
> Signed-off-by: Thomas Zimmermann
> ---
> .../drm/i915/display/intel_display_driver.c | 1 -
> drivers/gpu/drm/i915/display/intel_fbdev.c | 11 --
> drivers/gpu/drm/i915/display/intel_fbdev.h | 9
> drivers/gpu/drm/i915/i915_driver.c | 22 -
> --
> 4 files changed, 9 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
> b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 44b59ac301e69..ffdcddd1943e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -96,7 +96,6 @@ void intel_display_driver_init_hw(struct
> drm_i915_private *i915)
> static const struct drm_mode_config_funcs intel_mode_funcs = {
> .fb_create = intel_user_framebuffer_create,
> .get_format_info = intel_fb_get_format_info,
> - .output_poll_changed = intel_fbdev_output_poll_changed,
> .mode_valid = intel_mode_valid,
> .atomic_check = intel_atomic_check,
> .atomic_commit = intel_atomic_commit,
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index d9e69471a782a..39de61d4e7906 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -638,7 +638,7 @@ void intel_fbdev_set_suspend(struct drm_device
> *dev, int state, bool synchronous
> intel_fbdev_hpd_set_suspend(dev_priv, state);
> }
>
> -void intel_fbdev_output_poll_changed(struct drm_device *dev)
> +static void intel_fbdev_output_poll_changed(struct drm_device *dev)
Now as this isn't drm_mode_config_funcs callback anymore: Maybe you
could return error value/0 ?
> {
> struct intel_fbdev *ifbdev = to_i915(dev)-
> >display.fbdev.fbdev;
> bool send_hpd;
> @@ -657,7 +657,7 @@ void intel_fbdev_output_poll_changed(struct
> drm_device *dev)
> drm_fb_helper_hotplug_event(>helper);
> }
>
> -void intel_fbdev_restore_mode(struct drm_i915_private *dev_priv)
> +static void intel_fbdev_restore_mode(struct drm_i915_private
Similar comment as above. I.e. return error value/0 ?
BR,
Jouni Högander
> *dev_priv)
> {
> struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
>
> @@ -681,11 +681,18 @@ static void
> intel_fbdev_client_unregister(struct drm_client_dev *client)
>
> static int intel_fbdev_client_restore(struct drm_client_dev *client)
> {
> + struct drm_i915_private *dev_priv = to_i915(client->dev);
> +
> + intel_fbdev_restore_mode(dev_priv);
> + vga_switcheroo_process_delayed_switch();
> +
> return 0;
> }
>
> static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
> {
> + intel_fbdev_output_poll_changed(client->dev);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.h
> b/drivers/gpu/drm/i915/display/intel_fbdev.h
> index 04fd523a50232..8c953f102ba22 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.h
> @@ -19,8 +19,6 @@ void intel_fbdev_initial_config_async(struct
> drm_i915_private *dev_priv);
> void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
> void intel_fbdev_fini(struct drm_i915_private *dev_priv);
> void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool
> synchronous);
> -void intel_fbdev_output_poll_changed(struct drm_device *dev);
> -void intel_fbdev_restore_mode(struct drm_i915_private *dev_priv);
> struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
> *fbdev);
> #else
> static inline int intel_fbdev_init(struct drm_device *dev)
> @@ -44,13 +42,6 @@ static inline void intel_fbdev_set_suspend(struct
> drm_device *dev, int state, bo
> {
> }
>
> -static inline void intel_fbdev_output_poll_changed(struct drm_device
> *dev)
> -{
> -}
> -
> -static inline void intel_fbdev_restore_mode(struct drm_i915_private
> *i915)
> -{
> -}
> static inline struct intel_framebuffer
> *intel_fbdev_framebuffer(struct intel_fbdev *fbdev)
> {
> return NULL;
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index de19197d2e052..86460cd8167d1 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -924,27 +924,6 @@ static int i915_driver_open(struct drm_device
> *dev, struct drm_file *file)
> return 0;
> }
>
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915: Initialize fbdev DRM client with callback functions
Hi Thomas, One minor comment inline below.
On Wed, 2023-09-27 at 12:26 +0200, Thomas Zimmermann wrote:
> Initialize i915's fbdev client by giving an instance of struct
> drm_client_funcs to drm_client_init(). Also clean up with
> drm_client_release().
>
> Doing this in i915 prevents fbdev helpers from initializing and
> releasing the client internally (see drm_fb_helper_init()). No
> functional change yet; the client callbacks will be filled later.
>
> v2:
> * call drm_fb_helper_unprepare() in error handling (Jani)
> * fix typo in commit message (Sam)
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/i915/display/intel_fbdev.c | 43
> --
> 1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 2695c65b55ddc..d9e69471a782a 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -378,6 +378,7 @@ static void intel_fbdev_destroy(struct
> intel_fbdev *ifbdev)
> if (ifbdev->fb)
> drm_framebuffer_remove(>fb->base);
>
> + drm_client_release(>helper.client);
> drm_fb_helper_unprepare(>helper);
> kfree(ifbdev);
> }
> @@ -671,6 +672,30 @@ void intel_fbdev_restore_mode(struct
> drm_i915_private *dev_priv)
> intel_fbdev_invalidate(ifbdev);
> }
>
> +/*
> + * Fbdev client and struct drm_client_funcs
> + */
> +
> +static void intel_fbdev_client_unregister(struct drm_client_dev
> *client)
> +{ }
> +
> +static int intel_fbdev_client_restore(struct drm_client_dev *client)
> +{
> + return 0;
> +}
> +
> +static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
> +{
> + return 0;
> +}
> +
> +static const struct drm_client_funcs intel_fbdev_client_funcs = {
> + .owner = THIS_MODULE,
> + .unregister = intel_fbdev_client_unregister,
> + .restore= intel_fbdev_client_restore,
> + .hotplug= intel_fbdev_client_hotplug,
> +};
> +
> int intel_fbdev_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -692,16 +717,26 @@ int intel_fbdev_init(struct drm_device *dev)
> else
> ifbdev->preferred_bpp = ifbdev->helper.preferred_bpp;
>
> + ret = drm_client_init(dev, >helper.client, "i915-
> fbdev",
We are currently working on new driver named as Xe. Due to this it
might actually make sense to use intel-fbdev here rather than i915-
fbdev.
BR,
Jouni Högander
> + _fbdev_client_funcs);
> + if (ret)
> + goto err_drm_fb_helper_unprepare;
> +
> ret = drm_fb_helper_init(dev, >helper);
> - if (ret) {
> - kfree(ifbdev);
> - return ret;
> - }
> + if (ret)
> + goto err_drm_client_release;
>
> dev_priv->display.fbdev.fbdev = ifbdev;
> INIT_WORK(_priv->display.fbdev.suspend_work,
> intel_fbdev_suspend_worker);
>
> return 0;
> +
> +err_drm_client_release:
> + drm_client_release(>helper.client);
> +err_drm_fb_helper_unprepare:
> + drm_fb_helper_unprepare(>helper);
> + kfree(ifbdev);
> + return ret;
> }
>
> static void intel_fbdev_initial_config(void *data, async_cookie_t
> cookie)
Re: [PATCH v5 4/7] drm/i915: Move fbdev functions
On Wed, 2023-09-27 at 12:26 +0200, Thomas Zimmermann wrote:
> Move functions within intel_fbdev.c to simplify later updates. Minor
> style fixes to make checkpatch happy, but no functional changes.
>
> v5:
> * style fixes (checkpatch)
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Jouni Högander
> ---
> drivers/gpu/drm/i915/display/intel_fbdev.c | 154 ++-
> --
> 1 file changed, 77 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 31d0d695d5671..2695c65b55ddc 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -545,58 +545,6 @@ static void intel_fbdev_suspend_worker(struct
> work_struct *work)
> true);
> }
>
> -int intel_fbdev_init(struct drm_device *dev)
> -{
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_fbdev *ifbdev;
> - int ret;
> -
> - if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv)))
> - return -ENODEV;
> -
> - ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> - if (ifbdev == NULL)
> - return -ENOMEM;
> -
> - mutex_init(>hpd_lock);
> - drm_fb_helper_prepare(dev, >helper, 32,
> _fb_helper_funcs);
> -
> - if (intel_fbdev_init_bios(dev, ifbdev))
> - ifbdev->helper.preferred_bpp = ifbdev->preferred_bpp;
> - else
> - ifbdev->preferred_bpp = ifbdev->helper.preferred_bpp;
> -
> - ret = drm_fb_helper_init(dev, >helper);
> - if (ret) {
> - kfree(ifbdev);
> - return ret;
> - }
> -
> - dev_priv->display.fbdev.fbdev = ifbdev;
> - INIT_WORK(_priv->display.fbdev.suspend_work,
> intel_fbdev_suspend_worker);
> -
> - return 0;
> -}
> -
> -static void intel_fbdev_initial_config(void *data, async_cookie_t
> cookie)
> -{
> - struct intel_fbdev *ifbdev = data;
> -
> - /* Due to peculiar init order wrt to hpd handling this is
> separate. */
> - if (drm_fb_helper_initial_config(>helper))
> - intel_fbdev_unregister(to_i915(ifbdev->helper.dev));
> -}
> -
> -void intel_fbdev_initial_config_async(struct drm_i915_private
> *dev_priv)
> -{
> - struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
> -
> - if (!ifbdev)
> - return;
> -
> - ifbdev->cookie = async_schedule(intel_fbdev_initial_config,
> ifbdev);
> -}
> -
> static void intel_fbdev_sync(struct intel_fbdev *ifbdev)
> {
> if (!ifbdev->cookie)
> @@ -607,31 +555,6 @@ static void intel_fbdev_sync(struct intel_fbdev
> *ifbdev)
> ifbdev->cookie = 0;
> }
>
> -void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
> -{
> - struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
> -
> - if (!ifbdev)
> - return;
> -
> - intel_fbdev_set_suspend(_priv->drm,
> FBINFO_STATE_SUSPENDED, true);
> -
> - if (!current_is_async())
> - intel_fbdev_sync(ifbdev);
> -
> - drm_fb_helper_unregister_info(>helper);
> -}
> -
> -void intel_fbdev_fini(struct drm_i915_private *dev_priv)
> -{
> - struct intel_fbdev *ifbdev = fetch_and_zero(_priv-
> >display.fbdev.fbdev);
> -
> - if (!ifbdev)
> - return;
> -
> - intel_fbdev_destroy(ifbdev);
> -}
> -
> /* Suspends/resumes fbdev processing of incoming HPD events. When
> resuming HPD
> * processing, fbdev will perform a full connector reprobe if a
> hotplug event
> * was received while HPD was suspended.
> @@ -748,6 +671,83 @@ void intel_fbdev_restore_mode(struct
> drm_i915_private *dev_priv)
> intel_fbdev_invalidate(ifbdev);
> }
>
> +int intel_fbdev_init(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_fbdev *ifbdev;
> + int ret;
> +
> + if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv)))
> + return -ENODEV;
> +
> + ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> + if (!ifbdev)
> + return -ENOMEM;
> +
> + mutex_init(>hpd_lock);
> + drm_fb_helper_prepare(dev, >helper, 32,
> _fb_helper_funcs);
> +
> + if (intel_fbdev_init_bios(dev, ifbdev))
> + ifbdev->helper.preferred_bpp = ifbdev->preferred_bpp;
> + else
> + ifbdev->preferred_bpp = ifbdev->helper.preferred_bpp;
> +
> + ret = drm_fb_helper_init(dev, >helper);
> + if (ret) {
> + kfree(ifbdev);
> + return ret;
> + }
> +
> + dev_priv->display.fbdev.fbdev = ifbdev;
> + INIT_WORK(_priv->display.fbdev.suspend_work,
> intel_fbdev_suspend_worker);
> +
> + return 0;
> +}
> +
> +static void intel_fbdev_initial_config(void *data, async_cookie_t
> cookie)
> +{
> + struct intel_fbdev *ifbdev = data;
> +
> + /* Due to peculiar init order wrt to
Re: [Intel-gfx] [PATCH v5 1/7] drm/i915: Unregister in-kernel clients
On Wed, 2023-09-27 at 12:26 +0200, Thomas Zimmermann wrote:
> Unregister all in-kernel clients before unloading the i915 driver.
> For
> other drivers, drm_dev_unregister() does this automatically. As i915
> does not use this helper, it has to perform the call by itself.
>
> Note that there are currently no in-kernel clients in i915. The patch
> prepares the driver for a related update of its fbdev support.
Hi Thomas. You need to move patch 3/7 before this one. Otherwise this
doesn't build.
BR,
Jouni Högander
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/i915/i915_driver.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index d50347e5773a3..de19197d2e052 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -41,6 +41,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -855,6 +856,8 @@ void i915_driver_remove(struct drm_i915_private
> *i915)
> {
> intel_wakeref_t wakeref;
>
> + drm_client_dev_unregister(>drm);
> +
> wakeref = intel_runtime_pm_get(>runtime_pm);
>
> i915_driver_unregister(i915);
Re: [PATCH v5 3/6] drm/i915/panelreplay: Initializaton and compute config for panel replay
On Tue, 2023-09-05 at 13:05 +0530, Animesh Manna wrote:
> Modify existing PSR implementation to enable panel replay feature of
> DP 2.0
> which is similar to PSR feature of EDP panel. There is different DPCD
> address to check panel capability compare to PSR and vsc sdp header
> is different.
>
> v1: Initial version.
> v2:
> - Set source_panel_replay_support flag under HAS_PANEL_REPLAY()
> condition check. [Jouni]
> - Code restructured around intel_panel_replay_init
> and renamed to intel_panel_replay_init_dpcd. [Jouni]
> - Remove the initial code modification around has_psr2 flag. [Jouni]
> - Add CAN_PANEL_REPLAY() in intel_encoder_can_psr which is used to
> enable in intel_psr_post_plane_update. [Jouni]
> v3:
> - Initialize both psr and panel-replay. [Jouni]
> - Initialize both panel replay and psr if detected. [Jouni]
> - Refactoring psr function by introducing _psr_compute_config().
> [Jouni]
> - Add check for !is_edp while deriving source_panel_replay_support.
> [Jouni]
> - Enable panel replay dpcd initialization in a separate patch.
> [Jouni]
>
> v4:
> - HAS_PANEL_REPLAY() check not needed during sink capability check.
> [Jouni]
> - Set either panel replay source support or psr. [Jouni]
>
> v5:
> - HAS_PANEL_REPLAY() removed and use HAS_DP20() instead. [Jouni]
> - Move psr related code to intel_psr.c. [Jani]
> - Reset sink_panel_replay_support flag during disconnection. [Jani]
>
> Cc: Jouni Högander
> Signed-off-by: Animesh Manna
> ---
> .../drm/i915/display/intel_display_types.h | 14 +--
> drivers/gpu/drm/i915/display/intel_dp.c | 45 +++--
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +
> drivers/gpu/drm/i915/display/intel_psr.c | 96 +
> --
> drivers/gpu/drm/i915/display/intel_psr.h | 7 ++
> 5 files changed, 118 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index c21064794f32..4022d6d8281a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1202,6 +1202,7 @@ struct intel_crtc_state {
> bool has_psr2;
> bool enable_psr2_sel_fetch;
> bool req_psr2_sdp_prior_scanline;
> + bool has_panel_replay;
> bool wm_level_disabled;
> u32 dc3co_exitline;
> u16 su_y_granularity;
> @@ -1693,6 +1694,8 @@ struct intel_psr {
> bool irq_aux_error;
> u16 su_w_granularity;
> u16 su_y_granularity;
> + bool source_panel_replay_support;
> + bool sink_panel_replay_support;
> u32 dc3co_exitline;
> u32 dc3co_exit_delay;
> struct delayed_work dc3co_work;
> @@ -1980,17 +1983,6 @@ dp_to_lspcon(struct intel_dp *intel_dp)
>
> #define dp_to_i915(__intel_dp) to_i915(dp_to_dig_port(__intel_dp)-
> >base.base.dev)
>
> -#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
> - (intel_dp)->psr.source_support)
> -
> -static inline bool intel_encoder_can_psr(struct intel_encoder
> *encoder)
> -{
> - if (!intel_encoder_is_dp(encoder))
> - return false;
> -
> - return CAN_PSR(enc_to_intel_dp(encoder));
> -}
> -
> static inline struct intel_digital_port *
> hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
> {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 3faa68989d85..d8c151196a81 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2338,12 +2338,22 @@ static void
> intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
> struct intel_crtc *crtc = to_intel_crtc(crtc_state-
> >uapi.crtc);
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>
> - /*
> - * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> - * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> - * Colorimetry Format indication.
> - */
> - vsc->revision = 0x5;
> + if (crtc_state->has_panel_replay) {
> + /*
> + * Prepare VSC Header for SU as per DP 2.0 spec,
> Table 2-223
> + * VSC SDP supporting 3D stereo, Panel Replay, and
> Pixel
> + * Encoding/Colorimetry Format indication.
> + */
> + vsc->revision = 0x7;
> + } else {
> + /*
> + * Prepare VSC Header for SU as per DP 1.4 spec,
> Table 2-118
> + * VSC SDP supporting 3D stereo, PSR2, and Pixel
> Encoding/
> + * Colorimetry Format indication.
> + */
> + vsc->revision = 0x5;
> + }
> +
> vsc->length = 0x13;
>
> /* DP 1.4a spec, Table 2-120 */
> @@ -2452,6 +2462,21 @@ void intel_dp_compute_psr_vsc_sdp(struct
> intel_dp *intel_dp,
> vsc->revision = 0x4;
>
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On Thu, 2023-03-09 at 14:34 +0200, Ville Syrjälä wrote: > On Thu, Mar 09, 2023 at 12:09:55PM +0100, Maarten Lankhorst wrote: > > > > On 2023-03-09 12:04, Hogander, Jouni wrote: > > > On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote: > > > > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst > > > > wrote: > > > > > Hey, > > > > > > > > > > On 2023-03-06 16:23, Souza, Jose wrote: > > > > > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: > > > > > > > As a fallback if we decide not to merge the frontbuffer > > > > > > > tracking, allow > > > > > > > i915 to keep its own implementation, and do the right > > > > > > > thing in > > > > > > > Xe. > > > > > > > > > > > > > > The frontbuffer tracking for Xe is still done per-fb, > > > > > > > while > > > > > > > i915 can > > > > > > > keep doing the weird intel_frontbuffer + i915_active > > > > > > > thing > > > > > > > without > > > > > > > blocking Xe. > > > > > > Please also disable PSR and FBC with this or at least add a > > > > > > way > > > > > > for users to disable those features. > > > > > > Without frontbuffer tracker those two features will break > > > > > > in some > > > > > > cases. > > > > > FBC and PSR work completely as expected. I don't remove > > > > > frontbuffer > > > > > tracking; I only remove the GEM parts. > > > > > > > > > > Explicit invalidation using pageflip or CPU rendering + > > > > > DirtyFB > > > > > continue > > > > > to work, as I validated on my laptop with FBC. > > > > Neither of which are relevant to the removal of the gem hooks. > > > > > > > > Like I already said ~10 times in the last meeting, we need a > > > > proper > > > > testcase. Here's a rough idea what it should do: > > > > > > > > prepare a batch with > > > > 1. spinner > > > > 2. something that clobbers the fb > > > > > > > > Then > > > > 1. grab reference crc > > > > 2. execbuffer > > > > 3. dirtyfb > > > > 4. wait long enough for fbc to recompress > > > > 5. terminate spinner > > > > 6. gem_sync > > > > 7. grab crc and compare with reference > > > > > > > > No idea what the current status of PSR+CRC is, so not sure > > > > whether we can actually test PSR or not. > > > > > > > CRC calculation doesn't work with PSR currently. PSR is disabled > > > if CRC > > > capture is requested. > > > > > > Are we supposed to support frontbuffer rendering using GPU? > > > > No other driver does that. > > Every driver does that when you run X w/o a compositor. Assuming > there is an actual GPU in there. > > > It's fine if DirtyFB hangs instead until the > > job it waits on completes. > > No one tried to make it just wait for the fence(s) w/o doing > a full blown atomic commit. It might work, but might also > still suck too much. I guess depends on how overloaded the GPU > is. > > What we could do is do a frontbuffer invalidate on dirtyfb > invocation, and then once the fence(s) signal we do a frontbuffer > flush. That would most closely match the gem hook behaviour, except > the invalidate comes in a bit later. The alternative would be to > skip the invalidate, which should still guarantee correctness in > the end, just with possibly jankier interactivity. > I have implemented this approach here: https://patchwork.freedesktop.org/series/116620/ Review/comments are welcome. BR, Jouni Högander
Re: [Intel-gfx] [PATCH v3] drm/i915/mtl: Disable stolen memory backed FB for A0
On Tue, 2023-04-04 at 23:26 +0200, Das, Nirmoy wrote: > > On 4/4/2023 8:27 PM, Ville Syrjälä wrote: > > On Tue, Apr 04, 2023 at 08:13:42PM +0200, Nirmoy Das wrote: > > > Stolen memory is not usable for MTL A0 stepping beyond > > > certain access size and we have no control over userspace > > > access size of /dev/fb which can be backed by stolen memory. > > > So disable stolen memory backed fb by setting i915- > > > >dsm.usable_size > > > to zero. > > > > > > v2: remove hsdes reference and fix commit message(Andi) > > > v3: use revid as we want to target SOC stepping(Radhakrishna) > > > > > > Cc: Matthew Auld > > > Cc: Andi Shyti > > > Cc: Daniele Ceraolo Spurio > > > Cc: Lucas De Marchi > > > Cc: Radhakrishna Sripada > > > Signed-off-by: Nirmoy Das > > > Reviewed-by: Andi Shyti > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 8 > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > > > b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > > > index 8ac376c24aa2..ee492d823f1b 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c > > > @@ -535,6 +535,14 @@ static int i915_gem_init_stolen(struct > > > intel_memory_region *mem) > > > /* Basic memrange allocator for stolen space. */ > > > drm_mm_init(>mm.stolen, 0, i915->dsm.usable_size); > > > > > > + /* > > > + * Access to stolen lmem beyond certain size for MTL A0 > > > stepping > > > + * would crash the machine. Disable stolen lmem for > > > userspace access > > > + * by setting usable_size to zero. > > > + */ > > > + if (IS_METEORLAKE(i915) && INTEL_REVID(i915) == 0x0) > > > + i915->dsm.usable_size = 0; > > That certainly won't prevent FBC from using stolen. > > Are we sure that FBC accesses are fine? > > I think so. I remember Jouni tested this patch internally to unblock > a > FBC test. > > Jouni, could you please share your thoughts. I can't seem to find the > internal JIRA reference right now. I tested this patch and it was fixing the problem it was targeted. I didn't noticed any issue back then. > > > Regards, > > Nirmoy > > > > > > + > > > return 0; > > > } > > > > > > -- > > > 2.39.0
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote: > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote: > > Hey, > > > > On 2023-03-06 16:23, Souza, Jose wrote: > > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: > > > > As a fallback if we decide not to merge the frontbuffer > > > > tracking, allow > > > > i915 to keep its own implementation, and do the right thing in > > > > Xe. > > > > > > > > The frontbuffer tracking for Xe is still done per-fb, while > > > > i915 can > > > > keep doing the weird intel_frontbuffer + i915_active thing > > > > without > > > > blocking Xe. > > > Please also disable PSR and FBC with this or at least add a way > > > for users to disable those features. > > > Without frontbuffer tracker those two features will break in some > > > cases. > > > > FBC and PSR work completely as expected. I don't remove frontbuffer > > tracking; I only remove the GEM parts. > > > > Explicit invalidation using pageflip or CPU rendering + DirtyFB > > continue > > to work, as I validated on my laptop with FBC. > > Neither of which are relevant to the removal of the gem hooks. > > Like I already said ~10 times in the last meeting, we need a proper > testcase. Here's a rough idea what it should do: > > prepare a batch with > 1. spinner > 2. something that clobbers the fb > > Then > 1. grab reference crc > 2. execbuffer > 3. dirtyfb > 4. wait long enough for fbc to recompress > 5. terminate spinner > 6. gem_sync > 7. grab crc and compare with reference > > No idea what the current status of PSR+CRC is, so not sure > whether we can actually test PSR or not. > CRC calculation doesn't work with PSR currently. PSR is disabled if CRC capture is requested. Are we supposed to support frontbuffer rendering using GPU? BR, Jouni Högander
Re: [PATCH 0/2] Add quirk to disable PSR 2 on Tongfang PHxTxX1 and PHxTQx1
On Thu, 2023-02-23 at 20:02 +0100, Werner Sembach wrote: > > Am 23.02.23 um 19:56 schrieb Werner Sembach: > > > > Am 23.02.23 um 19:26 schrieb Hogander, Jouni: > > > On Wed, 2023-02-22 at 15:17 +0100, Werner Sembach wrote: > > > > On these Barebones PSR 2 is recognized as supported but is very > > > > buggy: > > > > - Upper third of screen does sometimes not updated, resulting > > > > in > > > > disappearing cursors or ghosts of already closed Windows saying > > > > behind. > > > > - Approximately 40 px from the bottom edge a 3 pixel wide strip > > > > of > > > > randomly > > > > colored pixels is flickering. > > > > > > > > PSR 1 is working fine however. > > > > > > > > This patchset introduces a new quirk to disable PSR 2 > > > > specifically on > > > > known > > > > buggy devices and applies it to the Tongfang PHxTxX1 and > > > > PHxTQx1 > > > > barebones. > > > I've been thinking something similar as there is still at least > > > one > > > issue which seems to be like panel side issue: > > > > > > https://gitlab.freedesktop.org/drm/intel/-/issues/7836 > > > > > > Did you considered dpcd_quirk_list in > > > drivers/gpu/drm/drm_dp_helper.c? > > > > > > I'm not sure which one is more correct though... > > Imho, since the proper fix lies within the Intel driver the quirk > > should also > > lie within the Intel driver, because even if the panel has the same > > problem > > combined with an AMD or NVIDIA card the proper fix for them will > > most likely > > be land in the same kernel version. So there could be a period > > where you no > > longer want the quirk for devices combining the panel with an Intel > > gpu but > > still with an AMD GPU or vice versa. > *the proper fix for them will most likely not be in the same kernel > version. Yeah, you are right here. Let's target fixing rootcause for your setup. First patch from your set could be anyways used for issue mentioned above. > > > > > > > Signed-off-by: Werner Sembach > > > > Cc: > > > > > > > > BR, Jouni Högander
Re: [PATCH 0/2] Add quirk to disable PSR 2 on Tongfang PHxTxX1 and PHxTQx1
On Wed, 2023-02-22 at 15:17 +0100, Werner Sembach wrote: > On these Barebones PSR 2 is recognized as supported but is very > buggy: > - Upper third of screen does sometimes not updated, resulting in > disappearing cursors or ghosts of already closed Windows saying > behind. > - Approximately 40 px from the bottom edge a 3 pixel wide strip of > randomly > colored pixels is flickering. > > PSR 1 is working fine however. > > This patchset introduces a new quirk to disable PSR 2 specifically on > known > buggy devices and applies it to the Tongfang PHxTxX1 and PHxTQx1 > barebones. I've been thinking something similar as there is still at least one issue which seems to be like panel side issue: https://gitlab.freedesktop.org/drm/intel/-/issues/7836 Did you considered dpcd_quirk_list in drivers/gpu/drm/drm_dp_helper.c? I'm not sure which one is more correct though... > > Signed-off-by: Werner Sembach > Cc: > >
Re: [Intel-gfx] [PATCH 2/2] Apply quirk to disable PSR 2 on Tongfang PHxTxX1 and PHxTQx1
On Wed, 2023-02-22 at 15:13 -0500, Rodrigo Vivi wrote:
> On Wed, Feb 22, 2023 at 03:17:55PM +0100, Werner Sembach wrote:
> > On these Barebones PSR 2 is recognized as supported but is very
> > buggy:
> > - Upper third of screen does sometimes not updated, resulting in
> > disappearing cursors or ghosts of already closed Windows saying
> > behind.
> > - Approximately 40 px from the bottom edge a 3 pixel wide strip of
> > randomly
> > colored pixels is flickering.
> >
> > PSR 1 is working fine however.
>
> I wonder if this is really about the panel's PSR2 or about the
> userspace
> there not marking the dirtyfb? I know I know... it is not userspace
> fault...
>
> But I wonder if the case you got here highlights the fact that we
> have
> a substantial bug in the i915 itself in regards to PSR2 API.
>
> Jose, Jouni, ideas on how to check what could be happening here?
There is already fix for this (Thanks to Werner Sembach for testing the
patch):
https://patchwork.freedesktop.org/series/114217/
>
> oh, btw, Werner, do we have an open gilab issue for this?
https://gitlab.freedesktop.org/drm/intel/-/issues/7347
>
> Thanks,
> Rodrigo.
>
> >
> > Signed-off-by: Werner Sembach
> > Cc:
> > ---
> > drivers/gpu/drm/i915/display/intel_quirks.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c
> > b/drivers/gpu/drm/i915/display/intel_quirks.c
> > index ce6d0fe6448f5..eeb32d3189f5c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_quirks.c
> > +++ b/drivers/gpu/drm/i915/display/intel_quirks.c
> > @@ -65,6 +65,10 @@ static void
> > quirk_no_pps_backlight_power_hook(struct drm_i915_private *i915)
> > drm_info(>drm, "Applying no pps backlight power
> > quirk\n");
> > }
> >
> > +/*
> > + * Tongfang PHxTxX1 and PHxTQx1 devices have support for PSR 2 but
> > it is broken
> > + * on Linux. PSR 1 however works just fine.
> > + */
> > static void quirk_no_psr2(struct drm_i915_private *i915)
> > {
> > intel_set_quirk(i915, QUIRK_NO_PSR2);
> > @@ -205,6 +209,10 @@ static struct intel_quirk intel_quirks[] = {
> > /* ECS Liva Q2 */
> > { 0x3185, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time
> > },
> > { 0x3184, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time
> > },
> > +
> > + /* Tongfang PHxTxX1 and PHxTQx1/TUXEDO InfinityBook 14 Gen6
> > */
> > + { 0x9a49, 0x1d05, 0x1105, quirk_no_psr2 },
> > + { 0x9a49, 0x1d05, 0x114c, quirk_no_psr2 },
> > };
> >
> > void intel_init_quirks(struct drm_i915_private *i915)
> > --
> > 2.34.1
> >
Re: [PATCH v3 0/2] Don't use stolen memory or BAR mappings for ring buffers
On Fri, 2023-02-17 at 09:18 -0800, John Harrison wrote: > On 2/17/2023 00:39, Hogander, Jouni wrote: > > On Wed, 2023-02-15 at 17:10 -0800, john.c.harri...@intel.com wrote: > > > From: John Harrison > > > > > > Instruction from hardware arch is that stolen memory and BAR > > > mappings > > > are unsafe for use as ring buffers. There can be issues with > > > cache > > > aliasing due to the CPU access going to memory via the BAR. So, > > > don't > > > do it. > > Tested these patches for GPU Hang I was debugging. Seem to fix that > > one > > as well: > > > > Tested-by: Jouni Högander > Sweet! Out of interest, which platform was that? And how reproducible > was it? It would be interesting to know if an IGT was actually > regularly > showing the issue and we had just been ignoring it! It was Alderlake. It wasn't igt test. Opened several browser instances: https://webglsamples.org/aquarium/aquarium.html Took max. couple of minutes to reproduce. For some reason PSR2 DEEP_SLEEP entry/exit was some kind of trigger for this. Without PSR2 DEEP_SLEEP I couldn't reproduce the issue. BR, Jouni Högander > John. > > > > > > v2: Dont use BAR mappings either. > > > Make conditional on LLC so as not to change platforms that don't > > > need > > > to change (Daniele). > > > Add 'Fixes' tags (Tvrtko). > > > v3: Fix dumb typo. > > > > > > Signed-off-by: John Harrison > > > > > > > > > John Harrison (2): > > > drm/i915: Don't use stolen memory for ring buffers with LLC > > > drm/i915: Don't use BAR mappings for ring buffers with LLC > > > > > > drivers/gpu/drm/i915/gt/intel_ring.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > >
Re: [PATCH v3 0/2] Don't use stolen memory or BAR mappings for ring buffers
On Wed, 2023-02-15 at 17:10 -0800, john.c.harri...@intel.com wrote: > From: John Harrison > > Instruction from hardware arch is that stolen memory and BAR mappings > are unsafe for use as ring buffers. There can be issues with cache > aliasing due to the CPU access going to memory via the BAR. So, don't > do it. Tested these patches for GPU Hang I was debugging. Seem to fix that one as well: Tested-by: Jouni Högander > > v2: Dont use BAR mappings either. > Make conditional on LLC so as not to change platforms that don't need > to change (Daniele). > Add 'Fixes' tags (Tvrtko). > v3: Fix dumb typo. > > Signed-off-by: John Harrison > > > John Harrison (2): > drm/i915: Don't use stolen memory for ring buffers with LLC > drm/i915: Don't use BAR mappings for ring buffers with LLC > > drivers/gpu/drm/i915/gt/intel_ring.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) >
Re: [PATCH 2/2] drm/fbdev: Move damage clip check to higher level
On Wed, 2022-12-21 at 13:05 +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 21.12.22 um 12:19 schrieb Jouni Högander:
> > Checking if damage clip is valid is common to all fb helpers.
> > Makes more sense to check it in higher level than adding into
> > all helpers.
>
> It was a deliberate decision to separate damage clipping and dirty
> updates; done in [1]. Clipping is an optional hint in our regular
> damage
> handling via drm_framebuffer_funcs.dirty. The fb_dirty callback now
> follows that semantics.
>
> I mentioned that it would be better to implement those fb_ops
> callbacks
> for i915. But if you go with fb_dirty, please implement the clipping
> test in your callback.
Thank you for your comments. I have sent a new version. Please check.
>
> Best regards
> Thomas
>
> [1]
> https://patchwork.freedesktop.org/patch/509958/?series=109943=3
>
> >
> > Cc: Ville Syrjälä
> > Cc: Thomas Zimmermann
> > Cc: Jani Nikula
> > Signed-off-by: Jouni Högander
> > ---
> > drivers/gpu/drm/drm_fb_helper.c | 4
> > drivers/gpu/drm/drm_fbdev_generic.c | 4
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c
> > index b3a731b9170a..78c889dbc610 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -384,6 +384,10 @@ static void drm_fb_helper_fb_dirty(struct
> > drm_fb_helper *helper)
> > clip->x2 = clip->y2 = 0;
> > spin_unlock_irqrestore(>damage_lock, flags);
> >
> > + /* Call damage handlers only if necessary */
> > + if (!(clip_copy.x1 < clip_copy.x2 && clip_copy.y1 <
> > clip_copy.y2))
> > + return;
> > +
> > ret = helper->funcs->fb_dirty(helper, _copy);
> > if (ret)
> > goto err;
> > diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
> > b/drivers/gpu/drm/drm_fbdev_generic.c
> > index 0a4c160e0e58..6c6bb0dd2ea8 100644
> > --- a/drivers/gpu/drm/drm_fbdev_generic.c
> > +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> > @@ -334,10 +334,6 @@ static int drm_fbdev_fb_dirty(struct
> > drm_fb_helper *helper, struct drm_clip_rect
> > if (!drm_fbdev_use_shadow_fb(helper))
> > return 0;
> >
> > - /* Call damage handlers only if necessary */
> > - if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
> > - return 0;
> > -
> > if (helper->buffer) {
> > ret = drm_fbdev_damage_blit(helper, clip);
> > if (drm_WARN_ONCE(dev, ret, "Damage blitter failed:
> > ret=%d\n", ret))
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
Re: [PATCH v2 0/4] Fixes for damage clips handling
On Tue, 2022-09-13 at 12:04 +0300, Ville Syrjälä wrote: > On Tue, Aug 23, 2022 at 02:29:16PM +0300, Jouni Högander wrote: > > Currently damage clips handling is broken for planes when using big > > framebuffer + offset in case kms driver adjusts drm_plane_state.src > > coords. This is because damage clips are using coords relative to > > original coords from user-space. > > > > This patchset is fixing this by using original > > coords from user-space instead of drm_plane_state.src when > > iterating > > damage_clips. > > > > v2: Modify drm unit tests accordingly > > > > Cc: Daniel Vetter > > Cc: Maarten Lankhorst > > Cc: Jani Nikula > > Cc: Ville Syrjälä > > Cc: José Roberto de Souza > > Cc: Mika Kahola > > Cc: Maíra Canal > > > > Jouni Högander (4): > > drm: Use original src rect while initializing damage iterator > > drm/i915/display: Use original src in psr2 sel fetch area > > calculation > > drm/i915/display: Use drm helper instead of own loop for damage > > clips > > drm/tests: Set also mock plane src_x, src_y, src_w and src_h > > Do these need to be applied into the same tree, or can > the drm vs. i915 stuff go in separately? Patch 1 and 2 are needed to fix that bigfb handling for i915. Patch 4 is also needed to prevent breaking tests. Patch 3 is more like cleanup. I think i915 patches could go via i915 tree. This just means that i915 bigfb handling isn't fixed by either of the sets alone. > > > > > drivers/gpu/drm/drm_damage_helper.c | 11 ++ > > drivers/gpu/drm/i915/display/intel_psr.c | 20 +++ > > > > .../gpu/drm/tests/drm_damage_helper_test.c | 5 + > > 3 files changed, 19 insertions(+), 17 deletions(-) > > > > -- > > 2.34.1 >
Re: [PATCH 1/3] drm: Use original src rect while initializing damage iterator
On Fri, 2022-08-19 at 09:09 -0300, Maíra Canal wrote:
> Hi Jouni,
>
> On 7/15/22 10:49, Jouni Högander wrote:
> > drm_plane_state->src might be modified by the driver. This is done
> > e.g. in i915 driver when there is bigger framebuffer than the plane
> > and there is some offset within framebuffer. I915 driver calculates
> > separate offset and adjusts src rect coords to be relative to this
> > offset. Damage clips are still relative to original src coords
> > provided by user-space.
> >
> > This patch ensures original coordinates provided by user-space are
> > used when initiliazing damage iterator.
> >
>
> drm_damage_helper has some KUnit tests on drivers/gpu/drm/tests, and
> by
> applying this patch the drm_damage_helper tests started to fail.
> Could
> you also refactor the drm_damage_helper tests?
>
> To run the tests, you can run:
> $ ./tools/testing/kunit/kunit.py run \
> --kunitconfig=drivers/gpu/drm/tests \
> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y \
> --kconfig_add CONFIG_VIRTIO_UML=y
>
> There is also some documentation on the DRM KUnit Tests on [1].
>
> [1] https://docs.kernel.org/gpu/drm-internals.html#unit-testing
Ok, thank you for these. I will take a look.
>
> Best Regards,
> - Maíra Canal
>
> > Signed-off-by: Jouni Högander > ---
> > drivers/gpu/drm/drm_damage_helper.c | 11 +++
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_damage_helper.c
> > b/drivers/gpu/drm/drm_damage_helper.c
> > index 937b699ac2a8..d8b2955e88fd 100644
> > --- a/drivers/gpu/drm/drm_damage_helper.c
> > +++ b/drivers/gpu/drm/drm_damage_helper.c
> > @@ -224,6 +224,7 @@ drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> >const struct drm_plane_state
> > *old_state,
> >const struct drm_plane_state *state)
> > {
> > + struct drm_rect src;
> > memset(iter, 0, sizeof(*iter));
> >
> > if (!state || !state->crtc || !state->fb || !state->visible)
> > @@ -233,10 +234,12 @@ drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> > iter->num_clips = drm_plane_get_damage_clips_count(state);
> >
> > /* Round down for x1/y1 and round up for x2/y2 to catch all
> > pixels */
> > - iter->plane_src.x1 = state->src.x1 >> 16;
> > - iter->plane_src.y1 = state->src.y1 >> 16;
> > - iter->plane_src.x2 = (state->src.x2 >> 16) + !!(state->src.x2 &
> > 0x);
> > - iter->plane_src.y2 = (state->src.y2 >> 16) + !!(state->src.y2 &
> > 0x);
> > + src = drm_plane_state_src(state);
> > +
> > + iter->plane_src.x1 = src.x1 >> 16;
> > + iter->plane_src.y1 = src.y1 >> 16;
> > + iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0x);
> > + iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0x);
> >
> > if (!iter->clips || !drm_rect_equals(>src, _state-
> > >src)) {
> > iter->clips = NULL;
Re: [Intel-gfx] [PATCH 1/3] drm: Use original src rect while initializing damage iterator
On Thu, 2022-08-11 at 18:23 +0200, Daniel Vetter wrote:
> On Fri, Jul 15, 2022 at 04:49:56PM +0300, Jouni Högander wrote:
> > drm_plane_state->src might be modified by the driver. This is done
> > e.g. in i915 driver when there is bigger framebuffer than the plane
> > and there is some offset within framebuffer. I915 driver calculates
> > separate offset and adjusts src rect coords to be relative to this
> > offset. Damage clips are still relative to original src coords
> > provided by user-space.
> >
> > This patch ensures original coordinates provided by user-space are
> > used when initiliazing damage iterator.
> >
> > Signed-off-by: Jouni Högander
>
> Ah kunit test for this would be awesome. Iirc we have a few already
> for
> rect/damage stuff, defo should extend/fix those.
Can you please provide me some pointer to these tests? I have written
earlier one igt test which reveals this issue:
https://patchwork.freedesktop.org/series/103661/
https://patchwork.freedesktop.org/series/104488/
> -Daniel
>
> > ---
> > drivers/gpu/drm/drm_damage_helper.c | 11 +++
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_damage_helper.c
> > b/drivers/gpu/drm/drm_damage_helper.c
> > index 937b699ac2a8..d8b2955e88fd 100644
> > --- a/drivers/gpu/drm/drm_damage_helper.c
> > +++ b/drivers/gpu/drm/drm_damage_helper.c
> > @@ -224,6 +224,7 @@ drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> >const struct drm_plane_state
> > *old_state,
> >const struct drm_plane_state *state)
> > {
> > + struct drm_rect src;
> > memset(iter, 0, sizeof(*iter));
> >
> > if (!state || !state->crtc || !state->fb || !state->visible)
> > @@ -233,10 +234,12 @@ drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> > iter->num_clips = drm_plane_get_damage_clips_count(state);
> >
> > /* Round down for x1/y1 and round up for x2/y2 to catch all
> > pixels */
> > - iter->plane_src.x1 = state->src.x1 >> 16;
> > - iter->plane_src.y1 = state->src.y1 >> 16;
> > - iter->plane_src.x2 = (state->src.x2 >> 16) + !!(state->src.x2 &
> > 0x);
> > - iter->plane_src.y2 = (state->src.y2 >> 16) + !!(state->src.y2 &
> > 0x);
> > + src = drm_plane_state_src(state);
> > +
> > + iter->plane_src.x1 = src.x1 >> 16;
> > + iter->plane_src.y1 = src.y1 >> 16;
> > + iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0x);
> > + iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0x);
> >
> > if (!iter->clips || !drm_rect_equals(>src, _state-
> > >src)) {
> > iter->clips = NULL;
> > --
> > 2.25.1
> >
Re: [PATCH 0/3] HDR aux backlight range calculation
On Fri, 2022-04-29 at 19:34 -0400, Lyude Paul wrote: > Cool! Tested this on three different laptops, and it seems to work > great on > all of them. so, this series is: > > Tested-by: Lyude Paul Thank you all for review/testing support. I will come back with updated patch set later. > > Would review, but I basically have the same comments as jani > > On Tue, 2022-04-26 at 15:30 +0300, Jouni Högander wrote: > > This patch set splits out static hdr metadata backlight range > > parsing > > from gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c into gpu/drm/drm- > > edid.c as > > a new function. This new function is then used in admgpu_dm.c and > > intel_dp_aux_backlight.c > > > > Cc: Maarten Lankhorst > > Cc: Rodrigo Siqueira > > Cc: Harry Wentland > > Cc: Lyude Paul > > Cc: Mika Kahola > > Cc: Jani Nikula > > > > Jouni Högander (3): > > drm: New function to get luminance range based on static hdr > > metadata > > drm/amdgpu_dm: Use split out luminance calculation function > > drm/i915: Use luminance range from static hdr metadata > > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 ++-- > > drivers/gpu/drm/drm_edid.c| 55 > > +++ > > .../drm/i915/display/intel_dp_aux_backlight.c | 9 ++- > > include/drm/drm_edid.h| 4 ++ > > 4 files changed, 70 insertions(+), 33 deletions(-) > >
Re: [PATCH 3/3] drm/i915: Fix for PHY_MISC_TC1 offset
On Wed, 2022-02-16 at 12:07 +0200, Ville Syrjälä wrote: > On Wed, Feb 16, 2022 at 09:36:02AM +0000, Hogander, Jouni wrote: > > On Wed, 2022-02-16 at 10:50 +0200, Ville Syrjälä wrote: > > > On Tue, Feb 15, 2022 at 11:21:54AM +0530, Ramalingam C wrote: > > > > From: Jouni Högander > > > > > > > > Currently ICL_PHY_MISC macro is returning offset 0x64C10 for > > > > PHY_E > > > > port. Correct offset is 0x64C14. > > > > > > Why is it PHY_E and not PHY_F? > > > > This is a valid question. It seems we have followed > > intel_phy_is_snps() > > here: > > > > // snip > > else if (IS_DG2(dev_priv)) > > /* > > * All four "combo" ports and the TC1 port (PHY E) use > > * Synopsis PHYs. > > */ > > return phy <= PHY_E; > > // snip > > > > According to spec port E is "No connection". Better place to fix > > this > > could be intel_phy_is_snps() itself? > > I think the crucial question is where are all the places that > the results of intel_port_to_phy() get used. > > I do see that for all the actual snps phy registers we > do want PHY_E, but maybe it would be better to have a local > SNPS_PHY enum just for intel_snps_phy.c, and leave the other > phy thing for everything else? > > Not sure if there is some other register we index with the > phy that specifically wants PHY_E? I went through registers accesses in intel_snps_phy.c. It is actually only this one register which offset is wrong with PHY_E. Everything else seems to be assuming PHY_E including those SNPS_* registers (as you mentioned). I'm starting to think it would be overkill to open up this phy enum for this purpose. I would propose to stick with current patch. Maybe just update commit message. What do you think? > > Also it kinda looks to me like for VBT port mapping we also > want PHY_F essentially since the modern platforms make the > VBT port mapping PHY based and xelpd_port_mapping() uses > PORT_TC1<->DVO_PORT_*F. Not that we actually use enum phy > in the VBT code atm, but I'm thinking we probably should > since it might allow us to get rid of all those different > mapping tables. Though the whole intel_port_to_phy() > disaster needs to get cleaned up first IMO. > BR, Jouni Högander
Re: [PATCH 3/3] drm/i915: Fix for PHY_MISC_TC1 offset
On Wed, 2022-02-16 at 10:50 +0200, Ville Syrjälä wrote:
> On Tue, Feb 15, 2022 at 11:21:54AM +0530, Ramalingam C wrote:
> > From: Jouni Högander
> >
> > Currently ICL_PHY_MISC macro is returning offset 0x64C10 for PHY_E
> > port. Correct offset is 0x64C14.
>
> Why is it PHY_E and not PHY_F?
This is a valid question. It seems we have followed intel_phy_is_snps()
here:
// snip
else if (IS_DG2(dev_priv))
/*
* All four "combo" ports and the TC1 port (PHY E) use
* Synopsis PHYs.
*/
return phy <= PHY_E;
// snip
According to spec port E is "No connection". Better place to fix this
could be intel_phy_is_snps() itself?
>
> > Fix this by handling PHY_E port seprately.
> >
> > Signed-off-by: Matt Roper
> > Signed-off-by: Jouni Högander
> > Signed-off-by: Ramalingam C
> > ---
> > drivers/gpu/drm/i915/display/intel_snps_phy.c | 2 +-
> > drivers/gpu/drm/i915/i915_reg.h | 6 --
> > 2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_snps_phy.c
> > b/drivers/gpu/drm/i915/display/intel_snps_phy.c
> > index c60575cb5368..f08061c748b3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_snps_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_snps_phy.c
> > @@ -32,7 +32,7 @@ void intel_snps_phy_wait_for_calibration(struct
> > drm_i915_private *i915)
> > if (!intel_phy_is_snps(i915, phy))
> > continue;
> >
> > - if (intel_de_wait_for_clear(i915, ICL_PHY_MISC(phy),
> > + if (intel_de_wait_for_clear(i915, DG2_PHY_MISC(phy),
> > DG2_PHY_DP_TX_ACK_MASK,
> > 25))
> > drm_err(>drm, "SNPS PHY %c failed to
> > calibrate after 25ms.\n",
> > phy);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 4d12abb2d7ff..354c25f483cb 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9559,8 +9559,10 @@ enum skl_power_gate {
> >
> > #define _ICL_PHY_MISC_A0x64C00
> > #define _ICL_PHY_MISC_B0x64C04
> > -#define ICL_PHY_MISC(port) _MMIO_PORT(port, _ICL_PHY_MISC_A, \
> > -_ICL_PHY_MISC_B)
> > +#define _DG2_PHY_MISC_TC1 0x64C14 /* TC1="PHY E" but offset as if
> > "PHY F" */
> > +#define ICL_PHY_MISC(port) _MMIO_PORT(port, _ICL_PHY_MISC_A,
> > _ICL_PHY_MISC_B)
> > +#define DG2_PHY_MISC(port) ((port) == PHY_E ?
> > _MMIO(_DG2_PHY_MISC_TC1) : \
> > +ICL_PHY_MISC(port))
> > #define ICL_PHY_MISC_MUX_DDID (1 << 28)
> > #define ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN (1 << 23)
> > #define DG2_PHY_DP_TX_ACK_MASKREG_GENMASK(23,
> > 20)
> > --
> > 2.20.1
BR,
Jouni Högander
Re: [Intel-gfx] [PATCH] drm/i915/psr: Disable PSR2 selective fetch for all TGL steps
Hello Paul,
On Mon, 2022-02-07 at 16:38 -0500, Lyude Paul wrote:
> As we've unfortunately started to come to expect from PSR on Intel
> platforms, PSR2 selective fetch is not at all ready to be enabled on
> Tigerlake as it results in severe flickering issues - at least on
> this
> ThinkPad X1 Carbon 9th generation. The easiest way I've found of
> reproducing these issues is to just move the cursor around the left
> border
> of the screen (suspicious…).
Sorry to hear you have this bad experience with i915 PSR2 selective
fetch support. There is already some discussion about these problems
and possible root causes here:
https://gitlab.freedesktop.org/drm/intel/-/issues/4951
I have Dell XPS 13 7390 which has panel supporting PSR2. Luckily I'm
not facing these problems with it.
>
> So, fix people's displays again and turn PSR2 selective fetch off for
> all
> steppings of Tigerlake. This can be re-enabled again if someone from
> Intel
> finds the time to fix this functionality on OEM machines.
>
> Signed-off-by: Lyude Paul
> Fixes: 7f6002e58025 ("drm/i915/display: Enable PSR2 selective fetch
> by default")
> Cc: Gwan-gyeong Mun
> Cc: Ville Syrjälä
> Cc: José Roberto de Souza
> Cc: Jani Nikula
> Cc: Rodrigo Vivi
> Cc: intel-...@lists.freedesktop.org
> Cc: # v5.16+
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index a1a663f362e7..25c16abcd9cd 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -737,10 +737,14 @@ static bool
> intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp,
> return false;
> }
>
> - /* Wa_14010254185 Wa_14010103792 */
> - if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) {
> + /*
> + * There's two things stopping this from being enabled on TGL:
> + * For steps A0-C0: workarounds Wa_14010254185 Wa_14010103792
> are missing
> + * For all steps: PSR2 selective fetch causes screen flickering
> + */
> + if (IS_TIGERLAKE(dev_priv)) {
> drm_dbg_kms(_priv->drm,
> - "PSR2 sel fetch not enabled, missing the
> implementation of WAs\n");
> + "PSR2 sel fetch not enabled, currently
> broken on TGL\n");
> return false;
> }
>
BR,
Jouni Högander
