All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Christian König" <christian.koenig@amd.com>
To: Jean Delvare <jdelvare@suse.de>
Cc: Alex Deucher <alexander.deucher@amd.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: userptr support in drm drivers
Date: Fri, 19 Feb 2016 15:58:24 +0100	[thread overview]
Message-ID: <56C72D90.5030403@amd.com> (raw)
In-Reply-To: <20160219151133.22231488@endymion>

> If a minority of users need to opt out from MMU_NOTIFIER, then
> DRM_RADEON_USERPTR and friends should default to y and be hidden behind
> EXPERT. That way you avoid asking yet more questions to everyone else.
> Would that work for you?
No, just the other way around.

When MMU_NOTIFIER is selected anyway we actually don't want to display 
the option, but that's not possible for none menu entries IIRC the 
Kconfig correctly.

If somebody configures the kernel and don't selects any of the other 
dependencies for MMU_NOTIFIER the feature should not be available by 
default.

Making it an expert option is fine with me.

Regards,
Christian.

Am 19.02.2016 um 15:11 schrieb Jean Delvare:
> Hi Christian,
>
> On Thu, 18 Feb 2016 10:48:18 +0100, Christian König wrote:
>> Am 18.02.2016 um 09:59 schrieb Jean Delvare:
>>> Maybe I was not clear enough in my original post, but I am really
>>> advocating for FEWER kconfig options, not more. Plus I just explained
>>> why I think the radeon and amdgpu drivers do it wrong, so I'm certainly
>>> not going to align i915 on that.
>>>
>>>> Distro kernels pretty much all select MMU_NOTIFIER already for unrelated
>>>> reasons, but it's good to be less suprising for everyone who builds their
>>>> own custom kernel.
>>> The current situation is actually very surprising and this is what I
>>> would like to change. The options of one driver depending on choices
>>> made somewhere else is utterly confusing. CONFIG_RADEON_USERPTR and
>>> CONFIG_AMDGPU_USERPTR give the user a little more control where i915
>>> gives none, but that's only in one direction (you can force full
>>> userptr support in, but you can't force it out.)
>> As I said, that is perfectly fine and exactly what we want.
> I don't know what "we" represent in your sentence, but I doubt a
> majority of users want that.
>
>>> I'm still waiting for someone to explain why we can't just
>>> unconditionally select MMU_NOTIFIER in these 3 drivers and be done with
>>> it. That's less work at kernel configuration time AND fewer
>>> preprocessing conditionals within the code, so easier/better testing
>>> coverage and more readable code. We would need a very good reason for
>>> NOT doing it.
>> Adding the MMU Notifier causes overhead in the MM which people want to
>> avoid when they don't need userptr support.
>>
>> We used to select MMU_NOTIFIER unconditionally before and explicitly
>> added this option on user request. So yes it is clearly necessary.
> Thanks for the explanation.
>
> Can you tell me more about the overhead? Is it actually measurable?
> Which people need this degree of optimization?
>
> Are the i915, radeon or amdgpu drm drivers used on embedded systems?
>
> Just because people asked for something doesn't mean that implementing
> it is a good idea. Keeping things simple is a valid goal on its own.
> Anyone could ask for features they don't need to become options they
> can disable, but in the long run this approach can't be sustained.
>
>> The additional userptr support code in the drivers are negligible so
>> always enabling the code when the MMU Notifier is available is also
>> perfectly fine.
> Indeed this is a small amount of code. OTOH you have ifdefs in place
> anyway. Using the right ones would turn the option into a real option,
> i.e. the user decides to enable a feature or not. Not a "you may or may
> not get the feature" option as we have at the moment. More control to
> the user and less confusion.
>
>> So if you want to clean it up make a common DRM option which selects MMU
>> Notifier if it isn't already selected.
> I will, as it will still be an improvement compared to the current
> situation. But I still believe this is confusing in its current form.
>
> If a minority of users need to opt out from MMU_NOTIFIER, then
> DRM_RADEON_USERPTR and friends should default to y and be hidden behind
> EXPERT. That way you avoid asking yet more questions to everyone else.
> Would that work for you?
>

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

  reply	other threads:[~2016-02-19 15:14 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-16 19:58 userptr support in drm drivers Jean Delvare
2016-02-16 20:33 ` Christian König
2016-02-16 21:14   ` Daniel Vetter
2016-02-18  8:59     ` Jean Delvare
2016-02-18  9:48       ` Christian König
2016-02-19 14:11         ` Jean Delvare
2016-02-19 14:58           ` Christian König [this message]
2016-02-29 15:35             ` Daniel Vetter
2016-02-18  8:49   ` Jean Delvare

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=56C72D90.5030403@amd.com \
    --to=christian.koenig@amd.com \
    --cc=alexander.deucher@amd.com \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jdelvare@suse.de \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.