[Intel-gfx] [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Daniel Vetter]

On Thu, Aug 14, 2014 at 6:45 PM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Thu, Aug 14, 2014 at 06:07:44PM +0200, Daniel Vetter wrote:
>> On Thu, Aug 14, 2014 at 5:45 PM, Ville Syrjälä
>> <ville.syrjala at linux.intel.com> wrote:
>> > My quick grep audit tells me the viewport check and
>> > drm_primary_helper_update() are the only places in the core that care.
>> > The latter is rather dubious anyway since the clipping should be done
>> > against the adjusted mode and not the user specified mode, so anyone
>> > using that is already dancing on thin ice.
>>
>> My understanding has always been that the requested mode is what
>> userspace plans to feed into the pipe, and the adjusted mode is what
>> actually lands in the sink. Yeah there's some hilarity in the vblank
>> code which somehow presumes that the vblank counter works with the
>> adjusted mode because that's what i915 needs.
>
> The problem with clipping planes to the user size is that the driver is
> free to frob the mode a bit to line it up with hardware constraints. So
> what the user requested might be a few pixels off compared to what the
> hardware will end up using, and then if you configure the plane
> blindly using the coordinates clipped against the user size, the
> hardware may get somewhat upset.

Hm, I've thought we didn't do that yet, but only frobbed the adjusted
mode to make it fit our requirements for e.g interlaced stuff. I don't
think it would be good if we start doing that since there's no way for
userspace to be aware of the resulting single-pixel border of garbage.

_If_ we need to do that I think we should frob modes when adding them
to the connector mode list.

>> It's that fundamental assumption that we break by making the pipe_src
>> stuff official and which is the part that freaks me out a bit.
>
> Yeah there's definitely some dangers with these properties. The biggest
> being when one master sets the properties, and then another master which
> doesn't know anything about these properties takes over. The result
> might be a failed modeset an then the user doesn't see anything.
>
> This is one reason why I'd prefer that we'd maintain the state
> per-master. For fbdev the "reset everything to default" trick seems
> sufficient but I'm not sure I'd like to that to actual users.

Yeah, this is unsolved territory. Not sure whether attaching the state
to the master should be helpful, or whether we should care at all. For
a generic distro we should boot-up sane-ish, and userspace can always
grab the full property picture and restore that if all else fails. The
fbcon restoring is really just for developers imo.

>> > The other drivers are something I would not touch. Given how many places
>> > we had to frob in i915 I'm somewhat sure I'd not like what I see there
>> > and then any patch I might cook up would be too half assed to satisfy my
>> > quality standards anyway.
>>
>> Yeah, other drivers only need to be audited I think once they start
>> supporting the pipe_src stuff. But I think the core+helpers should be
>> able to cope properly.
>>
>> > As far as always filling the crtc->w,h always goes, I'm not sure that's
>> > the best idea anyway since we still need the "0 is special" handling.
>> > Well, we could fill them out, but then we definitely need a flag or
>> > something to indicate that they came from the mode and not the
>> > properties, so that we know whether we should overwrite them from
>> > with {h,v}display during a subsquent modeset or if they should keep
>> > their current value.
>>
>> Hm, I guess we can keep that implicit meaning, but then we need a
>> small helper to get at the crtc viewport, e.g. drm_crtc_viewport_rect
>> or so. That would also be an excellent place to document this trickery
>> properly.
>
> Yeah such small helpers is what I had in mind when suggesting this
> originally.
>
>> Oh and: Such drm changes _really_ must be split out into separate prep
>> patches cc: dri-devel.
>
> Indeed.

Sounds like we have a rough plan.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch