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@xxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
