On Mon, Apr 29, 2013 at 05:59:53PM +0300, Imre Deak wrote:
> On Mon, 2013-04-29 at 16:43 +0200, Daniel Vetter wrote:
> > On Mon, Apr 29, 2013 at 05:02:20PM +0300, Imre Deak wrote:
> > > On Fri, 2013-04-19 at 11:24 +0200, Daniel Vetter wrote:
[snip]
> > > > @@ -7714,10 +7733,23 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > > > }
> > > > }
> > > >
> > > > - if (!(intel_crtc_compute_config(crtc, pipe_config))) {
> > > > + ret = intel_crtc_compute_config(crtc, pipe_config);
> > > > + if (ret < 0) {
> > > > DRM_DEBUG_KMS("CRTC fixup failed\n");
> > > > goto fail;
> > > > }
> > > > +
> > > > + if (ret == RETRY) {
> > > > + if (WARN(!retry, "loop in pipe configuration computation\n")) {
> > > > + ret = -EINVAL;
> > > > + goto fail;
> > >
> > > Isn't it possible that intel_dp_compute_config increases pipe_bpp when
> > > it forces pipe_bpp to what the firmware has set? In that case could hit
> > > this WARN.
> >
> > Yeah, but that would mean that the firmware asks us for a configuration
> > for which we simply do not have enough bandwidth. So I don't think that
> > we'll actually hit this in reality.
> >
> > If we do I guess we need to add a check into intel_dp_compute_config to
> > see whether we're in the a) retry loop and b) try to increase bpp and then
> > just fail the compute_config stage. But as long as the entire eDP mess is
> > a bit unclear I'd like to avoid such complexity until we have a proven
> > need.
> >
> > Want me to add something like the above to the commit message?
>
> Ok, after the IRC discussion things are clearer now. Yea, would be good
> to have have it in the commit message that eDP is a non-issue. With
> that:
>
> Reviewed-by: Imre Deak <imre.deak at intel.com>
Ok, slurped in the entire series, with this patch's commit message amended
to reflect our discussion.
Thanks for the review, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
