Re: [OSS-Tools] [PATCH platsch V3 3/4] platsch: split into platsch and libplatsch

["Ulrich Ölmann" <u.oelmann@pengutronix.de>]

Hi Qing-Wu Li,

On Thu, Jun 13 2024 at 09:07 +0200, LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  libplatsch.c | 590 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  libplatsch.h |  51 +++++
>  meson.build  |  13 +-
>  platsch.c    | 582 +-------------------------------------------------
>  4 files changed, 658 insertions(+), 578 deletions(-)
>  create mode 100644 libplatsch.c
>  create mode 100644 libplatsch.h
>
> diff --git a/libplatsch.c b/libplatsch.c
> new file mode 100644
> index 0000000..1d48e0e
> --- /dev/null
> +++ b/libplatsch.c
> @@ -0,0 +1,590 @@
> +/*
> + * Copyright (C) 2019 Pengutronix, Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> + *
> + * Permission to use, copy, modify, and/or distribute this software
> + * for any purpose with or without fee is hereby granted.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> + * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
> + * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
> + * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
> + * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + *
> + * Some code parts base on example code written in 2012 by David Herrmann
> + * <dh.herrmann@googlemail.com> and dedicated to the Public Domain. It was found
> + * in 2019 on
> + * https://raw.githubusercontent.com/dvdhrm/docs/master/drm-howto/modeset.c
> + */
> +
> +#include <assert.h>
> +#include <ctype.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <getopt.h>
> +#include <libgen.h>
> +#include <stdarg.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>

if there is no requirement for a new order of inclusion please keep the
sorting found in platsch.c.

> +#include "libplatsch.h"
> +#include <drm_fourcc.h>
> +#include <xf86drm.h>
> +#include <xf86drmMode.h>

The includes xf86drmMode.h and drm_fourcc.h are already included in
libplatsch.h, so there is probably no need to include them again here.

> +#include "libplatsch.h"

This was already done some lines above.

> +void redirect_stdfd(void)
> +{
> +	int devnull = open("/dev/null", O_RDWR, 0);
> +
> +	if (devnull < 0) {
> +		error("Failed to open /dev/null: %m\n");
> +		return;
> +	}
> +
> +	close(STDIN_FILENO);
> +	dup2(devnull, STDIN_FILENO);
> +	close(STDOUT_FILENO);
> +	dup2(devnull, STDOUT_FILENO);
> +	close(STDERR_FILENO);
> +	dup2(devnull, STDERR_FILENO);
> +	close(devnull);
> +}

[...]

> +static int drmfd;
> +
> +struct modeset_dev *init(void)
> +{
> +	static char drmdev[128];

This hasn't been static before, is that really needed?

> +	int ret = 0, i;
> +
> +	for (i = 0; i < 64; i++) {
> +		struct drm_mode_card_res res = {0};
> +
> +		/*
> +		 * XXX: Maybe use drmOpen instead?
> +		 * (Where should name/busid come from?)
> +		 * XXX: Loop through drm devices to find one with connectors.
> +		 */
> +		ret = snprintf(drmdev, sizeof(drmdev), DRM_DEV_NAME, DRM_DIR_NAME, i);
> +		if (ret >= sizeof(drmdev)) {
> +			error("Huh, device name overflowed buffer\n");
> +			goto execinit;
> +		}
> +
> +		drmfd = open(drmdev, O_RDWR | O_CLOEXEC, 0);
> +		if (drmfd < 0) {
> +			error("Failed to open drm device: %m\n");
> +			goto execinit;
> +		}
> +
> +		ret = drmIoctl(drmfd, DRM_IOCTL_MODE_GETRESOURCES, &res);
> +		if (ret < 0) {
> +			close(drmfd);
> +			continue;
> +		} else {
> +			/* Device found */
> +			break;
> +		}
> +	}
> +
> +	if (i == 64) {
> +		error("No suitable DRM device found\n");
> +		goto execinit;
> +	}
> +
> +	ret = drmprepare(drmfd);
> +	if (ret) {
> +		error("Failed to prepare DRM device\n");
> +		goto execinit;
> +	}
> +
> +	return modeset_list;
> +
> +execinit:

The label should probably be renamed as this new init()-function has
nothing to do with calling exec() for the next PID1 program anymore.

> +	return NULL;
> +}
> +
> +void update_display(struct modeset_dev *dev)
> +{
> +	int ret = 0;
> +
> +	if (dev->setmode) {
> +		ret = drmModeSetCrtc(drmfd, dev->crtc_id, dev->fb_id, 0, 0, &dev->conn_id, 1,
> +				     &dev->mode);
> +		if (ret) {
> +			error("Cannot set CRTC for connector #%u: %m\n", dev->conn_id);
> +		}
> +		dev->setmode = 0;

This line is new. If it is needed, please add it in a separate commit
and describe why it is needed.

> +	} else {
> +		ret = drmModePageFlip(drmfd, dev->crtc_id, dev->fb_id, 0, NULL);
> +		if (ret) {
> +			error("Page flip failed on connector #%u: %m\n", dev->conn_id);
> +		}
> +	}
> +}
> +
> +void draw(struct modeset_dev *dev, const char *dir, const char *base)
> +{
> +	draw_buffer(dev, dir, base);
> +	update_display(dev);
> +}
> +
> +void finish(void) { drmDropMaster(drmfd); }

The original code checked the return value here. Is drmDropMaster()
guaranteed to always succeed?

> +void deinit(void)
> +{
> +	struct modeset_dev *iter;
> +
> +	for (iter = modeset_list; iter; iter = iter->next) {

Use after free: you are accessing iter->next here...

> +		if (iter->map) {
> +			munmap(iter->map, iter->size);
> +		}
> +		if (iter->fb_id) {
> +			drmModeRmFB(drmfd, iter->fb_id);
> +		}
> +		free(iter);

... although iter was already freed here.

> +	}
> +}
> diff --git a/libplatsch.h b/libplatsch.h
> new file mode 100644
> index 0000000..77f74a9
> --- /dev/null
> +++ b/libplatsch.h
> @@ -0,0 +1,51 @@
> +#ifndef __LIBPLATSCH_H__
> +#define __LIBPLATSCH_H__
> +
> +#include <xf86drmMode.h>
> +#include <drm_fourcc.h>
> +#include "libplatsch.h"

Probably no need to include libplatsch.h from itself.

> +#include <unistd.h>
> +
> +
> +
> +#define debug(fmt, ...) printf("%s:%d: " fmt, __func__, __LINE__, ##__VA_ARGS__)
> +#define error(fmt, ...) fprintf(stderr, fmt, ##__VA_ARGS__)
> +
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(*a))
> +
> +struct platsch_format {
> +	uint32_t format;
> +	uint32_t bpp;
> +	const char *name;
> +};
> +
> +struct modeset_dev {
> +	struct modeset_dev *next;
> +
> +	uint32_t width;
> +	uint32_t height;
> +	uint32_t stride;
> +	uint32_t size;
> +	const struct platsch_format *format;
> +	uint32_t handle;
> +	void *map;
> +	bool setmode;
> +	drmModeModeInfo mode;
> +	uint32_t fb_id;
> +	uint32_t conn_id;
> +	uint32_t crtc_id;
> +};
> +
> +static const struct platsch_format platsch_formats[] = {
> +	{ DRM_FORMAT_RGB565, 16, "RGB565" }, /* default */
> +	{ DRM_FORMAT_XRGB8888, 32, "XRGB8888" },
> +};
> +
> +ssize_t readfull(int fd, void *buf, size_t count);
> +struct modeset_dev *init(void);
> +void draw(struct modeset_dev *dev, const char *dir, const char *base);
> +void finish(void);
> +void redirect_stdfd(void);
> +void update_display(struct modeset_dev *dev);

Only the minority of symbols in the new library are namespaced, compare
"platsch_format" against "draw" or "finish". Particularly the latter can
be expected to be exported from third party code which libplatsch is
then not able to be linked against. Thus please add the prefix
"platsch_" to at least all symbols that are published via libplatsch.h.

> +#endif
> \ No newline at end of file
> diff --git a/meson.build b/meson.build
> index 9f6be1e..dc55238 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1,15 +1,18 @@
>  project('platsch', 'c')
>  
>  platsch_dep = dependency('libdrm', version : '>= 2.4.112', required : true)

One feature of platsch is to start up fast, hence it is intentionally
linked against static libraries only. Therefore please use

  platsch_dep = dependency('libdrm',
      version: '>= 2.4.112',
      static: true,
      required: true
  )

instead.

> -sources = ['platsch.c']
> +sources = ['libplatsch.c']
>  
> -# Define the headers
> -headers = ['platsch.h']
>  
> -# Create the platsch executable
> -executable('platsch',
> +libplatsch = static_library('libplatsch',

The resulting library is called "liblibplatsch.a" now which is somewhat
redundant.

>      sources,
>      dependencies: platsch_dep,
> +)
> +
> +executable('platsch',
> +    'platsch.c',
> +    dependencies: platsch_dep,
> +    link_with: libplatsch,

Unfortunately picking the static variant of libdrm in its dependency()-
statement is not enough to get a statically linked executable "platsch"
as there are still some dynamically linked objects left, e.g. the
c-library. Thus please add

    link_args: '-static',

to the executable() statement.

Best regards
Ulrich


>      install: true,
>      include_directories: include_directories('.')
>  )

[...]

> -execinit:
>  	if (pid1) {
>  		ret = fork();
>  		if (ret < 0) {
-- 
Pengutronix e.K.                           | Ulrich Ölmann               |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |