[PATCH] drm/mgag200: Fix gamma lut not initialized for G200ER, G200EV, G200SE

[Jocelyn Falempe]

On 18/12/2023 12:31, Thomas Zimmermann wrote:
> Hi
> 
> Am 14.12.23 um 17:38 schrieb Jocelyn Falempe:
>> When mgag200 switched from simple KMS to regular atomic helpers,
>> the initialization of the gamma settings was lost.
>> This leads to a black screen, if the bios/uefi doesn't use the same
>> pixel color depth.
>> This has been fixed with commit ad81e23426a6 ("drm/mgag200: Fix gamma
>> lut not initialized.") for most G200, but G200ER, G200EV, G200SE use
>> their own version of crtc_helper_atomic_enable() and need to be fixed
>> too.
>>
>> Fixes: 1baf9127c482 ("drm/mgag200: Replace simple-KMS with regular 
>> atomic helpers")
>> Cc: <stable at vger.kernel.org> #v6.1+
>> Reported-by: Roger Sewell <roger.sewell at cantab.net>
>> Suggested-by: Roger Sewell <roger.sewell at cantab.net>
>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_drv.h    |  4 ++++
>>   drivers/gpu/drm/mgag200/mgag200_g200er.c |  2 ++
>>   drivers/gpu/drm/mgag200/mgag200_g200ev.c |  2 ++
>>   drivers/gpu/drm/mgag200/mgag200_g200se.c |  2 ++
>>   drivers/gpu/drm/mgag200/mgag200_mode.c   | 26 ++++++++++++++----------
>>   5 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h 
>> b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 57c7edcab602..ed90a92b5fcd 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -392,6 +392,10 @@ void 
>> mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane,
>>       .destroy = drm_plane_cleanup, \
>>       DRM_GEM_SHADOW_PLANE_FUNCS
>> +void mgag200_crtc_set_gamma(struct mga_device *mdev,
>> +                const struct drm_format_info *format,
>> +                struct drm_property_blob *gamma_lut);
>> +
>>   enum drm_mode_status mgag200_crtc_helper_mode_valid(struct drm_crtc 
>> *crtc,
>>                               const struct drm_display_mode *mode);
>>   int mgag200_crtc_helper_atomic_check(struct drm_crtc *crtc, struct 
>> drm_atomic_state *new_state);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c 
>> b/drivers/gpu/drm/mgag200/mgag200_g200er.c
>> index bce267e0f7de..38815cb94c61 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
>> @@ -202,6 +202,8 @@ static void 
>> mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>>       mgag200_g200er_reset_tagfifo(mdev);
>> +    mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut);
>> +
>>       mgag200_enable_display(mdev);
>>       if (funcs->enable_vidrst)
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c 
>> b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
>> index ac957f42abe1..e698a3a499bf 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
>> @@ -203,6 +203,8 @@ static void 
>> mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>>       mgag200_g200ev_set_hiprilvl(mdev);
>> +    mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut);
>> +
>>       mgag200_enable_display(mdev);
>>       if (funcs->enable_vidrst)
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c 
>> b/drivers/gpu/drm/mgag200/mgag200_g200se.c
>> index bd6e573c9a1a..7e4ea0046a6b 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
>> @@ -334,6 +334,8 @@ static void 
>> mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>>       mgag200_g200se_set_hiprilvl(mdev, adjusted_mode, format);
>> +    mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut);
>> +
>>       mgag200_enable_display(mdev);
>>       if (funcs->enable_vidrst)
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c 
>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index af3ce5a6a636..d2a04b317232 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -65,9 +65,9 @@ static void mgag200_crtc_set_gamma_linear(struct 
>> mga_device *mdev,
>>       }
>>   }
>> -static void mgag200_crtc_set_gamma(struct mga_device *mdev,
>> -                   const struct drm_format_info *format,
>> -                   struct drm_color_lut *lut)
>> +static void mgag200_crtc_set_gamma_table(struct mga_device *mdev,
>> +                     const struct drm_format_info *format,
>> +                     struct drm_color_lut *lut)
>>   {
>>       int i;
>> @@ -103,6 +103,16 @@ static void mgag200_crtc_set_gamma(struct 
>> mga_device *mdev,
>>       }
>>   }
>> +void mgag200_crtc_set_gamma(struct mga_device *mdev,
>> +                const struct drm_format_info *format,
>> +                struct drm_property_blob *gamma_lut)
>> +{
>> +    if (gamma_lut)
>> +        mgag200_crtc_set_gamma_table(mdev, format, gamma_lut->data);
>> +    else
>> +        mgag200_crtc_set_gamma_linear(mdev, format);
>> +}
> 
> Please keep this open-coded its callers. With that changed
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann at suse.de>

Thanks for the review, yes I will change that.
If no other comments, I will push it to drm-misc-fixes tomorrow.

-- 

Jocelyn
> 
>> +
>>   static inline void mga_wait_vsync(struct mga_device *mdev)
>>   {
>>       unsigned long timeout = jiffies + HZ/10;
>> @@ -616,10 +626,7 @@ void mgag200_crtc_helper_atomic_flush(struct 
>> drm_crtc *crtc, struct drm_atomic_s
>>       if (crtc_state->enable && crtc_state->color_mgmt_changed) {
>>           const struct drm_format_info *format = 
>> mgag200_crtc_state->format;
>> -        if (crtc_state->gamma_lut)
>> -            mgag200_crtc_set_gamma(mdev, format, 
>> crtc_state->gamma_lut->data);
>> -        else
>> -            mgag200_crtc_set_gamma_linear(mdev, format);
>> +        mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut);
>>       }
>>   }
>> @@ -642,10 +649,7 @@ void mgag200_crtc_helper_atomic_enable(struct 
>> drm_crtc *crtc, struct drm_atomic_
>>       if (funcs->pixpllc_atomic_update)
>>           funcs->pixpllc_atomic_update(crtc, old_state);
>> -    if (crtc_state->gamma_lut)
>> -        mgag200_crtc_set_gamma(mdev, format, 
>> crtc_state->gamma_lut->data);
>> -    else
>> -        mgag200_crtc_set_gamma_linear(mdev, format);
>> +    mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut);
>>       mgag200_enable_display(mdev);
>>
>> base-commit: 6c9dbee84cd005bed5f9d07b3a2797ae6414b435
>