[PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect

[PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect

[Imre Deak]

Suggested by Ville.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index fd86cef..17ced03 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1351,14 +1351,15 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 	struct edid *edid = NULL;
 	bool connected = false;
 
-	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
+	if (force) {
+		intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
 
-	if (force)
 		edid = drm_get_edid(connector,
 				    intel_gmbus_get_adapter(dev_priv,
 				    intel_hdmi->ddc_bus));
 
-	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
+		intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
+	}
 
 	to_intel_connector(connector)->detect_edid = edid;
 	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status

[Imre Deak]

There are platforms that don't need the full GMBUS power domain
(PCH, BXT) while others do (VLV/CHV). For optimizing this we
would need to add a new power domain, but it's not clear how much we
would benefit given the short time we hold the reference. So for now
let's keep things simple.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 17ced03..bdd462e 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1393,6 +1393,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
+
 	while (!live_status && --retry) {
 		live_status = intel_digital_port_connected(dev_priv,
 				hdmi_to_dig_port(intel_hdmi));
@@ -1412,6 +1414,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	} else
 		status = connector_status_disconnected;
 
+	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
+
 	return status;
 }
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote:
> There are platforms that don't need the full GMBUS power domain
> (PCH, BXT) while others do (VLV/CHV). For optimizing this we
> would need to add a new power domain, but it's not clear how much we
> would benefit given the short time we hold the reference. So for now
> let's keep things simple.

Actually on PCH platforms the gmbus domain means just just an
rpm ref since the gmbus hw lives in the PCH. And IIRC on BXT
gmbus lives in pw0 so same deal really.

And for vlv/chv we should just need the disp2d well, which
is exactly what we get with the gmbus domain.

So I don't think there's actually anything to optimize here
with current platforms.

Both patches look fine to me:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 17ced03..bdd462e 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1393,6 +1393,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
>  
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> +
>  	while (!live_status && --retry) {
>  		live_status = intel_digital_port_connected(dev_priv,
>  				hdmi_to_dig_port(intel_hdmi));
> @@ -1412,6 +1414,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  	} else
>  		status = connector_status_disconnected;
>  
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> +
>  	return status;
>  }
>  
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status

[Imre Deak]

On Thu, 2015-11-19 at 21:08 +0200, Ville Syrjälä wrote:
> On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote:
> > There are platforms that don't need the full GMBUS power domain
> > (PCH, BXT) while others do (VLV/CHV). For optimizing this we
> > would need to add a new power domain, but it's not clear how much
> > we
> > would benefit given the short time we hold the reference. So for
> > now
> > let's keep things simple.
> 
> Actually on PCH platforms the gmbus domain means just just an
> rpm ref since the gmbus hw lives in the PCH.

Ah right.

> And IIRC on BXT gmbus lives in pw0 so same deal really.

It's in PW2 there. I'll fix the commit message.

> And for vlv/chv we should just need the disp2d well, which
> is exactly what we get with the gmbus domain.
> 
> So I don't think there's actually anything to optimize here
> with current platforms.
> 
> Both patches look fine to me:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 17ced03..bdd462e 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1393,6 +1393,8 @@ intel_hdmi_detect(struct drm_connector
> > *connector, bool force)
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> >  		      connector->base.id, connector->name);
> >  
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> > +
> >  	while (!live_status && --retry) {
> >  		live_status =
> > intel_digital_port_connected(dev_priv,
> >  				hdmi_to_dig_port(intel_hdmi));
> > @@ -1412,6 +1414,8 @@ intel_hdmi_detect(struct drm_connector
> > *connector, bool force)
> >  	} else
> >  		status = connector_status_disconnected;
> >  
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> > +
> >  	return status;
> >  }
> >  
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 09:13:04PM +0200, Imre Deak wrote:
> On Thu, 2015-11-19 at 21:08 +0200, Ville Syrjälä wrote:
> > On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote:
> > > There are platforms that don't need the full GMBUS power domain
> > > (PCH, BXT) while others do (VLV/CHV). For optimizing this we
> > > would need to add a new power domain, but it's not clear how much
> > > we
> > > would benefit given the short time we hold the reference. So for
> > > now
> > > let's keep things simple.
> > 
> > Actually on PCH platforms the gmbus domain means just just an
> > rpm ref since the gmbus hw lives in the PCH.
> 
> Ah right.
> 
> > And IIRC on BXT gmbus lives in pw0 so same deal really.
> 
> It's in PW2 there. I'll fix the commit message.

