Re: [PATCH v2 0/7] drm: drm_fb_helper cleanup.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Daniel Vetter <daniel@xxxxxxxx>
> Sent: 03 March 2020 03:15
> To: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> Cc: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@xxxxxxxxx>; Jani Nikula
> <jani.nikula@xxxxxxxxxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Intel
> Graphics Development <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>; ML dri-devel <dri-
> devel@xxxxxxxxxxxxxxxxxxxxx>
> Subject: Re:  [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@xxxxxxxxx> 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 😊. Thanks for the feedback. 
Will send the new series with tags accumulated after 1-2 days.

Thanks,
Pankaj
> -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
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux