Re: [RFC 1/5] drm: Add DRM support for tiny LCD displays

["Noralf Trønnes" <noralf@tronnes.org>]

Den 23.03.2016 18:28, skrev Daniel Vetter:
> On Wed, Mar 23, 2016 at 06:07:56PM +0100, Noralf Trønnes wrote:
>> Den 18.03.2016 18:47, skrev Daniel Vetter:
>>> On Thu, Mar 17, 2016 at 10:51:55PM +0100, Noralf Trønnes wrote:
>>>> Den 16.03.2016 16:11, skrev Daniel Vetter:
>>>>> On Wed, Mar 16, 2016 at 02:34:15PM +0100, Noralf Trønnes wrote:
>>>>>> tinydrm provides a very simplified view of DRM for displays that has
>>>>>> onboard video memory and is connected through a slow bus like SPI/I2C.
>>>>>>
>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> Yay, it finally happens! I already made a comment on the cover letter
>>>>> about the fbdev stuff, I think that's the biggest part to split out from
>>>>> tinydrm here. I'm not entirely sure a detailed code review makes sense
>>>>> before that part is done (and hey we can start merging already), so just a
>>>>> high level review for now:
>> [...]
>>>
>>>>> In the case of tinydrm I think that means we should have a bunch of new
>>>>> drm helpers, or extensions for existing ones:
>>>>> - fbdev deferred io support using ->dirtyfb in drm_fb_helper.c.
>>>> Are you thinking something like this?
>>>>
>>>> struct drm_fb_helper_funcs {
>>>>      int (*dirtyfb)(struct drm_fb_helper *fb_helper,
>>>>                 struct drm_clip_rect *clip);
>>> We already have a dirty_fb function in
>>> dev->mode_config->funcs->dirty_fb(). This is the official interface native
>>> drm/kms userspace is supposed to use to flush frontbuffer rendering. The
>>> xfree86-video-modesetting driver uses it.
>> I couldn't find this dirty_fb() function, but I assume you mean
>> drm_framebuffer_funcs.dirty().
> Yup.
>
>>>> };
>>>>
>>>> struct drm_fb_helper {
>>>>      spinlock_t dirty_lock;
>>>>      struct drm_clip_rect *dirty_clip;
>>>> };
>>> Yeah, this part is needed for the delayed work for the fbdev helper.
>>> struct work dirty_fb_work; is missing.
>> This confuses me.
>> If we have this then there's no need for a fb->funcs->dirty() call,
>> the driver can just add a work function here instead.
>>
>> Possible fb dirty() call chain:
>> Calls to drm_fb_helper_sys_* or mmap page writes will schedule
>> fb_info->deferred_work. The worker func fb_deferred_io_work() calls
>> fb_info->fbdefio->deferred_io().
>> Then deferred_io() can call fb_helper->fb->funcs->dirty().
>>
>> In my use-case this dirty() function would schedule a delayed_work to run
>> immediately since it has already been deferred collecting changes.
>> The regular drm side framebuffer dirty() collects damage and schedules
>> the same worker to run deferred.
>>
>> I don't see an easy way for a driver to set the dirty() function in
>> drm_fb_cma_helper apart from doing this:
>>
>>   struct drm_fbdev_cma {
>>           struct drm_fb_helper    fb_helper;
>>           struct drm_fb_cma       *fb;
>> +        int (*dirty)(struct drm_framebuffer *framebuffer,
>> +                     struct drm_file *file_priv, unsigned flags,
>> +                     unsigned color, struct drm_clip_rect *clips,
>> +                     unsigned num_clips);
>>   };
> Well my point is that drm core already has a canonical interface
> (drm_framebuffer_funcs.dirty) to flush out rendering. And it's supposed to
> be called from process context, and userspace is supposed to batch up
> dirty updates.

Batched up into one ioctl call?
If that's the case, then I don't have to use delayed_work like I do now.
I can just queue a work_struct to run immediately.

This comment in include/uapi/drm/drm_mode.h made me think that I might
receive multiple calls:

  * The kernel or hardware is free to update more then just the
  * region specified by the clip rects. The kernel or hardware
  * may also delay and/or coalesce several calls to dirty into a
  * single update.

I have assumed that I can't run the whole display update from the ioctl
call since one full display update on the "worst" display takes ~200ms.
But maybe it's fine to run all this in the ioctl call?

> What I'd like is that the fbdev emulation uses exactly that interface,
> without requiring drivers to write any additional fbdev code (like qxl and
> udl currently have). Since the drm_framebuffer_funcs.dirty is already
> expected to run in process context I think the only bit we need is the
> deferred_work you already added in fbdev, so that we can schedule the
> driver's ->dirty() function.
>
> There shouldn't be any need to have another ->dirty() function anywhere
> else.

I'll try and see if I can explain better with some code.
First the drm_fb_helper part:

void drm_fb_helper_deferred_io(struct fb_info *info,
                                struct list_head *pagelist)
{
         struct drm_fb_helper *helper = info->par;
         unsigned long start, end, min, max;
         struct drm_clip_rect clip;
         struct page *page;

         if (!helper->fb->funcs->dirty)
                 return;

         spin_lock(&helper->dirty_lock);
         clip = *helper->dirty_clip;
         /* reset dirty_clip */
         spin_unlock(&helper->dirty_lock);

         min = ULONG_MAX;
         max = 0;
         list_for_each_entry(page, pagelist, lru) {
                 start = page->index << PAGE_SHIFT;
                 end = start + PAGE_SIZE - 1;
                 min = min(min, start);
                 max = max(max, end);
         }

         if (min < max) {
                 /* TODO merge with clip */
         }

         helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1);
}
EXPORT_SYMBOL(drm_fb_helper_deferred_io);

/* Called by the drm_fb_helper_sys_*() functions */
static void drm_fb_helper_sys_deferred(struct fb_info *info, u32 x, u32 y,
                                        u32 width, u32 height)
{
         struct drm_fb_helper *helper = info->par;
         struct drm_clip_rect clip;

         if (!info->fbdefio)
                 return;

         clip.x1 = x;
         clip.x2 = x + width -1;
         cliy.y1 = y;
         clip.y2 = y + height - 1;

         spin_lock(&helper->dirty_lock);
         /* TODO merge clip with helper->dirty_clip */
         spin_unlock(&helper->dirty_lock);

         schedule_delayed_work(&info->deferred_work, info->fbdefio->delay);
}


So the question I have asked is this: How can the driver set the
dirty() function within drm_fb_cma_helper?

Having looked at the code over and over again, I have a suggestion,
but it assumes that it's allowed to change fb->funcs.

First the necessary drm_fb_cma_helper changes:

EXPORT_SYMBOL(drm_fb_cma_destroy);
EXPORT_SYMBOL(drm_fb_cma_create_handle);
EXPORT_SYMBOL(drm_fbdev_cma_create);

/* This is the drm_fbdev_cma_init() code with one change */
struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
         unsigned int preferred_bpp, unsigned int num_crtc,
         unsigned int max_conn_count, struct drm_framebuffer_funcs *funcs)
{
[...]
         drm_fb_helper_prepare(dev, helper, funcs);
[...]
}

struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
         unsigned int preferred_bpp, unsigned int num_crtc,
         unsigned int max_conn_count)
{
         return drm_fbdev_cma_init_with_funcs(dev, preferred_bpp, num_crtc,
                                              max_conn_count,
&drm_fb_cma_helper_funcs);
}


Now tinydrm should be able to do this:

static int tinydrm_fbdev_dirty(struct drm_framebuffer *fb,
                                struct drm_file *file_priv,
                                unsigned flags, unsigned color,
                                struct drm_clip_rect *clips,
                                unsigned num_clips)
{
         struct drm_fb_helper *helper = info->par;
         struct tinydrm_device *tdev = helper->dev->dev_private;
         struct drm_framebuffer *fb = helper->fb;
         struct drm_gem_cma_object *cma_obj;

         if (tdev->plane.fb != fb)
                 return 0;

         cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
         if (!cma_obj) {
                 dev_err_once(info->dev, "Can't get cma_obj\n");
                 return -EINVAL;
         }

         return tdev->dirtyfb(fb, cma_obj->vaddr, flags, color, clips, 
num_clips);
}

static struct drm_framebuffer_funcs tinydrm_fb_cma_funcs = {
         .destroy        = drm_fb_cma_destroy,
         .create_handle  = drm_fb_cma_create_handle,
         .dirty          = tinydrm_fbdev_dirty,
};

static int tinydrm_fbdev_create(struct drm_fb_helper *helper,
                                 struct drm_fb_helper_surface_size *sizes)
{
         struct tinydrm_device *tdev = helper->dev->dev_private;
         struct fb_deferred_io *fbdefio;
         struct fb_ops *fbops;
         struct fb_info *info;

         /*
          * A per device structure is needed for:
          * - fbops because fb_deferred_io_cleanup() clears fbops.fb_mmap
          * - fbdefio to get per device delay
          */
         fbops = devm_kzalloc(helper->dev->dev, sizeof(*fbops), GFP_KERNEL);
         fbdefio = devm_kzalloc(helper->dev->dev, sizeof(*fbdefio), 
GFP_KERNEL);
         if (!fbops || !fbdefio)
                 return -ENOMEM;

         ret = drm_fbdev_cma_create(helper, sizes);
         if (ret)
                 return ret;

         helper->fb->funcs = &tinydrm_fb_cma_funcs;

         info = fb_helper->fbdev;
         /*
          * fb_deferred_io requires a vmalloc address or a physical address.
          * drm_fbdev_cma_create() sets smem_start to the dma address 
which is
          * a device address. This isn't always a physical address.
          */
         info->smem_start = page_to_phys(virt_to_page(info->screen_buffer));

         *fbops = *info->fbops;
         info->fbops = fbops;

         info->fbdefio = fbdefio;
         fbdefio->deferred_io = drm_fb_helper_deferred_io;
         fbdefio->delay = msecs_to_jiffies(tdev->deferred->defer_ms);
         /* delay=0 is turned into delay=HZ, so use 1 as a minimum */
         if (!fbdefio->delay)
                 fbdefio->delay = 1;
         fb_deferred_io_init(info);

         return 0;
}

static const struct drm_fb_helper_funcs tinydrm_fb_helper_funcs = {
         .fb_probe = tinydrm_fbdev_create,
};

int tinydrm_fbdev_init(struct tinydrm_device *tdev)
{
         struct drm_device *dev = tdev->base;
         struct drm_fbdev_cma *cma;

         cma = drm_fbdev_cma_init_with_funcs(dev, 16,
dev->mode_config.num_crtc,
dev->mode_config.num_connector,
&tinydrm_fb_helper_funcs);
         if (IS_ERR(cma))
                 return PTR_ERR(cma);

         tdev->fbdev_cma = cma;

         return 0;
}

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel