On Fri, 17 Feb 2023, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> On Fri, Feb 17, 2023 at 12:04:14PM +0200, Jani Nikula wrote:
>> On Thu, 16 Feb 2023, Imre Deak <imre.deak@xxxxxxxxx> wrote:
>> > On Thu, Feb 16, 2023 at 06:17:39PM +0200, Jani Nikula wrote:
>> >> sizeof(struct intel_dmc) > 1024 bytes, allocated on all platforms as
>> >> part of struct drm_i915_private, whether they have DMC or not.
>> >>
>> >> Allocate struct intel_dmc dynamically, and hide all the dmc details
>> >> behind an opaque pointer in intel_dmc.c.
>> >>
>> >> Care must be taken to take into account all cases: DMC not supported on
>> >> the platform, DMC supported but not initialized, and DMC initialized but
>> >> not loaded. For the second case, we need to move the wakeref out of
>> >> struct intel_dmc.
>> >>
>> >> Cc: Imre Deak <imre.deak@xxxxxxxxx>
>> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> >> ---
>> >> .../gpu/drm/i915/display/intel_display_core.h | 8 ++-
>> >> drivers/gpu/drm/i915/display/intel_dmc.c | 50 +++++++++++++++++--
>> >> drivers/gpu/drm/i915/display/intel_dmc.h | 34 +------------
>> >> .../drm/i915/display/intel_modeset_setup.c | 1 +
>> >> 4 files changed, 53 insertions(+), 40 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> index b870f7f47f2b..ff51149c5fcb 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> @@ -19,7 +19,6 @@
>> >> #include "intel_cdclk.h"
>> >> #include "intel_display_limits.h"
>> >> #include "intel_display_power.h"
>> >> -#include "intel_dmc.h"
>> >> #include "intel_dpll_mgr.h"
>> >> #include "intel_fbc.h"
>> >> #include "intel_global_state.h"
>> >> @@ -40,6 +39,7 @@ struct intel_cdclk_vals;
>> >> struct intel_color_funcs;
>> >> struct intel_crtc;
>> >> struct intel_crtc_state;
>> >> +struct intel_dmc;
>> >> struct intel_dpll_funcs;
>> >> struct intel_dpll_mgr;
>> >> struct intel_fbdev;
>> >> @@ -340,6 +340,11 @@ struct intel_display {
>> >> spinlock_t phy_lock;
>> >> } dkl;
>> >>
>> >> + struct {
>> >> + struct intel_dmc *dmc;
>> >> + intel_wakeref_t wakeref;
>> >> + } dmc;
>> >> +
>> >> struct {
>> >> /* VLV/CHV/BXT/GLK DSI MMIO register base address */
>> >> u32 mmio_base;
>> >> @@ -467,7 +472,6 @@ struct intel_display {
>> >>
>> >> /* Grouping using named structs. Keep sorted. */
>> >> struct intel_audio audio;
>> >> - struct intel_dmc dmc;
>> >> struct intel_dpll dpll;
>> >> struct intel_fbc *fbc[I915_MAX_FBCS];
>> >> struct intel_frontbuffer_tracking fb_tracking;
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>> >> index 1d156ac2eb4c..8428d08e0c3d 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>> >> @@ -38,9 +38,37 @@
>> >> * low-power state and comes back to normal.
>> >> */
>> >>
>> >> +enum intel_dmc_id {
>> >> + DMC_FW_MAIN = 0,
>> >> + DMC_FW_PIPEA,
>> >> + DMC_FW_PIPEB,
>> >> + DMC_FW_PIPEC,
>> >> + DMC_FW_PIPED,
>> >> + DMC_FW_MAX
>> >> +};
>> >> +
>> >> +struct intel_dmc {
>> >> + struct drm_i915_private *i915;
>> >> + struct work_struct work;
>> >> + const char *fw_path;
>> >> + u32 max_fw_size; /* bytes */
>> >> + u32 version;
>> >> + struct dmc_fw_info {
>> >> + u32 mmio_count;
>> >> + i915_reg_t mmioaddr[20];
>> >> + u32 mmiodata[20];
>> >> + u32 dmc_offset;
>> >> + u32 start_mmioaddr;
>> >> + u32 dmc_fw_size; /*dwords */
>> >> + u32 *payload;
>> >> + bool present;
>> >> + } dmc_info[DMC_FW_MAX];
>> >> +};
>> >> +
>> >> +/* Note: This may be NULL. */
>> >> static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
>> >> {
>> >> - return &i915->display.dmc;
>> >> + return i915->display.dmc.dmc;
>> >> }
>> >>
>> >> #define DMC_VERSION(major, minor) ((major) << 16 | (minor))
>> >> @@ -937,7 +965,10 @@ void intel_dmc_init(struct drm_i915_private *dev_priv)
>> >> if (!HAS_DMC(dev_priv))
>> >> return;
>> >>
>> >> - dmc = i915_to_dmc(dev_priv);
>> >> + dmc = kzalloc(sizeof(*dmc), GFP_KERNEL);
>> >> + if (!dmc)
>> >> + return;
>> >
>> > Couldn't driver loading fail in this case instead (simplifying the
>> > dmc==NULL checks elsewhere)?
>>
>> We could fail driver load, yes, but it still wouldn't simplify the NULL
>> checks because you could use i915.dmc_firmware_path="" to disable the
>> firmware, and what's the point in keeping the 1k memory allocated in
>> that case?
>
> Right, only noticed the same later. But that's only a debug feature so
> could be acceptable trade-off for simplicity imo; also the supported but
> non-initialized state wouldn't be needed then either.
>
> It's not a big deal and the check is local to intel_dmc.c, so either way
> on the patchset:
> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> (Querying the DMC FW presence in IGT needs to be fixed before merging.)
The IGT part seemed harder to fix than just preserving the debugfs
output for dmc == NULL. v3 at:
https://patchwork.freedesktop.org/series/114431/
>
>> BR,
>> Jani.
>>
>>
>> >
>> >> +
>> >> dmc->i915 = dev_priv;
>> >>
>> >> INIT_WORK(&dmc->work, dmc_load_work_fn);
>> >> @@ -991,10 +1022,9 @@ void intel_dmc_init(struct drm_i915_private *dev_priv)
>> >>
>> >> if (dev_priv->params.dmc_firmware_path) {
>> >> if (strlen(dev_priv->params.dmc_firmware_path) == 0) {
>> >> - dmc->fw_path = NULL;
>> >> drm_info(&dev_priv->drm,
>> >> "Disabling DMC firmware and runtime PM\n");
>> >> - return;
>> >> + goto out;
>> >> }
>> >>
>> >> dmc->fw_path = dev_priv->params.dmc_firmware_path;
>> >> @@ -1003,11 +1033,18 @@ void intel_dmc_init(struct drm_i915_private *dev_priv)
>> >> if (!dmc->fw_path) {
>> >> drm_dbg_kms(&dev_priv->drm,
>> >> "No known DMC firmware for platform, disabling runtime PM\n");
>> >> - return;
>> >> + goto out;
>> >> }
>> >>
>> >> + dev_priv->display.dmc.dmc = dmc;
>> >> +
>> >> drm_dbg_kms(&dev_priv->drm, "Loading %s\n", dmc->fw_path);
>> >> schedule_work(&dmc->work);
>> >> +
>> >> + return;
>> >> +
>> >> +out:
>> >> + kfree(dmc);
>> >> }
>> >>
>> >> /**
>> >> @@ -1074,6 +1111,9 @@ void intel_dmc_fini(struct drm_i915_private *dev_priv)
>> >> if (dmc) {
>> >> for_each_dmc_id(dmc_id)
>> >> kfree(dmc->dmc_info[dmc_id].payload);
>> >> +
>> >> + kfree(dmc);
>> >> + dev_priv->display.dmc.dmc = NULL;
>> >> }
>> >> }
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
>> >> index b74635497aa7..fd607afff2ef 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
>> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
>> >> @@ -6,44 +6,12 @@
>> >> #ifndef __INTEL_DMC_H__
>> >> #define __INTEL_DMC_H__
>> >>
>> >> -#include "i915_reg_defs.h"
>> >> -#include "intel_wakeref.h"
>> >> -#include <linux/workqueue.h>
>> >> +#include <linux/types.h>
>> >>
>> >> struct drm_i915_error_state_buf;
>> >> struct drm_i915_private;
>> >> -
>> >> enum pipe;
>> >>
>> >> -enum intel_dmc_id {
>> >> - DMC_FW_MAIN = 0,
>> >> - DMC_FW_PIPEA,
>> >> - DMC_FW_PIPEB,
>> >> - DMC_FW_PIPEC,
>> >> - DMC_FW_PIPED,
>> >> - DMC_FW_MAX
>> >> -};
>> >> -
>> >> -struct intel_dmc {
>> >> - struct drm_i915_private *i915;
>> >> - struct work_struct work;
>> >> - const char *fw_path;
>> >> - u32 max_fw_size; /* bytes */
>> >> - u32 version;
>> >> - struct dmc_fw_info {
>> >> - u32 mmio_count;
>> >> - i915_reg_t mmioaddr[20];
>> >> - u32 mmiodata[20];
>> >> - u32 dmc_offset;
>> >> - u32 start_mmioaddr;
>> >> - u32 dmc_fw_size; /*dwords */
>> >> - u32 *payload;
>> >> - bool present;
>> >> - } dmc_info[DMC_FW_MAX];
>> >> -
>> >> - intel_wakeref_t wakeref;
>> >> -};
>> >> -
>> >> void intel_dmc_init(struct drm_i915_private *i915);
>> >> void intel_dmc_load_program(struct drm_i915_private *i915);
>> >> void intel_dmc_disable_program(struct drm_i915_private *i915);
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> >> index 5359b9663a07..42bfd56fcbe4 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> >> @@ -22,6 +22,7 @@
>> >> #include "intel_display.h"
>> >> #include "intel_display_power.h"
>> >> #include "intel_display_types.h"
>> >> +#include "intel_dmc.h"
>> >> #include "intel_modeset_setup.h"
>> >> #include "intel_pch_display.h"
>> >> #include "intel_pm.h"
>> >> --
>> >> 2.34.1
>> >>
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
--
Jani Nikula, Intel Open Source Graphics Center
