Re: [RFC] drm/i915/dp: Preliminary support for 2 pipe 1 port mode for 5K@120

[Manasi Navare <manasi.d.navare@intel.com>]

On Thu, Jan 24, 2019 at 12:23:45PM +0100, Maarten Lankhorst wrote:
> 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@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@linux.intel.com>,
> >> Matt Roper <matthew.d.roper@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Matt Roper <matthew.d.roper@intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Signed-off-by: Manasi Navare <manasi.d.navare@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@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

Okay so these above fields can be part of the drm_crtc_uapi_state as suggested by Ville. So that these exposed to the userspace
do not change irrespective of master-slave behaviour and can be just used to configure master_crtc and slave_crtc Correct?
So these will not be part of drm_master_crtc_state and drm_slave_crtc_state

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

Shouldnt the mode represent the true mode and not the mode/2 going to each of the crtcs?
And then the adjusted mode can reflect  the mode/2?

> 
> 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;

So they can actually be in the drm_crtc_uapi_state and used for both master and slave in i915?

> 
> 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.

So leave these fields as is for both master and slave crtc states i i915, slave state derives from master's.

> 
> 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.
>

Correct, the stealing crtc logic should check for crtc_state->enable. So if the master and slave crtc states are created
right up in DRM, then once slave crtc is stolen its crtc_enable should be set then. 
So again drm_crtc_master_state and drm_crtc_slave_state each will have these fields, correct?

So to summarize your suggestion is that we create a drm_crtc_uapi_state which will be the state common to both
master and slave crtcs. Then have the derived states as drm_crtc_master_state and drm_crtc_slave_state
Also split the drm_plane_master_state and drm_plane_slave_state to map planes across the two crtcs.

Is my understanding correct so far, I analyze your plane_state comments next. First wanted to understand
if my interpretation of the crtc_state is correct

Manasi
 
> 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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx