Re: [Freedreno] [PATCH 01/23] drm: Add get_scanout_position() to struct drm_crtc_helper_funcs

[Thomas Zimmermann]

Hi

Am 14.01.20 um 16:31 schrieb Yannick FERTRE:
> Thanks for the patch.
> 
> Tested-by: Yannick Fertré  

Thanks for testing all these patches.

Best regards
Thomas

> 
> BR
> Yannick Fertré
> 
> 
> On 1/10/20 10:21 AM, Thomas Zimmermann wrote:
>> The new callback get_scanout_position() reads the current location of
>> the scanout process. The operation is currentyl located in struct
>> drm_driver, but really belongs to the CRTC. Drivers will be converted
>> in separate patches.
>>
>> Signed-off-by: Thomas Zimmermann 
>> ---
>>  drivers/gpu/drm/drm_vblank.c | 24 
>>  include/drm/drm_drv.h|  7 +---
>>  include/drm/drm_modeset_helper_vtables.h | 47 
>>  3 files changed, 65 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index 1659b13b178c..c12f0b333e14 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -30,6 +30,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  
>> @@ -590,7 +591,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
>>   * Implements calculation of exact vblank timestamps from given 
>> drm_display_mode
>>   * timings and current video scanout position of a CRTC. This can be 
>> directly
>>   * used as the _driver.get_vblank_timestamp implementation of a kms 
>> driver
>> - * if _driver.get_scanout_position is implemented.
>> + * if _crtc_helper_funcs.get_scanout_position is implemented.
>>   *
>>   * The current implementation only handles standard video modes. For double 
>> scan
>>   * and interlaced modes the driver is supposed to adjust the hardware mode
>> @@ -632,8 +633,9 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
>> drm_device *dev,
>>  }
>>  
>>  /* Scanout position query not supported? Should not happen. */
>> -if (!dev->driver->get_scanout_position) {
>> -DRM_ERROR("Called from driver w/o get_scanout_position()!?\n");
>> +if (!dev->driver->get_scanout_position ||
>> +!crtc->helper_private->get_scanout_position) {
>> +DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n");
>>  return false;
>>  }
>>  
>> @@ -664,11 +666,17 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
>> drm_device *dev,
>>   * Get vertical and horizontal scanout position vpos, hpos,
>>   * and bounding timestamps stime, etime, pre/post query.
>>   */
>> -vbl_status = dev->driver->get_scanout_position(dev, pipe,
>> -   in_vblank_irq,
>> -   , ,
>> -   , ,
>> -   mode);
>> +if (crtc->helper_private->get_scanout_position) {
>> +vbl_status =
>> +crtc->helper_private->get_scanout_position(
>> +crtc, in_vblank_irq, , ,
>> +, , mode);
>> +} else {
>> +vbl_status =
>> +dev->driver->get_scanout_position(
>> +dev, pipe, in_vblank_irq, ,
>> +, , , mode);
>> +}
>>  
>>  /* Return as no-op if scanout query unsupported or failed. */
>>  if (!vbl_status) {
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index cf13470810a5..d0049e5786fc 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -362,11 +362,8 @@ struct drm_driver {
>>   * True on success, false if a reliable scanout position counter could
>>   * not be read out.
>>   *
>> - * FIXME:
>> - *
>> - * Since this is a helper to implement @get_vblank_timestamp, we should
>> - * move it to  drm_crtc_helper_funcs, like all the other
>> - * helper-internal hooks.
>> + * This is deprecated and should not be used by new drivers.
>> + * Use _crtc_helper_funcs.get_scanout_position instead.
>>   */
>>  bool (*get_scanout_position) (struct drm_device *dev, unsigned int pipe,
>>bool in_vblank_irq, int *vpos, int *hpos,
>> diff --git a/include/drm/drm_modeset_helper_vtables.h 
>> b/include/drm/drm_modeset_helper_vtables.h
>> index 5a87f1bd7a3f..e398512bfd5f 100644
>> --- a/include/drm/drm_modeset_helper_vtables.h
>> +++ b/include/drm/drm_modeset_helper_vtables.h
>> @@ -450,6 +450,53 @@ struct drm_crtc_helper_funcs {
>>   */
>>  void (*atomic_disable)(struct drm_crtc *crtc,
>> struct drm_crtc_state *old_crtc_state);
>> +
>> +/**
>> + * @get_scanout_position:
>> + *
>> + * Called by vblank timestamping code.
>> + *
>> + * Returns the current display 

Re: [Freedreno] [PATCH 01/23] drm: Add get_scanout_position() to struct drm_crtc_helper_funcs

[Yannick FERTRE]

Thanks for the patch.

Tested-by: Yannick Fertré 

BR
Yannick Fertré


On 1/10/20 10:21 AM, Thomas Zimmermann wrote:

The new callback get_scanout_position() reads the current location of
the scanout process. The operation is currentyl located in struct
drm_driver, but really belongs to the CRTC. Drivers will be converted
in separate patches.

Signed-off-by: Thomas Zimmermann 

---
 drivers/gpu/drm/drm_vblank.c | 24 
 include/drm/drm_drv.h|  7 +---
 include/drm/drm_modeset_helper_vtables.h | 47 
 3 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 1659b13b178c..c12f0b333e14 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 

@@ -590,7 +591,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  * Implements calculation of exact vblank timestamps from given 
drm_display_mode
  * timings and current video scanout position of a CRTC. This can be directly
  * used as the _driver.get_vblank_timestamp implementation of a kms driver
- * if _driver.get_scanout_position is implemented.
+ * if _crtc_helper_funcs.get_scanout_position is implemented.
  *
  * The current implementation only handles standard video modes. For double 
scan
  * and interlaced modes the driver is supposed to adjust the hardware mode
@@ -632,8 +633,9 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
drm_device *dev,
}

/* Scanout position query not supported? Should not happen. */
-   if (!dev->driver->get_scanout_position) {
-   DRM_ERROR("Called from driver w/o get_scanout_position()!?\n");
+   if (!dev->driver->get_scanout_position ||
+   !crtc->helper_private->get_scanout_position) {
+   DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n");
return false;
}

@@ -664,11 +666,17 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
drm_device *dev,
 * Get vertical and horizontal scanout position vpos, hpos,
 * and bounding timestamps stime, etime, pre/post query.
 */
-   vbl_status = dev->driver->get_scanout_position(dev, pipe,
-  in_vblank_irq,
-  , ,
-  , ,
-  mode);
+   if (crtc->helper_private->get_scanout_position) {
+   vbl_status =
+   crtc->helper_private->get_scanout_position(
+   crtc, in_vblank_irq, , ,
+   , , mode);
+   } else {
+   vbl_status =
+   dev->driver->get_scanout_position(
+   dev, pipe, in_vblank_irq, ,
+   , , , mode);
+   }

/* Return as no-op if scanout query unsupported or failed. */
if (!vbl_status) {
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index cf13470810a5..d0049e5786fc 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -362,11 +362,8 @@ struct drm_driver {
 * True on success, false if a reliable scanout position counter could
 * not be read out.
 *
-* FIXME:
-*
-* Since this is a helper to implement @get_vblank_timestamp, we should
-* move it to  drm_crtc_helper_funcs, like all the other
-* helper-internal hooks.
+* This is deprecated and should not be used by new drivers.
+* Use _crtc_helper_funcs.get_scanout_position instead.
 */
bool (*get_scanout_position) (struct drm_device *dev, unsigned int pipe,
  bool in_vblank_irq, int *vpos, int *hpos,
diff --git a/include/drm/drm_modeset_helper_vtables.h 
b/include/drm/drm_modeset_helper_vtables.h
index 5a87f1bd7a3f..e398512bfd5f 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -450,6 +450,53 @@ struct drm_crtc_helper_funcs {
 */
void (*atomic_disable)(struct drm_crtc *crtc,
   struct drm_crtc_state *old_crtc_state);
+
+   /**
+* @get_scanout_position:
+*
+* Called by vblank timestamping code.
+*
+* Returns the current display scanout position from a CRTC and an
+* optional accurate ktime_get() timestamp of when the position was
+* measured. Note that this is a helper callback which is only used
+* if a driver uses 

Re: [Freedreno] [PATCH 01/23] drm: Add get_scanout_position() to struct drm_crtc_helper_funcs

[Jani Nikula]

On Fri, 10 Jan 2020, Thomas Zimmermann  wrote:
> The new callback get_scanout_position() reads the current location of
> the scanout process. The operation is currentyl located in struct
> drm_driver, but really belongs to the CRTC. Drivers will be converted
> in separate patches.
>
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_vblank.c | 24 
>  include/drm/drm_drv.h|  7 +---
>  include/drm/drm_modeset_helper_vtables.h | 47 
>  3 files changed, 65 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 1659b13b178c..c12f0b333e14 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -590,7 +591,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
>   * Implements calculation of exact vblank timestamps from given 
> drm_display_mode
>   * timings and current video scanout position of a CRTC. This can be directly
>   * used as the _driver.get_vblank_timestamp implementation of a kms 
> driver
> - * if _driver.get_scanout_position is implemented.
> + * if _crtc_helper_funcs.get_scanout_position is implemented.
>   *
>   * The current implementation only handles standard video modes. For double 
> scan
>   * and interlaced modes the driver is supposed to adjust the hardware mode
> @@ -632,8 +633,9 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
> drm_device *dev,
>   }
>  
>   /* Scanout position query not supported? Should not happen. */
> - if (!dev->driver->get_scanout_position) {
> - DRM_ERROR("Called from driver w/o get_scanout_position()!?\n");
> + if (!dev->driver->get_scanout_position ||
> + !crtc->helper_private->get_scanout_position) {

ITYM s/||/&&/

BR,
Jani.


> + DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n");
>   return false;
>   }
>  
> @@ -664,11 +666,17 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
> drm_device *dev,
>* Get vertical and horizontal scanout position vpos, hpos,
>* and bounding timestamps stime, etime, pre/post query.
>*/
> - vbl_status = dev->driver->get_scanout_position(dev, pipe,
> -in_vblank_irq,
> -, ,
> -, ,
> -mode);
> + if (crtc->helper_private->get_scanout_position) {
> + vbl_status =
> + crtc->helper_private->get_scanout_position(
> + crtc, in_vblank_irq, , ,
> + , , mode);
> + } else {
> + vbl_status =
> + dev->driver->get_scanout_position(
> + dev, pipe, in_vblank_irq, ,
> + , , , mode);
> + }
>  
>   /* Return as no-op if scanout query unsupported or failed. */
>   if (!vbl_status) {
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index cf13470810a5..d0049e5786fc 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -362,11 +362,8 @@ struct drm_driver {
>* True on success, false if a reliable scanout position counter could
>* not be read out.
>*
> -  * FIXME:
> -  *
> -  * Since this is a helper to implement @get_vblank_timestamp, we should
> -  * move it to  drm_crtc_helper_funcs, like all the other
> -  * helper-internal hooks.
> +  * This is deprecated and should not be used by new drivers.
> +  * Use _crtc_helper_funcs.get_scanout_position instead.
>*/
>   bool (*get_scanout_position) (struct drm_device *dev, unsigned int pipe,
> bool in_vblank_irq, int *vpos, int *hpos,
> diff --git a/include/drm/drm_modeset_helper_vtables.h 
> b/include/drm/drm_modeset_helper_vtables.h
> index 5a87f1bd7a3f..e398512bfd5f 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -450,6 +450,53 @@ struct drm_crtc_helper_funcs {
>*/
>   void (*atomic_disable)(struct drm_crtc *crtc,
>  struct drm_crtc_state *old_crtc_state);
> +
> + /**
> +  * @get_scanout_position:
> +  *
> +  * Called by vblank timestamping code.
> +  *
> +  * Returns the current display scanout position from a CRTC and an
> +  * optional accurate ktime_get() timestamp of when the position was
> +  * measured. Note that this is a helper callback which is only used
> +  * if a driver uses 

[Freedreno] [PATCH 01/23] drm: Add get_scanout_position() to struct drm_crtc_helper_funcs

[Thomas Zimmermann]

The new callback get_scanout_position() reads the current location of
the scanout process. The operation is currentyl located in struct
drm_driver, but really belongs to the CRTC. Drivers will be converted
in separate patches.

Signed-off-by: Thomas Zimmermann 
---
 drivers/gpu/drm/drm_vblank.c | 24 
 include/drm/drm_drv.h|  7 +---
 include/drm/drm_modeset_helper_vtables.h | 47 
 3 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 1659b13b178c..c12f0b333e14 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -590,7 +591,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  * Implements calculation of exact vblank timestamps from given 
drm_display_mode
  * timings and current video scanout position of a CRTC. This can be directly
  * used as the _driver.get_vblank_timestamp implementation of a kms driver
- * if _driver.get_scanout_position is implemented.
+ * if _crtc_helper_funcs.get_scanout_position is implemented.
  *
  * The current implementation only handles standard video modes. For double 
scan
  * and interlaced modes the driver is supposed to adjust the hardware mode
@@ -632,8 +633,9 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
drm_device *dev,
}
 
/* Scanout position query not supported? Should not happen. */
-   if (!dev->driver->get_scanout_position) {
-   DRM_ERROR("Called from driver w/o get_scanout_position()!?\n");
+   if (!dev->driver->get_scanout_position ||
+   !crtc->helper_private->get_scanout_position) {
+   DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n");
return false;
}
 
@@ -664,11 +666,17 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
drm_device *dev,
 * Get vertical and horizontal scanout position vpos, hpos,
 * and bounding timestamps stime, etime, pre/post query.
 */
-   vbl_status = dev->driver->get_scanout_position(dev, pipe,
-  in_vblank_irq,
-  , ,
-  , ,
-  mode);
+   if (crtc->helper_private->get_scanout_position) {
+   vbl_status =
+   crtc->helper_private->get_scanout_position(
+   crtc, in_vblank_irq, , ,
+   , , mode);
+   } else {
+   vbl_status =
+   dev->driver->get_scanout_position(
+   dev, pipe, in_vblank_irq, ,
+   , , , mode);
+   }
 
/* Return as no-op if scanout query unsupported or failed. */
if (!vbl_status) {
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index cf13470810a5..d0049e5786fc 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -362,11 +362,8 @@ struct drm_driver {
 * True on success, false if a reliable scanout position counter could
 * not be read out.
 *
-* FIXME:
-*
-* Since this is a helper to implement @get_vblank_timestamp, we should
-* move it to  drm_crtc_helper_funcs, like all the other
-* helper-internal hooks.
+* This is deprecated and should not be used by new drivers.
+* Use _crtc_helper_funcs.get_scanout_position instead.
 */
bool (*get_scanout_position) (struct drm_device *dev, unsigned int pipe,
  bool in_vblank_irq, int *vpos, int *hpos,
diff --git a/include/drm/drm_modeset_helper_vtables.h 
b/include/drm/drm_modeset_helper_vtables.h
index 5a87f1bd7a3f..e398512bfd5f 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -450,6 +450,53 @@ struct drm_crtc_helper_funcs {
 */
void (*atomic_disable)(struct drm_crtc *crtc,
   struct drm_crtc_state *old_crtc_state);
+
+   /**
+* @get_scanout_position:
+*
+* Called by vblank timestamping code.
+*
+* Returns the current display scanout position from a CRTC and an
+* optional accurate ktime_get() timestamp of when the position was
+* measured. Note that this is a helper callback which is only used
+* if a driver uses drm_calc_vbltimestamp_from_scanoutpos() for the
+* @drm_driver.get_vblank_timestamp callback.
+*
+* Parameters:
+*
+* crtc:
+* The CRTC.
+