| Message ID | 20200601101516.21018-1-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/i915: lpsp with hdmi/dp outputs | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > Anshuman Gupta > Sent: Monday, June 1, 2020 3:45 PM > To: intel-gfx@lists.freedesktop.org > Cc: stable@vger.kernel.org > Subject: [Intel-gfx] [RFC] drm/i915: lpsp with hdmi/dp outputs > > Gen12 hw are failing to enable lpsp configuration due to PG3 was left on due to > valid usgae count of POWER_DOMAIN_AUDIO. > It is not required to get POWER_DOMAIN_AUDIO ref-count when enabling a crtc, > it should be always i915_audio_component request to get/put > AUDIO_POWER_DOMAIN. > > Cc: stable@vger.kernel.org > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 6c3b11de2daf..f31a579d7a52 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -7356,7 +7356,11 @@ static u64 get_crtc_power_domains(struct > intel_crtc_state *crtc_state) > mask |= BIT_ULL(intel_encoder->power_domain); > } > > - if (HAS_DDI(dev_priv) && crtc_state->has_audio) > + /* > + * Gen12 can drive lpsp on hdmi/dp outpus, it doesn't require to > + * enable AUDIO power in order to enable a crtc. > + */ > + if (INTEL_GEN(dev_priv) < 12 && HAS_DDI(dev_priv) && > +crtc_state->has_audio) > mask |= BIT_ULL(POWER_DOMAIN_AUDIO); As part of ddi_get_config we determine has_audio using power well enabled: pipe_config->has_audio = intel_ddi_is_audio_enabled(dev_priv, cpu_transcoder); If audio power domain is not enabled, we may end up with this as false. Later this may get checked in intel_enable_ddi_hdmi to call audio codec enable if (crtc_state->has_audio) intel_audio_codec_enable(encoder, crtc_state, conn_state); This may cause detection to fail. Please verify this usecase once and confirm. Regards, Uma Shankar > if (crtc_state->shared_dpll) > -- > 2.26.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 01, 2020 at 03:45:16PM +0530, Anshuman Gupta wrote: > Gen12 hw are failing to enable lpsp configuration due to PG3 was left on > due to valid usgae count of POWER_DOMAIN_AUDIO. > It is not required to get POWER_DOMAIN_AUDIO ref-count when enabling > a crtc, it should be always i915_audio_component request to get/put > AUDIO_POWER_DOMAIN. > > Cc: stable@vger.kernel.org > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 6c3b11de2daf..f31a579d7a52 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -7356,7 +7356,11 @@ static u64 get_crtc_power_domains(struct intel_crtc_state *crtc_state) > mask |= BIT_ULL(intel_encoder->power_domain); > } > > - if (HAS_DDI(dev_priv) && crtc_state->has_audio) > + /* > + * Gen12 can drive lpsp on hdmi/dp outpus, it doesn't require to > + * enable AUDIO power in order to enable a crtc Nothing requires audio power to enable a crtc. What this is saying is that if we want audio then we must enable the audio power. If you didn't want audio then you wouldn't have .has_audio set. That said, looks like audio is moving into the always on well, but not yet in tgl. . > + */ > + if (INTEL_GEN(dev_priv) < 12 && HAS_DDI(dev_priv) && crtc_state->has_audio) > mask |= BIT_ULL(POWER_DOMAIN_AUDIO); > > if (crtc_state->shared_dpll) > -- > 2.26.2
On 2020-06-01 at 18:19:44 +0530, Shankar, Uma wrote: > > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > > Anshuman Gupta > > Sent: Monday, June 1, 2020 3:45 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: stable@vger.kernel.org > > Subject: [Intel-gfx] [RFC] drm/i915: lpsp with hdmi/dp outputs > > > > Gen12 hw are failing to enable lpsp configuration due to PG3 was left on due to > > valid usgae count of POWER_DOMAIN_AUDIO. > > It is not required to get POWER_DOMAIN_AUDIO ref-count when enabling a crtc, > > it should be always i915_audio_component request to get/put > > AUDIO_POWER_DOMAIN. > > > > Cc: stable@vger.kernel.org > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index 6c3b11de2daf..f31a579d7a52 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -7356,7 +7356,11 @@ static u64 get_crtc_power_domains(struct > > intel_crtc_state *crtc_state) > > mask |= BIT_ULL(intel_encoder->power_domain); > > } > > > > - if (HAS_DDI(dev_priv) && crtc_state->has_audio) > > + /* > > + * Gen12 can drive lpsp on hdmi/dp outpus, it doesn't require to > > + * enable AUDIO power in order to enable a crtc. > > + */ > > + if (INTEL_GEN(dev_priv) < 12 && HAS_DDI(dev_priv) && > > +crtc_state->has_audio) > > mask |= BIT_ULL(POWER_DOMAIN_AUDIO); > > As part of ddi_get_config we determine has_audio using power well enabled: > pipe_config->has_audio = > intel_ddi_is_audio_enabled(dev_priv, cpu_transcoder); IMO AUDIO power will also be requested by i915_audio_component get request, we can always use HDMI display without audio playback, AUDIO power should be enabled when audio driver request for it. if we get AUDIO_POWER_DOMAIN while enabling crtc PG3 will always kept on till CRTC is disabled, that is the issue required to be addressed here. This is just RFC to initiate a discussion around it. Thanks, Anshuman Gupta. > > If audio power domain is not enabled, we may end up with this as false. > Later this may get checked in intel_enable_ddi_hdmi to call audio codec enable > > if (crtc_state->has_audio) > intel_audio_codec_enable(encoder, crtc_state, conn_state); > > This may cause detection to fail. Please verify this usecase once and confirm. > > Regards, > Uma Shankar > > > if (crtc_state->shared_dpll) > > -- > > 2.26.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 2020-06-01 at 17:11:32 +0300, Ville Syrjälä wrote: > On Mon, Jun 01, 2020 at 03:45:16PM +0530, Anshuman Gupta wrote: > > Gen12 hw are failing to enable lpsp configuration due to PG3 was left on > > due to valid usgae count of POWER_DOMAIN_AUDIO. > > It is not required to get POWER_DOMAIN_AUDIO ref-count when enabling > > a crtc, it should be always i915_audio_component request to get/put > > AUDIO_POWER_DOMAIN. > > > > Cc: stable@vger.kernel.org > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 6c3b11de2daf..f31a579d7a52 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -7356,7 +7356,11 @@ static u64 get_crtc_power_domains(struct intel_crtc_state *crtc_state) > > mask |= BIT_ULL(intel_encoder->power_domain); > > } > > > > - if (HAS_DDI(dev_priv) && crtc_state->has_audio) > > + /* > > + * Gen12 can drive lpsp on hdmi/dp outpus, it doesn't require to > > + * enable AUDIO power in order to enable a crtc > > Nothing requires audio power to enable a crtc. What this is saying is > that if we want audio then we must enable the audio power. If you > didn't want audio then you wouldn't have .has_audio set. IMO i915_audio_component_get_power also enables audio power, and i915_audio_component_put_power releases the usage count based upon audio runtime idleness but here get_crtc_power_domains() gets the POWER_DOMAIN_AUDIO usages count, which will be released only when this crtc get disbaled. It may enable AUDIO power despite of fact that audio driver has released the usage count. Please correct me if i am wrong here. > > That said, looks like audio is moving into the always on well, but not > yet in tgl. Still some of audio functional stuff lies in PG3, not completely removed from PG3. Thanks, Anshuman Gupta. > > . > > + */ > > + if (INTEL_GEN(dev_priv) < 12 && HAS_DDI(dev_priv) && crtc_state->has_audio) > > mask |= BIT_ULL(POWER_DOMAIN_AUDIO); > > > > if (crtc_state->shared_dpll) > > -- > > 2.26.2 > > -- > Ville Syrjälä > Intel
> -----Original Message----- > From: Gupta, Anshuman <anshuman.gupta@intel.com> > Sent: Tuesday, June 2, 2020 5:37 PM > To: Shankar, Uma <uma.shankar@intel.com> > Cc: intel-gfx@lists.freedesktop.org; stable@vger.kernel.org > Subject: Re: [Intel-gfx] [RFC] drm/i915: lpsp with hdmi/dp outputs > > On 2020-06-01 at 18:19:44 +0530, Shankar, Uma wrote: > > > > > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf > > > Of Anshuman Gupta > > > Sent: Monday, June 1, 2020 3:45 PM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: stable@vger.kernel.org > > > Subject: [Intel-gfx] [RFC] drm/i915: lpsp with hdmi/dp outputs > > > > > > Gen12 hw are failing to enable lpsp configuration due to PG3 was > > > left on due to valid usgae count of POWER_DOMAIN_AUDIO. > > > It is not required to get POWER_DOMAIN_AUDIO ref-count when enabling > > > a crtc, it should be always i915_audio_component request to get/put > > > AUDIO_POWER_DOMAIN. > > > > > > Cc: stable@vger.kernel.org > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 6c3b11de2daf..f31a579d7a52 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -7356,7 +7356,11 @@ static u64 get_crtc_power_domains(struct > > > intel_crtc_state *crtc_state) > > > mask |= BIT_ULL(intel_encoder->power_domain); > > > } > > > > > > - if (HAS_DDI(dev_priv) && crtc_state->has_audio) > > > + /* > > > + * Gen12 can drive lpsp on hdmi/dp outpus, it doesn't require to > > > + * enable AUDIO power in order to enable a crtc. > > > + */ > > > + if (INTEL_GEN(dev_priv) < 12 && HAS_DDI(dev_priv) && > > > +crtc_state->has_audio) > > > mask |= BIT_ULL(POWER_DOMAIN_AUDIO); > > > > As part of ddi_get_config we determine has_audio using power well enabled: > > pipe_config->has_audio = > > intel_ddi_is_audio_enabled(dev_priv, cpu_transcoder); > IMO AUDIO power will also be requested by i915_audio_component get request, > we can always use HDMI display without audio playback, AUDIO power should > be enabled when audio driver request for it. > if we get AUDIO_POWER_DOMAIN while enabling crtc PG3 will always kept on till > CRTC is disabled, that is the issue required to be addressed here. Yes HDMI can be enabled without audio, but if we want audio we will need to notify audio driver through HSW_AUD_PIN_ELD_CP_VLD and also prepare and write ELD data in hardware register. If I understand correctly this will need power and by this time audio driver would not have requested for it. Hence this will fail audio detection. > This is just RFC to initiate a discussion around it. > Thanks, > Anshuman Gupta. > > > > If audio power domain is not enabled, we may end up with this as false. > > Later this may get checked in intel_enable_ddi_hdmi to call audio > > codec enable > > > > if (crtc_state->has_audio) > > intel_audio_codec_enable(encoder, crtc_state, > > conn_state); > > > > This may cause detection to fail. Please verify this usecase once and confirm. > > > > Regards, > > Uma Shankar > > > > > if (crtc_state->shared_dpll) > > > -- > > > 2.26.2 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jun 02, 2020 at 05:58:07PM +0530, Anshuman Gupta wrote: > On 2020-06-01 at 17:11:32 +0300, Ville Syrjälä wrote: > > On Mon, Jun 01, 2020 at 03:45:16PM +0530, Anshuman Gupta wrote: > > > Gen12 hw are failing to enable lpsp configuration due to PG3 was left on > > > due to valid usgae count of POWER_DOMAIN_AUDIO. > > > It is not required to get POWER_DOMAIN_AUDIO ref-count when enabling > > > a crtc, it should be always i915_audio_component request to get/put > > > AUDIO_POWER_DOMAIN. > > > > > > Cc: stable@vger.kernel.org > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index 6c3b11de2daf..f31a579d7a52 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -7356,7 +7356,11 @@ static u64 get_crtc_power_domains(struct intel_crtc_state *crtc_state) > > > mask |= BIT_ULL(intel_encoder->power_domain); > > > } > > > > > > - if (HAS_DDI(dev_priv) && crtc_state->has_audio) > > > + /* > > > + * Gen12 can drive lpsp on hdmi/dp outpus, it doesn't require to > > > + * enable AUDIO power in order to enable a crtc > > > > Nothing requires audio power to enable a crtc. What this is saying is > > that if we want audio then we must enable the audio power. If you > > didn't want audio then you wouldn't have .has_audio set. > IMO i915_audio_component_get_power also enables audio power, and > i915_audio_component_put_power releases the usage count based upon audio > runtime idleness but here get_crtc_power_domains() gets the POWER_DOMAIN_AUDIO usages > count, which will be released only when this crtc get disbaled. > It may enable AUDIO power despite of fact that audio driver has released the > usage count. > Please correct me if i am wrong here. The audio component stuff doesn't actually do the audio enable/disable sequence. > > > > > That said, looks like audio is moving into the always on well, but not > > yet in tgl. > Still some of audio functional stuff lies in PG3, not completely removed > from PG3. > Thanks, > Anshuman Gupta. > > > > . > > > + */ > > > + if (INTEL_GEN(dev_priv) < 12 && HAS_DDI(dev_priv) && crtc_state->has_audio) > > > mask |= BIT_ULL(POWER_DOMAIN_AUDIO); > > > > > > if (crtc_state->shared_dpll) > > > -- > > > 2.26.2 > > > > -- > > Ville Syrjälä > > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 6c3b11de2daf..f31a579d7a52 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -7356,7 +7356,11 @@ static u64 get_crtc_power_domains(struct intel_crtc_state *crtc_state) mask |= BIT_ULL(intel_encoder->power_domain); } - if (HAS_DDI(dev_priv) && crtc_state->has_audio) + /* + * Gen12 can drive lpsp on hdmi/dp outpus, it doesn't require to + * enable AUDIO power in order to enable a crtc. + */ + if (INTEL_GEN(dev_priv) < 12 && HAS_DDI(dev_priv) && crtc_state->has_audio) mask |= BIT_ULL(POWER_DOMAIN_AUDIO); if (crtc_state->shared_dpll)
Gen12 hw are failing to enable lpsp configuration due to PG3 was left on due to valid usgae count of POWER_DOMAIN_AUDIO. It is not required to get POWER_DOMAIN_AUDIO ref-count when enabling a crtc, it should be always i915_audio_component request to get/put AUDIO_POWER_DOMAIN. Cc: stable@vger.kernel.org Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)