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@xxxxxxxxxxxxxxx>,
> >> Matt Roper <matthew.d.roper@xxxxxxxxx>
> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
