Re: [PATCH 5/9] drm/i915: Setup clipped src/dest coordinates during FB reconstruction (v2)

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Dec 03, 2015 at 01:06:44PM +0100, Maarten Lankhorst wrote:
> Op 25-11-15 om 17:48 schreef Matt Roper:
> > Plane state objects contain two copies of src/dest coordinates:  the
> > original (requested by userspace) coordinates in the base
> > drm_plane_state object, and a second, clipped copy (i.e., what we
> > actually want to program to the hardware) in intel_plane_state.  We've
> > only been setting up the former set of values during boot time FB
> > reconstruction, but we should really be initializing both.
> >
> > Note that the code here probably still needs some more work since we
> > make a lot of assumptions about how the BIOS programmed the hardware
> > that may not always be true, especially on gen9+; e.g.,
> >  * Primary plane might not be positioned at 0,0
> >  * Primary plane could have been rotated by the BIOS
> >  * Primary plane might be scaled
> >  * The BIOS fb might be a single "extended mode" FB that spans
> >    multiple displays.
> >  * ...etc...
> >
> > v2: Reword/expand commit message description of assumptions we make
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > Reviewed-by(v1): Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 73e9bf9..00e4c37 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2599,6 +2599,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >  	struct drm_plane_state *plane_state = primary->state;
> >  	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
> >  	struct intel_plane *intel_plane = to_intel_plane(primary);
> > +	struct intel_plane_state *intel_state =
> > +		to_intel_plane_state(plane_state);
> >  	struct drm_framebuffer *fb;
> >  
> >  	if (!plane_config->fb)
> > @@ -2659,6 +2661,15 @@ valid_fb:
> >  	plane_state->crtc_w = fb->width;
> >  	plane_state->crtc_h = fb->height;
> >  
> > +	intel_state->src.x1 = plane_state->src_x;
> > +	intel_state->src.y1 = plane_state->src_y;
> > +	intel_state->src.x2 = plane_state->src_x + plane_state->src_w;
> > +	intel_state->src.y2 = plane_state->src_y + plane_state->src_h;
> > +	intel_state->dst.x1 = plane_state->crtc_x;
> > +	intel_state->dst.y1 = plane_state->crtc_y;
> > +	intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
> > +	intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h;
> >
> Why does it matter that those coordinates are set up? The first atomic commit would overwrite those anyway..

We potentially use these during watermark calculations when trying to
calculate what we think sane watermark values would be for the
hardware-readout state, which is before we get to our first commit.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx