[PATCH] drm/fb-helper: fixup set_config semantics

[Rob Clark]

On Mon, Feb 11, 2013 at 7:25 PM, Daniel Vetter <daniel.vetter at ffwll.ch> 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 at gmail.com>

>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  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
>