[Intel-gfx] [GLK MIPI DSI V2 9/9] drm/915: Parsing the missed out DTD fields from the VBT

[Chauhan, Madhav]

> -----Original Message-----
> From: Nikula, Jani
> Sent: Thursday, December 22, 2016 7:13 PM
> To: Chauhan, Madhav <madhav.chauhan at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Conselvan De Oliveira, Ander <ander.conselvan.de.oliveira at intel.com>;
> Saarinen, Jani <jani.saarinen at intel.com>; Konduru, Chandra
> <chandra.konduru at intel.com>; Shankar, Uma <uma.shankar at intel.com>;
> Mukherjee, Indranil <indranil.mukherjee at intel.com>; Kumar, Shobhit
> <shobhit.kumar at intel.com>; Tsou, Vincente <vincente.tsou at intel.com>
> Subject: RE: [GLK MIPI DSI V2 9/9] drm/915: Parsing the missed out DTD
> fields from the VBT
> 
> On Thu, 22 Dec 2016, "Chauhan, Madhav" <madhav.chauhan at intel.com>
> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani
> >> Sent: Thursday, December 22, 2016 5:09 PM
> >> To: Chauhan, Madhav <madhav.chauhan at intel.com>; intel-
> >> gfx at lists.freedesktop.org
> >> Cc: Conselvan De Oliveira, Ander
> >> <ander.conselvan.de.oliveira at intel.com>;
> >> Saarinen, Jani <jani.saarinen at intel.com>; Konduru, Chandra
> >> <chandra.konduru at intel.com>; Shankar, Uma
> <uma.shankar at intel.com>;
> >> Mukherjee, Indranil <indranil.mukherjee at intel.com>; Kumar, Shobhit
> >> <shobhit.kumar at intel.com>; Tsou, Vincente <vincente.tsou at intel.com>;
> >> Chauhan, Madhav <madhav.chauhan at intel.com>
> >> Subject: Re: [GLK MIPI DSI V2 9/9] drm/915: Parsing the missed out
> >> DTD fields from the VBT
> >>
> >> On Thu, 15 Dec 2016, Madhav Chauhan <madhav.chauhan at intel.com>
> >> wrote:
> >> > From: Vincente Tsou <vincente.tsou at intel.com>
> >> >
> >> > The upper bits of the vsync width, vsync offset and hsync width
> >> > were not parsed form the VBT. Parse these fields in this patch.
> >> >
> >> > Signed-off-by: Madhav Chauhan <madhav.chauhan at intel.com>
> >> > Signed-off-by: Vincente Tsou <vincente.tsou at intel.com>
> >>
> >> The author Signed-off-by should be first, others are added below.
> >>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_bios.c     | 8 +++++---
> >> >  drivers/gpu/drm/i915/intel_vbt_defs.h | 6 ++++--
> >> >  2 files changed, 9 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_bios.c
> >> > b/drivers/gpu/drm/i915/intel_bios.c
> >> > index eaade27..e1d014b 100644
> >> > --- a/drivers/gpu/drm/i915/intel_bios.c
> >> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> > @@ -114,16 +114,18 @@ static u32 get_blocksize(const void
> *block_data)
> >> >  	panel_fixed_mode->hsync_start = panel_fixed_mode->hdisplay +
> >> >  		((dvo_timing->hsync_off_hi << 8) | dvo_timing-
> hsync_off_lo);
> >> >  	panel_fixed_mode->hsync_end = panel_fixed_mode->hsync_start +
> >> > -		dvo_timing->hsync_pulse_width;
> >> > +		((dvo_timing->hsync_pulse_width_hi << 8) |
> >> > +				    dvo_timing->hsync_pulse_width);
> >> >  	panel_fixed_mode->htotal = panel_fixed_mode->hdisplay +
> >> >  		((dvo_timing->hblank_hi << 8) | dvo_timing->hblank_lo);
> >> >
> >> >  	panel_fixed_mode->vdisplay = (dvo_timing->vactive_hi << 8) |
> >> >  		dvo_timing->vactive_lo;
> >> >  	panel_fixed_mode->vsync_start = panel_fixed_mode->vdisplay +
> >> > -		dvo_timing->vsync_off;
> >> > +		((dvo_timing->vsync_off_hi << 4) | dvo_timing->vsync_off);
> >> >  	panel_fixed_mode->vsync_end = panel_fixed_mode->vsync_start +
> >> > -		dvo_timing->vsync_pulse_width;
> >> > +		((dvo_timing->vsync_pulse_width_hi << 4) |
> >> > +				    dvo_timing->vsync_pulse_width);
> >>
> >> The indentation for the above changes seem to be off.
> >>
> >> >  	panel_fixed_mode->vtotal = panel_fixed_mode->vdisplay +
> >> >  		((dvo_timing->vblank_hi << 8) | dvo_timing->vblank_lo);
> >> >  	panel_fixed_mode->clock = dvo_timing->clock * 10; diff --git
> >> > a/drivers/gpu/drm/i915/intel_vbt_defs.h
> >> > b/drivers/gpu/drm/i915/intel_vbt_defs.h
> >> > index 8886cab1..bf9d2d3 100644
> >> > --- a/drivers/gpu/drm/i915/intel_vbt_defs.h
> >> > +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
> >> > @@ -402,7 +402,9 @@ struct lvds_dvo_timing {
> >> >  	u8 hsync_pulse_width;
> >> >  	u8 vsync_pulse_width:4;
> >> >  	u8 vsync_off:4;
> >> > -	u8 rsvd0:6;
> >> > +	u8 vsync_pulse_width_hi:2;
> >> > +	u8 vsync_off_hi:2;
> >> > +	u8 hsync_pulse_width_hi:2;
> >>
> >> Please rename the lo counterparts of these fields to have _lo suffix,
> >> for consistency with other hi/lo split fields. With that, the
> >> compiler will help you in making sure you found all the places you
> >> need to fix. ;)
> >>
> >> >  	u8 hsync_off_hi:2;
> >> >  	u8 himage_lo;
> >> >  	u8 vimage_lo;
> >> > @@ -414,7 +416,7 @@ struct lvds_dvo_timing {
> >> >  	u8 digital:2;
> >> >  	u8 vsync_positive:1;
> >> >  	u8 hsync_positive:1;
> >> > -	u8 rsvd2:1;
> >> > +	u8 interlaced:1;
> >>
> >> This should be non_interlaced, as that's how the bit is defined.
> >>
> >> Otherwise, seems like a good find.
> >
> > Thanks Jani for review. Will address the review comment and resend it.
> 
> Please send it as a separate patch outside of this series, it's not really related.

Agree, had same thought when V1 series was sent. Included it so that review and merge can be combined :)

> 
> BR,
> Jani.
> 
> 
> >
> >>
> >> BR,
> >> Jani.
> >>
> >> >  } __packed;
> >> >
> >> >  struct lvds_pnp_id {
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> 
> --
> Jani Nikula, Intel Open Source Technology Center