[v2,0/7] drm: drm_fb_helper cleanup.
mbox series

Message ID 20200302162151.52349-1-pankaj.laxminarayan.bharadiya@intel.com
Headers show
Series
  • drm: drm_fb_helper cleanup.
Related show

Message

Laxminarayan Bharadiya, Pankaj March 2, 2020, 4:21 p.m. UTC
This series addresses below drm_fb_helper tasks from
Documentation/gpu/todo.rst.

- The max connector argument for drm_fb_helper_init() isn't used
  anymore and can be removed.

- The helper doesn't keep an array of connectors anymore so these can
  be removed: drm_fb_helper_single_add_all_connectors(),
  drm_fb_helper_add_one_connector() and
  drm_fb_helper_remove_one_connector().

Changes since v1:
   - Squashed warning fixes into the patch that introduced the
     warnings (into 5/7) (Laurent)
   - Fixed reflow in in 9/9 (Laurent)

Pankaj Bharadiya (7):
  drm: Remove unused arg from drm_fb_helper_init
  drm/radeon: remove radeon_fb_{add,remove}_connector functions
  drm/amdgpu: Remove drm_fb_helper_{add,remove}_one_connector calls
  drm/i915/display: Remove drm_fb_helper_{add,remove}_one_connector
    calls
  drm: Remove drm_fb_helper add, add all and remove connector calls
  drm/fb-helper: Remove drm_fb_helper add, add_all and remove connector
    functions
  drm/todo: Update drm_fb_helper tasks

 Documentation/gpu/todo.rst                    |  7 -----
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c        |  5 +---
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 13 ---------
 drivers/gpu/drm/armada/armada_fbdev.c         |  8 +-----
 drivers/gpu/drm/bridge/tc358764.c             |  3 ---
 drivers/gpu/drm/drm_fb_helper.c               |  6 ++---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c       |  1 -
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c     | 10 +------
 drivers/gpu/drm/gma500/framebuffer.c          |  6 +----
 drivers/gpu/drm/i915/display/intel_dp_mst.c   | 12 ---------
 drivers/gpu/drm/i915/display/intel_fbdev.c    |  4 +--
 drivers/gpu/drm/msm/msm_fbdev.c               |  6 +----
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  7 -----
 drivers/gpu/drm/nouveau/nouveau_fbcon.c       |  6 +----
 drivers/gpu/drm/omapdrm/omap_fbdev.c          |  6 +----
 drivers/gpu/drm/radeon/radeon_dp_mst.c        | 10 -------
 drivers/gpu/drm/radeon/radeon_fb.c            | 19 +------------
 drivers/gpu/drm/radeon/radeon_mode.h          |  3 ---
 drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c |  9 +------
 drivers/gpu/drm/tegra/fb.c                    |  8 +-----
 include/drm/drm_fb_helper.h                   | 27 ++-----------------
 21 files changed, 15 insertions(+), 161 deletions(-)

Comments

Emil Velikov March 2, 2020, 6:21 p.m. UTC | #1
Hi Pankaj,

On Mon, 2 Mar 2020 at 16:33, Pankaj Bharadiya
<pankaj.laxminarayan.bharadiya@intel.com> wrote:
>
> This series addresses below drm_fb_helper tasks from
> Documentation/gpu/todo.rst.
>
> - The max connector argument for drm_fb_helper_init() isn't used
>   anymore and can be removed.
>
> - The helper doesn't keep an array of connectors anymore so these can
>   be removed: drm_fb_helper_single_add_all_connectors(),
>   drm_fb_helper_add_one_connector() and
>   drm_fb_helper_remove_one_connector().
>
> Changes since v1:
>    - Squashed warning fixes into the patch that introduced the
>      warnings (into 5/7) (Laurent)
>    - Fixed reflow in in 9/9 (Laurent)
>
For the future, include the changelog in the respective patches. This
makes it easier for reviewers...
Plus you're already changing the commit - might as well mention what/why :-)

Also do include the R-B, Acked-by, other tags accumulated up-to that
point, when sending new revision.


That said, if you're interested in further cleaning this up, one can
cleanup the drm_dp_mst_topology_cbs hooks.
In particular ::register_connector is identical across the board -
create a helper function using it directly in core, killing the hook.

While for ::destroy_connector - there's some amdgpu specific code in
there... which I'm not sure if it should stay or not.
To be on the save side - create a helper which will be called for
drivers where the hook is !=NULL (aka everyone but amdgpu).

HTH
Emil
Daniel Vetter March 2, 2020, 9:43 p.m. UTC | #2
On Mon, Mar 02, 2020 at 06:21:23PM +0000, Emil Velikov wrote:
> Hi Pankaj,
> 
> On Mon, 2 Mar 2020 at 16:33, Pankaj Bharadiya
> <pankaj.laxminarayan.bharadiya@intel.com> wrote:
> >
> > This series addresses below drm_fb_helper tasks from
> > Documentation/gpu/todo.rst.
> >
> > - The max connector argument for drm_fb_helper_init() isn't used
> >   anymore and can be removed.
> >
> > - The helper doesn't keep an array of connectors anymore so these can
> >   be removed: drm_fb_helper_single_add_all_connectors(),
> >   drm_fb_helper_add_one_connector() and
> >   drm_fb_helper_remove_one_connector().
> >
> > Changes since v1:
> >    - Squashed warning fixes into the patch that introduced the
> >      warnings (into 5/7) (Laurent)
> >    - Fixed reflow in in 9/9 (Laurent)
> >
> For the future, include the changelog in the respective patches. This
> makes it easier for reviewers...
> Plus you're already changing the commit - might as well mention what/why :-)
> 
> Also do include the R-B, Acked-by, other tags accumulated up-to that
> point, when sending new revision.

Yup, can you pls resend that entire pile with all the ack/review tags from
the earlier versions included? If you don't do that you waste reviewers
time. Another one is that resend right away also kinda wastes peoples
time, because there's a much bigger chance that someone will review the
old version, instead of your new one. Better wait of at least 1-2 days or
so.

> That said, if you're interested in further cleaning this up, one can
> cleanup the drm_dp_mst_topology_cbs hooks.
> In particular ::register_connector is identical across the board -
> create a helper function using it directly in core, killing the hook.
> 
> While for ::destroy_connector - there's some amdgpu specific code in
> there... which I'm not sure if it should stay or not.
> To be on the save side - create a helper which will be called for
> drivers where the hook is !=NULL (aka everyone but amdgpu).

Yeah that stuff looks fishy. Smells a bit like old mst code developed
before Lyude fixed the refcounting for real, it seems to manually shut
down stuff that should be cleaned up with normal code paths (modeset
and/or final kref_put on the connector).
-Daniel
Daniel Vetter March 2, 2020, 9:45 p.m. UTC | #3
On Mon, Mar 02, 2020 at 10:43:19PM +0100, Daniel Vetter wrote:
> On Mon, Mar 02, 2020 at 06:21:23PM +0000, Emil Velikov wrote:
> > Hi Pankaj,
> > 
> > On Mon, 2 Mar 2020 at 16:33, Pankaj Bharadiya
> > <pankaj.laxminarayan.bharadiya@intel.com> wrote:
> > >
> > > This series addresses below drm_fb_helper tasks from
> > > Documentation/gpu/todo.rst.
> > >
> > > - The max connector argument for drm_fb_helper_init() isn't used
> > >   anymore and can be removed.
> > >
> > > - The helper doesn't keep an array of connectors anymore so these can
> > >   be removed: drm_fb_helper_single_add_all_connectors(),
> > >   drm_fb_helper_add_one_connector() and
> > >   drm_fb_helper_remove_one_connector().
> > >
> > > Changes since v1:
> > >    - Squashed warning fixes into the patch that introduced the
> > >      warnings (into 5/7) (Laurent)
> > >    - Fixed reflow in in 9/9 (Laurent)
> > >
> > For the future, include the changelog in the respective patches. This
> > makes it easier for reviewers...
> > Plus you're already changing the commit - might as well mention what/why :-)
> > 
> > Also do include the R-B, Acked-by, other tags accumulated up-to that
> > point, when sending new revision.
> 
> Yup, can you pls resend that entire pile with all the ack/review tags from
> the earlier versions included? If you don't do that you waste reviewers
> time. Another one is that resend right away also kinda wastes peoples
> time, because there's a much bigger chance that someone will review the
> old version, instead of your new one. Better wait of at least 1-2 days or
> so.

Hm just noticed that people are still reviewing v1. I'd say lets forget
about this v2 here, wait 1-2 days, and then resend with everything
combined. Hopefully not too many will re-review v2 here and waste time on
stuff that's reviewed already. Resending within hours is really not good
on mailing lists (with merge requests or whatever it doesn't matter,
because everyone always looks at the latest version).
-Daniel

> 
> > That said, if you're interested in further cleaning this up, one can
> > cleanup the drm_dp_mst_topology_cbs hooks.
> > In particular ::register_connector is identical across the board -
> > create a helper function using it directly in core, killing the hook.
> > 
> > While for ::destroy_connector - there's some amdgpu specific code in
> > there... which I'm not sure if it should stay or not.
> > To be on the save side - create a helper which will be called for
> > drivers where the hook is !=NULL (aka everyone but amdgpu).
> 
> Yeah that stuff looks fishy. Smells a bit like old mst code developed
> before Lyude fixed the refcounting for real, it seems to manually shut
> down stuff that should be cleaned up with normal code paths (modeset
> and/or final kref_put on the connector).
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Laxminarayan Bharadiya, Pankaj March 3, 2020, 4:38 a.m. UTC | #4
> -----Original Message-----
> From: Daniel Vetter <daniel@ffwll.ch>
> Sent: 03 March 2020 03:15
> To: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@intel.com>; Jani Nikula
> <jani.nikula@linux.intel.com>; Daniel Vetter <daniel@ffwll.ch>; Intel
> Graphics Development <intel-gfx@lists.freedesktop.org>; ML dri-devel <dri-
> devel@lists.freedesktop.org>
> Subject: Re: [Intel-gfx] [PATCH v2 0/7] drm: drm_fb_helper cleanup.
> 
> On Mon, Mar 02, 2020 at 10:43:19PM +0100, Daniel Vetter wrote:
> > On Mon, Mar 02, 2020 at 06:21:23PM +0000, Emil Velikov wrote:
> > > Hi Pankaj,
> > >
> > > On Mon, 2 Mar 2020 at 16:33, Pankaj Bharadiya
> > > <pankaj.laxminarayan.bharadiya@intel.com> wrote:
> > > >
> > > > This series addresses below drm_fb_helper tasks from
> > > > Documentation/gpu/todo.rst.
> > > >
> > > > - The max connector argument for drm_fb_helper_init() isn't used
> > > >   anymore and can be removed.
> > > >
> > > > - The helper doesn't keep an array of connectors anymore so these can
> > > >   be removed: drm_fb_helper_single_add_all_connectors(),
> > > >   drm_fb_helper_add_one_connector() and
> > > >   drm_fb_helper_remove_one_connector().
> > > >
> > > > Changes since v1:
> > > >    - Squashed warning fixes into the patch that introduced the
> > > >      warnings (into 5/7) (Laurent)
> > > >    - Fixed reflow in in 9/9 (Laurent)
> > > >
> > > For the future, include the changelog in the respective patches.
> > > This makes it easier for reviewers...
> > > Plus you're already changing the commit - might as well mention
> > > what/why :-)
> > >
> > > Also do include the R-B, Acked-by, other tags accumulated up-to that
> > > point, when sending new revision.
> >
> > Yup, can you pls resend that entire pile with all the ack/review tags
> > from the earlier versions included? If you don't do that you waste
> > reviewers time. Another one is that resend right away also kinda
> > wastes peoples time, because there's a much bigger chance that someone
> > will review the old version, instead of your new one. Better wait of
> > at least 1-2 days or so.
> 
> Hm just noticed that people are still reviewing v1. I'd say lets forget about
> this v2 here, wait 1-2 days, and then resend with everything combined.
> Hopefully not too many will re-review v2 here and waste time on stuff that's
> reviewed already. Resending within hours is really not good on mailing lists
> (with merge requests or whatever it doesn't matter, because everyone
> always looks at the latest version).

Noted 
Laxminarayan Bharadiya, Pankaj March 3, 2020, 4:40 a.m. UTC | #5
> -----Original Message-----
> From: Emil Velikov <emil.l.velikov@gmail.com>
> Sent: 02 March 2020 23:51
> To: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>; Daniel Vetter
> <daniel@ffwll.ch>; Intel Graphics Development <intel-
> gfx@lists.freedesktop.org>; ML dri-devel <dri-devel@lists.freedesktop.org>
> Subject: Re: [Intel-gfx] [PATCH v2 0/7] drm: drm_fb_helper cleanup.
> 
> Hi Pankaj,
> 
> On Mon, 2 Mar 2020 at 16:33, Pankaj Bharadiya
> <pankaj.laxminarayan.bharadiya@intel.com> wrote:
> >
> > This series addresses below drm_fb_helper tasks from
> > Documentation/gpu/todo.rst.
> >
> > - The max connector argument for drm_fb_helper_init() isn't used
> >   anymore and can be removed.
> >
> > - The helper doesn't keep an array of connectors anymore so these can
> >   be removed: drm_fb_helper_single_add_all_connectors(),
> >   drm_fb_helper_add_one_connector() and
> >   drm_fb_helper_remove_one_connector().
> >
> > Changes since v1:
> >    - Squashed warning fixes into the patch that introduced the
> >      warnings (into 5/7) (Laurent)
> >    - Fixed reflow in in 9/9 (Laurent)
> >
> For the future, include the changelog in the respective patches. This makes it
> easier for reviewers...
> Plus you're already changing the commit - might as well mention what/why :-
> )
> 
> Also do include the R-B, Acked-by, other tags accumulated up-to that point,
> when sending new revision.

Noted, Thank you for the feedback. Will send new series with tags accumulated
after 1-2 days. 

> 
> 
> That said, if you're interested in further cleaning this up, one can cleanup the
> drm_dp_mst_topology_cbs hooks.
> In particular ::register_connector is identical across the board - create a
> helper function using it directly in core, killing the hook.
> 
> While for ::destroy_connector - there's some amdgpu specific code in
> there... which I'm not sure if it should stay or not.
> To be on the save side - create a helper which will be called for drivers where
> the hook is !=NULL (aka everyone but amdgpu).

Will take a look.

Thanks,
Pankaj

> 
> HTH
> Emil