On Thu, Dec 12, 2019 at 05:03:07PM -0800, Matt Roper wrote:
> On Wed, Dec 11, 2019 at 01:14:24PM -0800, Manasi Navare wrote:
> > Add an extra check before making master slave assignments for tiled
> > displays to make sure we make these assignments only if all tiled
> > connectors are present. If not then initialize the state to defaults
> > so it does a normal non tiled modeset without transcoder port sync.
> >
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.nav...@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
> > 1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7263eaa66cda..c0a2dab3fe67 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct
> > intel_crtc_state *new_crtc_state)
> > return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
> > }
> >
> > +static void initialize_trans_port_sync_mode_state(struct intel_crtc_state
> > *crtc_state)
>
> "reset" rather than "initialize" might be more consistent with similar
> things elsewhere in the driver?
Yes I was considering other names like default/reset/initialize, but yes I think
reset sounds better, will change that
>
> > +{
> > + crtc_state->master_transcoder = INVALID_TRANSCODER;
> > + crtc_state->sync_mode_slaves_mask = 0;
> > +}
> > +
> > static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > {
> > struct drm_crtc *crtc = crtc_state->uapi.crtc;
> > @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct
> > intel_crtc_state *crtc_state)
> > struct drm_crtc *master_crtc = NULL;
> > struct drm_crtc_state *master_crtc_state;
> > struct intel_crtc_state *master_pipe_config;
> > - int i, tile_group_id;
> > + int i, tile_group_id = 0, num_tiled_conns = 0;
> >
> > if (INTEL_GEN(dev_priv) < 11)
> > return 0;
> >
> > + /* If all tiles not present do not make master slave assignments
>
> This comment seems like it should go farther down the function; this
> block isn't directly responsible for master/slave assignments, so it's a
> bit confusing here. Also, you might want to clarify what "present"
> means in this case. E.g., explain that since you've already added all
> tiles of the same monitor to the transaction (earlier in
> intel_atomic_check), then if the number of tiles in the tile group is
> smaller than the size of the tile group it means that at least one of
> the tiles isn't active.
>
Yes will move this comment down
> > + * Here we assume all tiles belong to the same tile group for now.
Will add a FIXME
I will make these changes and add your r-b, thanks Matt
Manasi
>
> Should this sentence be a FIXME:? If we plug in 2x 2-tile monitors on
> TGL, this would be problematic, right?
>
> The logic changes seem correct (if we assume that only a single tiled
> monitor is in use), so
>
> Reviewed-by: Matt Roper <matthew.d.ro...@intel.com>
>
>
> Matt
>
> > + */
> > + for_each_new_connector_in_state(&state->base, connector,
> > connector_state, i) {
> > + if (connector->has_tile) {
> > + if (!tile_group_id)
> > + tile_group_id = connector->tile_group->id;
> > + num_tiled_conns++;
> > + }
> > + }
> > +
> > /*
> > * In case of tiled displays there could be one or more slaves but
> > there is
> > * only one master. Lets make the CRTC used by the connector
> > corresponding
> > @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct
> > intel_crtc_state *crtc_state)
> > if (!connector->has_tile)
> > continue;
> > if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > - crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> > + crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
> > + initialize_trans_port_sync_mode_state(crtc_state);
> > return 0;
> > + }
> > + if (connector->tile_group->id == tile_group_id &&
> > + num_tiled_conns < connector->num_h_tile *
> > connector->num_v_tile) {
> > + initialize_trans_port_sync_mode_state(crtc_state);
> > + return 0;
> > + }
> > if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > connector->tile_v_loc == connector->num_v_tile - 1)
> > continue;
> > --
> > 2.19.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
