On Wed, 27 Apr 2016, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Tue, Apr 19, 2016 at 01:48:14PM +0530, Ramalingam C wrote:
>> In BXT DSI there is no regs programmed with few horizontal timings
>> in Pixels but txbyteclkhs.. So retrieval process adds some
>> ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
>>
>> Actually here for the given adjusted_mode, we are calculating the
>> value programmed to the port and then back to the horizontal timing
>> param in pixels. This is the expected value at the end of get_config,
>> including roundup errors. And if that is same as retrieved value
>> from port, then retrieved (HW state) adjusted_mode's horizontal
>> timings are corrected to match with SW state to nullify the errors.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/intel_dsi.c | 97 ++++++++++++++++++++++++++++++++++----
>> 1 file changed, 88 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index e0259e6..8a1b872 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -46,6 +46,14 @@ static const struct {
>> },
>> };
>>
>> +/* return pixels in terms of txbyteclkhs */
>> +static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
>> + u16 burst_mode_ratio)
>> +{
>> + return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
>> + 8 * 100), lane_count);
>> +}
>> +
>> /* return pixels equvalent to txbyteclkhs */
>> static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count,
>> u16 burst_mode_ratio)
>> @@ -779,11 +787,19 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct drm_display_mode *adjusted_mode =
>> &pipe_config->base.adjusted_mode;
>> + struct drm_display_mode *adjusted_mode_sw;
>> + struct intel_crtc *intel_crtc;
>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>> unsigned int lane_count = intel_dsi->lane_count;
>> unsigned int bpp, fmt;
>> enum port port;
>> u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
>> + u16 hfp_sw, hsync_sw, hbp_sw;
>> + u16 crtc_htotal_sw, crtc_hsync_start_sw, crtc_hsync_end_sw,
>> + crtc_hblank_start_sw, crtc_hblank_end_sw;
>> +
>> + intel_crtc = to_intel_crtc(encoder->base.crtc);
>> + adjusted_mode_sw = &intel_crtc->config->base.adjusted_mode;
>>
>
>> /*
>> * Atleast one port is active as encoder->get_config called only if
>> @@ -847,8 +863,79 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>> adjusted_mode->crtc_vsync_end = vsync + adjusted_mode->crtc_vsync_start;
>> adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay;
>> adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal;
>> -}
>>
>> + /*
>> + * In BXT DSI there is no regs programmed with few horizontal timings
>> + * in Pixels but txbyteclkhs.. So retrieval process adds some
>> + * ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
>> + * Actually here for the given adjusted_mode, we are calculating the
>> + * value programmed to the port and then back to the horizontal timing
>> + * param in pixels. This is the expected value, including roundup errors
>> + * And if that is same as retrieved value from port, then
>> + * (HW state) adjusted_mode's horizontal timings are corrected to
>> + * match with SW state to nullify the errors.
>> + */
>> + /* Calculating the value programmed to the Port register */
>> + hfp_sw = adjusted_mode_sw->crtc_hsync_start -
>> + adjusted_mode_sw->crtc_hdisplay;
>> + hsync_sw = adjusted_mode_sw->crtc_hsync_end -
>> + adjusted_mode_sw->crtc_hsync_start;
>> + hbp_sw = adjusted_mode_sw->crtc_htotal -
>> + adjusted_mode_sw->crtc_hsync_end;
>
> So during the initial state readout we get passed crtc->config as
> pipe_config, and so we'll end up comparing the thing with itself. That
> should be fine. A bit of extra care is needed to make sure we don't use
> anything from crtc->config before we've filled it out with something,
> but looks like the code does things in the right order (given the
> previous patch which fills out all the htimings with something).
>
> I think the biggest issue with this patch is seeing the forest from the
> trees. Some refactoring would be good so that we'd have some kind of
> htimings_{to,from}_txbyteclkhs() functions instead of duplicating the
> same code multiple times.
Agreed. However, I opted to push them as-is as they fix the boot hang,
and leave the refactoring as follow-up.
BR,
Jani.
>
> So while it does look quite strange to be using crtc->config in the
> .get_config() I think it should work out.
>
> Acked-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
>> +
>> + if (intel_dsi->dual_link) {
>> + hfp_sw /= 2;
>> + hsync_sw /= 2;
>> + hbp_sw /= 2;
>> + }
>> +
>> + hfp_sw = txbyteclkhs(hfp_sw, bpp, lane_count,
>> + intel_dsi->burst_mode_ratio);
>> + hsync_sw = txbyteclkhs(hsync_sw, bpp, lane_count,
>> + intel_dsi->burst_mode_ratio);
>> + hbp_sw = txbyteclkhs(hbp_sw, bpp, lane_count,
>> + intel_dsi->burst_mode_ratio);
>> +
>> + /* Reverse calculating the adjusted mode parameters from port reg vals*/
>> + hfp_sw = pixels_from_txbyteclkhs(hfp_sw, bpp, lane_count,
>> + intel_dsi->burst_mode_ratio);
>> + hsync_sw = pixels_from_txbyteclkhs(hsync_sw, bpp, lane_count,
>> + intel_dsi->burst_mode_ratio);
>> + hbp_sw = pixels_from_txbyteclkhs(hbp_sw, bpp, lane_count,
>> + intel_dsi->burst_mode_ratio);
>> +
>> + if (intel_dsi->dual_link) {
>> + hfp_sw *= 2;
>> + hsync_sw *= 2;
>> + hbp_sw *= 2;
>> + }
>> +
>> + crtc_htotal_sw = adjusted_mode_sw->crtc_hdisplay + hfp_sw +
>> + hsync_sw + hbp_sw;
>> + crtc_hsync_start_sw = hfp_sw + adjusted_mode_sw->crtc_hdisplay;
>> + crtc_hsync_end_sw = hsync_sw + crtc_hsync_start_sw;
>> + crtc_hblank_start_sw = adjusted_mode_sw->crtc_hdisplay;
>> + crtc_hblank_end_sw = crtc_htotal_sw;
>> +
>> + if (adjusted_mode->crtc_htotal == crtc_htotal_sw)
>> + adjusted_mode->crtc_htotal = adjusted_mode_sw->crtc_htotal;
>> +
>> + if (adjusted_mode->crtc_hsync_start == crtc_hsync_start_sw)
>> + adjusted_mode->crtc_hsync_start =
>> + adjusted_mode_sw->crtc_hsync_start;
>> +
>> + if (adjusted_mode->crtc_hsync_end == crtc_hsync_end_sw)
>> + adjusted_mode->crtc_hsync_end =
>> + adjusted_mode_sw->crtc_hsync_end;
>> +
>> + if (adjusted_mode->crtc_hblank_start == crtc_hblank_start_sw)
>> + adjusted_mode->crtc_hblank_start =
>> + adjusted_mode_sw->crtc_hblank_start;
>> +
>> + if (adjusted_mode->crtc_hblank_end == crtc_hblank_end_sw)
>> + adjusted_mode->crtc_hblank_end =
>> + adjusted_mode_sw->crtc_hblank_end;
>> +}
>>
>> static void intel_dsi_get_config(struct intel_encoder *encoder,
>> struct intel_crtc_state *pipe_config)
>> @@ -917,14 +1004,6 @@ static u16 txclkesc(u32 divider, unsigned int us)
>> }
>> }
>>
>> -/* return pixels in terms of txbyteclkhs */
>> -static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
>> - u16 burst_mode_ratio)
>> -{
>> - return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
>> - 8 * 100), lane_count);
>> -}
>> -
>> static void set_dsi_timings(struct drm_encoder *encoder,
>> const struct drm_display_mode *adjusted_mode)
>> {
>> --
>> 1.7.9.5
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
