From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 01/76] drm/fb-helper: don't clobber output routing in setup_crtcs Date: Wed, 29 Aug 2012 09:57:46 -0700 Message-ID: <20120829095746.0513cc6c@jbarnes-x220> References: <1343328581-2324-1-git-send-email-daniel.vetter@ffwll.ch> <1343328581-2324-2-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy8-pub.bluehost.com (oproxy8-pub.bluehost.com [69.89.22.20]) by gabe.freedesktop.org (Postfix) with SMTP id 3C0059EF0E for ; Wed, 29 Aug 2012 09:57:50 -0700 (PDT) In-Reply-To: <1343328581-2324-2-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, 26 Jul 2012 20:48:26 +0200 Daniel Vetter wrote: > Yet again the too close relationship between the fb helper and the > crtc helper code strikes. This time around the fb helper resets all > encoder->crtc pointers to NULL before starting to set up it's own > mode. Which is total bullocks, because this will clobber the existing > output routing, which the new drm/i915 code depends upon to be > absolutely correct. > > The crtc helper itself doesn't really care about that, since it > disables unused encoders in a rather roundabout way. > > Two places call drm_setup_crts: > > - For the initial fb config. I've auditted all current drivers, and > they all allocate their encoders with kzalloc. So there's no need to > clear encoder->crtc once more. > > - When processing hotplug events while showing the fb console. This > one is a bit more tricky, but both the crtc helper code and the new > drm/i915 modeset code disable encoders if their crtc is changed and > that encoder isn't part of the new config. Also, both disable any > disconnected outputs, too. > > Which only leaves encoders that are on, connected, but for some odd > reason the fb helper code doesn't want to use. That would be a bug > in the fb configuration selector, since it tries to light up as many > outputs as possible. > > v2: Kill the now unused encoders variable. > > Signed-Off-by: Daniel Vetter > --- > drivers/gpu/drm/drm_fb_helper.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > b/drivers/gpu/drm/drm_fb_helper.c index f546d1e..4ecc869 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1230,7 +1230,6 @@ static void drm_setup_crtcs(struct > drm_fb_helper *fb_helper) struct drm_device *dev = fb_helper->dev; > struct drm_fb_helper_crtc **crtcs; > struct drm_display_mode **modes; > - struct drm_encoder *encoder; > struct drm_mode_set *modeset; > bool *enabled; > int width, height; > @@ -1241,11 +1240,6 @@ static void drm_setup_crtcs(struct > drm_fb_helper *fb_helper) width = dev->mode_config.max_width; > height = dev->mode_config.max_height; > > - /* clean out all the encoder/crtc combos */ > - list_for_each_entry(encoder, &dev->mode_config.encoder_list, > head) { > - encoder->crtc = NULL; > - } > - > crtcs = kcalloc(dev->mode_config.num_connector, > sizeof(struct drm_fb_helper_crtc *), > GFP_KERNEL); modes = kcalloc(dev->mode_config.num_connector, I remember running into this when doing the fastboot stuff, but I thought it had other side effects too (maybe hot plug). But that was probably some other bug I worked through in while testing, since in theory the above should be ok. All of this stuff will need lots of tested-bys, but this one has my Reviewed-by: Jesse Barnes fwiw.