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

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

Den 29.03.2016 09:27, skrev Daniel Vetter:
> On Fri, Mar 25, 2016 at 11:41:44AM +0100, Noralf Trønnes wrote:
>> 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.

[...]

>
>>> 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,
>> };
> Ah, cma midlayer strikes again. We've had a similar discussion around the
> gem side when vc4 landed for similar reasons iirc - it's hard to overwrite
> specific helper functions to customize the behaviour.
>
> I guess the simplest solution would be to export
> drm_fb_cma_destroy/create_handle functions, and then add a
> drm_fb_cma_create_with_funcs helper which you can use like this:
>
> static int tinydrm_fbdev_create(struct drm_fb_helper *helper,
>                                  struct drm_fb_helper_surface_size *sizes)
> {
> 	return drm_fb_cma_create_with_funcs(helper, sizes, tinydrm_fb_cma_funcs);
> }
>
> Changing fb->funcs after drm_framebuffer_init is called is a no-go, since
> that function also registers the framebuffer and makes it userspace
> accessible. So after that point other threads can go&look at fb->funcs.
>
>> 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;
> Ok, first thing I've forgotten is that the fbdev cma helper doesn't call
> mode_config->funcs->fb_create and so gets the wrong fb->funcs. One thing
> I've pondered for different reasons lately is whether the fbdev emulation
> should grow a fake drm_file fpriv. With that we could just allocate a dumb
> buffer and pass it to ->fb_create (it takes a handle, which means we need
> a fake fpriv). I think that would fully demidlayer cma helpers. Or we
> create a new helper function to do ->fb_create but with gem objects
> instead of ids like what we've done for vc4. Or we just open-code it all.
>
> Not sure which is better here.
>
>>          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);
> I was wondering whether we couldn't avoid the _with_funcs here by setting
> up fbdefio iff ->dirty is set. Then you could add the generic defio setup
> code to drm_fbdev_cma_create after helper->fb is allocated, but only if
> helper->fb->funcs->dirty is set. Makes for a bit less boilerplate.
>
> Or did I miss something?

I don't see how I can avoid drm_fbdev_cma_init_with_funcs():

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)
{
         return drm_fbdev_cma_create_with_funcs(helper, sizes,
&tinydrm_fb_cma_funcs);
}

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;
}


Thanks for your feedback so far Daniel, I quite like the direction this is
taking. I'll try and implement it in a new version of the patchset.


Noralf.

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