From mboxrd@z Thu Jan 1 00:00:00 1970 Delivery-date: Fri, 14 Jun 2024 12:57:14 +0200 Received: from metis.whiteo.stw.pengutronix.de ([2a0a:edc0:2:b01:1d::104]) by lore.white.stw.pengutronix.de with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1sI4cM-005t7z-1O for lore@lore.pengutronix.de; Fri, 14 Jun 2024 12:57:14 +0200 Received: from localhost ([127.0.0.1] helo=metis.whiteo.stw.pengutronix.de) by metis.whiteo.stw.pengutronix.de with esmtp (Exim 4.92) (envelope-from ) id 1sI4cL-0000vi-9A; Fri, 14 Jun 2024 12:57:13 +0200 Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.whiteo.stw.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1sI4cK-0000vO-0b; Fri, 14 Jun 2024 12:57:12 +0200 Received: from [2a0a:edc0:2:b01:1d::c5] (helo=pty.whiteo.stw.pengutronix.de) by drehscheibe.grey.stw.pengutronix.de with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sI4cJ-002FUn-G9; Fri, 14 Jun 2024 12:57:11 +0200 Received: from uol by pty.whiteo.stw.pengutronix.de with local (Exim 4.96) (envelope-from ) id 1sI4cJ-00APFZ-1L; Fri, 14 Jun 2024 12:57:11 +0200 From: =?utf-8?Q?Ulrich_=C3=96lmann?= To: LI Qingwu In-Reply-To: <20240613070724.3400651-3-Qing-wu.Li@leica-geosystems.com.cn> (LI Qingwu's message of "Thu, 13 Jun 2024 09:07:23 +0200") References: <20240613070724.3400651-1-Qing-wu.Li@leica-geosystems.com.cn> <20240613070724.3400651-3-Qing-wu.Li@leica-geosystems.com.cn> User-Agent: mu4e 1.12.4; emacs 29.2 Date: Fri, 14 Jun 2024 12:57:11 +0200 Message-ID: <6rbk43n660.fsf@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [OSS-Tools] [PATCH platsch V3 3/4] platsch: split into platsch and libplatsch X-BeenThere: oss-tools@pengutronix.de X-Mailman-Version: 2.1.29 Precedence: list List-Id: Pengutronix Public Open-Source-Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: oss-tools@pengutronix.de, m.felsch@pengutronix.de Sender: "OSS-Tools" X-SA-Exim-Connect-IP: 127.0.0.1 X-SA-Exim-Mail-From: oss-tools-bounces@pengutronix.de X-SA-Exim-Scanned: No (on metis.whiteo.stw.pengutronix.de); SAEximRunCond expanded to false Hi Qing-Wu Li, On Thu, Jun 13 2024 at 09:07 +0200, LI Qingwu wrote: > Signed-off-by: LI Qingwu > --- > 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=C3=B6nig > + * > + * 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 > + * and dedicated to the Public Domain. It w= as found > + * in 2019 on > + * https://raw.githubusercontent.com/dvdhrm/docs/master/drm-howto/modese= t.c > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include if there is no requirement for a new order of inclusion please keep the sorting found in platsch.c. > +#include "libplatsch.h" > +#include > +#include > +#include 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 =3D 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 =3D 0, i; > + > + for (i =3D 0; i < 64; i++) { > + struct drm_mode_card_res res =3D {0}; > + > + /* > + * XXX: Maybe use drmOpen instead? > + * (Where should name/busid come from?) > + * XXX: Loop through drm devices to find one with connectors. > + */ > + ret =3D snprintf(drmdev, sizeof(drmdev), DRM_DEV_NAME, DRM_DIR_NAME, i= ); > + if (ret >=3D sizeof(drmdev)) { > + error("Huh, device name overflowed buffer\n"); > + goto execinit; > + } > + > + drmfd =3D open(drmdev, O_RDWR | O_CLOEXEC, 0); > + if (drmfd < 0) { > + error("Failed to open drm device: %m\n"); > + goto execinit; > + } > + > + ret =3D drmIoctl(drmfd, DRM_IOCTL_MODE_GETRESOURCES, &res); > + if (ret < 0) { > + close(drmfd); > + continue; > + } else { > + /* Device found */ > + break; > + } > + } > + > + if (i =3D=3D 64) { > + error("No suitable DRM device found\n"); > + goto execinit; > + } > + > + ret =3D 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 =3D 0; > + > + if (dev->setmode) { > + ret =3D drmModeSetCrtc(drmfd, dev->crtc_id, dev->fb_id, 0, 0, &dev->co= nn_id, 1, > + &dev->mode); > + if (ret) { > + error("Cannot set CRTC for connector #%u: %m\n", dev->conn_id); > + } > + dev->setmode =3D 0; This line is new. If it is needed, please add it in a separate commit and describe why it is needed. > + } else { > + ret =3D 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 =3D modeset_list; iter; iter =3D 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 > +#include > +#include "libplatsch.h" Probably no need to include libplatsch.h from itself. > +#include > + > + > + > +#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[] =3D { > + { 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') >=20=20 > platsch_dep =3D dependency('libdrm', version : '>=3D 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 =3D dependency('libdrm', version: '>=3D 2.4.112', static: true, required: true ) instead. > -sources =3D ['platsch.c'] > +sources =3D ['libplatsch.c'] >=20=20 > -# Define the headers > -headers =3D ['platsch.h'] >=20=20 > -# Create the platsch executable > -executable('platsch', > +libplatsch =3D 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 =3D fork(); > if (ret < 0) { --=20 Pengutronix e.K. | Ulrich =C3=96lmann = | 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 |