Re: [PATCH 3/6] drm/i915: allow re-use BIOS connector config for initial fbdev config v2

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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_framebuffer()
> > >         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 <jbarnes@virtuousgeek.org>
> > > ---
> > >  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/i915/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_crtc *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_mode = {
> > >  		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_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/i915/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 = intel_crtc->lut_b[regno] << 8;
> > >  }
> > >  
> > > +static struct drm_fb_helper_crtc *
> > > +intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < fb_helper->crtc_count; i++)
> > > +		if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
> > > +			return &fb_helper->crtc_info[i];
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > > +/*
> > > + * Try to read the BIOS display configuration and use it for the initial
> > > + * fb configuration.
> > > + *
> > > + * 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_framebuffer()
> > > + *         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
> > > + *
> > > + * Note that we don't make special consideration whether we could actually
> > > + * switch to the selected modes without a full modeset. E.g. when the display
> > > + * is in VGA mode we need to recalculate watermarks and set a new high-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 = 0; i < fb_helper->connector_count; i++) {
> > > +		struct drm_connector *connector;
> > > +		struct drm_encoder *encoder;
> > > +		struct drm_fb_helper_crtc *new_crtc;
> > > +
> > > +		connector = fb_helper->connector_info[i]->connector;
> > > +		if (!enabled[i]) {
> > > +			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
> > > +				      connector->base.id);
> > > +			continue;
> > > +		}
> > > +
> > > +		encoder = connector->encoder;
> > > +		if (!encoder || !encoder->crtc) {
> > 
> > You've missed the encoder->crtc change here from my review. I've converted
> > 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= cmdline options
> 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=n that'll break, too. But
> another issue to resolve that.

BTW another funky issue I noticed about the video= 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älä
Intel OTC