[PATCH v3 1/2] drm: automatic legacy gamma support

[Tomi Valkeinen]

On 10/12/2020 17:27, Daniel Vetter wrote:

>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index e82db0f4e771..80e3797f0f01 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -46,6 +46,7 @@
>>  #include <drm/drm_print.h>
>>  #include <drm/drm_vblank.h>
>>  
>> +#include "drm_crtc_internal.h"
> 
> So this is a bit annoying, because thus far we managed to have a very
> clear split between core and helpers. And I think we can keep that.
> 
>>  #include "drm_crtc_helper_internal.h"
>>  #include "drm_internal.h"
>>  
>> @@ -136,15 +137,15 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
>>  {
>>  	uint16_t *r_base, *g_base, *b_base;
>>  
>> -	if (crtc->funcs->gamma_set == NULL)
>> +	if (!drm_crtc_supports_legacy_gamma(crtc))
>>  		return;
>>  
>>  	r_base = crtc->gamma_store;
>>  	g_base = r_base + crtc->gamma_size;
>>  	b_base = g_base + crtc->gamma_size;
>>  
>> -	crtc->funcs->gamma_set(crtc, r_base, g_base, b_base,
>> -			       crtc->gamma_size, NULL);
>> +	drm_crtc_legacy_gamma_set(crtc, r_base, g_base, b_base,
>> +				  crtc->gamma_size, NULL);
> 
> This is only used by legacy non-atomic drivers. It's pretty much
> impossible to make kgdb work with atomic drivers, so really let's just not
> bother and keep the code as-is.

You're right.

>>  }
>>  
>>  /**
>> @@ -946,7 +947,7 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
>>  	drm_modeset_lock_all(fb_helper->dev);
>>  	drm_client_for_each_modeset(modeset, &fb_helper->client) {
>>  		crtc = modeset->crtc;
>> -		if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
>> +		if (!drm_crtc_supports_legacy_gamma(crtc)) {
>>  			ret = -EINVAL;
>>  			goto out;
>>  		}
>> @@ -964,8 +965,8 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
>>  		memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g));
>>  		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
>>  
>> -		ret = crtc->funcs->gamma_set(crtc, r, g, b,
>> -					     crtc->gamma_size, NULL);
>> +		ret = drm_crtc_legacy_gamma_set(crtc, r, g, b, crtc->gamma_size,
>> +						NULL);
>>  		if (ret)
>>  			goto out;
> 
> Same here.

Yep.

>>  	}
>> @@ -1024,12 +1025,10 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>>  	struct drm_device *dev = fb_helper->dev;
>>  	struct drm_property_blob *gamma_lut = NULL;
>>  	struct drm_modeset_acquire_ctx ctx;
>> -	struct drm_crtc_state *crtc_state;
>>  	struct drm_atomic_state *state;
>>  	struct drm_mode_set *modeset;
>>  	struct drm_crtc *crtc;
>>  	u16 *r, *g, *b;
>> -	bool replaced;
>>  	int ret = 0;
>>  
>>  	drm_modeset_acquire_init(&ctx, 0);
>> @@ -1053,18 +1052,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>>  			goto out_state;
>>  		}
>>  
>> -		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>> -		if (IS_ERR(crtc_state)) {
>> -			ret = PTR_ERR(crtc_state);
>> +		ret = drm_crtc_gamma_ramp_set(state, crtc, gamma_lut);
>> +		if (ret)
> 
> You're nesting an atomic commit in an atomic commit here, that will go
> boom. I guess ideally we'd move this into drm_client_modeset so it
> remembers the fbdev gamma ramp and does it all in one go. Otherwise I
> guess you need some kind of different helper, not sure what.

What do you mean? Are you mixing drm_crtc_legacy_gamma_set with drm_crtc_gamma_ramp_set (yeah, I
didn't quite know how to name the latter one...)? drm_crtc_gamma_ramp_set does the same thing as the
removed code, it sets the gamma_lut in to the state. It doesn't commit.

drm_crtc_gamma_ramp_set does a "setup the state so that this gamma ramp will be on screen", which
means setting/clearing GAMMA_LUT, DEGAMMA_LUT and CTM. I wanted to have that logic in one place,
which means we need to export it from drm.ko.

I could just inline drm_crtc_gamma_ramp_set, but then I need drm_mode_obj_find_prop_id, which is
again not exported. I could also inline drm_mode_obj_find_prop_id as it's trivial enough loop. But
this sounds uglier than exporting a function.

Personally, I don't remember when I have used fbdev the last time (other than simple tests), and I
could as well just leave the code here as it is. I have no idea if this fbdev setcmap is a big
feature that has to function also with HW that only has a pre-gamma table.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki