[PATCH weston 2/3] drm: port the drm backend to the new init api

[Pekka Paalanen]

On Mon, 24 Aug 2015 19:26:48 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Thu, Aug 13, 2015 at 01:53:05PM +0300, Pekka Paalanen wrote:
> > From: Giulio Camuffo <giuliocamuffo at gmail.com>
> 
> This is a big patch with a lot of changes, and I'm worried about landing
> it right as we're on the eve of beta.  If it could be broken up into
> smaller easy-to-review bits, it might make it more digestible...
> 
> That said, I like where this is going.  I'd be totally okay with landing
> it post-release.
>  
> > ---
> >  Makefile.am          |   3 +
> >  src/compositor-drm.c | 234 ++++++++++++++++-----------------------------------
> >  src/compositor-drm.h |  89 ++++++++++++++++++++
> >  src/main.c           | 127 +++++++++++++++++++++++++++-
> >  4 files changed, 292 insertions(+), 161 deletions(-)
> >  create mode 100644 src/compositor-drm.h

...

> Nice refactoring.  With this change, this function also becomes quite
> simple to write a test case for... just pass in various strings and
> verify the correct int is returned.
> 
> > @@ -2127,31 +2086,29 @@ get_gbm_format_from_section(struct weston_config_section *section,
> >   * Find the most suitable mode to use for initial setup (or reconfiguration on
> >   * hotplug etc) for a DRM output.
> >   *
> > + * @param backend The DRM backend object
> >   * @param output DRM output to choose mode for
> > - * @param kind Strategy and preference to use when choosing mode
> > - * @param width Desired width for this output
> > - * @param height Desired height for this output
> > + * @param config Desired configuration for the output
> >   * @param current_mode Mode currently being displayed on this output
> > - * @param modeline Manually-entered mode (may be NULL)
> >   * @returns A mode from the output's mode list, or NULL if none available
> >   */
> >  static struct drm_mode *
> > -drm_output_choose_initial_mode(struct drm_output *output,
> > -			       enum output_config kind,
> > -			       int width, int height,
> > -			       const drmModeModeInfo *current_mode,
> > -			       const drmModeModeInfo *modeline)
> > +drm_output_choose_initial_mode(struct drm_backend *backend,
> > +			       struct drm_output *output,
> > +			       struct weston_drm_backend_output_config *config,
> > +			       const drmModeModeInfo *current_mode)
> 
> This function has a number of exit points, making it a really good
> candidate for writing a thorough collection of test cases.  The tricky
> bits would be the two drm_output_add_mode() calls, which will need
> mocks; initially though a first cut test could leave those two branches
> as TODO.

Hi Bryce and Jon,

leaving all the other comments for later, there is one thing I'm
curious about:

How would you suggest we arrange the source code such that we can pick
arbitrary functions for unit tests without making a copy of that
function?

It should be non-disruptive for the source code used for production,
meaning that we cannot e.g. put #ifdef wrappers around every function.
I think putting one function per file is also too much.

I wouldn't want to make a copy and then later find out that we have
been testing the copy while the code actually used in production has
already changed. Maintaining a copy manually is a no-go.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150825/c951d8bd/attachment.sig>