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(&intel_dp->aux, > > > DP_RECEIVER_ALPM_CAP, > > > + &alpm_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 = &crtc_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 4/6] drm/i915/alpm: Add compute config for lobf
> -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(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, > > + &alpm_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 = &crtc_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; > > + 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) > > &g
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(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, > + &alpm_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 = &crtc_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 > b/dri
Re: [PATCH v3 4/6] drm/i915/alpm: Add compute config for lobf
Hi Animesh, kernel test robot noticed the following build errors: [auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on drm-tip/drm-tip drm/drm-next drm-exynos/exynos-drm-next next-20240426] [cannot apply to drm-intel/for-linux-next-fixes linus/master v6.9-rc5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Animesh-Manna/drm-i915-alpm-Move-alpm-parameters-from-intel_psr/20240425-025652 base: git://anongit.freedesktop.org/drm-intel for-linux-next patch link: https://lore.kernel.org/r/20240424183820.3591593-5-animesh.manna%40intel.com patch subject: [PATCH v3 4/6] drm/i915/alpm: Add compute config for lobf config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20240427/202404270117.qrlkmdn7-...@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240427/202404270117.qrlkmdn7-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202404270117.qrlkmdn7-...@intel.com/ All errors (new ones prefixed by >>, old ones prefixed by <<): WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/maps/map_funcs.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/hisi-spmi-controller.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/spmi-pmic-arb.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_cif.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_aec.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_netx.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_pruss.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_mf624.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/pcmcia/pcmcia_rsrc.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/pcmcia/yenta_socket.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/pcmcia/i82092.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/corsair-cpro.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/mr75203.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/vhost/vringh.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/greybus/greybus.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/greybus/gb-es2.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/rpmsg/rpmsg_char.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/adc/ingenic-adc.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/adc/xilinx-ams.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/buffer/kfifo_buf.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-core.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-hub.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-aspeed.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-gpio.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-ast-cf.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-scom.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/siox/siox-bus-gpio.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/counter/ftm-quaddec.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/core/snd-pcm-dmaengine.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/core/sound_kunit.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/drivers/snd-pcmtest.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/pci/hda/snd-hda-cirrus-scodec-test.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/soc-topology-test.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-ab8500-codec.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-sigmadsp.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-wm-adsp.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/fsl/imx-pcm-dma.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/intel/avs/boards/snd-soc-avs-da7219.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/intel/avs/boards/snd-soc-avs-dmic.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/intel/avs/boards/snd-soc-avs-i2s-test.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/intel/avs/boards/snd-soc-avs-max98927.o WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/intel/avs/boards/snd-soc-avs-max98357a.o WARNING: modpost: missing MODULE