[igt-dev] [PATCH i-g-t 10/11] lib/kms: Convert pipe id flags for a vblank using crtc offset

[Khajapasha, Mohammed]

> -----Original Message-----
> From: Hiler, Arkadiusz <arkadiusz.hiler at intel.com>
> Sent: Wednesday, July 15, 2020 8:20 PM
> To: Khajapasha, Mohammed <mohammed.khajapasha at intel.com>
> Cc: igt-dev at lists.freedesktop.org; Jani Nikula <jani.nikula at linux.intel.com>
> Subject: Re: [PATCH i-g-t 10/11] lib/kms: Convert pipe id flags for a vblank
> using crtc offset
> 
> On Sun, Jul 12, 2020 at 01:36:01AM +0530, Mohammed Khajapasha wrote:
> > In i915 with non-contiguous pipes display, pipes always not same as
> > crtc index in display pipes array. Hence reading pipe id flags for a
> > vblank event using crtc offset.
> >
> > Signed-off-by: Mohammed Khajapasha
> <mohammed.khajapasha at intel.com>
> > ---
> >  lib/igt_kms.c | 51
> > +++++++++++++++++++++++++++++++++++----------------
> >  lib/igt_kms.h | 14 +++++++++++---
> >  2 files changed, 46 insertions(+), 19 deletions(-)
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c index d56f2e56..5b9f2175
> > 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -1983,6 +1983,8 @@ void igt_display_require(igt_display_t *display,
> int drm_fd)
> >  		/* pipe is enabled/disabled */
> >  		pipe->enabled = true;
> >  		pipe->crtc_id = resources->crtcs[i];
> > +		/* offset of a pipe in crtcs list */
> > +		pipe->crtc_offset = i;
> >  	}
> >
> >  	drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
> 1); @@
> > -4131,18 +4133,23 @@ void igt_pipe_request_out_fence(igt_pipe_t
> *pipe)
> >  /**
> >   * igt_wait_for_vblank_count:
> >   * @drm_fd: A drm file descriptor
> > - * @pipe: Pipe to wait_for_vblank on
> > + * @crtc_offset: offset of a Pipe to wait_for_vblank on
> >   * @count: Number of vblanks to wait on
> >   *
> > + * With non-contiguous pipe display, pipe mapping always
> > + * not same with crtc mapping, for i915 pipe is enum id of i915's
> > + crtc object
> > + * and pipe id not equal to crtc offset in mode config list hence
> > + passing
> > + * crtc offset to read vblank count for a pipe.
> > + *
> 
> Hey,
> 
> In KMS & i915 there are two thing that are called pipe:
> 
> 1. offset of the crtc in the mode resources array, it's used by
> drm_wait_vblank_ioctl
> 
> https://elixir.bootlin.com/linux/v5.7.8/source/drivers/gpu/drm/drm_vblank
> .c#L1658

I was grepping the reason for using "pipe" & "pipe_index" terminology for these variable,
And even in code certain bit & left shift operations done using DRM_VBLANK_HIGH_CRTC_SHIFT macro
If it is a pipe then they should have using a macro which contains PIPE, but in this it is CRTC_SHIFT.

> 
> 2. intel concept of a pipe with its separate numbering:
> 
> https://elixir.bootlin.com/linux/v5.7.8/source/drivers/gpu/drm/i915/display
> /intel_display.c#L16722
> 
> Those two concepts are incompatible. We can see this in the kms_lease
> tests - if we lease PIPE_C only it becomes PIPE_A for (1) and stays PIPE_C in
> the sense of (2).
> 
> Why is it so important that in IGT we use the (2) meaning? How are you
> planning on using it? Why the kernel can't hide the missing pipe from our
> sight just as it does when leasing?

Not sure why pipe terminology has been used from initial stage of IGT.
As per my understanding pipe is a H/W specific terminology used to represent a h/w crtc
Still if driver allows it can assign id for h/w crtc using mode config list index.

> 
> I see few options with non-continuous pipes:
> 
> a. keep pipe as it is now (so it actually is "crtc offset") as this is
>    what most of the IOCTLs expect. If you ever need to use the (2)
>    meaning of pipe you can use intel_get_pipe_from_crtc_id_ioctl there
> 
> b. same as above but rename PIPE_ prefix to CRTC_, so we end up with
>    CRTC_A, CRTC_B, CRTC_C, etc.
> 
> c. this patch series - we use the second meaning of pipe (2) and
>    introduce some complications for the sake of places where we need the
>    first meaning (1)
> 
> I think this series lacks justification why this option has been chosen
> - it may be me not understating something that you find obvious.
> 
> Is there any practical reason besides people wanting to have test results for
> pipe-c corresponding to Intel's hardware understanding of what pipe-c is?
> 
> You may need to ELI5 that to me. I have very rudimentary understanding of
> this area and only learned about the multiple meanings of "pipe"
> while doing the review here :-)
> 
> The code looks good otherwise.
> 
> --
> Cheers,
> Arek