casaroli commented on code in PR #19911:
URL: https://github.com/apache/nuttx/pull/19911#discussion_r4434996232
##########
drivers/video/fb.c:
##########
@@ -871,14 +884,104 @@ static int fb_ioctl(FAR struct file *filep, int cmd,
unsigned long arg)
#endif
#ifdef CONFIG_FB_UPDATE
+ case FBIOC_WATCHAREA: /* Enable/disable dirty-area reporting */
+ {
+ FAR struct fb_priv_s *priv = filep->f_priv;
+
+ DEBUGASSERT(priv != NULL);
+ nxmutex_lock(&fb->lock);
+ priv->watch = (arg != 0);
+ priv->dirtyhead = 0;
+ priv->dirtytail = 0;
+ nxmutex_unlock(&fb->lock);
+ ret = OK;
+ }
+ break;
+
+ case FBIOC_GETDIRTY: /* Pop the next queued dirty area */
+ {
+ FAR struct fb_priv_s *priv = filep->f_priv;
+ FAR struct fb_area_s *out =
+ (FAR struct fb_area_s *)((uintptr_t)arg);
+
+ DEBUGASSERT(priv != NULL && out != NULL);
+ nxmutex_lock(&fb->lock);
+ if (priv->dirtytail == priv->dirtyhead)
+ {
+ ret = -EAGAIN;
+ }
+ else
+ {
+ *out = priv->dirty[priv->dirtytail];
+ priv->dirtytail = (priv->dirtytail + 1) % 16;
+ ret = OK;
+ }
+
+ nxmutex_unlock(&fb->lock);
+ }
+ break;
+
case FBIO_UPDATE: /* Update the modified framebuffer data */
{
struct fb_area_s *area = (FAR struct fb_area_s *)((uintptr_t)arg);
+ FAR struct fb_priv_s *priv;
DEBUGASSERT(fb->vtable != NULL);
+
+ /* Queue the area for every open file that asked to watch, and
+ * wake its pollers. Files that never asked cost nothing.
+ */
+
+ nxmutex_lock(&fb->lock);
+ for (priv = fb->head; priv != NULL; priv = priv->flink)
+ {
+ uint8_t next;
+
+ if (!priv->watch)
+ {
+ continue;
+ }
+
+ next = (priv->dirtyhead + 1) % 16;
+ if (next == priv->dirtytail)
+ {
+ /* Full: widen the newest entry to cover this area too */
+
+ FAR struct fb_area_s *last =
+ &priv->dirty[(priv->dirtyhead + 16 - 1) % 16];
+ fb_coord_t x2 = MAX(last->x + last->w,
Review Comment:
The area comes from the caller and is not checked. fb_coord_t is uint16_t,
so when x + w or y + h is larger than 65535 the merged area wraps and becomes
smaller than the areas it must cover; empty areas and areas outside the screen
are queued too.
Fix: clip the area to xres and yres, and drop it when it is empty, before
you queue it.
##########
drivers/video/fb.c:
##########
@@ -871,14 +884,104 @@ static int fb_ioctl(FAR struct file *filep, int cmd,
unsigned long arg)
#endif
#ifdef CONFIG_FB_UPDATE
+ case FBIOC_WATCHAREA: /* Enable/disable dirty-area reporting */
+ {
+ FAR struct fb_priv_s *priv = filep->f_priv;
+
+ DEBUGASSERT(priv != NULL);
+ nxmutex_lock(&fb->lock);
+ priv->watch = (arg != 0);
+ priv->dirtyhead = 0;
+ priv->dirtytail = 0;
+ nxmutex_unlock(&fb->lock);
+ ret = OK;
+ }
+ break;
+
+ case FBIOC_GETDIRTY: /* Pop the next queued dirty area */
+ {
+ FAR struct fb_priv_s *priv = filep->f_priv;
+ FAR struct fb_area_s *out =
+ (FAR struct fb_area_s *)((uintptr_t)arg);
+
+ DEBUGASSERT(priv != NULL && out != NULL);
+ nxmutex_lock(&fb->lock);
+ if (priv->dirtytail == priv->dirtyhead)
+ {
+ ret = -EAGAIN;
+ }
+ else
+ {
+ *out = priv->dirty[priv->dirtytail];
+ priv->dirtytail = (priv->dirtytail + 1) % 16;
+ ret = OK;
+ }
+
+ nxmutex_unlock(&fb->lock);
+ }
+ break;
+
case FBIO_UPDATE: /* Update the modified framebuffer data */
{
struct fb_area_s *area = (FAR struct fb_area_s *)((uintptr_t)arg);
+ FAR struct fb_priv_s *priv;
DEBUGASSERT(fb->vtable != NULL);
+
+ /* Queue the area for every open file that asked to watch, and
+ * wake its pollers. Files that never asked cost nothing.
+ */
+
+ nxmutex_lock(&fb->lock);
Review Comment:
This loop walks fb->head and calls poll_notify() on priv->fds with only
fb->lock held. fb_close() unlinks and frees priv, and fb_poll() changes the fds
slots, inside enter_critical_section() without fb->lock, so a close or a poll
teardown during FBIO_UPDATE uses freed memory.
Fix: walk the list and call poll_notify() inside enter_critical_section(),
as fb_notify_vsync() does.
##########
drivers/video/fb.c:
##########
@@ -92,6 +93,17 @@ struct fb_priv_s
FAR struct pollfd *fds[CONFIG_VIDEO_FB_NPOLLWAITERS];
+ /* Dirty-area reporting (FBIOC_WATCHAREA/FBIOC_GETDIRTY). A small ring
+ * of the areas applications reported through FBIO_UPDATE; when it
+ * overflows, the newest areas are merged into the last entry's
+ * bounding box, so nothing is lost, only precision.
+ */
+
+ bool watch; /* Reporting enabled for this file */
+ uint8_t dirtyhead; /* Ring indices */
+ uint8_t dirtytail;
+ struct fb_area_s dirty[16];
Review Comment:
The new fields add about 132 bytes to every open of every framebuffer, also
without CONFIG_FB_UPDATE, the ring size 16 is written five times, and nothing
uses the new `display` field (line 128).
Fix: put the fields under CONFIG_FB_UPDATE, use one define for the ring
size, and remove `display`.
##########
drivers/video/vfb.c:
##########
@@ -0,0 +1,217 @@
+/****************************************************************************
+ * drivers/video/vfb.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/* A framebuffer with no display behind it: memory, registered as
+ * /dev/fbN, with the geometry the configuration asks for.
+ *
+ * Applications draw into it exactly as they would into a panel, and
+ * whatever wants the pixels, a VNC server, a screen recorder, a test
+ * harness comparing renders, reads the same memory and learns what
+ * changed through the framebuffer's dirty-area reporting. That makes a
+ * board with no display, or one whose display is the wrong size for what
+ * is being developed, run the same graphics stack as one with a panel.
+ *
+ * There is deliberately nothing here about who consumes it.
+ */
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <debug.h>
+#include <errno.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <nuttx/kmalloc.h>
+#include <nuttx/video/fb.h>
+#include <nuttx/video/vfb.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define VFB_WIDTH CONFIG_VIDEO_VFB_WIDTH
+#define VFB_HEIGHT CONFIG_VIDEO_VFB_HEIGHT
+
+#ifdef CONFIG_VIDEO_VFB_FMT_RGB32
+# define VFB_FMT FB_FMT_RGB32
+# define VFB_BPP 32
+#else
+# define VFB_FMT FB_FMT_RGB16_565
+# define VFB_BPP 16
+#endif
+
+#define VFB_STRIDE (VFB_WIDTH * ((VFB_BPP + 7) >> 3))
+#define VFB_SIZE (VFB_STRIDE * VFB_HEIGHT)
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int vfb_getvideoinfo(FAR struct fb_vtable_s *vtable,
+ FAR struct fb_videoinfo_s *vinfo);
+static int vfb_getplaneinfo(FAR struct fb_vtable_s *vtable, int planeno,
+ FAR struct fb_planeinfo_s *pinfo);
+#ifdef CONFIG_FB_UPDATE
+static int vfb_updatearea(FAR struct fb_vtable_s *vtable,
+ FAR const struct fb_area_s *area);
+#endif
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static struct fb_vtable_s g_vfb_vtable =
+{
+ .getvideoinfo = vfb_getvideoinfo,
+ .getplaneinfo = vfb_getplaneinfo,
+#ifdef CONFIG_FB_UPDATE
+ .updatearea = vfb_updatearea,
+#endif
+};
+
+static FAR uint8_t *g_vfb_buffer;
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: vfb_getvideoinfo
+ ****************************************************************************/
+
+static int vfb_getvideoinfo(FAR struct fb_vtable_s *vtable,
+ FAR struct fb_videoinfo_s *vinfo)
+{
+ if (vtable == NULL || vinfo == NULL)
+ {
+ return -EINVAL;
+ }
+
+ memset(vinfo, 0, sizeof(*vinfo));
+ vinfo->fmt = VFB_FMT;
+ vinfo->xres = VFB_WIDTH;
+ vinfo->yres = VFB_HEIGHT;
+ vinfo->nplanes = 1;
+
+ return OK;
+}
+
+/****************************************************************************
+ * Name: vfb_getplaneinfo
+ ****************************************************************************/
+
+static int vfb_getplaneinfo(FAR struct fb_vtable_s *vtable, int planeno,
+ FAR struct fb_planeinfo_s *pinfo)
+{
+ if (vtable == NULL || pinfo == NULL || planeno != 0)
+ {
+ return -EINVAL;
+ }
+
+ memset(pinfo, 0, sizeof(*pinfo));
+ pinfo->fbmem = g_vfb_buffer;
+ pinfo->fblen = VFB_SIZE;
+ pinfo->stride = VFB_STRIDE;
+ pinfo->display = 0;
+ pinfo->bpp = VFB_BPP;
+
+ return OK;
+}
+
+/****************************************************************************
+ * Name: vfb_updatearea
+ *
+ * Description:
+ * There is no panel to push anything to. The call exists so that the
+ * framebuffer core sees a driver that supports updates and passes the
+ * areas on to whoever is watching.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_FB_UPDATE
+static int vfb_updatearea(FAR struct fb_vtable_s *vtable,
+ FAR const struct fb_area_s *area)
+{
+ UNUSED(vtable);
+ UNUSED(area);
+ return OK;
+}
+#endif
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: vfb_register
+ *
+ * Description:
+ * Allocate the framebuffer and register it at /dev/fbN. Nothing here
+ * is board specific, so a board that has a panel of its own can build
+ * this as well and give the two different display numbers.
+ *
+ ****************************************************************************/
+
+int vfb_register(int display)
+{
+ bool allocated = false;
+ int ret;
+
+ if (g_vfb_buffer == NULL)
+ {
+ g_vfb_buffer = kmm_zalloc(VFB_SIZE);
Review Comment:
kmm_zalloc() gives kernel heap memory, and fb_mmap() gives that address to
the user: in a protected build the MPU blocks it, and in a kernel build
vm_map_region() maps whole pages, so the user also gets the kernel heap next to
the buffer.
Fix: use kumm_zalloc() in protected builds and a page-aligned, page-sized
allocation in kernel builds.
##########
drivers/video/fb.c:
##########
@@ -871,14 +884,104 @@ static int fb_ioctl(FAR struct file *filep, int cmd,
unsigned long arg)
#endif
#ifdef CONFIG_FB_UPDATE
+ case FBIOC_WATCHAREA: /* Enable/disable dirty-area reporting */
+ {
+ FAR struct fb_priv_s *priv = filep->f_priv;
+
+ DEBUGASSERT(priv != NULL);
+ nxmutex_lock(&fb->lock);
+ priv->watch = (arg != 0);
+ priv->dirtyhead = 0;
+ priv->dirtytail = 0;
+ nxmutex_unlock(&fb->lock);
+ ret = OK;
+ }
+ break;
+
+ case FBIOC_GETDIRTY: /* Pop the next queued dirty area */
+ {
+ FAR struct fb_priv_s *priv = filep->f_priv;
+ FAR struct fb_area_s *out =
+ (FAR struct fb_area_s *)((uintptr_t)arg);
+
+ DEBUGASSERT(priv != NULL && out != NULL);
+ nxmutex_lock(&fb->lock);
+ if (priv->dirtytail == priv->dirtyhead)
+ {
+ ret = -EAGAIN;
+ }
+ else
+ {
+ *out = priv->dirty[priv->dirtytail];
+ priv->dirtytail = (priv->dirtytail + 1) % 16;
+ ret = OK;
+ }
+
+ nxmutex_unlock(&fb->lock);
+ }
+ break;
+
case FBIO_UPDATE: /* Update the modified framebuffer data */
{
struct fb_area_s *area = (FAR struct fb_area_s *)((uintptr_t)arg);
+ FAR struct fb_priv_s *priv;
DEBUGASSERT(fb->vtable != NULL);
+
+ /* Queue the area for every open file that asked to watch, and
+ * wake its pollers. Files that never asked cost nothing.
+ */
+
+ nxmutex_lock(&fb->lock);
+ for (priv = fb->head; priv != NULL; priv = priv->flink)
+ {
+ uint8_t next;
+
+ if (!priv->watch)
+ {
+ continue;
+ }
+
+ next = (priv->dirtyhead + 1) % 16;
+ if (next == priv->dirtytail)
+ {
+ /* Full: widen the newest entry to cover this area too */
+
+ FAR struct fb_area_s *last =
+ &priv->dirty[(priv->dirtyhead + 16 - 1) % 16];
+ fb_coord_t x2 = MAX(last->x + last->w,
+ area->x + area->w);
+ fb_coord_t y2 = MAX(last->y + last->h,
+ area->y + area->h);
+
+ last->x = MIN(last->x, area->x);
+ last->y = MIN(last->y, area->y);
+ last->w = x2 - last->x;
+ last->h = y2 - last->y;
+ }
+ else
+ {
+ priv->dirty[priv->dirtyhead] = *area;
+ priv->dirtyhead = next;
+ }
+
+ poll_notify(priv->fds, CONFIG_VIDEO_FB_NPOLLWAITERS,
Review Comment:
fb_notify_vsync() already sends POLLPRI to every open file (sim, goldfish
and virtio-gpu call it), so a watcher wakes on each vsync and FBIOC_GETDIRTY
returns -EAGAIN; an application cannot tell the two events apart.
Fix: report pending dirty areas with POLLIN, here and in fb_poll().
##########
include/nuttx/video/fb.h:
##########
@@ -1016,6 +1016,19 @@ extern "C"
#define EXTERN extern
#endif
+/* FBIOC_WATCHAREA - Enable (or, with zero, disable) per-open-file dirty
+ * area reporting: every area an application reports
+ * through FBIO_UPDATE is queued for this file and
+ * signalled with POLLPRI.
+ * Argument: int (boolean)
+ * FBIOC_GETDIRTY - Pop the next queued dirty area.
+ * Argument: writable struct fb_area_s*
+ * Returns -EAGAIN when nothing is pending.
+ */
+
+#define FBIOC_WATCHAREA _FBIOC(0x0030)
Review Comment:
The new ioctls are outside the ioctl list and use a new FBIOC_ prefix; all
other ioctls are FBIOGET_/FBIOSET_/FBIO_.
Fix: move them into the list after FBIOGET_ROTATION, for example
FBIOSET_WATCHAREA and FBIOGET_DIRTYAREA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
