From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>
Cc: "De Marchi, Lucas" <lucas.demarchi@intel.com>
Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915/dmc: Add intel_dmc_has_payload() helper
Date: Fri, 21 May 2021 19:28:09 +0000 [thread overview]
Message-ID: <459edf2c7b5841b681ada482d229c5f7@intel.com> (raw)
In-Reply-To: <87r1i0csja.fsf@intel.com>
> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Friday, May 21, 2021 3:04 AM
> To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: De Marchi, Lucas <lucas.demarchi@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915/dmc: Add
> intel_dmc_has_payload() helper
>
> On Thu, 20 May 2021, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
> > We check for dmc_payload being there at various points in the driver.
> > Replace it with the helper.
>
> Seems like a good idea. Some comments inline.
>
> BR,
> Jani.
>
> >
> > v2: rebased.
> > v3: Move intel_dmc to intel_dmc.h in another patch (Lucas)
> >
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> > .../gpu/drm/i915/display/intel_display_debugfs.c | 4 ++--
> > .../gpu/drm/i915/display/intel_display_power.c | 16 ++++++++--------
> > drivers/gpu/drm/i915/display/intel_dmc.c | 13 +++++++++----
> > drivers/gpu/drm/i915/display/intel_dmc.h | 5 +++++
> > drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> > 5 files changed, 25 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index 94e5cbd86e77..88bb05d5c483 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -542,10 +542,10 @@ static int i915_dmc_info(struct seq_file *m,
> > void *unused)
> >
> > wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> >
> > - seq_printf(m, "fw loaded: %s\n", yesno(dmc->dmc_payload));
> > + seq_printf(m, "fw loaded: %s\n",
> > +yesno(intel_dmc_has_payload(dev_priv)));
> > seq_printf(m, "path: %s\n", dmc->fw_path);
> >
> > - if (!dmc->dmc_payload)
> > + if (!intel_dmc_has_payload(dev_priv))
> > goto out;
> >
> > seq_printf(m, "version: %d.%d\n", DMC_VERSION_MAJOR(dmc-
> >version),
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 991ceea06a07..b546672c9b00 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -1220,7 +1220,7 @@ static void
> gen9_dc_off_power_well_enable(struct
> > drm_i915_private *dev_priv, static void
> gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
> > struct i915_power_well
> *power_well) {
> > - if (!dev_priv->dmc.dmc_payload)
> > + if (!intel_dmc_has_payload(dev_priv))
> > return;
> >
> > switch (dev_priv->dmc.target_dc_state) { @@ -5579,7 +5579,7 @@
> > static void skl_display_core_init(struct drm_i915_private *dev_priv,
> >
> > gen9_dbuf_enable(dev_priv);
> >
> > - if (resume && dev_priv->dmc.dmc_payload)
> > + if (resume && intel_dmc_has_payload(dev_priv))
> > intel_dmc_load_program(dev_priv);
> > }
> >
> > @@ -5646,7 +5646,7 @@ static void bxt_display_core_init(struct
> > drm_i915_private *dev_priv, bool resume
> >
> > gen9_dbuf_enable(dev_priv);
> >
> > - if (resume && dev_priv->dmc.dmc_payload)
> > + if (resume && intel_dmc_has_payload(dev_priv))
> > intel_dmc_load_program(dev_priv);
> > }
> >
> > @@ -5712,7 +5712,7 @@ static void cnl_display_core_init(struct
> drm_i915_private *dev_priv, bool resume
> > /* 6. Enable DBUF */
> > gen9_dbuf_enable(dev_priv);
> >
> > - if (resume && dev_priv->dmc.dmc_payload)
> > + if (resume && intel_dmc_has_payload(dev_priv))
> > intel_dmc_load_program(dev_priv);
> > }
> >
> > @@ -5869,7 +5869,7 @@ static void icl_display_core_init(struct
> drm_i915_private *dev_priv,
> > if (DISPLAY_VER(dev_priv) >= 12)
> > tgl_bw_buddy_init(dev_priv);
> >
> > - if (resume && dev_priv->dmc.dmc_payload)
> > + if (resume && intel_dmc_has_payload(dev_priv))
> > intel_dmc_load_program(dev_priv);
> >
> > /* Wa_14011508470 */
> > @@ -6230,7 +6230,7 @@ void intel_power_domains_suspend(struct
> drm_i915_private *i915,
> > */
> > if (!(i915->dmc.allowed_dc_mask & DC_STATE_EN_DC9) &&
> > suspend_mode == I915_DRM_SUSPEND_IDLE &&
> > - i915->dmc.dmc_payload) {
> > + intel_dmc_has_payload(i915)) {
> > intel_display_power_flush_work(i915);
> > intel_power_domains_verify_state(i915);
> > return;
> > @@ -6420,7 +6420,7 @@ void intel_display_power_resume(struct
> drm_i915_private *i915)
> > if (DISPLAY_VER(i915) >= 11) {
> > bxt_disable_dc9(i915);
> > icl_display_core_init(i915, true);
> > - if (i915->dmc.dmc_payload) {
> > + if (intel_dmc_has_payload(i915)) {
> > if (i915->dmc.allowed_dc_mask &
> > DC_STATE_EN_UPTO_DC6)
> > skl_enable_dc6(i915);
> > @@ -6431,7 +6431,7 @@ void intel_display_power_resume(struct
> drm_i915_private *i915)
> > } else if (IS_GEMINILAKE(i915) || IS_BROXTON(i915)) {
> > bxt_disable_dc9(i915);
> > bxt_display_core_init(i915, true);
> > - if (i915->dmc.dmc_payload &&
> > + if (intel_dmc_has_payload(i915) &&
> > (i915->dmc.allowed_dc_mask &
> DC_STATE_EN_UPTO_DC5))
> > gen9_enable_dc5(i915);
> > } else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) { diff --git
> > a/drivers/gpu/drm/i915/display/intel_dmc.c
> > b/drivers/gpu/drm/i915/display/intel_dmc.c
> > index 71ef6022d4af..14282e5fdf8b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > @@ -237,6 +237,11 @@ struct stepping_info {
> > char substepping;
> > };
> >
> > +bool intel_dmc_has_payload(struct drm_i915_private *dev_priv) {
> > + return dev_priv->dmc.dmc_payload;
>
> Please use i915 name for struct drm_i915_private when adding new code.
>
> > +}
> > +
> > static const struct stepping_info skl_stepping_info[] = {
> > {'A', '0'}, {'B', '0'}, {'C', '0'},
> > {'D', '0'}, {'E', '0'}, {'F', '0'},
> > @@ -320,7 +325,7 @@ void intel_dmc_load_program(struct
> drm_i915_private *dev_priv)
> > return;
> > }
> >
> > - if (!dev_priv->dmc.dmc_payload) {
> > + if (!intel_dmc_has_payload(dev_priv)) {
> > drm_err(&dev_priv->drm,
> > "Tried to program CSR with empty payload\n");
> > return;
> > @@ -658,7 +663,7 @@ static void dmc_load_work_fn(struct work_struct
> *work)
> > request_firmware(&fw, dev_priv->dmc.fw_path, dev_priv-
> >drm.dev);
> > parse_dmc_fw(dev_priv, fw);
> >
> > - if (dev_priv->dmc.dmc_payload) {
> > + if (intel_dmc_has_payload(dev_priv)) {
> > intel_dmc_load_program(dev_priv);
> > intel_dmc_runtime_pm_put(dev_priv);
> >
> > @@ -787,7 +792,7 @@ void intel_dmc_ucode_suspend(struct
> drm_i915_private *dev_priv)
> > flush_work(&dev_priv->dmc.work);
> >
> > /* Drop the reference held in case DMC isn't loaded. */
> > - if (!dev_priv->dmc.dmc_payload)
> > + if (!intel_dmc_has_payload(dev_priv))
> > intel_dmc_runtime_pm_put(dev_priv);
> > }
> >
> > @@ -807,7 +812,7 @@ void intel_dmc_ucode_resume(struct
> drm_i915_private *dev_priv)
> > * Reacquire the reference to keep RPM disabled in case DMC isn't
> > * loaded.
> > */
> > - if (!dev_priv->dmc.dmc_payload)
> > + if (!intel_dmc_has_payload(dev_priv))
> > intel_dmc_runtime_pm_get(dev_priv);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h
> > b/drivers/gpu/drm/i915/display/intel_dmc.h
> > index 57dd99da0ced..a928172459e3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> > @@ -6,6 +6,10 @@
> > #ifndef __INTEL_DMC_H__
> > #define __INTEL_DMC_H__
> >
> > +#include <drm/drm_util.h>
> > +#include "intel_wakeref.h"
> > +#include "i915_reg.h"
> > +
>
> You don't need any of these for the patch at hand. Please remove.
Actually for i915_reg_t used in the intel_dmc struct, I need to have the header included here.
I am making the other changes.
Thanks,
Anusha
> > struct drm_i915_private;
> >
> > #define DMC_VERSION(major, minor) ((major) << 16 | (minor))
> > @@ -17,5 +21,6 @@ void intel_dmc_load_program(struct
> drm_i915_private
> > *i915); void intel_dmc_ucode_fini(struct drm_i915_private *i915);
> > void intel_dmc_ucode_suspend(struct drm_i915_private *i915); void
> > intel_dmc_ucode_resume(struct drm_i915_private *i915);
> > +bool intel_dmc_has_payload(struct drm_i915_private *i915);
> >
> > #endif /* __INTEL_DMC_H__ */
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 8b964e355cb5..833d3e8b7631 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -792,7 +792,7 @@ static void __err_print_to_sgl(struct
> drm_i915_error_state_buf *m,
> > struct intel_dmc *dmc = &m->i915->dmc;
> >
> > err_printf(m, "DMC loaded: %s\n",
> > - yesno(dmc->dmc_payload));
> > + yesno(intel_dmc_has_payload(m->i915) != 0));
>
> The != 0 part is unnecessary.
>
> BR,
> Jani.
>
> > err_printf(m, "DMC fw version: %d.%d\n",
> > DMC_VERSION_MAJOR(dmc->version),
> > DMC_VERSION_MINOR(dmc->version));
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-05-21 19:29 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-20 23:53 [Intel-gfx] [PATCH 0/3] More DMC cleanup Anusha Srivatsa
2021-05-20 23:53 ` [Intel-gfx] [PATCH 1/3] drm/i915/dmc: s/DRM_ERROR/drm_err Anusha Srivatsa
2021-05-21 10:00 ` Jani Nikula
2021-05-20 23:53 ` [Intel-gfx] [PATCH 2/3] drm/i915/dmc: Add intel_dmc_has_payload() helper Anusha Srivatsa
2021-05-21 10:03 ` Jani Nikula
2021-05-21 19:28 ` Srivatsa, Anusha [this message]
2021-05-21 19:42 ` Srivatsa, Anusha
2021-05-20 23:53 ` [Intel-gfx] [PATCH 3/3] drm/i915/dmc: Move struct intel_dmc to intel_dmc.h Anusha Srivatsa
2021-05-21 0:27 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for More DMC cleanup (rev2) Patchwork
2021-05-21 0:57 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-05-22 11:15 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
-- strict thread matches above, loose matches on Subject: below --
2021-05-21 19:51 [Intel-gfx] [PATCH 0/3] More DMC cleanup Anusha Srivatsa
2021-05-21 19:51 ` [Intel-gfx] [PATCH 2/3] drm/i915/dmc: Add intel_dmc_has_payload() helper Anusha Srivatsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=459edf2c7b5841b681ada482d229c5f7@intel.com \
--to=anusha.srivatsa@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=lucas.demarchi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.