[Intel-gfx] [PATCH v2 10/23] drm/dsc: Add helpers for DSC picture parameter set infoframes

[Manasi Navare]

On Fri, Aug 03, 2018 at 08:43:51PM +0100, Chris Wilson wrote:
> Quoting Manasi Navare (2018-08-03 20:18:42)
> > On Tue, Jul 31, 2018 at 10:16:45PM +0100, Chris Wilson wrote:
> > > Quoting Manasi Navare (2018-07-31 22:07:06)
> > > > +       /* PPS 4 */
> > > > +       pps_sdp->pps_payload.pps_4 = (u8)((dsc_cfg->bits_per_pixel &
> > > > +                                          DSC_PPS_BPP_HIGH_MASK) >>
> > > > +                                         DSC_PPS_MSB_SHIFT) |
> > > 
> > > To avoid overhanging cliffs, insert the newline after the sequence
> > > point. Quite a few examples throughout the series that would benefit
> > > from more judicial placement of line breaks.
> > 
> > What exactly are you refering to by sequence point here?
> 
> Which ever makes sense visually, here '='.
> 
> pps_sdp->pps_payload.pps_4 =
> 	dsc_cfg->bits_per_pixel >> X << DSC_PPS_MSB_SHIFT |
> 	dsc_cfg->vbr_enable << DSC_PPS_VBR_EN_SHIFT |
> 	dsc_cfg->enable422 << DSC_PPS_SIMPLE422_SHIFT |
> 	dsc_cfg->convert_rgb << DSC_PPS_CONVERT_RGB_SHIFT |
> 	dsc_cfg->block_pred_enable << DSC_PPS_BLOCK_PRED_EN_SHIFT;

Ok understood about the sequence point

> 
> If you want to mask with 0xff do it without having to refer to C type promotion
> rules.

This point is still not clear. is this not correct:
(dsc_cfg->bits_per_pixel & DSC_PPS_BPP_HIGH_MASK) >> DSC_PPS_MSB_SHIFT

> 
> > > Furthermore, you only need the SPDX shorthand rather than full licence
> > > text.
> > 
> > Here the header is what all the other .c files in drm have. They all tend to use
> > complete text.
> 
> And they all need updating, eventually. There's no reason for new file
> to use the old style.
> 
> > Are you suggesting just adding SPDX-License-Identifier: GPL-2.0+ at the begining of the file?
> > Should I remove the entire paragraph about the Copyright?
> 
> No, the code is MIT. You replace the licence with just a SPDX link.

Could you give an example here for the correct way of adding SPDX info in the header and what part
of the existing header needs to stay?
That would be helpful.

Manasi
> -Chris