Doh. Not sure where I got the PW0 zero idea. Maybe I confused gmbus
with hpd. But yes, you're right about that.

> 
> > And for vlv/chv we should just need the disp2d well, which
> > is exactly what we get with the gmbus domain.
> > 
> > So I don't think there's actually anything to optimize here
> > with current platforms.
> > 
> > Both patches look fine to me:
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 17ced03..bdd462e 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -1393,6 +1393,8 @@ intel_hdmi_detect(struct drm_connector
> > > *connector, bool force)
> > >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > >  		      connector->base.id, connector->name);
> > >  
> > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> > > +
> > >  	while (!live_status && --retry) {
> > >  		live_status =
> > > intel_digital_port_connected(dev_priv,
> > >  				hdmi_to_dig_port(intel_hdmi));
> > > @@ -1412,6 +1414,8 @@ intel_hdmi_detect(struct drm_connector
> > > *connector, bool force)
> > >  	} else
> > >  		status = connector_status_disconnected;
> > >  
> > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> > > +
> > >  	return status;
> > >  }
> > >  
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status

[Imre Deak]

On Thu, 2015-11-19 at 21:17 +0200, Ville Syrjälä wrote:
> On Thu, Nov 19, 2015 at 09:13:04PM +0200, Imre Deak wrote:
> > On Thu, 2015-11-19 at 21:08 +0200, Ville Syrjälä wrote:
> > > On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote:
> > > > There are platforms that don't need the full GMBUS power domain
> > > > (PCH, BXT) while others do (VLV/CHV). For optimizing this we
> > > > would need to add a new power domain, but it's not clear how
> > > > much
> > > > we
> > > > would benefit given the short time we hold the reference. So
> > > > for
> > > > now
> > > > let's keep things simple.
> > > 
> > > Actually on PCH platforms the gmbus domain means just just an
> > > rpm ref since the gmbus hw lives in the PCH.
> > 
> > Ah right.
> > 
> > > And IIRC on BXT gmbus lives in pw0 so same deal really.
> > 
> > It's in PW2 there. I'll fix the commit message.
> 
> Doh. Not sure where I got the PW0 zero idea. Maybe I confused gmbus
> with hpd. But yes, you're right about that.
> 
> > 
> > > And for vlv/chv we should just need the disp2d well, which
> > > is exactly what we get with the gmbus domain.
> > > 
> > > So I don't think there's actually anything to optimize here
> > > with current platforms.
> > > 
> > > Both patches look fine to me:
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed 1/2 and 2/2 with the fixed commit message to dinq. Thanks for
the review.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status

[Imre Deak]

On Fri, 2015-11-20 at 11:54 +0200, Imre Deak wrote:
> On Thu, 2015-11-19 at 21:17 +0200, Ville Syrjälä wrote:
> > On Thu, Nov 19, 2015 at 09:13:04PM +0200, Imre Deak wrote:
> > > On Thu, 2015-11-19 at 21:08 +0200, Ville Syrjälä wrote:
> > > > On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote:
> > > > > There are platforms that don't need the full GMBUS power
> > > > > domain
> > > > > (PCH, BXT) while others do (VLV/CHV). For optimizing this we
> > > > > would need to add a new power domain, but it's not clear how
> > > > > much
> > > > > we
> > > > > would benefit given the short time we hold the reference. So
> > > > > for
> > > > > now
> > > > > let's keep things simple.
> > > > 
> > > > Actually on PCH platforms the gmbus domain means just just an
> > > > rpm ref since the gmbus hw lives in the PCH.
> > > 
> > > Ah right.
> > > 
> > > > And IIRC on BXT gmbus lives in pw0 so same deal really.
> > > 
> > > It's in PW2 there. I'll fix the commit message.
> > 
> > Doh. Not sure where I got the PW0 zero idea. Maybe I confused gmbus
> > with hpd. But yes, you're right about that.
> > 
> > > 
> > > > And for vlv/chv we should just need the disp2d well, which
> > > > is exactly what we get with the gmbus domain.
> > > > 
> > > > So I don't think there's actually anything to optimize here
> > > > with current platforms.
> > > > 
> > > > Both patches look fine to me:
> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Pushed 1/2 and 2/2 with the fixed commit message to dinq. Thanks for
> the review.

I just noticed that 2/2 fixes the following commit which is in v4.4-rc1 
already:

commit 237ed86c693d8a8e4db476976aeb30df4deac74b
Author: Sonika Jindal <sonika.jindal@intel.com>
Date:   Tue Sep 15 09:44:20 2015 +0530

    drm/i915: Check live status before reading edid

so AFAIU this should've gone through -fixes. Jani can you still apply
it there?

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status

[Jani Nikula]

On Fri, 20 Nov 2015, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, 2015-11-20 at 11:54 +0200, Imre Deak wrote:
>> On Thu, 2015-11-19 at 21:17 +0200, Ville Syrjälä wrote:
>> > On Thu, Nov 19, 2015 at 09:13:04PM +0200, Imre Deak wrote:
>> > > On Thu, 2015-11-19 at 21:08 +0200, Ville Syrjälä wrote:
>> > > > On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote:
>> > > > > There are platforms that don't need the full GMBUS power
>> > > > > domain
>> > > > > (PCH, BXT) while others do (VLV/CHV). For optimizing this we
>> > > > > would need to add a new power domain, but it's not clear how
>> > > > > much
>> > > > > we
>> > > > > would benefit given the short time we hold the reference. So
>> > > > > for
>> > > > > now
>> > > > > let's keep things simple.
>> > > > 
>> > > > Actually on PCH platforms the gmbus domain means just just an
>> > > > rpm ref since the gmbus hw lives in the PCH.
>> > > 
>> > > Ah right.
>> > > 
>> > > > And IIRC on BXT gmbus lives in pw0 so same deal really.
>> > > 
>> > > It's in PW2 there. I'll fix the commit message.
>> > 
>> > Doh. Not sure where I got the PW0 zero idea. Maybe I confused gmbus
>> > with hpd. But yes, you're right about that.
>> > 
>> > > 
>> > > > And for vlv/chv we should just need the disp2d well, which
>> > > > is exactly what we get with the gmbus domain.
>> > > > 
>> > > > So I don't think there's actually anything to optimize here
>> > > > with current platforms.
>> > > > 
>> > > > Both patches look fine to me:
>> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> Pushed 1/2 and 2/2 with the fixed commit message to dinq. Thanks for
>> the review.
>
> I just noticed that 2/2 fixes the following commit which is in v4.4-rc1 
> already:
>
> commit 237ed86c693d8a8e4db476976aeb30df4deac74b
> Author: Sonika Jindal <sonika.jindal@intel.com>
> Date:   Tue Sep 15 09:44:20 2015 +0530
>
>     drm/i915: Check live status before reading edid
>
> so AFAIU this should've gone through -fixes. Jani can you still apply
> it there?

Otherwise no problem but this has a dependency on

commit f0ab43e6c338896cadee64ced3fc30a5343890d9
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Mon Nov 9 16:48:19 2015 +0100

    drm/i915: Introduce a gmbus power domain

and didn't bother figuring out how deep the rabbit hole goes.

If you want this in -fixes, please either send a -fixes specific patch,
or see how many commits would need to be backported.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status

[Imre Deak]

On pe, 2015-11-20 at 13:13 +0200, Jani Nikula wrote:
> On Fri, 20 Nov 2015, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, 2015-11-20 at 11:54 +0200, Imre Deak wrote:
> > > On Thu, 2015-11-19 at 21:17 +0200, Ville Syrjälä wrote:
> > > > On Thu, Nov 19, 2015 at 09:13:04PM +0200, Imre Deak wrote:
> > > > > On Thu, 2015-11-19 at 21:08 +0200, Ville Syrjälä wrote:
> > > > > > On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote:
> > > > > > > There are platforms that don't need the full GMBUS power
> > > > > > > domain
> > > > > > > (PCH, BXT) while others do (VLV/CHV). For optimizing this
> > > > > > > we
> > > > > > > would need to add a new power domain, but it's not clear
> > > > > > > how
> > > > > > > much
> > > > > > > we
> > > > > > > would benefit given the short time we hold the reference.
> > > > > > > So
> > > > > > > for
> > > > > > > now
> > > > > > > let's keep things simple.
> > > > > > 
> > > > > > Actually on PCH platforms the gmbus domain means just just
> > > > > > an
> > > > > > rpm ref since the gmbus hw lives in the PCH.
> > > > > 
> > > > > Ah right.
> > > > > 
> > > > > > And IIRC on BXT gmbus lives in pw0 so same deal really.
> > > > > 
> > > > > It's in PW2 there. I'll fix the commit message.
> > > > 
> > > > Doh. Not sure where I got the PW0 zero idea. Maybe I confused
> > > > gmbus
> > > > with hpd. But yes, you're right about that.
> > > > 
> > > > > 
> > > > > > And for vlv/chv we should just need the disp2d well, which
> > > > > > is exactly what we get with the gmbus domain.
> > > > > > 
> > > > > > So I don't think there's actually anything to optimize here
> > > > > > with current platforms.
> > > > > > 
> > > > > > Both patches look fine to me:
> > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Pushed 1/2 and 2/2 with the fixed commit message to dinq. Thanks
> > > for
> > > the review.
> > 
> > I just noticed that 2/2 fixes the following commit which is in
> > v4.4-rc1 
> > already:
> > 
> > commit 237ed86c693d8a8e4db476976aeb30df4deac74b
> > Author: Sonika Jindal <sonika.jindal@intel.com>
> > Date:   Tue Sep 15 09:44:20 2015 +0530
> > 
> >     drm/i915: Check live status before reading edid
> > 
> > so AFAIU this should've gone through -fixes. Jani can you still
> > apply
> > it there?
> 
> Otherwise no problem but this has a dependency on
> 
> commit f0ab43e6c338896cadee64ced3fc30a5343890d9
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Mon Nov 9 16:48:19 2015 +0100
> 
>     drm/i915: Introduce a gmbus power domain
> 
> and didn't bother figuring out how deep the rabbit hole goes.
> 
> If you want this in -fixes, please either send a -fixes specific
> patch,
> or see how many commits would need to be backported.

Ok. I backported the 5 patches we'd need. The only issue with that
AFAICS is to resolve any conflict between drm-intel-fixes and dinq, but
that looks manageable. I did have to resolve one conflict in one of the
patches while backporting, so I pushed them to
https://github.com/ideak/linux/commits/gmbus-fix-backport

I can also send the rebased versions to the list if needed.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect

[Chris Wilson]

On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote:
> Suggested by Ville.

Do you mind explaining why this is done at the hdmi level and not the
gmbus level?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect

[Imre Deak]

On Thu, 2015-11-19 at 20:51 +0000, Chris Wilson wrote:
> On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote:
> > Suggested by Ville.
> 
> Do you mind explaining why this is done at the hdmi level and not the
> gmbus level?

To reduce the on/off toggling, since we don't have delayed power-off
implemented for power wells. gmbus_xfer also takes a ref to account for
accesses from the i2c device node. The solution would be to implement
delayed power-off I guess.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect

[Chris Wilson]

On Thu, Nov 19, 2015 at 11:01:49PM +0200, Imre Deak wrote:
> On Thu, 2015-11-19 at 20:51 +0000, Chris Wilson wrote:
> > On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote:
> > > Suggested by Ville.
> > 
> > Do you mind explaining why this is done at the hdmi level and not the
> > gmbus level?
> 
> To reduce the on/off toggling, since we don't have delayed power-off
> implemented for power wells. gmbus_xfer also takes a ref to account for
> accesses from the i2c device node. The solution would be to implement
> delayed power-off I guess.

As we chase ever finer grained wakelocks, yeah. Looking at the other
users of gmbus, they are the old platforms (dvo, sdvo, crt, lvds) so not
worth generalising the optimisation of holding the wakelock across the
entire i2c operation, I guess?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect

[Imre Deak]

On Thu, 2015-11-19 at 21:38 +0000, Chris Wilson wrote:
> On Thu, Nov 19, 2015 at 11:01:49PM +0200, Imre Deak wrote:
> > On Thu, 2015-11-19 at 20:51 +0000, Chris Wilson wrote:
> > > On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote:
> > > > Suggested by Ville.
> > > 
> > > Do you mind explaining why this is done at the hdmi level and not the
> > > gmbus level?
> > 
> > To reduce the on/off toggling, since we don't have delayed power-off
> > implemented for power wells. gmbus_xfer also takes a ref to account for
> > accesses from the i2c device node. The solution would be to implement
> > delayed power-off I guess.
> 
> As we chase ever finer grained wakelocks, yeah.

Ok, the delayed-off stuff shouldn't be difficult, since in case of
power wells we hold an RPM ref. So AFAICS we would only need to
synchronize during system suspend and driver unload. And then find a
good timeout value..

> Looking at the other users of gmbus, they are the old platforms (dvo,
> sdvo, crt, lvds) so not worth generalising the optimisation of
> holding the wakelock across the entire i2c operation, I guess?

If you mean to take an extra ref around the higher level op in those
places too: well in case of CRT it's also in new HW where it matters,
so that's inconsistent and I think we should do it there too. For
others it doesn't matter I think.

--Imre

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect

[Imre Deak]

On Thu, 2015-11-19 at 23:50 +0200, Imre Deak wrote:
> On Thu, 2015-11-19 at 21:38 +0000, Chris Wilson wrote:
> > On Thu, Nov 19, 2015 at 11:01:49PM +0200, Imre Deak wrote:
> > > On Thu, 2015-11-19 at 20:51 +0000, Chris Wilson wrote:
> > > > On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote:
> > > > > Suggested by Ville.
> > > > 
> > > > Do you mind explaining why this is done at the hdmi level and
> > > > not the
> > > > gmbus level?
> > > 
> > > To reduce the on/off toggling, since we don't have delayed power-
> > > off
> > > implemented for power wells. gmbus_xfer also takes a ref to
> > > account for
> > > accesses from the i2c device node. The solution would be to
> > > implement
> > > delayed power-off I guess.
> > 
> > As we chase ever finer grained wakelocks, yeah.
> 
> Ok, the delayed-off stuff shouldn't be difficult, since in case of
> power wells we hold an RPM ref. So AFAICS we would only need to
> synchronize during system suspend and driver unload. And then find a
> good timeout value..
> 
> > Looking at the other users of gmbus, they are the old platforms
> > (dvo,
> > sdvo, crt, lvds) so not worth generalising the optimisation of
> > holding the wakelock across the entire i2c operation, I guess?
> 
> If you mean to take an extra ref around the higher level op in those
> places too: well in case of CRT it's also in new HW where it matters,
> so that's inconsistent and I think we should do it there too. For
> others it doesn't matter I think.

Err, we do take a ref in the CRT detect code, but it's the port power
domain.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx