[Intel-gfx] [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison
Ramalingam C
ramalingam.c at intel.com
Tue Apr 5 09:40:39 UTC 2016
On Tuesday 05 April 2016 02:00 PM, Jani Nikula wrote:
> On Mon, 04 Apr 2016, Ramalingam C <ramalingam.c at intel.com> wrote:
>> On Thursday 31 March 2016 12:34 AM, Daniel Vetter wrote:
>>> On Wed, Mar 30, 2016 at 07:49:40PM +0530, Ramalingam C wrote:
>>>> On Wednesday 30 March 2016 05:02 PM, Daniel Vetter wrote:
>>>>> On Tue, Mar 29, 2016 at 11:04:51PM +0530, Ramalingam C wrote:
>>>>>> At BXT DSI, PIPE registers are inactive. So we can't get the
>>>>>> PIPE's mode parameters from them. The possible option is
>>>>>> retriving them from the PORT registers. But mode timing
>>>>>> parameters are progammed to port registers interms of byteclocks.
>>>>>>
>>>>>> The formula used to convert the pixels interms of byteclk is
>>>>>> DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
>>>>>> 8 * 100), lane_count);
>>>>>>
>>>>>> So we retrieve them, interms of pixels as
>>>>>> DIV_ROUND_UP((clk_hs * lane_count * 8 * 100),
>>>>>> (bpp * burst_mode_ratio));
>>>>>>
>>>>>> Due to the multiple DIV_ROUND_UP in both formulas we get the worst
>>>>>> case delta in the retrieved PIPE's timing parameter as below
>>>>>> DIV_ROUND_UP((8 * intel_dsi->lane_count * 100),
>>>>>> (dsi_pixel_format_bpp(intel_dsi->pixel_format) *
>>>>>> intel_dsi->burst_mode_ratio)))
>>>>>>
>>>>>> This converson of byteclk to pixel is required for hsync, hfp and hbp.
>>>>>> Which intern impacts horrizontal timing parameters. At worst case to
>>>>>> get htotal all there parameters are added with hactive.
>>>>>> Hence delta will be 3 times of above formula. Hence this value is
>>>>>> considered as tolerance for pipe_config comparison, in case of BXT DSI.
>>>>>>
>>>>>> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
>>>>> This is the wrong way round imo, better would be to adjust the adjusted
>>>>> mode in the bxt dsi compute_config function to match the hw granularity.
>>>>> Stuff _really_ should match exactly, the fuzzy clock matching is mostly
>>>>> because our clock cod is a mess, and we can't/don't properly
>>>>> forward-compuate the actual clock timings we program into the hardware.
>>>>> -Daniel
>>>> Daniel, I got your point. But the problem will be that difficulty(even if
>>>> possible) in adjusting the adjusted mode parameters.
>>>> Reason is we are not programing the mode parameter as such. We will derive
>>>> the hfp, hsync and hbp from
>>>> hsync_start, hsync_end, hdisplay and htotal. These will be adjusted(divided
>>>> by 2) for dual link scenario.
>>>> And then resultant will go into the conversion as mentioned in the commit
>>>> message (two DIV_ROUND_UP onwards
>>>> and one DIV_ROUND_UP backwards). For this we have to make the parameter
>>>> divisible by three different factors.
>>>> So IMHO, even if this is possible, it will look more messy.
>>>>
>>>> Predicting the max error and tolerating it in pipe_config_compare will be
>>>> the straight forward and more reasonable.
>>>> Please let me know if i can go ahead in this approach.
>>> Yeah I discussed this some more with Jani on irc. I'd say we should store
>>> this adjusted horizontal timings (the ones fudged with burst_mode_ratio,
>>> lane_count, dual-link and all these things applied) into
>>> crtc_state->base.adjusted_mode. And then ofc also read those values out.
>>>
>>> The overall idea of the state verify/compare logic is that we start out
>>> with requested state from userspace, then derive the real hw state. And
>>> then compare that computed hw state with what's there already. Except for
>>> clocks, where there's special reasons, we never go back, since going back
>>> requires us to apply a range. This is the only way to guarnatee that "hw
>>> has the same exact mode programmed in both cases" iff "intel_crtc_state
>>> matches per intel_crtc_config_compare".
>>>
>>> state->adjusted_mode is never exposed to userspace, so there's no problem
>>> if it's has "strange" values. And we already have pipe_src_h/w to express
>>> the logical input rectangle.
>>>
>>> The idea is similar to how we set adjusted_mode.flags to what we actually
>>> program, instead of trying to make something up that's not perfectly
>>> accurate.
>>> -Daniel
>> Daniel,
>>
>> I have tested by adjusting the adjusted_mode in set_dsi_timings()
>> instead of intel_dsi_compute_config().
>> Reason is if we modify the adjusted mode at intel_dsi_compute_config()
>> itself, then modified value will
>> be taken as input for set_dsi_timings. Hence the get_config will deviate
>> further. I hope this should be fine with you and Jani.
>>
>> This will work out, if set_dsi_timings() is called after the
>> dsi_compute_config() on every suspend and resume or modeset.
>> I will verify this on Android once and update.
>>
>> Please share your view on this, so that can update the patch with
>> corresponding changes.
> I can't speak for Daniel, but I think his point was to update adjusted
> mode in ->compute_config() in a way that can be used directly in
> set_dsi_timings(). Then, it should be possible to read the timings from
> the hardware, and compare.
No, thats not possible jani. I think i didn't elaborate the problem
statement enough.
If you can read the programmed value from the hardware without any
error, then there is no need for this patch itself.
Even if we program the modified adjusted mode, timing parameters read
from get_config() will not be same as of modified adjusted mode.
In BXT DSI only available hw registers doesn't provide all timing
parameters in terms of pixels but txbyteclkhs.
adjusted mode has the parameters(start and end of hsync, htotal and
hdisplay and others) in terms of pixels.
So some conversion involved in programming few parameters (hfp, hsync
and hbp) and also in retrieving them.
As discussed above port registers expects hfp, hsync and hbp interms of
txbyteclkhs.
Sequence of programing (set_dsi_timings) the dsi port registers:
parameters from mode ---> (calc hfp, hsync and hbp) ---> (adjust for
dual link) ----> (conversion of Pixels to txbyteclkhs) ---> Program to
Port register
Sequence of get_config():
Read from port register ---> (conversion of txbyteclkhs to Pixels) --->
(adjust for dual link) ---> (recalculate the adjusted mode parameters
from hfp, hsync and hbp and other readings)
Here if we assume the input to the set_dsi_timings is X(adjusted mode
parameter), output of get_config() will be (X + delta1).
Here delta1 is error due to multiple DIV_ROUND_UP() in the conversion of
bytes <===> txbyteclkhs.
So as daniel says if you modify the adjusted_mode in compute_config()
itself, input to the set_dsi_timings() will become (X + delta1)
and the readings from the get_config() will become (X + delta1 + delta2)
And it wouldn't be appropriate to program the hw with modified adjusted
mode. This modification is just to match it with the pipe_config read
from hw.
Hence adjusted mode can be modified after the hw programming only, so
the place to do is end of set_dsi_timings().
Hope I explained the situation enough.
>
> BR,
> Jani.
>
>
>>>>>> ---
>>>>>> Reviewed at https://lists.freedesktop.org/archives/intel-gfx/2016-March/089548.html
>>>>>>
>>>>>> drivers/gpu/drm/i915/intel_display.c | 62 +++++++++++++++++++++++++++++++---
>>>>>> 1 file changed, 57 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>> index c0627d6..282f036 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>> @@ -12557,6 +12557,9 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>>> bool adjust)
>>>>>> {
>>>>>> bool ret = true;
>>>>>> + struct intel_crtc *crtc = to_intel_crtc(current_config->base.crtc);
>>>>>> + struct intel_encoder *intel_encoder;
>>>>>> + struct intel_dsi *intel_dsi = NULL;
>>>>>> #define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
>>>>>> do { \
>>>>>> @@ -12593,6 +12596,54 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>>> ret = false; \
>>>>>> }
>>>>>> +/*
>>>>>> + * In case of BXT DSI, HW pipe_config will be retrieved from the port's timing
>>>>>> + * configuration. This retrival includes some errors due to the DIV_ROUND_UP.
>>>>>> + * So we are considering the max possible error at the comparison.
>>>>>> + */
>>>>>> +/*
>>>>>> + * htotal = hactive + hfp + hsync + hbp. Here last three lements might have
>>>>>> + * the converson error, hence we consider the 3 times of error as tolerance.
>>>>>> + */
>>>>>> +
>>>>>> +#define MAX_BXT_DSI_TIMING_RETRIVAL_ERR \
>>>>>> + (intel_dsi == NULL ? 0 : \
>>>>>> + DIV_ROUND_UP((3 * 8 * intel_dsi->lane_count * 100), \
>>>>>> + (dsi_pixel_format_bpp(intel_dsi->pixel_format) * \
>>>>>> + intel_dsi->burst_mode_ratio)))
>>>>>> +
>>>>>> +#define BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) { \
>>>>>> + for_each_encoder_on_crtc(dev, &crtc->base, \
>>>>>> + intel_encoder) { \
>>>>>> + if (intel_encoder->type == INTEL_OUTPUT_DSI) { \
>>>>>> + intel_dsi = enc_to_intel_dsi(&intel_encoder->base); \
>>>>>> + } \
>>>>>> + } \
>>>>>> + if (!(current_config->name < pipe_config->name && \
>>>>>> + current_config->name >= (pipe_config->name - \
>>>>>> + MAX_BXT_DSI_TIMING_RETRIVAL_ERR))) { \
>>>>>> + INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>>>>>> + "(expected %i, found %i(Err tolerance considered))\n", \
>>>>>> + current_config->name, \
>>>>>> + pipe_config->name); \
>>>>>> + ret = false; \
>>>>>> + } \
>>>>>> +}
>>>>>> +
>>>>>> +#define PIPE_CONF_CHECK_I_RANGE(name) { \
>>>>>> + if (current_config->name != pipe_config->name) { \
>>>>>> + if (IS_BROXTON(dev) && crtc->config->has_dsi_encoder) { \
>>>>>> + BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) \
>>>>>> + } else { \
>>>>>> + INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>>>>>> + "(expected %i, found %i)\n", \
>>>>>> + current_config->name, \
>>>>>> + pipe_config->name); \
>>>>>> + ret = false; \
>>>>>> + } \
>>>>>> + } \
>>>>>> +}
>>>>>> +
>>>>>> #define PIPE_CONF_CHECK_M_N(name) \
>>>>>> if (!intel_compare_link_m_n(¤t_config->name, \
>>>>>> &pipe_config->name,\
>>>>>> @@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>>> PIPE_CONF_CHECK_I(has_dsi_encoder);
>>>>>> PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
>>>>>> - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
>>>>>> - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
>>>>>> - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
>>>>>> - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
>>>>>> - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
>>>>>> + PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
>>>>>> + PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
>>>>>> + PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
>>>>>> + PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
>>>>>> + PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
>>>>>> PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
>>>>>> PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
>>>>>> @@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>>> #undef PIPE_CONF_CHECK_X
>>>>>> #undef PIPE_CONF_CHECK_I
>>>>>> +#undef PIPE_CONF_CHECK_I_RANGE
>>>>>> #undef PIPE_CONF_CHECK_P
>>>>>> #undef PIPE_CONF_CHECK_I_ALT
>>>>>> #undef PIPE_CONF_CHECK_FLAGS
>>>>>> --
>>>>>> 1.7.9.5
>>>>>>
>>>>>> _______________________________________________
>>>>>> Intel-gfx mailing list
>>>>>> Intel-gfx at lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>> --
>>>> Thanks,
>>>> --Ram
>>>>
--
Thanks,
--Ram
More information about the Intel-gfx
mailing list