On Wed, 13 Apr 2016, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Tue, Apr 05, 2016 at 03:10:39PM +0530, Ramalingam C wrote: >> >> On Tuesday 05 April 2016 02:00 PM, Jani Nikula wrote: >> >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. >> >> 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. > > Then fix adjusted_mode to have the timings in terms of txbyteclkhs > already. Problem solved. I let Ville convince me there would be problems with that. Ville, care to fill in the details? BR, Jani. > -Daniel > >> 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@xxxxxxxxxxxxxxxxxxxxx >> >>>>>>https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >>>>-- >> >>>>Thanks, >> >>>>--Ram >> >>>> >> >> -- >> Thanks, >> --Ram >> -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
