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-de...@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 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-de...@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;
> > + 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 + waketim
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 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
