Re: [Intel-gfx] [PATCH v10 07/11] drm/i915: Make hardware readout work on i915.

From: "Navare, Manasi" <manasi.d.navare@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v10 07/11] drm/i915: Make hardware readout work on i915.
Date: Tue, 20 Oct 2020 14:57:40 -0700	[thread overview]
Message-ID: <20201020215740.GA27654@labuser-Z97X-UD5H> (raw)
In-Reply-To: <20201020185752.GA18439@labuser-Z97X-UD5H>

On Tue, Oct 20, 2020 at 11:57:52AM -0700, Navare, Manasi wrote:
> On Tue, Oct 20, 2020 at 09:45:02PM +0300, Ville Syrjälä wrote:
> > On Mon, Oct 19, 2020 at 03:45:04PM -0700, Navare, Manasi wrote:
> > > On Mon, Oct 19, 2020 at 07:36:59PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Oct 08, 2020 at 02:45:31PM -0700, Manasi Navare wrote:
> > > > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > 
> > > > > Unfortunately I have no way to test this, but it should be correct
> > > > > if the bios sets up bigjoiner in a sane way.
> > > > > 
> > > > > Skip iterating over bigjoiner slaves, only the master has the state we
> > > > > care about.
> > > > > 
> > > > > Add the width of the bigjoiner slave to the reconstructed fb.
> > > > > 
> > > > > Hide the bigjoiner slave to userspace, and double the mode on bigjoiner
> > > > > master.
> > > > > 
> > > > > And last, disable bigjoiner slave from primary if reconstruction fails.
> > > > > 
> > > > > v2:
> > > > > * Manual Rebase (Manasi)
> > > > > 
> > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 64 +++++++++++++++++++-
> > > > >  1 file changed, 62 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index faf8bf757bed..aa981aa4f6a1 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -3619,6 +3619,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> > > > >  	struct intel_plane *intel_plane = to_intel_plane(primary);
> > > > >  	struct intel_plane_state *intel_state =
> > > > >  		to_intel_plane_state(plane_state);
> > > > > +	 struct intel_crtc_state *crtc_state =
> > > > 
> > > > Whitespace fail. Didn't checkpatch complain?
> > > 
> > > I will fix this
> > > 
> > > > 
> > > > > +		 to_intel_crtc_state(intel_crtc->base.state);
> > > > >  	struct drm_framebuffer *fb;
> > > > >  	struct i915_vma *vma;
> > > > >  
> > > > > @@ -3641,7 +3643,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> > > > >  		if (c == &intel_crtc->base)
> > > > >  			continue;
> > > > >  
> > > > > -		if (!to_intel_crtc(c)->active)
> > > > > +		if (!to_intel_crtc_state(c->state)->uapi.active)
> > > > >  			continue;
> > > > >  
> > > > >  		state = to_intel_plane_state(c->primary->state);
> > > > > @@ -3663,6 +3665,11 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> > > > >  	 * pretend the BIOS never had it enabled.
> > > > >  	 */
> > > > >  	intel_plane_disable_noatomic(intel_crtc, intel_plane);
> > > > > +	if (crtc_state->bigjoiner) {
> > > > > +		struct intel_crtc *slave =
> > > > > +			crtc_state->bigjoiner_linked_crtc;
> > > > > +		intel_plane_disable_noatomic(slave, to_intel_plane(slave->base.primary));
> > > > > +	}
> > > > >  
> > > > >  	return;
> > > > >  
> > > > > @@ -10687,6 +10694,7 @@ static void
> > > > >  skl_get_initial_plane_config(struct intel_crtc *crtc,
> > > > >  			     struct intel_initial_plane_config *plane_config)
> > > > >  {
> > > > > +	struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state);
> > > > >  	struct drm_device *dev = crtc->base.dev;
> > > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > >  	struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> > > > > @@ -10795,6 +10803,18 @@ skl_get_initial_plane_config(struct intel_crtc *crtc,
> > > > >  	fb->height = ((val >> 16) & 0xffff) + 1;
> > > > >  	fb->width = ((val >> 0) & 0xffff) + 1;
> > > > >  
> > > > > +	/* add bigjoiner slave as well, if the fb stretches both */
> > > > > +	if (crtc_state->bigjoiner) {
> > > > > +		enum pipe bigjoiner_pipe = crtc_state->bigjoiner_linked_crtc->pipe;
> > > > > +
> > > > > +		if (fb->width == crtc_state->pipe_src_w &&
> > > > > +		    (intel_de_read(dev_priv, PLANE_SURF(pipe, plane_id)) & 0xfffff000) == plane_config->base) {
> > > > > +			val = intel_de_read(dev_priv, PLANE_SIZE(bigjoiner_pipe, plane_id));
> > > > > +			fb->height += ((val >> 16) & 0xfff) + 1;
> > > > > +			fb->width += ((val >> 0) & 0x1fff) + 1;
> > > > 
> > > > This looks wrong.
> > > 
> > > Why is it wrong? Double checked the plane size width and height bits from bspec
> > > and the mask looks correct here. 
> > > Can you elaborate on what is wrong here?
> > 
> > The pipes are side-by-side.

Gotcha, so Only the fb->width needs to be modified to add the width of slave pipe and keep height as is
Right? I will fix this in the next rev

Manasi

> 
> Yes the master and slave pipes are consecutive, still not sure what is required to correct the logic here
> 
> Manasi
> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx