Re: [Intel-gfx] [PATCH v2 13/13] drm/i915: Make the PIPESC rect relative to the entire bigjoiner area

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

On Thu, Mar 03, 2022 at 02:41:23PM -0800, Navare, Manasi wrote:
> On Wed, Feb 23, 2022 at 03:13:15PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When using bigjoiner it's useful to know the offset of each
> > individual pipe in the whole set of joined pipes. Let's include
> > that information in our PIPESRC rectangle. With this we can make
> > the plane clipping code blissfully unaware of bigjoiner usage, as
> > all we have to do is remove the pipe's offset from the final plane
> > destination coordinates.
> > 
> > v2: Use intel_bigjoiner_num_pipes()
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_atomic_plane.c |  7 +++---
> >  drivers/gpu/drm/i915/display/intel_cursor.c   |  8 ++++---
> >  drivers/gpu/drm/i915/display/intel_display.c  | 21 ++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_overlay.c  | 22 +++++++++----------
> >  4 files changed, 40 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 3cbf66146da0..92ae4eebc62f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -824,10 +824,6 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> >  		return -ERANGE;
> >  	}
> >  
> > -	/* right side of the image is on the slave crtc, adjust dst to match */
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		drm_rect_translate(dst, -drm_rect_width(&crtc_state->pipe_src), 0);
> > -
> >  	/*
> >  	 * FIXME: This might need further adjustment for seamless scaling
> >  	 * with phase information, for the 2p2 and 2p1 scenarios.
> > @@ -844,6 +840,9 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> >  		return -EINVAL;
> >  	}
> >  
> > +	/* final plane coordinates will be relative to the plane's pipe */
> > +	drm_rect_translate(dst, -clip->x1, -clip->y1);
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index da6cf0515164..9279e2783e7e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -152,9 +152,11 @@ static int intel_check_cursor(struct intel_crtc_state *crtc_state,
> >  	/* Use the unclipped src/dst rectangles, which we program to hw */
> >  	plane_state->uapi.src = src;
> >  	plane_state->uapi.dst = dst;
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		drm_rect_translate(&plane_state->uapi.dst,
> > -				   -drm_rect_width(&crtc_state->pipe_src), 0);
> > +
> > +	/* final plane coordinates will be relative to the plane's pipe */
> > +	drm_rect_translate(&plane_state->uapi.dst,
> > +			   -crtc_state->pipe_src.x1,
> > +			   -crtc_state->pipe_src.y1);
> >  
> >  	ret = intel_cursor_check_surface(plane_state);
> >  	if (ret)
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7a09bb33c1eb..a9c15f27b948 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -3204,6 +3204,23 @@ static void intel_get_transcoder_timings(struct intel_crtc *crtc,
> >  	}
> >  }
> >  
> > +static void intel_bigjoiner_adjust_pipe_src(struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	int num_pipes = intel_bigjoiner_num_pipes(crtc_state);
> > +	enum pipe master_pipe, pipe = crtc->pipe;
> > +	int width;
> > +
> > +	if (num_pipes < 2)
> > +		return;
> > +
> > +	master_pipe = bigjoiner_master_pipe(crtc_state);
> > +	width = drm_rect_width(&crtc_state->pipe_src);
> > +
> > +	drm_rect_translate_to(&crtc_state->pipe_src,
> > +			      (pipe - master_pipe) * width, 0);
> > +}
> > +
> >  static void intel_get_pipe_src_size(struct intel_crtc *crtc,
> >  				    struct intel_crtc_state *pipe_config)
> >  {
> > @@ -3216,6 +3233,8 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
> >  	drm_rect_init(&pipe_config->pipe_src, 0, 0,
> >  		      REG_FIELD_GET(PIPESRC_WIDTH_MASK, tmp) + 1,
> >  		      REG_FIELD_GET(PIPESRC_HEIGHT_MASK, tmp) + 1);
> > +
> > +	intel_bigjoiner_adjust_pipe_src(pipe_config);
> >  }
> >  
> >  static void i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state)
> > @@ -5853,6 +5872,8 @@ intel_modeset_pipe_config_late(struct intel_crtc_state *crtc_state)
> >  	struct drm_connector *connector;
> >  	int i;
> >  
> > +	intel_bigjoiner_adjust_pipe_src(crtc_state);
> > +
> >  	for_each_new_connector_in_state(&state->base, connector,
> >  					conn_state, i) {
> >  		struct intel_encoder *encoder =
> > diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> > index 631e1f1dc5e6..ee46561b5ae8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> > @@ -958,21 +958,21 @@ static void update_pfit_vscale_ratio(struct intel_overlay *overlay)
> >  static int check_overlay_dst(struct intel_overlay *overlay,
> >  			     struct drm_intel_overlay_put_image *rec)
> >  {
> > -	const struct intel_crtc_state *pipe_config =
> > +	const struct intel_crtc_state *crtc_state =
> >  		overlay->crtc->config;
> > -	int pipe_src_w = drm_rect_width(&pipe_config->pipe_src);
> > -	int pipe_src_h = drm_rect_height(&pipe_config->pipe_src);
> > +	struct drm_rect req, clipped;
> >  
> > -	if (rec->dst_height == 0 || rec->dst_width == 0)
> > -		return -EINVAL;
> > +	drm_rect_init(&req, rec->dst_x, rec->dst_y,
> > +		      rec->dst_width, rec->dst_height);
> > +
> > +	clipped = req;
> > +	drm_rect_intersect(&clipped, &crtc_state->pipe_src);
> >  
> > -	if (rec->dst_x < pipe_src_w &&
> > -	    rec->dst_x + rec->dst_width <= pipe_src_w &&
> > -	    rec->dst_y < pipe_src_h &&
> > -	    rec->dst_y + rec->dst_height <= pipe_src_h)
> > -		return 0;
> > -	else
> > +	if (!drm_rect_visible(&clipped) ||
> > +	    !drm_rect_equals(&clipped, &req))
> >  		return -EINVAL;
> > +
> > +	return 0;
> 
> I dont quite understand what we are trying to do here with the clipped ?
> Can you elaborate a bit what this function does?

It checks whether the userspace provided coordinates were fully
within the pipe source rectangle. If not we return an error.

I suppose technically we should be checking against the uapi
hdisplay/vdisplay since this is uapi stuff, but no bigjoiner
on these old platforms so this will effectively do the same
thing.

-- 
Ville Syrjälä
Intel