From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 3/6] drm/i915: allow re-use BIOS connector config for initial fbdev config v2 Date: Wed, 12 Feb 2014 14:45:03 +0200 Message-ID: <20140212124503.GE3891@intel.com> References: <1392161341-6881-1-git-send-email-jbarnes@virtuousgeek.org> <1392161341-6881-3-git-send-email-jbarnes@virtuousgeek.org> <20140212091939.GS17001@phenom.ffwll.local> <20140212120428.GT17001@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id D6948FA97E for ; Wed, 12 Feb 2014 04:45:07 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140212120428.GT17001@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 Wed, Feb 12, 2014 at 01:04:28PM +0100, Daniel Vetter wrote: > On Wed, Feb 12, 2014 at 10:19:39AM +0100, Daniel Vetter wrote: > > On Tue, Feb 11, 2014 at 03:28:58PM -0800, Jesse Barnes wrote: > > > The BIOS or boot loader will generally create an initial display > > > configuration for us that includes some set of active pipes and > > > displays. This routine tries to figure out which pipes and connectors > > > are active and stuffs them into the crtcs and modes array given to us= by > > > the drm_fb_helper code. > > > = > > > The overall sequence is: > > > intel_fbdev_init - from driver load > > > intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data > > > drm_fb_helper_init - build fb helper structs > > > drm_fb_helper_single_add_all_connectors - more fb helper structs > > > intel_fbdev_initial_config - apply the config > > > drm_fb_helper_initial_config - call ->probe then register_framebu= ffer() > > > drm_setup_crtcs - build crtc config for fbdev > > > intel_fb_initial_config - find active connectors etc > > > drm_fb_helper_single_fb_probe - set up fbdev > > > intelfb_create - re-use or alloc fb, build out fbdev structs > > > = > > > v2: use BIOS connector config unconditionally if possible (Daniel) > > > check for crtc cloning and reject (Daniel) > > > fix up comments (Daniel) > > > = > > > Signed-off-by: Jesse Barnes > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 10 ++-- > > > drivers/gpu/drm/i915/intel_fbdev.c | 103 ++++++++++++++++++++++++= ++++++++++ > > > 2 files changed, 107 insertions(+), 6 deletions(-) > > > = > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i= 915/intel_display.c > > > index e800085..dea995d 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -51,7 +51,10 @@ static void ironlake_pch_clock_get(struct intel_cr= tc *crtc, > > > = > > > static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_= mode *mode, > > > int x, int y, struct drm_framebuffer *old_fb); > > > - > > > +static int intel_framebuffer_init(struct drm_device *dev, > > > + struct intel_framebuffer *ifb, > > > + struct drm_mode_fb_cmd2 *mode_cmd, > > > + struct drm_i915_gem_object *obj); > > > = > > > typedef struct { > > > int min, max; > > > @@ -7692,11 +7695,6 @@ static struct drm_display_mode load_detect_mod= e =3D { > > > 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MO= DE_FLAG_NVSYNC), > > > }; > > > = > > > -static int intel_framebuffer_init(struct drm_device *dev, > > > - struct intel_framebuffer *ifb, > > > - struct drm_mode_fb_cmd2 *mode_cmd, > > > - struct drm_i915_gem_object *obj); > > > - > > > struct drm_framebuffer * > > > __intel_framebuffer_create(struct drm_device *dev, > > > struct drm_mode_fb_cmd2 *mode_cmd, > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i91= 5/intel_fbdev.c > > > index cf46273..2a0badfc 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > @@ -242,7 +242,110 @@ static void intel_crtc_fb_gamma_get(struct drm_= crtc *crtc, u16 *red, u16 *green, > > > *blue =3D intel_crtc->lut_b[regno] << 8; > > > } > > > = > > > +static struct drm_fb_helper_crtc * > > > +intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crt= c *crtc) > > > +{ > > > + int i; > > > + > > > + for (i =3D 0; i < fb_helper->crtc_count; i++) > > > + if (fb_helper->crtc_info[i].mode_set.crtc =3D=3D crtc) > > > + return &fb_helper->crtc_info[i]; > > > + > > > + return NULL; > > > +} > > > + > > > +/* > > > + * Try to read the BIOS display configuration and use it for the ini= tial > > > + * fb configuration. > > > + * > > > + * The BIOS or boot loader will generally create an initial display > > > + * configuration for us that includes some set of active pipes and d= isplays. > > > + * This routine tries to figure out which pipes and connectors are a= ctive > > > + * and stuffs them into the crtcs and modes array given to us by the > > > + * drm_fb_helper code. > > > + * > > > + * The overall sequence is: > > > + * intel_fbdev_init - from driver load > > > + * intel_fbdev_init_bios - initialize the intel_fbdev using BIOS= data > > > + * drm_fb_helper_init - build fb helper structs > > > + * drm_fb_helper_single_add_all_connectors - more fb helper stru= cts > > > + * intel_fbdev_initial_config - apply the config > > > + * drm_fb_helper_initial_config - call ->probe then register_fra= mebuffer() > > > + * drm_setup_crtcs - build crtc config for fbdev > > > + * intel_fb_initial_config - find active connectors etc > > > + * drm_fb_helper_single_fb_probe - set up fbdev > > > + * intelfb_create - re-use or alloc fb, build out fbdev st= ructs > > > + * > > > + * Note that we don't make special consideration whether we could ac= tually > > > + * switch to the selected modes without a full modeset. E.g. when th= e display > > > + * is in VGA mode we need to recalculate watermarks and set a new hi= gh-res > > > + * framebuffer anyway. > > > + */ > > > +static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper, > > > + struct drm_fb_helper_crtc **crtcs, > > > + struct drm_display_mode **modes, > > > + bool *enabled, int width, int height) > > > +{ > > > + int i, j; > > > + > > > + for (i =3D 0; i < fb_helper->connector_count; i++) { > > > + struct drm_connector *connector; > > > + struct drm_encoder *encoder; > > > + struct drm_fb_helper_crtc *new_crtc; > > > + > > > + connector =3D fb_helper->connector_info[i]->connector; > > > + if (!enabled[i]) { > > > + DRM_DEBUG_KMS("connector %d not enabled, skipping\n", > > > + connector->base.id); > > > + continue; > > > + } > > > + > > > + encoder =3D connector->encoder; > > > + if (!encoder || !encoder->crtc) { > > = > > You've missed the encoder->crtc change here from my review. I've conver= ted > > it into a WARN_ON since our state sanitizer shouldn't let anything like > > this get through. > > = > > Otherwise merged the first 3 patches in this series to dinq. > = > Ville complained on irc that his box now bots into a 640x480 console. So I > think we need to add another check to compare the selected mode with the > preferred mode which fbcon would have picke to make sure that htotal and > vtotal match. Timings can obviously differe a bit. > = > Another regression is that now we don't parse/obey video=3D cmdline optio= ns > any more. This is fairly important since the enable/disable flags won't be > parsed any more so will upset Alan and his T100 again ;-) That itself is a > bit a design goof-up since with CONFIG_FBDEV=3Dn that'll break, too. But > another issue to resolve that. BTW another funky issue I noticed about the video=3D option is that if you specify a mode that's not part of the set of modes added by drm_add_modes_noedid(), then fbcon will happily use the specified mode, but when X starts the mode gets pruned from the mode list. It would seem better to keep the user specified mode on the list, no matter what. -- = Ville Syrj=E4l=E4 Intel OTC