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

[Ville Syrjälä]

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.

> 
> 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.

> 
> > 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.

-- 
Ville Syrjälä
Intel OTC