On Mon, Feb 11, 2013 at 7:25 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> While doing the modeset rework for drm/i915 I've noticed that the fb
> helper is very liberal with the semantics of the ->set_config
> interface:
> - It doesn't bother clearing stale modes (e.g. when unplugging a
> screen).
> - It unconditionally sets the fb, even if no mode will be set on a
> given crtc.
> - The initial setup is a bit fun since we need to pick crtcs to decide
> the desired fb size, but also should set the modeset->fb pointer.
> Explain what's going on in the fixup code after the fb is allocated.
>
> The crtc helper didn't really care, but the new i915 modeset
> infrastructure did, so I've had to add a bunch of special-cases to
> catch this.
>
> Fix this all up and enforce the interface by converting the checks in
> drm/i915/intel_display.c to BUG_ONs.
>
> v2: Fix commit message spell fail spotted by Rob Clark.
Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 27 +++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_display.c | 11 +++--------
> 2 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d841b68..809ef99 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -689,7 +689,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
> struct drm_fb_helper *fb_helper = info->par;
> struct drm_device *dev = fb_helper->dev;
> struct fb_var_screeninfo *var = &info->var;
> - struct drm_crtc *crtc;
> int ret;
> int i;
>
> @@ -700,7 +699,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
>
> drm_modeset_lock_all(dev);
> for (i = 0; i < fb_helper->crtc_count; i++) {
> - crtc = fb_helper->crtc_info[i].mode_set.crtc;
> ret = drm_mode_set_config_internal(&fb_helper->crtc_info[i].mode_set);
> if (ret) {
> drm_modeset_unlock_all(dev);
> @@ -841,9 +839,17 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>
> info = fb_helper->fbdev;
>
> - /* set the fb pointer */
> + /*
> + * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
> + * events, but at init time drm_setup_crtcs needs to be called before
> + * the fb is allocated (since we need to figure out the desired size of
> + * the fb before we can allocate it ...). Hence we need to fix things up
> + * here again.
> + */
> for (i = 0; i < fb_helper->crtc_count; i++)
> - fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
> + if (fb_helper->crtc_info[i].mode_set.num_connectors)
> + fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
> +
>
> if (new_fb) {
> info->var.pixclock = 0;
> @@ -1314,6 +1320,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> for (i = 0; i < fb_helper->crtc_count; i++) {
> modeset = &fb_helper->crtc_info[i].mode_set;
> modeset->num_connectors = 0;
> + modeset->fb = NULL;
> }
>
> for (i = 0; i < fb_helper->connector_count; i++) {
> @@ -1330,9 +1337,21 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> modeset->mode = drm_mode_duplicate(dev,
> fb_crtc->desired_mode);
> modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
> + modeset->fb = fb_helper->fb;
> }
> }
>
> + /* Clear out any old modes if there are no more connected outputs. */
> + for (i = 0; i < fb_helper->crtc_count; i++) {
> + modeset = &fb_helper->crtc_info[i].mode_set;
> + if (modeset->num_connectors == 0) {
> + BUG_ON(modeset->fb);
> + BUG_ON(modeset->num_connectors);
> + if (modeset->mode)
> + drm_mode_destroy(dev, modeset->mode);
> + modeset->mode = NULL;
> + }
> + }
> out:
> kfree(crtcs);
> kfree(modes);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 24f2654..ca8d592 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7978,14 +7978,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> BUG_ON(!set->crtc);
> BUG_ON(!set->crtc->helper_private);
>
> - if (!set->mode)
> - set->fb = NULL;
> -
> - /* The fb helper likes to play gross jokes with ->mode_set_config.
> - * Unfortunately the crtc helper doesn't do much at all for this case,
> - * so we have to cope with this madness until the fb helper is fixed up. */
> - if (set->fb && set->num_connectors == 0)
> - return 0;
> + /* Enforce sane interface api - has been abused by the fb helper. */
> + BUG_ON(!set->mode && set->fb);
> + BUG_ON(set->fb && set->num_connectors == 0);
>
> if (set->fb) {
> DRM_DEBUG_KMS("[CRTC:%d] [FB:%d] #connectors=%d (x y) (%i %i)\n",
> --
> 1.7.10.4
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
