All of lore.kernel.org
 help / color / mirror / Atom feed
From: Javier Martinez Canillas <javierm@redhat.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: linux-fbdev@vger.kernel.org,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Du Cheng <ducheng2@gmail.com>,
	Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Claudio Suarez <cssk@net-c.es>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH v2 06/19] fbcon: Use delayed work for cursor
Date: Wed, 9 Feb 2022 00:59:23 +0100	[thread overview]
Message-ID: <c58f7b96-8d8c-030e-9fd2-358e259127e4@redhat.com> (raw)
In-Reply-To: <20220208210824.2238981-7-daniel.vetter@ffwll.ch>

Hello Daniel,

On 2/8/22 22:08, Daniel Vetter wrote:
> Allows us to delete a bunch of hand-rolled stuff. Also to simplify the
> code we initialize the cursor_work completely when we allocate the
> fbcon_ops structure, instead of trying to cope with console
> re-initialization.
> 

Maybe also make it more explicit in the commit message that the delayed
work is replacing a timer that was used before for the cursor ?

> The motiviation here is that fbcon code stops using the fb_info.queue,

motivation

[snip]

>     /*
>      *    This is the interface between the low-level console driver and the
> @@ -68,7 +68,7 @@ struct fbcon_ops {
>  	int  (*update_start)(struct fb_info *info);
>  	int  (*rotate_font)(struct fb_info *info, struct vc_data *vc);
>  	struct fb_var_screeninfo var;  /* copy of the current fb_var_screeninfo */
> -	struct timer_list cursor_timer; /* Cursor timer */
> +	struct delayed_work cursor_work; /* Cursor timer */

A delayed_work uses a timer underneath but I wonder if the comment also
needs to be updated since technically isn't a timer anymore but deferred
work that gets re-scheduled each time on fb_flashcursor().

The patch looks good to me and makes the logic much simpler than before.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat


WARNING: multiple messages have this Message-ID (diff)
From: Javier Martinez Canillas <javierm@redhat.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: linux-fbdev@vger.kernel.org, Du Cheng <ducheng2@gmail.com>,
	Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Claudio Suarez <cssk@net-c.es>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH v2 06/19] fbcon: Use delayed work for cursor
Date: Wed, 9 Feb 2022 00:59:23 +0100	[thread overview]
Message-ID: <c58f7b96-8d8c-030e-9fd2-358e259127e4@redhat.com> (raw)
In-Reply-To: <20220208210824.2238981-7-daniel.vetter@ffwll.ch>

Hello Daniel,

On 2/8/22 22:08, Daniel Vetter wrote:
> Allows us to delete a bunch of hand-rolled stuff. Also to simplify the
> code we initialize the cursor_work completely when we allocate the
> fbcon_ops structure, instead of trying to cope with console
> re-initialization.
> 

Maybe also make it more explicit in the commit message that the delayed
work is replacing a timer that was used before for the cursor ?

> The motiviation here is that fbcon code stops using the fb_info.queue,

motivation

[snip]

>     /*
>      *    This is the interface between the low-level console driver and the
> @@ -68,7 +68,7 @@ struct fbcon_ops {
>  	int  (*update_start)(struct fb_info *info);
>  	int  (*rotate_font)(struct fb_info *info, struct vc_data *vc);
>  	struct fb_var_screeninfo var;  /* copy of the current fb_var_screeninfo */
> -	struct timer_list cursor_timer; /* Cursor timer */
> +	struct delayed_work cursor_work; /* Cursor timer */

A delayed_work uses a timer underneath but I wonder if the comment also
needs to be updated since technically isn't a timer anymore but deferred
work that gets re-scheduled each time on fb_flashcursor().

The patch looks good to me and makes the logic much simpler than before.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat


WARNING: multiple messages have this Message-ID (diff)
From: Javier Martinez Canillas <javierm@redhat.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: linux-fbdev@vger.kernel.org, Du Cheng <ducheng2@gmail.com>,
	Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [Intel-gfx] [PATCH v2 06/19] fbcon: Use delayed work for cursor
Date: Wed, 9 Feb 2022 00:59:23 +0100	[thread overview]
Message-ID: <c58f7b96-8d8c-030e-9fd2-358e259127e4@redhat.com> (raw)
In-Reply-To: <20220208210824.2238981-7-daniel.vetter@ffwll.ch>

Hello Daniel,

On 2/8/22 22:08, Daniel Vetter wrote:
> Allows us to delete a bunch of hand-rolled stuff. Also to simplify the
> code we initialize the cursor_work completely when we allocate the
> fbcon_ops structure, instead of trying to cope with console
> re-initialization.
> 

Maybe also make it more explicit in the commit message that the delayed
work is replacing a timer that was used before for the cursor ?

> The motiviation here is that fbcon code stops using the fb_info.queue,

motivation

[snip]

>     /*
>      *    This is the interface between the low-level console driver and the
> @@ -68,7 +68,7 @@ struct fbcon_ops {
>  	int  (*update_start)(struct fb_info *info);
>  	int  (*rotate_font)(struct fb_info *info, struct vc_data *vc);
>  	struct fb_var_screeninfo var;  /* copy of the current fb_var_screeninfo */
> -	struct timer_list cursor_timer; /* Cursor timer */
> +	struct delayed_work cursor_work; /* Cursor timer */

A delayed_work uses a timer underneath but I wonder if the comment also
needs to be updated since technically isn't a timer anymore but deferred
work that gets re-scheduled each time on fb_flashcursor().

The patch looks good to me and makes the logic much simpler than before.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat


  reply	other threads:[~2022-02-08 23:59 UTC|newest]

Thread overview: 145+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
2022-02-08 21:08 ` Daniel Vetter
2022-02-08 21:08 ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 01/19] fbcon: delete a few unneeded forward decl Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-10 11:17   ` Thomas Zimmermann
2022-02-10 11:17     ` [Intel-gfx] " Thomas Zimmermann
2022-02-10 11:17     ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 23:06   ` Javier Martinez Canillas
2022-02-08 23:06     ` [Intel-gfx] " Javier Martinez Canillas
2022-02-08 23:06     ` Javier Martinez Canillas
2022-02-10 11:17   ` Thomas Zimmermann
2022-02-10 11:17     ` [Intel-gfx] " Thomas Zimmermann
2022-02-10 11:17     ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 03/19] fbcon: Introduce wrapper for console->fb_info lookup Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-10 11:18   ` Thomas Zimmermann
2022-02-10 11:18     ` [Intel-gfx] " Thomas Zimmermann
2022-02-10 11:18     ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 04/19] fbcon: delete delayed loading code Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-10 11:20   ` Thomas Zimmermann
2022-02-10 11:20     ` [Intel-gfx] " Thomas Zimmermann
2022-02-10 11:20     ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 05/19] fbdev/sysfs: Fix locking Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-10 11:22   ` Thomas Zimmermann
2022-02-10 11:22     ` [Intel-gfx] " Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 06/19] fbcon: Use delayed work for cursor Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 23:59   ` Javier Martinez Canillas [this message]
2022-02-08 23:59     ` Javier Martinez Canillas
2022-02-08 23:59     ` Javier Martinez Canillas
2022-02-10 11:37   ` Thomas Zimmermann
2022-02-10 11:37     ` [Intel-gfx] " Thomas Zimmermann
2022-02-10 11:37     ` Thomas Zimmermann
2022-02-10 11:43   ` Tetsuo Handa
2022-02-10 11:43     ` [Intel-gfx] " Tetsuo Handa
2022-02-10 11:43     ` Tetsuo Handa
2022-04-05 20:54     ` Daniel Vetter
2022-04-05 20:54       ` Daniel Vetter
2022-04-05 20:54       ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 07/19] fbcon: Replace FBCON_FLAGS_INIT with a boolean Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 08/19] fb: Delete fb_info->queue Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-10 11:38   ` Thomas Zimmermann
2022-02-10 11:38     ` [Intel-gfx] " Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-10 11:46   ` Thomas Zimmermann
2022-02-10 11:46     ` [Intel-gfx] " Thomas Zimmermann
2022-02-10 11:46     ` Thomas Zimmermann
2022-04-05  8:45     ` Daniel Vetter
2022-04-05  8:45       ` Daniel Vetter
2022-04-05  8:45       ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 10/19] fbcon: Ditch error handling for con2fb_release_oldinfo Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-10 14:14   ` Thomas Zimmermann
2022-02-10 14:14     ` [Intel-gfx] " Thomas Zimmermann
2022-02-10 14:14     ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 11/19] fbcon: move more common code into fb_open() Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-10 14:16   ` Thomas Zimmermann
2022-02-10 14:16     ` [Intel-gfx] " Thomas Zimmermann
2022-02-10 14:16     ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 12/19] fbcon: use lock_fb_info in fbcon_open/release Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 13/19] fbcon: Consistently protect deferred_takeover with console_lock() Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 14/19] fbcon: Move console_lock for register/unlink/unregister Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 15/19] fbcon: Move more code into fbcon_release Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 16/19] fbcon: untangle fbcon_exit Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 17/19] fbcon: Maintain a private array of fb_info Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered" Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-09  0:19   ` Javier Martinez Canillas
2022-02-09  0:19     ` [Intel-gfx] " Javier Martinez Canillas
2022-02-09  0:19     ` Javier Martinez Canillas
2022-04-05  8:36     ` Daniel Vetter
2022-04-05  8:36       ` Daniel Vetter
2022-04-05  8:36       ` [Intel-gfx] " Daniel Vetter
2022-04-05  8:40       ` Daniel Vetter
2022-04-05  8:40         ` [Intel-gfx] " Daniel Vetter
2022-04-05  9:19         ` Javier Martinez Canillas
2022-04-05  9:19           ` [Intel-gfx] " Javier Martinez Canillas
2022-04-05  9:24           ` Daniel Vetter
2022-04-05  9:24             ` Daniel Vetter
2022-04-05  9:24             ` [Intel-gfx] " Daniel Vetter
2022-04-05  9:52             ` Javier Martinez Canillas
2022-04-05  9:52               ` Javier Martinez Canillas
2022-04-05  9:52               ` [Intel-gfx] " Javier Martinez Canillas
2022-04-05 10:34               ` Daniel Vetter
2022-04-05 10:34                 ` Daniel Vetter
2022-04-05 10:34                 ` [Intel-gfx] " Daniel Vetter
2022-04-05 13:24                 ` Geert Uytterhoeven
2022-04-05 13:24                   ` Geert Uytterhoeven
2022-04-05 13:24                   ` [Intel-gfx] " Geert Uytterhoeven
2022-04-05 13:33                   ` Greg KH
2022-04-05 13:33                     ` Greg KH
2022-04-05 13:33                     ` [Intel-gfx] " Greg KH
2022-04-05 16:12                     ` Daniel Vetter
2022-04-05 16:12                       ` Daniel Vetter
2022-04-05 16:12                       ` [Intel-gfx] " Daniel Vetter
2022-04-05 16:44                       ` Greg KH
2022-04-05 16:44                         ` [Intel-gfx] " Greg KH
2022-04-05 17:29                         ` Daniel Vetter
2022-04-05 17:29                           ` Daniel Vetter
2022-04-05 17:29                           ` [Intel-gfx] " Daniel Vetter
2022-04-07 17:26                           ` Greg KH
2022-04-07 17:26                             ` Greg KH
2022-04-07 17:26                             ` [Intel-gfx] " Greg KH
2022-04-05 13:25                 ` Javier Martinez Canillas
2022-04-05 13:25                   ` Javier Martinez Canillas
2022-04-05 13:25                   ` Javier Martinez Canillas
2022-02-08 21:08 ` [PATCH v2 19/19] fbdev: Make registered_fb[] private to fbmem.c Daniel Vetter
2022-02-08 21:08   ` [Intel-gfx] " Daniel Vetter
2022-02-08 21:08   ` Daniel Vetter
2022-02-08 23:15 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for fbcon patches, take two Patchwork
2022-02-08 23:18 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-02-08 23:50 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c58f7b96-8d8c-030e-9fd2-358e259127e4@redhat.com \
    --to=javierm@redhat.com \
    --cc=cssk@net-c.es \
    --cc=daniel.vetter@ffwll.ch \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=ducheng2@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=penguin-kernel@I-love.SAKURA.ne.jp \
    --cc=tzimmermann@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.