[Intel-gfx] [PATCH 09/12] drm: Shrink drm_display_mode timings

[Ville Syrjälä]

On Fri, Feb 21, 2020 at 06:27:22PM +0100, Sam Ravnborg wrote:
> Hi Ville.
> 
> On Wed, Feb 19, 2020 at 10:35:41PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > Store the timings (apart from the clock) as u16. The uapi mode
> > struct already uses u16 for everything so using something bigger
> > internally doesn't really help us.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> While touchign this - have you cinsidered to put all the timing
> fields in a dedicated struct - and then have two instanses of said
> struct here?

The normal vs. crtc timigns aren't the same. But I've been pondering
about pulling the crtc timings into a separate struct and using that
in some places where we currently use a full drm_display_mode but only
really care about the crtc timings.

> 
> Like:
> 
> /**
>  * struct panel_timing - timing configuratio for a panel
>  *
>  * The timing is in khz and ...
>  /*
> struct panel_timing {
> 	/**
> 	 * @clock: Panel clock in khz
> 	 */
> 	int clock;
> 
> 	/**
> 	 * @hdisplay: bla bla
> 	 */
> 	u16 hdisplay;
>         u16 hsync_start;
>         u16 hsync_end;
>         u16 htotal;
>         u16 hskew;
>         u16 vdisplay;
>         u16 vsync_start;
>         u16 vsync_end;
>         u16 vtotal;
>         u16 vscan;
> };
> 
> 
> And then in drm_display_mode:
> 
> 	/**
> 	 * @hw_timing: The timing for the panel
> 	 */
> 	struct panel_timing hw_timing;
> 
> 	/**
> 	 * @calc_timing: The calculated timing
> 	 */
> 	struct panel_timing calc_timing;
> 
> Or something like that.
> We could use the panel_timign struct in some other places I think.
> 
> 	Sam
> 
> > ---
> >  drivers/gpu/drm/drm_modes.c |  7 ------
> >  include/drm/drm_modes.h     | 46 ++++++++++++++++++-------------------
> >  2 files changed, 23 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 0e7c9ba241c4..cc9fc52f9f7c 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1917,13 +1917,6 @@ EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode);
> >  void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> >  			       const struct drm_display_mode *in)
> >  {
> > -	WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
> > -	     in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
> > -	     in->hskew > USHRT_MAX || in->vdisplay > USHRT_MAX ||
> > -	     in->vsync_start > USHRT_MAX || in->vsync_end > USHRT_MAX ||
> > -	     in->vtotal > USHRT_MAX || in->vscan > USHRT_MAX,
> > -	     "timing values too large for mode info\n");
> > -
> >  	out->clock = in->clock;
> >  	out->hdisplay = in->hdisplay;
> >  	out->hsync_start = in->hsync_start;
> > diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> > index b28c0234fcd7..b585074945b5 100644
> > --- a/include/drm/drm_modes.h
> > +++ b/include/drm/drm_modes.h
> > @@ -278,16 +278,16 @@ struct drm_display_mode {
> >  	 * Pixel clock in kHz.
> >  	 */
> >  	int clock;		/* in kHz */
> > -	int hdisplay;
> > -	int hsync_start;
> > -	int hsync_end;
> > -	int htotal;
> > -	int hskew;
> > -	int vdisplay;
> > -	int vsync_start;
> > -	int vsync_end;
> > -	int vtotal;
> > -	int vscan;
> > +	u16 hdisplay;
> > +	u16 hsync_start;
> > +	u16 hsync_end;
> > +	u16 htotal;
> > +	u16 hskew;
> > +	u16 vdisplay;
> > +	u16 vsync_start;
> > +	u16 vsync_end;
> > +	u16 vtotal;
> > +	u16 vscan;
> >  	/**
> >  	 * @flags:
> >  	 *
> > @@ -356,19 +356,19 @@ struct drm_display_mode {
> >  	 * difference is exactly a factor of 10.
> >  	 */
> >  	int crtc_clock;
> > -	int crtc_hdisplay;
> > -	int crtc_hblank_start;
> > -	int crtc_hblank_end;
> > -	int crtc_hsync_start;
> > -	int crtc_hsync_end;
> > -	int crtc_htotal;
> > -	int crtc_hskew;
> > -	int crtc_vdisplay;
> > -	int crtc_vblank_start;
> > -	int crtc_vblank_end;
> > -	int crtc_vsync_start;
> > -	int crtc_vsync_end;
> > -	int crtc_vtotal;
> > +	u16 crtc_hdisplay;
> > +	u16 crtc_hblank_start;
> > +	u16 crtc_hblank_end;
> > +	u16 crtc_hsync_start;
> > +	u16 crtc_hsync_end;
> > +	u16 crtc_htotal;
> > +	u16 crtc_hskew;
> > +	u16 crtc_vdisplay;
> > +	u16 crtc_vblank_start;
> > +	u16 crtc_vblank_end;
> > +	u16 crtc_vsync_start;
> > +	u16 crtc_vsync_end;
> > +	u16 crtc_vtotal;
> >  
> >  	/**
> >  	 * @private_flags:
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel