Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[Robin Murphy]

On 16/03/16 19:14, Russell King - ARM Linux wrote:
> On Wed, Mar 16, 2016 at 04:28:25PM +0100, Daniel Vetter wrote:
> > On Wed, Mar 16, 2016 at 02:57:49PM +0000, Robin Murphy wrote:
> > > In the absence of an fb_mmap callback, the fbdev code falls back to a
> > > naive implementation which relies upon the DMA address being the same
> > > as the physical address, and the buffer being physically contiguous
> > > from there. Whilst this often holds for standard CMA allocations via
> > > the platform's regular DMA ops, if the allocation is provided by an
> > > IOMMU then such assumptions can fall apart spectacularly.
> > >
> > > To resolve this, reroute the fb_mmap call to the appropriate DMA API
> > > implementation, as per the other cma_helper calls.
> > >
> > > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> > > ---
> > >
> > > Hi dri-devel,
> > >
> > > This is an empirical fix for something I tickled via the newly-added
> > > ARM HDLCD driver on a Juno platform - I have no idea whatsoever about
> > > how "proper" it is in terms of the DRM infrastructure, so feel free to
> > > treat this as a bug report rather than an actual patch if appropriate ;)
> >
> > I think the best case would be if we could have a generic fbdev helper
> > that remaps to dumb mmap support. But that's a bit tricky to pull of:
> > 1. from fb_info we can get at the fbdev drm_framebuffer.
> > 2. from a drm_framebuffer we can get at the underlying backing storage
> > object using fb->funcs->get_handle.
> > 3. With that handle we could go into the dumb mmap support (using als the
> > vma) and create the mmap.
> >
> > Except that ->get_handle needs a file_priv, and that just exist for the
> > fbdev emulation kms client. I guess we could fix that by creating a
> > minimal fake drm file_priv for the fbdev emulation (and treat it more like
> > any other kms client), but I think that's way too much work when this
> > simple patch here gets the job done.
>
> I think first, a different question needs to be answered:
>
> include/uapi/linux/fb.h:
>
> struct fb_fix_screeninfo {
>          char id[16];                    /* identification string eg "TT Builtin" */
>          unsigned long smem_start;       /* Start of frame buffer mem */
>                                          /* (physical address) */
>
> Should a DMA address be exposed through smem_start, rather than a
> physical address as the long-standing documentation quoted above
> has stated?
>
> Is it, in fact, a driver bug to store something that isn't a physical
> address there?

We could also go into whether it's right to store even a physical 
address in something which isn't necessarily big enough...

After the time I spent in the bowels of the fbdev code figuring out this 
crash, I fear that if we dig too deep we may awaken something in the 
darkness ;)

Robin.