| Message ID | 20190920114235.22411-16-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/23] drm/i915/dp: Fix dsc bpp calculations, v2. | expand |
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. > +{ > + struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > + struct intel_crtc_state *new_crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > + struct skl_ddb_entry entries_y[I915_MAX_PLANES]; > + struct skl_ddb_entry entries_uv[I915_MAX_PLANES]; > + u32 update_mask = new_crtc_state->update_planes; > + struct intel_plane *plane; > + > + memcpy(entries_y, old_crtc_state->wm.skl.plane_ddb_y, > + sizeof(old_crtc_state->wm.skl.plane_ddb_y)); > + memcpy(entries_uv, old_crtc_state->wm.skl.plane_ddb_uv, > + sizeof(old_crtc_state->wm.skl.plane_ddb_uv)); > + > + while ((plane = skl_next_plane_to_commit(state, crtc, > + entries_y, entries_uv, > + &update_mask))) { > + struct intel_plane_state *new_plane_state = > + intel_atomic_get_new_plane_state(state, plane); > + const struct intel_plane_state *master_plane_state; > + > + if (new_plane_state->base.visible) { > + master_plane_state = > + intel_atomic_get_new_plane_state(state, new_plane_state->bigjoiner_plane); > + > + intel_update_slave(plane, new_crtc_state, > + master_plane_state, new_plane_state); > + } else if (new_plane_state->slave) { > + /* > + * bigjoiner slave + planar slave. > + * The correct sequence is to get from the planar slave to planar master, > + * then to the master plane state for the master_plane_state. > + */ > + > + struct intel_plane *linked = new_plane_state->linked_plane; > + const struct intel_plane_state *uv_plane_state = > + intel_atomic_get_new_plane_state(state, linked); > + > + linked = uv_plane_state->bigjoiner_plane; > + master_plane_state = > + intel_atomic_get_new_plane_state(state, linked); > + > + intel_update_slave(plane, new_crtc_state, > + master_plane_state, new_plane_state); > + } else { > + intel_disable_plane(plane, new_crtc_state); > + } > + } > +} > + > void i9xx_update_planes_on_crtc(struct intel_atomic_state *state, > struct intel_crtc *crtc) > { > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > index 901a50e6e2d3..1cffda2b50b5 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > @@ -30,6 +30,8 @@ void intel_plane_free(struct intel_plane *plane); > struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); > void intel_plane_destroy_state(struct drm_plane *plane, > struct drm_plane_state *state); > +void icl_update_bigjoiner_planes_on_crtc(struct intel_atomic_state *state, > + struct intel_crtc *crtc); > void skl_update_planes_on_crtc(struct intel_atomic_state *state, > struct intel_crtc *crtc); > void i9xx_update_planes_on_crtc(struct intel_atomic_state *state, > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 06ceac4f1436..acb3c5974e99 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -14223,8 +14223,8 @@ static void intel_update_crtc(struct intel_crtc *crtc, > > commit_pipe_config(state, old_crtc_state, new_crtc_state); > > - if (new_crtc_state->bigjoiner) > - {/* Not supported yet */} > + if (new_crtc_state->bigjoiner_slave) > + icl_update_bigjoiner_planes_on_crtc(state, crtc); > else if (INTEL_GEN(dev_priv) >= 9) > skl_update_planes_on_crtc(state, crtc); > else > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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? We're using master_plane_state for all plane properties (fb/rotation/props) and plane_state for writing all state. >> +{ >> + struct intel_crtc_state *old_crtc_state = >> + intel_atomic_get_old_crtc_state(state, crtc); >> + struct intel_crtc_state *new_crtc_state = >> + intel_atomic_get_new_crtc_state(state, crtc); >> + struct skl_ddb_entry entries_y[I915_MAX_PLANES]; >> + struct skl_ddb_entry entries_uv[I915_MAX_PLANES]; >> + u32 update_mask = new_crtc_state->update_planes; >> + struct intel_plane *plane; >> + >> + memcpy(entries_y, old_crtc_state->wm.skl.plane_ddb_y, >> + sizeof(old_crtc_state->wm.skl.plane_ddb_y)); >> + memcpy(entries_uv, old_crtc_state->wm.skl.plane_ddb_uv, >> + sizeof(old_crtc_state->wm.skl.plane_ddb_uv)); >> + >> + while ((plane = skl_next_plane_to_commit(state, crtc, >> + entries_y, entries_uv, >> + &update_mask))) { >> + struct intel_plane_state *new_plane_state = >> + intel_atomic_get_new_plane_state(state, plane); >> + const struct intel_plane_state *master_plane_state; >> + >> + if (new_plane_state->base.visible) { >> + master_plane_state = >> + intel_atomic_get_new_plane_state(state, new_plane_state->bigjoiner_plane); >> + >> + intel_update_slave(plane, new_crtc_state, >> + master_plane_state, new_plane_state); >> + } else if (new_plane_state->slave) { >> + /* >> + * bigjoiner slave + planar slave. >> + * The correct sequence is to get from the planar slave to planar master, >> + * then to the master plane state for the master_plane_state. >> + */ >> + >> + struct intel_plane *linked = new_plane_state->linked_plane; >> + const struct intel_plane_state *uv_plane_state = >> + intel_atomic_get_new_plane_state(state, linked); >> + >> + linked = uv_plane_state->bigjoiner_plane; >> + master_plane_state = >> + intel_atomic_get_new_plane_state(state, linked); >> + >> + intel_update_slave(plane, new_crtc_state, >> + master_plane_state, new_plane_state); >> + } else { >> + intel_disable_plane(plane, new_crtc_state); >> + } >> + } >> +} >> + >> void i9xx_update_planes_on_crtc(struct intel_atomic_state *state, >> struct intel_crtc *crtc) >> { >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h >> index 901a50e6e2d3..1cffda2b50b5 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h >> @@ -30,6 +30,8 @@ void intel_plane_free(struct intel_plane *plane); >> struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); >> void intel_plane_destroy_state(struct drm_plane *plane, >> struct drm_plane_state *state); >> +void icl_update_bigjoiner_planes_on_crtc(struct intel_atomic_state *state, >> + struct intel_crtc *crtc); >> void skl_update_planes_on_crtc(struct intel_atomic_state *state, >> struct intel_crtc *crtc); >> void i9xx_update_planes_on_crtc(struct intel_atomic_state *state, >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index 06ceac4f1436..acb3c5974e99 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -14223,8 +14223,8 @@ static void intel_update_crtc(struct intel_crtc *crtc, >> >> commit_pipe_config(state, old_crtc_state, new_crtc_state); >> >> - if (new_crtc_state->bigjoiner) >> - {/* Not supported yet */} >> + if (new_crtc_state->bigjoiner_slave) >> + icl_update_bigjoiner_planes_on_crtc(state, crtc); >> else if (INTEL_GEN(dev_priv) >= 9) >> skl_update_planes_on_crtc(state, crtc); >> else >> -- >> 2.20.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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... > > We're using master_plane_state for all plane properties (fb/rotation/props) and plane_state for writing all state. > > >> +{ > >> + struct intel_crtc_state *old_crtc_state = > >> + intel_atomic_get_old_crtc_state(state, crtc); > >> + struct intel_crtc_state *new_crtc_state = > >> + intel_atomic_get_new_crtc_state(state, crtc); > >> + struct skl_ddb_entry entries_y[I915_MAX_PLANES]; > >> + struct skl_ddb_entry entries_uv[I915_MAX_PLANES]; > >> + u32 update_mask = new_crtc_state->update_planes; > >> + struct intel_plane *plane; > >> + > >> + memcpy(entries_y, old_crtc_state->wm.skl.plane_ddb_y, > >> + sizeof(old_crtc_state->wm.skl.plane_ddb_y)); > >> + memcpy(entries_uv, old_crtc_state->wm.skl.plane_ddb_uv, > >> + sizeof(old_crtc_state->wm.skl.plane_ddb_uv)); > >> + > >> + while ((plane = skl_next_plane_to_commit(state, crtc, > >> + entries_y, entries_uv, > >> + &update_mask))) { > >> + struct intel_plane_state *new_plane_state = > >> + intel_atomic_get_new_plane_state(state, plane); > >> + const struct intel_plane_state *master_plane_state; > >> + > >> + if (new_plane_state->base.visible) { > >> + master_plane_state = > >> + intel_atomic_get_new_plane_state(state, new_plane_state->bigjoiner_plane); > >> + > >> + intel_update_slave(plane, new_crtc_state, > >> + master_plane_state, new_plane_state); > >> + } else if (new_plane_state->slave) { > >> + /* > >> + * bigjoiner slave + planar slave. > >> + * The correct sequence is to get from the planar slave to planar master, > >> + * then to the master plane state for the master_plane_state. > >> + */ > >> + > >> + struct intel_plane *linked = new_plane_state->linked_plane; > >> + const struct intel_plane_state *uv_plane_state = > >> + intel_atomic_get_new_plane_state(state, linked); > >> + > >> + linked = uv_plane_state->bigjoiner_plane; > >> + master_plane_state = > >> + intel_atomic_get_new_plane_state(state, linked); > >> + > >> + intel_update_slave(plane, new_crtc_state, > >> + master_plane_state, new_plane_state); > >> + } else { > >> + intel_disable_plane(plane, new_crtc_state); > >> + } > >> + } > >> +} > >> + > >> void i9xx_update_planes_on_crtc(struct intel_atomic_state *state, > >> struct intel_crtc *crtc) > >> { > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > >> index 901a50e6e2d3..1cffda2b50b5 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > >> @@ -30,6 +30,8 @@ void intel_plane_free(struct intel_plane *plane); > >> struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); > >> void intel_plane_destroy_state(struct drm_plane *plane, > >> struct drm_plane_state *state); > >> +void icl_update_bigjoiner_planes_on_crtc(struct intel_atomic_state *state, > >> + struct intel_crtc *crtc); > >> void skl_update_planes_on_crtc(struct intel_atomic_state *state, > >> struct intel_crtc *crtc); > >> void i9xx_update_planes_on_crtc(struct intel_atomic_state *state, > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > >> index 06ceac4f1436..acb3c5974e99 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display.c > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >> @@ -14223,8 +14223,8 @@ static void intel_update_crtc(struct intel_crtc *crtc, > >> > >> commit_pipe_config(state, old_crtc_state, new_crtc_state); > >> > >> - if (new_crtc_state->bigjoiner) > >> - {/* Not supported yet */} > >> + if (new_crtc_state->bigjoiner_slave) > >> + icl_update_bigjoiner_planes_on_crtc(state, crtc); > >> else if (INTEL_GEN(dev_priv) >= 9) > >> skl_update_planes_on_crtc(state, crtc); > >> else > >> -- > >> 2.20.1 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
Op 26-09-2019 om 18:09 schreef Ville Syrjälä: > 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... So you basically want the same uapi/hw split for planes as we did with crtc's above?
On Thu, Sep 26, 2019 at 06:13:53PM +0200, Maarten Lankhorst wrote: > Op 26-09-2019 om 18:09 schreef Ville Syrjälä: > > 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... > > So you basically want the same uapi/hw split for planes as we did with crtc's above? Yes. And ideally doing the uapi->hw copy in one specific spot as early as possible, so that the rest of the driver can remain blissfully ignonarant about all of this.
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.
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.
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)) { ...
On Fri, Sep 27, 2019 at 05:41:49PM +0300, Ville Syrjälä wrote: > 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. Drat. At least kms_big_fb is busted. Need to figure out why.
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) +{ + struct intel_crtc_state *old_crtc_state = + intel_atomic_get_old_crtc_state(state, crtc); + struct intel_crtc_state *new_crtc_state = + intel_atomic_get_new_crtc_state(state, crtc); + struct skl_ddb_entry entries_y[I915_MAX_PLANES]; + struct skl_ddb_entry entries_uv[I915_MAX_PLANES]; + u32 update_mask = new_crtc_state->update_planes; + struct intel_plane *plane; + + memcpy(entries_y, old_crtc_state->wm.skl.plane_ddb_y, + sizeof(old_crtc_state->wm.skl.plane_ddb_y)); + memcpy(entries_uv, old_crtc_state->wm.skl.plane_ddb_uv, + sizeof(old_crtc_state->wm.skl.plane_ddb_uv)); + + while ((plane = skl_next_plane_to_commit(state, crtc, + entries_y, entries_uv, + &update_mask))) { + struct intel_plane_state *new_plane_state = + intel_atomic_get_new_plane_state(state, plane); + const struct intel_plane_state *master_plane_state; + + if (new_plane_state->base.visible) { + master_plane_state = + intel_atomic_get_new_plane_state(state, new_plane_state->bigjoiner_plane); + + intel_update_slave(plane, new_crtc_state, + master_plane_state, new_plane_state); + } else if (new_plane_state->slave) { + /* + * bigjoiner slave + planar slave. + * The correct sequence is to get from the planar slave to planar master, + * then to the master plane state for the master_plane_state. + */ + + struct intel_plane *linked = new_plane_state->linked_plane; + const struct intel_plane_state *uv_plane_state = + intel_atomic_get_new_plane_state(state, linked); + + linked = uv_plane_state->bigjoiner_plane; + master_plane_state = + intel_atomic_get_new_plane_state(state, linked); + + intel_update_slave(plane, new_crtc_state, + master_plane_state, new_plane_state); + } else { + intel_disable_plane(plane, new_crtc_state); + } + } +} + void i9xx_update_planes_on_crtc(struct intel_atomic_state *state, struct intel_crtc *crtc) { diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h index 901a50e6e2d3..1cffda2b50b5 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h @@ -30,6 +30,8 @@ void intel_plane_free(struct intel_plane *plane); struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); +void icl_update_bigjoiner_planes_on_crtc(struct intel_atomic_state *state, + struct intel_crtc *crtc); void skl_update_planes_on_crtc(struct intel_atomic_state *state, struct intel_crtc *crtc); void i9xx_update_planes_on_crtc(struct intel_atomic_state *state, diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 06ceac4f1436..acb3c5974e99 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -14223,8 +14223,8 @@ static void intel_update_crtc(struct intel_crtc *crtc, commit_pipe_config(state, old_crtc_state, new_crtc_state); - if (new_crtc_state->bigjoiner) - {/* Not supported yet */} + if (new_crtc_state->bigjoiner_slave) + icl_update_bigjoiner_planes_on_crtc(state, crtc); else if (INTEL_GEN(dev_priv) >= 9) skl_update_planes_on_crtc(state, crtc); else
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(-)