diff mbox

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

Message ID 1392161341-6881-3-git-send-email-jbarnes@virtuousgeek.org (mailing list archive)
State New, archived
Headers show

Commit Message

Jesse Barnes Feb. 11, 2014, 11:28 p.m. UTC
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(-)

Comments

Daniel Vetter Feb. 12, 2014, 9:19 a.m. UTC | #1
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.
-Daniel

> +			DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
> +				      connector->base.id);
> +			enabled[i] = false;
> +			continue;
> +		}
> +
> +		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> +
> +		/*
> +		 * Make sure we're not trying to drive multiple connectors
> +		 * with a single CRTC, since our cloning support may not
> +		 * match the BIOS.
> +		 */
> +		for (j = 0; j < fb_helper->connector_count; j++) {
> +			if (crtcs[j] == new_crtc)
> +				return false;
> +		}
> +
> +		/*
> +		 * IMPORTANT: We want to use the adjusted mode (i.e. after the
> +		 * panel fitter upscaling) as the initial config, not the input
> +		 * mode, which is what crtc->mode usually contains. But since
> +		 * our current fastboot code puts a mode derived from the
> +		 * post-pfit timings into crtc->mode this works out correctly.
> +		 * We don't use hwmode anywhere right now, so use it for this
> +		 * since the fb helper layer wants a pointer to something
> +		 * we own.
> +		 */
> +		intel_mode_from_pipe_config(&encoder->crtc->hwmode,
> +					    &to_intel_crtc(encoder->crtc)->config);
> +		modes[i] = &encoder->crtc->hwmode;
> +		crtcs[i] = new_crtc;
> +
> +		DRM_DEBUG_KMS("connector %s on crtc %d: %s\n",
> +			      drm_get_connector_name(connector),
> +			      encoder->crtc->base.id,
> +			      modes[i]->name);
> +	}
> +
> +	return true;
> +}
> +
>  static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> +	.initial_config = intel_fb_initial_config,
>  	.gamma_set = intel_crtc_fb_gamma_set,
>  	.gamma_get = intel_crtc_fb_gamma_get,
>  	.fb_probe = intelfb_create,
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Feb. 12, 2014, 12:04 p.m. UTC | #2
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.

I haven't looked at the details of how to best solve this, but I guess we
could move the call to driver->initial_config down after all the fbdev
parsing has happened already. Then the driver just fixes things up. The
i915 initial_config logic will be a bit more tricky, but I think it should
work out.

I've dropped this patch for now until we figure this out.
-Daniel
Ville Syrjälä Feb. 12, 2014, 12:45 p.m. UTC | #3
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.
Daniel Vetter Feb. 12, 2014, 1:15 p.m. UTC | #4
On Wed, Feb 12, 2014 at 1:45 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> 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.

Yeah, I think we need to move the cmdline parsing out of the fbdev
helper into crtc helpers so that this kind of stuff Just Works. Then
we could also add the cmdline mode as the first preferred one.
-Daniel
Chris Wilson Feb. 12, 2014, 1:19 p.m. UTC | #5
On Wed, Feb 12, 2014 at 02:15:18PM +0100, Daniel Vetter wrote:
> On Wed, Feb 12, 2014 at 1:45 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > 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.
> 
> Yeah, I think we need to move the cmdline parsing out of the fbdev
> helper into crtc helpers so that this kind of stuff Just Works. Then
> we could also add the cmdline mode as the first preferred one.

fyi, https://bugs.freedesktop.org/show_bug.cgi?id=73154
-Chris
diff mbox

Patch

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) {
+			DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
+				      connector->base.id);
+			enabled[i] = false;
+			continue;
+		}
+
+		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
+
+		/*
+		 * Make sure we're not trying to drive multiple connectors
+		 * with a single CRTC, since our cloning support may not
+		 * match the BIOS.
+		 */
+		for (j = 0; j < fb_helper->connector_count; j++) {
+			if (crtcs[j] == new_crtc)
+				return false;
+		}
+
+		/*
+		 * IMPORTANT: We want to use the adjusted mode (i.e. after the
+		 * panel fitter upscaling) as the initial config, not the input
+		 * mode, which is what crtc->mode usually contains. But since
+		 * our current fastboot code puts a mode derived from the
+		 * post-pfit timings into crtc->mode this works out correctly.
+		 * We don't use hwmode anywhere right now, so use it for this
+		 * since the fb helper layer wants a pointer to something
+		 * we own.
+		 */
+		intel_mode_from_pipe_config(&encoder->crtc->hwmode,
+					    &to_intel_crtc(encoder->crtc)->config);
+		modes[i] = &encoder->crtc->hwmode;
+		crtcs[i] = new_crtc;
+
+		DRM_DEBUG_KMS("connector %s on crtc %d: %s\n",
+			      drm_get_connector_name(connector),
+			      encoder->crtc->base.id,
+			      modes[i]->name);
+	}
+
+	return true;
+}
+
 static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
+	.initial_config = intel_fb_initial_config,
 	.gamma_set = intel_crtc_fb_gamma_set,
 	.gamma_get = intel_crtc_fb_gamma_get,
 	.fb_probe = intelfb_create,