From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 3/5] drm/i915: retrieve current fb config into new plane_config structure at init v4 Date: Tue, 26 Nov 2013 10:11:23 -0800 Message-ID: <20131126101123.20e2324a@jbarnes-desktop> References: <1385423479-3821-1-git-send-email-jbarnes@virtuousgeek.org> <1385423479-3821-3-git-send-email-jbarnes@virtuousgeek.org> <20131126174353.GL27344@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from alt-proxy22.mail.unifiedlayer.com (alt-proxy22.mail.unifiedlayer.com [67.20.81.214]) by gabe.freedesktop.org (Postfix) with SMTP id 1C56AFB873 for ; Tue, 26 Nov 2013 10:10:07 -0800 (PST) In-Reply-To: <20131126174353.GL27344@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, 26 Nov 2013 18:43:53 +0100 Daniel Vetter wrote: > > + plane_config->fb->base.pitches[0] = > > + intel_framebuffer_pitch_for_width(dev_priv, > > + plane_config->fb->base.width, > > + plane_config->fb->base.bits_per_pixel, > > + plane_config->tiled); > > Shouldn't we read out the stride from the respective hw register, too? Hm that's quite an idea. > > > + > > + plane_config->size = ALIGN(plane_config->fb->base.pitches[0] * > > + plane_config->fb->base.height, PAGE_SIZE); > > If you bother with tiling I think you also need to bother with correct > size alignement. Yeah just fixed up the framebuffer size function per Ville's comments. I should be able to just use that. > > @@ -10562,6 +10672,7 @@ static void intel_init_display(struct drm_device *dev) > > dev_priv->display.update_plane = ironlake_update_plane; > > Sad Haswell is sad it seems. Yeah for two reasons: I'm not testing HSW at all, and I'm still fairly ignorant of HSW display. Well that and I don't want to pile on more work for this patchset which has already seen a bunch of churn... > > + if (dev_priv->display.get_plane_config) > > + dev_priv->display.get_plane_config(crtc, > > + &crtc->plane_config); > > + } > > If we go with Chris' suggestion to disable the crtc if we can't get at a > sane framebuffer for it then this would make much more sense in the > hardware state readout code. Especially since always having framebuffers > around would allow us to ditch a bit of special-casing code all over the > place. I don't think so; reading out and allocating an fb on every plane config readout would be overkill. We'd need to poke around in the struct for cross checking, then free it somewhere. Plus it won't always be preallocated, and creating a duplicate fb when the object still exists would fail. This is actually another argument for simply duplicating a few fields from the fb struct into the plane config struct. That makes cross checking and readout simple, and allows us to allocate if possible in the BIOS functions. But damnit, then I'd have to drop back to an earlier version of the patch and lose some changes... ugg -- Jesse Barnes, Intel Open Source Technology Center