Re: [PATCH 16/23] drm/i915: Program planes in bigjoiner mode.

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 16/23] drm/i915: Program planes in bigjoiner mode.
Date: Fri, 27 Sep 2019 17:41:49 +0300	[thread overview]
Message-ID: <20190927144149.GS1208@intel.com> (raw)
In-Reply-To: <ad1bccaa-86f4-aa75-b646-4207b00067ae@linux.intel.com>

On Fri, Sep 27, 2019 at 10:56:16AM +0200, Maarten Lankhorst wrote:
> Op 26-09-2019 om 21:11 schreef Ville Syrjälä:
> > On Thu, Sep 26, 2019 at 07:09:22PM +0300, Ville Syrjälä wrote:
> >> On Thu, Sep 26, 2019 at 05:50:05PM +0200, Maarten Lankhorst wrote:
> >>> Op 26-09-2019 om 15:06 schreef Ville Syrjälä:
> >>>> On Fri, Sep 20, 2019 at 01:42:28PM +0200, Maarten Lankhorst wrote:
> >>>>> Now that we can program planes from the update_slave callback, and
> >>>>> we have done all fb pinning correctly, it's time to program those
> >>>>> planes as well.
> >>>>>
> >>>>> We use the update_slave callback as it allows us to use the
> >>>>> separate states correctly.
> >>>>>
> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> ---
> >>>>>  .../gpu/drm/i915/display/intel_atomic_plane.c | 53 +++++++++++++++++++
> >>>>>  .../gpu/drm/i915/display/intel_atomic_plane.h |  2 +
> >>>>>  drivers/gpu/drm/i915/display/intel_display.c  |  4 +-
> >>>>>  3 files changed, 57 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> >>>>> index cc088676f0a2..5db091e4ad6a 100644
> >>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> >>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> >>>>> @@ -366,6 +366,59 @@ void skl_update_planes_on_crtc(struct intel_atomic_state *state,
> >>>>>  	}
> >>>>>  }
> >>>>>  
> >>>>> +void icl_update_bigjoiner_planes_on_crtc(struct intel_atomic_state *state,
> >>>>> +					 struct intel_crtc *crtc)
> >>>> This plane stuff is where things go very much off the rails IMO.
> >>>> Planes should not have to know anything about bigjoiner. They should
> >>>> just have their appropriate hw state and blindly bash it into the
> >>>> hardware.
> >>> We already need this for planar slave/master, what's the issue exactly?
> >> That already is too fragile. I don't want this spreading all over
> >> the driver and coupling everything to the bigjoiner logic.
> >>
> >> Here's a crude idea how I think we might avoid this:
> >> git://github.com/vsyrjala/linux.git uapi_hw_state_split
> >>
> >> But I didn't dare boot it yet...
> > It took a handful extra fixes to get that to boot. But now I at least
> > get a picture on the screen instead of oopses.
> >
> > Fixes pushed to the same branch.
> >
> > Looks like still something going wrong with the cursor ioctl though.
> >
> I've done a uapi-hw split in my series, is that ok with you?
> 
> I will do a similar split for planes.

I sent out some prep fixes, and respun my test branch as
uapi_hw_state_split_2. Even the legacy cursor now works.
So I think we might even have a chance of making this state
split thing work.

My suggestion on how to do it would be:

1. split the state and do the uapi<->hw copies, but leave the hw state still
   called 'base' to minimize the diff. We can then more easily see all
   the places where we have to consult to the uapi state.
2. probably do the base->hw rename with cocci or something

I think at least one thing missing from your split crtc state was
plane_mask. I added the required helpers for that in my prep patches.

I'm not 100% sure what we should do with the mode_change etc. flags. I
guess we can leave them just in the uapi state as long as we don't get
confused by the appearance of the 'uapi' name in various places.

Oh and to split the state for planes we'll need to hand roll our own
drm_atomic_helper_check_plane_state(). I didn't do that in my prep
patches because I was lazy.

Oh and drm_atomic_helper_setup_commit() is a big problem. In my test
branch I just copy pasted it to i915 and changed it to check the hw
state instead of the uapi state. But I don't really like that apporoach
so maybe we could reuse this helper still and just abstract away the
crtc active check.

Maybe have the caller pass in a function like so?

int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
                                  bool nonblock,
				  bool (*crtc_is_active)(const struct drm_crtc_state *crtc_state));
{
	...
-	if (!old_crtc_state->active && !new_crtc_state->active) {
+       if (!crtc_is_active(old_crtc_state) && !crtc_is_active(new_crtc_state)) {
	...

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx