linux-kernel - Re: [PATCH 4/5] drm: Use mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170508075549.lympqv4bfby63np3@phenom.ffwll.local>
Date:   Mon, 8 May 2017 09:55:49 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     Ville Syrjälä 
        <ville.syrjala@...ux.intel.com>, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org,
        Carlos Palminha <CARLOS.PALMINHA@...opsys.com>,
        Alexey Brodkin <Alexey.Brodkin@...opsys.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Dave Airlie <airlied@...ux.ie>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Archit Taneja <architt@...eaurora.org>
Subject: Re: [PATCH 4/5] drm: Use mode_valid() in atomic modeset

On Thu, May 04, 2017 at 04:13:51PM +0100, Jose Abreu wrote:
> On 04-05-2017 15:40, Ville Syrjälä wrote:
> > On Thu, May 04, 2017 at 03:11:41PM +0100, Jose Abreu wrote:
> >> +					    struct drm_encoder *encoder,
> >> +					    struct drm_crtc *crtc,
> >> +					    struct drm_display_mode *mode)
> >> +{
> >> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> >> +	const struct drm_connector_helper_funcs *connector_funcs =
> >> +		connector->helper_private;
> >> +	const struct drm_encoder_helper_funcs *encoder_funcs =
> >> +		encoder->helper_private;
> >> +	enum drm_mode_status ret = MODE_OK;
> >> +
> >> +	if (connector_funcs && connector_funcs->mode_valid)
> >> +		ret = connector_funcs->mode_valid(connector, mode);
> > As I mentioned earlier, this would break i915. We either can't call the 
> > connector hook at all here, or we have to make it somehow opt-in if some
> > drivers really want to do it.
> 
> Ok. You said you let users exceed the limits, but why doesn't it
> fail in atomic_check and will fail in mode_valid? I guess I can
> remove it, but this can lead to lots of confusion, i.e. with this
> series the mode_valid callbacks for all components are called, if
> I remove just one call the whole point will fall apart.
> 
> Can we think about something else ? I think making it opt-in is
> not ideal, if the helper is there then it should be used, but if
> thats the best solution then shall we add a flag which will call
> or not *all* the mode_valid callbacks in this stage
> (drm_device.allow_modevalid_call, ...) ?

This is a general issue, not a driver opt-in. With your new callbacks
ideally we'll have:
- all the source checks (clock limits, size limits) are in the
  crtc/encoder/bridge callbacks
- only the sink limit checks (derived from edid or DP aux) are in the
  connector callback

Letting userspace overwrite these seems like a reasonable idea that we
should support in general I think. See also my comment on patch 1 for the
connector's mode_valid kerneldoc.

For arcpgu that might mean that you need to move part of the connector's
mode_valid checks into the encoder, but since all the encoder modeset is
in there already, that seems like a good cleanup of the drm modeset
framework.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch