Message-ID: <20160315081056.GS14170@phenom.ffwll.local>
Date:	Tue, 15 Mar 2016 09:10:57 +0100
From:	Daniel Vetter <daniel@...ll.ch>
To:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>
Cc:	"daniel@...ll.ch" <daniel@...ll.ch>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Jose.Abreu@...opsys.com" <Jose.Abreu@...opsys.com>,
	"linux-snps-arc@...ts.infradead.org" 
	<linux-snps-arc@...ts.infradead.org>
Subject: Re: [PATCH 1/4 v3] drm: Add support of ARC PGU display controller

On Mon, Mar 14, 2016 at 11:15:59AM +0000, Alexey Brodkin wrote:
> On Mon, 2016-03-14 at 08:00 +0100, Daniel Vetter wrote:
> > On Fri, Mar 11, 2016 at 06:42:36PM +0300, Alexey Brodkin wrote:
> > > +static int arcpgu_atomic_commit(struct drm_device *dev,
> > > +				    struct drm_atomic_state *state, bool async)
> > > +{
> > > +	return drm_atomic_helper_commit(dev, state, false);
> > Note that this isn't really async if you ever get around to implement
> > fence support or vblank support. Just fyi.
> 
> Ok but for now should I leave it as it is?

Yeah ok as-is, hence just fyi.

> > > +static struct drm_driver arcpgu_drm_driver = {
> > > +	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
> > > +			   DRIVER_ATOMIC,
> > > +	.preclose = arcpgu_preclose,
> > > +	.lastclose = arcpgu_lastclose,
> > > +	.name = "drm-arcpgu",
> > > +	.desc = "ARC PGU Controller",
> > > +	.date = "20160219",
> > > +	.major = 1,
> > > +	.minor = 0,
> > > +	.patchlevel = 0,
> > > +	.fops = &arcpgu_drm_ops,
> > > +	.load = arcpgu_load,
> > > +	.unload = arcpgu_unload,
> > Load and unload hooks are deprecated (it's a classic midlayer mistake).
> > Please use drm_dev_alloc/register pairs directly instead, and put your
> > device setup code in-between. Similar for unloading. There's a bunch of
> > example drivers converted already.
> 
> Ok I took "atmel-hlcdc" as example.
> And that's interesting.
> 
> If I put my arcpgu_load() in between drm_dev_alloc() and
> drm_dev_register() then I'm getting this on the driver probe:
> ---------------------------------->8-------------------------------
> [drm] Initialized drm 1.1.0 20060810
> arcpgu e0017000.pgu: arc_pgu ID: 0xabbabaab
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at lib/kobject.c:244 kobject_add_internal+0x17c/0x498()
> kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0)
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.5.0-rc3-01062-ga447822-dirty #17
> 
> Stack Trace:
>   arc_unwind_core.constprop.1+0xa4/0x110
>   warn_slowpath_fmt+0x6e/0xfc
>   kobject_add_internal+0x17c/0x498
>   kobject_add+0x98/0xe4
>   device_add+0xc6/0x734
>   device_create_with_groups+0x12a/0x144
>   drm_sysfs_connector_add+0x54/0xe8
>   arcpgu_drm_hdmi_init+0xd4/0x17c
>   arcpgu_probe+0x138/0x24c
>   platform_drv_probe+0x2e/0x6c
>   really_probe+0x212/0x35c
>   __driver_attach+0x90/0x94
>   bus_for_each_dev+0x46/0x80
>   bus_add_driver+0x14e/0x1b4
>   driver_register+0x64/0x108
>   do_one_initcall+0x86/0x194
>   kernel_init_freeable+0xf0/0x188
> ---[ end trace c67166ad43ddcce2 ]---
> [drm:drm_sysfs_connector_add] adding "HDMI-A-1" to sysfs
> [drm:drm_sysfs_connector_add] *ERROR* failed to register connector device: -2
> arcpgu e0017000.pgu: failed to regiter DRM connector and helper funcs
> arcpgu: probe of e0017000.pgu failed with error -2
> ---------------------------------->8-------------------------------
> 
> But if I move arcpgu_load() after drm_dev_register() then everything
> starts properly and I may see HDMI screen works perfectly fine.
> 
> Any thoughts?

Oops, yeah missed that detail. If you look at atmel it has a loop to
register all the drm connectors _after_ calling drm_dev_register().
Totally forgot about that. Can you pls
- Extract a new drm_connector_register_all() function
  (atmel_hlcdc_dc_connector_plug_all seems to be the best template),
  including kerneldoc.
- Adjust kerneldoc of drm_dev_register() to mention
  drm_connector_register_all() and that ordering constraint.
- Roll that helper out to all the drivers that currently hand-roll it (one
  patch per driver).

I know a bit of work but imo not too much, and by doing some small
refactoring every time someone stumbles over a drm pitfall we keep the
subsystem clean&easy to understand. You're up for this? Would be a prep
series, I'll happily review it to get it merged fast. Just a few weeks ago
I merged 20+ patches to make ->mode_fixup hooks optional and remove dummy
ones all over the subsystem, in other words: You'll have my full attention
;-)

> > > +	.dumb_create = drm_gem_cma_dumb_create,
> > > +	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
> > > +	.dumb_destroy = drm_gem_dumb_destroy,
> > > +	.get_vblank_counter = drm_vblank_no_hw_counter,
> > > +	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> > > +	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> > > +	.gem_free_object = drm_gem_cma_free_object,
> > > +	.gem_vm_ops = &drm_gem_cma_vm_ops,
> > > +	.gem_prime_export = drm_gem_prime_export,
> > > +	.gem_prime_import = drm_gem_prime_import,
> > > +	.gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
> > > +	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> > > +	.gem_prime_vmap = drm_gem_cma_prime_vmap,
> > > +	.gem_prime_vunmap = drm_gem_cma_prime_vunmap,
> > > +	.gem_prime_mmap = drm_gem_cma_prime_mmap,
> > > +};
> > > +
> > > +static int arcpgu_probe(struct platform_device *pdev)
> > > +{
> > > +	return drm_platform_init(&arcpgu_drm_driver, pdev);
> > ... or read the kerneldoc of this function, which also explains what you
> > should do ;-)
> 
> Could you please point me to the relevant document?
> I wasn't able to find anything related from the first glance :(

The kerneldoc comment for drm_platform_init says it's deprecated, please
use drm_dev_alloc/register instead.
> 
> > > +
> > > +/*
> > > + * This function is the only reason to have a copy of drm_fbdev_cma_init()
> > > + * here in this driver.
> > > + *
> > > + * In its turn this mmap() is required to mark user-space page as non-cached
> > > + * because it is just a mirror or real hardware frame-buffer.
> > > + */
> > > +static int arcpgu_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > > +{
> > > +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> > > +	return vm_iomap_memory(vma, info->fix.smem_start, info->fix.smem_len);
> > > +}
> > This looks very fishy, no other drm driver even bothers with providing an
> > fb_mmap hook. What exactly do you need this for? Assuming you've mmapped
> > your fbcon drm_framebuffer correctly for kernel access things should just
> > work ...
> 
> Indeed for kernel there's non need to that hack. Kernel deals directly with HW
> frame-buffer area (that address we get from gem->paddr). And so every byte written gets
> picked up by PGU and is then rendered on the display.
> 
> But when user-space opens /dev/fb0 and mmaps() it deals with memory pages which
> are by default (at least on ARC) marked as "cached". I.e. user-space application
> (I use that nice demo app https://github.com/qtproject/qt/blob/4.8/examples/qws/framebuffer/main.c)
> deals with frame-buffer via data cache. And that has 2 problems:
>  [1] Since no explicit cache flush gets executed some data is left in data cache,
>      i.e. some parts of the picture never reaches real PGU.
>      See what happens on display - http://imgur.com/iAbnnx3
>      Those missing lines are exactly those 32-byte missing cache lines.
>  [2] Even if we manage to flush data somehow massive amount of data that goes
>      through data cache (let's sat 1080p@...z) will thrash it and as a result
>      there will be no benefit for other cache users to use cache.
> 
> So we fix it simply marking pages mapped to user-space apps as uncached
> that effectively routes all FB data directly to memry instead of polluting cache.
> 
> Hopefully that explanation makes sense.

Still strange that you're the only one who needs this. I think best case
would be to provide some mmap helpers for fbdev which remap to dumb buffer
helpers or something similar. That way other drivers don't need to

Another piece is that right now fbdev emulation hasn't wired up the manual
update mode to the drm ->dirtyfb call. Drivers that need to manual flush
stuff to the screen currently all have to hand-roll those calls one way or
the other (udl, qxl, i915).

But just an observation really, nothing to fix up for this driver imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists