| Message ID | 20200403204008.14864-16-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Put drm_display_mode on diet | expand |
On Fri, Apr 03, 2020 at 11:40:06PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > gma500 only uses mode->private_flags to convey the sdvo pixel > multiplier from the encoder .mode_fixup() hook to the encoder > .mode_set() hook. Those always seems get called as a pair so > let's just stuff the pixel multiplier into the encoder itself > as there are no state objects we could use in this non-atomic > driver. > > Paves the way for nuking mode->private_flag. Yeah the sdvo multiplier is why I originally started with creating the intel_pipe_config stuff ... The only semi-nasty thing with legacy crtc helpers is that they don't clean up anything from mode_fixup failures. But since the next modeset will go through the full dance again I think that's ok. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com> > CC: Sam Ravnborg <sam@ravnborg.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/gma500/psb_intel_drv.h | 19 ------------------- > drivers/gpu/drm/gma500/psb_intel_sdvo.c | 11 ++++++----- > 2 files changed, 6 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h > index fb601983cef0..3dd5718c3e31 100644 > --- a/drivers/gpu/drm/gma500/psb_intel_drv.h > +++ b/drivers/gpu/drm/gma500/psb_intel_drv.h > @@ -56,25 +56,6 @@ > #define INTEL_OUTPUT_DISPLAYPORT 9 > #define INTEL_OUTPUT_EDP 10 > > -#define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) > -#define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) > - > -static inline void > -psb_intel_mode_set_pixel_multiplier(struct drm_display_mode *mode, > - int multiplier) > -{ > - mode->clock *= multiplier; > - mode->private_flags |= multiplier; > -} > - > -static inline int > -psb_intel_mode_get_pixel_multiplier(const struct drm_display_mode *mode) > -{ > - return (mode->private_flags & INTEL_MODE_PIXEL_MULTIPLIER_MASK) > - >> INTEL_MODE_PIXEL_MULTIPLIER_SHIFT; > -} > - > - > /* > * Hold information useally put on the device driver privates here, > * since it needs to be shared across multiple of devices drivers privates. > diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > index 264d7ad004b4..9e88a37f55e9 100644 > --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c > +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > @@ -132,6 +132,8 @@ struct psb_intel_sdvo { > /* DDC bus used by this SDVO encoder */ > uint8_t ddc_bus; > > + u8 pixel_multiplier; > + > /* Input timings for adjusted_mode */ > struct psb_intel_sdvo_dtd input_dtd; > > @@ -958,7 +960,6 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > struct drm_display_mode *adjusted_mode) > { > struct psb_intel_sdvo *psb_intel_sdvo = to_psb_intel_sdvo(encoder); > - int multiplier; > > /* We need to construct preferred input timings based on our > * output timings. To do that, we have to set the output > @@ -985,8 +986,9 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > /* Make the CRTC code factor in the SDVO pixel multiplier. The > * SDVO device will factor out the multiplier during mode_set. > */ > - multiplier = psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > - psb_intel_mode_set_pixel_multiplier(adjusted_mode, multiplier); > + psb_intel_sdvo->pixel_multiplier = > + psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > + adjusted_mode->clock *= psb_intel_sdvo->pixel_multiplier; > > return true; > } > @@ -1002,7 +1004,6 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > u32 sdvox; > struct psb_intel_sdvo_in_out_map in_out; > struct psb_intel_sdvo_dtd input_dtd; > - int pixel_multiplier = psb_intel_mode_get_pixel_multiplier(adjusted_mode); > int rate; > int need_aux = IS_MRST(dev) ? 1 : 0; > > @@ -1060,7 +1061,7 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > > (void) psb_intel_sdvo_set_input_timing(psb_intel_sdvo, &input_dtd); > > - switch (pixel_multiplier) { > + switch (psb_intel_sdvo->pixel_multiplier) { > default: > case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break; > case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break; > -- > 2.24.1 >
Hi Ville. On Fri, Apr 03, 2020 at 11:40:06PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > gma500 only uses mode->private_flags to convey the sdvo pixel > multiplier from the encoder .mode_fixup() hook to the encoder > .mode_set() hook. Those always seems get called as a pair so > let's just stuff the pixel multiplier into the encoder itself > as there are no state objects we could use in this non-atomic > driver. > > Paves the way for nuking mode->private_flag. Nice little clean-up. One comment below. Sam > > Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com> > CC: Sam Ravnborg <sam@ravnborg.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/gma500/psb_intel_drv.h | 19 ------------------- > drivers/gpu/drm/gma500/psb_intel_sdvo.c | 11 ++++++----- > 2 files changed, 6 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h > index fb601983cef0..3dd5718c3e31 100644 > --- a/drivers/gpu/drm/gma500/psb_intel_drv.h > +++ b/drivers/gpu/drm/gma500/psb_intel_drv.h > @@ -56,25 +56,6 @@ > #define INTEL_OUTPUT_DISPLAYPORT 9 > #define INTEL_OUTPUT_EDP 10 > > -#define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) > -#define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) > - > -static inline void > -psb_intel_mode_set_pixel_multiplier(struct drm_display_mode *mode, > - int multiplier) > -{ > - mode->clock *= multiplier; > - mode->private_flags |= multiplier; > -} > - > -static inline int > -psb_intel_mode_get_pixel_multiplier(const struct drm_display_mode *mode) > -{ > - return (mode->private_flags & INTEL_MODE_PIXEL_MULTIPLIER_MASK) > - >> INTEL_MODE_PIXEL_MULTIPLIER_SHIFT; > -} > - > - > /* > * Hold information useally put on the device driver privates here, > * since it needs to be shared across multiple of devices drivers privates. > diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > index 264d7ad004b4..9e88a37f55e9 100644 > --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c > +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > @@ -132,6 +132,8 @@ struct psb_intel_sdvo { > /* DDC bus used by this SDVO encoder */ > uint8_t ddc_bus; > > + u8 pixel_multiplier; > + There is really no good reason to use an u8 here. psb_intel_sdvo_get_pixel_multiplier() return an int, so use an int here too. With this fixed: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > /* Input timings for adjusted_mode */ > struct psb_intel_sdvo_dtd input_dtd; > > @@ -958,7 +960,6 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > struct drm_display_mode *adjusted_mode) > { > struct psb_intel_sdvo *psb_intel_sdvo = to_psb_intel_sdvo(encoder); > - int multiplier; > > /* We need to construct preferred input timings based on our > * output timings. To do that, we have to set the output > @@ -985,8 +986,9 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > /* Make the CRTC code factor in the SDVO pixel multiplier. The > * SDVO device will factor out the multiplier during mode_set. > */ > - multiplier = psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > - psb_intel_mode_set_pixel_multiplier(adjusted_mode, multiplier); > + psb_intel_sdvo->pixel_multiplier = > + psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > + adjusted_mode->clock *= psb_intel_sdvo->pixel_multiplier; > > return true; > } > @@ -1002,7 +1004,6 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > u32 sdvox; > struct psb_intel_sdvo_in_out_map in_out; > struct psb_intel_sdvo_dtd input_dtd; > - int pixel_multiplier = psb_intel_mode_get_pixel_multiplier(adjusted_mode); > int rate; > int need_aux = IS_MRST(dev) ? 1 : 0; > > @@ -1060,7 +1061,7 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > > (void) psb_intel_sdvo_set_input_timing(psb_intel_sdvo, &input_dtd); > > - switch (pixel_multiplier) { > + switch (psb_intel_sdvo->pixel_multiplier) { > default: > case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break; > case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break; > -- > 2.24.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Apr 07, 2020 at 08:56:53PM +0200, Sam Ravnborg wrote: > Hi Ville. > > On Fri, Apr 03, 2020 at 11:40:06PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > gma500 only uses mode->private_flags to convey the sdvo pixel > > multiplier from the encoder .mode_fixup() hook to the encoder > > .mode_set() hook. Those always seems get called as a pair so > > let's just stuff the pixel multiplier into the encoder itself > > as there are no state objects we could use in this non-atomic > > driver. > > > > Paves the way for nuking mode->private_flag. > Nice little clean-up. One comment below. > > Sam > > > > Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com> > > CC: Sam Ravnborg <sam@ravnborg.org> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/gma500/psb_intel_drv.h | 19 ------------------- > > drivers/gpu/drm/gma500/psb_intel_sdvo.c | 11 ++++++----- > > 2 files changed, 6 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h > > index fb601983cef0..3dd5718c3e31 100644 > > --- a/drivers/gpu/drm/gma500/psb_intel_drv.h > > +++ b/drivers/gpu/drm/gma500/psb_intel_drv.h > > @@ -56,25 +56,6 @@ > > #define INTEL_OUTPUT_DISPLAYPORT 9 > > #define INTEL_OUTPUT_EDP 10 > > > > -#define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) > > -#define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) > > - > > -static inline void > > -psb_intel_mode_set_pixel_multiplier(struct drm_display_mode *mode, > > - int multiplier) > > -{ > > - mode->clock *= multiplier; > > - mode->private_flags |= multiplier; > > -} > > - > > -static inline int > > -psb_intel_mode_get_pixel_multiplier(const struct drm_display_mode *mode) > > -{ > > - return (mode->private_flags & INTEL_MODE_PIXEL_MULTIPLIER_MASK) > > - >> INTEL_MODE_PIXEL_MULTIPLIER_SHIFT; > > -} > > - > > - > > /* > > * Hold information useally put on the device driver privates here, > > * since it needs to be shared across multiple of devices drivers privates. > > diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > index 264d7ad004b4..9e88a37f55e9 100644 > > --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > @@ -132,6 +132,8 @@ struct psb_intel_sdvo { > > /* DDC bus used by this SDVO encoder */ > > uint8_t ddc_bus; > > > > + u8 pixel_multiplier; > > + > > There is really no good reason to use an u8 here. Wastes less space. > psb_intel_sdvo_get_pixel_multiplier() return an int, so use an int here > too. > > With this fixed: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > > /* Input timings for adjusted_mode */ > > struct psb_intel_sdvo_dtd input_dtd; > > > > @@ -958,7 +960,6 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > > struct drm_display_mode *adjusted_mode) > > { > > struct psb_intel_sdvo *psb_intel_sdvo = to_psb_intel_sdvo(encoder); > > - int multiplier; > > > > /* We need to construct preferred input timings based on our > > * output timings. To do that, we have to set the output > > @@ -985,8 +986,9 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > > /* Make the CRTC code factor in the SDVO pixel multiplier. The > > * SDVO device will factor out the multiplier during mode_set. > > */ > > - multiplier = psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > > - psb_intel_mode_set_pixel_multiplier(adjusted_mode, multiplier); > > + psb_intel_sdvo->pixel_multiplier = > > + psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > > + adjusted_mode->clock *= psb_intel_sdvo->pixel_multiplier; > > > > return true; > > } > > @@ -1002,7 +1004,6 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > > u32 sdvox; > > struct psb_intel_sdvo_in_out_map in_out; > > struct psb_intel_sdvo_dtd input_dtd; > > - int pixel_multiplier = psb_intel_mode_get_pixel_multiplier(adjusted_mode); > > int rate; > > int need_aux = IS_MRST(dev) ? 1 : 0; > > > > @@ -1060,7 +1061,7 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > > > > (void) psb_intel_sdvo_set_input_timing(psb_intel_sdvo, &input_dtd); > > > > - switch (pixel_multiplier) { > > + switch (psb_intel_sdvo->pixel_multiplier) { > > default: > > case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break; > > case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break; > > -- > > 2.24.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Apr 07, 2020 at 10:08:00PM +0300, Ville Syrjälä wrote: > On Tue, Apr 07, 2020 at 08:56:53PM +0200, Sam Ravnborg wrote: > > Hi Ville. > > > > On Fri, Apr 03, 2020 at 11:40:06PM +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > gma500 only uses mode->private_flags to convey the sdvo pixel > > > multiplier from the encoder .mode_fixup() hook to the encoder > > > .mode_set() hook. Those always seems get called as a pair so > > > let's just stuff the pixel multiplier into the encoder itself > > > as there are no state objects we could use in this non-atomic > > > driver. > > > > > > Paves the way for nuking mode->private_flag. > > Nice little clean-up. One comment below. > > > > Sam > > > > > > Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com> > > > CC: Sam Ravnborg <sam@ravnborg.org> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/gma500/psb_intel_drv.h | 19 ------------------- > > > drivers/gpu/drm/gma500/psb_intel_sdvo.c | 11 ++++++----- > > > 2 files changed, 6 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h > > > index fb601983cef0..3dd5718c3e31 100644 > > > --- a/drivers/gpu/drm/gma500/psb_intel_drv.h > > > +++ b/drivers/gpu/drm/gma500/psb_intel_drv.h > > > @@ -56,25 +56,6 @@ > > > #define INTEL_OUTPUT_DISPLAYPORT 9 > > > #define INTEL_OUTPUT_EDP 10 > > > > > > -#define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) > > > -#define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) > > > - > > > -static inline void > > > -psb_intel_mode_set_pixel_multiplier(struct drm_display_mode *mode, > > > - int multiplier) > > > -{ > > > - mode->clock *= multiplier; > > > - mode->private_flags |= multiplier; > > > -} > > > - > > > -static inline int > > > -psb_intel_mode_get_pixel_multiplier(const struct drm_display_mode *mode) > > > -{ > > > - return (mode->private_flags & INTEL_MODE_PIXEL_MULTIPLIER_MASK) > > > - >> INTEL_MODE_PIXEL_MULTIPLIER_SHIFT; > > > -} > > > - > > > - > > > /* > > > * Hold information useally put on the device driver privates here, > > > * since it needs to be shared across multiple of devices drivers privates. > > > diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > index 264d7ad004b4..9e88a37f55e9 100644 > > > --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > @@ -132,6 +132,8 @@ struct psb_intel_sdvo { > > > /* DDC bus used by this SDVO encoder */ > > > uint8_t ddc_bus; > > > > > > + u8 pixel_multiplier; > > > + > > > > There is really no good reason to use an u8 here. > > Wastes less space. When there is a good reason - use the size limited variants. But in this use case there is no reason to space optimize it. When in the slightly pedantic mode, using u8 is not consistent. ddc_bus defined above usese uint8_t. Sam > > > psb_intel_sdvo_get_pixel_multiplier() return an int, so use an int here > > too. > > > > With this fixed: > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > > > > /* Input timings for adjusted_mode */ > > > struct psb_intel_sdvo_dtd input_dtd; > > > > > > @@ -958,7 +960,6 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > > > struct drm_display_mode *adjusted_mode) > > > { > > > struct psb_intel_sdvo *psb_intel_sdvo = to_psb_intel_sdvo(encoder); > > > - int multiplier; > > > > > > /* We need to construct preferred input timings based on our > > > * output timings. To do that, we have to set the output > > > @@ -985,8 +986,9 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > > > /* Make the CRTC code factor in the SDVO pixel multiplier. The > > > * SDVO device will factor out the multiplier during mode_set. > > > */ > > > - multiplier = psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > > > - psb_intel_mode_set_pixel_multiplier(adjusted_mode, multiplier); > > > + psb_intel_sdvo->pixel_multiplier = > > > + psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > > > + adjusted_mode->clock *= psb_intel_sdvo->pixel_multiplier; > > > > > > return true; > > > } > > > @@ -1002,7 +1004,6 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > > > u32 sdvox; > > > struct psb_intel_sdvo_in_out_map in_out; > > > struct psb_intel_sdvo_dtd input_dtd; > > > - int pixel_multiplier = psb_intel_mode_get_pixel_multiplier(adjusted_mode); > > > int rate; > > > int need_aux = IS_MRST(dev) ? 1 : 0; > > > > > > @@ -1060,7 +1061,7 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > > > > > > (void) psb_intel_sdvo_set_input_timing(psb_intel_sdvo, &input_dtd); > > > > > > - switch (pixel_multiplier) { > > > + switch (psb_intel_sdvo->pixel_multiplier) { > > > default: > > > case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break; > > > case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break; > > > -- > > > 2.24.1 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel
On Tue, Apr 07, 2020 at 09:35:37PM +0200, Sam Ravnborg wrote: > On Tue, Apr 07, 2020 at 10:08:00PM +0300, Ville Syrjälä wrote: > > On Tue, Apr 07, 2020 at 08:56:53PM +0200, Sam Ravnborg wrote: > > > Hi Ville. > > > > > > On Fri, Apr 03, 2020 at 11:40:06PM +0300, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > gma500 only uses mode->private_flags to convey the sdvo pixel > > > > multiplier from the encoder .mode_fixup() hook to the encoder > > > > .mode_set() hook. Those always seems get called as a pair so > > > > let's just stuff the pixel multiplier into the encoder itself > > > > as there are no state objects we could use in this non-atomic > > > > driver. > > > > > > > > Paves the way for nuking mode->private_flag. > > > Nice little clean-up. One comment below. > > > > > > Sam > > > > > > > > Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com> > > > > CC: Sam Ravnborg <sam@ravnborg.org> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/gma500/psb_intel_drv.h | 19 ------------------- > > > > drivers/gpu/drm/gma500/psb_intel_sdvo.c | 11 ++++++----- > > > > 2 files changed, 6 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h > > > > index fb601983cef0..3dd5718c3e31 100644 > > > > --- a/drivers/gpu/drm/gma500/psb_intel_drv.h > > > > +++ b/drivers/gpu/drm/gma500/psb_intel_drv.h > > > > @@ -56,25 +56,6 @@ > > > > #define INTEL_OUTPUT_DISPLAYPORT 9 > > > > #define INTEL_OUTPUT_EDP 10 > > > > > > > > -#define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) > > > > -#define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) > > > > - > > > > -static inline void > > > > -psb_intel_mode_set_pixel_multiplier(struct drm_display_mode *mode, > > > > - int multiplier) > > > > -{ > > > > - mode->clock *= multiplier; > > > > - mode->private_flags |= multiplier; > > > > -} > > > > - > > > > -static inline int > > > > -psb_intel_mode_get_pixel_multiplier(const struct drm_display_mode *mode) > > > > -{ > > > > - return (mode->private_flags & INTEL_MODE_PIXEL_MULTIPLIER_MASK) > > > > - >> INTEL_MODE_PIXEL_MULTIPLIER_SHIFT; > > > > -} > > > > - > > > > - > > > > /* > > > > * Hold information useally put on the device driver privates here, > > > > * since it needs to be shared across multiple of devices drivers privates. > > > > diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > index 264d7ad004b4..9e88a37f55e9 100644 > > > > --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > @@ -132,6 +132,8 @@ struct psb_intel_sdvo { > > > > /* DDC bus used by this SDVO encoder */ > > > > uint8_t ddc_bus; > > > > > > > > + u8 pixel_multiplier; > > > > + > > > > > > There is really no good reason to use an u8 here. > > > > Wastes less space. > > When there is a good reason - use the size limited variants. > But in this use case there is no reason to space optimize it. IMO when it's stuffed into a structure there's no reason not to optimize it. At some point it all starts to add up. At least i915 suffers a lot from bloated structures (dev_priv and atomic state structs being the prime examples) where we could probably shave dozens if not hundreds of bytes if everything just used the smallest type possible. In fact this series does shave dozens of bytes from the crtc state alone. > > When in the slightly pedantic mode, using u8 is not consistent. > ddc_bus defined above usese uint8_t. u8 & co. are preferred in kernel code. Checkpatch even complains when you use the stdint types. The uint8_t here is some old leftovers. > > Sam > > > > > psb_intel_sdvo_get_pixel_multiplier() return an int, so use an int here > > > too. > > > > > > With this fixed: > > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > > > > > > /* Input timings for adjusted_mode */ > > > > struct psb_intel_sdvo_dtd input_dtd; > > > > > > > > @@ -958,7 +960,6 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > > > > struct drm_display_mode *adjusted_mode) > > > > { > > > > struct psb_intel_sdvo *psb_intel_sdvo = to_psb_intel_sdvo(encoder); > > > > - int multiplier; > > > > > > > > /* We need to construct preferred input timings based on our > > > > * output timings. To do that, we have to set the output > > > > @@ -985,8 +986,9 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, > > > > /* Make the CRTC code factor in the SDVO pixel multiplier. The > > > > * SDVO device will factor out the multiplier during mode_set. > > > > */ > > > > - multiplier = psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > > > > - psb_intel_mode_set_pixel_multiplier(adjusted_mode, multiplier); > > > > + psb_intel_sdvo->pixel_multiplier = > > > > + psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); > > > > + adjusted_mode->clock *= psb_intel_sdvo->pixel_multiplier; > > > > > > > > return true; > > > > } > > > > @@ -1002,7 +1004,6 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > > > > u32 sdvox; > > > > struct psb_intel_sdvo_in_out_map in_out; > > > > struct psb_intel_sdvo_dtd input_dtd; > > > > - int pixel_multiplier = psb_intel_mode_get_pixel_multiplier(adjusted_mode); > > > > int rate; > > > > int need_aux = IS_MRST(dev) ? 1 : 0; > > > > > > > > @@ -1060,7 +1061,7 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, > > > > > > > > (void) psb_intel_sdvo_set_input_timing(psb_intel_sdvo, &input_dtd); > > > > > > > > - switch (pixel_multiplier) { > > > > + switch (psb_intel_sdvo->pixel_multiplier) { > > > > default: > > > > case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break; > > > > case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break; > > > > -- > > > > 2.24.1 > > > > > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Ville Syrjälä > > Intel
On Thu, Apr 09, 2020 at 10:49:52PM +0300, Ville Syrjälä wrote: > On Tue, Apr 07, 2020 at 09:35:37PM +0200, Sam Ravnborg wrote: > > On Tue, Apr 07, 2020 at 10:08:00PM +0300, Ville Syrjälä wrote: > > > On Tue, Apr 07, 2020 at 08:56:53PM +0200, Sam Ravnborg wrote: > > > > Hi Ville. > > > > > > > > On Fri, Apr 03, 2020 at 11:40:06PM +0300, Ville Syrjala wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > gma500 only uses mode->private_flags to convey the sdvo pixel > > > > > multiplier from the encoder .mode_fixup() hook to the encoder > > > > > .mode_set() hook. Those always seems get called as a pair so > > > > > let's just stuff the pixel multiplier into the encoder itself > > > > > as there are no state objects we could use in this non-atomic > > > > > driver. > > > > > > > > > > Paves the way for nuking mode->private_flag. > > > > Nice little clean-up. One comment below. > > > > > > > > Sam > > > > > > > > > > Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com> > > > > > CC: Sam Ravnborg <sam@ravnborg.org> > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/gma500/psb_intel_drv.h | 19 ------------------- > > > > > drivers/gpu/drm/gma500/psb_intel_sdvo.c | 11 ++++++----- > > > > > 2 files changed, 6 insertions(+), 24 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h > > > > > index fb601983cef0..3dd5718c3e31 100644 > > > > > --- a/drivers/gpu/drm/gma500/psb_intel_drv.h > > > > > +++ b/drivers/gpu/drm/gma500/psb_intel_drv.h > > > > > @@ -56,25 +56,6 @@ > > > > > #define INTEL_OUTPUT_DISPLAYPORT 9 > > > > > #define INTEL_OUTPUT_EDP 10 > > > > > > > > > > -#define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) > > > > > -#define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) > > > > > - > > > > > -static inline void > > > > > -psb_intel_mode_set_pixel_multiplier(struct drm_display_mode *mode, > > > > > - int multiplier) > > > > > -{ > > > > > - mode->clock *= multiplier; > > > > > - mode->private_flags |= multiplier; > > > > > -} > > > > > - > > > > > -static inline int > > > > > -psb_intel_mode_get_pixel_multiplier(const struct drm_display_mode *mode) > > > > > -{ > > > > > - return (mode->private_flags & INTEL_MODE_PIXEL_MULTIPLIER_MASK) > > > > > - >> INTEL_MODE_PIXEL_MULTIPLIER_SHIFT; > > > > > -} > > > > > - > > > > > - > > > > > /* > > > > > * Hold information useally put on the device driver privates here, > > > > > * since it needs to be shared across multiple of devices drivers privates. > > > > > diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > > index 264d7ad004b4..9e88a37f55e9 100644 > > > > > --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > > +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > > @@ -132,6 +132,8 @@ struct psb_intel_sdvo { > > > > > /* DDC bus used by this SDVO encoder */ > > > > > uint8_t ddc_bus; > > > > > > > > > > + u8 pixel_multiplier; > > > > > + > > > > > > > > There is really no good reason to use an u8 here. > > > > > > Wastes less space. > > > > When there is a good reason - use the size limited variants. > > But in this use case there is no reason to space optimize it. > > IMO when it's stuffed into a structure there's no reason not to > optimize it. At some point it all starts to add up. > > At least i915 suffers a lot from bloated structures (dev_priv > and atomic state structs being the prime examples) where we > could probably shave dozens if not hundreds of bytes if > everything just used the smallest type possible. In fact > this series does shave dozens of bytes from the crtc state > alone. Make that hundreds of bytes actually. I think we have three or more copies of drm_display_mode embedded in there and this series shrinks each one by 80 bytes (iirc).
Hi Ville. > > > > > index 264d7ad004b4..9e88a37f55e9 100644 > > > > > --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > > +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > > @@ -132,6 +132,8 @@ struct psb_intel_sdvo { > > > > > /* DDC bus used by this SDVO encoder */ > > > > > uint8_t ddc_bus; > > > > > > > > > > + u8 pixel_multiplier; > > > > > + > > > > > > > > There is really no good reason to use an u8 here. > > > > > > Wastes less space. > > > > When there is a good reason - use the size limited variants. > > But in this use case there is no reason to space optimize it. > > IMO when it's stuffed into a structure there's no reason not to > optimize it. At some point it all starts to add up. > > At least i915 suffers a lot from bloated structures (dev_priv > and atomic state structs being the prime examples) where we > could probably shave dozens if not hundreds of bytes if > everything just used the smallest type possible. In fact > this series does shave dozens of bytes from the crtc state > alone. There is a difference between a structure used many times - And a structure used once or only a few times. If everyone started to optimize the types used, then we would end up with code that is hard to maintain. The point here is that we have a structure allocated maybe once and a field assinged from a int - which using integer promotion is then stuffed into an u8. If we one day start to be clever and use values above 255 we need to find all the places where a u8 was used to optimize size of some random struct. If this was a struct instantiated many times and used all over the story was another - but thats not the case here. Here the principle of least suprises hold - do not change the type. I try to explain the rationale behind the argument to use int. Feel free to disagree. > > > > > When in the slightly pedantic mode, using u8 is not consistent. > > ddc_bus defined above usese uint8_t. > > u8 & co. are preferred in kernel code. Checkpatch even complains when > you use the stdint types. The uint8_t here is some old leftovers. Mixing coding practice makes code less readable, no matter the output of checkpatch. The right fix would be to update gma500 to migrate away from the stdint types. But that would be a sepearte patch for another day. My orginal feedback on the patch has not changed. Feel free to move forward with the patch without my r-b. Sam
On Thu, Apr 09, 2020 at 10:47:43PM +0200, Sam Ravnborg wrote: > Hi Ville. > > > > > > > index 264d7ad004b4..9e88a37f55e9 100644 > > > > > > --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > > > +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c > > > > > > @@ -132,6 +132,8 @@ struct psb_intel_sdvo { > > > > > > /* DDC bus used by this SDVO encoder */ > > > > > > uint8_t ddc_bus; > > > > > > > > > > > > + u8 pixel_multiplier; > > > > > > + > > > > > > > > > > There is really no good reason to use an u8 here. > > > > > > > > Wastes less space. > > > > > > When there is a good reason - use the size limited variants. > > > But in this use case there is no reason to space optimize it. > > > > IMO when it's stuffed into a structure there's no reason not to > > optimize it. At some point it all starts to add up. > > > > At least i915 suffers a lot from bloated structures (dev_priv > > and atomic state structs being the prime examples) where we > > could probably shave dozens if not hundreds of bytes if > > everything just used the smallest type possible. In fact > > this series does shave dozens of bytes from the crtc state > > alone. > > There is a difference between a structure used many times - > And a structure used once or only a few times. > If everyone started to optimize the types used, then we > would end up with code that is hard to maintain. > > The point here is that we have a structure allocated maybe > once and a field assinged from a int - which using integer promotion > is then stuffed into an u8. If we one day start to be clever and > use values above 255 we need to find all the places where a > u8 was used to optimize size of some random struct. Never going to happen. The pixel multiplier can only have values <=4. Also a lot of other things here are already size optimized (eg. the ddc_bus which was even part of the patch context). > > If this was a struct instantiated many times and used all over > the story was another - but thats not the case here. That would mostly lead to inconsistencies with the same thing potentially using multiple different types between different structs. IMO best to use the smallest type everywhere to make things consistent. Also helps with refactoring when you don't have to change types when moving things between structs. > Here the principle of least suprises hold - do not change the type. > > I try to explain the rationale behind the argument to use int. > Feel free to disagree. > > > > > > > > > When in the slightly pedantic mode, using u8 is not consistent. > > > ddc_bus defined above usese uint8_t. > > > > u8 & co. are preferred in kernel code. Checkpatch even complains when > > you use the stdint types. The uint8_t here is some old leftovers. > > Mixing coding practice makes code less readable, no matter > the output of checkpatch. > The right fix would be to update gma500 to migrate away from the > stdint types. But that would be a sepearte patch for another day. Have you actually looked at this file? It's already a mismash of both types. > > My orginal feedback on the patch has not changed. > Feel free to move forward with the patch without my r-b. > > Sam
diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h index fb601983cef0..3dd5718c3e31 100644 --- a/drivers/gpu/drm/gma500/psb_intel_drv.h +++ b/drivers/gpu/drm/gma500/psb_intel_drv.h @@ -56,25 +56,6 @@ #define INTEL_OUTPUT_DISPLAYPORT 9 #define INTEL_OUTPUT_EDP 10 -#define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) -#define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) - -static inline void -psb_intel_mode_set_pixel_multiplier(struct drm_display_mode *mode, - int multiplier) -{ - mode->clock *= multiplier; - mode->private_flags |= multiplier; -} - -static inline int -psb_intel_mode_get_pixel_multiplier(const struct drm_display_mode *mode) -{ - return (mode->private_flags & INTEL_MODE_PIXEL_MULTIPLIER_MASK) - >> INTEL_MODE_PIXEL_MULTIPLIER_SHIFT; -} - - /* * Hold information useally put on the device driver privates here, * since it needs to be shared across multiple of devices drivers privates. diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c index 264d7ad004b4..9e88a37f55e9 100644 --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c @@ -132,6 +132,8 @@ struct psb_intel_sdvo { /* DDC bus used by this SDVO encoder */ uint8_t ddc_bus; + u8 pixel_multiplier; + /* Input timings for adjusted_mode */ struct psb_intel_sdvo_dtd input_dtd; @@ -958,7 +960,6 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode) { struct psb_intel_sdvo *psb_intel_sdvo = to_psb_intel_sdvo(encoder); - int multiplier; /* We need to construct preferred input timings based on our * output timings. To do that, we have to set the output @@ -985,8 +986,9 @@ static bool psb_intel_sdvo_mode_fixup(struct drm_encoder *encoder, /* Make the CRTC code factor in the SDVO pixel multiplier. The * SDVO device will factor out the multiplier during mode_set. */ - multiplier = psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); - psb_intel_mode_set_pixel_multiplier(adjusted_mode, multiplier); + psb_intel_sdvo->pixel_multiplier = + psb_intel_sdvo_get_pixel_multiplier(adjusted_mode); + adjusted_mode->clock *= psb_intel_sdvo->pixel_multiplier; return true; } @@ -1002,7 +1004,6 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, u32 sdvox; struct psb_intel_sdvo_in_out_map in_out; struct psb_intel_sdvo_dtd input_dtd; - int pixel_multiplier = psb_intel_mode_get_pixel_multiplier(adjusted_mode); int rate; int need_aux = IS_MRST(dev) ? 1 : 0; @@ -1060,7 +1061,7 @@ static void psb_intel_sdvo_mode_set(struct drm_encoder *encoder, (void) psb_intel_sdvo_set_input_timing(psb_intel_sdvo, &input_dtd); - switch (pixel_multiplier) { + switch (psb_intel_sdvo->pixel_multiplier) { default: case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break; case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break;