| Message ID | 1459272891-6830-2-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ramalingam,
[auto build test ERROR on drm-intel/for-linux-next]
[cannot apply to v4.6-rc1 next-20160329]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Ramalingam-C/drm-i915-BXT-Get-pipe-conf-from-the-port-registers/20160330-014708
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-s1-201613 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from include/linux/list.h:8:0,
from include/linux/dmi.h:4,
from drivers/gpu/drm/i915/intel_display.c:27:
drivers/gpu/drm/i915/intel_display.c: In function 'intel_pipe_config_compare':
>> drivers/gpu/drm/i915/intel_display.c:12612:4: error: implicit declaration of function 'dsi_pixel_format_bpp' [-Werror=implicit-function-declaration]
(dsi_pixel_format_bpp(intel_dsi->pixel_format) * \
^
include/linux/kernel.h:67:36: note: in definition of macro 'DIV_ROUND_UP'
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
^
drivers/gpu/drm/i915/intel_display.c:12624:4: note: in expansion of macro 'MAX_BXT_DSI_TIMING_RETRIVAL_ERR'
MAX_BXT_DSI_TIMING_RETRIVAL_ERR))) { \
^
drivers/gpu/drm/i915/intel_display.c:12636:4: note: in expansion of macro 'BXT_DSI_PIPE_CONF_CHECK_I_RANGE'
BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) \
^
drivers/gpu/drm/i915/intel_display.c:12751:2: note: in expansion of macro 'PIPE_CONF_CHECK_I_RANGE'
PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
^
cc1: some warnings being treated as errors
vim +/dsi_pixel_format_bpp +12612 drivers/gpu/drm/i915/intel_display.c
12606 * the converson error, hence we consider the 3 times of error as tolerance.
12607 */
12608
12609 #define MAX_BXT_DSI_TIMING_RETRIVAL_ERR \
12610 (intel_dsi == NULL ? 0 : \
12611 DIV_ROUND_UP((3 * 8 * intel_dsi->lane_count * 100), \
12612 (dsi_pixel_format_bpp(intel_dsi->pixel_format) * \
12613 intel_dsi->burst_mode_ratio)))
12614
12615 #define BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) { \
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, 29 Mar 2016, Ramalingam C <ramalingam.c@intel.com> 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@intel.com> > --- > 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) { \ Please drop the platform and encoder type checks here, and move them to a higher level. > + 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; \ > + } \ > + } \ > +} I think you should model this after PIPE_CONF_CHECK_CLOCK_FUZZY. Please add a function to do the check, similar to intel_fuzzy_clock_check(), and this will be much easier to read and understand. Maybe call the macro PIPE_CONF_CHECK_DSI_TIMING_FUZZY or something, since I guess it has to be encoder specific. > + > #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); Please add the platform and encoder type checks at this level for clarity (and as I suggested to drop the checks from the macro itself). if (IS_BROXTON(dev_priv) && crtc->config->has_dsi_encoder) { ... } else { ... } BR, Jani. > > 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
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@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 > --- > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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. > >> --- >> 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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 > > > >>--- > >>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@lists.freedesktop.org > >>https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Thanks, > --Ram >
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@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. > >>>> --- >>>> 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@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> -- >> Thanks, >> --Ram >>
On Mon, 04 Apr 2016, Ramalingam C <ramalingam.c@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@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. 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@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> -- >>> Thanks, >>> --Ram >>>
On Tuesday 05 April 2016 02:00 PM, Jani Nikula wrote: > On Mon, 04 Apr 2016, Ramalingam C <ramalingam.c@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@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@lists.freedesktop.org >>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >>>> -- >>>> Thanks, >>>> --Ram >>>>
On Tue, Apr 05, 2016 at 11:30:19AM +0300, Jani Nikula wrote: > On Mon, 04 Apr 2016, Ramalingam C <ramalingam.c@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@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. Yup, that's the idea. And the idea furthermore is to store _exactly_ the timings we store in the dsi controller (including clock fudging and whatever) and _not_ try to compute things backwards. The idea behind the hw state checker is that you take the requested mode and compute forward until you are at the values you can program into the hw. Currently what we have in ->adjusted_mode is not actually what we program into the hw, so not yet good enough. Then the hw state verifier only compares things 1:1. Clocks are a bit an exception, for special reasons. But for anything else we shouldn't ever try to reconstruct the original requested mode from userspace, due to aliasing issues and rounding problems. This means that for dsi ->adjusted_mode will have even less to do with the requested mode from userspace, but that's no issue. We only use that internally to track the actual hw state. -Daniel > > 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@lists.freedesktop.org > >>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >>> -- > >>> Thanks, > >>> --Ram > >>> > > -- > Jani Nikula, Intel Open Source Technology Center
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@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@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. Then fix adjusted_mode to have the timings in terms of txbyteclkhs already. Problem solved. -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@lists.freedesktop.org > >>>>>>https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >>>>-- > >>>>Thanks, > >>>>--Ram > >>>> > > -- > Thanks, > --Ram >
On Wed, 13 Apr 2016, Daniel Vetter <daniel@ffwll.ch> 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@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@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. > > 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@lists.freedesktop.org >> >>>>>>https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >>>>-- >> >>>>Thanks, >> >>>>--Ram >> >>>> >> >> -- >> Thanks, >> --Ram >>
On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: >> 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? If we change them too hard the accurate vblank timestamp stuff will be upset. But then we only need to adjust horizontal timings for dsi, whereas on gen5+ the vblank ts code uses the line counter (i.e. vertical timings) only. If it's just that it should work, and I don't think we have any other users of the adjusted_mode. -Daniel
On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: >>> 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? > > If we change them too hard the accurate vblank timestamp stuff will be > upset. But then we only need to adjust horizontal timings for dsi, > whereas on gen5+ the vblank ts code uses the line counter (i.e. > vertical timings) only. > > If it's just that it should work, and I don't think we have any other > users of the adjusted_mode. Ok, I was wrong and we obviously need the right dotclock to compute linedur_ns correctly in drm_calc_timestamping_constants(). So either we adjust the dotclock of adjusted_mode too (imo makes most sense), or we need yet another mode somewhere and use that for dsi cross checking (real ugly imo). More I missed? -Daniel
On Wednesday 13 April 2016 05:27 PM, Daniel Vetter wrote: > On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: >>>> 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? >> If we change them too hard the accurate vblank timestamp stuff will be >> upset. But then we only need to adjust horizontal timings for dsi, >> whereas on gen5+ the vblank ts code uses the line counter (i.e. >> vertical timings) only. >> >> If it's just that it should work, and I don't think we have any other >> users of the adjusted_mode. > Ok, I was wrong and we obviously need the right dotclock to compute > linedur_ns correctly in drm_calc_timestamping_constants(). So either > we adjust the dotclock of adjusted_mode too (imo makes most sense), or > we need yet another mode somewhere and use that for dsi cross checking > (real ugly imo). More I missed? Another point to be considered: And we program hsync, hfp and hbp in terms of txbyteclkhs to port register, which are not part of adjusted_mode. So for BXT DSI, we have to store them interms of txbyteclkhs and compare with hsync, hfp and hbp read from HW?? > -Daniel
On Wed, Apr 13, 2016 at 06:34:25PM +0530, Ramalingam C wrote: > > On Wednesday 13 April 2016 05:27 PM, Daniel Vetter wrote: > >On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > >>On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: > >>>>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? > >>If we change them too hard the accurate vblank timestamp stuff will be > >>upset. But then we only need to adjust horizontal timings for dsi, > >>whereas on gen5+ the vblank ts code uses the line counter (i.e. > >>vertical timings) only. > >> > >>If it's just that it should work, and I don't think we have any other > >>users of the adjusted_mode. > >Ok, I was wrong and we obviously need the right dotclock to compute > >linedur_ns correctly in drm_calc_timestamping_constants(). So either > >we adjust the dotclock of adjusted_mode too (imo makes most sense), or > >we need yet another mode somewhere and use that for dsi cross checking > >(real ugly imo). More I missed? > Another point to be considered: > And we program hsync, hfp and hbp in terms of txbyteclkhs to port register, > which are not part of adjusted_mode. > So for BXT DSI, we have to store them interms of txbyteclkhs and compare > with hsync, hfp and hbp read from HW?? Yeah that's my idea. Plus we should probably store txbyteclkhs somewhere, too. Or at least we need to adjust the clock in adjusted_mode to match txbyteclkhs, otherwise the vblank ts code goes off the rails. -Daniel
On Wednesday 13 April 2016 08:16 PM, Daniel Vetter wrote: > On Wed, Apr 13, 2016 at 06:34:25PM +0530, Ramalingam C wrote: >> On Wednesday 13 April 2016 05:27 PM, Daniel Vetter wrote: >>> On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: >>>>>> 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? >>>> If we change them too hard the accurate vblank timestamp stuff will be >>>> upset. But then we only need to adjust horizontal timings for dsi, >>>> whereas on gen5+ the vblank ts code uses the line counter (i.e. >>>> vertical timings) only. >>>> >>>> If it's just that it should work, and I don't think we have any other >>>> users of the adjusted_mode. >>> Ok, I was wrong and we obviously need the right dotclock to compute >>> linedur_ns correctly in drm_calc_timestamping_constants(). So either >>> we adjust the dotclock of adjusted_mode too (imo makes most sense), or >>> we need yet another mode somewhere and use that for dsi cross checking >>> (real ugly imo). More I missed? >> Another point to be considered: >> And we program hsync, hfp and hbp in terms of txbyteclkhs to port register, >> which are not part of adjusted_mode. >> So for BXT DSI, we have to store them interms of txbyteclkhs and compare >> with hsync, hfp and hbp read from HW?? > Yeah that's my idea. Plus we should probably store txbyteclkhs somewhere, > too. Or at least we need to adjust the clock in adjusted_mode to match > txbyteclkhs, otherwise the vblank ts code goes off the rails. Ok. I will post a RFC for this. But for that we need to finalize few points. 1. We need to add three more variables in pipe_config for caching(SW and HW State) the hfp, hsync and hbp. 2. On BXT DSI, We will compare the hdisplay, hfp, hsync and hbp only on pipe_config_compare not all horizontal timing param of adjusted mode. 3. So we dont need to recalculate all horizontal timing parameters of adjusted_mode from port register at get_config(). Should we fill them too (No harm Though)? Please clarify if we are fine with above points. -Ram > -Daniel
On Friday 15 April 2016 04:27 PM, Ramalingam C wrote: > > On Wednesday 13 April 2016 08:16 PM, Daniel Vetter wrote: >> On Wed, Apr 13, 2016 at 06:34:25PM +0530, Ramalingam C wrote: >>> On Wednesday 13 April 2016 05:27 PM, Daniel Vetter wrote: >>>> On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> >>>> wrote: >>>>> On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula >>>>> <jani.nikula@intel.com> wrote: >>>>>>> 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? >>>>> If we change them too hard the accurate vblank timestamp stuff >>>>> will be >>>>> upset. But then we only need to adjust horizontal timings for dsi, >>>>> whereas on gen5+ the vblank ts code uses the line counter (i.e. >>>>> vertical timings) only. >>>>> >>>>> If it's just that it should work, and I don't think we have any other >>>>> users of the adjusted_mode. >>>> Ok, I was wrong and we obviously need the right dotclock to compute >>>> linedur_ns correctly in drm_calc_timestamping_constants(). So either >>>> we adjust the dotclock of adjusted_mode too (imo makes most sense), or >>>> we need yet another mode somewhere and use that for dsi cross checking >>>> (real ugly imo). More I missed? >>> Another point to be considered: >>> And we program hsync, hfp and hbp in terms of txbyteclkhs to port >>> register, >>> which are not part of adjusted_mode. >>> So for BXT DSI, we have to store them interms of txbyteclkhs and >>> compare >>> with hsync, hfp and hbp read from HW?? >> Yeah that's my idea. Plus we should probably store txbyteclkhs >> somewhere, >> too. Or at least we need to adjust the clock in adjusted_mode to match >> txbyteclkhs, otherwise the vblank ts code goes off the rails. > Ok. I will post a RFC for this. But for that we need to finalize few > points. > > 1. We need to add three more variables in pipe_config for caching(SW > and HW State) the hfp, hsync and hbp. > 2. On BXT DSI, We will compare the hdisplay, hfp, hsync and hbp only > on pipe_config_compare not all horizontal timing param of adjusted mode. > 3. So we dont need to recalculate all horizontal timing parameters of > adjusted_mode from port register at get_config(). Should we fill them > too (No harm Though)? > > Please clarify if we are fine with above points. Hi, Based on the IRC discussion, we decided to nullify the round_up error in the dsi_get_config() itself. I have taken the AR. And the RFC is submitted for review at https://lists.freedesktop.org/archives/intel-gfx/2016-April/092946.html https://lists.freedesktop.org/archives/intel-gfx/2016-April/092947.html Please review the same. Thanks --Ram > > -Ram >> -Daniel >
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
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@intel.com> --- 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(-)