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

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 22 Dec 2016, "Chauhan, Madhav" <madhav.chauhan@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani
>> Sent: Thursday, December 22, 2016 5:09 PM
>> To: Chauhan, Madhav <madhav.chauhan@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Cc: Conselvan De Oliveira, Ander <ander.conselvan.de.oliveira@intel.com>;
>> Saarinen, Jani <jani.saarinen@intel.com>; Konduru, Chandra
>> <chandra.konduru@intel.com>; Shankar, Uma <uma.shankar@intel.com>;
>> Mukherjee, Indranil <indranil.mukherjee@intel.com>; Kumar, Shobhit
>> <shobhit.kumar@intel.com>; Tsou, Vincente <vincente.tsou@intel.com>;
>> Chauhan, Madhav <madhav.chauhan@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@intel.com>
>> wrote:
>> > From: Vincente Tsou <vincente.tsou@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@intel.com>
>> > Signed-off-by: Vincente Tsou <vincente.tsou@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.

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx