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

[Jon A. Cruz]

On 08/25/2015 03:08 AM, Pekka Paalanen wrote:
> 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.

Well... there are a few approaches in general that could help. First of
all, you are correct about avoiding duplication.

One good point is that if we use a framework that does not require a
single executable per test then linking and source code arrangement is
greatly simplified.

I've found it very helpful to have source directories structured to
match the deployed layout. That is, if there are header files that will
be exposed to included by end users then those should be in an include
folder in the source tree to begin with (as opposed to some that have
all source files together and then have the build process rearrange them
for installation).

Then it ends up easy to add headers that declare functions that might be
used in tests. Since they do not go into the include folders, the chance
of external code using them is minimized. These can also have
"_internal" or such as part of their name.

Upon occasion I've used a #define of UNIT_TEST to avoid exposing certain
things during normal builds. This doesn't necessarily need to expose
functions directly, but could be some hook/proxy function. However, my
preference has been to minimize such #ifdef use.


With C++ code arrangement gets a bit more regular, with one .cpp per
class and one or two corresponding .h (foo.h and foo_internal.h) files
becoming the norm. Since we are not in that arena it is not a big factor.

Since we are dealing with C code, required layouts are much more liberated.

-- 
Jon A. Cruz - Senior Open Source Developer
Samsung Open Source Group
jonc at osg.samsung.com