diff mbox

[13/16] drm/fb-helper: streamline drm_fb_helper_single_fb_probe

Message ID 1359044448-3861-14-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter Jan. 24, 2013, 4:20 p.m. UTC
No need to check whether we've allocated a new fb since we're not
always doing that. Also, we always need to register the fbdev and add
it to the panic notifier.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Comments

Rob Clark Feb. 12, 2013, 12:24 a.m. UTC | #1
On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> No need to check whether we've allocated a new fb since we're not
> always doing that. Also, we always need to register the fbdev and add
> it to the panic notifier.

hmm, how is this not leading to a register_framebuffer() on every hotplug event?

BR,
-R


>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c |   29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 90c1117..edbfcde 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -752,10 +752,14 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>  }
>  EXPORT_SYMBOL(drm_fb_helper_pan_display);
>
> +/*
> + * Allocates the backing storage through the ->fb_probe callback and then
> + * registers the fbdev and sets up the panic notifier.
> + */
>  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>                                          int preferred_bpp)
>  {
> -       int new_fb = 0;
> +       int ret = 0;
>         int crtc_count = 0;
>         int i;
>         struct fb_info *info;
> @@ -833,9 +837,9 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>         }
>
>         /* push down into drivers */
> -       new_fb = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
> -       if (new_fb < 0)
> -               return new_fb;
> +       ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
> +       if (ret < 0)
> +               return ret;
>
>         info = fb_helper->fbdev;
>
> @@ -851,15 +855,12 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>                         fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
>
>
> -       if (new_fb) {
> -               info->var.pixclock = 0;
> -               if (register_framebuffer(info) < 0)
> -                       return -EINVAL;
> -
> -               dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
> -                               info->node, info->fix.id);
> +       info->var.pixclock = 0;
> +       if (register_framebuffer(info) < 0)
> +               return -EINVAL;
>
> -       }
> +       dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
> +                       info->node, info->fix.id);
>
>         /* Switch back to kernel console on panic */
>         /* multi card linked list maybe */
> @@ -869,8 +870,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>                                                &paniced);
>                 register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
>         }
> -       if (new_fb)
> -               list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> +
> +       list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
>
>         return 0;
>  }
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Feb. 12, 2013, 12:33 a.m. UTC | #2
On Tue, Feb 12, 2013 at 1:24 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> No need to check whether we've allocated a new fb since we're not
>> always doing that. Also, we always need to register the fbdev and add
>> it to the panic notifier.
>
> hmm, how is this not leading to a register_framebuffer() on every hotplug event?

drm_fb_helper_hotplug_event calls drm_fb_helper_set_par directly, so
bypasses all the initial setup which I'm streamlining here. See
"drm/fb-helper: directly call set_par from the hotplug handler".
-Daniel
Rob Clark Feb. 12, 2013, 12:35 a.m. UTC | #3
On Mon, Feb 11, 2013 at 7:33 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Tue, Feb 12, 2013 at 1:24 AM, Rob Clark <robdclark@gmail.com> wrote:
>> On Thu, Jan 24, 2013 at 10:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> No need to check whether we've allocated a new fb since we're not
>>> always doing that. Also, we always need to register the fbdev and add
>>> it to the panic notifier.
>>
>> hmm, how is this not leading to a register_framebuffer() on every hotplug event?
>
> drm_fb_helper_hotplug_event calls drm_fb_helper_set_par directly, so
> bypasses all the initial setup which I'm streamlining here. See
> "drm/fb-helper: directly call set_par from the hotplug handler".

doh, that's the problem with reviewing long patchset without applying
them as I go..

ok, then looks good

Reviewed-by: Rob Clark <robdclark@gmail.com>

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

Patch

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 90c1117..edbfcde 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -752,10 +752,14 @@  int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
+/*
+ * Allocates the backing storage through the ->fb_probe callback and then
+ * registers the fbdev and sets up the panic notifier.
+ */
 static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					 int preferred_bpp)
 {
-	int new_fb = 0;
+	int ret = 0;
 	int crtc_count = 0;
 	int i;
 	struct fb_info *info;
@@ -833,9 +837,9 @@  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	}
 
 	/* push down into drivers */
-	new_fb = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
-	if (new_fb < 0)
-		return new_fb;
+	ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
+	if (ret < 0)
+		return ret;
 
 	info = fb_helper->fbdev;
 
@@ -851,15 +855,12 @@  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 			fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
 
 
-	if (new_fb) {
-		info->var.pixclock = 0;
-		if (register_framebuffer(info) < 0)
-			return -EINVAL;
-
-		dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
-				info->node, info->fix.id);
+	info->var.pixclock = 0;
+	if (register_framebuffer(info) < 0)
+		return -EINVAL;
 
-	}
+	dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
+			info->node, info->fix.id);
 
 	/* Switch back to kernel console on panic */
 	/* multi card linked list maybe */
@@ -869,8 +870,8 @@  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					       &paniced);
 		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
-	if (new_fb)
-		list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+
+	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
 
 	return 0;
 }