Re: [PATCH 05/35] drm/amd/display: Remove byte swapping for dmcub abm config table

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 04/19, Christian König wrote:
> Am 17.04.20 um 23:43 schrieb Rodrigo Siqueira:
> > Hi,
> > 
> > Wyatt made the below patch for fixing this issue. I can apply it on top
> > of this patchset if you all agree.
> > 
> > [Why]
> > Current code does not guarantee the correct endianness of memory being
> > copied to fw, specifically in the case where cpu isn't little endian.
> > 
> > [How]
> > Windows and Diags are always little endian, so we define a macro that
> > does nothing.  Linux already defines this macro and will do the correct
> > endianness conversion.
> > 
> > Signed-off-by: Wyatt Wood <wyatt.wood@xxxxxxx>
> > Reviewed-by: Harry Wentland <Harry.Wentland@xxxxxxx>
> > Acked-by: Anthony Koo <Anthony.Koo@xxxxxxx>
> > Acked-by: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>
> > ---
> >   .../amd/display/modules/power/power_helpers.c | 58 ++++++++++---------
> >   1 file changed, 31 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/display/modules/power/power_helpers.c b/drivers/gpu/drm/amd/display/modules/power/power_helpers.c
> > index edb446455f6b..8c37bcc27132 100644
> > --- a/drivers/gpu/drm/amd/display/modules/power/power_helpers.c
> > +++ b/drivers/gpu/drm/amd/display/modules/power/power_helpers.c
> > @@ -265,9 +265,11 @@ static void fill_backlight_transform_table_v_2_2(struct dmcu_iram_parameters par
> >   		ASSERT(lut_index < params.backlight_lut_array_size);
> >   		table->backlight_thresholds[i] = (big_endian) ?
> > -			cpu_to_be16(DIV_ROUNDUP((i * 65536), num_entries)) : DIV_ROUNDUP((i * 65536), num_entries);
> > +			cpu_to_be16(DIV_ROUNDUP((i * 65536), num_entries)) :
> > +			cpu_to_le16(DIV_ROUNDUP((i * 65536), num_entries));
> >   		table->backlight_offsets[i] = (big_endian) ?
> > -			cpu_to_be16(params.backlight_lut_array[lut_index]) : params.backlight_lut_array[lut_index];
> > +			cpu_to_be16(params.backlight_lut_array[lut_index]) :
> > +			cpu_to_le16(params.backlight_lut_array[lut_index]);
> >   	}
> >   }
> > @@ -596,7 +598,9 @@ void fill_iram_v_2_3(struct iram_table_v_2_2 *ram_table, struct dmcu_iram_parame
> >   	unsigned int set = params.set;
> >   	ram_table->flags = 0x0;
> > -	ram_table->min_abm_backlight = (big_endian) ? cpu_to_be16(params.min_abm_backlight) : params.min_abm_backlight;
> > +	ram_table->min_abm_backlight = (big_endian) ?
> > +		cpu_to_be16(params.min_abm_backlight) :
> > +		cpu_to_le16(params.min_abm_backlight);
> >   	for (i = 0; i < NUM_AGGR_LEVEL; i++) {
> >   		ram_table->hybrid_factor[i] = abm_settings[set][i].brightness_gain;
> > @@ -620,30 +624,30 @@ void fill_iram_v_2_3(struct iram_table_v_2_2 *ram_table, struct dmcu_iram_parame
> >   	ram_table->iir_curve[4] = 0x65;
> >   	//Gamma 2.2
> > -	ram_table->crgb_thresh[0] = (big_endian) ? cpu_to_be16(0x127c) : 0x127c;
> > -	ram_table->crgb_thresh[1] = (big_endian) ? cpu_to_be16(0x151b) : 0x151b;
> > -	ram_table->crgb_thresh[2] = (big_endian) ? cpu_to_be16(0x17d5) : 0x17d5;
> > -	ram_table->crgb_thresh[3] = (big_endian) ? cpu_to_be16(0x1a56) : 0x1a56;
> > -	ram_table->crgb_thresh[4] = (big_endian) ? cpu_to_be16(0x1c83) : 0x1c83;
> > -	ram_table->crgb_thresh[5] = (big_endian) ? cpu_to_be16(0x1e72) : 0x1e72;
> > -	ram_table->crgb_thresh[6] = (big_endian) ? cpu_to_be16(0x20f0) : 0x20f0;
> > -	ram_table->crgb_thresh[7] = (big_endian) ? cpu_to_be16(0x232b) : 0x232b;
> > -	ram_table->crgb_offset[0] = (big_endian) ? cpu_to_be16(0x2999) : 0x2999;
> > -	ram_table->crgb_offset[1] = (big_endian) ? cpu_to_be16(0x3999) : 0x3999;
> > -	ram_table->crgb_offset[2] = (big_endian) ? cpu_to_be16(0x4666) : 0x4666;
> > -	ram_table->crgb_offset[3] = (big_endian) ? cpu_to_be16(0x5999) : 0x5999;
> > -	ram_table->crgb_offset[4] = (big_endian) ? cpu_to_be16(0x6333) : 0x6333;
> > -	ram_table->crgb_offset[5] = (big_endian) ? cpu_to_be16(0x7800) : 0x7800;
> > -	ram_table->crgb_offset[6] = (big_endian) ? cpu_to_be16(0x8c00) : 0x8c00;
> > -	ram_table->crgb_offset[7] = (big_endian) ? cpu_to_be16(0xa000) : 0xa000;
> > -	ram_table->crgb_slope[0]  = (big_endian) ? cpu_to_be16(0x3609) : 0x3609;
> > -	ram_table->crgb_slope[1]  = (big_endian) ? cpu_to_be16(0x2dfa) : 0x2dfa;
> > -	ram_table->crgb_slope[2]  = (big_endian) ? cpu_to_be16(0x27ea) : 0x27ea;
> > -	ram_table->crgb_slope[3]  = (big_endian) ? cpu_to_be16(0x235d) : 0x235d;
> > -	ram_table->crgb_slope[4]  = (big_endian) ? cpu_to_be16(0x2042) : 0x2042;
> > -	ram_table->crgb_slope[5]  = (big_endian) ? cpu_to_be16(0x1dc3) : 0x1dc3;
> > -	ram_table->crgb_slope[6]  = (big_endian) ? cpu_to_be16(0x1b1a) : 0x1b1a;
> > -	ram_table->crgb_slope[7]  = (big_endian) ? cpu_to_be16(0x1910) : 0x1910;
> > +	ram_table->crgb_thresh[0] = (big_endian) ? cpu_to_be16(0x127c) : cpu_to_le16(0x127c);
> > +	ram_table->crgb_thresh[1] = (big_endian) ? cpu_to_be16(0x151b) : cpu_to_le16(0x151b);
> > +	ram_table->crgb_thresh[2] = (big_endian) ? cpu_to_be16(0x17d5) : cpu_to_le16(0x17d5);
> > +	ram_table->crgb_thresh[3] = (big_endian) ? cpu_to_be16(0x1a56) : cpu_to_le16(0x1a56);
> > +	ram_table->crgb_thresh[4] = (big_endian) ? cpu_to_be16(0x1c83) : cpu_to_le16(0x1c83);
> > +	ram_table->crgb_thresh[5] = (big_endian) ? cpu_to_be16(0x1e72) : cpu_to_le16(0x1e72);
> > +	ram_table->crgb_thresh[6] = (big_endian) ? cpu_to_be16(0x20f0) : cpu_to_le16(0x20f0);
> > +	ram_table->crgb_thresh[7] = (big_endian) ? cpu_to_be16(0x232b) : cpu_to_le16(0x232b);
> > +	ram_table->crgb_offset[0] = (big_endian) ? cpu_to_be16(0x2999) : cpu_to_le16(0x2999);
> > +	ram_table->crgb_offset[1] = (big_endian) ? cpu_to_be16(0x3999) : cpu_to_le16(0x3999);
> > +	ram_table->crgb_offset[2] = (big_endian) ? cpu_to_be16(0x4666) : cpu_to_le16(0x4666);
> > +	ram_table->crgb_offset[3] = (big_endian) ? cpu_to_be16(0x5999) : cpu_to_le16(0x5999);
> > +	ram_table->crgb_offset[4] = (big_endian) ? cpu_to_be16(0x6333) : cpu_to_le16(0x6333);
> > +	ram_table->crgb_offset[5] = (big_endian) ? cpu_to_be16(0x7800) : cpu_to_le16(0x7800);
> > +	ram_table->crgb_offset[6] = (big_endian) ? cpu_to_be16(0x8c00) : cpu_to_le16(0x8c00);
> > +	ram_table->crgb_offset[7] = (big_endian) ? cpu_to_be16(0xa000) : cpu_to_le16(0xa000);
> > +	ram_table->crgb_slope[0]  = (big_endian) ? cpu_to_be16(0x3609) : cpu_to_le16(0x3609);
> > +	ram_table->crgb_slope[1]  = (big_endian) ? cpu_to_be16(0x2dfa) : cpu_to_le16(0x2dfa);
> > +	ram_table->crgb_slope[2]  = (big_endian) ? cpu_to_be16(0x27ea) : cpu_to_le16(0x27ea);
> > +	ram_table->crgb_slope[3]  = (big_endian) ? cpu_to_be16(0x235d) : cpu_to_le16(0x235d);
> > +	ram_table->crgb_slope[4]  = (big_endian) ? cpu_to_be16(0x2042) : cpu_to_le16(0x2042);
> > +	ram_table->crgb_slope[5]  = (big_endian) ? cpu_to_be16(0x1dc3) : cpu_to_le16(0x1dc3);
> > +	ram_table->crgb_slope[6]  = (big_endian) ? cpu_to_be16(0x1b1a) : cpu_to_le16(0x1b1a);
> > +	ram_table->crgb_slope[7]  = (big_endian) ? cpu_to_be16(0x1910) : cpu_to_le16(0x1910);
> 
> That you have to duplicate the values is rather ugly here.
> 
> Since this is all in one file maybe come up with a helper for this? E.g.
> conditional_bswap16(big_endian, value)

I'm going to prepare a patch with your suggestion.

Thanks
 
> Regards,
> Christian.
> 
> >   	fill_backlight_transform_table_v_2_2(
> >   			params, ram_table, big_endian);
> 

-- 
Rodrigo Siqueira
https://siqueira.tech

Attachment: signature.asc
Description: PGP signature

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux