[PATCH 0/4] drm/i915/intel_dsi: Read back and use pclk set by the GOP

[PATCH 0/4] drm/i915/intel_dsi: Read back and use pclk set by the GOP

[Hans de Goede]

Hi All,

This patch-set is the result of the work I've been doing recently to
give people a smooth "flickerfree" boot experience where the display
keeps displaying the logo put there by the firmware until it smoothly
fades into the Linux GUI (e.g. gdm).

While testing this on some BYT/CHT devices I noticed the screen going
black for 1 second or so, because the first modeset was not a fast
modeset despite passing i915.fastboot=1 on the kernel commandline.

This is caused by the GOP and the i915 code differing in what they
think the DSI pclk should be. This patch-set fixes this by reading
back the pclk set by the GOP and if it is reasonably close to the
clk calculated by the i915 code, using the GOP set pclk.

Regards,

Hans

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

[PATCH 1/4] drm/i915/intel_dsi: Allow calling intel_dsi_get_pclk with a NULL config

[Hans de Goede]

Allow calling intel_dsi_get_pclk without passing in an intel_crtc_state.

This is a preparation patch for reading back the GOP configured DSI
clk during probe.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi_pll.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
index 2ff2ee7f3b78..71ce6e3157d4 100644
--- a/drivers/gpu/drm/i915/intel_dsi_pll.c
+++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
@@ -279,8 +279,10 @@ static u32 vlv_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp,
 	pll_div = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_DIVIDER);
 	mutex_unlock(&dev_priv->sb_lock);
 
-	config->dsi_pll.ctrl = pll_ctl & ~DSI_PLL_LOCK;
-	config->dsi_pll.div = pll_div;
+	if (config) {
+		config->dsi_pll.ctrl = pll_ctl & ~DSI_PLL_LOCK;
+		config->dsi_pll.div = pll_div;
+	}
 
 	/* mask out other bits and extract the P1 divisor */
 	pll_ctl &= DSI_PLL_P1_POST_DIV_MASK;
@@ -330,6 +332,7 @@ static u32 vlv_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp,
 static u32 bxt_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp,
 			    struct intel_crtc_state *config)
 {
+	u32 ctrl;
 	u32 pclk;
 	u32 dsi_clk;
 	u32 dsi_ratio;
@@ -342,9 +345,12 @@ static u32 bxt_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp,
 		return 0;
 	}
 
-	config->dsi_pll.ctrl = I915_READ(BXT_DSI_PLL_CTL);
+	ctrl = I915_READ(BXT_DSI_PLL_CTL);
+
+	if (config)
+		config->dsi_pll.ctrl = ctrl;
 
-	dsi_ratio = config->dsi_pll.ctrl & BXT_DSI_PLL_RATIO_MASK;
+	dsi_ratio = ctrl & BXT_DSI_PLL_RATIO_MASK;
 
 	dsi_clk = (dsi_ratio * BXT_REF_CLOCK_KHZ) / 2;
 
-- 
2.17.1

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

[PATCH 2/4] drm/i915/intel_dsi: Move initialization of encoder variables up a bit

[Hans de Goede]

Move the initialization of encoder variables a bit higher inside the
intel_dsi_init() function. So that the encoder can safely be passed to
intel_connector_get_hw_state() inside intel_dsi_vbt_init().

This is a preparation patch for reading back the GOP configured pclk
from intel_dsi_vbt_init().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 3b7acb5a70b3..103fe460f78c 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1769,6 +1769,9 @@ void intel_dsi_init(struct drm_i915_private *dev_priv)
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
 
 	intel_encoder->port = port;
+	intel_encoder->type = INTEL_OUTPUT_DSI;
+	intel_encoder->power_domain = POWER_DOMAIN_PORT_DSI;
+	intel_encoder->cloneable = 0;
 
 	/*
 	 * On BYT/CHV, pipe A maps to MIPI DSI port A, pipe B maps to MIPI DSI
@@ -1820,9 +1823,6 @@ void intel_dsi_init(struct drm_i915_private *dev_priv)
 		}
 	}
 
-	intel_encoder->type = INTEL_OUTPUT_DSI;
-	intel_encoder->power_domain = POWER_DOMAIN_PORT_DSI;
-	intel_encoder->cloneable = 0;
 	drm_connector_init(dev, connector, &intel_dsi_connector_funcs,
 			   DRM_MODE_CONNECTOR_DSI);
 
-- 
2.17.1

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

[PATCH 3/4] drm/i915/intel_dsi: Make intel_connector_get_hw_state() non static

[Hans de Goede]

Make intel_connector_get_hw_state() non static so that it can be called
from the intel_dsi_vbt.c code.

This is a preparation patch for reading back the GOP configured pclk
from intel_dsi_vbt_init().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 3 +--
 drivers/gpu/drm/i915/intel_dsi.h | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 103fe460f78c..5e2bb13f7b6d 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1005,8 +1005,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	intel_dsi_msleep(intel_dsi, intel_dsi->panel_pwr_cycle_delay);
 }
 
-static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
-				   enum pipe *pipe)
+bool intel_dsi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 7afeb9580f41..34ce3ef395ef 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -132,6 +132,7 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
 /* intel_dsi.c */
 void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
 enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt);
+bool intel_dsi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
 
 /* intel_dsi_pll.c */
 bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
-- 
2.17.1

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

[PATCH 4/4] drm/i915/intel_dsi: Read back pclk set by GOP and use that as pclk

[Hans de Goede]

On BYT and CHT the GOP sometimes initializes the pclk at a (slightly)
different frequency then the pclk which we've calculated.

This commit makes the DSI code read-back the pclk set by the GOP and
if that is within a reasonable margin of the calculated pclk, uses
that instead.

This fixes the first modeset being a full modeset instead of a
fast modeset on systems where the GOP pclk is different.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
index 4d6ffa7b3e7b..d4cc6099012c 100644
--- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
@@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
 	u32 mul;
 	u16 burst_mode_ratio;
 	enum port port;
+	enum pipe pipe;
 
 	DRM_DEBUG_KMS("\n");
 
@@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
 	} else
 		burst_mode_ratio = 100;
 
+	/*
+	 * On BYT / CRC the GOP sometimes picks a slightly different pclk,
+	 * read back the GOP configured pclk and prefer it over ours.
+	 */
+	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
+	    intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) {
+		u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL);
+
+		DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop);
+		if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10))
+			pclk = gop;
+	}
+
 	intel_dsi->burst_mode_ratio = burst_mode_ratio;
 	intel_dsi->pclk = pclk;
 
-- 
2.17.1

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

Re: [PATCH 4/4] drm/i915/intel_dsi: Read back pclk set by GOP and use that as pclk

[Ville Syrjälä]

On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote:
> On BYT and CHT the GOP sometimes initializes the pclk at a (slightly)
> different frequency then the pclk which we've calculated.
> 
> This commit makes the DSI code read-back the pclk set by the GOP and
> if that is within a reasonable margin of the calculated pclk, uses
> that instead.
> 
> This fixes the first modeset being a full modeset instead of a
> fast modeset on systems where the GOP pclk is different.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
> index 4d6ffa7b3e7b..d4cc6099012c 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
> @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>  	u32 mul;
>  	u16 burst_mode_ratio;
>  	enum port port;
> +	enum pipe pipe;
>  
>  	DRM_DEBUG_KMS("\n");
>  
> @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>  	} else
>  		burst_mode_ratio = 100;
>  
> +	/*
> +	 * On BYT / CRC the GOP sometimes picks a slightly different pclk,
> +	 * read back the GOP configured pclk and prefer it over ours.
> +	 */
> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> +	    intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) {
> +		u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL);
> +
> +		DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop);
> +		if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10))
> +			pclk = gop;
> +	}

Is the gop acually picking a different clock that what we pick in the
end, or is it just that the value in the vbt doesn't quite match what we
(and the gop) end up using on account of limitations of the pll?

For that particular problem I think I had patches long ago to go through
the pll computation during init so that we basically fix up the slightly
bogus clock from the vbt.

Any kind of hack that involves reading out the hardware state should go
into something like intel_sanitize_encoder(). Actually by that time we
have already read out the hw state, so it shouldn't require any
modifications to the existing dsi code itself.

> +
>  	intel_dsi->burst_mode_ratio = burst_mode_ratio;
>  	intel_dsi->pclk = pclk;
>  
> -- 
> 2.17.1

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/i915/intel_dsi: Read back pclk set by GOP and use that as pclk

[Hans de Goede]

Hi,

On 07/06/2018 04:16 PM, Ville Syrjälä wrote:
> On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote:
>> On BYT and CHT the GOP sometimes initializes the pclk at a (slightly)
>> different frequency then the pclk which we've calculated.
>>
>> This commit makes the DSI code read-back the pclk set by the GOP and
>> if that is within a reasonable margin of the calculated pclk, uses
>> that instead.
>>
>> This fixes the first modeset being a full modeset instead of a
>> fast modeset on systems where the GOP pclk is different.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> index 4d6ffa7b3e7b..d4cc6099012c 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>> @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>   	u32 mul;
>>   	u16 burst_mode_ratio;
>>   	enum port port;
>> +	enum pipe pipe;
>>   
>>   	DRM_DEBUG_KMS("\n");
>>   
>> @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>   	} else
>>   		burst_mode_ratio = 100;
>>   
>> +	/*
>> +	 * On BYT / CRC the GOP sometimes picks a slightly different pclk,
>> +	 * read back the GOP configured pclk and prefer it over ours.
>> +	 */
>> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>> +	    intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) {
>> +		u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL);
>> +
>> +		DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop);
>> +		if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10))
>> +			pclk = gop;
>> +	}
> 
> Is the gop acually picking a different clock that what we pick in the
> end, or is it just that the value in the vbt doesn't quite match what we
> (and the gop) end up using on account of limitations of the pll?

I *think* the GOP is picking a different clock, IIRC (*) it is something like
90Mhz for the GOP and the VBT says 87Mhz (and our calculations leave it
unmodified. With this patch which puts pclk at 90Mhz on the specific
tablet I developed this on, the PLL settings calculated by our PLL code
end up being exactly the same as the once chosen by the GOP once we have
the pclk set to 90MHz.

Note I've seen these small (and sometimes somewhat bigger) differences
between GOP and VBT pclk on a lot of devices, not just the one tablet
I developed it on. Since submitting this I've run this on at least
5 different CHT/BYT devices and it works as advertised so far.

> For that particular problem I think I had patches long ago to go through
> the pll computation during init so that we basically fix up the slightly
> bogus clock from the vbt.

We do end up with a slightly different clock then the 87MHhz when going
though the PLL calculations, something like 86.33MHz or some such from
the top of my head, but the problem is not the pclk not matching the
intel_pipe_config_compare() function does not look at it, it looks at
dsi_pll.ctrl dsi_pll.div and those don't match, where as they do match
if we fixup the VBT clock to be the one confgured by the GOP.

> Any kind of hack that involves reading out the hardware state should go
> into something like intel_sanitize_encoder(). Actually by that time we
> have already read out the hw state, so it shouldn't require any
> modifications to the existing dsi code itself.

I do not think that intel_sanitize encoder is the right place to do this:

* I don't want to modify the read-back state, I want to modify our
   calculated "new/ideal" state to match the read-back state
* I don't want to directly modify our calculated new/ideal state,
   instead I want to cleanup / sanitize the values we got from the VBT
   so that the initial-modeset *and* any future modesets will use the
   GOP pclk. I believe it is important that if we're going to use the
   GOP pclk we use it for all modesets consistently.
* I only want to do this once, at boot when we are sure the mode was
   set by the GOP and not after suspend/resume when we don't know if the
   GOP will have touched things or not.
* It is DSI specific, whereas sofar intel_sanitize_encoder seems to
   not contain any output specific code.

Regards,

Hans



> 
>> +
>>   	intel_dsi->burst_mode_ratio = burst_mode_ratio;
>>   	intel_dsi->pclk = pclk;
>>   
>> -- 
>> 2.17.1
> 

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

Re: [PATCH 4/4] drm/i915/intel_dsi: Read back pclk set by GOP and use that as pclk

[Rodrigo Vivi]

On Sat, Jul 07, 2018 at 08:32:16AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 07/06/2018 04:16 PM, Ville Syrjälä wrote:
> > On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote:
> > > On BYT and CHT the GOP sometimes initializes the pclk at a (slightly)
> > > different frequency then the pclk which we've calculated.
> > > 
> > > This commit makes the DSI code read-back the pclk set by the GOP and
> > > if that is within a reasonable margin of the calculated pclk, uses
> > > that instead.
> > > 
> > > This fixes the first modeset being a full modeset instead of a
> > > fast modeset on systems where the GOP pclk is different.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++
> > >   1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
> > > index 4d6ffa7b3e7b..d4cc6099012c 100644
> > > --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
> > > +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
> > > @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
> > >   	u32 mul;
> > >   	u16 burst_mode_ratio;
> > >   	enum port port;
> > > +	enum pipe pipe;
> > >   	DRM_DEBUG_KMS("\n");
> > > @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
> > >   	} else
> > >   		burst_mode_ratio = 100;
> > > +	/*
> > > +	 * On BYT / CRC the GOP sometimes picks a slightly different pclk,
> > > +	 * read back the GOP configured pclk and prefer it over ours.
> > > +	 */
> > > +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> > > +	    intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) {
> > > +		u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL);
> > > +
> > > +		DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop);
> > > +		if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10))
> > > +			pclk = gop;
> > > +	}
> > 
> > Is the gop acually picking a different clock that what we pick in the
> > end, or is it just that the value in the vbt doesn't quite match what we
> > (and the gop) end up using on account of limitations of the pll?
> 
> I *think* the GOP is picking a different clock, IIRC (*) it is something like
> 90Mhz for the GOP and the VBT says 87Mhz (and our calculations leave it
> unmodified. With this patch which puts pclk at 90Mhz on the specific
> tablet I developed this on, the PLL settings calculated by our PLL code
> end up being exactly the same as the once chosen by the GOP once we have
> the pclk set to 90MHz.
> 
> Note I've seen these small (and sometimes somewhat bigger) differences
> between GOP and VBT pclk on a lot of devices, not just the one tablet
> I developed it on. Since submitting this I've run this on at least
> 5 different CHT/BYT devices and it works as advertised so far.

It seems GOP is just not respecting VBT and using the whatever it judge
the safest option?!

Probably not optimal, but I believe we should stay on the safest side as well,
if this is the case.

> 
> > For that particular problem I think I had patches long ago to go through
> > the pll computation during init so that we basically fix up the slightly
> > bogus clock from the vbt.
> 
> We do end up with a slightly different clock then the 87MHhz when going
> though the PLL calculations, something like 86.33MHz or some such from
> the top of my head, but the problem is not the pclk not matching the
> intel_pipe_config_compare() function does not look at it, it looks at
> dsi_pll.ctrl dsi_pll.div and those don't match, where as they do match
> if we fixup the VBT clock to be the one confgured by the GOP.
> 
> > Any kind of hack that involves reading out the hardware state should go
> > into something like intel_sanitize_encoder(). Actually by that time we
> > have already read out the hw state, so it shouldn't require any
> > modifications to the existing dsi code itself.
> 
> I do not think that intel_sanitize encoder is the right place to do this:
> 
> * I don't want to modify the read-back state, I want to modify our
>   calculated "new/ideal" state to match the read-back state
> * I don't want to directly modify our calculated new/ideal state,
>   instead I want to cleanup / sanitize the values we got from the VBT
>   so that the initial-modeset *and* any future modesets will use the
>   GOP pclk. I believe it is important that if we're going to use the
>   GOP pclk we use it for all modesets consistently.
> * I only want to do this once, at boot when we are sure the mode was
>   set by the GOP and not after suspend/resume when we don't know if the
>   GOP will have touched things or not.
> * It is DSI specific, whereas sofar intel_sanitize_encoder seems to
>   not contain any output specific code.
> 
> Regards,
> 
> Hans
> 
> 
> 
> > 
> > > +
> > >   	intel_dsi->burst_mode_ratio = burst_mode_ratio;
> > >   	intel_dsi->pclk = pclk;
> > > -- 
> > > 2.17.1
> > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/intel_dsi: Read back pclk set by GOP and use that as pclk

[Hans de Goede]

Hi,

On 07/09/2018 07:37 PM, Rodrigo Vivi wrote:
> On Sat, Jul 07, 2018 at 08:32:16AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07/06/2018 04:16 PM, Ville Syrjälä wrote:
>>> On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote:
>>>> On BYT and CHT the GOP sometimes initializes the pclk at a (slightly)
>>>> different frequency then the pclk which we've calculated.
>>>>
>>>> This commit makes the DSI code read-back the pclk set by the GOP and
>>>> if that is within a reasonable margin of the calculated pclk, uses
>>>> that instead.
>>>>
>>>> This fixes the first modeset being a full modeset instead of a
>>>> fast modeset on systems where the GOP pclk is different.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>>>> index 4d6ffa7b3e7b..d4cc6099012c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
>>>> @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>>>    	u32 mul;
>>>>    	u16 burst_mode_ratio;
>>>>    	enum port port;
>>>> +	enum pipe pipe;
>>>>    	DRM_DEBUG_KMS("\n");
>>>> @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
>>>>    	} else
>>>>    		burst_mode_ratio = 100;
>>>> +	/*
>>>> +	 * On BYT / CRC the GOP sometimes picks a slightly different pclk,
>>>> +	 * read back the GOP configured pclk and prefer it over ours.
>>>> +	 */
>>>> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>>>> +	    intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) {
>>>> +		u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL);
>>>> +
>>>> +		DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop);
>>>> +		if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10))
>>>> +			pclk = gop;
>>>> +	}
>>>
>>> Is the gop acually picking a different clock that what we pick in the
>>> end, or is it just that the value in the vbt doesn't quite match what we
>>> (and the gop) end up using on account of limitations of the pll?
>>
>> I *think* the GOP is picking a different clock, IIRC (*) it is something like
>> 90Mhz for the GOP and the VBT says 87Mhz (and our calculations leave it
>> unmodified. With this patch which puts pclk at 90Mhz on the specific
>> tablet I developed this on, the PLL settings calculated by our PLL code
>> end up being exactly the same as the once chosen by the GOP once we have
>> the pclk set to 90MHz.
>>
>> Note I've seen these small (and sometimes somewhat bigger) differences
>> between GOP and VBT pclk on a lot of devices, not just the one tablet
>> I developed it on. Since submitting this I've run this on at least
>> 5 different CHT/BYT devices and it works as advertised so far.
> 
> It seems GOP is just not respecting VBT and using the whatever it judge
> the safest option?!

Yes, that or it is using some unknown rounding mechanism, to get
to a pclk which the pll can represent exactly. If I don't fixup the pclk
in the VBT the actually set pclk is somewhat different then the one originally
in the VBT as the PLL can not generate an exact match.

> Probably not optimal, but I believe we should stay on the safest side as well,
> if this is the case.

Agreed.

Regards,

Hans


> 
>>
>>> For that particular problem I think I had patches long ago to go through
>>> the pll computation during init so that we basically fix up the slightly
>>> bogus clock from the vbt.
>>
>> We do end up with a slightly different clock then the 87MHhz when going
>> though the PLL calculations, something like 86.33MHz or some such from
>> the top of my head, but the problem is not the pclk not matching the
>> intel_pipe_config_compare() function does not look at it, it looks at
>> dsi_pll.ctrl dsi_pll.div and those don't match, where as they do match
>> if we fixup the VBT clock to be the one confgured by the GOP.
>>
>>> Any kind of hack that involves reading out the hardware state should go
>>> into something like intel_sanitize_encoder(). Actually by that time we
>>> have already read out the hw state, so it shouldn't require any
>>> modifications to the existing dsi code itself.
>>
>> I do not think that intel_sanitize encoder is the right place to do this:
>>
>> * I don't want to modify the read-back state, I want to modify our
>>    calculated "new/ideal" state to match the read-back state
>> * I don't want to directly modify our calculated new/ideal state,
>>    instead I want to cleanup / sanitize the values we got from the VBT
>>    so that the initial-modeset *and* any future modesets will use the
>>    GOP pclk. I believe it is important that if we're going to use the
>>    GOP pclk we use it for all modesets consistently.
>> * I only want to do this once, at boot when we are sure the mode was
>>    set by the GOP and not after suspend/resume when we don't know if the
>>    GOP will have touched things or not.
>> * It is DSI specific, whereas sofar intel_sanitize_encoder seems to
>>    not contain any output specific code.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>
>>>> +
>>>>    	intel_dsi->burst_mode_ratio = burst_mode_ratio;
>>>>    	intel_dsi->pclk = pclk;
>>>> -- 
>>>> 2.17.1
>>>
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/intel_dsi: Read back pclk set by GOP and use that as pclk

[Ville Syrjälä]

On Sat, Jul 07, 2018 at 08:32:16AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 07/06/2018 04:16 PM, Ville Syrjälä wrote:
> > On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote:
> >> On BYT and CHT the GOP sometimes initializes the pclk at a (slightly)
> >> different frequency then the pclk which we've calculated.
> >>
> >> This commit makes the DSI code read-back the pclk set by the GOP and
> >> if that is within a reasonable margin of the calculated pclk, uses
> >> that instead.
> >>
> >> This fixes the first modeset being a full modeset instead of a
> >> fast modeset on systems where the GOP pclk is different.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++
> >>   1 file changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
> >> index 4d6ffa7b3e7b..d4cc6099012c 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
> >> @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
> >>   	u32 mul;
> >>   	u16 burst_mode_ratio;
> >>   	enum port port;
> >> +	enum pipe pipe;
> >>   
> >>   	DRM_DEBUG_KMS("\n");
> >>   
> >> @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
> >>   	} else
> >>   		burst_mode_ratio = 100;
> >>   
> >> +	/*
> >> +	 * On BYT / CRC the GOP sometimes picks a slightly different pclk,
> >> +	 * read back the GOP configured pclk and prefer it over ours.
> >> +	 */
> >> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> >> +	    intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) {
> >> +		u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL);
> >> +
> >> +		DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop);
> >> +		if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10))
> >> +			pclk = gop;
> >> +	}
> > 
> > Is the gop acually picking a different clock that what we pick in the
> > end, or is it just that the value in the vbt doesn't quite match what we
> > (and the gop) end up using on account of limitations of the pll?
> 
> I *think* the GOP is picking a different clock, IIRC (*) it is something like
> 90Mhz for the GOP and the VBT says 87Mhz (and our calculations leave it
> unmodified. With this patch which puts pclk at 90Mhz on the specific
> tablet I developed this on, the PLL settings calculated by our PLL code
> end up being exactly the same as the once chosen by the GOP once we have
> the pclk set to 90MHz.
> 
> Note I've seen these small (and sometimes somewhat bigger) differences
> between GOP and VBT pclk on a lot of devices, not just the one tablet
> I developed it on. Since submitting this I've run this on at least
> 5 different CHT/BYT devices and it works as advertised so far.
> 
> > For that particular problem I think I had patches long ago to go through
> > the pll computation during init so that we basically fix up the slightly
> > bogus clock from the vbt.
> 
> We do end up with a slightly different clock then the 87MHhz when going
> though the PLL calculations, something like 86.33MHz or some such from
> the top of my head, but the problem is not the pclk not matching the
> intel_pipe_config_compare() function does not look at it, it looks at
> dsi_pll.ctrl dsi_pll.div and those don't match, where as they do match
> if we fixup the VBT clock to be the one confgured by the GOP.
> 
> > Any kind of hack that involves reading out the hardware state should go
> > into something like intel_sanitize_encoder(). Actually by that time we
> > have already read out the hw state, so it shouldn't require any
> > modifications to the existing dsi code itself.
> 
> I do not think that intel_sanitize encoder is the right place to do this:
> 
> * I don't want to modify the read-back state, I want to modify our
>    calculated "new/ideal" state to match the read-back state

I wasn't suggesting that. What I meant is that you already have the
state there to look so you don't have to hack the readout functions
to function without a state being around.

That said, we do already have intel_encoder_current_mode() which is doing
something similar to what you're proposing. So probably should just
try to reuse that.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/i915/intel_dsi: Read back pclk set by GOP and use that as pclk

[Hans de Goede]

Hi,

On 07/09/2018 08:14 PM, Ville Syrjälä wrote:
> On Sat, Jul 07, 2018 at 08:32:16AM +0200, Hans de Goede wrote:

<snip>

>>> Any kind of hack that involves reading out the hardware state should go
>>> into something like intel_sanitize_encoder(). Actually by that time we
>>> have already read out the hw state, so it shouldn't require any
>>> modifications to the existing dsi code itself.
>>
>> I do not think that intel_sanitize encoder is the right place to do this:
>>
>> * I don't want to modify the read-back state, I want to modify our
>>     calculated "new/ideal" state to match the read-back state
> 
> I wasn't suggesting that. What I meant is that you already have the
> state there to look so you don't have to hack the readout functions
> to function without a state being around.
> 
> That said, we do already have intel_encoder_current_mode() which is doing
> something similar to what you're proposing. So probably should just
> try to reuse that.

Ah yes that should work and will allow me to drop 2 of the 3 preparation
patches (I still need the patch which initializes the encoder enough that
it can be passed to get_hw_state() from intel_dsi_init_vbt().

So something like this ?   :

 From 82db375919be9b95e680dc5b1f660d5ef1efcc94 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 10 Jul 2018 08:59:27 +0200
Subject: [PATCH] drm/i915/intel_dsi: Read back pclk set by GOP and use that as
  pclk

On BYT and CHT the GOP sometimes initializes the pclk at a (slightly)
different frequency then the pclk which we've calculated.

This commit makes the DSI code read-back the pclk set by the GOP and
if that is within a reasonable margin of the calculated pclk, uses
that instead.

This fixes the first modeset being a full modeset instead of a
fast modeset on systems where the GOP pclk is different.

Changes in v2:
-Use intel_encoder_current_mode() to get the pclk setup by the GOP

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
  drivers/gpu/drm/i915/intel_dsi_vbt.c | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
index 4d6ffa7b3e7b..828d5ac0dfef 100644
--- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
@@ -506,6 +506,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
  	struct mipi_config *mipi_config = dev_priv->vbt.dsi.config;
  	struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps;
  	struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode;
+	struct drm_display_mode *curr;
  	u32 bpp;
  	u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui;
  	u32 ui_num, ui_den;
@@ -583,6 +584,23 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
  	} else
  		burst_mode_ratio = 100;

+	/*
+	 * On BYT / CRC the GOP sometimes picks a slightly different pclk,
+	 * read back the GOP configured pclk and prefer it over ours.
+	 */
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		curr = intel_encoder_current_mode(&intel_dsi->base);
+		if (curr) {
+			DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n",
+				      pclk, curr->clock);
+			if (curr->clock >= (pclk * 9 / 10) &&
+			    curr->clock <= (pclk * 11 / 10))
+				pclk = curr->clock;
+
+			kfree(curr);
+		}
+	}
+
  	intel_dsi->burst_mode_ratio = burst_mode_ratio;
  	intel_dsi->pclk = pclk;


Note this is untested as I'm travelling at the moment. I will test
this when I'm back home and send out a v2.

Regards,

Hans



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

Re: [PATCH 0/4] drm/i915/intel_dsi: Read back and use pclk set by the GOP

[Hans de Goede]

Hi,

On 19-06-18 22:18, Hans de Goede wrote:
> Hi All,
> 
> This patch-set is the result of the work I've been doing recently to
> give people a smooth "flickerfree" boot experience where the display
> keeps displaying the logo put there by the firmware until it smoothly
> fades into the Linux GUI (e.g. gdm).
> 
> While testing this on some BYT/CHT devices I noticed the screen going
> black for 1 second or so, because the first modeset was not a fast
> modeset despite passing i915.fastboot=1 on the kernel commandline.
> 
> This is caused by the GOP and the i915 code differing in what they
> think the DSI pclk should be. This patch-set fixes this by reading
> back the pclk set by the GOP and if it is reasonably close to the
> clk calculated by the i915 code, using the GOP set pclk.

Ping? A review of this series would be appreciated.

Regards,

Hans

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

Re: [PATCH 0/4] drm/i915/intel_dsi: Read back and use pclk set by the GOP

[Jani Nikula]

On Fri, 29 Jun 2018, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 19-06-18 22:18, Hans de Goede wrote:
>> Hi All,
>> 
>> This patch-set is the result of the work I've been doing recently to
>> give people a smooth "flickerfree" boot experience where the display
>> keeps displaying the logo put there by the firmware until it smoothly
>> fades into the Linux GUI (e.g. gdm).
>> 
>> While testing this on some BYT/CHT devices I noticed the screen going
>> black for 1 second or so, because the first modeset was not a fast
>> modeset despite passing i915.fastboot=1 on the kernel commandline.
>> 
>> This is caused by the GOP and the i915 code differing in what they
>> think the DSI pclk should be. This patch-set fixes this by reading
>> back the pclk set by the GOP and if it is reasonably close to the
>> clk calculated by the i915 code, using the GOP set pclk.
>
> Ping? A review of this series would be appreciated.

Sorry, swamped, and I was hoping Ville would chime in. ;)

The first two patches are trivial, ack. But I'd really like to know what
the GOP is doing, and why we're different. Not sure if that's possible.

I'm afraid after much bikeshedding there were some file and function
renames, and no matter what this series needs a rebase. See [1]. Sorry
about the extra effort this causes; I truly appreciate your efforts with
the DSI code, and I regret I haven't given this series the love it
needs.

BR,
Jani.

[1] https://patchwork.freedesktop.org/series/45984/


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

Re: [PATCH 0/4] drm/i915/intel_dsi: Read back and use pclk set by the GOP

[Hans de Goede]

Hi,

On 07/06/2018 03:14 PM, Jani Nikula wrote:
> On Fri, 29 Jun 2018, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 19-06-18 22:18, Hans de Goede wrote:
>>> Hi All,
>>>
>>> This patch-set is the result of the work I've been doing recently to
>>> give people a smooth "flickerfree" boot experience where the display
>>> keeps displaying the logo put there by the firmware until it smoothly
>>> fades into the Linux GUI (e.g. gdm).
>>>
>>> While testing this on some BYT/CHT devices I noticed the screen going
>>> black for 1 second or so, because the first modeset was not a fast
>>> modeset despite passing i915.fastboot=1 on the kernel commandline.
>>>
>>> This is caused by the GOP and the i915 code differing in what they
>>> think the DSI pclk should be. This patch-set fixes this by reading
>>> back the pclk set by the GOP and if it is reasonably close to the
>>> clk calculated by the i915 code, using the GOP set pclk.
>>
>> Ping? A review of this series would be appreciated.
> 
> Sorry, swamped, and I was hoping Ville would chime in. ;)
> 
> The first two patches are trivial, ack. But I'd really like to know what
> the GOP is doing, and why we're different. Not sure if that's possible.
> 
> I'm afraid after much bikeshedding there were some file and function
> renames, and no matter what this series needs a rebase. See [1]. Sorry
> about the extra effort this causes; I truly appreciate your efforts with
> the DSI code, and I regret I haven't given this series the love it
> needs.

No problem. I'll pick up those patches and do a rebase, I'm currently
travelling so I don't have access to hardware to test this for a couple of
days. I will post a rebased version when I'm back.

Regards,

Hans





_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel