| Message ID | 1370099783-20328-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>: > We currently mutliply the link_bw of the fdi link with the pixel > multiplier, which is wrong: The FDI link doesn't suddenly grow more > bandwidth. In reality the pixel mutliplication only happens in the PCH, > before the pixels are fed into the port. > > But since we our code treats the uses the target clock after pixels > are doubled (tripled, ...) already, we need to correct this. > > Semantically it's clearer to divide the target clock to get the fdi > dotclock instead of multiplying the bw, so do that instead. > > Note that the target clock is already multiplied by the same factor, > so the division will never loose accuracy for the M/N computation. > > The lane computation otoh used the wrong value, we also need to feed > the fdi dotclock to that. > > Split out on a request from Paulo Zanoni. > > v2: Also fix the lane computation. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Do we ever test the pixel_multiplier case? Does it work at all? Why is intel_ddi_get_config the only get_config function that has "pipe_config->pixel_multiplier = 1"? If we assigned "1" to everybody that don't need other values we would be able to remove many of those "if (pixel_multiplier > 1)" checks. Anyway, the patch seems to do what it says, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>. > --- > drivers/gpu/drm/i915/intel_display.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a29295e..761254d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, > { > struct drm_device *dev = intel_crtc->base.dev; > struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode; > - int target_clock, lane, link_bw; > + int target_clock, lane, link_bw, fdi_dotclock; > bool setup_ok, needs_recompute = false; > > retry: > @@ -4010,14 +4010,16 @@ retry: > else > target_clock = adjusted_mode->clock; > > - lane = ironlake_get_lanes_required(target_clock, link_bw, > + fdi_dotclock = target_clock; > + if (pipe_config->pixel_multiplier > 1) > + fdi_dotclock /= pipe_config->pixel_multiplier; > + > + lane = ironlake_get_lanes_required(fdi_dotclock, link_bw, > pipe_config->pipe_bpp); > > pipe_config->fdi_lanes = lane; > > - if (pipe_config->pixel_multiplier > 1) > - link_bw *= pipe_config->pixel_multiplier; > - intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock, > + intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock, > link_bw, &pipe_config->fdi_m_n); > > setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev, > -- > 1.7.11.7 >
2013/6/3 Paulo Zanoni <przanoni@gmail.com>: > 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>: >> We currently mutliply the link_bw of the fdi link with the pixel >> multiplier, which is wrong: The FDI link doesn't suddenly grow more >> bandwidth. In reality the pixel mutliplication only happens in the PCH, >> before the pixels are fed into the port. >> >> But since we our code treats the uses the target clock after pixels >> are doubled (tripled, ...) already, we need to correct this. >> >> Semantically it's clearer to divide the target clock to get the fdi >> dotclock instead of multiplying the bw, so do that instead. >> >> Note that the target clock is already multiplied by the same factor, >> so the division will never loose accuracy for the M/N computation. >> >> The lane computation otoh used the wrong value, we also need to feed >> the fdi dotclock to that. >> >> Split out on a request from Paulo Zanoni. >> >> v2: Also fix the lane computation. >> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Do we ever test the pixel_multiplier case? Does it work at all? > > Why is intel_ddi_get_config the only get_config function that has > "pipe_config->pixel_multiplier = 1"? If we assigned "1" to everybody > that don't need other values we would be able to remove many of those > "if (pixel_multiplier > 1)" checks. > > Anyway, the patch seems to do what it says, so: Reviewed-by: Paulo > Zanoni <paulo.r.zanoni@intel.com>. Actually I noticed this patch is different from the one I reviewed on Friday. Last Friday's patch: 1) ironlake_get_lanes_required(target_clock, etc) 2) fdi_dotclock = target_clock 3) if (pixel_multiplier > 1) etc 4) intel_link_compute_m_n(fdi_dotclock) Today's patch: 1) fdi_dotclock = target_clock 2) if (pixel_multiplier > 1) etc 3) ironlake_get_lanes_required(fdi_dotclock) 4) intel_link_compute_m_n(fdi_dotclock) The difference is the argument used in ironlake_get_lanes_required. Is this intentional? > >> --- >> drivers/gpu/drm/i915/intel_display.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index a29295e..761254d 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, >> { >> struct drm_device *dev = intel_crtc->base.dev; >> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode; >> - int target_clock, lane, link_bw; >> + int target_clock, lane, link_bw, fdi_dotclock; >> bool setup_ok, needs_recompute = false; >> >> retry: >> @@ -4010,14 +4010,16 @@ retry: >> else >> target_clock = adjusted_mode->clock; >> >> - lane = ironlake_get_lanes_required(target_clock, link_bw, >> + fdi_dotclock = target_clock; >> + if (pipe_config->pixel_multiplier > 1) >> + fdi_dotclock /= pipe_config->pixel_multiplier; >> + >> + lane = ironlake_get_lanes_required(fdi_dotclock, link_bw, >> pipe_config->pipe_bpp); >> >> pipe_config->fdi_lanes = lane; >> >> - if (pipe_config->pixel_multiplier > 1) >> - link_bw *= pipe_config->pixel_multiplier; >> - intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock, >> + intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock, >> link_bw, &pipe_config->fdi_m_n); >> >> setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev, >> -- >> 1.7.11.7 >> > > > > -- > Paulo Zanoni
On Mon, Jun 03, 2013 at 01:39:27PM -0300, Paulo Zanoni wrote: > 2013/6/3 Paulo Zanoni <przanoni@gmail.com>: > > 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>: > >> We currently mutliply the link_bw of the fdi link with the pixel > >> multiplier, which is wrong: The FDI link doesn't suddenly grow more > >> bandwidth. In reality the pixel mutliplication only happens in the PCH, > >> before the pixels are fed into the port. > >> > >> But since we our code treats the uses the target clock after pixels > >> are doubled (tripled, ...) already, we need to correct this. > >> > >> Semantically it's clearer to divide the target clock to get the fdi > >> dotclock instead of multiplying the bw, so do that instead. > >> > >> Note that the target clock is already multiplied by the same factor, > >> so the division will never loose accuracy for the M/N computation. > >> > >> The lane computation otoh used the wrong value, we also need to feed > >> the fdi dotclock to that. > >> > >> Split out on a request from Paulo Zanoni. > >> > >> v2: Also fix the lane computation. > >> > >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Do we ever test the pixel_multiplier case? Does it work at all? > > > > Why is intel_ddi_get_config the only get_config function that has > > "pipe_config->pixel_multiplier = 1"? If we assigned "1" to everybody > > that don't need other values we would be able to remove many of those > > "if (pixel_multiplier > 1)" checks. > > > > Anyway, the patch seems to do what it says, so: Reviewed-by: Paulo > > Zanoni <paulo.r.zanoni@intel.com>. > > Actually I noticed this patch is different from the one I reviewed on Friday. > > Last Friday's patch: > > 1) ironlake_get_lanes_required(target_clock, etc) > 2) fdi_dotclock = target_clock > 3) if (pixel_multiplier > 1) etc > 4) intel_link_compute_m_n(fdi_dotclock) > > Today's patch: > 1) fdi_dotclock = target_clock > 2) if (pixel_multiplier > 1) etc > 3) ironlake_get_lanes_required(fdi_dotclock) > 4) intel_link_compute_m_n(fdi_dotclock) > > The difference is the argument used in ironlake_get_lanes_required. Is > this intentional? Yep, see the commit message: "v2: Also fix the lane computation." I might need to be a notch more verbose here ... > > > > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 12 +++++++----- > >> 1 file changed, 7 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index a29295e..761254d 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, > >> { > >> struct drm_device *dev = intel_crtc->base.dev; > >> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode; > >> - int target_clock, lane, link_bw; > >> + int target_clock, lane, link_bw, fdi_dotclock; > >> bool setup_ok, needs_recompute = false; > >> > >> retry: > >> @@ -4010,14 +4010,16 @@ retry: > >> else > >> target_clock = adjusted_mode->clock; > >> > >> - lane = ironlake_get_lanes_required(target_clock, link_bw, > >> + fdi_dotclock = target_clock; > >> + if (pipe_config->pixel_multiplier > 1) > >> + fdi_dotclock /= pipe_config->pixel_multiplier; > >> + > >> + lane = ironlake_get_lanes_required(fdi_dotclock, link_bw, > >> pipe_config->pipe_bpp); > >> > >> pipe_config->fdi_lanes = lane; > >> > >> - if (pipe_config->pixel_multiplier > 1) > >> - link_bw *= pipe_config->pixel_multiplier; > >> - intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock, > >> + intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock, > >> link_bw, &pipe_config->fdi_m_n); > >> > >> setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev, > >> -- > >> 1.7.11.7 > >> > > > > > > > > -- > > Paulo Zanoni > > > > -- > Paulo Zanoni
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a29295e..761254d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc, { struct drm_device *dev = intel_crtc->base.dev; struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode; - int target_clock, lane, link_bw; + int target_clock, lane, link_bw, fdi_dotclock; bool setup_ok, needs_recompute = false; retry: @@ -4010,14 +4010,16 @@ retry: else target_clock = adjusted_mode->clock; - lane = ironlake_get_lanes_required(target_clock, link_bw, + fdi_dotclock = target_clock; + if (pipe_config->pixel_multiplier > 1) + fdi_dotclock /= pipe_config->pixel_multiplier; + + lane = ironlake_get_lanes_required(fdi_dotclock, link_bw, pipe_config->pipe_bpp); pipe_config->fdi_lanes = lane; - if (pipe_config->pixel_multiplier > 1) - link_bw *= pipe_config->pixel_multiplier; - intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock, + intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock, link_bw, &pipe_config->fdi_m_n); setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev,
We currently mutliply the link_bw of the fdi link with the pixel multiplier, which is wrong: The FDI link doesn't suddenly grow more bandwidth. In reality the pixel mutliplication only happens in the PCH, before the pixels are fed into the port. But since we our code treats the uses the target clock after pixels are doubled (tripled, ...) already, we need to correct this. Semantically it's clearer to divide the target clock to get the fdi dotclock instead of multiplying the bw, so do that instead. Note that the target clock is already multiplied by the same factor, so the division will never loose accuracy for the M/N computation. The lane computation otoh used the wrong value, we also need to feed the fdi dotclock to that. Split out on a request from Paulo Zanoni. v2: Also fix the lane computation. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)