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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@intel.com wrote:
> > > > From: Akash Goel <akash.goel@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.

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.

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.

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.

-- 
Ville Syrjälä
Intel OTC