Re: [igt-dev] [[RFC] i-g-t 1/1] lib/igt_kms: Add support for display with non-contiguous pipes

[Petri Latvala <petri.latvala@intel.com>]

On Wed, Jun 17, 2020 at 02:20:26PM +0300, Khajapasha, Mohammed wrote:
> 
> 
> > -----Original Message-----
> > From: Latvala, Petri <petri.latvala@intel.com>
> > Sent: Tuesday, June 16, 2020 7:14 PM
> > To: Khajapasha, Mohammed <mohammed.khajapasha@intel.com>
> > Cc: igt-dev@lists.freedesktop.org
> > Subject: Re: [igt-dev] [[RFC] i-g-t 1/1] lib/igt_kms: Add support for display
> > with non-contiguous pipes
> >
> > On Thu, Jun 11, 2020 at 01:07:24AM +0530, Mohammed Khajapasha wrote:
> > > Add support for non-contiguous pipe display by allocating upper bound
> > > pipes array for display. Set the pipe enum name to igt pipe for
> > > enabled pipes in drm.
> > >
> > > Signed-off-by: Mohammed Khajapasha
> > <mohammed.khajapasha@intel.com>
> > > ---
> > >  lib/igt_kms.c | 30 +++++++++++++++++++++++++-----  lib/igt_kms.h | 13
> > > ++++++++-----
> > >  2 files changed, 33 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c index afef5939..2b1cf878
> > > 100644
> > > --- a/lib/igt_kms.c
> > > +++ b/lib/igt_kms.c
> > > @@ -1921,10 +1921,26 @@ void igt_display_require(igt_display_t
> > *display, int drm_fd)
> > >   * We cache the number of pipes, that number is a physical limit of
> > the
> > >   * hardware and cannot change of time (for now, at least).
> > >   */
> > > -display->n_pipes = resources->count_crtcs;
> > > -display->pipes = calloc(sizeof(igt_pipe_t), display->n_pipes);
> > > +display->n_pipes = IGT_MAX_PIPES;
> > > +display->pipes = calloc(sizeof(igt_pipe_t), IGT_MAX_PIPES);
> > >  igt_assert_f(display->pipes, "Failed to allocate memory for %d
> > > pipes\n", display->n_pipes);
> > >
> > > +for(i = 0; i < resources->count_crtcs; i++) {
> > > +igt_pipe_t *pipe;
> > > +struct drm_i915_get_pipe_from_crtc_id get_pipe;
> > > +
> > > +/* Get right pipe enum from kernel for a pipe */
> > > +get_pipe.pipe = 0;
> > > +get_pipe.crtc_id =  resources->crtcs[i];
> > > +do_ioctl(display->drm_fd,
> > > +DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID,
> > &get_pipe);
> > > +
> > > +pipe = &display->pipes[get_pipe.pipe];
> > > +pipe->pipe = get_pipe.pipe;
> > > +pipe->enabled = true;
> > > +pipe->crtc_id = resources->crtcs[i];
> > > +}
> > > +
> >
> > You need to eventually make sure to only do this dancing for i915 devices.
> > Might as well get that done early. For non-i915 devices you need to make
> > pipe->pipe equal to i directly, and so on.
> >
> > You also need to store i in the pipe objects. possible_crtcs from a drm
> > plane refers to the value of i, not the pipe enum. That also needs to be
> > handled correctly futher down.
> >
> >
> > >  drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
> > 1);
> > >  if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
> > >  display->is_atomic = 1;
> > > @@ -1959,9 +1975,7 @@ void igt_display_require(igt_display_t *display,
> > int drm_fd)
> > >  int j, type;
> > >  uint8_t last_plane = 0, n_planes = 0;
> > >
> > > -pipe->crtc_id = resources->crtcs[i];
> > >  pipe->display = display;
> > > -pipe->pipe = i;
> > >  pipe->plane_cursor = -1;
> > >  pipe->plane_primary = -1;
> > >  pipe->planes = NULL;
> > > @@ -2409,12 +2423,18 @@ static bool
> > > output_is_internal_panel(igt_output_t *output)
> > >
> > >  igt_output_t **__igt_pipe_populate_outputs(igt_display_t *display,
> > > igt_output_t **chosen_outputs)  {
> > > -unsigned full_pipe_mask = (1 << (display->n_pipes)) - 1,
> > assigned_pipes = 0;
> > > +unsigned full_pipe_mask, assigned_pipes = 0;
> > >  igt_output_t *output;
> > >  int i, j;
> > >
> > >  memset(chosen_outputs, 0, sizeof(*chosen_outputs) *
> > > display->n_pipes);
> > >
> > > +for( i = 0; i < display->n_pipes; i++) {
> > > +igt_pipe_t *pipe = &display->pipes[i];
> > > +if (pipe->enabled)
> > > +full_pipe_mask |= (1 << pipe->pipe);
> >
> > full_pipe_mask operates on config->valid_crtc_idx_mask later in this
> > function, so it's one of those that needs to operate on the index (i), not the
> > pipe->pipe value.
> 
> With PIPE_A & PIPE_C configuration, if we use index(i) rather than pipe->pipe the full_pipe_mask will be 0x11
> But from kernel the encoder->possible_crtcs can be set from individual encoder's pipe_mask
> Snippet
> /*
> for_each_intel_crtc(dev, crtc)
> {
> if (encoder->pipe_mask &
> BIT(crtc->pipe)) possible_crtcs |= drm_crtc_mask(&crtc->base);
> }
> 
> and pipe_mask for invidual encoder can be set by
> 
> intel_encoder->pipe_mask = BIT(PIPE_A) | BIT(PIPE_C) --> which is 0x0101
> 
> */
> 
> So with PIPE_A & PIPE_C, the output connected to PIPE_C can be miss while populating outputs for pipes.

Yeah but possible_crtcs is set with drm_crtc_mask, not with pipe_mask.

static inline uint32_t drm_crtc_mask(const struct drm_crtc *crtc)
{
        return 1 << drm_crtc_index(crtc);
}



-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev