[Intel-gfx] [PATCH 5/6] drm/i915: allow re-use BIOS connector config for initial fbdev config

[Daniel Vetter]

On Fri, Feb 07, 2014 at 12:10:39PM -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.
> 
> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>

Bunch of comments below.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c |   91 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index fb07ba6..8ce3405 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -246,6 +246,97 @@ 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

I'd shovel this great sequence overview into the commit message - too high
a chance it'll grow stale without getting updated. And git blame on our
intial_config can always bring it up again.

> + *
> + * If the BIOS or boot loader leaves the display in VGA mode, there's not
> + * much we can do; switching out of that mode involves allocating a new,
> + * high res buffer, and also recalculating bandwidth requirements for the
> + * new bpp configuration.

This sounds like a comment for fastboot, so feels a bit misplaced here.
Maybe a simple

"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."

instead? Feel free to bikeshed a bit more ...

> + */
> +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;
> +
> +	for (i = 0; i < fb_helper->connector_count; i++) {
> +		struct drm_connector *connector;
> +		struct drm_encoder *encoder;
> +
> +		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;
> +		}
> +
> +		if (WARN_ON(!encoder->crtc->enabled)) {
> +			DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n",
> +				      drm_get_connector_name(connector),
> +				      encoder->crtc->base.id);
> +			return false;
> +		}
> +
> +		if (!to_intel_crtc(encoder->crtc)->active) {
> +			DRM_DEBUG_KMS("connector %s on inactive crtc %d, borting\n",
> +				      drm_get_connector_name(connector),
> +				      encoder->crtc->base.id);
> +			return false;
> +		}

The above two crtc->enabled/active state checks are redundant same for
checking encoder->crtc, the hw state santizer we run at boot will make
sure that if connector->encoder is set, everything else is valid (and
enabled), too. So either replace them with BUG_ONs or just remove it.

> +
> +		modes[i] = &encoder->crtc->mode;

This confused me a bit until I've realized that our fastboot code smashes
the adjusted mode into crtc->mode. I think a comment here would be good:

/*
 * 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.
 */

Another thing: I expect this to fall over with fastboot=0, since then
crtc->mode isn't pre-filled. We might want to call
intel_crtc_mode_from_pipe_config directly to avoid this.

> +		crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);

You need to check here whether the crtc is already in use - the bios uses
a lot more cloning configurations than we do, so if we suggest such a
cloned config the resulting errors might be surprising. Having a 1:1
connector:crtc connection is the easiest option and also what we're
currently using.

> +
> +		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 = {
>  	.gamma_set = intel_crtc_fb_gamma_set,
>  	.gamma_get = intel_crtc_fb_gamma_get,
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch