[PATCH] drm/dsc: Return unsigned long on compute offset

[Ville Syrjälä]

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 at amd.com>
> >>> Sent: November 19, 2019 10:08 AM
> >>> To: Ville Syrjälä <ville.syrjala at linux.intel.com>; Lipski, Mikita 
> >>> <Mikita.Lipski at amd.com>
> >>> Cc: amd-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; 
> >>> Cornij, Nikola <Nikola.Cornij at 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 at amd.com wrote:
> >>>>> From: Mikita Lipski <mikita.lipski at 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 at amd.com>
> >>>>> Cc: Harry Wentland <harry.wentland at amd.com>
> >>>>> Signed-off-by: Mikita Lipski <mikita.lipski at 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 at lists.freedesktop.org
> >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>
> >>>
> >>> -- 
> >>> Thanks,
> >>> Mikita Lipski
> >>> Software Engineer 2, AMD
> >>> mikita.lipski at amd.com
> >>
> > 
> 
> -- 
> Thanks,
> Mikita Lipski
> Software Engineer 2, AMD
> mikita.lipski at amd.com

-- 
Ville Syrjälä
Intel