[Intel-gfx] [RFC] drm/i915/dp: Preliminary support for 2 pipe 1 port mode for 5K at 120

[Maarten Lankhorst]

Op 23-01-2019 om 18:31 schreef Matt Roper:
> On Tue, Jan 22, 2019 at 01:12:07PM -0800, Manasi Navare wrote:
>> On Gen 11 platform, to enable resolutions like 5K at 120 where
>> the pixel clock is greater than pipe pixel rate, we need to split it across
>> 2 pipes and enable it using DSC and big joiner. In order to support this
>> dual pipe single port mode, we need to link two crtcs involved in this
>> ganged mode.
>>
>> This patch is a RFC patch that links two crtcs using linked_crtc pointer
>> in intel_crtc_state and slave to indicate if the crtc is a master or slave.
>> Here the HW necessitates the first CRTC to be the master CRTC through which
>> the final output will be driven and the next consecutive CRTC should be
>> slave crtc.
>>
>> This is currently not tested, but I wanted to get some inputs on this approach.
>> The idea is to follow the same approach used in Ganged plane mode for NV12
>> planes.
>>
>> Suggested-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>,
>> Matt Roper <matthew.d.roper at intel.com>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper at intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> Looks like the right general approach of using slave/master.  I agree
> with Ville and Maarten's feedback as well.
>
> The other thing we're going to need to worry about is dealing with all
> the planes on the two crtc's.  Suppose we have a userspace-visible state
> of:
>
>    Pipe A:  off
>    Pipe B:  5000x2000 at 120 (using round numbers to make example simpler)
>       Plane 0:  RGB32: pos (0,0), width=5000, height=2000
>       Plane 1:  RGB32: pos (100,100), width=100, height=100
>       Plane 2:  RGB32: pos (4800,1800), width=100, height=100
>       Plane 3:  RGB32: pos (0,0), width=3000, height=2000
>       Plane 4:  NV12:  pos (2000,0), width=1000, height=2000
>       Plane 5:  off
>       Plane 6:  off
>    Pipe C:  off
>
> this means that we need to grab extra planes on the other CRTC and
> adjust their size, position, and/or surface offsets accordingly.  So the
> internal driver state that we actually program into the hardware needs
> to be something like:
>
>    Pipe A:  off
>    Pipe B:  2500x1000 (master)
>       Plane 0:  pos (0,0), width=2500, height=2000
>       Plane 1:  pos (100,100), width=100, height=100
>       Plane 2:  off
>       Plane 3:  pos (0, 0), width=2500, height=2000
>       Plane 4{UV}: pos (2000, 0), width=500, height=2000
>       Plane 5{Y}:  pos (2000, 0), width=500, height=2000
>       Plane 6:  off
>    Pipe C:  2500x1000 (slave)
>       Plane 0:  pos (0,0), offset=(2500,0), width=2500, height=2000
>       Plane 1:  off
>       Plane 2:  pos (2300,1800), width=100, height=100
>       Plane 3:  pos (0, 0), offset=(2500, 0), width=500, height=2000
>       Plane 4{UV}: pos (0, 0), offset=(500,0), width=500, height=2000
>       Plane 5{Y}:  pos (0, 0), offset=(500,0), width=500, height=2000
>       Plane 6:  off
>
> So I think Ville is right; we're going to need to really copy a lot of
> the userspace-facing state data into our own internal state structures
> so that we can do the various adjustments on it.  As you can see above
> there are cases (2pi1po + nv12) where a single userspace plane request
> translates into us programming four different sets of plane register
> values.

Yeah I fear you're right.
Lets see, we would need to sanitize the driver big time..


struct drm_crtc_state {
  struct drm_crtc *crtc;
  bool enable;
  bool active;
  bool planes_changed : 1;
  bool mode_changed : 1;
  bool active_changed : 1;
  bool connectors_changed : 1;
  bool zpos_changed : 1; // unused
  bool color_mgmt_changed : 1;
  bool no_vblank : 1; //unused
  u32 plane_mask;
  u32 connector_mask;
  u32 encoder_mask;
  struct drm_display_mode adjusted_mode;
  struct drm_display_mode mode;
  struct drm_property_blob *mode_blob;
  struct drm_property_blob *degamma_lut;
  struct drm_property_blob *ctm;
  struct drm_property_blob *gamma_lut;
  u32 target_vblank; // unused
  u32 pageflip_flags; // unused
  struct drm_pending_vblank_event *event;
  struct drm_crtc_commit *commit;
  struct drm_atomic_state *state;
};

First the good, it looks like we can safely re-use the following in the slave, without affecting userspace:
  struct drm_crtc * crtc;
  u32 last_vblank_count; // ignored in i915
  struct drm_display_mode adjusted_mode; // sort of handled by core, but on disable we can write our own mode here
  struct drm_pending_vblank_event * event; // handled by core
  struct drm_atomic_state * state; // handled by core


Then one we might use inside i915, but should not inside the driver:
  struct drm_display_mode mode; -> use crtc_state.adjusted_mode..

Then the bad, we can easily get from the master safely. We only need to add special handling in intel_color.c:
  struct drm_property_blob * degamma_lut;
  struct drm_property_blob * ctm;
  struct drm_property_blob * gamma_lut;

Also bad,
  u32 plane_mask; // See below.
  u32 connector_mask; // Hmm, I don't think we care about connectors on slave, do we? Just special handling in the modeset calculations needed
  u32 encoder_mask; // Handled by core, but probably fine to leave as 0. Can't touch this. Don't need to either.
  bool planes_changed:1;
  bool mode_changed:1;
  bool active_changed:1;
  bool connectors_changed:1;
  bool color_mgmt_changed:1;

Whatever for most of those, can ignore the masks probably. Just make sure master plane state checks
the slave plane state as well, and ignore direct checks of slave plane state. Because this is a 1:1 mapping,
it should be fine.

And finally, the ugly:
  bool enable;
  bool active;

The biggest issues are crtc_state->active and crtc_state->enable. i915 uses it a lot, and even the core makes
a lot of decisions based on it. We should probably only steal crtc's that doesn't have crtc_state->enable set.

But one problem at a time..

plane_state is relatively easy, because even if we can't touch plane_mask and the atomic details for the planes,
we need to derive this from the master plane check function anyway. Fortunately we have our own
intel_crtc_state->active_planes already used in the plane update functions we just need to be sure that the
planes are part of the state. We should probably completely ignore the slave planes in the plane atomic check
called from the core when it's used as a slave..

Once for plane_state:
struct drm_plane_state {
  struct drm_plane *plane;
  struct drm_crtc *crtc;
  struct drm_framebuffer *fb;
  struct dma_fence *fence;
  int32_t crtc_x;
  int32_t crtc_y;
  uint32_t crtc_w, crtc_h;
  uint32_t src_x;
  uint32_t src_y;
  uint32_t src_h, src_w;
  u16 alpha;
  uint16_t pixel_blend_mode;
  unsigned int rotation;
  unsigned int zpos;
  unsigned int normalized_zpos;
  enum drm_color_encoding color_encoding;
  enum drm_color_range color_range;
  struct drm_rect src, dst;
  bool visible;
  struct drm_crtc_commit *commit;
  struct drm_atomic_state *state;
};

Good:
  struct drm_plane *plane;
  struct dma_fence *fence; // must be copied from master plane_state
  struct drm_rect src, dst; // can fill in what we want here..
  bool visible; // ignored by core mostly, but set and used by driver
  unsigned int zpos;
  unsigned int normalized_zpos;
  struct drm_crtc_commit *commit;
  struct drm_atomic_state *state;


Bad:
  // We don't use the below anyway, we must use the src and dst rects..
  int32_t crtc_x;
  int32_t crtc_y;
  uint32_t crtc_w, crtc_h;
  uint32_t src_x;
  uint32_t src_y;
  uint32_t src_h, src_w;

  // Props we should use from master, I think we can ignore most of this by copying intel_plane_state->plane_ctl / intel_plane_state->plane_color_ctl from the master plane.
  u16 alpha;
  unsigned int rotation;
  uint16_t pixel_blend_mode;
  enum drm_color_encoding color_encoding;
  enum drm_color_range color_range;

  On hdr planes only, we program the CSC, so we need special care for those in that case:
  enum drm_color_encoding color_encoding;
  enum drm_color_range color_range;

Ugly:
  struct drm_crtc *crtc; // might be NULL
  struct drm_framebuffer *fb; // is NULL, or even worse it may point to a different FB, so treat anything touching plane_state->fb with suspicion

So to sum it all up, we need to be very careful in a lot of places to handle deviating values correctly..

Most important ones are crtc_state->active and crtc_state->enable. For planes, plane_state->crtc might be null, and plane_state->fb anything. Planes are easier to solve though, but crtc_state is all over the place. :(



~Maarten