Re: [PATCH v4 15/22] drm/stm: Convert to CRTC VBLANK callbacks

[Philippe CORNU]

Dear Thomas,
Thank you for your patch,
For this stm part,
Acked-by: Philippe Cornu 

Philippe :-)

On 1/23/20 2:59 PM, Thomas Zimmermann wrote:
> VBLANK callbacks in struct drm_driver are deprecated in favor of
> their equivalents in struct drm_crtc_funcs. Convert stm over.
> 
> Signed-off-by: Thomas Zimmermann 
> Tested-by: Yannick Fertré 
> ---
>   drivers/gpu/drm/stm/drv.c  | 1 -
>   drivers/gpu/drm/stm/ltdc.c | 1 +
>   2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index 486985604109..ea9fcbdc68b3 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -72,7 +72,6 @@ static struct drm_driver drv_driver = {
>   .gem_prime_vmap = drm_gem_cma_prime_vmap,
>   .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
>   .gem_prime_mmap = drm_gem_cma_prime_mmap,
> - .get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
>   };
>   
>   static int drv_load(struct drm_device *ddev)
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 8b6d1a2252e3..ee2a8cac59cb 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -722,6 +722,7 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
>   .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>   .enable_vblank = ltdc_crtc_enable_vblank,
>   .disable_vblank = ltdc_crtc_disable_vblank,
> + .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
>   .gamma_set = drm_atomic_helper_legacy_gamma_set,
>   };
>   
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4 14/22] drm/stm: Convert to struct drm_crtc_helper_funcs.get_scanout_position()

[Philippe CORNU]

Dear Thomas,
Thank you for your patch,
For this stm part,
Acked-by: Philippe Cornu 

Philippe :-)

On 1/23/20 2:59 PM, Thomas Zimmermann wrote:
> The callback struct drm_driver.get_scanout_position() is deprecated in
> favor of struct drm_crtc_helper_funcs.get_scanout_position(). Convert stm
> over.
> 
> Signed-off-by: Thomas Zimmermann 
> Tested-by: Yannick Fertré 
> ---
>   drivers/gpu/drm/stm/drv.c  |  1 -
>   drivers/gpu/drm/stm/ltdc.c | 65 --
>   drivers/gpu/drm/stm/ltdc.h |  5 ---
>   3 files changed, 34 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index 5a9f9aca8bc2..486985604109 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -72,7 +72,6 @@ static struct drm_driver drv_driver = {
>   .gem_prime_vmap = drm_gem_cma_prime_vmap,
>   .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
>   .gem_prime_mmap = drm_gem_cma_prime_mmap,
> - .get_scanout_position = ltdc_crtc_scanoutpos,
>   .get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
>   };
>   
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index c2815e8ae1da..8b6d1a2252e3 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -636,38 +636,13 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc 
> *crtc,
>   }
>   }
>   
> -static const struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
> - .mode_valid = ltdc_crtc_mode_valid,
> - .mode_fixup = ltdc_crtc_mode_fixup,
> - .mode_set_nofb = ltdc_crtc_mode_set_nofb,
> - .atomic_flush = ltdc_crtc_atomic_flush,
> - .atomic_enable = ltdc_crtc_atomic_enable,
> - .atomic_disable = ltdc_crtc_atomic_disable,
> -};
> -
> -static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
> -{
> - struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> -
> - DRM_DEBUG_DRIVER("\n");
> - reg_set(ldev->regs, LTDC_IER, IER_LIE);
> -
> - return 0;
> -}
> -
> -static void ltdc_crtc_disable_vblank(struct drm_crtc *crtc)
> -{
> - struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> -
> - DRM_DEBUG_DRIVER("\n");
> - reg_clear(ldev->regs, LTDC_IER, IER_LIE);
> -}
> -
> -bool ltdc_crtc_scanoutpos(struct drm_device *ddev, unsigned int pipe,
> -   bool in_vblank_irq, int *vpos, int *hpos,
> -   ktime_t *stime, ktime_t *etime,
> -   const struct drm_display_mode *mode)
> +static bool ltdc_crtc_get_scanout_position(struct drm_crtc *crtc,
> +bool in_vblank_irq,
> +int *vpos, int *hpos,
> +ktime_t *stime, ktime_t *etime,
> +const struct drm_display_mode *mode)
>   {
> + struct drm_device *ddev = crtc->dev;
>   struct ltdc_device *ldev = ddev->dev_private;
>   int line, vactive_start, vactive_end, vtotal;
>   
> @@ -710,6 +685,34 @@ bool ltdc_crtc_scanoutpos(struct drm_device *ddev, 
> unsigned int pipe,
>   return true;
>   }
>   
> +static const struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
> + .mode_valid = ltdc_crtc_mode_valid,
> + .mode_fixup = ltdc_crtc_mode_fixup,
> + .mode_set_nofb = ltdc_crtc_mode_set_nofb,
> + .atomic_flush = ltdc_crtc_atomic_flush,
> + .atomic_enable = ltdc_crtc_atomic_enable,
> + .atomic_disable = ltdc_crtc_atomic_disable,
> + .get_scanout_position = ltdc_crtc_get_scanout_position,
> +};
> +
> +static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
> +{
> + struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> +
> + DRM_DEBUG_DRIVER("\n");
> + reg_set(ldev->regs, LTDC_IER, IER_LIE);
> +
> + return 0;
> +}
> +
> +static void ltdc_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> + struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> +
> + DRM_DEBUG_DRIVER("\n");
> + reg_clear(ldev->regs, LTDC_IER, IER_LIE);
> +}
> +
>   static const struct drm_crtc_funcs ltdc_crtc_funcs = {
>   .destroy = drm_crtc_cleanup,
>   .set_config = drm_atomic_helper_set_config,
> diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
> index a1ad0ae3b006..c5467d74e707 100644
> --- a/drivers/gpu/drm/stm/ltdc.h
> +++ b/drivers/gpu/drm/stm/ltdc.h
> @@ -39,11 +39,6 @@ struct ltdc_device {
>   struct drm_atomic_state *suspend_state;
>   };
>   
> -bool ltdc_crtc_scanoutpos(struct drm_device *dev, unsigned int pipe,
> -   bool in_vblank_irq, 

Re: [PATCH] drm/gamma: Clarify gamma lut uapi

[Philippe CORNU]

On 3/29/19 10:20 AM, Daniel Vetter wrote:
> Interpreting it as a 0.16 fixed point means we can't accurately
> represent 1.0. Which is one of the values we really should be able to
> represent.
> 
> Since most (all?) luts have lower precision this will only affect
> rounding of 0x.
> 
> Cc: Uma Shankar 
> Cc: Ville Syrjälä 
> Cc: Shashank Sharma 
> Cc: "Kumar, Kiran S" 
> Cc: Kausal Malladi 
> Cc: Lionel Landwerlin 
> Cc: Matt Roper 
> Cc: Rob Bradford 
> Cc: Daniel Stone 
> Cc: Stefan Schake 
> Cc: Eric Anholt 
> Cc: Maarten Lankhorst 
> Cc: Harry Wentland 
> Cc: Leo Li 
> Cc: amd-gfx@lists.freedesktop.org
> Cc: James (Qian) Wang 
> Cc: Liviu Dudau 
> Cc: Mali DP Maintainers 
> Cc: CK Hu 
> Cc: Philipp Zabel 
> Cc: Yannick Fertre 
> Cc: Philippe Cornu 
> Cc: Benjamin Gaignard 
> Cc: Vincent Abriou 
> Cc: Tomi Valkeinen 
> Cc: Boris Brezillon 
> Signed-off-by: Daniel Vetter Signed-off-by: Daniel 
> Vetter 
> ---
>   include/uapi/drm/drm_mode.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 09d72966899a..83cd1636b9be 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -621,7 +621,8 @@ struct drm_color_ctm {
>   
>   struct drm_color_lut {
>   /*
> -  * Data is U0.16 fixed point format.
> +  * Values are mapped linearly to 0.0 - 1.0 range, with 0x0 == 0.0 and
> +  * 0x == 1.0.

for stm,
Reviewed-by: Philippe Cornu 
Many thanks
Philippe :-)

>*/
>   __u16 red;
>   __u16 green;
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 13/14] drm: stm: remove dead code and pointless local lut storage

[Philippe CORNU]

On 06/22/2017 08:06 AM, Peter Rosin wrote:
> The redundant fb helper .load_lut is no longer used, and can not
> work right without also providing the fb helpers .gamma_set and
> .gamma_get thus rendering the code in this driver suspect.
> 

Hi Peter,
STM32 chipsets supports 8-bit CLUT mode but this driver version does not 
support it "yet" (final patch has not been upstreamed because it was a 
too big fbdev patch for simply adding CLUT...).

Regarding your patch below, if it helps you to ease the drm framework 
update then I am agree to "acknowledge it" asap, else if you are not in 
a hurry, I would prefer a better and definitive patch handling 8-bit 
CLUT properly and I am ok to help or/and to do it : )

Extra questions:
- any plan to update modetest with the DRM_FORMAT_C8 support + gamma 
get/set?
- do you have a simple way to test clut with fbdev, last year we where 
using an old version of the SDL but I am still looking for a small piece 
of code to do it (else I will do it myself but C8 on fbdev is not really 
a priority ;-)

best regards,
Philippe

> Just remove the dead code.
> 
> Signed-off-by: Peter Rosin 
> ---
>   drivers/gpu/drm/stm/ltdc.c | 12 
>   drivers/gpu/drm/stm/ltdc.h |  1 -
>   2 files changed, 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 1b9483d..87829b9 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -375,17 +375,6 @@ static irqreturn_t ltdc_irq(int irq, void *arg)
>* DRM_CRTC
>*/
>   
> -static void ltdc_crtc_load_lut(struct drm_crtc *crtc)
> -{
> - struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> - unsigned int i, lay;
> -
> - for (lay = 0; lay < ldev->caps.nb_layers; lay++)
> - for (i = 0; i < 256; i++)
> - reg_write(ldev->regs, LTDC_L1CLUTWR + lay * LAY_OFS,
> -   ldev->clut[i]);
> -}
> -
>   static void ltdc_crtc_enable(struct drm_crtc *crtc)
>   {
>   struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> @@ -523,7 +512,6 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc *crtc,
>   }
>   
>   static struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
> - .load_lut = ltdc_crtc_load_lut,
>   .enable = ltdc_crtc_enable,
>   .disable = ltdc_crtc_disable,
>   .mode_set_nofb = ltdc_crtc_mode_set_nofb,
> diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
> index d7a9c73..620ca55 100644
> --- a/drivers/gpu/drm/stm/ltdc.h
> +++ b/drivers/gpu/drm/stm/ltdc.h
> @@ -27,7 +27,6 @@ struct ltdc_device {
>   struct drm_panel *panel;
>   struct mutex err_lock;  /* protecting error_status */
>   struct ltdc_caps caps;
> - u32 clut[256];  /* color look up table */
>   u32 error_status;
>   u32 irq_status;
>   };
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx