[Intel-gfx] [PATCH] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison
Ramalingam C
ramalingam.c at intel.com
Tue Mar 29 17:42:40 UTC 2016
On Tuesday 29 March 2016 01:08 PM, Jani Nikula wrote:
> On Mon, 28 Mar 2016, Ramalingam C <ramalingam.c at intel.com> wrote:
>> On Monday 21 March 2016 07:13 PM, Jani Nikula wrote:
>>> On Fri, 11 Mar 2016, Ramalingam C <ramalingam.c at intel.com> wrote:
>>>> At BXT DSI, PIPE registers are inactive. So we can 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.
>>> I think you should take the encoder specific callback parts from [1],
>>> and instead of adding a new ->get_pipe_config hook, do them in
>>> ->get_hw_state, and add the rebased work from this patch on top.
>>>
>>> BR,
>>> Jani.
>>>
>>> [1] http://patchwork.freedesktop.org/patch/msgid/1456771760-18823-1-git-send-email-ramalingam.c@intel.com
>> Jani,
>>
>> At present, encoder->get_hw_state is used for the getting the encoder's
>> activeness(dsi port is active or not) along with the pipe it is
>> connected to it.
>> By definition of that encoder function declaration, I dont think we
>> should use it for retrieving the pipe timing parameters from port.
>>
>> I feel adding a new encoder function is making more sense, for handling
>> this special case of retrieving the timing parameters
>> from port register for BXT.
>>
>> But If we dont want to use new encoder function I feel
>> encoder->get_config is the option. As this is supposed to get called after
>> display->get_pipe_config and also this function fills the
>> pipe_config->base.adjusted_mode.crtc_clock already.
>>
>> Please tell me if you agree with the separate encoder function.
> Please use encoder->get_config().
>
> BR,
> Jani.
>
Please find the change at
https://lists.freedesktop.org/archives/intel-gfx/2016-March/090999.html
--Ram
>> --Ram
>>
>>>
>>>> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
>>>>
>>>> This change is required for
>>>> https://lists.freedesktop.org/archives/intel-gfx/2016-February/088653.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 7068dc1..46ce662 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12653,6 +12653,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 { \
>>>> @@ -12680,6 +12683,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,\
>>>> @@ -12784,11 +12835,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);
>>>> @@ -12866,6 +12917,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_I_ALT
>>>> #undef PIPE_CONF_CHECK_FLAGS
>>>> #undef PIPE_CONF_CHECK_CLOCK_FUZZY
--
Thanks,
--Ram
More information about the Intel-gfx
mailing list