| Message ID | 1542787159-32662-1-git-send-email-shawn.c.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Release power well if load DMC failed | expand |
On Tue, 20 Nov 2018, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote: > Driver obtain power well at intel_csr_ucode_init(). > And release it after load DMC firmware successful. Correct. > An issue happened when DMC was not found or failed > to load. Power well would not be released and just > output some error messages. Driver have to release > power well properly to keep put/get balance. No. We intentionally do not release it until dmc firmware load succeeds. See the comment in intel_csr_ucode_init(), as well as this in the branch where dmc load fails: dev_notice(dev_priv->drm.dev, "Failed to load DMC firmware %s." " Disabling runtime power management.\n", csr->fw_path); We don't support runtime pm without dmc on platforms with dmc. BR, Jani. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Jose Roberto de Souza <jose.souza@intel.com> > Cc: Cooper Chiou <cooper.chiou@intel.com> > > Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com> > --- > drivers/gpu/drm/i915/intel_csr.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index a516697bf57d..8d04d7b6f00a 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -425,8 +425,6 @@ static void csr_load_work_fn(struct work_struct *work) > if (dev_priv->csr.dmc_payload) { > intel_csr_load_program(dev_priv); > > - intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > - > DRM_INFO("Finished loading DMC firmware %s (v%u.%u)\n", > dev_priv->csr.fw_path, > CSR_VERSION_MAJOR(csr->version), > @@ -440,6 +438,7 @@ static void csr_load_work_fn(struct work_struct *work) > INTEL_UC_FIRMWARE_URL); > } > > + intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > release_firmware(fw); > }
On Tue, 20 Nov 2018, "Jani Nikula" <jani.nikula@intel.com> wrote: >> Driver obtain power well at intel_csr_ucode_init(). >> And release it after load DMC firmware successful. > >Correct. > >> An issue happened when DMC was not found or failed to load. Power well >> would not be released and just output some error messages. Driver have >> to release power well properly to keep put/get balance. > >No. We intentionally do not release it until dmc firmware load succeeds. If load DMC failed, we found DP phy was always on even without external display connected. So it looks like an expected behavior, right? > >See the comment in intel_csr_ucode_init(), as well as this in the branch where dmc load fails: > > dev_notice(dev_priv->drm.dev, > "Failed to load DMC firmware %s." > " Disabling runtime power management.\n", > csr->fw_path); > >We don't support runtime pm without dmc on platforms with dmc. > >BR, >Jani. > >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Jose Roberto de Souza <jose.souza@intel.com> >> Cc: Cooper Chiou <cooper.chiou@intel.com> >> >> Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com> >> --- >> drivers/gpu/drm/i915/intel_csr.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_csr.c >> b/drivers/gpu/drm/i915/intel_csr.c >> index a516697bf57d..8d04d7b6f00a 100644 >> --- a/drivers/gpu/drm/i915/intel_csr.c >> +++ b/drivers/gpu/drm/i915/intel_csr.c >> @@ -425,8 +425,6 @@ static void csr_load_work_fn(struct work_struct *work) >> if (dev_priv->csr.dmc_payload) { >> intel_csr_load_program(dev_priv); >> >> - intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >> - >> DRM_INFO("Finished loading DMC firmware %s (v%u.%u)\n", >> dev_priv->csr.fw_path, >> CSR_VERSION_MAJOR(csr->version), >> @@ -440,6 +438,7 @@ static void csr_load_work_fn(struct work_struct *work) >> INTEL_UC_FIRMWARE_URL); >> } >> >> + intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >> release_firmware(fw); >> } > >-- >Jani Nikula, Intel Open Source Graphics Center
On Wed, 21 Nov 2018, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote: > On Tue, 20 Nov 2018, "Jani Nikula" <jani.nikula@intel.com> wrote: >>> Driver obtain power well at intel_csr_ucode_init(). >>> And release it after load DMC firmware successful. >> >>Correct. >> >>> An issue happened when DMC was not found or failed to load. Power well >>> would not be released and just output some error messages. Driver have >>> to release power well properly to keep put/get balance. >> >>No. We intentionally do not release it until dmc firmware load succeeds. > > If load DMC failed, we found DP phy was always on even without > external display connected. So it looks like an expected behavior, > right? I'll put it this way, we don't really go out of our way to support everything without the DMC firmware. Every choice like this doubles the testing requirements. Do you see issues with DMC firmware loaded? Do you have issues with loading DMC firmware? BR, Jani. > >> >>See the comment in intel_csr_ucode_init(), as well as this in the branch where dmc load fails: >> >> dev_notice(dev_priv->drm.dev, >> "Failed to load DMC firmware %s." >> " Disabling runtime power management.\n", >> csr->fw_path); >> >>We don't support runtime pm without dmc on platforms with dmc. >> >>BR, >>Jani. >> >>> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Cc: Jose Roberto de Souza <jose.souza@intel.com> >>> Cc: Cooper Chiou <cooper.chiou@intel.com> >>> >>> Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_csr.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_csr.c >>> b/drivers/gpu/drm/i915/intel_csr.c >>> index a516697bf57d..8d04d7b6f00a 100644 >>> --- a/drivers/gpu/drm/i915/intel_csr.c >>> +++ b/drivers/gpu/drm/i915/intel_csr.c >>> @@ -425,8 +425,6 @@ static void csr_load_work_fn(struct work_struct *work) >>> if (dev_priv->csr.dmc_payload) { >>> intel_csr_load_program(dev_priv); >>> >>> - intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >>> - >>> DRM_INFO("Finished loading DMC firmware %s (v%u.%u)\n", >>> dev_priv->csr.fw_path, >>> CSR_VERSION_MAJOR(csr->version), >>> @@ -440,6 +438,7 @@ static void csr_load_work_fn(struct work_struct *work) >>> INTEL_UC_FIRMWARE_URL); >>> } >>> >>> + intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >>> release_firmware(fw); >>> } >> >>-- >>Jani Nikula, Intel Open Source Graphics Center
On Tue, 20 Nov 2018, "Jani Nikula" <jani.nikula@intel.com> wrote: >On Wed, 21 Nov 2018, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote: >> On Tue, 20 Nov 2018, "Jani Nikula" <jani.nikula@intel.com> wrote: >>>> Driver obtain power well at intel_csr_ucode_init(). >>>> And release it after load DMC firmware successful. >>> >>>Correct. >>> >>>> An issue happened when DMC was not found or failed to load. Power >>>> well would not be released and just output some error messages. >>>> Driver have to release power well properly to keep put/get balance. >>> >>>No. We intentionally do not release it until dmc firmware load succeeds. >> >> If load DMC failed, we found DP phy was always on even without >> external display connected. So it looks like an expected behavior, >> right? > >I'll put it this way, we don't really go out of our way to support everything without the DMC firmware. Every choice like this doubles the testing requirements. > Understood. This is not a normal case (without DMC) on customer system. We just think it will be better to release power well if we already got it before. >Do you see issues with DMC firmware loaded? Do you have issues with loading DMC firmware? No issue with DMC firmware loaded. Thanks! > >BR, >Jani. > > >> >>> >>>See the comment in intel_csr_ucode_init(), as well as this in the branch where dmc load fails: >>> >>> dev_notice(dev_priv->drm.dev, >>> "Failed to load DMC firmware %s." >>> " Disabling runtime power management.\n", >>> csr->fw_path); >>> >>>We don't support runtime pm without dmc on platforms with dmc. >>> >>>BR, >>>Jani. >>> >>>> >>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Cc: Jose Roberto de Souza <jose.souza@intel.com> >>>> Cc: Cooper Chiou <cooper.chiou@intel.com> >>>> >>>> Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_csr.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_csr.c >>>> b/drivers/gpu/drm/i915/intel_csr.c >>>> index a516697bf57d..8d04d7b6f00a 100644 >>>> --- a/drivers/gpu/drm/i915/intel_csr.c >>>> +++ b/drivers/gpu/drm/i915/intel_csr.c >>>> @@ -425,8 +425,6 @@ static void csr_load_work_fn(struct work_struct *work) >>>> if (dev_priv->csr.dmc_payload) { >>>> intel_csr_load_program(dev_priv); >>>> >>>> - intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >>>> - >>>> DRM_INFO("Finished loading DMC firmware %s (v%u.%u)\n", >>>> dev_priv->csr.fw_path, >>>> CSR_VERSION_MAJOR(csr->version), @@ -440,6 +438,7 @@ static >>>> void csr_load_work_fn(struct work_struct *work) >>>> INTEL_UC_FIRMWARE_URL); >>>> } >>>> >>>> + intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >>>> release_firmware(fw); >>>> } >>> >>>-- >>>Jani Nikula, Intel Open Source Graphics Center > >-- >Jani Nikula, Intel Open Source Graphics Center
On Wed, 21 Nov 2018, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote: > On Tue, 20 Nov 2018, "Jani Nikula" <jani.nikula@intel.com> wrote: >>On Wed, 21 Nov 2018, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote: >>> On Tue, 20 Nov 2018, "Jani Nikula" <jani.nikula@intel.com> wrote: >>>>> Driver obtain power well at intel_csr_ucode_init(). >>>>> And release it after load DMC firmware successful. >>>> >>>>Correct. >>>> >>>>> An issue happened when DMC was not found or failed to load. Power >>>>> well would not be released and just output some error messages. >>>>> Driver have to release power well properly to keep put/get balance. >>>> >>>>No. We intentionally do not release it until dmc firmware load succeeds. >>> >>> If load DMC failed, we found DP phy was always on even without >>> external display connected. So it looks like an expected behavior, >>> right? >> >>I'll put it this way, we don't really go out of our way to support everything without the DMC firmware. Every choice like this doubles the testing requirements. >> > > Understood. This is not a normal case (without DMC) on customer > system. We just think it will be better to release power well if we > already got it before. Why are you supporting a non-DMC setup on a customer system? Don't do it. BR, Jani. > >>Do you see issues with DMC firmware loaded? Do you have issues with loading DMC firmware? > > No issue with DMC firmware loaded. Thanks! > >> >>BR, >>Jani. >> >> >>> >>>> >>>>See the comment in intel_csr_ucode_init(), as well as this in the branch where dmc load fails: >>>> >>>> dev_notice(dev_priv->drm.dev, >>>> "Failed to load DMC firmware %s." >>>> " Disabling runtime power management.\n", >>>> csr->fw_path); >>>> >>>>We don't support runtime pm without dmc on platforms with dmc. >>>> >>>>BR, >>>>Jani. >>>> >>>>> >>>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>>> Cc: Jose Roberto de Souza <jose.souza@intel.com> >>>>> Cc: Cooper Chiou <cooper.chiou@intel.com> >>>>> >>>>> Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_csr.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_csr.c >>>>> b/drivers/gpu/drm/i915/intel_csr.c >>>>> index a516697bf57d..8d04d7b6f00a 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_csr.c >>>>> +++ b/drivers/gpu/drm/i915/intel_csr.c >>>>> @@ -425,8 +425,6 @@ static void csr_load_work_fn(struct work_struct *work) >>>>> if (dev_priv->csr.dmc_payload) { >>>>> intel_csr_load_program(dev_priv); >>>>> >>>>> - intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >>>>> - >>>>> DRM_INFO("Finished loading DMC firmware %s (v%u.%u)\n", >>>>> dev_priv->csr.fw_path, >>>>> CSR_VERSION_MAJOR(csr->version), @@ -440,6 +438,7 @@ static >>>>> void csr_load_work_fn(struct work_struct *work) >>>>> INTEL_UC_FIRMWARE_URL); >>>>> } >>>>> >>>>> + intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >>>>> release_firmware(fw); >>>>> } >>>> >>>>-- >>>>Jani Nikula, Intel Open Source Graphics Center >> >>-- >>Jani Nikula, Intel Open Source Graphics Center
On Wed, 21 Nov 2018, "Jani Nikula" <jani.nikula@intel.com> wrote: >On Wed, 21 Nov 2018, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote: >> On Tue, 20 Nov 2018, "Jani Nikula" <jani.nikula@intel.com> wrote: >>>On Wed, 21 Nov 2018, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote: >>>> On Tue, 20 Nov 2018, "Jani Nikula" <jani.nikula@intel.com> wrote: >>>>>> Driver obtain power well at intel_csr_ucode_init(). >>>>>> And release it after load DMC firmware successful. >>>>> >>>>>Correct. >>>>> >>>>>> An issue happened when DMC was not found or failed to load. Power >>>>>> well would not be released and just output some error messages. >>>>>> Driver have to release power well properly to keep put/get balance. >>>>> >>>>>No. We intentionally do not release it until dmc firmware load succeeds. >>>> >>>> If load DMC failed, we found DP phy was always on even without >>>> external display connected. So it looks like an expected behavior, >>>> right? >>> >>>I'll put it this way, we don't really go out of our way to support everything without the DMC firmware. Every choice like this doubles the testing requirements. >>> >> >> Understood. This is not a normal case (without DMC) on customer >> system. We just think it will be better to release power well if we >> already got it before. > >Why are you supporting a non-DMC setup on a customer system? Don't do it. > As we mention before. It is an abnormal case that DMC was lost on customer system. Then we got an issue like this. Everything works normally if DMC is available. Thanks for comments! >BR, >Jani. > >> >>>Do you see issues with DMC firmware loaded? Do you have issues with loading DMC firmware? >> >> No issue with DMC firmware loaded. Thanks! >> >>> >>>BR, >>>Jani. >>> >>> >>>> >>>>> >>>>>See the comment in intel_csr_ucode_init(), as well as this in the branch where dmc load fails: >>>>> >>>>> dev_notice(dev_priv->drm.dev, >>>>> "Failed to load DMC firmware %s." >>>>> " Disabling runtime power management.\n", >>>>> csr->fw_path); >>>>> >>>>>We don't support runtime pm without dmc on platforms with dmc. >>>>> >>>>>BR, >>>>>Jani. >>>>> >>>>>> >>>>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>>>> Cc: Jose Roberto de Souza <jose.souza@intel.com> >>>>>> Cc: Cooper Chiou <cooper.chiou@intel.com> >>>>>> >>>>>> Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_csr.c | 3 +-- >>>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_csr.c >>>>>> b/drivers/gpu/drm/i915/intel_csr.c >>>>>> index a516697bf57d..8d04d7b6f00a 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_csr.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_csr.c >>>>>> @@ -425,8 +425,6 @@ static void csr_load_work_fn(struct work_struct *work) >>>>>> if (dev_priv->csr.dmc_payload) { >>>>>> intel_csr_load_program(dev_priv); >>>>>> >>>>>> - intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >>>>>> - >>>>>> DRM_INFO("Finished loading DMC firmware %s (v%u.%u)\n", >>>>>> dev_priv->csr.fw_path, >>>>>> CSR_VERSION_MAJOR(csr->version), @@ -440,6 +438,7 @@ static >>>>>> void csr_load_work_fn(struct work_struct *work) >>>>>> INTEL_UC_FIRMWARE_URL); >>>>>> } >>>>>> >>>>>> + intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >>>>>> release_firmware(fw); >>>>>> } >>>>> >>>>>-- >>>>>Jani Nikula, Intel Open Source Graphics Center >>> >>>-- >>>Jani Nikula, Intel Open Source Graphics Center > >-- >Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index a516697bf57d..8d04d7b6f00a 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -425,8 +425,6 @@ static void csr_load_work_fn(struct work_struct *work) if (dev_priv->csr.dmc_payload) { intel_csr_load_program(dev_priv); - intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); - DRM_INFO("Finished loading DMC firmware %s (v%u.%u)\n", dev_priv->csr.fw_path, CSR_VERSION_MAJOR(csr->version), @@ -440,6 +438,7 @@ static void csr_load_work_fn(struct work_struct *work) INTEL_UC_FIRMWARE_URL); } + intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); release_firmware(fw); }
Driver obtain power well at intel_csr_ucode_init(). And release it after load DMC firmware successful. An issue happened when DMC was not found or failed to load. Power well would not be released and just output some error messages. Driver have to release power well properly to keep put/get balance. Cc: Jani Nikula <jani.nikula@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Jose Roberto de Souza <jose.souza@intel.com> Cc: Cooper Chiou <cooper.chiou@intel.com> Signed-off-by: Lee, Shawn C <shawn.c.lee@intel.com> --- drivers/gpu/drm/i915/intel_csr.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)