| Message ID | 20191119144526.31797-1-mikita.lipski@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/dsc: Return unsigned long on compute offset | expand |
On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lipski@amd.com wrote: > From: Mikita Lipski <mikita.lipski@amd.com> > > We shouldn't compare int with unsigned long to find the max value > and since we are not expecting negative value returned from > compute_offset we should make this function return unsigned long > so we can compare the values when computing rc parameters. Why are there other unsigned longs in dsc parameter computation in the first place? > > Cc: Nikola Cornij <nikola.cornij@amd.com> > Cc: Harry Wentland <harry.wentland@amd.com> > Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > --- > drivers/gpu/drm/drm_dsc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c > index 74f3527f567d..ec40604ab6a2 100644 > --- a/drivers/gpu/drm/drm_dsc.c > +++ b/drivers/gpu/drm/drm_dsc.c > @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload, > } > EXPORT_SYMBOL(drm_dsc_pps_payload_pack); > > -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, > +static unsigned long compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, > int groups_per_line, int grpcnt) > { > - int offset = 0; > - int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); > + unsigned long offset = 0; > + unsigned long grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); > > if (grpcnt <= grpcnt_id) > offset = DIV_ROUND_UP(grpcnt * pixels_per_group * vdsc_cfg->bits_per_pixel, 16); > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 19/11/2019 09:56, Ville Syrjälä wrote: > On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lipski@amd.com wrote: >> From: Mikita Lipski <mikita.lipski@amd.com> >> >> We shouldn't compare int with unsigned long to find the max value >> and since we are not expecting negative value returned from >> compute_offset we should make this function return unsigned long >> so we can compare the values when computing rc parameters. > > Why are there other unsigned longs in dsc parameter computation > in the first place? I believe it was initially set to be unsigned long for variable consistency, when we ported intel_compute_rc_parameters into drm_dsc_compute_rc_parameters. But now that I look at it, we can actually just set them to u32 or u64, as nothing should exceed that. > >> >> Cc: Nikola Cornij <nikola.cornij@amd.com> >> Cc: Harry Wentland <harry.wentland@amd.com> >> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> >> --- >> drivers/gpu/drm/drm_dsc.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c >> index 74f3527f567d..ec40604ab6a2 100644 >> --- a/drivers/gpu/drm/drm_dsc.c >> +++ b/drivers/gpu/drm/drm_dsc.c >> @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload, >> } >> EXPORT_SYMBOL(drm_dsc_pps_payload_pack); >> >> -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, >> +static unsigned long compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, >> int groups_per_line, int grpcnt) >> { >> - int offset = 0; >> - int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); >> + unsigned long offset = 0; >> + unsigned long grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); >> >> if (grpcnt <= grpcnt_id) >> offset = DIV_ROUND_UP(grpcnt * pixels_per_group * vdsc_cfg->bits_per_pixel, 16); >> -- >> 2.17.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
If you're going to make all of them the same, then u64, please. This is because I'm not sure if calculations require 64-bit at some stage. -----Original Message----- From: Lipski, Mikita <Mikita.Lipski@amd.com> Sent: November 19, 2019 10:08 AM To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Lipski, Mikita <Mikita.Lipski@amd.com> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Cornij, Nikola <Nikola.Cornij@amd.com> Subject: Re: [PATCH] drm/dsc: Return unsigned long on compute offset On 19/11/2019 09:56, Ville Syrjälä wrote: > On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lipski@amd.com wrote: >> From: Mikita Lipski <mikita.lipski@amd.com> >> >> We shouldn't compare int with unsigned long to find the max value and >> since we are not expecting negative value returned from >> compute_offset we should make this function return unsigned long so >> we can compare the values when computing rc parameters. > > Why are there other unsigned longs in dsc parameter computation in the > first place? I believe it was initially set to be unsigned long for variable consistency, when we ported intel_compute_rc_parameters into drm_dsc_compute_rc_parameters. But now that I look at it, we can actually just set them to u32 or u64, as nothing should exceed that. > >> >> Cc: Nikola Cornij <nikola.cornij@amd.com> >> Cc: Harry Wentland <harry.wentland@amd.com> >> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> >> --- >> drivers/gpu/drm/drm_dsc.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c >> index 74f3527f567d..ec40604ab6a2 100644 >> --- a/drivers/gpu/drm/drm_dsc.c >> +++ b/drivers/gpu/drm/drm_dsc.c >> @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload, >> } >> EXPORT_SYMBOL(drm_dsc_pps_payload_pack); >> >> -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, >> +static unsigned long compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, >> int groups_per_line, int grpcnt) >> { >> - int offset = 0; >> - int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); >> + unsigned long offset = 0; >> + unsigned long grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); >> >> if (grpcnt <= grpcnt_id) >> offset = DIV_ROUND_UP(grpcnt * pixels_per_group * vdsc_cfg->bits_per_pixel, 16); >> -- >> 2.17.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Tue, Nov 19, 2019 at 04:59:40PM +0000, Cornij, Nikola wrote: > If you're going to make all of them the same, then u64, please. > > This is because I'm not sure if calculations require 64-bit at some stage. If it does then it's already broken. Someone should probably figure out what's actally needed instead of shooting ducks with an icbm. > > -----Original Message----- > From: Lipski, Mikita <Mikita.Lipski@amd.com> > Sent: November 19, 2019 10:08 AM > To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Lipski, Mikita <Mikita.Lipski@amd.com> > Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Cornij, Nikola <Nikola.Cornij@amd.com> > Subject: Re: [PATCH] drm/dsc: Return unsigned long on compute offset > > > > On 19/11/2019 09:56, Ville Syrjälä wrote: > > On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lipski@amd.com wrote: > >> From: Mikita Lipski <mikita.lipski@amd.com> > >> > >> We shouldn't compare int with unsigned long to find the max value and > >> since we are not expecting negative value returned from > >> compute_offset we should make this function return unsigned long so > >> we can compare the values when computing rc parameters. > > > > Why are there other unsigned longs in dsc parameter computation in the > > first place? > > I believe it was initially set to be unsigned long for variable consistency, when we ported intel_compute_rc_parameters into drm_dsc_compute_rc_parameters. But now that I look at it, we can actually just set them to u32 or u64, as nothing should exceed that. > > > >> > >> Cc: Nikola Cornij <nikola.cornij@amd.com> > >> Cc: Harry Wentland <harry.wentland@amd.com> > >> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > >> --- > >> drivers/gpu/drm/drm_dsc.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c > >> index 74f3527f567d..ec40604ab6a2 100644 > >> --- a/drivers/gpu/drm/drm_dsc.c > >> +++ b/drivers/gpu/drm/drm_dsc.c > >> @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload, > >> } > >> EXPORT_SYMBOL(drm_dsc_pps_payload_pack); > >> > >> -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, > >> +static unsigned long compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, > >> int groups_per_line, int grpcnt) > >> { > >> - int offset = 0; > >> - int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); > >> + unsigned long offset = 0; > >> + unsigned long grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); > >> > >> if (grpcnt <= grpcnt_id) > >> offset = DIV_ROUND_UP(grpcnt * pixels_per_group * vdsc_cfg->bits_per_pixel, 16); > >> -- > >> 2.17.1 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Thanks, > Mikita Lipski > Software Engineer 2, AMD > mikita.lipski@amd.com
On 19/11/2019 12:11, Ville Syrjälä wrote: > On Tue, Nov 19, 2019 at 04:59:40PM +0000, Cornij, Nikola wrote: >> If you're going to make all of them the same, then u64, please. >> >> This is because I'm not sure if calculations require 64-bit at some stage. > > If it does then it's already broken. Someone should probably figure out > what's actally needed instead of shooting ducks with an icbm. > I don't think it is not broken, cause I'm currently testing DSC. The patch I sent early simply fixes the error of comparing signed and unsigned variables. We can then submit a second patch addressing the issue of using unsigned long int instead of u32. Also, since the variables in drm_dsc_config structure are all of type u8 and u16, the calculation values shouldn't exceed the size of u32. Thanks >> >> -----Original Message----- >> From: Lipski, Mikita <Mikita.Lipski@amd.com> >> Sent: November 19, 2019 10:08 AM >> To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Lipski, Mikita <Mikita.Lipski@amd.com> >> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Cornij, Nikola <Nikola.Cornij@amd.com> >> Subject: Re: [PATCH] drm/dsc: Return unsigned long on compute offset >> >> >> >> On 19/11/2019 09:56, Ville Syrjälä wrote: >>> On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lipski@amd.com wrote: >>>> From: Mikita Lipski <mikita.lipski@amd.com> >>>> >>>> We shouldn't compare int with unsigned long to find the max value and >>>> since we are not expecting negative value returned from >>>> compute_offset we should make this function return unsigned long so >>>> we can compare the values when computing rc parameters. >>> >>> Why are there other unsigned longs in dsc parameter computation in the >>> first place? >> >> I believe it was initially set to be unsigned long for variable consistency, when we ported intel_compute_rc_parameters into drm_dsc_compute_rc_parameters. But now that I look at it, we can actually just set them to u32 or u64, as nothing should exceed that. >>> >>>> >>>> Cc: Nikola Cornij <nikola.cornij@amd.com> >>>> Cc: Harry Wentland <harry.wentland@amd.com> >>>> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> >>>> --- >>>> drivers/gpu/drm/drm_dsc.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c >>>> index 74f3527f567d..ec40604ab6a2 100644 >>>> --- a/drivers/gpu/drm/drm_dsc.c >>>> +++ b/drivers/gpu/drm/drm_dsc.c >>>> @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload, >>>> } >>>> EXPORT_SYMBOL(drm_dsc_pps_payload_pack); >>>> >>>> -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, >>>> +static unsigned long compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, >>>> int groups_per_line, int grpcnt) >>>> { >>>> - int offset = 0; >>>> - int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); >>>> + unsigned long offset = 0; >>>> + unsigned long grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); >>>> >>>> if (grpcnt <= grpcnt_id) >>>> offset = DIV_ROUND_UP(grpcnt * pixels_per_group * vdsc_cfg->bits_per_pixel, 16); >>>> -- >>>> 2.17.1 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >> >> -- >> Thanks, >> Mikita Lipski >> Software Engineer 2, AMD >> mikita.lipski@amd.com >
On 19/11/2019 16:09, Mikita Lipski wrote: > > > On 19/11/2019 12:11, Ville Syrjälä wrote: >> On Tue, Nov 19, 2019 at 04:59:40PM +0000, Cornij, Nikola wrote: >>> If you're going to make all of them the same, then u64, please. >>> >>> This is because I'm not sure if calculations require 64-bit at some >>> stage. >> >> If it does then it's already broken. Someone should probably figure out >> what's actally needed instead of shooting ducks with an icbm. >> Sorry made a type below. Supposed to be "I don't think it is broken" > I don't think it is not broken, cause I'm currently testing DSC. > The patch I sent early simply fixes the error of comparing signed and > unsigned variables. > > We can then submit a second patch addressing the issue of using unsigned > long int instead of u32. Also, since the variables in drm_dsc_config > structure are all of type u8 and u16, the calculation values shouldn't > exceed the size of u32. > > Thanks > >>> >>> -----Original Message----- >>> From: Lipski, Mikita <Mikita.Lipski@amd.com> >>> Sent: November 19, 2019 10:08 AM >>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Lipski, Mikita >>> <Mikita.Lipski@amd.com> >>> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; >>> Cornij, Nikola <Nikola.Cornij@amd.com> >>> Subject: Re: [PATCH] drm/dsc: Return unsigned long on compute offset >>> >>> >>> >>> On 19/11/2019 09:56, Ville Syrjälä wrote: >>>> On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lipski@amd.com wrote: >>>>> From: Mikita Lipski <mikita.lipski@amd.com> >>>>> >>>>> We shouldn't compare int with unsigned long to find the max value and >>>>> since we are not expecting negative value returned from >>>>> compute_offset we should make this function return unsigned long so >>>>> we can compare the values when computing rc parameters. >>>> >>>> Why are there other unsigned longs in dsc parameter computation in the >>>> first place? >>> >>> I believe it was initially set to be unsigned long for variable >>> consistency, when we ported intel_compute_rc_parameters into >>> drm_dsc_compute_rc_parameters. But now that I look at it, we can >>> actually just set them to u32 or u64, as nothing should exceed that. >>>> >>>>> >>>>> Cc: Nikola Cornij <nikola.cornij@amd.com> >>>>> Cc: Harry Wentland <harry.wentland@amd.com> >>>>> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> >>>>> --- >>>>> drivers/gpu/drm/drm_dsc.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c >>>>> index 74f3527f567d..ec40604ab6a2 100644 >>>>> --- a/drivers/gpu/drm/drm_dsc.c >>>>> +++ b/drivers/gpu/drm/drm_dsc.c >>>>> @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct >>>>> drm_dsc_picture_parameter_set *pps_payload, >>>>> } >>>>> EXPORT_SYMBOL(drm_dsc_pps_payload_pack); >>>>> -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int >>>>> pixels_per_group, >>>>> +static unsigned long compute_offset(struct drm_dsc_config >>>>> *vdsc_cfg, int pixels_per_group, >>>>> int groups_per_line, int grpcnt) >>>>> { >>>>> - int offset = 0; >>>>> - int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, >>>>> pixels_per_group); >>>>> + unsigned long offset = 0; >>>>> + unsigned long grpcnt_id = >>>>> DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); >>>>> if (grpcnt <= grpcnt_id) >>>>> offset = DIV_ROUND_UP(grpcnt * pixels_per_group * >>>>> vdsc_cfg->bits_per_pixel, 16); >>>>> -- >>>>> 2.17.1 >>>>> >>>>> _______________________________________________ >>>>> dri-devel mailing list >>>>> dri-devel@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>> >>> >>> -- >>> Thanks, >>> Mikita Lipski >>> Software Engineer 2, AMD >>> mikita.lipski@amd.com >> >
On Tue, Nov 19, 2019 at 04:11:43PM -0500, Mikita Lipski wrote: > > > On 19/11/2019 16:09, Mikita Lipski wrote: > > > > > > On 19/11/2019 12:11, Ville Syrjälä wrote: > >> On Tue, Nov 19, 2019 at 04:59:40PM +0000, Cornij, Nikola wrote: > >>> If you're going to make all of them the same, then u64, please. > >>> > >>> This is because I'm not sure if calculations require 64-bit at some > >>> stage. > >> > >> If it does then it's already broken. Someone should probably figure out > >> what's actally needed instead of shooting ducks with an icbm. > >> > > > Sorry made a type below. Supposed to be "I don't think it is broken" I mean that it's broken if it actually needs u64 when it's currently using unsigned long. So u64 is either overkill or the code is currently broken. > > > I don't think it is not broken, cause I'm currently testing DSC. > > The patch I sent early simply fixes the error of comparing signed and > > unsigned variables. > > > > We can then submit a second patch addressing the issue of using unsigned > > long int instead of u32. Also, since the variables in drm_dsc_config > > structure are all of type u8 and u16, the calculation values shouldn't > > exceed the size of u32. > > > > Thanks > > > >>> > >>> -----Original Message----- > >>> From: Lipski, Mikita <Mikita.Lipski@amd.com> > >>> Sent: November 19, 2019 10:08 AM > >>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Lipski, Mikita > >>> <Mikita.Lipski@amd.com> > >>> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > >>> Cornij, Nikola <Nikola.Cornij@amd.com> > >>> Subject: Re: [PATCH] drm/dsc: Return unsigned long on compute offset > >>> > >>> > >>> > >>> On 19/11/2019 09:56, Ville Syrjälä wrote: > >>>> On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lipski@amd.com wrote: > >>>>> From: Mikita Lipski <mikita.lipski@amd.com> > >>>>> > >>>>> We shouldn't compare int with unsigned long to find the max value and > >>>>> since we are not expecting negative value returned from > >>>>> compute_offset we should make this function return unsigned long so > >>>>> we can compare the values when computing rc parameters. > >>>> > >>>> Why are there other unsigned longs in dsc parameter computation in the > >>>> first place? > >>> > >>> I believe it was initially set to be unsigned long for variable > >>> consistency, when we ported intel_compute_rc_parameters into > >>> drm_dsc_compute_rc_parameters. But now that I look at it, we can > >>> actually just set them to u32 or u64, as nothing should exceed that. > >>>> > >>>>> > >>>>> Cc: Nikola Cornij <nikola.cornij@amd.com> > >>>>> Cc: Harry Wentland <harry.wentland@amd.com> > >>>>> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > >>>>> --- > >>>>> drivers/gpu/drm/drm_dsc.c | 6 +++--- > >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c > >>>>> index 74f3527f567d..ec40604ab6a2 100644 > >>>>> --- a/drivers/gpu/drm/drm_dsc.c > >>>>> +++ b/drivers/gpu/drm/drm_dsc.c > >>>>> @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct > >>>>> drm_dsc_picture_parameter_set *pps_payload, > >>>>> } > >>>>> EXPORT_SYMBOL(drm_dsc_pps_payload_pack); > >>>>> -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int > >>>>> pixels_per_group, > >>>>> +static unsigned long compute_offset(struct drm_dsc_config > >>>>> *vdsc_cfg, int pixels_per_group, > >>>>> int groups_per_line, int grpcnt) > >>>>> { > >>>>> - int offset = 0; > >>>>> - int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, > >>>>> pixels_per_group); > >>>>> + unsigned long offset = 0; > >>>>> + unsigned long grpcnt_id = > >>>>> DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); > >>>>> if (grpcnt <= grpcnt_id) > >>>>> offset = DIV_ROUND_UP(grpcnt * pixels_per_group * > >>>>> vdsc_cfg->bits_per_pixel, 16); > >>>>> -- > >>>>> 2.17.1 > >>>>> > >>>>> _______________________________________________ > >>>>> dri-devel mailing list > >>>>> dri-devel@lists.freedesktop.org > >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >>>> > >>> > >>> -- > >>> Thanks, > >>> Mikita Lipski > >>> Software Engineer 2, AMD > >>> mikita.lipski@amd.com > >> > > > > -- > Thanks, > Mikita Lipski > Software Engineer 2, AMD > mikita.lipski@amd.com
On 20/11/2019 05:17, Ville Syrjälä wrote: > On Tue, Nov 19, 2019 at 04:11:43PM -0500, Mikita Lipski wrote: >> >> >> On 19/11/2019 16:09, Mikita Lipski wrote: >>> >>> >>> On 19/11/2019 12:11, Ville Syrjälä wrote: >>>> On Tue, Nov 19, 2019 at 04:59:40PM +0000, Cornij, Nikola wrote: >>>>> If you're going to make all of them the same, then u64, please. >>>>> >>>>> This is because I'm not sure if calculations require 64-bit at some >>>>> stage. >>>> >>>> If it does then it's already broken. Someone should probably figure out >>>> what's actally needed instead of shooting ducks with an icbm. >>>> >> >> >> Sorry made a type below. Supposed to be "I don't think it is broken" > > I mean that it's broken if it actually needs u64 when it's > currently using unsigned long. So u64 is either overkill or the > code is currently broken. > None of the calculations exceed u32, so u64 would be an overkill, since none of the variables in the structure exceed 16 bits. Therefore u32 is enough. >> >>> I don't think it is not broken, cause I'm currently testing DSC. >>> The patch I sent early simply fixes the error of comparing signed and >>> unsigned variables. >>> >>> We can then submit a second patch addressing the issue of using unsigned >>> long int instead of u32. Also, since the variables in drm_dsc_config >>> structure are all of type u8 and u16, the calculation values shouldn't >>> exceed the size of u32. >>> >>> Thanks >>> >>>>> >>>>> -----Original Message----- >>>>> From: Lipski, Mikita <Mikita.Lipski@amd.com> >>>>> Sent: November 19, 2019 10:08 AM >>>>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Lipski, Mikita >>>>> <Mikita.Lipski@amd.com> >>>>> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; >>>>> Cornij, Nikola <Nikola.Cornij@amd.com> >>>>> Subject: Re: [PATCH] drm/dsc: Return unsigned long on compute offset >>>>> >>>>> >>>>> >>>>> On 19/11/2019 09:56, Ville Syrjälä wrote: >>>>>> On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lipski@amd.com wrote: >>>>>>> From: Mikita Lipski <mikita.lipski@amd.com> >>>>>>> >>>>>>> We shouldn't compare int with unsigned long to find the max value and >>>>>>> since we are not expecting negative value returned from >>>>>>> compute_offset we should make this function return unsigned long so >>>>>>> we can compare the values when computing rc parameters. >>>>>> >>>>>> Why are there other unsigned longs in dsc parameter computation in the >>>>>> first place? >>>>> >>>>> I believe it was initially set to be unsigned long for variable >>>>> consistency, when we ported intel_compute_rc_parameters into >>>>> drm_dsc_compute_rc_parameters. But now that I look at it, we can >>>>> actually just set them to u32 or u64, as nothing should exceed that. >>>>>> >>>>>>> >>>>>>> Cc: Nikola Cornij <nikola.cornij@amd.com> >>>>>>> Cc: Harry Wentland <harry.wentland@amd.com> >>>>>>> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/drm_dsc.c | 6 +++--- >>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c >>>>>>> index 74f3527f567d..ec40604ab6a2 100644 >>>>>>> --- a/drivers/gpu/drm/drm_dsc.c >>>>>>> +++ b/drivers/gpu/drm/drm_dsc.c >>>>>>> @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct >>>>>>> drm_dsc_picture_parameter_set *pps_payload, >>>>>>> } >>>>>>> EXPORT_SYMBOL(drm_dsc_pps_payload_pack); >>>>>>> -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int >>>>>>> pixels_per_group, >>>>>>> +static unsigned long compute_offset(struct drm_dsc_config >>>>>>> *vdsc_cfg, int pixels_per_group, >>>>>>> int groups_per_line, int grpcnt) >>>>>>> { >>>>>>> - int offset = 0; >>>>>>> - int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, >>>>>>> pixels_per_group); >>>>>>> + unsigned long offset = 0; >>>>>>> + unsigned long grpcnt_id = >>>>>>> DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); >>>>>>> if (grpcnt <= grpcnt_id) >>>>>>> offset = DIV_ROUND_UP(grpcnt * pixels_per_group * >>>>>>> vdsc_cfg->bits_per_pixel, 16); >>>>>>> -- >>>>>>> 2.17.1 >>>>>>> >>>>>>> _______________________________________________ >>>>>>> dri-devel mailing list >>>>>>> dri-devel@lists.freedesktop.org >>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>>> >>>>> >>>>> -- >>>>> Thanks, >>>>> Mikita Lipski >>>>> Software Engineer 2, AMD >>>>> mikita.lipski@amd.com >>>> >>> >> >> -- >> Thanks, >> Mikita Lipski >> Software Engineer 2, AMD >> mikita.lipski@amd.com >
diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c index 74f3527f567d..ec40604ab6a2 100644 --- a/drivers/gpu/drm/drm_dsc.c +++ b/drivers/gpu/drm/drm_dsc.c @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload, } EXPORT_SYMBOL(drm_dsc_pps_payload_pack); -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, +static unsigned long compute_offset(struct drm_dsc_config *vdsc_cfg, int pixels_per_group, int groups_per_line, int grpcnt) { - int offset = 0; - int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); + unsigned long offset = 0; + unsigned long grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay, pixels_per_group); if (grpcnt <= grpcnt_id) offset = DIV_ROUND_UP(grpcnt * pixels_per_group * vdsc_cfg->bits_per_pixel, 16);