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