Re: [PATCH] drm/ttm: don't set page->mapping

[Daniel Stone]

Hi,

On Wed, 25 Nov 2020 at 18:06, Jason Gunthorpe  wrote:
> On Wed, Nov 25, 2020 at 05:28:32PM +0100, Daniel Vetter wrote:
> > Apologies again, this shouldn't have been included. But at least I
> > have an idea now why this patch somehow was included in the git
> > send-email. Lovely interface :-/
>
> I wrote a bit of a script around this because git send-email just too
> hard to use
>
> The key workflow change I made was to have it prepare all the emails
> to send and open them in an editor for review - exactly as they would
> be sent to the lists.
>
> It uses a empty 'cover-letter' commit and automatically transforms it
> into exactly the right stuff. Keeps track of everything you send in
> git, and there is a little tool to auto-run git range-diff to help
> build change logs..

This sounds a fair bit like patman, which does something similar and
also lets you annotate commit messages with changelogs.

But of course, suggesting different methods of carving patches into
stone tablets to someone who's written their own, is even more of a
windmill tilt than rDMA. ;)

Cheers,
Daniel

Re: [PATCH 00/49] DRM driver for Hikey 970

[Daniel Stone]

Hi Mauro,

On Tue, 25 Aug 2020 at 12:30, Mauro Carvalho Chehab
 wrote:
> Sorry, but I can't agree that review is more important than to be able
> to properly indicate copyrights in a valid way at the legal systems that
> it would apply ;-)

The way to properly indicate copyright coverage is to insert a
copyright statement in the file. This has been the accepted way of
communicating copyright notices since approximately the dawn of time.
The value of the 'author' field within a chain of git commits does not
have privileged legal value.

If what you were saying is true, it would be impossible for any
project to copy code from any other project, unless they did git
filter-branch and made sure to follow renames too. As others have
noted, it would also be impossible for any patches to be developed
collaboratively by different copyright holders, or for maintainers to
apply changes.

This is accepted community practice and has passed signoffs from a
million different lawyers and copyright holders. If you wish to break
with this and do something different, the onus is on you to provide
the community with _specific_ legal advice; if this is accepted, the
development model would have to drastically change in the presence of
single pieces of code developed by multiple distinct copyright
holders.

Cheers,
Daniel

Re: [Intel-gfx] [PATCH 03/18] dma-fence: basic lockdep annotations

[Daniel Stone]

Hi,
Jumping in after a couple of weeks where I've paged most everything
out of my brain ...

On Fri, 19 Jun 2020 at 10:43, Daniel Vetter  wrote:
> On Fri, Jun 19, 2020 at 10:13:35AM +0100, Chris Wilson wrote:
> > > The proposed patches might very well encode the wrong contract, that's
> > > all up for discussion. But fundamentally questioning that we need one
> > > is missing what upstream is all about.
> >
> > Then I have not clearly communicated, as my opinion is not that
> > validation is worthless, but that the implementation is enshrining a
> > global property on a low level primitive that prevents it from being
> > used elsewhere. And I want to replace completion [chains] with fences, and
> > bio with fences, and closures with fences, and what other equivalencies
> > there are in the kernel. The fence is as central a locking construct as
> > struct completion and deserves to be a foundational primitive provided
> > by kernel/ used throughout all drivers for discrete problem domains.
> >
> > This is narrowing dma_fence whereby adding
> >   struct lockdep_map *dma_fence::wait_map
> > and annotating linkage, allows you to continue to specify that all
> > dma_fence used for a particular purpose must follow common rules,
> > without restricting the primitive for uses outside of this scope.
>
> Somewhere else in this thread I had discussions with Jason Gunthorpe about
> this topic. It might maybe change somewhat depending upon exact rules, but
> his take is very much "I don't want dma_fence in rdma". Or pretty close to
> that at least.
>
> Similar discussions with habanalabs, they're using dma_fence internally
> without any of the uapi. Discussion there has also now concluded that it's
> best if they remove them, and simply switch over to a wait_queue or
> completion like every other driver does.
>
> The next round of the patches already have a paragraph to at least
> somewhat limit how non-gpu drivers use dma_fence. And I guess actual
> consensus might be pointing even more strongly at dma_fence being solely
> something for gpus and closely related subsystem (maybe media) for syncing
> dma-buf access.
>
> So dma_fence as general replacement for completion chains I think just
> wont happen.
>
> What might make sense is if e.g. the lockdep annotations could be reused,
> at least in design, for wait_queue or completion or anything else
> really. I do think that has a fair chance compared to the automagic
> cross-release annotations approach, which relied way too heavily on
> guessing where barriers are. My experience from just a bit of playing
> around with these patches here and discussing them with other driver
> maintainers is that accurately deciding where critical sections start and
> end is a job for humans only. And if you get it wrong, you will have a
> false positive.
>
> And you're indeed correct that if we'd do annotations for completions and
> wait queues, then that would need to have a class per semantically
> equivalent user, like we have lockdep classes for mutexes, not just one
> overall.
>
> But dma_fence otoh is something very specific, which comes with very
> specific rules attached - it's not a generic wait_queue at all. Originally
> it did start out as one even, but it is a very specialized wait_queue.
>
> So there's imo two cases:
>
> - Your completion is entirely orthogonal of dma_fences, and can never ever
>   block a dma_fence. Don't use dma_fence for this, and no problem. It's
>   just another wait_queue somewhere.
>
> - Your completion can eventually, maybe through lots of convolutions and
>   depdencies, block a dma_fence. In that case full dma_fence rules apply,
>   and the only thing you can do with a custom annotation is make the rules
>   even stricter. E.g. if a sub-timeline in the scheduler isn't allowed to
>   take certain scheduler locks. But the userspace visible/published fence
>   do take them, maybe as part of command submission or retirement.
>   Entirely hypotethical, no idea any driver actually needs this.

I don't claim to understand the implementation of i915's scheduler and
GEM handling, and it seems like there's some public context missing
here. But to me, the above is a good statement of what I (and a lot of
other userspace) have been relying on - that dma-fence is a very
tightly scoped thing which is very predictable but in extremis.

It would be great to have something like this enshrined in dma-fence
documentation, visible to both kernel and external users. The
properties we've so far been assuming for the graphics pipeline -
covering production & execution of vertex/fragment workloads on the
GPU, framebuffer display, and to the extent this is necessary
involving compute - are something like this:

A single dma-fence with no dependencies represents (the tail of) a
unit of work, which has been all but committed to the hardware. Once
committed to the hardware, this work will complete (successfully or in
error) in bounded time. The unit of work referred to 

Re: [git pull] drm for 5.8-rc1

[Daniel Stone]

Hi,

On Wed, 1 Jul 2020 at 20:45, James Jones  wrote:
> OK, I think I see what's going on.  In the Xorg modesetting driver, the
> logic is basically:
>
> if (gbm_has_modifiers && DRM_CAP_ADDFB2_MODIFIERS != 0) {
>drmModeAddFB2WithModifiers(..., gbm_bo_get_modifier(bo->gbm));
> } else {
>drmModeAddFB(...);
> }

I read this thread expecting to explain the correct behaviour we
implement in Weston and how modesetting needs to be fixed, but ...
that seems OK to me? As long as `gbm_has_modifiers` is a proxy for 'we
used gbm_(bo|surface)_create_with_modifiers to allocate the buffer'.

> There's no attempt to verify the DRM-KMS device supports the modifier,
> but then, why would there be?  GBM presumably chose a supported modifier
> at buffer creation time, and we don't know which plane the FB is going
> to be used with yet.  GBM doesn't actually ask the kernel which
> modifiers it supports here either though.

Right, it doesn't ask, because userspace tells it which modifiers to
use. The correct behaviour is to take the list from the KMS
`IN_FORMATS` property and then pass that to
`gbm_(bo|surface)_create_with_modifiers`; GBM must then select from
that list and only that list. If that call does not succeed and Xorg
falls back to `gbm_surface_create`, then it must not call
`gbm_bo_get_modifier` - so that would be a modesetting bug. If that
call does succeed and `gbm_bo_get_modifier` subsequently reports a
modifier which was not in the list, that's a Mesa driver bug.

> It just goes into Mesa via
> DRI and reports the modifier (unpatched) Mesa chose on its own.  Mesa
> just hard-codes the modifiers in its driver backends since its thinking
> in terms of a device's 3D engine, not display.  In theory, Mesa's DRI
> drivers could query KMS for supported modifiers if allocating from GBM
> using the non-modifiers path and the SCANOUT flag is set (perhaps some
> drivers do this or its equivalent?  Haven't checked.), but that seems
> pretty gnarly and doesn't fix the modifier-based GBM allocation path
> AFAIK.  Bit of a mess.

Two options for GBM users:
* call gbm_*_create_with_modifiers, it succeeds, call
gbm_bo_get_modifier, pass modifier into AddFB
* call gbm_*_create (without modifiers), it succeeds, do not call
gbm_bo_get_modifier, do not pass a modifier into AddFB

Anything else is a bug in the user. Note that falling back from 1 to 2
is fine: if `gbm_*_create_with_modifiers()` fails, you can fall back
to the non-modifier path, provided you don't later try to get a
modifier back out.

> For a quick userspace fix that could probably be pushed out everywhere
> (Only affects Xorg server 1.20+ AFAIK), just retrying
> drmModeAddFB2WithModifiers() without the DRM_MODE_FB_MODIFIERS flag on
> failure should be sufficient.

This would break other drivers.

> Still need to verify as I'm having
> trouble wrangling my Xorg build at the moment and I'm pressed for time.
> A more complete fix would be quite involved, as modesetting isn't really
> properly plumbed to validate GBM's modifiers against KMS planes, and it
> doesn't seem like GBM/Mesa/DRI should be responsible for this as noted
> above given the general modifier workflow/design.
>
> Most importantly, options I've considered for fixing from the kernel side:
>
> -Accept "legacy" modifiers in nouveau in addition to the new modifiers,
> though avoid reporting them to userspace as supported to avoid further
> proliferation.  This is pretty straightforward.  I'll need to modify
> both the AddFB2 handler (nouveau_validate_decode_mod) and the mode set
> plane validation logic (nv50_plane_format_mod_supported), but it should
> end up just being a few lines of code.

I do think that they should also be reported to userspace if they are
accepted. Other users can and do look at the modifier list to see if
the buffer is acceptable for a given plane, so the consistency is good
here. Of course, in Mesa you would want to prioritise the new
modifiers over the legacy ones, and not allocate or return the legacy
ones unless that was all you were asked for. This would involve
tracking the used modifier explicitly through Mesa, rather than
throwing it away at alloc time and then later divining it from the
tiling mode.

Cheers,
Daniel

Re: [PATCH v2 0/5] 180 degrees rotation support for NVIDIA Tegra DRM

[Daniel Stone]

Hi,

On Tue, 16 Jun 2020 at 22:16, Dmitry Osipenko  wrote:
> The panel's orientation could be parsed by any panel driver and then
> assigned as the connector's property in order to allow userspace/FB-core
> to decide what to do with the rotated display. Apparently upstream
> kernel supports only that one Samsung device which has display panel
> mounted upside-down and it already uses the custom DT properties for
> achieving the 180 rotation. So I don't quite see any panel drivers that
> instantly could benefit from using the rotation property. Perhaps I can
> add the orientation support to the panel-simple driver, but will it be
> useful to anyone?

Yes, exposing it to userspace is helpful, since Weston at least will
parse the property and then apply the correct transform:
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/315

Cheers,
Daniel

Re: [Intel-gfx] [PATCH 03/18] dma-fence: basic lockdep annotations

[Daniel Stone]

Hi,

On Thu, 11 Jun 2020 at 09:44, Dave Airlie  wrote:
> On Thu, 11 Jun 2020 at 18:01, Chris Wilson  wrote:
> > Introducing a global lockmap that cannot capture the rules correctly,
>
> Can you document the rules all drivers should be following then,
> because from here it looks to get refactored every version of i915,
> and it would be nice if we could all aim for the same set of things
> roughly. We've already had enough problems with amdgpu vs i915 vs
> everyone else with fences, if this stops that in the future then I'd
> rather we have that than just some unwritten rules per driver and
> untestable.

As someone who has sunk a bunch of work into explicit-fencing
awareness in my compositor so I can never be blocked, I'd be
disappointed if the infrastructure was ultimately pointless because
the documented fencing rules were \_o_/ or thereabouts. Lockdep
definitely isn't my area of expertise so I can't comment on the patch
per se, but having something to ensure we don't hit deadlocks sure
seems a lot better than nothing.

Cheers,
Daniel

Re: [PATCH] lib/scatterlist: Provide a DMA page iterator

[Daniel Stone]

On Wed, 16 Jan 2019 at 16:06, h...@lst.de  wrote:
> On Wed, Jan 16, 2019 at 07:28:13AM +, Koenig, Christian wrote:
> > To summarize once more: We have an array of struct pages and want to
> > coherently map that to a device.
>
> And the answer to that is very simple: you can't.  What is so hard
> to understand about?  If you want to map arbitrary memory it simply
> can't be done in a coherent way on about half of our platforms.
>
> > If that is not possible because of whatever reason we want to get an
> > error code or even not load the driver from the beginning.
>
> That is a bullshit attitude.  Just like everyone else makes their
> drivers work you should not be lazy.

Can you not talk to people like that? Even if you think that is an OK
way to treat anyone - which it isn't, certainly not on dri-devel@ with
the fd.o Code of Conduct, and not according to the kernel's either - I
have absolutely no idea how you can look at the work the AMD people
have put in over many years and conclude that they're 'lazy'.

If this makes you so angry, step back from the keyboard for a few
minutes, and if you still can't participate in reasonable discussion
like an adult, maybe step out of the thread entirely.

Re: Linux 4.17-rc7

[Daniel Stone]

Hi Pavel,

On 30 May 2018 at 12:17, Pavel Machek  wrote:
>> The bulk of it is really pretty trivial one-liners, and nothing looks
>> particularly scary. Let's see how next week looks, but if nothing really
>> happens I suspect we can make do without an rc8.
>>
>> Shortlog appended as usual. Go out and test.
>
> Any chance to still get in this one?
>
> https://github.com/freedesktop/drm-misc/commit/2bc5ff0bdc00d81d719dad74589317a260d583ed
>
> ...it fixes display on Nokia N900, and display is rather important for
> cellphone.

The pull request was sent to Dave just yesterday:
https://lists.freedesktop.org/archives/dri-devel/2018-May/178606.html

Cheers,
Daniel

Re: Linux 4.17-rc7

[Daniel Stone]

Hi Pavel,

On 30 May 2018 at 12:17, Pavel Machek  wrote:
>> The bulk of it is really pretty trivial one-liners, and nothing looks
>> particularly scary. Let's see how next week looks, but if nothing really
>> happens I suspect we can make do without an rc8.
>>
>> Shortlog appended as usual. Go out and test.
>
> Any chance to still get in this one?
>
> https://github.com/freedesktop/drm-misc/commit/2bc5ff0bdc00d81d719dad74589317a260d583ed
>
> ...it fixes display on Nokia N900, and display is rather important for
> cellphone.

The pull request was sent to Dave just yesterday:
https://lists.freedesktop.org/archives/dri-devel/2018-May/178606.html

Cheers,
Daniel

Re: [PATCHv3 3/8] drm/omap: add support for manually updated displays

[Daniel Stone]

Hi Tony!

On 20 April 2018 at 15:25, Tony Lindgren <t...@atomide.com> wrote:
> * Daniel Stone <dan...@fooishbar.org> [180420 10:21]:
>> On 20 April 2018 at 08:09, Tomi Valkeinen <tomi.valkei...@ti.com> wrote:
>> > It's actually not quite clear to me how manual update displays work with
>> > DRM...
>> >
>> > As far as I see, we have essentially two cases: 1) single buffering,
>> > where the userspace must set an area in the fb dirty, which then
>> > triggers the update, 2) multi buffering, which doesn't need fb dirty,
>> > but just a page flip which triggers the update.
>> >
>> > In the 2) case (which I think is the optimal case which all the modern
>> > apps should use), there's no need for delayed work or any work, and the
>> > code flow should be very similar to the auto-update model.
>>
>> Correct. There's been talk (and I think patches?) of adding a
>> per-plane dirty property, so userspace can as an optimisation inform
>> the kernel of the area changed between frames. But short of that, a
>> pageflip needs to trigger a full-plane update, with no dirtyfb
>> required.
>
> For per-plane dirty property patches, which ones do you refer to?

Here's the latest iteration of that series:
https://lists.freedesktop.org/archives/dri-devel/2018-April/171900.html
<1522885748-67122-1-git-send-email-dra...@vmware.com>

> Then for xorg, there's my second attempt on fixing the command mode
> rotation at [0]. Not sure if that's enough for a fix?
>
> It seems not very efficient to me and I don't really know where
> the the per crtc dirty flag should be stored..

I try to deny all knowledge of X11 these days, I'm afraid.

Cheers,
Daniel

Re: [PATCHv3 3/8] drm/omap: add support for manually updated displays

[Daniel Stone]

Hi Tony!

On 20 April 2018 at 15:25, Tony Lindgren  wrote:
> * Daniel Stone  [180420 10:21]:
>> On 20 April 2018 at 08:09, Tomi Valkeinen  wrote:
>> > It's actually not quite clear to me how manual update displays work with
>> > DRM...
>> >
>> > As far as I see, we have essentially two cases: 1) single buffering,
>> > where the userspace must set an area in the fb dirty, which then
>> > triggers the update, 2) multi buffering, which doesn't need fb dirty,
>> > but just a page flip which triggers the update.
>> >
>> > In the 2) case (which I think is the optimal case which all the modern
>> > apps should use), there's no need for delayed work or any work, and the
>> > code flow should be very similar to the auto-update model.
>>
>> Correct. There's been talk (and I think patches?) of adding a
>> per-plane dirty property, so userspace can as an optimisation inform
>> the kernel of the area changed between frames. But short of that, a
>> pageflip needs to trigger a full-plane update, with no dirtyfb
>> required.
>
> For per-plane dirty property patches, which ones do you refer to?

Here's the latest iteration of that series:
https://lists.freedesktop.org/archives/dri-devel/2018-April/171900.html
<1522885748-67122-1-git-send-email-dra...@vmware.com>

> Then for xorg, there's my second attempt on fixing the command mode
> rotation at [0]. Not sure if that's enough for a fix?
>
> It seems not very efficient to me and I don't really know where
> the the per crtc dirty flag should be stored..

I try to deny all knowledge of X11 these days, I'm afraid.

Cheers,
Daniel

Re: [PATCHv3 3/8] drm/omap: add support for manually updated displays

[Daniel Stone]

Hi Tomi,

On 20 April 2018 at 08:09, Tomi Valkeinen  wrote:
> It's actually not quite clear to me how manual update displays work with
> DRM...
>
> As far as I see, we have essentially two cases: 1) single buffering,
> where the userspace must set an area in the fb dirty, which then
> triggers the update, 2) multi buffering, which doesn't need fb dirty,
> but just a page flip which triggers the update.
>
> In the 2) case (which I think is the optimal case which all the modern
> apps should use), there's no need for delayed work or any work, and the
> code flow should be very similar to the auto-update model.

Correct. There's been talk (and I think patches?) of adding a
per-plane dirty property, so userspace can as an optimisation inform
the kernel of the area changed between frames. But short of that, a
pageflip needs to trigger a full-plane update, with no dirtyfb
required.

Cheers,
Daniel

Re: [PATCHv3 3/8] drm/omap: add support for manually updated displays

[Daniel Stone]

Hi Tomi,

On 20 April 2018 at 08:09, Tomi Valkeinen  wrote:
> It's actually not quite clear to me how manual update displays work with
> DRM...
>
> As far as I see, we have essentially two cases: 1) single buffering,
> where the userspace must set an area in the fb dirty, which then
> triggers the update, 2) multi buffering, which doesn't need fb dirty,
> but just a page flip which triggers the update.
>
> In the 2) case (which I think is the optimal case which all the modern
> apps should use), there's no need for delayed work or any work, and the
> code flow should be very similar to the auto-update model.

Correct. There's been talk (and I think patches?) of adding a
per-plane dirty property, so userspace can as an optimisation inform
the kernel of the area changed between frames. But short of that, a
pageflip needs to trigger a full-plane update, with no dirtyfb
required.

Cheers,
Daniel

Re: [RfC PATCH] Add udmabuf misc device

[Daniel Stone]

Hi Gerd,

On 14 March 2018 at 08:03, Gerd Hoffmann  wrote:
>> Either mlock account (because it's mlocked defacto), and get_user_pages
>> won't do that for you.
>>
>> Or you write the full-blown userptr implementation, including mmu_notifier
>> support (see i915 or amdgpu), but that also requires Christian Königs
>> latest ->invalidate_mapping RFC for dma-buf (since atm exporting userptr
>> buffers is a no-go).
>
> I guess I'll look at mlock accounting for starters then.  Easier for
> now, and leaves the door open to switch to userptr later as this should
> be transparent to userspace.

Out of interest, do you have usecases for full userptr support? Maybe
another way would be to allow creation of dmabufs from memfds.

Cheers,
Daniel

Re: [RfC PATCH] Add udmabuf misc device

[Daniel Stone]

Hi Gerd,

On 14 March 2018 at 08:03, Gerd Hoffmann  wrote:
>> Either mlock account (because it's mlocked defacto), and get_user_pages
>> won't do that for you.
>>
>> Or you write the full-blown userptr implementation, including mmu_notifier
>> support (see i915 or amdgpu), but that also requires Christian Königs
>> latest ->invalidate_mapping RFC for dma-buf (since atm exporting userptr
>> buffers is a no-go).
>
> I guess I'll look at mlock accounting for starters then.  Easier for
> now, and leaves the door open to switch to userptr later as this should
> be transparent to userspace.

Out of interest, do you have usecases for full userptr support? Maybe
another way would be to allow creation of dmabufs from memfds.

Cheers,
Daniel

Re: [PATCH 08/10] drm/fourcc: Add definitions for Allwinner vendor and MB32 tiled format

[Daniel Stone]

Hi Paul,

On 21 March 2018 at 15:29, Paul Kocialkowski
 wrote:
> +/*
> + * Allwinner "MB32" tiled format
> + *
> + * This is the primary layout coming out of the VPU, where pixels are tiled
> + * 32x32.
> + */

Can you please be a bit more specific here, following the other
examples? In particular, it should list whether the tile order is row-
or column-major.

Cheers,
Daniel

Re: [PATCH 08/10] drm/fourcc: Add definitions for Allwinner vendor and MB32 tiled format

[Daniel Stone]

Hi Paul,

On 21 March 2018 at 15:29, Paul Kocialkowski
 wrote:
> +/*
> + * Allwinner "MB32" tiled format
> + *
> + * This is the primary layout coming out of the VPU, where pixels are tiled
> + * 32x32.
> + */

Can you please be a bit more specific here, following the other
examples? In particular, it should list whether the tile order is row-
or column-major.

Cheers,
Daniel

Re: [PATCH] drm/vc4: Add support for SAND modifier.

[Daniel Stone]

Hey Eric,

On 3 March 2018 at 01:34, Eric Anholt <e...@anholt.net> wrote:
> Ccing a couple of folks who are likely to have opinions about
> drm_fourcc.h additions (Do we have enough docs?  Are the macros OK?),
> and Bootlin who are likely reviewers.
>
> The plan is to use these modifiers in VC4 GL imports as well, and for
> buffers coming from the v4l2 mmal camera driver.  You can find a demo
> using this in KMS planes at
> https://github.com/anholt/drm_mmal/commit/sand for now.

I had a dig through and this seems like the most sensible thing to do
if you have a reasonable variety of tile heights. If you only see a
couple of combinations in the wild, then hardcoding them as separate
modifiers might make things easier than hiving off 24 bits. If you do
keep the split though, and especially if you're envisioning future
flexible tile formats, maybe something like an 8 code / 48 params
split would make more sense.

Either way, those are just my opinions (you did ask), and I don't see
anything really objectionable, so if you think that's a good split
then this is:
Acked-by: Daniel Stone <dani...@collabora.com>

Cheers,
Daniel

Re: [PATCH] drm/vc4: Add support for SAND modifier.

[Daniel Stone]

Hey Eric,

On 3 March 2018 at 01:34, Eric Anholt  wrote:
> Ccing a couple of folks who are likely to have opinions about
> drm_fourcc.h additions (Do we have enough docs?  Are the macros OK?),
> and Bootlin who are likely reviewers.
>
> The plan is to use these modifiers in VC4 GL imports as well, and for
> buffers coming from the v4l2 mmal camera driver.  You can find a demo
> using this in KMS planes at
> https://github.com/anholt/drm_mmal/commit/sand for now.

I had a dig through and this seems like the most sensible thing to do
if you have a reasonable variety of tile heights. If you only see a
couple of combinations in the wild, then hardcoding them as separate
modifiers might make things easier than hiving off 24 bits. If you do
keep the split though, and especially if you're envisioning future
flexible tile formats, maybe something like an 8 code / 48 params
split would make more sense.

Either way, those are just my opinions (you did ask), and I don't see
anything really objectionable, so if you think that's a good split
then this is:
Acked-by: Daniel Stone 

Cheers,
Daniel

Re: [git pull] drm pull for v4.16-rc1

[Daniel Stone]

On 2 February 2018 at 02:50, Dave Airlie  wrote:
> On 2 February 2018 at 12:44, Linus Torvalds
>  wrote:
>> On Thu, Feb 1, 2018 at 6:22 PM, Dave Airlie  wrote:
>>>
>>> Turned out I was running on wayland instead of X.org and my cut-n-paste from
>>> gedit to firefox got truncated, wierd. I'll go annoy some people, and make 
>>> sure
>>> it doesn't happen again.
>>
>> Heh, so there's some Wayland clipboard buffer limit.
>
> Yup or some bug getting the second chunks across from one place to another.

The transfer part of Wayland's clipboard protocol is an FD between
both clients for them to send data directly. But Firefox isn't yet
native, and I can fully believe that GNOME Shell's Xwayland clipboard
translator isn't perfect.

>> But that reminds me: is there any *standard* tool to programmatically
>> feed into the clipboard?
>>
>> I occasionally do things like
>>
>> git shortlog A..B | xsel
>>
>> in order to then paste it into some browser window or other.
>>
>> And sure, that works well. But I do it seldom enough that I never
>> remember the command, and half the time it's not even installed
>> because I've switched machines or something, and xsel is always some
>> add-on.
>>
>> What's the thing "real" X people do/use?
>
> I use gedit to move things from files to clip now, for mostly the same 
> reasons,
> I know it's installed usually. xclip and xsel are two utilities I know
> off, but I don'
> think anything gets installed by default.

That's the state of the art for X11.

Cheers,
Daniel

Re: [git pull] drm pull for v4.16-rc1

[Daniel Stone]

On 2 February 2018 at 02:50, Dave Airlie  wrote:
> On 2 February 2018 at 12:44, Linus Torvalds
>  wrote:
>> On Thu, Feb 1, 2018 at 6:22 PM, Dave Airlie  wrote:
>>>
>>> Turned out I was running on wayland instead of X.org and my cut-n-paste from
>>> gedit to firefox got truncated, wierd. I'll go annoy some people, and make 
>>> sure
>>> it doesn't happen again.
>>
>> Heh, so there's some Wayland clipboard buffer limit.
>
> Yup or some bug getting the second chunks across from one place to another.

The transfer part of Wayland's clipboard protocol is an FD between
both clients for them to send data directly. But Firefox isn't yet
native, and I can fully believe that GNOME Shell's Xwayland clipboard
translator isn't perfect.

>> But that reminds me: is there any *standard* tool to programmatically
>> feed into the clipboard?
>>
>> I occasionally do things like
>>
>> git shortlog A..B | xsel
>>
>> in order to then paste it into some browser window or other.
>>
>> And sure, that works well. But I do it seldom enough that I never
>> remember the command, and half the time it's not even installed
>> because I've switched machines or something, and xsel is always some
>> add-on.
>>
>> What's the thing "real" X people do/use?
>
> I use gedit to move things from files to clip now, for mostly the same 
> reasons,
> I know it's installed usually. xclip and xsel are two utilities I know
> off, but I don'
> think anything gets installed by default.

That's the state of the art for X11.

Cheers,
Daniel

Re: [RFC PATCH v2 00/13] Kernel based bootsplash

[Daniel Stone]

Hi Johannes,

On 20 December 2017 at 11:08, Johannes Thumshirn  wrote:
> On Tue, Dec 19, 2017 at 05:16:30PM +0100, Daniel Vetter wrote:
>> Ok I've realized that my assumptions about why you need this aren't
>> So from reading these patches it sounded like you want an in-kernel boot
>> splash because that would be on the display faster than a userspace one
>> like plymouth. That's the only reasons I can see for this (if there's
>> another good justification, please bring it up).
>>
>> I only know of very embedded setups (tv top boxes, in vehicle
>> entertainment) where that kind of "time to first image" really matters,
>> and those systems:
>> - have a real hw kms driver
>> - don't have fbcon or fbdev emulation enabled (except for some closed
>>   source stacks that are a bit slow to adapt to the new world, and we
>>   don't care about those in gfx).
>>
>> But from discussions it sounds like you very much want to use this on
>> servers, which makes 0 sense to me. On a server something like plymouth
>> should do a perfectly reasonable job.
>
> For _one_ reason we'd like to see this is (I was one of the requesters of this
> implementation), plymouth in it's infinite wisdom also grabs the serial (IPMI)
> console and escape characters in a screen log are (you can think of the rest
> of this sentence yourself I think).

You can set 'plymouth.ignore-serial-consoles' on your boot line to
disable this behaviour.

> Also plymouth grabs the escape character of HPE iLOs, which is a serious
> no-go.

I'm not entirely sure what this means, but maybe it's best addressed
as a bug report to the Plymouth developers? One of them is in this
thread.

Cheers,
Daniel

Re: [RFC PATCH v2 00/13] Kernel based bootsplash

[Daniel Stone]

Hi Johannes,

On 20 December 2017 at 11:08, Johannes Thumshirn  wrote:
> On Tue, Dec 19, 2017 at 05:16:30PM +0100, Daniel Vetter wrote:
>> Ok I've realized that my assumptions about why you need this aren't
>> So from reading these patches it sounded like you want an in-kernel boot
>> splash because that would be on the display faster than a userspace one
>> like plymouth. That's the only reasons I can see for this (if there's
>> another good justification, please bring it up).
>>
>> I only know of very embedded setups (tv top boxes, in vehicle
>> entertainment) where that kind of "time to first image" really matters,
>> and those systems:
>> - have a real hw kms driver
>> - don't have fbcon or fbdev emulation enabled (except for some closed
>>   source stacks that are a bit slow to adapt to the new world, and we
>>   don't care about those in gfx).
>>
>> But from discussions it sounds like you very much want to use this on
>> servers, which makes 0 sense to me. On a server something like plymouth
>> should do a perfectly reasonable job.
>
> For _one_ reason we'd like to see this is (I was one of the requesters of this
> implementation), plymouth in it's infinite wisdom also grabs the serial (IPMI)
> console and escape characters in a screen log are (you can think of the rest
> of this sentence yourself I think).

You can set 'plymouth.ignore-serial-consoles' on your boot line to
disable this behaviour.

> Also plymouth grabs the escape character of HPE iLOs, which is a serious
> no-go.

I'm not entirely sure what this means, but maybe it's best addressed
as a bug report to the Plymouth developers? One of them is in this
thread.

Cheers,
Daniel

Re: [RFC PATCH 1/6] drm: Add Content Protection property

[Daniel Stone]

Hi Pavel,

On 5 December 2017 at 17:34, Pavel Machek  wrote:
> Yes, so... This patch makes it more likely to see machines with locked
> down kernels, preventing developers from working with systems their
> own, running hardware. That is evil, and direct threat to Free
> software movement.
>
> Users compiling their own kernels get no benefit from it. Actually it
> looks like this only benefits Intel and Disney. We don't want that.

With all due respect, you can't claim to speak for the entire kernel
and FLOSS community of users and developers.

The feature is optional: it does not enforce additional constraints on
users, but exposes additional functionality already present in
hardware, for those who wish to opt in to it. Those who wish to avoid
it can do so, by simply not making active use of it.

Cheers,
Daniel

Re: [RFC PATCH 1/6] drm: Add Content Protection property

[Daniel Stone]

Hi Pavel,

On 5 December 2017 at 17:34, Pavel Machek  wrote:
> Yes, so... This patch makes it more likely to see machines with locked
> down kernels, preventing developers from working with systems their
> own, running hardware. That is evil, and direct threat to Free
> software movement.
>
> Users compiling their own kernels get no benefit from it. Actually it
> looks like this only benefits Intel and Disney. We don't want that.

With all due respect, you can't claim to speak for the entire kernel
and FLOSS community of users and developers.

The feature is optional: it does not enforce additional constraints on
users, but exposes additional functionality already present in
hardware, for those who wish to opt in to it. Those who wish to avoid
it can do so, by simply not making active use of it.

Cheers,
Daniel

Re: [PATCH 2/2] drm: vblank: remove drm_timestamp_monotonic parameter

[Daniel Stone]

On 11 October 2017 at 16:20, Arnd Bergmann <a...@arndb.de> wrote:
> There is a risk of overflowing vblank timestamps in 2038 or 2106 if
> someone sets the drm_timestamp_monotonic module parameter to zero.
>
> I found no indication of anyone ever setting the parameter, or
> complaining about the default being wrong, after it was introduced
> as a way to handle backwards-compatibility with linux prior to
> c61eef726a78 ("drm: add support for monotonic vblank timestamps"),
> so it's probably safer to just remove the parameter completely
> and only allowing the default behavior.

I don't think there's any reason to remain suspicious of
CLOCK_MONOTONIC in 2017. Not to mention that removing it wrecks
Weston/etc's ability to give clients present-timing feedback, so
removing it is a net uABI improvement.

Acked-by: Daniel Stone <dani...@collabora.com>

Cheers,
Daniel

Re: [PATCH 2/2] drm: vblank: remove drm_timestamp_monotonic parameter

[Daniel Stone]

On 11 October 2017 at 16:20, Arnd Bergmann  wrote:
> There is a risk of overflowing vblank timestamps in 2038 or 2106 if
> someone sets the drm_timestamp_monotonic module parameter to zero.
>
> I found no indication of anyone ever setting the parameter, or
> complaining about the default being wrong, after it was introduced
> as a way to handle backwards-compatibility with linux prior to
> c61eef726a78 ("drm: add support for monotonic vblank timestamps"),
> so it's probably safer to just remove the parameter completely
> and only allowing the default behavior.

I don't think there's any reason to remain suspicious of
CLOCK_MONOTONIC in 2017. Not to mention that removing it wrecks
Weston/etc's ability to give clients present-timing feedback, so
removing it is a net uABI improvement.

Acked-by: Daniel Stone 

Cheers,
Daniel

Re: [PATCH 2/2] drm/vc4: Add get/set tiling ioctls.

[Daniel Stone]

On 13 June 2017 at 16:49, Eric Anholt <e...@anholt.net> wrote:
> Daniel Stone <dan...@fooishbar.org> writes:
>> I posted a DRI3 v1.1 patch series which can advertise and also transit
>> modifiers directly under X11, and have also typed up the support for
>> Wayland which is working just fine with Weston from git. If you
>> implement DRIimage v15 to advertise and import modifiers, then you can
>> transit them for free without a magic-back-channel ioctl. Would that
>> be enough to convince you to drop this series?
>
> Not really -- this patch is pretty small, and doesn't require updating
> the entire world.

The modifier interface is already landed in mainline for KMS, GBM, and
Gallium. It's supported in i965 and freedreno, and Lucas has patches
to support it for etnaviv/imx-drm as well.

While I get that the {get,set}_tiling interface is necessary to route
around the X11 support not existing until very recently, I'm unhappy
that it's now landed in mainline imposing a performance penalty on
everyone else (Wayland compositors, Kodi, etc etc), with no way to
route around it.

Being that the impetus was an upcoming Raspbian release, I'd have been
a lot happier if it were carried as a downstream patch. As it is,
mainline now has an end-run around generic infrastructure to benefit
one specific user, leaving everyone else to write and try to land VC4
modifier support, then explicitly filter out the tiling modifier in
their KMS code, so they can un-regress their performance.

Re: [PATCH 2/2] drm/vc4: Add get/set tiling ioctls.

[Daniel Stone]

On 13 June 2017 at 16:49, Eric Anholt  wrote:
> Daniel Stone  writes:
>> I posted a DRI3 v1.1 patch series which can advertise and also transit
>> modifiers directly under X11, and have also typed up the support for
>> Wayland which is working just fine with Weston from git. If you
>> implement DRIimage v15 to advertise and import modifiers, then you can
>> transit them for free without a magic-back-channel ioctl. Would that
>> be enough to convince you to drop this series?
>
> Not really -- this patch is pretty small, and doesn't require updating
> the entire world.

The modifier interface is already landed in mainline for KMS, GBM, and
Gallium. It's supported in i965 and freedreno, and Lucas has patches
to support it for etnaviv/imx-drm as well.

While I get that the {get,set}_tiling interface is necessary to route
around the X11 support not existing until very recently, I'm unhappy
that it's now landed in mainline imposing a performance penalty on
everyone else (Wayland compositors, Kodi, etc etc), with no way to
route around it.

Being that the impetus was an upcoming Raspbian release, I'd have been
a lot happier if it were carried as a downstream patch. As it is,
mainline now has an end-run around generic infrastructure to benefit
one specific user, leaving everyone else to write and try to land VC4
modifier support, then explicitly filter out the tiling modifier in
their KMS code, so they can un-regress their performance.

[PATCH] Revert "HID: magicmouse: Set multi-touch keybits for Magic Mouse"

[Daniel Stone]

Setting these bits causes libinput to fail to initialize the device;
setting BTN_TOUCH and BTN_TOOL_FINGER causes it to treat the mouse as a
touchpad, and it then refuses to continue when it discovers ABS_X is not
set.

This breaks all known Wayland compositors, as well as Xorg when the
libinput driver is being used.

This reverts commit f4b65b9563216b3e01a5cc844c3ba68901d9b195.

Signed-off-by: Daniel Stone <dani...@collabora.com>
Cc: Che-Liang Chiou <clch...@chromium.org>
Cc: Thierry Escande <thierry.esca...@collabora.com>
Cc: Jiri Kosina <jkos...@suse.cz>
Cc: Benjamin Tissoires <benjamin.tissoi...@redhat.com>
---
 drivers/hid/hid-magicmouse.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

Jiri, can you please get this into 4.12 final?

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 1d6c997b3001..20b40ad26325 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -349,7 +349,6 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 
if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
magicmouse_emit_buttons(msc, clicks & 3);
-   input_mt_report_pointer_emulation(input, true);
input_report_rel(input, REL_X, x);
input_report_rel(input, REL_Y, y);
} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
@@ -389,16 +388,16 @@ static int magicmouse_setup_input(struct input_dev 
*input, struct hid_device *hd
__clear_bit(BTN_RIGHT, input->keybit);
__clear_bit(BTN_MIDDLE, input->keybit);
__set_bit(BTN_MOUSE, input->keybit);
+   __set_bit(BTN_TOOL_FINGER, input->keybit);
+   __set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
+   __set_bit(BTN_TOOL_TRIPLETAP, input->keybit);
+   __set_bit(BTN_TOOL_QUADTAP, input->keybit);
+   __set_bit(BTN_TOOL_QUINTTAP, input->keybit);
+   __set_bit(BTN_TOUCH, input->keybit);
+   __set_bit(INPUT_PROP_POINTER, input->propbit);
__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
}
 
-   __set_bit(BTN_TOOL_FINGER, input->keybit);
-   __set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
-   __set_bit(BTN_TOOL_TRIPLETAP, input->keybit);
-   __set_bit(BTN_TOOL_QUADTAP, input->keybit);
-   __set_bit(BTN_TOOL_QUINTTAP, input->keybit);
-   __set_bit(BTN_TOUCH, input->keybit);
-   __set_bit(INPUT_PROP_POINTER, input->propbit);
 
__set_bit(EV_ABS, input->evbit);
 
-- 
2.11.0

[PATCH] Revert "HID: magicmouse: Set multi-touch keybits for Magic Mouse"

[Daniel Stone]

Setting these bits causes libinput to fail to initialize the device;
setting BTN_TOUCH and BTN_TOOL_FINGER causes it to treat the mouse as a
touchpad, and it then refuses to continue when it discovers ABS_X is not
set.

This breaks all known Wayland compositors, as well as Xorg when the
libinput driver is being used.

This reverts commit f4b65b9563216b3e01a5cc844c3ba68901d9b195.

Signed-off-by: Daniel Stone 
Cc: Che-Liang Chiou 
Cc: Thierry Escande 
Cc: Jiri Kosina 
Cc: Benjamin Tissoires 
---
 drivers/hid/hid-magicmouse.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

Jiri, can you please get this into 4.12 final?

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 1d6c997b3001..20b40ad26325 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -349,7 +349,6 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 
if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
magicmouse_emit_buttons(msc, clicks & 3);
-   input_mt_report_pointer_emulation(input, true);
input_report_rel(input, REL_X, x);
input_report_rel(input, REL_Y, y);
} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
@@ -389,16 +388,16 @@ static int magicmouse_setup_input(struct input_dev 
*input, struct hid_device *hd
__clear_bit(BTN_RIGHT, input->keybit);
__clear_bit(BTN_MIDDLE, input->keybit);
__set_bit(BTN_MOUSE, input->keybit);
+   __set_bit(BTN_TOOL_FINGER, input->keybit);
+   __set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
+   __set_bit(BTN_TOOL_TRIPLETAP, input->keybit);
+   __set_bit(BTN_TOOL_QUADTAP, input->keybit);
+   __set_bit(BTN_TOOL_QUINTTAP, input->keybit);
+   __set_bit(BTN_TOUCH, input->keybit);
+   __set_bit(INPUT_PROP_POINTER, input->propbit);
__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
}
 
-   __set_bit(BTN_TOOL_FINGER, input->keybit);
-   __set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
-   __set_bit(BTN_TOOL_TRIPLETAP, input->keybit);
-   __set_bit(BTN_TOOL_QUADTAP, input->keybit);
-   __set_bit(BTN_TOOL_QUINTTAP, input->keybit);
-   __set_bit(BTN_TOUCH, input->keybit);
-   __set_bit(INPUT_PROP_POINTER, input->propbit);
 
__set_bit(EV_ABS, input->evbit);
 
-- 
2.11.0

Re: [PATCH 2/2] drm/vc4: Add get/set tiling ioctls.

[Daniel Stone]

Hi Eric,

On 8 June 2017 at 01:13, Eric Anholt  wrote:
> This allows mesa to set the tiling format for a BO and have that
> tiling format be respected by mesa on the other side of an
> import/export (and by vc4 scanout in the kernel), without defining a
> protocol to pass the tiling through userspace.

I posted a DRI3 v1.1 patch series which can advertise and also transit
modifiers directly under X11, and have also typed up the support for
Wayland which is working just fine with Weston from git. If you
implement DRIimage v15 to advertise and import modifiers, then you can
transit them for free without a magic-back-channel ioctl. Would that
be enough to convince you to drop this series?

Cheers,
Daniel

Re: [PATCH 2/2] drm/vc4: Add get/set tiling ioctls.

[Daniel Stone]

Hi Eric,

On 8 June 2017 at 01:13, Eric Anholt  wrote:
> This allows mesa to set the tiling format for a BO and have that
> tiling format be respected by mesa on the other side of an
> import/export (and by vc4 scanout in the kernel), without defining a
> protocol to pass the tiling through userspace.

I posted a DRI3 v1.1 patch series which can advertise and also transit
modifiers directly under X11, and have also typed up the support for
Wayland which is working just fine with Weston from git. If you
implement DRIimage v15 to advertise and import modifiers, then you can
transit them for free without a magic-back-channel ioctl. Would that
be enough to convince you to drop this series?

Cheers,
Daniel

Re: [PATCH] drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again

[Daniel Stone]

Hi Michel,

On 23 March 2017 at 08:53, Michel Dänzer <mic...@daenzer.net> wrote:
> Otherwise this can also prevent modesets e.g. for switching VTs, when
> multiple monitors with different native resolutions are connected.
>
> The depths must match though, so keep the != test for that.
>
> Also update the DRM_DEBUG output to be slightly more accurate, this
> doesn't only affect requests from userspace.

This test looks much more sensible, and also fixes VT switching for me.

Reviewed-by: Daniel Stone <dani...@collabora.com>

Cheers,
Daniel

Re: [PATCH] drm/fb-helper: Allow var->x/yres(_virtual) < fb->width/height again

[Daniel Stone]

Hi Michel,

On 23 March 2017 at 08:53, Michel Dänzer  wrote:
> Otherwise this can also prevent modesets e.g. for switching VTs, when
> multiple monitors with different native resolutions are connected.
>
> The depths must match though, so keep the != test for that.
>
> Also update the DRM_DEBUG output to be slightly more accurate, this
> doesn't only affect requests from userspace.

This test looks much more sensible, and also fixes VT switching for me.

Reviewed-by: Daniel Stone 

Cheers,
Daniel

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Daniel Stone]

Hi,

On 16 March 2017 at 09:55, Michel Dänzer  wrote:
> Otherwise this can also prevent modesets e.g. for switching VTs.
>
> FB_MISC_USER_EVENT is set when the request originates from userspace,
> which is what we're interested in here according to the DRM_DEBUG
> output.
>
> Bugzilla: https://bugs.freedesktop.org/99841
> Fixes: 865afb11949e ("drm/fb-helper: reject any changes to the fbdev")
> Signed-off-by: Michel Dänzer 
> ---
>
> I'm not entirely sure why the values can not match for a VT switch. If
> anybody thinks this just papers over the real issue, please speak up.

It happens for me in multi-head with different resolutions. A real
compositor will set native resolutions with separate framebuffers, and
then fbcon will try to set one buffer for both outputs. This works on
the output with the larger resolution, but the one with the smaller
resolution will fail due to [xy]res_virtual (IIRC) being different.

I'll test this out later today.

Cheers,
Daniel

Re: [PATCH] drm/fb-helper: Only reject FB changes if FB_MISC_USER_EVENT is set

[Daniel Stone]

Hi,

On 16 March 2017 at 09:55, Michel Dänzer  wrote:
> Otherwise this can also prevent modesets e.g. for switching VTs.
>
> FB_MISC_USER_EVENT is set when the request originates from userspace,
> which is what we're interested in here according to the DRM_DEBUG
> output.
>
> Bugzilla: https://bugs.freedesktop.org/99841
> Fixes: 865afb11949e ("drm/fb-helper: reject any changes to the fbdev")
> Signed-off-by: Michel Dänzer 
> ---
>
> I'm not entirely sure why the values can not match for a VT switch. If
> anybody thinks this just papers over the real issue, please speak up.

It happens for me in multi-head with different resolutions. A real
compositor will set native resolutions with separate framebuffers, and
then fbcon will try to set one buffer for both outputs. This works on
the output with the larger resolution, but the one with the smaller
resolution will fail due to [xy]res_virtual (IIRC) being different.

I'll test this out later today.

Cheers,
Daniel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Stone]

Hi,

On 17 February 2017 at 14:56, Ville Syrjälä
 wrote:
> On Fri, Feb 17, 2017 at 02:42:26PM +, Lionel Landwerlin wrote:
>> If we're talking fixed point reprsentation, ChromeOS is using this :
>>
>> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice=209
>
> So it's already using the sign+magnitude stuff. Which presumably
> means we can't change it to two's complement anymore :( Maybe we add a
> CTM2 property ;)

I wouldn't be so sure; AFAIK it only ships on platforms where the
kernel is also built from the same tree, and generally where the
support is backported anyway. So it would be possible to atomically
land a change to CrOS such that the kernels and Chrome move together
to a changed representation.

Cheers,
Daniel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Stone]

Hi,

On 17 February 2017 at 14:56, Ville Syrjälä
 wrote:
> On Fri, Feb 17, 2017 at 02:42:26PM +, Lionel Landwerlin wrote:
>> If we're talking fixed point reprsentation, ChromeOS is using this :
>>
>> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice=209
>
> So it's already using the sign+magnitude stuff. Which presumably
> means we can't change it to two's complement anymore :( Maybe we add a
> CTM2 property ;)

I wouldn't be so sure; AFAIK it only ships on platforms where the
kernel is also built from the same tree, and generally where the
support is backported anyway. So it would be possible to atomically
land a change to CrOS such that the kernels and Chrome move together
to a changed representation.

Cheers,
Daniel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Stone]

Hi,

On 15 February 2017 at 11:39, Ville Syrjälä
 wrote:
> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>>  wrote:
>> > Hmm. Two's complement is what I was thinking it is. Which shows that
>> > I never managed to read the code in any detail. Definitely needs to
>> > be documented properly.
>>
>> That sounds supremely backwards. I guess we can't fix this anymore?
>
> I have no idea. Anyone else?

I don't know of any implementation using this; maybe closed Intel
Android stuff? Certainly GitHub showed no-one using it, and neither X
nor Weston/Mutter are using it yet.

Cheers,
Daniel

Re: [PATCH v2] drm/color: Document CTM eqations

[Daniel Stone]

Hi,

On 15 February 2017 at 11:39, Ville Syrjälä
 wrote:
> On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
>> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
>>  wrote:
>> > Hmm. Two's complement is what I was thinking it is. Which shows that
>> > I never managed to read the code in any detail. Definitely needs to
>> > be documented properly.
>>
>> That sounds supremely backwards. I guess we can't fix this anymore?
>
> I have no idea. Anyone else?

I don't know of any implementation using this; maybe closed Intel
Android stuff? Certainly GitHub showed no-one using it, and neither X
nor Weston/Mutter are using it yet.

Cheers,
Daniel

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Stone]

Hi John,

On 14 February 2017 at 19:25, John Stultz  wrote:
> +static enum drm_mode_status
> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> +  struct drm_display_mode *mode)
> +{
> +   struct drm_device *dev = connector->dev;
> +   const struct drm_crtc_helper_funcs *crtc_funcs;
> +   struct drm_crtc *c;
> +
> +   if (mode->status != MODE_OK)
> +   return mode->status;
> +
> +   /* Check all the crtcs on a connector to make sure the mode is valid 
> */
> +   drm_for_each_crtc(c, dev) {
> +   crtc_funcs = c->helper_private;
> +   if (crtc_funcs && crtc_funcs->mode_valid)
> +   mode->status = crtc_funcs->mode_valid(c, mode);
> +   if (mode->status != MODE_OK)
> +   break;
> +   }
> +   return mode->status;
> +}

Hm, that's unfortunate: it limits the mode list for every connector,
to those which are supported by every single CRTC. So if you have one
CRTC serving low-res LVDS, and another serving higher-res HDMI,
suddenly you can't get bigger modes on HDMI. The idea seems sound
enough, but a little more nuance might be good ...

Cheers,
Daniel

Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Stone]

Hi John,

On 14 February 2017 at 19:25, John Stultz  wrote:
> +static enum drm_mode_status
> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> +  struct drm_display_mode *mode)
> +{
> +   struct drm_device *dev = connector->dev;
> +   const struct drm_crtc_helper_funcs *crtc_funcs;
> +   struct drm_crtc *c;
> +
> +   if (mode->status != MODE_OK)
> +   return mode->status;
> +
> +   /* Check all the crtcs on a connector to make sure the mode is valid 
> */
> +   drm_for_each_crtc(c, dev) {
> +   crtc_funcs = c->helper_private;
> +   if (crtc_funcs && crtc_funcs->mode_valid)
> +   mode->status = crtc_funcs->mode_valid(c, mode);
> +   if (mode->status != MODE_OK)
> +   break;
> +   }
> +   return mode->status;
> +}

Hm, that's unfortunate: it limits the mode list for every connector,
to those which are supported by every single CRTC. So if you have one
CRTC serving low-res LVDS, and another serving higher-res HDMI,
suddenly you can't get bigger modes on HDMI. The idea seems sound
enough, but a little more nuance might be good ...

Cheers,
Daniel

Re: [PATCH v2 1/2] drm/cma-helper: Add multi buffer support for cma fbdev

[Daniel Stone]

Hi Maxime,

On 13 February 2017 at 10:54, Maxime Ripard
 wrote:
> On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
>> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
>> > This patch add a config to support to create multi buffer for cma fbdev.
>> > Such as double buffer and triple buffer.
>> >
>> > Cma fbdev is convient to add a legency fbdev. And still many Android
>> > devices use fbdev now and at least double buffer is needed for these
>> > Android devices, so that a buffer flip can be operated. It will need
>> > some time for Android device vendors to abondon legency fbdev. So multi
>> > buffer for fbdev is needed.
>>
>> How exactly do we expect Android to move away from fbdev if we add features 
>> to
>> the fbdev compat layer ? I'd much rather make it clear to them that fbdev is 
>> a
>> thing from the past and that they'd better migrate now.
>
> If your point is that merging this patch will slow down the Android
> move away from fbdev, I disagree with that (obviously).
>
> I don't care at all about Android on my platform of choice, but don't
> see how that merging this patch will change anything.
>
> Let's be honest, Android trees typically have thousands of patches on
> top of mainline. Do you think a simple, 15 LoC, patch will make any
> difference to vendors? If they want to stay on fbdev and have that
> feature, they'll just merge this patch, done.

So, in that case, why not just let them do that? They'd already have
to add patches to use this, surely; we don't have anything in mainline
kernels which allows people to actually use this larger allocation.
Apart from software mmap() and using panning to do flips, but I'm
taking it as a given that people shipping Android on their devices
aren't using software rendering.

> However, what I do see is that three different people/organisations
> have now expressed interest in that feature, on three different
> SoCs. If that patch needed a significant rework of the fbdev layer,
> then yes, I might agree that it's not worth it. But in this case, it's
> pretty trivial.
>
> The only people you're "punishing" here with that kind of concern are
> the people who actually play fair and want not to have any patches and
> everything upstream.

I would hazard a guess that most users of this have out-of-tree GPU drivers.

> I guess a much better strategy would be to provide an incentive to
> moving to KMS. And I truely think there's one already, so it's just a
> matter of time before people switch over. Fbdev emulation or not.

The concern makes sense, but on the other hand, fbdev is deprecated:
no new drivers, and no new features.

Cheers,
Daniel

Re: [PATCH v2 1/2] drm/cma-helper: Add multi buffer support for cma fbdev

[Daniel Stone]

Hi Maxime,

On 13 February 2017 at 10:54, Maxime Ripard
 wrote:
> On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
>> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
>> > This patch add a config to support to create multi buffer for cma fbdev.
>> > Such as double buffer and triple buffer.
>> >
>> > Cma fbdev is convient to add a legency fbdev. And still many Android
>> > devices use fbdev now and at least double buffer is needed for these
>> > Android devices, so that a buffer flip can be operated. It will need
>> > some time for Android device vendors to abondon legency fbdev. So multi
>> > buffer for fbdev is needed.
>>
>> How exactly do we expect Android to move away from fbdev if we add features 
>> to
>> the fbdev compat layer ? I'd much rather make it clear to them that fbdev is 
>> a
>> thing from the past and that they'd better migrate now.
>
> If your point is that merging this patch will slow down the Android
> move away from fbdev, I disagree with that (obviously).
>
> I don't care at all about Android on my platform of choice, but don't
> see how that merging this patch will change anything.
>
> Let's be honest, Android trees typically have thousands of patches on
> top of mainline. Do you think a simple, 15 LoC, patch will make any
> difference to vendors? If they want to stay on fbdev and have that
> feature, they'll just merge this patch, done.

So, in that case, why not just let them do that? They'd already have
to add patches to use this, surely; we don't have anything in mainline
kernels which allows people to actually use this larger allocation.
Apart from software mmap() and using panning to do flips, but I'm
taking it as a given that people shipping Android on their devices
aren't using software rendering.

> However, what I do see is that three different people/organisations
> have now expressed interest in that feature, on three different
> SoCs. If that patch needed a significant rework of the fbdev layer,
> then yes, I might agree that it's not worth it. But in this case, it's
> pretty trivial.
>
> The only people you're "punishing" here with that kind of concern are
> the people who actually play fair and want not to have any patches and
> everything upstream.

I would hazard a guess that most users of this have out-of-tree GPU drivers.

> I guess a much better strategy would be to provide an incentive to
> moving to KMS. And I truely think there's one already, so it's just a
> matter of time before people switch over. Fbdev emulation or not.

The concern makes sense, but on the other hand, fbdev is deprecated:
no new drivers, and no new features.

Cheers,
Daniel

Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

[Daniel Stone]

Hi,

On 9 February 2017 at 17:01, Daniel Vetter  wrote:
> On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
>> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned 
>> long arg)
>> +{
>> + struct drm_fb_helper *fb_helper = info->par;
>> + struct drm_device *dev = fb_helper->dev;
>> + unsigned int i;
>> + int ret = 0;
>> +
>> + drm_modeset_lock_all(dev);
>> + if (!drm_fb_helper_is_bound(fb_helper)) {
>> + drm_modeset_unlock_all(dev);
>> + return -EBUSY;
>> + }
>> +
>> + switch (cmd) {
>> + case FBIO_WAITFORVSYNC:
>> + for (i = 0; i < fb_helper->crtc_count; i++) {
>> + struct drm_mode_set *mode_set;
>> + struct drm_crtc *crtc;
>> +
>> + mode_set = _helper->crtc_info[i].mode_set;
>> + crtc = mode_set->crtc;
>> +
>> + /*
>> +  * Only call drm_crtc_wait_one_vblank for crtcs that
>> +  * are currently enabled.
>> +  */
>> + if (!crtc->enabled)
>> + continue;
>
> This needs locking

More locking than the drm_modeset_lock_all() above the switch? Ouch. :)

Cheers,
Daniel

Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

[Daniel Stone]

Hi,

On 9 February 2017 at 17:01, Daniel Vetter  wrote:
> On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
>> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned 
>> long arg)
>> +{
>> + struct drm_fb_helper *fb_helper = info->par;
>> + struct drm_device *dev = fb_helper->dev;
>> + unsigned int i;
>> + int ret = 0;
>> +
>> + drm_modeset_lock_all(dev);
>> + if (!drm_fb_helper_is_bound(fb_helper)) {
>> + drm_modeset_unlock_all(dev);
>> + return -EBUSY;
>> + }
>> +
>> + switch (cmd) {
>> + case FBIO_WAITFORVSYNC:
>> + for (i = 0; i < fb_helper->crtc_count; i++) {
>> + struct drm_mode_set *mode_set;
>> + struct drm_crtc *crtc;
>> +
>> + mode_set = _helper->crtc_info[i].mode_set;
>> + crtc = mode_set->crtc;
>> +
>> + /*
>> +  * Only call drm_crtc_wait_one_vblank for crtcs that
>> +  * are currently enabled.
>> +  */
>> + if (!crtc->enabled)
>> + continue;
>
> This needs locking

More locking than the drm_modeset_lock_all() above the switch? Ouch. :)

Cheers,
Daniel

Re: [RFC] [GPU][DRM][PROPERTY] -Added a new ioctl in Linux DRM KMS driver.

[Daniel Stone]

Hi Satendra,

On 20 January 2017 at 08:12, Satendra Singh Thakur
 wrote:
> -Added a new ioctl in Linux DRM KMS driver.
>  This ioctl allows user to set the values of an object’s multiple
>  properties in one go.
> -In the absence of such ioctl, User would be calling one ioctl to set each
>  property value;
>  Thus, user needs to call N ioctls to set values of N properties of an
>  object,  which is a time consuming and costly because each ioctl involves
>   a system call entering/exiting kernel (context switch).
> -This ioctl will set N properties (provided that HW allows it)
>  in a single context switch
> -This ioctl can be used to set multiple properties of ether a plane
>  or a crtc or a connector (i.e. single object )
> -This ioctl can't be used to set one property of a plane and
>  one property of crtc in one go.

The atomic API already exists to set multiple properties at once, and
has the advantage of being, well, atomic. The API you propose
duplicates atomic, but it also has the possibility of only
half-succeeding, leaving the device in a strange halfway state which
is not what the user intended.

Cheers,
Daniel

Re: [RFC] [GPU][DRM][PROPERTY] -Added a new ioctl in Linux DRM KMS driver.

[Daniel Stone]

Hi Satendra,

On 20 January 2017 at 08:12, Satendra Singh Thakur
 wrote:
> -Added a new ioctl in Linux DRM KMS driver.
>  This ioctl allows user to set the values of an object’s multiple
>  properties in one go.
> -In the absence of such ioctl, User would be calling one ioctl to set each
>  property value;
>  Thus, user needs to call N ioctls to set values of N properties of an
>  object,  which is a time consuming and costly because each ioctl involves
>   a system call entering/exiting kernel (context switch).
> -This ioctl will set N properties (provided that HW allows it)
>  in a single context switch
> -This ioctl can be used to set multiple properties of ether a plane
>  or a crtc or a connector (i.e. single object )
> -This ioctl can't be used to set one property of a plane and
>  one property of crtc in one go.

The atomic API already exists to set multiple properties at once, and
has the advantage of being, well, atomic. The API you propose
duplicates atomic, but it also has the possibility of only
half-succeeding, leaving the device in a strange halfway state which
is not what the user intended.

Cheers,
Daniel

Re: [PATCH] drm/i915/dp: Stop enabling limited color ranges for everything

[Daniel Stone]

Hi,

On 5 January 2017 at 08:52, Daniel Vetter  wrote:
> On Thu, Jan 05, 2017 at 10:41:07AM +0200, Jani Nikula wrote:
>> No matter what we do here, the question remains what to do with
>> Chamelium. Changing the color range is really a workaround for
>> Chamelium, not a fix. Using CEA range is perfectly fine per DP spec.
>
> Can we just set a non-CEA mode/edid for chamelium, problem solved? We want
> to do that anyway for HDMI, where you really have to do the limited range
> dance to make stuff display correctly.

Or, if you set the 'Broadcast RGB' connector property to 'Full',
you'll never hit the color_range_auto branch in the first place ...

Cheers,
Daniel

Re: [PATCH] drm/i915/dp: Stop enabling limited color ranges for everything

[Daniel Stone]

Hi,

On 5 January 2017 at 08:52, Daniel Vetter  wrote:
> On Thu, Jan 05, 2017 at 10:41:07AM +0200, Jani Nikula wrote:
>> No matter what we do here, the question remains what to do with
>> Chamelium. Changing the color range is really a workaround for
>> Chamelium, not a fix. Using CEA range is perfectly fine per DP spec.
>
> Can we just set a non-CEA mode/edid for chamelium, problem solved? We want
> to do that anyway for HDMI, where you really have to do the limited range
> dance to make stuff display correctly.

Or, if you set the 'Broadcast RGB' connector property to 'Full',
you'll never hit the color_range_auto branch in the first place ...

Cheers,
Daniel

Re: [PATCH v2 1/2] drm_fourcc: Add new P010, P016 video format

[Daniel Stone]

Hi Randy,

On 4 January 2017 at 16:29, Randy Li  wrote:
> index 90d2cc8..23c8e99 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -165,6 +165,9 @@ const struct drm_format_info *__drm_format_info(u32 
> format)
> { .format = DRM_FORMAT_UYVY,.depth = 0,  
> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> { .format = DRM_FORMAT_VYUY,.depth = 0,  
> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> { .format = DRM_FORMAT_AYUV,.depth = 0,  
> .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +   /* FIXME a pixel in Y for P010 is 10 bits */
> +   { .format = DRM_FORMAT_P010,.depth = 0,  
> .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },

It seems like P010 stores each Y component in a 16-bit value, with the
bottom 6 bits ignored. So I think cpp here should be 2.

Cheers,
Daniel

Re: [PATCH v2 1/2] drm_fourcc: Add new P010, P016 video format

[Daniel Stone]

Hi Randy,

On 4 January 2017 at 16:29, Randy Li  wrote:
> index 90d2cc8..23c8e99 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -165,6 +165,9 @@ const struct drm_format_info *__drm_format_info(u32 
> format)
> { .format = DRM_FORMAT_UYVY,.depth = 0,  
> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> { .format = DRM_FORMAT_VYUY,.depth = 0,  
> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> { .format = DRM_FORMAT_AYUV,.depth = 0,  
> .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +   /* FIXME a pixel in Y for P010 is 10 bits */
> +   { .format = DRM_FORMAT_P010,.depth = 0,  
> .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },

It seems like P010 stores each Y component in a 16-bit value, with the
bottom 6 bits ignored. So I think cpp here should be 2.

Cheers,
Daniel

Re: [PATCH 1/2] drm_fourcc: Add new P010 video format

[Daniel Stone]

Hi Randy,

On 2 January 2017 at 09:50, Randy Li  wrote:
> P010 is a planar 4:2:0 YUV with interleaved UV plane, 10 bits
> per channel video format. Rockchip's vop support this
> video format(little endian only) as the input video format.
>
> Signed-off-by: Randy Li 
> ---
>  include/uapi/drm/drm_fourcc.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 9e1bb7f..d2721da 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -119,6 +119,7 @@ extern "C" {
>  #define DRM_FORMAT_NV61fourcc_code('N', 'V', '6', '1') /* 
> 2x1 subsampled Cb:Cr plane */
>  #define DRM_FORMAT_NV24fourcc_code('N', 'V', '2', '4') /* 
> non-subsampled Cr:Cb plane */
>  #define DRM_FORMAT_NV42fourcc_code('N', 'V', '4', '2') /* 
> non-subsampled Cb:Cr plane */
> +#define DRM_FORMAT_P010fourcc_code('P', '0', '1', '0') /* 
> 2x2 subsampled Cr:Cb plane 10 bits per channel */

Thanks, this looks good, but I have two requests. Firstly, the
Microsoft page here also mentions that P016 is a preferred format
along P010, so please add P016 as well:
https://msdn.microsoft.com/en-us/library/windows/desktop/bb970578(v=vs.85).aspx

I don't see much use of the other (P21x/P41x/Yxxx) formats defined
there, so there's probably no use going wild and adding them just yet.

Secondly, please update the format_info table in drm_fourcc.c for
these two formats, to avoid throwing a WARN_ON every time they are
used.

Cheers,
Daniel

Re: [PATCH 1/2] drm_fourcc: Add new P010 video format

[Daniel Stone]

Hi Randy,

On 2 January 2017 at 09:50, Randy Li  wrote:
> P010 is a planar 4:2:0 YUV with interleaved UV plane, 10 bits
> per channel video format. Rockchip's vop support this
> video format(little endian only) as the input video format.
>
> Signed-off-by: Randy Li 
> ---
>  include/uapi/drm/drm_fourcc.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 9e1bb7f..d2721da 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -119,6 +119,7 @@ extern "C" {
>  #define DRM_FORMAT_NV61fourcc_code('N', 'V', '6', '1') /* 
> 2x1 subsampled Cb:Cr plane */
>  #define DRM_FORMAT_NV24fourcc_code('N', 'V', '2', '4') /* 
> non-subsampled Cr:Cb plane */
>  #define DRM_FORMAT_NV42fourcc_code('N', 'V', '4', '2') /* 
> non-subsampled Cb:Cr plane */
> +#define DRM_FORMAT_P010fourcc_code('P', '0', '1', '0') /* 
> 2x2 subsampled Cr:Cb plane 10 bits per channel */

Thanks, this looks good, but I have two requests. Firstly, the
Microsoft page here also mentions that P016 is a preferred format
along P010, so please add P016 as well:
https://msdn.microsoft.com/en-us/library/windows/desktop/bb970578(v=vs.85).aspx

I don't see much use of the other (P21x/P41x/Yxxx) formats defined
there, so there's probably no use going wild and adding them just yet.

Secondly, please update the format_info table in drm_fourcc.c for
these two formats, to avoid throwing a WARN_ON every time they are
used.

Cheers,
Daniel

Re: [PATCH 2/2] [media] v4l: Add 10-bits per channel YUV pixel formats

[Daniel Stone]

Hi all,

On 2 January 2017 at 13:03, ayaka  wrote:
> On 01/02/2017 07:07 PM, Sakari Ailus wrote:
>> On Mon, Jan 02, 2017 at 06:53:16PM +0800, ayaka wrote:
>>> On 01/02/2017 05:10 PM, Sakari Ailus wrote:
 If the format resembles the existing formats but on a different bit
 depth,
 it should be named in similar fashion.
>>>
>>> Do you mean it would be better if it is called as NV12_10?
>>
>> If it otherwise resembles NV12 but just has 10 bits per pixel, I think
>> NV12_10 is a good name for it.
>
> The main reason I don't like it is that there is a various of software
> having used the P010 for this kind of pixel format. It would leadi a
> conflict between them(and I never saw it is used as NV12_10), as the P010 is
> more common to be used.

Indeed; GStreamer, FFmpeg and Microsoft all call this P010:
https://msdn.microsoft.com/en-us/library/windows/desktop/bb970578(v=vs.85).aspx

fourcc.org is supposed to reflect Microsoft's registry, but seems to
not be actively maintained anymore.

Cheers,
Daniel

Re: [PATCH 2/2] [media] v4l: Add 10-bits per channel YUV pixel formats

[Daniel Stone]

Hi all,

On 2 January 2017 at 13:03, ayaka  wrote:
> On 01/02/2017 07:07 PM, Sakari Ailus wrote:
>> On Mon, Jan 02, 2017 at 06:53:16PM +0800, ayaka wrote:
>>> On 01/02/2017 05:10 PM, Sakari Ailus wrote:
 If the format resembles the existing formats but on a different bit
 depth,
 it should be named in similar fashion.
>>>
>>> Do you mean it would be better if it is called as NV12_10?
>>
>> If it otherwise resembles NV12 but just has 10 bits per pixel, I think
>> NV12_10 is a good name for it.
>
> The main reason I don't like it is that there is a various of software
> having used the P010 for this kind of pixel format. It would leadi a
> conflict between them(and I never saw it is used as NV12_10), as the P010 is
> more common to be used.

Indeed; GStreamer, FFmpeg and Microsoft all call this P010:
https://msdn.microsoft.com/en-us/library/windows/desktop/bb970578(v=vs.85).aspx

fourcc.org is supposed to reflect Microsoft's registry, but seems to
not be actively maintained anymore.

Cheers,
Daniel

Re: linux-next: problems fetching the drm-intel, etc trees

[Daniel Stone]

Hi Stephen,

On 1 December 2016 at 20:45, Stephen Rothwell <s...@canb.auug.org.au> wrote:
> On Thu, 01 Dec 2016 11:02:26 +0000 Daniel Stone <dani...@collabora.com> wrote:
>> Sorry about this, it is quite bad. I think having mirrors for the key DRM
>> trees on GitHub is a good idea though, and I can get to setting that up.
>> Stephen, you need DRM (airlied), drm-misc, drm-panel, drm-intel, drm-tegra,
>> drm-exynos and drm-msm, right?
>
> Well, here are the trees I fetch from *.freedesktop.org:
>
> [...]
>
> I did not have any trouble with the people.fd.o ones.

That's actually interesting, given that they lie on the same host.
Perhaps it means that the locally-mounted ones were fine, and it was
the NFS (cough) mount between git/anongit which took a dive ... thanks
for the datapoint!

Cheers,
Daniel

Re: linux-next: problems fetching the drm-intel, etc trees

[Daniel Stone]

Hi Stephen,

On 1 December 2016 at 20:45, Stephen Rothwell  wrote:
> On Thu, 01 Dec 2016 11:02:26 +0000 Daniel Stone  wrote:
>> Sorry about this, it is quite bad. I think having mirrors for the key DRM
>> trees on GitHub is a good idea though, and I can get to setting that up.
>> Stephen, you need DRM (airlied), drm-misc, drm-panel, drm-intel, drm-tegra,
>> drm-exynos and drm-msm, right?
>
> Well, here are the trees I fetch from *.freedesktop.org:
>
> [...]
>
> I did not have any trouble with the people.fd.o ones.

That's actually interesting, given that they lie on the same host.
Perhaps it means that the locally-mounted ones were fine, and it was
the NFS (cough) mount between git/anongit which took a dive ... thanks
for the datapoint!

Cheers,
Daniel

Re: [PATCH] drm/vc4: Fix race between page flip completion event and clean-up

[Daniel Stone]

On 28 November 2016 at 22:31, Eric Anholt <e...@anholt.net> wrote:
> There was a small window where a userspace program could submit
> a pageflip after receiving a pageflip completion event yet still
> receive EBUSY.
>
> Signed-off-by: Derek Foreman <der...@osg.samsung.com>
> Signed-off-by: Eric Anholt <e...@anholt.net>
> Reviewed-by: Eric Anholt <e...@anholt.net>

Reviewed-by: Daniel Stone <dani...@collabora.com>

Re: [PATCH] drm/vc4: Fix race between page flip completion event and clean-up

[Daniel Stone]

On 28 November 2016 at 22:31, Eric Anholt  wrote:
> There was a small window where a userspace program could submit
> a pageflip after receiving a pageflip completion event yet still
> receive EBUSY.
>
> Signed-off-by: Derek Foreman 
> Signed-off-by: Eric Anholt 
> Reviewed-by: Eric Anholt 

Reviewed-by: Daniel Stone 

Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs

[Daniel Stone]

Hi Tomeu,

On 22 July 2016 at 15:10, Tomeu Vizoso  wrote:
> +/**
> + * DOC: CRC ABI
> + *
> + * DRM device drivers can provide to userspace CRC information of each frame 
> as
> + * it reached a given hardware component (a "source").
> + *
> + * Userspace can control generation of CRCs in a given CRTC by writing to the

s/can/must/

Is it worth having 'auto' as a default source perhaps?

> + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the 
> CRTC.
> + * Accepted values are source names (which are driver-specific) and the 
> "none"
> + * and "auto" keywords. "none" will disable CRC generation and "auto" will 
> let
> + * the driver select a default source of frame CRCs for this CRTC.

Is it also worth having 'connector-%s' (named as per sysfs, e.g.
connector-HDMI-A-0) as a standardised entry, for cloneable CRTCs which
have CRC control on the connector rather than the CRTC?

Cheers,
Daniel

Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs

[Daniel Stone]

Hi Tomeu,

On 22 July 2016 at 15:10, Tomeu Vizoso  wrote:
> +/**
> + * DOC: CRC ABI
> + *
> + * DRM device drivers can provide to userspace CRC information of each frame 
> as
> + * it reached a given hardware component (a "source").
> + *
> + * Userspace can control generation of CRCs in a given CRTC by writing to the

s/can/must/

Is it worth having 'auto' as a default source perhaps?

> + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the 
> CRTC.
> + * Accepted values are source names (which are driver-specific) and the 
> "none"
> + * and "auto" keywords. "none" will disable CRC generation and "auto" will 
> let
> + * the driver select a default source of frame CRCs for this CRTC.

Is it also worth having 'connector-%s' (named as per sysfs, e.g.
connector-HDMI-A-0) as a standardised entry, for cloneable CRTCs which
have CRC control on the connector rather than the CRTC?

Cheers,
Daniel

Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5

[Daniel Stone]

On 24 May 2016 at 09:27, Tomeu Vizoso <tomeu.viz...@collabora.com> wrote:
> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
> updated is requested and there is an earlier updated pending".
>
> v2: Use the status of the workqueue instead of vop->event, and don't add
> a superfluous wait on the workqueue.
>
> v3: Drop work_busy, as there's a sizeable delay when the worker
> finishes, which introduces a race in which the client has already
> received the last flip event but the next page flip ioctl will still
> return -EBUSY because work_busy returns outdated information.
>
> v4: Hold dev->event_lock while checking the VOP's event field as
> suggested by Daniel Stone.
>
> v5: Only block if there's outstanding work if it's a blocking call.
>
> Signed-off-by: Tomeu Vizoso <tomeu.viz...@collabora.com>

Reviewed-by: Daniel Stone <dani...@collabora.com>

Cheers,
Daniel

Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5

[Daniel Stone]

On 24 May 2016 at 09:27, Tomeu Vizoso  wrote:
> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
> updated is requested and there is an earlier updated pending".
>
> v2: Use the status of the workqueue instead of vop->event, and don't add
> a superfluous wait on the workqueue.
>
> v3: Drop work_busy, as there's a sizeable delay when the worker
> finishes, which introduces a race in which the client has already
> received the last flip event but the next page flip ioctl will still
> return -EBUSY because work_busy returns outdated information.
>
> v4: Hold dev->event_lock while checking the VOP's event field as
> suggested by Daniel Stone.
>
> v5: Only block if there's outstanding work if it's a blocking call.
>
> Signed-off-by: Tomeu Vizoso 

Reviewed-by: Daniel Stone 

Cheers,
Daniel

Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3

[Daniel Stone]

Hi Tomeu,

On 5 April 2016 at 16:07, Tomeu Vizoso <tomeu.viz...@collabora.com> wrote:
> On 4 April 2016 at 17:44, Daniel Stone <dan...@fooishbar.org> wrote:
>> On 4 April 2016 at 14:55, Tomeu Vizoso <tomeu.viz...@collabora.com> wrote:
>>> +   if (async) {
>>> +   for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>> +   if (crtc->state->event ||
>>> +   rockchip_drm_crtc_has_pending_event(crtc)) {
>>> +   return -EBUSY;
>>> +   }
>>> +   }
>>> +   }
>>
>> Hmmm ...
>>
>> Doesn't this trigger before the VOP atomic_begin() helper, meaning
>> that anything with an event set will trigger the check? Seems like it
>> should be && rather than ||.
>
> So, these are the two cases that this code aims to handle:
>
> 1. A previous request with an event set hasn't progressed to
> atomic_begin yet, so the event field in drm_crtc_state (at this point,
> the old state) is still populated but vop->event still isn't.

Ah right, this was what I was missing: the async (non-blocking)
implementation. Sounds good to me then.

Cheers,
Daniel

Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3

[Daniel Stone]

Hi Tomeu,

On 5 April 2016 at 16:07, Tomeu Vizoso  wrote:
> On 4 April 2016 at 17:44, Daniel Stone  wrote:
>> On 4 April 2016 at 14:55, Tomeu Vizoso  wrote:
>>> +   if (async) {
>>> +   for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>> +   if (crtc->state->event ||
>>> +   rockchip_drm_crtc_has_pending_event(crtc)) {
>>> +   return -EBUSY;
>>> +   }
>>> +   }
>>> +   }
>>
>> Hmmm ...
>>
>> Doesn't this trigger before the VOP atomic_begin() helper, meaning
>> that anything with an event set will trigger the check? Seems like it
>> should be && rather than ||.
>
> So, these are the two cases that this code aims to handle:
>
> 1. A previous request with an event set hasn't progressed to
> atomic_begin yet, so the event field in drm_crtc_state (at this point,
> the old state) is still populated but vop->event still isn't.

Ah right, this was what I was missing: the async (non-blocking)
implementation. Sounds good to me then.

Cheers,
Daniel

Re: [RFC v2 5/8] drm/fence: add in-fences support

[Daniel Stone]

Hi,

On 28 April 2016 at 23:28, Rob Clark  wrote:
> On Wed, Apr 27, 2016 at 2:39 AM, Daniel Vetter  wrote:
>> On Tue, Apr 26, 2016 at 01:48:02PM -0700, Greg Hackmann wrote:
>>> A (per-CRTC?) array of fences would be more flexible.  And even in the cases
>>> where you could make a 1-to-1 mapping between planes and fences, it's not
>>> that much more work for userspace to assemble those fences into an array
>>> anyway.
>>
>> I'm ok with an array too if that's what you folks prefer (it's meant to be
>> used by you after all). I just don't want just 1 fence for the entire op,
>> forcing userspace to first merge them all together. That seems silly.
>
> I was kinda more a fan of array too, if for no other reason that to be
> consistent w/ how out-fences work.  (And using property just for
> in-fence seemed slightly weird/abusive to me)

I don't think it's really useful to look for much consistency between
the two, beyond the name. I'm more concerned with consistency between
in-fences and the implicit fences on buffers/FBs, and between
out-fences and the page_flip_events.

>> One side-effect of that is that we'd also have to rework all the internal
>> bits and move fences around in atomic. Which means change a pile of
>> drivers. Not sure that's worth it, but I'd be ok either way really.
>
> hmm, well we could keep the array per-plane (and if one layer is using
> multiple planes, just list the same fd multiple times).. then it
> mostly comes down to changes in the ioctl fxn itself.

... and new API in libdrm, which is going to be a serious #ifdef and
distribution pain. The core property API has been available since
2.4.62 last June, but for this we'd have to write the code, wait for
the kernel code, wait for HWC, get everything together, and then merge
and release. That gives minimum one year of libdrm releases which have
had atomic but not in-fence API support, if we're adding a new array.
And I just don't really see what it buys us, apart from the need for
the core atomic_get_property helper to statically return -1 when
requesting FENCE_FD.

Cheers,
Daniel

Re: [RFC v2 5/8] drm/fence: add in-fences support

[Daniel Stone]

Hi,

On 28 April 2016 at 23:28, Rob Clark  wrote:
> On Wed, Apr 27, 2016 at 2:39 AM, Daniel Vetter  wrote:
>> On Tue, Apr 26, 2016 at 01:48:02PM -0700, Greg Hackmann wrote:
>>> A (per-CRTC?) array of fences would be more flexible.  And even in the cases
>>> where you could make a 1-to-1 mapping between planes and fences, it's not
>>> that much more work for userspace to assemble those fences into an array
>>> anyway.
>>
>> I'm ok with an array too if that's what you folks prefer (it's meant to be
>> used by you after all). I just don't want just 1 fence for the entire op,
>> forcing userspace to first merge them all together. That seems silly.
>
> I was kinda more a fan of array too, if for no other reason that to be
> consistent w/ how out-fences work.  (And using property just for
> in-fence seemed slightly weird/abusive to me)

I don't think it's really useful to look for much consistency between
the two, beyond the name. I'm more concerned with consistency between
in-fences and the implicit fences on buffers/FBs, and between
out-fences and the page_flip_events.

>> One side-effect of that is that we'd also have to rework all the internal
>> bits and move fences around in atomic. Which means change a pile of
>> drivers. Not sure that's worth it, but I'd be ok either way really.
>
> hmm, well we could keep the array per-plane (and if one layer is using
> multiple planes, just list the same fd multiple times).. then it
> mostly comes down to changes in the ioctl fxn itself.

... and new API in libdrm, which is going to be a serious #ifdef and
distribution pain. The core property API has been available since
2.4.62 last June, but for this we'd have to write the code, wait for
the kernel code, wait for HWC, get everything together, and then merge
and release. That gives minimum one year of libdrm releases which have
had atomic but not in-fence API support, if we're adding a new array.
And I just don't really see what it buys us, apart from the need for
the core atomic_get_property helper to statically return -1 when
requesting FENCE_FD.

Cheers,
Daniel

Re: [RFC v2 7/8] drm/fence: add fence timeline to drm_crtc

[Daniel Stone]

Hi,

On 26 April 2016 at 00:33, Gustavo Padovan  wrote:
> +static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
> +{
> +   if (fence->ops != _crtc_fence_ops)
> +   return NULL;

Since this is (currently) only used before unconditional dereferences,
maybe turn this into a BUG_ON instead of return NULL?

Cheers,
Daniel

Re: [RFC v2 7/8] drm/fence: add fence timeline to drm_crtc

[Daniel Stone]

Hi,

On 26 April 2016 at 00:33, Gustavo Padovan  wrote:
> +static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
> +{
> +   if (fence->ops != _crtc_fence_ops)
> +   return NULL;

Since this is (currently) only used before unconditional dereferences,
maybe turn this into a BUG_ON instead of return NULL?

Cheers,
Daniel

Re: [RFC v2 5/8] drm/fence: add in-fences support

[Daniel Stone]

Hi,

On 26 April 2016 at 21:48, Greg Hackmann  wrote:
> On 04/26/2016 01:05 PM, Daniel Vetter wrote:
>> On Tue, Apr 26, 2016 at 09:55:06PM +0300, Ville Syrjälä wrote:
>>> What are they doing that can't stuff the fences into an array
>>> instead of props?
>>
>> The hw composer interface is one in-fence per plane. That's really the
>> major reason why the kernel interface is built to match. And I really
>> don't think we should diverge just because we have a slight different
>> color preference ;-)
>
> The relationship between layers and fences is only fuzzy and indirect
> though.  The relationship is really between the buffer you're displaying on
> that layer, and the fence representing the work done to render into that
> buffer.  SurfaceFlinger just happens to bundle them together inside the same
> struct hwc_layer_1 as an API convenience.

Right, and when using implicit fencing, this comes as a plane
property, by virtue of plane -> fb -> buffer -> fence.

> Which is kind of splitting hairs as long as you have a 1-to-1 relationship
> between layers and DRM planes.  But that's not always the case.

Can you please elaborate?

> A (per-CRTC?) array of fences would be more flexible.  And even in the cases
> where you could make a 1-to-1 mapping between planes and fences, it's not
> that much more work for userspace to assemble those fences into an array
> anyway.

As Ville says, I don't want to go down the path of scheduling CRTC
updates separately, because that breaks MST pretty badly. If you don't
want your updates to display atomically, then don't schedule them
atomically ... ? That's the only reason I can see for making fencing
per-CRTC, rather than just a pile of unassociated fences appended to
the request. Per-CRTC fences also forces userspace to merge fences
before submission when using multiple planes per CRTC, which is pretty
punitive.

I think having it semantically attached to the plane is a little bit
nicer for tracing (why was this request delayed? -> a fence -> which
buffer was that fence for?) at a glance. Also the 'pile of appended
fences' model is a bit awkward for more generic userspace, which
creates a libdrm request and builds it (add a plane, try it out, wind
back) incrementally. Using properties makes that really easy, but
without properties, we'd have to add separate codepaths - and thus
separate ABI, which complicates distribution - to libdrm to account
for a separate plane array which shares a cursor with the properties.
So for that reason if none other, I'd really prefer not to go down
that route.

Cheers,
Daniel

Re: [RFC v2 5/8] drm/fence: add in-fences support

[Daniel Stone]

Hi,

On 26 April 2016 at 21:48, Greg Hackmann  wrote:
> On 04/26/2016 01:05 PM, Daniel Vetter wrote:
>> On Tue, Apr 26, 2016 at 09:55:06PM +0300, Ville Syrjälä wrote:
>>> What are they doing that can't stuff the fences into an array
>>> instead of props?
>>
>> The hw composer interface is one in-fence per plane. That's really the
>> major reason why the kernel interface is built to match. And I really
>> don't think we should diverge just because we have a slight different
>> color preference ;-)
>
> The relationship between layers and fences is only fuzzy and indirect
> though.  The relationship is really between the buffer you're displaying on
> that layer, and the fence representing the work done to render into that
> buffer.  SurfaceFlinger just happens to bundle them together inside the same
> struct hwc_layer_1 as an API convenience.

Right, and when using implicit fencing, this comes as a plane
property, by virtue of plane -> fb -> buffer -> fence.

> Which is kind of splitting hairs as long as you have a 1-to-1 relationship
> between layers and DRM planes.  But that's not always the case.

Can you please elaborate?

> A (per-CRTC?) array of fences would be more flexible.  And even in the cases
> where you could make a 1-to-1 mapping between planes and fences, it's not
> that much more work for userspace to assemble those fences into an array
> anyway.

As Ville says, I don't want to go down the path of scheduling CRTC
updates separately, because that breaks MST pretty badly. If you don't
want your updates to display atomically, then don't schedule them
atomically ... ? That's the only reason I can see for making fencing
per-CRTC, rather than just a pile of unassociated fences appended to
the request. Per-CRTC fences also forces userspace to merge fences
before submission when using multiple planes per CRTC, which is pretty
punitive.

I think having it semantically attached to the plane is a little bit
nicer for tracing (why was this request delayed? -> a fence -> which
buffer was that fence for?) at a glance. Also the 'pile of appended
fences' model is a bit awkward for more generic userspace, which
creates a libdrm request and builds it (add a plane, try it out, wind
back) incrementally. Using properties makes that really easy, but
without properties, we'd have to add separate codepaths - and thus
separate ABI, which complicates distribution - to libdrm to account
for a separate plane array which shares a cursor with the properties.
So for that reason if none other, I'd really prefer not to go down
that route.

Cheers,
Daniel

Re: [RFC 1/6] drm/fence: add FENCE_FD property to planes

[Daniel Stone]

Hi,

On 5 April 2016 at 15:04, Rob Clark <robdcl...@gmail.com> wrote:
> On Tue, Apr 5, 2016 at 8:57 AM, Daniel Stone <dan...@fooishbar.org> wrote:
>> I've been assuming that it would have to be write-only; I don't
>> believe there would be any meaningful usecases for read. Do you have
>> any in mind, or is it just a symmetry/cleanliness thing?
>
> no, don't see a use-case for it.. but this patch didn't seem to be
> preventing it.  And it was storing the fence_fd on the kernel side
> which is a no-no as well.

Yeah, both should be fixed. :)

>>> The issue is that 'struct file' / 'int fd' have a fundamentally
>>> different lifecycle compared to 'kms obj' / 'u32 obj_id'.  For the kms
>>> objects (like framebuffers) where we can use them with properties, the
>>> id is tied to the kernel side object.  This is not true for file
>>> descriptors.  Resulting in a few problems:
>>
>> Surely the fence FD tied to the kernel-side struct fence, in exactly
>> the same way, no?
>
> well, what I mean is you can't keep around the int fd on the kernel
> side, like this patch does
>
> A write-only property, which immediately (ie. during the ioctl call)
> is converted into a fence object, would work.  Although given that we
> need to handle fences differently (ie. not a property) for out-fences,
> it seems odd to shoehorn them into a property for in-fences.

Depends on how you look at it, I guess. From the point of view of all
fence-like things being consistent, yes, it falls down. But from the
point of view of in-fences being attached to an FB, and out-fences
(like events) being separately attached to a request, it's a lot more
consistent.

>>> I think it is really better to pass in an array of 'struct { u32
>>> plane; int fd }' (which the atomic ioctl code converts into 'struct
>>> fence's and attaches to the plane state) and an array of 'struct { u32
>>> crtc; int fd }' filled in on the kernel side for the out-fences.
>>
>> Mmm, it definitely makes ioctl parsing harder, and still you rely on
>> drivers to implement the more-difficult-to-not-screw-up part, which
>> (analagous to crtc_state->event) is the driver managing the lifecycle
>> of that component of the state. We already enforce this with events
>> though, and the difficult part wasn't in the userspace interface, but
>> the backend handling. This doesn't change at all regardless of whether
>> we use a property or an external array, so ...
>
> hmm, I'm assuming that the in/out arrays are handled in
> drm_mode_atomic_ioctl() and the drivers never see 'em..

True, but it complicates the (already not hugely straightforward)
parsing that the ioctl has to do. It also makes extending the ioctl a
little harder to do in future, because you're adding in two
variable-size elements, and have to do some fairly complicated parsing
to even figure out what the size _should_ be. So I'd rather not do it
if there was any way out of it; at the very least it'd have to be
userptr rather than array.

Cheers,
Daniel

Re: [RFC 1/6] drm/fence: add FENCE_FD property to planes

[Daniel Stone]

Hi,

On 5 April 2016 at 15:04, Rob Clark  wrote:
> On Tue, Apr 5, 2016 at 8:57 AM, Daniel Stone  wrote:
>> I've been assuming that it would have to be write-only; I don't
>> believe there would be any meaningful usecases for read. Do you have
>> any in mind, or is it just a symmetry/cleanliness thing?
>
> no, don't see a use-case for it.. but this patch didn't seem to be
> preventing it.  And it was storing the fence_fd on the kernel side
> which is a no-no as well.

Yeah, both should be fixed. :)

>>> The issue is that 'struct file' / 'int fd' have a fundamentally
>>> different lifecycle compared to 'kms obj' / 'u32 obj_id'.  For the kms
>>> objects (like framebuffers) where we can use them with properties, the
>>> id is tied to the kernel side object.  This is not true for file
>>> descriptors.  Resulting in a few problems:
>>
>> Surely the fence FD tied to the kernel-side struct fence, in exactly
>> the same way, no?
>
> well, what I mean is you can't keep around the int fd on the kernel
> side, like this patch does
>
> A write-only property, which immediately (ie. during the ioctl call)
> is converted into a fence object, would work.  Although given that we
> need to handle fences differently (ie. not a property) for out-fences,
> it seems odd to shoehorn them into a property for in-fences.

Depends on how you look at it, I guess. From the point of view of all
fence-like things being consistent, yes, it falls down. But from the
point of view of in-fences being attached to an FB, and out-fences
(like events) being separately attached to a request, it's a lot more
consistent.

>>> I think it is really better to pass in an array of 'struct { u32
>>> plane; int fd }' (which the atomic ioctl code converts into 'struct
>>> fence's and attaches to the plane state) and an array of 'struct { u32
>>> crtc; int fd }' filled in on the kernel side for the out-fences.
>>
>> Mmm, it definitely makes ioctl parsing harder, and still you rely on
>> drivers to implement the more-difficult-to-not-screw-up part, which
>> (analagous to crtc_state->event) is the driver managing the lifecycle
>> of that component of the state. We already enforce this with events
>> though, and the difficult part wasn't in the userspace interface, but
>> the backend handling. This doesn't change at all regardless of whether
>> we use a property or an external array, so ...
>
> hmm, I'm assuming that the in/out arrays are handled in
> drm_mode_atomic_ioctl() and the drivers never see 'em..

True, but it complicates the (already not hugely straightforward)
parsing that the ioctl has to do. It also makes extending the ioctl a
little harder to do in future, because you're adding in two
variable-size elements, and have to do some fairly complicated parsing
to even figure out what the size _should_ be. So I'd rather not do it
if there was any way out of it; at the very least it'd have to be
userptr rather than array.

Cheers,
Daniel

Re: [RFC 1/6] drm/fence: add FENCE_FD property to planes

[Daniel Stone]

Hi,

On 5 April 2016 at 13:36, Rob Clark  wrote:
> ok, so I've been slacking on writing up the reasons that I don't like
> the idea of using a property for fd's (including fence fd's).. I did
> at one point assume we'd use properties for fence fd's, but that idea
> falls apart pretty quickly when you think about the 'struct file' vs
> fd lifecycle.  It could *possibly* work if it was a write-only
> property, but I don't think that is a good solution.

I've been assuming that it would have to be write-only; I don't
believe there would be any meaningful usecases for read. Do you have
any in mind, or is it just a symmetry/cleanliness thing?

> The issue is that 'struct file' / 'int fd' have a fundamentally
> different lifecycle compared to 'kms obj' / 'u32 obj_id'.  For the kms
> objects (like framebuffers) where we can use them with properties, the
> id is tied to the kernel side object.  This is not true for file
> descriptors.  Resulting in a few problems:

Surely the fence FD tied to the kernel-side struct fence, in exactly
the same way, no?

> 1) if it was a readable property, reading it would (should) take a
> reference..  we can't just return fence_fd that was previously set
> (since in the mean time userspace might have close()d the fd).  So we
> have to return a fresh fd.  And if userspace (like modetest) doesn't
> realize it is responsible to close() that fd we have a leak

Again, assuming that read would always return -1.

> 2) basically we shouldn't be tracking fd's on the kernel side, since
> we can only count on them being valid during the duration of the ioctl
> call.  Once the call returns, you must assume userspace has close()d
> the fd.  So in the ioctl we need to convert fd -> file, and from then
> on out track the file object (or in this case the underlying fence
> object).

Right, it would have to be the same as KMS objects, where userspace
passes in an ID (in this case an entry in a non-IDR table, but still),
and the kernel maps that to struct fence and handles the lifetime. So,
almost exactly the same as what we do with KMS objects ...

> I guess we *could* have something analogous to dmabuf, where we import
> the fence fd and get a kms drm_fence object (with an id tied to the
> kernel side object), and then use a property to attach the drm_fence
> object.. but that seems kind of pointless and just trying to force the
> 'everything is a property' mantra.

I think that would be pointless indirection as well.

> I think it is really better to pass in an array of 'struct { u32
> plane; int fd }' (which the atomic ioctl code converts into 'struct
> fence's and attaches to the plane state) and an array of 'struct { u32
> crtc; int fd }' filled in on the kernel side for the out-fences.

Mmm, it definitely makes ioctl parsing harder, and still you rely on
drivers to implement the more-difficult-to-not-screw-up part, which
(analagous to crtc_state->event) is the driver managing the lifecycle
of that component of the state. We already enforce this with events
though, and the difficult part wasn't in the userspace interface, but
the backend handling. This doesn't change at all regardless of whether
we use a property or an external array, so ...

Cheers,
Daniel

Re: [RFC 1/6] drm/fence: add FENCE_FD property to planes

[Daniel Stone]

Hi,

On 5 April 2016 at 13:36, Rob Clark  wrote:
> ok, so I've been slacking on writing up the reasons that I don't like
> the idea of using a property for fd's (including fence fd's).. I did
> at one point assume we'd use properties for fence fd's, but that idea
> falls apart pretty quickly when you think about the 'struct file' vs
> fd lifecycle.  It could *possibly* work if it was a write-only
> property, but I don't think that is a good solution.

I've been assuming that it would have to be write-only; I don't
believe there would be any meaningful usecases for read. Do you have
any in mind, or is it just a symmetry/cleanliness thing?

> The issue is that 'struct file' / 'int fd' have a fundamentally
> different lifecycle compared to 'kms obj' / 'u32 obj_id'.  For the kms
> objects (like framebuffers) where we can use them with properties, the
> id is tied to the kernel side object.  This is not true for file
> descriptors.  Resulting in a few problems:

Surely the fence FD tied to the kernel-side struct fence, in exactly
the same way, no?

> 1) if it was a readable property, reading it would (should) take a
> reference..  we can't just return fence_fd that was previously set
> (since in the mean time userspace might have close()d the fd).  So we
> have to return a fresh fd.  And if userspace (like modetest) doesn't
> realize it is responsible to close() that fd we have a leak

Again, assuming that read would always return -1.

> 2) basically we shouldn't be tracking fd's on the kernel side, since
> we can only count on them being valid during the duration of the ioctl
> call.  Once the call returns, you must assume userspace has close()d
> the fd.  So in the ioctl we need to convert fd -> file, and from then
> on out track the file object (or in this case the underlying fence
> object).

Right, it would have to be the same as KMS objects, where userspace
passes in an ID (in this case an entry in a non-IDR table, but still),
and the kernel maps that to struct fence and handles the lifetime. So,
almost exactly the same as what we do with KMS objects ...

> I guess we *could* have something analogous to dmabuf, where we import
> the fence fd and get a kms drm_fence object (with an id tied to the
> kernel side object), and then use a property to attach the drm_fence
> object.. but that seems kind of pointless and just trying to force the
> 'everything is a property' mantra.

I think that would be pointless indirection as well.

> I think it is really better to pass in an array of 'struct { u32
> plane; int fd }' (which the atomic ioctl code converts into 'struct
> fence's and attaches to the plane state) and an array of 'struct { u32
> crtc; int fd }' filled in on the kernel side for the out-fences.

Mmm, it definitely makes ioctl parsing harder, and still you rely on
drivers to implement the more-difficult-to-not-screw-up part, which
(analagous to crtc_state->event) is the driver managing the lifecycle
of that component of the state. We already enforce this with events
though, and the difficult part wasn't in the userspace interface, but
the backend handling. This doesn't change at all regardless of whether
we use a property or an external array, so ...

Cheers,
Daniel

Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3

[Daniel Stone]

Hi Tomeu,

On 4 April 2016 at 14:55, Tomeu Vizoso  wrote:
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 3b8f652698f8..8305bbd2a4d7 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -280,7 +280,18 @@ int rockchip_drm_atomic_commit(struct drm_device *dev,
>  {
> struct rockchip_drm_private *private = dev->dev_private;
> struct rockchip_atomic_commit *commit = >commit;
> -   int ret;
> +   struct drm_crtc_state *crtc_state;
> +   struct drm_crtc *crtc;
> +   int i, ret;
> +
> +   if (async) {
> +   for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +   if (crtc->state->event ||
> +   rockchip_drm_crtc_has_pending_event(crtc)) {
> +   return -EBUSY;
> +   }
> +   }
> +   }

Hmmm ...

Doesn't this trigger before the VOP atomic_begin() helper, meaning
that anything with an event set will trigger the check? Seems like it
should be && rather than ||.

Also, I know rockchip_drm_vop.c isn't holding dev->event_lock when it
checks vop->event, but it really should be ... and so should this
check. Otherwise you can race with the IRQ handler, which isn't
required to hold the CRTC lock.

Cheers,
Daniel

Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3

[Daniel Stone]

Hi Tomeu,

On 4 April 2016 at 14:55, Tomeu Vizoso  wrote:
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 3b8f652698f8..8305bbd2a4d7 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -280,7 +280,18 @@ int rockchip_drm_atomic_commit(struct drm_device *dev,
>  {
> struct rockchip_drm_private *private = dev->dev_private;
> struct rockchip_atomic_commit *commit = >commit;
> -   int ret;
> +   struct drm_crtc_state *crtc_state;
> +   struct drm_crtc *crtc;
> +   int i, ret;
> +
> +   if (async) {
> +   for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +   if (crtc->state->event ||
> +   rockchip_drm_crtc_has_pending_event(crtc)) {
> +   return -EBUSY;
> +   }
> +   }
> +   }

Hmmm ...

Doesn't this trigger before the VOP atomic_begin() helper, meaning
that anything with an event set will trigger the check? Seems like it
should be && rather than ||.

Also, I know rockchip_drm_vop.c isn't holding dev->event_lock when it
checks vop->event, but it really should be ... and so should this
check. Otherwise you can race with the IRQ handler, which isn't
required to hold the CRTC lock.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi Inki,

On 31 March 2016 at 12:26, Inki Dae <daei...@gmail.com> wrote:
> 2016-03-31 19:56 GMT+09:00 Daniel Stone <dan...@fooishbar.org>:
>> On 31 March 2016 at 11:05, Inki Dae <inki@samsung.com> wrote:
>>> Then, existing drivers would need additional works for explicit fencing 
>>> support. This wouldn't be really what the drivers have to but should be 
>>> handled with this patch series because this would affect exising device 
>>> drivers which use implicit fencing.
>>
>> Well, yes. Anyone implementing their own atomic commit would need to
>> ensure that the commit works properly for fences. The helpers could
>> also add it, but the helpers are not mandatory, and you are not
>> required to use every part of the helper to use one part of the
>> helper. There is no magic wand you can wave that instantly makes it
>> work for every driver
>
> I meant there are already several DRM drivers which work properly for
> implicit fence. So if atomic helper framework of DRM core is
> considered only for the explicit fence, then fencing operation would
> affect the existing DRM drivers. So I hope this trying could consider
> existing implicit fence users.

Yes, absolutely. Implicit fencing is already part of userspace ABI
that we can effectively never remove: it would break everyone's
desktops on Intel alone, as well as many others. So explicit will be
opt-in from the user and the driver both, and only when the
combination is fully supported will explicit fencing be used.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi Inki,

On 31 March 2016 at 12:26, Inki Dae  wrote:
> 2016-03-31 19:56 GMT+09:00 Daniel Stone :
>> On 31 March 2016 at 11:05, Inki Dae  wrote:
>>> Then, existing drivers would need additional works for explicit fencing 
>>> support. This wouldn't be really what the drivers have to but should be 
>>> handled with this patch series because this would affect exising device 
>>> drivers which use implicit fencing.
>>
>> Well, yes. Anyone implementing their own atomic commit would need to
>> ensure that the commit works properly for fences. The helpers could
>> also add it, but the helpers are not mandatory, and you are not
>> required to use every part of the helper to use one part of the
>> helper. There is no magic wand you can wave that instantly makes it
>> work for every driver
>
> I meant there are already several DRM drivers which work properly for
> implicit fence. So if atomic helper framework of DRM core is
> considered only for the explicit fence, then fencing operation would
> affect the existing DRM drivers. So I hope this trying could consider
> existing implicit fence users.

Yes, absolutely. Implicit fencing is already part of userspace ABI
that we can effectively never remove: it would break everyone's
desktops on Intel alone, as well as many others. So explicit will be
opt-in from the user and the driver both, and only when the
combination is fully supported will explicit fencing be used.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi Inki,

On 31 March 2016 at 11:05, Inki Dae <inki@samsung.com> wrote:
> 2016년 03월 31일 18:35에 Daniel Stone 이(가) 쓴 글:
>> On 31 March 2016 at 08:45, Inki Dae <inki@samsung.com> wrote:
>>> As of now, it seems that this wouldn't be optional but mandatory if 
>>> explicit fence support is added to the atomic helper framework. This would 
>>> definitely be duplication and it seems not clear enough even if one of them 
>>> is just skipped in runtime.
>>
>> Drivers would have to opt in to explicit fencing support, and part of
>> that would be ensuring that the driver does not wait on implicit
>> fences when the user has requested explicit fencing be used.
>>
>
> Then, existing drivers would need additional works for explicit fencing 
> support. This wouldn't be really what the drivers have to but should be 
> handled with this patch series because this would affect exising device 
> drivers which use implicit fencing.

Well, yes. Anyone implementing their own atomic commit would need to
ensure that the commit works properly for fences. The helpers could
also add it, but the helpers are not mandatory, and you are not
required to use every part of the helper to use one part of the
helper. There is no magic wand you can wave that instantly makes it
work for every driver.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi Inki,

On 31 March 2016 at 11:05, Inki Dae  wrote:
> 2016년 03월 31일 18:35에 Daniel Stone 이(가) 쓴 글:
>> On 31 March 2016 at 08:45, Inki Dae  wrote:
>>> As of now, it seems that this wouldn't be optional but mandatory if 
>>> explicit fence support is added to the atomic helper framework. This would 
>>> definitely be duplication and it seems not clear enough even if one of them 
>>> is just skipped in runtime.
>>
>> Drivers would have to opt in to explicit fencing support, and part of
>> that would be ensuring that the driver does not wait on implicit
>> fences when the user has requested explicit fencing be used.
>>
>
> Then, existing drivers would need additional works for explicit fencing 
> support. This wouldn't be really what the drivers have to but should be 
> handled with this patch series because this would affect exising device 
> drivers which use implicit fencing.

Well, yes. Anyone implementing their own atomic commit would need to
ensure that the commit works properly for fences. The helpers could
also add it, but the helpers are not mandatory, and you are not
required to use every part of the helper to use one part of the
helper. There is no magic wand you can wave that instantly makes it
work for every driver.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi Inki,

On 31 March 2016 at 08:45, Inki Dae  wrote:
> 2016년 03월 29일 22:23에 Rob Clark 이(가) 쓴 글:
>> On Mon, Mar 28, 2016 at 10:18 PM, Inki Dae  wrote:
>>> In addition, I wonder how explicit and implicit fences could coexist 
>>> together.
>>> Rob said,
>>> "Implicit sync ofc remains the default, but userspace could opt-in to 
>>> explicit sync instead"
>>>
>>> This would mean that if we use explicit sync for user-space then it 
>>> coexists with implicit sync. However, these two sync fences can't see same 
>>> DMA buffer because explicit fence has a different file object from implicit 
>>> one.
>>> So in this case, I think explicit fence would need to be hung up on the 
>>> reservation object of dmabuf object somehow. Otherwise, although they 
>>> coexist together, are these fences - explicit and implicit - used for 
>>> differenct purpose separately?
>>>
>>
>> I'm not entirely sure about coexistance at the same time.  It ofc
>> shouldn't be a problem for one kernel to support both kinds of
>> userspace (pure explicit and pure implicit).  And how this would work
>> on kms atomic ioctl (compositor/consumer) side seems clear enough..
>> ie. some sort of flag, which if set user provides an explicit fence
>> fd, and if not set we fall back to current behaviour (ie. get fences
>> from resv object).
>
> With this patch series, users can register explicit fence(s) to atomic 
> kms(consumer side) through kms property interface for the explicit sync.
>
> However, now several DRM drivers(also consumer) already have beeen using 
> implicit fence. So while GPU(producer side) is accessing DMA buffer after 
> registering its explicit fence to atomic kms, and if atomic commit is 
> requested by user-space, then atomic helper framework will try to synchronize 
> with the producer - waiting for the signal of GPU side(producer), and device 
> specific page flip function will also try to do same thing.

Well, it has to be one or the other: mixing explicit and implicit,
defeats the purpose of using explicit fencing. So, when explicit
fencing is in use, implicit fences must be ignored.

> As of now, it seems that this wouldn't be optional but mandatory if explicit 
> fence support is added to the atomic helper framework. This would definitely 
> be duplication and it seems not clear enough even if one of them is just 
> skipped in runtime.

Drivers would have to opt in to explicit fencing support, and part of
that would be ensuring that the driver does not wait on implicit
fences when the user has requested explicit fencing be used.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi Inki,

On 31 March 2016 at 08:45, Inki Dae  wrote:
> 2016년 03월 29일 22:23에 Rob Clark 이(가) 쓴 글:
>> On Mon, Mar 28, 2016 at 10:18 PM, Inki Dae  wrote:
>>> In addition, I wonder how explicit and implicit fences could coexist 
>>> together.
>>> Rob said,
>>> "Implicit sync ofc remains the default, but userspace could opt-in to 
>>> explicit sync instead"
>>>
>>> This would mean that if we use explicit sync for user-space then it 
>>> coexists with implicit sync. However, these two sync fences can't see same 
>>> DMA buffer because explicit fence has a different file object from implicit 
>>> one.
>>> So in this case, I think explicit fence would need to be hung up on the 
>>> reservation object of dmabuf object somehow. Otherwise, although they 
>>> coexist together, are these fences - explicit and implicit - used for 
>>> differenct purpose separately?
>>>
>>
>> I'm not entirely sure about coexistance at the same time.  It ofc
>> shouldn't be a problem for one kernel to support both kinds of
>> userspace (pure explicit and pure implicit).  And how this would work
>> on kms atomic ioctl (compositor/consumer) side seems clear enough..
>> ie. some sort of flag, which if set user provides an explicit fence
>> fd, and if not set we fall back to current behaviour (ie. get fences
>> from resv object).
>
> With this patch series, users can register explicit fence(s) to atomic 
> kms(consumer side) through kms property interface for the explicit sync.
>
> However, now several DRM drivers(also consumer) already have beeen using 
> implicit fence. So while GPU(producer side) is accessing DMA buffer after 
> registering its explicit fence to atomic kms, and if atomic commit is 
> requested by user-space, then atomic helper framework will try to synchronize 
> with the producer - waiting for the signal of GPU side(producer), and device 
> specific page flip function will also try to do same thing.

Well, it has to be one or the other: mixing explicit and implicit,
defeats the purpose of using explicit fencing. So, when explicit
fencing is in use, implicit fences must be ignored.

> As of now, it seems that this wouldn't be optional but mandatory if explicit 
> fence support is added to the atomic helper framework. This would definitely 
> be duplication and it seems not clear enough even if one of them is just 
> skipped in runtime.

Drivers would have to opt in to explicit fencing support, and part of
that would be ensuring that the driver does not wait on implicit
fences when the user has requested explicit fencing be used.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi Inki,

On 28 March 2016 at 02:26, Inki Dae <inki@samsung.com> wrote:
> 2016년 03월 25일 21:10에 Daniel Stone 이(가) 쓴 글:
>> Second, really. Vulkan avoids implicit sync entirely, and exposes
>> fence-like primitives throughout its whole API. These include being
>> able to pass prerequisite fences for display (what Gustavo is adding
>> here: something to block on before display), and also when the user
>> acquires a buffer as a render target, it is given another prerequisite
>> fence (the other side of what Gustavo is suggesting, i.e. the fence
>> triggers when the buffer is no longer displayed and becomes available
>> for rendering).
>>
>> In order to implement this correctly, and avoid performance bubbles,
>> we need a primitive like this exposed through the KMS API, from both
>> sides. This is especially important when you take the case of
>> userspace suballocation, where userspace allocates larger blocks and
>> divides the allocation internally for different uses. Implicit sync
>> does not work at all for that case.
>
> Can you give me more details why implicit sync cannot take care of the case 
> of userspace suballocation?

Implicit sync does not know about suballocation, so implicit will
operate for every range in the buffer, not just the one you want.

Say you have one kernel buffer, which userspace subdivides into four
independent buffers. It can perform operations on these buffers which
are completely independent of each other, and an explicit sync model
allows this independence to be kept. Implicit sync ties them together,
so that you cannot do any operations on buffer 1 until all operations
on buffer 2 have completed.

> And is there any reason that fence fd shouldn't dependent of DMABUF - now 
> fence fd is a new file, not DMABUF fd?

Because dmabuf is for buffer sharing, and fences aren't buffers (they
will never export page ranges). Is there any particular benefit you
think you would get from doing this?

>> good thing. This is also the model that ChromeOS is moving towards, so
>> it becomes more important from that point of view as well.
>
> I think Gustavo should had explaned this path series enough to other people 
> when posting them - ie, what relationship explict and implicit fences have, 
> and why implicit fence - which is independent of DMABUF - is required, and 
> what use cases there are in real users, and etc.

Fair enough, the summary could perhaps contain something like this.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi Inki,

On 28 March 2016 at 02:26, Inki Dae  wrote:
> 2016년 03월 25일 21:10에 Daniel Stone 이(가) 쓴 글:
>> Second, really. Vulkan avoids implicit sync entirely, and exposes
>> fence-like primitives throughout its whole API. These include being
>> able to pass prerequisite fences for display (what Gustavo is adding
>> here: something to block on before display), and also when the user
>> acquires a buffer as a render target, it is given another prerequisite
>> fence (the other side of what Gustavo is suggesting, i.e. the fence
>> triggers when the buffer is no longer displayed and becomes available
>> for rendering).
>>
>> In order to implement this correctly, and avoid performance bubbles,
>> we need a primitive like this exposed through the KMS API, from both
>> sides. This is especially important when you take the case of
>> userspace suballocation, where userspace allocates larger blocks and
>> divides the allocation internally for different uses. Implicit sync
>> does not work at all for that case.
>
> Can you give me more details why implicit sync cannot take care of the case 
> of userspace suballocation?

Implicit sync does not know about suballocation, so implicit will
operate for every range in the buffer, not just the one you want.

Say you have one kernel buffer, which userspace subdivides into four
independent buffers. It can perform operations on these buffers which
are completely independent of each other, and an explicit sync model
allows this independence to be kept. Implicit sync ties them together,
so that you cannot do any operations on buffer 1 until all operations
on buffer 2 have completed.

> And is there any reason that fence fd shouldn't dependent of DMABUF - now 
> fence fd is a new file, not DMABUF fd?

Because dmabuf is for buffer sharing, and fences aren't buffers (they
will never export page ranges). Is there any particular benefit you
think you would get from doing this?

>> good thing. This is also the model that ChromeOS is moving towards, so
>> it becomes more important from that point of view as well.
>
> I think Gustavo should had explaned this path series enough to other people 
> when posting them - ie, what relationship explict and implicit fences have, 
> and why implicit fence - which is independent of DMABUF - is required, and 
> what use cases there are in real users, and etc.

Fair enough, the summary could perhaps contain something like this.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi all,

On 25 March 2016 at 11:58, Rob Clark  wrote:
> On Thu, Mar 24, 2016 at 7:49 PM, Inki Dae  wrote:
>> It's definitely different case. This tries to add new user-space interfaces 
>> to expose fences to user-space. At least, implicit interfaces are embedded 
>> into drivers.
>> So I'd like to give you a question. Why exposing fences to user-space is 
>> required? To provide easy-to-debug solution to rendering pipleline? To 
>> provide merge fence feature?
>
> Well, implicit sync and explicit sync are two different cases.
> Implicit sync ofc remains the default, but userspace could opt-in to
> explicit sync instead.  For example, on the gpu side of things,
> depending on flags userspace passes in to the submit ioctl we would
> either attach the fence to all the written buffers (implicit) or
> return it as a fence fd to userspace (explicit), which userspace could
> then pass in to atomic ioctl to synchronize pageflip.
>
> And visa-versa, we can pass the pageflip (atomic) completion fence
> back in to gpu so it doesn't start rendering the next frame until the
> buffer is off screen.
>
> fwiw, currently android is the first user of explicit sync (although I
> expect wayland/weston to follow suit).

Second, really. Vulkan avoids implicit sync entirely, and exposes
fence-like primitives throughout its whole API. These include being
able to pass prerequisite fences for display (what Gustavo is adding
here: something to block on before display), and also when the user
acquires a buffer as a render target, it is given another prerequisite
fence (the other side of what Gustavo is suggesting, i.e. the fence
triggers when the buffer is no longer displayed and becomes available
for rendering).

In order to implement this correctly, and avoid performance bubbles,
we need a primitive like this exposed through the KMS API, from both
sides. This is especially important when you take the case of
userspace suballocation, where userspace allocates larger blocks and
divides the allocation internally for different uses. Implicit sync
does not work at all for that case.

As stated before, there are other benefits, including much better
traceability. I would expect Wayland/Weston to also start pushing
support for this API relatively soon.

> A couple linaro folks have
> android running with an upstream kernel + mesa + atomic/kms hwc on a
> couple devices (nexus7 and db410c with freedreno, and qemu with
> virgl).  But there are some limitations due to missing the
> EGL_ANDROID_native_fence_sync extension in mesa.  I plan to implement
> that, but I ofc need the fence fd stuff in order to do so ;-)

Yes, having that would be a godsend for a lot of people.

>> And if we need really to expose fences to user-space and there is really 
>> real user, then we have already good candidates, DMA-BUF-IOCTL-SYNC or maybe 
>> fcntl system call because we share already DMA buffer between CPU <-> DMA 
>> and DMA <-> DMA using DMABUF.
>> For DMA-BUF-IOCTL-SYNC, I think you remember that was what I tried long time 
>> ago because you was there. Several years ago, I tried to couple exposing the 
>> fences to user-space with cache operation although at that time, I really 
>> misleaded the fence machnism. My trying was also for the potential users.
>
> Note that this is not (just) about sw sync, but also sync between
> multiple hw devices.

Sync isn't quite good enough, because it's a mandatory blocking point
for userspace. We want to push the explicit fences further down the
line, so userspace can parallelise its work.

Even if none of the above requirements held true, I don't think being
able to support Android is a bad thing. It's completely right to be
worried about pushing in Android work and APIs for the sake of it -
hence why we didn't take ADF! - but in this case it's definitely a
good thing. This is also the model that ChromeOS is moving towards, so
it becomes more important from that point of view as well.

Cheers,
Daniel

Re: [RFC 0/6] drm/fences: add in-fences to DRM

[Daniel Stone]

Hi all,

On 25 March 2016 at 11:58, Rob Clark  wrote:
> On Thu, Mar 24, 2016 at 7:49 PM, Inki Dae  wrote:
>> It's definitely different case. This tries to add new user-space interfaces 
>> to expose fences to user-space. At least, implicit interfaces are embedded 
>> into drivers.
>> So I'd like to give you a question. Why exposing fences to user-space is 
>> required? To provide easy-to-debug solution to rendering pipleline? To 
>> provide merge fence feature?
>
> Well, implicit sync and explicit sync are two different cases.
> Implicit sync ofc remains the default, but userspace could opt-in to
> explicit sync instead.  For example, on the gpu side of things,
> depending on flags userspace passes in to the submit ioctl we would
> either attach the fence to all the written buffers (implicit) or
> return it as a fence fd to userspace (explicit), which userspace could
> then pass in to atomic ioctl to synchronize pageflip.
>
> And visa-versa, we can pass the pageflip (atomic) completion fence
> back in to gpu so it doesn't start rendering the next frame until the
> buffer is off screen.
>
> fwiw, currently android is the first user of explicit sync (although I
> expect wayland/weston to follow suit).

Second, really. Vulkan avoids implicit sync entirely, and exposes
fence-like primitives throughout its whole API. These include being
able to pass prerequisite fences for display (what Gustavo is adding
here: something to block on before display), and also when the user
acquires a buffer as a render target, it is given another prerequisite
fence (the other side of what Gustavo is suggesting, i.e. the fence
triggers when the buffer is no longer displayed and becomes available
for rendering).

In order to implement this correctly, and avoid performance bubbles,
we need a primitive like this exposed through the KMS API, from both
sides. This is especially important when you take the case of
userspace suballocation, where userspace allocates larger blocks and
divides the allocation internally for different uses. Implicit sync
does not work at all for that case.

As stated before, there are other benefits, including much better
traceability. I would expect Wayland/Weston to also start pushing
support for this API relatively soon.

> A couple linaro folks have
> android running with an upstream kernel + mesa + atomic/kms hwc on a
> couple devices (nexus7 and db410c with freedreno, and qemu with
> virgl).  But there are some limitations due to missing the
> EGL_ANDROID_native_fence_sync extension in mesa.  I plan to implement
> that, but I ofc need the fence fd stuff in order to do so ;-)

Yes, having that would be a godsend for a lot of people.

>> And if we need really to expose fences to user-space and there is really 
>> real user, then we have already good candidates, DMA-BUF-IOCTL-SYNC or maybe 
>> fcntl system call because we share already DMA buffer between CPU <-> DMA 
>> and DMA <-> DMA using DMABUF.
>> For DMA-BUF-IOCTL-SYNC, I think you remember that was what I tried long time 
>> ago because you was there. Several years ago, I tried to couple exposing the 
>> fences to user-space with cache operation although at that time, I really 
>> misleaded the fence machnism. My trying was also for the potential users.
>
> Note that this is not (just) about sw sync, but also sync between
> multiple hw devices.

Sync isn't quite good enough, because it's a mandatory blocking point
for userspace. We want to push the explicit fences further down the
line, so userspace can parallelise its work.

Even if none of the above requirements held true, I don't think being
able to support Android is a bad thing. It's completely right to be
worried about pushing in Android work and APIs for the sake of it -
hence why we didn't take ADF! - but in this case it's definitely a
good thing. This is also the model that ChromeOS is moving towards, so
it becomes more important from that point of view as well.

Cheers,
Daniel

Re: [PATCH 2/2] drm/virtio: send vblank event on plane atomic update

[Daniel Stone]

Hi,

On 21 March 2016 at 19:23, Gustavo Padovan  wrote:
> @@ -96,6 +98,11 @@ static void virtio_gpu_plane_atomic_update(struct 
> drm_plane *plane,
>   plane->state->crtc_y,
>   plane->state->crtc_w,
>   plane->state->crtc_h);
> +
> +   spin_lock_irqsave(>dev->event_lock, flags);
> +   if (crtc->state->event)
> +   drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +   spin_unlock_irqrestore(>dev->event_lock, flags);

This seems like the wrong place to do it, in that it will generate one
flip event per plane, rather than one per CRTC. So this should
probably be done in the overall atomic_commit hook I think.

Also, without some kind of delay, this means that we'll generate
flip-complete events immediately, which will cause compositors like
Weston to render infinitely fast. It's probably worth looking at what
happened when this came up with Bochs - I'm not sure if we fake a 16ms
delay, or refuse to do async modesets, or what.

Cheers,
Daniel

Re: [PATCH 2/2] drm/virtio: send vblank event on plane atomic update

[Daniel Stone]

Hi,

On 21 March 2016 at 19:23, Gustavo Padovan  wrote:
> @@ -96,6 +98,11 @@ static void virtio_gpu_plane_atomic_update(struct 
> drm_plane *plane,
>   plane->state->crtc_y,
>   plane->state->crtc_w,
>   plane->state->crtc_h);
> +
> +   spin_lock_irqsave(>dev->event_lock, flags);
> +   if (crtc->state->event)
> +   drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +   spin_unlock_irqrestore(>dev->event_lock, flags);

This seems like the wrong place to do it, in that it will generate one
flip event per plane, rather than one per CRTC. So this should
probably be done in the overall atomic_commit hook I think.

Also, without some kind of delay, this means that we'll generate
flip-complete events immediately, which will cause compositors like
Weston to render infinitely fast. It's probably worth looking at what
happened when this came up with Bochs - I'm not sure if we fake a 16ms
delay, or refuse to do async modesets, or what.

Cheers,
Daniel

Re: [PATCH 0/2] drm/vc4: Fixes for Raspberry Pi 3

[Daniel Stone]

Hi,

On 1 March 2016 at 01:52, Eric Anholt <e...@anholt.net> wrote:
> These are for fixing the vc4 driver on the Pi 3.  Note that patch 2
> will also be necessary for fixing HPD on the Pi2, which we've been
> carrying downstream patches to work around until now.

Indeed. Having cherry-picked 80032d2e61 and added ACTIVE_LOW to the
bcm2836-rpi-2-b DT, I now get HPD reported correctly.

I think it might still need work: on RPi2 again, DDC fails on boot
(the same BCM2835_I2C_S_ERR bit being set, i.e. 'i2c transfer failed:
100'), and does work after a hotplug cycle. But the machine then
immediately hard-hangs - no serial console - after cat
/sys/class/drm/card0-HDMI-A-1/edid.

On the grounds that the VC4 node isn't yet in upstream DT though, and
this _does_ indeed fix HPD:
Tested-by: Daniel Stone <dani...@collabora.com>

Cheers,
Daniel

Re: [PATCH 0/2] drm/vc4: Fixes for Raspberry Pi 3

[Daniel Stone]

Hi,

On 1 March 2016 at 01:52, Eric Anholt  wrote:
> These are for fixing the vc4 driver on the Pi 3.  Note that patch 2
> will also be necessary for fixing HPD on the Pi2, which we've been
> carrying downstream patches to work around until now.

Indeed. Having cherry-picked 80032d2e61 and added ACTIVE_LOW to the
bcm2836-rpi-2-b DT, I now get HPD reported correctly.

I think it might still need work: on RPi2 again, DDC fails on boot
(the same BCM2835_I2C_S_ERR bit being set, i.e. 'i2c transfer failed:
100'), and does work after a hotplug cycle. But the machine then
immediately hard-hangs - no serial console - after cat
/sys/class/drm/card0-HDMI-A-1/edid.

On the grounds that the VC4 node isn't yet in upstream DT though, and
this _does_ indeed fix HPD:
Tested-by: Daniel Stone 

Cheers,
Daniel

Re: [PATCH] component: remove device from master match list on failed add

[Daniel Stone]

Russell,

On 12 February 2016 at 00:57, Akshay Bhat <akshay.b...@timesys.com> wrote:
> Daniel Stone  collabora.com> writes:
>> Fixes: ffc30b74fd6d01588bd3fdebc3b1acc0857e6fc8
>> Signed-off-by: Daniel Stone  collabora.com>
>
> Tested-by: Akshay Bhat <akshay.b...@timesys.com>
>
> Tested on imx6 processor based board where re-probe was broken after a
> probe deferral.

One-week ping; this breaks quite a few drivers, even despite
57480484f9 already being present. Another option could just be to
revert the original match-array commit (and the subsequent two fixups)
until you can work out a fix.

Cheers,
Daniel

Re: [PATCH] component: remove device from master match list on failed add

[Daniel Stone]

Russell,

On 12 February 2016 at 00:57, Akshay Bhat  wrote:
> Daniel Stone  collabora.com> writes:
>> Fixes: ffc30b74fd6d01588bd3fdebc3b1acc0857e6fc8
>> Signed-off-by: Daniel Stone  collabora.com>
>
> Tested-by: Akshay Bhat 
>
> Tested on imx6 processor based board where re-probe was broken after a
> probe deferral.

One-week ping; this breaks quite a few drivers, even despite
57480484f9 already being present. Another option could just be to
revert the original match-array commit (and the subsequent two fixups)
until you can work out a fix.

Cheers,
Daniel

Re: [RFC] component: Fix: Unassign components' masters if bringing up master fails

[Daniel Stone]

Hi Archit,

On 11 February 2016 at 09:35, Archit Taneja  wrote:
> component_master_add_with_match can fail if the master's bind op doesn't
> go through successfully. In such a scenario, all the components in the
> master's match array have their 'master' pointer set to the given master.
> These pointers need to be set to NULL again. If they aren't, successive
> calls to component_master_add_with_match will fail because the driver
> thinks these components already have a master.
>
> This issue can be seen when a driver defers probe because of missing
> resources. It is seen after the introduction of commit:
>
> "component: track components via array rather than list"
>
> Add 'master_remove_components' which sets the all the components's masters
> in the match array to NULL. This function is also re-used in
> component_master_del and replaces code that did the same thing.

Jon already fixed this (in a slightly more limited way perhaps?) in 57480484f9.

Cheers,
Daniel

Re: [RFC] component: Fix: Unassign components' masters if bringing up master fails

[Daniel Stone]

Hi Archit,

On 11 February 2016 at 09:35, Archit Taneja  wrote:
> component_master_add_with_match can fail if the master's bind op doesn't
> go through successfully. In such a scenario, all the components in the
> master's match array have their 'master' pointer set to the given master.
> These pointers need to be set to NULL again. If they aren't, successive
> calls to component_master_add_with_match will fail because the driver
> thinks these components already have a master.
>
> This issue can be seen when a driver defers probe because of missing
> resources. It is seen after the introduction of commit:
>
> "component: track components via array rather than list"
>
> Add 'master_remove_components' which sets the all the components's masters
> in the match array to NULL. This function is also re-used in
> component_master_del and replaces code that did the same thing.

Jon already fixed this (in a slightly more limited way perhaps?) in 57480484f9.

Cheers,
Daniel

[PATCH] component: remove device from master match list on failed add

[Daniel Stone]

Calling component_add() may result in the completion of a set of
devices, which will try to bring up a master. In bringing the master
up, we populate its match array with the current set of children.

If binding any of the devices fails, component_add() itself will fail,
free the struct component entry, and return to the caller. The
now-freed entry is never removed from the master's match array, and
will later be used in a futile attempt to bind to freed memory.

Bring component_add's behaviour on failure to bring up a master into
line with component_del by removing the (to-be-freed) component from
the master's match array.

The specific case which broke was:
  - rockchip_drm_drv adds a component master
  - dwhdmi_rockchip adds a child component in probe (master incomplete)
  - rockchip_drm_vop adds two children in probe, which completes the
set
  - inside component_add, we try to bring up the master, having
populated the master's match array, and fail with EPROBE_DEFER from
dwhdmi_rockchip; we delete the putative component
  - rockchip_drm_vop's probe fails and returns EPROBE_DEFER
  - we later re-probe rockchip_drm_vop and add the component; the
master is complete, so we attempt to bring it up again
  - walking the match array, we find the previous child, whose master
pointer doesn't match (as it has been freed in the meantime)
  - rockchip_drm_vop probe fails, and will never be attempted again

Fixes: ffc30b74fd6d01588bd3fdebc3b1acc0857e6fc8
Signed-off-by: Daniel Stone 
Cc: Russell King 
Cc: Thierry Reding 
Cc: Laurent Pinchart 
---
 drivers/base/component.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 2738039..04a1582 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -491,6 +491,8 @@ int component_add(struct device *dev, const struct 
component_ops *ops)
 
ret = try_to_bring_up_masters(component);
if (ret < 0) {
+   if (component->master)
+   remove_component(component->master, component);
list_del(>node);
 
kfree(component);
-- 
2.5.0

[PATCH] component: remove device from master match list on failed add

[Daniel Stone]

Calling component_add() may result in the completion of a set of
devices, which will try to bring up a master. In bringing the master
up, we populate its match array with the current set of children.

If binding any of the devices fails, component_add() itself will fail,
free the struct component entry, and return to the caller. The
now-freed entry is never removed from the master's match array, and
will later be used in a futile attempt to bind to freed memory.

Bring component_add's behaviour on failure to bring up a master into
line with component_del by removing the (to-be-freed) component from
the master's match array.

The specific case which broke was:
  - rockchip_drm_drv adds a component master
  - dwhdmi_rockchip adds a child component in probe (master incomplete)
  - rockchip_drm_vop adds two children in probe, which completes the
set
  - inside component_add, we try to bring up the master, having
populated the master's match array, and fail with EPROBE_DEFER from
dwhdmi_rockchip; we delete the putative component
  - rockchip_drm_vop's probe fails and returns EPROBE_DEFER
  - we later re-probe rockchip_drm_vop and add the component; the
master is complete, so we attempt to bring it up again
  - walking the match array, we find the previous child, whose master
pointer doesn't match (as it has been freed in the meantime)
  - rockchip_drm_vop probe fails, and will never be attempted again

Fixes: ffc30b74fd6d01588bd3fdebc3b1acc0857e6fc8
Signed-off-by: Daniel Stone <dani...@collabora.com>
Cc: Russell King <rmk+ker...@arm.linux.org.uk>
Cc: Thierry Reding <tred...@nvidia.com>
Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
---
 drivers/base/component.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 2738039..04a1582 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -491,6 +491,8 @@ int component_add(struct device *dev, const struct 
component_ops *ops)
 
ret = try_to_bring_up_masters(component);
if (ret < 0) {
+   if (component->master)
+   remove_component(component->master, component);
list_del(>node);
 
kfree(component);
-- 
2.5.0

Re: [PATCH v7 2/4] drm: Add support for ARM's HDLCD controller.

[Daniel Stone]

Hi,

On 22 December 2015 at 17:41, Liviu Dudau  wrote:
> The HDLCD controller is a display controller that supports resolutions
> up to 4096x4096 pixels. It is present on various development boards
> produced by ARM Ltd and emulated by the latest Fast Models from the
> company.

I didn't get to take as close a look as last time, but I only had
fairly minor quibbles then, and nothing jumped out at me this time
either.

Reviewed-by: Daniel Stone 

Cheers,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/