[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 05:26:27PM +0200, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 06:06:58PM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 14, 2014 at 04:42:01PM +0200, Daniel Vetter wrote:
> > > On Thu, Aug 14, 2014 at 02:54:27PM +0530, akash.goel at intel.com wrote:
> > > > From: Akash Goel <akash.goel at intel.com>
> > > > +		/* Check if the current FB is compatible with new requested
> > > > +		   Pipesrc values by the User */
> > > > +		if (new_width > fb->width ||
> > > > +			new_height > fb->height ||
> > > > +			crtc->x > fb->width - new_width ||
> > > > +			crtc->y > fb->height - new_height) {
> > > > +			DRM_DEBUG_KMS("New Pipe Src values %dx%d is incompatible with current fb size & viewport %ux%u+%d+%d\n",
> > > > +			      new_width, new_height, fb->width, fb->height, crtc->x, crtc->y);
> > > > +			return -EINVAL;
> > > > +		}
> > > 
> > > I think this part here is the achilles heel of your approach. Right now we
> > > use crtc->mode.hdisplay/vdisplay
> > 
> > We don't use it in i915. If we do that's a bug. All the relevant places
> > should be loooking at pipe_src_{w,h}. In the core the viewport check
> > should be about the only place that cares about this stuff.
> 
> Well within i915, but not anywhere in the drm core or helper code since
> they're simply not aware of pipe_src_w/h. E.g. the plane helper code also
> uses crtc->mode.h/vdisplay.
> 
> Changing that basic drm assumption is really invasive imo.
> 
> > > in a lot of places both in i915 and the
> > > drm core to make sure that the primary plane fb, sprite fb and positioning
> > > and cursor placement all make sense.
> > > 
> > > If my understanding of the pipe src rectangle is correct (please correct
> > > me if I'm wrong) we should switch _all_ these checks to instead look at
> > > the new crtc_w/h field. Even worse that we need to change drm core code
> > > and as a result of that all drm drivers. Awful lot of code to check, test
> > > and for i915 validate with i-g-t testcases.
> > > 
> > > Now the solution thus far (for the normal panel fitter operation) is that
> > > the logical input mode for the crtc ends up in crtc->config.mode and as a
> > > copy in crtc->mode. And the actual output mode is in
> > > crtc->config.adjusted_mode.
> > > 
> > > Our modeset code already takes care of copying crtc->config.mode to
> > > crtc->mode, so we only need to concern ourselfs with crtc->config.mode. If
> > > we'd copy the pipe_src_w/h values back into it the existing code would
> > > still be able to check all the sprite/cursor/fb constraints.
> > > 
> > > So the flow on a modeset (and that's what we'll end up calling from the
> > > set_property function too) is:
> > > 
> > > 1. Userspace requested mode goes into pipe_config->mode.
> > > 2. We make a copy into pipe_config->adjusted_mode and frob it more.
> > > 3. crtc compute_config code notices the special pipe src window, and
> > > adjusts pipe_config->mode plus computes the panel fitter configuration.
> > > 
> > > If all that checks out we continue with the modeset sequence.
> > > 
> > > 4. We store the new pipe_config into crtc->config.
> > > 5. Actual hw register writes for the modeset change happens.
> > > 6. We copy crtc->config.mode into crtc->mode so that drm core and helper
> > > functions can validate fb/sprite/cursors again.
> > 
> > We shouldn't just magically change the user specified mode, we need it
> > to stay intact for a subsequent modeset so that we can start the
> > adjusted_mode frobbery fresh next time around. It also seems weird to
> > report back a different mode to userspace than what the user provided.
> > What you suggest was exactly the previous approach and I NAKed it.
> 
> Oh I'm fully aware that it's in the leagues of cross hacks ;-) But doing
> the crtc->src_w/h thing correctly rolled out across the entire drm
> subsystem will be a big chunk more work than just adding new variables to
> struct drm_crtc which are only used in i915.
> 
> > > The result would be that the set_property function here would do _no_
> > > argument checking, but would instead fully rely upon the modeset sequence
> > > to compute the desired configuration.
> > 
> > We don't have sufficient checks in the modeset path. The
> > drm_crtc_check_viewport() call is in drm_mode_setcrtc() which is too
> > high up in the stack to affect modesets originating from property
> > changes. That being said we should definitely use drm_crtc_check_viewport()
> > here instead of hand rolling the exacty same thing in i915.
> 
> I guess then it needs to be moved down eventually.
> 
> > And to avoid having to touch too much code, drm_crtc_check_viewport()
> > should encapsulate the logic to fall back to mode->{h,v}display when
> > crtc->{w,h} are zero.
> 
> Actually you get to do a full drm audit anyway, since there's more than
> check_viewport. They should all switch to crtc->src_w/h, and we should
> fill that out by default in the crtc helpers. Imo stuffing magic new stuff
> into the core only used by one driver isn't good, if we go this route we
> should do it properly.
> 
> Or we ditch the struct drm_crtc change and pull it all into i915.

We can't since we need to get past drm_crtc_check_viewport().

-- 
Ville Syrjälä
Intel OTC