dri-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Max Staudt <mstaudt@suse.de>
Cc: b.zolnierkie@samsung.com, linux-fbdev@vger.kernel.org,
	michal@markovi.net, sndirsch@suse.com, oneukum@suse.com,
	tiwai@suse.com, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, bernhard.rosenkranzer@linaro.org,
	philm@manjaro.org
Subject: Re: [RFC PATCH v2 03/13] bootsplash: Flush framebuffer after drawing
Date: Tue, 19 Dec 2017 14:57:15 +0100
Message-ID: <20171219135715.GG26573@phenom.ffwll.local> (raw)
In-Reply-To: <8bbb0497-4a10-2f81-0040-6e7cd4e7353c@suse.de>

On Tue, Dec 19, 2017 at 02:34:22PM +0100, Max Staudt wrote:
> On 12/19/2017 01:23 PM, Daniel Vetter wrote:
> > On Thu, Dec 14, 2017 at 04:36:49PM +0100, Max Staudt wrote:
> >> 2) We need to go out of the way when a graphical application starts, and
> >> come back when it's done. fbcon already has the logic for this, and
> >> fbcon is also the thing we're trying to hide. So it seems natural to add
> >> the splash on top of fbcon - at least for now.
> > 
> > And this "automatically disappear" semantics is horribly ill-defined
> > between fbdev and native kms. So you're not really solving a problem,
> > you're just not noticing the hacks because they're one layer removed (in
> > the fbdev emulation code).
> That's a general complaint about fbcon and/or the fbdev emulation in KMS drivers, right?
> 
> I can't see how it relates to my bootsplash, as I'm just replacing
> fbcon's output, wherever fbcon desires to draw at the given moment, and
> in no other case.
> 
> So when a graphical application sets the VT mode to KD_GRAPHICS, we get
> a call to do_blank_screen(), and then fbcon -and thus the bootsplash- is
> muted. The ioctl API has always been like this, and it's not specific to
> the patch in question.
> 
> Similarly, when a graphical application allocates a framebuffer via the
> KMS ioctl()s, and selects it for scanout, the driver will display that
> instead of the framebuffer it has allocated internally for the fbdev
> emulation.
> 
> 
> >> 3) I can't use DRM from the kernel, for the same reason for which there
> >> is no "drmcon" to supplant fbcon: There is no interface to reserve
> >> framebuffer memory from kernel space: To get memory for a framebuffer,
> >> one needs to have a struct file that is passed through the DRM stack
> >> down into the drivers.
> > 
> > On recent kernels you only need a struct drm_file, not a struct file. That
> > can be NULL. We've done this to make drmcon possible/easier.
> 
> Oh that's cool, I missed that. Thanks!
> 
> Maybe a fb2drm compat layer will become reality, after all.
> 
> The bootsplash code is fairly straightforward to port to a future drmcon, and I'm happy to make the changes once drmcon is available.
> But for now, we only have fbcon. And a *lot* of FB drivers. And we want them to show a bootsplash instead of text. So that's where the bootsplash needs to hook into.
> 
> 
> >> If this interface existed, then there could be a generic "fb2drm"
> >> translation layer, and we would no longer need FB compatibility code in
> >> each KMS driver. Actually, I tried to implement this translation layer a
> >> year ago, and hit too many walls.
> > 
> > We're pretty much there already I think. The reason it's not entirely gone
> > is that there's some nasty interactions between drm and the fbdev
> > emulation, and just having a pile of drivers that aren't too trivial to
> > convert.
> 
> Sounds like the state of the art last year - drm_file in most cases, but
> struct file deep in the drivers :(

Where do drivers deal with struct file deep down?

> >> 4) I don't fully understand what you'd like me to do. Last time I tried
> >> to add a new entry to the fbops struct (namely fb_open_adj_file()), you
> >> told me not to touch the framebuffer subsystem anymore, as it is meant
> >> to die and driver developers shall use KMS instead. Have I
> >> misunderstood?
> > 
> > I still don't like anyone adding features to fbdev :-)
> 
> So I must not touch fbops, correct?

The problem is that defio is totally not how a real driver works. So
preferrably bootsplash would use kms directly, and use the explict dirtyfb
callback. But if you insist on using fbdev, then I think the beast course
here is to wire up a new fb_ops->flush callback.

Note that you only need to type the 1 trivial implementation for the drm
fbdev emulation, as long as the callback is optional. Trying to make defio
work correctly, as fbdev assumes it should work, in all cases, on top of
drm is imo an entirely pointless endeavour.

> >> Something like fb->flush() to finish kernel space accesses would be nice
> >> to have, but would need to be implemented for all affected drivers
> >> separately. The copy op hack is ugly, but solves the problem
> >> generically.
> > 
> > Well, with defio being the hack it is (and because of that, a bunch of drm
> > drivers not really supporting it) I'm not sure things actually work better
> > without all this.
> 
> I don't understand what you mean.
> 
> What I do know is that fb_defio is here, and it's here to stay because some drivers need it.
> 
> What I also know is that I need to flush the screen after drawing my bootsplash.

Yes, so lets ignore defio and do the flushing correctly, at least for kms
drivers.

> >> What shall I do?
> >>
> >> Shall I add a new FB op for flushing when writing to the raw memory from the kernel?
> >> As far as I can see, it would be needed for defio drivers only, is that correct?
> > 
> > Yes, which are kinda horrible anyway. I guess you could at least not do
> > all these hacks if it's not a defio driver.
> 
> Again, I don't understand.
> 
> In my patch (see below), I explicitly check for info->fbdefio, as well
> as three known broken drmfb emulations. I don't do the copy hack on any
> other device.

Yeah, and if we'd to the explicit flush, you wouldn't even need to check
for that. So

if (fbops->flush)
	fbops->flush(); /* this covers all drm drivers */
else if (fb->defio)
	copyarea hack, if you really still need to support some defio
	fbdev drivers, but really I think that's questionable
else
	; /* nothing */

> So, what shall I do? As it is, the hack is already specific to devices that really, really need it.
> 
> Would you like me to extend the FB API or not?

Yes. Well for real I'd like you to do kms, so maybe you need to explain
why exactly you absolutely have to use fbdev (aka which driver isn't
supported by drm that you want to enable this on).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

  reply index

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-13 19:47 [RFC PATCH v2 00/13] Kernel based bootsplash Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 01/13] bootsplash: Initial implementation showing black screen Max Staudt
2017-12-13 23:55   ` Randy Dunlap
2017-12-14 15:37     ` Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 02/13] bootsplash: Add file reading and picture rendering Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 03/13] bootsplash: Flush framebuffer after drawing Max Staudt
2017-12-13 21:35   ` Daniel Vetter
2017-12-14 15:36     ` Max Staudt
2017-12-19 12:23       ` Daniel Vetter
2017-12-19 13:34         ` Max Staudt
2017-12-19 13:57           ` Daniel Vetter [this message]
2017-12-19 14:07             ` Oliver Neukum
2017-12-31 12:53               ` Alan Cox
2018-01-03 18:04                 ` Max Staudt
2017-12-19 15:41             ` Max Staudt
2017-12-19 16:02               ` Daniel Vetter
2017-12-19 16:23                 ` Max Staudt
2017-12-20  9:45                   ` Daniel Vetter
2017-12-19 16:09               ` Daniel Vetter
2017-12-19 16:26                 ` Max Staudt
2017-12-19 21:01               ` Ray Strode
2017-12-20 13:14                 ` Max Staudt
2017-12-20 15:35                   ` Ray Strode
2017-12-20 16:52                     ` Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 04/13] bootsplash: Add corner positioning Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 05/13] bootsplash: Add animation support Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 06/13] vt: Redraw bootsplash fully on console_unblank Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 07/13] vt: Add keyboard hook to disable bootsplash Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 08/13] sysrq: Disable bootsplash on SAK Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 09/13] fbcon: Disable bootsplash on oops Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 10/13] Documentation: Add bootsplash main documentation Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 11/13] bootsplash: sysfs entries to load and unload files Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 12/13] tools/bootsplash: Add a basic splash file creation tool Max Staudt
2017-12-13 19:47 ` [RFC PATCH v2 13/13] tools/bootsplash: Add script and data to create sample file Max Staudt
2017-12-13 19:52 ` [RFC PATCH v2 00/13] Kernel based bootsplash Max Staudt
2017-12-19 16:16 ` Daniel Vetter
2017-12-19 17:04   ` Max Staudt
2017-12-19 17:26     ` Daniel Vetter
2017-12-19 18:40       ` Max Staudt
2017-12-20  9:43         ` Daniel Vetter
2017-12-20 10:06           ` Neil Armstrong
2017-12-20 10:14             ` Daniel Vetter
2017-12-20 14:55               ` Max Staudt
2017-12-20 15:11                 ` Daniel Vetter
2017-12-20 15:19                   ` Daniel Vetter
2017-12-20 15:22                     ` Daniel Vetter
2017-12-20 16:23                     ` Max Staudt
2017-12-20 16:15                   ` Max Staudt
2017-12-31 12:44                   ` Alan Cox
2018-01-03 18:00                     ` Max Staudt
2017-12-20 14:16             ` Max Staudt
2017-12-20 14:10           ` Max Staudt
2017-12-31 12:35         ` Alan Cox
2018-01-03 17:56           ` Max Staudt
2017-12-19 20:30     ` Ray Strode
2017-12-20 13:03       ` Max Staudt
2017-12-20 15:21         ` Ray Strode
2017-12-20 16:44           ` Max Staudt
2017-12-21 14:51             ` Ray Strode
2017-12-21 16:32               ` Max Staudt
2017-12-20 11:08   ` Johannes Thumshirn
2017-12-20 11:22     ` Daniel Stone
2017-12-20 12:48       ` Johannes Thumshirn
2017-12-29 17:13   ` Jani Nikula
2018-01-03 17:38     ` Max Staudt
2017-12-21  9:48 ` Daniel Vetter
2017-12-21 16:52   ` Max Staudt
2017-12-21 15:00 ` Philip Müller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171219135715.GG26573@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=b.zolnierkie@samsung.com \
    --cc=bernhard.rosenkranzer@linaro.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=michal@markovi.net \
    --cc=mstaudt@suse.de \
    --cc=oneukum@suse.com \
    --cc=philm@manjaro.org \
    --cc=sndirsch@suse.com \
    --cc=tiwai@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git