Re: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
On Wed, 17 Apr 2024, Luca Coelho wrote: > On Wed, 2024-04-17 at 12:42 +0300, Jani Nikula wrote: >> On Mon, 15 Apr 2024, Luca Coelho wrote: >> > Thanks a lot for your reviews! Now I just need to get someone to merge >> > this series, since I don't have commit rights to the repo yet. >> >> Thanks for the patches and review, merged to drm-intel-next with a >> slightly heavy heart because it sets me back with [1] in a pretty >> annoying way. Oh well. >> >> BR, >> Jani. >> >> [1] >> https://lore.kernel.org/r/0b48d6bebfe90aa2f901a05be8279ed887d99d7a.1712665176.git.jani.nik...@intel.com > > Oh, no! But you do have cocci and scripts, so it should be easy? Let me > know if I can help you rebase your change. The cocci script completely broke apart with the changes. :( I don't know how to tell it to "first do all these transformations, then these, and then these" which seems to be necessary. BR, Jani. > > In any case, thanks for merging my patches! > > -- > Cheers, > Luca. -- Jani Nikula, Intel
Re: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
On Wed, 2024-04-17 at 12:42 +0300, Jani Nikula wrote: > On Mon, 15 Apr 2024, Luca Coelho wrote: > > Thanks a lot for your reviews! Now I just need to get someone to merge > > this series, since I don't have commit rights to the repo yet. > > Thanks for the patches and review, merged to drm-intel-next with a > slightly heavy heart because it sets me back with [1] in a pretty > annoying way. Oh well. > > BR, > Jani. > > [1] > https://lore.kernel.org/r/0b48d6bebfe90aa2f901a05be8279ed887d99d7a.1712665176.git.jani.nik...@intel.com Oh, no! But you do have cocci and scripts, so it should be easy? Let me know if I can help you rebase your change. In any case, thanks for merging my patches! -- Cheers, Luca.
Re: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
On Mon, 15 Apr 2024, Luca Coelho wrote: > Thanks a lot for your reviews! Now I just need to get someone to merge > this series, since I don't have commit rights to the repo yet. Thanks for the patches and review, merged to drm-intel-next with a slightly heavy heart because it sets me back with [1] in a pretty annoying way. Oh well. BR, Jani. [1] https://lore.kernel.org/r/0b48d6bebfe90aa2f901a05be8279ed887d99d7a.1712665176.git.jani.nik...@intel.com -- Jani Nikula, Intel
Re: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
On Mon, 2024-04-15 at 05:05 +, Shankar, Uma wrote: > > > -Original Message- > > From: Luca Coelho > > Sent: Friday, April 12, 2024 5:57 PM > > To: Shankar, Uma ; Coelho, Luciano > > ; intel-gfx@lists.freedesktop.org > > Cc: intel...@lists.freedesktop.org; ville.syrj...@linux.intel.com; Nikula, > > Jani > > > > Subject: Re: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks > > > > On Fri, 2024-04-12 at 10:30 +, Shankar, Uma wrote: > > > > > > > -Original Message- > > > > From: Coelho, Luciano > > > > Sent: Friday, April 12, 2024 3:12 PM > > > > To: intel-gfx@lists.freedesktop.org > > > > Cc: intel...@lists.freedesktop.org; Shankar, Uma > > > > ; ville.syrj...@linux.intel.com; Nikula, Jani > > > > > > > > Subject: [PATCH v5 1/4] drm/i915/display: add support for DMC > > > > wakelocks > > > > > > > > In order to reduce the DC5->DC2 restore time, wakelocks have been > > > > introduced in DMC so the driver can tell it when registers and other > > > > memory areas are going to be accessed and keep their respective blocks > > awake. > > > > > > > > Implement this in the driver by adding the concept of DMC wakelocks. > > > > When the driver needs to access memory which lies inside pre-defined > > > > ranges, it will tell DMC to set the wakelock, access the memory, > > > > then wait for a while and clear the wakelock. > > > > > > > > The wakelock state is protected in the driver with spinlocks to > > > > prevent concurrency issues. > > > > > > Hi Luca, > > > Seems you missed to add the version history. > > > > I've been sending the version history in the cover letter, because I don't > > think it > > adds any information after it gets to the mainline kernel. The history is > > lost > > anyway, so the mailing list is a better place to store it (it's unique and > > meaningful > > there). > > Its matter of preference, but being part of the patch's commit message it > stays with it > and can be checked with a git show. Cover letter details gets lost though as > it doesn't > end up in the tree. Yes, but the change history in the commit message doesn't tell the full story. If I write, for instance, "In v2: refactor intel_foo_bar()", there's no way to know what it looked like before. That's why I think that, if we want to keep the version history accessible from the git tree, we should have a link to the mailing list discussions, as apparently we do in most cases anyway. > > Bur as I said to someone else before, I can add it to the commit message if > > you > > think that it's needed. > > Not needed Luca, it was a simple nitpick 😊. You can skip that. Thanks for pointing out, anyway! 😉 I don't want to keep flaming about this, so I'll just try to remember to add it next time, so it doesn't come up again. > > > > > > Anyways, changes look good to me. > > > Reviewed-by: Uma Shankar > > > > Thanks a lot! > > > > Though you didn't review patch 3/4, the one about the module parameter. > > Was that intentional or did you just miss it? > > I think I have reviewed and RB'ed it. The entire series is RB'ed now. Oh, right. "Someone" was not paying attention. I even already had the r-b in the commit message itself. Silly me. Thanks a lot for your reviews! Now I just need to get someone to merge this series, since I don't have commit rights to the repo yet. -- Cheers, Luca.
RE: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
> -Original Message- > From: Luca Coelho > Sent: Friday, April 12, 2024 5:57 PM > To: Shankar, Uma ; Coelho, Luciano > ; intel-gfx@lists.freedesktop.org > Cc: intel...@lists.freedesktop.org; ville.syrj...@linux.intel.com; Nikula, > Jani > > Subject: Re: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks > > On Fri, 2024-04-12 at 10:30 +, Shankar, Uma wrote: > > > > > -Original Message- > > > From: Coelho, Luciano > > > Sent: Friday, April 12, 2024 3:12 PM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: intel...@lists.freedesktop.org; Shankar, Uma > > > ; ville.syrj...@linux.intel.com; Nikula, Jani > > > > > > Subject: [PATCH v5 1/4] drm/i915/display: add support for DMC > > > wakelocks > > > > > > In order to reduce the DC5->DC2 restore time, wakelocks have been > > > introduced in DMC so the driver can tell it when registers and other > > > memory areas are going to be accessed and keep their respective blocks > awake. > > > > > > Implement this in the driver by adding the concept of DMC wakelocks. > > > When the driver needs to access memory which lies inside pre-defined > > > ranges, it will tell DMC to set the wakelock, access the memory, > > > then wait for a while and clear the wakelock. > > > > > > The wakelock state is protected in the driver with spinlocks to > > > prevent concurrency issues. > > > > Hi Luca, > > Seems you missed to add the version history. > > I've been sending the version history in the cover letter, because I don't > think it > adds any information after it gets to the mainline kernel. The history is > lost > anyway, so the mailing list is a better place to store it (it's unique and > meaningful > there). Its matter of preference, but being part of the patch's commit message it stays with it and can be checked with a git show. Cover letter details gets lost though as it doesn't end up in the tree. > Bur as I said to someone else before, I can add it to the commit message if > you > think that it's needed. Not needed Luca, it was a simple nitpick 😊. You can skip that. > > > > Anyways, changes look good to me. > > Reviewed-by: Uma Shankar > > Thanks a lot! > > Though you didn't review patch 3/4, the one about the module parameter. > Was that intentional or did you just miss it? I think I have reviewed and RB'ed it. The entire series is RB'ed now. Regards, Uma Shankar > -- > Cheers, > Luca.
Re: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
On Fri, 2024-04-12 at 10:30 +, Shankar, Uma wrote: > > > -Original Message- > > From: Coelho, Luciano > > Sent: Friday, April 12, 2024 3:12 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: intel...@lists.freedesktop.org; Shankar, Uma ; > > ville.syrj...@linux.intel.com; Nikula, Jani > > Subject: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks > > > > In order to reduce the DC5->DC2 restore time, wakelocks have been introduced > > in DMC so the driver can tell it when registers and other memory areas are > > going > > to be accessed and keep their respective blocks awake. > > > > Implement this in the driver by adding the concept of DMC wakelocks. > > When the driver needs to access memory which lies inside pre-defined > > ranges, it > > will tell DMC to set the wakelock, access the memory, then wait for a while > > and > > clear the wakelock. > > > > The wakelock state is protected in the driver with spinlocks to prevent > > concurrency issues. > > Hi Luca, > Seems you missed to add the version history. I've been sending the version history in the cover letter, because I don't think it adds any information after it gets to the mainline kernel. The history is lost anyway, so the mailing list is a better place to store it (it's unique and meaningful there). Bur as I said to someone else before, I can add it to the commit message if you think that it's needed. > > Anyways, changes look good to me. > Reviewed-by: Uma Shankar Thanks a lot! Though you didn't review patch 3/4, the one about the module parameter. Was that intentional or did you just miss it? -- Cheers, Luca.
RE: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
> -Original Message-
> From: Coelho, Luciano
> Sent: Friday, April 12, 2024 3:12 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: intel...@lists.freedesktop.org; Shankar, Uma ;
> ville.syrj...@linux.intel.com; Nikula, Jani
> Subject: [PATCH v5 1/4] drm/i915/display: add support for DMC wakelocks
>
> In order to reduce the DC5->DC2 restore time, wakelocks have been introduced
> in DMC so the driver can tell it when registers and other memory areas are
> going
> to be accessed and keep their respective blocks awake.
>
> Implement this in the driver by adding the concept of DMC wakelocks.
> When the driver needs to access memory which lies inside pre-defined ranges,
> it
> will tell DMC to set the wakelock, access the memory, then wait for a while
> and
> clear the wakelock.
>
> The wakelock state is protected in the driver with spinlocks to prevent
> concurrency issues.
Hi Luca,
Seems you missed to add the version history.
Anyways, changes look good to me.
Reviewed-by: Uma Shankar
Regards,
Uma Shankar
> BSpec: 71583
> Signed-off-by: Luca Coelho
> ---
> Documentation/gpu/i915.rst| 9 +
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/display/intel_de.h | 97 +++-
> .../gpu/drm/i915/display/intel_display_core.h | 2 +
> drivers/gpu/drm/i915/display/intel_dmc_regs.h | 6 +
> drivers/gpu/drm/i915/display/intel_dmc_wl.c | 234 ++
> drivers/gpu/drm/i915/display/intel_dmc_wl.h | 31 +++
> drivers/gpu/drm/xe/Makefile | 1 +
> 8 files changed, 373 insertions(+), 8 deletions(-) create mode 100644
> drivers/gpu/drm/i915/display/intel_dmc_wl.c
> create mode 100644 drivers/gpu/drm/i915/display/intel_dmc_wl.h
>
> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst index
> 0ca1550fd9dc..17261ba18313 100644
> --- a/Documentation/gpu/i915.rst
> +++ b/Documentation/gpu/i915.rst
> @@ -204,6 +204,15 @@ DMC Firmware Support .. kernel-doc::
> drivers/gpu/drm/i915/display/intel_dmc.c
> :internal:
>
> +DMC wakelock support
> +
> +
> +.. kernel-doc:: drivers/gpu/drm/i915/display/intel_dmc_wl.c
> + :doc: DMC wakelock support
> +
> +.. kernel-doc:: drivers/gpu/drm/i915/display/intel_dmc_wl.c
> + :internal:
> +
> Video BIOS Table (VBT)
> --
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index af9e871daf1d..7cad944b825c 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -266,6 +266,7 @@ i915-y += \
> display/intel_display_rps.o \
> display/intel_display_wa.o \
> display/intel_dmc.o \
> + display/intel_dmc_wl.o \
> display/intel_dpio_phy.o \
> display/intel_dpll.o \
> display/intel_dpll_mgr.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_de.h
> b/drivers/gpu/drm/i915/display/intel_de.h
> index ba7a1c6ebc2a..0a0fba81e7af 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -13,52 +13,125 @@
> static inline u32
> intel_de_read(struct drm_i915_private *i915, i915_reg_t reg) {
> - return intel_uncore_read(&i915->uncore, reg);
> + u32 val;
> +
> + intel_dmc_wl_get(i915, reg);
> +
> + val = intel_uncore_read(&i915->uncore, reg);
> +
> + intel_dmc_wl_put(i915, reg);
> +
> + return val;
> }
>
> static inline u8
> intel_de_read8(struct drm_i915_private *i915, i915_reg_t reg) {
> - return intel_uncore_read8(&i915->uncore, reg);
> + u8 val;
> +
> + intel_dmc_wl_get(i915, reg);
> +
> + val = intel_uncore_read8(&i915->uncore, reg);
> +
> + intel_dmc_wl_put(i915, reg);
> +
> + return val;
> }
>
> static inline u64
> intel_de_read64_2x32(struct drm_i915_private *i915,
>i915_reg_t lower_reg, i915_reg_t upper_reg) {
> - return intel_uncore_read64_2x32(&i915->uncore, lower_reg,
> upper_reg);
> + u64 val;
> +
> + intel_dmc_wl_get(i915, lower_reg);
> + intel_dmc_wl_get(i915, upper_reg);
> +
> + val = intel_uncore_read64_2x32(&i915->uncore, lower_reg, upper_reg);
> +
> + intel_dmc_wl_put(i915, upper_reg);
> + intel_dmc_wl_put(i915, lower_reg);
> +
> + return val;
> }
>
> static inline void
> intel_de_posting_read(struct drm_i915_private *i915, i915_reg_t reg) {
> + intel_dmc_wl_get(i915, reg);
> +
> intel_uncore_posting_read(&i915->uncore, reg);
> +
> + intel_dmc_wl_put(i915, reg);
> }
>
> static inline void
> intel_de_write(struct drm_i915_private *i915, i915_reg_t reg, u32 val) {
> + intel_dmc_wl_get(i915, reg);
> +
> intel_uncore_write(&i915->uncore, reg, val);
> +
> + intel_dmc_wl_put(i915, reg);
> }
>
> static inline u32
> -intel_de_rmw(struct drm_i915_private *i915, i915_reg_t reg, u32 clear, u32
> set)
> +__intel_de_rmw_nowl(struct drm_i915_private *i915, i915_reg_t reg,
> + u32 clear, u32 set)
> {
> retur
