[Intel-gfx] [PATCH 3/4] drm/i915/dsi: Adjust crtc_clock for burst_mode_ratio

[Hans de Goede]

Hi,

On 21-01-19 15:26, Hans de Goede wrote:
> 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 at 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

p.s.

The 4th patch in this series is independent of the others, it fixes
a small bug (not freeing a resource) in an exit error path which I
noticed. It would be great if someone can review the 4th patch then
I can push that one too and then this patch will be the only unmerged
patch from this series.

Regards,

Hans