Hi Ville and Ankit, I actually do not think this is a problem with the DSC logic, but it is a problem with the intel_dp_link_compute_config() where we should do something if max_bpp is 0 instead of just returning a -EINVAL directly. My question here: - In case of PCON, yuv format, is it a valid case to have max bpp set to 0? - This is where I am seeing it as set to 0 - If it isnt then the problem is probably where it computes max bpp for hdmi case for yuv format Manasi On Tue, Apr 18, 2023 at 7:54 AM Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > > On Tue, Apr 18, 2023 at 08:09:16PM +0530, Nautiyal, Ankit K wrote: > > > > On 4/18/2023 6:16 PM, Ville Syrjälä wrote: > > > On Mon, Apr 17, 2023 at 03:48:12PM -0700, Manasi Navare wrote: > > >> Hi Ville, > > >> > > >> Could you suggest how to handle the intel_dp_link_compute_config() > > >> when the max_bpp is returned as 0, currently > > >> it just exits the loop and returns a -EINVAL and this triggers the DSC path. > > >> While we should be completely failing the modeset and encoder_config > > >> in this case instead of trying DSC, correct? > > > The DSC path needs to handle the bpp limits correctly: > > > 1. Take the baseline limits already computed > > > 2. Further restrict them based on sink/source DSC capabilities/etc. > > > 3. Make sure the uncompressed bpp value chosen is between the min/max > > > > I have some older patch to try similar thing [1]. We try to iterate over > > bpc to find pipe_bpp in the limits, then try to find out compressed_bpp. > > > > But if intel_dp_max_bpp returns 0, limits.max_bpp is set to 0, so we > > discard this for dsc case and re-calculate the limits.max_bpp? > > You shouldn't discard anything. DSC should take the already computed > limits and potentially just shrink them further based on DSC specific > constraints. > > Or is there some weird case where DSC would allow lower/higher bpp > than what our uncompressed bpp limits declare? > > > > > > > [1] https://patchwork.freedesktop.org/patch/519346/?series=111391&rev=3 > > > > > > > >> Manasi > > >> > > >> On Thu, Apr 13, 2023 at 8:23 AM Manasi Navare <navaremanasi@xxxxxxxxxxxx> wrote: > > >>> On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä > > >>> <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > > >>>> On Tue, Apr 11, 2023 at 05:07:01PM -0700, Manasi Navare wrote: > > >>>>> On Tue, Apr 11, 2023 at 10:42 AM Ville Syrjälä > > >>>>> <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > > >>>>>> On Tue, Apr 11, 2023 at 05:34:08PM +0000, Manasi Navare wrote: > > >>>>>>> In the function intel_dp_max_bpp(), currently if bpc < 0 in case of error, > > >>>>>>> we return 0 instead of returning an err code of -EINVAL. > > >>>>>>> This throws off the logic in the calling function. > > >>>>>> What logic? The caller doesn't expect to get an error. > > >>>>> If this returns a 0, we end up using limits.max_bpp = 0 and in > > >>>>> intel_dp_compute_link_config_wide(), > > >>>>> since max_bpp is 0, it exits this for loop: > > >>>>> > > >>>>> for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) and returns > > >>>>> -EINVAL which then wrongly goes to enable DSC even when link BW is > > >>>>> sufficient without DSC. > > >>>> And how woud max_bpp<0 prevent that? > > >>>> > > >>>> The real problem seems to be that the DSC code totally > > >>>> ignores bpp limits. > > >>> Hi Ville, > > >>> > > >>> So I see a few concerns/questions: > > >>> - Why is the Max bpp value 0 in intel_dp_max_bpp, is that a valid case > > >>> and how should our link configurations handle that case when max_bpp > > >>> is 0? > > >>> - This is happening in a bug I am looking at with HDMI PCON, @Ankit > > >>> Nautiyal have we ever seen something similar where max_bpp for HDMi > > >>> PCON > > >>> is returned 0? > > >>> - I dont think its a problem with DSC code, but rather > > >>> intel_dp_compute_link_config() outer for loop where we vary > > >>> from max_bpp to min_bpp and see if any bpp works with available link > > >>> bw, how should we handle this when max_bpp = 0 if you are saying thats > > >>> a valid case? > > >>> - In this patch if I return -EINVAL instead of 0, then atleast the > > >>> entire encoder_config will fail and that will fail the modeset, since > > >>> it assumes max_bpp cannot be 0 > > >>> > > >>> Could you please help answer above concerns and how to handle max bpp > > >>> = 0 case if that is valid? This patch is simply making that invalid > > >>> resulting into modeset failure > > >>> > > >>> Manasi > > >>>> -- > > >>>> Ville Syrjälä > > >>>> Intel > > -- > Ville Syrjälä > Intel
