Re: [PATCH 3/4] drm/i915/dsi: Adjust crtc_clock for burst_mode_ratio

From: Hans de Goede <hdegoede@redhat.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 3/4] drm/i915/dsi: Adjust crtc_clock for burst_mode_ratio
Date: Mon, 21 Jan 2019 15:26:03 +0100	[thread overview]
Message-ID: <dc6048cc-81b9-0707-5cef-3642b20a122b@redhat.com> (raw)
In-Reply-To: <20190115150024.GL20097@intel.com>

Hi,

On 15-01-19 16:00, Ville Syrjälä wrote:
> On Sat, Dec 01, 2018 at 12:31:47PM +0100, Hans de Goede wrote:
>> On devices with a burst_mode_ratio which is not 100 (1:1), the pclk
>> will have a different value then drm_display_mode.clock .
>>
>> On a Prowise PT301 tablet where vbt.lfp_lvds_vbt_mode.clock is 66100 and
>> burst_mode_ratio is 130 this leads to the following errors:
>>
>> [drm:pipe_config_err [i915]] *ERROR* mismatch in
>> pixel_rate (expected 66100, found 86458)
>> [drm:pipe_config_err [i915]] *ERROR* mismatch in
>> base.adjusted_mode.crtc_clock (expected 66100, found 86458)
>> [drm:pipe_config_err [i915]] *ERROR* mismatch in
>> port_clock (expected 66100, found 86458)
>>
>> This commit makes intel_dsi_compute_config() set
>> pipe_config.adjusted_mode.crtc_clock, taking the burst_mode_ratio into
>> account fixing this.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/i915/vlv_dsi.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c
>> index c21cbfa9653c..d72ccf557a9c 100644
>> --- a/drivers/gpu/drm/i915/vlv_dsi.c
>> +++ b/drivers/gpu/drm/i915/vlv_dsi.c
>> @@ -347,6 +347,10 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>>   			return false;
>>   	}
>>   
>> +	adjusted_mode->crtc_clock =
>> +		DIV_ROUND_UP(adjusted_mode->crtc_clock *
>> +					 intel_dsi->burst_mode_ratio, 100);
> 
> Hmm. Won't this cause incorrect refresh rate to be used in eg.
> vblank timestmap calculations?

I guess so.

Note that this patch does not change any values actually written to
the hardware. It seems that devices which actually use the burst mode
are quite rare (this is the first one encounter in probably over 40
different byt/cht devices I've tested).

I've a feeling that the entire pipeline is actually running at
the higher rate and that the framerate really is 30% higher.

Looking at the code, it seems that what a burst_mode_ratio of 130'does is make
all the values in the "modeline" except for the visual area 30% larger, which
means that we are probably already messing up the vblank calculations
anyways, since using either the uncorrected or the corrected clock is
wrong when using htotal from the original modeline, as looking at
txbyteclkhs we will use bigger values for all drm_display_mode
values except for the active region.

I think that the right way to deal with this is to isolate the
burst_ratio handling to intel_dsi_vbt.c and adjust the modeline
coming from the VBT by multiplying the clock and all timing
parameters (except h/vdisplay) there by the burst_ratio and
then recalculating h/vtotal.

This should lead to getting the vblank timestamp stuff right and
allows removing of burst_mode_ratio from all code except for the
vbt code.

If that is too invasive, given that this setup is quite rate,
then I suggest we just go with this patch. My main concern is fixing
the WARN_ON. This patch successfully does that.

> OTOH if the pipe is really fetching data at the higher burst
> rate then we should rather want to calculate the watermarks/cdclk
> based on that higher rate.

Right, the more I think about this, the more I believe calculating
a new modeline correcting for burst_ratio inside the vbt code and
dropping burst_mode_ratio handling everywhere else is the right thing
to do.

Regards,

Hans

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