On Mon, 04 Apr 2016, Ramalingam C <ramalingam.c@xxxxxxxxx> 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@xxxxxxxxx> >>>> 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. 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@xxxxxxxxxxxxxxxxxxxxx >>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> -- >>> Thanks, >>> --Ram >>> -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
