Re: [PATCH v2 5/8] drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation

[Leandro Ribeiro]

Hi,

On 11/3/21 12:03, Igor Torrente wrote:
> Hi Leandro,
> 
> On 10/28/21 6:38 PM, Leandro Ribeiro wrote:
>> Hi,
>>
>> On 10/26/21 08:34, Igor Torrente wrote:
>>> Add a helper function to validate the connector configuration receive in
>>> the encoder atomic_check by the drivers.
>>>
>>> So the drivers don't need do these common validations themselves.
>>>
>>> Signed-off-by: Igor Torrente 
>>> ---
>>> V2: Move the format verification to a new helper at the
>>> drm_atomic_helper.c
>>>  (Thomas Zimmermann).
>>> ---
>>>   drivers/gpu/drm/drm_atomic_helper.c   | 47 +++
>>>   drivers/gpu/drm/vkms/vkms_writeback.c |  9 +++--
>>>   include/drm/drm_atomic_helper.h   |  3 ++
>>>   3 files changed, 54 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>> index 2c0c6ec92820..c2653b9824b5 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct
>>> drm_device *dev,
>>>   }
>>>   EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>>>   +/**
>>> + * drm_atomic_helper_check_wb_connector_state() - Check writeback
>>> encoder state
>>> + * @encoder: encoder state to check
>>> + * @conn_state: connector state to check
>>> + *
>>> + * Checks if the wriback connector state is valid, and returns a
>>> erros if it
>>> + * isn't.
>>> + *
>>> + * RETURNS:
>>> + * Zero for success or -errno
>>> + */
>>> +int
>>> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
>>> + struct drm_connector_state *conn_state)
>>> +{
>>> +    struct drm_writeback_job *wb_job = conn_state->writeback_job;
>>> +    struct drm_property_blob *pixel_format_blob;
>>> +    bool format_supported = false;
>>> +    struct drm_framebuffer *fb;
>>> +    int i, n_formats;
>>> +    u32 *formats;
>>> +
>>> +    if (!wb_job || !wb_job->fb)
>>> +    return 0;
>>
>> I think that this should be removed and that this functions should
>> assume that (wb_job && wb_job->fb) == true.
> 
> Ok.
> 
>>
>> Actually, it's weird to have conn_state as argument and only use it to
>> get the wb_job. Instead, this function could receive wb_job directly.
> 
> In the Thomas review of v1, he said that maybe other things could be
> tested in this helper. I'm not sure what these additional checks could
> be, so I tried to design the function signature expecting more things
> to be added after his review.
> 
> As you can see, the helper is receiving the `drm_encoder` and doing
> nothing with it.
> 
> If we, eventually, don't find anything else that this helper can do, I
> will revert to something very similar (if not equal) to your proposal.
> I just want to wait for Thomas's review first.
>

Sure, that makes sense.

Thanks,
Leandro Ribeiro

>>
>> Of course, its name/description would have to change.
>>
>>> +
>>> +    pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
>>> +    n_formats = pixel_format_blob->length / sizeof(u32);
>>> +    formats = pixel_format_blob->data;
>>> +    fb = wb_job->fb;
>>> +
>>> +    for (i = 0; i < n_formats; i++) {
>>> +    if (fb->format->format == formats[i]) {
>>> +    format_supported = true;
>>> +    break;
>>> +    }
>>> +    }
>>> +
>>> +    if (!format_supported) {
>>> +    DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
>>> +  >format->format);
>>> +    return -EINVAL;
>>> +    }
>>> +
>>> +    return 0;
>>
>> If you do this, you can get rid of the format_supported flag:
>>
>> for(...) {
>>     if (fb->format->format == formats[i])
>>     return 0;
>> }
>>
>>
>> DRM_DEBUG_KMS(...);
>> return -EINVAL;
>>
> 
> Indeed. Thanks!
> 
>> Thanks,
>> Leandro Ribeiro
>>
>>> +}
>>> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
>>> +
>>>   /**
>>>    * drm_atomic_helper_check_plane_state() - Check plane state for
>>> validity
>>>    * 

Re: [PATCH v2 5/8] drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation

[Leandro Ribeiro]

Hi,

On 10/26/21 08:34, Igor Torrente wrote:
> Add a helper function to validate the connector configuration receive in
> the encoder atomic_check by the drivers.
> 
> So the drivers don't need do these common validations themselves.
> 
> Signed-off-by: Igor Torrente 
> ---
> V2: Move the format verification to a new helper at the drm_atomic_helper.c
> (Thomas Zimmermann).
> ---
>  drivers/gpu/drm/drm_atomic_helper.c   | 47 +++
>  drivers/gpu/drm/vkms/vkms_writeback.c |  9 +++--
>  include/drm/drm_atomic_helper.h   |  3 ++
>  3 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 2c0c6ec92820..c2653b9824b5 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>  
> +/**
> + * drm_atomic_helper_check_wb_connector_state() - Check writeback encoder 
> state
> + * @encoder: encoder state to check
> + * @conn_state: connector state to check
> + *
> + * Checks if the wriback connector state is valid, and returns a erros if it
> + * isn't.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int
> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
> +  struct drm_connector_state *conn_state)
> +{
> + struct drm_writeback_job *wb_job = conn_state->writeback_job;
> + struct drm_property_blob *pixel_format_blob;
> + bool format_supported = false;
> + struct drm_framebuffer *fb;
> + int i, n_formats;
> + u32 *formats;
> +
> + if (!wb_job || !wb_job->fb)
> + return 0;

I think that this should be removed and that this functions should
assume that (wb_job && wb_job->fb) == true.

Actually, it's weird to have conn_state as argument and only use it to
get the wb_job. Instead, this function could receive wb_job directly.

Of course, its name/description would have to change.

> +
> + pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
> + n_formats = pixel_format_blob->length / sizeof(u32);
> + formats = pixel_format_blob->data;
> + fb = wb_job->fb;
> +
> + for (i = 0; i < n_formats; i++) {
> + if (fb->format->format == formats[i]) {
> + format_supported = true;
> + break;
> + }
> + }
> +
> + if (!format_supported) {
> + DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
> +   >format->format);
> + return -EINVAL;
> + }
> +
> +     return 0;

If you do this, you can get rid of the format_supported flag:

for(...) {
if (fb->format->format == formats[i])
return 0;
}


DRM_DEBUG_KMS(...);
return -EINVAL;

Thanks,
Leandro Ribeiro

> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
> +
>  /**
>   * drm_atomic_helper_check_plane_state() - Check plane state for validity
>   * @plane_state: plane state to check
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c 
> b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 32734cdbf6c2..42f3396c523a 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder 
> *encoder,
>  {
>   struct drm_framebuffer *fb;
>   const struct drm_display_mode *mode = _state->mode;
> + int ret;
>  
>   if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
>   return 0;
> @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder 
> *encoder,
>   return -EINVAL;
>   }
>  
> - if (fb->format->format != vkms_wb_formats[0]) {
> - DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
> -   >format->format);
> - return -EINVAL;
> - }
> + ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state);
> + if (ret < 0)
> + return ret;
>  
>   return 0;
>  }
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 4045e2507e11..3fbf695da60f 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -40,6 +40,9 @@ struct drm_private_state;
>  
>  int drm_atomic_helper_check_modeset(struct drm_device *dev,
>   struct drm_atomic_state *state);
> +int
> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
> +  struct drm_connector_state 
> *conn_state);
>  int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
>   const struct drm_crtc_state *crtc_state,
>   int min_scale,
> 

[PATCH v6 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

Add a small description and document struct fields of
drm_mode_get_plane.

Signed-off-by: Leandro Ribeiro 
---
 include/uapi/drm/drm_mode.h | 32 
 1 file changed, 32 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 9b6722d45f36..98bf130feda5 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -312,16 +312,48 @@ struct drm_mode_set_plane {
__u32 src_w;
 };

+/**
+ * struct drm_mode_get_plane - Get plane metadata.
+ *
+ * Userspace can perform a GETPLANE ioctl to retrieve information about a
+ * plane.
+ *
+ * To retrieve the number of formats supported, set @count_format_types to zero
+ * and call the ioctl. @count_format_types will be updated with the value.
+ *
+ * To retrieve these formats, allocate an array with the memory needed to store
+ * @count_format_types formats. Point @format_type_ptr to this array and call
+ * the ioctl again (with @count_format_types still set to the value returned in
+ * the first ioctl call).
+ */
 struct drm_mode_get_plane {
+   /**
+* @plane_id: Object ID of the plane whose information should be
+* retrieved. Set by caller.
+*/
__u32 plane_id;

+   /** @crtc_id: Object ID of the current CRTC. */
__u32 crtc_id;
+   /** @fb_id: Object ID of the current fb. */
__u32 fb_id;

+   /**
+* @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
+* are created and they receive an index, which corresponds to their
+* position in the bitmask. Bit N corresponds to
+* :ref:`CRTC index` N.
+*/
__u32 possible_crtcs;
+   /** @gamma_size: Never used. */
__u32 gamma_size;

+   /** @count_format_types: Number of formats. */
__u32 count_format_types;
+   /**
+* @format_type_ptr: Pointer to ``__u32`` array of formats that are
+* supported by the plane. These formats do not require modifiers.
+*/
__u64 format_type_ptr;
 };

--
2.31.1

[PATCH v6 0/1] Document drm_mode_get_plane

[Leandro Ribeiro]

v2: possible_crtcs field is a bitmask, not a pointer. Suggested by
Ville Syrjälä 

v3: document how userspace should find out CRTC index. Also,
document that field 'gamma_size' represents the number of
entries in the lookup table. Suggested by Pekka Paalanen
 and Daniel Vetter 

v4: document IN and OUT fields and make the description more
concise. Suggested by Pekka Paalanen 

v5: CRTC index patch already merged, only patch to document drm_mode_get_plane
now. Added that gamma LUT size is deprecated and dropped incorrect text
documenting that plane number of formats may change from one ioctl to the
other. Suggested by Ville Syrjälä 

v6: document that gamma_size field was never used. Suggested by Pekka Paalanen
 and Daniel Vetter 

Leandro Ribeiro (1):
  drm/doc: document drm_mode_get_plane

 include/uapi/drm/drm_mode.h | 32 
 1 file changed, 32 insertions(+)

--
2.31.1

Re: [PATCH v5 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 6/11/21 4:33 AM, Daniel Vetter wrote:
> On Fri, Jun 11, 2021 at 9:20 AM Pekka Paalanen  wrote:
>>
>> On Thu, 10 Jun 2021 17:38:24 -0300
>> Leandro Ribeiro  wrote:
>>
>>> Add a small description and document struct fields of
>>> drm_mode_get_plane.
>>>
>>> Signed-off-by: Leandro Ribeiro 
>>> ---
>>>  include/uapi/drm/drm_mode.h | 35 +++
>>>  1 file changed, 35 insertions(+)
>>>
>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>> index 9b6722d45f36..698559d9336b 100644
>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -312,16 +312,51 @@ struct drm_mode_set_plane {
>>>   __u32 src_w;
>>>  };
>>>
>>> +/**
>>> + * struct drm_mode_get_plane - Get plane metadata.
>>> + *
>>> + * Userspace can perform a GETPLANE ioctl to retrieve information about a
>>> + * plane.
>>> + *
>>> + * To retrieve the number of formats supported, set @count_format_types to 
>>> zero
>>> + * and call the ioctl. @count_format_types will be updated with the value.
>>> + *
>>> + * To retrieve these formats, allocate an array with the memory needed to 
>>> store
>>> + * @count_format_types formats. Point @format_type_ptr to this array and 
>>> call
>>> + * the ioctl again (with @count_format_types still set to the value 
>>> returned in
>>> + * the first ioctl call).
>>> + */
>>>  struct drm_mode_get_plane {
>>> + /**
>>> +  * @plane_id: Object ID of the plane whose information should be
>>> +  * retrieved. Set by caller.
>>> +  */
>>>   __u32 plane_id;
>>>
>>> + /** @crtc_id: Object ID of the current CRTC. */
>>>   __u32 crtc_id;
>>> + /** @fb_id: Object ID of the current fb. */
>>>   __u32 fb_id;
>>>
>>> + /**
>>> +  * @possible_crtcs: Bitmask of CRTC's compatible with the plane. 
>>> CRTC's
>>> +  * are created and they receive an index, which corresponds to their
>>> +  * position in the bitmask. Bit N corresponds to
>>> +  * :ref:`CRTC index` N.
>>> +  */
>>>   __u32 possible_crtcs;
>>> + /**
>>> +  * @gamma_size: Number of entries of the legacy gamma lookup table.
>>> +  * Deprecated.
>>> +  */
>>>   __u32 gamma_size;
>>
>> Hi,
>>
>> I wonder, has this field ever been used?
>>
>> "The legacy gamma" refers to CRTC gamma LUT AFAIK, but this here is
>> about planes. I forgot that at first, so didn't see anything funny.
> 
> Yeah "Deprecated" isn't really conveying that this was never used or
> implemented anywehere ever. I think we should put that into the docs
> to make this clear, otherwise someone is going to wonder whether maybe
> they still need to parse it since it's only deprecated and there's no
> other plane gamma (yet). I wouldn't even put any further  docs than
> that in it, because stating that it's the number of entries for
> something we never implemented is going to be confusing at best :-)
> -Daniel
> 

Nice, thanks for elaborating.

I'll update to "@gamma_size: Never used".

>>
>> Anyway, whether the doc for this field is as is, or is changed to
>> "never used" or "unused" or "reserved" or whatever, you have my:
>>
>> Reviewed-by: Pekka Paalanen 
>>
>> With the caveat that I didn't actually build the docs to see how they
>> look.
>>
>>
>> Thanks,
>> pq
>>
>>>
>>> + /** @count_format_types: Number of formats. */
>>>   __u32 count_format_types;
>>> + /**
>>> +  * @format_type_ptr: Pointer to ``__u32`` array of formats that are
>>> +  * supported by the plane. These formats do not require modifiers.
>>> +  */
>>>   __u64 format_type_ptr;
>>>  };
>>>
>>> --
>>> 2.31.1
>>>
>>
> 
> 

[PATCH v5 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

Add a small description and document struct fields of
drm_mode_get_plane.

Signed-off-by: Leandro Ribeiro 
---
 include/uapi/drm/drm_mode.h | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 9b6722d45f36..698559d9336b 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -312,16 +312,51 @@ struct drm_mode_set_plane {
__u32 src_w;
 };

+/**
+ * struct drm_mode_get_plane - Get plane metadata.
+ *
+ * Userspace can perform a GETPLANE ioctl to retrieve information about a
+ * plane.
+ *
+ * To retrieve the number of formats supported, set @count_format_types to zero
+ * and call the ioctl. @count_format_types will be updated with the value.
+ *
+ * To retrieve these formats, allocate an array with the memory needed to store
+ * @count_format_types formats. Point @format_type_ptr to this array and call
+ * the ioctl again (with @count_format_types still set to the value returned in
+ * the first ioctl call).
+ */
 struct drm_mode_get_plane {
+   /**
+* @plane_id: Object ID of the plane whose information should be
+* retrieved. Set by caller.
+*/
__u32 plane_id;

+   /** @crtc_id: Object ID of the current CRTC. */
__u32 crtc_id;
+   /** @fb_id: Object ID of the current fb. */
__u32 fb_id;

+   /**
+* @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
+* are created and they receive an index, which corresponds to their
+* position in the bitmask. Bit N corresponds to
+* :ref:`CRTC index` N.
+*/
__u32 possible_crtcs;
+   /**
+* @gamma_size: Number of entries of the legacy gamma lookup table.
+* Deprecated.
+*/
__u32 gamma_size;

+   /** @count_format_types: Number of formats. */
__u32 count_format_types;
+   /**
+* @format_type_ptr: Pointer to ``__u32`` array of formats that are
+* supported by the plane. These formats do not require modifiers.
+*/
__u64 format_type_ptr;
 };

--
2.31.1

[PATCH v5 0/1] Document drm_mode_get_plane

[Leandro Ribeiro]

v2: possible_crtcs field is a bitmask, not a pointer. Suggested by
Ville Syrjälä 

v3: document how userspace should find out CRTC index. Also,
document that field 'gamma_size' represents the number of
entries in the lookup table. Suggested by Pekka Paalanen
 and Daniel Vetter 

v4: document IN and OUT fields and make the description more
concise. Suggested by Pekka Paalanen 

v5: CRTC index patch already merged, only patch to document drm_mode_get_plane
now. Added that gamma LUT size is deprecated and dropped incorrect text
documenting that plane number of formats may change from one ioctl to the
other. Suggested by Ville Syrjälä 

Leandro Ribeiro (1):
  drm/doc: document drm_mode_get_plane

 include/uapi/drm/drm_mode.h | 35 +++
 1 file changed, 35 insertions(+)

--
2.31.1

Re: [PATCH v4 2/2] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 6/9/21 8:00 PM, Leandro Ribeiro wrote:
> Add a small description and document struct fields of
> drm_mode_get_plane.
> 
> Signed-off-by: Leandro Ribeiro 
> ---
>  include/uapi/drm/drm_mode.h | 36 
>  1 file changed, 36 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index a5e76aa06ad5..67bcd8e1931c 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -312,16 +312,52 @@ struct drm_mode_set_plane {
>   __u32 src_w;
>  };
> 
> +/**
> + * struct drm_mode_get_plane - Get plane metadata.
> + *
> + * Userspace can perform a GETPLANE ioctl to retrieve information about a
> + * plane.
> + *
> + * To retrieve the number of formats supported, set @count_format_types to 
> zero
> + * and call the ioctl. @count_format_types will be updated with the value.
> + *
> + * To retrieve these formats, allocate an array with the memory needed to 
> store
> + * @count_format_types formats. Point @format_type_ptr to this array and call
> + * the ioctl again (with @count_format_types still set to the value returned 
> in
> + * the first ioctl call).
> + *
> + * Between one ioctl and the other, the number of formats may change.
> + * Userspace should retry the last ioctl until this number stabilizes. The
> + * kernel won't fill any array which doesn't have the expected length.
> + */

Actually I don't know if this last paragraph applies. For connectors,
for instance, I can see this happening because of hot-plugging. But for
plane formats I have no idea. As in libdrm we have this algorithm, I've
decided to describe it here.

>  struct drm_mode_get_plane {
> + /**
> +  * @plane_id: Object ID of the plane whose information should be
> +  * retrieved. Set by caller.
> +  */
>   __u32 plane_id;
> 
> + /** @crtc_id: Object ID of the current CRTC. */
>   __u32 crtc_id;
> + /** @fb_id: Object ID of the current fb. */
>   __u32 fb_id;
> 
> + /**
> +  * @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
> +  * are created and they receive an index, which corresponds to their
> +  * position in the bitmask. Bit N corresponds to
> +  * :ref:`CRTC index` N.
> +  */
>   __u32 possible_crtcs;
> + /** @gamma_size: Number of entries of the legacy gamma lookup table. */
>   __u32 gamma_size;
> 
> + /** @count_format_types: Number of formats. */
>   __u32 count_format_types;
> + /**
> +  * @format_type_ptr: Pointer to ``__u32`` array of formats that are
> +  * supported by the plane. These formats do not require modifiers.
> +  */
>   __u64 format_type_ptr;
>  };
> 
> --
> 2.31.1
> 
> 

[PATCH v4 2/2] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

Add a small description and document struct fields of
drm_mode_get_plane.

Signed-off-by: Leandro Ribeiro 
---
 include/uapi/drm/drm_mode.h | 36 
 1 file changed, 36 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a5e76aa06ad5..67bcd8e1931c 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -312,16 +312,52 @@ struct drm_mode_set_plane {
__u32 src_w;
 };

+/**
+ * struct drm_mode_get_plane - Get plane metadata.
+ *
+ * Userspace can perform a GETPLANE ioctl to retrieve information about a
+ * plane.
+ *
+ * To retrieve the number of formats supported, set @count_format_types to zero
+ * and call the ioctl. @count_format_types will be updated with the value.
+ *
+ * To retrieve these formats, allocate an array with the memory needed to store
+ * @count_format_types formats. Point @format_type_ptr to this array and call
+ * the ioctl again (with @count_format_types still set to the value returned in
+ * the first ioctl call).
+ *
+ * Between one ioctl and the other, the number of formats may change.
+ * Userspace should retry the last ioctl until this number stabilizes. The
+ * kernel won't fill any array which doesn't have the expected length.
+ */
 struct drm_mode_get_plane {
+   /**
+* @plane_id: Object ID of the plane whose information should be
+* retrieved. Set by caller.
+*/
__u32 plane_id;

+   /** @crtc_id: Object ID of the current CRTC. */
__u32 crtc_id;
+   /** @fb_id: Object ID of the current fb. */
__u32 fb_id;

+   /**
+* @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
+* are created and they receive an index, which corresponds to their
+* position in the bitmask. Bit N corresponds to
+* :ref:`CRTC index` N.
+*/
__u32 possible_crtcs;
+   /** @gamma_size: Number of entries of the legacy gamma lookup table. */
__u32 gamma_size;

+   /** @count_format_types: Number of formats. */
__u32 count_format_types;
+   /**
+* @format_type_ptr: Pointer to ``__u32`` array of formats that are
+* supported by the plane. These formats do not require modifiers.
+*/
__u64 format_type_ptr;
 };

--
2.31.1

[PATCH v4 1/2] drm/doc: document how userspace should find out CRTC index

[Leandro Ribeiro]

In this patch we add a section to document what userspace should do to
find out the CRTC index. This is important as they may be many places in
the documentation that need this, so it's better to just point to this
section and avoid repetition.

Signed-off-by: Leandro Ribeiro 
---
 Documentation/gpu/drm-uapi.rst| 13 +
 drivers/gpu/drm/drm_debugfs_crc.c |  8 
 include/uapi/drm/drm.h|  4 ++--
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 04bdc7a91d53..7e51dd40bf6e 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -457,6 +457,19 @@ Userspace API Structures
 .. kernel-doc:: include/uapi/drm/drm_mode.h
:doc: overview

+.. _crtc_index:
+
+CRTC index
+--
+
+CRTC's have both an object ID and an index, and they are not the same thing.
+The index is used in cases where a densely packed identifier for a CRTC is
+needed, for instance a bitmask of CRTC's. The member possible_crtcs of struct
+drm_mode_get_plane is an example.
+
+DRM_IOCTL_MODE_GETRESOURCES populates a structure with an array of CRTC ID's,
+and the CRTC index is its position in this array.
+
 .. kernel-doc:: include/uapi/drm/drm.h
:internal:

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c 
b/drivers/gpu/drm/drm_debugfs_crc.c
index 3dd70d813f69..bbc3bc4ba844 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -46,10 +46,10 @@
  * it reached a given hardware component (a CRC sampling "source").
  *
  * Userspace can control generation of CRCs in a given CRTC by writing to the
- * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the 
CRTC.
- * Accepted values are source names (which are driver-specific) and the "auto"
- * keyword, which will let the driver select a default source of frame CRCs
- * for this CRTC.
+ * file dri/0/crtc-N/crc/control in debugfs, with N being the :ref:`index of
+ * the CRTC`. Accepted values are source names (which are
+ * driver-specific) and the "auto" keyword, which will let the driver select a
+ * default source of frame CRCs for this CRTC.
  *
  * Once frame CRC generation is enabled, userspace can capture them by reading
  * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 67b94bc3c885..bbf4e76daa55 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -635,8 +635,8 @@ struct drm_gem_open {
 /**
  * DRM_CAP_VBLANK_HIGH_CRTC
  *
- * If set to 1, the kernel supports specifying a CRTC index in the high bits of
- * _wait_vblank_request.type.
+ * If set to 1, the kernel supports specifying a :ref:`CRTC index`
+ * in the high bits of _wait_vblank_request.type.
  *
  * Starting kernel version 2.6.39, this capability is always set to 1.
  */
--
2.31.1

[PATCH v4 0/2] Document drm_mode_get_plane

[Leandro Ribeiro]

v2: possible_crtcs field is a bitmask, not a pointer. Suggested by
Ville Syrjälä 

v3: document how userspace should find out CRTC index. Also,
document that field 'gamma_size' represents the number of
entries in the lookup table. Suggested by Pekka Paalanen
 and Daniel Vetter 

v4: document IN and OUT fields and make the description more
concise. Suggested by Pekka Paalanen 

Leandro Ribeiro (2):
  drm/doc: document how userspace should find out CRTC index
  drm/doc: document drm_mode_get_plane

 Documentation/gpu/drm-uapi.rst| 13 +++
 drivers/gpu/drm/drm_debugfs_crc.c |  8 +++
 include/uapi/drm/drm.h|  4 ++--
 include/uapi/drm/drm_mode.h   | 36 +++
 4 files changed, 55 insertions(+), 6 deletions(-)

--
2.31.1

Re: [PATCH 2/2] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 5/6/21 6:10 AM, Pekka Paalanen wrote:
> On Wed, 28 Apr 2021 18:36:51 -0300
> Leandro Ribeiro  wrote:
> 
>> Add a small description and document struct fields of
>> drm_mode_get_plane.
>>
>> Signed-off-by: Leandro Ribeiro 
> 
> Hi,
> 
> thanks a lot for revising these.
> 
>> ---
>>  include/uapi/drm/drm_mode.h | 22 ++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index a5e76aa06ad5..8fa6495cd948 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -312,16 +312,38 @@ struct drm_mode_set_plane {
>>  __u32 src_w;
>>  };
>>
>> +/**
>> + * struct drm_mode_get_plane - Get plane metadata.
>> + *
>> + * Userspace can perform a GETPLANE ioctl to retrieve information about a
>> + * plane.
>> + */
>>  struct drm_mode_get_plane {
>> +/** @plane_id: Object ID of the plane. */
> 
> This is an "in" field, right?
> 
> "in" meaning that userspace sets it to the ID of the plane it wants
> information on.
> 
> "out" field is a field written by the kernel as a response.
> 
> I'm not sure if the kernel has a habit of documenting these, because we
> use libdrm to abstract this so users do not need to care, but I think
> it would be nice.
> 

In a quick look, I couldn't find anything. But I can change the phrasing
to the following:

"@plane_id: Object ID of the plane whose information should be
retrieved. IN field, set by userspace."

>>  __u32 plane_id;
>>
>> +/** @crtc_id: Object ID of the current CRTC. */
>>  __u32 crtc_id;
>> +/** @fb_id: Object ID of the current fb. */
>>  __u32 fb_id;
>>
>> +/**
>> + * @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
>> + * are created and they receive an index, which corresponds to their
>> + * position in the bitmask. CRTC with index 0 will be in bit 0, and so
>> + * on. To learn how to find out the index of a certain CRTC, please see
>> + * :ref:`crtc_index`.
> 
> This could be shortened to something like bit N corresponds to CRTC
> index N, and make "CRTC index N" a hyperlink.
> 

Nice, I'll apply this change.

>> + */
>>  __u32 possible_crtcs;
>> +/** @gamma_size: Number of entries of the legacy gamma lookup table. */
>>  __u32 gamma_size;
>>
>> +/** @count_format_types: Number of formats. */
>>  __u32 count_format_types;
>> +/**
>> + * @format_type_ptr: Pointer to ``__u32`` array of formats that are
>> + * supported by the plane. These formats do not require modifiers.
>> + */
>>  __u64 format_type_ptr;
> 
> The count/ptr fields have an interesting usage pattern, which I suppose
> is common for all DRM ioctls. Makes me wonder if it should be documented.
> 
> AFAIU, count is in+out field: when set to 0, the kernel uses it to
> return the count needed. Then userspace allocates space and calls the
> ioctl again with the right count and ptr set to point to the allocated
> array of count elements. This is so that kernel never allocates memory
> on behalf of userspace for the return data, making things much simpler
> at the cost of maybe needing to call the ioctl twice to first figure
> out long the array should be.
> 
> This can be seen in libdrm code for drmModeGetPlane().
>
> There is certainly no point in explaining all that here, that is too
> much. But if there was a way to annotate the count member as in+out,
> that would be nice. And the ptr member as "in".
> 

This is documented in the description of struct drm_mode_get_connector:

https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#c.drm_mode_get_connector

Would be enough to have something similar in struct drm_mode_get_plane?

> 
> Thanks,
> pq
> 
>>  };
>>
>> --
>> 2.31.1
>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 1/2] drm/doc: document how userspace should find out CRTC index

[Leandro Ribeiro]

On 5/12/21 10:04 AM, Pekka Paalanen wrote:
> On Wed, 12 May 2021 09:50:14 -0300
> Leandro Ribeiro  wrote:
> 
>> On 5/6/21 5:50 AM, Pekka Paalanen wrote:
>>> On Wed, 28 Apr 2021 18:36:50 -0300
>>> Leandro Ribeiro  wrote:
>>>   
>>>> In this patch we add a section to document what userspace should do to
>>>> find out the CRTC index. This is important as there are multiple places
>>>> in the documentation that need this, so it's better to just point to
>>>> this section and avoid repetition.
>>>>
>>>> Signed-off-by: Leandro Ribeiro 
>>>> ---
>>>>  Documentation/gpu/drm-uapi.rst| 14 ++
>>>>  drivers/gpu/drm/drm_debugfs_crc.c |  9 +
>>>>  include/uapi/drm/drm.h|  3 ++-
>>>>  3 files changed, 21 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/Documentation/gpu/drm-uapi.rst 
>>>> b/Documentation/gpu/drm-uapi.rst
>>>> index 04bdc7a91d53..1aa52a6ac567 100644
>>>> --- a/Documentation/gpu/drm-uapi.rst
>>>> +++ b/Documentation/gpu/drm-uapi.rst
>>>> @@ -457,6 +457,20 @@ Userspace API Structures
>>>>  .. kernel-doc:: include/uapi/drm/drm_mode.h
>>>> :doc: overview
>>>>
>>>> +.. _crtc_index:
>>>> +
>>>> +CRTC index
>>>> +--
>>>> +
>>>> +In some situations, it is important for userspace to find out the index 
>>>> of a  
>>>
>>> That could be said about everything, so this sentence has no
>>> information value. Instead, you could start by stating that CRTCs have
>>> both an object ID and an index, and they are not the same thing. CRTC
>>> index is used in cases where a densely packed identifier for a CRTC is
>>> needed, e.g. in bit-for-crtc masks, where using the object ID would not
>>> work.
>>>  
>>>> +CRTC. The CRTC index should not be confused with its object id.
>>>> +
>>>> +In order to do this, userspace should first query the resources object  
>>>
>>> Instead of saying what userspace must do, you could just explain where
>>> it can be observed.
>>>   
>>>> +from the device that owns the CRTC (using the DRM_IOCTL_MODE_GETRESOURCES 
>>>>  
>>>
>>> So here you might start with: DRM_IOCTL_MODE_GETRESOURCES populates a
>>> structure with an array of CRTC IDs. CRTC's index is its index in that
>>> array.
>>>   
>>>> +ioctl). The resources object contains a pointer to an array of CRTC's, 
>>>> and also
>>>> +the number of entries of the array. The index of the CRTC is the same as 
>>>> its
>>>> +position in this array.  
>>>
>>> Anyway, the idea here is right.
>>>   
>>
>> So what about:
>>
>> CRTC's have both an object ID and an index, and they should not be
>> confused. The index is used in cases where a densely packed identifier
>> for a CRTC is needed, for instance in a bitmask of CRTC's. (maybe a link
>> to the possible_crtcs field of struct drm_mode_get_plane? as example)
>>
>> DRM_IOCTL_MODE_GETRESOURCES populates a structure with an array of CRTC
>> id's, and the CRTC index is its position in this array.
> 
> Sure, sounds good.
> 
> Capitalized 'ID'?
> 

Sure, I'll add this change.

Thanks,
Leandro

Re: [PATCH 1/2] drm/doc: document how userspace should find out CRTC index

[Leandro Ribeiro]

On 5/6/21 5:50 AM, Pekka Paalanen wrote:
> On Wed, 28 Apr 2021 18:36:50 -0300
> Leandro Ribeiro  wrote:
> 
>> In this patch we add a section to document what userspace should do to
>> find out the CRTC index. This is important as there are multiple places
>> in the documentation that need this, so it's better to just point to
>> this section and avoid repetition.
>>
>> Signed-off-by: Leandro Ribeiro 
>> ---
>>  Documentation/gpu/drm-uapi.rst| 14 ++
>>  drivers/gpu/drm/drm_debugfs_crc.c |  9 +
>>  include/uapi/drm/drm.h|  3 ++-
>>  3 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
>> index 04bdc7a91d53..1aa52a6ac567 100644
>> --- a/Documentation/gpu/drm-uapi.rst
>> +++ b/Documentation/gpu/drm-uapi.rst
>> @@ -457,6 +457,20 @@ Userspace API Structures
>>  .. kernel-doc:: include/uapi/drm/drm_mode.h
>> :doc: overview
>>
>> +.. _crtc_index:
>> +
>> +CRTC index
>> +--
>> +
>> +In some situations, it is important for userspace to find out the index of a
> 
> That could be said about everything, so this sentence has no
> information value. Instead, you could start by stating that CRTCs have
> both an object ID and an index, and they are not the same thing. CRTC
> index is used in cases where a densely packed identifier for a CRTC is
> needed, e.g. in bit-for-crtc masks, where using the object ID would not
> work.
>
>> +CRTC. The CRTC index should not be confused with its object id.
>> +
>> +In order to do this, userspace should first query the resources object
> 
> Instead of saying what userspace must do, you could just explain where
> it can be observed.
> 
>> +from the device that owns the CRTC (using the DRM_IOCTL_MODE_GETRESOURCES
> 
> So here you might start with: DRM_IOCTL_MODE_GETRESOURCES populates a
> structure with an array of CRTC IDs. CRTC's index is its index in that
> array.
> 
>> +ioctl). The resources object contains a pointer to an array of CRTC's, and 
>> also
>> +the number of entries of the array. The index of the CRTC is the same as its
>> +position in this array.
> 
> Anyway, the idea here is right.
> 

So what about:

CRTC's have both an object ID and an index, and they should not be
confused. The index is used in cases where a densely packed identifier
for a CRTC is needed, for instance in a bitmask of CRTC's. (maybe a link
to the possible_crtcs field of struct drm_mode_get_plane? as example)

DRM_IOCTL_MODE_GETRESOURCES populates a structure with an array of CRTC
id's, and the CRTC index is its position in this array.

>> +
>>  .. kernel-doc:: include/uapi/drm/drm.h
>> :internal:
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c 
>> b/drivers/gpu/drm/drm_debugfs_crc.c
>> index 3dd70d813f69..9575188d97ee 100644
>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>> @@ -46,10 +46,11 @@
>>   * it reached a given hardware component (a CRC sampling "source").
>>   *
>>   * Userspace can control generation of CRCs in a given CRTC by writing to 
>> the
>> - * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the 
>> CRTC.
>> - * Accepted values are source names (which are driver-specific) and the 
>> "auto"
>> - * keyword, which will let the driver select a default source of frame CRCs
>> - * for this CRTC.
>> + * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the
>> + * CRTC. To learn how to find out the index of a certain CRTC, please see
>> + * :ref:`crtc_index`. Accepted values are source names (which are
> 
> This a bit verbose: "To learn..." It could be more concise, like
> making the words "the index" a hyperlink instead of adding a whole
> sentence.
> 

Nice, I'll apply this change.

>> + * driver-specific) and the "auto" keyword, which will let the driver 
>> select a
>> + * default source of frame CRCs for this CRTC.
>>   *
>>   * Once frame CRC generation is enabled, userspace can capture them by 
>> reading
>>   * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 67b94bc3c885..6944f08ab1a6 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -636,7 +636,8 @@ struct drm_gem_open {
>>   * DRM_CAP_VBLANK_HIGH_CRTC
>>   *
>>   * If set to 1, the kernel supports specifying a CRTC index in the high

Re: [PATCH v2 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 4/27/21 6:00 AM, Daniel Vetter wrote:
> On Tue, Apr 27, 2021 at 10:40:24AM +0300, Pekka Paalanen wrote:
>> On Mon, 26 Apr 2021 14:30:53 -0300
>> Leandro Ribeiro  wrote:
>>
>>> On 4/26/21 7:58 AM, Simon Ser wrote:
>>>> On Monday, April 26th, 2021 at 9:36 AM, Pekka Paalanen 
>>>>  wrote:
>>>>   
>>>>>>> This should probably explain what the bits in the mask correspond to.
>>>>>>> As in, which CRTC does bit 0 refer to, and so on.  
>>>>>>
>>>>>> What about:
>>>>>>
>>>>>> "possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's are
>>>>>> created and they receive an index, which corresponds to their position
>>>>>> in the bitmask. CRTC with index 0 will be in bit 0, and so on."  
>>>>>
>>>>> This would still need to explain where can I find this index.  
>>>>   
>>>
>>> What do you mean?
>>>
>>>> This closed merge request had some docs about possible CRTCs:
>>>>
>>>> https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/102
>>>>   
>>> I'm afraid I don't know exactly what you expect to be documented here
>>> that is still missing. Could you please elaborate?
>>>
>>> Thanks a lot!
>>
>> The documentation you add is talking about "CRTC index". What defines a
>> CRTC object's index? How do I determine what index a CRTC object has?
>>
>> The answer is, AFAIK, that the index is never stored explicitly
>> anywhere. You have to get the DRM resources structure, which has an
>> array for CRTC IDs. The index is the index to that array, IIRC. So if
>> one does not already know this, it is going to be really hard to figure
>> out what the "index" is. It might even be confused with the object ID,
>> which it is not but the ID might by complete accident be less than 32
>> so it would look ok at first glance.
>>
>> If the index is already explained somewhere else, a reference to that
>> documentation would be enough.
> 
> I think if we do this we should have a DOC: section in the drm_mode.h uapi
> header which explains how the index is computed, and then we reference
> that everywhere. Because otherwise there's going to be a _lot_ of
> duplication of this all over. Kernel-internally we solve this by just
> referencing drm_foo_index() family of functions, but for the uapi there's
> really nothing, so needs text.
> 
> -Daniel
>
Ok, I've sent a v3 with a small section to document how to get the index
of a CRTC object from userspace perspective. But I could only find two
comments that would benefit from it (at least in "Userland interfaces"
page).

Thanks!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/doc: document how userspace should find out CRTC index

[Leandro Ribeiro]

In this patch we add a section to document what userspace should do to
find out the CRTC index. This is important as there are multiple places
in the documentation that need this, so it's better to just point to
this section and avoid repetition.

Signed-off-by: Leandro Ribeiro 
---
 Documentation/gpu/drm-uapi.rst| 14 ++
 drivers/gpu/drm/drm_debugfs_crc.c |  9 +
 include/uapi/drm/drm.h|  3 ++-
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 04bdc7a91d53..1aa52a6ac567 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -457,6 +457,20 @@ Userspace API Structures
 .. kernel-doc:: include/uapi/drm/drm_mode.h
:doc: overview

+.. _crtc_index:
+
+CRTC index
+--
+
+In some situations, it is important for userspace to find out the index of a
+CRTC. The CRTC index should not be confused with its object id.
+
+In order to do this, userspace should first query the resources object
+from the device that owns the CRTC (using the DRM_IOCTL_MODE_GETRESOURCES
+ioctl). The resources object contains a pointer to an array of CRTC's, and also
+the number of entries of the array. The index of the CRTC is the same as its
+position in this array.
+
 .. kernel-doc:: include/uapi/drm/drm.h
:internal:

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c 
b/drivers/gpu/drm/drm_debugfs_crc.c
index 3dd70d813f69..9575188d97ee 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -46,10 +46,11 @@
  * it reached a given hardware component (a CRC sampling "source").
  *
  * Userspace can control generation of CRCs in a given CRTC by writing to the
- * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the 
CRTC.
- * Accepted values are source names (which are driver-specific) and the "auto"
- * keyword, which will let the driver select a default source of frame CRCs
- * for this CRTC.
+ * file dri/0/crtc-N/crc/control in debugfs, with N being the index of the
+ * CRTC. To learn how to find out the index of a certain CRTC, please see
+ * :ref:`crtc_index`. Accepted values are source names (which are
+ * driver-specific) and the "auto" keyword, which will let the driver select a
+ * default source of frame CRCs for this CRTC.
  *
  * Once frame CRC generation is enabled, userspace can capture them by reading
  * the dri/0/crtc-N/crc/data file. Each line in that file contains the frame
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 67b94bc3c885..6944f08ab1a6 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -636,7 +636,8 @@ struct drm_gem_open {
  * DRM_CAP_VBLANK_HIGH_CRTC
  *
  * If set to 1, the kernel supports specifying a CRTC index in the high bits of
- * _wait_vblank_request.type.
+ * _wait_vblank_request.type. To learn how to find out the index of a
+ * certain CRTC, please see :ref:`crtc_index`.
  *
  * Starting kernel version 2.6.39, this capability is always set to 1.
  */
--
2.31.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

Add a small description and document struct fields of
drm_mode_get_plane.

Signed-off-by: Leandro Ribeiro 
---
 include/uapi/drm/drm_mode.h | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a5e76aa06ad5..8fa6495cd948 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -312,16 +312,38 @@ struct drm_mode_set_plane {
__u32 src_w;
 };

+/**
+ * struct drm_mode_get_plane - Get plane metadata.
+ *
+ * Userspace can perform a GETPLANE ioctl to retrieve information about a
+ * plane.
+ */
 struct drm_mode_get_plane {
+   /** @plane_id: Object ID of the plane. */
__u32 plane_id;

+   /** @crtc_id: Object ID of the current CRTC. */
__u32 crtc_id;
+   /** @fb_id: Object ID of the current fb. */
__u32 fb_id;

+   /**
+* @possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's
+* are created and they receive an index, which corresponds to their
+* position in the bitmask. CRTC with index 0 will be in bit 0, and so
+* on. To learn how to find out the index of a certain CRTC, please see
+* :ref:`crtc_index`.
+*/
__u32 possible_crtcs;
+   /** @gamma_size: Number of entries of the legacy gamma lookup table. */
__u32 gamma_size;

+   /** @count_format_types: Number of formats. */
__u32 count_format_types;
+   /**
+* @format_type_ptr: Pointer to ``__u32`` array of formats that are
+* supported by the plane. These formats do not require modifiers.
+*/
__u64 format_type_ptr;
 };

--
2.31.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 0/2] Document drm_mode_get_plane

[Leandro Ribeiro]

v2: possible_crtcs field is a bitmask, not a pointer. Suggested by
Ville Syrjälä 

v3: document how userspace should find out CRTC index. Also,
document that field 'gamma_size' represents the number of
entries in the lookup table. Suggested by Pekka Paalanen
 and Daniel Vetter 

Leandro Ribeiro (2):
  drm/doc: document how userspace should find out CRTC index
  drm/doc: document drm_mode_get_plane

 Documentation/gpu/drm-uapi.rst| 14 ++
 drivers/gpu/drm/drm_debugfs_crc.c |  9 +
 include/uapi/drm/drm.h|  3 ++-
 include/uapi/drm/drm_mode.h   | 22 ++
 4 files changed, 43 insertions(+), 5 deletions(-)

--
2.31.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 4/26/21 7:58 AM, Simon Ser wrote:
> On Monday, April 26th, 2021 at 9:36 AM, Pekka Paalanen  
> wrote:
> 
 This should probably explain what the bits in the mask correspond to.
 As in, which CRTC does bit 0 refer to, and so on.
>>>
>>> What about:
>>>
>>> "possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's are
>>> created and they receive an index, which corresponds to their position
>>> in the bitmask. CRTC with index 0 will be in bit 0, and so on."
>>
>> This would still need to explain where can I find this index.
> 

What do you mean?

> This closed merge request had some docs about possible CRTCs:
> 
> https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/102
> 
I'm afraid I don't know exactly what you expect to be documented here
that is still missing. Could you please elaborate?

Thanks a lot!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 4/23/21 8:11 AM, Pekka Paalanen wrote:
> On Thu, 22 Apr 2021 15:10:04 -0300
> Leandro Ribeiro  wrote:
> 
>> Add a small description and document struct fields of
>> drm_mode_get_plane.
>>
>> Signed-off-by: Leandro Ribeiro 
>> ---
>>  include/uapi/drm/drm_mode.h | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index a5e76aa06ad5..3e85de928db9 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -312,16 +312,32 @@ struct drm_mode_set_plane {
>>  __u32 src_w;
>>  };
>>
>> +/**
>> + * struct drm_mode_get_plane - Get plane metadata.
>> + *
>> + * Userspace can perform a GETPLANE ioctl to retrieve information about a
>> + * plane.
>> + */
>>  struct drm_mode_get_plane {
>> +/** @plane_id: Object ID of the plane. */
>>  __u32 plane_id;
>>
>> +/** @crtc_id: Object ID of the current CRTC. */
>>  __u32 crtc_id;
>> +/** @fb_id: Object ID of the current fb. */
>>  __u32 fb_id;
>>
>> +/** @possible_crtcs: Bitmask of CRTC's compatible with the plane. */
> 
> This should probably explain what the bits in the mask correspond to.
> As in, which CRTC does bit 0 refer to, and so on.
> 

What about:

"possible_crtcs: Bitmask of CRTC's compatible with the plane. CRTC's are
created and they receive an index, which corresponds to their position
in the bitmask. CRTC with index 0 will be in bit 0, and so on."

>>  __u32 possible_crtcs;
>> +/** @gamma_size: Size of the legacy gamma table. */
> 
> What are the units? Entries? Bytes?
> 

The number of entries. I'll update to "gamma_size: Number of entries of
the legacy gamma lookup table" in the next version.

>>  __u32 gamma_size;
>>
>> +/** @count_format_types: Number of formats. */
>>  __u32 count_format_types;
>> +/**
>> + * @format_type_ptr: Pointer to ``__u32`` array of formats that are
>> + * supported by the plane. These formats do not require modifiers.
> 
> I wonder if the "do not require modifiers" is again going too far in
> making a difference between this list and IN_FORMATS?
> 

Yes that's true, I'll drop this phrase.

>> + */
>>  __u64 format_type_ptr;
>>  };
> 
> Other than those, looks like a significant improvement to me.
> 
> 
> Thanks,
> pq
> 
>>
>> --
>> 2.31.1
>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/1] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

Add a small description and document struct fields of
drm_mode_get_plane.

Signed-off-by: Leandro Ribeiro 
---
 include/uapi/drm/drm_mode.h | 16 
 1 file changed, 16 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a5e76aa06ad5..3e85de928db9 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -312,16 +312,32 @@ struct drm_mode_set_plane {
__u32 src_w;
 };

+/**
+ * struct drm_mode_get_plane - Get plane metadata.
+ *
+ * Userspace can perform a GETPLANE ioctl to retrieve information about a
+ * plane.
+ */
 struct drm_mode_get_plane {
+   /** @plane_id: Object ID of the plane. */
__u32 plane_id;

+   /** @crtc_id: Object ID of the current CRTC. */
__u32 crtc_id;
+   /** @fb_id: Object ID of the current fb. */
__u32 fb_id;

+   /** @possible_crtcs: Bitmask of CRTC's compatible with the plane. */
__u32 possible_crtcs;
+   /** @gamma_size: Size of the legacy gamma table. */
__u32 gamma_size;

+   /** @count_format_types: Number of formats. */
__u32 count_format_types;
+   /**
+* @format_type_ptr: Pointer to ``__u32`` array of formats that are
+* supported by the plane. These formats do not require modifiers.
+*/
__u64 format_type_ptr;
 };

--
2.31.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/1] Document drm_mode_get_plane

[Leandro Ribeiro]

This is a follow up of the patchset "Document how userspace should use
plane format list and IN_FORMATS":

https://lists.freedesktop.org/archives/dri-devel/2021-April/302433.html

The first patch of the series ("drm/doc: document drm_mode_get_plane")
is still useful, although the other commit of the series was incorrect.
So I'm pushing the first commit again.

v2: possible_crtcs field is a bitmask, not a pointer. Suggested by
Ville Syrjälä 

Leandro Ribeiro (1):
  drm/doc: document drm_mode_get_plane

 include/uapi/drm/drm_mode.h | 16 
 1 file changed, 16 insertions(+)

--
2.31.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS

[Leandro Ribeiro]

On 4/8/21 8:35 AM, Daniel Vetter wrote:
> On Tue, Apr 06, 2021 at 04:21:16PM -0300, Leandro Ribeiro wrote:
>> This patch is to emphasize how userspace should use the plane format list and
>> the IN_FORMATS blob. The plane format list contains the formats that do not
>> require modifiers, and the blob property has the formats that support
>> modifiers.
>
> Uh this is a very strong statement that I don't think is supported by
> kernel driver code. Where is this from.
>
>> Note that these are not disjoint sets. If a format supports modifiers but the
>> driver can also handle it without a modifier, it should be present in both 
>> the
>> IN_FORMATS blob property and the plane format list.
> 
> Same here ...
> 

Yes, sorry. The wording was not good. To clarify:

I'm trying to document a good approach that userspace *can* (not must)
take to be able to tell if a certain format can be used in the
pre-modifier kernel uAPI or if it only works with modifiers.

The background is that we are reworking the way that Weston stores the
formats and modifiers supported by the planes, and there were some wrong
assumptions in the code related to what we can assume that the KMS
driver supports.

We've discussed and decided to send a patch to raise a discussion and
check if the conclusions that we've made were reasonable. And if not,
what would be a better approach.

This is part of a MR in which we add support for the dmabuf-hints
protocol extension in Weston. In sort, in Weston we store the formats
and modifiers supported by the planes. Then we send them to the client
and it may pick one of these format/modifier pairs to allocate its
buffers, increasing the chances of its surface ending up in a plane.

Here are two commits of the MR that are related to this discussion:

https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/544/diffs?commit_id=de6fc18bc35c2e43dff936dd85f310d1f778a7b8

https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/544/diffs?commit_id=75363bdb121bda2f326109afca5f4c3259423b7d

Thanks!

> I thought these two lists are 100% consistent. If not sounds like driver
> bugs that we need to maybe validate in drm_plane_init.
> 
>> This is important for userspace, as there are situations in which we need to
>> find out if the KMS driver can handle a certain format without any modifiers.
> 
> I don't think you can rely on this. No modifiers means implicit modifier,
> and the only thing that can give you such buffers is defacto mesa
> userspace drivers (since that all depends upon driver private magic, with
> maybe some kernel metadata passed around in private ioctls on the render
> node).
> 
> Maybe for more context, what's the problem you've hit and trying to
> clarify here?
> -Daniel
> 
>>
>> Leandro Ribeiro (2):
>>   drm/doc: document drm_mode_get_plane
>>   drm/doc: emphasize difference between plane formats and IN_FORMATS
>> blob
>>
>>  drivers/gpu/drm/drm_plane.c |  4 
>>  include/uapi/drm/drm_mode.h | 22 ++
>>  2 files changed, 26 insertions(+)
>>
>> -- 
>> 2.31.1
>>
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

On 4/7/21 10:45 AM, Ville Syrjälä wrote:
> On Tue, Apr 06, 2021 at 04:21:17PM -0300, Leandro Ribeiro wrote:
>> Add a small description and document struct fields of
>> drm_mode_get_plane.
>>
>> Signed-off-by: Leandro Ribeiro 
>> ---
>>  include/uapi/drm/drm_mode.h | 19 +++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index d1a93d2a85f9..96fc9a6da608 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -312,16 +312,35 @@ struct drm_mode_set_plane {
>>  __u32 src_w;
>>  };
>>  
>> +/**
>> + * struct drm_mode_get_plane - Get plane metadata.
>> + *
>> + * Userspace can perform a GETPLANE ioctl to retrieve information about a
>> + * plane.
>> + */
>>  struct drm_mode_get_plane {
>> +/** @plane_id: Object ID of the plane. */
>>  __u32 plane_id;
>>  
>> +/** @crtc_id: Object ID of the current CRTC. */
>>  __u32 crtc_id;
>> +/** @fb_id: Object ID of the current fb. */
>>  __u32 fb_id;
>>  
>> +/**
>> + * @possible_crtcs: Pointer to ``__u32`` array of CRTC's that are
>> + * compatible with the plane.
>> + */
> 
> It's a bitmask.

Thank you, I'll fix this in the next version.

> 
>>  __u32 possible_crtcs;
>> +/** @gamma_size: Size of the legacy gamma table. */
>>  __u32 gamma_size;
>>  
>> +/** @count_format_types: Number of formats. */
>>  __u32 count_format_types;
>> +/**
>> + * @format_type_ptr: Pointer to ``__u32`` array of formats that are
>> + * supported by the plane. These formats do not require modifiers.
>> + */
>>  __u64 format_type_ptr;
>>  };
>>  
>> -- 
>> 2.31.1
>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/doc: document drm_mode_get_plane

[Leandro Ribeiro]

Add a small description and document struct fields of
drm_mode_get_plane.

Signed-off-by: Leandro Ribeiro 
---
 include/uapi/drm/drm_mode.h | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index d1a93d2a85f9..96fc9a6da608 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -312,16 +312,35 @@ struct drm_mode_set_plane {
__u32 src_w;
 };
 
+/**
+ * struct drm_mode_get_plane - Get plane metadata.
+ *
+ * Userspace can perform a GETPLANE ioctl to retrieve information about a
+ * plane.
+ */
 struct drm_mode_get_plane {
+   /** @plane_id: Object ID of the plane. */
__u32 plane_id;
 
+   /** @crtc_id: Object ID of the current CRTC. */
__u32 crtc_id;
+   /** @fb_id: Object ID of the current fb. */
__u32 fb_id;
 
+   /**
+* @possible_crtcs: Pointer to ``__u32`` array of CRTC's that are
+* compatible with the plane.
+*/
__u32 possible_crtcs;
+   /** @gamma_size: Size of the legacy gamma table. */
__u32 gamma_size;
 
+   /** @count_format_types: Number of formats. */
__u32 count_format_types;
+   /**
+* @format_type_ptr: Pointer to ``__u32`` array of formats that are
+* supported by the plane. These formats do not require modifiers.
+*/
__u64 format_type_ptr;
 };
 
-- 
2.31.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/2] Document how userspace should use plane format list and IN_FORMATS

[Leandro Ribeiro]

This patch is to emphasize how userspace should use the plane format list and
the IN_FORMATS blob. The plane format list contains the formats that do not
require modifiers, and the blob property has the formats that support
modifiers.

Note that these are not disjoint sets. If a format supports modifiers but the
driver can also handle it without a modifier, it should be present in both the
IN_FORMATS blob property and the plane format list.

This is important for userspace, as there are situations in which we need to
find out if the KMS driver can handle a certain format without any modifiers.

Leandro Ribeiro (2):
  drm/doc: document drm_mode_get_plane
  drm/doc: emphasize difference between plane formats and IN_FORMATS
blob

 drivers/gpu/drm/drm_plane.c |  4 
 include/uapi/drm/drm_mode.h | 22 ++
 2 files changed, 26 insertions(+)

-- 
2.31.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/doc: emphasize difference between plane formats and IN_FORMATS blob

[Leandro Ribeiro]

Emphasize how userspace should use the plane format list
(format_type_ptr) and the IN_FORMATS blob property.

Formats exposed in the plane format list (format_type_ptr) do not
require modifiers, and formats that are present in the IN_FORMATS blob
property support modifiers.

Note that these are not disjoint sets. If a format supports modifiers
but the driver can also handle it without a modifier, it should be
present in both the IN_FORMATS blob property and the plane format list.

Signed-off-by: Leandro Ribeiro 
---
 drivers/gpu/drm/drm_plane.c | 4 
 include/uapi/drm/drm_mode.h | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 0dd43882fe7c..b48d9bd81a59 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -128,6 +128,10 @@
  * pairs supported by this plane. The blob is a struct
  * drm_format_modifier_blob. Without this property the plane doesn't
  * support buffers with modifiers. Userspace cannot change this property.
+ *
+ * To find out the list of buffer formats which are supported without a
+ * modifier, userspace should not look at this blob property, but at the
+ * formats list of the plane: _mode_get_plane.format_type_ptr.
  */
 
 static unsigned int drm_num_planes(struct drm_device *dev)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 96fc9a6da608..4293800ec095 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -340,6 +340,9 @@ struct drm_mode_get_plane {
/**
 * @format_type_ptr: Pointer to ``__u32`` array of formats that are
 * supported by the plane. These formats do not require modifiers.
+*
+* To find out the list of formats that support modifiers, userspace
+* must use the plane IN_FORMATS blob property.
 */
__u64 format_type_ptr;
 };
-- 
2.31.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/vkms: detect modes during output initialization

[Leandro Ribeiro]

In userspace we can use drmGetConnector() or drmGetConnectorCurrent() in
order to retrieve connector information. The difference between both is
that the former retrieves the complete set of modes and encoders
associated with the connector, while the latter only retrieves the
currently known set of modes and encoders - but is much faster.

This performance improvement is the reason why userspace applications
may prefer to use drmGetConnectorCurrent() when they need to retrieve
information from a device. The problem is that until now VKMS used to
init its output with an encoder, but without any valid mode, so
these userspace applications would not be able to use VKMS.

Call drm_helper_probe_single_connector_modes() during VKMS output
initialization in order to start with the set of all valid modes.

Signed-off-by: Leandro Ribeiro 
---
 drivers/gpu/drm/vkms/vkms_output.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_output.c 
b/drivers/gpu/drm/vkms/vkms_output.c
index 4a1848b0318f..20343592d38a 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -80,6 +80,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
goto err_attach;
}
 
+   ret = drm_helper_probe_single_connector_modes(connector, XRES_MAX, 
YRES_MAX);
+   if (ret == 0) {
+   DRM_ERROR("Failed to get modes for connector\n");
+   goto err_attach;
+   }
+
ret = vkms_enable_writeback_connector(vkmsdev);
if (ret)
DRM_ERROR("Failed to init writeback connector\n");
-- 
2.29.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vkms: detect modes during output initialization

[Leandro Ribeiro]

On 11/27/20 6:10 PM, Leandro Ribeiro wrote:
> In userspace we can use drmGetConnector() or drmGetConnectorCurrent() in
> order to retrieve connector information. The difference between both is
> that the former retrieves the complete set of modes and encoders
> associated with the connector, while the latter only retrieves the
> currently known set of modes and encoders - but is much faster.
> 
> This performance improvement is the reason why userspace applications
> may prefer to use drmGetConnectorCurrent() when they need to retrieve
> information from a device. The problem is that until now VKMS used to
> init its output with an encoder, but without any valid mode, so
> these userspace applications would not be able to use VKMS.
> 
> Call drm_helper_probe_single_connector_modes() during VKMS output
> initialization in order to start with the set of all valid modes.
> 
> Signed-off-by: Leandro Ribeiro 
> ---
>  drivers/gpu/drm/vkms/vkms_output.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c 
> b/drivers/gpu/drm/vkms/vkms_output.c
> index 4a1848b0318f..20343592d38a 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -80,6 +80,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int 
> index)
>   goto err_attach;
>   }
>  
> + ret = drm_helper_probe_single_connector_modes(connector, XRES_MAX, 
> YRES_MAX);
> + if (ret == 0) {
> + DRM_ERROR("Failed to get modes for connector\n");
> + goto err_attach;
> + }
> +
>   ret = vkms_enable_writeback_connector(vkmsdev);
>   if (ret)
>   DRM_ERROR("Failed to init writeback connector\n");
> 

After this patch we start to receive the warning from
drivers/gpu/drm/drm_modes.c:110

Thanks,
Leandro Ribeiro.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [BUG] drm/vkms: Failure when using drmGetConnectorCurrent()

[Leandro Ribeiro]

On 11/24/20 11:39 AM, Daniel Vetter wrote:

On Fri, Nov 20, 2020 at 01:19:04PM -0300, Leandro Ribeiro wrote:

Hello,

We have a patch in Weston to replace drmGetConnector() calls with
drmGetConnectorCurrent():

https://gitlab.freedesktop.org/wayland/weston/-/issues/437
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/518

Unfortunately this is not working when we use VKMS (upstream version
tested). Please see

https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/518#note_700345


I guess this is without fbdev configured on vkms? That's what usually
papers over this problem for most drivers.


Yes, without fbdev configured.


for more information, and feel free to jump into the discussion. If there's
more helpful information that I can share, please let me know.


Like Simon suggested, please submit that patch you have for discussion.
-Daniel


Sure, I'll submit the patch.

Thank you,
Leandro Ribeiro.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[BUG] drm/vkms: Failure when using drmGetConnectorCurrent()

[Leandro Ribeiro]

Hello,

We have a patch in Weston to replace drmGetConnector() calls with 
drmGetConnectorCurrent():


https://gitlab.freedesktop.org/wayland/weston/-/issues/437
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/518

Unfortunately this is not working when we use VKMS (upstream version 
tested). Please see


https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/518#note_700345

for more information, and feel free to jump into the discussion. If 
there's more helpful information that I can share, please let me know.


Thank you,
Leandro
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vkms: add missing drm_crtc_vblank_put to the get/put pair on flush

[Leandro Ribeiro]

Hello everybody!

I'm currently working on a writeback connector screenshooter for Weston. 
In order
to test it, I'm using VKMS with Rodrigo's writeback connector patch 
<https://lkml.org/lkml/2020/5/11/449>.Here is the
link with the MR in Weston 
<https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/458> 
with more details of how I've tested it.


The reason why I'm writing this is that in the first writeback connector 
screenshot
VKMSgets stuck. And I believe (from what I've tried to debug) that what 
happens is
that thewriteback job gets stuck in the queue waiting for a vsync 
signal. Then from
the second screenshot on everything works fine. So I believe this is 
related to this

issue somehow.

Melissa's idea to add drm_crtc_vblank_put(crtc) made it work, although 
VKMS started

to print this warn message:

WARNING: CPU: 0 PID: 168 at drivers/gpu/drm/vkms/vkms_crtc.c:21 
vkms_vblank_simulate+0x101/0x110


I've decided to share this info with you, as it may help you somehow. 
I'm also

investigating to help understand what is happening.

Thanks,
Leandro Ribeiro

On 7/31/20 1:47 PM, Melissa Wen wrote:

On 07/31, Sidong Yang wrote:

On Fri, Jul 31, 2020 at 11:08:34AM +0200, dan...@ffwll.ch wrote:

On Thu, Jul 30, 2020 at 07:09:25AM -0300, Melissa Wen wrote:

On 07/29, Daniel Vetter wrote:

On Wed, Jul 29, 2020 at 9:09 PM Melissa Wen  wrote:

Melissa Wen

On Sat, Jul 25, 2020 at 3:12 PM Daniel Vetter  wrote:

On Sat, Jul 25, 2020 at 7:45 PM Melissa Wen  wrote:

On 07/25, Daniel Vetter wrote:

On Sat, Jul 25, 2020 at 5:12 AM Sidong Yang  wrote:

On Wed, Jul 22, 2020 at 05:17:05PM +0200, Daniel Vetter wrote:

On Wed, Jul 22, 2020 at 4:06 PM Melissa Wen  wrote:

On 07/22, dan...@ffwll.ch wrote:

On Wed, Jul 22, 2020 at 08:04:11AM -0300, Melissa Wen wrote:

This patch adds a missing drm_crtc_vblank_put op to the pair
drm_crtc_vblank_get/put (inc/decrement counter to guarantee vblanks).

It clears the execution of the following kms_cursor_crc subtests:
1. pipe-A-cursor-[size,alpha-opaque, NxN-(on-screen, off-screen, sliding,
random, fast-moving])] - successful when running individually.
2. pipe-A-cursor-dpms passes again
3. pipe-A-cursor-suspend also passes

The issue was initially tracked in the sequential execution of IGT
kms_cursor_crc subtest: when running the test sequence or one of its
subtests twice, the odd execs complete and the pairs get stuck in an
endless wait. In the IGT code, calling a wait_for_vblank before the start
of CRC capture prevented the busy-wait. But the problem persisted in the
pipe-A-cursor-dpms and -suspend subtests.

Checking the history, the pipe-A-cursor-dpms subtest was successful when,
in vkms_atomic_commit_tail, instead of using the flip_done op, it used
wait_for_vblanks. Another way to prevent blocking was wait_one_vblank when
enabling crtc. However, in both cases, pipe-A-cursor-suspend persisted
blocking in the 2nd start of CRC capture, which may indicate that
something got stuck in the step of CRC setup. Indeed, wait_one_vblank in
the crc setup was able to sync things and free all kms_cursor_crc
subtests.

Tracing and comparing a clean run with a blocked one:
- in a clean one, vkms_crtc_atomic_flush enables vblanks;
- when blocked, only in next op, vkms_crtc_atomic_enable, the vblanks
started. Moreover, a series of vkms_vblank_simulate flow out until
disabling vblanks.
Also watching the steps of vkms_crtc_atomic_flush, when the very first
drm_crtc_vblank_get returned an error, the subtest crashed. On the other
hand, when vblank_get succeeded, the subtest completed. Finally, checking
the flush steps: it increases counter to hold a vblank reference (get),
but there isn't a op to decreased it and release vblanks (put).

Cc: Daniel Vetter 
Cc: Rodrigo Siqueira 
Cc: Haneen Mohammed 
Signed-off-by: Melissa Wen 
---
  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index ac85e17428f8..a99d6b4a92dd 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -246,6 +246,7 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,

 spin_unlock(>dev->event_lock);

+   drm_crtc_vblank_put(crtc);

Uh so I reviewed this a bit more carefully now, and I dont think this is
the correct bugfix. From the kerneldoc of drm_crtc_arm_vblank_event():

  * Caller must hold a vblank reference for the event @e acquired by a
  * drm_crtc_vblank_get(), which will be dropped when the next vblank arrives.

So when we call drm_crtc_arm_vblank_event then the vblank_put gets called
for us. And that's the only case where we successfully acquired a vblank
interrupt reference since on failure of drm_crtc_vblank_get (0 indicates
success for that function, failure negative error number) we directly send
out the event.

So something else fishy is going on, and now I'm totally confused why this
even happens.

We also have a pile

Re: [PATCH] drm/vkms: add missing drm_crtc_vblank_put to the get/put pair on flush

[Leandro Ribeiro]

Hello everybody!

I'm currently working on a writeback connector screenshooter for Weston. 
In order to test it, I'm using VKMS with Rodrigo's writeback connector 
patch: https://lkml.org/lkml/2020/5/11/449


Here is the link with the MR in Weston with more details of how I've 
tested it: 
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/458


The reason why I'm writing this is that in the first writeback connector 
screenshot VKMS gets stuck. And I believe (from what I've tried to 
debug) that what happens is that the writeback job gets stuck in the 
queue waiting for a vsync signal. Then from the second screenshot on 
everything works fine. So I believe this is related to this issue somehow.


Melissa's idea to add `drm_crtc_vblank_put(crtc)` made it work, although 
VKMS started to print this warn message:


WARNING: CPU: 0 PID: 168 at drivers/gpu/drm/vkms/vkms_crtc.c:21 
vkms_vblank_simulate+0x101/0x110


From what I've read from this thread it seems like this is not the 
right fix, but I've decided to share this info with you anyway, as it 
may help. I'm also trying to understand what is happening.


Thanks,
Leandro Ribeiro

On 7/31/20 1:47 PM, Melissa Wen wrote:

On 07/31, Sidong Yang wrote:

On Fri, Jul 31, 2020 at 11:08:34AM +0200, dan...@ffwll.ch wrote:

On Thu, Jul 30, 2020 at 07:09:25AM -0300, Melissa Wen wrote:

On 07/29, Daniel Vetter wrote:

On Wed, Jul 29, 2020 at 9:09 PM Melissa Wen  wrote:


Melissa Wen

On Sat, Jul 25, 2020 at 3:12 PM Daniel Vetter  wrote:


On Sat, Jul 25, 2020 at 7:45 PM Melissa Wen  wrote:


On 07/25, Daniel Vetter wrote:

On Sat, Jul 25, 2020 at 5:12 AM Sidong Yang  wrote:


On Wed, Jul 22, 2020 at 05:17:05PM +0200, Daniel Vetter wrote:

On Wed, Jul 22, 2020 at 4:06 PM Melissa Wen  wrote:


On 07/22, dan...@ffwll.ch wrote:

On Wed, Jul 22, 2020 at 08:04:11AM -0300, Melissa Wen wrote:

This patch adds a missing drm_crtc_vblank_put op to the pair
drm_crtc_vblank_get/put (inc/decrement counter to guarantee vblanks).

It clears the execution of the following kms_cursor_crc subtests:
1. pipe-A-cursor-[size,alpha-opaque, NxN-(on-screen, off-screen, sliding,
random, fast-moving])] - successful when running individually.
2. pipe-A-cursor-dpms passes again
3. pipe-A-cursor-suspend also passes

The issue was initially tracked in the sequential execution of IGT
kms_cursor_crc subtest: when running the test sequence or one of its
subtests twice, the odd execs complete and the pairs get stuck in an
endless wait. In the IGT code, calling a wait_for_vblank before the start
of CRC capture prevented the busy-wait. But the problem persisted in the
pipe-A-cursor-dpms and -suspend subtests.

Checking the history, the pipe-A-cursor-dpms subtest was successful when,
in vkms_atomic_commit_tail, instead of using the flip_done op, it used
wait_for_vblanks. Another way to prevent blocking was wait_one_vblank when
enabling crtc. However, in both cases, pipe-A-cursor-suspend persisted
blocking in the 2nd start of CRC capture, which may indicate that
something got stuck in the step of CRC setup. Indeed, wait_one_vblank in
the crc setup was able to sync things and free all kms_cursor_crc
subtests.

Tracing and comparing a clean run with a blocked one:
- in a clean one, vkms_crtc_atomic_flush enables vblanks;
- when blocked, only in next op, vkms_crtc_atomic_enable, the vblanks
started. Moreover, a series of vkms_vblank_simulate flow out until
disabling vblanks.
Also watching the steps of vkms_crtc_atomic_flush, when the very first
drm_crtc_vblank_get returned an error, the subtest crashed. On the other
hand, when vblank_get succeeded, the subtest completed. Finally, checking
the flush steps: it increases counter to hold a vblank reference (get),
but there isn't a op to decreased it and release vblanks (put).

Cc: Daniel Vetter 
Cc: Rodrigo Siqueira 
Cc: Haneen Mohammed 
Signed-off-by: Melissa Wen 
---
  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index ac85e17428f8..a99d6b4a92dd 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -246,6 +246,7 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,

 spin_unlock(>dev->event_lock);

+   drm_crtc_vblank_put(crtc);


Uh so I reviewed this a bit more carefully now, and I dont think this is
the correct bugfix. From the kerneldoc of drm_crtc_arm_vblank_event():

  * Caller must hold a vblank reference for the event @e acquired by a
  * drm_crtc_vblank_get(), which will be dropped when the next vblank arrives.

So when we call drm_crtc_arm_vblank_event then the vblank_put gets called
for us. And that's the only case where we successfully acquired a vblank
interrupt reference since on failure of drm_crtc_vblank_get (0 indicates
success for that function, failure negative error number) we directly send
out the event.

So something else fishy is going on, and n

Re: [PATCH] drm/vkms: add missing drm_crtc_vblank_put to the get/put pair on flush

[Leandro Ribeiro]

Hello everybody!

I'm currently working on a writeback connector screenshooter for Weston. 
In order

to test it, I'm using VKMS with Rodrigo's writeback connector patch:
https://lkml.org/lkml/2020/5/11/449

Here is the link with the MR in Weston with more details of how I've 
tested it:

https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/458

The reason why I'm writing this is that in the first writeback connector 
screenshot
VKMSgets stuck. And I believe (from what I've tried to debug) that what 
happens is
that thewriteback job gets stuck in the queue waiting for a vsync 
signal. Then from
the second screenshot on everything works fine. So I believe this is 
related to this

issue somehow.

Melissa's idea to add drm_crtc_vblank_put(crtc) made it work, although 
VKMS started

to print this warn message:

WARNING: CPU: 0 PID: 168 at drivers/gpu/drm/vkms/vkms_crtc.c:21 
vkms_vblank_simulate+0x101/0x110


I've decided to share this info with you, as it may help you somehow. 
I'm also

investigating to help understand what is happening.

Thanks,
Leandro Ribeiro

On 7/31/20 1:47 PM, Melissa Wen wrote:

On 07/31, Sidong Yang wrote:

On Fri, Jul 31, 2020 at 11:08:34AM +0200, dan...@ffwll.ch wrote:

On Thu, Jul 30, 2020 at 07:09:25AM -0300, Melissa Wen wrote:

On 07/29, Daniel Vetter wrote:

On Wed, Jul 29, 2020 at 9:09 PM Melissa Wen  wrote:

Melissa Wen

On Sat, Jul 25, 2020 at 3:12 PM Daniel Vetter  wrote:

On Sat, Jul 25, 2020 at 7:45 PM Melissa Wen  wrote:

On 07/25, Daniel Vetter wrote:

On Sat, Jul 25, 2020 at 5:12 AM Sidong Yang  wrote:

On Wed, Jul 22, 2020 at 05:17:05PM +0200, Daniel Vetter wrote:

On Wed, Jul 22, 2020 at 4:06 PM Melissa Wen  wrote:

On 07/22, dan...@ffwll.ch wrote:

On Wed, Jul 22, 2020 at 08:04:11AM -0300, Melissa Wen wrote:

This patch adds a missing drm_crtc_vblank_put op to the pair
drm_crtc_vblank_get/put (inc/decrement counter to guarantee vblanks).

It clears the execution of the following kms_cursor_crc subtests:
1. pipe-A-cursor-[size,alpha-opaque, NxN-(on-screen, off-screen, sliding,
random, fast-moving])] - successful when running individually.
2. pipe-A-cursor-dpms passes again
3. pipe-A-cursor-suspend also passes

The issue was initially tracked in the sequential execution of IGT
kms_cursor_crc subtest: when running the test sequence or one of its
subtests twice, the odd execs complete and the pairs get stuck in an
endless wait. In the IGT code, calling a wait_for_vblank before the start
of CRC capture prevented the busy-wait. But the problem persisted in the
pipe-A-cursor-dpms and -suspend subtests.

Checking the history, the pipe-A-cursor-dpms subtest was successful when,
in vkms_atomic_commit_tail, instead of using the flip_done op, it used
wait_for_vblanks. Another way to prevent blocking was wait_one_vblank when
enabling crtc. However, in both cases, pipe-A-cursor-suspend persisted
blocking in the 2nd start of CRC capture, which may indicate that
something got stuck in the step of CRC setup. Indeed, wait_one_vblank in
the crc setup was able to sync things and free all kms_cursor_crc
subtests.

Tracing and comparing a clean run with a blocked one:
- in a clean one, vkms_crtc_atomic_flush enables vblanks;
- when blocked, only in next op, vkms_crtc_atomic_enable, the vblanks
started. Moreover, a series of vkms_vblank_simulate flow out until
disabling vblanks.
Also watching the steps of vkms_crtc_atomic_flush, when the very first
drm_crtc_vblank_get returned an error, the subtest crashed. On the other
hand, when vblank_get succeeded, the subtest completed. Finally, checking
the flush steps: it increases counter to hold a vblank reference (get),
but there isn't a op to decreased it and release vblanks (put).

Cc: Daniel Vetter 
Cc: Rodrigo Siqueira 
Cc: Haneen Mohammed 
Signed-off-by: Melissa Wen 
---
  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index ac85e17428f8..a99d6b4a92dd 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -246,6 +246,7 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,

 spin_unlock(>dev->event_lock);

+   drm_crtc_vblank_put(crtc);

Uh so I reviewed this a bit more carefully now, and I dont think this is
the correct bugfix. From the kerneldoc of drm_crtc_arm_vblank_event():

  * Caller must hold a vblank reference for the event @e acquired by a
  * drm_crtc_vblank_get(), which will be dropped when the next vblank arrives.

So when we call drm_crtc_arm_vblank_event then the vblank_put gets called
for us. And that's the only case where we successfully acquired a vblank
interrupt reference since on failure of drm_crtc_vblank_get (0 indicates
success for that function, failure negative error number) we directly send
out the event.

So something else fishy is going on, and now I'm totally confused why this
even happens.

We also have a pile of WARN_ON 

Re: [PATCH] drm/vkms: add missing drm_crtc_vblank_put to the get/put pair on flush

[Leandro Ribeiro]

Hello everybody!

I'm currently working on a writeback connector screenshooter for Weston. 
In order to test it, I'm using VKMS with Rodrigo's writeback connector 
patch: https://lkml.org/lkml/2020/5/11/449


Here is the link with the MR in Weston with more details of how I've 
tested it: 
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/458


The reason why I'm writing this is that in the first writeback connector 
screenshot VKMS gets stuck. And I believe (from what I've tried to 
debug) that what happens is that the writeback job gets stuck in the 
queue waiting for a vsync signal. Then from the second screenshot on 
everything works fine. So I believe this is related to this issue somehow.


Melissa's idea to add `drm_crtc_vblank_put(crtc)` made it work, although 
VKMS started to print this warn message:


WARNING: CPU: 0 PID: 168 at drivers/gpu/drm/vkms/vkms_crtc.c:21 
vkms_vblank_simulate+0x101/0x110


From what I've read from this thread it seems like this is not the 
right fix, but I've decided to share this info with you anyway, as it 
may help. I'm also trying to understand what is happening.


Thanks,
Leandro Ribeiro

On 7/31/20 3:33 PM, Daniel Vetter wrote:


On Fri, Jul 31, 2020 at 6:47 PM Melissa Wen  wrote:


On 07/31, Sidong Yang wrote:

On Fri, Jul 31, 2020 at 11:08:34AM +0200, dan...@ffwll.ch wrote:

On Thu, Jul 30, 2020 at 07:09:25AM -0300, Melissa Wen wrote:

On 07/29, Daniel Vetter wrote:

On Wed, Jul 29, 2020 at 9:09 PM Melissa Wen  wrote:


Melissa Wen

On Sat, Jul 25, 2020 at 3:12 PM Daniel Vetter  wrote:


On Sat, Jul 25, 2020 at 7:45 PM Melissa Wen  wrote:


On 07/25, Daniel Vetter wrote:

On Sat, Jul 25, 2020 at 5:12 AM Sidong Yang  wrote:


On Wed, Jul 22, 2020 at 05:17:05PM +0200, Daniel Vetter wrote:

On Wed, Jul 22, 2020 at 4:06 PM Melissa Wen  wrote:


On 07/22, dan...@ffwll.ch wrote:

On Wed, Jul 22, 2020 at 08:04:11AM -0300, Melissa Wen wrote:

This patch adds a missing drm_crtc_vblank_put op to the pair
drm_crtc_vblank_get/put (inc/decrement counter to guarantee vblanks).

It clears the execution of the following kms_cursor_crc subtests:
1. pipe-A-cursor-[size,alpha-opaque, NxN-(on-screen, off-screen, sliding,
random, fast-moving])] - successful when running individually.
2. pipe-A-cursor-dpms passes again
3. pipe-A-cursor-suspend also passes

The issue was initially tracked in the sequential execution of IGT
kms_cursor_crc subtest: when running the test sequence or one of its
subtests twice, the odd execs complete and the pairs get stuck in an
endless wait. In the IGT code, calling a wait_for_vblank before the start
of CRC capture prevented the busy-wait. But the problem persisted in the
pipe-A-cursor-dpms and -suspend subtests.

Checking the history, the pipe-A-cursor-dpms subtest was successful when,
in vkms_atomic_commit_tail, instead of using the flip_done op, it used
wait_for_vblanks. Another way to prevent blocking was wait_one_vblank when
enabling crtc. However, in both cases, pipe-A-cursor-suspend persisted
blocking in the 2nd start of CRC capture, which may indicate that
something got stuck in the step of CRC setup. Indeed, wait_one_vblank in
the crc setup was able to sync things and free all kms_cursor_crc
subtests.

Tracing and comparing a clean run with a blocked one:
- in a clean one, vkms_crtc_atomic_flush enables vblanks;
- when blocked, only in next op, vkms_crtc_atomic_enable, the vblanks
started. Moreover, a series of vkms_vblank_simulate flow out until
disabling vblanks.
Also watching the steps of vkms_crtc_atomic_flush, when the very first
drm_crtc_vblank_get returned an error, the subtest crashed. On the other
hand, when vblank_get succeeded, the subtest completed. Finally, checking
the flush steps: it increases counter to hold a vblank reference (get),
but there isn't a op to decreased it and release vblanks (put).

Cc: Daniel Vetter 
Cc: Rodrigo Siqueira 
Cc: Haneen Mohammed 
Signed-off-by: Melissa Wen 
---
  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index ac85e17428f8..a99d6b4a92dd 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -246,6 +246,7 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,

 spin_unlock(>dev->event_lock);

+   drm_crtc_vblank_put(crtc);


Uh so I reviewed this a bit more carefully now, and I dont think this is
the correct bugfix. From the kerneldoc of drm_crtc_arm_vblank_event():

  * Caller must hold a vblank reference for the event @e acquired by a
  * drm_crtc_vblank_get(), which will be dropped when the next vblank arrives.

So when we call drm_crtc_arm_vblank_event then the vblank_put gets called
for us. And that's the only case where we successfully acquired a vblank
interrupt reference since on failure of drm_crtc_vblank_get (0 indicates
success for that function, failure negative error number) we directly se

[PATCH] drm/doc: Update IGT documentation

[Leandro Ribeiro]

The IGT documentation in this page is telling us to build it using
make. According to commit 67993c1 ("automake: Point builders at
meson") from the IGT project, this is deprecated and IGT should be
built with meson. Instead of having a documentation for IGT in this
page, point to their GitLab README, which should always be up to
date.

Signed-off-by: Leandro Ribeiro 
---
 Documentation/gpu/drm-uapi.rst | 32 ++--
 1 file changed, 2 insertions(+), 30 deletions(-)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 94f90521f58c..996f7354e05c 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -254,36 +254,8 @@ Validating changes with IGT
 There's a collection of tests that aims to cover the whole functionality of
 DRM drivers and that can be used to check that changes to DRM drivers or the
 core don't regress existing functionality. This test suite is called IGT and
-its code can be found in https://cgit.freedesktop.org/drm/igt-gpu-tools/.
-
-To build IGT, start by installing its build dependencies. In Debian-based
-systems::
-
-   # apt-get build-dep intel-gpu-tools
-
-And in Fedora-based systems::
-
-   # dnf builddep intel-gpu-tools
-
-Then clone the repository::
-
-   $ git clone git://anongit.freedesktop.org/drm/igt-gpu-tools
-
-Configure the build system and start the build::
-
-   $ cd igt-gpu-tools && ./autogen.sh && make -j6
-
-Download the piglit dependency::
-
-   $ ./scripts/run-tests.sh -d
-
-And run the tests::
-
-   $ ./scripts/run-tests.sh -t kms -t core -s
-
-run-tests.sh is a wrapper around piglit that will execute the tests matching
-the -t options. A report in HTML format will be available in
-./results/html/index.html. Results can be compared with piglit.
+its code and instructions to build and run can be found in
+https://gitlab.freedesktop.org/drm/igt-gpu-tools/.
 
 Display CRC Support
 ---
-- 
2.23.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel