Do not give a warning for the eDP panels if the "bus_format" is
not specified, since most eDP panels can support more than one
bus formats and this can be auto-detected.
Also, update the check to include bpc=10 for the eDP panel.
Signed-off-by: Rajeev Nandan <[email protected]>
---
Changes in v8:
- New patch, to address the review comments of Sam Ravnborg [1]
[1] https://lore.kernel.org/dri-devel/[email protected]/
drivers/gpu/drm/panel/panel-simple.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 86e5a45..f966b562 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -772,10 +772,8 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc,
desc->bpc != 8);
break;
case DRM_MODE_CONNECTOR_eDP:
- if (desc->bus_format == 0)
- dev_warn(dev, "Specify missing bus_format\n");
- if (desc->bpc != 6 && desc->bpc != 8)
- dev_warn(dev, "Expected bpc in {6,8} but got: %u\n", desc->bpc);
+ if (desc->bpc != 6 && desc->bpc != 8 && desc->bpc != 10)
+ dev_warn(dev, "Expected bpc in {6,8,10} but got: %u\n", desc->bpc);
break;
case DRM_MODE_CONNECTOR_DSI:
if (desc->bpc != 6 && desc->bpc != 8)
--
2.7.4
Hi Rajeev,
On Sat, Jun 26, 2021 at 10:21:06PM +0530, Rajeev Nandan wrote:
> Do not give a warning for the eDP panels if the "bus_format" is
> not specified, since most eDP panels can support more than one
> bus formats and this can be auto-detected.
> Also, update the check to include bpc=10 for the eDP panel.
>
> Signed-off-by: Rajeev Nandan <[email protected]>
> ---
>
> Changes in v8:
> - New patch, to address the review comments of Sam Ravnborg [1]
>
> [1] https://lore.kernel.org/dri-devel/[email protected]/
>
> drivers/gpu/drm/panel/panel-simple.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 86e5a45..f966b562 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -772,10 +772,8 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc,
> desc->bpc != 8);
> break;
> case DRM_MODE_CONNECTOR_eDP:
> - if (desc->bus_format == 0)
> - dev_warn(dev, "Specify missing bus_format\n");
> - if (desc->bpc != 6 && desc->bpc != 8)
> - dev_warn(dev, "Expected bpc in {6,8} but got: %u\n", desc->bpc);
> + if (desc->bpc != 6 && desc->bpc != 8 && desc->bpc != 10)
> + dev_warn(dev, "Expected bpc in {6,8,10} but got: %u\n", desc->bpc);
You'll still get a warning is bpc == 0, is that intentional ?
> break;
> case DRM_MODE_CONNECTOR_DSI:
> if (desc->bpc != 6 && desc->bpc != 8)
--
Regards,
Laurent Pinchart
Hi Laurent,
On 27-06-2021 23:48, Laurent Pinchart wrote:
> Hi Rajeev,
>
> On Sat, Jun 26, 2021 at 10:21:06PM +0530, Rajeev Nandan wrote:
>> Do not give a warning for the eDP panels if the "bus_format" is
>> not specified, since most eDP panels can support more than one
>> bus formats and this can be auto-detected.
>> Also, update the check to include bpc=10 for the eDP panel.
>>
>> Signed-off-by: Rajeev Nandan <[email protected]>
>> ---
>>
>> Changes in v8:
>> - New patch, to address the review comments of Sam Ravnborg [1]
>>
>> [1]
>> https://lore.kernel.org/dri-devel/[email protected]/
>>
>> drivers/gpu/drm/panel/panel-simple.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-simple.c
>> b/drivers/gpu/drm/panel/panel-simple.c
>> index 86e5a45..f966b562 100644
>> --- a/drivers/gpu/drm/panel/panel-simple.c
>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>> @@ -772,10 +772,8 @@ static int panel_simple_probe(struct device *dev,
>> const struct panel_desc *desc,
>> desc->bpc != 8);
>> break;
>> case DRM_MODE_CONNECTOR_eDP:
>> - if (desc->bus_format == 0)
>> - dev_warn(dev, "Specify missing bus_format\n");
>> - if (desc->bpc != 6 && desc->bpc != 8)
>> - dev_warn(dev, "Expected bpc in {6,8} but got: %u\n", desc->bpc);
>> + if (desc->bpc != 6 && desc->bpc != 8 && desc->bpc != 10)
>> + dev_warn(dev, "Expected bpc in {6,8,10} but got: %u\n",
>> desc->bpc);
>
> You'll still get a warning is bpc == 0, is that intentional ?
This was not intentional, I missed considering bpc=0 case. As we are
removing the warning for bus_format=0 then a similar thing can be done
for the bpc=0 also. The bpc value should be a valid one if it is
specified. Unlike the bus_format, bpc has few possible values that can
be checked here along with 0. Please correct me if I misunderstood the
concept.
I will fix this.
Thanks,
Rajeev
Hi Rajeev,
On Mon, Jun 28, 2021 at 05:46:24PM +0530, [email protected] wrote:
> On 27-06-2021 23:48, Laurent Pinchart wrote:
> > On Sat, Jun 26, 2021 at 10:21:06PM +0530, Rajeev Nandan wrote:
> >> Do not give a warning for the eDP panels if the "bus_format" is
> >> not specified, since most eDP panels can support more than one
> >> bus formats and this can be auto-detected.
> >> Also, update the check to include bpc=10 for the eDP panel.
> >>
> >> Signed-off-by: Rajeev Nandan <[email protected]>
> >> ---
> >>
> >> Changes in v8:
> >> - New patch, to address the review comments of Sam Ravnborg [1]
> >>
> >> [1]
> >> https://lore.kernel.org/dri-devel/[email protected]/
> >>
> >> drivers/gpu/drm/panel/panel-simple.c | 6 ++----
> >> 1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> >> b/drivers/gpu/drm/panel/panel-simple.c
> >> index 86e5a45..f966b562 100644
> >> --- a/drivers/gpu/drm/panel/panel-simple.c
> >> +++ b/drivers/gpu/drm/panel/panel-simple.c
> >> @@ -772,10 +772,8 @@ static int panel_simple_probe(struct device *dev,
> >> const struct panel_desc *desc,
> >> desc->bpc != 8);
> >> break;
> >> case DRM_MODE_CONNECTOR_eDP:
> >> - if (desc->bus_format == 0)
> >> - dev_warn(dev, "Specify missing bus_format\n");
> >> - if (desc->bpc != 6 && desc->bpc != 8)
> >> - dev_warn(dev, "Expected bpc in {6,8} but got: %u\n", desc->bpc);
> >> + if (desc->bpc != 6 && desc->bpc != 8 && desc->bpc != 10)
> >> + dev_warn(dev, "Expected bpc in {6,8,10} but got: %u\n", desc->bpc);
> >
> > You'll still get a warning is bpc == 0, is that intentional ?
>
> This was not intentional, I missed considering bpc=0 case. As we are
> removing the warning for bus_format=0 then a similar thing can be done
> for the bpc=0 also. The bpc value should be a valid one if it is
> specified. Unlike the bus_format, bpc has few possible values that can
> be checked here along with 0. Please correct me if I misunderstood the
> concept.
> I will fix this.
What's the point of specifying bpc if it's optional though ? Users of
the panel will need to support the case where bpc is set to 0. Have you
ensured that they all do ? Can they meaningfully use the bpc value if
they need to be ready to support bpc == 0 ?
--
Regards,
Laurent Pinchart
Hi,
On Mon, Jun 28, 2021 at 6:33 AM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Rajeev,
>
> On Mon, Jun 28, 2021 at 05:46:24PM +0530, [email protected] wrote:
> > On 27-06-2021 23:48, Laurent Pinchart wrote:
> > > On Sat, Jun 26, 2021 at 10:21:06PM +0530, Rajeev Nandan wrote:
> > >> Do not give a warning for the eDP panels if the "bus_format" is
> > >> not specified, since most eDP panels can support more than one
> > >> bus formats and this can be auto-detected.
> > >> Also, update the check to include bpc=10 for the eDP panel.
> > >>
> > >> Signed-off-by: Rajeev Nandan <[email protected]>
> > >> ---
> > >>
> > >> Changes in v8:
> > >> - New patch, to address the review comments of Sam Ravnborg [1]
> > >>
> > >> [1]
> > >> https://lore.kernel.org/dri-devel/[email protected]/
> > >>
> > >> drivers/gpu/drm/panel/panel-simple.c | 6 ++----
> > >> 1 file changed, 2 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> > >> b/drivers/gpu/drm/panel/panel-simple.c
> > >> index 86e5a45..f966b562 100644
> > >> --- a/drivers/gpu/drm/panel/panel-simple.c
> > >> +++ b/drivers/gpu/drm/panel/panel-simple.c
> > >> @@ -772,10 +772,8 @@ static int panel_simple_probe(struct device *dev,
> > >> const struct panel_desc *desc,
> > >> desc->bpc != 8);
> > >> break;
> > >> case DRM_MODE_CONNECTOR_eDP:
> > >> - if (desc->bus_format == 0)
> > >> - dev_warn(dev, "Specify missing bus_format\n");
> > >> - if (desc->bpc != 6 && desc->bpc != 8)
> > >> - dev_warn(dev, "Expected bpc in {6,8} but got: %u\n", desc->bpc);
> > >> + if (desc->bpc != 6 && desc->bpc != 8 && desc->bpc != 10)
> > >> + dev_warn(dev, "Expected bpc in {6,8,10} but got: %u\n", desc->bpc);
> > >
> > > You'll still get a warning is bpc == 0, is that intentional ?
> >
> > This was not intentional, I missed considering bpc=0 case. As we are
> > removing the warning for bus_format=0 then a similar thing can be done
> > for the bpc=0 also. The bpc value should be a valid one if it is
> > specified. Unlike the bus_format, bpc has few possible values that can
> > be checked here along with 0. Please correct me if I misunderstood the
> > concept.
> > I will fix this.
>
> What's the point of specifying bpc if it's optional though ? Users of
> the panel will need to support the case where bpc is set to 0. Have you
> ensured that they all do ? Can they meaningfully use the bpc value if
> they need to be ready to support bpc == 0 ?
I must be missing something, but to me it seems like Rajeev's patch is
fine as-is. From my reading of the code:
* Removes the warning if bus_format == 0. This is correct since I
don't think specifying bus format for eDP panels makes lots of sense.
* Removes the warning if bpc == 10. This is correct since we've seen
eDP panels with 10bpc.
* Keeps the warning if bpc == 0. IMO we can/should still require
panels to specify their BPC. I guess I'm treating this as a "max BPC".
I know that we use this field in the sn65dsi86 driver, so if it's OK
for this to be 0 then we'll have to change that driver to handle it.
Does that sound right to you Laurent? So since I think Rajeev's patch
is OK, I'm happy with:
Reviewed-by: Douglas Anderson <[email protected]>
Unless I missed something and this patch needs to change then it feels
like Rajeev's patch series is in pretty good shape to land. I'm happy
to commit it but since Sam made comments on the previous version I'd
plan to wait a bit to make sure he has a chance for another look if he
wants to. I've also only got 2 days left before I vanish for 1 week of
vacation. ...so my plan is:
* If Sam / Laurent come back before tomorrow and say they're happy
then I'll commit.
* If I hear nothing then I'll check back after my vacation. If someone
else has committed then I'll be happy. If not and there has just been
silence then I'll commit it myself.
Please yell if that's not OK. :-)
-Doug
Hi Doug,
On Mon, Jun 28, 2021 at 08:34:04AM -0700, Doug Anderson wrote:
> On Mon, Jun 28, 2021 at 6:33 AM Laurent Pinchart wrote:
> > On Mon, Jun 28, 2021 at 05:46:24PM +0530, [email protected] wrote:
> > > On 27-06-2021 23:48, Laurent Pinchart wrote:
> > > > On Sat, Jun 26, 2021 at 10:21:06PM +0530, Rajeev Nandan wrote:
> > > >> Do not give a warning for the eDP panels if the "bus_format" is
> > > >> not specified, since most eDP panels can support more than one
> > > >> bus formats and this can be auto-detected.
> > > >> Also, update the check to include bpc=10 for the eDP panel.
> > > >>
> > > >> Signed-off-by: Rajeev Nandan <[email protected]>
> > > >> ---
> > > >>
> > > >> Changes in v8:
> > > >> - New patch, to address the review comments of Sam Ravnborg [1]
> > > >>
> > > >> [1]
> > > >> https://lore.kernel.org/dri-devel/[email protected]/
> > > >>
> > > >> drivers/gpu/drm/panel/panel-simple.c | 6 ++----
> > > >> 1 file changed, 2 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> > > >> b/drivers/gpu/drm/panel/panel-simple.c
> > > >> index 86e5a45..f966b562 100644
> > > >> --- a/drivers/gpu/drm/panel/panel-simple.c
> > > >> +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > >> @@ -772,10 +772,8 @@ static int panel_simple_probe(struct device *dev,
> > > >> const struct panel_desc *desc,
> > > >> desc->bpc != 8);
> > > >> break;
> > > >> case DRM_MODE_CONNECTOR_eDP:
> > > >> - if (desc->bus_format == 0)
> > > >> - dev_warn(dev, "Specify missing bus_format\n");
> > > >> - if (desc->bpc != 6 && desc->bpc != 8)
> > > >> - dev_warn(dev, "Expected bpc in {6,8} but got: %u\n", desc->bpc);
> > > >> + if (desc->bpc != 6 && desc->bpc != 8 && desc->bpc != 10)
> > > >> + dev_warn(dev, "Expected bpc in {6,8,10} but got: %u\n", desc->bpc);
> > > >
> > > > You'll still get a warning is bpc == 0, is that intentional ?
> > >
> > > This was not intentional, I missed considering bpc=0 case. As we are
> > > removing the warning for bus_format=0 then a similar thing can be done
> > > for the bpc=0 also. The bpc value should be a valid one if it is
> > > specified. Unlike the bus_format, bpc has few possible values that can
> > > be checked here along with 0. Please correct me if I misunderstood the
> > > concept.
> > > I will fix this.
> >
> > What's the point of specifying bpc if it's optional though ? Users of
> > the panel will need to support the case where bpc is set to 0. Have you
> > ensured that they all do ? Can they meaningfully use the bpc value if
> > they need to be ready to support bpc == 0 ?
>
> I must be missing something, but to me it seems like Rajeev's patch is
> fine as-is. From my reading of the code:
>
> * Removes the warning if bus_format == 0. This is correct since I
> don't think specifying bus format for eDP panels makes lots of sense.
This is embarassing, I've been reading it as desc->bpc == 0 from the
beginning :-( My bad. The bpc change is correct.
> * Removes the warning if bpc == 10. This is correct since we've seen
> eDP panels with 10bpc.
>
> * Keeps the warning if bpc == 0. IMO we can/should still require
> panels to specify their BPC. I guess I'm treating this as a "max BPC".
> I know that we use this field in the sn65dsi86 driver, so if it's OK
> for this to be 0 then we'll have to change that driver to handle it.
>
> Does that sound right to you Laurent? So since I think Rajeev's patch
> is OK, I'm happy with:
>
> Reviewed-by: Douglas Anderson <[email protected]>
>
> Unless I missed something and this patch needs to change then it feels
> like Rajeev's patch series is in pretty good shape to land. I'm happy
> to commit it but since Sam made comments on the previous version I'd
> plan to wait a bit to make sure he has a chance for another look if he
> wants to. I've also only got 2 days left before I vanish for 1 week of
> vacation. ...so my plan is:
> * If Sam / Laurent come back before tomorrow and say they're happy
> then I'll commit.
> * If I hear nothing then I'll check back after my vacation. If someone
> else has committed then I'll be happy. If not and there has just been
> silence then I'll commit it myself.
>
> Please yell if that's not OK. :-)
--
Regards,
Laurent Pinchart